From f6f38598260133b50b2386704d571134e7ecda79 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Mon, 1 Dec 2025 12:19:02 -0800 Subject: [PATCH] Fix #7575: Correct interface check for filer address function in admin server (#7588) * Fix #7575: Correct interface check for filer address function in admin server Problem: User creation in object store was failing with error: 'filer_etc: filer address function not configured' Root Cause: In admin_server.go, the code checked for incorrect interface method SetFilerClient(string, grpc.DialOption) instead of the actual SetFilerAddressFunc(func() pb.ServerAddress, grpc.DialOption) This interface mismatch prevented the filer address function from being configured, causing user creation operations to fail. Solution: - Fixed interface check to use SetFilerAddressFunc - Updated function call to properly configure filer address function - Function now dynamically returns current active filer address Tests Added: - Unit tests in weed/admin/dash/user_management_test.go - Integration tests in test/admin/user_creation_integration_test.go - Documentation in test/admin/README.md All tests pass successfully. * Fix #7575: Correct interface check for filer address function in admin UI Problem: User creation in Admin UI was failing with error: 'filer_etc: filer address function not configured' Root Cause: In admin_server.go, the code checked for incorrect interface method SetFilerClient(string, grpc.DialOption) instead of the actual SetFilerAddressFunc(func() pb.ServerAddress, grpc.DialOption) This interface mismatch prevented the filer address function from being configured, causing user creation operations to fail in the Admin UI. Note: This bug only affects the Admin UI. The S3 API and weed shell commands (s3.configure) were unaffected as they use the correct interface or bypass the credential manager entirely. Solution: - Fixed interface check in admin_server.go to use SetFilerAddressFunc - Updated function call to properly configure filer address function - Function now dynamically returns current active filer (HA-aware) - Cleaned up redundant comments in the code Tests Added: - Unit tests in weed/admin/dash/user_management_test.go * TestFilerAddressFunctionInterface - verifies correct interface * TestGenerateAccessKey - tests key generation * TestGenerateSecretKey - tests secret generation * TestGenerateAccountId - tests account ID generation All tests pass and will run automatically in CI. * Fix #7575: Correct interface check for filer address function in admin UI Problem: User creation in Admin UI was failing with error: 'filer_etc: filer address function not configured' Root Cause: 1. In admin_server.go, the code checked for incorrect interface method SetFilerClient(string, grpc.DialOption) instead of the actual SetFilerAddressFunc(func() pb.ServerAddress, grpc.DialOption) 2. The admin command was missing the filer_etc import, so the store was never registered This interface mismatch prevented the filer address function from being configured, causing user creation operations to fail in the Admin UI. Note: This bug only affects the Admin UI. The S3 API and weed shell commands (s3.configure) were unaffected as they use the correct interface or bypass the credential manager entirely. Solution: - Added filer_etc import to weed/command/admin.go to register the store - Fixed interface check in admin_server.go to use SetFilerAddressFunc - Updated function call to properly configure filer address function - Function now dynamically returns current active filer (HA-aware) - Hoisted credentialManager assignment to reduce code duplication Tests Added: - Unit tests in weed/admin/dash/user_management_test.go * TestFilerAddressFunctionInterface - verifies correct interface * TestGenerateAccessKey - tests key generation * TestGenerateSecretKey - tests secret generation * TestGenerateAccountId - tests account ID generation All tests pass and will run automatically in CI. --- weed/admin/dash/admin_server.go | 27 +++--- weed/admin/dash/user_management_test.go | 109 ++++++++++++++++++++++++ weed/command/admin.go | 1 + 3 files changed, 122 insertions(+), 15 deletions(-) create mode 100644 weed/admin/dash/user_management_test.go diff --git a/weed/admin/dash/admin_server.go b/weed/admin/dash/admin_server.go index 4a1dd592f..4ce357502 100644 --- a/weed/admin/dash/admin_server.go +++ b/weed/admin/dash/admin_server.go @@ -98,33 +98,30 @@ func NewAdminServer(masters string, templateFS http.FileSystem, dataDir string) glog.Warningf("Failed to initialize credential manager: %v", err) // Continue without credential manager - will fall back to legacy approach } else { - // For stores that need filer client details, set them + server.credentialManager = credentialManager + + // For stores that need filer address function, set them if store := credentialManager.GetStore(); store != nil { - if filerClientSetter, ok := store.(interface { - SetFilerClient(string, grpc.DialOption) + if filerFuncSetter, ok := store.(interface { + SetFilerAddressFunc(func() pb.ServerAddress, grpc.DialOption) }); ok { - // We'll set the filer client later when we discover filers - // For now, just store the credential manager - server.credentialManager = credentialManager - - // Set up a goroutine to set filer client once we discover filers + // Set up a goroutine to configure filer address function once we discover filers go func() { for { filerAddr := server.GetFilerAddress() if filerAddr != "" { - filerClientSetter.SetFilerClient(filerAddr, server.grpcDialOption) - glog.V(1).Infof("Set filer client for credential manager: %s", filerAddr) + // Configure the function to dynamically return the current active filer (HA-aware) + filerFuncSetter.SetFilerAddressFunc(func() pb.ServerAddress { + return pb.ServerAddress(server.GetFilerAddress()) + }, server.grpcDialOption) + glog.V(1).Infof("Set filer address function for credential manager: %s", filerAddr) break } glog.V(1).Infof("Waiting for filer discovery for credential manager...") - time.Sleep(5 * time.Second) // Retry every 5 seconds + time.Sleep(5 * time.Second) } }() - } else { - server.credentialManager = credentialManager } - } else { - server.credentialManager = credentialManager } } diff --git a/weed/admin/dash/user_management_test.go b/weed/admin/dash/user_management_test.go new file mode 100644 index 000000000..6e63bfa31 --- /dev/null +++ b/weed/admin/dash/user_management_test.go @@ -0,0 +1,109 @@ +package dash + +import ( + "testing" + + "github.com/seaweedfs/seaweedfs/weed/credential" + _ "github.com/seaweedfs/seaweedfs/weed/credential/filer_etc" // Import to register filer_etc store + "github.com/seaweedfs/seaweedfs/weed/pb" + "google.golang.org/grpc" +) + +// TestFilerAddressFunctionInterface tests that the filer_etc store +// implements the correct SetFilerAddressFunc interface (issue #7575) +func TestFilerAddressFunctionInterface(t *testing.T) { + // Create credential manager with filer_etc store + credentialManager, err := credential.NewCredentialManagerWithDefaults("") + if err != nil { + t.Fatalf("Failed to initialize credential manager: %v", err) + } + + store := credentialManager.GetStore() + if store == nil { + t.Fatal("Credential store is nil") + } + + // Check if store is filer_etc type + if store.GetName() != credential.StoreTypeFilerEtc { + t.Skipf("Skipping test - store is not filer_etc (got: %s)", store.GetName()) + } + + // Check if store implements SetFilerAddressFunc interface + // This is the critical check for bug #7575 + filerFuncSetter, ok := store.(interface { + SetFilerAddressFunc(func() pb.ServerAddress, grpc.DialOption) + }) + if !ok { + t.Fatal("FilerEtcStore does not implement SetFilerAddressFunc interface - bug #7575") + } + + // Verify we can call the method without panic + mockFilerAddress := pb.ServerAddress("localhost:8888") + filerFuncSetter.SetFilerAddressFunc(func() pb.ServerAddress { + return mockFilerAddress + }, grpc.WithInsecure()) + + t.Log("FilerEtcStore correctly implements SetFilerAddressFunc interface") +} + +// TestGenerateAccessKey tests the access key generation function +func TestGenerateAccessKey(t *testing.T) { + key1 := generateAccessKey() + key2 := generateAccessKey() + + // Check length + if len(key1) != 20 { + t.Errorf("Expected access key length 20, got %d", len(key1)) + } + + // Check uniqueness + if key1 == key2 { + t.Error("Generated access keys should be unique") + } + + // Check character set + for _, c := range key1 { + if !((c >= 'A' && c <= 'Z') || (c >= '0' && c <= '9')) { + t.Errorf("Access key contains invalid character: %c", c) + } + } +} + +// TestGenerateSecretKey tests the secret key generation function +func TestGenerateSecretKey(t *testing.T) { + key1 := generateSecretKey() + key2 := generateSecretKey() + + // Check length (base64 encoding of 30 bytes = 40 characters) + if len(key1) != 40 { + t.Errorf("Expected secret key length 40, got %d", len(key1)) + } + + // Check uniqueness + if key1 == key2 { + t.Error("Generated secret keys should be unique") + } +} + +// TestGenerateAccountId tests the account ID generation function +func TestGenerateAccountId(t *testing.T) { + id1 := generateAccountId() + id2 := generateAccountId() + + // Check length + if len(id1) != 12 { + t.Errorf("Expected account ID length 12, got %d", len(id1)) + } + + // Check that it's a number + for _, c := range id1 { + if c < '0' || c > '9' { + t.Errorf("Account ID contains non-digit character: %c", c) + } + } + + // Check uniqueness (they should usually be different) + if id1 == id2 { + t.Log("Warning: Generated account IDs are the same (rare but possible)") + } +} diff --git a/weed/command/admin.go b/weed/command/admin.go index ded85a2ee..c8a4a4b12 100644 --- a/weed/command/admin.go +++ b/weed/command/admin.go @@ -22,6 +22,7 @@ import ( "github.com/seaweedfs/seaweedfs/weed/admin" "github.com/seaweedfs/seaweedfs/weed/admin/dash" "github.com/seaweedfs/seaweedfs/weed/admin/handlers" + _ "github.com/seaweedfs/seaweedfs/weed/credential/filer_etc" // Register filer_etc credential store "github.com/seaweedfs/seaweedfs/weed/pb" "github.com/seaweedfs/seaweedfs/weed/security" "github.com/seaweedfs/seaweedfs/weed/util"