From 44cbb74707c9701480c48bac5e3f39aaa42d13aa Mon Sep 17 00:00:00 2001 From: Shayan Namaghi Date: Mon, 17 Aug 2026 13:18:02 -0700 Subject: [PATCH 1/2] fix(nico-dhcp): pin DHCP server identifier (option 54) to the stable VIP MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Kea defaults the server identifier to the pod IP, which is unreachable from BMCs. After a nico-dhcp pod replacement, leased BMCs unicast-renew against the dead pod IP and stay orphaned from DHCP and discovery until the rebind timer forces a broadcast (issue #3663). Pin option 54 to the stable DHCP VIP so unicast renewals route through the LoadBalancer and survive pod churn. Kea (>=1.9.3; the image ships 2.x from Debian 12) both advertises the configured identifier and accepts unicast REQUESTs whose option 54 matches it. Resolution order, designed so upgrades from earlier releases cannot break or change behavior unexpectedly: 1. config.kea.serverIdentifier — explicit override; render fails on a value that is not a single IPv4 address. 2. Auto-derived from the externalService metallb.universe.tf/loadBalancerIPs annotation (first entry) — sites that already pin the DHCP VIP get the fix on upgrade with no values changes. Used only when it parses as a valid IPv4; anything else falls through silently so a render can never fail from pre-existing values. 3. Neither — option-data is omitted entirely and the rendered config is byte-identical to the previous chart version (kea falls back to the pod IP as before). The raw keaConfigJsonRaw escape hatch is unaffected. Fixes #3663 --- helm-prereqs/values/nico-core.yaml | 7 +- helm/charts/nico-dhcp/templates/_helpers.tpl | 40 +++++++ .../tests/server_identifier_test.yaml | 101 ++++++++++++++++++ helm/charts/nico-dhcp/values.yaml | 18 ++++ 4 files changed, 165 insertions(+), 1 deletion(-) create mode 100644 helm/charts/nico-dhcp/tests/server_identifier_test.yaml diff --git a/helm-prereqs/values/nico-core.yaml b/helm-prereqs/values/nico-core.yaml index b39ab741e7..9c5132fe0b 100644 --- a/helm-prereqs/values/nico-core.yaml +++ b/helm-prereqs/values/nico-core.yaml @@ -377,7 +377,12 @@ nico-dhcp: externalService: enabled: true # must be explicitly enabled - defaults to false in chart annotations: - metallb.universe.tf/loadBalancerIPs: "" # REQUIRED: DHCP VIP (internal pool) + ## REQUIRED: DHCP VIP (internal pool). Besides pinning the LoadBalancer + ## IP, this VIP is also auto-advertised to DHCP clients as the server + ## identifier (option 54) so BMC unicast renewals survive nico-dhcp pod + ## replacement (issue #3663). Override the advertised identifier + ## independently via nico-dhcp.config.kea.serverIdentifier if needed. + metallb.universe.tf/loadBalancerIPs: "" ## DHCP hook libraries' advertised IPs. ## diff --git a/helm/charts/nico-dhcp/templates/_helpers.tpl b/helm/charts/nico-dhcp/templates/_helpers.tpl index 339662a5e8..8e86b49025 100644 --- a/helm/charts/nico-dhcp/templates/_helpers.tpl +++ b/helm/charts/nico-dhcp/templates/_helpers.tpl @@ -146,6 +146,38 @@ controls the YAML→JSON name mapping. {{- if hasKey $k "declineProbationPeriod" -}} {{- $declineProbationPeriod = $k.declineProbationPeriod -}} {{- end -}} +{{/* + DHCP server identifier (option 54) — pin to the stable DHCP VIP so client + unicast renewals survive pod replacement (issue #3663). Without this, kea + advertises the ephemeral pod IP: a rescheduled pod leaves every leased BMC + unicast-renewing against a dead address until the rebind timer forces a + broadcast. Resolution order: + 1. config.kea.serverIdentifier — explicit override, validated as IPv4 + (render fails on a malformed explicit value). + 2. externalService.annotations["metallb.universe.tf/loadBalancerIPs"] — + the VIP the chart already advertises; first entry when comma-separated. + Used only when it parses as a single IPv4 — a missing/odd annotation + silently falls through so upgrades from values that predate this + field can never fail at render time. + 3. Neither — the option is omitted entirely and kea falls back to its + default (pod IP), which is byte-identical to the pre-#3663 rendering. +*/}} +{{- $ipv4Pattern := "^((25[0-5]|2[0-4][0-9]|1[0-9]{2}|[1-9]?[0-9])\\.){3}(25[0-5]|2[0-4][0-9]|1[0-9]{2}|[1-9]?[0-9])$" -}} +{{- $serverIdentifier := trim (default "" $k.serverIdentifier) -}} +{{- if and $serverIdentifier (not (regexMatch $ipv4Pattern $serverIdentifier)) -}} +{{- fail (printf "nico-dhcp: config.kea.serverIdentifier must be a single IPv4 address (the stable DHCP VIP); got %q" $serverIdentifier) -}} +{{- end -}} +{{- if not $serverIdentifier -}} +{{- $es := default (dict) .Values.externalService -}} +{{- if $es.enabled -}} +{{- $esAnnotations := default (dict) $es.annotations -}} +{{- $vipAnnotation := index $esAnnotations "metallb.universe.tf/loadBalancerIPs" -}} +{{- $firstVip := trim (first (splitList "," (printf "%v" (default "" $vipAnnotation)))) -}} +{{- if regexMatch $ipv4Pattern $firstVip -}} +{{- $serverIdentifier = $firstVip -}} +{{- end -}} +{{- end -}} +{{- end -}} { "Dhcp4": { "interfaces-config": { @@ -162,6 +194,14 @@ controls the YAML→JSON name mapping. "rebind-timer": {{ default 1800 $k.rebindTimer | toJson }}, "valid-lifetime": {{ default 3600 $k.validLifetime | toJson }}, "decline-probation-period": {{ $declineProbationPeriod | toJson }}, +{{- if $serverIdentifier }} + "option-data": [ + { + "name": "dhcp-server-identifier", + "data": {{ $serverIdentifier | quote }} + } + ], +{{- end }} {{/* Hook parameters — write both nico-* and carbide-* keys with identical values so the kea hook library (crates/dhcp/src/kea/loader.cc still diff --git a/helm/charts/nico-dhcp/tests/server_identifier_test.yaml b/helm/charts/nico-dhcp/tests/server_identifier_test.yaml new file mode 100644 index 0000000000..5e766522bc --- /dev/null +++ b/helm/charts/nico-dhcp/tests/server_identifier_test.yaml @@ -0,0 +1,101 @@ +suite: DHCP server identifier (option 54) pinning — issue #3663 +templates: + - configmap.yaml +release: + namespace: nico-system +tests: + - it: omits option-data entirely when no VIP source is configured (pre-#3663 rendering) + asserts: + - notMatchRegex: + path: data["kea_config.json"] + pattern: 'dhcp-server-identifier' + - notMatchRegex: + path: data["kea_config.json"] + pattern: '"option-data"' + + - it: derives the server identifier from the externalService loadBalancerIPs annotation + set: + externalService: + enabled: true + annotations: + metallb.universe.tf/loadBalancerIPs: "10.180.126.161" + asserts: + - matchRegex: + path: data["kea_config.json"] + pattern: '"name": "dhcp-server-identifier"' + - matchRegex: + path: data["kea_config.json"] + pattern: '"data": "10.180.126.161"' + + - it: uses the first entry of a comma-separated loadBalancerIPs annotation + set: + externalService: + enabled: true + annotations: + metallb.universe.tf/loadBalancerIPs: "10.180.126.161, 10.180.126.162" + asserts: + - matchRegex: + path: data["kea_config.json"] + pattern: '"data": "10.180.126.161"' + + - it: does not derive from the annotation when externalService is disabled + set: + externalService: + enabled: false + annotations: + metallb.universe.tf/loadBalancerIPs: "10.180.126.161" + asserts: + - notMatchRegex: + path: data["kea_config.json"] + pattern: 'dhcp-server-identifier' + + - it: silently omits the option when the annotation is not a valid IPv4 (upgrade-safe) + set: + externalService: + enabled: true + annotations: + metallb.universe.tf/loadBalancerIPs: "not-an-ip" + asserts: + - notMatchRegex: + path: data["kea_config.json"] + pattern: 'dhcp-server-identifier' + + - it: silently omits the option when the annotation is empty (upgrade-safe) + set: + externalService: + enabled: true + annotations: + metallb.universe.tf/loadBalancerIPs: "" + asserts: + - notMatchRegex: + path: data["kea_config.json"] + pattern: 'dhcp-server-identifier' + + - it: explicit serverIdentifier takes precedence over the annotation + set: + config.kea.serverIdentifier: "10.180.126.170" + externalService: + enabled: true + annotations: + metallb.universe.tf/loadBalancerIPs: "10.180.126.161" + asserts: + - matchRegex: + path: data["kea_config.json"] + pattern: '"data": "10.180.126.170"' + - notMatchRegex: + path: data["kea_config.json"] + pattern: '10\.180\.126\.161' + + - it: rejects an explicit serverIdentifier that is not a single IPv4 address + set: + config.kea.serverIdentifier: "10.180.126.161,10.180.126.162" + asserts: + - failedTemplate: + errorPattern: 'config\.kea\.serverIdentifier must be a single IPv4 address' + + - it: rejects an explicit serverIdentifier with an out-of-range octet + set: + config.kea.serverIdentifier: "10.180.126.256" + asserts: + - failedTemplate: + errorPattern: 'config\.kea\.serverIdentifier must be a single IPv4 address' diff --git a/helm/charts/nico-dhcp/values.yaml b/helm/charts/nico-dhcp/values.yaml index 71ff522dff..60e3b04af1 100644 --- a/helm/charts/nico-dhcp/values.yaml +++ b/helm/charts/nico-dhcp/values.yaml @@ -129,6 +129,24 @@ config: validLifetime: 3600 declineProbationPeriod: 900 + ## DHCP server identifier (option 54) advertised to clients — issue #3663. + ## + ## Kea defaults option 54 to the POD IP, which is unreachable from BMCs. + ## After a pod replacement, clients unicast-renew against the dead pod IP + ## and stay orphaned until the rebind timer (rebindTimer above) forces a + ## broadcast. Pinning option 54 to the stable DHCP VIP makes unicast + ## renewals survive pod churn. + ## + ## Leave empty (default) to AUTO-DERIVE from the first entry of the + ## externalService `metallb.universe.tf/loadBalancerIPs` annotation — + ## sites that already pin the DHCP VIP there get the fix with no values + ## change. Set explicitly only when the advertised identifier must differ + ## from that annotation (e.g. externalService managed outside this chart). + ## An explicit value must be a single IPv4 address (render fails + ## otherwise); when neither source yields a valid IPv4 the option is + ## omitted and kea falls back to the pod IP (pre-#3663 behavior). + serverIdentifier: "" + ## libdhcp hook parameters — these become the JSON object at ## Dhcp4.hooks-libraries[0].parameters and are read by the ## NICo-specific kea hook (libdhcp.so). From dede7f10096cf52457ce065ce55f8cdcf5469235 Mon Sep 17 00:00:00 2001 From: Shayan Namaghi Date: Mon, 17 Aug 2026 13:32:49 -0700 Subject: [PATCH 2/2] fix(nico-dhcp): harden serverIdentifier validation and raw-config conflict MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two follow-ups from review: - Coerce non-string serverIdentifier overrides (bare numbers, maps) to a string before trimming so they hit the friendly IPv4-validation failure instead of erroring inside sprig's trim with an opaque type message. - Fail at render time when config.kea.serverIdentifier is set together with the config.keaConfigJsonRaw escape hatch. The raw blob skips the structured block entirely, so the override would be silently ignored — and the operator would believe the #3663 pinning is active when it is not. The values docs now also spell out that raw-blob users must add the dhcp-server-identifier option-data themselves. --- helm/charts/nico-dhcp/templates/_helpers.tpl | 5 +++- .../charts/nico-dhcp/templates/configmap.yaml | 8 +++++ .../tests/server_identifier_test.yaml | 30 +++++++++++++++++++ helm/charts/nico-dhcp/values.yaml | 7 +++++ 4 files changed, 49 insertions(+), 1 deletion(-) diff --git a/helm/charts/nico-dhcp/templates/_helpers.tpl b/helm/charts/nico-dhcp/templates/_helpers.tpl index 8e86b49025..5f66fd8faf 100644 --- a/helm/charts/nico-dhcp/templates/_helpers.tpl +++ b/helm/charts/nico-dhcp/templates/_helpers.tpl @@ -163,7 +163,10 @@ controls the YAML→JSON name mapping. default (pod IP), which is byte-identical to the pre-#3663 rendering. */}} {{- $ipv4Pattern := "^((25[0-5]|2[0-4][0-9]|1[0-9]{2}|[1-9]?[0-9])\\.){3}(25[0-5]|2[0-4][0-9]|1[0-9]{2}|[1-9]?[0-9])$" -}} -{{- $serverIdentifier := trim (default "" $k.serverIdentifier) -}} +{{/* printf "%v" coerces non-string overrides (bare numbers, maps) to a string + so they reach the friendly IPv4-validation fail below instead of erroring + inside sprig's trim with an opaque type message. */}} +{{- $serverIdentifier := trim (printf "%v" (default "" $k.serverIdentifier)) -}} {{- if and $serverIdentifier (not (regexMatch $ipv4Pattern $serverIdentifier)) -}} {{- fail (printf "nico-dhcp: config.kea.serverIdentifier must be a single IPv4 address (the stable DHCP VIP); got %q" $serverIdentifier) -}} {{- end -}} diff --git a/helm/charts/nico-dhcp/templates/configmap.yaml b/helm/charts/nico-dhcp/templates/configmap.yaml index 7e08035af5..5f30d78d60 100644 --- a/helm/charts/nico-dhcp/templates/configmap.yaml +++ b/helm/charts/nico-dhcp/templates/configmap.yaml @@ -16,6 +16,14 @@ {{- if and (hasKey .Values.config "keaConfigJsonRaw") .Values.config.keaConfigJsonRaw (not (kindIs "string" .Values.config.keaConfigJsonRaw)) }} {{- fail (printf "nico-dhcp: `config.keaConfigJsonRaw` must be a string (got %s). Set to a complete kea_config.json blob, or leave empty/omit to use the structured `config.kea` block." (kindOf .Values.config.keaConfigJsonRaw)) }} {{- end }} +{{- /* Conflict guard: the raw escape hatch skips the structured `config.kea` + block entirely, including the server-identifier pinning (issue #3663). + An explicitly set `config.kea.serverIdentifier` alongside a raw blob + would be silently ignored — fail loudly so the operator either drops + the structured override or adds the option-data to the raw JSON. */ -}} +{{- if and (trim (default "" .Values.config.keaConfigJsonRaw)) (trim (printf "%v" (dig "kea" "serverIdentifier" "" .Values.config))) }} +{{- fail "nico-dhcp: `config.kea.serverIdentifier` is ignored when `config.keaConfigJsonRaw` is set. Remove the structured override, or add the dhcp-server-identifier option-data to the raw kea_config.json blob (see issue #3663)." }} +{{- end }} apiVersion: v1 kind: ConfigMap metadata: diff --git a/helm/charts/nico-dhcp/tests/server_identifier_test.yaml b/helm/charts/nico-dhcp/tests/server_identifier_test.yaml index 5e766522bc..ac20e27ab4 100644 --- a/helm/charts/nico-dhcp/tests/server_identifier_test.yaml +++ b/helm/charts/nico-dhcp/tests/server_identifier_test.yaml @@ -99,3 +99,33 @@ tests: asserts: - failedTemplate: errorPattern: 'config\.kea\.serverIdentifier must be a single IPv4 address' + + - it: rejects a non-string serverIdentifier with the friendly IPv4 message + set: + config.kea.serverIdentifier: 10 + asserts: + - failedTemplate: + errorPattern: 'config\.kea\.serverIdentifier must be a single IPv4 address' + + - it: rejects serverIdentifier combined with the raw config escape hatch + set: + config.kea.serverIdentifier: "10.180.126.170" + config.keaConfigJsonRaw: '{"Dhcp4": {}}' + asserts: + - failedTemplate: + errorPattern: 'serverIdentifier` is ignored when `config\.keaConfigJsonRaw` is set' + + - it: raw config escape hatch alone renders without the pinning applied + set: + config.keaConfigJsonRaw: '{"Dhcp4": {}}' + externalService: + enabled: true + annotations: + metallb.universe.tf/loadBalancerIPs: "10.180.126.161" + asserts: + - notMatchRegex: + path: data["kea_config.json"] + pattern: 'dhcp-server-identifier' + - matchRegex: + path: data["kea_config.json"] + pattern: 'Dhcp4' diff --git a/helm/charts/nico-dhcp/values.yaml b/helm/charts/nico-dhcp/values.yaml index 60e3b04af1..5b722e8e9b 100644 --- a/helm/charts/nico-dhcp/values.yaml +++ b/helm/charts/nico-dhcp/values.yaml @@ -99,6 +99,13 @@ config: ## Note: placeholder substitution applies ONLY to this raw string, NOT ## to values inside `config.kea` below — set those values to fully ## resolved strings (or leave empty to use the helper's derivation). + ## + ## ⚠ The server-identifier pinning (issue #3663, `config.kea.serverIdentifier` + ## below) does NOT apply to a raw blob. Include the equivalent option-data + ## in the raw JSON yourself, or unicast renewals will target the ephemeral + ## pod IP and orphan clients on pod replacement. Setting + ## `config.kea.serverIdentifier` together with a raw blob fails at render + ## time to make this conflict loud. keaConfigJsonRaw: "" ## Structured kea-dhcp4 configuration. The configmap template assembles