feat(compose): warn on unsupported compose-file attributes - #14196
Conversation
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
This PR cleanly centralizes detection of unsupported compose-file attributes into DetectUnsupportedAttributes, wired once at ToProject. The old ad-hoc warnings in create.go and service_containers.go are correctly removed. The unit-test coverage and the e2e scenario are solid. Two low-severity usability nits in the new detection logic are noted inline.
| func checkPortsMode(name string, service types.ServiceConfig) []api.UnsupportedAttribute { | ||
| var findings []api.UnsupportedAttribute | ||
| for _, port := range service.Ports { | ||
| if port.Mode == "host" { | ||
| findings = append(findings, api.UnsupportedAttribute{ | ||
| Service: name, | ||
| Path: "ports[].mode", | ||
| Reason: "ports[].mode: host is only honored by the Swarm routing mesh", | ||
| }) | ||
| } | ||
| } | ||
| return findings | ||
| } |
There was a problem hiding this comment.
[low] checkPortsMode emits one warning per host-mode port, producing duplicate log lines for services with N host-mode ports
When a service declares multiple ports with mode: host, checkPortsMode appends one UnsupportedAttribute per port entry, all with the same Service and Path (ports[].mode). A service with three host-mode ports produces three identical warnings:
service "web": ports[].mode: ports[].mode: host is only honored by the Swarm routing mesh
service "web": ports[].mode: ports[].mode: host is only honored by the Swarm routing mesh
service "web": ports[].mode: ports[].mode: host is only honored by the Swarm routing mesh
Because UnsupportedAttribute carries no port-number field there is no way to distinguish the entries, so the repetition adds noise without adding information. Returning early after the first host-mode port de-duplicates at zero cost:
| func checkPortsMode(name string, service types.ServiceConfig) []api.UnsupportedAttribute { | |
| var findings []api.UnsupportedAttribute | |
| for _, port := range service.Ports { | |
| if port.Mode == "host" { | |
| findings = append(findings, api.UnsupportedAttribute{ | |
| Service: name, | |
| Path: "ports[].mode", | |
| Reason: "ports[].mode: host is only honored by the Swarm routing mesh", | |
| }) | |
| } | |
| } | |
| return findings | |
| } | |
| func checkPortsMode(name string, service types.ServiceConfig) []api.UnsupportedAttribute { | |
| for _, port := range service.Ports { | |
| if port.Mode == "host" { | |
| return []api.UnsupportedAttribute{{ | |
| Service: name, | |
| Path: "ports[].mode", | |
| Reason: "ports[].mode: host is only honored by the Swarm routing mesh", | |
| }} | |
| } | |
| } | |
| return nil | |
| } |
| Confidence | Score |
|---|---|
| 🟢 strong | 100/100 |
| return findings | ||
| } | ||
|
|
||
| func fileReferenceFindings(name, prefix, uid, gid string, mode *types.FileMode) []api.UnsupportedAttribute { |
There was a problem hiding this comment.
[low] fileReferenceFindings uses generic paths that don't identify which config/secret entry triggered the warning
The prefix passed to fileReferenceFindings is always the bare bracket placeholder ("configs[]" or "secrets[]"), so when a service references two configs that both set uid, the caller gets two findings with identical Service and Path (configs[].uid) and the user cannot tell which config entry needs fixing.
Including the entry's Source name in the prefix — e.g. fmt.Sprintf("configs[%s]", config.Source) — makes each warning self-contained and actionable:
service "web": configs[myconfig].uid: uid is not supported outside Swarm mode and will be ignored
The change only requires updating the two call-sites in checkConfigFileReferences and checkSecretFileReferences:
// in checkConfigFileReferences:
findings = append(findings, fileReferenceFindings(name, fmt.Sprintf("configs[%s]", config.Source), config.UID, config.GID, config.Mode)...)
// in checkSecretFileReferences:
findings = append(findings, fileReferenceFindings(name, fmt.Sprintf("secrets[%s]", secret.Source), secret.UID, secret.GID, secret.Mode)...)(No change to fileReferenceFindings itself or to the test data — just update the expected Path strings in the unit tests.)
| Confidence | Score |
|---|---|
| 🟢 strong | 100/100 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
1efc983 to
c96812a
Compare
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
Lower-confidence findings (not posted inline)
- [medium]
pkg/e2e/testdata/TestUnsupportedAttributesWarning/compose.yaml:52 — Cluster volume sourcemy-csi-volumenot declared in top-levelvolumes:may cause loader rejection before warnings are checked (confidence: weak 45/100)
The e2e compose.yaml referencestype: cluster, source: my-csi-volumein thecluster-volservice, but there is no top-levelvolumes:declaration formy-csi-volume. If compose-go validates that cluster volume sources must be declared at the top level,docker compose configwould fail beforeDetectUnsupportedAttributesruns, causing the e2e test to fail silently. The verifier notes this is speculative —type: clustermay bypass this validation since CSI volumes are externally managed — but it is worth confirming by running the e2e suite.
| return nil, metrics, err | ||
| } | ||
|
|
||
| for _, finding := range backend.DetectUnsupportedAttributes(project) { |
There was a problem hiding this comment.
[medium] DetectUnsupportedAttributes fires on every ToProject call, including name-only lookups, potentially emitting duplicate warnings
ToProject is the single implementation called by every code path that needs a project — including projectOrName and toProjectName, which invoke it with cli.WithDiscardEnvFile, cli.WithoutEnvironmentResolution solely to extract the project name. By wiring DetectUnsupportedAttributes unconditionally inside ToProject, warnings now fire on every call, not just the ones that need the full project analysis.
In practice, several CLI commands (down, start, stop, kill, logs, ps, pause, remove, restart, volumes, wait) route through projectOrName → ToProject. For these commands the user sees a warning even though they are operating on already-running containers — which can be surprising or noisy. More importantly, if any command calls ToProject more than once (e.g. to re-resolve after environment expansion via WithServices), the user would see each warning twice.
Suggestion: thread a boolean flag (or an opts ...LoadOption variant) through ToProject to suppress detection when the call is for name resolution only, or move the detection call to a point earlier in each command's RunE that is only reached once per user-visible invocation (e.g., inside WithServices).
| Confidence | Score |
|---|---|
| 🟡 moderate | 57/100 |
c96812a to
769033a
Compare
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
This PR cleanly centralizes the unsupported-attribute detection into a pure DetectUnsupportedAttributes function and wires it at the shared ToProject choke-point. The design is sound — the old ad-hoc logrus.Warn calls in create.go and service_containers.go are properly removed and the table-driven approach is well-tested. Three issues found:
- Non-deterministic warning order (pkg/compose/unsupported_attributes.go:58) — map iteration over
project.Servicesmakes warning output non-deterministic across runs. - Ambiguous port path for TCP/UDP collisions (pkg/compose/unsupported_attributes.go:173) —
ports[80].modeis used for both TCP and UDP port 80, producing identicalPathstrings when both usemode: host. - Runtime warning gap in
waitDependency(pkg/compose/service_containers.go:226) —docker compose startreconstructs the project from container labels viaprojectFromName, bypassingToProjectandDetectUnsupportedAttributes; an unknown condition now silently skips the dependency wait with no warning or error.
| // mode. | ||
| func (s *composeService) DetectUnsupportedAttributes(project *types.Project) []api.UnsupportedAttribute { | ||
| var findings []api.UnsupportedAttribute | ||
| for name, service := range project.Services { |
There was a problem hiding this comment.
[medium] DetectUnsupportedAttributes returns findings in non-deterministic map iteration order
project.Services is a map[string]ServiceConfig (Go map), so for name, service := range project.Services iterates in a different random order on each invocation. The per-check helpers checkConfigsLabels and checkSecretsDriverOptsAndLabels also range over maps internally. The caller in cmd/compose/compose.go prints each returned finding immediately:
for _, finding := range backend.DetectUnsupportedAttributes(project) {
logrus.Warn(finding)
}This means users running the same command twice may see warnings in a different order. The unit test works around non-determinism by sorting findings before comparing — a clear signal that the authors are aware the output is unordered. The API comment states it is a "pure function … safe to call multiple times", which implies repeatable output, but that contract is currently broken for multi-service projects.
Suggested fix: Sort the returned slice before returning it, e.g.:
| for name, service := range project.Services { | |
| var findings []api.UnsupportedAttribute | |
| for name, service := range project.Services { | |
| for _, check := range serviceChecks { | |
| findings = append(findings, check(name, service)...) | |
| } | |
| } | |
| for _, check := range projectChecks { | |
| findings = append(findings, check(project)...) | |
| } | |
| slices.SortFunc(findings, func(a, b api.UnsupportedAttribute) int { | |
| if c := strings.Compare(a.Service, b.Service); c != 0 { | |
| return c | |
| } | |
| return strings.Compare(a.Path, b.Path) | |
| }) | |
| return findings |
| Confidence | Score |
|---|---|
| 🟢 strong | 92/100 |
| if port.Mode == "host" { | ||
| findings = append(findings, api.UnsupportedAttribute{ | ||
| Service: name, | ||
| Path: fmt.Sprintf("ports[%d].mode", port.Target), |
There was a problem hiding this comment.
[medium] checkPortsMode uses port.Target (port number) as array subscript, producing identical paths for same-port TCP/UDP entries
The Path field is built as fmt.Sprintf("ports[%d].mode", port.Target), where port.Target is the container port number (e.g. 80), not the slice index. This is intentional (documented in the comment: "identified by its target port"), but it has a correctness hazard: if a service exposes the same port for both TCP and UDP with mode: host, both findings produce the identical string ports[80].mode, making them indistinguishable in the output.
Docker Compose does support specifying the same target port with different protocols:
ports:
- target: 80
published: "8080"
protocol: tcp
mode: host
- target: 80
published: "8080"
protocol: udp
mode: hostBoth would emit service "x": ports[80].mode: …, making it impossible to tell which entry (or whether both) triggered the warning.
Suggested fix: Include the protocol in the path when it is non-empty:
| Path: fmt.Sprintf("ports[%d].mode", port.Target), | |
| Path: fmt.Sprintf("ports[%d/%s].mode", port.Target, port.Protocol), |
(This matches the notation used in Docker port-mapping output, e.g. 0.0.0.0:8080->80/tcp.) Alternatively use the slice index i with for i, port := range service.Ports.
| Confidence | Score |
|---|---|
| 🟢 strong | 92/100 |
| done, err = s.checkDependencyCompleted(ctx, dep, config, waitingFor) | ||
| default: | ||
| logrus.Warnf("unsupported depends_on condition: %s", config.Condition) | ||
| return nil |
There was a problem hiding this comment.
[high] Removing the runtime logrus.Warn from waitDependency's default branch leaves docker compose start silently skipping dependency waits for unknown conditions
The PR removes the logrus.Warnf("unsupported depends_on condition: %s", config.Condition) call from the default: branch and replaces it with a load-time check (checkDependsOnCondition called during ToProject). However, the docker compose start command takes a different code path that bypasses ToProject entirely:
startcallsprojectFromNameto reconstruct a project from live container labels.projectFromNamereads the dependency condition directly from thecom.docker.compose.depends_oncontainer label (format:"dep:condition:restart"), setting it verbatim atcondition = dcArr[1].- This reconstructed project is passed directly to
waitDependency—DetectUnsupportedAttributesis never called.
If a container was created by an older Compose version with a non-standard condition stored in the label, or if the label is set manually, waitDependency will now silently hit default: return nil — no wait is performed, no error is returned, and no warning is emitted. The dependent service will start before its dependency reaches the expected state, which can cause silent startup failures.
The original runtime warning was a safety net precisely for this kind of runtime-only code path. Consider reinstating it as a defensive fallback:
default:
logrus.Warnf("unsupported depends_on condition %q, skipping wait for %q", config.Condition, dep)
return nil| Confidence | Score |
|---|---|
| 🟠 weak | 52/100 |
769033a to
c37f3d8
Compare
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
| // intentionally skip that, and start can also rebuild the project | ||
| // from live container labels without loading a compose file at | ||
| // all. This is the only signal those paths get. | ||
| if !isKnownDependsOnCondition(config.Condition) { |
There was a problem hiding this comment.
[medium] Double warning for unknown depends_on conditions when running up or create
The PR adds checkDependsOnCondition to the load-time DetectUnsupportedAttributes machinery, and WithServices (used by up and create) always passes warnUnsupportedAttributes. As a result, an unknown condition such as service_ready now fires two warnings:
- Load time —
ToProject→backend.DetectUnsupportedAttributes→checkDependsOnCondition→logrus.Warn(finding) - Runtime —
waitDependencies→waitDependencydefault branch →!isKnownDependsOnCondition==true→logrus.Warnf(…)
The comment on this branch acknowledges that the load-time warning exists but justifies keeping the runtime one for start/restart (which skip load-time detection). However, for up and create the user sees the same condition reported twice, which is confusing and regresses from the pre-PR behaviour of a single runtime warning.
Suggested fix: skip the runtime warning when the load-time warning already fired, for example by gating on whether the project was loaded with warnUnsupportedAttributes. Alternatively, move checkDependsOnCondition out of serviceChecks and keep the runtime warning as the sole signal — it already carries the richer message format introduced in this PR (service %q: unsupported depends_on condition %q, skipping wait for %q).
| Confidence | Score |
|---|---|
| 🟢 strong | 97/100 |
| checkClusterVolumes, | ||
| checkConfigFileReferences, | ||
| checkSecretFileReferences, | ||
| checkDependsOnCondition, |
There was a problem hiding this comment.
[low] checkDependsOnCondition is not a Swarm-only check but is grouped with Swarm-only checks
All other entries in serviceChecks flag attributes that are accepted by the schema but only meaningful in Swarm mode (deploy.mode, credential_spec, cluster volumes, etc.). An unknown depends_on.X.condition value is different: it is unrecognized in any context — Swarm mode would also silently skip an unknown condition. Grouping it here conflates two distinct categories:
- Swarm-only attributes: valid outside Swarm but have no effect (e.g.,
deploy.placement) - Unrecognized schema values: not understood by the runtime in any mode (e.g.,
service_ready)
In practice the warning is useful either way, but the semantic mismatch could mislead users into thinking the condition would work in Swarm mode when it would not. Consider either documenting this distinction in a comment, or surfacing the finding with a different message (e.g., "unrecognized depends_on condition: …" rather than implicitly grouping it with Swarm-specific attribute warnings).
| Confidence | Score |
|---|---|
| 🟡 moderate | 67/100 |
|
IMHO this would be simpler to implement this as a compose-go loader option, based on a set of yaml.Path pattern of supported attributs, so any unsupported (including newer) attribute get identified as unsupported/ignored |
c37f3d8 to
7b5e1aa
Compare
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
🟢 No issues found — LGTM! View logs.
|
Alternative approach implemented by Claude: compose-spec/compose-go#926 |
7b5e1aa to
d01741a
Compare
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
⏱️ PR Review Timed Out — The review agent hit the 2700 s time limit. This usually happens on large or complex diffs. Re-request a review from docker-agent to retry — if it times out again, consider splitting the PR into smaller pieces.
|
/review |
|
👋 Heads up: The |
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
This PR is well-structured — the allowlist-based fail-closed approach cleanly solves the silent-misconfiguration gap from #13150. The compose-go loader integration, the value-conditional attribute checks (ports mode, cluster volumes, file-reference uid/gid/mode), and the sync.OnceValue lazy schema parse are all sound. Test coverage is thorough.
One low-severity cosmetic issue is posted inline. One additional speculative finding is listed below.
Lower-confidence findings (not posted inline)
- [medium]
pkg/compose/unsupported_attributes.go:292 —nilpassed as second argument toloader.WithSupportedAttributescould cause a nil pointer dereference if the fork's API dereferences it unconditionally; based on naming conventions an optional callback is more likely, making this safe (confidence: ⚪ negligible 26/100)
| func fileReferenceReport(kind string) func(loader.UnsupportedAttribute) []api.UnsupportedAttribute { | ||
| return func(finding loader.UnsupportedAttribute) []api.UnsupportedAttribute { | ||
| ref, _ := finding.Value.(map[string]any) | ||
| source, _ := ref["source"].(string) |
There was a problem hiding this comment.
[low] Missing guard for empty source produces malformed path configs..uid in warning
When a service-level configs or secrets reference is matched by hasFileReferenceOverride, fileReferenceReport reads source from the map via a comma-ok type assertion that silently returns "" if the key is absent:
source, _ := ref["source"].(string)
// ...
Path: fmt.Sprintf("%s.%s.%s", kind, source, field),If source is empty the formatted path becomes "configs..uid" — two consecutive dots — which is syntactically malformed and produces a confusing warning like:
service "web": configs..uid: uid is not supported outside Swarm mode and will be ignored
The Compose Specification states that a service-level configs entry's source is required when uid/gid/mode are set, and compose-go typically normalises it, so this edge case is unlikely in practice. However, the code has no defensive fallback. A simple guard improves the user-facing message:
| source, _ := ref["source"].(string) | |
| source, _ := ref["source"].(string) | |
| if source == "" { | |
| source = "(anonymous)" | |
| } |
Alternatively, keep it as a single fallback expression:
source, _ := ref["source"].(string); if source == "" { source = "(anonymous)" }
| Confidence | Score |
|---|---|
| 🟡 moderate | 67/100 |
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
Lower-confidence findings (not posted inline)
- [low]
pkg/compose/unsupported_attributes.go:60 —label_filewarning message claims Swarm-only restriction; the feature is unimplemented in this runtime (not Swarm-specific) — users may incorrectly infer Swarm is required (confidence: weak 52/100)
| func fileReferenceReport(kind string) func(loader.UnsupportedAttribute) []api.UnsupportedAttribute { | ||
| return func(finding loader.UnsupportedAttribute) []api.UnsupportedAttribute { | ||
| ref, _ := finding.Value.(map[string]any) | ||
| source, _ := ref["source"].(string) |
There was a problem hiding this comment.
[low] fileReferenceReport may produce a malformed double-dot path when source is absent
source, _ := ref["source"].(string) silently yields "" if the raw map entry has no source key. fmt.Sprintf("%s.%s.%s", kind, source, field) then produces "configs..uid" — a double-dot path that is confusing in the warning output.
In practice compose-go's loader normalises every service-level config/secret reference so that source is always populated (defaulting to the config/secret name). However, this function receives the raw YAML map from the compose-go tree-walker, not the fully-normalised types.ServiceConfig, so the normalisation may not yet have run at call-time. Adding a fallback guard makes the output unambiguous regardless of loader ordering:
| source, _ := ref["source"].(string) | |
| source, _ := ref["source"].(string) | |
| if source == "" { | |
| source = "<unknown>" | |
| } |
| Confidence | Score |
|---|---|
| 🟡 moderate | 57/100 |
d01741a to
d44e060
Compare
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
This PR introduces a well-structured fail-closed allowlist mechanism for warning users about unsupported Compose-file attributes. The core detection logic in pkg/compose/unsupported_attributes.go is sound, with value-conditional checks for ports[].mode: host, cluster volumes, and config/secret file-reference overrides all correctly implemented.
One issue requires attention before merging: the go.mod replace directive points to a personal fork — see the inline comment.
Lower-confidence findings (reviewed but not posted inline)
- [medium]
cmd/compose/unsupported_attributes_test.go:54 —captureWarningsmutates global logrus state (not parallel-test-safe) — DISMISSED by verifier: standard test helper pattern, no production risk - [medium]
pkg/compose/unsupported_attributes.go:249 — value-conditional findings Service field overwrite — DISMISSED by verifier: all patterns start withservices.*sosplitServicePathalways returns non-empty service name
| github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 | ||
| ) | ||
|
|
||
| replace github.com/compose-spec/compose-go/v2 => github.com/glours/compose-go/v2 v2.0.0-20260908091111-b3a5d864f669 |
There was a problem hiding this comment.
[medium] go.mod replace directive points to a personal fork — shipping risk if not removed before release
go.mod contains:
replace github.com/compose-spec/compose-go/v2 => github.com/glours/compose-go/v2 v2.0.0-20260908091111-b3a5d864f669
The PR description acknowledges this is temporary (pending an unreleased compose-go change), but shipping with this replace in place carries real risk:
- Fork availability —
github.com/glours/compose-gois a personal repository. If it is renamed, made private, force-pushed, or deleted after merge, all reproducible builds of this release break with an unresolvable module reference. - Future-dated pseudo-version — the commit timestamp
20260908is from 2026 (today is 2026-09-08), indicating this was cut very recently from HEAD; there is no guarantee the fork's history is stable. - No upstream review guarantee — the personal fork's commit is not subject to the same review as upstream
compose-spec/compose-go.
The replace must be removed (and the upstream compose-go module updated to the released version) before this PR is merged to a release branch. A merge without removing it is a ticking reproducibility failure.
| Confidence | Score |
|---|---|
| 🟡 moderate | 70/100 |
|
compose-spec/compose-go#927 has been merged, you can bump compose-go ref to latest commit then we can merge |
Compose silently ignored several spec-valid attributes it doesn't honor outside Swarm mode: deploy.update_config, credential_spec, ports[].mode: host, and others had no visible effect on a plain `docker compose up`. Detection is wired into compose-go's loader as a fail-closed allowlist: every attribute path the schema declares, minus the handful this runtime deliberately doesn't implement, plus a few value-conditional checks for attributes only unsupported for one specific value. A newly-specified attribute gets flagged automatically, with no corresponding code change needed here. Depends on an unreleased compose-go change; go.mod carries a temporary replace to a fork branch until it's released. Signed-off-by: Guillaume Lours <glours@users.noreply.github.com>
d44e060 to
e7fe397
Compare
ndeloof
left a comment
ndeloof
left a comment
There was a problem hiding this comment.
CI green across the matrix, all docker-agent findings addressed at head (runtime warning kept in waitDependency's default branch, anonymous-source guard in fileReferenceReport, no more go.mod replace — compose-go pinned to an upstream pseudo-version). LGTM.
3 participants
What I did
Compose accepted attributes that are valid per the Compose Specification but had no effect outside Swarm mode,
deploy.update_config,credential_spec,label_file,ports[].mode: host, cluster volumes, and configs/secretsuid/gid/modewithout ever telling the user, which led to silent misconfiguration.Detection is wired into compose-go's loader as a fail-closed allowlist: every attribute path the Compose Specification declares, minus the handful this runtime deliberately doesn't implement, plus a few value-conditional checks for attributes only unsupported for one specific value (
ports[].mode: host, cluster volumes, file-reference overrides).A newly-specified attribute is flagged automatically, with no corresponding code change needed here — this closes the actual gap, not just today's known list.
This depends on an unreleased compose-go change (
WithSupportedAttributes/WithUnsupportedAttributesCheckloader option);go.modcarries a temporaryreplaceto a fork branch until it's released.Covered by an integration-style test suite loading real compose files through the actual loader, plus an e2e scenario exercising the warnings through the CLI end to end.
Related issue
Fixes #13150
(not mandatory) A picture of a cute animal, if possible in relation to what you did
