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..5f66fd8faf 100644 --- a/helm/charts/nico-dhcp/templates/_helpers.tpl +++ b/helm/charts/nico-dhcp/templates/_helpers.tpl @@ -146,6 +146,41 @@ 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])$" -}} +{{/* 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 -}} +{{- 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 +197,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/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 new file mode 100644 index 0000000000..ac20e27ab4 --- /dev/null +++ b/helm/charts/nico-dhcp/tests/server_identifier_test.yaml @@ -0,0 +1,131 @@ +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' + + - 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 71ff522dff..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 @@ -129,6 +136,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).