Skip to content

Commit c948770

Browse files
authored
fix: model Docker's identifier grammar and split it from the volume discriminator (#146)
Closes #143.
1 parent a61a551 commit c948770

11 files changed

Lines changed: 323 additions & 37 deletions

File tree

‎CONTEXT.md‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,17 @@ shared namespace ([ADR-0003](docs/adr/0003-the-shared-namespace-decides-key-clas
6969
and the grammar of the `/etc/hosts` compose2pod writes
7070
([ADR-0006](docs/adr/0006-docker-rejection-parity.md)).
7171

72+
**Name grammar / volume discriminator**:
73+
Two different questions Docker answers with two different rules, once conflated
74+
here into one pattern. The *name grammar* (`values.TOP_LEVEL_NAME`,
75+
`[a-zA-Z0-9._-]+`) is what a top-level `services`/`volumes`/`secrets`/`configs`
76+
key and a service's long-form `networks` key must match. The *discriminator*
77+
(`parsing._is_named_volume_source`) asks a different thing of a short-form
78+
volume source — named volume, or bind? — and does not use the grammar at all:
79+
a leading `.`, `/` or `~` is a host path, anything else is a volume name. Say
80+
which one you mean; one pattern doing both jobs is the bug
81+
[ADR-0006](docs/adr/0006-docker-rejection-parity.md) records.
82+
7283
**Store**:
7384
The umbrella noun for a Compose `secret` or `config` — the two `StoreKind`s in
7485
`stores.py`. Use *store* for anything true of both, which is nearly everything.

‎compose2pod/parsing.py‎

Lines changed: 66 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -130,19 +130,45 @@ def _validate_service_healthcheck(name: str, svc: dict[str, Any]) -> None:
130130
_HEALTHCHECK_SCALAR_VALIDATORS[key](name, key, healthcheck[key])
131131

132132

133+
# A short-form volume source Docker reads as a host path rather than a volume
134+
# name. Measured over 19 spellings against `docker compose config` v5.1.2.
135+
_BIND_SOURCE_PREFIXES = (".", "/", "~")
136+
137+
138+
def _is_named_volume_source(source: str) -> bool:
139+
r"""Whether a SHORT-form volume source names a volume, as Docker reads it.
140+
141+
A leading `.`, `/` or `~` makes it a host path; every other spelling is a
142+
volume name, `a/b`, `a b` and `a@b` included -- each measured as
143+
`refers to undefined volume` with no top-level declaration. The name
144+
grammar does not decide this and must not be asked to: it would call
145+
those three binds, which is exactly how they stayed accepted until issue
146+
143 measured them. An earlier split used this same leading-character rule
147+
without `~` and swept `~/data` into "named" wrongly; `~` was the missing
148+
prefix, not the grammar.
149+
150+
A `${VAR}`-carrying source stays unnamed on purpose: what it resolves to
151+
is a fact about the shell that will run the script, not about the
152+
document, the carve-out `_validate_volume_references` documents.
153+
154+
No drive-shaped entry (`C:\\data:/var`, `v:/data`) reaches this:
155+
`_validate_service_volumes` refuses the family first, so a `named` verdict
156+
here always names a volume Docker would name too.
157+
"""
158+
return not source.startswith(_BIND_SOURCE_PREFIXES) and not values.has_variable(source)
159+
160+
133161
def _classify_volume(volume: str) -> tuple[str, str | None]:
134162
r"""Classify one short-syntax volume entry: its kind, and its source when the kind is 'named'.
135163
136164
Returns `("anonymous", None)` for a colon-less entry (a bare container
137-
path), `("named", source)` for a colon-form entry whose source matches
138-
Docker's own volume-name grammar (`stores.NAME_PATTERN`, the identical
139-
pattern a secret/config name is checked against:
140-
`^[a-zA-Z0-9][a-zA-Z0-9_.-]*$`), or `("bind", None)` for every other
141-
colon-form entry -- a relative (`./rel`) or absolute (`/abs`) host path, a
142-
`~`-prefixed home-relative path (`~/data`, measured ACCEPT against
143-
`docker compose config` v5.1.2 with no top-level declaration -- Docker
144-
expands it against the invoking user's home directory), or a `${VAR}`
145-
reference (`$`, `{`, `}` are none of them pattern characters either).
165+
path), `("named", source)` per `_is_named_volume_source` above, or
166+
`("bind", None)` for every other colon-form entry -- a relative (`./rel`)
167+
or absolute (`/abs`) host path, a `~`-prefixed home-relative path
168+
(`~/data`, measured ACCEPT against `docker compose config` v5.1.2 with no
169+
top-level declaration -- Docker expands it against the invoking user's
170+
home directory), or a `${VAR}` reference (`$`, `{`, `}` are none of them
171+
name-grammar characters either).
146172
147173
Shared by `_validate_service_volumes` (only cares whether an entry is
148174
anonymous, to check its own shape) and `_named_volume_source` (only cares
@@ -162,7 +188,7 @@ def _classify_volume(volume: str) -> tuple[str, str | None]:
162188
if ":" not in volume:
163189
return "anonymous", None
164190
source, _, _ = volume.partition(":")
165-
if stores.NAME_PATTERN.fullmatch(source):
191+
if _is_named_volume_source(source):
166192
return "named", source
167193
return "bind", None
168194

@@ -427,14 +453,17 @@ def _named_volume_source(volume: object) -> str | None:
427453
"""Return a volume entry's bare-identifier named source, or None if it needs no declaration.
428454
429455
A colon-form short-syntax string is classified via `_classify_volume`. A
430-
long-syntax mapping needs its own check: only a `volume`-type entry names
431-
a volume at all, and only when its `source` is a bare identifier (an
456+
long-syntax mapping needs its own check, and a laxer one: `type` has
457+
already said which kind the entry is, so a `volume`-type entry names a
458+
volume whatever its `source` looks like -- `source: /abs` and
459+
`source: ./r` are both `refers to undefined volume` to Docker, where the
460+
short form reads the same strings as host paths (measured, v5.1.2). An
432461
absent source is an anonymous volume; a `bind`/`tmpfs` entry's `source`,
433-
if any, is a host path, never a name to cross-check).
462+
if any, is a host path, never a name to cross-check.
434463
"""
435464
if isinstance(volume, dict):
436465
source = volume.get("source")
437-
if volume.get("type") == "volume" and isinstance(source, str) and stores.NAME_PATTERN.fullmatch(source):
466+
if volume.get("type") == "volume" and isinstance(source, str) and not values.has_variable(source):
438467
return source
439468
return None
440469
# _validate_service_volumes has already confirmed every non-dict entry is
@@ -1105,9 +1134,8 @@ def _validate_volume_references(compose: dict[str, Any], services: dict[str, Any
11051134
undeclared when it resolves to a bare identifier, a fact about the shell
11061135
that will later run the generated script, not about this document, so it
11071136
does not bind here either way. `_named_volume_source` already returns None
1108-
for any `${...}`-carrying source unconditionally -- `$`, `{`, and `}` are
1109-
none of them `stores.NAME_PATTERN` characters -- so this function never
1110-
needs to ask `values.has_variable` itself.
1137+
for any `${...}`-carrying source, in both syntaxes and by an explicit
1138+
`values.has_variable` test, so this function never needs to ask again.
11111139
"""
11121140
top_volumes = compose.get("volumes")
11131141
if top_volumes is not None and not isinstance(top_volumes, dict):
@@ -1175,6 +1203,7 @@ def _validate_network_entry_value(name: str, network: str, value: Any) -> None:
11751203
can never satisfy "must be a mapping" regardless of its runtime value --
11761204
the rejection is a fact about the document, not the host.
11771205
"""
1206+
values.validate_compose_name(f"service {name!r}: network name", network)
11781207
if value is None:
11791208
return
11801209
if not isinstance(value, dict):
@@ -1558,6 +1587,25 @@ def _validate_top_level_scalar_strings(compose: dict[str, Any]) -> None:
15581587
raise UnsupportedComposeError(msg)
15591588

15601589

1590+
# The top-level blocks whose KEYS Docker holds to its name grammar. `networks`
1591+
# is absent by measurement, not oversight: `networks: {"a b": {}}` on its own
1592+
# is accepted by `docker compose config` v5.1.2, and the grammar bites on the
1593+
# service's long-form `networks` mapping key instead (`_validate_network_entries`).
1594+
NAME_CHECKED_TOP_LEVEL_BLOCKS = ("services", "volumes", "secrets", "configs")
1595+
1596+
1597+
def _validate_top_level_names(compose: dict[str, Any]) -> None:
1598+
"""Every key of every name-checked top-level block, against Docker's one grammar.
1599+
1600+
A block that is not a mapping is left to the validator that owns its shape.
1601+
"""
1602+
for block in NAME_CHECKED_TOP_LEVEL_BLOCKS:
1603+
definitions = compose.get(block)
1604+
if isinstance(definitions, dict):
1605+
for name in definitions:
1606+
values.validate_compose_name(f"top-level {block!r}: name", name)
1607+
1608+
15611609
def validate(compose: dict[str, Any]) -> list[str]:
15621610
"""Check the compose document against the supported subset.
15631611
@@ -1579,6 +1627,7 @@ def validate(compose: dict[str, Any]) -> list[str]:
15791627
raise UnsupportedComposeError(msg)
15801628
_reject_null_top_level_blocks(compose)
15811629
_validate_top_level_scalar_strings(compose)
1630+
_validate_top_level_names(compose)
15821631
if "networks" in compose:
15831632
warnings.append("ignoring top-level 'networks' (all services share the pod namespace)")
15841633
if "volumes" in compose:

‎compose2pod/stores.py‎

Lines changed: 4 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -19,13 +19,6 @@
1919

2020

2121
_LONG_FORM_KEYS = {"source", "target", "uid", "gid", "mode"}
22-
# Docker's shared identifier grammar for a secret/config *name* -- and, per
23-
# `parsing._named_volume_source`, for telling a named-volume reference apart
24-
# from a bind-mount source (a relative or absolute path, or a `~`-prefixed
25-
# home-relative path all fail this pattern, since none of `.`, `/`, `~` is a
26-
# pattern character). Public (no leading underscore) because parsing.py, in a
27-
# different module, reuses it rather than duplicating it.
28-
NAME_PATTERN = re.compile(r"^[a-zA-Z0-9][a-zA-Z0-9_.-]*$")
2922
_ENV_NAME = re.compile(r"^[a-zA-Z_][a-zA-Z0-9_]*$")
3023

3124

@@ -67,9 +60,6 @@ class StoreKind:
6760

6861

6962
def _validate_def(name: str, definition: Any, kind: StoreKind) -> None: # noqa: ANN401 - Compose values are untyped
70-
if not NAME_PATTERN.fullmatch(name):
71-
msg = f"{kind.label} name {name!r} must match [a-zA-Z0-9][a-zA-Z0-9_.-]*"
72-
raise UnsupportedComposeError(msg)
7363
if not isinstance(definition, dict):
7464
msg = f"{kind.label} {name!r} must be a mapping"
7565
raise UnsupportedComposeError(msg)
@@ -209,6 +199,10 @@ def _create(definition: dict[str, Any], store: str, project_dir: str) -> CreateS
209199
Whichever source the store declares, the expandable text is bound once and
210200
both outputs are read off that binding, so a line cannot expand a variable
211201
the caller is never told about.
202+
203+
`store` is interpolated unquoted, which is safe because the gate holds
204+
every top-level key to `values.NAME_GRAMMAR` -- a check that lives in
205+
`parsing`, not here, since issue 143 made it one grammar for five blocks.
212206
"""
213207
if isinstance(definition.get("file"), str):
214208
value = str(Path(project_dir, definition["file"]))

‎compose2pod/values.py‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -309,6 +309,32 @@ def validate_duration(name: str, key: str, value: Any) -> None: # noqa: ANN401
309309
raise UnsupportedComposeError(msg)
310310

311311

312+
NAME_GRAMMAR = re.compile(r"^[a-zA-Z0-9._-]+\Z")
313+
314+
315+
def validate_compose_name(where: str, name: str) -> None:
316+
"""Refuse an identifier Docker's schema will not take in the position `where` names.
317+
318+
One grammar, `[a-zA-Z0-9._-]+`, measured against `docker compose config`
319+
v5.1.2 and pinned by `tests/conformance/test_matrix.py`. A leading `.`,
320+
`-` or `_` is legal, which is where the old store-only pattern was
321+
stricter than Docker. `where` is pre-formatted by the caller, as
322+
`keys.require_string_keys` takes one, because the positions this is
323+
enforced from share no argument shape.
324+
325+
Where Docker enforces it is not where it reads most naturally: the keys of
326+
top-level `services`, `volumes`, `secrets` and `configs`, and a service's
327+
long-form `networks` mapping key, but NOT a top-level `networks:` key --
328+
`networks: {"a b": {}}` alone is a document Docker accepts.
329+
330+
This is rule one, not a narrowing: a document carrying one of these is a
331+
document `docker compose config` rejects outright.
332+
"""
333+
if not NAME_GRAMMAR.fullmatch(name):
334+
msg = f"{where} {name!r} must match [a-zA-Z0-9._-]+"
335+
raise UnsupportedComposeError(msg)
336+
337+
312338
_UNSPELLABLE_IN_HOSTS_FILE = frozenset(" \t\n\r\v\f#")
313339

314340

‎docs/adr/0006-docker-rejection-parity.md‎

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,27 @@ named volume) ([#114](https://github.com/modern-python/compose2pod/issues/114)),
7979
`mode` is refused at the other end of the range, where podman 6.0.1's `crun` will not mount it.
8080
The `integration` job pins `ubuntu-24.04` for the same reason: it is the runner that ships the
8181
floor, and on a newer one the job would measure a podman no user of the floor has.
82-
The hard rule has no exceptions left. It had two, and both were the same one: a `depends_on`
82+
The hard rule has no exceptions left. It had three classes, and the third outlived the sentence
83+
that first said this. Docker's identifier grammar, `[a-zA-Z0-9._-]+`, was never modelled: a
84+
service, volume, secret or config could be named `a b`, `a/b` or `""`, all of which
85+
`docker compose config` refuses outright and all of which compose2pod took
86+
([#143](https://github.com/modern-python/compose2pod/issues/143)). One pattern in `values`
87+
holds it now, applied where Docker applies it -- which is not where it reads: the keys of
88+
top-level `services`, `volumes`, `secrets` and `configs`, and a service's *long-form* `networks`
89+
mapping key, but neither a top-level `networks:` key nor a short-form list entry, each measured
90+
rather than assumed and each pinned. The same pattern had a second job it was quietly bad at:
91+
`stores.NAME_PATTERN` demanded an alphanumeric first character Docker does not, over-rejecting
92+
`.a`, `-a` and `_a`, while doubling as the short-form bind-versus-named-volume test, which is not
93+
a grammar question at all: Docker reads a leading `.`, `/` or `~` as a host path and every other
94+
spelling as a volume name, `a/b` and `a b` included. One pattern could not be both -- widening it
95+
would have read `.env:/app/.env` as a named volume, and leaving it would have kept `a/b:/x`
96+
accepted where Docker says `refers to undefined volume`. They are two rules now, and the long
97+
form is a third: there `type` has already said which kind the entry is, so any source names a
98+
volume.
99+
Why the harness never saw it: the generated matrix varies a key's *value* over hostile shapes and
100+
never touches a map key, so no probe could reach a name. `test_identifier_grammar_matches_docker`
101+
now generates the missing axis from the same tuple the gate reads.
102+
The first two exceptions were the same one: a `depends_on`
83103
naming an undefined service, and a dependency cycle, among services outside the target's closure
84104
were rejected by Docker and accepted here
85105
([#87](https://github.com/modern-python/compose2pod/issues/87)), because both checks lived in the
Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
# Measured ACCEPT by `docker compose config` v5.1.2: a top-level `networks:` key is NOT
2+
# held to the identifier grammar the other blocks' keys are, and neither is a service's
3+
# short-form (list) reference to one. Only the long-form mapping key is. Pinned so the
4+
# grammar cannot be tidied into applying everywhere, which would over-reject (issue 143).
5+
services:
6+
app:
7+
image: nginx
8+
networks:
9+
- "a b"
10+
networks:
11+
"a b": {}

‎tests/conformance/test_corpus.py‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -253,3 +253,16 @@ def test_extra_hosts_address_newline_is_a_catalogued_over_rejection(
253253
"""
254254
path = Path(__file__).parent / "corpus" / "service_extra_hosts_address_newline.yaml"
255255
assert assert_rule(yaml.safe_load(path.read_text())) == "over-reject"
256+
257+
258+
def test_top_level_network_name_is_accepted_by_both(
259+
assert_rule: Callable[[dict[str, Any]], str],
260+
) -> None:
261+
"""Docker's identifier grammar does not reach a top-level `networks:` key.
262+
263+
Pinned to `both-accept` rather than left to the generic run, which tolerates an
264+
over-rejection: the risk here is the opposite of the usual one, that the grammar
265+
issue 143 added gets extended to a position Docker leaves alone.
266+
"""
267+
path = Path(__file__).parent / "corpus" / "networks_top_level_name_unchecked.yaml"
268+
assert assert_rule(yaml.safe_load(path.read_text())) == "both-accept"

‎tests/conformance/test_matrix.py‎

Lines changed: 54 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@
1111
import pytest
1212

1313
from compose2pod.keys import SERVICE_KEYS, STRUCTURAL_KEYS
14-
from compose2pod.parsing import IGNORED_SERVICE_KEYS
14+
from compose2pod.parsing import IGNORED_SERVICE_KEYS, NAME_CHECKED_TOP_LEVEL_BLOCKS
1515

1616

1717
KEYS = sorted(set(SERVICE_KEYS) | STRUCTURAL_KEYS | set(IGNORED_SERVICE_KEYS))
@@ -84,3 +84,56 @@ def test_docker_rejection_implies_our_rejection(
8484
pytest.skip("host-dependent shape -- see CARVE_OUT's comment")
8585
compose = {"services": {"app": {"image": "nginx:alpine", key: SHAPES[shape]}}}
8686
assert_rule(compose)
87+
88+
89+
REFUSED_NAMES = ["a b", "a/b", "a:b", "a#b", "ab!", "a+b", "a~b", "a@b", "a$b", "", "\xe4"]
90+
# The four Docker takes that the old store-only pattern refused -- the over-rejection
91+
# issue 143 lifted. They carry `both-accept` below, so lifting it cannot silently undo.
92+
ACCEPTED_NAMES = [".a", "-a", "_a", "a.b"]
93+
# The full set on the two positions the grammar is enforced from, a hostile pair on the
94+
# remaining blocks: Docker applies one grammar to all of them (measured), and the pair is
95+
# what would catch a block that stopped sharing it.
96+
CROSS_BLOCK_NAMES = ("a b", "")
97+
_OTHER_BLOCKS = [block for block in NAME_CHECKED_TOP_LEVEL_BLOCKS if block != "services"]
98+
NAME_POSITIONS = (
99+
[("services", name, "both-reject") for name in REFUSED_NAMES]
100+
+ [("services", name, "both-accept") for name in ACCEPTED_NAMES]
101+
+ [("service-networks", name, "both-reject") for name in REFUSED_NAMES]
102+
+ [("service-networks", name, "both-accept") for name in ACCEPTED_NAMES]
103+
+ [(block, name, "both-reject") for block in _OTHER_BLOCKS for name in CROSS_BLOCK_NAMES]
104+
)
105+
106+
107+
def _document_named(position: str, name: str) -> dict[str, Any]:
108+
"""Build a document whose only variable is one identifier, in the position under probe."""
109+
app: dict[str, Any] = {"image": "nginx:alpine"}
110+
compose: dict[str, Any] = {"services": {"app": app}}
111+
if position == "services":
112+
compose["services"][name] = {"image": "nginx:alpine"}
113+
elif position == "service-networks":
114+
app["networks"] = {name: {}}
115+
compose["networks"] = {name: {}}
116+
elif position in ("secrets", "configs"):
117+
compose[position] = {name: {"file": "./f"}}
118+
else:
119+
compose[position] = {name: {}}
120+
return compose
121+
122+
123+
@pytest.mark.parametrize(("position", "name", "verdict"), NAME_POSITIONS, ids=lambda value: repr(value).strip("'"))
124+
def test_identifier_grammar_matches_docker(
125+
position: str,
126+
name: str,
127+
verdict: str,
128+
assert_rule: Callable[[dict[str, Any]], str],
129+
) -> None:
130+
"""Both directions, per position: what Docker refuses we refuse, what it takes we take.
131+
132+
The verdict is asserted rather than left to `assert_rule` alone, which tolerates an
133+
over-rejection -- and an over-rejection on `ACCEPTED_NAMES` is precisely the defect
134+
issue 143 removed, so the generic contract cannot see the thing under test.
135+
136+
Generated from `NAME_CHECKED_TOP_LEVEL_BLOCKS` so a block added to that tuple is
137+
probed the moment it is added, the way a new `SERVICE_KEYS` entry is above.
138+
"""
139+
assert assert_rule(_document_named(position, name)) == verdict

0 commit comments

Comments
 (0)