fix(hash): pin the service config-hash byte layout, decoupled from compose-go struct refactorings - #14215
fix(hash): pin the service config-hash byte layout, decoupled from compose-go struct refactorings#14215ndeloof wants to merge 1 commit into
Conversation
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
The canonical-hash migration is well-reasoned and correctly implemented. The core canonicalHash approach (marshal → UseNumber decode into map[string]any → re-marshal with sorted keys) is sound and will be stable across compose-go struct reorderings. The dual-acceptance logic for volumes in reconcileVolumes is also correct: it recomputes the legacy hash from the current desired config, which correctly diverges when the config actually changed and matches only when unchanged.
The one finding the drafter flagged as medium — that legacyVolumeHash would compute the wrong legacy hash if a future compose-go upgrade reordered VolumeConfig fields — was dismissed by the verifier as an inherent limitation of the pre-existing legacy approach, consciously accepted and documented in the PR. The bridge only needs to survive until users' volumes have been recreated/updated once; afterward the canonical hash takes over and becomes layout-independent. This is a reasonable tradeoff given that volume recreation is destructive.
No CONFIRMED or LIKELY bugs were found in the introduced code.
90f7cad to
59a1518
Compare
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
One medium-severity finding in new test code introduced by this PR.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…form ServiceHash digests json.Marshal of types.ServiceConfig, which couples every recorded config-hash to the DECLARATION ORDER of compose-go struct fields: encoding/json emits struct fields in that order and flattens embedded structs at their embedding position. Any compose-go refactoring that moves a field — such as the upcoming container-spec layering, which regroups the whole struct — would change the bytes, and with them the hash, of configurations that did not change at all: every container recreated on the first `up` after an upgrade. The hash now re-emits the marshaled object with its ROOT keys in a frozen list reproducing the historical order (generated by reflection over the last pre-layering compose-go), values byte-verbatim. For today, the output is byte-identical to the direct marshal — proven by a continuity test — so every existing container stamp stays valid: no migration, no recreation, full backward compatibility. From the first struct reorder on, the frozen list alone carries that continuity, locked by golden-value tests; a root attribute added later is appended in sorted order and, thanks to omitempty, only moves the hash of configurations that use it — exactly like a field addition always did. A reflection test fails when compose-go grows a root attribute missing from the list, so extending the hash surface stays a reviewed decision. Nested objects keep their own struct marshal: a reorder inside one of them would still move hashes — the golden tests exist to turn that into a caught, reviewed event rather than a silent side effect. Network and volume hashes are unchanged (their structs are not being reordered) and gain the same golden locks. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
59a1518 to
ce4e45d
Compare
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
The PR correctly pins the service config-hash byte layout using a frozen top-level key order, decoupling it from compose-go struct field ordering. The implementation is sound:
pinRootKeyOrdercorrectly reconstructs JSON with frozen key ordering, usingbytes.Buffer.Write(which never errors) andjson.Marshal(key)(safe for plain strings)- The sorted-append for unknown keys is deterministic and correct
- The golden test mechanism will catch future unintended hash changes
- The struct coverage test (
TestServiceHashKeyOrderCoversStruct) ensures the frozen list stays in sync with compose-go via reflection - The continuity test correctly validates byte-for-byte identity before compose-go reorders fields
No bugs were found in the introduced code.
What this PR does, in one sentence: the service config-hash byte layout is pinned to its historical form, so a compose-go struct refactoring can never again change hashes — and existing container stamps stay valid verbatim: no migration, no recreation.
Context
ServiceHash— the value behindcom.docker.compose.config-hash, which decides whetheruprecreates a container — digestsjson.Marshaloftypes.ServiceConfigdirectly.encoding/jsonemits struct fields in declaration order and flattens embedded structs at their embedding position, so every recorded hash is silently coupled to compose-go's struct layout. That has held only because compose-go never reordered its fields: the upcoming container-spec layering (compose-spec/compose-go#866, adopted by #14093) regroups the whole struct, and without this fix its adoption would recreate every running container on the firstupafter upgrade — the very kind of undisclosed side effect the hash exists to prevent.What the PR brings
omitemptyonly configurations using the new attribute see their hash move, exactly like a field addition always did. A reflection test fails when compose-go grows a root attribute absent from the list, keeping the hash surface a reviewed decision.deploy,healthcheck, …) keep their own struct marshal — a reorder inside one of them would still move hashes. The golden tests turn that from a silent side effect into a caught, reviewed event at the offending PR. Network and volume hashes are unchanged (their structs are not being reordered) and gain the same golden locks.Why this is the right next brick
#14093 reorders
ServiceConfigfields as a structural consequence of the jobs/container-spec work; landing this first means that PR ships with zero hash impact — nothing to put in its release notes — and any future compose-go layout change stays free as well.🤖 Generated with Claude Code