diff --git a/CLAUDE.md b/CLAUDE.md index b930f83..0e770cb 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -98,6 +98,9 @@ DATABASE_URL=postgres://... go test -tags integration ./internal/master/store/ without the other has caused a fix commit already. - **`` C# convention from global CLAUDE.md does not apply here** — this is Go. Use idiomatic GoDoc (`// FuncName does X.`). +- **The override file gets the owner of the existing override, else of the stack's first + compose file.** The agent runs as root and renames a new file into place; it chowns it + afterwards. A failure is a `warning:` line in the job output, never a failed job. - **Compose loads only one override file.** Of `compose.override.yml`, `compose.override.yaml`, `docker-compose.override.yml`, `docker-compose.override.yaml` only the first that exists is read. Hence the refusal described above. diff --git a/internal/agent/updater/compose.go b/internal/agent/updater/compose.go index 3d54470..30c337b 100644 --- a/internal/agent/updater/compose.go +++ b/internal/agent/updater/compose.go @@ -2,11 +2,15 @@ package updater import ( "context" + "errors" "fmt" "io" + "io/fs" "os" "os/exec" + "path/filepath" "strings" + "syscall" "time" "github.com/docker/docker/api/types/container" @@ -27,6 +31,26 @@ const ( // job's error message; the full output is in the job log. const errorOutputTail = 2 << 10 +// chown is os.Chown; tests replace it. +var chown = os.Chown + +// stackOwner returns the uid and gid the override file should have: those of +// the existing override if there is one, else those of the stack's compose file. +func stackOwner(overridePath, composeFile string) (uid, gid int, err error) { + fi, err := os.Stat(overridePath) + if errors.Is(err, fs.ErrNotExist) { + fi, err = os.Stat(composeFile) + } + if err != nil { + return 0, 0, err + } + st, ok := fi.Sys().(*syscall.Stat_t) + if !ok { + return 0, 0, errors.New("file owner is not available on this platform") + } + return int(st.Uid), int(st.Gid), nil +} + // tailWriter adapts a tailBuffer to io.Writer. type tailWriter struct{ b *tailBuffer } @@ -152,9 +176,19 @@ func (e *ComposeExecutor) Execute(ctx context.Context, job protocol.JobStart, ou } fmt.Fprintln(out, resolveNote(job.Ref, pinned)) + // The agent runs as root and WriteOverride renames a new file into place, + // so the override would end up root-owned. Hand it back to the stack's + // owner. A failure here is only a warning: the update itself is fine. + overridePath := filepath.Join(dir, OverrideName) + uid, gid, ownerErr := stackOwner(overridePath, files[0]) if _, err := WriteOverride(dir, service, pinned); err != nil { return "", err } + if ownerErr != nil { + fmt.Fprintf(out, "warning: could not read the owner of %s or %s to give it to %s: %v\n", overridePath, files[0], overridePath, ownerErr) + } else if err := chown(overridePath, uid, gid); err != nil { + fmt.Fprintf(out, "warning: could not give %s the owner %d:%d: %v\n", overridePath, uid, gid, err) + } base := composeBase(project, workingDir, files, envs) for _, step := range [][]string{{"pull", service}, {"up", "-d", service}} { diff --git a/internal/agent/updater/compose_test.go b/internal/agent/updater/compose_test.go index 0b9f9cb..0523d30 100644 --- a/internal/agent/updater/compose_test.go +++ b/internal/agent/updater/compose_test.go @@ -7,6 +7,7 @@ import ( "os" "path/filepath" "strings" + "syscall" "testing" "github.com/docker/docker/api/types/container" @@ -64,6 +65,48 @@ func (r *recorder) run(_ context.Context, dir string, out io.Writer, args ...str return nil } +// stubChown replaces chown for the test. Tests that use it must not run in parallel. +func stubChown(t *testing.T, f func(path string, uid, gid int) error) { + t.Helper() + orig := chown + chown = f + t.Cleanup(func() { chown = orig }) +} + +func TestStackOwner(t *testing.T) { + dir := t.TempDir() + override := filepath.Join(dir, OverrideName) + compose := filepath.Join(dir, "compose.yaml") + + t.Run("falls back to the compose file when there is no override", func(t *testing.T) { + require.NoError(t, os.WriteFile(compose, nil, 0o644)) + uid, gid, err := stackOwner(override, compose) + require.NoError(t, err) + assert.Equal(t, os.Getuid(), uid) + assert.Equal(t, os.Getgid(), gid) + }) + t.Run("prefers the existing override", func(t *testing.T) { + require.NoError(t, os.WriteFile(override, nil, 0o644)) + require.NoError(t, os.Remove(compose)) + uid, _, err := stackOwner(override, compose) + require.NoError(t, err, "the compose file is not consulted") + assert.Equal(t, os.Getuid(), uid) + }) + t.Run("fails when neither file exists", func(t *testing.T) { + require.NoError(t, os.Remove(override)) + _, _, err := stackOwner(override, compose) + assert.Error(t, err) + }) +} + +func TestChown_RealToOwnIDs(t *testing.T) { + f := filepath.Join(t.TempDir(), "f") + require.NoError(t, os.WriteFile(f, nil, 0o644)) + uid, gid, err := stackOwner(f, f) + require.NoError(t, err) + assert.NoError(t, chown(f, uid, gid)) +} + func TestComposeExecutor(t *testing.T) { const d = "sha256:d1d1d1d1d1d1d1d1d1d1d1d1d1d1d1d1d1d1d1d1d1d1d1d1d1d1d1d1d1d1d1d1" root, _ := filepath.EvalSymlinks(t.TempDir()) @@ -108,6 +151,43 @@ func TestComposeExecutor(t *testing.T) { assert.Equal(t, pinned, serviceImage(doc, "web")) assert.Contains(t, out.String(), "resolved "+pinned) }) + t.Run("the override is chowned to the owner of the compose file", func(t *testing.T) { + t.Cleanup(cleanOverride) + composeFile := filepath.Join(stack, "compose.yaml") + require.NoError(t, os.WriteFile(composeFile, []byte("services: {}\n"), 0o644)) + t.Cleanup(func() { os.Remove(composeFile) }) + fi, err := os.Stat(composeFile) + require.NoError(t, err) + st := fi.Sys().(*syscall.Stat_t) + var gotPath string + var gotUID, gotGID int + stubChown(t, func(path string, uid, gid int) error { + gotPath, gotUID, gotGID = path, uid, gid + return nil + }) + var out strings.Builder + _, err = setup(fakeCompose{rw, []container.Summary{ct}}, &fakeDist{digest: d}, &recorder{}).Execute(context.Background(), job, &out) + require.NoError(t, err) + assert.Equal(t, overridePath, gotPath) + assert.Equal(t, int(st.Uid), gotUID) + assert.Equal(t, int(st.Gid), gotGID) + assert.NotContains(t, out.String(), "warning") + }) + t.Run("a chown failure is a warning and the job still succeeds", func(t *testing.T) { + t.Cleanup(cleanOverride) + composeFile := filepath.Join(stack, "compose.yaml") + require.NoError(t, os.WriteFile(composeFile, []byte("services: {}\n"), 0o644)) + t.Cleanup(func() { os.Remove(composeFile) }) + stubChown(t, func(string, int, int) error { return errors.New("operation not permitted") }) + var out strings.Builder + got, err := setup(fakeCompose{rw, []container.Summary{ct}}, &fakeDist{digest: d}, &recorder{}).Execute(context.Background(), job, &out) + require.NoError(t, err) + assert.Equal(t, pinned, got) + assert.Contains(t, out.String(), "warning: could not give "+overridePath+" the owner ") + assert.Contains(t, out.String(), "operation not permitted") + _, doc := readOverride(t, stack) + assert.Equal(t, pinned, serviceImage(doc, "web")) + }) t.Run("a pinned ref is written verbatim and not resolved", func(t *testing.T) { t.Cleanup(cleanOverride) for _, ref := range []string{"gitea.shcizo.se/shcizo/slusk:1.3@" + d, "gitea.shcizo.se/shcizo/slusk@" + d} {