Repository navigation
Keep the port protocol in generated snippets - #63
Merged
Merged
Conversation
A service that publishes the same port on both protocols rendered an invalid
compose file. The generator dropped the `/proto` suffix from the Podman Compose
and Podman CLI snippets, so a compose.yaml declaring
ports:
- 53:53/udp
- 53:53/tcp
produced
ports:
- "53:53"
- "53:53"
Podman rejects the duplicate mapping. Where a runtime tolerates it, the
copy-pasted result is TCP-only: a DNS resolver that answers `dig +tcp` and
nothing a normal stub resolver sends, which looks like a broken image rather
than a broken snippet. The AppJail snippets always carried `proto:udp` /
`proto:tcp`, and so did the parameters table, so the protocol was known and only
the docker-style mappings lost it.
Fixed with a `proto_suffix()` macro that emits the suffix only when the protocol
is not the `tcp` default. A bare `4000:4000` already means tcp for both compose
and `podman run -p`, so emitting it everywhere would churn every README in the
catalogue for no benefit; this way single-protocol images render byte-identical
to before. The comparison is case-insensitive and the value is emitted
lowercased, because compose accepts `protocol: TCP` and `53:53/UDP` and those
were stored verbatim — a raw comparison would have appended `/TCP` to images
that should have been untouched.
Also fixes the single-field short form in config.py. `ports: ["53/udp"]` is one
string for both ports, so the protocol stayed stuck on the published port and
the snippet rendered `53/udp:53/udp`. Stripped at parse time rather than in the
template, since `published` is documented as a port number and every consumer
of cfg.ports would otherwise need the same guard.
Separately, the implementation-details line contradicted itself for images that
run as root:
**User:** `root` (UID/GID via PUID/PGID, defaults to 1000:1000)
That tells the reader both that the container runs as root and that it runs as
1000:1000. A root image stays root deliberately — binding a port below 1024
needs it, and FreeBSD has no capability mechanism to delegate one. The line is
now conditional, and the wording states only what is certain, that the service
process stays root. It deliberately does not claim PUID/PGID are unused: a root
image can still honour them for the base init's chown, and some READMEs print a
PUID row in the env table on the same page.
Applied to both README.j2 and README.mkdocs.j2. The mkdocs template overrides
the implementation_details block, so a fix in README.j2 alone never reaches the
docs site, which is the page most readers see. Both now carry a comment pointing
at the other.
Known limitation, left alone deliberately: substitution of the configurable port
is keyed on `loop.first`, so for a dual-protocol service only the first mapping
is parameterised and the docs-site port control splits the service across two
host ports. That predates this change and fixing it means keying on target port
across README.j2 and appjail-director.yml.j2, which affects every multi-port
image. Filed separately rather than widened into this one.
Not covered by tests. The existing suite passes (46 docs tests), but neither new
behaviour has a test; worth adding if the project wants the gate to hold.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this fixes
53:53lines in its compose andpodman runsnippets. UDP was lost."53/udp"parsed wrong. The protocol stayed on the published port, giving53/udp:53/udp.rootand as1000:1000.Before / after
Same compose input (
53:53/udp,53:53/tcp,user: root), rendered onmainand on this branch.Compose snippet:
podman runsnippet:Implementation details:
Parsed
ports: ["53/udp"]:How
proto_suffix()macro inREADME.j2adds/<proto>only when it isn'ttcp. TCP is the default for compose andpodman run -p, so TCP-only images render byte-identical. The comparison ignores case, soprotocol: TCPdoesn't add a suffix.config.pystrips/<proto>frompublishedin the single-field short form.config.pylowercases the protocol at parse time, in both the compose and legacyconfig.yamlpaths.53:53/UDPused to reach the AppJail snippet asproto:UDP. An empty legacyprotocol:now defaults totcp, where it used to render asproto:None.User:line is conditional onuser == "root"in bothREADME.j2andREADME.mkdocs.j2. The mkdocs template overrides that block, so fixing only one would miss the docs site.Tests
New
tests/test_port_protocol.py, 7 tests:mainpodman run53/udp: parses topublished: 53, renders53:53/udp1000:1000claim (README + docs site)/tcpadded8080:8080/TCP: no suffix, AppJail getsproto:tcp53:53/UDP: lowercased in compose, AppJail andpodman runThe two that pass on
mainare regression guards. They make sure the fix doesn't change output it shouldn't.Full suite: 579 tests, no failures.
test_screenshotfails to import on bothmainand this branch, because it needspytestand the test environment has none. That is unrelated to this change.ruffwas not run.