From a64fdf7c9a9089bb85092a0209b234ff3a1eb90f Mon Sep 17 00:00:00 2001 From: Nicolas De Loof Date: Fri, 28 Aug 2026 15:20:12 +0200 Subject: [PATCH] =?UTF-8?q?cli:=20one=20project-resolution=20story=20?= =?UTF-8?q?=E2=80=94=20single=20name=20precedence,=20no=20swallowed=20load?= =?UTF-8?q?=20error,=20strict=20service=20validation?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit projectOrName and toProjectName resolved the project with opposite precedences, and projectOrName silently swallowed any load error when COMPOSE_PROJECT_NAME was set: a broken compose file sent stop, down, ps... into label-based reconstruction without a word — even when the file was named explicitly with --file. One precedence now, documented on both resolvers and applied identically by compose-go while loading: --project-name, then COMPOSE_PROJECT_NAME, then the model's name. The failure policy becomes explicit: an unreadable explicit --file is a hard error; no file around with COMPOSE_PROJECT_NAME set stays the silent file-less workflow; a present-but-broken implicit file falls back to label-based mode with a warning. Service-name validation follows one rule — strict whenever a model is available: restart and wait no longer silently no-op on a typo (validateServiceNames, profile-disabled services remain legitimate targets), and the hand-rolled checks in ps and volumes are removed as dead code, the load-time selection already rejecting unknown names (pinned by test). Epic #14074, F.4. Rebased onto main, which since merged the jobs work (#14093, #14234): projectOrName's job-target detection (jobTargetErr) is restored ahead of the new explicit-file hard-error branch -- the file loaded fine here, only the target's selection failed -- and validateServiceNames now checks project.AllJobs() too, since restart/wait route their service arguments through it instead of projectOrName's own selection. docker-agent review: the "compose file found but could not be loaded" warning's suppression guard only matched errdefs.IsNotFoundError (compose-go's own ErrNotFound sentinel) -- a raw os.ErrNotExist (e.g. a nonexistent --project-directory) wasn't recognized and would have printed a misleading warning. Added errors.Is(err, os.ErrNotExist) as a fallback, and TestProjectOrNameResolution now asserts the warning's presence/absence in both directions instead of just the fallback name. Signed-off-by: Nicolas De Loof --- cmd/compose/compose.go | 62 ++++++- cmd/compose/project_resolution_test.go | 163 ++++++++++++++++++ cmd/compose/ps.go | 10 +- cmd/compose/restart.go | 3 + cmd/compose/volumes.go | 15 +- cmd/compose/wait.go | 5 +- pkg/e2e/jobs_test.go | 18 ++ .../TestRestartRefusesJob/compose.yaml | 10 ++ .../testdata/TestWaitRefusesJob/compose.yaml | 10 ++ 9 files changed, 276 insertions(+), 20 deletions(-) create mode 100644 cmd/compose/project_resolution_test.go create mode 100644 pkg/e2e/testdata/TestRestartRefusesJob/compose.yaml create mode 100644 pkg/e2e/testdata/TestWaitRefusesJob/compose.yaml diff --git a/cmd/compose/compose.go b/cmd/compose/compose.go index 5b39a335939..cc798f16900 100644 --- a/cmd/compose/compose.go +++ b/cmd/compose/compose.go @@ -31,6 +31,7 @@ import ( "github.com/compose-spec/compose-go/v2/cli" "github.com/compose-spec/compose-go/v2/dotenv" + "github.com/compose-spec/compose-go/v2/errdefs" "github.com/compose-spec/compose-go/v2/loader" composepaths "github.com/compose-spec/compose-go/v2/paths" "github.com/compose-spec/compose-go/v2/types" @@ -243,6 +244,24 @@ func defaultStringArrayVar(env string) []string { }) } +// projectOrName resolves the target project for commands that exploit the +// compose model when one is available and fall back to container labels +// otherwise. The project name follows one precedence everywhere, shared with +// toProjectName and applied identically by compose-go while loading: +// --project-name, then COMPOSE_PROJECT_NAME, then the model's name. +// +// When the model cannot be loaded: +// - a service argument naming a declared job is reported clearly instead, +// regardless of the cases below: the file loaded fine, only the +// target's selection failed, and every caller here treats a job as +// run-only; +// - an explicit --file is a hard error: the user named the file, failing +// to read it cannot be ignored; +// - no compose file around and a name available from COMPOSE_PROJECT_NAME +// is the normal file-less workflow: label-based mode, silently; +// - a compose file present but broken, with COMPOSE_PROJECT_NAME set, +// falls back to label-based mode with an explicit warning (this used to +// happen silently). func (o *ProjectOptions) projectOrName(ctx context.Context, dockerCli command.Cli, services ...string) (*types.Project, string, error) { name := o.ProjectName var project *types.Project @@ -260,12 +279,20 @@ func (o *ProjectOptions) projectOrName(ctx context.Context, dockerCli command.Cl // either the raw "no such service" below or, worse, silently // falling back to the label-driven project next -- a job that // was never run left no container behind for that fallback to - // find, so it would otherwise look like a successful no-op. + // find, so it would otherwise look like a successful no-op. This + // takes priority over the explicit-file hard error just below: + // the file loaded fine, only the target's selection failed. if jobErr, replaced := jobTargetErr(ctx, dockerCli, o, services, err); replaced { return nil, "", jobErr } + if len(o.ConfigPaths) > 0 { + return nil, "", err + } envProjectName := os.Getenv(ComposeProjectName) if envProjectName != "" { + if !errdefs.IsNotFoundError(err) && !errors.Is(err, os.ErrNotExist) { + logrus.Warnf("compose file found but could not be loaded (%s) — falling back to label-based mode for project %q", err, envProjectName) + } return nil, envProjectName, nil } return nil, "", err @@ -276,6 +303,39 @@ func (o *ProjectOptions) projectOrName(ctx context.Context, dockerCli command.Cl return project, name, nil } +// validateServiceNames rejects service arguments that don't exist in the +// loaded model — profile-disabled services are legitimate targets (commands +// like restart enable them on demand). With no model (label-based mode) no +// validation is possible: a name without containers cannot be told apart +// from a container already removed. +// +// Callers pass the unselected project (they call projectOrName with no +// services, precisely so this function can validate the full name instead — +// see runRestart/runWait), so project.AllJobs() already reflects every +// declared job without a second reload. +func validateServiceNames(project *types.Project, services []string) error { + if project == nil { + return nil + } + for _, service := range services { + if _, ok := project.Services[service]; ok { + continue + } + if _, ok := project.DisabledServices[service]; ok { + continue + } + if _, ok := project.AllJobs()[service]; ok { + return fmt.Errorf("job %q can only be triggered with \"docker compose run\"", service) + } + return fmt.Errorf("no such service: %s", service) + } + return nil +} + +// toProjectName resolves the project name for commands that only need the +// name, never the model. Same precedence as projectOrName: --project-name, +// then COMPOSE_PROJECT_NAME, then the loaded model's name — the two first +// short-circuit the load entirely. func (o *ProjectOptions) toProjectName(ctx context.Context, dockerCli command.Cli) (string, error) { if o.ProjectName != "" { return o.ProjectName, nil diff --git a/cmd/compose/project_resolution_test.go b/cmd/compose/project_resolution_test.go new file mode 100644 index 00000000000..fbb2c6f4863 --- /dev/null +++ b/cmd/compose/project_resolution_test.go @@ -0,0 +1,163 @@ +/* + Copyright 2020 Docker Compose CLI authors + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +package compose + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/compose-spec/compose-go/v2/types" + "github.com/docker/cli/cli/streams" + logrustest "github.com/sirupsen/logrus/hooks/test" + "go.uber.org/mock/gomock" + "gotest.tools/v3/assert" + + "github.com/docker/compose/v5/pkg/mocks" +) + +func projectDir(t *testing.T, composeContent string) string { + t.Helper() + dir := t.TempDir() + if composeContent != "" { + assert.NilError(t, os.WriteFile(filepath.Join(dir, "compose.yaml"), []byte(composeContent), 0o600)) + } + return dir +} + +func resolutionCli(t *testing.T) *mocks.MockCli { + t.Helper() + ctrl := gomock.NewController(t) + cli := mocks.NewMockCli(ctrl) + cli.EXPECT().Out().Return(streams.NewOut(os.Stdout)).AnyTimes() + cli.EXPECT().Err().Return(streams.NewOut(os.Stderr)).AnyTimes() + return cli +} + +const validCompose = "services:\n web:\n image: alpine\n gated:\n image: alpine\n profiles: [debug]\n" + +// The resolution matrix of projectOrName: one name precedence +// (--project-name, then COMPOSE_PROJECT_NAME, then the model), a hard error +// for an explicit --file that cannot be read, and a label-based fallback +// only when COMPOSE_PROJECT_NAME provides a name. +func TestProjectOrNameResolution(t *testing.T) { + unsetEnv := func(t *testing.T) { + t.Setenv(ComposeProjectName, "") + assert.NilError(t, os.Unsetenv(ComposeProjectName)) + } + + t.Run("broken implicit file falls back to COMPOSE_PROJECT_NAME", func(t *testing.T) { + t.Setenv(ComposeProjectName, "fallback") + dir := projectDir(t, "services: {invalid") + opts := ProjectOptions{ProjectDir: dir} + hook := logrustest.NewGlobal() + + project, name, err := opts.projectOrName(t.Context(), resolutionCli(t)) + assert.NilError(t, err) + assert.Equal(t, name, "fallback") + assert.Assert(t, project == nil) + assert.Equal(t, len(hook.AllEntries()), 1) + assert.Assert(t, strings.Contains(hook.LastEntry().Message, "falling back to label-based mode"), hook.LastEntry().Message) + }) + + t.Run("broken explicit --file is a hard error even with COMPOSE_PROJECT_NAME", func(t *testing.T) { + t.Setenv(ComposeProjectName, "fallback") + dir := projectDir(t, "services: {invalid") + opts := ProjectOptions{ConfigPaths: []string{filepath.Join(dir, "compose.yaml")}, ProjectDir: dir} + + _, _, err := opts.projectOrName(t.Context(), resolutionCli(t)) + assert.ErrorContains(t, err, "yaml") + }) + + t.Run("no file at all with COMPOSE_PROJECT_NAME is the file-less workflow", func(t *testing.T) { + t.Setenv(ComposeProjectName, "labels-only") + dir := projectDir(t, "") + opts := ProjectOptions{ProjectDir: dir} + hook := logrustest.NewGlobal() + + project, name, err := opts.projectOrName(t.Context(), resolutionCli(t)) + assert.NilError(t, err) + assert.Equal(t, name, "labels-only") + assert.Assert(t, project == nil) + // no file exists at all, so the "found but could not be loaded" + // warning would be factually wrong here — this is the normal + // file-less workflow, not a fallback from a broken file. + assert.Equal(t, len(hook.AllEntries()), 0, hook.AllEntries()) + }) + + t.Run("no file and no name errors", func(t *testing.T) { + unsetEnv(t) + dir := projectDir(t, "") + opts := ProjectOptions{ProjectDir: dir} + + _, _, err := opts.projectOrName(t.Context(), resolutionCli(t)) + assert.Assert(t, err != nil) + }) + + t.Run("COMPOSE_PROJECT_NAME overrides the loaded model's name", func(t *testing.T) { + t.Setenv(ComposeProjectName, "from-env") + dir := projectDir(t, validCompose) + opts := ProjectOptions{ProjectDir: dir} + + project, name, err := opts.projectOrName(t.Context(), resolutionCli(t)) + assert.NilError(t, err) + assert.Equal(t, name, "from-env") + assert.Assert(t, project != nil) + }) + + t.Run("--project-name without --file skips loading, even a broken file", func(t *testing.T) { + unsetEnv(t) + dir := projectDir(t, "services: {invalid") + opts := ProjectOptions{ProjectName: "explicit", ProjectDir: dir} + + project, name, err := opts.projectOrName(t.Context(), resolutionCli(t)) + assert.NilError(t, err) + assert.Equal(t, name, "explicit") + assert.Assert(t, project == nil) + }) + + t.Run("unknown requested service is rejected by the load itself", func(t *testing.T) { + unsetEnv(t) + dir := projectDir(t, validCompose) + opts := ProjectOptions{ProjectDir: dir} + + _, _, err := opts.projectOrName(t.Context(), resolutionCli(t), "typo") + assert.ErrorContains(t, err, "no such service") + }) +} + +// validateServiceNames backs the commands that don't pass their service +// arguments through the load-time selection (restart, wait): strict when a +// model is available, no-op in label-based mode. +func TestValidateServiceNames(t *testing.T) { + project := &types.Project{ + Services: types.Services{"web": {Name: "web"}}, + DisabledServices: types.Services{"gated": {Name: "gated"}}, + Jobs: types.Jobs{"migrate": {Name: "migrate"}}, + } + + assert.NilError(t, validateServiceNames(nil, []string{"anything"})) + assert.NilError(t, validateServiceNames(project, []string{"web"})) + // profile-disabled services are legitimate targets: restart enables them + assert.NilError(t, validateServiceNames(project, []string{"gated"})) + assert.Error(t, validateServiceNames(project, []string{"typo"}), "no such service: typo") + // restart/wait pass no services to projectOrName, so a job target never + // reaches jobTargetErr there -- validateServiceNames must recognize it + // itself instead of falling through to the generic "no such service". + assert.Error(t, validateServiceNames(project, []string{"migrate"}), `job "migrate" can only be triggered with "docker compose run"`) +} diff --git a/cmd/compose/ps.go b/cmd/compose/ps.go index 805533ae8af..482ae4b97ce 100644 --- a/cmd/compose/ps.go +++ b/cmd/compose/ps.go @@ -99,14 +99,10 @@ func runPs(ctx context.Context, dockerCli command.Cli, backendOptions *BackendOp } if project != nil { + // unknown requested services were already rejected while loading the + // project (service selection in ToProject) names := project.ServiceNames() - if len(services) > 0 { - for _, service := range services { - if !slices.Contains(names, service) { - return fmt.Errorf("no such service: %s", service) - } - } - } else if !opts.Orphans { + if len(services) == 0 && !opts.Orphans { // until user asks to list orphaned services, we only include those declared in project services = names } diff --git a/cmd/compose/restart.go b/cmd/compose/restart.go index a9d97c50263..c8ce7a2e58e 100644 --- a/cmd/compose/restart.go +++ b/cmd/compose/restart.go @@ -59,6 +59,9 @@ func runRestart(ctx context.Context, dockerCli command.Cli, backendOptions *Back if err != nil { return err } + if err := validateServiceNames(project, services); err != nil { + return err + } if project != nil && len(services) > 0 { project, err = project.WithServicesEnabled(services...) diff --git a/cmd/compose/volumes.go b/cmd/compose/volumes.go index e0da4f82e3b..cbf6865a1d2 100644 --- a/cmd/compose/volumes.go +++ b/cmd/compose/volumes.go @@ -19,7 +19,6 @@ package compose import ( "context" "fmt" - "slices" "github.com/docker/cli/cli/command" "github.com/docker/cli/cli/command/formatter" @@ -57,20 +56,14 @@ func volumesCommand(p *ProjectOptions, dockerCli command.Cli, backendOptions *Ba } func runVol(ctx context.Context, dockerCli command.Cli, backendOptions *BackendOptions, services []string, options volumesOptions) error { - project, name, err := options.projectOrName(ctx, dockerCli, services...) + // unknown requested services are rejected while loading the project + // (service selection in ToProject); label-based mode has no model to + // validate against + _, name, err := options.projectOrName(ctx, dockerCli, services...) if err != nil { return err } - if project != nil { - names := project.ServiceNames() - for _, service := range services { - if !slices.Contains(names, service) { - return fmt.Errorf("no such service: %s", service) - } - } - } - backend, err := compose.NewComposeService(dockerCli, backendOptions.Options...) if err != nil { return err diff --git a/cmd/compose/wait.go b/cmd/compose/wait.go index 9d86fd314cf..ef270465b84 100644 --- a/cmd/compose/wait.go +++ b/cmd/compose/wait.go @@ -63,10 +63,13 @@ func waitCommand(p *ProjectOptions, dockerCli command.Cli, backendOptions *Backe } func runWait(ctx context.Context, dockerCli command.Cli, backendOptions *BackendOptions, opts *waitOptions) (int64, error) { - _, name, err := opts.projectOrName(ctx, dockerCli) + project, name, err := opts.projectOrName(ctx, dockerCli) if err != nil { return 0, err } + if err := validateServiceNames(project, opts.services); err != nil { + return 0, err + } backend, err := compose.NewComposeService(dockerCli, backendOptions.Options...) if err != nil { diff --git a/pkg/e2e/jobs_test.go b/pkg/e2e/jobs_test.go index b64f1700bd2..b00bd61eb79 100644 --- a/pkg/e2e/jobs_test.go +++ b/pkg/e2e/jobs_test.go @@ -141,3 +141,21 @@ func TestDownRefusesJobWithProjectNameEnv(t *testing.T) { StderrContains(`job "migrate" can only be triggered with "docker compose run"`), ServiceNotCreated("migrate")) } + +// restart and wait don't pass their service arguments to projectOrName (see +// runRestart/runWait), so jobTargetErr never sees them there -- the refusal +// for these two instead comes from validateServiceNames itself recognizing +// project.AllJobs(). A separate code path, so it needs its own coverage. +func TestRestartRefusesJob(t *testing.T) { + NewScenario(t, "restart must refuse a job by name, naming run as the right command"). + Step("restart fails naming the job", + ComposeCmd("restart", "migrate").MayFail(), + StderrContains(`job "migrate" can only be triggered with "docker compose run"`)) +} + +func TestWaitRefusesJob(t *testing.T) { + NewScenario(t, "wait must refuse a job by name, naming run as the right command"). + Step("wait fails naming the job", + ComposeCmd("wait", "migrate").MayFail(), + StderrContains(`job "migrate" can only be triggered with "docker compose run"`)) +} diff --git a/pkg/e2e/testdata/TestRestartRefusesJob/compose.yaml b/pkg/e2e/testdata/TestRestartRefusesJob/compose.yaml new file mode 100644 index 00000000000..7cbebdadebd --- /dev/null +++ b/pkg/e2e/testdata/TestRestartRefusesJob/compose.yaml @@ -0,0 +1,10 @@ +services: + web: + image: alpine + command: sleep infinity +jobs: + migrate: + image: alpine + command: sh -c 'echo "migration done"' + triggers: + manual: true diff --git a/pkg/e2e/testdata/TestWaitRefusesJob/compose.yaml b/pkg/e2e/testdata/TestWaitRefusesJob/compose.yaml new file mode 100644 index 00000000000..7cbebdadebd --- /dev/null +++ b/pkg/e2e/testdata/TestWaitRefusesJob/compose.yaml @@ -0,0 +1,10 @@ +services: + web: + image: alpine + command: sleep infinity +jobs: + migrate: + image: alpine + command: sh -c 'echo "migration done"' + triggers: + manual: true