From 839c450d21a3ae58803c3f8e78734ba20bcf1523 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Tue, 25 Nov 2025 20:08:08 -0800 Subject: [PATCH] Address PR review comments: implement full failover for IAM operations Critical fixes based on code review feedback: 1. **IAM API Failover (Critical)**: - Replace pb.WithGrpcFilerClient with pb.WithOneOfGrpcFilerClients in: * GetS3ApiConfigurationFromFiler() * PutS3ApiConfigurationToFiler() * GetPolicies() * PutPolicies() - Now all IAM operations support automatic failover across multiple filers 2. **Validation Improvements**: - Add validation in NewIamApiServerWithStore() to require at least one filer - Add validation in NewS3ApiServerWithStore() to require at least one filer - Add warning log when no filers configured for credential store 3. **Error Logging**: - Circuit breaker now logs when config load fails instead of silently ignoring - Helps operators understand why circuit breaker limits aren't applied 4. **Code Quality**: - Use ToGrpcAddress() for filer address in credential store setup - More consistent with rest of codebase and future-proof These changes ensure IAM operations have the same high availability guarantees as S3 operations, completing the multi-filer failover implementation. --- weed/iamapi/iamapi_server.go | 12 ++++++++---- weed/s3api/auth_credentials.go | 7 ++++--- weed/s3api/s3api_circuit_breaker.go | 1 + weed/s3api/s3api_server.go | 4 ++++ 4 files changed, 17 insertions(+), 7 deletions(-) diff --git a/weed/iamapi/iamapi_server.go b/weed/iamapi/iamapi_server.go index 90a175ca5..11005d00c 100644 --- a/weed/iamapi/iamapi_server.go +++ b/weed/iamapi/iamapi_server.go @@ -69,6 +69,10 @@ func NewIamApiServer(router *mux.Router, option *IamServerOption) (iamApiServer } func NewIamApiServerWithStore(router *mux.Router, option *IamServerOption, explicitStore string) (iamApiServer *IamApiServer, err error) { + if len(option.Filers) == 0 { + return nil, fmt.Errorf("at least one filer address is required") + } + masterClient := wdclient.NewMasterClient(option.GrpcDialOption, "", "iam", "", "", "", *pb.NewServiceDiscoveryFromMap(option.Masters)) // Create a cancellable context for the master client connection @@ -158,7 +162,7 @@ func (iama *IamS3ApiConfigure) PutS3ApiConfigurationToCredentialManager(s3cfg *i func (iama *IamS3ApiConfigure) GetS3ApiConfigurationFromFiler(s3cfg *iam_pb.S3ApiConfiguration) (err error) { var buf bytes.Buffer - err = pb.WithGrpcFilerClient(false, 0, iama.getFilerAddress(), iama.option.GrpcDialOption, func(client filer_pb.SeaweedFilerClient) error { + err = pb.WithOneOfGrpcFilerClients(false, iama.option.Filers, iama.option.GrpcDialOption, func(client filer_pb.SeaweedFilerClient) error { if err = filer.ReadEntry(iama.masterClient, client, filer.IamConfigDirectory, filer.IamIdentityFile, &buf); err != nil { return err } @@ -180,7 +184,7 @@ func (iama *IamS3ApiConfigure) PutS3ApiConfigurationToFiler(s3cfg *iam_pb.S3ApiC if err := filer.ProtoToText(&buf, s3cfg); err != nil { return fmt.Errorf("ProtoToText: %s", err) } - return pb.WithGrpcFilerClient(false, 0, iama.getFilerAddress(), iama.option.GrpcDialOption, func(client filer_pb.SeaweedFilerClient) error { + return pb.WithOneOfGrpcFilerClients(false, iama.option.Filers, iama.option.GrpcDialOption, func(client filer_pb.SeaweedFilerClient) error { err = util.Retry("saveIamIdentity", func() error { return filer.SaveInsideFiler(client, filer.IamConfigDirectory, filer.IamIdentityFile, buf.Bytes()) }) @@ -193,7 +197,7 @@ func (iama *IamS3ApiConfigure) PutS3ApiConfigurationToFiler(s3cfg *iam_pb.S3ApiC func (iama *IamS3ApiConfigure) GetPolicies(policies *Policies) (err error) { var buf bytes.Buffer - err = pb.WithGrpcFilerClient(false, 0, iama.getFilerAddress(), iama.option.GrpcDialOption, func(client filer_pb.SeaweedFilerClient) error { + err = pb.WithOneOfGrpcFilerClients(false, iama.option.Filers, iama.option.GrpcDialOption, func(client filer_pb.SeaweedFilerClient) error { if err = filer.ReadEntry(iama.masterClient, client, filer.IamConfigDirectory, filer.IamPoliciesFile, &buf); err != nil { return err } @@ -217,7 +221,7 @@ func (iama *IamS3ApiConfigure) PutPolicies(policies *Policies) (err error) { if b, err = json.Marshal(policies); err != nil { return err } - return pb.WithGrpcFilerClient(false, 0, iama.getFilerAddress(), iama.option.GrpcDialOption, func(client filer_pb.SeaweedFilerClient) error { + return pb.WithOneOfGrpcFilerClients(false, iama.option.Filers, iama.option.GrpcDialOption, func(client filer_pb.SeaweedFilerClient) error { if err := filer.SaveInsideFiler(client, filer.IamConfigDirectory, filer.IamPoliciesFile, b); err != nil { return err } diff --git a/weed/s3api/auth_credentials.go b/weed/s3api/auth_credentials.go index a91b3572f..e9c6bbedc 100644 --- a/weed/s3api/auth_credentials.go +++ b/weed/s3api/auth_credentials.go @@ -142,11 +142,12 @@ func NewIdentityAccessManagementWithStore(option *S3ApiServerOption, explicitSto SetFilerClient(string, grpc.DialOption) }); ok { // Use the first filer address for credential store - filerAddr := "" if len(option.Filers) > 0 { - filerAddr = string(option.Filers[0]) + filerAddr := option.Filers[0].ToGrpcAddress() + filerClientSetter.SetFilerClient(filerAddr, option.GrpcDialOption) + } else { + glog.Warningf("No filer addresses configured for credential store") } - filerClientSetter.SetFilerClient(filerAddr, option.GrpcDialOption) } } diff --git a/weed/s3api/s3api_circuit_breaker.go b/weed/s3api/s3api_circuit_breaker.go index 0d7dc283a..2f5e1f580 100644 --- a/weed/s3api/s3api_circuit_breaker.go +++ b/weed/s3api/s3api_circuit_breaker.go @@ -42,6 +42,7 @@ func NewCircuitBreaker(option *S3ApiServerOption) *CircuitBreaker { }) if err != nil { + glog.Warningf("S3 circuit breaker disabled; failed to load config from any filer: %v", err) } return cb diff --git a/weed/s3api/s3api_server.go b/weed/s3api/s3api_server.go index 444e1fd10..bb54f543b 100644 --- a/weed/s3api/s3api_server.go +++ b/weed/s3api/s3api_server.go @@ -69,6 +69,10 @@ func NewS3ApiServer(router *mux.Router, option *S3ApiServerOption) (s3ApiServer } func NewS3ApiServerWithStore(router *mux.Router, option *S3ApiServerOption, explicitStore string) (s3ApiServer *S3ApiServer, err error) { + if len(option.Filers) == 0 { + return nil, fmt.Errorf("at least one filer address is required") + } + startTsNs := time.Now().UnixNano() v := util.GetViper()