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 2 commits 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.
glours
left a comment
There was a problem hiding this comment.
Reviewed the pinning mechanism itself (pinRootKeyOrder, the frozen serviceHashKeyOrder list, golden/continuity tests) and found it sound: verified the JSON re-serialization is byte-verbatim with no precision/escaping/duplicate-key issues, and cross-checked all 97 entries in serviceHashKeyOrder against the exact pinned compose-go version field-by-field with no gaps. go build, go test ./pkg/..., and golangci-lint are all clean on this branch.
Three small, non-blocking items below — a couple of reuse/dedup nits and one test-hardening suggestion for the reflection walker. None of these affect the correctness of the hash itself; happy to see this merge once you've had a look.
The trimming of hash-excluded fields becomes trimServiceHashFields, shared between ServiceHash and the continuity test so the exclusion set cannot silently drift; pinRootKeyOrder reuses the package's sortedKeys helper; the key-order coverage walker now follows encoding/json promotion rules for non-struct embeds and unexported fields. Golden hashes unchanged. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
docker-agent
left a comment
There was a problem hiding this comment.
This review covers only the commits pushed since ce4e45db1dcd.
Assessment: 🟢 APPROVE
The incremental changes are a clean refactoring with no bugs introduced:
trimServiceHashFieldsextraction is correct:types.ServiceConfigis passed and returned by value;ServiceHashcaptures the result witho = trimServiceHashFields(o), so all pointer-field nil-ings take effect. TheDeploydeep-copy inside the function is preserved correctly.sortedKeyscall replaces the equivalent inline sort-and-iterate loop.sortedKeys[V any](m map[string]V) []stringis defined inpkg/compose/reconcile.goin the same package — in scope, correct signature, semantically identical to the removed code."sort"import removal is safe: the only use ofsortinhash.gowas the now-replacedsort.Strings(rest)call.
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