From 8d20fff313bfa8a4fd4b30200bf59f14e508ee94 Mon Sep 17 00:00:00 2001 From: jtrotsky <5625795+jtrotsky@users.noreply.github.com> Date: Sun, 1 Nov 2026 15:06:39 +1300 Subject: [PATCH 1/2] Keep the port protocol in generated snippets; fix the root-user line MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- dbuild/config.py | 7 +++++ dbuild/templates/README.j2 | 52 ++++++++++++++++++++++++++++--- dbuild/templates/README.mkdocs.j2 | 7 +++++ 3 files changed, 62 insertions(+), 4 deletions(-) diff --git a/dbuild/config.py b/dbuild/config.py index e0d1395..d1f37b5 100644 --- a/dbuild/config.py +++ b/dbuild/config.py @@ -655,6 +655,13 @@ def _parse_service_data( proto = "tcp" if "/" in tgt: tgt, proto = tgt.split("/", 1) + # The single-field short form is one string for BOTH ports, so + # `53/udp` leaves the protocol stuck on the published port too and + # the snippet renders `53/udp:53/udp`. Strip it here rather than in + # the template: `published` is documented as a port number, and + # every consumer of cfg.ports would otherwise need the same guard. + if "/" in pub: + pub = pub.split("/", 1)[0] else: pub = str(p.get("published", "")) tgt = str(p.get("target", "")) diff --git a/dbuild/templates/README.j2 b/dbuild/templates/README.j2 index 1cd8be9..d5293d4 100644 --- a/dbuild/templates/README.j2 +++ b/dbuild/templates/README.j2 @@ -31,6 +31,31 @@ {{ v.path }}{{ ":" ~ v.options if v.options else "" }} {%- endmacro -%} +{#- A port's `/proto` suffix, emitted only when it is load-bearing. + + tcp is the default for both compose and `podman run -p`, so a bare + `4000:4000` already means tcp and adding the suffix everywhere would churn + every README in the catalogue to no benefit. + + It IS load-bearing when a service publishes the same port on both + protocols. Without the suffix the two entries render identically — blocky + declares `53:53/udp` and `53:53/tcp` and the snippet emitted `53:53` twice, + which podman rejects as a duplicate mapping. Where a runtime tolerates it, + the copy-pasted result is TCP-only DNS: a resolver that answers `dig +tcp` + and nothing a normal client sends. + + The parameters table (`p.protocol|upper`) was always right; only the + copy-pasteable snippets dropped it. + + Compared case-insensitively and emitted lowercased: compose accepts + `protocol: TCP` and `53:53/UDP`, and config.py stores whatever the author wrote + verbatim. A raw comparison would let `TCP` through as a non-default and append + `/TCP` to every port of an image that should have been left alone. -#} +{%- macro proto_suffix(p) -%} +{%- set proto = (p.protocol | default("", true) | lower) -%} +{{ "/" ~ proto if proto and proto != "tcp" else "" }} +{%- endmacro -%} + {%- macro compose_snippet(override_port=None) -%} services: {{ name }}: @@ -56,9 +81,9 @@ services: ports: {%- for p in ports %} {%- if loop.first %} - - "{{ override_port if override_port else port_var }}:{{ p.target }}" + - "{{ override_port if override_port else port_var }}:{{ p.target }}{{ proto_suffix(p) }}" {%- else %} - - "{{ p.port }}:{{ p.target }}" + - "{{ p.port }}:{{ p.target }}{{ proto_suffix(p) }}" {%- endif %} {%- endfor %} {%- endif %} @@ -202,9 +227,9 @@ podman run -d --name {{ name }} \ {%- endif %} {%- for p in ports %} {%- if loop.first %} - -p {{ override_port if override_port else port_var }}:{{ p.target }} \ + -p {{ override_port if override_port else port_var }}:{{ p.target }}{{ proto_suffix(p) }} \ {%- else %} - -p {{ p.port }}:{{ p.target }} \ + -p {{ p.port }}:{{ p.target }}{{ proto_suffix(p) }} \ {%- endif %} {%- endfor %} {%- for a in annotations %} @@ -645,7 +670,26 @@ Open `http://:{{ ports[0].port if render_mode != "mkdocs" else port_var {%- block implementation_details %} **Architectures:** {{ architectures | default(['amd64']) | join(', ') }} +{#- PUID/PGID only mean something for an image whose init drops privilege. A + root image (`user: "root"` in x-daemonless) stays root on purpose — blocky + binds :53, and FreeBSD reserves ports below 1024 to root with no capability + mechanism to delegate one. Printing "root (UID/GID via PUID/PGID, defaults + to 1000:1000)" told the reader both that it runs as root and that it runs as + 1000:1000. + + The wording says what is certain — the PROCESS stays root — and deliberately + does NOT claim PUID/PGID are unused. A root image can still honour them for + the base init's chown of /config, and some READMEs print a PUID row in the + env table on the same page, so "not used" would contradict the document a few + lines up. + + 🔴 README.mkdocs.j2 OVERRIDES this whole block. A fix applied only here never + reaches the docs site. Both are updated; keep them in step. -#} +{%- if user == "root" %} +**User:** `root` — the service process stays root by design, not dropped to `bsd` +{%- else %} **User:** `{{ user }}` (UID/GID via PUID/PGID, defaults to 1000:1000) +{%- endif %} **Base:** FreeBSD {{ base_version }} {%- endblock %} {%- block community %} diff --git a/dbuild/templates/README.mkdocs.j2 b/dbuild/templates/README.mkdocs.j2 index 260dff6..6be795f 100644 --- a/dbuild/templates/README.mkdocs.j2 +++ b/dbuild/templates/README.mkdocs.j2 @@ -332,7 +332,14 @@ Save as `run.sh`, then run `sh run.sh`. !!! info "Implementation Details" - **Architectures:** {{ architectures | default(['amd64']) | join(', ') }} +{#- Mirrors the same conditional in README.j2. This block OVERRIDES that one, so a fix + applied only there silently never reaches the docs site — which is the page most + readers actually see. Keep the two in step. -#} +{%- if user == "root" %} + - **User:** `root` — the service process stays root by design, not dropped to `bsd`. +{%- else %} - **User:** `{{ user }}` (UID/GID set via [PUID/PGID](../guides/permissions.md)). Defaults to `1000:1000`. +{%- endif %} - **Base:** Built on `@REGISTRY@/base` (FreeBSD {{ base_version }}). {%- if mlock %} - **mlock:** Requires `--annotation 'org.freebsd.jail.allow.mlock=true'` ([ocijail 0.5.0+](/guides/ocijail-patch/)) From 63b688e9d7761a7d7c0f6bd3987b5527545a1d83 Mon Sep 17 00:00:00 2001 From: jtrotsky <5625795+jtrotsky@users.noreply.github.com> Date: Sun, 1 Nov 2026 16:21:52 +1300 Subject: [PATCH 2/2] fix pr --- dbuild/config.py | 6 +- tests/test_port_protocol.py | 108 ++++++++++++++++++++++++++++++++++++ 2 files changed, 112 insertions(+), 2 deletions(-) create mode 100644 tests/test_port_protocol.py diff --git a/dbuild/config.py b/dbuild/config.py index d1f37b5..72e6848 100644 --- a/dbuild/config.py +++ b/dbuild/config.py @@ -666,7 +666,9 @@ def _parse_service_data( pub = str(p.get("published", "")) tgt = str(p.get("target", "")) proto = p.get("protocol", "tcp") - ports.append({"published": pub, "target": tgt, "protocol": proto}) + # Compose accepts `TCP`/`53:53/UDP`; every consumer compares or emits + # lowercase (README suffix, appjail `proto:`), so normalise once here. + ports.append({"published": pub, "target": tgt, "protocol": str(proto or "tcp").lower()}) # 2. Fallback to legacy config.yaml (if compose missing or lists empty) if not env: @@ -682,7 +684,7 @@ def _parse_service_data( ports.append({ "published": str(p.get("port", "")), "target": str(p.get("port", "")), - "protocol": p.get("protocol", "tcp") + "protocol": str(p.get("protocol") or "tcp").lower() }) return env, volumes, ports diff --git a/tests/test_port_protocol.py b/tests/test_port_protocol.py new file mode 100644 index 0000000..7e8025d --- /dev/null +++ b/tests/test_port_protocol.py @@ -0,0 +1,108 @@ +"""Port protocol in generated snippets, and the root-user implementation line.""" + +from __future__ import annotations + +import argparse +import tempfile +import unittest +from pathlib import Path + +from dbuild import config as dbuild_config +from dbuild import docs + +from tests.test_docs import CONTAINERFILE_J2, _chdir + +COMPOSE = """\ +name: dns +x-daemonless: + title: "DNS" + description: "A resolver." + category: "Network" + icon: ":test:" + upstream_url: "https://example.com" + user: "{user}" +services: + dns: + image: ghcr.io/daemonless/dns:latest + ports: +{ports} +""" + + +def _render(ports: list[str], user: str = "bsd") -> tuple[dict, str, str, str]: + """Return (cfg.ports[0], README.md, docs-site page, podman run snippet). + + The podman run snippet only reaches a page when there is no compose snippet + (CLI images), so it is rendered straight from its macro. + """ + port_lines = "\n".join(f" - {p}" for p in ports) + with tempfile.TemporaryDirectory() as d: + repo = Path(d) + (repo / "compose.yaml").write_text(COMPOSE.format(user=user, ports=port_lines)) + (repo / "Containerfile.j2").write_text(CONTAINERFILE_J2) + args = argparse.Namespace(community=None) + with _chdir(repo): + cfg = dbuild_config.load(repo) + outputs, errors = docs.render_generated(cfg, args, repo) + context = docs._enrich_metadata(cfg, None) + env = docs._get_jinja_env(repo) + site = env.get_template("README.mkdocs.j2").render(context, render_mode="mkdocs") + run = env.get_template("README.j2").make_module( + dict(context, render_mode="github") + ).cli_snippet() + assert not errors, errors + return cfg.ports[0], outputs["README.md"], site, run + + +class TestPortProtocol(unittest.TestCase): + def test_dual_protocol_keeps_both_mappings_distinct(self): + _, readme, site, run = _render(['"53:53/udp"', '"53:53/tcp"']) + # udp keeps its suffix; tcp stays bare because it is the default + self.assertIn('- "53:53/udp"\n - "53:53"\n', readme) + self.assertIn(':53/udp"\n - "53:53"\n', site) + self.assertIn("-p 53:53/udp \\\n -p 53:53 \\", run) + + def test_tcp_only_image_renders_unchanged(self): + _, readme, _, _ = _render(['"8080:8080"']) + self.assertNotIn("/tcp", readme) + + def test_uppercase_tcp_is_still_the_default(self): + _, readme, site, _ = _render(['"8080:8080/TCP"']) + for page in (readme, site): + self.assertNotIn("/TCP", page) + self.assertNotIn("/tcp", page) + # the AppJail snippet emits the protocol verbatim, so it must arrive lowercase + self.assertNotIn("proto:TCP", page) + self.assertIn("proto:tcp", page) + + def test_uppercase_udp_is_lowercased_everywhere(self): + _, readme, _, run = _render(['"53:53/UDP"']) + self.assertIn('- "53:53/udp"', readme) + self.assertIn("proto:udp", readme) + self.assertNotIn("UDP'", readme) + self.assertIn("-p 53:53/udp", run) + + def test_single_field_short_form_strips_proto_from_published(self): + port, readme, _, _ = _render(['"53/udp"']) + self.assertIn('- "53:53/udp"', readme) + self.assertNotIn("53/udp:", readme) + self.assertEqual(port["published"], "53") + self.assertEqual(port["target"], "53") + self.assertEqual(port["protocol"], "udp") + + +class TestRootUserLine(unittest.TestCase): + def test_root_image_does_not_claim_1000(self): + _, readme, site, _ = _render(['"53:53/udp"'], user="root") + for page in (readme, site): + self.assertIn("stays root by design", page) + self.assertNotIn("`root` (UID/GID", page) + + def test_non_root_image_keeps_puid_line(self): + _, readme, site, _ = _render(['"8080:8080"']) + self.assertIn("**User:** `bsd` (UID/GID via PUID/PGID", readme) + self.assertIn("**User:** `bsd` (UID/GID set via", site) + + +if __name__ == "__main__": + unittest.main()