From 36030f40111de0cd5c980e76b80ef8c0851ad0ce Mon Sep 17 00:00:00 2001 From: Jan Novak Date: Thu, 8 Oct 2026 20:29:48 +0200 Subject: [PATCH 01/11] docs: add plan for porting house conventions Co-Authored-By: Claude Opus 5.5 --- .../2026-10-08-2028-house-conventions.md | 12 ++ .../2026-10-08-2028-house-conventions.md | 203 ++++++++++++++++++ 2 files changed, 215 insertions(+) create mode 100644 docs/plans-executions/2026-10-08-2028-house-conventions.md create mode 100644 docs/plans/2026-10-08-2028-house-conventions.md diff --git a/docs/plans-executions/2026-10-08-2028-house-conventions.md b/docs/plans-executions/2026-10-08-2028-house-conventions.md new file mode 100644 index 0000000..39c2f7c --- /dev/null +++ b/docs/plans-executions/2026-10-08-2028-house-conventions.md @@ -0,0 +1,12 @@ +# Execution: Bring gateway-helper-operator up to house conventions + +Plan: `docs/plans/2026-10-08-2028-house-conventions.md` + +- [x] Step 0 — Save and commit the plan +- [ ] Step 1 — CLAUDE.md +- [ ] Step 2 — `.claude/` and docs scaffolding +- [ ] Step 3 — Makefile, lint, formatting +- [ ] Step 4 — Module path and version identity +- [ ] Step 5 — Baseline tests +- [ ] Step 6 — CI build workflow +- [ ] Step 7 — Close out diff --git a/docs/plans/2026-10-08-2028-house-conventions.md b/docs/plans/2026-10-08-2028-house-conventions.md new file mode 100644 index 0000000..8c27c7a --- /dev/null +++ b/docs/plans/2026-10-08-2028-house-conventions.md @@ -0,0 +1,203 @@ +# Plan: Bring gateway-helper-operator up to house conventions + +**Created:** 2026-10-08 20:28 CEST + +## Context + +This repo dates from March 2026: 381 lines of Go, no tests, no Makefile, no lint +config, a `/init`-generated CLAUDE.md (uncommitted, with factual errors) and a +one-line `/ship`. The newer repos share a consistent set of practices, distilled in +the `go-project-bootstrap` skill (which names this repo as its operator example) and +extended in `egress-proxies-operator`, the closest analog. This plan ports those +practices here so the following bug-fix and feature work runs on the same rails: +tests, a quality gate, plans with execution logs, a changelog, and branch + MR flow. + +No reconcile behaviour changes in this plan. Bugs found during the survey are listed +at the end as the backlog for the next phase. + +### What the newer projects have in common + +| Practice | Source | Here today | +|---|---|---| +| CLAUDE.md in five sections (What this is, Commands, Architecture, Conventions, Gotchas), under ~150 lines | skill | `/init` output, three factual errors | +| House Go rules (ctx first, `%w` / `errors.Join`, no `init()`, stdlib first, fail fast) | skill, uh-cli, egress | absent | +| Testing rules: stdlib `testing`, table-driven, `Test_`, `t.Parallel()`, `-short` guard, `-race` | skill, uh-cli, egress | no tests at all | +| Plans in `docs/plans/YYYY-MM-DD-HHMM-.md`, committed alone before work | uh-cli, egress, capi-lab | absent | +| Paired `docs/plans-executions/` log with a resumable step checklist | egress, capi-lab | absent | +| Timestamped `CHANGELOG.md`, newest first, written after you confirm it works | all three | absent | +| `feat/` branch, `tea pr create`, never merge or delete from the CLI | all three | everything on `main` | +| `/ship` = commit + push with branch guard; `/release` = tag + confirm + push | egress, capi-lab | `/ship` tags every commit `0.NN` | +| Reviewer agents, findings saved to `docs/reviews/` | egress | absent | +| Committed `.claude/settings.json` allowlist, `settings.local.json` ignored | uh-cli, egress, capi-lab | only an untracked, un-ignored local file | +| Makefile with `help` and a one-shot `check` gate | uh-cli | absent | +| `.golangci.yml` v2 | uh-cli, egress | absent | +| Commit baked into the binary (`internal/version`, `--version`, startup log, OCI label, `sha-<12>` image tag) | egress | CI passes build args the Dockerfile discards | +| CI `check` job gating the image build; `:latest` moves only on tag pushes | egress | build only, `:latest` on every run | + +### Decisions + +- Docs workflow: egress-style (plans, execution logs, changelog, reviews). No + decisions/sessions/future-tasks files. +- `/ship` and `/release` split; new tags are semver `vMAJOR.MINOR.PATCH`, starting + at `v0.1.0`. Old `0.NN` tags stay as they are. +- `kubernetes-deploy.yaml` is not touched here (deferred to the bug-fix phase). +- Deviations from the house text, written into CLAUDE.md as such: + - Logging stays on logr/zap through controller-runtime, not `slog` (same as egress + does in practice). + - Integration tests mean envtest behind `testing.Short()`, not testcontainers. + - Commit subjects keep this repo's existing prefixes (`feat:`, `fix:`, `deploy:`, + `ci:`, `docs:`). +- `.agents/`, `.codex/`, `AGENTS.md` are not ported: in egress they are untracked + auto-migrations or kubebuilder scaffold, not conventions. + +## Steps (one commit each, execution-log entry per step) + +Branch: `feat/house-conventions` off `main`. + +### Step 0 — Save and commit the plan + +Copy this file to `docs/plans/2026-10-08-2028-house-conventions.md`, create +`docs/plans-executions/2026-10-08-2028-house-conventions.md` with the step +checklist, commit alone. + +### Step 1 — CLAUDE.md + +Rewrite `CLAUDE.md` in the skill's five-section shape +(`~/.claude/skills/go-project-bootstrap/references/claude-md-structure.md`). + +- **What this is / Commands / Architecture**: keep the annotation table and + reconcile-loop write-up; point to `docs/flows.md` for detail. Fix the errors: + leader election is not enabled; cert deletion does not clean up; `listener-port` + must not be listed as supported. +- **Conventions**: paste verbatim from the skill references + (`go-conventions.md`, `docs-conventions.md`, `git-workflow.md` Gitea variant, + `containers-k8s.md`) plus egress's "Plan execution summaries" and plan "Ordering + matters" paragraphs from + `/Users/jan.novak/srv/go/egress-proxies-operator/CLAUDE.md`. Remote: + `gitea.home.hrajfrisbee.cz/kacerr/gatewayapi-certificates-operator`. Add the + deviations above, the semver tag rule, and the push rule (confirm before + `git push`, except inside `/ship`). +- **Gotchas**: three project names (directory, remote/image, module/manifests); + port annotation declared but unused; stale listeners on cert delete, annotation + removal and retarget; `--metrics-bind-address` parsed but ignored (metrics still + serve on `:8080`); `ListenersEqual` is partial; `auto-` prefix is the only + ownership marker; deployment uses mutable `:latest`; Dockerfile base images are + not digest-pinned; `kubernetes-deploy.yaml` runs on every branch push. + +Also in this step: commit `docs/flows.md` after correcting its three wrong claims +(metrics "never bound"; cleanup via resync; name collisions being "last write +wins" — the API server rejects the patch), and move `docs/README.md` to the repo +root with `kubectl apply -f deploy/manifests.yaml` corrected to +`kubectl apply -k deploy/`. + +### Step 2 — `.claude/` and docs scaffolding + +- `.claude/settings.json`: the skill's `assets/settings.json` starter allowlist + plus `Bash(tea pr create:*)`, `Bash(make *)` for the Step 3 targets and + `Bash(docker build:*)`. No `git commit`, `git push` or `git tag`. +- `.claude/commands/ship.md` and `release.md`: copy from + `egress-proxies-operator/.claude/commands/`; in `release.md` replace the "no + tagging scheme" paragraph with the semver rule. +- `.claude/agents/operator-reviewer.md`, `go-operator-reviewer.md`: copy from + `egress-proxies-operator/.claude/agents/` unchanged. +- `CHANGELOG.md` (`# Changelog` header only), `docs/reviews/.gitkeep`. +- `.gitignore`: add `.claude/settings.local.json`, `bin/`, `cover.out`; fix the + missing trailing newline. + +### Step 3 — Makefile, lint, formatting + +- `Makefile` modelled on `/Users/jan.novak/srv/go/uh-cli/Makefile`: `help` + (self-documenting), `build` (to `bin/manager`), `run`, `test` (`-race -cover`), + `test-short`, `vet`, `fmt`, `lint`, `tidy`, `check` (lint + vet + test-short), + `docker-build` (passes `GIT_COMMIT`, using egress's dirty-aware definition), + `clean`. +- `.golangci.yml`: v2, default linters, as in uh-cli. Egress's config is unmodified + kubebuilder scaffold and currently does not pass, so it is not the model. +- `.dockerignore`: egress's allowlist form. +- `gofmt` `main.go` and `internal/gateway/patch.go`; fix whatever `make lint` + reports (the dead `gatewayGroup` / `gatewayKind` vars are expected). +- Add the quality-gate sentence for `make check` to CLAUDE.md. + +### Step 4 — Module path and version identity + +- Rename the module from `github.com/example/gateway-cert-operator` to + `gitea.home.hrajfrisbee.cz/kacerr/gatewayapi-certificates-operator` (`go.mod`, + the import in `main.go`). +- `internal/version/version.go` + test, ported from + `egress-proxies-operator/internal/version/` (`Commit` via ldflags, fallback to + `debug.ReadBuildInfo`). +- `main.go`: `--version` flag; first log line carries commit and Go version; zap + options bound to flags (`opts.BindFlags`, `Development: true` default, so output + is unchanged unless flags are passed); pass `metricsAddr` to + `metricsserver.Options` so the existing flag takes effect (default stays `:8080`). +- `Dockerfile`: `ARG GIT_COMMIT=unknown`, `-ldflags -X …/internal/version.Commit`, + `GOARCH=${TARGETARCH:-amd64}`, `org.opencontainers.image.revision` label, drop the + unused `GIT_TAG` / `BUILD_DATE` expectations. + +### Step 5 — Baseline tests + +`internal/gateway/patch_test.go`: table-driven tests for +`BuildListenersFromCertificates`, `MergeListeners`, `ListenersEqual` and +`listenerNameFor`, covering current correct behaviour only (single and multiple +DNS names, sort order, manual listeners preserved, stale `auto-` listeners +dropped, equality ignoring order). Known-buggy inputs (wildcards, collisions, +truncation) are left for the bug-fix phase so the tests do not pin wrong +behaviour. Reconciler tests with the fake client come with the first reconcile fix. + +### Step 6 — CI build workflow + +`.gitea/workflows/build.yaml`, following +`egress-proxies-operator/.gitea/workflows/build.yaml`: `check` job (`go vet`, +`go build`, `go test -short`) gating `build`; `GIT_COMMIT` build arg and OCI +labels; push `:` and `:sha-<12>`; `:latest` only on real tag pushes; +concurrency group. Update the CI/CD paragraph in CLAUDE.md to match. + +Not verifiable locally. If `actions/setup-go@v5` fails on the runner (uh-cli hit +that), fall back to uh-cli's curl install from the `go.mod` version. + +### Step 7 — Close out + +Run both reviewer agents on the branch, save findings to +`docs/reviews/-house-conventions.md`, fix what is in scope. Push with +`/ship`, open the MR with `tea pr create`. The CHANGELOG entry is added after you +confirm it works. + +## Verification + +```bash +make check # lint + vet + test-short, all green +go test -race -cover ./... # new tests pass under -race +go build ./... && go vet ./... +make build && ./bin/manager --version # prints the 12-char commit +make docker-build IMG=gateway-cert-operator:dev +docker run --rm gateway-cert-operator:dev --version +kubectl kustomize deploy/ >/dev/null # manifests still render +``` + +Live check after merge: push tag `v0.1.0`, confirm the `check` job runs before +`build`, and that the registry has `:v0.1.0`, `:sha-<12>` and `:latest`. + +## Out of scope — backlog for the bug-fix phase + +Found during the survey, with file references; each is high confidence unless noted. + +1. Stale listeners after cert deletion, annotation removal or retarget + (`certificate_controller.go:71-91`). Root cause: the reconcile key is the + Certificate but the unit of desired state is the Gateway. +2. Wildcard DNS names produce an invalid listener name, and the rejected patch + blocks every cert on that Gateway (`patch.go:145`). +3. Listener name collisions: across namespaces, across cert/DNS splits, and after + truncation at 253 chars (`patch.go:143-151`). +4. `listener-port` annotation never read (`certificate_controller.go:25`, + `patch.go:44`). +5. Whole-array merge patch from a cached read, no optimistic lock + (`certificate_controller.go:118`). +6. `ListenersEqual` ignores protocol, allowed routes, TLS mode and extra refs + (`patch.go:93-139`). +7. Missing Gateway retried forever as an error (`certificate_controller.go:105-108`). +8. Cross-namespace secret refs without a ReferenceGrant; any Certificate author + can add `AllowedRoutes: All` listeners to any Gateway. +9. No events, status or metrics; no watch predicates; no leader election. +10. `kubernetes-deploy.yaml`: Vault, Kanidm and OIDC tokens echoed to the log, + TLS verification off, deploys on every branch push. +11. Deployment pinned to `:latest`, so new builds do not roll out. -- 2.54.0 From 7a559bacd080f1a863394975137d9ce66d322fca Mon Sep 17 00:00:00 2001 From: Jan Novak Date: Thu, 8 Oct 2026 20:31:00 +0200 Subject: [PATCH 02/11] docs: rewrite CLAUDE.md to house conventions, fix flows.md and README - CLAUDE.md in five sections with Go, testing, plan, changelog, branching and tagging rules; known sharp edges moved to Gotchas - docs/flows.md: metrics are served on :8080, name collisions are rejected by the API server, last-cert deletion never cleans up - README moved to the repo root, deploy command corrected Co-Authored-By: Claude Opus 5.5 --- CLAUDE.md | 247 ++++++++- docs/README.md => README.md | 6 +- docs/flows.md | 493 ++++++++++++++++++ .../2026-10-08-2028-house-conventions.md | 24 +- 4 files changed, 764 insertions(+), 6 deletions(-) rename docs/README.md => README.md (86%) create mode 100644 docs/flows.md diff --git a/CLAUDE.md b/CLAUDE.md index 63516e1..3eb53d0 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1,5 +1,246 @@ -# Claude Instructions +# CLAUDE.md -## Git +This file provides guidance to Claude Code (claude.ai/code) when working with code in this repository. -- Always ask for confirmation before running `git push`. \ No newline at end of file +## What this is + +A Kubernetes operator (plain controller-runtime, no CRDs) that bridges cert-manager +`Certificate` resources and Gateway API `Gateway` resources. When a `Certificate` +carries the right annotations, the operator manages HTTPS listeners on the target +Gateway: one listener per entry in `spec.dnsNames`, terminating TLS with the +Certificate's secret. + +## Commands + +```bash +go build ./... # build +go vet ./... # vet +go test ./... # all tests +go test -race -v -run TestName ./path/to/package # single test +golangci-lint run # lint +go run . # run against the current kubeconfig context +docker build -t gateway-cert-operator . +kubectl kustomize deploy/ # render the manifests +``` + +## Architecture + +| Path | Role | +|---|---| +| `main.go` | Composition root: scheme, flags, manager, health probes. No leader election. | +| `internal/controller/certificate_controller.go` | `CertificateReconciler`: watches `Certificate` and `Gateway`, runs the reconcile loop | +| `internal/gateway/patch.go` | Pure functions: `BuildListenersFromCertificates`, `MergeListeners`, `ListenersEqual` | +| `deploy/` | Kustomize manifests; namespace `gateway-cert-operator-system`, single replica | + +Annotations on `cert-manager.io/v1` `Certificate`: + +| Annotation | Required | Default | +|---|---|---| +| `gateway-cert-operator.io/gateway-name` | yes | — | +| `gateway-cert-operator.io/gateway-namespace` | no | Certificate's namespace | + +Reconcile loop (keyed by Certificate): + +1. Fetch the `Certificate`; on not-found return without doing anything. +2. Resolve the target Gateway from the annotations; no annotation, no action. +3. List all Certificates targeting that Gateway through the `IndexFieldGatewayTarget` + field index (key `"namespace/name"`). +4. `BuildListenersFromCertificates` computes the desired `auto-*` listeners. +5. `MergeListeners` keeps every non-`auto-` listener and replaces the rest. +6. Patch the Gateway only when `ListenersEqual` reports a difference. + +A reverse watch maps Gateway events back to the targeting Certificates through the +same index, so manual Gateway edits are reverted for `auto-*` listeners. + +Listeners named `auto-{certName}-{dnsName with dots as dashes}` belong to the +operator. Any other listener is never modified or removed. + +`docs/flows.md` has the full control-path reference (startup, watch wiring, decision +tree, scenarios). + +## Conventions + +### Go Conventions + +- Use the latest stable Go version (check https://go.dev/dl/ before starting a new feature). +- Logging: `logr` through controller-runtime (`log.FromContext(ctx)`), not `slog` — + a deliberate deviation from the house rule, because the manager, client and + controller all log through logr. Keys are lowerCamel. +- `context.Context` is the first parameter of every function that does I/O or can be cancelled. +- No `init()` unless there is no other option. +- Error wrapping: `fmt.Errorf("...: %w", err)` for single errors; `errors.Join` for multiple. +- Prefer stdlib over third-party dependencies where reasonable. +- No vendoring unless explicitly requested. +- Fail fast, return early. +- No obvious comments. + +### Testing + +**All new Go code must have corresponding tests.** + +- Use stdlib `testing` — no testify unless already present in the module. +- Table-driven tests are the default pattern. +- Test files live next to source: `foo.go` → `foo_test.go`. +- Name tests `Test_` (e.g. `TestMergeListeners_keepsManual`). +- Use `t.Parallel()` where safe. +- Use subtests for table entries: `t.Run(tc.name, func(t *testing.T) { ... })`. +- Test behaviour, not implementation. +- Guard slow tests with `testing.Short()`: `if testing.Short() { t.Skip() }`. +- Reconciler tests use controller-runtime's fake client (`WithIndex` for the field + index). Reach for envtest only for what the fake cannot do (API-server validation + of listeners), and guard it with `testing.Short()`. This replaces the house + `testcontainers-go` rule, which does not fit an operator. +- A bug fix lands with a regression test. + +### Plans + +When Claude Code's plan mode is used, save the plan file inside the repo at +`docs/plans/YYYY-MM-DD-HHMM-.md` instead of the default `~/.claude/plans/` +location. Get the timestamp with `date "+%Y-%m-%d-%H%M"`. The `` is a short +kebab-case summary of the plan's topic. + +Include the same timestamp in the plan's header, e.g.: + + # Plan: + **Created:** 2026-08-07 15:30 + +Plan files are committed to the repo so other contributors can review historical +decisions. + +**Ordering matters:** plan mode itself restricts writes to the default +`~/.claude/plans/` location — that's a hard tool restriction during plan mode, not a +choice. So the copy into `docs/plans/` cannot happen until *after* `ExitPlanMode`. +When it happens, it must be the very first action taken post-exit, committed on its +own, *before* any implementation work starts. Do not fold the plan-file commit into +the first implementation commit. + +### Plan execution summaries + +After finishing each step of an approved plan, append a short, human-readable summary +to `docs/plans-executions/YYYY-MM-DD-HHMM-<slug>.md` — same timestamp and slug as the +originating plan, so the two files pair up 1:1. Sections go in chronological order +(bottom of the file), not newest-first like `CHANGELOG.md`. + +- **Status checklist.** The top of the file is a markdown checklist with one line per + plan step (`- [ ] Step 0 — <title>`). Check a box in the same commit that adds that + step's entry. This checklist is the authoritative status record: a new session + should be able to read this file alone and know where to pick up. +- **Keep the prose light** — what was done, plus anything non-obvious (a judgment + call, a spec gap, something that did not work as expected). Skip steps that went + exactly as planned. +- **Include exact commands** for anything that is not a plain file edit, as fenced + blocks, each with one line on why it was run that way. Call out deviations from + the plan explicitly; close with a short "worth noting" paragraph. +- Commit the summary update together with that step's implementation commit. + +`docs/plans-executions/2026-10-08-2028-house-conventions.md` is the reference example. + +### Changelog + +Maintain a running changelog in `CHANGELOG.md` at the repo root. After every +significant change, fix, or update — once the user confirms it works — append a new +entry **at the top** of the file in this format: + + ## YYYY-MM-DD HH:MM TZ — short title + + - One-line summary of what changed and why. + - Key files touched (optional, only if useful for traceability). + +Get the timestamp with `date "+%Y-%m-%d %H:%M %Z"`. Never write a literal placeholder +like `HH:MM` — always run `date` first to get the real current time. Skip trivial edits +(typos, formatting, comment tweaks); only log changes a future reader would care about. + +### Reviews + +Before opening an MR for a feature, run the `operator-reviewer` and +`go-operator-reviewer` agents on the branch and save the findings to +`docs/reviews/YYYY-MM-DD-HHMM-<slug>.md`, most severe first, as a `- [ ]` list that is +ticked as items are processed. + +### Branching & Merge Requests + +The remote is Gitea (`gitea.home.hrajfrisbee.cz/kacerr/gatewayapi-certificates-operator`). +For **features**, do not commit to `main` directly. Use a branch + merge request flow: + +1. **Create a branch off `main`** before starting work: `feat/<slug>` for features, + `fix/<slug>` for bug-fix branches the user explicitly asks for; `<slug>` is short + kebab-case. +2. **Commit on the branch** following the commit conventions below. +3. **Push the branch** to `origin` with `-u` so it tracks. +4. **Open the MR with `tea`** rather than printing a compare URL: + + ```bash + tea pr create --title "<short title>" --description "<body>" --base main --head <branch> + ``` + + Print the resulting PR URL. If `tea` is unavailable, fall back to printing the + compare URL + (`gitea.home.hrajfrisbee.cz/kacerr/gatewayapi-certificates-operator/compare/main...<branch>`). +5. **Do not merge or delete the branch** from the CLI — neither via `tea`, `gh`, nor + `git push --delete`. The user does that in Gitea. + +**Exceptions — committing straight to `main` is fine for:** small bug fixes or +hotfixes the user describes as such; typo, comment and formatting tweaks; edits the +user explicitly says to push to `main`. When uncertain whether something is a feature +or a small fix, ask before branching. + +### Git Commits, Pushes and Tags + +- Subjects use a lowercase type prefix, as the existing history does: `feat:`, `fix:`, + `deploy:`, `ci:`, `docs:`, `test:`, `chore:`. +- Always append a `Co-Authored-By:` trailer for Claude. +- Ask for confirmation before running `git push`. The exception is `/ship`, which + commits and pushes without asking. +- Tags are semver `vMAJOR.MINOR.PATCH` and mark releases, not commits. They are + created only through `/release`, which confirms before pushing. The old `0.01`–`0.07` + tags predate this rule and stay as they are. + +### CI/CD + +Gitea Actions, workflows in `.gitea/workflows/`: + +- `build.yaml` builds the image and pushes it to + `gitea.home.hrajfrisbee.cz/kacerr/gatewayapi-certificates-operator` on any tag push + or manual dispatch. Needs the `REGISTRY_TOKEN` repo secret. +- `kubernetes-deploy.yaml` applies `deploy/` to the home-lab cluster through a + Vault → Kanidm → OIDC auth chain. + +### Containers + +- Multi-stage builds; distroless/scratch final images. +- Non-root user, read-only rootfs for production builds. +- Pin base image digests in production Dockerfiles. + +### K8s Manifests + +- Always include resource requests/limits. +- Liveness/readiness probes with sane defaults. +- PodDisruptionBudgets for replicated workloads. + +## Gotchas + +- **Three names for one project.** Directory `gateway-helper-operator`; git remote and + image `gatewayapi-certificates-operator`; binary, manifests, namespace and annotation + prefix `gateway-cert-operator`. +- **`listener-port` annotation is not implemented.** `AnnotationListenerPort` is + declared, never read; every listener is on port 443. +- **Stale listeners.** Deleting a Certificate, removing its annotation or retargeting + it does not clean up the old Gateway. A sibling Certificate's next reconcile fixes + it; with no sibling the listener stays forever. +- **A single bad listener blocks the whole Gateway.** The patch replaces the full + listeners array, so a wildcard DNS name (`*` ends up in the listener name) or a name + collision makes the API server reject every update for that Gateway. +- **`ListenersEqual` is partial**: name, hostname, port and the first secret ref only. + Changing what `BuildListenersFromCertificates` generates in other fields will not + roll out to existing listeners. +- **`auto-` prefix is the only ownership marker.** A user listener named `auto-…` gets + deleted. +- **`--metrics-bind-address` is parsed but ignored**; metrics are still served on + controller-runtime's default `:8080`. Logging is hardcoded to zap dev mode. +- **No leader election**; safe only at `replicas: 1`. +- **Deployment uses the mutable `:latest` tag**, so a new image does not roll out on + its own. +- **Dockerfile base images are tags, not digests** (contradicts the Containers rule; + not yet fixed). +- **`kubernetes-deploy.yaml` runs on every push to any branch** and prints auth + tokens to the job log. Do not copy its patterns; fixing it is on the backlog. diff --git a/docs/README.md b/README.md similarity index 86% rename from docs/README.md rename to README.md index ae9edd4..1545963 100644 --- a/docs/README.md +++ b/README.md @@ -53,12 +53,14 @@ spec: 3. **Patching**: For each DNS name defined in `spec.dnsNames`, it mathematically appends a new HTTPS `Listener` on port `443` terminating TLS using the secret created by `cert-manager` (`my-app-tls`). 4. **Safe Coexistence**: To safely coexist with manually defined listeners, the operator uniquely prefixes its configured listeners with `auto-` (e.g., `auto-my-app-cert-app-example-com`). It solely manages listeners with this prefix. -When a certificate is deleted or updated (for instance if a DNS name is changed), the operator will instantly reconcile the modified states by cleanly removing older `auto-*` listeners and inserting the new ones while preserving the manually defined resources. +When a certificate is updated (for instance if a DNS name is changed), the operator removes the older `auto-*` listeners and inserts the new ones while preserving the manually defined resources. + +Known limitation: when a certificate is deleted, its listeners are removed only on the next reconcile of another certificate targeting the same Gateway. ## Deployment To deploy the operator, applying the provided manifest sets up the necessary RBAC permissions and the controller manager: ```bash -kubectl apply -f deploy/manifests.yaml +kubectl apply -k deploy/ ``` diff --git a/docs/flows.md b/docs/flows.md new file mode 100644 index 0000000..1bbda45 --- /dev/null +++ b/docs/flows.md @@ -0,0 +1,493 @@ +# Runtime Flows + +Deep-dive reference for every control path in the gateway-cert-operator. Complements the user-facing overview in `docs/README.md`. + +--- + +## 1. Overview + +The operator sits between two Kubernetes APIs and keeps them in sync: + +``` + +--------------------+ +------------------------+ + | cert-manager | | gateway-cert-operator | + | Certificate |----(watches)-------->| (this binary) | + | (source of | | | + | truth for TLS) | +----------+-------------+ + +--------------------+ | + | patches Spec.Listeners + v + +--------------------+ +------------------------+ + | Gateway API | | Gateway | + | controller |<----(watches)--------| (target object) | + | (routes traffic) | +------------------------+ + +--------------------+ +``` + +The operator makes no changes to cert-manager objects. Its only write action is a `PATCH` on `Gateway.Spec.Listeners`. All other interactions are reads. + +--- + +## 2. Startup Flow + +Executed once at process start (`main.go`): + +``` +process start + │ + ├─ init() [main.go:22-26] + │ ├─ clientgoscheme.AddToScheme(scheme) # core/v1: Secrets, Events, … + │ ├─ cmv1.AddToScheme(scheme) # cert-manager.io/v1 Certificate + │ └─ gwapiv1.Install(scheme) # gateway.networking.k8s.io/v1 Gateway + │ + └─ main() [main.go:28] + │ + ├─ flag.StringVar --metrics-bind-address default ":8080" [main.go:32] + ├─ flag.StringVar --health-probe-bind-address default ":8081" [main.go:33] + ├─ flag.Parse() [main.go:34] + │ + ├─ ctrl.SetLogger(zap, UseDevMode=true, AddCaller) [main.go:36-41] + │ dev-mode console logger with file:line call site + │ + ├─ ctrl.GetConfigOrDie() # KUBECONFIG or in-cluster config + │ on err ──► os.Exit(1) + │ + ├─ ctrl.NewManager(cfg, Options{Scheme, HealthProbeBindAddress: ":8081"}) + │ on err ──► log + os.Exit(1) [main.go:44-47] + │ NOTE: metricsAddr is parsed above but NOT passed here; + │ the manager falls back to its own default, which + │ is also ":8080" (see §10, item 2) + │ + ├─ controller.SetupCertificateReconciler(mgr) [main.go:53] + │ registers field index + watches (see §3) + │ on err ──► log + os.Exit(1) + │ + ├─ mgr.AddHealthzCheck("healthz", healthz.Ping) → GET /healthz:8081 + ├─ mgr.AddReadyzCheck("readyz", healthz.Ping) → GET /readyz:8081 + │ on err ──► log + os.Exit(1) + │ + └─ mgr.Start(ctrl.SetupSignalHandler()) [main.go:68] + SetupSignalHandler installs SIGINT / SIGTERM cancel + mgr.Start blocks; on shutdown → graceful stop + on err ──► log + os.Exit(1) +``` + +--- + +## 3. Watch Wiring and Event Sources + +`SetupCertificateReconciler` registers two watches before starting the controller +(`certificate_controller.go:59-63`): + +``` + controller-runtime manager + │ + ├─ For(&cmv1.Certificate{}) primary watch + │ every Certificate create/update/delete + │ ──► enqueue Reconcile(cert.Namespace/cert.Name) + │ + └─ Watches(&gwapiv1.Gateway{}, + EnqueueRequestsFromMapFunc(r.gatewayToCertificates)) + secondary watch + every Gateway create/update/delete + ──► gatewayToCertificates maps Gateway → []Certificates + ──► enqueue Reconcile for each matching Certificate +``` + +Full trigger table: + +| ID | Event | Enqueued object | Code path | +|----|-------|-----------------|-----------| +| E1 | Certificate **create** | that Certificate | primary watch | +| E2 | Certificate **update** (any field — no predicate filter) | that Certificate | primary watch | +| E3 | Certificate **delete** | that Certificate | primary watch | +| E4 | Gateway **create / update / delete** | every Certificate targeting it (via field index) | secondary watch + `gatewayToCertificates` | +| E5 | Reconcile returns non-nil error | same Certificate | controller-runtime rate-limited backoff re-queue | +| E6 | Controller-runtime cache resync (~10 h default) | every known Certificate | implicit periodic replay | + +--- + +## 4. Field Index + +The field index is the only mechanism that lets the operator find all Certificates pointing at a given Gateway without scanning the entire namespace. It is registered during setup and queried in two places. + +``` + IndexFieldGatewayTarget = ".metadata.annotations.gatewayTarget" + (identifier used by controller-runtime; no annotation by this name exists) + + Indexer function (certificate_controller.go:44-55): + ┌─────────────────────────────────────────────────────────┐ + │ for each Certificate obj: │ + │ gwName = obj.Annotations["…/gateway-name"] │ + │ if gwName == "" → return nil (not indexed) │ + │ gwNs = obj.Annotations["…/gateway-namespace"] │ + │ if gwNs == "" → gwNs = obj.Namespace (fallback) │ + │ return [ "<gwNs>/<gwName>" ] │ + └─────────────────────────────────────────────────────────┘ + + Key shape: "<targetGatewayNamespace>/<targetGatewayName>" + + Two query sites: + + ┌──────────────────────────────────────────────────────┐ + │ Reconcile [certificate_controller.go:95-99] │ + │ List(Certs, MatchingFields{index: "<gwNs>/<gwName>"})│ + │ returns all sibling Certs targeting the same GW │ + └──────────────────────────────────────────────────────┘ + + ┌──────────────────────────────────────────────────────┐ + │ gatewayToCertificates [certificate_controller.go:138]│ + │ List(Certs, MatchingFields{index: "<gw.Ns>/<gw.Name>"})│ + │ used by the secondary watch to fan-out GW events │ + │ to the relevant Certs │ + └──────────────────────────────────────────────────────┘ +``` + +The namespace-fallback rule (`gwNs = obj.Namespace` when the annotation is absent) is duplicated in two places: the indexer (`:51-53`) and `Reconcile` (`:89-91`). Both must apply the same rule or the index key used during write will differ from the key used during lookup. + +--- + +## 5. Reconcile Decision Tree + +Every enqueued item arrives here. Inputs: one `Reconcile.Request` containing the namespace/name of a Certificate. Side effects: 0 or 1 `PATCH` on a Gateway. + +``` +Reconcile(ctx, req) + │ + ├─(A) r.Get(ctx, req.NamespacedName, &cert) [controller:71] + │ │ + │ ├─ NotFound ──► client.IgnoreNotFound → nil err + │ │ return Result{}, nil ►► STOP — NO ACTION + │ │ + │ │ ┌─ IMPORTANT: the deleted cert's listener is NOT removed here. + │ │ │ The controller has no record of which Gateway was targeted. + │ │ │ Cleanup happens on the next E2/E4/E6 event for any sibling + │ │ │ cert that still references the same Gateway — at that point + │ │ │ BuildListenersFromCertificates omits the deleted cert and + │ │ │ MergeListeners replaces all auto-* listeners. Until that + │ │ │ event fires, the stale listener remains in the Gateway. + │ │ │ (See §10, item 3 for details.) + │ │ └─────────────────────────────────────────────────────────── + │ │ + │ └─ other error ──► return Result{}, err ►► REQUEUE (E5) + │ + ├─(B) gwName, ok := cert.Annotations["…/gateway-name"] [controller:82] + │ │ + │ └─ !ok ──► log V(1) "missing gateway annotation" + │ return Result{}, nil ►► STOP — NO ACTION + │ + ├─(C) gwNamespace := cert.Annotations["…/gateway-namespace"] [controller:88] + │ └─ if "" ──► gwNamespace = cert.Namespace (DEFAULT FALLBACK) + │ + ├─(D) r.List(ctx, &certList, [controller:95] + │ MatchingFields{IndexFieldGatewayTarget: "<gwNs>/<gwName>"}) + │ │ + │ └─ err ──► return Result{}, err ►► REQUEUE + │ + ├─(E) desiredListeners := + │ gw.BuildListenersFromCertificates(certList.Items, "auto-") + │ (see §6a) [controller:102] + │ + ├─(F) r.Get(ctx, {gwNs, gwName}, &gateway) [controller:106] + │ │ + │ └─ err (including NotFound) ──► return Result{}, err ►► REQUEUE + │ + │ NOTE: Gateway-not-found is NOT specially handled — the + │ reconciler will retry with backoff indefinitely. A typo'd + │ gateway-name annotation produces a perpetual retry loop. + │ (See §10, item 5.) + │ + ├─(G) updated := gw.MergeListeners( [controller:111] + │ gateway.Spec.Listeners, + │ desiredListeners, "auto-") + │ (see §6b) + │ + ├─(H) gw.ListenersEqual(gateway.Spec.Listeners, updated) [controller:113] + │ │ + │ └─ true ──► log V(1) "already up to date" + │ return Result{}, nil ►► SHORT-CIRCUIT — NO PATCH + │ + ├─(I) patch := client.MergeFrom(gateway.DeepCopy()) [controller:118] + │ gateway.Spec.Listeners = updated + │ r.Patch(ctx, &gateway, patch) [controller:120] + │ │ + │ └─ err ──► return Result{}, err ►► REQUEUE + │ + └─(J) log Info "patched gateway listeners" [controller:124] + return Result{}, nil ►► DONE + +Cluster I/O per reconcile: + Reads: 1 Certificate Get (step A) + 1 Certificate List via index (step D) + 1 Gateway Get (step F) + Writes: 0 or 1 Gateway Patch (step I, only when listeners changed) +``` + +--- + +## 6. Build / Merge / Equal Pipeline + +The three pure functions in `internal/gateway/patch.go` do all computation. `Reconcile` only calls them; it holds no business logic of its own. + +``` + certList.Items + │ + v + BuildListenersFromCertificates(certs, "auto-") [patch.go:22] + │ + └──► desired []gwapiv1.Listener + │ + │ gateway.Spec.Listeners (current state from API server) + │ │ + v v + MergeListeners(existing, desired, "auto-") [patch.go:76] + │ + └──► updated []gwapiv1.Listener + │ + v + ListenersEqual(gateway.Spec.Listeners, updated) [patch.go:93] + │ + ├─ true ──► short-circuit, no patch + └─ false ──► Patch(gateway) +``` + +### 6a. BuildListenersFromCertificates + +``` +for cert in certs: + │ + ├─ if cert.DeletionTimestamp != nil ──► skip [patch.go:26] + │ (cert is being deleted; omitting it removes its listeners + │ from the desired set and MergeListeners will drop them) + │ + ├─ if cert.Spec.SecretName == "" ──► skip [patch.go:30] + │ + └─ for dns in cert.Spec.DNSNames: [patch.go:34] + │ + └─ emit Listener{ + Name: "auto-<certName>-<sanitized-dns>" + Hostname: dns + Port: 443 (hardcoded, see §8) + Protocol: HTTPS + AllowedRoutes: { From: All } + TLS: { + Mode: Terminate + CertificateRefs: [{ + Group: "" (core) + Kind: "Secret" + Name: cert.Spec.SecretName + Namespace: cert.Namespace (same as cert) + }] + } + } + +sort by Name ascending [patch.go:67] +(deterministic order prevents spurious diffs on every reconcile) +``` + +### 6b. MergeListeners + +``` +existing listeners + │ + ├─ Name starts with "auto-" ──► DROP (managed, will be replaced) + └─ Name does NOT start with "auto-" ──► KEEP (user-managed) + +merged = [kept user listeners] + [all desired auto-* listeners] +``` + +The result: user-created listeners appear first in their original order; operator-managed listeners follow, sorted by name. + +**Edge case:** a user-created listener whose name happens to start with `auto-` will be treated as managed and will be deleted on the next reconcile. + +### 6c. ListenersEqual + +`ListenersEqual` is a **partial comparison** — only the fields the operator actively manages are checked: + +| Compared | Ignored | +|----------|---------| +| listener `Name` | `Protocol` | +| `Hostname` | `AllowedRoutes` | +| `Port` | `TLS.Options` | +| first `CertificateRef` (`<ns>/<name>`) | cert refs beyond index 0 | +| | `Group`, `Kind` of ref | + +Drift in an ignored field will not trigger a corrective patch. For example: if someone manually flips the Protocol of an auto-managed listener from HTTPS to HTTP, the operator will consider it "equal" and leave it alone. + +The comparison is done via a name-keyed map (`listenerMap`, `patch.go:118`), so listener order does not affect the result. + +--- + +## 7. Listener Naming and Sanitization + +`listenerNameFor` (`patch.go:143`): + +``` +input: prefix="auto-" certName="my-app-cert" dnsName="app.example.com" + +step 1: replace every "." with "-" + "app.example.com" ──► "app-example-com" + +step 2: concatenate + "auto-" + "my-app-cert" + "-" + "app-example-com" + ──► "auto-my-app-cert-app-example-com" + +step 3: if len > 253 bytes: name = name[:253] (hard byte slice) +``` + +Example outputs for a cert with two DNS names: + +| `dnsName` | listener `Name` | +|-----------|-----------------| +| `app.example.com` | `auto-my-app-cert-app-example-com` | +| `*.example.com` | `auto-my-app-cert-*-example-com` | + +The second entry is a risk: `*` is not valid in a Gateway API `SectionName` (which must match RFC 1123). The API server will reject the patch and the reconciler will requeue forever (see §10, item 7). + +The sanitization is also lossy: `a.b.com` and `a-b-com` produce the same result. If a certificate lists both, two listeners with the same name are appended; the API server rejects the patch (listener names must be unique), so no listener on that Gateway is updated. + +--- + +## 8. Defaults Baked In + +All of these are hardcoded in `internal/gateway/patch.go`. None are configurable via annotations (the `listener-port` annotation exists as a constant but is not read — see §10, item 1). + +| Property | Value | Source | +|----------|-------|--------| +| Listener port | `443` | `defaultPort` `patch.go:13` | +| TLS mode | `TLSModeTerminate` | `tlsModeTerminate` `patch.go:14` | +| Allowed routes | `NamespacesFromAll` | `namespacesFromAll` `patch.go:15` | +| Protocol | `HTTPSProtocolType` | inline `patch.go:45` | +| CertificateRef Group | `""` (core API group) | `patch.go:55` | +| CertificateRef Kind | `"Secret"` | `patch.go:56` | +| Secret namespace | cert's own namespace | `patch.go:39,58` | +| Listener name prefix | `"auto-"` | `ListenerPrefix` `controller:28` | +| SectionName max length | 253 bytes | `patch.go:147` | +| Gateway namespace fallback | cert's own namespace | `controller:51-53` and `controller:89-91` | + +--- + +## 9. End-to-End Scenarios + +### Scenario 1 — First Certificate created, Gateway has no auto-listeners + +``` +1. User creates Certificate "my-cert" (namespace: default) + Annotations: gateway-name=my-gateway + Spec: dnsNames=[app.example.com], secretName=my-cert-tls + +2. (E1) Cert create event fires → Reconcile(default/my-cert) + +3. (A) Get cert → found + (B) gateway-name="my-gateway" → ok + (C) gateway-namespace missing → fallback to "default" + (D) List certs by index "default/my-gateway" → [my-cert] + (E) BuildListeners → [Listener{Name:"auto-my-cert-app-example-com", Port:443, …}] + (F) Get gateway "default/my-gateway" → found, Listeners=[] + (G) MergeListeners([], desired) → [auto-my-cert-app-example-com] + (H) ListenersEqual([], updated) → false (len differs) + (I) Patch gateway → Spec.Listeners=[auto-my-cert-app-example-com] + (J) Done. Log: "patched gateway listeners" + +Cluster writes: 1 Gateway PATCH +``` + +### Scenario 2 — Second Certificate added for the same Gateway + +``` +1. User creates Certificate "other-cert" targeting the same Gateway + dnsNames=[api.example.com], secretName=other-cert-tls + +2. (E1) Reconcile(default/other-cert) + +3. (D) List certs by index "default/my-gateway" → [my-cert, other-cert] + (E) BuildListeners → [ + auto-my-cert-app-example-com, + auto-other-cert-api-example-com + ] (sorted alphabetically by name) + (F) Gateway still has [auto-my-cert-app-example-com] + (G) MergeListeners → [auto-my-cert-app-example-com, + auto-other-cert-api-example-com] + (H) ListenersEqual(old 1-item, new 2-item) → false + (I) Patch → Gateway now has both listeners + +Cluster writes: 1 Gateway PATCH +``` + +### Scenario 3 — Certificate dnsNames updated + +``` +1. User edits "my-cert": dnsNames=[app.example.com] → [app.example.com, www.example.com] + +2. (E2) Cert update event → Reconcile(default/my-cert) + +3. (E) BuildListeners produces TWO listeners for my-cert now: + auto-my-cert-app-example-com + auto-my-cert-www-example-com + (plus any from other-cert if still present) + (H) ListenersEqual: len changed → false + (I) Patch gateway with new full set + +Cluster writes: 1 Gateway PATCH +``` + +### Scenario 4 — Certificate deleted + +``` +1. User deletes "my-cert" + +2. (E3) Cert delete event → Reconcile(default/my-cert) + +3. (A) Get cert → NotFound + return Result{}, nil ──► STOP, NO ACTION + +4. Gateway still contains "auto-my-cert-app-example-com" — stale. + +5. Cleanup happens only when the next relevant event fires: + - If "other-cert" is updated or created → (E2) Reconcile(other-cert) + (D) List by index returns [other-cert] only (my-cert is gone) + (E) BuildListeners → only other-cert's listeners + (G) MergeListeners drops the stale auto-my-cert-* listener + (I) Patch removes it + - OR if the Gateway is manually edited → (E4) all targeting Certs + re-enqueued, same convergence path + - OR on the next cache resync (E6, ~10 h) + + Every path above needs at least one surviving Certificate that targets + the Gateway. If my-cert was the last one, nothing is ever enqueued for + it and the stale listener stays until someone removes it by hand. + +Cluster writes: 0 at deletion time; 1 Gateway PATCH at next sibling event +``` + +--- + +## 10. Known Limitations + +Recorded as-found. None are fixed by this document. + +**1. `AnnotationListenerPort` is dead code** +`certificate_controller.go:25` declares the constant; `BuildListenersFromCertificates` (`patch.go:44`) always uses `defaultPort = 443`. Setting `gateway-cert-operator.io/listener-port` on a Certificate has no effect. + +**2. `--metrics-bind-address` flag is parsed but not wired** +`main.go:32` parses the flag; `main.go:44-47` never passes `MetricsBindAddress` to `ctrl.Options`. controller-runtime defaults an empty bind address to `:8080`, so metrics are served and the Service in `deploy/service_gateway-cert-operator-metrics.yaml` works; only changing the flag has no effect. + +**3. No finalizer → cert deletion leaves stale listeners** +When a Certificate is deleted, Reconcile returns early at branch (A) with no cluster write. The stale `auto-*` listener persists in the Gateway until a sibling event triggers re-convergence (see Scenario 4 above), or permanently if no other Certificate targets that Gateway. Adding a finalizer to the Certificate would close this gap by allowing cleanup before the object is removed from the API server. + +**4. `ListenersEqual` is a partial comparison** +Described in §6c. Drift in `Protocol`, `AllowedRoutes`, `TLS.Options`, or any `CertificateRef` beyond the first will not be detected or corrected. + +**5. Gateway-not-found retries forever** +Branch (F) of Reconcile treats all Gateway `Get` errors the same (including `NotFound`) and returns them as re-queue-able errors. A Certificate with a typo'd `gateway-name` annotation will retry with rate-limited backoff indefinitely, generating log noise and API load. There is no terminal failure path. + +**6. No leader election** +`ctrl.NewManager` at `main.go:44` does not set `LeaderElection: true`. The `deploy/deployment_gateway-cert-operator.yaml` runs `replicas: 1` which makes this safe today. Scaling to multiple replicas without enabling leader election would result in concurrent Gateway patches. + +**7. `listenerNameFor` edge cases** +- **Wildcard hostnames**: `*.example.com` produces `SectionName` `auto-…-*-example-com`. The `*` character is not valid in an RFC 1123 label; the Gateway API server will reject the Patch, causing an error-retry loop. +- **Collisions after sanitization**: `a.b.com` and `a-b-com` produce identical sanitized names. The same happens across certificates (cert `a` with `b-c.com` and cert `a-b` with `c.com`) and across namespaces, since the name omits the cert namespace. Both listeners are appended and the API server rejects the patch for the whole Gateway. +- **Truncation at 253 bytes**: `patch.go:147` slices on bytes, not runes (safe for ASCII DNS names). Two DNS names sharing the first 253 characters after sanitization will map to the same listener name. + +**Dead symbols in `patch.go`** +`gatewayGroup` and `gatewayKind` are declared at `patch.go:16-17` but never referenced. They can be removed without effect. diff --git a/docs/plans-executions/2026-10-08-2028-house-conventions.md b/docs/plans-executions/2026-10-08-2028-house-conventions.md index 39c2f7c..d07411f 100644 --- a/docs/plans-executions/2026-10-08-2028-house-conventions.md +++ b/docs/plans-executions/2026-10-08-2028-house-conventions.md @@ -3,10 +3,32 @@ Plan: `docs/plans/2026-10-08-2028-house-conventions.md` - [x] Step 0 — Save and commit the plan -- [ ] Step 1 — CLAUDE.md +- [x] Step 1 — CLAUDE.md - [ ] Step 2 — `.claude/` and docs scaffolding - [ ] Step 3 — Makefile, lint, formatting - [ ] Step 4 — Module path and version identity - [ ] Step 5 — Baseline tests - [ ] Step 6 — CI build workflow - [ ] Step 7 — Close out + +## Step 1 — CLAUDE.md + +Rewrote `CLAUDE.md` in the five-section shape. The Go, Testing, Changelog and +Branching text comes from the `go-project-bootstrap` skill references; the plan +ordering and execution-summary rules come from `egress-proxies-operator/CLAUDE.md`, +condensed from prose into bullets. + +Deviations from the plan: + +- The house container rules about Ubuntu dev images and separate dev/prod + Dockerfiles were left out: this repo has one production Dockerfile and no dev + image, and listing rules nobody follows only adds noise. +- The `listener-port` row was removed from the annotation table and moved to + Gotchas, so the table lists only what works. +- Added a short Reviews subsection so the reviewer agents and `docs/reviews/` have a + documented trigger. +- `README.md` also claimed that deleting a certificate removes its listeners + "instantly". Reworded and added the known limitation. + +Worth noting: Commands and CI/CD describe the repo as it is at this commit; Steps 3, +4 and 6 update them. `docs/flows.md` cites `main.go` line numbers that Step 4 shifts. -- 2.54.0 From 2be69b9aace7a259327dd5312317fccd90c4e900 Mon Sep 17 00:00:00 2001 From: Jan Novak <kacerr.cz@gmail.com> Date: Thu, 8 Oct 2026 20:31:26 +0200 Subject: [PATCH 03/11] chore: add Claude settings, ship/release commands, reviewer agents, changelog - /ship now commits and pushes with a branch guard and no longer tags; /release tags vMAJOR.MINOR.PATCH and confirms before pushing - operator-reviewer and go-operator-reviewer agents from egress-proxies-operator - committed permission allowlist; settings.local.json ignored Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --- .claude/agents/go-operator-reviewer.md | 115 ++++++++++++++++++ .claude/agents/operator-reviewer.md | 15 +++ .claude/commands/release.md | 18 +++ .claude/commands/ship.md | 23 +++- .claude/settings.json | 41 +++++++ .gitignore | 5 +- CHANGELOG.md | 1 + .../2026-10-08-2028-house-conventions.md | 21 +++- docs/reviews/.gitkeep | 0 9 files changed, 236 insertions(+), 3 deletions(-) create mode 100644 .claude/agents/go-operator-reviewer.md create mode 100644 .claude/agents/operator-reviewer.md create mode 100644 .claude/commands/release.md create mode 100644 .claude/settings.json create mode 100644 CHANGELOG.md create mode 100644 docs/reviews/.gitkeep diff --git a/.claude/agents/go-operator-reviewer.md b/.claude/agents/go-operator-reviewer.md new file mode 100644 index 0000000..0d3ba04 --- /dev/null +++ b/.claude/agents/go-operator-reviewer.md @@ -0,0 +1,115 @@ +--- +name: go-operator-reviewer +description: Critically reviews Go controller-runtime code for bugs, correctness, and over-abstraction. Invoke after a milestone or before merging a branch. +tools: Read, Grep, Glob, Bash +model: opus +--- + +You are a senior Go reviewer auditing a Kubernetes operator built on controller-runtime. +Your job: find real problems and call out over-engineering. You are NOT here to +nitpick anything `gofmt`, `goimports`, or `golangci-lint` already enforce — assume +those run in CI. Be terse. Every flagged item costs the reader attention; spend it +only on things that change a decision. + +Adopt a skeptical, "delete code" mindset. Default assumption: an abstraction is +guilty until it proves it earns its keep. + +## Method (do this in order) + +1. Run the toolchain first and read its output — do not redo by hand what tools + already report: + - `golangci-lint run ./... 2>&1 | head -100` (if no config, run with + `--enable=staticcheck,gocritic,revive,unparam,unconvert,ineffassign,prealloc,gocyclo,gocognit`) + - `go vet ./...` + - `deadcode ./...` (golang.org/x/tools/cmd/deadcode) — unreachable funcs + - `gocyclo -over 15 .` and `gocognit -over 20 .` — complexity hotspots + Summarize only what's actionable; don't paste raw tool dumps. + +2. Build an interface inventory before reading logic: + `grep -rn 'type .* interface' --include='*.go'`, then for each interface count + its implementations. This is your single highest-signal artifact for an operator + that talks to the API server through client-go/controller-runtime interfaces. + +3. Map controller wiring: where do `Reconcile(ctx, req)` methods and + `SetupWithManager` live, and does business logic leak into `Reconcile` itself + instead of a testable package? + +4. Only then read the logic-heavy packages in full. + +## What to flag — over-abstraction (testable rules) + +- **Single-implementation interface.** Any interface with exactly 1 impl → name the + interface, name the impl, recommend collapsing to the concrete type. Exception: + a genuine test seam that's actually mocked (e.g. a narrowed `client.Client` seam), + or a documented second impl in flight. "Might need it later" is not an exception. +- **Constructor returns an interface.** `New*()` returning an interface instead of + `*Concrete` → flag. Rule: accept interfaces, return structs. +- **Interface defined next to its producer**, not at the consumer. Recommend moving + it to where it's consumed (or deleting per the rule above). +- **Microservice-template layout on an operator** (domain/usecase/repository/handler + ceremony, deep `internal/` nesting that wraps one type each, when a flat + `controllers/` + `api/` + `internal/` layout would do) → recommend flattening. +- **Wrapper types whose methods only forward** (Manager/Service/Provider that adds + no behavior over `client.Client`) → recommend inlining. +- **Premature generics** (one concrete instantiation) and **DI frameworks** + (wire/fx) where hand-wiring in `main` is clearer. +- **Logic inside `Reconcile`** → should fetch the object, delegate to a testable + package for resolve/compute-desired-state/diff, and only handle status/requeue + wiring itself; business logic belongs outside the handler. + +## What to flag — correctness & bugs + +- Error handling model inconsistency: mixed sentinel / typed / `fmt.Errorf` string + errors; missing `%w` wrapping where callers need `errors.Is/As`. +- `context.Context` not propagated to I/O (API server calls), or `context.TODO()` + left in real reconcile paths. +- **Reconcile-loop specifics:** + - Missing `client.IgnoreNotFound` on the initial `Get` (spurious errors/requeues + on delete). + - No finalizer handling where the controller owns external (non-cluster) + resources that need cleanup on delete. + - Missing or wrong owner references, causing orphaned child objects or GC loops. + - Status updates that don't use `Status().Update()`/`Status().Patch()` (silently + clobbering spec, or clobbering status subresource semantics). + - Reconcile not idempotent: side effects that aren't safe to repeat, or that + aren't guarded against being re-run on every requeue. + - Uncontrolled requeue: `RequeueAfter` values that busy-loop, or errors returned + as `(ctrl.Result{}, err)` when a controlled backoff would be correct instead. + - Full-object `Update()` where a `Patch` would avoid clobbering concurrent + writers, or vice versa — check which is actually intended. + - List calls without label selectors/field indexers where the object count could + grow unbounded (scales badly, watches too much). +- Goroutine leaks, unbuffered-channel deadlock risk, `WaitGroup`/`errgroup` misuse, + data races on package-level vars (esp. shared informer caches). +- Unchecked errors on `Close()`/`Flush()` where the result matters. +- Resource leaks (files, HTTP bodies, contexts not cancelled). +- Global mutable state that makes the tool untestable or unsafe under `-race`. + +## Scope + +New and modified code must comply. Don't rewrite untouched existing code unless it +directly blocks a fix. If broad cleanup is warranted, say so as a separate timeboxed +task, don't fold it into this review. + +## Output format + +**Verdict:** one line — Approve / Approve with comments / Request changes. + +**Critical (bugs/correctness):** numbered, `file:line`, the problem in one sentence, +then the concrete fix as a minimal Go snippet. Skip if none. + +**Simplification:** the over-abstraction findings, ranked by impact × (inverse) effort +so the cheap high-value deletes come first. `file:line`, what to collapse, what it +becomes. + +**Nice-to-have:** anything minor worth noting, max 3 items. Optional. + +Cap the whole thing at the top 5–7 issues across all sections. Rank ruthlessly. + +## Hard rules + +- Never edit or write files. If you find a fix, describe it; do not implement it. +- Cite `file:line` for every flagged issue. No line ref = don't flag it. +- Never speculate about code you didn't read. If you skipped files, say which. +- Don't flag anything gofmt/golangci-lint already catches. +- Total output under 1200 words. Concision is part of the value. diff --git a/.claude/agents/operator-reviewer.md b/.claude/agents/operator-reviewer.md new file mode 100644 index 0000000..326097a --- /dev/null +++ b/.claude/agents/operator-reviewer.md @@ -0,0 +1,15 @@ +--- +name: operator-reviewer +description: Reviews Kubernetes operator PRs for controller-runtime correctness, reconcile semantics, and API design +tools: Read, Grep, Glob, Bash +--- +You are a senior reviewer specializing in Kubernetes operators. +Review with focus on: +- Reconcile idempotency and requeue behavior; no state assumptions between reconciles +- Informer cache reads vs direct API reads; stale-cache races +- Finalizer handling, deletion flow, orphaned resources +- CRD schema evolution, conversion webhooks, status subresource / conditions conventions +- RBAC minimality vs what the controller actually touches +- Leader election, watch predicates, event filtering for churn reduction +- Go: context propagation, error wrapping, client.Object handling +Output: findings ranked by severity, with file:line refs. No praise padding. \ No newline at end of file diff --git a/.claude/commands/release.md b/.claude/commands/release.md new file mode 100644 index 0000000..caadc6f --- /dev/null +++ b/.claude/commands/release.md @@ -0,0 +1,18 @@ +--- +description: Tag a release and push it after confirmation +--- + +Commit all staged and unstaged changes into git, assign the next release tag, then +ask for confirmation before pushing. + +Before committing, check the current branch (`git branch --show-current`). Releases +are cut from `main`; on any other branch, stop and ask. + +Tags are semver `vMAJOR.MINOR.PATCH` (see CLAUDE.md, "Git Commits, Pushes and Tags"). +Find the latest with `git tag --list 'v*' --sort=-v:refname | head -1`; if there is +none, start at `v0.1.0`. The old `0.01`–`0.07` tags are not part of the sequence. +Default to a patch bump for fixes and a minor bump for features; state the tag you +picked and why when asking for confirmation. + +Pushing the tag triggers `.gitea/workflows/build.yaml`, which builds and pushes the +image and moves `:latest`. diff --git a/.claude/commands/ship.md b/.claude/commands/ship.md index acfdbc6..f67a8e1 100644 --- a/.claude/commands/ship.md +++ b/.claude/commands/ship.md @@ -1 +1,22 @@ -Commit all staged and unstaged changes into git, assign the next tag in sequence, then ask for confirmation before pushing. \ No newline at end of file +--- +description: Commit current changes and push to the remote +--- + +Commit the current working-tree changes and push them. + +1. Run `git status` and `git diff` to see what will be committed. +2. Check the current branch (`git branch --show-current`). If it's `main` and the + change looks feature-sized (not one of CLAUDE.md's "commit straight to main" + exceptions: small fixes, typos/formatting, or the user explicitly said `main`), + stop and tell the user to branch first — don't push a feature to `main`. +3. Stage the changed files (`git add -A` is fine here). +4. Create a commit with a short message describing the changes (infer it from the + diff), including a `Co-Authored-By:` trailer for Claude. + Pass the message via a HEREDOC. +5. Push with `git push`. If the current branch has no upstream, use `git push -u origin HEAD`. +6. If the branch isn't `main`, remind the user to open an MR with `tea pr create` + (see CLAUDE.md's Branching & Merge Requests section) rather than opening one + automatically. +7. Report the commit hash and push result. + +If there are no changes, stop and say so — do not create an empty commit. diff --git a/.claude/settings.json b/.claude/settings.json new file mode 100644 index 0000000..6a0c283 --- /dev/null +++ b/.claude/settings.json @@ -0,0 +1,41 @@ +{ + "permissions": { + "allow": [ + "Bash(git status)", + "Bash(git status:*)", + "Bash(git diff:*)", + "Bash(git log:*)", + "Bash(git branch:*)", + "Bash(git add:*)", + "Bash(git checkout:*)", + "Bash(git pull:*)", + "Bash(git fetch:*)", + "Bash(date:*)", + "Read(//tmp/**)", + "Read(//private/tmp/**)", + "Bash(go build:*)", + "Bash(go test:*)", + "Bash(go vet:*)", + "Bash(go mod:*)", + "Bash(go run:*)", + "Bash(go version:*)", + "Bash(go get:*)", + "Bash(golangci-lint run:*)", + "Bash(golangci-lint --version)", + "Bash(gofumpt:*)", + "Bash(tea pr create:*)", + "Bash(make help *)", + "Bash(make build *)", + "Bash(make test *)", + "Bash(make test-short *)", + "Bash(make vet *)", + "Bash(make fmt *)", + "Bash(make lint *)", + "Bash(make tidy *)", + "Bash(make check *)", + "Bash(make docker-build *)", + "Bash(docker build:*)", + "Bash(kubectl kustomize:*)" + ] + } +} diff --git a/.gitignore b/.gitignore index 907f0fc..634a238 100644 --- a/.gitignore +++ b/.gitignore @@ -10,4 +10,7 @@ *.out # OS -.DS_Store \ No newline at end of file +.DS_Store + +# Claude Code +.claude/settings.local.json diff --git a/CHANGELOG.md b/CHANGELOG.md new file mode 100644 index 0000000..825c32f --- /dev/null +++ b/CHANGELOG.md @@ -0,0 +1 @@ +# Changelog diff --git a/docs/plans-executions/2026-10-08-2028-house-conventions.md b/docs/plans-executions/2026-10-08-2028-house-conventions.md index d07411f..d2344fb 100644 --- a/docs/plans-executions/2026-10-08-2028-house-conventions.md +++ b/docs/plans-executions/2026-10-08-2028-house-conventions.md @@ -4,7 +4,7 @@ Plan: `docs/plans/2026-10-08-2028-house-conventions.md` - [x] Step 0 — Save and commit the plan - [x] Step 1 — CLAUDE.md -- [ ] Step 2 — `.claude/` and docs scaffolding +- [x] Step 2 — `.claude/` and docs scaffolding - [ ] Step 3 — Makefile, lint, formatting - [ ] Step 4 — Module path and version identity - [ ] Step 5 — Baseline tests @@ -32,3 +32,22 @@ Deviations from the plan: Worth noting: Commands and CI/CD describe the repo as it is at this commit; Steps 3, 4 and 6 update them. `docs/flows.md` cites `main.go` line numbers that Step 4 shifts. + +## Step 2 — `.claude/` and docs scaffolding + +Copied the two reviewer agents and `/ship` from `egress-proxies-operator/.claude/`. +`/release` was rewritten for the semver rule instead of copied, because the egress +text says "no tagging scheme is documented" and this repo now has one. + +Deviations from the plan: + +- `/ship` names "a `Co-Authored-By:` trailer for Claude" instead of the fixed + `Claude <noreply@anthropic.com>` string, matching CLAUDE.md. +- `.gitignore` already covered `/bin/` and `*.out`, so only + `.claude/settings.local.json` was added. +- `settings.json` also allows `kubectl kustomize`, which the Commands section uses. + +Worth noting: `.claude/settings.json` leaves out `git commit`, `git push` and +`git tag` on purpose, but `~/.claude/settings.json` allows `git commit` and +`git push` globally, so on this machine the push confirmation rests on CLAUDE.md, +not on the permission system. diff --git a/docs/reviews/.gitkeep b/docs/reviews/.gitkeep new file mode 100644 index 0000000..e69de29 -- 2.54.0 From 0eb588e8bd3a9c37f392326b23414e521b35c6ff Mon Sep 17 00:00:00 2001 From: Jan Novak <kacerr.cz@gmail.com> Date: Thu, 8 Oct 2026 20:32:53 +0200 Subject: [PATCH 04/11] chore: add Makefile with check gate, golangci-lint config, dockerignore - make check runs lint + vet + test-short; lint is pinned to the go.mod toolchain so it works when the host Go is newer than golangci-lint's - gofumpt main.go and patch.go; drop unused gatewayGroup/gatewayKind vars Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --- .dockerignore | 11 ++++ .golangci.yml | 1 + CLAUDE.md | 22 ++++++-- Makefile | 56 +++++++++++++++++++ .../2026-10-08-2028-house-conventions.md | 35 +++++++++++- internal/gateway/patch.go | 8 +-- main.go | 2 +- 7 files changed, 122 insertions(+), 13 deletions(-) create mode 100644 .dockerignore create mode 100644 .golangci.yml create mode 100644 Makefile diff --git a/.dockerignore b/.dockerignore new file mode 100644 index 0000000..9af8280 --- /dev/null +++ b/.dockerignore @@ -0,0 +1,11 @@ +# More info: https://docs.docker.com/engine/reference/builder/#dockerignore-file +# Ignore everything by default and re-include only needed files +** + +# Re-include Go source files (but not *_test.go) +!**/*.go +**/*_test.go + +# Re-include Go module files +!go.mod +!go.sum diff --git a/.golangci.yml b/.golangci.yml new file mode 100644 index 0000000..26b4afd --- /dev/null +++ b/.golangci.yml @@ -0,0 +1 @@ +version: "2" diff --git a/CLAUDE.md b/CLAUDE.md index 3eb53d0..27dfc0c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -13,13 +13,14 @@ Certificate's secret. ## Commands ```bash -go build ./... # build -go vet ./... # vet -go test ./... # all tests +make help # list targets +make check # lint + vet + test-short (the pre-commit gate) +make build # bin/manager +make test # full suite, -race -cover +make fmt # gofumpt -w . +make run # run against the current kubeconfig context +make docker-build IMG=gateway-cert-operator:dev go test -race -v -run TestName ./path/to/package # single test -golangci-lint run # lint -go run . # run against the current kubeconfig context -docker build -t gateway-cert-operator . kubectl kustomize deploy/ # render the manifests ``` @@ -150,6 +151,12 @@ Get the timestamp with `date "+%Y-%m-%d %H:%M %Z"`. Never write a literal placeh like `HH:MM` — always run `date` first to get the real current time. Skip trivial edits (typos, formatting, comment tweaks); only log changes a future reader would care about. +### Quality gate + +`make check` is the one-shot pre-commit gate — it runs `golangci-lint`, `go vet` and +`go test -short`, in that order. Do not declare a task complete if `make check` +reports any failures. + ### Reviews Before opening an MR for a feature, run the `operator-reviewer` and @@ -237,6 +244,9 @@ Gitea Actions, workflows in `.gitea/workflows/`: deleted. - **`--metrics-bind-address` is parsed but ignored**; metrics are still served on controller-runtime's default `:8080`. Logging is hardcoded to zap dev mode. +- **`make lint` pins `GOTOOLCHAIN` to the go.mod version.** golangci-lint cannot + type-check a standard library newer than the Go it was built with, so running + `golangci-lint run` directly fails when the host Go is ahead of it. - **No leader election**; safe only at `replicas: 1`. - **Deployment uses the mutable `:latest` tag**, so a new image does not roll out on its own. diff --git a/Makefile b/Makefile new file mode 100644 index 0000000..c13e53c --- /dev/null +++ b/Makefile @@ -0,0 +1,56 @@ +BINARY := manager +BIN_DIR := bin +IMG ?= gateway-cert-operator:dev + +# GIT_COMMIT is baked into the image (-ldflags in the Dockerfile); -dirty +# covers staged and untracked changes too, which `git diff --quiet` misses. +GIT_COMMIT ?= $(shell git rev-parse --short=12 HEAD 2>/dev/null || echo unknown)$(shell test -z "$$(git status --porcelain 2>/dev/null)" || echo -dirty) + +# golangci-lint type-checks the standard library of whatever toolchain is active, +# and fails on one newer than the Go it was built with. Linting with the go.mod +# version keeps it working when the host Go is ahead, and matches CI. +GO_MOD_VERSION := $(shell awk '$$1 == "go" {print $$2}' go.mod) + +.PHONY: all build run test test-short lint vet fmt tidy check docker-build clean help + +all: check build + +build: ## Build the manager binary to bin/manager + mkdir -p $(BIN_DIR) + go build -o $(BIN_DIR)/$(BINARY) . + +run: ## Run the manager against the current kubeconfig context + go run . + +test: ## Run the full test suite with the race detector + go test -race -cover ./... + +test-short: ## Run unit tests only — skips tests guarded by testing.Short() + go test -short -cover ./... + +lint: ## Run golangci-lint + GOTOOLCHAIN=go$(GO_MOD_VERSION) golangci-lint run ./... + +vet: ## Run go vet + go vet ./... + +fmt: ## Format all Go files with gofumpt + gofumpt -w . + +tidy: ## Tidy go.mod and go.sum + go mod tidy + +check: lint vet test-short ## Run all quality checks (lint + vet + test-short) + +docker-build: ## Build the container image (IMG=name:tag) + docker build --build-arg GIT_COMMIT=$(GIT_COMMIT) -t $(IMG) . + +clean: ## Remove build artifacts + rm -rf $(BIN_DIR) + +help: ## Show available targets + @echo 'Usage: make <target>' + @echo '' + @echo 'Targets:' + @grep -E '^[a-zA-Z_-]+:.*##' $(MAKEFILE_LIST) | \ + awk 'BEGIN {FS = ":.*## "}; {printf " \033[36m%-14s\033[0m %s\n", $$1, $$2}' diff --git a/docs/plans-executions/2026-10-08-2028-house-conventions.md b/docs/plans-executions/2026-10-08-2028-house-conventions.md index d2344fb..189bf39 100644 --- a/docs/plans-executions/2026-10-08-2028-house-conventions.md +++ b/docs/plans-executions/2026-10-08-2028-house-conventions.md @@ -5,7 +5,7 @@ Plan: `docs/plans/2026-10-08-2028-house-conventions.md` - [x] Step 0 — Save and commit the plan - [x] Step 1 — CLAUDE.md - [x] Step 2 — `.claude/` and docs scaffolding -- [ ] Step 3 — Makefile, lint, formatting +- [x] Step 3 — Makefile, lint, formatting - [ ] Step 4 — Module path and version identity - [ ] Step 5 — Baseline tests - [ ] Step 6 — CI build workflow @@ -51,3 +51,36 @@ Worth noting: `.claude/settings.json` leaves out `git commit`, `git push` and `git tag` on purpose, but `~/.claude/settings.json` allows `git commit` and `git push` globally, so on this machine the push confirmation rests on CLAUDE.md, not on the permission system. + +## Step 3 — Makefile, lint, formatting + +Added `Makefile` (uh-cli's shape plus `run`, `docker-build` and `-race` on `test`), +`.golangci.yml` (v2 defaults) and `.dockerignore` (egress's allowlist). + +`make lint` failed on first run, before reaching this repo's code: + +``` +$ golangci-lint run ./... +.../go/1.27.1/libexec/src/crypto/internal/randutil/randutil.go:11:2: could not import math/rand/v2 +(.../math/rand/v2/rand.go:213:17: method must have no type parameters) (typecheck) +``` + +The installed golangci-lint 2.12.2 was built with go1.26.4 and cannot type-check the +Go 1.27 standard library. The `lint` target now runs under the go.mod toolchain, +which is also what CI uses: + +```make +GO_MOD_VERSION := $(shell awk '$$1 == "go" {print $$2}' go.mod) +lint: + GOTOOLCHAIN=go$(GO_MOD_VERSION) golangci-lint run ./... +``` + +With that, lint reported the two expected dead variables (`gatewayGroup`, +`gatewayKind` in `patch.go`), which were removed. `make fmt` reformatted `main.go` +(import order) and the `patch.go` var block. + +Worth noting: this is likely the same failure the egress changelog records as +"`make lint` is blocked by a Go 1.27 toolchain/staticcheck panic"; the same +`GOTOOLCHAIN` pin should unblock it there. Rebuilding golangci-lint with Go 1.27 is +the other fix. `.golangci.yml` carries no settings yet: uh-cli's `errcheck` +exclusions are CLI-specific and nothing here needed one. diff --git a/internal/gateway/patch.go b/internal/gateway/patch.go index 911424f..ed62409 100644 --- a/internal/gateway/patch.go +++ b/internal/gateway/patch.go @@ -10,11 +10,9 @@ import ( ) var ( - defaultPort gwapiv1.PortNumber = 443 - tlsModeTerminate = gwapiv1.TLSModeTerminate - namespacesFromAll gwapiv1.FromNamespaces = gwapiv1.NamespacesFromAll - gatewayGroup gwapiv1.Group = gwapiv1.GroupName - gatewayKind gwapiv1.Kind = "Gateway" + defaultPort gwapiv1.PortNumber = 443 + tlsModeTerminate = gwapiv1.TLSModeTerminate + namespacesFromAll gwapiv1.FromNamespaces = gwapiv1.NamespacesFromAll ) // BuildListenersFromCertificates creates the desired set of managed listeners diff --git a/main.go b/main.go index 043bd61..b5892f8 100644 --- a/main.go +++ b/main.go @@ -4,8 +4,8 @@ import ( "flag" "os" - gozap "go.uber.org/zap" cmv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" + gozap "go.uber.org/zap" "k8s.io/apimachinery/pkg/runtime" utilruntime "k8s.io/apimachinery/pkg/util/runtime" clientgoscheme "k8s.io/client-go/kubernetes/scheme" -- 2.54.0 From 3766e94b354c908c3ca6b7af89ba14d2f38458c4 Mon Sep 17 00:00:00 2001 From: Jan Novak <kacerr.cz@gmail.com> Date: Thu, 8 Oct 2026 20:34:49 +0200 Subject: [PATCH 05/11] feat: stamp the build commit, add --version and zap flags, rename module - module path is now gitea.home.hrajfrisbee.cz/kacerr/gatewayapi-certificates-operator - internal/version resolves the commit from ldflags or Go build info; --version prints it and the first log line carries it - --metrics-bind-address is passed to the manager instead of being dropped - Dockerfile takes GIT_COMMIT, honours TARGETARCH, labels the image revision Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --- CLAUDE.md | 13 +++- Dockerfile | 16 +++- docs/flows.md | 9 +-- .../2026-10-08-2028-house-conventions.md | 31 +++++++- go.mod | 2 +- internal/controller/certificate_controller.go | 2 +- internal/version/version.go | 47 ++++++++++++ internal/version/version_test.go | 74 +++++++++++++++++++ main.go | 24 ++++-- 9 files changed, 195 insertions(+), 23 deletions(-) create mode 100644 internal/version/version.go create mode 100644 internal/version/version_test.go diff --git a/CLAUDE.md b/CLAUDE.md index 27dfc0c..1aebf9d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -30,6 +30,7 @@ kubectl kustomize deploy/ # render the manifests |---|---| | `main.go` | Composition root: scheme, flags, manager, health probes. No leader election. | | `internal/controller/certificate_controller.go` | `CertificateReconciler`: watches `Certificate` and `Gateway`, runs the reconcile loop | +| `internal/version/` | Build commit: ldflags stamp, falling back to Go's VCS build info | | `internal/gateway/patch.go` | Pure functions: `BuildListenersFromCertificates`, `MergeListeners`, `ListenersEqual` | | `deploy/` | Kustomize manifests; namespace `gateway-cert-operator-system`, single replica | @@ -226,8 +227,8 @@ Gitea Actions, workflows in `.gitea/workflows/`: ## Gotchas -- **Three names for one project.** Directory `gateway-helper-operator`; git remote and - image `gatewayapi-certificates-operator`; binary, manifests, namespace and annotation +- **Three names for one project.** Directory `gateway-helper-operator`; git remote, + Go module and image `gatewayapi-certificates-operator`; binary, manifests, namespace and annotation prefix `gateway-cert-operator`. - **`listener-port` annotation is not implemented.** `AnnotationListenerPort` is declared, never read; every listener is on port 443. @@ -242,8 +243,12 @@ Gitea Actions, workflows in `.gitea/workflows/`: roll out to existing listeners. - **`auto-` prefix is the only ownership marker.** A user listener named `auto-…` gets deleted. -- **`--metrics-bind-address` is parsed but ignored**; metrics are still served on - controller-runtime's default `:8080`. Logging is hardcoded to zap dev mode. +- **Logging defaults to zap dev mode** (console encoder, debug level). The deployment + does not pass `--zap-devel=false`, so production logs are not JSON. +- **Version identity is the commit, not a tag.** `--version` and the first log line + print the 12-char commit from `internal/version`. Image builds have no `.git` in + the context (`.dockerignore`), so the commit arrives through the `GIT_COMMIT` + build arg; a plain `docker build` without it reports `unknown`. - **`make lint` pins `GOTOOLCHAIN` to the go.mod version.** golangci-lint cannot type-check a standard library newer than the Go it was built with, so running `golangci-lint run` directly fails when the host Go is ahead of it. diff --git a/Dockerfile b/Dockerfile index 638894c..b17fd67 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,15 +1,25 @@ FROM golang:1.25 AS builder +ARG TARGETOS +ARG TARGETARCH +ARG GIT_COMMIT=unknown WORKDIR /workspace COPY go.mod go.sum ./ RUN go mod download -COPY main.go main.go -COPY internal/ internal/ +# .dockerignore filters the context down to Go sources and module files. +COPY . . -RUN CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build -ldflags="-s -w" -o manager . +# The build context has no .git, so Go's automatic VCS stamp is absent here; +# the commit comes in through GIT_COMMIT instead. +RUN CGO_ENABLED=0 GOOS=${TARGETOS:-linux} GOARCH=${TARGETARCH:-amd64} go build \ + -trimpath \ + -ldflags "-s -w -X gitea.home.hrajfrisbee.cz/kacerr/gatewayapi-certificates-operator/internal/version.Commit=${GIT_COMMIT}" \ + -o manager . FROM gcr.io/distroless/static:nonroot +ARG GIT_COMMIT=unknown +LABEL org.opencontainers.image.revision="${GIT_COMMIT}" WORKDIR / COPY --from=builder /workspace/manager . USER 65532:65532 diff --git a/docs/flows.md b/docs/flows.md index 1bbda45..08d5030 100644 --- a/docs/flows.md +++ b/docs/flows.md @@ -52,11 +52,8 @@ process start ├─ ctrl.GetConfigOrDie() # KUBECONFIG or in-cluster config │ on err ──► os.Exit(1) │ - ├─ ctrl.NewManager(cfg, Options{Scheme, HealthProbeBindAddress: ":8081"}) + ├─ ctrl.NewManager(cfg, Options{Scheme, Metrics, HealthProbeBindAddress}) │ on err ──► log + os.Exit(1) [main.go:44-47] - │ NOTE: metricsAddr is parsed above but NOT passed here; - │ the manager falls back to its own default, which - │ is also ":8080" (see §10, item 2) │ ├─ controller.SetupCertificateReconciler(mgr) [main.go:53] │ registers field index + watches (see §3) @@ -469,8 +466,8 @@ Recorded as-found. None are fixed by this document. **1. `AnnotationListenerPort` is dead code** `certificate_controller.go:25` declares the constant; `BuildListenersFromCertificates` (`patch.go:44`) always uses `defaultPort = 443`. Setting `gateway-cert-operator.io/listener-port` on a Certificate has no effect. -**2. `--metrics-bind-address` flag is parsed but not wired** -`main.go:32` parses the flag; `main.go:44-47` never passes `MetricsBindAddress` to `ctrl.Options`. controller-runtime defaults an empty bind address to `:8080`, so metrics are served and the Service in `deploy/service_gateway-cert-operator-metrics.yaml` works; only changing the flag has no effect. +**2. `--metrics-bind-address` flag was not wired** (fixed) +The flag used to be parsed and dropped; metrics were served anyway because controller-runtime defaults an empty bind address to `:8080`. `main.go` now passes it through `metricsserver.Options`. **3. No finalizer → cert deletion leaves stale listeners** When a Certificate is deleted, Reconcile returns early at branch (A) with no cluster write. The stale `auto-*` listener persists in the Gateway until a sibling event triggers re-convergence (see Scenario 4 above), or permanently if no other Certificate targets that Gateway. Adding a finalizer to the Certificate would close this gap by allowing cleanup before the object is removed from the API server. diff --git a/docs/plans-executions/2026-10-08-2028-house-conventions.md b/docs/plans-executions/2026-10-08-2028-house-conventions.md index 189bf39..ac9d386 100644 --- a/docs/plans-executions/2026-10-08-2028-house-conventions.md +++ b/docs/plans-executions/2026-10-08-2028-house-conventions.md @@ -6,7 +6,7 @@ Plan: `docs/plans/2026-10-08-2028-house-conventions.md` - [x] Step 1 — CLAUDE.md - [x] Step 2 — `.claude/` and docs scaffolding - [x] Step 3 — Makefile, lint, formatting -- [ ] Step 4 — Module path and version identity +- [x] Step 4 — Module path and version identity - [ ] Step 5 — Baseline tests - [ ] Step 6 — CI build workflow - [ ] Step 7 — Close out @@ -84,3 +84,32 @@ Worth noting: this is likely the same failure the egress changelog records as `GOTOOLCHAIN` pin should unblock it there. Rebuilding golangci-lint with Go 1.27 is the other fix. `.golangci.yml` carries no settings yet: uh-cli's `errcheck` exclusions are CLI-specific and nothing here needed one. + +## Step 4 — Module path and version identity + +Module renamed to `gitea.home.hrajfrisbee.cz/kacerr/gatewayapi-certificates-operator`. +`internal/version` copied from egress unchanged (code and test). `main.go` gained +`--version`, a first log line with commit and Go version, the standard `--zap-*` +flags, and passes `--metrics-bind-address` to the manager. + +``` +$ make build && ./bin/manager --version +0eb588e8bd3a-dirty +$ make docker-build && docker run --rm gateway-cert-operator:dev --version +0eb588e8bd3a-dirty +$ docker inspect gateway-cert-operator:dev --format '{{index .Config.Labels "org.opencontainers.image.revision"}} {{.Config.User}}' +0eb588e8bd3a-dirty 65532:65532 +``` + +Deviations from the plan: + +- The Dockerfile build also got `-trimpath`, and keeps `-s -w` (egress has neither). +- The Dockerfile now does `COPY . .` and relies on the new `.dockerignore` + allowlist, instead of copying `main.go` and `internal/` by name. +- The `gozap.AddCaller()` option was kept so log lines look the same as before. + +Worth noting: the manager was not started against a cluster in this step; the +flag wiring is verified by `--help` output and the build only. `GOARCH` defaults to +`amd64` when `TARGETARCH` is empty (legacy builder, as on the CI runner) and follows +the host under BuildKit, so a local build on an Apple Silicon machine is arm64. +`docs/flows.md` still cites pre-change `main.go` line numbers in §2. diff --git a/go.mod b/go.mod index e1aad4d..efe940a 100644 --- a/go.mod +++ b/go.mod @@ -1,4 +1,4 @@ -module github.com/example/gateway-cert-operator +module gitea.home.hrajfrisbee.cz/kacerr/gatewayapi-certificates-operator go 1.25.0 diff --git a/internal/controller/certificate_controller.go b/internal/controller/certificate_controller.go index 8e6084c..8d76c1e 100644 --- a/internal/controller/certificate_controller.go +++ b/internal/controller/certificate_controller.go @@ -13,7 +13,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/reconcile" gwapiv1 "sigs.k8s.io/gateway-api/apis/v1" - gw "github.com/example/gateway-cert-operator/internal/gateway" + gw "gitea.home.hrajfrisbee.cz/kacerr/gatewayapi-certificates-operator/internal/gateway" ) const ( diff --git a/internal/version/version.go b/internal/version/version.go new file mode 100644 index 0000000..1396e50 --- /dev/null +++ b/internal/version/version.go @@ -0,0 +1,47 @@ +// Package version reports which commit the binary was built from. Docker +// builds stamp it via -ldflags (the build context has no .git, so Go's +// automatic VCS stamp is absent there); host builds fall back to that +// automatic stamp. +package version + +import "runtime/debug" + +// Commit is set at link time via +// -ldflags "-X <module>/internal/version.Commit=<hash>". +var Commit string + +// Resolve returns the commit the binary was built from, or "unknown" when +// neither the ldflags stamp nor build info is available (e.g. go test). +func Resolve() string { + return resolve(Commit, debug.ReadBuildInfo) +} + +func resolve(ldflagsCommit string, readBuildInfo func() (*debug.BuildInfo, bool)) string { + if ldflagsCommit != "" { + return ldflagsCommit + } + bi, ok := readBuildInfo() + if !ok { + return "unknown" + } + var revision string + var modified bool + for _, s := range bi.Settings { + switch s.Key { + case "vcs.revision": + revision = s.Value + case "vcs.modified": + modified = s.Value == "true" + } + } + if revision == "" { + return "unknown" + } + if len(revision) > 12 { + revision = revision[:12] + } + if modified { + revision += "-dirty" + } + return revision +} diff --git a/internal/version/version_test.go b/internal/version/version_test.go new file mode 100644 index 0000000..2bcebb6 --- /dev/null +++ b/internal/version/version_test.go @@ -0,0 +1,74 @@ +package version + +import ( + "runtime/debug" + "testing" +) + +func buildInfoWith(settings ...debug.BuildSetting) func() (*debug.BuildInfo, bool) { + return func() (*debug.BuildInfo, bool) { + return &debug.BuildInfo{Settings: settings}, true + } +} + +func TestResolve_precedenceAndFallback(t *testing.T) { + t.Parallel() + + noBuildInfo := func() (*debug.BuildInfo, bool) { return nil, false } + + tests := []struct { + name string + ldflagsCommit string + readBuildInfo func() (*debug.BuildInfo, bool) + want string + }{ + { + name: "ldflags stamp wins over build info", + ldflagsCommit: "abc123def456-dirty", + readBuildInfo: buildInfoWith(debug.BuildSetting{Key: "vcs.revision", Value: "ffffffffffffffffffffffffffffffffffffffff"}), + want: "abc123def456-dirty", + }, + { + name: "no stamp, no build info", + readBuildInfo: noBuildInfo, + want: "unknown", + }, + { + name: "build info without vcs settings", + readBuildInfo: buildInfoWith(), + want: "unknown", + }, + { + name: "full revision truncated to 12 chars", + readBuildInfo: buildInfoWith( + debug.BuildSetting{Key: "vcs.revision", Value: "0123456789abcdef0123456789abcdef01234567"}, + debug.BuildSetting{Key: "vcs.modified", Value: "false"}, + ), + want: "0123456789ab", + }, + { + name: "modified tree gets dirty suffix", + readBuildInfo: buildInfoWith( + debug.BuildSetting{Key: "vcs.revision", Value: "0123456789abcdef0123456789abcdef01234567"}, + debug.BuildSetting{Key: "vcs.modified", Value: "true"}, + ), + want: "0123456789ab-dirty", + }, + { + name: "short revision kept as-is", + readBuildInfo: buildInfoWith( + debug.BuildSetting{Key: "vcs.revision", Value: "abc123"}, + ), + want: "abc123", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + if got := resolve(tc.ldflagsCommit, tc.readBuildInfo); got != tc.want { + t.Errorf("resolve() = %q, want %q", got, tc.want) + } + }) + } +} diff --git a/main.go b/main.go index b5892f8..ff65c84 100644 --- a/main.go +++ b/main.go @@ -2,7 +2,9 @@ package main import ( "flag" + "fmt" "os" + goruntime "runtime" cmv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" gozap "go.uber.org/zap" @@ -12,9 +14,11 @@ import ( ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/healthz" "sigs.k8s.io/controller-runtime/pkg/log/zap" + metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server" gwapiv1 "sigs.k8s.io/gateway-api/apis/v1" - "github.com/example/gateway-cert-operator/internal/controller" + "gitea.home.hrajfrisbee.cz/kacerr/gatewayapi-certificates-operator/internal/controller" + "gitea.home.hrajfrisbee.cz/kacerr/gatewayapi-certificates-operator/internal/version" ) var scheme = runtime.NewScheme() @@ -28,21 +32,27 @@ func init() { func main() { var metricsAddr string var probeAddr string + var showVersion bool flag.StringVar(&metricsAddr, "metrics-bind-address", ":8080", "The address the metrics endpoint binds to.") flag.StringVar(&probeAddr, "health-probe-bind-address", ":8081", "The address the probe endpoint binds to.") + flag.BoolVar(&showVersion, "version", false, "Print the commit this binary was built from and exit.") + opts := zap.Options{Development: true, ZapOpts: []gozap.Option{gozap.AddCaller()}} + opts.BindFlags(flag.CommandLine) flag.Parse() - ctrl.SetLogger(zap.New( - zap.UseDevMode(true), - func(o *zap.Options) { - o.ZapOpts = append(o.ZapOpts, gozap.AddCaller()) - }, - )) + if showVersion { + fmt.Println(version.Resolve()) + return + } + + ctrl.SetLogger(zap.New(zap.UseFlagOptions(&opts))) log := ctrl.Log.WithName("setup") + log.Info("starting gateway-cert-operator", "commit", version.Resolve(), "goVersion", goruntime.Version()) mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{ Scheme: scheme, + Metrics: metricsserver.Options{BindAddress: metricsAddr}, HealthProbeBindAddress: probeAddr, }) if err != nil { -- 2.54.0 From ab408403bc1400ee24f67271e0646b09d5efd6fa Mon Sep 17 00:00:00 2001 From: Jan Novak <kacerr.cz@gmail.com> Date: Thu, 8 Oct 2026 20:36:11 +0200 Subject: [PATCH 06/11] test: add baseline table-driven tests for listener build, merge and compare Covers current correct behaviour of the pure functions in internal/gateway. Known-buggy inputs (wildcards, collisions, truncation) are left out on purpose. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --- .../2026-10-08-2028-house-conventions.md | 19 +- internal/gateway/patch_test.go | 279 ++++++++++++++++++ 2 files changed, 297 insertions(+), 1 deletion(-) create mode 100644 internal/gateway/patch_test.go diff --git a/docs/plans-executions/2026-10-08-2028-house-conventions.md b/docs/plans-executions/2026-10-08-2028-house-conventions.md index ac9d386..b27af46 100644 --- a/docs/plans-executions/2026-10-08-2028-house-conventions.md +++ b/docs/plans-executions/2026-10-08-2028-house-conventions.md @@ -7,7 +7,7 @@ Plan: `docs/plans/2026-10-08-2028-house-conventions.md` - [x] Step 2 — `.claude/` and docs scaffolding - [x] Step 3 — Makefile, lint, formatting - [x] Step 4 — Module path and version identity -- [ ] Step 5 — Baseline tests +- [x] Step 5 — Baseline tests - [ ] Step 6 — CI build workflow - [ ] Step 7 — Close out @@ -113,3 +113,20 @@ flag wiring is verified by `--help` output and the build only. `GOARCH` defaults `amd64` when `TARGETARCH` is empty (legacy builder, as on the CI runner) and follows the host under BuildKit, so a local build on an Apple Silicon machine is arm64. `docs/flows.md` still cites pre-change `main.go` line numbers in §2. + +## Step 5 — Baseline tests + +`internal/gateway/patch_test.go`: five table-driven tests over the pure functions. + +``` +$ go test -race -cover ./... +ok .../internal/gateway 1.870s coverage: 96.9% of statements +ok .../internal/version 2.196s coverage: 94.7% of statements +``` + +Worth noting: `internal/controller` and `main` are at 0%; reconciler tests with the +fake client come with the first reconcile fix. Wildcard names, name collisions and +truncation are deliberately not asserted, so the suite does not pin behaviour that +the bug-fix phase will change. `TestListenersEqual_managedFields` only asserts the +four fields the comparison covers today; it says nothing about protocol, allowed +routes or TLS mode. diff --git a/internal/gateway/patch_test.go b/internal/gateway/patch_test.go new file mode 100644 index 0000000..d5da493 --- /dev/null +++ b/internal/gateway/patch_test.go @@ -0,0 +1,279 @@ +package gateway + +import ( + "testing" + "time" + + cmv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + gwapiv1 "sigs.k8s.io/gateway-api/apis/v1" +) + +const testPrefix = "auto-" + +func cert(namespace, name, secret string, dnsNames ...string) cmv1.Certificate { + return cmv1.Certificate{ + ObjectMeta: metav1.ObjectMeta{Namespace: namespace, Name: name}, + Spec: cmv1.CertificateSpec{SecretName: secret, DNSNames: dnsNames}, + } +} + +func listener(name, hostname, secretNamespace, secret string, port gwapiv1.PortNumber) gwapiv1.Listener { + h := gwapiv1.Hostname(hostname) + ns := gwapiv1.Namespace(secretNamespace) + return gwapiv1.Listener{ + Name: gwapiv1.SectionName(name), + Hostname: &h, + Port: port, + Protocol: gwapiv1.HTTPSProtocolType, + TLS: &gwapiv1.ListenerTLSConfig{ + CertificateRefs: []gwapiv1.SecretObjectReference{ + {Name: gwapiv1.ObjectName(secret), Namespace: &ns}, + }, + }, + } +} + +func names(listeners []gwapiv1.Listener) []string { + out := make([]string, 0, len(listeners)) + for _, l := range listeners { + out = append(out, string(l.Name)) + } + return out +} + +func equalStrings(a, b []string) bool { + if len(a) != len(b) { + return false + } + for i := range a { + if a[i] != b[i] { + return false + } + } + return true +} + +func TestBuildListenersFromCertificates_names(t *testing.T) { + t.Parallel() + + deleted := cert("default", "gone", "gone-tls", "gone.example.com") + now := metav1.NewTime(time.Unix(0, 0)) + deleted.DeletionTimestamp = &now + + tests := []struct { + name string + certs []cmv1.Certificate + want []string + }{ + { + name: "no certificates", + }, + { + name: "one listener per DNS name", + certs: []cmv1.Certificate{cert("default", "web", "web-tls", "a.example.com", "b.example.com")}, + want: []string{"auto-web-a-example-com", "auto-web-b-example-com"}, + }, + { + name: "sorted by listener name across certificates", + certs: []cmv1.Certificate{ + cert("default", "zeta", "zeta-tls", "z.example.com"), + cert("default", "alpha", "alpha-tls", "a.example.com"), + }, + want: []string{"auto-alpha-a-example-com", "auto-zeta-z-example-com"}, + }, + { + name: "certificate being deleted is skipped", + certs: []cmv1.Certificate{deleted, cert("default", "web", "web-tls", "a.example.com")}, + want: []string{"auto-web-a-example-com"}, + }, + { + name: "certificate without secret name is skipped", + certs: []cmv1.Certificate{cert("default", "web", "", "a.example.com")}, + }, + { + name: "certificate without DNS names yields nothing", + certs: []cmv1.Certificate{cert("default", "web", "web-tls")}, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + got := names(BuildListenersFromCertificates(tc.certs, testPrefix)) + if !equalStrings(got, tc.want) { + t.Errorf("listener names = %v, want %v", got, tc.want) + } + }) + } +} + +func TestBuildListenersFromCertificates_listenerFields(t *testing.T) { + t.Parallel() + + got := BuildListenersFromCertificates( + []cmv1.Certificate{cert("apps", "web", "web-tls", "app.example.com")}, testPrefix) + if len(got) != 1 { + t.Fatalf("got %d listeners, want 1", len(got)) + } + l := got[0] + + if l.Hostname == nil || *l.Hostname != "app.example.com" { + t.Errorf("hostname = %v, want app.example.com", l.Hostname) + } + if l.Port != 443 { + t.Errorf("port = %d, want 443", l.Port) + } + if l.Protocol != gwapiv1.HTTPSProtocolType { + t.Errorf("protocol = %q, want HTTPS", l.Protocol) + } + if l.TLS == nil || l.TLS.Mode == nil || *l.TLS.Mode != gwapiv1.TLSModeTerminate { + t.Fatalf("TLS mode is not Terminate: %+v", l.TLS) + } + if len(l.TLS.CertificateRefs) != 1 { + t.Fatalf("got %d certificate refs, want 1", len(l.TLS.CertificateRefs)) + } + ref := l.TLS.CertificateRefs[0] + if ref.Name != "web-tls" { + t.Errorf("secret name = %q, want web-tls", ref.Name) + } + if ref.Namespace == nil || *ref.Namespace != "apps" { + t.Errorf("secret namespace = %v, want the certificate's namespace apps", ref.Namespace) + } + if ref.Kind == nil || *ref.Kind != "Secret" { + t.Errorf("ref kind = %v, want Secret", ref.Kind) + } + if l.AllowedRoutes == nil || l.AllowedRoutes.Namespaces == nil || + l.AllowedRoutes.Namespaces.From == nil || + *l.AllowedRoutes.Namespaces.From != gwapiv1.NamespacesFromAll { + t.Errorf("allowed routes = %+v, want namespaces from All", l.AllowedRoutes) + } +} + +func TestMergeListeners_ownership(t *testing.T) { + t.Parallel() + + manual := listener("https", "manual.example.com", "default", "manual-tls", 443) + stale := listener("auto-old-old-example-com", "old.example.com", "default", "old-tls", 443) + desired := listener("auto-web-a-example-com", "a.example.com", "default", "web-tls", 443) + + tests := []struct { + name string + existing []gwapiv1.Listener + desired []gwapiv1.Listener + want []string + }{ + { + name: "both empty", + }, + { + name: "manual listeners are kept", + existing: []gwapiv1.Listener{manual}, + desired: []gwapiv1.Listener{desired}, + want: []string{"https", "auto-web-a-example-com"}, + }, + { + name: "stale managed listeners are dropped", + existing: []gwapiv1.Listener{manual, stale}, + desired: []gwapiv1.Listener{desired}, + want: []string{"https", "auto-web-a-example-com"}, + }, + { + name: "no desired listeners removes every managed one", + existing: []gwapiv1.Listener{stale, manual}, + want: []string{"https"}, + }, + { + name: "existing managed listener is not duplicated", + existing: []gwapiv1.Listener{desired}, + desired: []gwapiv1.Listener{desired}, + want: []string{"auto-web-a-example-com"}, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + got := names(MergeListeners(tc.existing, tc.desired, testPrefix)) + if !equalStrings(got, tc.want) { + t.Errorf("merged names = %v, want %v", got, tc.want) + } + }) + } +} + +func TestListenersEqual_managedFields(t *testing.T) { + t.Parallel() + + a := listener("auto-web-a-example-com", "a.example.com", "default", "web-tls", 443) + b := listener("auto-web-b-example-com", "b.example.com", "default", "web-tls", 443) + + tests := []struct { + name string + x, y []gwapiv1.Listener + want bool + }{ + {name: "both empty", want: true}, + {name: "identical", x: []gwapiv1.Listener{a, b}, y: []gwapiv1.Listener{a, b}, want: true}, + {name: "order is ignored", x: []gwapiv1.Listener{a, b}, y: []gwapiv1.Listener{b, a}, want: true}, + {name: "different length", x: []gwapiv1.Listener{a, b}, y: []gwapiv1.Listener{a}, want: false}, + {name: "different name", x: []gwapiv1.Listener{a}, y: []gwapiv1.Listener{b}, want: false}, + { + name: "different hostname", + x: []gwapiv1.Listener{a}, + y: []gwapiv1.Listener{listener("auto-web-a-example-com", "other.example.com", "default", "web-tls", 443)}, + want: false, + }, + { + name: "different port", + x: []gwapiv1.Listener{a}, + y: []gwapiv1.Listener{listener("auto-web-a-example-com", "a.example.com", "default", "web-tls", 8443)}, + want: false, + }, + { + name: "different secret name", + x: []gwapiv1.Listener{a}, + y: []gwapiv1.Listener{listener("auto-web-a-example-com", "a.example.com", "default", "other-tls", 443)}, + want: false, + }, + { + name: "different secret namespace", + x: []gwapiv1.Listener{a}, + y: []gwapiv1.Listener{listener("auto-web-a-example-com", "a.example.com", "other", "web-tls", 443)}, + want: false, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + if got := ListenersEqual(tc.x, tc.y); got != tc.want { + t.Errorf("ListenersEqual() = %v, want %v", got, tc.want) + } + }) + } +} + +func TestListenerNameFor_sanitizesDots(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + certName string + dnsName string + want string + }{ + {name: "dots become dashes", certName: "web", dnsName: "app.example.com", want: "auto-web-app-example-com"}, + {name: "single label", certName: "web", dnsName: "localhost", want: "auto-web-localhost"}, + {name: "existing dashes are kept", certName: "my-cert", dnsName: "my-app.example.com", want: "auto-my-cert-my-app-example-com"}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + if got := listenerNameFor(testPrefix, tc.certName, tc.dnsName); got != tc.want { + t.Errorf("listenerNameFor() = %q, want %q", got, tc.want) + } + }) + } +} -- 2.54.0 From 8c074288c6e4adf15dadee8243bf2c09f7c2133f Mon Sep 17 00:00:00 2001 From: Jan Novak <kacerr.cz@gmail.com> Date: Thu, 8 Oct 2026 20:36:22 +0200 Subject: [PATCH 07/11] ci: gate the image build on vet/build/test, push sha tags, protect latest - check job runs go vet, go build and go test -short before the build - images are pushed as :<tag> and :sha-<12>; :latest moves only on tag pushes - GIT_COMMIT build arg and OCI source/created labels Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --- .gitea/workflows/build.yaml | 63 ++++++++++++++----- CLAUDE.md | 5 +- .../2026-10-08-2028-house-conventions.md | 13 +++- 3 files changed, 65 insertions(+), 16 deletions(-) diff --git a/.gitea/workflows/build.yaml b/.gitea/workflows/build.yaml index 1ceec3c..66fccac 100644 --- a/.gitea/workflows/build.yaml +++ b/.gitea/workflows/build.yaml @@ -11,31 +11,66 @@ on: tags: - '*' +concurrency: + group: build-${{ github.ref }} + cancel-in-progress: true + jobs: + check: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-go@v5 + with: + go-version-file: go.mod + cache: true + - name: Vet + run: go vet ./... + - name: Build + run: go build ./... + - name: Test (short) + run: go test -short ./... + build: + needs: check runs-on: ubuntu-latest permissions: contents: read packages: write - steps: - uses: actions/checkout@v4 + - name: Compute image tags + id: meta + run: | + if [ "${{ github.event_name }}" = "workflow_dispatch" ]; then + TAG="${{ inputs.tag }}" + else + TAG="${{ github.ref_name }}" + fi + echo "tag=$TAG" >> "$GITHUB_OUTPUT" + echo "sha=sha-$(echo '${{ github.sha }}' | cut -c1-12)" >> "$GITHUB_OUTPUT" + - name: Login to Gitea registry run: echo "${{ secrets.REGISTRY_TOKEN }}" | docker login -u ${{ github.actor }} --password-stdin gitea.home.hrajfrisbee.cz - name: Build and push run: | - TAG=${{ github.ref_name }} - if [ "${{ github.event_name }}" = "workflow_dispatch" ]; then - TAG=${{ inputs.tag }} - fi - REPO=gitea.home.hrajfrisbee.cz/${{ github.repository }} - docker build -f Dockerfile \ - --build-arg GIT_TAG=$TAG \ - --build-arg GIT_COMMIT=${{ github.sha }} \ - --build-arg BUILD_DATE=$(date -u +%Y-%m-%dT%H:%M:%SZ) \ - -t $REPO:$TAG \ - -t $REPO:latest . - docker push $REPO:$TAG - docker push $REPO:latest + IMAGE=gitea.home.hrajfrisbee.cz/${{ github.repository }} + docker build \ + --build-arg GIT_COMMIT=$(echo '${{ github.sha }}' | cut -c1-12) \ + --label org.opencontainers.image.source=https://gitea.home.hrajfrisbee.cz/${{ github.repository }} \ + --label org.opencontainers.image.created=$(date -u +%Y-%m-%dT%H:%M:%SZ) \ + -t "$IMAGE:${{ steps.meta.outputs.tag }}" \ + -t "$IMAGE:${{ steps.meta.outputs.sha }}" \ + . + docker push "$IMAGE:${{ steps.meta.outputs.tag }}" + docker push "$IMAGE:${{ steps.meta.outputs.sha }}" + + # Only real tag pushes move :latest — an ad-hoc dispatch of an old ref must not clobber it. + - name: Push latest (tag builds only) + if: github.event_name == 'push' + run: | + IMAGE=gitea.home.hrajfrisbee.cz/${{ github.repository }} + docker tag "$IMAGE:${{ steps.meta.outputs.tag }}" "$IMAGE:latest" + docker push "$IMAGE:latest" diff --git a/CLAUDE.md b/CLAUDE.md index 1aebf9d..6a1d374 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -209,7 +209,10 @@ Gitea Actions, workflows in `.gitea/workflows/`: - `build.yaml` builds the image and pushes it to `gitea.home.hrajfrisbee.cz/kacerr/gatewayapi-certificates-operator` on any tag push - or manual dispatch. Needs the `REGISTRY_TOKEN` repo secret. + or manual dispatch with a `tag` input. A `check` job (`go vet`, `go build`, + `go test -short`) gates the build. Every build pushes `:<tag>` and an immutable + `:sha-<12-char-commit>`; `:latest` moves only on real tag pushes. Needs the + `REGISTRY_TOKEN` repo secret. - `kubernetes-deploy.yaml` applies `deploy/` to the home-lab cluster through a Vault → Kanidm → OIDC auth chain. diff --git a/docs/plans-executions/2026-10-08-2028-house-conventions.md b/docs/plans-executions/2026-10-08-2028-house-conventions.md index b27af46..92f65a3 100644 --- a/docs/plans-executions/2026-10-08-2028-house-conventions.md +++ b/docs/plans-executions/2026-10-08-2028-house-conventions.md @@ -8,7 +8,7 @@ Plan: `docs/plans/2026-10-08-2028-house-conventions.md` - [x] Step 3 — Makefile, lint, formatting - [x] Step 4 — Module path and version identity - [x] Step 5 — Baseline tests -- [ ] Step 6 — CI build workflow +- [x] Step 6 — CI build workflow - [ ] Step 7 — Close out ## Step 1 — CLAUDE.md @@ -130,3 +130,14 @@ truncation are deliberately not asserted, so the suite does not pin behaviour th the bug-fix phase will change. `TestListenersEqual_managedFields` only asserts the four fields the comparison covers today; it says nothing about protocol, allowed routes or TLS mode. + +## Step 6 — CI build workflow + +`.gitea/workflows/build.yaml` is now a verbatim copy of the egress workflow; it has +no project-specific strings (the image name comes from `github.repository`). + +Worth noting: not run yet. The first tag push or manual dispatch is the test. Two +things could differ from egress on this repo's runner: `actions/setup-go@v5` (uh-cli +had to install Go with curl instead), and lint is not part of the `check` job, same +as egress. The unused `GIT_TAG` and `BUILD_DATE` build args are gone; +`kubernetes-deploy.yaml` is untouched. -- 2.54.0 From 60360ca5b93e05db5a029e8b3ed976d72b0cd797 Mon Sep 17 00:00:00 2001 From: Jan Novak <kacerr.cz@gmail.com> Date: Thu, 8 Oct 2026 20:43:02 +0200 Subject: [PATCH 08/11] fix: address review findings on the conventions branch - ci: reject a manual dispatch with tag "latest"; concurrency group per tag - Dockerfile: declare GIT_COMMIT after the module download layer - Makefile: docker-build targets linux/amd64 by default - patch.go: no shared package-level pointers; main.go: no init() - tests: assert that merge replaces managed listener content - docs: flows.md startup section matches main.go; review saved to docs/reviews Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --- .gitea/workflows/build.yaml | 10 +++- CLAUDE.md | 12 ++-- Dockerfile | 5 +- Makefile | 6 +- docs/flows.md | 36 +++++++----- .../2026-10-08-2028-house-conventions.md | 12 ++++ .../2026-10-08-2040-house-conventions.md | 56 +++++++++++++++++++ internal/gateway/patch.go | 10 +--- internal/gateway/patch_test.go | 18 ++++++ main.go | 13 ++--- 10 files changed, 138 insertions(+), 40 deletions(-) create mode 100644 docs/reviews/2026-10-08-2040-house-conventions.md diff --git a/.gitea/workflows/build.yaml b/.gitea/workflows/build.yaml index 66fccac..534f053 100644 --- a/.gitea/workflows/build.yaml +++ b/.gitea/workflows/build.yaml @@ -4,15 +4,14 @@ on: workflow_dispatch: inputs: tag: - description: 'Image tag' + description: 'Image tag (not "latest")' required: true - default: 'latest' push: tags: - '*' concurrency: - group: build-${{ github.ref }} + group: build-${{ github.ref }}-${{ inputs.tag }} cancel-in-progress: true jobs: @@ -45,6 +44,11 @@ jobs: run: | if [ "${{ github.event_name }}" = "workflow_dispatch" ]; then TAG="${{ inputs.tag }}" + # The Deployment runs :latest; only real tag pushes may move it. + if [ "$TAG" = "latest" ]; then + echo "refusing to dispatch with tag 'latest'" >&2 + exit 1 + fi else TAG="${{ github.ref_name }}" fi diff --git a/CLAUDE.md b/CLAUDE.md index 6a1d374..3147d61 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -52,7 +52,8 @@ Reconcile loop (keyed by Certificate): 6. Patch the Gateway only when `ListenersEqual` reports a difference. A reverse watch maps Gateway events back to the targeting Certificates through the -same index, so manual Gateway edits are reverted for `auto-*` listeners. +same index. Manual edits to `auto-*` listeners are reverted only in the fields +`ListenersEqual` compares, and only while at least one Certificate targets the Gateway. Listeners named `auto-{certName}-{dnsName with dots as dashes}` belong to the operator. Any other listener is never modified or removed. @@ -210,9 +211,10 @@ Gitea Actions, workflows in `.gitea/workflows/`: - `build.yaml` builds the image and pushes it to `gitea.home.hrajfrisbee.cz/kacerr/gatewayapi-certificates-operator` on any tag push or manual dispatch with a `tag` input. A `check` job (`go vet`, `go build`, - `go test -short`) gates the build. Every build pushes `:<tag>` and an immutable - `:sha-<12-char-commit>`; `:latest` moves only on real tag pushes. Needs the - `REGISTRY_TOKEN` repo secret. + `go test -short`) gates the build; it does not run lint or `-race`, so run + `make check` before tagging. Every build pushes `:<tag>` and an immutable + `:sha-<12-char-commit>`; `:latest` moves only on real tag pushes, and a dispatch + with `tag: latest` is rejected. Needs the `REGISTRY_TOKEN` repo secret. - `kubernetes-deploy.yaml` applies `deploy/` to the home-lab cluster through a Vault → Kanidm → OIDC auth chain. @@ -255,6 +257,8 @@ Gitea Actions, workflows in `.gitea/workflows/`: - **`make lint` pins `GOTOOLCHAIN` to the go.mod version.** golangci-lint cannot type-check a standard library newer than the Go it was built with, so running `golangci-lint run` directly fails when the host Go is ahead of it. +- **`make docker-build` builds `linux/amd64`** regardless of the host, because that + is what the cluster runs. Override with `PLATFORM=`. - **No leader election**; safe only at `replicas: 1`. - **Deployment uses the mutable `:latest` tag**, so a new image does not roll out on its own. diff --git a/Dockerfile b/Dockerfile index b17fd67..4de424f 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,7 +1,6 @@ FROM golang:1.25 AS builder ARG TARGETOS ARG TARGETARCH -ARG GIT_COMMIT=unknown WORKDIR /workspace COPY go.mod go.sum ./ @@ -11,7 +10,9 @@ RUN go mod download COPY . . # The build context has no .git, so Go's automatic VCS stamp is absent here; -# the commit comes in through GIT_COMMIT instead. +# the commit comes in through GIT_COMMIT instead. Declared this late so a new +# commit does not invalidate the module-download layer. +ARG GIT_COMMIT=unknown RUN CGO_ENABLED=0 GOOS=${TARGETOS:-linux} GOARCH=${TARGETARCH:-amd64} go build \ -trimpath \ -ldflags "-s -w -X gitea.home.hrajfrisbee.cz/kacerr/gatewayapi-certificates-operator/internal/version.Commit=${GIT_COMMIT}" \ diff --git a/Makefile b/Makefile index c13e53c..a4cb63a 100644 --- a/Makefile +++ b/Makefile @@ -1,6 +1,8 @@ BINARY := manager BIN_DIR := bin IMG ?= gateway-cert-operator:dev +# The cluster is amd64; without this an Apple Silicon host builds arm64. +PLATFORM ?= linux/amd64 # GIT_COMMIT is baked into the image (-ldflags in the Dockerfile); -dirty # covers staged and untracked changes too, which `git diff --quiet` misses. @@ -8,7 +10,7 @@ GIT_COMMIT ?= $(shell git rev-parse --short=12 HEAD 2>/dev/null || echo unknown) # golangci-lint type-checks the standard library of whatever toolchain is active, # and fails on one newer than the Go it was built with. Linting with the go.mod -# version keeps it working when the host Go is ahead, and matches CI. +# version keeps it working when the host Go is ahead. GO_MOD_VERSION := $(shell awk '$$1 == "go" {print $$2}' go.mod) .PHONY: all build run test test-short lint vet fmt tidy check docker-build clean help @@ -43,7 +45,7 @@ tidy: ## Tidy go.mod and go.sum check: lint vet test-short ## Run all quality checks (lint + vet + test-short) docker-build: ## Build the container image (IMG=name:tag) - docker build --build-arg GIT_COMMIT=$(GIT_COMMIT) -t $(IMG) . + docker build --platform $(PLATFORM) --build-arg GIT_COMMIT=$(GIT_COMMIT) -t $(IMG) . clean: ## Remove build artifacts rm -rf $(BIN_DIR) diff --git a/docs/flows.md b/docs/flows.md index 08d5030..48447ef 100644 --- a/docs/flows.md +++ b/docs/flows.md @@ -35,27 +35,35 @@ Executed once at process start (`main.go`): ``` process start │ - ├─ init() [main.go:22-26] - │ ├─ clientgoscheme.AddToScheme(scheme) # core/v1: Secrets, Events, … - │ ├─ cmv1.AddToScheme(scheme) # cert-manager.io/v1 Certificate - │ └─ gwapiv1.Install(scheme) # gateway.networking.k8s.io/v1 Gateway - │ - └─ main() [main.go:28] + └─ main() [main.go:24] │ - ├─ flag.StringVar --metrics-bind-address default ":8080" [main.go:32] - ├─ flag.StringVar --health-probe-bind-address default ":8081" [main.go:33] + ├─ flag.StringVar --metrics-bind-address default ":8080" [main.go:29] + ├─ flag.StringVar --health-probe-bind-address default ":8081" [main.go:30] + ├─ flag.BoolVar --version [main.go:31] + ├─ zap.Options{Development: true, AddCaller}.BindFlags [main.go:32-33] + │ adds --zap-devel, --zap-encoder, --zap-log-level, … ├─ flag.Parse() [main.go:34] │ - ├─ ctrl.SetLogger(zap, UseDevMode=true, AddCaller) [main.go:36-41] - │ dev-mode console logger with file:line call site + ├─ --version ──► print version.Resolve(), return [main.go:36-39] + │ before any logger or cluster access + │ + ├─ scheme := runtime.NewScheme() [main.go:41-44] + │ ├─ clientgoscheme.AddToScheme(scheme) # core/v1: Secrets, Events, … + │ ├─ cmv1.AddToScheme(scheme) # cert-manager.io/v1 Certificate + │ └─ gwapiv1.Install(scheme) # gateway.networking.k8s.io/v1 Gateway + │ + ├─ ctrl.SetLogger(zap.New(zap.UseFlagOptions(&opts))) [main.go:46] + │ dev-mode console logger with file:line call site unless + │ --zap-devel=false is passed + ├─ log "starting gateway-cert-operator" commit=… goVersion=… [main.go:48] │ ├─ ctrl.GetConfigOrDie() # KUBECONFIG or in-cluster config │ on err ──► os.Exit(1) │ ├─ ctrl.NewManager(cfg, Options{Scheme, Metrics, HealthProbeBindAddress}) - │ on err ──► log + os.Exit(1) [main.go:44-47] + │ on err ──► log + os.Exit(1) [main.go:50-54] │ - ├─ controller.SetupCertificateReconciler(mgr) [main.go:53] + ├─ controller.SetupCertificateReconciler(mgr) [main.go:60] │ registers field index + watches (see §3) │ on err ──► log + os.Exit(1) │ @@ -63,7 +71,7 @@ process start ├─ mgr.AddReadyzCheck("readyz", healthz.Ping) → GET /readyz:8081 │ on err ──► log + os.Exit(1) │ - └─ mgr.Start(ctrl.SetupSignalHandler()) [main.go:68] + └─ mgr.Start(ctrl.SetupSignalHandler()) [main.go:75] SetupSignalHandler installs SIGINT / SIGTERM cancel mgr.Start blocks; on shutdown → graceful stop on err ──► log + os.Exit(1) @@ -479,7 +487,7 @@ Described in §6c. Drift in `Protocol`, `AllowedRoutes`, `TLS.Options`, or any ` Branch (F) of Reconcile treats all Gateway `Get` errors the same (including `NotFound`) and returns them as re-queue-able errors. A Certificate with a typo'd `gateway-name` annotation will retry with rate-limited backoff indefinitely, generating log noise and API load. There is no terminal failure path. **6. No leader election** -`ctrl.NewManager` at `main.go:44` does not set `LeaderElection: true`. The `deploy/deployment_gateway-cert-operator.yaml` runs `replicas: 1` which makes this safe today. Scaling to multiple replicas without enabling leader election would result in concurrent Gateway patches. +`ctrl.NewManager` at `main.go:50` does not set `LeaderElection: true`. The `deploy/deployment_gateway-cert-operator.yaml` runs `replicas: 1` which makes this safe today. Scaling to multiple replicas without enabling leader election would result in concurrent Gateway patches. **7. `listenerNameFor` edge cases** - **Wildcard hostnames**: `*.example.com` produces `SectionName` `auto-…-*-example-com`. The `*` character is not valid in an RFC 1123 label; the Gateway API server will reject the Patch, causing an error-retry loop. diff --git a/docs/plans-executions/2026-10-08-2028-house-conventions.md b/docs/plans-executions/2026-10-08-2028-house-conventions.md index 92f65a3..cdac887 100644 --- a/docs/plans-executions/2026-10-08-2028-house-conventions.md +++ b/docs/plans-executions/2026-10-08-2028-house-conventions.md @@ -141,3 +141,15 @@ things could differ from egress on this repo's runner: `actions/setup-go@v5` (uh had to install Go with curl instead), and lint is not part of the `check` job, same as egress. The unused `GIT_TAG` and `BUILD_DATE` build args are gone; `kubernetes-deploy.yaml` is untouched. + +## Step 7 — Close out (review) + +Ran both reviewer agents on `main...HEAD`; findings and their status are in +`docs/reviews/2026-10-08-2040-house-conventions.md`. Ten items fixed on the branch, four new +bugs deferred to the bug-fix phase, one test simplification declined. + +Worth noting: the reviewer agents were not registered as agent types when the review +started (they were added in this session), so they were run as general-purpose +agents told to follow the agent files. The `:latest` dispatch hole came in with the +workflow copied from egress and is still open there. Step 7 stays unchecked until +the branch is pushed and the MR is open. diff --git a/docs/reviews/2026-10-08-2040-house-conventions.md b/docs/reviews/2026-10-08-2040-house-conventions.md new file mode 100644 index 0000000..4c6fdfd --- /dev/null +++ b/docs/reviews/2026-10-08-2040-house-conventions.md @@ -0,0 +1,56 @@ +# Review: house conventions port + +**Created:** 2026-10-08 20:40 CEST +**Scope:** `main...feat/house-conventions` at `8c07428` (7 commits) +**Reviewers:** `operator-reviewer`, `go-operator-reviewer` + +Both reviewers ran `make check` and `go test -race ./...` (green) and confirmed the +branch changes no reconcile behaviour. No merge-blockers. + +## Worth fixing + +- [x] `build.yaml`: a manual dispatch with the default `tag: latest` overwrote + `:latest`, the tag the Deployment runs. Default removed; `latest` is rejected. + (Inherited from the egress workflow, which still has it.) +- [x] `Dockerfile`: `ARG GIT_COMMIT` sat above `go mod download`, so every commit + re-downloaded all modules. Moved to just above the build. +- [x] `patch_test.go`: `TestMergeListeners_ownership` compared names only; a + mutation that kept stale listener content passed. Added + `TestMergeListeners_replacesManagedContent`. +- [x] `make docker-build` followed the host architecture (arm64 on this Mac, where + the Dockerfile used to hard-code amd64). Now builds `linux/amd64` by default. +- [x] `docs/flows.md` §2 described the old `main.go` (line refs, hardcoded dev + logger, no `--version`). Rewritten against the current file. +- [x] `CLAUDE.md` overstated the reverse watch. Reworded. +- [x] `build.yaml`: two dispatches from the same ref shared a concurrency group and + cancelled each other. Group now includes the tag input. + +## Simplifications + +- [x] `patch.go`: package-level `tlsModeTerminate` / `namespacesFromAll` were shared + by address across every listener. Replaced with `ptrTo(...)`; `defaultPort` is a + const. +- [x] `main.go`: `init()` removed, scheme built in `main()` (CLAUDE.md says no + `init()`). +- [x] `Makefile` comment claimed the lint toolchain pin "matches CI"; CI does not + lint. Claim dropped. +- [ ] `TestListenerNameFor_sanitizesDots` tests an unexported helper and overlaps the + names table. Kept as the home for truncation and collision cases in the bug-fix + phase. + +## Deferred to the bug-fix phase (new, not in the plan's backlog) + +- [ ] Empty desired set: when a Gateway has only `auto-*` listeners and no cert + yields any, `MergeListeners` returns nil and the patch sends `listeners: null`, + which the API server rejects (`MinItems=1`). Permanent error requeue. Any fix for + "last cert deleted" runs straight into this. `patch.go` `MergeListeners`, + `certificate_controller.go:119-120`. +- [ ] Listeners are added before the Certificate is Ready, so they reference a + Secret that may not exist yet. +- [ ] ClusterRole grants `events` create/patch; nothing records events. +- [ ] CI `check` job has no golangci-lint and no `-race`. Needs a linter install + step on the runner (uh-cli does it from a pinned tarball). + +## Nice-to-have + +- [x] Assert the secret ref `Group` in `TestBuildListenersFromCertificates_listenerFields`. diff --git a/internal/gateway/patch.go b/internal/gateway/patch.go index ed62409..54c4b39 100644 --- a/internal/gateway/patch.go +++ b/internal/gateway/patch.go @@ -9,11 +9,7 @@ import ( gwapiv1 "sigs.k8s.io/gateway-api/apis/v1" ) -var ( - defaultPort gwapiv1.PortNumber = 443 - tlsModeTerminate = gwapiv1.TLSModeTerminate - namespacesFromAll gwapiv1.FromNamespaces = gwapiv1.NamespacesFromAll -) +const defaultPort gwapiv1.PortNumber = 443 // BuildListenersFromCertificates creates the desired set of managed listeners // from all annotated Certificate resources targeting a single Gateway. @@ -43,11 +39,11 @@ func BuildListenersFromCertificates(certs []cmv1.Certificate, prefix string) []g Protocol: gwapiv1.HTTPSProtocolType, AllowedRoutes: &gwapiv1.AllowedRoutes{ Namespaces: &gwapiv1.RouteNamespaces{ - From: &namespacesFromAll, + From: ptrTo(gwapiv1.NamespacesFromAll), }, }, TLS: &gwapiv1.ListenerTLSConfig{ - Mode: &tlsModeTerminate, + Mode: ptrTo(gwapiv1.TLSModeTerminate), CertificateRefs: []gwapiv1.SecretObjectReference{ { Group: ptrTo(gwapiv1.Group("")), diff --git a/internal/gateway/patch_test.go b/internal/gateway/patch_test.go index d5da493..5da7542 100644 --- a/internal/gateway/patch_test.go +++ b/internal/gateway/patch_test.go @@ -143,6 +143,9 @@ func TestBuildListenersFromCertificates_listenerFields(t *testing.T) { if ref.Kind == nil || *ref.Kind != "Secret" { t.Errorf("ref kind = %v, want Secret", ref.Kind) } + if ref.Group == nil || *ref.Group != "" { + t.Errorf("ref group = %v, want the core group", ref.Group) + } if l.AllowedRoutes == nil || l.AllowedRoutes.Namespaces == nil || l.AllowedRoutes.Namespaces.From == nil || *l.AllowedRoutes.Namespaces.From != gwapiv1.NamespacesFromAll { @@ -202,6 +205,21 @@ func TestMergeListeners_ownership(t *testing.T) { } } +func TestMergeListeners_replacesManagedContent(t *testing.T) { + t.Parallel() + + old := listener("auto-web-a-example-com", "a.example.com", "default", "old-tls", 443) + desired := listener("auto-web-a-example-com", "a.example.com", "default", "web-tls", 443) + + got := MergeListeners([]gwapiv1.Listener{old}, []gwapiv1.Listener{desired}, testPrefix) + if len(got) != 1 { + t.Fatalf("got %d listeners, want 1", len(got)) + } + if name := got[0].TLS.CertificateRefs[0].Name; name != "web-tls" { + t.Errorf("secret name = %q, want the desired web-tls", name) + } +} + func TestListenersEqual_managedFields(t *testing.T) { t.Parallel() diff --git a/main.go b/main.go index ff65c84..ed6ba62 100644 --- a/main.go +++ b/main.go @@ -21,14 +21,6 @@ import ( "gitea.home.hrajfrisbee.cz/kacerr/gatewayapi-certificates-operator/internal/version" ) -var scheme = runtime.NewScheme() - -func init() { - utilruntime.Must(clientgoscheme.AddToScheme(scheme)) - utilruntime.Must(cmv1.AddToScheme(scheme)) - utilruntime.Must(gwapiv1.Install(scheme)) -} - func main() { var metricsAddr string var probeAddr string @@ -46,6 +38,11 @@ func main() { return } + scheme := runtime.NewScheme() + utilruntime.Must(clientgoscheme.AddToScheme(scheme)) + utilruntime.Must(cmv1.AddToScheme(scheme)) + utilruntime.Must(gwapiv1.Install(scheme)) + ctrl.SetLogger(zap.New(zap.UseFlagOptions(&opts))) log := ctrl.Log.WithName("setup") log.Info("starting gateway-cert-operator", "commit", version.Resolve(), "goVersion", goruntime.Version()) -- 2.54.0 From 6698eab1d8ea7e7bf89050127f965e44026c0636 Mon Sep 17 00:00:00 2001 From: Jan Novak <kacerr.cz@gmail.com> Date: Thu, 8 Oct 2026 20:46:47 +0200 Subject: [PATCH 09/11] docs: add plan for sanitizing listener names Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --- .../2026-10-08-2046-listener-name-sanitize.md | 9 ++ .../2026-10-08-2046-listener-name-sanitize.md | 111 ++++++++++++++++++ 2 files changed, 120 insertions(+) create mode 100644 docs/plans-executions/2026-10-08-2046-listener-name-sanitize.md create mode 100644 docs/plans/2026-10-08-2046-listener-name-sanitize.md diff --git a/docs/plans-executions/2026-10-08-2046-listener-name-sanitize.md b/docs/plans-executions/2026-10-08-2046-listener-name-sanitize.md new file mode 100644 index 0000000..d0107a8 --- /dev/null +++ b/docs/plans-executions/2026-10-08-2046-listener-name-sanitize.md @@ -0,0 +1,9 @@ +# Execution: Sanitize listener names derived from certificate DNS names + +Plan: `docs/plans/2026-10-08-2046-listener-name-sanitize.md` + +- [x] Step 0 — Save and commit the plan +- [ ] Step 1 — Regression tests first +- [ ] Step 2 — Fix +- [ ] Step 3 — Docs +- [ ] Step 4 — Ship diff --git a/docs/plans/2026-10-08-2046-listener-name-sanitize.md b/docs/plans/2026-10-08-2046-listener-name-sanitize.md new file mode 100644 index 0000000..7c2ae75 --- /dev/null +++ b/docs/plans/2026-10-08-2046-listener-name-sanitize.md @@ -0,0 +1,111 @@ +# Plan: Sanitize listener names derived from certificate DNS names + +**Created:** 2026-10-08 20:46 CEST + +## Context + +The operator in the home-lab cluster fails every reconcile of +`kube-system/wildcard-home-hrajfrisbee`: + +``` +spec.listeners[4].name: Invalid value: "auto-wildcard-home-hrajfrisbee-*-home-hrajfrisbee-cz": +should match '^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$' +``` + +`listenerNameFor` (`internal/gateway/patch.go:137`) only replaces dots in the DNS +name, so `*.home.hrajfrisbee.cz` puts a `*` into the listener name. Gateway API +listener names are `SectionName`: lowercase alphanumerics, `-` and `.`, each +dot-separated label starting and ending alphanumeric, max 253 chars. Because the +patch replaces the whole listeners array, this one name blocks every other +certificate on `cilium-gateway` too. + +Outcome: any DNS name cert-manager accepts yields a valid listener name, and names +that are valid today do not change (no listener churn on existing Gateways). + +Lands on `feat/house-conventions` (decided), in the same MR as the conventions port. + +## Design + +Only the DNS-name part is rewritten; the listener `Hostname` keeps the original +value (`*.home.hrajfrisbee.cz` is a valid Gateway hostname). + +`listenerNameFor(prefix, certName, dnsName)` in `internal/gateway/patch.go`: + +1. Sanitize the DNS name with a new unexported `sanitizeDNSName`: + - lowercase; + - a leading `*` label becomes `wildcard` (`*.example.com` → `wildcard-example-com`); + - `.` becomes `-` (as today); + - any other character outside `[a-z0-9-]` becomes `-`; + - trim leading and trailing `-`. +2. Name is `prefix + certName + "-" + sanitized`, as today. +3. If longer than 253: cut to 244, trim trailing `-` and `.`, append `-` plus the + first 8 hex chars of the SHA-256 of the untruncated name. This replaces the + current blind `name[:253]`, which can end in `-` (invalid) and makes long names + collide. + +Result for the failing cert: +`auto-wildcard-home-hrajfrisbee-wildcard-home-hrajfrisbee-cz`. + +Not changed here (stay on the backlog): collisions between distinct inputs that +sanitize to the same name (e.g. `*.x.com` and `wildcard.x.com` in one cert, or the +same cert name in two namespaces), uppercase in the listener `Hostname`, and the +other items in `docs/reviews/`. + +## Steps + +### Step 0 — Save and commit the plan + +Copy to `docs/plans/2026-10-08-2046-listener-name-sanitize.md`, create the paired +execution log, commit alone. + +### Step 1 — Regression tests first + +In `internal/gateway/patch_test.go`, extend `TestListenerNameFor_sanitizesDots` +(rename to `TestListenerNameFor_validSectionName`) with cases, each asserting the +exact name and that it matches the `SectionName` pattern and length: + +- `*.home.hrajfrisbee.cz` with cert `wildcard-home-hrajfrisbee` (the production case); +- uppercase DNS name; +- a character outside the allowed set; +- a name over 253 chars: result is ≤ 253, valid, ends with the hash suffix, and two + long inputs sharing a 253-char prefix produce different names; +- existing plain names unchanged (already in the table). + +Add a wildcard case to `TestBuildListenersFromCertificates_names` and assert in +`_listenerFields` style that the hostname stays `*.example.com`. Run and confirm the +new cases fail before the fix. + +### Step 2 — Fix + +Implement the design in `internal/gateway/patch.go` (stdlib only: `strings`, +`crypto/sha256`, `encoding/hex`). Update the function comment. + +### Step 3 — Docs + +- `CLAUDE.md`: Gotchas — drop the wildcard half of "a single bad listener blocks the + whole Gateway" (collisions remain); Architecture — describe the name as sanitized. +- `docs/flows.md` §7 and §10 item 7: wildcard and truncation rows updated. +- `docs/reviews/…-house-conventions.md` is not touched (it predates this). + +### Step 4 — Ship + +`make check` and `go test -race ./...`, then `/ship` (pushes +`feat/house-conventions`) and open the MR with `tea pr create`. Tick Step 7 of the +conventions execution log in the same commit. + +## Verification + +```bash +go test -race -run 'TestListenerNameFor|TestBuildListeners' -v ./internal/gateway/ +make check +``` + +Live, after you merge the MR: + +1. `/release` → tag `v0.1.0`; CI builds and moves `:latest`. +2. `kubectl -n gateway-cert-operator-system rollout restart deploy/gateway-cert-operator` + (the Deployment pins `:latest`, so it does not roll on its own). +3. The "Reconciler error" for `wildcard-home-hrajfrisbee` stops, and + `kubectl -n kube-system get gateway cilium-gateway -o jsonpath='{.spec.listeners[*].name}'` + lists `auto-wildcard-home-hrajfrisbee-wildcard-home-hrajfrisbee-cz`. +4. Then the CHANGELOG entries for both the conventions port and this fix. -- 2.54.0 From 42b53b753832898a2213a69222c587f6b65d05be Mon Sep 17 00:00:00 2001 From: Jan Novak <kacerr.cz@gmail.com> Date: Thu, 8 Oct 2026 20:47:44 +0200 Subject: [PATCH 10/11] fix: produce valid listener names for wildcard and unusual DNS names A DNS name like *.home.hrajfrisbee.cz put "*" into the listener name; the API server rejected the patch, which blocked every certificate on that Gateway. - lowercase the DNS part, spell "*" as "wildcard", map other characters outside [a-z0-9] to "-" - names over 253 chars are cut and get a hash suffix instead of a blind cut - names that were valid before are unchanged Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --- CLAUDE.md | 11 ++-- docs/flows.md | 19 +++--- .../2026-10-08-2046-listener-name-sanitize.md | 31 +++++++++- internal/gateway/patch.go | 38 +++++++++--- internal/gateway/patch_test.go | 62 ++++++++++++++++++- 5 files changed, 137 insertions(+), 24 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 3147d61..ca02674 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -55,8 +55,10 @@ A reverse watch maps Gateway events back to the targeting Certificates through t same index. Manual edits to `auto-*` listeners are reverted only in the fields `ListenersEqual` compares, and only while at least one Certificate targets the Gateway. -Listeners named `auto-{certName}-{dnsName with dots as dashes}` belong to the -operator. Any other listener is never modified or removed. +Listeners named `auto-{certName}-{sanitized dnsName}` belong to the operator. The DNS +name is lowercased, `*` becomes `wildcard`, anything else outside `[a-z0-9]` becomes +`-`; names over 253 chars are cut and get a hash suffix. Changing this scheme renames +existing listeners on live Gateways. Any other listener is never modified or removed. `docs/flows.md` has the full control-path reference (startup, watch wiring, decision tree, scenarios). @@ -241,8 +243,9 @@ Gitea Actions, workflows in `.gitea/workflows/`: it does not clean up the old Gateway. A sibling Certificate's next reconcile fixes it; with no sibling the listener stays forever. - **A single bad listener blocks the whole Gateway.** The patch replaces the full - listeners array, so a wildcard DNS name (`*` ends up in the listener name) or a name - collision makes the API server reject every update for that Gateway. + listeners array, so a listener-name collision (same cert name in two namespaces, + or two DNS names that sanitize alike, such as `a.b.com` and `a-b.com`) makes the + API server reject every update for that Gateway. - **`ListenersEqual` is partial**: name, hostname, port and the first secret ref only. Changing what `BuildListenersFromCertificates` generates in other fields will not roll out to existing listeners. diff --git a/docs/flows.md b/docs/flows.md index 48447ef..bfbe67f 100644 --- a/docs/flows.md +++ b/docs/flows.md @@ -325,19 +325,22 @@ The comparison is done via a name-keyed map (`listenerMap`, `patch.go:118`), so ## 7. Listener Naming and Sanitization -`listenerNameFor` (`patch.go:143`): +`listenerNameFor` and `sanitizeDNSName` (`patch.go`): ``` input: prefix="auto-" certName="my-app-cert" dnsName="app.example.com" -step 1: replace every "." with "-" +step 1: sanitize the DNS name + lowercase; "*" ──► "wildcard"; every other character outside + [a-z0-9] ──► "-"; trim leading and trailing "-" "app.example.com" ──► "app-example-com" step 2: concatenate "auto-" + "my-app-cert" + "-" + "app-example-com" ──► "auto-my-app-cert-app-example-com" -step 3: if len > 253 bytes: name = name[:253] (hard byte slice) +step 3: if len > 253: cut to 244, trim trailing "-" and ".", append + "-" + first 8 hex chars of sha256(full name) ``` Example outputs for a cert with two DNS names: @@ -345,9 +348,9 @@ Example outputs for a cert with two DNS names: | `dnsName` | listener `Name` | |-----------|-----------------| | `app.example.com` | `auto-my-app-cert-app-example-com` | -| `*.example.com` | `auto-my-app-cert-*-example-com` | +| `*.example.com` | `auto-my-app-cert-wildcard-example-com` | -The second entry is a risk: `*` is not valid in a Gateway API `SectionName` (which must match RFC 1123). The API server will reject the patch and the reconciler will requeue forever (see §10, item 7). +The listener `Hostname` keeps the original DNS name, wildcard included. The sanitization is also lossy: `a.b.com` and `a-b-com` produce the same result. If a certificate lists both, two listeners with the same name are appended; the API server rejects the patch (listener names must be unique), so no listener on that Gateway is updated. @@ -490,9 +493,9 @@ Branch (F) of Reconcile treats all Gateway `Get` errors the same (including `Not `ctrl.NewManager` at `main.go:50` does not set `LeaderElection: true`. The `deploy/deployment_gateway-cert-operator.yaml` runs `replicas: 1` which makes this safe today. Scaling to multiple replicas without enabling leader election would result in concurrent Gateway patches. **7. `listenerNameFor` edge cases** -- **Wildcard hostnames**: `*.example.com` produces `SectionName` `auto-…-*-example-com`. The `*` character is not valid in an RFC 1123 label; the Gateway API server will reject the Patch, causing an error-retry loop. -- **Collisions after sanitization**: `a.b.com` and `a-b-com` produce identical sanitized names. The same happens across certificates (cert `a` with `b-c.com` and cert `a-b` with `c.com`) and across namespaces, since the name omits the cert namespace. Both listeners are appended and the API server rejects the patch for the whole Gateway. -- **Truncation at 253 bytes**: `patch.go:147` slices on bytes, not runes (safe for ASCII DNS names). Two DNS names sharing the first 253 characters after sanitization will map to the same listener name. +- **Wildcard hostnames** (fixed): `*` used to end up in the listener name and the API server rejected the patch. It is now spelled out as `wildcard`. +- **Collisions after sanitization**: `a.b.com` and `a-b-com` produce identical sanitized names, as do `*.x.com` and `wildcard.x.com`. The same happens across certificates (cert `a` with `b-c.com` and cert `a-b` with `c.com`) and across namespaces, since the name omits the cert namespace. Both listeners are appended and the API server rejects the patch for the whole Gateway. +- **Truncation at 253 bytes** (fixed): long names are cut and get a hash of the full name appended, so they stay valid and distinct. **Dead symbols in `patch.go`** `gatewayGroup` and `gatewayKind` are declared at `patch.go:16-17` but never referenced. They can be removed without effect. diff --git a/docs/plans-executions/2026-10-08-2046-listener-name-sanitize.md b/docs/plans-executions/2026-10-08-2046-listener-name-sanitize.md index d0107a8..8080a53 100644 --- a/docs/plans-executions/2026-10-08-2046-listener-name-sanitize.md +++ b/docs/plans-executions/2026-10-08-2046-listener-name-sanitize.md @@ -3,7 +3,32 @@ Plan: `docs/plans/2026-10-08-2046-listener-name-sanitize.md` - [x] Step 0 — Save and commit the plan -- [ ] Step 1 — Regression tests first -- [ ] Step 2 — Fix -- [ ] Step 3 — Docs +- [x] Step 1 — Regression tests first +- [x] Step 2 — Fix +- [x] Step 3 — Docs - [ ] Step 4 — Ship + +## Steps 1–3 — Tests, fix, docs + +Tests were written first and failed on the old code (wildcard, uppercase, `_`, +trailing dot, long names), then passed after the change to `listenerNameFor`. + +``` +$ go test -race -run 'TestListenerNameFor|TestBuildListeners' -v ./internal/gateway/ +--- PASS: TestListenerNameFor_validSectionName/wildcard_label_is_spelled_out +--- PASS: TestListenerNameFor_longNames +--- PASS: TestBuildListenersFromCertificates_wildcard +$ make check +ok .../internal/gateway coverage: 98.6% of statements +``` + +Deviations from the plan: + +- A trailing `-` or `.` is trimmed from the whole name, not only from the DNS part, + so a DNS name that sanitizes to nothing cannot leave the name ending in `-`. +- One extra case: a trailing dot (`app.example.com.`) is dropped. +- Steps 1–3 are one commit, since the tests do not pass without the fix. + +Worth noting: names that were valid before are unchanged, so existing listeners are +not renamed. Not verified against a cluster; the production case is covered by the +test using the exact cert and DNS name from the error log. diff --git a/internal/gateway/patch.go b/internal/gateway/patch.go index 54c4b39..8226c63 100644 --- a/internal/gateway/patch.go +++ b/internal/gateway/patch.go @@ -1,6 +1,8 @@ package gateway import ( + "crypto/sha256" + "encoding/hex" "fmt" "sort" "strings" @@ -132,16 +134,38 @@ func listenerMap(listeners []gwapiv1.Listener) map[string]listenerKey { return m } +// maxSectionNameLen is the Gateway API limit for listener names. +const maxSectionNameLen = 253 + // listenerNameFor generates a deterministic listener name from cert name and DNS name. -// Truncates to stay within Gateway API's 253-char limit for section names. +// The result is a valid Gateway API SectionName: names that would exceed the limit +// are cut and get a hash of the full name appended, so distinct long names stay distinct. func listenerNameFor(prefix, certName, dnsName string) string { - // Replace dots with dashes for DNS names, combine with cert name. - sanitized := strings.ReplaceAll(dnsName, ".", "-") - name := fmt.Sprintf("%s%s-%s", prefix, certName, sanitized) - if len(name) > 253 { - name = name[:253] + name := strings.TrimRight(fmt.Sprintf("%s%s-%s", prefix, certName, sanitizeDNSName(dnsName)), "-.") + if len(name) <= maxSectionNameLen { + return name } - return name + sum := sha256.Sum256([]byte(name)) + suffix := "-" + hex.EncodeToString(sum[:4]) + return strings.TrimRight(name[:maxSectionNameLen-len(suffix)], "-.") + suffix +} + +// sanitizeDNSName maps a certificate DNS name onto the characters allowed in a +// listener name. A wildcard label is spelled out so "*.example.com" and +// "example.com" stay distinct. +func sanitizeDNSName(dnsName string) string { + var b strings.Builder + for _, r := range strings.ToLower(dnsName) { + switch { + case r >= 'a' && r <= 'z', r >= '0' && r <= '9': + b.WriteRune(r) + case r == '*': + b.WriteString("wildcard") + default: + b.WriteByte('-') + } + } + return strings.Trim(b.String(), "-") } func ptrTo[T any](v T) *T { diff --git a/internal/gateway/patch_test.go b/internal/gateway/patch_test.go index 5da7542..bbe1707 100644 --- a/internal/gateway/patch_test.go +++ b/internal/gateway/patch_test.go @@ -1,6 +1,8 @@ package gateway import ( + "regexp" + "strings" "testing" "time" @@ -272,7 +274,20 @@ func TestListenersEqual_managedFields(t *testing.T) { } } -func TestListenerNameFor_sanitizesDots(t *testing.T) { +// sectionNamePattern is the Gateway API SectionName validation rule. +var sectionNamePattern = regexp.MustCompile(`^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$`) + +func assertValidSectionName(t *testing.T, name string) { + t.Helper() + if len(name) > 253 { + t.Errorf("name is %d chars, limit is 253", len(name)) + } + if !sectionNamePattern.MatchString(name) { + t.Errorf("name %q is not a valid SectionName", name) + } +} + +func TestListenerNameFor_validSectionName(t *testing.T) { t.Parallel() tests := []struct { @@ -284,14 +299,57 @@ func TestListenerNameFor_sanitizesDots(t *testing.T) { {name: "dots become dashes", certName: "web", dnsName: "app.example.com", want: "auto-web-app-example-com"}, {name: "single label", certName: "web", dnsName: "localhost", want: "auto-web-localhost"}, {name: "existing dashes are kept", certName: "my-cert", dnsName: "my-app.example.com", want: "auto-my-cert-my-app-example-com"}, + { + name: "wildcard label is spelled out", + certName: "wildcard-home-hrajfrisbee", + dnsName: "*.home.hrajfrisbee.cz", + want: "auto-wildcard-home-hrajfrisbee-wildcard-home-hrajfrisbee-cz", + }, + {name: "uppercase is lowered", certName: "web", dnsName: "App.Example.COM", want: "auto-web-app-example-com"}, + {name: "other characters become dashes", certName: "web", dnsName: "my_app.example.com", want: "auto-web-my-app-example-com"}, + {name: "trailing dot is dropped", certName: "web", dnsName: "app.example.com.", want: "auto-web-app-example-com"}, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { t.Parallel() - if got := listenerNameFor(testPrefix, tc.certName, tc.dnsName); got != tc.want { + got := listenerNameFor(testPrefix, tc.certName, tc.dnsName) + if got != tc.want { t.Errorf("listenerNameFor() = %q, want %q", got, tc.want) } + assertValidSectionName(t, got) }) } } + +func TestListenerNameFor_longNames(t *testing.T) { + t.Parallel() + + // 62 dashes right where the name is cut, so a blind cut would end in "-". + shared := strings.Repeat("a", 200) + strings.Repeat("-", 62) + first := listenerNameFor(testPrefix, "web", shared+"x.example.com") + second := listenerNameFor(testPrefix, "web", shared+"y.example.com") + + assertValidSectionName(t, first) + assertValidSectionName(t, second) + if first == second { + t.Errorf("two long DNS names sharing a prefix got the same listener name %q", first) + } + if again := listenerNameFor(testPrefix, "web", shared+"x.example.com"); again != first { + t.Errorf("name is not deterministic: %q then %q", first, again) + } +} + +func TestBuildListenersFromCertificates_wildcard(t *testing.T) { + t.Parallel() + + got := BuildListenersFromCertificates( + []cmv1.Certificate{cert("default", "wild", "wild-tls", "*.example.com")}, testPrefix) + if len(got) != 1 { + t.Fatalf("got %d listeners, want 1", len(got)) + } + assertValidSectionName(t, string(got[0].Name)) + if got[0].Hostname == nil || *got[0].Hostname != "*.example.com" { + t.Errorf("hostname = %v, want the wildcard kept as *.example.com", got[0].Hostname) + } +} -- 2.54.0 From 5c4a148b36bca7d538f72468c0b52f40311929d7 Mon Sep 17 00:00:00 2001 From: Jan Novak <kacerr.cz@gmail.com> Date: Thu, 8 Oct 2026 20:47:53 +0200 Subject: [PATCH 11/11] docs: close out the conventions and listener-name plans Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --- .../2026-10-08-2028-house-conventions.md | 7 ++++--- .../2026-10-08-2046-listener-name-sanitize.md | 9 ++++++++- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/docs/plans-executions/2026-10-08-2028-house-conventions.md b/docs/plans-executions/2026-10-08-2028-house-conventions.md index cdac887..bbfe425 100644 --- a/docs/plans-executions/2026-10-08-2028-house-conventions.md +++ b/docs/plans-executions/2026-10-08-2028-house-conventions.md @@ -9,7 +9,7 @@ Plan: `docs/plans/2026-10-08-2028-house-conventions.md` - [x] Step 4 — Module path and version identity - [x] Step 5 — Baseline tests - [x] Step 6 — CI build workflow -- [ ] Step 7 — Close out +- [x] Step 7 — Close out ## Step 1 — CLAUDE.md @@ -151,5 +151,6 @@ bugs deferred to the bug-fix phase, one test simplification declined. Worth noting: the reviewer agents were not registered as agent types when the review started (they were added in this session), so they were run as general-purpose agents told to follow the agent files. The `:latest` dispatch hole came in with the -workflow copied from egress and is still open there. Step 7 stays unchecked until -the branch is pushed and the MR is open. +workflow copied from egress and is still open there. Pushed together with the listener-name fix +(`docs/plans/2026-10-08-2046-listener-name-sanitize.md`) as one MR. The CHANGELOG +entry is still owed, after the owner confirms it works. diff --git a/docs/plans-executions/2026-10-08-2046-listener-name-sanitize.md b/docs/plans-executions/2026-10-08-2046-listener-name-sanitize.md index 8080a53..3aa6e5f 100644 --- a/docs/plans-executions/2026-10-08-2046-listener-name-sanitize.md +++ b/docs/plans-executions/2026-10-08-2046-listener-name-sanitize.md @@ -6,7 +6,7 @@ Plan: `docs/plans/2026-10-08-2046-listener-name-sanitize.md` - [x] Step 1 — Regression tests first - [x] Step 2 — Fix - [x] Step 3 — Docs -- [ ] Step 4 — Ship +- [x] Step 4 — Ship ## Steps 1–3 — Tests, fix, docs @@ -32,3 +32,10 @@ Deviations from the plan: Worth noting: names that were valid before are unchanged, so existing listeners are not renamed. Not verified against a cluster; the production case is covered by the test using the exact cert and DNS name from the error log. + +## Step 4 — Ship + +Pushed `feat/house-conventions` and opened one MR covering the conventions port and +this fix. Still owed after merge: tag `v0.1.0` with `/release`, restart the +deployment (it pins `:latest`), confirm the reconcile error is gone, then the +CHANGELOG entries. -- 2.54.0