From 42b53b753832898a2213a69222c587f6b65d05be Mon Sep 17 00:00:00 2001 From: Jan Novak Date: Thu, 8 Oct 2026 20:47:44 +0200 Subject: [PATCH] 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 --- 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) + } +}