diff --git a/dbuild/config.py b/dbuild/config.py index e0d1395..72e6848 100644 --- a/dbuild/config.py +++ b/dbuild/config.py @@ -655,11 +655,20 @@ 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", "")) 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: @@ -675,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/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/)) 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()