Provisioning: Check For Resource Ownership Before Operations (#109582)
This commit is contained in:
@@ -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()
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
@@ -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")
|
||||
})
|
||||
}
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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")
|
||||
})
|
||||
}
|
||||
Reference in New Issue
Block a user