diff --git a/pkg/registry/apis/provisioning/repository/github.go b/pkg/registry/apis/provisioning/repository/github.go index 90b64584666..21ff70f3ae5 100644 --- a/pkg/registry/apis/provisioning/repository/github.go +++ b/pkg/registry/apis/provisioning/repository/github.go @@ -337,10 +337,18 @@ func (r *githubRepository) Write(ctx context.Context, path string, ref string, d if ref == "" { ref = r.config.Spec.GitHub.Branch } + ctx, _ = r.logger(ctx, ref) finalPath := safepath.Join(r.config.Spec.GitHub.Path, path) + _, err := r.Read(ctx, finalPath, ref) + if err != nil && !(errors.Is(err, ErrFileNotFound)) { + return fmt.Errorf("failed to check if file exists before writing: %w", err) + } + if err == nil { + return r.Update(ctx, finalPath, ref, data, message) + } - return writeWithReadThenCreateOrUpdate(ctx, r, finalPath, ref, data, message) + return r.Create(ctx, finalPath, ref, data, message) } func (r *githubRepository) Delete(ctx context.Context, path, ref, comment string) error { diff --git a/pkg/registry/apis/provisioning/repository/local.go b/pkg/registry/apis/provisioning/repository/local.go index 7c5a237a94c..5ee716ffaf5 100644 --- a/pkg/registry/apis/provisioning/repository/local.go +++ b/pkg/registry/apis/provisioning/repository/local.go @@ -112,35 +112,34 @@ func (r *localRepository) Config() *provisioning.Repository { } // Validate implements provisioning.Repository. -func (r *localRepository) Validate() (fields field.ErrorList) { +func (r *localRepository) Validate() field.ErrorList { cfg := r.config.Spec.Local if cfg == nil { - fields = append(fields, &field.Error{ + return field.ErrorList{&field.Error{ Type: field.ErrorTypeRequired, Field: "spec.local", - }) - return fields + }} } // The path value must be set for local provisioning if cfg.Path == "" { - fields = append(fields, field.Required(field.NewPath("spec", "local", "path"), - "must enter a path to local file")) + return field.ErrorList{field.Required(field.NewPath("spec", "local", "path"), + "must enter a path to local file")} + } + + if err := safepath.IsSafe(cfg.Path); err != nil { + return field.ErrorList{field.Invalid(field.NewPath("spec", "local", "path"), + cfg.Path, err.Error())} } // Check if it is valid _, err := r.resolver.LocalPath(cfg.Path) if err != nil { - fields = append(fields, field.Invalid(field.NewPath("spec", "local", "path"), - cfg.Path, err.Error())) + return field.ErrorList{field.Invalid(field.NewPath("spec", "local", "path"), + cfg.Path, err.Error())} } - if err := safepath.IsSafe(cfg.Path); err != nil { - fields = append(fields, field.Invalid(field.NewPath("spec", "local", "path"), - cfg.Path, err.Error())) - } - - return fields + return nil } // Test implements provisioning.Repository. @@ -172,18 +171,7 @@ func (r *localRepository) validateRequest(ref string) error { if ref != "" { return apierrors.NewBadRequest("local repository does not support ref") } - if r.path == "" { - _, err := r.resolver.LocalPath(r.config.Spec.Local.Path) - if err != nil { - return err - } - return &apierrors.StatusError{ - ErrStatus: metav1.Status{ - Message: "the service is missing a root path", - Code: http.StatusFailedDependency, - }, - } - } + return nil } @@ -261,7 +249,7 @@ func (r *localRepository) ReadTree(ctx context.Context, ref string) ([]FileTreeE entry.Blob = true entry.Hash, _, err = r.calculateFileHash(path) if err != nil { - return fmt.Errorf("failed to read and calculate hash of path %s: %w", path, err) + return fmt.Errorf("read and calculate hash of path %s: %w", path, err) } } // TODO: do folders have a trailing slash? @@ -282,7 +270,7 @@ func (r *localRepository) calculateFileHash(path string) (string, int64, error) //nolint:gosec file, err := os.OpenFile(path, os.O_RDONLY, 0) if err != nil { - return "", 0, err + return "", 0, fmt.Errorf("open file: %w", err) } // TODO: Define what hashing algorithm we want to use for the entire repository. Maybe a config option? @@ -290,7 +278,7 @@ func (r *localRepository) calculateFileHash(path string) (string, int64, error) // TODO: context-aware io.Copy? Is that even possible with a reasonable impl? size, err := io.Copy(hasher, file) if err != nil { - return "", 0, err + return "", 0, fmt.Errorf("copy file: %w", err) } // NOTE: EncodeToString (& hex.Encode for that matter) return lower-case hex. return hex.EncodeToString(hasher.Sum(nil)), size, nil @@ -339,9 +327,14 @@ func (r *localRepository) Update(ctx context.Context, path string, ref string, d return apierrors.NewBadRequest("cannot update a directory") } - if _, err := os.Stat(path); errors.Is(err, os.ErrNotExist) { + f, err := os.Stat(path) + if err != nil && errors.Is(err, os.ErrNotExist) { return ErrFileNotFound } + if f.IsDir() { + return apierrors.NewBadRequest("path exists but it is a directory") + } + return os.WriteFile(path, data, 0600) } diff --git a/pkg/registry/apis/provisioning/repository/local_test.go b/pkg/registry/apis/provisioning/repository/local_test.go index 4fc6c6545a1..c924a03d910 100644 --- a/pkg/registry/apis/provisioning/repository/local_test.go +++ b/pkg/registry/apis/provisioning/repository/local_test.go @@ -2,13 +2,23 @@ package repository import ( "context" + "errors" + "net/http" + "os" + "path/filepath" + "sort" + "strings" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" + field "k8s.io/apimachinery/pkg/util/validation/field" - "github.com/grafana/grafana/pkg/apis/provisioning/v0alpha1" + provisioning "github.com/grafana/grafana/pkg/apis/provisioning/v0alpha1" ) func TestLocalResolver(t *testing.T) { @@ -27,9 +37,9 @@ func TestLocalResolver(t *testing.T) { require.Error(t, err) // Check valid errors - r := NewLocal(&v0alpha1.Repository{ - Spec: v0alpha1.RepositorySpec{ - Local: &v0alpha1.LocalRepositoryConfig{ + r := NewLocal(&provisioning.Repository{ + Spec: provisioning.RepositorySpec{ + Local: &provisioning.LocalRepositoryConfig{ Path: "github", }, }, @@ -89,9 +99,9 @@ func TestLocal(t *testing.T) { {"absolute path with multiple prefixes", "/devenv/test", []string{"/home/grafana", "/devenv"}, "/devenv/test/"}, } { t.Run("valid: "+tc.Name, func(t *testing.T) { - r := NewLocal(&v0alpha1.Repository{ - Spec: v0alpha1.RepositorySpec{ - Local: &v0alpha1.LocalRepositoryConfig{ + r := NewLocal(&provisioning.Repository{ + Spec: provisioning.RepositorySpec{ + Local: &provisioning.LocalRepositoryConfig{ Path: tc.Path, }, }, @@ -115,9 +125,9 @@ func TestLocal(t *testing.T) { {"unconfigured prefix", "invalid/path", []string{"devenv", "/tmp", "test"}}, } { t.Run("invalid: "+tc.Name, func(t *testing.T) { - r := NewLocal(&v0alpha1.Repository{ - Spec: v0alpha1.RepositorySpec{ - Local: &v0alpha1.LocalRepositoryConfig{ + r := NewLocal(&provisioning.Repository{ + Spec: provisioning.RepositorySpec{ + Local: &provisioning.LocalRepositoryConfig{ Path: tc.Path, }, }, @@ -135,3 +145,1313 @@ func TestLocal(t *testing.T) { }) } } + +func TestLocalRepository_Test(t *testing.T) { + // Test cases for the Test method + testCases := []struct { + name string + path string + pathExists bool + expectedCode int + expectedResult bool + }{ + { + name: "valid path that exists", + path: "valid/path/", + pathExists: true, + expectedCode: http.StatusOK, + expectedResult: true, + }, + { + name: "valid path that doesn't exist", + path: "valid/nonexistent", + pathExists: false, + expectedCode: http.StatusBadRequest, + expectedResult: false, + }, + { + name: "invalid path with path traversal", + path: "../../../etc/passwd", + pathExists: false, + expectedCode: http.StatusBadRequest, + expectedResult: false, + }, + { + name: "invalid path with special characters", + path: "path/with/*/wildcards", + pathExists: false, + expectedCode: http.StatusBadRequest, + expectedResult: false, + }, + { + name: "empty path", + path: "", + pathExists: false, + expectedCode: http.StatusBadRequest, + expectedResult: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + // Create a temporary directory for testing + tempDir := t.TempDir() + + // Setup the test directory if needed + testPath := filepath.Join(tempDir, tc.path) + if tc.pathExists { + err := os.MkdirAll(testPath, 0750) + require.NoError(t, err, "Failed to create test directory") + } + + // Create a resolver that permits the temp directory + resolver := &LocalFolderResolver{ + PermittedPrefixes: []string{tempDir}, + HomePath: tempDir, + } + + // Create the repository with the test path + repo := NewLocal(&provisioning.Repository{ + Spec: provisioning.RepositorySpec{ + Local: &provisioning.LocalRepositoryConfig{ + Path: tc.path, + }, + }, + }, resolver) + + // If we're testing a valid path, set it to our test path + if tc.path != "" { + repo.path = testPath + } + + // Call the Test method + results, err := repo.Test(context.Background()) + + // Verify results + require.NoError(t, err, "Test method should not return an error") + assert.Equal(t, tc.expectedResult, results.Success, "Success flag should match expected") + assert.Equal(t, tc.expectedCode, results.Code, "Status code should match expected") + }) + } +} + +func TestLocalRepository_Validate(t *testing.T) { + testCases := []struct { + name string + config *provisioning.LocalRepositoryConfig + permittedPath string + expectedErrs []field.Error + }{ + { + name: "valid configuration", + config: &provisioning.LocalRepositoryConfig{Path: "valid/path"}, + permittedPath: "valid", + expectedErrs: nil, + }, + { + name: "missing local config", + config: nil, + permittedPath: "valid", + expectedErrs: []field.Error{ + { + Type: field.ErrorTypeRequired, + Field: "spec.local", + }, + }, + }, + { + name: "empty path", + config: &provisioning.LocalRepositoryConfig{Path: ""}, + permittedPath: "valid", + expectedErrs: []field.Error{ + { + Type: field.ErrorTypeRequired, + Field: "spec.local.path", + Detail: "must enter a path to local file", + BadValue: "", + }, + }, + }, + { + name: "path not in permitted prefixes", + config: &provisioning.LocalRepositoryConfig{Path: "invalid/path"}, + permittedPath: "valid", + expectedErrs: []field.Error{ + { + Type: field.ErrorTypeInvalid, + Field: "spec.local.path", + BadValue: "invalid/path", + Detail: "the path given ('invalid/path') is invalid for a local repository (the path matches no permitted prefix)", + }, + }, + }, + { + name: "unsafe path with directory traversal", + config: &provisioning.LocalRepositoryConfig{Path: "../../../etc/passwd"}, + permittedPath: "valid", + expectedErrs: []field.Error{ + { + Type: field.ErrorTypeInvalid, + Field: "spec.local.path", + BadValue: "../../../etc/passwd", + Detail: "path contains traversal attempt (./ or ../)", + }, + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + // Create a temporary directory for testing + tempDir := t.TempDir() + permittedPath := filepath.Join(tempDir, tc.permittedPath) + + // Create the permitted directory + if tc.permittedPath != "" { + err := os.MkdirAll(permittedPath, 0750) + require.NoError(t, err, "Failed to create permitted directory") + } + + // Create a resolver that permits the specific path + resolver := &LocalFolderResolver{ + PermittedPrefixes: []string{permittedPath}, + HomePath: tempDir, + } + + // Create repository config + repoConfig := &provisioning.Repository{ + Spec: provisioning.RepositorySpec{ + Local: tc.config, + }, + } + + // Create the repository + repo := NewLocal(repoConfig, resolver) + + // Call the Validate method + errors := repo.Validate() + + // Verify results + if tc.expectedErrs == nil { + assert.Empty(t, errors, "Expected no validation errors") + } else { + assert.Len(t, errors, len(tc.expectedErrs), "Number of validation errors should match expected") + for i, expectedErr := range tc.expectedErrs { + assert.Equal(t, expectedErr.Type, errors[i].Type, "Error type should match") + assert.Equal(t, expectedErr.Field, errors[i].Field, "Error field should match") + assert.Equal(t, expectedErr.Detail, errors[i].Detail, "Error detail should match") + assert.Equal(t, expectedErr.BadValue, errors[i].BadValue, "Error bad value should match") + } + } + }) + } +} + +func TestInvalidLocalFolderError(t *testing.T) { + testCases := []struct { + name string + path string + additionalInfo string + expectedMsg string + expectedStatus metav1.Status + }{ + { + name: "basic error", + path: "/invalid/path", + additionalInfo: "not allowed", + expectedMsg: "the path given ('/invalid/path') is invalid for a local repository (not allowed)", + expectedStatus: metav1.Status{ + Status: metav1.StatusFailure, + Code: http.StatusBadRequest, + Reason: metav1.StatusReasonBadRequest, + Message: "the path given ('/invalid/path') is invalid for a local repository (not allowed)", + }, + }, + { + name: "empty path", + path: "", + additionalInfo: "path cannot be empty", + expectedMsg: "the path given ('') is invalid for a local repository (path cannot be empty)", + expectedStatus: metav1.Status{ + Status: metav1.StatusFailure, + Code: http.StatusBadRequest, + Reason: metav1.StatusReasonBadRequest, + Message: "the path given ('') is invalid for a local repository (path cannot be empty)", + }, + }, + { + name: "no permitted prefixes", + path: "/some/path", + additionalInfo: "no permitted prefixes were configured", + expectedMsg: "the path given ('/some/path') is invalid for a local repository (no permitted prefixes were configured)", + expectedStatus: metav1.Status{ + Status: metav1.StatusFailure, + Code: http.StatusBadRequest, + Reason: metav1.StatusReasonBadRequest, + Message: "the path given ('/some/path') is invalid for a local repository (no permitted prefixes were configured)", + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + // Create the error + err := &InvalidLocalFolderError{ + Path: tc.path, + AdditionalInfo: tc.additionalInfo, + } + + // Test Error() method + assert.Equal(t, tc.expectedMsg, err.Error(), "Error message should match expected") + + // Test Status() method + status := err.Status() + assert.Equal(t, tc.expectedStatus.Status, status.Status, "Status should match") + assert.Equal(t, tc.expectedStatus.Code, status.Code, "Status code should match") + assert.Equal(t, tc.expectedStatus.Reason, status.Reason, "Status reason should match") + assert.Equal(t, tc.expectedStatus.Message, status.Message, "Status message should match") + + // Verify it implements the expected interfaces + var apiStatus apierrors.APIStatus + assert.True(t, errors.As(err, &apiStatus), "Should implement APIStatus interface") + }) + } +} + +func TestLocalRepository_Delete(t *testing.T) { + testCases := []struct { + name string + setup func(t *testing.T) (string, *localRepository) + path string + ref string + comment string + expectedErr error + }{ + { + name: "delete existing file", + setup: func(t *testing.T) (string, *localRepository) { + // Create a temporary directory for testing + tempDir := t.TempDir() + + // Create a test file + testFilePath := filepath.Join(tempDir, "test-file.txt") + err := os.WriteFile(testFilePath, []byte("test content"), 0600) + require.NoError(t, err) + + // Create repository with the temp directory as permitted prefix + repo := &localRepository{ + config: &provisioning.Repository{ + Spec: provisioning.RepositorySpec{ + Local: &provisioning.LocalRepositoryConfig{ + Path: tempDir, + }, + }, + }, + resolver: &LocalFolderResolver{ + PermittedPrefixes: []string{tempDir}, + }, + path: tempDir, + } + + return tempDir, repo + }, + path: "test-file.txt", + ref: "", + comment: "test delete", + expectedErr: nil, + }, + { + name: "delete non-existent file", + setup: func(t *testing.T) (string, *localRepository) { + // Create a temporary directory for testing + tempDir := t.TempDir() + + // Create repository with the temp directory as permitted prefix + repo := &localRepository{ + config: &provisioning.Repository{ + Spec: provisioning.RepositorySpec{ + Local: &provisioning.LocalRepositoryConfig{ + Path: tempDir, + }, + }, + }, + resolver: &LocalFolderResolver{ + PermittedPrefixes: []string{tempDir}, + }, + path: tempDir, + } + + return tempDir, repo + }, + path: "non-existent-file.txt", + ref: "", + comment: "test delete non-existent", + expectedErr: os.ErrNotExist, + }, + { + name: "delete with ref not supported", + setup: func(t *testing.T) (string, *localRepository) { + // Create a temporary directory for testing + tempDir := t.TempDir() + + // Create repository with the temp directory as permitted prefix + repo := &localRepository{ + config: &provisioning.Repository{ + Spec: provisioning.RepositorySpec{ + Local: &provisioning.LocalRepositoryConfig{ + Path: tempDir, + }, + }, + }, + resolver: &LocalFolderResolver{ + PermittedPrefixes: []string{tempDir}, + }, + path: tempDir, + } + + return tempDir, repo + }, + path: "test-file.txt", + ref: "main", + comment: "test delete with ref", + expectedErr: apierrors.NewBadRequest("local repository does not support ref"), + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + // Setup test environment + _, repo := tc.setup(t) + + // Execute the delete operation + err := repo.Delete(context.Background(), tc.path, tc.ref, tc.comment) + + // Verify results + if tc.expectedErr != nil { + require.Error(t, err) + if errors.Is(tc.expectedErr, os.ErrNotExist) { + assert.True(t, errors.Is(err, os.ErrNotExist), "Expected os.ErrNotExist error") + } else { + assert.Equal(t, tc.expectedErr.Error(), err.Error(), "Error message should match expected") + } + } else { + require.NoError(t, err) + + // Verify the file was actually deleted + _, statErr := os.Stat(filepath.Join(repo.path, tc.path)) + assert.True(t, errors.Is(statErr, os.ErrNotExist), "File should be deleted") + } + }) + } +} + +func TestLocalRepository_Update(t *testing.T) { + testCases := []struct { + name string + setup func(t *testing.T) (string, *localRepository) + path string + ref string + data []byte + comment string + expectedErr error + }{ + { + name: "update existing file", + setup: func(t *testing.T) (string, *localRepository) { + tempDir := t.TempDir() + + // Create a file to update + filePath := filepath.Join(tempDir, "existing-file.txt") + require.NoError(t, os.WriteFile(filePath, []byte("initial content"), 0600)) + + repo := &localRepository{ + config: &provisioning.Repository{ + Spec: provisioning.RepositorySpec{ + Local: &provisioning.LocalRepositoryConfig{ + Path: tempDir, + }, + }, + }, + resolver: &LocalFolderResolver{ + PermittedPrefixes: []string{tempDir}, + }, + path: tempDir, + } + + return tempDir, repo + }, + path: "existing-file.txt", + ref: "", + data: []byte("updated content"), + comment: "", + expectedErr: nil, + }, + { + name: "update existing directory as a file", + setup: func(t *testing.T) (string, *localRepository) { + tempDir := t.TempDir() + + // Create a directory + dirPath := filepath.Join(tempDir, "existing-dir") + require.NoError(t, os.MkdirAll(dirPath, 0700)) + + repo := &localRepository{ + config: &provisioning.Repository{ + Spec: provisioning.RepositorySpec{ + Local: &provisioning.LocalRepositoryConfig{ + Path: tempDir, + }, + }, + }, + resolver: &LocalFolderResolver{ + PermittedPrefixes: []string{tempDir}, + }, + path: tempDir, + } + + return tempDir, repo + }, + path: "existing-dir", + ref: "", + data: []byte("file content"), + comment: "", + expectedErr: apierrors.NewBadRequest("path exists but it is a directory"), + }, + { + name: "update non-existent file", + setup: func(t *testing.T) (string, *localRepository) { + tempDir := t.TempDir() + + repo := &localRepository{ + config: &provisioning.Repository{ + Spec: provisioning.RepositorySpec{ + Local: &provisioning.LocalRepositoryConfig{ + Path: tempDir, + }, + }, + }, + resolver: &LocalFolderResolver{ + PermittedPrefixes: []string{tempDir}, + }, + path: tempDir, + } + + return tempDir, repo + }, + path: "non-existent-file.txt", + ref: "", + data: []byte("content"), + comment: "", + expectedErr: ErrFileNotFound, + }, + { + name: "update directory", + setup: func(t *testing.T) (string, *localRepository) { + tempDir := t.TempDir() + + // Create a directory + dirPath := filepath.Join(tempDir, "test-dir") + require.NoError(t, os.MkdirAll(dirPath, 0700)) + + repo := &localRepository{ + config: &provisioning.Repository{ + Spec: provisioning.RepositorySpec{ + Local: &provisioning.LocalRepositoryConfig{ + Path: tempDir, + }, + }, + }, + resolver: &LocalFolderResolver{ + PermittedPrefixes: []string{tempDir}, + }, + path: tempDir, + } + + return tempDir, repo + }, + path: "test-dir/", + ref: "", + data: []byte("content"), + comment: "", + expectedErr: apierrors.NewBadRequest("cannot update a directory"), + }, + { + name: "update with ref", + setup: func(t *testing.T) (string, *localRepository) { + tempDir := t.TempDir() + + // Create a file to update + filePath := filepath.Join(tempDir, "test-file.txt") + require.NoError(t, os.WriteFile(filePath, []byte("initial content"), 0600)) + + repo := &localRepository{ + config: &provisioning.Repository{ + Spec: provisioning.RepositorySpec{ + Local: &provisioning.LocalRepositoryConfig{ + Path: tempDir, + }, + }, + }, + resolver: &LocalFolderResolver{ + PermittedPrefixes: []string{tempDir}, + }, + path: tempDir, + } + + return tempDir, repo + }, + path: "test-file.txt", + ref: "main", + data: []byte("updated content"), + comment: "test update with ref", + expectedErr: apierrors.NewBadRequest("local repository does not support ref"), + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + // Setup test environment + _, repo := tc.setup(t) + + // Execute the update operation + err := repo.Update(context.Background(), tc.path, tc.ref, tc.data, tc.comment) + + // Verify results + if tc.expectedErr != nil { + require.Error(t, err) + assert.Equal(t, tc.expectedErr.Error(), err.Error(), "Error message should match expected") + } else { + require.NoError(t, err) + + // Verify the file was actually updated + updatedContent, readErr := os.ReadFile(filepath.Join(repo.path, tc.path)) + require.NoError(t, readErr) + assert.Equal(t, tc.data, updatedContent, "File content should be updated") + } + }) + } +} + +func TestLocalRepository_Write(t *testing.T) { + testCases := []struct { + name string + setup func(t *testing.T) (string, *localRepository) + path string + ref string + data []byte + comment string + expectedErr error + }{ + { + name: "write new file", + setup: func(t *testing.T) (string, *localRepository) { + tempDir := t.TempDir() + repo := &localRepository{ + config: &provisioning.Repository{ + Spec: provisioning.RepositorySpec{ + Local: &provisioning.LocalRepositoryConfig{ + Path: tempDir, + }, + }, + }, + resolver: &LocalFolderResolver{ + PermittedPrefixes: []string{tempDir}, + }, + path: tempDir, + } + + return tempDir, repo + }, + path: "new-file.txt", + data: []byte("new content"), + comment: "test write new file", + }, + { + name: "overwrite existing file", + setup: func(t *testing.T) (string, *localRepository) { + tempDir := t.TempDir() + + // Create a file to be overwritten + existingFilePath := filepath.Join(tempDir, "existing-file.txt") + require.NoError(t, os.WriteFile(existingFilePath, []byte("original content"), 0600)) + + repo := &localRepository{ + config: &provisioning.Repository{ + Spec: provisioning.RepositorySpec{ + Local: &provisioning.LocalRepositoryConfig{ + Path: tempDir, + }, + }, + }, + resolver: &LocalFolderResolver{ + PermittedPrefixes: []string{tempDir}, + }, + path: tempDir, + } + + return tempDir, repo + }, + path: "existing-file.txt", + data: []byte("updated content"), + comment: "test overwrite existing file", + }, + { + name: "create directory", + setup: func(t *testing.T) (string, *localRepository) { + tempDir := t.TempDir() + repo := &localRepository{ + config: &provisioning.Repository{ + Spec: provisioning.RepositorySpec{ + Local: &provisioning.LocalRepositoryConfig{ + Path: tempDir, + }, + }, + }, + resolver: &LocalFolderResolver{ + PermittedPrefixes: []string{tempDir}, + }, + path: tempDir, + } + + return tempDir, repo + }, + path: "new-dir/", + data: nil, + comment: "test create directory", + }, + { + name: "create file in nested directory", + setup: func(t *testing.T) (string, *localRepository) { + tempDir := t.TempDir() + repo := &localRepository{ + config: &provisioning.Repository{ + Spec: provisioning.RepositorySpec{ + Local: &provisioning.LocalRepositoryConfig{ + Path: tempDir, + }, + }, + }, + resolver: &LocalFolderResolver{ + PermittedPrefixes: []string{tempDir}, + }, + path: tempDir, + } + + return tempDir, repo + }, + path: "nested/dir/file.txt", + data: []byte("nested file content"), + comment: "test create file in nested directory", + }, + { + name: "write with ref should fail", + setup: func(t *testing.T) (string, *localRepository) { + tempDir := t.TempDir() + repo := &localRepository{ + config: &provisioning.Repository{ + Spec: provisioning.RepositorySpec{ + Local: &provisioning.LocalRepositoryConfig{ + Path: tempDir, + }, + }, + }, + resolver: &LocalFolderResolver{ + PermittedPrefixes: []string{tempDir}, + }, + path: tempDir, + } + + return tempDir, repo + }, + path: "test-file.txt", + ref: "main", + data: []byte("content with ref"), + comment: "test write with ref", + expectedErr: apierrors.NewBadRequest("local repository does not support ref"), + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + // Setup test environment + _, repo := tc.setup(t) + + // Execute the write operation + err := repo.Write(context.Background(), tc.path, tc.ref, tc.data, tc.comment) + + // Verify results + if tc.expectedErr != nil { + require.Error(t, err) + assert.Equal(t, tc.expectedErr.Error(), err.Error(), "Error message should match expected") + } else { + require.NoError(t, err) + + // Verify the file or directory was created + targetPath := filepath.Join(repo.path, tc.path) + + // Check if it's a directory + if strings.HasSuffix(tc.path, "/") || tc.data == nil { + info, statErr := os.Stat(targetPath) + require.NoError(t, statErr) + assert.True(t, info.IsDir(), "Path should be a directory") + } else { + // Verify file content + //nolint:gosec + content, readErr := os.ReadFile(targetPath) + require.NoError(t, readErr) + assert.Equal(t, tc.data, content, "File content should match written data") + } + } + }) + } +} + +func TestLocalRepository_Create(t *testing.T) { + testCases := []struct { + name string + setup func(t *testing.T) (string, *localRepository) + path string + ref string + data []byte + comment string + expectedErr error + }{ + { + name: "create new file", + setup: func(t *testing.T) (string, *localRepository) { + tempDir := t.TempDir() + repo := &localRepository{ + config: &provisioning.Repository{ + Spec: provisioning.RepositorySpec{ + Local: &provisioning.LocalRepositoryConfig{ + Path: tempDir, + }, + }, + }, + resolver: &LocalFolderResolver{ + PermittedPrefixes: []string{tempDir}, + }, + path: tempDir, + } + + return tempDir, repo + }, + path: "new-file.txt", + data: []byte("new content"), + comment: "test create new file", + }, + { + name: "create file in nested directory", + setup: func(t *testing.T) (string, *localRepository) { + tempDir := t.TempDir() + repo := &localRepository{ + config: &provisioning.Repository{ + Spec: provisioning.RepositorySpec{ + Local: &provisioning.LocalRepositoryConfig{ + Path: tempDir, + }, + }, + }, + resolver: &LocalFolderResolver{ + PermittedPrefixes: []string{tempDir}, + }, + path: tempDir, + } + + return tempDir, repo + }, + path: "nested/dir/new-file.txt", + data: []byte("nested content"), + comment: "test create file in nested directory", + }, + { + name: "create directory", + setup: func(t *testing.T) (string, *localRepository) { + tempDir := t.TempDir() + repo := &localRepository{ + config: &provisioning.Repository{ + Spec: provisioning.RepositorySpec{ + Local: &provisioning.LocalRepositoryConfig{ + Path: tempDir, + }, + }, + }, + resolver: &LocalFolderResolver{ + PermittedPrefixes: []string{tempDir}, + }, + path: tempDir, + } + + return tempDir, repo + }, + path: "new-dir/", + data: nil, + comment: "test create directory", + }, + { + name: "create file that already exists", + setup: func(t *testing.T) (string, *localRepository) { + tempDir := t.TempDir() + + // Create a file that will conflict + existingFilePath := filepath.Join(tempDir, "existing-file.txt") + require.NoError(t, os.WriteFile(existingFilePath, []byte("original content"), 0600)) + + repo := &localRepository{ + config: &provisioning.Repository{ + Spec: provisioning.RepositorySpec{ + Local: &provisioning.LocalRepositoryConfig{ + Path: tempDir, + }, + }, + }, + resolver: &LocalFolderResolver{ + PermittedPrefixes: []string{tempDir}, + }, + path: tempDir, + } + + return tempDir, repo + }, + path: "existing-file.txt", + data: []byte("new content"), + comment: "test create existing file", + expectedErr: apierrors.NewAlreadyExists(schema.GroupResource{}, "existing-file.txt"), + }, + { + name: "create directory with data", + setup: func(t *testing.T) (string, *localRepository) { + tempDir := t.TempDir() + repo := &localRepository{ + config: &provisioning.Repository{ + Spec: provisioning.RepositorySpec{ + Local: &provisioning.LocalRepositoryConfig{ + Path: tempDir, + }, + }, + }, + resolver: &LocalFolderResolver{ + PermittedPrefixes: []string{tempDir}, + }, + path: tempDir, + } + + return tempDir, repo + }, + path: "invalid-dir/", + data: []byte("directory with data"), + comment: "test create directory with data", + expectedErr: apierrors.NewBadRequest("data cannot be provided for a directory"), + }, + { + name: "create with ref", + setup: func(t *testing.T) (string, *localRepository) { + tempDir := t.TempDir() + repo := &localRepository{ + config: &provisioning.Repository{ + Spec: provisioning.RepositorySpec{ + Local: &provisioning.LocalRepositoryConfig{ + Path: tempDir, + }, + }, + }, + resolver: &LocalFolderResolver{ + PermittedPrefixes: []string{tempDir}, + }, + path: tempDir, + } + + return tempDir, repo + }, + path: "file-with-ref.txt", + ref: "main", + data: []byte("content with ref"), + comment: "test create with ref", + expectedErr: apierrors.NewBadRequest("local repository does not support ref"), + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + // Setup test environment + _, repo := tc.setup(t) + + // Execute the create operation + err := repo.Create(context.Background(), tc.path, tc.ref, tc.data, tc.comment) + + // Verify results + if tc.expectedErr != nil { + require.Error(t, err) + assert.Equal(t, tc.expectedErr.Error(), err.Error(), "Error message should match expected") + } else { + require.NoError(t, err) + + // Verify the file or directory was created + targetPath := filepath.Join(repo.path, tc.path) + + // Check if it's a directory + if strings.HasSuffix(tc.path, "/") || tc.data == nil { + info, statErr := os.Stat(targetPath) + require.NoError(t, statErr) + assert.True(t, info.IsDir(), "Path should be a directory") + } else { + // Verify file content + //nolint:gosec + content, readErr := os.ReadFile(targetPath) + require.NoError(t, readErr) + assert.Equal(t, tc.data, content, "File content should match written data") + } + } + }) + } +} + +func TestLocalRepository_Read(t *testing.T) { + testCases := []struct { + name string + setup func(t *testing.T) (string, *localRepository) + path string + ref string + expectedErr error + expected *FileInfo + }{ + { + name: "read existing file", + setup: func(t *testing.T) (string, *localRepository) { + tempDir := t.TempDir() + + // Create a file to read + filePath := filepath.Join(tempDir, "test-file.txt") + fileContent := []byte("test content") + require.NoError(t, os.WriteFile(filePath, fileContent, 0600)) + + repo := &localRepository{ + config: &provisioning.Repository{ + Spec: provisioning.RepositorySpec{ + Local: &provisioning.LocalRepositoryConfig{ + Path: tempDir, + }, + }, + }, + resolver: &LocalFolderResolver{ + PermittedPrefixes: []string{tempDir}, + }, + path: tempDir, + } + + return tempDir, repo + }, + path: "test-file.txt", + expected: &FileInfo{ + Path: "test-file.txt", + Modified: &metav1.Time{Time: time.Now()}, + Data: []byte("test content"), + Hash: "1eebdf4fdc9fc7bf283031b93f9aef3338de9052", + }, + }, + { + name: "read non-existent file", + setup: func(t *testing.T) (string, *localRepository) { + tempDir := t.TempDir() + repo := &localRepository{ + config: &provisioning.Repository{ + Spec: provisioning.RepositorySpec{ + Local: &provisioning.LocalRepositoryConfig{ + Path: tempDir, + }, + }, + }, + resolver: &LocalFolderResolver{ + PermittedPrefixes: []string{tempDir}, + }, + path: tempDir, + } + + return tempDir, repo + }, + path: "non-existent-file.txt", + expectedErr: ErrFileNotFound, + }, + { + name: "read with ref should fail", + setup: func(t *testing.T) (string, *localRepository) { + tempDir := t.TempDir() + + // Create a file to read + filePath := filepath.Join(tempDir, "test-file.txt") + fileContent := []byte("test content") + require.NoError(t, os.WriteFile(filePath, fileContent, 0600)) + + repo := &localRepository{ + config: &provisioning.Repository{ + Spec: provisioning.RepositorySpec{ + Local: &provisioning.LocalRepositoryConfig{ + Path: tempDir, + }, + }, + }, + resolver: &LocalFolderResolver{ + PermittedPrefixes: []string{tempDir}, + }, + path: tempDir, + } + + return tempDir, repo + }, + path: "test-file.txt", + ref: "main", + expectedErr: apierrors.NewBadRequest("local repository does not support ref"), + }, + { + name: "read existing directory", + setup: func(t *testing.T) (string, *localRepository) { + tempDir := t.TempDir() + + // Create a directory to read + dirPath := filepath.Join(tempDir, "test-dir") + require.NoError(t, os.Mkdir(dirPath, 0750)) + + repo := &localRepository{ + config: &provisioning.Repository{ + Spec: provisioning.RepositorySpec{ + Local: &provisioning.LocalRepositoryConfig{ + Path: tempDir, + }, + }, + }, + resolver: &LocalFolderResolver{ + PermittedPrefixes: []string{tempDir}, + }, + path: tempDir, + } + + return tempDir, repo + }, + path: "test-dir", + expected: &FileInfo{ + Path: "test-dir", + Modified: &metav1.Time{Time: time.Now()}, + }, + expectedErr: nil, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + // Setup test environment + _, repo := tc.setup(t) + + // Execute the read operation + data, err := repo.Read(context.Background(), tc.path, tc.ref) + + // Verify results + if tc.expectedErr != nil { + require.Error(t, err) + assert.Equal(t, tc.expectedErr.Error(), err.Error(), "Error message should match expected") + } else { + require.NoError(t, err) + assert.Equal(t, tc.expected.Path, data.Path, "Path should match expected") + assert.NotNil(t, data.Modified, "Modified time should not be nil") + assert.Equal(t, tc.expected.Data, data.Data, "Data should match expected") + assert.Equal(t, tc.expected.Hash, data.Hash, "Hash should match expected") + assert.Empty(t, data.Ref, "Ref should be empty") + } + }) + } +} + +func TestLocalRepository_ReadTree(t *testing.T) { + testCases := []struct { + name string + setup func(t *testing.T) (string, *localRepository) + ref string + expectedErr error + expected []FileTreeEntry + }{ + { + name: "read empty directory", + setup: func(t *testing.T) (string, *localRepository) { + tempDir := t.TempDir() + repo := &localRepository{ + config: &provisioning.Repository{ + Spec: provisioning.RepositorySpec{ + Local: &provisioning.LocalRepositoryConfig{ + Path: tempDir, + }, + }, + }, + resolver: &LocalFolderResolver{ + PermittedPrefixes: []string{tempDir}, + }, + path: tempDir, + } + + return tempDir, repo + }, + expected: []FileTreeEntry{}, + expectedErr: nil, + }, + { + name: "read directory with files", + setup: func(t *testing.T) (string, *localRepository) { + tempDir := t.TempDir() + + // Create a file structure + require.NoError(t, os.WriteFile(filepath.Join(tempDir, "file1.txt"), []byte("content1"), 0600)) + require.NoError(t, os.WriteFile(filepath.Join(tempDir, "file2.txt"), []byte("content2"), 0600)) + require.NoError(t, os.MkdirAll(filepath.Join(tempDir, "subdir"), 0700)) + require.NoError(t, os.WriteFile(filepath.Join(tempDir, "subdir", "file3.txt"), []byte("content3"), 0600)) + + repo := &localRepository{ + config: &provisioning.Repository{ + Spec: provisioning.RepositorySpec{ + Local: &provisioning.LocalRepositoryConfig{ + Path: tempDir, + }, + }, + }, + resolver: &LocalFolderResolver{ + PermittedPrefixes: []string{tempDir}, + }, + path: tempDir, + } + + return tempDir, repo + }, + expected: []FileTreeEntry{ + {Path: "file1.txt", Blob: true, Size: 8}, + {Path: "file2.txt", Blob: true, Size: 8}, + {Path: "subdir", Blob: false}, + {Path: "subdir/file3.txt", Blob: true, Size: 8}, + }, + expectedErr: nil, + }, + { + name: "read with ref", + setup: func(t *testing.T) (string, *localRepository) { + tempDir := t.TempDir() + repo := &localRepository{ + config: &provisioning.Repository{ + Spec: provisioning.RepositorySpec{ + Local: &provisioning.LocalRepositoryConfig{ + Path: tempDir, + }, + }, + }, + resolver: &LocalFolderResolver{ + PermittedPrefixes: []string{tempDir}, + }, + path: tempDir, + } + + return tempDir, repo + }, + ref: "main", + expectedErr: apierrors.NewBadRequest("local repository does not support ref"), + }, + { + name: "read non-existent directory", + setup: func(t *testing.T) (string, *localRepository) { + tempDir := t.TempDir() + nonExistentDir := filepath.Join(tempDir, "non-existent") + + repo := &localRepository{ + config: &provisioning.Repository{ + Spec: provisioning.RepositorySpec{ + Local: &provisioning.LocalRepositoryConfig{ + Path: nonExistentDir, + }, + }, + }, + resolver: &LocalFolderResolver{ + PermittedPrefixes: []string{tempDir}, + }, + path: nonExistentDir, + } + + return tempDir, repo + }, + expected: []FileTreeEntry{}, + expectedErr: nil, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + // Setup test environment + _, repo := tc.setup(t) + + // Execute the readTree operation + entries, err := repo.ReadTree(context.Background(), tc.ref) + + // Verify results + if tc.expectedErr != nil { + require.Error(t, err) + assert.Equal(t, tc.expectedErr.Error(), err.Error(), "Error message should match expected") + } else { + require.NoError(t, err) + + if len(tc.expected) == 0 { + assert.Empty(t, entries, "Expected empty entries") + } else { + // Sort both expected and actual entries by path for comparison + sort.Slice(entries, func(i, j int) bool { + return entries[i].Path < entries[j].Path + }) + + // We need to verify each entry individually since hash values will be different + assert.Equal(t, len(tc.expected), len(entries), "Number of entries should match") + + for i, expected := range tc.expected { + if i < len(entries) { + assert.Equal(t, expected.Path, entries[i].Path, "Path should match") + assert.Equal(t, expected.Blob, entries[i].Blob, "Blob flag should match") + + if expected.Blob { + assert.Equal(t, expected.Size, entries[i].Size, "Size should match") + assert.NotEmpty(t, entries[i].Hash, "Hash should not be empty for files") + } + } + } + } + } + }) + } +} + +func TestLocalRepository_Config(t *testing.T) { + testCases := []struct { + name string + config *provisioning.Repository + }{ + { + name: "returns the same config that was provided", + config: &provisioning.Repository{ + Spec: provisioning.RepositorySpec{ + Local: &provisioning.LocalRepositoryConfig{ + Path: "/some/path", + }, + }, + }, + }, + { + name: "returns nil config", + config: nil, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + // Create repository with the test config + repo := &localRepository{ + config: tc.config, + } + + // Call the Config method + result := repo.Config() + + // Verify the result is the same as the input config + assert.Equal(t, tc.config, result, "Config() should return the same config that was provided") + }) + } +} diff --git a/pkg/registry/apis/provisioning/repository/repository.go b/pkg/registry/apis/provisioning/repository/repository.go index a02ae539b08..05c8e74ee6e 100644 --- a/pkg/registry/apis/provisioning/repository/repository.go +++ b/pkg/registry/apis/provisioning/repository/repository.go @@ -2,8 +2,6 @@ package repository import ( "context" - "errors" - "fmt" "io" "net/http" "time" @@ -201,14 +199,3 @@ type Versioned interface { LatestRef(ctx context.Context) (string, error) CompareFiles(ctx context.Context, base, ref string) ([]VersionedFileChange, error) } - -func writeWithReadThenCreateOrUpdate(ctx context.Context, r ReaderWriter, path, ref string, data []byte, comment string) error { - _, err := r.Read(ctx, path, ref) - if err != nil && !(errors.Is(err, ErrFileNotFound)) { - return fmt.Errorf("failed to check if file exists before writing: %w", err) - } - if err == nil { - return r.Update(ctx, path, ref, data, comment) - } - return r.Create(ctx, path, ref, data, comment) -} diff --git a/pkg/registry/apis/provisioning/repository/test_test.go b/pkg/registry/apis/provisioning/repository/test_test.go index 1dd09596617..38b88b388d2 100644 --- a/pkg/registry/apis/provisioning/repository/test_test.go +++ b/pkg/registry/apis/provisioning/repository/test_test.go @@ -175,6 +175,44 @@ func TestValidateRepository(t *testing.T) { // 3. sync interval too low // 4. reserved name }, + { + name: "branch workflow for non-github repository", + repository: func() *MockRepository { + m := NewMockRepository(t) + m.On("Config").Return(&provisioning.Repository{ + Spec: provisioning.RepositorySpec{ + Title: "Test Repo", + Type: provisioning.LocalRepositoryType, + Workflows: []provisioning.Workflow{provisioning.BranchWorkflow}, + }, + }) + m.On("Validate").Return(field.ErrorList{}) + return m + }(), + expectedErrs: 1, + validateError: func(t *testing.T, errors field.ErrorList) { + require.Contains(t, errors.ToAggregate().Error(), "spec.workflow: Invalid value: \"branch\": branch is only supported on git repositories") + }, + }, + { + name: "invalid workflow in the list", + repository: func() *MockRepository { + m := NewMockRepository(t) + m.On("Config").Return(&provisioning.Repository{ + Spec: provisioning.RepositorySpec{ + Title: "Test Repo", + Type: provisioning.GitHubRepositoryType, + Workflows: []provisioning.Workflow{provisioning.WriteWorkflow, "invalid"}, + }, + }) + m.On("Validate").Return(field.ErrorList{}) + return m + }(), + expectedErrs: 1, + validateError: func(t *testing.T, errors field.ErrorList) { + require.Contains(t, errors.ToAggregate().Error(), "spec.workflow: Invalid value: \"invalid\": invalid workflow") + }, + }, } for _, tt := range tests { @@ -324,3 +362,67 @@ func TestTester_TestRepository(t *testing.T) { require.Equal(t, http.StatusOK, results.Code) require.True(t, results.Success) } + +func TestFromFieldError(t *testing.T) { + tests := []struct { + name string + fieldError *field.Error + expectedCode int + expectedField string + expectedType metav1.CauseType + expectedDetail string + }{ + { + name: "required field error", + fieldError: &field.Error{ + Type: field.ErrorTypeRequired, + Field: "spec.title", + Detail: "a repository title must be given", + }, + expectedCode: http.StatusBadRequest, + expectedField: "spec.title", + expectedType: metav1.CauseTypeFieldValueRequired, + expectedDetail: "a repository title must be given", + }, + { + name: "invalid field error", + fieldError: &field.Error{ + Type: field.ErrorTypeInvalid, + Field: "spec.sync.intervalSeconds", + Detail: "Interval must be at least 10 seconds", + }, + expectedCode: http.StatusBadRequest, + expectedField: "spec.sync.intervalSeconds", + expectedType: metav1.CauseTypeFieldValueInvalid, + expectedDetail: "Interval must be at least 10 seconds", + }, + { + name: "not supported field error", + fieldError: &field.Error{ + Type: field.ErrorTypeNotSupported, + Field: "spec.workflow", + Detail: "branch is only supported on git repositories", + }, + expectedCode: http.StatusBadRequest, + expectedField: "spec.workflow", + expectedType: metav1.CauseTypeFieldValueNotSupported, + expectedDetail: "branch is only supported on git repositories", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := fromFieldError(tt.fieldError) + + require.NotNil(t, result) + require.Equal(t, tt.expectedCode, result.Code) + require.False(t, result.Success) + require.Len(t, result.Errors, 1) + + errorDetail := result.Errors[0] + require.Equal(t, tt.expectedField, errorDetail.Field) + require.Equal(t, tt.expectedType, errorDetail.Type) + require.Equal(t, tt.expectedDetail, errorDetail.Detail) + }) + } +}