From 1ff39510d3d17b35e9cbc8453d01221017e1628b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roberto=20Jim=C3=A9nez=20S=C3=A1nchez?= Date: Fri, 15 Aug 2025 09:05:53 +0200 Subject: [PATCH] Provisioning: Check For Resource Ownership Before Operations (#109582) --- pkg/registry/apis/provisioning/files.go | 10 +- .../apis/provisioning/resources/dualwriter.go | 31 ++- .../apis/provisioning/resources/parser.go | 90 ++++++++- .../provisioning/resources/repository_test.go | 130 ++++++++++++ .../apis/provisioning/resources/resources.go | 52 +++++ pkg/registry/apis/provisioning/response.go | 18 ++ pkg/tests/apis/provisioning/files_test.go | 188 ++++++++++++++++++ pkg/tests/apis/provisioning/helper_test.go | 13 +- pkg/tests/apis/provisioning/pulljob_test.go | 141 +++++++++++++ 9 files changed, 647 insertions(+), 26 deletions(-) create mode 100644 pkg/registry/apis/provisioning/response.go create mode 100644 pkg/tests/apis/provisioning/pulljob_test.go diff --git a/pkg/registry/apis/provisioning/files.go b/pkg/registry/apis/provisioning/files.go index 1df860b9738..8fe1a933e37 100644 --- a/pkg/registry/apis/provisioning/files.go +++ b/pkg/registry/apis/provisioning/files.go @@ -135,7 +135,7 @@ func (c *filesConnector) Connect(ctx context.Context, name string, opts runtime. case http.MethodGet: resource, err := dualReadWriter.Read(ctx, opts.Path, opts.Ref) if err != nil { - responder.Error(err) + respondWithError(responder, err) return } obj = resource.AsResourceWrapper() @@ -153,7 +153,7 @@ func (c *filesConnector) Connect(ctx context.Context, name string, opts runtime. resource, err := dualReadWriter.MoveResource(ctx, opts) if err != nil { - responder.Error(err) + respondWithError(responder, err) return } obj = resource.AsResourceWrapper() @@ -169,7 +169,7 @@ func (c *filesConnector) Connect(ctx context.Context, name string, opts runtime. var resource *resources.ParsedResource resource, err = dualReadWriter.CreateResource(ctx, opts) if err != nil { - responder.Error(err) + respondWithError(responder, err) return } obj = resource.AsResourceWrapper() @@ -187,7 +187,7 @@ func (c *filesConnector) Connect(ctx context.Context, name string, opts runtime. resource, err := dualReadWriter.UpdateResource(ctx, opts) if err != nil { - responder.Error(err) + respondWithError(responder, err) return } obj = resource.AsResourceWrapper() @@ -195,7 +195,7 @@ func (c *filesConnector) Connect(ctx context.Context, name string, opts runtime. case http.MethodDelete: resource, err := dualReadWriter.Delete(ctx, opts) if err != nil { - responder.Error(err) + respondWithError(responder, err) return } obj = resource.AsResourceWrapper() diff --git a/pkg/registry/apis/provisioning/resources/dualwriter.go b/pkg/registry/apis/provisioning/resources/dualwriter.go index bfe4ca499a6..24e605847e2 100644 --- a/pkg/registry/apis/provisioning/resources/dualwriter.go +++ b/pkg/registry/apis/provisioning/resources/dualwriter.go @@ -62,7 +62,7 @@ func (r *DualReadWriter) Read(ctx context.Context, path string, ref string) (*Pa // Fail as we use the dry run for this response and it's not about updating the resource if err := parsed.DryRun(ctx); err != nil { - return nil, apierrors.NewBadRequest(fmt.Sprintf("Dry run failed: %v", err)) + return nil, fmt.Errorf("error running dryRun: %w", err) } // Authorize based on the existing resource @@ -105,25 +105,22 @@ func (r *DualReadWriter) Delete(ctx context.Context, opts DualWriteOptions) (*Pa } parsed.Action = provisioning.ResourceActionDelete + + // Use the parser's DryRun method like create/update operations + if !opts.SkipDryRun { + if err := parsed.DryRun(ctx); err != nil { + return nil, fmt.Errorf("error running dryRun for delete: %w", err) + } + } + err = r.repo.Delete(ctx, opts.Path, opts.Ref, opts.Message) if err != nil { return nil, fmt.Errorf("delete file from repository: %w", err) } - // Delete the file in the grafana database + // Delete the file in the grafana database using the parser's Run method if opts.Ref == "" { - ctx, _, err := identity.WithProvisioningIdentity(ctx, parsed.Obj.GetNamespace()) - if err != nil { - return parsed, err - } - - // FIXME: empty folders with no repository files will remain in the system - // until the next reconciliation. - err = parsed.Client.Delete(ctx, parsed.Obj.GetName(), metav1.DeleteOptions{}) - if apierrors.IsNotFound(err) { - err = nil // ignorable - } - + err = parsed.Run(ctx) if err != nil { return nil, fmt.Errorf("delete resource from storage: %w", err) } @@ -224,13 +221,12 @@ func (r *DualReadWriter) createOrUpdate(ctx context.Context, create bool, opts D logger := logging.FromContext(ctx).With("path", opts.Path, "name", parsed.Obj.GetName(), "ref", opts.Ref) logger.Warn("failed to dry run resource on create", "error", err) - // TODO: return this as a 400 rather than 500 - return nil, fmt.Errorf("error running dryRun %w", err) + return nil, fmt.Errorf("error running dryRun: %w", err) } } if len(parsed.Errors) > 0 { - // TODO: return this as a 400 rather than 500 + // Now returns BadRequest (400) for validation errors return nil, fmt.Errorf("errors while parsing file [%v]", parsed.Errors) } @@ -449,6 +445,7 @@ func (r *DualReadWriter) moveFile(ctx context.Context, opts DualWriteOptions) (* // Perform the move operation in the repository // If we have new content, we need to update the file content as part of the move if len(opts.Data) > 0 { + // FIXME: I think we should MOVE + UPDATE instead of Delete / Create // For moves with content updates, we need to delete the old file and create the new one if err = r.repo.Delete(ctx, opts.OriginalPath, opts.Ref, opts.Message); err != nil { return nil, fmt.Errorf("delete original file in repository: %w", err) diff --git a/pkg/registry/apis/provisioning/resources/parser.go b/pkg/registry/apis/provisioning/resources/parser.go index 8c6c3b94587..35b94007390 100644 --- a/pkg/registry/apis/provisioning/resources/parser.go +++ b/pkg/registry/apis/provisioning/resources/parser.go @@ -168,8 +168,8 @@ func (r *parser) Parse(ctx context.Context, info *repository.FileInfo) (parsed * if obj.GetNamespace() != "" && obj.GetNamespace() != r.repo.Namespace { return nil, apierrors.NewBadRequest("the file namespace does not match target namespace") } - obj.SetNamespace(r.repo.Namespace) + parsed.Meta.SetManagerProperties(utils.ManagerProperties{ Kind: utils.ManagerKindRepo, Identity: r.repo.Name, @@ -232,9 +232,48 @@ func (f *ParsedResource) DryRun(ctx context.Context) error { fieldValidation = "Ignore" // FIXME: temporary while we improve validation } + // Handle deletion action separately + if f.Action == provisioning.ResourceActionDelete { + // For delete, we need the existing resource to validate deletion + f.Existing, err = f.Client.Get(ctx, f.Obj.GetName(), metav1.GetOptions{}) + if err != nil { + if apierrors.IsNotFound(err) { + // Resource doesn't exist, nothing to delete - this is fine for dry run + return nil + } + return fmt.Errorf("failed to get existing resource for delete dry run: %w", err) + } + + // Check for ownership conflicts + requestingManager := utils.ManagerProperties{ + Kind: utils.ManagerKindRepo, + Identity: f.Repo.Name, + } + if err := CheckResourceOwnership(f.Existing, f.Obj.GetName(), requestingManager); err != nil { + return err + } + + // For delete dry run, we simulate the delete operation + // The dry run response will be the existing resource that would be deleted + f.DryRunResponse = f.Existing.DeepCopy() + return nil + } + // FIXME: shouldn't we check for the specific error? // Dry run CREATE or UPDATE f.Existing, _ = f.Client.Get(ctx, f.Obj.GetName(), metav1.GetOptions{}) + + // Check for ownership conflicts after fetching existing resource + requestingManager := utils.ManagerProperties{ + Kind: utils.ManagerKindRepo, + Identity: f.Repo.Name, + } + + // Check for ownership conflicts after fetching existing resource + if err := CheckResourceOwnership(f.Existing, f.Obj.GetName(), requestingManager); err != nil { + return err + } + if f.Existing == nil { f.Action = provisioning.ResourceActionCreate f.DryRunResponse, err = f.Client.Create(ctx, f.Obj, metav1.CreateOptions{ @@ -268,6 +307,55 @@ func (f *ParsedResource) Run(ctx context.Context) error { fieldValidation = "Ignore" // FIXME: temporary while we improve validation } + // Check for ownership conflicts + requestingManager := utils.ManagerProperties{ + Kind: utils.ManagerKindRepo, + Identity: f.Repo.Name, + } + + // Handle deletion action + if f.Action == provisioning.ResourceActionDelete { + // If we don't have existing resource from DryRun, fetch it now + if f.DryRunResponse == nil { + f.Existing, err = f.Client.Get(ctx, f.Obj.GetName(), metav1.GetOptions{}) + if err != nil { + if apierrors.IsNotFound(err) { + // Resource doesn't exist, nothing to delete - this is fine + return nil + } + return fmt.Errorf("failed to get existing resource for delete: %w", err) + } + } + + // Check ownership with the existing resource + if err := CheckResourceOwnership(f.Existing, f.Obj.GetName(), requestingManager); err != nil { + return err + } + + // Perform the actual delete + err = f.Client.Delete(ctx, f.Obj.GetName(), metav1.DeleteOptions{}) + if apierrors.IsNotFound(err) { + err = nil // ignorable - resource was already deleted + } + + // Set the deleted resource as the result + if err == nil && f.Existing != nil { + f.Upsert = f.Existing.DeepCopy() + } + + return err + } + + // If we don't have existing resource from DryRun, fetch it now + if f.DryRunResponse == nil { + f.Existing, _ = f.Client.Get(ctx, f.Obj.GetName(), metav1.GetOptions{}) + } + + // Check ownership with the existing resource (if any) + if err := CheckResourceOwnership(f.Existing, f.Obj.GetName(), requestingManager); err != nil { + return err + } + // If we have already tried loading existing, start with create if f.DryRunResponse != nil && f.Existing == nil { f.Action = provisioning.ResourceActionCreate diff --git a/pkg/registry/apis/provisioning/resources/repository_test.go b/pkg/registry/apis/provisioning/resources/repository_test.go index c53512c3c0a..0c1c97f564f 100644 --- a/pkg/registry/apis/provisioning/resources/repository_test.go +++ b/pkg/registry/apis/provisioning/resources/repository_test.go @@ -5,6 +5,7 @@ import ( "errors" "testing" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -435,3 +436,132 @@ func (m *MockDynamicResourceInterface) ApplyStatus(ctx context.Context, name str // Ensure MockDynamicResourceInterface implements dynamic.ResourceInterface var _ dynamic.ResourceInterface = (*MockDynamicResourceInterface)(nil) + +func TestCheckResourceOwnership(t *testing.T) { + tests := []struct { + name string + existingResource *unstructured.Unstructured + requestingManager utils.ManagerProperties + expectError bool + expectedMessage string + }{ + { + name: "no existing resource - allow operation", + existingResource: nil, // Explicitly nil to represent non-existing resource + requestingManager: utils.ManagerProperties{ + Kind: utils.ManagerKindRepo, + Identity: "repo-1", + }, + expectError: false, + }, + { + name: "existing resource with no manager - allow operation", + existingResource: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "metadata": map[string]interface{}{ + "name": "test-resource", + }, + }, + }, + requestingManager: utils.ManagerProperties{ + Kind: utils.ManagerKindRepo, + Identity: "repo-1", + }, + expectError: false, + }, + { + name: "same manager - allow operation", + existingResource: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "metadata": map[string]interface{}{ + "name": "test-resource", + "annotations": map[string]interface{}{ + utils.AnnoKeyManagerKind: "repo", + utils.AnnoKeyManagerIdentity: "repo-1", + }, + }, + }, + }, + requestingManager: utils.ManagerProperties{ + Kind: utils.ManagerKindRepo, + Identity: "repo-1", + }, + expectError: false, + }, + { + name: "different manager but allows edits - allow operation", + existingResource: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "metadata": map[string]interface{}{ + "name": "test-resource", + "annotations": map[string]interface{}{ + utils.AnnoKeyManagerKind: "repo", + utils.AnnoKeyManagerIdentity: "repo-1", + utils.AnnoKeyManagerAllowsEdits: "true", + }, + }, + }, + }, + requestingManager: utils.ManagerProperties{ + Kind: utils.ManagerKindRepo, + Identity: "repo-2", + }, + expectError: false, + }, + { + name: "different manager and doesn't allow edits - deny operation", + existingResource: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "metadata": map[string]interface{}{ + "name": "test-resource", + "annotations": map[string]interface{}{ + utils.AnnoKeyManagerKind: "repo", + utils.AnnoKeyManagerIdentity: "repo-1", + }, + }, + }, + }, + requestingManager: utils.ManagerProperties{ + Kind: utils.ManagerKindRepo, + Identity: "repo-2", + }, + expectError: true, + expectedMessage: "resource 'test-resource' is managed by repo 'repo-1' and cannot be modified by repo 'repo-2'", + }, + { + name: "different manager types - deny operation", + existingResource: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "metadata": map[string]interface{}{ + "name": "test-resource", + "annotations": map[string]interface{}{ + utils.AnnoKeyManagerKind: "terraform", + utils.AnnoKeyManagerIdentity: "tf-stack-1", + }, + }, + }, + }, + requestingManager: utils.ManagerProperties{ + Kind: utils.ManagerKindRepo, + Identity: "repo-1", + }, + expectError: true, + expectedMessage: "resource 'test-resource' is managed by terraform 'tf-stack-1' and cannot be modified by repo 'repo-1'", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Test the package-level ownership check function directly + err := CheckResourceOwnership(tt.existingResource, "test-resource", tt.requestingManager) + + if tt.expectError { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.expectedMessage) + assert.True(t, apierrors.IsBadRequest(err)) + } else { + require.NoError(t, err) + } + }) + } +} diff --git a/pkg/registry/apis/provisioning/resources/resources.go b/pkg/registry/apis/provisioning/resources/resources.go index ec5942c21ec..cbd5cb120c6 100644 --- a/pkg/registry/apis/provisioning/resources/resources.go +++ b/pkg/registry/apis/provisioning/resources/resources.go @@ -25,6 +25,19 @@ var ( ErrMissingName = field.Required(field.NewPath("name", "metadata", "name"), "missing name in resource") ) +// NewResourceOwnershipConflictError creates a BadRequest error for when a resource +// is owned by a different repository or manager and cannot be modified +func NewResourceOwnershipConflictError(resourceName string, currentManager utils.ManagerProperties, requestingManager utils.ManagerProperties) error { + message := fmt.Sprintf("resource '%s' is managed by %s '%s' and cannot be modified by %s '%s'", + resourceName, + currentManager.Kind, + currentManager.Identity, + requestingManager.Kind, + requestingManager.Identity) + + return apierrors.NewBadRequest(message) +} + type WriteOptions struct { Path string Ref string @@ -54,6 +67,45 @@ func NewResourcesManager(repo repository.ReaderWriter, folders *FolderManager, p } } +// CheckResourceOwnership validates that the requesting manager can modify the existing resource +// Returns an error if the existing resource is owned by a different manager that doesn't allow edits +// If existingResource is nil, no ownership conflict exists (new resource) +// This is a package-level function that can be used without a ResourcesManager instance +func CheckResourceOwnership(existingResource *unstructured.Unstructured, resourceName string, requestingManager utils.ManagerProperties) error { + if existingResource == nil { + // Resource doesn't exist, so no ownership conflict + return nil + } + + // Check if the existing resource has manager properties + existingMeta, err := utils.MetaAccessor(existingResource) + if err != nil { + // If we can't get metadata, allow the operation + return nil + } + + currentManager, hasManager := existingMeta.GetManagerProperties() + if !hasManager { + // No manager information, so no ownership conflict + return nil + } + + // Check if this is the same manager + if currentManager.Kind == requestingManager.Kind && currentManager.Identity == requestingManager.Identity { + // Same manager, no conflict + return nil + } + + // Check if the current manager allows edits + if currentManager.AllowsEdits { + // Manager allows edits from others, no conflict + return nil + } + + // Different manager and edits not allowed - return ownership conflict error + return NewResourceOwnershipConflictError(resourceName, currentManager, requestingManager) +} + // CreateResource writes an object to the repository func (r *ResourcesManager) WriteResourceFileFromObject(ctx context.Context, obj *unstructured.Unstructured, options WriteOptions) (string, error) { if err := ctx.Err(); err != nil { diff --git a/pkg/registry/apis/provisioning/response.go b/pkg/registry/apis/provisioning/response.go new file mode 100644 index 00000000000..20fab310c68 --- /dev/null +++ b/pkg/registry/apis/provisioning/response.go @@ -0,0 +1,18 @@ +package provisioning + +import ( + "errors" + + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apiserver/pkg/registry/rest" +) + +// respondWithError checks if the provided error contains an API error and unwraps it before passing it to the responder. +func respondWithError(responder rest.Responder, err error) { + var statusErr *apierrors.StatusError + if errors.As(err, &statusErr) { + responder.Error(statusErr) + } else { + responder.Error(err) + } +} diff --git a/pkg/tests/apis/provisioning/files_test.go b/pkg/tests/apis/provisioning/files_test.go index 0867bce6c9a..afe7bfd62b9 100644 --- a/pkg/tests/apis/provisioning/files_test.go +++ b/pkg/tests/apis/provisioning/files_test.go @@ -2,9 +2,11 @@ package provisioning import ( "context" + "errors" "fmt" "io" "net/http" + "path" "testing" "github.com/grafana/grafana/pkg/apimachinery/utils" @@ -390,3 +392,189 @@ func TestIntegrationProvisioning_MoveResources(t *testing.T) { }) }) } + +func TestIntegrationProvisioning_FilesOwnershipProtection(t *testing.T) { + if testing.Short() { + t.Skip("skipping integration test") + } + + helper := runGrafana(t) + ctx := context.Background() + + // Create first repository targeting "folder-1" with its own subdirectory + const repo1 = "ownership-repo-1" + helper.CreateRepo(t, TestRepo{ + Name: repo1, + Path: path.Join(helper.ProvisioningPath, "repo1"), + Target: "folder", + Copies: map[string]string{ + "testdata/all-panels.json": "dashboard1.json", + }, + ExpectedDashboards: 1, + ExpectedFolders: 1, + }) + + // Create second repository targeting "folder-2" with its own subdirectory + const repo2 = "ownership-repo-2" + path2 := path.Join(helper.ProvisioningPath, "repo2") + helper.CreateRepo(t, TestRepo{ + Name: repo2, + Path: path2, + Target: "folder", + Copies: map[string]string{ + "testdata/timeline-demo.json": "dashboard2.json", + }, + ExpectedDashboards: 2, // Total across both repos + ExpectedFolders: 2, // Total across both repos + }) + + t.Run("CREATE file with UID already owned by different repository - should fail", func(t *testing.T) { + // Try to create a dashboard in repo2 that has the same UID as the one in repo1 + // The all-panels.json has UID "n1jR8vnnz" which is already owned by repo1 + result := helper.AdminREST.Post(). + Namespace("default"). + Resource("repositories"). + Name(repo2). // Using repo2 to try to create resource with same UID as repo1 + SubResource("files", "conflicting-dashboard.json"). + Body(helper.LoadFile("testdata/all-panels.json")). // Same file = same UID + SetHeader("Content-Type", "application/json"). + Do(ctx) + + // This should fail with ownership conflict + require.Error(t, result.Error(), "creating resource with UID already owned by different repository should fail") + + // Get detailed error information + err := result.Error() + t.Logf("CREATE operation error: %T - %v", err, err) + if statusErr := apierrors.APIStatus(nil); errors.As(err, &statusErr) { + t.Logf("Status error details: code=%d, reason=%s, message=%s", + statusErr.Status().Code, statusErr.Status().Reason, statusErr.Status().Message) + } + + // Verify it returns BadRequest (400) for ownership conflicts + if !apierrors.IsBadRequest(err) { + t.Errorf("Expected BadRequest error but got: %T - %v", err, err) + return + } + + // Check error message contains ownership conflict information + errorMsg := err.Error() + t.Logf("Error message: %s", errorMsg) + require.Contains(t, errorMsg, fmt.Sprintf("managed by repo '%s'", repo1)) + require.Contains(t, errorMsg, fmt.Sprintf("cannot be modified by repo '%s'", repo2)) + }) + + t.Run("UPDATE with UID already owned by different repository - should fail", func(t *testing.T) { + // Try to update the dashboard owned by repo1 using repo2 + result := helper.AdminREST.Put(). + Namespace("default"). + Resource("repositories"). + Name(repo2). // Using repo2 to try to update repo1's resource + SubResource("files", "conflicting-update.json"). + Body(helper.LoadFile("testdata/all-panels.json")). // Same UID as repo1's dashboard + SetHeader("Content-Type", "application/json"). + Do(ctx) + + // This should fail with ownership conflict + require.Error(t, result.Error(), "updating resource owned by different repository should fail") + + // Get detailed error information + err := result.Error() + t.Logf("UPDATE operation error: %T - %v", err, err) + if statusErr := apierrors.APIStatus(nil); errors.As(err, &statusErr) { + t.Logf("Status error details: code=%d, reason=%s, message=%s", + statusErr.Status().Code, statusErr.Status().Reason, statusErr.Status().Message) + } + + // Verify it returns BadRequest (400) for ownership conflicts + if !apierrors.IsBadRequest(err) { + t.Errorf("Expected BadRequest error but got: %T - %v", err, err) + return + } + + // Check error message contains ownership conflict information + errorMsg := err.Error() + t.Logf("Error message: %s", errorMsg) + require.Contains(t, errorMsg, fmt.Sprintf("managed by repo '%s'", repo1)) + require.Contains(t, errorMsg, fmt.Sprintf("cannot be modified by repo '%s'", repo2)) + }) + + t.Run("DELETE resource owned by different repository - should fail", func(t *testing.T) { + // Create a file manually in the second repo which is already in first one + helper.CopyToProvisioningPath(t, "testdata/all-panels.json", "repo2/conflicting-delete.json") + printFileTree(t, helper.ProvisioningPath) + + result := helper.AdminREST.Delete(). + Namespace("default"). + Resource("repositories"). + Name(repo2). + SubResource("files", "conflicting-delete.json"). + SetHeader("Content-Type", "application/json"). + Do(ctx) + + // This should fail with ownership conflict + require.Error(t, result.Error(), "deleting resource owned by different repository should fail") + + // Get detailed error information + err := result.Error() + t.Logf("DELETE operation error: %T - %v", err, err) + if statusErr := apierrors.APIStatus(nil); errors.As(err, &statusErr) { + t.Logf("Status error details: code=%d, reason=%s, message=%s", + statusErr.Status().Code, statusErr.Status().Reason, statusErr.Status().Message) + } + + // Verify it returns BadRequest (400) for ownership conflicts + if !apierrors.IsBadRequest(err) { + t.Errorf("Expected BadRequest error but got: %T - %v", err, err) + return + } + + // Check error message contains ownership conflict information + errorMsg := err.Error() + t.Logf("Error message: %s", errorMsg) + require.Contains(t, errorMsg, fmt.Sprintf("managed by repo '%s'", repo1)) + require.Contains(t, errorMsg, fmt.Sprintf("cannot be modified by repo '%s'", repo2)) + }) + + t.Run("MOVE and UPDATE file with UID already owned by different repository - should fail", func(t *testing.T) { + resp := helper.postFilesRequest(t, repo2, filesPostOptions{ + targetPath: "moved-dashboard.json", + originalPath: path.Join("dashboard2.json"), + message: "attempt to move file from different repository", + body: string(helper.LoadFile("testdata/all-panels.json")), // Content to move with the conflicting UID + }) + // nolint:errcheck + defer resp.Body.Close() + + // This should fail with ownership conflict + require.NotEqual(t, http.StatusOK, resp.StatusCode, "moving resource owned by different repository should fail") + // Read response body to check error message + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + errorMsg := string(body) + + // Log detailed error information + t.Logf("MOVE operation HTTP status: %d", resp.StatusCode) + t.Logf("MOVE operation error response: %s", errorMsg) + + require.Equal(t, http.StatusBadRequest, resp.StatusCode, "should return BadRequest (400) for ownership conflict") + // Check error message contains ownership conflict information + require.Contains(t, errorMsg, fmt.Sprintf("managed by repo '%s'", repo1)) + require.Contains(t, errorMsg, fmt.Sprintf("cannot be modified by repo '%s'", repo2)) + }) + + t.Run("verify original resources remain intact", func(t *testing.T) { + const allPanelsUID = "n1jR8vnnz" // UID from all-panels.json (repo1) + const timelineUID = "mIJjFy8Kz" // UID from timeline-demo.json (repo2) + + // Verify repo1's dashboard is still owned by repo1 + dashboard1, err := helper.DashboardsV1.Resource.Get(ctx, allPanelsUID, metav1.GetOptions{}) + require.NoError(t, err, "repo1's dashboard should still exist") + require.Equal(t, repo1, dashboard1.GetAnnotations()[utils.AnnoKeyManagerIdentity], "repo1's dashboard should still be owned by repo1") + + // Verify repo2's dashboard is still owned by repo2 + dashboard2, err := helper.DashboardsV1.Resource.Get(ctx, timelineUID, metav1.GetOptions{}) + require.NoError(t, err, "repo2's dashboard should still exist") + require.Equal(t, repo2, dashboard2.GetAnnotations()[utils.AnnoKeyManagerIdentity], "repo2's dashboard should still be owned by repo2") + }) +} diff --git a/pkg/tests/apis/provisioning/helper_test.go b/pkg/tests/apis/provisioning/helper_test.go index 894f7bd8174..554ad36bff8 100644 --- a/pkg/tests/apis/provisioning/helper_test.go +++ b/pkg/tests/apis/provisioning/helper_test.go @@ -315,6 +315,7 @@ func (h *provisioningTestHelper) RenderObject(t *testing.T, filePath string, val // The from path is relative to test file's directory. func (h *provisioningTestHelper) CopyToProvisioningPath(t *testing.T, from, to string) { fullPath := path.Join(h.ProvisioningPath, to) + t.Logf("Copying file from '%s' to provisioning path '%s'", from, fullPath) err := os.MkdirAll(path.Dir(fullPath), 0750) require.NoError(t, err, "failed to create directories for provisioning path") @@ -439,15 +440,21 @@ func (h *provisioningTestHelper) logRepositoryObject(t *testing.T, obj map[strin t.Logf("%s%d items:", prefix, len(v)) for i, item := range v { if itemMap, ok := item.(map[string]interface{}); ok { - t.Logf("%s├── item %d:", prefix, i+1) + // Try to get the actual file path from the item + if pathVal, exists := itemMap["path"]; exists { + t.Logf("%s├── %v", prefix, pathVal) + } else { + t.Logf("%s├── item %d:", prefix, i+1) + } h.logRepositoryObject(t, itemMap, prefix+" ", newPath) } } } default: // This could be file content or metadata - if key != "kind" && key != "apiVersion" { - t.Logf("%s├── %s", prefix, key) + // Skip common metadata fields that are not useful for debugging + if key != "kind" && key != "apiVersion" && key != "path" && key != "size" && key != "hash" { + t.Logf("%s├── %s: %v", prefix, key, value) } } } diff --git a/pkg/tests/apis/provisioning/pulljob_test.go b/pkg/tests/apis/provisioning/pulljob_test.go new file mode 100644 index 00000000000..2af5ca64654 --- /dev/null +++ b/pkg/tests/apis/provisioning/pulljob_test.go @@ -0,0 +1,141 @@ +package provisioning + +import ( + "context" + "fmt" + "os" + "path" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + + provisioning "github.com/grafana/grafana/apps/provisioning/pkg/apis/provisioning/v0alpha1" + "github.com/grafana/grafana/pkg/apimachinery/utils" +) + +func TestIntegrationProvisioning_PullJobOwnershipProtection(t *testing.T) { + if testing.Short() { + t.Skip("skipping integration test") + } + + helper := runGrafana(t) + ctx := context.Background() + + // Create two repositories with folder targets and separate paths to avoid file conflicts + const repo1 = "pulljob-repo-1" + const repo2 = "pulljob-repo-2" + + // Create first repository targeting "folder" with its own subdirectory + helper.CreateRepo(t, TestRepo{ + Name: repo1, + Path: path.Join(helper.ProvisioningPath, "repo1"), + Target: "folder", + Copies: map[string]string{ + "testdata/all-panels.json": "dashboard1.json", + }, + ExpectedDashboards: 1, + ExpectedFolders: 1, + }) + + // Create second repository targeting "folder" with its own subdirectory + helper.CreateRepo(t, TestRepo{ + Name: repo2, + Path: path.Join(helper.ProvisioningPath, "repo2"), + Target: "folder", + Copies: map[string]string{ + "testdata/timeline-demo.json": "dashboard2.json", + }, + ExpectedDashboards: 2, // Total across both repos + ExpectedFolders: 2, // Total across both repos + }) + + // Test: Pull job should fail when trying to manage resources owned by another repository + t.Run("pull job should fail when trying to manage resources owned by another repository", func(t *testing.T) { + // Step 1: Try to add a file with the same UID as repo1's dashboard to repo2's directory + // This simulates a scenario where repo2 tries to manage a resource that repo1 already owns + const allPanelsUID = "n1jR8vnnz" // UID from all-panels.json (owned by repo1) + + // Copy the same file (same UID) to repo2's directory to create ownership conflict + conflictingFilePath := "repo2/conflicting-dashboard.json" + helper.CopyToProvisioningPath(t, "testdata/all-panels.json", conflictingFilePath) + printFileTree(t, helper.ProvisioningPath) + + // Step 2: Try to pull repo2 - should fail due to ownership conflict + job := helper.TriggerJobAndWaitForComplete(t, repo2, provisioning.JobSpec{ + Action: provisioning.JobActionPull, + Pull: &provisioning.SyncJobOptions{}, + }) + + // Step 3: Verify the job failed with ownership conflict error + jobObj := &provisioning.Job{} + err := runtime.DefaultUnstructuredConverter.FromUnstructured(job.Object, jobObj) + require.NoError(t, err) + + // The job completes with "warning" state instead of "error" state when it doesn't have too many errors + t.Logf("Job state: %s", jobObj.Status.State) + t.Logf("Job errors: %v", jobObj.Status.Errors) + + require.Equal(t, provisioning.JobStateWarning, jobObj.Status.State, "job should complete with warnings due to ownership conflicts") + require.NotEmpty(t, jobObj.Status.Errors, "should have error details") + + // Check that error mentions ownership conflict + found := false + for _, errMsg := range jobObj.Status.Errors { + t.Logf("Error message: %s", errMsg) + if assert.Contains(t, errMsg, fmt.Sprintf("managed by repo '%s'", repo1)) && + assert.Contains(t, errMsg, fmt.Sprintf("cannot be modified by repo '%s'", repo2)) { + found = true + break + } + } + require.True(t, found, "should have ownership conflict error") + + // Step 4: Verify original resource is still owned by repo1 and unchanged + originalDashboard, err := helper.DashboardsV1.Resource.Get(ctx, allPanelsUID, metav1.GetOptions{}) + require.NoError(t, err, "original dashboard should still exist") + require.Equal(t, repo1, originalDashboard.GetAnnotations()[utils.AnnoKeyManagerIdentity], "ownership should remain with repo1") + + // Clean up the conflicting file for subsequent tests + err = os.Remove(filepath.Join(helper.ProvisioningPath, conflictingFilePath)) + require.NoError(t, err, "should clean up conflicting file") + }) + + // Test: Repositories should not delete resources owned by other repositories during pull + t.Run("repositories should not delete resources owned by other repositories during pull", func(t *testing.T) { + // Both repositories were created with their own resources (repo1 has all-panels.json, repo2 has timeline-demo.json) + // Verify that pulling one repository doesn't affect the other's resources + + // Step 1: Verify both repositories have their own resources + const allPanelsUID = "n1jR8vnnz" // UID from all-panels.json (repo1) + const timelineUID = "mIJjFy8Kz" // UID from timeline-demo.json (repo2) + + repo1Dashboard, err := helper.DashboardsV1.Resource.Get(ctx, allPanelsUID, metav1.GetOptions{}) + require.NoError(t, err, "repo1's dashboard should exist") + require.Equal(t, repo1, repo1Dashboard.GetAnnotations()[utils.AnnoKeyManagerIdentity], "should be owned by repo1") + + repo2Dashboard, err := helper.DashboardsV1.Resource.Get(ctx, timelineUID, metav1.GetOptions{}) + require.NoError(t, err, "repo2's dashboard should exist") + require.Equal(t, repo2, repo2Dashboard.GetAnnotations()[utils.AnnoKeyManagerIdentity], "should be owned by repo2") + + // Step 2: Pull repo1 (which doesn't manage repo2's resource) - should complete successfully + helper.SyncAndWait(t, repo1, nil) + + // Step 3: Verify that repo2's resource is still intact after repo1's pull + persistentRepo2Dashboard, err := helper.DashboardsV1.Resource.Get(ctx, timelineUID, metav1.GetOptions{}) + require.NoError(t, err, "repo2's dashboard should still exist after repo1 pull") + require.Equal(t, repo2, persistentRepo2Dashboard.GetAnnotations()[utils.AnnoKeyManagerIdentity], "ownership should remain with repo2") + require.Equal(t, repo2Dashboard.GetResourceVersion(), persistentRepo2Dashboard.GetResourceVersion(), "repo2's resource should not be modified by repo1 pull") + + // Step 4: Pull repo2 and verify repo1's resource is still intact + helper.SyncAndWait(t, repo2, nil) + + persistentRepo1Dashboard, err := helper.DashboardsV1.Resource.Get(ctx, allPanelsUID, metav1.GetOptions{}) + require.NoError(t, err, "repo1's dashboard should still exist after repo2 pull") + require.Equal(t, repo1, persistentRepo1Dashboard.GetAnnotations()[utils.AnnoKeyManagerIdentity], "ownership should remain with repo1") + require.Equal(t, repo1Dashboard.GetResourceVersion(), persistentRepo1Dashboard.GetResourceVersion(), "repo1's resource should not be modified by repo2 pull") + }) +}