From a49d428deff7762fa5dae86ceb98d2d29e7ab7c6 Mon Sep 17 00:00:00 2001 From: convee Date: Sat, 5 Sep 2026 18:20:15 +0000 Subject: [PATCH 01/17] fix(exposure): stop assembling a public URL from an undeclared host port mapping A public deployment on any cluster but the reference trial reported `http://127.0.0.1:1809x` in `sites status`, in the CR's `status.url`, in the database, and behind the console's Open action. Measured on kind: nodePort 30080 was reported as 18090, which nothing was listening on. `HOST_PORT_BASE` defaulted to 18090, the host port the repository's own Lima relay forwards 30080..30088 to. That is a fact about the machine in front of the cluster, not about the cluster, and the Chart hardcoded `SITES_PUBLIC_URL_HOST` with no value at all. The wrong answer is invisible: the deployment is healthy and server-side verification passes because it probes the in-cluster address. The mapping is now declared or absent, for the same reason `clusterNetwork.podCIDR` has no default. Undeclared, `public_url` returns None instead of guessing. New Chart values `nodePort.hostPortBase` (no default) and `nodePort.publicUrlHost`; `cluster.sh` gains matching `SITES_HOST_PORT_BASE` and `SITES_PUBLIC_URL_HOST` seams. `host_port_base()` reads the environment per call for the same reason `backend()` does. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01365xzFrkTZV7Fr9WmNe6is --- charts/site/templates/10-control-plane.yaml | 10 ++++- charts/site/values.schema.json | 28 ++++++++++++ charts/site/values.yaml | 14 ++++++ scripts/cluster.sh | 15 +++++++ src/sites/exposure.py | 28 ++++++++++-- tests/test_exposure.py | 47 ++++++++++++++++++++- 6 files changed, 136 insertions(+), 6 deletions(-) diff --git a/charts/site/templates/10-control-plane.yaml b/charts/site/templates/10-control-plane.yaml index 10b739d..d21cf74 100644 --- a/charts/site/templates/10-control-plane.yaml +++ b/charts/site/templates/10-control-plane.yaml @@ -595,7 +595,15 @@ spec: - name: SITES_CONTROL_NAMESPACE value: {{ .Values.namespaces.control | quote }} - name: SITES_PUBLIC_URL_HOST - value: http://127.0.0.1 + value: {{ .Values.nodePort.publicUrlHost | quote }} + # Declared only where this environment really does forward a host + # port to the NodePort pool. Left out, exposure.host_port_base() + # returns None and a public deployment reports no URL rather than one + # built from a mapping this host may not have. See values.yaml. + {{- with .Values.nodePort.hostPortBase }} + - name: SITES_HOST_PORT_BASE + value: {{ . | quote }} + {{- end }} # The Pod network this release's NetworkPolicies exclude, and this # Pod's own address to check it against. The operator refuses to # start if the address is outside the declaration, or if the diff --git a/charts/site/values.schema.json b/charts/site/values.schema.json index b6a06f8..a59621a 100644 --- a/charts/site/values.schema.json +++ b/charts/site/values.schema.json @@ -13,6 +13,7 @@ "mcpEndpoint", "monitoring", "namespaces", + "nodePort", "postgresql", "service", "storage", @@ -362,6 +363,33 @@ "pattern": "^$|^([0-9]{1,3}\\.){3}[0-9]{1,3}/[0-9]{1,2}$" } } + }, + "nodePort": { + "type": "object", + "additionalProperties": false, + "required": [ + "publicUrlHost", + "hostPortBase" + ], + "properties": { + "publicUrlHost": { + "type": "string", + "minLength": 1 + }, + "hostPortBase": { + "oneOf": [ + { + "type": "string", + "pattern": "^$|^[0-9]{1,5}$" + }, + { + "type": "integer", + "minimum": 1, + "maximum": 65535 + } + ] + } + } } }, "$defs": { diff --git a/charts/site/values.yaml b/charts/site/values.yaml index 8ecd435..7cd686b 100644 --- a/charts/site/values.yaml +++ b/charts/site/values.yaml @@ -109,6 +109,20 @@ gateway: httpNodePort: null metricsNodePort: null +# How a browser outside the cluster reaches a site published by the `nodeport` +# exposure backend. Both keys describe the machine in front of the cluster, not +# the cluster, so hostPortBase has no default for the same reason +# clusterNetwork.podCIDR has none: a wrong one is not visibly wrong. The +# reference kubeadm trial owns a relay that forwards guest 30080..30088 to host +# 18090..18098 and therefore declares hostPortBase: 18090. Where no such mapping +# exists, leaving it empty makes a public deployment report no URL, instead of +# one assembled from a host port that something unrelated may be serving. +# Server-side verification is unaffected either way: it uses the in-cluster +# address and remains the evidence that the site serves traffic. +nodePort: + publicUrlHost: http://127.0.0.1 + hostPortBase: "" + monitoring: enabled: false diff --git a/scripts/cluster.sh b/scripts/cluster.sh index 7ba2825..9f3a921 100755 --- a/scripts/cluster.sh +++ b/scripts/cluster.sh @@ -24,6 +24,11 @@ Standard inputs: SITES_CLUSTER_POD_CIDR Actual cluster Pod CIDR; overrides the values file. SITES_LOCAL_PATH_PROVISIONER_ENABLED true/false; set false when the cluster already has one. + SITES_HOST_PORT_BASE Host port this environment forwards the NodePort + pool's first port to. Unset means no such forward + exists and a public deployment reports no URL. + SITES_PUBLIC_URL_HOST Scheme and host those forwards answer on + (default http://127.0.0.1). SITES_CONTROL_IMAGE_REPOSITORY Optional pre-published control image repository. SITES_CONTROL_IMAGE_TAG Optional control image tag. SITES_CONTROL_IMAGE_DIGEST Optional immutable sha256 digest. @@ -73,6 +78,16 @@ if [[ -n "${SITES_LOCAL_PATH_PROVISIONER_ENABLED:-}" ]]; then esac cluster_args+=(--set "localPathProvisioner.enabled=$SITES_LOCAL_PATH_PROVISIONER_ENABLED") fi +if [[ -n "${SITES_HOST_PORT_BASE:-}" ]]; then + [[ "$SITES_HOST_PORT_BASE" =~ ^[0-9]{1,5}$ ]] || { + printf 'SITES_HOST_PORT_BASE must be a port number\n' >&2 + exit 2 + } + cluster_args+=(--set-string "nodePort.hostPortBase=$SITES_HOST_PORT_BASE") +fi +if [[ -n "${SITES_PUBLIC_URL_HOST:-}" ]]; then + cluster_args+=(--set-string "nodePort.publicUrlHost=$SITES_PUBLIC_URL_HOST") +fi if [[ -n "${SITES_CONTROL_IMAGE_TAG:-}" ]]; then image_args+=(--set "images.control.tag=$SITES_CONTROL_IMAGE_TAG") fi diff --git a/src/sites/exposure.py b/src/sites/exposure.py index 7412e01..447b80e 100644 --- a/src/sites/exposure.py +++ b/src/sites/exposure.py @@ -45,13 +45,32 @@ def bounded_public_route_default(configured: int) -> int: """Clamp a default quota to the selected backend's physical capacity.""" capacity = backend().capacity return min(configured, capacity) if capacity is not None else configured -# Public NodePort URLs use host_port = HOST_PORT_BASE + nodePort - NODE_PORT_MIN. +# Public NodePort URLs use host_port = host_port_base() + nodePort - NODE_PORT_MIN. # topology.py and its contract tests pin the same mapping, preventing a deployment # from becoming ready at a host URL that cannot actually be opened. PUBLIC_URL_HOST = ( getenv("SITES_PUBLIC_URL_HOST", "http://127.0.0.1") or "http://127.0.0.1" ) -HOST_PORT_BASE = int(getenv("SITES_HOST_PORT_BASE", "18090") or "18090") + + +def host_port_base() -> int | None: + """Host port this environment maps NODE_PORT_MIN to, or None if none is declared. + + A NodePort Service is reachable at ``:``; the extra hop from + a host port to that node port is a fact about the machine in front of the + cluster, not about the cluster. Only the reference kubeadm trial publishes + one (Lima forwards guest 30080..30088 to host 18090..18098 and the Chart + declares ``nodePort.hostPortBase``), so there is deliberately no default: + assuming 18090 everywhere hands foreign clusters a URL that resolves to + whatever else happens to own that host port, and the caller cannot tell that + from a working one. Undeclared, ``public_url`` reports nothing rather than a + guess -- the same reason ``clusterNetwork.podCIDR`` has no default. + + Read per call rather than frozen at import so the Chart, the trial script and + the tests can each declare their own mapping, exactly like ``backend()``. + """ + declared = (getenv("SITES_HOST_PORT_BASE", "") or "").strip() + return int(declared) if declared else None # --- Gateway backend parameters --- # sslip.io resolves ..sslip.io to the IP, so the local environment can provide each @@ -210,7 +229,10 @@ def public_url(self, spec: dict[str, Any]) -> str | None: return None if port <= 0: return None - return f"{PUBLIC_URL_HOST}:{HOST_PORT_BASE + port - NODE_PORT_MIN}" + base = host_port_base() + if base is None: + return None + return f"{PUBLIC_URL_HOST}:{base + port - NODE_PORT_MIN}" class GatewayExposure(ExposureBackend): diff --git a/tests/test_exposure.py b/tests/test_exposure.py index 0b396a3..ea8ddb4 100644 --- a/tests/test_exposure.py +++ b/tests/test_exposure.py @@ -53,6 +53,33 @@ def using_backend(name: str): os.environ["SITES_EXPOSURE_BACKEND"] = previous +# The one environment that really does forward this host port to NODE_PORT_MIN: +# the repository's own kubeadm trial, via dev/kubeadm/lima.yaml. +REFERENCE_HOST_PORT_BASE = "18090" + + +@contextmanager +def using_host_port_base(base: str | None): + """Temporarily declare, or withdraw, this environment's host port mapping. + + exposure.host_port_base() rereads env per call for the same reason backend() + does: the mapping belongs to whoever set the host up, so a module constant + frozen at import would let the first test decide it for every later one. + """ + previous = os.environ.get("SITES_HOST_PORT_BASE") + if base is None: + os.environ.pop("SITES_HOST_PORT_BASE", None) + else: + os.environ["SITES_HOST_PORT_BASE"] = base + try: + yield + finally: + if previous is None: + os.environ.pop("SITES_HOST_PORT_BASE", None) + else: + os.environ["SITES_HOST_PORT_BASE"] = previous + + def _spec(*, merchant=DEFAULT_MERCHANT_ID, user="local", **overrides): payload = { "name": "demo", @@ -685,12 +712,28 @@ def test_both_control_plane_components_are_covered(self) -> None: class PublicUrlTest(unittest.TestCase): def test_nodeport_url_is_host_plus_mapped_port(self) -> None: - with using_backend("nodeport"): + with using_backend("nodeport"), using_host_port_base( + REFERENCE_HOST_PORT_BASE + ): url = public_url_for_spec(_spec()) self.assertEqual( - url, f"{exposure.PUBLIC_URL_HOST}:{exposure.HOST_PORT_BASE}" + url, f"{exposure.PUBLIC_URL_HOST}:{REFERENCE_HOST_PORT_BASE}" ) + def test_nodeport_url_is_absent_when_no_host_mapping_is_declared(self) -> None: + """No declared mapping must yield no URL, not the reference one. + + The offset formula describes a host that really does forward 18090.. to + 30080.., which the trial does and an arbitrary cluster does not. While + 18090 was the default, every other installation was handed a URL naming + whatever else owned that host port, and nothing could tell it from a + working one: server-side verification passes either way because it probes + the in-cluster address, so the failure only ever surfaced as a user + clicking Open and getting nothing. + """ + with using_backend("nodeport"), using_host_port_base(None): + self.assertIsNone(public_url_for_spec(_spec())) + def test_gateway_url_is_a_hostname_not_a_port_offset(self) -> None: with using_backend("gateway"): url = public_url_for_spec(_spec()) From 49e50f4564f06b1711e2ae2fd90906d4dc1db590 Mon Sep 17 00:00:00 2001 From: convee Date: Sat, 5 Sep 2026 18:20:44 +0000 Subject: [PATCH 02/17] fix(quickstart): detect busy host ports regardless of who owns them `make quickstart-doctor` reported "local ports: available" while 18090 and 18091 were held by a root-owned process. Two-sided control on one address, 127.0.0.1:18092: held by the invoking user the doctor refuses correctly; held by root it passes. Only the owner differs. `lsof` lists sockets owned by the user running it, so a port held by anyone else returns non-zero and reads as free. Lima binds these forwards on 127.0.0.1, so this is a guaranteed failure several minutes later -- exactly what the doctor's own comment says it exists to prevent. Replaced with a bind probe: it answers whether a server can take the address, for any owner, and sets SO_REUSEADDR so a lingering TIME_WAIT socket does not read as a conflict. Both preflights use it, `lsof` drops out of the prerequisites, and the fixed-port list is derived from `host_port_base` instead of repeating 18090 a second time in the same file. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01365xzFrkTZV7Fr9WmNe6is --- docs/DEPLOYMENT.md | 2 +- scripts/quickstart-kubeadm.sh | 58 +++++++++++++++++++++++++++-------- 2 files changed, 46 insertions(+), 14 deletions(-) diff --git a/docs/DEPLOYMENT.md b/docs/DEPLOYMENT.md index aaeeb58..2be41ce 100644 --- a/docs/DEPLOYMENT.md +++ b/docs/DEPLOYMENT.md @@ -65,7 +65,7 @@ workflow, plus a separate `site-values-VERSION.yaml` digest-pinned override and ## Local kubeadm environment Prerequisites for the complete local trial: Lima, a running Docker daemon, kubectl, Helm, -curl, uv, lsof, and Python 3.12+. Run `make quickstart-doctor` for the executable preflight. +curl, uv, and Python 3.12+. Run `make quickstart-doctor` for the executable preflight. The default three VMs allocate 8 CPUs, 10 GiB RAM, and 70 GiB of sparse disk in total; outbound HTTPS and host ports 18090–18098 plus 18447 are required. The repository creates one control-plane and two worker VMs on its own Lima network and bootstraps Kubernetes with diff --git a/scripts/quickstart-kubeadm.sh b/scripts/quickstart-kubeadm.sh index 93fa24d..ff93f03 100755 --- a/scripts/quickstart-kubeadm.sh +++ b/scripts/quickstart-kubeadm.sh @@ -12,6 +12,10 @@ worker_memory=${SITES_QUICKSTART_WORKER_MEMORY_GIB:-3} worker_disk=${SITES_QUICKSTART_WORKER_DISK_GIB:-20} api_port=${SITES_QUICKSTART_KUBE_API_PORT:-18447} console_port=${SITES_QUICKSTART_CONSOLE_PORT:-18091} +# Host port this trial's relay maps NODE_PORT_MIN to. It is the trial harness +# that owns the mapping -- Lima forwards guest 30080..30088 here -- so the trial +# is what declares it to the Chart. Keep aligned with dev/kubeadm/lima.yaml. +host_port_base=${SITES_QUICKSTART_HOST_PORT_BASE:-18090} pod_cidr=${SITES_QUICKSTART_POD_CIDR:-10.201.0.0/16} service_cidr=${SITES_QUICKSTART_SERVICE_CIDR:-10.202.0.0/16} cilium_version=${SITES_QUICKSTART_CILIUM_VERSION:-1.19.6} @@ -71,7 +75,7 @@ access Forward and open the console at http://127.0.0.1:18091/console/. token Print the disposable trial's local admin token for console login. clean Delete only the site-quickstart VMs, network, and this checkout's local state. -Prerequisites: Lima, a running Docker daemon, kubectl, Helm, curl, uv, lsof, +Prerequisites: Lima, a running Docker daemon, kubectl, Helm, curl, uv, and Python 3.12+. The default three-node topology allocates 8 CPUs, 10 GiB RAM, and 70 GiB of sparse disk across its VMs. No other repository, pre-created Lima network, or published Site image is used. @@ -85,9 +89,37 @@ require_command() { } } +# Of the ports given, echo those the host cannot hand to Lima. +# +# A bind probe, not a listener listing. `lsof` only reports sockets owned by the +# user running it, so a port held by root or by another account reads as free and +# the trial then fails minutes later at the exact forward this check exists to +# catch. Binding answers the question actually being asked -- can a server take +# this address -- for any owner. Lima publishes these forwards on 127.0.0.1, so +# that is the address probed, and SO_REUSEADDR matches what a server would set, +# keeping a lingering TIME_WAIT socket from reading as a conflict. +ports_in_use() { + python3 - "$@" <<'PY' +import socket +import sys + +busy = [] +for port in sys.argv[1:]: + probe = socket.socket(socket.AF_INET, socket.SOCK_STREAM) + probe.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) + try: + probe.bind(("127.0.0.1", int(port))) + except OSError: + busy.append(port) + finally: + probe.close() +print(" ".join(busy)) +PY +} + check_prerequisites() { local command_name missing=() - for command_name in limactl docker kubectl helm curl uv python3 lsof; do + for command_name in limactl docker kubectl helm curl uv python3; do command -v "$command_name" >/dev/null 2>&1 || missing+=("$command_name") done if ((${#missing[@]})); then @@ -111,17 +143,16 @@ PY exit 2 fi - # These are the complete fixed-port contract of the reference VM. Checking - # all of them now avoids a Lima forwarding failure several minutes later. + # The complete fixed-port contract of the reference VM: the Kubernetes API + # forward plus the nine contiguous host ports starting at host_port_base -- the + # NodePort pool, with the console occupying the one Lima does not forward. + # Checking all of them now avoids a Lima forwarding failure several minutes + # later. if ! vm_running; then - local port busy=() - for port in "$api_port" "$console_port" 18090 18092 18093 18094 18095 18096 18097 18098; do - if lsof -nP -iTCP:"$port" -sTCP:LISTEN >/dev/null 2>&1; then - busy+=("$port") - fi - done - if ((${#busy[@]})); then - printf 'quickstart host ports are already in use: %s\n' "${busy[*]}" >&2 + local busy + busy=$(ports_in_use "$api_port" $(seq "$host_port_base" $((host_port_base + 8)))) || exit 2 + if [[ -n "$busy" ]]; then + printf 'quickstart host ports are already in use: %s\n' "$busy" >&2 echo 'Stop the process or older Site trial using those ports, then rerun make quickstart.' >&2 exit 2 fi @@ -408,7 +439,7 @@ create_cluster() { fi if ! vm_exists; then - if lsof -nP -iTCP:"$api_port" -sTCP:LISTEN >/dev/null 2>&1; then + if [[ -n "$(ports_in_use "$api_port")" ]]; then printf 'host port %s is already in use; set SITES_QUICKSTART_KUBE_API_PORT and update the Lima template forward\n' "$api_port" >&2 exit 2 fi @@ -608,6 +639,7 @@ prove_site() { --set images.control.repository=site-control \ --set images.control.tag=quickstart \ --set "localPathProvisioner.allowedNodeNames[0]=${vm}-w1" \ + --set "nodePort.hostPortBase=$host_port_base" \ --set-value monitoring.enabled=true # A truly new node may need several minutes to pull the pinned database, # registry, proxy, and Prometheus images. Do not spend the API rollout budget From a29ac82bb563b673dcbc7a787cbaa5fba0b7a483 Mon Sep 17 00:00:00 2001 From: convee Date: Sat, 5 Sep 2026 18:20:44 +0000 Subject: [PATCH 03/17] fix(bundles): make the shipped examples deployable and surface their evidence README points at `deploy-specs/` for bundle examples and neither file could be submitted. `example-single.json` was a flat single-deployment shape the bundle endpoint refuses outright (`unsupported bundle fields`); `example-stack.json` carried a `command` field that is not a deployment field. Nothing had ever submitted them, so both drifted out of the contract they illustrate. Submitting them then showed a second gap: bundle components reported `verification: null` while the operator had written the evidence onto every CR. docs/AGENT_CONTRACT.md makes `status.verification` the only success criterion and bundles are one of its deployment entry points, so the projection was hiding the one thing a caller is told to check. Both examples are now real bundles that reach Running with 200s on a live cluster, their component names no longer collide so either can be submitted first, and the bundle projection carries `verification`. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01365xzFrkTZV7Fr9WmNe6is --- deploy-specs/example-single.json | 15 ++++++++++----- deploy-specs/example-stack.json | 15 +++++++-------- src/sites/serializers.py | 7 +++++++ 3 files changed, 24 insertions(+), 13 deletions(-) diff --git a/deploy-specs/example-single.json b/deploy-specs/example-single.json index 0483318..f0b0405 100644 --- a/deploy-specs/example-single.json +++ b/deploy-specs/example-single.json @@ -1,7 +1,12 @@ { - "name": "example-web", - "image": "nginx:1.29-alpine", - "port": 80, - "healthPath": "/", - "exposure": "public" + "name": "example-single", + "components": [ + { + "name": "single-web", + "image": "nginxinc/nginx-unprivileged:1.29-alpine", + "port": 8080, + "healthPath": "/", + "exposure": "public" + } + ] } diff --git a/deploy-specs/example-stack.json b/deploy-specs/example-stack.json index 861d527..3e8d3a6 100644 --- a/deploy-specs/example-stack.json +++ b/deploy-specs/example-stack.json @@ -2,19 +2,18 @@ "name": "example-stack", "components": [ { - "name": "web", - "image": "nginx:1.29-alpine", - "port": 80, + "name": "stack-web", + "image": "nginxinc/nginx-unprivileged:1.29-alpine", + "port": 8080, "healthPath": "/", "exposure": "public" }, { - "name": "worker", - "image": "busybox:1.37", + "name": "stack-worker", + "image": "nginxinc/nginx-unprivileged:1.29-alpine", "port": 8080, - "healthPath": "/healthz", - "exposure": "internal", - "command": ["sh", "-c", "httpd -f -p 8080"] + "healthPath": "/", + "exposure": "internal" } ] } diff --git a/src/sites/serializers.py b/src/sites/serializers.py index 0135144..42570b0 100644 --- a/src/sites/serializers.py +++ b/src/sites/serializers.py @@ -318,6 +318,13 @@ def bundle_response( and observed == generation ), "url": status.get("url"), + # docs/AGENT_CONTRACT.md makes status.verification the only + # success criterion, and bundles are one of its deployment entry + # points. The operator writes this per component exactly as it + # does for a standalone deployment; leaving it out of the + # projection meant a bundle caller saw phase Running and nothing + # else, with no way to reach evidence that already existed. + "verification": status.get("verification"), } ) # The expected number of components is the currently found group: the control plane does not preset any bundle composition. From 4c47e19591f42aeee18a3dec60cbeb3096fcce5e Mon Sep 17 00:00:00 2001 From: convee Date: Sat, 5 Sep 2026 18:20:44 +0000 Subject: [PATCH 04/17] test(contracts): gate every claim this round fixed, and drop a stale README limitation A gate per fix, each verified by mutation (restore the old behaviour, watch it go red): - no declared host mapping means no public URL; the default render must not set SITES_HOST_PORT_BASE; the trial's declared base must equal its Lima forward - the port preflight must bind rather than list sockets it owns - every file in deploy-specs/ passes the endpoint's own validator, and no two examples claim the same component name - bundle components carry verification Also drops "Historical UIDs remain in the Git history": the repository is public, its history is 22 commits, and scanning all 324 blobs finds no UID or Aliyun identifier -- the only `aliyuncs.com` occurrences are the OSS endpoint check and its tests. The console's `The only one in the merchant` placeholder, a machine translation, becomes `Unique within the merchant`. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01365xzFrkTZV7Fr9WmNe6is --- README.md | 17 ++-- console/src/components/TenantsView.tsx | 2 +- console/src/i18n.tsx | 4 +- docs/CONFIGURATION.md | 3 +- tests/test_helm_package.py | 60 +++++++++++- tests/test_sites.py | 126 +++++++++++++++++++++++-- 6 files changed, 195 insertions(+), 17 deletions(-) diff --git a/README.md b/README.md index a9c3a98..6a2f086 100644 --- a/README.md +++ b/README.md @@ -26,12 +26,12 @@ environments, not a public hosting platform — [Known limitations](#known-limit The supported source-checkout trial runs on macOS or Linux with hardware virtualization, outbound HTTPS, and enough capacity for three Lima VMs configured with **8 CPUs, 10 GiB RAM, and 70 GiB of sparse disk in total**. Install Git, Lima, Docker, `kubectl`, Helm, `curl`, `uv`, -`lsof`, and Python 3.12+. Docker must be running, not merely installed. +and Python 3.12+. Docker must be running, not merely installed. On macOS with Homebrew, the command-line dependencies are: ```bash -brew install git lima kubectl helm uv python lsof +brew install git lima kubectl helm uv python brew install --cask docker open -a Docker # wait until Docker reports that it is running ``` @@ -175,7 +175,7 @@ git clone https://github.com/hullwork/site.git cd site uv sync --locked --extra dev make test-db # starts a throwaway PostgreSQL on 127.0.0.1:55439 -make test # 1031 tests +make test # 1038 tests make test-db-down ``` @@ -208,6 +208,14 @@ SITES_LOCAL_PATH_PROVISIONER_ENABLED=false \ scripts/cluster.sh up ``` +A public deployment on such a cluster reports **no URL** until you say how the outside +reaches it. The `nodeport` backend puts each site on one node port from 30080–30088; only +the kubeadm trial above also forwards those to host ports 18090–18098, so nothing else can +be assumed. Add `SITES_HOST_PORT_BASE` (and `SITES_PUBLIC_URL_HOST` when the forwards do +not answer on `http://127.0.0.1`) once such a forward exists. Server-side verification is +independent of this: it probes the in-cluster address, so `status.verification` is evidence +the site serves traffic whether or not a public URL is declared. + The bootstrap helper generates credentials into a mode-0700 temporary directory and never writes them into the repository or a values file. **Do not use it for production.** For the lower-level Helm lifecycle and production Secret contract, see @@ -428,9 +436,6 @@ production is worse off than one who reads them here. **Housekeeping** -- **Historical UIDs remain in the Git history.** The working tree is clean, but publishing - with history attached does not clear them; a `git filter-repo` pass is required before the - repository is made public. - 45 `SITES_*` environment variables are read by `src/sites/` but appear in no document or chart — mostly activator, gateway, NodePort-pool, KEDA, and operator tuning. They have working defaults, and the most useful ones are now listed under diff --git a/console/src/components/TenantsView.tsx b/console/src/components/TenantsView.tsx index f205952..a827494 100644 --- a/console/src/components/TenantsView.tsx +++ b/console/src/components/TenantsView.tsx @@ -246,7 +246,7 @@ export default function TenantsView({ {t("Tenant ID")} setDraftUser(event.target.value)} /> diff --git a/console/src/i18n.tsx b/console/src/i18n.tsx index 2ef8207..361c1e2 100644 --- a/console/src/i18n.tsx +++ b/console/src/i18n.tsx @@ -128,7 +128,7 @@ const en = { "New tenant": "New tenant", "Search tenants": "Search tenants", "Search for merchant or tenant ID": "Search for merchant or tenant ID", - "The only one in the merchant": "The only one in the merchant", + "Unique within the merchant": "Unique within the merchant", "Create and issue token": "Create and issue token", "No matching tenant": "No matching tenant", "There are no tenants on the platform yet": "There are no tenants on the platform yet", @@ -554,7 +554,7 @@ const zhCN: Record = { "New tenant": "新建租户", "Search tenants": "搜索租户", "Search for merchant or tenant ID": "搜索商户或租户 ID", - "The only one in the merchant": "商户内唯一", + "Unique within the merchant": "商户内唯一", "Create and issue token": "创建并签发 token", "No matching tenant": "没有匹配的租户", "There are no tenants on the platform yet": "平台上还没有租户", diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index 8cdca3e..1c21c77 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -169,7 +169,8 @@ and an invalid value generally raises at import time rather than falling back. | `SITES_GATEWAY_NODE_PORT` | `30080` | NodePort the gateway listener is published on | | `SITES_NODE_PORT_MIN` / `SITES_NODE_PORT_MAX` | `30080` / `30088` | Allocation pool for the `nodeport` exposure backend | | `SITES_NODE_PORT_EXCLUDED` | `30081` | Ports removed from that pool | -| `SITES_HOST_PORT_BASE` | `18090` | Base host port used by the local reference topology | +| `SITES_HOST_PORT_BASE` | unset | Host port this environment forwards `SITES_NODE_PORT_MIN` to. Unset means no such forward exists and a `nodeport` deployment reports no public URL rather than guessing one; the Chart sets it from `nodePort.hostPortBase`, which only the kubeadm trial declares | +| `SITES_PUBLIC_URL_HOST` | `http://127.0.0.1` | Scheme and host those forwards answer on; the Chart sets it from `nodePort.publicUrlHost` | | `SITES_SOURCE_PVC` | `sites-sources` | PVC that holds source packages when `SITES_SOURCE_BACKEND=pvc` | | `SITES_SOURCE_ROOT` | `/var/lib/sites/sources` | Mount path for that PVC | | `SITES_OSS_AUTH_SECRET` | `sites-oss-auth` | Secret holding object-storage credentials | diff --git a/tests/test_helm_package.py b/tests/test_helm_package.py index 19f71e7..79176c8 100644 --- a/tests/test_helm_package.py +++ b/tests/test_helm_package.py @@ -41,6 +41,27 @@ def test_quickstart_is_a_repository_owned_kubeadm_workflow(self) -> None: self.assertNotIn('node-role.kubernetes.io/control-plane-', script) self.assertIn("dev/kubeadm/lima.yaml", script) + def test_the_port_preflight_binds_rather_than_listing_owned_sockets(self) -> None: + """The preflight must detect a busy port whoever owns it. + + `lsof` reports only sockets owned by the user running it, so a quickstart + port held by root or another account read as free and `make quickstart` + then failed several minutes later on the Lima forward this check exists + to catch -- a false pass in exactly its own scenario. Binding the address + Lima binds answers the question for any owner. + """ + script = (ROOT / "scripts" / "quickstart-kubeadm.sh").read_text(encoding="utf-8") + self.assertNotIn("lsof", script.replace("`lsof`", "")) + self.assertIn("ports_in_use()", script) + self.assertIn('probe.bind(("127.0.0.1", int(port)))', script) + self.assertIn("socket.SO_REUSEADDR", script) + # Both preflights go through it: the fixed-port contract and the + # API-server forward checked again just before the VM is created. + self.assertIn( + 'busy=$(ports_in_use "$api_port" $(seq "$host_port_base"', script + ) + self.assertIn('if [[ -n "$(ports_in_use "$api_port")" ]]; then', script) + def test_quickstart_lima_template_needs_no_precreated_network(self) -> None: template = yaml.safe_load( (ROOT / "dev" / "kubeadm" / "lima.yaml").read_text(encoding="utf-8") @@ -389,7 +410,44 @@ def test_standalone_scripts_are_syntax_checked_and_never_embed_secret_values(sel self.assertIn("SITES_CONTROL_IMAGE_DIGEST", adapter) self.assertNotIn("INFRA_", adapter) self.assertNotIn("limactl", adapter) - + self.assertIn("SITES_HOST_PORT_BASE", adapter) + self.assertIn("nodePort.hostPortBase", adapter) + + def test_default_install_declares_no_host_port_mapping(self) -> None: + """The Chart must not assume a host forwards anything to the NodePort pool. + + Only the environment around a cluster can know that, and a wrong guess is + not visibly wrong: the deployment is healthy and control-plane + verification passes, because verification probes the in-cluster address. + A default here therefore reaches the user as a public URL naming whatever + else happens to own that host port. Absent the variable, + exposure.host_port_base() returns None and no URL is claimed at all. + """ + rendered = subprocess.run( + ["helm", "template", "site", str(CHART), *POD_CIDR], + check=True, + capture_output=True, + text=True, + ).stdout + self.assertNotIn("SITES_HOST_PORT_BASE", rendered) + self.assertIn("SITES_PUBLIC_URL_HOST", rendered) + + declared = subprocess.run( + [ + "helm", + "template", + "site", + str(CHART), + *POD_CIDR, + "--set-string", + "nodePort.hostPortBase=18090", + ], + check=True, + capture_output=True, + text=True, + ).stdout + self.assertIn("SITES_HOST_PORT_BASE", declared) + self.assertIn('value: "18090"', declared) if __name__ == "__main__": diff --git a/tests/test_sites.py b/tests/test_sites.py index 7db923f..17d8761 100644 --- a/tests/test_sites.py +++ b/tests/test_sites.py @@ -22,6 +22,7 @@ import yaml from tests import chart +from tests.test_exposure import REFERENCE_HOST_PORT_BASE, using_host_port_base from tests.test_support import postgres_connection, postgres_store from sites import exposure as _exposure @@ -69,6 +70,7 @@ DEFAULT_MAX_DEPLOYMENTS, DEFAULT_MAX_PUBLIC_ROUTES, DEFAULT_MERCHANT_ID, + dns_label, INLINE_ARTIFACT_MAX_FILES, INLINE_ARTIFACT_MAX_TOTAL_BYTES, MAX_ENV_VARS, @@ -1230,6 +1232,91 @@ def test_bundle_response_reports_the_submitted_name(self) -> None: _bundle_response("demo-stack", objects)["phase"], "Deploying" ) + def test_shipped_bundle_examples_are_accepted_by_the_real_validator(self) -> None: + """Every file README points at must survive the endpoint's own checks. + + Nothing submitted these, so both drifted out of the contract they + illustrate and stayed in the tree: one carried a flat single-deployment + shape the bundle endpoint refuses outright, the other a `command` field + that is not a deployment field. A reader following README's "examples in + deploy-specs/" got a 400 from the first command they ran. + + This goes through the same functions /v1/bundles does -- the top-level + field check and bundle_resources -- rather than a copy of their rules, + so an example cannot pass here and fail there. + """ + specs = sorted( + (Path(__file__).resolve().parent.parent / "deploy-specs").glob("*.json") + ) + self.assertTrue(specs, "deploy-specs/ has no examples to check") + for spec_path in specs: + with self.subTest(example=spec_path.name): + payload = json.loads(spec_path.read_text(encoding="utf-8")) + self.assertEqual( + set(payload) - {"name", "components"}, + set(), + "the bundle endpoint refuses unknown top-level fields", + ) + objects = bundle_resources( + dns_label(str(payload["name"])), + payload.get("components"), + DEFAULT_MERCHANT_ID, + "local", + ) + self.assertEqual(len(objects), len(payload["components"])) + + def test_shipped_bundle_examples_do_not_claim_each_other_names(self) -> None: + # Component names are unique per tenant, so two examples sharing one + # would make whichever is submitted second fail with a 409 that reads + # like a bug in the reader's own cluster. + seen: dict[str, str] = {} + for spec_path in sorted( + (Path(__file__).resolve().parent.parent / "deploy-specs").glob("*.json") + ): + payload = json.loads(spec_path.read_text(encoding="utf-8")) + for component in payload["components"]: + name = component["name"] + self.assertNotIn( + name, + seen, + f"{spec_path.name} reuses component {name!r} from {seen.get(name)}", + ) + seen[name] = spec_path.name + + def test_bundle_components_carry_the_verification_evidence(self) -> None: + """Each component must surface the evidence the operator wrote for it. + + docs/AGENT_CONTRACT.md makes status.verification the only success + criterion, and a bundle is one of its deployment entry points. The + operator collects it per component, so dropping it from this projection + left a bundle caller with phase Running and nothing to check -- the + evidence existed on the resource the whole time and was simply + unreachable through the API. + """ + evidence = { + "ok": True, + "httpStatus": 200, + "bodySha256": "a" * 64, + "revision": "7", + } + objects = bundle_resources( + "demo-stack", + self._sample_components(), + DEFAULT_MERCHANT_ID, + "local", + ) + for resource in objects: + resource["metadata"]["generation"] = 2 + resource["status"] = { + "phase": "Running", + "observedGeneration": 2, + "verification": evidence, + } + response = _bundle_response("demo-stack", objects) + self.assertTrue(response["components"]) + for component in response["components"]: + self.assertEqual(component["verification"], evidence) + def _crd_spec_properties() -> set[str]: """Read the SiteDeployment spec fields the CRD actually declares. @@ -1588,13 +1675,40 @@ def test_pool_ports_have_unique_host_mappings(self) -> None: def test_the_mapping_matches_the_url_formula(self) -> None: # The only source of truth for URL formulas is in sites/exposure.py - just take the public_url from the backend # Reconciling port_forwards, what is nailed is the real invariant of "exposure constant ↔ host mapping". + # The base is declared here rather than defaulted: this pins the trial's + # own topology, and every other environment gets no URL at all until it + # declares the forward it actually publishes. mappings = self._mappings() - for port in NODE_PORT_RANGE: - self.assertEqual( - _exposure.NodePortExposure().public_url({"nodePort": port}), - f"http://127.0.0.1:{mappings[port]}", - f"nodePort {port} The URL does not match the portForward mapping", - ) + with using_host_port_base(REFERENCE_HOST_PORT_BASE): + for port in NODE_PORT_RANGE: + self.assertEqual( + _exposure.NodePortExposure().public_url({"nodePort": port}), + f"http://127.0.0.1:{mappings[port]}", + f"nodePort {port} The URL does not match the portForward mapping", + ) + + def test_the_reference_base_is_what_the_trial_declares_to_the_chart(self) -> None: + """The trial script must hand the Chart the base its Lima forwards use. + + Two spellings of one fact: dev/kubeadm/lima.yaml opens the host ports and + scripts/quickstart-kubeadm.sh tells the control plane which base to build + URLs from. Drift between them is invisible -- the deployment is healthy, + verification passes on the in-cluster address -- until somebody opens the + URL. + """ + script = ( + Path(__file__).resolve().parent.parent + / "scripts" + / "quickstart-kubeadm.sh" + ).read_text(encoding="utf-8") + self.assertIn( + f"host_port_base=${{SITES_QUICKSTART_HOST_PORT_BASE:-{REFERENCE_HOST_PORT_BASE}}}", + script, + ) + self.assertIn('--set "nodePort.hostPortBase=$host_port_base"', script) + self.assertEqual( + int(REFERENCE_HOST_PORT_BASE), self._mappings()[min(NODE_PORT_RANGE)] + ) def test_there_is_exactly_one_source_for_the_pod_cidr(self) -> None: # 🔴 Successor to test_cluster_cidrs_match_the_profile, which asserted From bf5ba513d3d9cfb59b2c0599063b3a67b9bb0595 Mon Sep 17 00:00:00 2001 From: convee Date: Sat, 5 Sep 2026 18:46:37 +0000 Subject: [PATCH 05/17] fix(builds): create the docker config the build Job mounts, and follow the install namespace `sites build submit` always failed, and the only signal after five minutes was "Job was active longer than specified deadline". The build Pod never started: `MountVolume.SetUp failed for volume "registry-auth": references non-existent secret key: config.json`. builds.py selects `config.json` out of `sites-registry-auth` -- DOCKER_CONFIG points at that mount and Docker looks for that filename -- and nothing in the repository ever created the key. Selecting a missing key is not a soft failure: kubelet refuses to set the volume up, so the entire source-build entry point was dead on every installation that followed this repository's own instructions, and the error named neither the Secret nor the key. Same area, second defect: SITES_REGISTRY_PUSH_HOST and SITES_REGISTRY_API defaulted to a hardcoded `sites-local` namespace and the Chart never set them, so any other namespace pushed builds at, and resolved digests against, a Service that is not there. bootstrap-standalone-secrets.sh now generates the docker config from the same registry password (host defaults to `sites-registry..svc:5000`, overridable), requires the key when reusing an existing Secret so an older installation is told which key is missing, and the Chart renders both registry addresses from `namespaces.control` for the API and the operator. Verified on a fresh install: build, push, deploy, verified 200 in about 30 seconds. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01365xzFrkTZV7Fr9WmNe6is --- charts/site/templates/07-build-plane.yaml | 3 +- charts/site/templates/10-control-plane.yaml | 18 ++++++++ scripts/bootstrap-standalone-secrets.sh | 25 ++++++++++- tests/test_builds.py | 48 +++++++++++++++++++++ tests/test_helm_package.py | 32 ++++++++++++++ 5 files changed, 123 insertions(+), 3 deletions(-) diff --git a/charts/site/templates/07-build-plane.yaml b/charts/site/templates/07-build-plane.yaml index 72de62f..7ef6bc2 100644 --- a/charts/site/templates/07-build-plane.yaml +++ b/charts/site/templates/07-build-plane.yaml @@ -401,7 +401,8 @@ spec: policyTypes: - Egress egress: - # The target pushed by buildctl is sites-registry.sites-local.svc:5000, and the service name must be resolved first. + # The target pushed by buildctl is the sites-registry Service in + # namespaces.control on port 5000, and the service name must be resolved first. - to: - namespaceSelector: matchLabels: diff --git a/charts/site/templates/10-control-plane.yaml b/charts/site/templates/10-control-plane.yaml index d21cf74..608ebc5 100644 --- a/charts/site/templates/10-control-plane.yaml +++ b/charts/site/templates/10-control-plane.yaml @@ -336,6 +336,15 @@ spec: # It will also be printed out by any env in the same Pod. - name: SITES_REGISTRY_USERNAME value: sites + # The registry Service is created in namespaces.control, so its + # address is not a constant. Leaving these to the code defaults + # pinned the namespace at sites-local: any other install pushed + # builds at, and resolved digests against, a Service that is not + # there, and nothing said so. + - name: SITES_REGISTRY_PUSH_HOST + value: {{ printf "sites-registry.%s.svc:5000" .Values.namespaces.control | quote }} + - name: SITES_REGISTRY_API + value: {{ printf "http://sites-registry.%s.svc:5000" .Values.namespaces.control | quote }} - name: SITES_CONTROL_NAMESPACE value: {{ .Values.namespaces.control | quote }} - name: SITES_HTTP_PORT @@ -592,6 +601,15 @@ spec: # It will also be printed out by any env in the same Pod. - name: SITES_REGISTRY_USERNAME value: sites + # The registry Service is created in namespaces.control, so its + # address is not a constant. Leaving these to the code defaults + # pinned the namespace at sites-local: any other install pushed + # builds at, and resolved digests against, a Service that is not + # there, and nothing said so. + - name: SITES_REGISTRY_PUSH_HOST + value: {{ printf "sites-registry.%s.svc:5000" .Values.namespaces.control | quote }} + - name: SITES_REGISTRY_API + value: {{ printf "http://sites-registry.%s.svc:5000" .Values.namespaces.control | quote }} - name: SITES_CONTROL_NAMESPACE value: {{ .Values.namespaces.control | quote }} - name: SITES_PUBLIC_URL_HOST diff --git a/scripts/bootstrap-standalone-secrets.sh b/scripts/bootstrap-standalone-secrets.sh index a3ab603..cec63d8 100755 --- a/scripts/bootstrap-standalone-secrets.sh +++ b/scripts/bootstrap-standalone-secrets.sh @@ -5,6 +5,11 @@ namespace=sites-local api_secret=sites-api-token database_secret=sites-postgres-auth registry_secret=sites-registry-auth +# In-cluster address the BuildKit Job pushes to, and therefore the key the +# generated docker config authenticates. Must equal SITES_REGISTRY_PUSH_HOST as +# the Chart renders it, or the push is a 401 nothing else explains. Resolved +# after argument parsing so --namespace decides it. +registry_host=${SITES_REGISTRY_PUSH_HOST:-} context=${SITES_KUBE_CONTEXT:-} kube() { @@ -27,6 +32,9 @@ Options: --api-secret NAME API Secret name (default: sites-api-token) --database-secret NAME PostgreSQL Secret name (default: sites-postgres-auth) --registry-secret NAME Registry Secret name (default: sites-registry-auth) + --registry-host HOST Registry address the build plane pushes to + (default: sites-registry..svc:5000; + also read from SITES_REGISTRY_PUSH_HOST) --help Show this help EOF } @@ -37,6 +45,7 @@ while [[ $# -gt 0 ]]; do --api-secret) api_secret=${2:?missing API Secret name}; shift 2 ;; --database-secret) database_secret=${2:?missing database Secret name}; shift 2 ;; --registry-secret) registry_secret=${2:?missing registry Secret name}; shift 2 ;; + --registry-host) registry_host=${2:?missing registry host}; shift 2 ;; --help|-h) usage; exit 0 ;; *) echo "unknown argument: $1" >&2; usage >&2; exit 2 ;; esac @@ -57,6 +66,8 @@ if [[ ${#namespace} -gt 63 ]]; then exit 2 fi +registry_host=${registry_host:-sites-registry.$namespace.svc:5000} + secret_dir=$(mktemp -d "${TMPDIR:-/tmp}/site-secrets.XXXXXX") chmod 0700 "$secret_dir" trap 'find "$secret_dir" -type f -exec chmod 0600 {} + 2>/dev/null || true; rm -rf -- "$secret_dir"' EXIT @@ -70,6 +81,15 @@ openssl rand -base64 36 | tr -d '\n' >"$secret_dir/database-password" openssl rand -base64 36 | tr -d '\n' >"$secret_dir/registry-password" registry_sha=$(openssl dgst -binary -sha1 <"$secret_dir/registry-password" | openssl base64 -A) printf 'sites:{SHA}%s\n' "$registry_sha" >"$secret_dir/htpasswd" +# The third form of the same credential, for the BuildKit Job. It sets +# DOCKER_CONFIG to the directory this Secret is mounted at, and Docker looks for +# `config.json` there -- the name is Docker's, not a preference. Without this key +# the Job's volume cannot be set up at all, so the Pod never starts and the only +# thing the caller ever sees is the build deadline expiring several minutes +# later, naming nothing. +registry_auth=$(printf 'sites:%s' "$(cat "$secret_dir/registry-password")" | openssl base64 -A) +printf '{"auths":{"%s":{"auth":"%s"}}}\n' "$registry_host" "$registry_auth" \ + >"$secret_dir/registry-config.json" kube create namespace "$namespace" --dry-run=client -o yaml | kube apply -f - >/dev/null @@ -102,11 +122,12 @@ else --from-file=password="$secret_dir/database-password" >/dev/null fi if kube -n "$namespace" get secret "$registry_secret" >/dev/null 2>&1; then - require_existing_keys "$registry_secret" password htpasswd + require_existing_keys "$registry_secret" password htpasswd config.json else kube -n "$namespace" create secret generic "$registry_secret" \ --from-file=password="$secret_dir/registry-password" \ - --from-file=htpasswd="$secret_dir/htpasswd" >/dev/null + --from-file=htpasswd="$secret_dir/htpasswd" \ + --from-file=config.json="$secret_dir/registry-config.json" >/dev/null fi echo "site development Secrets are present in namespace $namespace (existing values preserved)" diff --git a/tests/test_builds.py b/tests/test_builds.py index 19ca5ba..7cc358e 100644 --- a/tests/test_builds.py +++ b/tests/test_builds.py @@ -214,6 +214,54 @@ def test_job_has_no_host_or_kubernetes_credentials(self) -> None: self.assertEqual(job["spec"]["backoffLimit"], 0) self.assertEqual(job["spec"]["activeDeadlineSeconds"], 300) + def test_every_secret_key_the_job_mounts_is_one_the_bootstrap_creates(self) -> None: + """Nothing may mount a Secret key no supported path ever writes. + + The Job selects `config.json` out of sites-registry-auth, and the + bootstrap helper created only `password` and `htpasswd`. A missing + selected key is not a soft failure: kubelet refuses to set the volume + up, so the Pod never starts and the Job simply runs out its deadline. + The caller's only signal was "Job was active longer than specified + deadline" five minutes later, which names neither the Secret nor the + key, and the whole source-build entry point was dead on every + installation that followed this repository's own instructions. + """ + root = Path(__file__).resolve().parent.parent + bootstrap = (root / "scripts" / "bootstrap-standalone-secrets.sh").read_text( + encoding="utf-8" + ) + pod = build_job_resource(_build(), namespace="sites-local")["spec"][ + "template" + ]["spec"] + selected: list[tuple[str, str]] = [] + for volume in pod["volumes"]: + secret = volume.get("secret") or {} + for item in secret.get("items") or []: + selected.append((secret["secretName"], item["key"])) + self.assertIn(("sites-registry-auth", "config.json"), selected) + required = next( + line + for line in bootstrap.splitlines() + if 'require_existing_keys "$registry_secret"' in line + ) + for secret_name, key in selected: + with self.subTest(secret=secret_name, key=key): + self.assertIn( + f"--from-file={key}=", + bootstrap, + f"{secret_name} key {key!r} is mounted but never created", + ) + # An installation from before this key existed keeps its Secret, + # so a fresh-install-only fix would leave it in the same silent + # failure. Listing the key here makes the rerun say which key is + # missing instead of leaving it to the build deadline. + self.assertIn( + key, + required, + f"an existing {secret_name} missing {key!r} must be refused, " + "not left to fail as a build deadline", + ) + def test_job_reports_its_own_digest_onto_the_build_volume(self) -> None: build = _build() container = build_job_resource(build, namespace="sites-local")[ diff --git a/tests/test_helm_package.py b/tests/test_helm_package.py index 79176c8..fc87785 100644 --- a/tests/test_helm_package.py +++ b/tests/test_helm_package.py @@ -413,6 +413,38 @@ def test_standalone_scripts_are_syntax_checked_and_never_embed_secret_values(sel self.assertIn("SITES_HOST_PORT_BASE", adapter) self.assertIn("nodePort.hostPortBase", adapter) + def test_registry_address_follows_the_namespace_it_is_installed_into(self) -> None: + """The registry Service address must come from namespaces.control. + + The Service is created in that namespace, but the code defaults pinned + it at `sites-registry.sites-local.svc:5000`. Installed anywhere else, + builds pushed to a Service that is not there and the control plane + resolved digests against the same missing address -- with the Chart + rendering cleanly and nothing at any layer naming the namespace. + """ + rendered = subprocess.run( + [ + "helm", + "template", + "site", + str(CHART), + *POD_CIDR, + "--set-string", + "namespaces.control=elsewhere", + ], + check=True, + capture_output=True, + text=True, + ).stdout + self.assertIn('value: "sites-registry.elsewhere.svc:5000"', rendered) + self.assertIn('value: "http://sites-registry.elsewhere.svc:5000"', rendered) + self.assertNotIn('value: "sites-registry.sites-local.svc:5000"', rendered) + # Both processes reach the registry: the API resolves and deletes + # images, the operator runs the build. One of the two carrying it is + # the same defect with a smaller blast radius. + self.assertEqual(rendered.count("SITES_REGISTRY_PUSH_HOST"), 2) + self.assertEqual(rendered.count("SITES_REGISTRY_API"), 2) + def test_default_install_declares_no_host_port_mapping(self) -> None: """The Chart must not assume a host forwards anything to the NodePort pool. From 8a0adab4a7fdac4b165aa704513e459cb8403178 Mon Sep 17 00:00:00 2001 From: convee Date: Sat, 5 Sep 2026 18:46:37 +0000 Subject: [PATCH 06/17] fix(cli): state the flag defaults a caller cannot otherwise see `--health-path` defaults to `/` for `deploy` and `deploy-static` and `/healthz` for `build submit`, and none of the three said so. Taking the sibling default on faith produced a build that pushed a correct image and then spent the whole readiness window failing a probe on a path the image does not serve, reported as "Deployment does not have minimum availability" -- which points at the workload rather than at the flag. `--port` and `--health-path` now print their real defaults, and the source-build one says why it differs. The defaults themselves are unchanged. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01365xzFrkTZV7Fr9WmNe6is --- README.md | 2 +- src/sites/cli.py | 36 ++++++++++++++++++++++++++++++------ tests/test_interface.py | 39 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 70 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index 6a2f086..8c6652f 100644 --- a/README.md +++ b/README.md @@ -175,7 +175,7 @@ git clone https://github.com/hullwork/site.git cd site uv sync --locked --extra dev make test-db # starts a throwaway PostgreSQL on 127.0.0.1:55439 -make test # 1038 tests +make test # 1041 tests make test-db-down ``` diff --git a/src/sites/cli.py b/src/sites/cli.py index cf5499e..8bbf16f 100644 --- a/src/sites/cli.py +++ b/src/sites/cli.py @@ -349,8 +349,14 @@ def _run(args: argparse.Namespace) -> dict[str, Any]: def _add_deploy_flags(parser: argparse.ArgumentParser) -> None: parser.add_argument("--name", required=True, help="service name") parser.add_argument("--image", required=True, help="container image") - parser.add_argument("--port", type=int, default=8080) - parser.add_argument("--health-path", default="/") + parser.add_argument( + "--port", type=int, default=8080, + help="container port the workload listens on (default: %(default)s)", + ) + parser.add_argument( + "--health-path", default="/", + help="readiness and liveness path (default: %(default)s)", + ) parser.add_argument( "--liveness-path", default="", @@ -537,8 +543,14 @@ def build_parser() -> argparse.ArgumentParser: ) static.add_argument("--name", required=True) static.add_argument("--directory", required=True) - static.add_argument("--port", type=int, default=8080) - static.add_argument("--health-path", default="/") + static.add_argument( + "--port", type=int, default=8080, + help="container port of the fixed static runtime (default: %(default)s)", + ) + static.add_argument( + "--health-path", default="/", + help="readiness and liveness path (default: %(default)s)", + ) static.add_argument( "--exposure", choices=("public", "internal"), default="public" ) @@ -563,8 +575,20 @@ def build_parser() -> argparse.ArgumentParser: ) build_submit.add_argument("--name", required=True) build_submit.add_argument("--directory", required=True) - build_submit.add_argument("--port", type=int, default=8080) - build_submit.add_argument("--health-path", default="/healthz") + build_submit.add_argument( + "--port", type=int, default=8080, + help="container port your Dockerfile exposes (default: %(default)s)", + ) + build_submit.add_argument( + # Deliberately not `/` like the other two: a source build is an + # application the caller wrote, so it is expected to answer a real + # health endpoint. Silent, and different from its siblings, that + # difference cost a build the full readiness timeout and reported only + # "Deployment does not have minimum availability". + "--health-path", default="/healthz", + help="readiness and liveness path your image serves " + "(default: %(default)s, unlike deploy/deploy-static which default to /)", + ) build_status = build_sub.add_parser("status") build_status.add_argument("name") build_delete = build_sub.add_parser("delete") diff --git a/tests/test_interface.py b/tests/test_interface.py index e322375..ac32082 100644 --- a/tests/test_interface.py +++ b/tests/test_interface.py @@ -1902,6 +1902,45 @@ def test_gateway_unbounded_capacity_is_rendered_unambiguously(self) -> None: self.assertIn("there is no configured public-route capacity limit", sentence) self.assertNotIn("None", sentence) + def test_silent_flag_defaults_are_stated_in_help(self) -> None: + """Defaults a caller cannot see must at least be printed. + + `--health-path` is `/` for deploy and deploy-static and `/healthz` for + build submit, and none of the three said so. Taking the sibling default + on faith produced a build that pushed a correct image and then spent the + whole readiness window failing a probe on a path the image does not + serve, reported as "Deployment does not have minimum availability" -- + which points at the workload rather than at the flag. + """ + expected = { + ("deploy",): "/", + ("deploy-static",): "/", + ("build", "submit"): "/healthz", + } + for path, default in expected.items(): + with self.subTest(command=" ".join(path)): + parser = build_parser() + for name in path: + parser = next( + action.choices[name] + for action in parser._subparsers._group_actions + if name in action.choices + ) + for flag in ("--health-path", "--port"): + action = next( + item for item in parser._actions if flag in item.option_strings + ) + self.assertTrue( + action.help, f"{' '.join(path)} {flag} has no help at all" + ) + self.assertIn("%(default)s", action.help, flag) + health = next( + item + for item in parser._actions + if "--health-path" in item.option_strings + ) + self.assertEqual(health.default, default) + if __name__ == "__main__": unittest.main() From 7ad1f5e671bc9e5136b28e499db8612f0a629d0c Mon Sep 17 00:00:00 2001 From: convee Date: Sat, 5 Sep 2026 18:57:23 +0000 Subject: [PATCH 07/17] fix(cli): say "digest", not "summary", and name the rotate command each group has `sites admin merchants create --help` read "only the summary is stored in the library". Two machine translations in one sentence: a digest is not a summary (the security meaning differs) and "library" is a literal rendering of "database". The tenant equivalent said it differently again. Both now say the control plane stores only a digest, and each names the rotate command its own group actually has. A gate checks that: the first attempt at this message said `rotate-token`, which tenants do not have, and a substring check would have accepted it because `rotate` is its prefix. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01365xzFrkTZV7Fr9WmNe6is --- README.md | 2 +- src/sites/cli.py | 4 ++-- tests/test_interface.py | 37 +++++++++++++++++++++++++++++++++++++ 3 files changed, 40 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 8c6652f..ff275f2 100644 --- a/README.md +++ b/README.md @@ -175,7 +175,7 @@ git clone https://github.com/hullwork/site.git cd site uv sync --locked --extra dev make test-db # starts a throwaway PostgreSQL on 127.0.0.1:55439 -make test # 1041 tests +make test # 1042 tests make test-db-down ``` diff --git a/src/sites/cli.py b/src/sites/cli.py index 8bbf16f..cd92b8b 100644 --- a/src/sites/cli.py +++ b/src/sites/cli.py @@ -428,7 +428,7 @@ def _add_admin_commands(sub: argparse._SubParsersAction) -> None: merchant_create = merchants_sub.add_parser( "create", help="create a merchant and print its API key once", - description="The apiKey plaintext is only returned this time, and only the summary is stored in the library; if it is lost, you can only rotate-key.", + description="The plaintext API key is returned only this time; the control plane stores only its digest. A lost key can only be replaced with rotate-key.", ) merchant_create.add_argument("merchant_id") merchant_create.add_argument("--display-name", required=True) @@ -469,7 +469,7 @@ def _add_admin_commands(sub: argparse._SubParsersAction) -> None: tenant_create = tenants_sub.add_parser( "create", help="create a tenant and print its token once", - description="The token plaintext is returned only this time, and only the digest is stored in the library. If it is lost, it can only be rotated.", + description="The plaintext token is returned only this time; the control plane stores only its digest. A lost token can only be replaced with rotate.", ) tenant_create.add_argument("name") tenant_create.add_argument("--merchant", required=True) diff --git a/tests/test_interface.py b/tests/test_interface.py index ac32082..53b2064 100644 --- a/tests/test_interface.py +++ b/tests/test_interface.py @@ -9,6 +9,7 @@ import hashlib import io import json +import re import os import tempfile import threading @@ -1902,6 +1903,42 @@ def test_gateway_unbounded_capacity_is_rendered_unambiguously(self) -> None: self.assertIn("there is no configured public-route capacity limit", sentence) self.assertNotIn("None", sentence) + +class CliHelpContractTests(unittest.TestCase): + """What `--help` promises, since nothing else ever executes it.""" + + def test_admin_help_only_names_commands_that_exist(self) -> None: + """Help that tells the reader what to run next must name a real command. + + Both `create` descriptions end by saying how to replace a lost + credential, and the two groups spell that differently -- merchants use + `rotate-key`, tenants use `rotate`. Text like this is never executed, so + a wrong name here is only found by someone typing it and getting + `invalid choice`. + """ + parser = build_parser() + admin = next( + action.choices["admin"] + for action in parser._subparsers._group_actions + if "admin" in action.choices + ) + groups = next( + action.choices for action in admin._subparsers._group_actions + ) + for group_name, expected in (("merchants", "rotate-key"), ("tenants", "rotate")): + with self.subTest(group=group_name): + commands = next( + action.choices + for action in groups[group_name]._subparsers._group_actions + ) + self.assertIn(expected, commands) + description = commands["create"].description or "" + # Whole tokens, not a substring search: "rotate" is a prefix of + # "rotate-token", so `in` would accept the name this group does + # not have. + named = set(re.findall(r"rotate[a-z-]*", description)) + self.assertEqual(named, {expected}, description) + def test_silent_flag_defaults_are_stated_in_help(self) -> None: """Defaults a caller cannot see must at least be printed. From dd4b6517539b52895ac3ea26e470d800f605d779 Mon Sep 17 00:00:00 2001 From: convee Date: Sat, 5 Sep 2026 19:04:03 +0000 Subject: [PATCH 08/17] docs(agent-contract): specify the deployment authorization every MCP write tool requires Following AGENT_CONTRACT's own client configuration, all six MCP write tools answer `deployment_authorization_required`, and the message says only "do not retry" -- nothing an integrator can implement. The reserved `_agent_deployment_authorization` argument is deliberately absent from every tools/list schema, so the model never learns it exists. That leaves the document as the only place a host can read its shape, and the shape was not there. Adds the field table (version must be 1, runId 1-128, nonce at least 24 chars, expiresAt in the future, allowInternal for internal exposure) and the three rules that make it worth having. The gate checks each documented field against the validator rather than a copy of it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01365xzFrkTZV7Fr9WmNe6is --- README.md | 2 +- docs/AGENT_CONTRACT.md | 44 ++++++++++++++++++++++++++ tests/test_interface.py | 69 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 114 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index ff275f2..364cb4b 100644 --- a/README.md +++ b/README.md @@ -175,7 +175,7 @@ git clone https://github.com/hullwork/site.git cd site uv sync --locked --extra dev make test-db # starts a throwaway PostgreSQL on 127.0.0.1:55439 -make test # 1042 tests +make test # 1043 tests make test-db-down ``` diff --git a/docs/AGENT_CONTRACT.md b/docs/AGENT_CONTRACT.md index 4ee4379..94420c7 100644 --- a/docs/AGENT_CONTRACT.md +++ b/docs/AGENT_CONTRACT.md @@ -165,6 +165,50 @@ X-Acting-Subject: None: class DeploymentIntentBoundaryTests(unittest.TestCase): + def test_the_authorization_an_agent_host_must_inject_is_documented(self) -> None: + """A host cannot implement an object whose shape is written nowhere. + + Every MCP write tool refuses without `_agent_deployment_authorization`, + and the argument is deliberately kept out of `tools/list` so the model + never learns it exists. That leaves the document as the only place a + host integrator can read it. Following AGENT_CONTRACT's own client + configuration, the whole write surface answered + `deployment_authorization_required` and the message names nothing to + implement. + + Each field is checked against the validator rather than a copy of it, so + the table cannot drift away from what is actually enforced. + """ + from sites import mcp as sites_mcp + + contract = ( + Path(__file__).resolve().parent.parent / "docs" / "AGENT_CONTRACT.md" + ).read_text(encoding="utf-8") + self.assertIn(sites_mcp.DEPLOYMENT_AUTHORIZATION_ARGUMENT, contract) + for field in ("version", "runId", "nonce", "expiresAt", "allowInternal"): + with self.subTest(field=field): + self.assertIn(f"`{field}`", contract) + + valid = { + "version": 1, + "runId": "run-1", + "nonce": "n" * 24, + "expiresAt": time.time() + 60, + } + base = { + "deploymentIntent": "publish this and give me the URL", + sites_mcp.DEPLOYMENT_AUTHORIZATION_ARGUMENT: valid, + } + self.assertEqual( + sites_mcp._require_deployment_intent(base), + "publish this and give me the URL", + ) + # Each documented rule must be the reason a call is refused when broken. + for field, broken in ( + ("version", 2), + ("runId", ""), + ("nonce", "n" * 23), + ("expiresAt", time.time() - 1), + ): + with self.subTest(broken=field): + arguments = dict(base) + arguments[sites_mcp.DEPLOYMENT_AUTHORIZATION_ARGUMENT] = { + **valid, + field: broken, + } + with self.assertRaises(sites_mcp.ValidationError) as caught: + sites_mcp._require_deployment_intent(arguments) + self.assertIn("deployment_authorization_required", str(caught.exception)) + # allowInternal is what the table says it is: absent means internal is refused. + with self.assertRaises(sites_mcp.ValidationError): + sites_mcp._require_standalone_exposure_authorization( + {**base, "exposure": "internal"} + ) + sites_mcp._require_standalone_exposure_authorization( + { + "exposure": "internal", + sites_mcp.DEPLOYMENT_AUTHORIZATION_ARGUMENT: { + **valid, + "allowInternal": True, + }, + } + ) + def test_preview_only_intent_is_rejected_before_dispatch(self) -> None: from sites import mcp as sites_mcp From 2d656d1be6594152baef2aa903ec9dfaa3b43bfd Mon Sep 17 00:00:00 2001 From: convee Date: Sat, 5 Sep 2026 19:24:04 +0000 Subject: [PATCH 09/17] fix(contract): state that verification evidence belongs to the revision it names Observed on a live cluster: a dynamic site's version 2 never became ready, and the same `sites status` response carried `phase: Failed` next to `verification.ok: true, httpStatus: 200` -- version 1's evidence. The retention is deliberate; the operator comment says so, and the evidence names the revision it was collected for. What was missing is the rule: AGENT_CONTRACT made `verification.ok=true` the success criterion without saying to compare `verification.revision` with the deployment's `revision`, so a caller reading `ok` alone reports a rollout that never came up as a live site. Both halves are now gated: the response must carry both revisions (or the comparison is impossible) and the contract must state it (or nobody makes it). Also fixes the `scaleToZero` refusal text, where two translated fragments were concatenated without a space and reached the caller as "there will be noThe link can receive the request". A new sweep checks all 408 refusal messages for that shape; its judge is self-tested in both directions, because the first version filtered out the very word it was looking for and reported zero. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01365xzFrkTZV7Fr9WmNe6is --- README.md | 9 +++- docs/AGENT_CONTRACT.md | 16 +++++++ src/sites/validation.py | 5 ++- tests/test_architecture.py | 92 ++++++++++++++++++++++++++++++++++++++ tests/test_sites.py | 52 +++++++++++++++++++++ 5 files changed, 170 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 364cb4b..e589afa 100644 --- a/README.md +++ b/README.md @@ -175,7 +175,7 @@ git clone https://github.com/hullwork/site.git cd site uv sync --locked --extra dev make test-db # starts a throwaway PostgreSQL on 127.0.0.1:55439 -make test # 1043 tests +make test # 1047 tests make test-db-down ``` @@ -235,7 +235,12 @@ sites capabilities `bodySha256` from a request the control plane made itself. Redirects are not followed and only 2xx counts, because those are the two facts a tenant cannot forge. The evidence is bounded on purpose: it proves something at that address returned 2xx with that body - digest — not that the body is semantically correct, since the tenant produced it. + digest — not that the body is semantically correct, since the tenant produced it. It is + also bounded in time: the evidence names the `revision` it was collected for and is kept + when a later revision fails to roll out, so `ok: true` beside `phase: Failed` is last + time's answer, not this one's. Compare `verification.revision` with the deployment's + `revision` — [docs/AGENT_CONTRACT.md](docs/AGENT_CONTRACT.md#success-criteria) states the + full acceptance rule. - **The caller cannot name the tenancy it writes into.** Identity is a `(merchant, tenant)` pair decided entirely by the credential. `X-Merchant-ID` and `X-User-ID` are **refused with 403**, not ignored, so a misconfigured client fails loudly instead of quietly diff --git a/docs/AGENT_CONTRACT.md b/docs/AGENT_CONTRACT.md index 94420c7..08e0d16 100644 --- a/docs/AGENT_CONTRACT.md +++ b/docs/AGENT_CONTRACT.md @@ -50,6 +50,22 @@ completed real HTTP evidence collection. The result also records `httpStatus` an `bodySha256`. The public or host entry point is a separate network path: request the returned URL from the user side and compare its response digest with `bodySha256`. +**Evidence belongs to the revision it names.** `verification.revision` is the revision the +probe ran against, and it is deliberately kept when a later revision fails to roll out — +it remains true that *that* revision served traffic. So `ok=true` next to +`phase: Failed` is not a contradiction and is not success: it is last time's answer. A +caller that reads `ok` alone will report a rollout that never came up as a live site. +Accept a deployment only when **all** of these hold: + +``` +phase == "Running" and ready == true + and verification.ok == true + and verification.revision == revision # the top-level revision of the same response +``` + +Both fields are in every `sites status` and `GET /v1/deployments/{name}` response, so the +comparison needs no extra call. + ## Identity **[AUTH.md](AUTH.md) is the contract.** It is written for any client, diff --git a/src/sites/validation.py b/src/sites/validation.py index e6903bd..4c4238f 100644 --- a/src/sites/validation.py +++ b/src/sites/validation.py @@ -607,8 +607,9 @@ def normalize_deploy_payload( if not exposure_backend.backend().supports_scale_to_zero: raise ValidationError( "scaleToZero requires L7 gateway (SITES_EXPOSURE_BACKEND=gateway): " - "NodePort directly DNATs external traffic to the site Pod. After shrinking to 0, there will be no" - "The link can receive the request and trigger the wake-up" + "NodePort DNATs external traffic straight to the site Pod, so once it " + "scales to 0 there is no hop left to receive the request and trigger " + "the wake-up" ) # Only write spec when true, the same as nodePort: the shape of the existing CR is not affected, # On the reading side, spec.get("scaleToZero") is used to judge. diff --git a/tests/test_architecture.py b/tests/test_architecture.py index 9429724..f81d505 100644 --- a/tests/test_architecture.py +++ b/tests/test_architecture.py @@ -2,6 +2,7 @@ from __future__ import annotations import ast +import re import unittest from pathlib import Path @@ -52,5 +53,96 @@ def visit(module: str) -> None: visit(module) +class RefusalTextTests(unittest.TestCase): + """Every message the caller is left holding has to read as one sentence.""" + + RUN_TOGETHER = re.compile(r"^[a-z]{2,}[A-Z][a-z]{2,}$") + + @staticmethod + def _repository_word_counts() -> dict[str, int]: + """How often each word occurs across the sources a name can live in. + + No hand-written vocabulary, which is what rots after a rename and then + passes everything. A real field name is written down somewhere else too + -- a schema, the Chart, a document -- while a word produced by joining + two message fragments exists in exactly one place: the message. + """ + counts: dict[str, int] = {} + files = [ + *PACKAGE.glob("*.py"), + *(ROOT / "charts").rglob("*.yaml"), + *(ROOT / "docs").glob("*.md"), + ROOT / "README.md", + ] + for path in files: + for word in re.findall(r"[A-Za-z][A-Za-z0-9_]*", path.read_text(encoding="utf-8")): + counts[word] = counts.get(word, 0) + 1 + return counts + + @classmethod + def _run_together(cls, text: str, counts: dict[str, int]) -> str | None: + for match in re.finditer(r"[A-Za-z][A-Za-z0-9_]*", text): + word = match.group(0) + if cls.RUN_TOGETHER.match(word) and counts.get(word, 0) <= 1: + return word + return None + + def test_the_check_can_tell_the_two_apart(self) -> None: + """Prove the judge before trusting the sweep below. + + Its first version stripped every token containing a capital as + "an identifier" -- including the run-together word it was looking for -- + and reported zero across the whole package. A sweep that cannot fail is + not evidence, so both directions are asserted here. + """ + counts = {"scaleToZero": 40, "healthPath": 30, "noThe": 1} + self.assertEqual( + self._run_together("there will be noThe link can receive", counts), + "noThe", + ) + self.assertIsNone(self._run_together("scaleToZero needs a gateway", counts)) + self.assertIsNone(self._run_together("healthPath is /", counts)) + + def test_field_names_used_elsewhere_are_not_flagged(self) -> None: + # Names that appear only inside message text still count as real when + # the Chart, a schema or a document also writes them down. + counts = self._repository_word_counts() + for field in ("clusterNetwork", "fieldRef", "deploymentIntent", "claimValue"): + with self.subTest(field=field): + self.assertGreater(counts.get(field, 0), 1) + + def test_no_refusal_message_runs_two_sentences_together(self) -> None: + """Two translated fragments joined without a space become one bad word. + + `"...there will be no" "The link can receive..."` reached the caller as + "there will be noThe link can receive", and it is the only thing the + caller gets: the request is refused with a 400 whose whole explanation is + that sentence. + """ + counts = self._repository_word_counts() + offenders = [] + scanned = 0 + for source in sorted(PACKAGE.glob("*.py")): + tree = ast.parse(source.read_text(encoding="utf-8")) + for node in ast.walk(tree): + if not isinstance(node, ast.Raise): + continue + for child in ast.walk(node): + if ( + isinstance(child, ast.Constant) + and isinstance(child.value, str) + and len(child.value) > 25 + ): + scanned += 1 + text = " ".join(child.value.split()) + word = self._run_together(text, counts) + if word: + offenders.append(f"{source.name}:{child.lineno} {word!r}") + # A zero with a zero denominator would mean the sweep found nothing to + # look at, which is not the same answer. + self.assertGreater(scanned, 100, "no refusal messages were examined") + self.assertEqual(offenders, []) + + if __name__ == "__main__": unittest.main() diff --git a/tests/test_sites.py b/tests/test_sites.py index 17d8761..5b21252 100644 --- a/tests/test_sites.py +++ b/tests/test_sites.py @@ -1283,6 +1283,58 @@ def test_shipped_bundle_examples_do_not_claim_each_other_names(self) -> None: ) seen[name] = spec_path.name + def test_stale_evidence_stays_and_is_distinguishable_by_revision(self) -> None: + """A failed rollout keeps the previous revision's passing evidence. + + That retention is deliberate -- it is still true that the earlier + revision served traffic -- but it means a response can read + `phase: Failed` and `verification.ok: true` at the same time. Observed + live: a dynamic site's version 2 never became ready, and `sites status` + answered Failed while carrying version 1's 200 and body digest. + + The only thing separating the two is the revision each names, so both + must be in the same response and the contract must say to compare them. + Documenting the rule without the fields, or shipping the fields without + the rule, leaves the caller reading `ok` alone. + """ + obj = { + "metadata": {"name": "local-local-demo-0", "generation": 2}, + "spec": { + "merchantID": DEFAULT_MERCHANT_ID, + "userID": "local", + "serviceName": "demo", + "image": "example.invalid/demo@sha256:" + "b" * 64, + "port": 8080, + "revision": "200", + "exposure": "public", + }, + "status": { + "phase": "Failed", + "observedGeneration": 2, + "message": "Deployment was not ready within 120s", + "verification": { + "ok": True, + "httpStatus": 200, + "bodySha256": "c" * 64, + "revision": "100", + }, + }, + } + response = _deployment_response(obj) + self.assertEqual(response["phase"], "Failed") + self.assertTrue(response["verification"]["ok"]) + self.assertEqual(response["revision"], "200") + self.assertEqual(response["verification"]["revision"], "100") + self.assertNotEqual( + response["revision"], response["verification"]["revision"] + ) + + contract = ( + Path(__file__).resolve().parent.parent / "docs" / "AGENT_CONTRACT.md" + ).read_text(encoding="utf-8") + self.assertIn("verification.revision == revision", contract) + self.assertIn("Evidence belongs to the revision it names", contract) + def test_bundle_components_carry_the_verification_evidence(self) -> None: """Each component must surface the evidence the operator wrote for it. From 4003779934fd046be6f603cdca319ee4900853ce Mon Sep 17 00:00:00 2001 From: convee Date: Sat, 5 Sep 2026 19:41:30 +0000 Subject: [PATCH 10/17] fix(operator): name the quota that refused the pod instead of reporting a timeout The fifth site in a tenant is accepted with a 200 and fails 120 seconds later with "Deployment was not ready within 120s (Deployment does not have minimum availability.)". The cause was only in a ReplicaSet event: `exceeded quota: sites-tenant-quota, requested: limits.cpu=1, used: limits.cpu=4`. `_rollout_stall_reason` exists to separate OOMKilled from CrashLoopBackOff from ImagePullBackOff, and reads the Available condition and pod container states. A namespace ResourceQuota rejects creation at the ReplicaSet, so there is no pod to inspect -- the one stall it cannot see is the one whose Available message is content-free. Kubernetes copies the reason onto the Deployment as a ReplicaFailure condition, which the operator already holds. Two more of the same shape: bundle components reported `Failed` with no message at all, and the two tenant limits disagree -- admission counts `maxDeployments` (10) while the namespace quota allows four concurrent sites. The mismatch is now in Known limitations, with a gate that asks for the limitation to be deleted rather than updated if the numbers ever agree. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01365xzFrkTZV7Fr9WmNe6is --- README.md | 9 +++- docs/CONFIGURATION.md | 2 +- src/sites/operator.py | 23 ++++++++-- src/sites/serializers.py | 6 +++ tests/test_scale_to_zero.py | 91 +++++++++++++++++++++++++++++++++++++ tests/test_sites.py | 28 ++++++++++++ 6 files changed, 154 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index e589afa..57de060 100644 --- a/README.md +++ b/README.md @@ -175,7 +175,7 @@ git clone https://github.com/hullwork/site.git cd site uv sync --locked --extra dev make test-db # starts a throwaway PostgreSQL on 127.0.0.1:55439 -make test # 1047 tests +make test # 1050 tests make test-db-down ``` @@ -414,6 +414,13 @@ production is worse off than one who reads them here. the reference Chart. Admission uses a process-local lock and reconciliation has no leader election, so scaling past one replica requires distributed admission and leader election first. +- **Two tenant limits disagree, and only one of them refuses at submission time.** Admission + counts deployments against `maxDeployments` (default 10) and answers `429 quota_exceeded`. + The tenant namespace also carries a `ResourceQuota` of `SITES_TENANT_CPU_LIMIT` (default 4) + against a fixed `limits.cpu: 1` per site, so a tenant can run **4** sites at once. Numbers + 5 through 10 are accepted with a 200 and then fail to roll out. The refusal now names the + exhausted quota rather than only reporting a readiness timeout, but the two numbers are + still set independently. - `sync_once()` holds the API's mutation lock across a Kubernetes `GET`. The Kubernetes client timeout (10s) and the mutation-lock acquire timeout (`SITES_MUTATION_LOCK_TIMEOUT`, 10s) are the same order of magnitude, so a slow apiserver can make write paths return diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index 1c21c77..f0824c1 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -175,7 +175,7 @@ and an invalid value generally raises at import time rather than falling back. | `SITES_SOURCE_ROOT` | `/var/lib/sites/sources` | Mount path for that PVC | | `SITES_OSS_AUTH_SECRET` | `sites-oss-auth` | Secret holding object-storage credentials | | `SITES_OSS_AUTH_MOUNT` | `/var/run/sites-oss` | Where that Secret is mounted | -| `SITES_TENANT_CPU_LIMIT` | `4` | Per-tenant namespace CPU quota | +| `SITES_TENANT_CPU_LIMIT` | `4` | Per-tenant namespace CPU quota. Each site container is capped at `limits.cpu: 1`, so this is also the number of sites one tenant can run at once — independently of, and by default lower than, the tenant's `maxDeployments`. See [README known limitations](../README.md#known-limitations) | | `SITES_TENANT_MEMORY_LIMIT` | `4Gi` | Per-tenant namespace memory quota | | `SITES_TENANT_POD_LIMIT` | `16` | Per-tenant namespace Pod quota | diff --git a/src/sites/operator.py b/src/sites/operator.py index 777a79b..7948944 100644 --- a/src/sites/operator.py +++ b/src/sites/operator.py @@ -342,14 +342,31 @@ def _rollout_stall_reason( """ reasons: list[str] = [] try: - for condition in (deployment.get("status") or {}).get("conditions") or []: + conditions = (deployment.get("status") or {}).get("conditions") or [] + # ReplicaFailure first, because the pod loop below cannot see the one + # stall that produces no pod at all. A namespace ResourceQuota rejects + # creation at the ReplicaSet, so every container state is empty and the + # Available condition says only "does not have minimum availability" -- + # the caller was told the workload was slow when it had in fact been + # refused, and the exceeded quota was named nowhere it would look. + # kubelet copies the reason onto this object, so reading it costs nothing. + for condition in conditions: if ( - str(condition.get("type")) == "Available" - and str(condition.get("status")) != "True" + str(condition.get("type")) == "ReplicaFailure" + and str(condition.get("status")) == "True" and (message := str(condition.get("message") or "").strip()) ): reasons.append(message) break + if not reasons: + for condition in conditions: + if ( + str(condition.get("type")) == "Available" + and str(condition.get("status")) != "True" + and (message := str(condition.get("message") or "").strip()) + ): + reasons.append(message) + break selector = urllib.parse.quote( ",".join( f"{key}={value}" diff --git a/src/sites/serializers.py b/src/sites/serializers.py index 42570b0..622d3e7 100644 --- a/src/sites/serializers.py +++ b/src/sites/serializers.py @@ -325,6 +325,12 @@ def bundle_response( # projection meant a bundle caller saw phase Running and nothing # else, with no way to reach evidence that already existed. "verification": status.get("verification"), + # Same omission on the failure side: a component that could not + # roll out reported `Failed` and nothing else, while the sentence + # naming the cause -- an exhausted tenant quota, an image that + # will not pull -- was already on the resource. A standalone + # deployment has carried it all along. + "message": status.get("message", ""), } ) # The expected number of components is the currently found group: the control plane does not preset any bundle composition. diff --git a/tests/test_scale_to_zero.py b/tests/test_scale_to_zero.py index 4d3ec4f..cd4f403 100644 --- a/tests/test_scale_to_zero.py +++ b/tests/test_scale_to_zero.py @@ -1120,6 +1120,97 @@ def test_genuine_stall_without_scale_event_still_fails(self) -> None: self.assertEqual(status["phase"], "Failed") self.assertIn("not ready within", status["message"]) + def test_a_stall_that_creates_no_pod_still_names_its_cause(self) -> None: + """The one stall with nothing to inspect is the one that must be reported. + + A namespace ResourceQuota refuses the pod at the ReplicaSet, so there is + no container state to read and the Available condition says only + "Deployment does not have minimum availability". Observed live: the fifth + site in a tenant hit `limits.cpu=4` and the caller was told, 120 seconds + after a 200, that the workload was not ready -- while the sentence naming + the exhausted quota sat unread on the Deployment the operator already had. + """ + from sites.operator import _rollout_stall_reason + + class _NoPods: + def get(self, path: str): + return {"items": []} + + quota = ( + 'pods "demo-6cf4c7f4df-gjhfn" is forbidden: exceeded quota: ' + "sites-tenant-quota, requested: limits.cpu=1, used: limits.cpu=4, " + "limited: limits.cpu=4" + ) + deployment = { + "status": { + "conditions": [ + { + "type": "Available", + "status": "False", + "message": "Deployment does not have minimum availability.", + }, + { + "type": "ReplicaFailure", + "status": "True", + "reason": "FailedCreate", + "message": quota, + }, + ] + } + } + reason = _rollout_stall_reason(_NoPods(), deployment, "ns-a", "demo") + self.assertIn("exceeded quota", reason) + self.assertIn("limits.cpu=4", reason) + # The generic sentence adds nothing once the specific one is present. + self.assertNotIn("minimum availability", reason) + + # Without a ReplicaFailure the previous behaviour is unchanged. + deployment["status"]["conditions"] = [ + { + "type": "Available", + "status": "False", + "message": "Deployment does not have minimum availability.", + } + ] + self.assertIn( + "minimum availability", + _rollout_stall_reason(_NoPods(), deployment, "ns-a", "demo"), + ) + + def test_the_two_tenant_limits_are_documented_with_their_real_numbers(self) -> None: + """Whatever the two limits are, the document must state both. + + `maxDeployments` refuses at submission; the namespace ResourceQuota + refuses at rollout. Deriving one from the other would be a product + decision, so this only pins that the smaller one is written down with the + number it actually is -- a reader who sees 10 and no second number plans + for 10 sites and discovers the fourth is the last. + """ + from sites.k8s_resources import TENANT_CPU_LIMIT + from sites.validation import DEFAULT_MAX_DEPLOYMENTS + + spec = normalize_deploy_payload(payload(), DEFAULT_MERCHANT_ID, "local") + containers = deployment_resource(spec, "ns-a")["spec"]["template"]["spec"][ + "containers" + ] + self.assertTrue(containers) + per_site_cpu = { + container["resources"]["limits"]["cpu"] for container in containers + } + self.assertEqual(per_site_cpu, {"1"}, "one CPU per site is what makes the two comparable") + concurrent = int(TENANT_CPU_LIMIT) + self.assertLess( + concurrent, + DEFAULT_MAX_DEPLOYMENTS, + "if these ever agree, drop the limitation instead of updating it", + ) + readme = (pathlib.Path(__file__).resolve().parent.parent / "README.md").read_text( + encoding="utf-8" + ) + self.assertIn("Two tenant limits disagree", readme) + self.assertIn(f"run **{concurrent}** sites at once", readme) + self.assertIn(f"`maxDeployments` (default {DEFAULT_MAX_DEPLOYMENTS})", readme) + def test_new_revision_is_verified_before_keda_is_enabled(self) -> None: with using_backend("gateway"): spec = normalize_deploy_payload( diff --git a/tests/test_sites.py b/tests/test_sites.py index 5b21252..e403412 100644 --- a/tests/test_sites.py +++ b/tests/test_sites.py @@ -1369,6 +1369,34 @@ def test_bundle_components_carry_the_verification_evidence(self) -> None: for component in response["components"]: self.assertEqual(component["verification"], evidence) + def test_bundle_components_carry_the_reason_they_failed(self) -> None: + # A component that never rolled out reported `Failed` and nothing else, + # so the caller had no way to tell an exhausted tenant quota from an + # image that will not pull. The operator writes the reason onto the + # resource either way; only the projection dropped it. + objects = bundle_resources( + "demo-stack", + self._sample_components(), + DEFAULT_MERCHANT_ID, + "local", + ) + reason = ( + "Deployment was not ready within 120s (exceeded quota: " + "sites-tenant-quota, requested: limits.cpu=1, used: limits.cpu=4)" + ) + for resource in objects: + resource["metadata"]["generation"] = 1 + resource["status"] = { + "phase": "Failed", + "observedGeneration": 1, + "message": reason, + } + response = _bundle_response("demo-stack", objects) + self.assertEqual(response["phase"], "Failed") + self.assertTrue(response["components"]) + for component in response["components"]: + self.assertEqual(component["message"], reason) + def _crd_spec_properties() -> set[str]: """Read the SiteDeployment spec fields the CRD actually declares. From 19d9eac8c0358a2cc471f9987db8096efa78daad Mon Sep 17 00:00:00 2001 From: convee Date: Sun, 6 Sep 2026 00:50:20 +0000 Subject: [PATCH 11/17] fix(contract): bound the evidence to the path it was actually collected from A site whose root returns 403 reported `phase: Running`, `ready: true`, `verification.ok: true`, `httpStatus: 200`, and a public URL that answers 403. The probe requests `spec.healthPath`, not the site root, and writes that address into `verification.url`. `deploy` and `deploy-static` default to `/` so the two have always coincided; `build submit` defaults to `/healthz`, and there they are different resources. AGENT_CONTRACT nonetheless tells callers to compare the public URL's digest with `bodySha256`, which can only hold at `/`, and the quickstart's "public URL body: matches verification digest" assertion passes only because it takes the default. The bound is now stated where the criterion is. Gated on all three sides: the URL is built from healthPath, the document says so, and the trial passes no `--health-path` (its own assertion would fail if it did). Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01365xzFrkTZV7Fr9WmNe6is --- README.md | 6 +++++- docs/AGENT_CONTRACT.md | 16 ++++++++++++++++ tests/test_sites.py | 35 +++++++++++++++++++++++++++++++++++ 3 files changed, 56 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 57de060..394f42d 100644 --- a/README.md +++ b/README.md @@ -175,7 +175,7 @@ git clone https://github.com/hullwork/site.git cd site uv sync --locked --extra dev make test-db # starts a throwaway PostgreSQL on 127.0.0.1:55439 -make test # 1050 tests +make test # 1051 tests make test-db-down ``` @@ -236,6 +236,10 @@ sites capabilities only 2xx counts, because those are the two facts a tenant cannot forge. The evidence is bounded on purpose: it proves something at that address returned 2xx with that body digest — not that the body is semantically correct, since the tenant produced it. It is + bounded in *place* too: the probe requests the deployment's `healthPath`, and + `verification.url` names it. With the default `/` that is the address a person opens; with + a health path such as `/healthz` it is not, and a site can be verified while its public URL + answers 403. It is also bounded in time: the evidence names the `revision` it was collected for and is kept when a later revision fails to roll out, so `ok: true` beside `phase: Failed` is last time's answer, not this one's. Compare `verification.revision` with the deployment's diff --git a/docs/AGENT_CONTRACT.md b/docs/AGENT_CONTRACT.md index 08e0d16..c18da63 100644 --- a/docs/AGENT_CONTRACT.md +++ b/docs/AGENT_CONTRACT.md @@ -50,6 +50,22 @@ completed real HTTP evidence collection. The result also records `httpStatus` an `bodySha256`. The public or host entry point is a separate network path: request the returned URL from the user side and compare its response digest with `bodySha256`. +**Evidence covers `healthPath`, not the whole site.** The probe requests +`http://..svc:` and writes that exact address into +`verification.url`. So the digest comparison above is only meaningful when `healthPath` is +`/`: at any other value the two are different resources and comparing them must fail. +`sites deploy` and `sites deploy-static` default to `/`, **`sites build submit` defaults to +`/healthz`**, and a site deployed that way can be `Running`, `ready`, `verification.ok`, +and hold a public URL that answers `403` — measured, not hypothetical. Before treating the +evidence as being about the address a person will open, check that the two agree: + +``` +verification.url.endswith(healthPath) and healthPath == "/" +``` + +Otherwise the evidence says only that the health endpoint answered 2xx with that digest, +which is exactly what it was collected from. + **Evidence belongs to the revision it names.** `verification.revision` is the revision the probe ran against, and it is deliberately kept when a later revision fails to roll out — it remains true that *that* revision served traffic. So `ok=true` next to diff --git a/tests/test_sites.py b/tests/test_sites.py index e403412..07b67ef 100644 --- a/tests/test_sites.py +++ b/tests/test_sites.py @@ -1283,6 +1283,41 @@ def test_shipped_bundle_examples_do_not_claim_each_other_names(self) -> None: ) seen[name] = spec_path.name + def test_evidence_is_collected_from_health_path_and_says_so(self) -> None: + """The probe address must be the documented one, and documented as bounded. + + The contract tells a caller to compare the public URL's response digest + with `bodySha256`, which only holds while `healthPath` is `/`. Measured: + a source build (whose `--health-path` defaults to `/healthz`) came up + `Running`, `ready`, `verification.ok=true`, `httpStatus=200`, with a + public URL that answered 403 — the probe had asked `/healthz` and the + person opens `/`. + + Both halves are pinned because either alone is useless: the URL builder + without the caveat is an undocumented trap, and the caveat without the + builder is a claim about code that may have moved on. + """ + source = ( + Path(__file__).resolve().parent.parent / "src" / "sites" / "operator.py" + ).read_text(encoding="utf-8") + self.assertIn('f"{spec[\'healthPath\']}"', source) + contract = ( + Path(__file__).resolve().parent.parent / "docs" / "AGENT_CONTRACT.md" + ).read_text(encoding="utf-8") + self.assertIn("Evidence covers `healthPath`, not the whole site", contract) + self.assertIn("`sites build submit` defaults to\n`/healthz`", contract) + + # The trial prints "public URL body: matches verification digest" and + # hard-fails when they differ, which is only reachable because it takes + # the default health path. Passing one would break the trial itself. + quickstart = ( + Path(__file__).resolve().parent.parent + / "scripts" + / "quickstart-kubeadm.sh" + ).read_text(encoding="utf-8") + self.assertIn("public URL body digest", quickstart) + self.assertNotIn("--health-path", quickstart) + def test_stale_evidence_stays_and_is_distinguishable_by_revision(self) -> None: """A failed rollout keeps the previous revision's passing evidence. From 134411836fbd40a77934aca61c38313c1e5c27bd Mon Sep 17 00:00:00 2001 From: convee Date: Sun, 6 Sep 2026 01:01:55 +0000 Subject: [PATCH 12/17] fix(standalone): take the cluster's Pod CIDR and values file like cluster.sh already does `scripts/cluster_benchmark.py` could not run on any cluster whose Pod network is not the reference topology's. It installs through `standalone.sh`, which hardcoded `values-dev.yaml` and therefore podCIDR 10.201.0.0/16; the install succeeds and the operator then refuses to start, by design, because its own address falls outside the declaration. `cluster.sh` has had SITES_HELM_VALUES and SITES_CLUSTER_POD_CIDR all along. Adding the same two seams to its sibling makes the benchmark -- the script whose report invites reproduction -- runnable anywhere. Default behaviour is unchanged. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01365xzFrkTZV7Fr9WmNe6is --- README.md | 2 +- scripts/standalone.sh | 15 +++++++++++++-- tests/test_helm_package.py | 26 ++++++++++++++++++++++++++ 3 files changed, 40 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 394f42d..16a62f7 100644 --- a/README.md +++ b/README.md @@ -175,7 +175,7 @@ git clone https://github.com/hullwork/site.git cd site uv sync --locked --extra dev make test-db # starts a throwaway PostgreSQL on 127.0.0.1:55439 -make test # 1051 tests +make test # 1052 tests make test-db-down ``` diff --git a/scripts/standalone.sh b/scripts/standalone.sh index 5158937..96e2f03 100755 --- a/scripts/standalone.sh +++ b/scripts/standalone.sh @@ -6,7 +6,13 @@ action=${1:-} if [[ -n "$action" ]]; then shift; fi namespace=sites-local release=site -values_file="$root/charts/site/values-dev.yaml" +# Same two seams cluster.sh already exposes, and for the same reason: the values +# overlay names one specific local topology, and clusterNetwork.podCIDR is a fact +# about the target cluster that the operator refuses to start without. Without +# them this script -- and therefore scripts/cluster_benchmark.py, which installs +# through it -- could only ever run against a cluster whose Pod network happens +# to be the overlay's. +values_file=${SITES_HELM_VALUES:-$root/charts/site/values-dev.yaml} helm_set=() helm_set_value=() context=${SITES_KUBE_CONTEXT:-} @@ -30,13 +36,18 @@ Usage: standalone.sh [options] Options: --namespace NAME Kubernetes namespace (default: sites-local) --release NAME Helm release name (default: site) - --values FILE Helm values file (default: charts/site/values-dev.yaml) + --values FILE Helm values file (default: charts/site/values-dev.yaml, + or $SITES_HELM_VALUES) --set KEY=VALUE Additional Helm string value (repeatable) --set-value KEY=VALUE Additional typed Helm value (repeatable; booleans/numbers) EOF } +if [[ -n "${SITES_CLUSTER_POD_CIDR:-}" ]]; then + helm_set+=(--set-string "clusterNetwork.podCIDR=$SITES_CLUSTER_POD_CIDR") +fi + while [[ $# -gt 0 ]]; do case "$1" in --namespace) namespace=${2:?missing namespace}; shift 2 ;; diff --git a/tests/test_helm_package.py b/tests/test_helm_package.py index fc87785..19a2c28 100644 --- a/tests/test_helm_package.py +++ b/tests/test_helm_package.py @@ -413,6 +413,32 @@ def test_standalone_scripts_are_syntax_checked_and_never_embed_secret_values(sel self.assertIn("SITES_HOST_PORT_BASE", adapter) self.assertIn("nodePort.hostPortBase", adapter) + def test_both_install_paths_take_the_same_cluster_facts(self) -> None: + """standalone.sh must accept what cluster.sh accepts. + + The two are the repository's install seams and cluster_benchmark.py goes + through standalone.sh. That one hardcoded values-dev.yaml, whose + clusterNetwork.podCIDR names the reference kubeadm topology, and the + operator refuses to start when its own address falls outside it. So the + benchmark -- the script whose report invites reproduction -- could only + run on a cluster whose Pod network happened to match, and nothing said + so: the install succeeds and the operator then refuses. + """ + standalone = (ROOT / "scripts" / "standalone.sh").read_text(encoding="utf-8") + adapter = (ROOT / "scripts" / "cluster.sh").read_text(encoding="utf-8") + for seam in ("SITES_HELM_VALUES", "SITES_CLUSTER_POD_CIDR"): + with self.subTest(seam=seam): + self.assertIn(seam, adapter) + self.assertIn(seam, standalone) + self.assertIn( + 'values_file=${SITES_HELM_VALUES:-$root/charts/site/values-dev.yaml}', + standalone, + ) + self.assertIn( + 'helm_set+=(--set-string "clusterNetwork.podCIDR=$SITES_CLUSTER_POD_CIDR")', + standalone, + ) + def test_registry_address_follows_the_namespace_it_is_installed_into(self) -> None: """The registry Service address must come from namespaces.control. From 8bca9d00a1c2ede3715654368313d722c4048f33 Mon Sep 17 00:00:00 2001 From: convee Date: Sun, 6 Sep 2026 01:07:56 +0000 Subject: [PATCH 13/17] docs(configuration): finish the SITES_* set instead of restating a stale count README and CONFIGURATION both said "45 SITES_* variables ... appear in no document or chart". Measured on that sentence's own definition: 116 are read, 86 appear in documents, 40 in the Chart or scripts, and 8 appear in neither. Even counting documents alone gives 30. Same shape as the "historical UIDs" bullet removed earlier: a limitation that overstates a problem which has largely been closed, drifting in the safe direction so nothing surfaces it. With eight left they are simply documented, the count goes to zero, and the README bullet now states what is genuinely still true -- these variables are not schema-validated. A gate requires every variable the code reads to be named in a document or the Chart, and self-tests its own scan so an empty read set cannot pass as success. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01365xzFrkTZV7Fr9WmNe6is --- README.md | 12 ++++----- docs/CONFIGURATION.md | 23 +++++++++++----- tests/test_readme_claims.py | 52 +++++++++++++++++++++++++++++++++++++ 3 files changed, 74 insertions(+), 13 deletions(-) diff --git a/README.md b/README.md index 16a62f7..2523425 100644 --- a/README.md +++ b/README.md @@ -175,7 +175,7 @@ git clone https://github.com/hullwork/site.git cd site uv sync --locked --extra dev make test-db # starts a throwaway PostgreSQL on 127.0.0.1:55439 -make test # 1052 tests +make test # 1054 tests make test-db-down ``` @@ -452,11 +452,11 @@ production is worse off than one who reads them here. **Housekeeping** -- 45 `SITES_*` environment variables are read by `src/sites/` but appear in no document or - chart — mostly activator, gateway, NodePort-pool, KEDA, and operator tuning. They have - working defaults, and the most useful ones are now listed under - [Undocumented tuning variables](docs/CONFIGURATION.md#undocumented-tuning-variables), but - the set is not yet complete or schema-validated. +- `SITES_*` environment variables are still not schema-validated: an invalid value + generally raises at import time rather than falling back. Every one `src/sites/` reads is + named in a document or the chart — the leftovers are under + [Undocumented tuning variables](docs/CONFIGURATION.md#undocumented-tuning-variables) — and + a test keeps that true, but "named" is not "validated". - `api.py`, `operator.py`, and `storage.py` are large, and environment reads are spread across process-owned modules instead of validated once at startup. diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index f0824c1..c11fda2 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -124,14 +124,15 @@ rather than being guessed at. Helm exposes it as `mcpEndpoint.enabled`. ## Undocumented tuning variables -45 `SITES_*` variables are read by `src/sites/` but appear in no other document and in no -Chart template. They all have working defaults, so nothing breaks by leaving them unset; -they are listed here because "not discoverable" is a different problem from "not -supported". This is operator/GitOps configuration in the sense of the table above: it is -read once at process start and a change requires a rollout. +Every `SITES_*` variable `src/sites/` reads now appears in a document or a Chart template; +this table holds the ones that appear nowhere else. They all have working defaults, so +nothing breaks by leaving them unset; they are listed because "not discoverable" is a +different problem from "not supported". This is operator/GitOps configuration in the sense +of the table above: it is read once at process start and a change requires a rollout. -The list below is the useful subset, not the complete set. It is not schema-validated, -and an invalid value generally raises at import time rather than falling back. +The set is complete and a test keeps it that way, so adding a new `getenv("SITES_...")` +means adding a row here or naming it in the Chart. It is still not schema-validated, and an +invalid value generally raises at import time rather than falling back. | Variable | Default | Meaning | | --- | --- | --- | @@ -178,6 +179,14 @@ and an invalid value generally raises at import time rather than falling back. | `SITES_TENANT_CPU_LIMIT` | `4` | Per-tenant namespace CPU quota. Each site container is capped at `limits.cpu: 1`, so this is also the number of sites one tenant can run at once — independently of, and by default lower than, the tenant's `maxDeployments`. See [README known limitations](../README.md#known-limitations) | | `SITES_TENANT_MEMORY_LIMIT` | `4Gi` | Per-tenant namespace memory quota | | `SITES_TENANT_POD_LIMIT` | `16` | Per-tenant namespace Pod quota | +| `SITES_DB_BACKEND` | empty | Rejected unless empty, `postgres`, or `postgresql`; there is no second backend to select | +| `SITES_BUILDKIT_IMAGE` | pinned `moby/buildkit` rootless digest | Builder image the BuildKit Job runs | +| `SITES_OSS_DOWNLOADER_IMAGE` | pinned `site-control` release | initContainer that materializes a versioned static artifact | +| `SITES_REGISTRY_AUTH_SECRET` | `sites-registry-auth` | Secret holding the registry password, htpasswd, and the `config.json` the build Job mounts | +| `SITES_REGISTRY_AUTH_MOUNT` | `/etc/sites-registry` | Where that Secret is mounted; also the Job's `DOCKER_CONFIG` | +| `SITES_REGISTRY_PASSWORD_FILE` | `/var/run/sites-registry/password` | File the control plane reads its own registry password from | +| `SITES_GATEWAY_POD_LABEL_KEY` | `gateway.envoyproxy.io/owning-gateway-name` | Label that identifies this Gateway's data-plane Pods to the tenant NetworkPolicy | +| `SITES_GATEWAY_POD_LABEL_VALUE` | `SITES_GATEWAY_NAME` | Value for that label | ## Current cleanup and remaining work diff --git a/tests/test_readme_claims.py b/tests/test_readme_claims.py index 62bf3ec..9635538 100644 --- a/tests/test_readme_claims.py +++ b/tests/test_readme_claims.py @@ -94,5 +94,57 @@ def test_readme_states_the_count_make_test_collects(self) -> None: ) +class EnvironmentVariableDiscoverabilityTests(unittest.TestCase): + """Every knob the code reads has to be named somewhere a reader looks. + + README carried "45 `SITES_*` environment variables ... appear in no document + or chart" long after the number stopped being true: measured on the same + definition the sentence uses, it was 8. A count nobody recomputes is a claim, + and this one drifted the safe direction -- overstating a gap that had mostly + been closed -- which is why nothing surfaced it. + """ + + READ = re.compile( + r"(?:getenv|environ\.get|environ)\(\s*[\"'](SITES_[A-Z0-9_]+)[\"']" + ) + + @staticmethod + def _named_in(paths) -> set[str]: + found: set[str] = set() + for path in paths: + if path.is_file(): + found |= set( + re.findall(r"SITES_[A-Z0-9_]+", path.read_text(encoding="utf-8")) + ) + return found + + def _read_by_source(self) -> set[str]: + names: set[str] = set() + for path in (REPO_ROOT / "src" / "sites").glob("*.py"): + names |= set(self.READ.findall(path.read_text(encoding="utf-8"))) + return names + + def test_the_scan_finds_the_variables_it_is_meant_to(self) -> None: + """Guard the denominator: an empty read set would pass silently.""" + read = self._read_by_source() + self.assertGreater(len(read), 50, "the getenv scan matched almost nothing") + for known in ("SITES_HOST_PORT_BASE", "SITES_EXPOSURE_BACKEND"): + self.assertIn(known, read) + + def test_every_variable_the_code_reads_is_named_somewhere(self) -> None: + documented = self._named_in( + list((REPO_ROOT / "docs").glob("*.md")) + [README] + ) + charted = self._named_in((REPO_ROOT / "charts").rglob("*")) + missing = sorted(self._read_by_source() - documented - charted) + self.assertEqual( + missing, + [], + "read by src/sites/ but named in no document and no chart template; " + "add a row to docs/CONFIGURATION.md#undocumented-tuning-variables " + f"or set it in the chart: {missing}", + ) + + if __name__ == "__main__": unittest.main() From 00bbe43f19508942c5fb0cab69bbf14582fc5709 Mon Sep 17 00:00:00 2001 From: convee Date: Sun, 6 Sep 2026 01:09:34 +0000 Subject: [PATCH 14/17] docs(standalone): say that the Chart's cluster-scoped CRDs make one release exclusive Installing a second release into another namespace fails with Helm's ownership error about `meta.helm.sh/release-namespace` -- a message about Helm annotations that mentions neither Site nor the limit it is enforcing. Both CRDs are cluster-scoped and rendered as ordinary templates, so the first release owns them, while `--namespace` on both install scripts implies the opposite. Documented, with the real error text, plus a gate that also pins the premise: if the CRDs ever move to the Chart's `crds/` directory the exclusivity disappears and the note should be deleted rather than edited. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01365xzFrkTZV7Fr9WmNe6is --- README.md | 2 +- docs/STANDALONE.md | 15 +++++++++++++++ tests/test_helm_package.py | 38 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 54 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 2523425..8cb6930 100644 --- a/README.md +++ b/README.md @@ -175,7 +175,7 @@ git clone https://github.com/hullwork/site.git cd site uv sync --locked --extra dev make test-db # starts a throwaway PostgreSQL on 127.0.0.1:55439 -make test # 1054 tests +make test # 1055 tests make test-db-down ``` diff --git a/docs/STANDALONE.md b/docs/STANDALONE.md index be588a9..370bd37 100644 --- a/docs/STANDALONE.md +++ b/docs/STANDALONE.md @@ -49,6 +49,21 @@ on control-plane reboot and removed with the disposable VM. deletes its VMs, repository-owned Lima network, every application inside it, and this checkout's local kubeconfig. +**One release per cluster.** The Chart's two CustomResourceDefinitions +(`sitedeployments.sites.local`, `sitebuilds.sites.local`) are cluster-scoped and are +rendered as ordinary templates, so the release that installs them owns them. A second +install into any other namespace stops at Helm's ownership check: + +```text +Error: Unable to continue with install: CustomResourceDefinition +"sitedeployments.sites.local" in namespace "" exists and cannot be imported into the +current release: invalid ownership metadata; annotation validation error: +key "meta.helm.sh/release-namespace" must equal "": current value is "" +``` + +`--namespace` selects where the control plane runs, not how many control planes a cluster +can hold. Uninstall the existing release before installing another, or use another cluster. + The lower-level path installs Site into the Kubernetes context selected by `kubectl`. Review the current context before continuing: diff --git a/tests/test_helm_package.py b/tests/test_helm_package.py index 19a2c28..ac4bef7 100644 --- a/tests/test_helm_package.py +++ b/tests/test_helm_package.py @@ -413,6 +413,44 @@ def test_standalone_scripts_are_syntax_checked_and_never_embed_secret_values(sel self.assertIn("SITES_HOST_PORT_BASE", adapter) self.assertIn("nodePort.hostPortBase", adapter) + def test_cluster_scoped_crds_make_the_release_exclusive_and_say_so(self) -> None: + """`--namespace` looks like it permits a second install. It does not. + + Both CRDs are cluster-scoped and rendered as ordinary templates, so the + first release owns them and any later install anywhere in the cluster + stops on Helm's ownership check -- an error about + `meta.helm.sh/release-namespace` annotations, which says nothing about + Site to a reader who has not met it before. Nothing said the limit + existed, while `--namespace` on both install scripts implied the + opposite. + + If the CRDs ever move to the Chart's `crds/` directory the exclusivity + goes away, and this test should be deleted rather than adjusted. + """ + rendered = subprocess.run( + ["helm", "template", "site", str(CHART), *POD_CIDR], + check=True, + capture_output=True, + text=True, + ).stdout + crds = [ + item + for item in yaml.safe_load_all(rendered) + if isinstance(item, dict) + and item.get("kind") == "CustomResourceDefinition" + ] + self.assertEqual( + sorted(item["metadata"]["name"] for item in crds), + ["sitebuilds.sites.local", "sitedeployments.sites.local"], + ) + self.assertFalse( + (CHART / "crds").exists(), + "CRDs moved out of templates/; the exclusivity note is now wrong", + ) + standalone_doc = (ROOT / "docs" / "STANDALONE.md").read_text(encoding="utf-8") + self.assertIn("One release per cluster", standalone_doc) + self.assertIn("meta.helm.sh/release-namespace", standalone_doc) + def test_both_install_paths_take_the_same_cluster_facts(self) -> None: """standalone.sh must accept what cluster.sh accepts. From 60ff88090261c3c9a0ea1e2be52d90e8af113bed Mon Sep 17 00:00:00 2001 From: convee Date: Sun, 6 Sep 2026 01:11:57 +0000 Subject: [PATCH 15/17] docs(readme): put the two missing modules on the map and count the mixins from the code The per-module table listed 43 of the package's 45 modules. Missing were `api_mcp.py` -- the `POST /mcp` endpoint that README and AGENT_CONTRACT both treat as a headline surface -- and `http_kit.py`. The composition-root sentence counted eight endpoint mixins while `Handler` combines nine. A hand-maintained table drifts silently because adding a module gives nobody a reason to reopen it. Gated three ways: every module has a row, every row names a file that exists, and the mixin count and its spelled-out word are derived from which `api_*.py` files actually define a Mixin. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01365xzFrkTZV7Fr9WmNe6is --- README.md | 9 +++--- tests/test_readme_claims.py | 60 +++++++++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 8cb6930..4f0830e 100644 --- a/README.md +++ b/README.md @@ -175,7 +175,7 @@ git clone https://github.com/hullwork/site.git cd site uv sync --locked --extra dev make test-db # starts a throwaway PostgreSQL on 127.0.0.1:55439 -make test # 1055 tests +make test # 1058 tests make test-db-down ``` @@ -308,8 +308,8 @@ so the reference Deployments intentionally run **one replica each**. ### Module map -`api.py` is the composition root and combines eight endpoint mixins (auth, tenants, -merchants, admin, builds, bundles, deployments, sites). +`api.py` is the composition root and combines nine endpoint mixins (auth, tenants, +merchants, admin, builds, bundles, deployments, sites, mcp) over the shared HTTP kit.
Per-module responsibilities under src/sites/ @@ -318,7 +318,8 @@ merchants, admin, builds, bundles, deployments, sites). |---|---| | `api.py` | Composition root: combines mixins, assembles `serve()`, applies exception mappings, owns metric route templates | | `api_errors.py` | Ordered exception-to-HTTP mappings shared by mutation endpoints | -| `api_auth.py`, `api_tenants.py`, `api_merchants.py`, `api_admin.py`, `api_builds.py`, `api_bundles.py`, `api_deployments.py`, `api_sites.py` | The endpoint mixins | +| `api_auth.py`, `api_tenants.py`, `api_merchants.py`, `api_admin.py`, `api_builds.py`, `api_bundles.py`, `api_deployments.py`, `api_sites.py`, `api_mcp.py` | The endpoint mixins; `api_mcp.py` is the MCP tool surface as `POST /mcp` | +| `http_kit.py` | JSON and static-file helpers, bounded body reads, route matching, console SPA fallback; no control-plane logic | | `identity.py` | Pure authentication: `(headers, store, tokens) → Identity or Refusal` | | `admission.py` | Pure admission, quota, and port-allocation logic plus refusal exceptions | | `validation.py` | Input validation (`normalize_*`), identity and quota constants, `DEPLOY_FIELDS`, `STATIC_IMAGE` | diff --git a/tests/test_readme_claims.py b/tests/test_readme_claims.py index 9635538..aa7df9c 100644 --- a/tests/test_readme_claims.py +++ b/tests/test_readme_claims.py @@ -94,6 +94,66 @@ def test_readme_states_the_count_make_test_collects(self) -> None: ) +class ModuleMapTests(unittest.TestCase): + """The per-module table has to still describe this package. + + It listed 43 of 45 modules: `api_mcp.py` -- the `POST /mcp` endpoint, which + the README and AGENT_CONTRACT both treat as a headline surface -- and + `http_kit.py` were simply absent, and the composition-root sentence counted + "eight endpoint mixins" while `Handler` combined nine. A table maintained by + hand drifts silently, because nothing about adding a module makes anyone + reopen it. + """ + + TABLE = re.compile( + r"\| File under `src/sites/` \| Responsibility \|(?P.+?)
", + re.S, + ) + + def setUp(self) -> None: + self.text = README.read_text(encoding="utf-8") + self.modules = sorted( + path.name + for path in (REPO_ROOT / "src" / "sites").glob("*.py") + if path.name != "__init__.py" + ) + + def test_the_table_is_where_this_gate_thinks_it_is(self) -> None: + self.assertIsNotNone(self.TABLE.search(self.text)) + self.assertGreater(len(self.modules), 20) + + def test_every_module_appears_in_the_map(self) -> None: + body = self.TABLE.search(self.text).group("body") + named = set(re.findall(r"`([a-z_0-9]+\.py)`", body)) + self.assertEqual( + sorted(set(self.modules) - named), + [], + "modules with no row in the README map", + ) + self.assertEqual( + sorted(named - set(self.modules)), + [], + "rows naming files that no longer exist", + ) + + def test_the_mixin_sentence_names_every_endpoint_mixin(self) -> None: + """Derived from the classes, not from a number someone typed.""" + mixins = sorted( + path.stem.removeprefix("api_") + for path in (REPO_ROOT / "src" / "sites").glob("api_*.py") + if re.search(r"^class \w+Mixin", path.read_text(encoding="utf-8"), re.M) + ) + self.assertIn("mcp", mixins) + spelled = { + 8: "eight", 9: "nine", 10: "ten", 11: "eleven", 12: "twelve", + }.get(len(mixins)) + self.assertIsNotNone(spelled, f"add {len(mixins)} to the spelled-out numbers") + self.assertIn(f"combines {spelled} endpoint mixins", self.text) + for name in mixins: + with self.subTest(mixin=name): + self.assertRegex(self.text, rf"endpoint mixins \([^)]*\b{name}\b") + + class EnvironmentVariableDiscoverabilityTests(unittest.TestCase): """Every knob the code reads has to be named somewhere a reader looks. From 53a86276ab38b5ea249da89cd8e247c4a13ea41b Mon Sep 17 00:00:00 2001 From: convee Date: Sun, 6 Sep 2026 01:17:27 +0000 Subject: [PATCH 16/17] fix(gateway): make the host-facing URL parts Chart values instead of template literals The three quantities that compose a gateway URL -- domain suffix, scheme, host port -- were literals in the template, and the comment said the last two are the local Lima forward. Same shape as the NodePort backend's HOST_PORT_BASE on the other backend: routing works, only the address handed to the caller is wrong, and server-side verification passes either way because it uses the in-cluster address. The suffix was worse than hardcoded: it appeared twice, in the ConfigMap and in the listener wildcard, with the file instructing operators to change both by hand and a test to catch them if they did not. One value renders both now, so they cannot be changed apart. Defaults are byte-identical, so the reference topology is unaffected. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01365xzFrkTZV7Fr9WmNe6is --- README.md | 2 +- charts/site/templates/08-gateway.yaml | 24 ++++++++-------- charts/site/values.schema.json | 18 ++++++++++++ charts/site/values.yaml | 11 ++++++++ tests/test_exposure.py | 40 +++++++++++++++++++++++++++ 5 files changed, 83 insertions(+), 12 deletions(-) diff --git a/README.md b/README.md index 4f0830e..d0cda3f 100644 --- a/README.md +++ b/README.md @@ -175,7 +175,7 @@ git clone https://github.com/hullwork/site.git cd site uv sync --locked --extra dev make test-db # starts a throwaway PostgreSQL on 127.0.0.1:55439 -make test # 1058 tests +make test # 1059 tests make test-db-down ``` diff --git a/charts/site/templates/08-gateway.yaml b/charts/site/templates/08-gateway.yaml index 4c7af35..44835b8 100644 --- a/charts/site/templates/08-gateway.yaml +++ b/charts/site/templates/08-gateway.yaml @@ -24,11 +24,12 @@ # URL of the party. When the three places are allocated separately, the performance of any one place drifting is "the site cannot be accessed" - and three copies # The configuration alone is reasonable. When placed in ConfigMap, there is no such state as "only two changes" physically. # -# Change to your own wildcard domain name: change SITES_DOMAIN_SUFFIX below, **at the same time** change Gateway listener -# hostname (below this file, two places are nailed by GatewayManifestContractTest), and then give -# `*.` Add a DNS record pointing to the gateway. Domain name goes 80/443 when SITES_GATEWAY_SCHEME -# and SITES_GATEWAY_HOST_PORT must also be changed accordingly, otherwise the URL returned to the caller will contain local -# 18090 port. +# Change to your own wildcard domain name with `gateway.domainSuffix`: it renders both +# SITES_DOMAIN_SUFFIX below and the Gateway listener's wildcard, so the two cannot be +# changed apart (they used to be two literals kept together by +# GatewayManifestContractTest). Point `*.` at the gateway in DNS, and set +# `gateway.scheme`/`gateway.hostPort` to how the host really answers -- they default to +# the reference Lima forward, and left alone they hand the caller a URL naming port 18090. apiVersion: v1 kind: ConfigMap metadata: @@ -41,10 +42,10 @@ data: # Only when this value is gateway, the site will use the L7 entrance and scaleToZero will be allowed. SITES_EXPOSURE_BACKEND: "gateway" # Site hostname = -.. - SITES_DOMAIN_SUFFIX: "127.0.0.1.sslip.io" + SITES_DOMAIN_SUFFIX: {{ .Values.gateway.domainSuffix | quote }} # Host-facing URL scheme and port for the local Lima port forwarding. - SITES_GATEWAY_SCHEME: "http" - SITES_GATEWAY_HOST_PORT: "18090" + SITES_GATEWAY_SCHEME: {{ .Values.gateway.scheme | quote }} + SITES_GATEWAY_HOST_PORT: {{ .Values.gateway.hostPort | quote }} # The tenant image is in the workload registry, not the CP Registry that hosts the control plane image. # The node containerd receives this stable name from the deployment portal to the Registry pull Service. SITES_REGISTRY_PULL_HOST: {{ .Values.registry.pullHost | quote }} @@ -134,9 +135,10 @@ spec: # This is especially true for real domain names - `*.*.apps.example.com` is a two-level wildcard that most DNS service providers # Not supported. The host_for of sites/exposure.py and this together form the same contract. # - # When changing to self-built wildcard DNS, change SITES_DOMAIN_SUFFIX and the hostname here. The two must be synchronized. - # (GatewayManifestContractTest pinned). - hostname: "*.127.0.0.1.sslip.io" + # Rendered from gateway.domainSuffix, the same value SITES_DOMAIN_SUFFIX above + # takes, so self-built wildcard DNS is one value rather than two edits that + # have to be kept in step (GatewayManifestContractTest still checks the pair). + hostname: {{ printf "*.%s" .Values.gateway.domainSuffix | quote }} allowedRoutes: # 🔴 Must be Selector instead of Same: HTTPRoute is built in the **tenant** Namespace, # And Gateway is in sites-gateway. Leaving it as the default Same will cause every tenant route to be diff --git a/charts/site/values.schema.json b/charts/site/values.schema.json index a59621a..a14123c 100644 --- a/charts/site/values.schema.json +++ b/charts/site/values.schema.json @@ -272,7 +272,10 @@ "type": "object", "additionalProperties": false, "required": [ + "domainSuffix", "enabled", + "hostPort", + "scheme", "serviceType" ], "properties": { @@ -301,6 +304,21 @@ ], "minimum": 30000, "maximum": 32767 + }, + "domainSuffix": { + "type": "string", + "minLength": 1 + }, + "scheme": { + "enum": [ + "http", + "https" + ] + }, + "hostPort": { + "type": "integer", + "minimum": 1, + "maximum": 65535 } } }, diff --git a/charts/site/values.yaml b/charts/site/values.yaml index 7cd686b..602cb01 100644 --- a/charts/site/values.yaml +++ b/charts/site/values.yaml @@ -108,6 +108,17 @@ gateway: serviceType: LoadBalancer httpNodePort: null metricsNodePort: null + # The host-facing half of a gateway URL: what sits between a site's own name + # and its path. scheme and hostPort are only what the caller is handed -- + # routing does not read them -- and default to the reference trial's Lima + # forward, so on any other host they are the first two values to set. + # domainSuffix is load-bearing: it is both the listener's wildcard and the + # hostname the operator writes into every HTTPRoute. One value feeds both, so + # changing it cannot leave the two disagreeing; point `*.` at the + # gateway in DNS before using anything but the sslip.io default. + domainSuffix: 127.0.0.1.sslip.io + scheme: http + hostPort: 18090 # How a browser outside the cluster reaches a site published by the `nodeport` # exposure backend. Both keys describe the machine in front of the cluster, not diff --git a/tests/test_exposure.py b/tests/test_exposure.py index ea8ddb4..1f0f1a2 100644 --- a/tests/test_exposure.py +++ b/tests/test_exposure.py @@ -606,6 +606,46 @@ def test_listener_hostname_covers_the_domain_suffix(self) -> None: listener = self._doc("Gateway")["spec"]["listeners"][0] self.assertEqual(listener["hostname"], f"*.{exposure.DOMAIN_SUFFIX}") + def test_the_host_facing_url_parts_are_values_not_literals(self) -> None: + """A different host must not require editing the template. + + These three decide the URL the caller is handed, and all three were + literals: `127.0.0.1.sslip.io`, `http`, and `18090` -- the reference + Lima forward, written into every installation's gateway URLs. The + suffix was worse than merely hardcoded: it appeared twice, in the + ConfigMap and in the listener wildcard, and the file told the operator + to change both by hand. One value renders both now, so they cannot be + changed apart. + """ + overrides = ( + "--set-string", "gateway.domainSuffix=apps.example.test", + "--set-string", "gateway.scheme=https", + "--set", "gateway.hostPort=443", + ) + docs = chart.documents("08-gateway.yaml", *overrides) + config = next( + doc for doc in docs + if doc["kind"] == "ConfigMap" and "SITES_DOMAIN_SUFFIX" in doc["data"] + ) + self.assertEqual(config["data"]["SITES_DOMAIN_SUFFIX"], "apps.example.test") + self.assertEqual(config["data"]["SITES_GATEWAY_SCHEME"], "https") + self.assertEqual(config["data"]["SITES_GATEWAY_HOST_PORT"], "443") + listener = next( + doc for doc in docs if doc["kind"] == "Gateway" + )["spec"]["listeners"][0] + self.assertEqual(listener["hostname"], "*.apps.example.test") + # The default render still is what it was, so this is a seam and not a + # behaviour change for anyone already on the reference topology. + default = next( + doc for doc in self._docs() + if doc["kind"] == "ConfigMap" and "SITES_DOMAIN_SUFFIX" in doc["data"] + ) + self.assertEqual(default["data"]["SITES_DOMAIN_SUFFIX"], exposure.DOMAIN_SUFFIX) + self.assertEqual( + default["data"]["SITES_GATEWAY_HOST_PORT"], + str(exposure.GATEWAY_HOST_PORT), + ) + def test_listener_admits_routes_from_tenant_namespaces(self) -> None: """Must be a Selector: HTTPRoute is built in tenant ns, and Gateway is in its own ns. From eda9ecf344d09dd56606042308e8b97ed5405d9c Mon Sep 17 00:00:00 2001 From: convee Date: Sun, 6 Sep 2026 01:45:48 +0000 Subject: [PATCH 17/17] fix(gateway): tell the control plane which Gateway the Chart actually created On the gateway backend a site reported Running with `verification.ok: true` and a public URL that answered 404, and its HTTPRoute had no parent status at all. The Gateway object is named after `namespaces.gateway`, while SITES_GATEWAY_NAME and SITES_GATEWAY_NAMESPACE defaulted to the literal `sites-gateway` and the Chart never set them. They agree only when the release happens to be installed into a namespace of that name -- and both install scripts set `namespaces.gateway` to the release namespace, which defaults to `sites-local`. Gateway routing has therefore been broken on every installation made through this repository's own scripts. A second consequence from the same missing pair: the tenant NetworkPolicy admits `owning-gateway-name: `, so it allowed a label the real data-plane Pods do not carry -- precisely the symptom exposure.py's comment predicts, where the policy is valid, applies cleanly, and the site still receives rejected traffic. None of the three layers reports an error. The Chart now renders both variables from `namespaces.gateway`, so one source feeds the parentRef and the policy label. Measured before and after on kind with Envoy Gateway v1.2.4 and KEDA 2.20.2, installed into `sites-gw`: parentRef `sites-gateway/sites-gateway` with no parent status and a 404, then parentRef `sites-gw/sites-gw`, Accepted=True, ResolvedRefs=True, policy label `sites-gw`, and a 200 whose body digest matches the recorded verification digest. The existing identity test renders the defaults, where both sides read `sites-gateway` and agree regardless of what the Chart does; the new one uses a different namespace. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01365xzFrkTZV7Fr9WmNe6is --- README.md | 2 +- charts/site/templates/08-gateway.yaml | 12 +++++++++ tests/test_exposure.py | 39 +++++++++++++++++++++++++++ 3 files changed, 52 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index d0cda3f..be48033 100644 --- a/README.md +++ b/README.md @@ -175,7 +175,7 @@ git clone https://github.com/hullwork/site.git cd site uv sync --locked --extra dev make test-db # starts a throwaway PostgreSQL on 127.0.0.1:55439 -make test # 1059 tests +make test # 1060 tests make test-db-down ``` diff --git a/charts/site/templates/08-gateway.yaml b/charts/site/templates/08-gateway.yaml index 44835b8..2957594 100644 --- a/charts/site/templates/08-gateway.yaml +++ b/charts/site/templates/08-gateway.yaml @@ -44,6 +44,18 @@ data: # Site hostname = -.. SITES_DOMAIN_SUFFIX: {{ .Values.gateway.domainSuffix | quote }} # Host-facing URL scheme and port for the local Lima port forwarding. + # Which Gateway the control plane points at. The object below is named after + # namespaces.gateway, while the code defaulted both of these to the literal + # "sites-gateway", so they agreed only when the release happened to be + # installed into a namespace of that name -- and both install scripts set + # namespaces.gateway to the release namespace, which by default is + # sites-local. Everywhere else the operator wrote HTTPRoutes whose parentRef + # named a Gateway that does not exist and a tenant NetworkPolicy that admitted + # a data-plane Pod label nothing carries. Neither shows up as an error: the + # route is created, the deployment is Running, verification passes on the + # in-cluster address, and the public URL answers 404. + SITES_GATEWAY_NAME: {{ .Values.namespaces.gateway | quote }} + SITES_GATEWAY_NAMESPACE: {{ .Values.namespaces.gateway | quote }} SITES_GATEWAY_SCHEME: {{ .Values.gateway.scheme | quote }} SITES_GATEWAY_HOST_PORT: {{ .Values.gateway.hostPort | quote }} # The tenant image is in the workload registry, not the CP Registry that hosts the control plane image. diff --git a/tests/test_exposure.py b/tests/test_exposure.py index 1f0f1a2..100f520 100644 --- a/tests/test_exposure.py +++ b/tests/test_exposure.py @@ -606,6 +606,45 @@ def test_listener_hostname_covers_the_domain_suffix(self) -> None: listener = self._doc("Gateway")["spec"]["listeners"][0] self.assertEqual(listener["hostname"], f"*.{exposure.DOMAIN_SUFFIX}") + def test_the_gateway_identity_survives_a_non_default_namespace(self) -> None: + """The control plane must be told which Gateway the Chart made. + + The Gateway object is named after `namespaces.gateway`, while + `SITES_GATEWAY_NAME` and `SITES_GATEWAY_NAMESPACE` defaulted to the + literal `sites-gateway` and the Chart never set them. They agreed only + when the release happened to live in a namespace of that name -- and + both install scripts set `namespaces.gateway` to the release namespace, + which defaults to `sites-local`. + + Measured on a real cluster before the fix: every HTTPRoute carried + `parentRef: sites-gateway/sites-gateway`, no Gateway claimed it (the + route had no parent status at all), the tenant NetworkPolicy admitted a + data-plane Pod label nothing carried, and the site was `Running` with + `verification.ok` while its public URL answered 404. After: route + Accepted=True, policy label `sites-gw`, URL 200 with the verified digest. + + The existing identity test above renders the defaults, where both sides + read `sites-gateway` and agree no matter what the Chart does; this one + has to use a different namespace or it asserts nothing. + """ + overrides = ("--set-string", "namespaces.gateway=elsewhere-gw") + docs = chart.documents("08-gateway.yaml", *overrides) + gateway = next(doc for doc in docs if doc["kind"] == "Gateway") + config = next( + doc for doc in docs + if doc["kind"] == "ConfigMap" and "SITES_GATEWAY_NAME" in doc["data"] + ) + self.assertEqual(gateway["metadata"]["name"], "elsewhere-gw") + self.assertEqual(config["data"]["SITES_GATEWAY_NAME"], "elsewhere-gw") + self.assertEqual(config["data"]["SITES_GATEWAY_NAMESPACE"], "elsewhere-gw") + self.assertEqual( + config["data"]["SITES_GATEWAY_NAME"], gateway["metadata"]["name"] + ) + self.assertEqual( + config["data"]["SITES_GATEWAY_NAMESPACE"], + gateway["metadata"]["namespace"], + ) + def test_the_host_facing_url_parts_are_values_not_literals(self) -> None: """A different host must not require editing the template.