Skip to content

Commit 4bdcfdd

Browse files
authored
fix: derive the extends merge policy from each key's validator (#58)
* fix: derive the extends merge policy from each key's validator resolve_extends runs ahead of validate(), so a merge that normalizes a value decides what the gate then sees. The policy was a hand-maintained pair of key sets, and it had drifted from the gate in both directions. It laundered invalid input into valid: ulimits has no list form and the gate refuses one, but _merge_map coerced `ulimits: ["nofile=2"]` into {"nofile": "2"} before the gate ran. Same for every list-only key given a bare scalar -- cap_add, devices, security_opt, group_add, volumes, secrets, configs all refuse a scalar standalone and all accepted one via extends. And it refused valid input: extra_hosts has a list form, accepted standalone, but rejected on merge. One rule now: a merge may normalize a value only through a form that key actually has. Registry keys run spec.validate on both sides before spec.merge, so the merge's accepted forms ARE the gate's, by construction. Structural normalizers are narrowed to the forms Compose defines -- scalar only for tmpfs/env_file, list form only for environment/extra_hosts/depends_on. extra_hosts gets a real normalizer: its list form is colon-separated, so pairs_to_mapping (which splits on '=') would have produced a single {'host:ip': None} key. Splits on the first colon, so IPv6 survives. A parametrized test pins the invariant for every mergeable key: a form the gate refuses standalone must be refused through extends too. Verified to go red without the fix. * fix: attribute merge errors to the right service; make the invariant test real The base-side spec.validate reported the base's malformed value under the extending service's name. _merge now takes base_name and attributes each side's failure to the service the value belongs to. The parametrized invariant test was 10/11 vacuous: it paired the list-only keys with a dict, which the old code already refused, instead of the bare scalar that actually laundered. It now uses the laundering shape, covers all 17 mergeable keys, and asserts its own completeness so a new key cannot be added without a case. Against the pre-fix code it goes red on nine keys; before, only one. * docs: correct the normalization list and attribution claim; pin structural attribution The Extends section claimed 'nothing else is coerced' while omitting list-form labels/annotations, which are legitimately normalized through their own validator-approved list form. The 'error always names the owning service' claim had one reachable exception: spec.merge only receives the extending service's name, so a null ulimits in the base is reported against the child. Stated rather than overclaimed. Adds tests pinning base-side attribution on both structural paths (_as_mapping, _as_concat_list), which were correct but unpinned.
1 parent 7ccdd0c commit 4bdcfdd

4 files changed

Lines changed: 436 additions & 35 deletions

File tree

architecture/supported-subset.md

Lines changed: 33 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -234,26 +234,42 @@ slot (`architecture/glossary.md`).
234234
`entrypoint` (argv replaced wholesale, never concatenated) — also
235235
covers unknown keys, which `validate()` then rejects downstream exactly
236236
as it would without `extends`.
237-
- **Normalization before merge:** list-form `environment` and list-form
238-
`depends_on` (a bare service-name list) are normalized to mappings
239-
before the mapping-merge; scalar-form `tmpfs`/`env_file` are normalized
240-
to a one-element list before the concatenation.
237+
- **Normalization before merge — a merge never widens the gate.** A merged
238+
side is normalized only through a form that key *actually has*: list-form
239+
`environment`, `labels`, `annotations`, `extra_hosts` and `depends_on`
240+
become mappings before the mapping-merge, and scalar-form `tmpfs`/`env_file`
241+
become a one-element list before the concatenation. Nothing else is
242+
coerced. This matters because `resolve_extends` runs **ahead of**
243+
`validate()`: normalizing a form a key does not have would hand the gate a
244+
document it would have
245+
refused standalone. So `ulimits` and `healthcheck` (no list form) refuse a
246+
list, and every list-only key (`cap_add`, `cap_drop`, `security_opt`,
247+
`devices`, `group_add`, `volumes`, `secrets`, `configs`) refuses a bare
248+
scalar — exactly as each does outside `extends`. For a **registry** key the
249+
accepted forms are read from the key's own validator (`spec.validate` runs
250+
on both sides before `spec.merge`), so those cannot drift from the gate by
251+
construction. The **structural** keys have no `KeySpec`, so their accepted
252+
forms are declared in `extends.py` (`_STRUCTURAL_*`, `_SCALAR_FORM_KEYS`)
253+
and kept honest by a test asserting, for every mergeable key, that a form
254+
the gate refuses standalone is refused through `extends` too.
255+
`extra_hosts`' list form is `host:ip` — colon-separated, split on the
256+
*first* colon so an IPv6 address survives (`myhost:::1`
257+
`{myhost: ::1}`).
241258
- **Refused loudly:** cross-file `extends: {file: ..., service: ...}`; a
242259
bare-string (or any other non-mapping) `extends`; an unrecognized key
243260
under `extends` other than `service`; a non-string `service`; a `service`
244-
naming one that doesn't exist; and a merge across incompatible forms (a
245-
mapping-merge key that is neither a mapping nor list-form
246-
`environment`/`depends_on`; a sequence-concatenate key that is neither a
247-
list nor a scalar string).
248-
- **Divergences from Compose:** `environment`/`depends_on` accept list form
249-
directly in `extends.py`'s own merge (`_as_mapping`); `extra_hosts`/
250-
`healthcheck` do not — list form on a merged side is refused for those two.
251-
`labels`/`annotations`/`ulimits` instead route through their `SERVICE_KEYS`
252-
merge policy (`_merge_map`), which *does* coerce list form, via the same
253-
`pairs_to_mapping` normalizer `environment` uses — both paths reject a
254-
non-string list element identically, since it's the one function they
255-
share. Short-form `volumes` are concatenated rather than merged by target
256-
path; podman resolves duplicate mounts at run time. Referenced resources
261+
naming one that doesn't exist; and a merge across incompatible forms — a
262+
side whose form that key does not have. A registry key raises with its own
263+
validator's message (`'cap_add' must be a list`), the same message the value
264+
produces standalone; a structural key raises `cannot merge '<key>' across
265+
incompatible forms`. A malformed *form* is reported against the service the
266+
value belongs to — the base, when it is the base's value at fault, not the
267+
service extending it. (One exception: an explicit `null` on a mergeable key
268+
is valid standalone but refused on merge, and a null in the base is reported
269+
against the extending service.)
270+
- **Divergences from Compose:** short-form `volumes` are concatenated rather
271+
than merged by target path; podman resolves duplicate mounts at run time.
272+
Referenced resources
257273
(top-level `volumes`, `networks`, `secrets`, `configs`) are not
258274
auto-imported — as in Compose, the extending service must declare what it
259275
needs.

compose2pod/extends.py

Lines changed: 69 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -3,16 +3,23 @@
33
from typing import Any
44

55
from compose2pod.exceptions import UnsupportedComposeError
6-
from compose2pod.keys import SERVICE_KEYS, concat_list, pairs_to_mapping
6+
from compose2pod.keys import SERVICE_KEYS, as_list, pairs_to_mapping
77

88

99
# Merge policy for keys with a SERVICE_KEYS KeySpec comes from spec.merge (see
1010
# _merge below); these two sets cover only the remaining structural keys, which
11-
# have no KeySpec. Unifying structural-key merge policy is deferred — see
12-
# decisions/2026-07-12-reject-structural-key-registry.md's revisit trigger.
11+
# have no KeySpec. Both categories obey one rule: a merge may normalize a value
12+
# only through a form that key actually has, so it can never accept a shape the
13+
# gate would reject standalone.
1314
_STRUCTURAL_MERGE_KEYS = {"environment", "extra_hosts", "healthcheck", "depends_on"}
1415
_STRUCTURAL_CONCAT_KEYS = {"secrets", "configs", "volumes", "tmpfs", "env_file"}
1516

17+
# The only concat keys Compose gives a bare-string form. The gate accepts a
18+
# scalar for exactly these two and requires a list for the rest, so only these
19+
# two may be normalized scalar -> [scalar] on a merged side; normalizing any
20+
# other would accept a shape the gate refuses standalone.
21+
_SCALAR_FORM_KEYS = {"tmpfs", "env_file"}
22+
1623

1724
def _extends_target(name: str, ext: Any) -> str: # noqa: ANN401 - Compose values are untyped
1825
"""Return the referenced service name, after refusing cross-file and malformed forms."""
@@ -39,19 +46,54 @@ def _extends_target(name: str, ext: Any) -> str: # noqa: ANN401 - Compose value
3946
return service
4047

4148

49+
def _extra_hosts_to_mapping(name: str, value: list[Any]) -> dict[str, Any]:
50+
"""Normalize list-form `extra_hosts` ('host:ip') to a mapping.
51+
52+
Separated by a colon, not '=', so `pairs_to_mapping` would mangle the whole
53+
entry into a single `{'host:ip': None}` key. Split on the *first* colon only:
54+
an IPv6 address is itself full of them (`myhost:::1` -> `{'myhost': '::1'}`).
55+
"""
56+
result: dict[str, Any] = {}
57+
for item in value:
58+
if not isinstance(item, str) or ":" not in item:
59+
msg = f"service {name!r}: extra_hosts entries must be 'host:ip' strings"
60+
raise UnsupportedComposeError(msg)
61+
host, _sep, address = item.partition(":")
62+
result[host] = address
63+
return result
64+
65+
66+
def _as_concat_list(name: str, key: str, value: Any) -> list[Any]: # noqa: ANN401 - Compose values are untyped
67+
"""Normalize a structural concat key's value to a list, without widening the gate.
68+
69+
`keys.as_list` turns a bare string into a one-element list, which is right
70+
for `tmpfs`/`env_file` (Compose gives them a scalar form and the gate accepts
71+
one) and wrong for `volumes`/`secrets`/`configs`, where the gate requires a
72+
list -- normalizing there would let `extends` accept a scalar the same
73+
document would be refused for standalone.
74+
"""
75+
if isinstance(value, str) and key not in _SCALAR_FORM_KEYS:
76+
msg = f"service {name!r}: '{key}' must be a list"
77+
raise UnsupportedComposeError(msg)
78+
return as_list(name, key, value)
79+
80+
4281
def _as_mapping(name: str, key: str, value: Any) -> dict[str, Any]: # noqa: ANN401 - Compose values are untyped
4382
"""Normalize a structural mapping-merge key's value to a mapping.
4483
45-
Deliberately stricter than `keys.pairs_to_mapping`: list form is accepted
46-
only for `environment` and `depends_on`, the two keys Compose actually
47-
defines a list form for. `extra_hosts`/`healthcheck` in list form on a
48-
merged side are refused as an incompatible form rather than coerced.
84+
List form is accepted for exactly the keys Compose defines one for --
85+
`environment`, `extra_hosts`, `depends_on` -- each through the normalizer
86+
that key's own list form actually needs. `healthcheck` has no list form, so
87+
a list is refused rather than coerced: a merge must not accept a shape the
88+
gate would reject standalone (see `_merge`).
4989
"""
5090
if isinstance(value, dict):
5191
return value
5292
if isinstance(value, list):
5393
if key == "environment":
5494
return pairs_to_mapping(name, key, value)
95+
if key == "extra_hosts":
96+
return _extra_hosts_to_mapping(name, value)
5597
if key == "depends_on":
5698
for dep in value:
5799
if not isinstance(dep, str):
@@ -62,17 +104,32 @@ def _as_mapping(name: str, key: str, value: Any) -> dict[str, Any]: # noqa: ANN
62104
raise UnsupportedComposeError(msg)
63105

64106

65-
def _merge(base: dict[str, Any], local: dict[str, Any], name: str) -> dict[str, Any]:
66-
"""Merge `local` onto `base` per key category: mapping-merge, sequence-concat, else override."""
107+
def _merge(base: dict[str, Any], local: dict[str, Any], name: str, base_name: str) -> dict[str, Any]:
108+
"""Merge `local` onto `base` per key category: mapping-merge, sequence-concat, else override.
109+
110+
`name` is the extending service, `base_name` the one it extends: each side's
111+
failures are reported against the service the offending value actually
112+
belongs to.
113+
"""
67114
merged: dict[str, Any] = dict(base)
68115
for key, local_val in local.items():
69116
spec = SERVICE_KEYS.get(key)
70117
if key in base and spec is not None and spec.merge is not None:
118+
# A merge must never *widen* what the gate accepts. `resolve_extends`
119+
# runs ahead of `validate()`, so a normalizing merge (list -> mapping)
120+
# can launder a form the key does not have into one it does:
121+
# `ulimits: ["nofile=2"]` is refused standalone, but coercing it here
122+
# would produce a valid-looking `{"nofile": "2"}` that then sails
123+
# through the gate. Checking each side with the key's own validator
124+
# first derives the merge's accepted forms from the gate's, so the
125+
# two cannot drift apart.
126+
spec.validate(base_name, key, base[key])
127+
spec.validate(name, key, local_val)
71128
merged[key] = spec.merge(name, key, base[key], local_val)
72129
elif key in base and key in _STRUCTURAL_MERGE_KEYS:
73-
merged[key] = {**_as_mapping(name, key, base[key]), **_as_mapping(name, key, local_val)}
130+
merged[key] = {**_as_mapping(base_name, key, base[key]), **_as_mapping(name, key, local_val)}
74131
elif key in base and key in _STRUCTURAL_CONCAT_KEYS:
75-
merged[key] = concat_list(name, key, base[key], local_val)
132+
merged[key] = _as_concat_list(base_name, key, base[key]) + _as_concat_list(name, key, local_val)
76133
else:
77134
merged[key] = local_val
78135
return merged
@@ -115,7 +172,7 @@ def resolve(name: str) -> Any: # noqa: ANN401 - Compose values are untyped
115172
if not isinstance(base, dict):
116173
resolved[name] = local
117174
return local
118-
merged = _merge(base, local, name)
175+
merged = _merge(base, local, name, base_name)
119176
resolved[name] = merged
120177
return merged
121178

Lines changed: 126 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,126 @@
1+
---
2+
summary: An extends merge may normalize a value only through a form that key actually has, derived from the key's own validator, so a merge can no longer launder a shape the gate refuses standalone (ulimits/cap_add/volumes scalars and lists) nor refuse one it accepts (list-form extra_hosts).
3+
---
4+
5+
# Design: Derive the extends merge policy from each key's validator
6+
7+
## Summary
8+
9+
`resolve_extends` runs **ahead of** `validate()`. A merge that *normalizes* a
10+
value (list → mapping, scalar → list) therefore decides what the gate will
11+
subsequently see — and if it normalizes a form the key does not actually have,
12+
it turns an invalid document into a valid one. That is what it was doing.
13+
14+
One rule now governs every mergeable key: **a merge may normalize a value only
15+
through a form that key actually has.** For registry keys the accepted forms are
16+
derived from the key's own validator, so those cannot drift from the gate by
17+
construction. Structural keys have no `KeySpec`, so theirs stay declared in
18+
`extends.py` — kept honest by a test that asserts the invariant for *every*
19+
mergeable key and fails if a new one is added without a case.
20+
21+
## Motivation
22+
23+
The merge policy had drifted out of step with the gate in *both* directions.
24+
25+
**It laundered invalid input into valid** (the serious half). `ulimits` has no
26+
list form in Compose, and the gate refuses one:
27+
28+
```
29+
$ validate({"app": {"ulimits": ["nofile=2"]}}) -> 'ulimits' must be a mapping
30+
```
31+
32+
But arriving through `extends`, `_merge_map` coerced that list into
33+
`{"nofile": "2"}` *before* the gate ran, and the document sailed through. The
34+
same held for every list-only key given a bare scalar — `cap_add`, `devices`,
35+
`security_opt`, `group_add`, `volumes`, `secrets`, `configs` all refuse a scalar
36+
standalone, and all silently accepted one via `extends`.
37+
38+
**And it refused valid input.** `extra_hosts` *does* have a list form
39+
(`- "host:1.2.3.4"`), accepted standalone — but a merge rejected it as an
40+
"incompatible form".
41+
42+
The asymmetry was not cosmetic: it was two bugs pointing in opposite directions,
43+
both caused by the policy being an enumeration maintained by hand instead of
44+
being derived from what each key actually accepts.
45+
46+
## Design
47+
48+
**Registry keys** (`SERVICE_KEYS`): `_merge` runs `spec.validate` on *both* sides
49+
before `spec.merge`. The merge's accepted forms are therefore exactly the gate's,
50+
by construction. `ulimits: [...]` and `cap_add: "SCALAR"` now raise — with the
51+
validator's own message (`'cap_add' must be a list`), which is also the message
52+
the same value produces standalone.
53+
54+
**Structural keys** (no `KeySpec`): the two normalizers are narrowed to the forms
55+
Compose actually defines.
56+
57+
- `_as_concat_list` normalizes `scalar → [scalar]` only for `tmpfs` and
58+
`env_file` — the only concat keys with a bare-string form, and the only two the
59+
gate accepts a scalar for. `volumes`/`secrets`/`configs` must stay lists.
60+
- `_as_mapping` accepts list form for `environment`, `extra_hosts`, and
61+
`depends_on` — each through the normalizer that key's list form actually needs.
62+
`healthcheck` has no list form, so a list is still refused.
63+
64+
`extra_hosts` gets a real normalizer: its list form is `host:ip`, **colon**-
65+
separated, so routing it through `pairs_to_mapping` (which splits on `=`) would
66+
have silently produced a single `{'host:ip': None}` key. It splits on the *first*
67+
colon only, so an IPv6 address survives (`myhost:::1``{'myhost': '::1'}`).
68+
69+
No structural-key registry — `decisions/2026-07-12` stands. This derives policy
70+
from validators that already exist; it does not build a dispatch table.
71+
72+
## The invariant, pinned
73+
74+
A parametrized test asserts, for **every** mergeable key, that a form the gate
75+
refuses standalone is also refused through `extends`, using the shape that
76+
actually laundered (a bare scalar for the list-only keys, a list for the
77+
mapping-only ones) rather than merely any invalid value. A companion test
78+
asserts the table covers every mergeable key, so a new key cannot be added
79+
without a case — the drift this change exists to end.
80+
81+
Run against the pre-fix `extends.py` it goes red on nine keys (`cap_add`,
82+
`cap_drop`, `configs`, `devices`, `group_add`, `secrets`, `security_opt`,
83+
`ulimits`, `volumes`), which is the laundering it now prevents.
84+
85+
## Non-goals
86+
87+
- Not validating the whole service pre-merge (the heavier structural option).
88+
Deriving per-key from the validator closes the class with a much smaller
89+
change, and the invariant test guards the seam.
90+
- Not changing what the gate itself accepts. Every shape valid standalone before
91+
this change is still valid; every shape invalid standalone is now *also*
92+
invalid through `extends`.
93+
94+
## Behavior change
95+
96+
Three previously-accepted merges now raise, all of which produced documents the
97+
gate would have refused standalone:
98+
99+
- `ulimits` in list form on a merged side
100+
- any list-only key (`cap_add`, `devices`, `security_opt`, `group_add`,
101+
`volumes`, `secrets`, `configs`) given a bare scalar on a merged side
102+
- (unchanged, still refused) `healthcheck` in list form
103+
104+
One previously-refused merge now works: list-form `extra_hosts`.
105+
106+
Registry-key merge errors now come from the key's own validator (`'cap_add'
107+
must be a list`) rather than the vaguer `cannot merge 'cap_add' across
108+
incompatible forms`, matching what the same value yields standalone. Structural
109+
keys keep the `cannot merge ...` message.
110+
111+
Merge errors also now name the service the offending value belongs to: a
112+
malformed value in the *base* is reported against the base, not against the
113+
service extending it.
114+
115+
## Testing
116+
117+
`just test-ci` at 100%. The invariant test above is the centrepiece; new tests
118+
cover the `extra_hosts` list form (including IPv6 colon-safety and a
119+
missing-colon refusal) and the scalar-form narrowing.
120+
121+
## Risk
122+
123+
- **A user relying on the laundering** — e.g. a base with `cap_add: "NET_ADMIN"`
124+
(scalar) — now gets an error. This is the intended correction: that document
125+
was already invalid without `extends`, and the tool was accepting it only by
126+
accident of merge order. The error names the key and the required form.

0 commit comments

Comments
 (0)