From eff2da96d0d6777d3ff89ddea312b29de04c543d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roberto=20Jim=C3=A9nez=20S=C3=A1nchez?= Date: Wed, 26 Mar 2025 13:39:22 +0100 Subject: [PATCH] Remote Provisioning: Fix empty folder synchronization (#102789) * Add behavior for hidden * Rename to IsFilePathSupported * Modify sync * Revert renaming * Fix the tests * Add more tests * Maintain empty folders until next full pulling * Fix wording * Consider the file as ignored * Handle folder creation in sync * Record folder creation / update * Fix in manual test * Ensure / slash for folders * Refactor the tests * Keep safe path * Fix some cases * Remove log lines --- .../apis/provisioning/jobs/sync/changes.go | 47 +++- .../provisioning/jobs/sync/changes_test.go | 239 ++++++++++++++---- .../apis/provisioning/jobs/sync/worker.go | 52 +++- .../apis/provisioning/safepath/safe.go | 47 +++- .../apis/provisioning/safepath/safe_test.go | 68 +++++ 5 files changed, 386 insertions(+), 67 deletions(-) diff --git a/pkg/registry/apis/provisioning/jobs/sync/changes.go b/pkg/registry/apis/provisioning/jobs/sync/changes.go index 312c023854b..719fd2cd02d 100644 --- a/pkg/registry/apis/provisioning/jobs/sync/changes.go +++ b/pkg/registry/apis/provisioning/jobs/sync/changes.go @@ -3,6 +3,7 @@ package sync import ( "fmt" "sort" + "strings" folders "github.com/grafana/grafana/pkg/apis/folder/v0alpha1" provisioning "github.com/grafana/grafana/pkg/apis/provisioning/v0alpha1" @@ -29,19 +30,21 @@ func Changes(source []repository.FileTreeEntry, target *provisioning.ResourceLis } continue } + + // TODO: why do we have to do this here? + if item.Group == folders.GROUP && !strings.HasSuffix(item.Path, "/") { + item.Path = item.Path + "/" + } + lookup[item.Path] = &item } keep := safepath.NewTrie() changes := make([]ResourceFileChange, 0, len(source)) for _, file := range source { - if !file.Blob { - continue // skip folder references? - } - check, ok := lookup[file.Path] if ok { - if check.Hash != file.Hash { + if check.Hash != file.Hash && check.Resource != folders.RESOURCE { changes = append(changes, ResourceFileChange{ Action: repository.FileActionUpdated, Path: check.Path, @@ -53,16 +56,46 @@ func Changes(source []repository.FileTreeEntry, target *provisioning.ResourceLis return nil, fmt.Errorf("failed to add path to keep trie: %w", err) } - delete(lookup, file.Path) + if check.Resource != folders.RESOURCE { + delete(lookup, file.Path) + } + continue } - // TODO: does this work with empty folders? if resources.IsPathSupported(file.Path) == nil { changes = append(changes, ResourceFileChange{ Action: repository.FileActionCreated, // or previously ignored/failed Path: file.Path, }) + + if err := keep.Add(file.Path); err != nil { + return nil, fmt.Errorf("failed to add path to keep trie: %w", err) + } + + continue + } + + // Maintain the safe segment for empty folders + safeSegment := safepath.SafeSegment(file.Path) + if !safepath.IsDir(safeSegment) { + safeSegment = safepath.Dir(safeSegment) + } + + if safeSegment != "" && resources.IsPathSupported(safeSegment) == nil { + if err := keep.Add(safeSegment); err != nil { + return nil, fmt.Errorf("failed to add path to keep trie: %w", err) + } + + _, ok := lookup[safeSegment] + if ok { + continue + } + + changes = append(changes, ResourceFileChange{ + Action: repository.FileActionCreated, // or previously ignored/failed + Path: safeSegment, + }) } } diff --git a/pkg/registry/apis/provisioning/jobs/sync/changes_test.go b/pkg/registry/apis/provisioning/jobs/sync/changes_test.go index 94c415f958d..43b7ffb2e95 100644 --- a/pkg/registry/apis/provisioning/jobs/sync/changes_test.go +++ b/pkg/registry/apis/provisioning/jobs/sync/changes_test.go @@ -1,7 +1,6 @@ package sync import ( - "encoding/json" "testing" "github.com/stretchr/testify/require" @@ -12,17 +11,25 @@ import ( func TestChanges(t *testing.T) { t.Run("start the same", func(t *testing.T) { - source, target := getBase(t) + source := []repository.FileTreeEntry{ + {Path: "simplelocal/dashboard.json", Hash: "xyz", Blob: true}, + } + target := &provisioning.ResourceList{ + Items: []provisioning.ResourceListItem{ + {Path: "simplelocal/dashboard.json", Hash: "xyz"}, + }, + } + changes, err := Changes(source, target) require.NoError(t, err) require.Empty(t, changes) }) t.Run("create a source file", func(t *testing.T) { - source, target := getBase(t) - source = append(source, repository.FileTreeEntry{ - Path: "muta.json", Hash: "xyz", Blob: true, - }) + source := []repository.FileTreeEntry{ + {Path: "muta.json", Hash: "xyz", Blob: true}, + } + target := &provisioning.ResourceList{} changes, err := Changes(source, target) require.NoError(t, err) @@ -33,9 +40,78 @@ func TestChanges(t *testing.T) { }, changes[0]) }) + t.Run("create empty folder structure for folders with unsupported file types", func(t *testing.T) { + source := []repository.FileTreeEntry{ + {Path: "one/two/first.md", Hash: "xyz", Blob: true}, + {Path: "other/second.md", Hash: "xyz", Blob: true}, + } + + target := &provisioning.ResourceList{} + changes, err := Changes(source, target) + require.NoError(t, err) + require.Len(t, changes, 2) + + require.Equal(t, ResourceFileChange{ + Action: repository.FileActionCreated, + Path: "one/two/", + }, changes[0]) + require.Equal(t, ResourceFileChange{ + Action: repository.FileActionCreated, + Path: "other/", + }, changes[1]) + }) + + t.Run("keep empty folders when unsupported file types are present", func(t *testing.T) { + source := []repository.FileTreeEntry{ + {Path: "one/two/first.md", Hash: "xyz", Blob: true}, + {Path: "other/second.md", Hash: "xyz", Blob: true}, + } + + target := &provisioning.ResourceList{ + Items: []provisioning.ResourceListItem{ + {Path: "one/two/", Resource: "folders"}, + {Path: "other/", Resource: "folders"}, + }, + } + changes, err := Changes(source, target) + require.NoError(t, err) + require.Empty(t, changes) + }) + t.Run("keep common path to unsupported file types", func(t *testing.T) { + source := []repository.FileTreeEntry{ + {Path: "common/first.md", Hash: "xyz", Blob: true}, + {Path: "alsocommon/second.md", Hash: "xyz", Blob: true}, + } + + target := &provisioning.ResourceList{ + Items: []provisioning.ResourceListItem{ + {Path: "common/", Resource: "folders"}, + {Path: "common/not-common/", Resource: "folders", Name: "uncommon-name", Hash: "xyz"}, + {Path: "alsocommon/", Resource: "folders"}, + }, + } + changes, err := Changes(source, target) + require.NoError(t, err) + require.Len(t, changes, 1) + require.Equal(t, ResourceFileChange{ + Action: repository.FileActionDeleted, + Path: "common/not-common/", + Existing: &provisioning.ResourceListItem{ + Path: "common/not-common/", + Resource: "folders", + Name: "uncommon-name", + Hash: "xyz", + }, + }, changes[0], "the uncommon path should be deleted") + }) + t.Run("delete a source file", func(t *testing.T) { - source, target := getBase(t) - source = []repository.FileTreeEntry{source[0]} + source := []repository.FileTreeEntry{} + target := &provisioning.ResourceList{ + Items: []provisioning.ResourceListItem{ + {Path: "adsl62h.yaml", Hash: "xyz", Group: "dashboard.grafana.app", Resource: "dashboards", Name: "adsl62h-hrw-f-fvlt2dghp-gufrc4lisksgmq-c"}, + }, + } changes, err := Changes(source, target) require.NoError(t, err) @@ -48,7 +124,7 @@ func TestChanges(t *testing.T) { Group: "dashboard.grafana.app", Resource: "dashboards", Name: "adsl62h-hrw-f-fvlt2dghp-gufrc4lisksgmq-c", - Hash: "ce5d497c4deadde6831162ce8509e2b2b1776237", + Hash: "xyz", }, }, changes[0]) }) @@ -77,6 +153,7 @@ func TestChanges(t *testing.T) { } require.Equal(t, []string{ "zzz/longest/path/here.json", // not sorted yet + "x/y/z/", "x/y/file.json", "short/file.yml", "a.json", @@ -84,8 +161,20 @@ func TestChanges(t *testing.T) { }) t.Run("modify a file", func(t *testing.T) { - source, target := getBase(t) - source[1].Hash = "different" + source := []repository.FileTreeEntry{ + {Path: "adsl62h.yaml", Hash: "modified", Blob: true}, + } + target := &provisioning.ResourceList{ + Items: []provisioning.ResourceListItem{ + { + Path: "adsl62h.yaml", + Group: "dashboard.grafana.app", + Resource: "dashboards", + Name: "adsl62h-hrw-f-fvlt2dghp-gufrc4lisksgmq-c", + Hash: "original", + }, + }, + } changes, err := Changes(source, target) require.NoError(t, err) @@ -98,48 +187,104 @@ func TestChanges(t *testing.T) { Group: "dashboard.grafana.app", Resource: "dashboards", Name: "adsl62h-hrw-f-fvlt2dghp-gufrc4lisksgmq-c", - Hash: "ce5d497c4deadde6831162ce8509e2b2b1776237", + Hash: "original", }, }, changes[0]) }) -} -func getBase(t *testing.T) (source []repository.FileTreeEntry, target *provisioning.ResourceList) { - target = &provisioning.ResourceList{} - err := json.Unmarshal([]byte(`{ - "kind": "ResourceList", - "apiVersion": "provisioning.grafana.app/v0alpha1", - "metadata": {}, - "items": [ - { - "path": "", - "group": "folder.grafana.app", - "resource": "folders", - "name": "simplelocal-3794ab9", - "hash": "" + t.Run("keep folder with hidden files", func(t *testing.T) { + source := []repository.FileTreeEntry{ + {Path: "folder/.hidden.json", Hash: "xyz", Blob: true}, + } + target := &provisioning.ResourceList{ + Items: []provisioning.ResourceListItem{ + {Path: "folder/", Resource: "folders"}, }, - { - "path": "ad4lwp2.yaml", - "group": "dashboard.grafana.app", - "resource": "dashboards", - "name": "ad4lwp2-xofjsuo-mr5blr1zwimlfi0ds0pyrrpd", - "hash": "ca83d64b9c4a23fed975aacdf47e7de8878b4ae0" + } + changes, err := Changes(source, target) + require.NoError(t, err) + require.Empty(t, changes, "folder should be kept when it contains hidden files") + }) + + t.Run("keep folder with invalid hidden paths", func(t *testing.T) { + source := []repository.FileTreeEntry{ + {Path: "folder/.invalid/path.json", Hash: "xyz", Blob: true}, + } + target := &provisioning.ResourceList{ + Items: []provisioning.ResourceListItem{ + {Path: "folder/", Resource: "folders"}, }, + } + changes, err := Changes(source, target) + require.NoError(t, err) + require.Empty(t, changes, "folder should be kept when it contains invalid hidden paths") + }) + + t.Run("keep folder with hidden folders", func(t *testing.T) { + source := []repository.FileTreeEntry{ + {Path: "folder/.hidden/valid.json", Hash: "xyz", Blob: true}, + } + target := &provisioning.ResourceList{ + Items: []provisioning.ResourceListItem{ + {Path: "folder/", Resource: "folders"}, + }, + } + changes, err := Changes(source, target) + require.NoError(t, err) + require.Empty(t, changes, "folder should be kept when it contains hidden folders") + }) + + t.Run("unhidden path from hidden file", func(t *testing.T) { + source := []repository.FileTreeEntry{ + {Path: "folder/.hidden/dashboard.json", Hash: "xyz", Blob: true}, + } + target := &provisioning.ResourceList{ + Items: []provisioning.ResourceListItem{ + {Path: "folder/", Resource: "folders"}, + }, + } + changes, err := Changes(source, target) + require.NoError(t, err) + require.Empty(t, changes, "hidden file should not be unhidden") + }) + t.Run("hidden path to file", func(t *testing.T) { + source := []repository.FileTreeEntry{ + {Path: "one/two/.hidden/dashboard.json", Hash: "xyz", Blob: true}, + } + target := &provisioning.ResourceList{} + expected := []ResourceFileChange{ { - "path": "adsl62h.yaml", - "group": "dashboard.grafana.app", - "resource": "dashboards", - "name": "adsl62h-hrw-f-fvlt2dghp-gufrc4lisksgmq-c", - "hash": "ce5d497c4deadde6831162ce8509e2b2b1776237" - } - ] - }`), target) - require.NoError(t, err) + Action: repository.FileActionCreated, + Path: "one/two/", + }, + } + changes, err := Changes(source, target) + require.NoError(t, err) + require.Equal(t, expected, changes) + }) + t.Run("hidden path to folder", func(t *testing.T) { + source := []repository.FileTreeEntry{ + {Path: "one/two/.hidden/folder/", Hash: "xyz", Blob: true}, + } + target := &provisioning.ResourceList{} + expected := []ResourceFileChange{ + { + Action: repository.FileActionCreated, + Path: "one/two/", + }, + } + changes, err := Changes(source, target) + require.NoError(t, err) + require.Equal(t, expected, changes) + }) + t.Run("hidden at the root", func(t *testing.T) { + source := []repository.FileTreeEntry{ + {Path: ".hidden/dashboard.json", Hash: "xyz", Blob: true}, + } - source = []repository.FileTreeEntry{ - {Path: "ad4lwp2.yaml", Hash: "ca83d64b9c4a23fed975aacdf47e7de8878b4ae0", Blob: true}, - {Path: "adsl62h.yaml", Hash: "ce5d497c4deadde6831162ce8509e2b2b1776237", Blob: true}, - } - - return // named values! + target := &provisioning.ResourceList{} + changes, err := Changes(source, target) + require.NoError(t, err) + require.Empty(t, changes) + }) } diff --git a/pkg/registry/apis/provisioning/jobs/sync/worker.go b/pkg/registry/apis/provisioning/jobs/sync/worker.go index aa5757aabba..82565d752e6 100644 --- a/pkg/registry/apis/provisioning/jobs/sync/worker.go +++ b/pkg/registry/apis/provisioning/jobs/sync/worker.go @@ -6,7 +6,6 @@ import ( "encoding/json" "errors" "fmt" - "sort" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" @@ -263,15 +262,9 @@ func (r *syncJob) applyChanges(ctx context.Context, changes []ResourceFileChange return fmt.Errorf("this should be empty") } - // Do the deepest paths first (important for delete) - sort.Slice(changes, func(i, j int) bool { - return safepath.Depth(changes[i].Path) > safepath.Depth(changes[j].Path) - }) - r.progress.SetTotal(ctx, len(changes)) r.progress.SetMessage(ctx, "replicating changes") - // Create folder structure first for _, change := range changes { if err := r.progress.TooManyErrors(); err != nil { return err @@ -304,6 +297,28 @@ func (r *syncJob) applyChanges(ctx context.Context, changes []ResourceFileChange continue } + // If folder ensure it exists + if safepath.IsDir(change.Path) { + result := jobs.JobResourceResult{ + Path: change.Path, + Action: change.Action, + } + + folder, err := r.folders.EnsureFolderPathExist(ctx, change.Path) + if err != nil { + result.Error = fmt.Errorf("create folder: %w", err) + r.progress.Record(ctx, result) + continue + } + + result.Name = folder + result.Resource = folders.RESOURCE + result.Group = folders.GROUP + r.progress.Record(ctx, result) + + continue + } + // Write the resource file r.progress.Record(ctx, r.writeResourceFromFile(ctx, change.Path, "", change.Action)) } @@ -334,6 +349,29 @@ func (r *syncJob) applyVersionedChanges(ctx context.Context, repo repository.Ver } if err := resources.IsPathSupported(change.Path); err != nil { + // Maintain the safe segment for empty folders + safeSegment := safepath.SafeSegment(change.Path) + if !safepath.IsDir(safeSegment) { + safeSegment = safepath.Dir(safeSegment) + } + + if safeSegment != "" && resources.IsPathSupported(safeSegment) == nil { + folder, err := r.folders.EnsureFolderPathExist(ctx, safeSegment) + if err != nil { + return fmt.Errorf("unable to create empty file folder: %w", err) + } + + r.progress.Record(ctx, jobs.JobResourceResult{ + Path: safeSegment, + Action: repository.FileActionCreated, + Resource: folders.RESOURCE, + Group: folders.GROUP, + Name: folder, + }) + + continue + } + r.progress.Record(ctx, jobs.JobResourceResult{ Path: change.Path, Action: repository.FileActionIgnored, diff --git a/pkg/registry/apis/provisioning/safepath/safe.go b/pkg/registry/apis/provisioning/safepath/safe.go index 9e11220e00c..cfe4d71abd5 100644 --- a/pkg/registry/apis/provisioning/safepath/safe.go +++ b/pkg/registry/apis/provisioning/safepath/safe.go @@ -54,17 +54,14 @@ func IsSafe(path string) error { } parts := Split(path) - - // Check for path traversal attempts first for _, part := range parts { + // Check for path traversal attempts first if part == ".." || part == "." { return ErrPathTraversalAttempt } - } - // Check for hidden files/directories in any part of the path - for _, part := range parts { - if part != "" && part[0] == '.' && part != ".." && part != "." { + // Check for hidden files/directories in any part of the path + if part == "" || strings.HasPrefix(part, ".") { return ErrHiddenPath } } @@ -79,3 +76,41 @@ func IsSafe(path string) error { return nil } + +// SafeSegment returns a safe part of the path +// It ensures the path is free from traversal attempts, hidden files, +// and other potentially dangerous patterns. +func SafeSegment(path string) string { + if path == "" { + return "" + } + + parts := Split(path) + if len(parts) == 0 { + return "" + } + + // Build up the path segment by segment, checking safety + var safePath string + for _, part := range parts { + // Check if this segment is safe + testPath := Join(safePath, part) + if IsSafe(testPath) != nil || part == "" { + // If this segment is unsafe, return the path up to but not including this segment + // Add trailing slash for directories + if safePath != "" { + return safePath + "/" + } + return "" + } + safePath = testPath + } + + // If we made it through all segments, the path is safe + // Preserve trailing slash if original path had one + if IsDir(path) && safePath != "" { + return safePath + "/" + } + + return safePath +} diff --git a/pkg/registry/apis/provisioning/safepath/safe_test.go b/pkg/registry/apis/provisioning/safepath/safe_test.go index 3f99a83b9da..cffdddb2e86 100644 --- a/pkg/registry/apis/provisioning/safepath/safe_test.go +++ b/pkg/registry/apis/provisioning/safepath/safe_test.go @@ -233,3 +233,71 @@ func TestIsSafe(t *testing.T) { }) } } + +func TestSafeSegment(t *testing.T) { + tests := []struct { + name string + path string + wantPath string + }{ + { + name: "empty path", + path: "", + wantPath: "", + }, + { + name: "simple valid path", + path: "path/to/file.txt", + wantPath: "path/to/file.txt", + }, + { + name: "path with valid special characters", + path: "my-path/some_file/test.json", + wantPath: "my-path/some_file/test.json", + }, + { + name: "path with trailing slash", + path: "path/to/folder/", + wantPath: "path/to/folder/", + }, + { + name: "path with multiple extensions", + path: "path/to/file.min.js", + wantPath: "path/to/file.min.js", + }, + { + name: "path with invalid characters", + path: "path/to/file#.txt", + wantPath: "path/to/", + }, + { + name: "path with traversal attempt", + path: "path/../file.txt", + wantPath: "path/", + }, + { + name: "path with hidden file", + path: "path/to/.hidden", + wantPath: "path/to/", + }, + { + name: "path with percent character", + path: "path/to/%20file.txt", + wantPath: "path/to/", + }, + { + name: "path with double slashes", + path: "path//to/file.txt", + wantPath: "path/", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gotPath := SafeSegment(tt.path) + if gotPath != tt.wantPath { + t.Errorf("SafeSegment() = %v, want %v", gotPath, tt.wantPath) + } + }) + } +}