fix(s3api,iamapi): avoid full SaveConfiguration when creating a single IAM user (#9261)

* fix(s3api,iamapi): avoid full SaveConfiguration when creating a single IAM user

When CreateUser was called, both the embedded IAM path (s3api_embedded_iam.go)
and the standalone IAM path (iamapi_management_handlers.go) would:
  1. Load the full config (all N users)
  2. Append the new user
  3. Call SaveConfiguration, which re-writes ALL N user files in the filer_etc
     store, triggering N file-change events and N reload cycles.

The fix replaces the full-save path with credentialManager.CreateUser, which
writes only the single new user's file. Set changed=false to skip the redundant
SaveConfiguration call, and add "CreateUser" to the reload-after-targeted-write
block so the in-memory config is refreshed.

Also adds a nil guard around iama.iam.GetCredentialManager() in DoActions to
avoid a nil-pointer panic in legacy test fixtures that leave iam unset.

Add SetCredentialManagerForTest to IdentityAccessManagement so tests can inject
an in-memory credential store without touching production code paths.

* test(s3api,iamapi): regression tests for CreateUser targeted-write fix

Add TestCreateUserDoesNotSaveAllUsers (iamapi) and
TestEmbeddedIamCreateUserDoesNotSaveAllUsers (s3api) to guard against the
regression where CreateUser would call SaveConfiguration and re-write all
N existing user files.

Both tests:
  - Pre-populate 3 existing users
  - Invoke CreateUser via the HTTP API
  - Assert PutS3ApiConfiguration (full-config save) was NOT called
  - Assert the new user is visible in the credential store
  - Assert all pre-existing users are still intact

Also update TestEmbeddedIamExecuteAction to verify persistence via the
credential manager directly (mockConfig is no longer updated on CreateUser
since we skip the SaveConfiguration path).

* refactor(s3api,iamapi): share credential-error to IAM-code mapping

Move the credentialErrToIamErrCode helper from weed/iamapi to
weed/s3api as exported CredentialErrToIamErrCode and call it from
both the standalone IAM handler (iamapi) and the embedded IAM
handler (s3api). Previously the standalone path used the helper
while the embedded path duplicated the same switch inline; the two
sites could drift out of sync.

Also extend the mapping to cover ErrUserNotFound and
ErrAccessKeyNotFound (404 NoSuchEntity) so non-CreateUser callers
that opt into the helper get the right HTTP status.

* test(s3api): seed credential store explicitly in CreateUser regression

Previously the test relied on getS3ApiConfigurationFunc's syncOnce
side effect to populate the credential store with mockConfig
identities. If that fixture ever stopped seeding, the post-test
"pre-existing users still exist" assertion would silently start
passing for the wrong reason (the users were never created).

Seed cm.SaveConfiguration directly and assert the seed precondition
before the API call so any seeding regression fails loudly.

* fix(s3api): honor skipPersist in embedded CreateUser targeted path

ExecuteAction documents that skipPersist=true means "the changed
configuration is not saved to the persistent store", but the
targeted credentialManager.CreateUser added for the avoid-bulk-save
fix ran unconditionally. Gate it on !skipPersist so no-persist
callers don't silently leak a write to the credential store. Leave
changed=true in the skipPersist branch so the tail's existing
`if changed { if !skipPersist { ... } }` block keeps suppressing
persistence the same way it does for every other action.

Add TestEmbeddedIamCreateUserSkipPersist to pin the contract: a
CreateUser invocation with skipPersist=true must not call
PutS3ApiConfiguration and must not leave the user in the
credential store.

---------

Co-authored-by: Chris Lu <chris.lu@gmail.com>

weed/iamapi/iamapi_handlers.go

@@ -37,7 +37,7 @@ func writeIamErrorResponse(w http.ResponseWriter, r *http.Request, reqID string,
 		s3err.WriteXMLResponse(w, r, http.StatusNotFound, errorResp)
 	case iam.ErrCodeMalformedPolicyDocumentException, iam.ErrCodeInvalidInputException:
 		s3err.WriteXMLResponse(w, r, http.StatusBadRequest, errorResp)
-	case iam.ErrCodeDeleteConflictException:
+	case iam.ErrCodeDeleteConflictException, iam.ErrCodeEntityAlreadyExistsException:
 		s3err.WriteXMLResponse(w, r, http.StatusConflict, errorResp)
 	case iam.ErrCodeServiceFailureException:
 		// We do not want to expose internal server error to the client

weed/iamapi/iamapi_management_handlers.go

@@ -18,6 +18,7 @@ import (
 	iamlib "github.com/seaweedfs/seaweedfs/weed/iam"
 	"github.com/seaweedfs/seaweedfs/weed/pb/filer_pb"
 	"github.com/seaweedfs/seaweedfs/weed/pb/iam_pb"
+	"github.com/seaweedfs/seaweedfs/weed/s3api"
 	"github.com/seaweedfs/seaweedfs/weed/s3api/policy_engine"
 	"github.com/seaweedfs/seaweedfs/weed/s3api/s3err"
 	"github.com/seaweedfs/seaweedfs/weed/util/request_id"
@@ -1123,6 +1124,18 @@ func (iama *IamApiServer) DoActions(w http.ResponseWriter, r *http.Request) {
 		changed = false
 	case "CreateUser":
 		response = iama.CreateUser(s3cfg, values)
+		// Use targeted create to avoid rewriting all existing user files via SaveConfiguration.
+		// credentialManager.CreateUser writes only the new user's file.
+		if iama.iam != nil {
+			if cm := iama.iam.GetCredentialManager(); cm != nil {
+				userName := values.Get("UserName")
+				if err := cm.CreateUser(r.Context(), &iam_pb.Identity{Name: userName}); err != nil {
+					writeIamErrorResponse(w, r, reqID, &IamError{Code: s3api.CredentialErrToIamErrCode(err), Error: err})
+					return
+				}
+				changed = false
+			}
+		}
 	case "GetUser":
 		userName := values.Get("UserName")
 		var err *IamError

weed/iamapi/iamapi_test.go

@@ -1,6 +1,7 @@
 package iamapi
 
 import (
+	"context"
 	"encoding/json"
 	"encoding/xml"
 	"net/http"
@@ -14,11 +15,14 @@ import (
 	"github.com/aws/aws-sdk-go/service/iam"
 	"github.com/gorilla/mux"
 	"github.com/jinzhu/copier"
+	"github.com/seaweedfs/seaweedfs/weed/credential"
+	"github.com/seaweedfs/seaweedfs/weed/credential/memory"
 	"github.com/seaweedfs/seaweedfs/weed/pb/iam_pb"
 	"github.com/seaweedfs/seaweedfs/weed/s3api"
 	"github.com/seaweedfs/seaweedfs/weed/s3api/policy_engine"
 	"github.com/seaweedfs/seaweedfs/weed/util/request_id"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 var GetS3ApiConfiguration func(s3cfg *iam_pb.S3ApiConfiguration) (err error)
@@ -385,3 +389,101 @@ func mustMarshalJSON(t *testing.T, v interface{}) []byte {
 	}
 	return data
 }
+
+// iamApiServerWithCredentialManager builds an IamApiServer backed by a real
+// in-memory credential store. Used for tests that exercise the targeted
+// credentialManager paths (e.g. CreateUser) instead of the legacy mock.
+func iamApiServerWithCredentialManager() (*IamApiServer, *credential.CredentialManager, *countingConfigSaver) {
+	store := &memory.MemoryStore{}
+	store.Initialize(nil, "")
+	cm := &credential.CredentialManager{Store: store}
+
+	counter := &countingConfigSaver{cm: cm}
+	iamInstance := &s3api.IdentityAccessManagement{}
+	// Wire up the credential manager so GetCredentialManager() returns it.
+	iamInstance.SetCredentialManagerForTest(cm)
+
+	srv := &IamApiServer{
+		s3ApiConfig: counter,
+		iam:         iamInstance,
+	}
+	return srv, cm, counter
+}
+
+// countingConfigSaver wraps the credential manager and counts full-config saves.
+type countingConfigSaver struct {
+	cm        *credential.CredentialManager
+	putCalled int
+}
+
+func (c *countingConfigSaver) GetS3ApiConfiguration(s3cfg *iam_pb.S3ApiConfiguration) error {
+	config, err := c.cm.LoadConfiguration(context.Background())
+	if err != nil {
+		return err
+	}
+	s3cfg.Identities = config.Identities
+	s3cfg.Policies = config.Policies
+	return nil
+}
+
+func (c *countingConfigSaver) PutS3ApiConfiguration(s3cfg *iam_pb.S3ApiConfiguration) error {
+	c.putCalled++
+	return c.cm.SaveConfiguration(context.Background(), s3cfg)
+}
+
+func (c *countingConfigSaver) GetPolicies(policies *Policies) error {
+	return nil
+}
+
+func (c *countingConfigSaver) PutPolicies(policies *Policies) error {
+	return nil
+}
+
+func executeRequestWith(srv *IamApiServer, req *http.Request, v interface{}) (*httptest.ResponseRecorder, error) {
+	rr := httptest.NewRecorder()
+	apiRouter := mux.NewRouter().SkipClean(true)
+	apiRouter.Path("/").Methods(http.MethodPost).HandlerFunc(srv.DoActions)
+	apiRouter.ServeHTTP(rr, req)
+	return rr, xml.Unmarshal(rr.Body.Bytes(), &v)
+}
+
+// TestCreateUserDoesNotSaveAllUsers is a regression test for the bug where
+// creating a new user triggered a full SaveConfiguration over all existing
+// identities (causing N file writes + reload cycles in the filer_etc store).
+// The fix uses credentialManager.CreateUser for a targeted single-file write.
+func TestCreateUserDoesNotSaveAllUsers(t *testing.T) {
+	srv, cm, counter := iamApiServerWithCredentialManager()
+	ctx := context.Background()
+
+	// Pre-populate three existing users.
+	for _, name := range []string{"existing-1", "existing-2", "existing-3"} {
+		require.NoError(t, cm.CreateUser(ctx, &iam_pb.Identity{Name: name}))
+	}
+
+	// Create a new user via the HTTP API.
+	params := &iam.CreateUserInput{UserName: aws.String("new-user")}
+	req, _ := iam.New(session.New()).CreateUserRequest(params)
+	_ = req.Build()
+	out := CreateUserResponse{}
+	resp, err := executeRequestWith(srv, req.HTTPRequest, &out)
+	require.NoError(t, err)
+	require.Equal(t, http.StatusOK, resp.Code)
+
+	// The new user must appear in the store.
+	newUser, userErr := cm.GetUser(ctx, "new-user")
+	require.NoError(t, userErr)
+	require.Equal(t, "new-user", newUser.Name)
+
+	// Critical: full SaveConfiguration (PutS3ApiConfiguration) must NOT have
+	// been called. Before the fix it was called once per CreateUser, rewriting
+	// every existing user file and triggering a cascade of reload events.
+	assert.Equal(t, 0, counter.putCalled,
+		"CreateUser must not trigger a full PutS3ApiConfiguration over all users")
+
+	// All pre-existing users must still be intact.
+	for _, name := range []string{"existing-1", "existing-2", "existing-3"} {
+		u, err := cm.GetUser(ctx, name)
+		require.NoError(t, err, "pre-existing user %s should still exist", name)
+		assert.Equal(t, name, u.Name)
+	}
+}

weed/s3api/auth_credentials.go

@@ -1635,6 +1635,12 @@ func (iam *IdentityAccessManagement) GetCredentialManager() *credential.Credenti
 	return iam.credentialManager
 }
 
+// SetCredentialManagerForTest replaces the credential manager. Only intended
+// for use in tests that need to inject a pre-configured in-memory store.
+func (iam *IdentityAccessManagement) SetCredentialManagerForTest(cm *credential.CredentialManager) {
+	iam.credentialManager = cm
+}
+
 type managedPolicyLoader interface {
 	LoadManagedPolicies(ctx context.Context) ([]*iam_pb.Policy, error)
 }

weed/s3api/credential_iam_errors.go

@@ -0,0 +1,27 @@
+package s3api
+
+import (
+	"errors"
+
+	"github.com/aws/aws-sdk-go/service/iam"
+	"github.com/seaweedfs/seaweedfs/weed/credential"
+)
+
+// CredentialErrToIamErrCode maps a credential store error to the appropriate
+// IAM error code. Permanent client errors (e.g. duplicate entity, missing
+// entity) are mapped to their specific IAM codes; all other errors fall back
+// to ServiceFailure so the caller receives an HTTP 500 rather than a 200.
+//
+// This is shared between the standalone IAM server (weed/iamapi) and the
+// embedded IAM handler (weed/s3api) so error granularity stays in sync.
+func CredentialErrToIamErrCode(err error) string {
+	switch {
+	case errors.Is(err, credential.ErrUserAlreadyExists):
+		return iam.ErrCodeEntityAlreadyExistsException
+	case errors.Is(err, credential.ErrUserNotFound),
+		errors.Is(err, credential.ErrAccessKeyNotFound):
+		return iam.ErrCodeNoSuchEntityException
+	default:
+		return iam.ErrCodeServiceFailureException
+	}
+}

weed/s3api/s3api_embedded_iam.go

@@ -2195,6 +2195,18 @@ func (e *EmbeddedIamApi) ExecuteAction(ctx context.Context, values url.Values, s
 		if iamErr != nil {
 			return nil, iamErr
 		}
+		// Use targeted create to avoid rewriting all existing user files via SaveConfiguration.
+		// credentialManager.CreateUser writes only the new user's file.
+		// Skip when skipPersist is set: the contract is no persistent write, so leave
+		// changed=true and let the tail's `if changed { if !skipPersist { ... } }` block
+		// suppress persistence the same way it does for every other action.
+		if e.credentialManager != nil && !skipPersist {
+			userName := values.Get("UserName")
+			if err := e.credentialManager.CreateUser(ctx, &iam_pb.Identity{Name: userName}); err != nil {
+				return nil, &iamError{Code: CredentialErrToIamErrCode(err), Error: err}
+			}
+			changed = false
+		}
 	case "GetUser":
 		userName := values.Get("UserName")
 		var iamErr *iamError
@@ -2469,11 +2481,11 @@ func (e *EmbeddedIamApi) ExecuteAction(ctx context.Context, values url.Values, s
 			glog.Errorf("Failed to reload IAM configuration after mutation: %v", err)
 			// Don't fail the request since the persistent save succeeded
 		}
-	} else if action == "AttachUserPolicy" || action == "DetachUserPolicy" || action == "CreatePolicy" || action == "DeletePolicy" {
+	} else if action == "AttachUserPolicy" || action == "DetachUserPolicy" || action == "CreatePolicy" || action == "DeletePolicy" || action == "CreateUser" {
 		// Even if changed=false (persisted via credentialManager), we should still reload
 		// if we are utilizing the local in-memory cache for speed
 		if err := e.ReloadConfiguration(); err != nil {
-			glog.Errorf("Failed to reload IAM configuration after managed policy mutation: %v", err)
+			glog.Errorf("Failed to reload IAM configuration after mutation: %v", err)
 		}
 	}
 	response.SetRequestId(reqID)

weed/s3api/s3api_embedded_iam_test.go

@@ -190,6 +190,91 @@ func TestEmbeddedIamCreateUser(t *testing.T) {
 	assert.Equal(t, "TestUser", api.mockConfig.Identities[0].Name)
 }
 
+// TestEmbeddedIamCreateUserDoesNotSaveAllUsers is a regression test for the bug
+// where creating a new user triggered a full SaveConfiguration over all existing
+// identities (causing N file writes + reload cycles in the filer_etc store).
+// The fix uses credentialManager.CreateUser for a targeted single-file write.
+func TestEmbeddedIamCreateUserDoesNotSaveAllUsers(t *testing.T) {
+	api := NewEmbeddedIamApiForTest()
+	ctx := context.Background()
+
+	// Seed both mockConfig and the credential store directly so the test does
+	// not rely on the getS3ApiConfigurationFunc fixture's syncOnce side effect
+	// to populate the store. Pre-call assertions below fail loudly if seeding
+	// breaks for any reason.
+	existing := []string{"existing-user-1", "existing-user-2", "existing-user-3"}
+	api.mockConfig = &iam_pb.S3ApiConfiguration{}
+	for _, name := range existing {
+		api.mockConfig.Identities = append(api.mockConfig.Identities, &iam_pb.Identity{Name: name})
+	}
+	require.NoError(t, api.credentialManager.SaveConfiguration(ctx, api.mockConfig))
+	for _, name := range existing {
+		u, err := api.credentialManager.GetUser(ctx, name)
+		require.NoError(t, err, "seed precondition: %s must exist", name)
+		require.Equal(t, name, u.Name)
+	}
+
+	// Track calls to PutS3ApiConfiguration (the full-config write path).
+	putConfigCalls := 0
+	api.putS3ApiConfigurationFunc = func(s3cfg *iam_pb.S3ApiConfiguration) error {
+		putConfigCalls++
+		api.mockConfig = proto.Clone(s3cfg).(*iam_pb.S3ApiConfiguration)
+		return api.credentialManager.SaveConfiguration(ctx, s3cfg)
+	}
+
+	params := &iam.CreateUserInput{UserName: aws.String("new-user")}
+	req, _ := iam.New(session.New()).CreateUserRequest(params)
+	_ = req.Build()
+	out := iamCreateUserResponse{}
+	response, err := executeEmbeddedIamRequest(api, req.HTTPRequest, &out)
+	require.NoError(t, err)
+	require.Equal(t, http.StatusOK, response.Code)
+
+	// The new user must be visible.
+	newUser, userErr := api.credentialManager.GetUser(ctx, "new-user")
+	require.NoError(t, userErr)
+	require.Equal(t, "new-user", newUser.Name)
+
+	// The critical assertion: full SaveConfiguration must NOT have been called.
+	// Previously this was called once for every CreateUser, re-writing all N user
+	// files and triggering N reload loops.
+	assert.Equal(t, 0, putConfigCalls, "CreateUser must not call PutS3ApiConfiguration (full-config save)")
+
+	// Pre-existing users must be untouched.
+	for _, name := range existing {
+		u, err := api.credentialManager.GetUser(ctx, name)
+		require.NoError(t, err, "pre-existing user %s should still exist", name)
+		assert.Equal(t, name, u.Name)
+	}
+}
+
+// TestEmbeddedIamCreateUserSkipPersist asserts that ExecuteAction("CreateUser",
+// skipPersist=true) performs no persistent write to the credential store. The
+// targeted-create optimization must respect the skipPersist contract documented
+// on ExecuteAction; otherwise no-persist callers would silently leak writes.
+func TestEmbeddedIamCreateUserSkipPersist(t *testing.T) {
+	api := NewEmbeddedIamApiForTest()
+	ctx := context.Background()
+
+	api.putS3ApiConfigurationFunc = func(s3cfg *iam_pb.S3ApiConfiguration) error {
+		t.Fatalf("PutS3ApiConfiguration must not be called when skipPersist=true")
+		return nil
+	}
+
+	vals := url.Values{}
+	vals.Set("Action", "CreateUser")
+	vals.Set("UserName", "no-persist-user")
+
+	resp, iamErr := api.ExecuteAction(ctx, vals, true /*skipPersist*/, "")
+	require.Nil(t, iamErr)
+	require.NotNil(t, resp)
+
+	// The credential store must not have the user — skipPersist contract.
+	_, err := api.credentialManager.GetUser(ctx, "no-persist-user")
+	require.ErrorIs(t, err, credential.ErrUserNotFound,
+		"skipPersist=true must not write the new user to the credential store")
+}
+
 // TestEmbeddedIamListUsers tests listing users via the embedded IAM API
 func TestEmbeddedIamListUsers(t *testing.T) {
 	api := NewEmbeddedIamApiForTest()
@@ -2503,17 +2588,13 @@ func TestEmbeddedIamExecuteAction(t *testing.T) {
 	api := NewEmbeddedIamApiForTest()
 	api.mockConfig = &iam_pb.S3ApiConfiguration{}
 
-	// Explicitly set hook to debug panic
-	api.EmbeddedIamApi.reloadConfigurationFunc = func() error {
-		return nil
-	}
-
 	// Test case: CreateUser via ExecuteAction
 	vals := url.Values{}
 	vals.Set("Action", "CreateUser")
 	vals.Set("UserName", "ExecuteActionUser")
 
-	resp, iamErr := api.ExecuteAction(context.Background(), vals, false, "")
+	ctx := context.Background()
+	resp, iamErr := api.ExecuteAction(ctx, vals, false, "")
 	assert.Nil(t, iamErr)
 
 	// Verify response type
@@ -2521,9 +2602,11 @@ func TestEmbeddedIamExecuteAction(t *testing.T) {
 	assert.True(t, ok)
 	assert.Equal(t, "ExecuteActionUser", *createResp.CreateUserResult.User.UserName)
 
-	// Verify persistence
-	assert.Len(t, api.mockConfig.Identities, 1)
-	assert.Equal(t, "ExecuteActionUser", api.mockConfig.Identities[0].Name)
+	// CreateUser now uses a targeted write (credentialManager.CreateUser) rather than
+	// SaveConfiguration, so persistence is checked via the credential manager directly.
+	user, err := api.credentialManager.GetUser(ctx, "ExecuteActionUser")
+	assert.NoError(t, err)
+	assert.Equal(t, "ExecuteActionUser", user.Name)
 }
 
 // TestEmbeddedIamReadOnly tests that write operations are blocked when readOnly is true