From 43315ae30405daa3a2bc91b698135931029ead98 Mon Sep 17 00:00:00 2001 From: Obinna Odirionye Date: Sun, 18 May 2025 22:32:35 +0400 Subject: [PATCH 1/2] fix: ensure proper prefix for tenant repositories in storage operations --- pkg/chartmuseum/server/multitenant/api.go | 14 +- pkg/chartmuseum/server/multitenant/cache.go | 7 +- .../server/multitenant/server_test.go | 141 ++++++++++++++++++ 3 files changed, 159 insertions(+), 3 deletions(-) diff --git a/pkg/chartmuseum/server/multitenant/api.go b/pkg/chartmuseum/server/multitenant/api.go index ac3f667a..7ba6035f 100644 --- a/pkg/chartmuseum/server/multitenant/api.go +++ b/pkg/chartmuseum/server/multitenant/api.go @@ -197,7 +197,12 @@ func (server *MultiTenantServer) uploadProvenanceFile(log cm_logger.LoggingFn, r func (server *MultiTenantServer) checkStorageLimit(repo string, filename string, force bool) (bool, error) { if server.MaxStorageObjects > 0 { - allObjects, err := server.StorageBackend.ListObjects(repo) + // Ensure proper prefix for tenant repos by adding trailing slash + prefix := repo + if repo != "" { + prefix = repo + "/" + } + allObjects, err := server.StorageBackend.ListObjects(prefix) if err != nil { return false, err } @@ -246,7 +251,12 @@ func (server *MultiTenantServer) PutWithLimit(ctx *gin.Context, log cm_logger.Lo defer server.ChartLimits.Unlock() // clean the oldest chart(both index and storage) // storage cache first - objs, err := server.StorageBackend.ListObjects(repo) + // Ensure proper prefix for tenant repos by adding trailing slash + prefix := repo + if repo != "" { + prefix = repo + "/" + } + objs, err := server.StorageBackend.ListObjects(prefix) if err != nil { return err } diff --git a/pkg/chartmuseum/server/multitenant/cache.go b/pkg/chartmuseum/server/multitenant/cache.go index 60fdfb9b..2bcda164 100644 --- a/pkg/chartmuseum/server/multitenant/cache.go +++ b/pkg/chartmuseum/server/multitenant/cache.go @@ -198,7 +198,12 @@ func (server *MultiTenantServer) fetchChartsInStorage(log cm_logger.LoggingFn, r log(cm_logger.DebugLevel, "Fetching chart list from storage", "repo", repo, ) - allObjects, err := server.StorageBackend.ListObjects(repo) + // Ensure proper prefix for tenant repos by adding trailing slash + prefix := repo + if repo != "" { + prefix = repo + "/" + } + allObjects, err := server.StorageBackend.ListObjects(prefix) if err != nil { return []cm_storage.Object{}, err } diff --git a/pkg/chartmuseum/server/multitenant/server_test.go b/pkg/chartmuseum/server/multitenant/server_test.go index 02dfa8a3..ba2054f1 100644 --- a/pkg/chartmuseum/server/multitenant/server_test.go +++ b/pkg/chartmuseum/server/multitenant/server_test.go @@ -18,6 +18,7 @@ package multitenant import ( "bytes" + "errors" "fmt" "io" "mime/multipart" @@ -39,6 +40,8 @@ import ( "sigs.k8s.io/yaml" ) +var TenantPrefixFixTestError = errors.New("tenant prefix test error") + var maxUploadSize = 1024 * 1024 * 20 // These are generated from scripts/setup-test-environment.sh @@ -1359,3 +1362,141 @@ func (suite *MultiTenantServerTestSuite) getBodyWithMultipartFormFiles(fields [] func TestMultiTenantServerTestSuite(t *testing.T) { suite.Run(t, new(MultiTenantServerTestSuite)) } + +type TenantPrefixTestSuite struct { + suite.Suite + StorageBackend storage.Backend + Logger *cm_logger.Logger + Server *MultiTenantServer +} + +func (suite *TenantPrefixTestSuite) SetupSuite() { + // Create test logger + logger, err := cm_logger.NewLogger(cm_logger.LoggerOptions{ + Debug: true, + }) + suite.Nil(err, "no error creating logger") + suite.Logger = logger + + // No physical files needed, just a mock backend + suite.StorageBackend = getMockStorageBackend() + + // Create server with the mock backend and logger + router := cm_router.NewRouter(cm_router.RouterOptions{ + Logger: logger, + Depth: 1, + }) + server, err := NewMultiTenantServer(MultiTenantServerOptions{ + Logger: logger, + Router: router, + StorageBackend: suite.StorageBackend, + TimestampTolerance: time.Second * 0, + EnableAPI: true, + ChartPostFormFieldName: "chart", + ProvPostFormFieldName: "prov", + CacheInterval: time.Second * 0, + }) + suite.Nil(err, "no error creating server") + suite.Server = server +} + +// Custom mock storage backend to simulate the issue with prefixes +type mockStorageBackend struct { + objects []storage.Object +} + +func (m *mockStorageBackend) ListObjects(prefix string) ([]storage.Object, error) { + result := []storage.Object{} + for _, object := range m.objects { + if prefix == "" || strings.HasPrefix(object.Path, prefix) { + result = append(result, object) + } + } + return result, nil +} + +func (m *mockStorageBackend) GetObject(path string) (storage.Object, error) { + for _, object := range m.objects { + if object.Path == path { + return object, nil + } + } + + // This mock needs to handle the fact that our test charts aren't real charts + // Return fake data for any xyz chart to allow the test to continue + if strings.Contains(path, "xyz") { + return storage.Object{ + Path: path, + Content: []byte(`{"metadata":{"name":"xyz","version":"1.0.0"}}`), + }, nil + } + + return storage.Object{}, TenantPrefixFixTestError +} + +func (m *mockStorageBackend) PutObject(path string, content []byte) error { + // Not needed for this test + return nil +} + +func (m *mockStorageBackend) DeleteObject(path string) error { + // Not needed for this test + return nil +} + +// getMockStorageBackend creates a storage backend with files arranged to simulate the issue: +// - abc-def-1.0.0.tgz (root) +// - abc/xyz-1.0.0.tgz +// - abc/xyz-1.0.1.tgz +func getMockStorageBackend() storage.Backend { + now := time.Now() + return &mockStorageBackend{ + objects: []storage.Object{ + { + Path: "abc-def-1.0.0.tgz", + Content: []byte("not-actually-a-chart"), + LastModified: now, + }, + { + Path: "abc/xyz-1.0.0.tgz", + Content: []byte("xyz-chart-content-1.0.0"), + LastModified: now, + }, + { + Path: "abc/xyz-1.0.1.tgz", + Content: []byte("xyz-chart-content-1.0.1"), + LastModified: now, + }, + }, + } +} + +// TestTenantPrefixSeparation tests that similar prefixes don't bleed between repos +func (suite *TenantPrefixTestSuite) TestTenantPrefixSeparation() { + log := suite.Logger.ContextLoggingFn(&gin.Context{}) + + // Directly test the fetchChartsInStorage method which is where our fix was applied + objects, err := suite.Server.fetchChartsInStorage(log, "abc") + suite.Nil(err, "no error fetching charts for abc tenant") + + // Should find exactly 2 objects in abc tenant + suite.Equal(2, len(objects), "should find exactly 2 objects in abc tenant") + + // Each object should be inside the tenant directory + for _, obj := range objects { + suite.True(strings.HasPrefix(obj.Path, "abc/"), "object path should be inside tenant directory") + suite.NotEqual("abc-def-1.0.0.tgz", obj.Path, "should not include root-level abc-def charts") + } + + // Verify the paths of the returned objects + foundPaths := map[string]bool{} + for _, obj := range objects { + foundPaths[obj.Path] = true + } + suite.True(foundPaths["abc/xyz-1.0.0.tgz"], "should include abc/xyz-1.0.0.tgz") + suite.True(foundPaths["abc/xyz-1.0.1.tgz"], "should include abc/xyz-1.0.1.tgz") +} + +func TestTenantPrefixTestSuite(t *testing.T) { + suite.Run(t, new(TenantPrefixTestSuite)) +} From 393915ba37915bf1ffc6db3bfedbb01ef0421aac Mon Sep 17 00:00:00 2001 From: Obinna Odirionye Date: Mon, 19 May 2025 14:37:01 +0400 Subject: [PATCH 2/2] test: enhance tenant prefix isolation tests with depth configurations --- .../server/multitenant/server_test.go | 193 ++++++++---------- 1 file changed, 89 insertions(+), 104 deletions(-) diff --git a/pkg/chartmuseum/server/multitenant/server_test.go b/pkg/chartmuseum/server/multitenant/server_test.go index ba2054f1..c87f4bdf 100644 --- a/pkg/chartmuseum/server/multitenant/server_test.go +++ b/pkg/chartmuseum/server/multitenant/server_test.go @@ -1363,11 +1363,21 @@ func TestMultiTenantServerTestSuite(t *testing.T) { suite.Run(t, new(MultiTenantServerTestSuite)) } +var depthPrefixes = map[int]string{ + 0: "", // depth 0: charts in root + 1: "org1", // depth 1: org/charts + 2: "org1/team1", // depth 2: org/team/charts + 3: "org1/team1/repo1", // depth 3: org/team/repo/charts +} + +// TenantPrefixTestSuite tests that tenant prefixes are properly isolated +// across different depth configurations. It uses real chart files and local +// storage to validate the prefix-handling behavior. type TenantPrefixTestSuite struct { suite.Suite StorageBackend storage.Backend Logger *cm_logger.Logger - Server *MultiTenantServer + TempDirectory string } func (suite *TenantPrefixTestSuite) SetupSuite() { @@ -1378,123 +1388,98 @@ func (suite *TenantPrefixTestSuite) SetupSuite() { suite.Nil(err, "no error creating logger") suite.Logger = logger - // No physical files needed, just a mock backend - suite.StorageBackend = getMockStorageBackend() + // Create temporary directory + timestamp := time.Now().Format("20060102150405") + suite.TempDirectory = fmt.Sprintf("../../../../.test/chartmuseum-tenant-prefix/%s", timestamp) + err = os.MkdirAll(suite.TempDirectory, os.ModePerm) + suite.Nil(err, "no error creating temp directory") + + // Create test directories for each depth + for depth, path := range depthPrefixes { + if path != "" { + err = os.MkdirAll(pathutil.Join(suite.TempDirectory, path), os.ModePerm) + suite.Nil(err, fmt.Sprintf("no error creating directory for depth %d", depth)) + } - // Create server with the mock backend and logger - router := cm_router.NewRouter(cm_router.RouterOptions{ - Logger: logger, - Depth: 1, - }) - server, err := NewMultiTenantServer(MultiTenantServerOptions{ - Logger: logger, - Router: router, - StorageBackend: suite.StorageBackend, - TimestampTolerance: time.Second * 0, - EnableAPI: true, - ChartPostFormFieldName: "chart", - ProvPostFormFieldName: "prov", - CacheInterval: time.Second * 0, - }) - suite.Nil(err, "no error creating server") - suite.Server = server -} + // Copy test charts into each directory + testFiles := []struct { + src string + dest string + }{ + {testTarballPath, pathutil.Join(suite.TempDirectory, path, "mychart-0.1.0.tgz")}, + {testTarballPath, pathutil.Join(suite.TempDirectory, path, "mychart-0.2.0.tgz")}, + } -// Custom mock storage backend to simulate the issue with prefixes -type mockStorageBackend struct { - objects []storage.Object -} + for _, tf := range testFiles { + srcFile, err := os.Open(tf.src) + suite.Nil(err, fmt.Sprintf("no error opening %s", tf.src)) + defer srcFile.Close() -func (m *mockStorageBackend) ListObjects(prefix string) ([]storage.Object, error) { - result := []storage.Object{} - for _, object := range m.objects { - if prefix == "" || strings.HasPrefix(object.Path, prefix) { - result = append(result, object) - } - } - return result, nil -} + destFile, err := os.Create(tf.dest) + suite.Nil(err, fmt.Sprintf("no error creating %s", tf.dest)) + defer destFile.Close() -func (m *mockStorageBackend) GetObject(path string) (storage.Object, error) { - for _, object := range m.objects { - if object.Path == path { - return object, nil + _, err = io.Copy(destFile, srcFile) + suite.Nil(err, fmt.Sprintf("no error copying %s to %s", tf.src, tf.dest)) } } - - // This mock needs to handle the fact that our test charts aren't real charts - // Return fake data for any xyz chart to allow the test to continue - if strings.Contains(path, "xyz") { - return storage.Object{ - Path: path, - Content: []byte(`{"metadata":{"name":"xyz","version":"1.0.0"}}`), - }, nil - } - - return storage.Object{}, TenantPrefixFixTestError -} - -func (m *mockStorageBackend) PutObject(path string, content []byte) error { - // Not needed for this test - return nil -} -func (m *mockStorageBackend) DeleteObject(path string) error { - // Not needed for this test - return nil + // Create storage backend pointing to temp directory + suite.StorageBackend = storage.NewLocalFilesystemBackend(suite.TempDirectory) } -// getMockStorageBackend creates a storage backend with files arranged to simulate the issue: -// - abc-def-1.0.0.tgz (root) -// - abc/xyz-1.0.0.tgz -// - abc/xyz-1.0.1.tgz -func getMockStorageBackend() storage.Backend { - now := time.Now() - return &mockStorageBackend{ - objects: []storage.Object{ - { - Path: "abc-def-1.0.0.tgz", - Content: []byte("not-actually-a-chart"), - LastModified: now, - }, - { - Path: "abc/xyz-1.0.0.tgz", - Content: []byte("xyz-chart-content-1.0.0"), - LastModified: now, - }, - { - Path: "abc/xyz-1.0.1.tgz", - Content: []byte("xyz-chart-content-1.0.1"), - LastModified: now, - }, - }, - } +func (suite *TenantPrefixTestSuite) TearDownSuite() { + os.RemoveAll(suite.TempDirectory) } -// TestTenantPrefixSeparation tests that similar prefixes don't bleed between repos +// TestTenantPrefixSeparation tests that tenant prefixes are properly isolated +// across different depth configurations (0-3) func (suite *TenantPrefixTestSuite) TestTenantPrefixSeparation() { - log := suite.Logger.ContextLoggingFn(&gin.Context{}) - - // Directly test the fetchChartsInStorage method which is where our fix was applied - objects, err := suite.Server.fetchChartsInStorage(log, "abc") - suite.Nil(err, "no error fetching charts for abc tenant") - - // Should find exactly 2 objects in abc tenant - suite.Equal(2, len(objects), "should find exactly 2 objects in abc tenant") - - // Each object should be inside the tenant directory - for _, obj := range objects { - suite.True(strings.HasPrefix(obj.Path, "abc/"), "object path should be inside tenant directory") - suite.NotEqual("abc-def-1.0.0.tgz", obj.Path, "should not include root-level abc-def charts") - } + for _, depth := range []int{0, 1, 2, 3} { + suite.Run(fmt.Sprintf("depth%d", depth), func() { + log := suite.Logger.ContextLoggingFn(&gin.Context{}) + + router := cm_router.NewRouter(cm_router.RouterOptions{ + Logger: suite.Logger, + Depth: depth, + }) + + server, err := NewMultiTenantServer(MultiTenantServerOptions{ + Logger: suite.Logger, + Router: router, + StorageBackend: suite.StorageBackend, + TimestampTolerance: time.Second * 0, + EnableAPI: true, + ChartPostFormFieldName: "chart", + ProvPostFormFieldName: "prov", + CacheInterval: time.Second * 0, + }) + suite.Nil(err, "no error creating server") + + // Test fetchChartsInStorage which handles prefix isolation + prefix := depthPrefixes[depth] + objects, err := server.fetchChartsInStorage(log, prefix) + suite.Nil(err, "no error fetching charts") + + // Should find exactly 2 objects + suite.Equal(2, len(objects), fmt.Sprintf("should find exactly 2 objects for depth %d", depth)) + + // Verify the paths of the returned objects + foundPaths := map[string]bool{} + for _, obj := range objects { + foundPaths[obj.Path] = true + } + + // Storage backend strips the prefix, so we always expect bare filenames + expectedPath1 := "mychart-0.1.0.tgz" + expectedPath2 := "mychart-0.2.0.tgz" - // Verify the paths of the returned objects - foundPaths := map[string]bool{} - for _, obj := range objects { - foundPaths[obj.Path] = true + suite.True(foundPaths[expectedPath1], + fmt.Sprintf("should include %s", expectedPath1)) + suite.True(foundPaths[expectedPath2], + fmt.Sprintf("should include %s", expectedPath2)) + }) } - suite.True(foundPaths["abc/xyz-1.0.0.tgz"], "should include abc/xyz-1.0.0.tgz") - suite.True(foundPaths["abc/xyz-1.0.1.tgz"], "should include abc/xyz-1.0.1.tgz") } func TestTenantPrefixTestSuite(t *testing.T) {