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>
2026-10-08 20:43:02 +02:00
parent 8c074288c6
commit 60360ca5b9
10 changed files with 138 additions and 40 deletions
@@ -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
@@ -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.
@@ -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}" \
@@ -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)
@@ -35,27 +35,35 @@ Executed once at process start (`main.go`):
 ```
 process start
    │
-    ├─ init()                                                [main.go:22-26]
+    └─ main()                                               [main.go:24]
+         │
+         ├─ 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]
+         │
+         ├─ --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
         │
-    └─ 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.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.
@@ -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.
@@ -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`.
@@ -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("")),
@@ -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()

@@ -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())