From 805f8e7642e9cc8919fc8b0aec1ec398d9cdfd7d Mon Sep 17 00:00:00 2001 From: Luca Voges Date: Wed, 9 Sep 2026 10:38:29 +0200 Subject: [PATCH 1/2] feat(repo-server): patch multiple files in one atomic commit --- README.md | 54 +++- internal/git/git_test.go | 62 +++++ internal/git/inspect.go | 56 ++++ internal/git/restore.go | 39 +++ internal/patch/method_git.go | 245 ++++++++++++++---- internal/patch/method_git_test.go | 244 ++++++++++++++++- internal/patch/method_server.go | 75 ++++-- internal/patch/method_server_test.go | 134 ++++++++++ internal/patch/patch.go | 83 ++++++ internal/patch/patch_test.go | 158 +++++++++++ .../repo_server/server/patch_controller.go | 8 +- .../repo_server/server/patches_controller.go | 47 ++++ .../server/patches_controller_test.go | 217 ++++++++++++++++ internal/repo_server/server/server.go | 10 +- test/bruno/patch multiple files.bru | 43 +++ 15 files changed, 1391 insertions(+), 84 deletions(-) create mode 100644 internal/git/inspect.go create mode 100644 internal/git/restore.go create mode 100644 internal/patch/method_server_test.go create mode 100644 internal/patch/patch_test.go create mode 100644 internal/repo_server/server/patches_controller.go create mode 100644 internal/repo_server/server/patches_controller_test.go create mode 100644 test/bruno/patch multiple files.bru diff --git a/README.md b/README.md index 8f3e378..1fdfe20 100644 --- a/README.md +++ b/README.md @@ -429,4 +429,56 @@ curl --request PUT \ # Equivalent CLI command # Evaluates the previously exported env vars GITOPS_REPOSITORY_SERVER and GITOPS_REPOSITORY_SERVER_API_KEY gitops patch applications/dev/service-foo/values.yaml .service.image.tag v42.0.1 -``` \ No newline at end of file +``` + +#### Patching multiple files atomically +To patch more than one file, use `PUT ://:/api/v1/patches`. +All patches of the request are applied and committed as a **single commit**, which is useful for +matrix builds that update several services of the same release at once. + +```bash +curl --request PUT \ + --url $GITOPS_REPOSITORY_SERVER/patches \ + --header 'content-type: application/json' \ + --header "X-API-Key: $GITOPS_REPOSITORY_SERVER_API_KEY" \ + --data '{ + "actor": "ci-bot", + "files": [ + { + "filePath": "applications/dev/service-foo/values.yaml", + "patches": [ + { + "selector": ".service.image.tag", + "value": "v42.0.1" + } + ] + }, + { + "filePath": "applications/dev/service-bar/values.yaml", + "patches": [ + { + "selector": ".service.image.tag", + "value": "v42.0.1" + } + ] + } + ] +}' +``` + +The response contains the id of the created commit: + +```json +{ "message": "ok", "commit": "8f1c0a2e2a3b4c5d6e7f8091a2b3c4d5e6f70819" } +``` + +If none of the patches changed anything, the request still succeeds and `commit` is empty. +Requests are rejected with `400` and a descriptive error message if `files` is empty, if a file has +no patches, if a selector is empty, if a file path is listed twice, or if a file path is absolute or +points outside of the repository. +If a single file of the request cannot be patched (e.g. the file does not exist or a selector does +not match), the whole request fails with `500` and **no** file is changed. + +The `actor` is optional and added to the commit message as a `Triggered by:` footer. + +The single file endpoint `PUT /api/v1/patch` remains available and unchanged. \ No newline at end of file diff --git a/internal/git/git_test.go b/internal/git/git_test.go index 625299f..936afe2 100644 --- a/internal/git/git_test.go +++ b/internal/git/git_test.go @@ -193,3 +193,65 @@ func TestGitPullRebase(t *testing.T) { assert.NoError(t, err) assert.Equal(t, "test B", string(data)) } + +func TestGitRestore(t *testing.T) { + + tempConnection := cloneTempRepository(t) + assert.NotNil(t, tempConnection) + + relativeFilePath := path.Join("applications", "dev", "service-test", "values.yaml") + absoluteFilePath := path.Join(tempConnection.Options.Directory, relativeFilePath) + + originalContents, err := os.ReadFile(absoluteFilePath) + assert.NoError(t, err) + assert.NotEmpty(t, originalContents) + + err = os.WriteFile(absoluteFilePath, []byte("clobbered: true\n"), 0644) + assert.NoError(t, err) + + hasChanges, err := tempConnection.HasChanges() + assert.NoError(t, err) + assert.True(t, hasChanges) + + err = tempConnection.Restore([]string{relativeFilePath}) + assert.NoError(t, err) + + hasChanges, err = tempConnection.HasChanges() + assert.NoError(t, err) + assert.False(t, hasChanges) + + restoredContents, err := os.ReadFile(absoluteFilePath) + assert.NoError(t, err) + assert.Equal(t, string(originalContents), string(restoredContents)) +} + +func TestGitCommitFiles(t *testing.T) { + + tempConnection := cloneTempRepository(t) + assert.NotNil(t, tempConnection) + + headBefore, err := tempConnection.RevParse("HEAD") + assert.NoError(t, err) + assert.NotEmpty(t, headBefore) + + testFileNameA := "test-file-" + uuid.New().String() + testFileNameB := "test-file-" + uuid.New().String() + + for _, testFileName := range []string{testFileNameA, testFileNameB} { + err = os.WriteFile(path.Join(tempConnection.Options.Directory, testFileName), []byte("test"), 0644) + assert.NoError(t, err) + } + + hash, err := tempConnection.Commit([]string{testFileNameA, testFileNameB}, "Test commit with two files") + assert.NoError(t, err) + assert.NotEmpty(t, hash) + + files, err := tempConnection.CommitFiles(hash) + assert.NoError(t, err) + assert.ElementsMatch(t, []string{testFileNameA, testFileNameB}, files) + + // the commit must sit directly on top of the previously resolved HEAD + parent, err := tempConnection.RevParse(hash + "^") + assert.NoError(t, err) + assert.Equal(t, headBefore, parent) +} diff --git a/internal/git/inspect.go b/internal/git/inspect.go new file mode 100644 index 0000000..9c8de78 --- /dev/null +++ b/internal/git/inspect.go @@ -0,0 +1,56 @@ +package git + +import ( + "fmt" + "strings" + + "github.com/ldez/go-git-cmd-wrapper/v2/git" + "github.com/ldez/go-git-cmd-wrapper/v2/revparse" + "github.com/ldez/go-git-cmd-wrapper/v2/types" + "github.com/rs/zerolog/log" +) + +// RevParse resolves the given revision to its commit id. +func (c *Connection) RevParse(revision string) (string, error) { + directory := c.Options.Directory + if directory == "" { + return "", fmt.Errorf("directory is not specified") + } + + commitId, err := git.RevParse(runGitIn(directory), revparse.Args(revision)) + if err != nil { + log.Error().Err(err).Str("output", commitId).Msgf("Failed to resolve revision %s", revision) + return "", err + } + + return strings.TrimSpace(commitId), nil +} + +// CommitFiles returns the repository relative paths of all files touched by the +// given commit. +func (c *Connection) CommitFiles(commitId string) ([]string, error) { + directory := c.Options.Directory + if directory == "" { + return nil, fmt.Errorf("directory is not specified") + } + + msg, err := git.Raw("show", runGitIn(directory), func(g *types.Cmd) { + g.AddOptions("--name-only") + g.AddOptions("--format=") + g.AddOptions(commitId) + }) + if err != nil { + log.Error().Err(err).Str("output", msg).Msgf("Failed to list files of commit %s", commitId) + return nil, err + } + + files := []string{} + for _, line := range strings.Split(msg, "\n") { + line = strings.TrimSpace(line) + if line != "" { + files = append(files, line) + } + } + + return files, nil +} diff --git a/internal/git/restore.go b/internal/git/restore.go new file mode 100644 index 0000000..2011a64 --- /dev/null +++ b/internal/git/restore.go @@ -0,0 +1,39 @@ +package git + +import ( + "fmt" + + "github.com/ldez/go-git-cmd-wrapper/v2/git" + "github.com/ldez/go-git-cmd-wrapper/v2/types" + "github.com/rs/zerolog/log" +) + +// Restore discards working tree changes of the given repository relative file +// paths by running `git checkout -- `. It intentionally does not touch +// commits, so a local commit whose push failed is preserved and pushed by a +// subsequent pull/push cycle. +func (c *Connection) Restore(files []string) error { + directory := c.Options.Directory + if directory == "" { + return fmt.Errorf("directory is not specified") + } + + if len(files) == 0 { + return nil + } + + msg, err := git.Raw("checkout", runGitIn(directory), func(g *types.Cmd) { + g.AddOptions("--") + for _, file := range files { + g.AddOptions(file) + } + }) + if err != nil { + log.Error().Err(err).Str("output", msg).Msg("Failed to restore files") + return err + } + + log.Debug().Msgf("Restored %d file(s) in working tree", len(files)) + + return nil +} diff --git a/internal/patch/method_git.go b/internal/patch/method_git.go index 36ddb62..84fda36 100644 --- a/internal/patch/method_git.go +++ b/internal/patch/method_git.go @@ -2,8 +2,9 @@ package patch import ( "fmt" + "io/fs" "os" - "path" + "path/filepath" "time" "github.com/mxcd/gitops-cli/internal/git" @@ -13,6 +14,9 @@ import ( "github.com/rs/zerolog/log" ) +// pushRetryCount is the number of pull/push attempts before giving up. +const pushRetryCount = 5 + type GitPatcherOptions struct { GitConnectionOptions *git.ConnectionOptions GitConnection *git.Connection @@ -109,48 +113,136 @@ func (p *GitPatcher) Prepare(options *PrepareOptions) error { return nil } -func (p *GitPatcher) Patch(patchTasks []PatchTask) error { +// patchedFile holds the fully patched contents of a single file before it is +// written to disk. +type patchedFile struct { + RelativePath string + AbsolutePath string + Mode fs.FileMode + Contents []byte +} - err := p.GitConnection.Pull() +// preparePatchedFile reads a file and applies all of its patches in memory. No +// changes are written to disk, so a failing file never leaves a partially +// patched working tree behind. +func (p *GitPatcher) preparePatchedFile(file FilePatch) (*patchedFile, error) { + relativeFilePath, err := cleanRelativeFilePath(file.FilePath) if err != nil { - return err + return nil, fmt.Errorf("%w: filePath '%s': %s", ErrInvalidPatchBatch, file.FilePath, err.Error()) } - for _, patchTask := range patchTasks { - relativeFilePath := patchTask.FilePath + absoluteFilePath := filepath.Join(p.GitConnection.Options.Directory, relativeFilePath) + + fileStat, err := os.Stat(absoluteFilePath) + if err != nil { + log.Error().Err(err).Msgf("Failed to stat file %s", relativeFilePath) + return nil, fmt.Errorf("failed to stat file %s: %w", relativeFilePath, err) + } + + fileContents, err := os.ReadFile(absoluteFilePath) + if err != nil { + log.Error().Err(err).Msgf("Failed to read file %s", relativeFilePath) + return nil, fmt.Errorf("failed to read file %s: %w", relativeFilePath, err) + } - absoluteFilePath := path.Join(p.GitConnection.Options.Directory, relativeFilePath) + log.Debug().Msgf("original yaml file: %s", string(fileContents)) - fileStat, err := os.Stat(absoluteFilePath) + for _, filePatch := range file.Patches { + selector := filePatch.Selector + value := filePatch.Value + + log.Debug().Msgf("patching file '%s' with selector '%s' and value '%s'", relativeFilePath, selector, value) + patchedData, err := yaml.PatchYaml(fileContents, selector, value) if err != nil { - log.Error().Err(err).Msg("Failed to stat file") + return nil, fmt.Errorf("failed to patch file %s with selector '%s': %w", relativeFilePath, selector, err) } + log.Debug().Msgf("patched yaml file:\n%s", string(patchedData)) + + fileContents = patchedData + } + + return &patchedFile{ + RelativePath: relativeFilePath, + AbsolutePath: absoluteFilePath, + Mode: fileStat.Mode(), + Contents: fileContents, + }, nil +} - fileContents, err := os.ReadFile(absoluteFilePath) +// writePatchedFile writes the patched contents back to disk. +func (p *GitPatcher) writePatchedFile(file *patchedFile) error { + if err := os.WriteFile(file.AbsolutePath, file.Contents, file.Mode); err != nil { + log.Error().Err(err).Msgf("Failed to write file %s", file.RelativePath) + return fmt.Errorf("failed to write file %s: %w", file.RelativePath, err) + } + return nil +} + +// pushWithRetry pulls and pushes with a linear backoff to resolve races with +// concurrent writers to the repository. +func (p *GitPatcher) pushWithRetry() error { + executePush := func() error { + err := p.GitConnection.Pull() if err != nil { - log.Error().Err(err).Msg("Failed to read file") + log.Error().Err(err).Msg("Error pulling prior to push") return err } + return p.GitConnection.Push() + } - log.Debug().Msgf("original yaml file: %s", string(fileContents)) - - for _, patch := range patchTask.Patches { - selector := patch.Selector - value := patch.Value + var err error + for i := 0; i < pushRetryCount; i++ { + err = executePush() + if err == nil { + return nil + } + if i < pushRetryCount-1 { + time.Sleep(time.Duration(i+1) * time.Second) + } + } - log.Debug().Msgf("patching file with selector '%s' and value '%s'", selector, value) - patchedData, err := yaml.PatchYaml(fileContents, selector, value) - if err != nil { - return err - } - log.Debug().Msgf("patched yaml file:\n%s", string(patchedData)) + return err +} - fileContents = patchedData +// buildCommitMessage builds the commit message for the given repository +// relative file paths. +func buildCommitMessage(relativeFilePaths []string, actor string) string { + var message string + if len(relativeFilePaths) == 1 { + message = fmt.Sprintf("feat(gitops): patching %s", relativeFilePaths[0]) + } else { + message = fmt.Sprintf("feat(gitops): patching %d files\n", len(relativeFilePaths)) + for _, relativeFilePath := range relativeFilePaths { + message += fmt.Sprintf("\n- %s", relativeFilePath) } + } + + if actor != "" { + message += fmt.Sprintf("\n\nTriggered by: %s", actor) + } + + return message +} + +func (p *GitPatcher) Patch(patchTasks []PatchTask) error { - err = os.WriteFile(absoluteFilePath, fileContents, fileStat.Mode()) + err := p.GitConnection.Pull() + if err != nil { + return err + } + + commitCount := 0 + + for _, patchTask := range patchTasks { + preparedFile, err := p.preparePatchedFile(FilePatch{ + FilePath: patchTask.FilePath, + Patches: patchTask.Patches, + }) if err != nil { - log.Error().Err(err).Msg("Failed to write file") + return err + } + + if err := p.writePatchedFile(preparedFile); err != nil { return err } @@ -161,41 +253,102 @@ func (p *GitPatcher) Patch(patchTasks []PatchTask) error { } if !hasChanges { - log.Info().Msg("No changes detected, exiting") - return nil - } else { - log.Debug().Msg("Changes detected, committing") + log.Info().Msgf("No changes detected for %s, skipping commit", preparedFile.RelativePath) + continue } - commitFooter := "" + log.Debug().Msg("Changes detected, committing") - if patchTask.Actor != "" { - commitFooter = fmt.Sprintf("\n\nTriggered by: %s", patchTask.Actor) - } - - commitHash, err := p.GitConnection.Commit([]string{relativeFilePath}, fmt.Sprintf("feat(gitops): patching %s%s", relativeFilePath, commitFooter)) + commitHash, err := p.GitConnection.Commit( + []string{preparedFile.RelativePath}, + buildCommitMessage([]string{preparedFile.RelativePath}, patchTask.Actor), + ) if err != nil { return err } + commitCount++ log.Info().Msgf("Created patch commit: %s", commitHash) } - executePush := func() error { - err := p.GitConnection.Pull() + if commitCount == 0 { + log.Info().Msg("No changes detected, nothing to push") + return nil + } + + return p.pushWithRetry() +} + +// PatchBatch applies all patches of the batch and commits them as a single +// atomic commit. It returns the commit id, or an empty string if the batch did +// not change anything. +func (p *GitPatcher) PatchBatch(batch PatchBatch) (hash string, err error) { + + if err := ValidatePatchBatch(batch); err != nil { + return "", err + } + + if err := p.GitConnection.Pull(); err != nil { + return "", err + } + + // patch all files in memory first, so that a failure of any file does not + // leave the working tree with a partially applied batch + preparedFiles := make([]*patchedFile, 0, len(batch.Files)) + relativeFilePaths := make([]string, 0, len(batch.Files)) + for _, file := range batch.Files { + preparedFile, err := p.preparePatchedFile(file) if err != nil { - log.Error().Err(err).Msg("Error pulling prior to push") - return err + return "", err } - return p.GitConnection.Push() + preparedFiles = append(preparedFiles, preparedFile) + relativeFilePaths = append(relativeFilePaths, preparedFile.RelativePath) } - for i := 0; i < 5; i++ { - err = executePush() - if err == nil { - break + filesWritten := false + defer func() { + if err != nil && filesWritten { + log.Warn().Msg("Restoring working tree after failed batch patch") + if restoreErr := p.GitConnection.Restore(relativeFilePaths); restoreErr != nil { + log.Error().Err(restoreErr).Msg("Failed to restore working tree after failed batch patch") + } + } + }() + + for _, preparedFile := range preparedFiles { + filesWritten = true + if err = p.writePatchedFile(preparedFile); err != nil { + return "", err } - time.Sleep(time.Duration(i+1) * time.Second) } - return err + log.Debug().Msg("checking for changes") + hasChanges, err := p.GitConnection.HasChanges() + if err != nil { + return "", err + } + + if !hasChanges { + log.Info().Msg("No changes detected, nothing to commit") + return "", nil + } + + log.Debug().Msg("Changes detected, committing") + + hash, err = p.GitConnection.Commit(relativeFilePaths, buildCommitMessage(relativeFilePaths, batch.Actor)) + if err != nil { + return "", err + } + + // the changes are committed, so the working tree is clean and must not be + // restored if the push fails. The commit stays local and is pushed by the + // next pull/push cycle + filesWritten = false + log.Info().Msgf("Created patch commit: %s", hash) + + if err = p.pushWithRetry(); err != nil { + log.Error().Err(err).Msgf("Failed to push commit %s, it remains local and is retried on the next patch", hash) + return "", err + } + + return hash, nil } diff --git a/internal/patch/method_git_test.go b/internal/patch/method_git_test.go index 58399bc..94e34d0 100644 --- a/internal/patch/method_git_test.go +++ b/internal/patch/method_git_test.go @@ -26,19 +26,20 @@ func getSshKeyData(t *testing.T) []byte { return sshKey } -func TestGitSshPatch(t *testing.T) { +// newTestPatcher clones the soft-serve test repository into a fresh sandbox +// directory and returns a prepared patcher for it. +func newTestPatcher(t *testing.T) *GitPatcher { sshKey := getSshKeyData(t) baseDir, err := util.GetGitRepoRoot() assert.NoError(t, err) - uuidA := uuid.New().String() - repositoryPathA := path.Join(baseDir, "sandbox", "gitops-test-"+uuidA) - err = os.MkdirAll(repositoryPathA, 0755) + repositoryPath := path.Join(baseDir, "sandbox", "gitops-test-"+uuid.New().String()) + err = os.MkdirAll(repositoryPath, 0755) assert.NoError(t, err) - gitConnectionOptionsA := &git.ConnectionOptions{ + gitConnectionOptions := &git.ConnectionOptions{ Repository: "ssh://localhost:23231/gitops-test.git", - Directory: repositoryPathA, + Directory: repositoryPath, Branch: "main", IgnoreSshHostKey: true, Authentication: &git.Authentication{ @@ -49,17 +50,55 @@ func TestGitSshPatch(t *testing.T) { } patcher, err := NewGitPatcher(&GitPatcherOptions{ - GitConnectionOptions: gitConnectionOptionsA, + GitConnectionOptions: gitConnectionOptions, }) assert.NoError(t, err) assert.NotNil(t, patcher) - err = patcher.Prepare(&PrepareOptions{ - Clone: true, - }) + err = patcher.Prepare(&PrepareOptions{Clone: true}) + assert.NoError(t, err) assert.NotNil(t, patcher.GitConnection) + + return patcher +} + +// seedFixtureFile creates a unique values file in the repository, pushes it and +// returns its repository relative path. Every test works on its own files, so +// tests do not interfere with each other. +func seedFixtureFile(t *testing.T, patcher *GitPatcher) string { + baseDir, err := util.GetGitRepoRoot() + assert.NoError(t, err) + + fixtureContents, err := os.ReadFile(path.Join(baseDir, "hack", "soft-serve", "fixtures", "values.yaml")) + assert.NoError(t, err) + assert.NotEmpty(t, fixtureContents) + + relativeFilePath := path.Join("applications", "dev", "service-test-"+uuid.New().String(), "values.yaml") + absoluteFilePath := path.Join(patcher.GitConnection.Options.Directory, relativeFilePath) + + err = os.MkdirAll(path.Dir(absoluteFilePath), 0755) + assert.NoError(t, err) + err = os.WriteFile(absoluteFilePath, fixtureContents, 0644) assert.NoError(t, err) + _, err = patcher.GitConnection.Commit([]string{relativeFilePath}, "test: seed "+relativeFilePath) + assert.NoError(t, err) + + err = patcher.pushWithRetry() + assert.NoError(t, err) + + return relativeFilePath +} + +func readRepositoryFile(t *testing.T, connection *git.Connection, relativeFilePath string) string { + contents, err := os.ReadFile(path.Join(connection.Options.Directory, relativeFilePath)) + assert.NoError(t, err) + return string(contents) +} + +func TestGitSshPatch(t *testing.T) { + patcher := newTestPatcher(t) + patchTask := PatchTask{ FilePath: "applications/dev/service-test/values.yaml", Patches: []Patch{ @@ -70,6 +109,189 @@ func TestGitSshPatch(t *testing.T) { }, } - err = patcher.Patch([]PatchTask{patchTask}) + err := patcher.Patch([]PatchTask{patchTask}) + assert.NoError(t, err) +} + +func TestGitSshPatchMissingFile(t *testing.T) { + patcher := newTestPatcher(t) + + err := patcher.Patch([]PatchTask{{ + FilePath: "applications/does-not-exist/values.yaml", + Patches: []Patch{{Selector: ".service.image.tag", Value: "v1.0.1"}}, + }}) + assert.Error(t, err) + + hasChanges, err := patcher.GitConnection.HasChanges() + assert.NoError(t, err) + assert.False(t, hasChanges) +} + +func TestGitSshPatchBatch(t *testing.T) { + patcher := newTestPatcher(t) + + relativeFilePathA := seedFixtureFile(t, patcher) + relativeFilePathB := seedFixtureFile(t, patcher) + + batch := PatchBatch{ + Actor: "ci-bot", + Files: []FilePatch{ + {FilePath: relativeFilePathA, Patches: []Patch{{Selector: ".service.image.tag", Value: "v2.0.0"}}}, + {FilePath: relativeFilePathB, Patches: []Patch{{Selector: ".service.global.namespace", Value: "batch-namespace"}}}, + }, + } + + commitHash, err := patcher.PatchBatch(batch) + assert.NoError(t, err) + assert.NotEmpty(t, commitHash) + + // both files must be part of the very same commit + committedFiles, err := patcher.GitConnection.CommitFiles(commitHash) + assert.NoError(t, err) + assert.ElementsMatch(t, []string{relativeFilePathA, relativeFilePathB}, committedFiles) + + // the changes must be visible in an independent clone of the repository + verificationPatcher := newTestPatcher(t) + resolvedCommitHash, err := verificationPatcher.GitConnection.RevParse(commitHash) + assert.NoError(t, err) + assert.Equal(t, commitHash, resolvedCommitHash) + + assert.Contains(t, readRepositoryFile(t, verificationPatcher.GitConnection, relativeFilePathA), "tag: v2.0.0") + assert.Contains(t, readRepositoryFile(t, verificationPatcher.GitConnection, relativeFilePathB), "namespace: batch-namespace") +} + +func TestGitSshPatchBatchSingleFile(t *testing.T) { + patcher := newTestPatcher(t) + + relativeFilePath := seedFixtureFile(t, patcher) + + commitHash, err := patcher.PatchBatch(PatchBatch{ + Files: []FilePatch{ + {FilePath: relativeFilePath, Patches: []Patch{{Selector: ".service.image.tag", Value: "v3.0.0"}}}, + }, + }) + assert.NoError(t, err) + assert.NotEmpty(t, commitHash) + + committedFiles, err := patcher.GitConnection.CommitFiles(commitHash) + assert.NoError(t, err) + assert.Equal(t, []string{relativeFilePath}, committedFiles) +} + +func TestGitSshPatchBatchNoChanges(t *testing.T) { + patcher := newTestPatcher(t) + + relativeFilePath := seedFixtureFile(t, patcher) + + batch := PatchBatch{ + Files: []FilePatch{ + {FilePath: relativeFilePath, Patches: []Patch{{Selector: ".service.image.tag", Value: "v4.0.0"}}}, + }, + } + + commitHash, err := patcher.PatchBatch(batch) + assert.NoError(t, err) + assert.NotEmpty(t, commitHash) + + // applying the same batch again must not create another commit + commitHash, err = patcher.PatchBatch(batch) + assert.NoError(t, err) + assert.Empty(t, commitHash) +} + +func TestGitSshPatchBatchMissingFile(t *testing.T) { + patcher := newTestPatcher(t) + + relativeFilePath := seedFixtureFile(t, patcher) + originalContents := readRepositoryFile(t, patcher.GitConnection, relativeFilePath) + + commitHash, err := patcher.PatchBatch(PatchBatch{ + Files: []FilePatch{ + {FilePath: relativeFilePath, Patches: []Patch{{Selector: ".service.image.tag", Value: "v5.0.0"}}}, + {FilePath: "applications/does-not-exist/values.yaml", Patches: []Patch{{Selector: ".service.image.tag", Value: "v5.0.0"}}}, + }, + }) + assert.Error(t, err) + assert.Empty(t, commitHash) + + // the valid file must not have been touched + assert.Equal(t, originalContents, readRepositoryFile(t, patcher.GitConnection, relativeFilePath)) + + hasChanges, err := patcher.GitConnection.HasChanges() + assert.NoError(t, err) + assert.False(t, hasChanges) +} + +func TestGitSshPatchBatchInvalidSelector(t *testing.T) { + patcher := newTestPatcher(t) + + relativeFilePathA := seedFixtureFile(t, patcher) + relativeFilePathB := seedFixtureFile(t, patcher) + originalContents := readRepositoryFile(t, patcher.GitConnection, relativeFilePathA) + + commitHash, err := patcher.PatchBatch(PatchBatch{ + Files: []FilePatch{ + {FilePath: relativeFilePathA, Patches: []Patch{{Selector: ".service.image.tag", Value: "v6.0.0"}}}, + {FilePath: relativeFilePathB, Patches: []Patch{{Selector: ".does.not.exist", Value: "v6.0.0"}}}, + }, + }) + assert.Error(t, err) + assert.Empty(t, commitHash) + + // the failure happens while patching in memory, so no file is written + assert.Equal(t, originalContents, readRepositoryFile(t, patcher.GitConnection, relativeFilePathA)) + + hasChanges, err := patcher.GitConnection.HasChanges() + assert.NoError(t, err) + assert.False(t, hasChanges) +} + +func TestGitSshPatchBatchInvalidFilePath(t *testing.T) { + patcher := newTestPatcher(t) + + commitHash, err := patcher.PatchBatch(PatchBatch{ + Files: []FilePatch{ + {FilePath: "../escape.yaml", Patches: []Patch{{Selector: ".service.image.tag", Value: "v7.0.0"}}}, + }, + }) + assert.Error(t, err) + assert.ErrorIs(t, err, ErrInvalidPatchBatch) + assert.Empty(t, commitHash) +} + +func TestGitSshPatchBatchWriteFailureRestoresWorkingTree(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("read only files are still writable as root") + } + + patcher := newTestPatcher(t) + + relativeFilePathA := seedFixtureFile(t, patcher) + relativeFilePathB := seedFixtureFile(t, patcher) + originalContents := readRepositoryFile(t, patcher.GitConnection, relativeFilePathA) + + // make the second file read only so that the write phase fails after the + // first file has already been written + absoluteFilePathB := path.Join(patcher.GitConnection.Options.Directory, relativeFilePathB) + err := os.Chmod(absoluteFilePathB, 0444) + assert.NoError(t, err) + defer func() { + assert.NoError(t, os.Chmod(absoluteFilePathB, 0644)) + }() + + commitHash, err := patcher.PatchBatch(PatchBatch{ + Files: []FilePatch{ + {FilePath: relativeFilePathA, Patches: []Patch{{Selector: ".service.image.tag", Value: "v8.0.0"}}}, + {FilePath: relativeFilePathB, Patches: []Patch{{Selector: ".service.image.tag", Value: "v8.0.0"}}}, + }, + }) + assert.Error(t, err) + assert.Empty(t, commitHash) + + // the already written file must have been restored + assert.Equal(t, originalContents, readRepositoryFile(t, patcher.GitConnection, relativeFilePathA)) + + hasChanges, err := patcher.GitConnection.HasChanges() assert.NoError(t, err) + assert.False(t, hasChanges) } diff --git a/internal/patch/method_server.go b/internal/patch/method_server.go index d6f5b75..6067bb6 100644 --- a/internal/patch/method_server.go +++ b/internal/patch/method_server.go @@ -42,30 +42,29 @@ func (p *RepositoryServerPatcher) Prepare(options *PrepareOptions) error { return nil } -func (p *RepositoryServerPatcher) Patch(patchTasks []PatchTask) error { - if len(patchTasks) == 0 { - log.Warn().Msg("No patch tasks provided, skipping patching") - return nil - } - - if len(patchTasks) > 1 { - log.Warn().Msg("More than one patch task provided, only the first one will be applied") - } +// patchesResponse is the response body of the repository server patch +// endpoints. +type patchesResponse struct { + Message string `json:"message"` + Commit string `json:"commit"` +} - jsonData, err := json.Marshal(patchTasks[0]) +// doPut marshals the payload and sends it to the given repository server path. +func (p *RepositoryServerPatcher) doPut(path string, payload any) ([]byte, error) { + jsonData, err := json.Marshal(payload) if err != nil { - log.Error().Err(err).Msg("Failed to marshal patch tasks") - return fmt.Errorf("failed to marshal patch tasks: %w", err) + log.Error().Err(err).Msg("Failed to marshal patch payload") + return nil, fmt.Errorf("failed to marshal patch payload: %w", err) } - log.Debug().Msgf("Patch task JSON: %s", string(jsonData)) + log.Debug().Msgf("Patch payload JSON: %s", string(jsonData)) - requestURL := fmt.Sprintf("%s/patch", p.RepositoryServerURL) + requestURL := fmt.Sprintf("%s%s", p.RepositoryServerURL, path) log.Debug().Msgf("Request URL: %s", requestURL) req, err := http.NewRequest(http.MethodPut, requestURL, bytes.NewReader(jsonData)) if err != nil { log.Error().Err(err).Msg("Failed to create HTTP request") - return fmt.Errorf("failed to create HTTP request: %w", err) + return nil, fmt.Errorf("failed to create HTTP request: %w", err) } req.Header.Set("Content-Type", "application/json") @@ -76,21 +75,61 @@ func (p *RepositoryServerPatcher) Patch(patchTasks []PatchTask) error { resp, err := client.Do(req) if err != nil { log.Error().Err(err).Msg("Failed to send request to repository server") - return fmt.Errorf("failed to send request to repository server: %w", err) + return nil, fmt.Errorf("failed to send request to repository server: %w", err) } defer resp.Body.Close() body, err := io.ReadAll(resp.Body) if err != nil { log.Error().Err(err).Msg("Failed to read response body") - return fmt.Errorf("failed to read response body: %w", err) + return nil, fmt.Errorf("failed to read response body: %w", err) } if resp.StatusCode != http.StatusOK { log.Error().Msgf("Repository server returned status %d: %s", resp.StatusCode, string(body)) - return fmt.Errorf("repository server returned status %d: %s", resp.StatusCode, string(body)) + return nil, fmt.Errorf("repository server returned status %d: %s", resp.StatusCode, string(body)) + } + + return body, nil +} + +func (p *RepositoryServerPatcher) Patch(patchTasks []PatchTask) error { + if len(patchTasks) == 0 { + log.Warn().Msg("No patch tasks provided, skipping patching") + return nil + } + + if len(patchTasks) > 1 { + log.Warn().Msg("More than one patch task provided, only the first one will be applied") + } + + if _, err := p.doPut("/patch", patchTasks[0]); err != nil { + return err } log.Info().Msg("Patch applied successfully via repository server.") return nil } + +// PatchBatch sends all files of the batch to the repository server so they are +// applied in a single commit. It returns the commit id reported by the server. +func (p *RepositoryServerPatcher) PatchBatch(batch PatchBatch) (string, error) { + if len(batch.Files) == 0 { + log.Warn().Msg("No files provided, skipping patching") + return "", nil + } + + body, err := p.doPut("/patches", batch) + if err != nil { + return "", err + } + + var response patchesResponse + if err := json.Unmarshal(body, &response); err != nil { + log.Error().Err(err).Msg("Failed to unmarshal response body") + return "", fmt.Errorf("failed to unmarshal response body: %w", err) + } + + log.Info().Msgf("Batch of %d file(s) applied successfully via repository server. Commit: %s", len(batch.Files), response.Commit) + return response.Commit, nil +} diff --git a/internal/patch/method_server_test.go b/internal/patch/method_server_test.go new file mode 100644 index 0000000..d66bed8 --- /dev/null +++ b/internal/patch/method_server_test.go @@ -0,0 +1,134 @@ +package patch + +import ( + "encoding/json" + "io" + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" +) + +type recordedRequest struct { + Method string + Path string + ApiKey string + ContentType string + Body []byte +} + +// newRecordingServer starts a test server that records the incoming request +// and answers with the given status code and body. +func newRecordingServer(t *testing.T, statusCode int, responseBody string) (*httptest.Server, *recordedRequest) { + recorded := &recordedRequest{} + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + body, err := io.ReadAll(r.Body) + assert.NoError(t, err) + + recorded.Method = r.Method + recorded.Path = r.URL.Path + recorded.ApiKey = r.Header.Get("X-API-Key") + recorded.ContentType = r.Header.Get("Content-Type") + recorded.Body = body + + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(statusCode) + _, err = w.Write([]byte(responseBody)) + assert.NoError(t, err) + })) + t.Cleanup(server.Close) + + return server, recorded +} + +func TestRepositoryServerPatcherPatchBatch(t *testing.T) { + server, recorded := newRecordingServer(t, http.StatusOK, `{"message":"ok","commit":"abc123"}`) + + patcher := &RepositoryServerPatcher{ + RepositoryServerURL: server.URL, + RepositoryServerApiKey: "test-api-key", + } + + batch := PatchBatch{ + Actor: "ci-bot", + Files: []FilePatch{ + {FilePath: "applications/dev/service-a/values.yaml", Patches: []Patch{{Selector: ".service.image.tag", Value: "v1.0.0"}}}, + {FilePath: "applications/dev/service-b/values.yaml", Patches: []Patch{{Selector: ".service.image.tag", Value: "v1.0.0"}}}, + }, + } + + commit, err := patcher.PatchBatch(batch) + assert.NoError(t, err) + assert.Equal(t, "abc123", commit) + + assert.Equal(t, http.MethodPut, recorded.Method) + assert.Equal(t, "/patches", recorded.Path) + assert.Equal(t, "test-api-key", recorded.ApiKey) + assert.Equal(t, "application/json", recorded.ContentType) + + var sentBatch PatchBatch + err = json.Unmarshal(recorded.Body, &sentBatch) + assert.NoError(t, err) + assert.Equal(t, "ci-bot", sentBatch.Actor) + assert.Len(t, sentBatch.Files, 2, "all files of the batch must be sent") + assert.Equal(t, "applications/dev/service-a/values.yaml", sentBatch.Files[0].FilePath) + assert.Equal(t, "applications/dev/service-b/values.yaml", sentBatch.Files[1].FilePath) + assert.Equal(t, ".service.image.tag", sentBatch.Files[1].Patches[0].Selector) +} + +func TestRepositoryServerPatcherPatchBatchNoFiles(t *testing.T) { + server, recorded := newRecordingServer(t, http.StatusOK, `{"message":"ok","commit":"abc123"}`) + + patcher := &RepositoryServerPatcher{ + RepositoryServerURL: server.URL, + RepositoryServerApiKey: "test-api-key", + } + + commit, err := patcher.PatchBatch(PatchBatch{}) + assert.NoError(t, err) + assert.Empty(t, commit) + assert.Empty(t, recorded.Method, "no request must be sent for an empty batch") +} + +func TestRepositoryServerPatcherPatchBatchServerError(t *testing.T) { + server, _ := newRecordingServer(t, http.StatusInternalServerError, `{"error":"error executing patching"}`) + + patcher := &RepositoryServerPatcher{ + RepositoryServerURL: server.URL, + RepositoryServerApiKey: "test-api-key", + } + + commit, err := patcher.PatchBatch(PatchBatch{ + Files: []FilePatch{{FilePath: "applications/dev/service-a/values.yaml", Patches: []Patch{{Selector: ".service.image.tag", Value: "v1.0.0"}}}}, + }) + assert.Error(t, err) + assert.Empty(t, commit) + assert.Contains(t, err.Error(), "500") + assert.Contains(t, err.Error(), "error executing patching") +} + +func TestRepositoryServerPatcherPatchUsesSingleFileEndpoint(t *testing.T) { + server, recorded := newRecordingServer(t, http.StatusOK, `{"message":"ok"}`) + + patcher := &RepositoryServerPatcher{ + RepositoryServerURL: server.URL, + RepositoryServerApiKey: "test-api-key", + } + + err := patcher.Patch([]PatchTask{{ + Actor: "ci-bot", + FilePath: "applications/dev/service-a/values.yaml", + Patches: []Patch{{Selector: ".service.image.tag", Value: "v1.0.0"}}, + }}) + assert.NoError(t, err) + + assert.Equal(t, http.MethodPut, recorded.Method) + assert.Equal(t, "/patch", recorded.Path) + + var sentTask PatchTask + err = json.Unmarshal(recorded.Body, &sentTask) + assert.NoError(t, err) + assert.Equal(t, "applications/dev/service-a/values.yaml", sentTask.FilePath) +} diff --git a/internal/patch/patch.go b/internal/patch/patch.go index 790e28e..78c0cb9 100644 --- a/internal/patch/patch.go +++ b/internal/patch/patch.go @@ -2,6 +2,9 @@ package patch import ( "errors" + "fmt" + "path/filepath" + "strings" log "github.com/rs/zerolog/log" @@ -19,6 +22,19 @@ type Patch struct { Value string `json:"value"` } +// FilePatch describes all patches to be applied to a single file. +type FilePatch struct { + FilePath string `json:"filePath"` + Patches []Patch `json:"patches"` +} + +// PatchBatch describes patches for multiple files that are applied and +// committed atomically. +type PatchBatch struct { + Actor string `json:"actor"` + Files []FilePatch `json:"files"` +} + type PrepareOptions struct { Clone bool } @@ -26,6 +42,73 @@ type PrepareOptions struct { type PatchMethod interface { Prepare(options *PrepareOptions) error Patch(patchTasks []PatchTask) error + // PatchBatch applies all patches of the batch in a single commit and + // returns the commit id. An empty commit id is returned if the batch did + // not result in any change. + PatchBatch(batch PatchBatch) (string, error) +} + +// ErrInvalidPatchBatch wraps all validation errors of a patch batch so callers +// can distinguish invalid input from execution failures. +var ErrInvalidPatchBatch = errors.New("invalid patch batch") + +// cleanRelativeFilePath validates a file path of a patch request and returns +// its cleaned, repository relative form. +func cleanRelativeFilePath(filePath string) (string, error) { + if strings.TrimSpace(filePath) == "" { + return "", errors.New("must not be empty") + } + + if filepath.IsAbs(filePath) || strings.HasPrefix(filePath, "/") || strings.HasPrefix(filePath, "\\") { + return "", errors.New("must be relative to the repository root") + } + + // paths are passed to `git add` without a `--` separator, so a leading + // dash would be interpreted as an option + if strings.HasPrefix(filePath, "-") { + return "", errors.New("must not start with '-'") + } + + cleanedFilePath := filepath.Clean(filePath) + if cleanedFilePath == "." || cleanedFilePath == ".." || strings.HasPrefix(cleanedFilePath, ".."+string(filepath.Separator)) { + return "", errors.New("must not escape the repository root") + } + + return cleanedFilePath, nil +} + +// ValidatePatchBatch checks a patch batch for structural errors. All returned +// errors wrap ErrInvalidPatchBatch. +func ValidatePatchBatch(batch PatchBatch) error { + if len(batch.Files) == 0 { + return fmt.Errorf("%w: files: must not be empty", ErrInvalidPatchBatch) + } + + seenFilePaths := map[string]int{} + + for fileIndex, file := range batch.Files { + cleanedFilePath, err := cleanRelativeFilePath(file.FilePath) + if err != nil { + return fmt.Errorf("%w: files[%d].filePath: %s", ErrInvalidPatchBatch, fileIndex, err.Error()) + } + + if previousIndex, ok := seenFilePaths[cleanedFilePath]; ok { + return fmt.Errorf("%w: files[%d].filePath: duplicate of files[%d].filePath ('%s')", ErrInvalidPatchBatch, fileIndex, previousIndex, cleanedFilePath) + } + seenFilePaths[cleanedFilePath] = fileIndex + + if len(file.Patches) == 0 { + return fmt.Errorf("%w: files[%d].patches: must not be empty", ErrInvalidPatchBatch, fileIndex) + } + + for patchIndex, filePatch := range file.Patches { + if strings.TrimSpace(filePatch.Selector) == "" { + return fmt.Errorf("%w: files[%d].patches[%d].selector: must not be empty", ErrInvalidPatchBatch, fileIndex, patchIndex) + } + } + } + + return nil } func PatchCommand(c *cli.Context) error { diff --git a/internal/patch/patch_test.go b/internal/patch/patch_test.go new file mode 100644 index 0000000..52ffc6b --- /dev/null +++ b/internal/patch/patch_test.go @@ -0,0 +1,158 @@ +package patch + +import ( + "errors" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestCleanRelativeFilePath(t *testing.T) { + testCases := []struct { + name string + filePath string + expected string + expectError bool + }{ + {name: "plain relative path", filePath: "applications/dev/service-foo/values.yaml", expected: "applications/dev/service-foo/values.yaml"}, + {name: "path is cleaned", filePath: "applications/./dev/../dev/values.yaml", expected: "applications/dev/values.yaml"}, + {name: "traversal inside the repository is allowed", filePath: "a/../b.yaml", expected: "b.yaml"}, + {name: "empty path", filePath: "", expectError: true}, + {name: "blank path", filePath: " ", expectError: true}, + {name: "absolute path", filePath: "/etc/passwd", expectError: true}, + {name: "parent traversal", filePath: "../values.yaml", expectError: true}, + {name: "nested parent traversal", filePath: "a/../../values.yaml", expectError: true}, + {name: "leading dash", filePath: "-flag.yaml", expectError: true}, + {name: "current directory", filePath: ".", expectError: true}, + } + + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + cleanedFilePath, err := cleanRelativeFilePath(testCase.filePath) + if testCase.expectError { + assert.Error(t, err) + return + } + assert.NoError(t, err) + assert.Equal(t, testCase.expected, cleanedFilePath) + }) + } +} + +func TestValidatePatchBatch(t *testing.T) { + validPatches := []Patch{{Selector: ".service.image.tag", Value: "v1.0.0"}} + + testCases := []struct { + name string + batch PatchBatch + expectError bool + }{ + { + name: "valid batch with two files", + batch: PatchBatch{ + Actor: "ci-bot", + Files: []FilePatch{ + {FilePath: "applications/dev/service-a/values.yaml", Patches: validPatches}, + {FilePath: "applications/dev/service-b/values.yaml", Patches: validPatches}, + }, + }, + }, + { + name: "valid batch without actor", + batch: PatchBatch{ + Files: []FilePatch{{FilePath: "applications/dev/service-a/values.yaml", Patches: validPatches}}, + }, + }, + { + name: "no files", + batch: PatchBatch{Files: []FilePatch{}}, + expectError: true, + }, + { + name: "nil files", + batch: PatchBatch{}, + expectError: true, + }, + { + name: "empty file path", + batch: PatchBatch{ + Files: []FilePatch{{FilePath: "", Patches: validPatches}}, + }, + expectError: true, + }, + { + name: "absolute file path", + batch: PatchBatch{ + Files: []FilePatch{{FilePath: "/etc/passwd", Patches: validPatches}}, + }, + expectError: true, + }, + { + name: "traversing file path", + batch: PatchBatch{ + Files: []FilePatch{{FilePath: "../values.yaml", Patches: validPatches}}, + }, + expectError: true, + }, + { + name: "nested traversing file path", + batch: PatchBatch{ + Files: []FilePatch{{FilePath: "a/../../values.yaml", Patches: validPatches}}, + }, + expectError: true, + }, + { + name: "file path starting with a dash", + batch: PatchBatch{ + Files: []FilePatch{{FilePath: "-flag.yaml", Patches: validPatches}}, + }, + expectError: true, + }, + { + name: "duplicate file paths", + batch: PatchBatch{ + Files: []FilePatch{ + {FilePath: "a/./b.yaml", Patches: validPatches}, + {FilePath: "a/b.yaml", Patches: validPatches}, + }, + }, + expectError: true, + }, + { + name: "file without patches", + batch: PatchBatch{ + Files: []FilePatch{{FilePath: "applications/dev/service-a/values.yaml", Patches: []Patch{}}}, + }, + expectError: true, + }, + { + name: "patch without selector", + batch: PatchBatch{ + Files: []FilePatch{{FilePath: "applications/dev/service-a/values.yaml", Patches: []Patch{{Selector: "", Value: "v1.0.0"}}}}, + }, + expectError: true, + }, + { + name: "second file is invalid", + batch: PatchBatch{ + Files: []FilePatch{ + {FilePath: "applications/dev/service-a/values.yaml", Patches: validPatches}, + {FilePath: "../escape.yaml", Patches: validPatches}, + }, + }, + expectError: true, + }, + } + + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + err := ValidatePatchBatch(testCase.batch) + if testCase.expectError { + assert.Error(t, err) + assert.True(t, errors.Is(err, ErrInvalidPatchBatch), "error should wrap ErrInvalidPatchBatch") + return + } + assert.NoError(t, err) + }) + } +} diff --git a/internal/repo_server/server/patch_controller.go b/internal/repo_server/server/patch_controller.go index 7e19f0f..4ecfe64 100644 --- a/internal/repo_server/server/patch_controller.go +++ b/internal/repo_server/server/patch_controller.go @@ -1,8 +1,6 @@ package server import ( - "sync" - "github.com/gin-gonic/gin" "github.com/mxcd/gitops-cli/internal/patch" ) @@ -13,8 +11,6 @@ func (s *Server) registerPatchRoute() error { } func (s *Server) getPatchHandler() gin.HandlerFunc { - lock := sync.Mutex{} - return func(c *gin.Context) { var input patch.PatchTask if err := c.ShouldBindJSON(&input); err != nil { @@ -22,8 +18,8 @@ func (s *Server) getPatchHandler() gin.HandlerFunc { return } - lock.Lock() - defer lock.Unlock() + s.patchLock.Lock() + defer s.patchLock.Unlock() err := s.GitPatcher.Patch([]patch.PatchTask{input}) if err != nil { diff --git a/internal/repo_server/server/patches_controller.go b/internal/repo_server/server/patches_controller.go new file mode 100644 index 0000000..09cc36d --- /dev/null +++ b/internal/repo_server/server/patches_controller.go @@ -0,0 +1,47 @@ +package server + +import ( + "errors" + + "github.com/gin-gonic/gin" + "github.com/mxcd/gitops-cli/internal/patch" + "github.com/rs/zerolog/log" +) + +func (s *Server) registerPatchesRoute() error { + s.Engine.PUT(s.Options.ApiBaseUrl+"/patches", s.getPatchesHandler()) + return nil +} + +func (s *Server) getPatchesHandler() gin.HandlerFunc { + return func(c *gin.Context) { + var input patch.PatchBatch + if err := c.ShouldBindJSON(&input); err != nil { + c.JSON(400, gin.H{"error": "invalid input"}) + return + } + + // validate before acquiring the lock so invalid requests do not block + // concurrent patches + if err := patch.ValidatePatchBatch(input); err != nil { + c.JSON(400, gin.H{"error": err.Error()}) + return + } + + s.patchLock.Lock() + defer s.patchLock.Unlock() + + commitHash, err := s.GitPatcher.PatchBatch(input) + if err != nil { + if errors.Is(err, patch.ErrInvalidPatchBatch) { + c.JSON(400, gin.H{"error": err.Error()}) + return + } + log.Error().Err(err).Msg("Error executing batch patching") + c.JSON(500, gin.H{"error": "error executing patching"}) + return + } + + c.JSON(200, gin.H{"message": "ok", "commit": commitHash}) + } +} diff --git a/internal/repo_server/server/patches_controller_test.go b/internal/repo_server/server/patches_controller_test.go new file mode 100644 index 0000000..bd44e58 --- /dev/null +++ b/internal/repo_server/server/patches_controller_test.go @@ -0,0 +1,217 @@ +package server + +import ( + "encoding/json" + "errors" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/gin-gonic/gin" + "github.com/mxcd/gitops-cli/internal/patch" + "github.com/stretchr/testify/assert" +) + +func init() { + gin.SetMode(gin.TestMode) +} + +// fakePatcher records the calls it receives and returns preconfigured results. +type fakePatcher struct { + PatchCalls [][]patch.PatchTask + PatchBatchCalls []patch.PatchBatch + CommitHash string + Err error +} + +func (f *fakePatcher) Prepare(options *patch.PrepareOptions) error { + return nil +} + +func (f *fakePatcher) Patch(patchTasks []patch.PatchTask) error { + f.PatchCalls = append(f.PatchCalls, patchTasks) + return f.Err +} + +func (f *fakePatcher) PatchBatch(batch patch.PatchBatch) (string, error) { + f.PatchBatchCalls = append(f.PatchBatchCalls, batch) + if f.Err != nil { + return "", f.Err + } + return f.CommitHash, nil +} + +func newTestServer(t *testing.T, patcher patch.PatchMethod) *Server { + server, err := NewServer(&RouterOptions{ + DevMode: true, + Port: 0, + ApiBaseUrl: "/api/v1", + ApiKeys: []string{"test-api-key"}, + }, patcher) + assert.NoError(t, err) + assert.NotNil(t, server) + + server.RegisterMiddlewares() + err = server.RegisterRoutes() + assert.NoError(t, err) + + return server +} + +// executeRequest sends a request to the server and returns the recorder. +func executeRequest(t *testing.T, server *Server, method string, url string, body string, apiKey string) *httptest.ResponseRecorder { + req, err := http.NewRequest(method, url, strings.NewReader(body)) + assert.NoError(t, err) + req.Header.Set("Content-Type", "application/json") + if apiKey != "" { + req.Header.Set("X-API-Key", apiKey) + } + + recorder := httptest.NewRecorder() + server.Engine.ServeHTTP(recorder, req) + + return recorder +} + +const validBatchBody = `{ + "actor": "ci-bot", + "files": [ + {"filePath": "applications/dev/service-a/values.yaml", "patches": [{"selector": ".service.image.tag", "value": "v1.0.0"}]}, + {"filePath": "applications/dev/service-b/values.yaml", "patches": [{"selector": ".service.image.tag", "value": "v1.0.0"}]} + ] +}` + +func TestPatchesHandlerSuccess(t *testing.T) { + patcher := &fakePatcher{CommitHash: "abc123"} + server := newTestServer(t, patcher) + + recorder := executeRequest(t, server, http.MethodPut, "/api/v1/patches", validBatchBody, "test-api-key") + assert.Equal(t, 200, recorder.Code) + + var response map[string]any + err := json.Unmarshal(recorder.Body.Bytes(), &response) + assert.NoError(t, err) + assert.Equal(t, "ok", response["message"]) + assert.Equal(t, "abc123", response["commit"]) + + assert.Len(t, patcher.PatchBatchCalls, 1) + batch := patcher.PatchBatchCalls[0] + assert.Equal(t, "ci-bot", batch.Actor) + assert.Len(t, batch.Files, 2) + assert.Equal(t, "applications/dev/service-a/values.yaml", batch.Files[0].FilePath) + assert.Equal(t, "applications/dev/service-b/values.yaml", batch.Files[1].FilePath) + assert.Equal(t, ".service.image.tag", batch.Files[1].Patches[0].Selector) +} + +func TestPatchesHandlerNoChanges(t *testing.T) { + patcher := &fakePatcher{CommitHash: ""} + server := newTestServer(t, patcher) + + recorder := executeRequest(t, server, http.MethodPut, "/api/v1/patches", validBatchBody, "test-api-key") + assert.Equal(t, 200, recorder.Code) + + var response map[string]any + err := json.Unmarshal(recorder.Body.Bytes(), &response) + assert.NoError(t, err) + assert.Equal(t, "ok", response["message"]) + assert.Equal(t, "", response["commit"]) +} + +func TestPatchesHandlerInvalidRequests(t *testing.T) { + testCases := []struct { + name string + body string + expectedError string + }{ + {name: "invalid json", body: `{"files": [`, expectedError: "invalid input"}, + {name: "empty files", body: `{"files": []}`, expectedError: "files"}, + {name: "missing files", body: `{"actor": "ci-bot"}`, expectedError: "files"}, + {name: "traversing file path", body: `{"files": [{"filePath": "../escape.yaml", "patches": [{"selector": ".a", "value": "b"}]}]}`, expectedError: "escape the repository root"}, + {name: "absolute file path", body: `{"files": [{"filePath": "/etc/passwd", "patches": [{"selector": ".a", "value": "b"}]}]}`, expectedError: "relative to the repository root"}, + {name: "file without patches", body: `{"files": [{"filePath": "a/values.yaml", "patches": []}]}`, expectedError: "patches"}, + {name: "patch without selector", body: `{"files": [{"filePath": "a/values.yaml", "patches": [{"selector": "", "value": "b"}]}]}`, expectedError: "selector"}, + { + name: "duplicate file paths", + body: `{"files": [{"filePath": "a/values.yaml", "patches": [{"selector": ".a", "value": "b"}]}, {"filePath": "a/./values.yaml", "patches": [{"selector": ".a", "value": "b"}]}]}`, + expectedError: "duplicate", + }, + } + + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + patcher := &fakePatcher{CommitHash: "abc123"} + server := newTestServer(t, patcher) + + recorder := executeRequest(t, server, http.MethodPut, "/api/v1/patches", testCase.body, "test-api-key") + assert.Equal(t, 400, recorder.Code) + assert.Contains(t, recorder.Body.String(), testCase.expectedError) + assert.Empty(t, patcher.PatchBatchCalls, "the patcher must not be called for invalid input") + }) + } +} + +func TestPatchesHandlerPatcherError(t *testing.T) { + patcher := &fakePatcher{Err: errors.New("git exploded")} + server := newTestServer(t, patcher) + + recorder := executeRequest(t, server, http.MethodPut, "/api/v1/patches", validBatchBody, "test-api-key") + assert.Equal(t, 500, recorder.Code) + assert.Contains(t, recorder.Body.String(), "error executing patching") + // the internal error must not leak to the client + assert.NotContains(t, recorder.Body.String(), "git exploded") + assert.Len(t, patcher.PatchBatchCalls, 1) +} + +func TestPatchesHandlerPatcherValidationError(t *testing.T) { + patcher := &fakePatcher{Err: patch.ErrInvalidPatchBatch} + server := newTestServer(t, patcher) + + recorder := executeRequest(t, server, http.MethodPut, "/api/v1/patches", validBatchBody, "test-api-key") + assert.Equal(t, 400, recorder.Code) + assert.Contains(t, recorder.Body.String(), "invalid patch batch") +} + +func TestPatchesHandlerAuthentication(t *testing.T) { + testCases := []struct { + name string + apiKey string + }{ + {name: "no api key", apiKey: ""}, + {name: "wrong api key", apiKey: "wrong-api-key"}, + } + + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + patcher := &fakePatcher{CommitHash: "abc123"} + server := newTestServer(t, patcher) + + recorder := executeRequest(t, server, http.MethodPut, "/api/v1/patches", validBatchBody, testCase.apiKey) + assert.Equal(t, 401, recorder.Code) + assert.Empty(t, patcher.PatchBatchCalls) + }) + } +} + +func TestPatchRouteStillWorks(t *testing.T) { + patcher := &fakePatcher{} + server := newTestServer(t, patcher) + + body := `{"actor": "ci-bot", "filePath": "applications/dev/service-a/values.yaml", "patches": [{"selector": ".service.image.tag", "value": "v1.0.0"}]}` + recorder := executeRequest(t, server, http.MethodPut, "/api/v1/patch", body, "test-api-key") + assert.Equal(t, 200, recorder.Code) + assert.Contains(t, recorder.Body.String(), `"message":"ok"`) + + assert.Len(t, patcher.PatchCalls, 1) + assert.Len(t, patcher.PatchCalls[0], 1) + assert.Equal(t, "applications/dev/service-a/values.yaml", patcher.PatchCalls[0][0].FilePath) + assert.Empty(t, patcher.PatchBatchCalls) +} + +func TestHealthRouteIsUnprotected(t *testing.T) { + server := newTestServer(t, &fakePatcher{}) + + recorder := executeRequest(t, server, http.MethodGet, "/api/v1/health", "", "") + assert.Equal(t, 200, recorder.Code) + assert.Contains(t, recorder.Body.String(), `"status":"ok"`) +} diff --git a/internal/repo_server/server/server.go b/internal/repo_server/server/server.go index 2ad4b6e..f68d7ed 100644 --- a/internal/repo_server/server/server.go +++ b/internal/repo_server/server/server.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "net/http" + "sync" "github.com/gin-gonic/gin" "github.com/mxcd/gitops-cli/internal/patch" @@ -20,10 +21,14 @@ type Server struct { Engine *gin.Engine HttpServer *http.Server Options *RouterOptions - GitPatcher *patch.GitPatcher + GitPatcher patch.PatchMethod + + // patchLock serializes all git operations of the patch endpoints against + // each other + patchLock sync.Mutex } -func NewServer(options *RouterOptions, gitPatcher *patch.GitPatcher) (*Server, error) { +func NewServer(options *RouterOptions, gitPatcher patch.PatchMethod) (*Server, error) { if !options.DevMode { gin.SetMode(gin.ReleaseMode) } @@ -55,6 +60,7 @@ func (s *Server) RegisterMiddlewares() { func (s *Server) RegisterRoutes() error { s.registerHealthRoute() s.registerPatchRoute() + s.registerPatchesRoute() return nil } diff --git a/test/bruno/patch multiple files.bru b/test/bruno/patch multiple files.bru new file mode 100644 index 0000000..8dd4630 --- /dev/null +++ b/test/bruno/patch multiple files.bru @@ -0,0 +1,43 @@ +meta { + name: patch multiple files + type: http + seq: 4 +} + +put { + url: http://localhost:8080/api/v1/patches + body: json + auth: apikey +} + +auth:apikey { + key: X-API-Key + value: test + placement: header +} + +body:json { + { + "actor": "ci-bot", + "files": [ + { + "filePath": "applications/dev/service-foo/values.yaml", + "patches": [ + { + "selector": ".service.image.tag", + "value": "v1.1.1" + } + ] + }, + { + "filePath": "applications/dev/service-bar/values.yaml", + "patches": [ + { + "selector": ".service.image.tag", + "value": "v1.1.1" + } + ] + } + ] + } +} From a31f22e34a27ac32670872c299ce15e46df5082b Mon Sep 17 00:00:00 2001 From: Luca Voges Date: Wed, 9 Sep 2026 10:53:52 +0200 Subject: [PATCH 2/2] fix(patch): harden batch validation and failure recovery - reject leading-dash paths after cleaning (./-n.yaml) - restore from HEAD so staged changes of a failed commit are dropped - push a pending local commit when a retried batch changes nothing - validate batches client-side before sending --- internal/git/git_test.go | 43 ++++++++++++++++++++++++++++ internal/git/restore.go | 10 ++++--- internal/patch/method_git.go | 32 +++++++++++++++++++-- internal/patch/method_git_test.go | 35 ++++++++++++++++++++++ internal/patch/method_server.go | 6 ++-- internal/patch/method_server_test.go | 4 +-- internal/patch/patch.go | 13 +++++---- internal/patch/patch_test.go | 2 ++ 8 files changed, 128 insertions(+), 17 deletions(-) diff --git a/internal/git/git_test.go b/internal/git/git_test.go index 936afe2..97013fe 100644 --- a/internal/git/git_test.go +++ b/internal/git/git_test.go @@ -7,6 +7,9 @@ import ( "testing" "github.com/google/uuid" + "github.com/ldez/go-git-cmd-wrapper/v2/add" + "github.com/ldez/go-git-cmd-wrapper/v2/git" + "github.com/ldez/go-git-cmd-wrapper/v2/types" "github.com/mxcd/gitops-cli/internal/util" "github.com/stretchr/testify/assert" ) @@ -225,6 +228,46 @@ func TestGitRestore(t *testing.T) { assert.Equal(t, string(originalContents), string(restoredContents)) } +func TestGitRestoreStagedChanges(t *testing.T) { + + tempConnection := cloneTempRepository(t) + assert.NotNil(t, tempConnection) + + relativeFilePath := path.Join("applications", "dev", "service-test", "values.yaml") + absoluteFilePath := path.Join(tempConnection.Options.Directory, relativeFilePath) + + originalContents, err := os.ReadFile(absoluteFilePath) + assert.NoError(t, err) + + // simulate a commit that failed after `git add` already staged the change + err = os.WriteFile(absoluteFilePath, []byte("clobbered: true\n"), 0644) + assert.NoError(t, err) + _, err = git.Add(runGitIn(tempConnection.Options.Directory), add.PathSpec(relativeFilePath)) + assert.NoError(t, err) + assert.True(t, hasStagedChanges(t, tempConnection)) + + err = tempConnection.Restore([]string{relativeFilePath}) + assert.NoError(t, err) + + assert.False(t, hasStagedChanges(t, tempConnection)) + hasChanges, err := tempConnection.HasChanges() + assert.NoError(t, err) + assert.False(t, hasChanges) + + restoredContents, err := os.ReadFile(absoluteFilePath) + assert.NoError(t, err) + assert.Equal(t, string(originalContents), string(restoredContents)) +} + +// hasStagedChanges reports whether the index differs from HEAD. +func hasStagedChanges(t *testing.T, connection *Connection) bool { + _, err := git.Raw("diff", runGitIn(connection.Options.Directory), func(g *types.Cmd) { + g.AddOptions("--cached") + g.AddOptions("--quiet") + }) + return err != nil +} + func TestGitCommitFiles(t *testing.T) { tempConnection := cloneTempRepository(t) diff --git a/internal/git/restore.go b/internal/git/restore.go index 2011a64..6e7cce2 100644 --- a/internal/git/restore.go +++ b/internal/git/restore.go @@ -8,10 +8,11 @@ import ( "github.com/rs/zerolog/log" ) -// Restore discards working tree changes of the given repository relative file -// paths by running `git checkout -- `. It intentionally does not touch -// commits, so a local commit whose push failed is preserved and pushed by a -// subsequent pull/push cycle. +// Restore discards working tree and index changes of the given repository +// relative file paths by running `git checkout HEAD -- `. Restoring from +// HEAD instead of the index also discards changes that were already staged by a +// failed commit. It intentionally does not touch commits, so a local commit +// whose push failed is preserved and pushed by a subsequent pull/push cycle. func (c *Connection) Restore(files []string) error { directory := c.Options.Directory if directory == "" { @@ -23,6 +24,7 @@ func (c *Connection) Restore(files []string) error { } msg, err := git.Raw("checkout", runGitIn(directory), func(g *types.Cmd) { + g.AddOptions("HEAD") g.AddOptions("--") for _, file := range files { g.AddOptions(file) diff --git a/internal/patch/method_git.go b/internal/patch/method_git.go index 84fda36..983ed4e 100644 --- a/internal/patch/method_git.go +++ b/internal/patch/method_git.go @@ -278,6 +278,35 @@ func (p *GitPatcher) Patch(patchTasks []PatchTask) error { return p.pushWithRetry() } +// pushPendingCommit handles a batch that did not change the working tree. If a +// previous batch was committed but its push failed, the retried batch finds +// nothing to change, so the pending local commit is pushed here and its id is +// returned. Otherwise an empty id is returned. +func (p *GitPatcher) pushPendingCommit() (string, error) { + head, err := p.GitConnection.RevParse("HEAD") + if err != nil { + return "", err + } + + upstream, err := p.GitConnection.RevParse("origin/" + p.GitConnection.Options.Branch) + if err != nil { + return "", err + } + + if head == upstream { + log.Info().Msg("No changes detected, nothing to commit") + return "", nil + } + + log.Info().Msgf("No changes detected, but local commit %s has not been pushed yet, pushing", head) + if err := p.pushWithRetry(); err != nil { + log.Error().Err(err).Msgf("Failed to push pending commit %s, it remains local and is retried on the next patch", head) + return "", err + } + + return head, nil +} + // PatchBatch applies all patches of the batch and commits them as a single // atomic commit. It returns the commit id, or an empty string if the batch did // not change anything. @@ -328,8 +357,7 @@ func (p *GitPatcher) PatchBatch(batch PatchBatch) (hash string, err error) { } if !hasChanges { - log.Info().Msg("No changes detected, nothing to commit") - return "", nil + return p.pushPendingCommit() } log.Debug().Msg("Changes detected, committing") diff --git a/internal/patch/method_git_test.go b/internal/patch/method_git_test.go index 94e34d0..17fa5fe 100644 --- a/internal/patch/method_git_test.go +++ b/internal/patch/method_git_test.go @@ -199,6 +199,41 @@ func TestGitSshPatchBatchNoChanges(t *testing.T) { assert.Empty(t, commitHash) } +func TestGitSshPatchBatchPushesPendingLocalCommit(t *testing.T) { + patcher := newTestPatcher(t) + + relativeFilePath := seedFixtureFile(t, patcher) + + batch := PatchBatch{ + Files: []FilePatch{ + {FilePath: relativeFilePath, Patches: []Patch{{Selector: ".service.image.tag", Value: "v9.0.0"}}}, + }, + } + + // simulate a batch whose push failed: the change is committed locally only + preparedFile, err := patcher.preparePatchedFile(batch.Files[0]) + assert.NoError(t, err) + assert.NoError(t, patcher.writePatchedFile(preparedFile)) + localCommitHash, err := patcher.GitConnection.Commit([]string{relativeFilePath}, "local only") + assert.NoError(t, err) + + // retrying the same batch finds nothing to change but must push the pending commit + commitHash, err := patcher.PatchBatch(batch) + assert.NoError(t, err) + assert.Equal(t, localCommitHash, commitHash) + + verificationPatcher := newTestPatcher(t) + resolvedCommitHash, err := verificationPatcher.GitConnection.RevParse(commitHash) + assert.NoError(t, err) + assert.Equal(t, commitHash, resolvedCommitHash) + assert.Contains(t, readRepositoryFile(t, verificationPatcher.GitConnection, relativeFilePath), "tag: v9.0.0") + + // a further retry has nothing left to push + commitHash, err = patcher.PatchBatch(batch) + assert.NoError(t, err) + assert.Empty(t, commitHash) +} + func TestGitSshPatchBatchMissingFile(t *testing.T) { patcher := newTestPatcher(t) diff --git a/internal/patch/method_server.go b/internal/patch/method_server.go index 6067bb6..2809cf5 100644 --- a/internal/patch/method_server.go +++ b/internal/patch/method_server.go @@ -114,9 +114,9 @@ func (p *RepositoryServerPatcher) Patch(patchTasks []PatchTask) error { // PatchBatch sends all files of the batch to the repository server so they are // applied in a single commit. It returns the commit id reported by the server. func (p *RepositoryServerPatcher) PatchBatch(batch PatchBatch) (string, error) { - if len(batch.Files) == 0 { - log.Warn().Msg("No files provided, skipping patching") - return "", nil + // validate locally so an obviously invalid batch never reaches the server + if err := ValidatePatchBatch(batch); err != nil { + return "", err } body, err := p.doPut("/patches", batch) diff --git a/internal/patch/method_server_test.go b/internal/patch/method_server_test.go index d66bed8..c6c9767 100644 --- a/internal/patch/method_server_test.go +++ b/internal/patch/method_server_test.go @@ -87,9 +87,9 @@ func TestRepositoryServerPatcherPatchBatchNoFiles(t *testing.T) { } commit, err := patcher.PatchBatch(PatchBatch{}) - assert.NoError(t, err) + assert.ErrorIs(t, err, ErrInvalidPatchBatch) assert.Empty(t, commit) - assert.Empty(t, recorded.Method, "no request must be sent for an empty batch") + assert.Empty(t, recorded.Method, "no request must be sent for an invalid batch") } func TestRepositoryServerPatcherPatchBatchServerError(t *testing.T) { diff --git a/internal/patch/patch.go b/internal/patch/patch.go index 78c0cb9..0757ae2 100644 --- a/internal/patch/patch.go +++ b/internal/patch/patch.go @@ -63,17 +63,18 @@ func cleanRelativeFilePath(filePath string) (string, error) { return "", errors.New("must be relative to the repository root") } - // paths are passed to `git add` without a `--` separator, so a leading - // dash would be interpreted as an option - if strings.HasPrefix(filePath, "-") { - return "", errors.New("must not start with '-'") - } - cleanedFilePath := filepath.Clean(filePath) if cleanedFilePath == "." || cleanedFilePath == ".." || strings.HasPrefix(cleanedFilePath, ".."+string(filepath.Separator)) { return "", errors.New("must not escape the repository root") } + // the cleaned path is passed to `git add` without a `--` separator, so a + // leading dash would be interpreted as an option. Check the cleaned path, + // because e.g. `./-n.yaml` cleans to `-n.yaml`. + if strings.HasPrefix(cleanedFilePath, "-") { + return "", errors.New("must not start with '-'") + } + return cleanedFilePath, nil } diff --git a/internal/patch/patch_test.go b/internal/patch/patch_test.go index 52ffc6b..fc05c78 100644 --- a/internal/patch/patch_test.go +++ b/internal/patch/patch_test.go @@ -23,6 +23,8 @@ func TestCleanRelativeFilePath(t *testing.T) { {name: "parent traversal", filePath: "../values.yaml", expectError: true}, {name: "nested parent traversal", filePath: "a/../../values.yaml", expectError: true}, {name: "leading dash", filePath: "-flag.yaml", expectError: true}, + {name: "leading dash hidden behind current directory", filePath: "./-flag.yaml", expectError: true}, + {name: "leading dash hidden behind parent traversal", filePath: "a/../-flag.yaml", expectError: true}, {name: "current directory", filePath: ".", expectError: true}, }