fix(charts): filter default-route discovery helpers to IPv4 family - #145
Conversation
Add two failing tests covering the dual-stack regression:
- TestDefaultGatewayIsIPv4OnDualStack: helper-level assertion that
talm.discovered.default_gateway returns the IPv4 default-route gateway
on a node with both IPv4 and IPv6 default routes (IPv6 ordered first
in discovery, mirroring real Hetzner-style nodes).
- TestCozystackChartRendersIPv4GatewayOnDualStack: end-to-end assertion
on the cozystack v1.12 multi-doc chart output. Both LinkConfig.routes
gateway and LinkConfig.addresses must use the IPv4 default-route data
even when an IPv6 default route also exists.
Both fail today: default_gateway and default_addresses_by_gateway both
iterate routes and break at the first default route without filtering
by family, so an IPv6-first discovery yields:
- gateway: fe80::1 paired with an implicit IPv4 destination, which
Talos rejects as a malformed route (the headline VIP-not-addressable
symptom users hit).
- addresses: empty, because the IPv6 family selected by the first
default route filters out the node's IPv4 addresses, leaving the
primary NIC unconfigured.
Add a dualStackNicLookup() fixture next to simpleNicLookup() so the
chart-rendering test path can exercise dual-stack discovery without
disturbing the existing IPv4-only fixtures the rest of the suite
depends on.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
talm.discovered.default_gateway and talm.discovered.default_addresses_by_gateway
now skip routes whose family is not inet4. The chart consumers (cozystack
and generic) pair the returned gateway with a hardcoded IPv4 destination
(network: 0.0.0.0/0 on the legacy schema, or no network field on the typed
RouteConfig schema where Talos defaults to IPv4), and the addresses helper
filters per-link addresses by the route's family.
Without the filter, a node with both IPv4 and IPv6 default routes (the
typical Hetzner / dual-stack setup) ends up with:
- gateway: <ipv6 link-local> in a route block whose destination is
IPv4 — Talos rejects the malformed entry and dependent features
(Layer2 VIP, default routing through the rendered chart config)
silently break.
- addresses: empty in the rendered LinkConfig because the helper
inherits the IPv6 family from the first iterated default route and
the address filter then drops every IPv4 entry on the link.
The IPv4-only filter is symmetric with gateway_by_link, which already
documents "IPv4-only by convention to avoid family/address mismatch on
dual-stack nodes". Doc comments updated to spell out the chain so a
future maintainer who adds an IPv6-aware variant for new typed routes
does not silently widen the existing helpers and re-introduce the
regression.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
📝 WalkthroughWalkthroughThis PR updates Helm template helpers to explicitly prefer IPv4 default routes in dual-stack network environments, supported by comprehensive regression tests and clarified configuration documentation for VIP and subnet selection behavior. ChangesIPv4-Prioritized Dual-Stack Network Discovery
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request ensures that network discovery helpers in the talm chart specifically target IPv4 routes by adding an 'inet4' family filter. This change prevents configuration issues on dual-stack nodes where IPv6 routes might be incorrectly selected. The PR also updates documentation in values.yaml and adds extensive unit tests with dual-stack fixtures. Feedback suggests refactoring duplicated test setup logic into a shared helper function to improve maintainability.
| helpersSrc, err := os.ReadFile("../../charts/talm/templates/_helpers.tpl") | ||
| if err != nil { | ||
| t.Fatalf("read helpers: %v", err) | ||
| } | ||
| if err := os.WriteFile(filepath.Join(chartRoot, "templates", "_helpers.tpl"), helpersSrc, 0o644); err != nil { | ||
| t.Fatalf("write vendored helpers: %v", err) | ||
| } |
There was a problem hiding this comment.
The code to create a test chart and vendor the _helpers.tpl file is duplicated in TestDefaultGatewayIsIPv4OnDualStack and TestDefaultLinkHelpersFollowIPv4OnTwoNicDualStack. To avoid repetition and improve test maintainability, you could extract this logic into a new test helper function.
For example, you could create a function like createTestChartWithTalmHelpers that encapsulates this setup, and then call it from both tests.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
pkg/engine/render_test.go (1)
3581-3788: 💤 Low valueFixtures look clean, but consider extracting a small builder to cut duplication.
dualStackNicLookupanddualStackTwoNicsLookupeach repeat the same boilerplate (linksList,addressesList,nodeDefault,resolvers, theswitch resourcedispatcher) that already appears insimpleNicLookupandsecondaryNicLookup. As more fixtures land (multi-link, IPv6-only, etc.) this will keep growing. Optional: introduce a small builder (e.g.lookupBuilder{links, addresses, routes, nodeAddress, resolvers}.Build()) so each fixture only declares the data it cares about. Not a blocker — purely a maintainability nit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/engine/render_test.go` around lines 3581 - 3788, dualStackNicLookup and dualStackTwoNicsLookup duplicate boilerplate that appears in simpleNicLookup and secondaryNicLookup; refactor by introducing a small fixture builder (e.g. a lookupBuilder type with fields links, addresses, routes, nodeAddress, resolvers and a Build() method) and update dualStackNicLookup, dualStackTwoNicsLookup (and optionally simpleNicLookup/secondaryNicLookup) to call lookupBuilder.Build() and only supply the differing pieces (routes, eth0/eth1 definitions) so the shared linksList/addressesList/nodeDefault/resolvers and the switch resource dispatcher are centralized in the builder.charts/talm/templates/_helpers.tpl (2)
102-121: 💤 Low value
$familyis now effectively constant — consider hardcoding for clarity.After the route filter narrows to
family=inet4,$familycan only be"inet4"(or""when no IPv4 default exists, in which case$linkNameis also""and the address-filter loop produces an empty list either way). The two-stage capture-and-filter pattern made sense when the route filter was family-agnostic; now it just adds indirection. Either inline"inet4"or drop a comment explaining why the variable is kept (e.g. "future-proofing for an IPv6-aware sibling helper").♻️ Optional simplification
{{- define "talm.discovered.default_addresses_by_gateway" }} {{- $linkName := "" }} -{{- $family := "" }} {{- range (lookup "routes" "" "").items }} {{- if and (eq .spec.dst "") (not (eq .spec.gateway "")) (eq .spec.table "main") (eq (.spec.family | toString) "inet4") }} {{- $linkName = .spec.outLinkName }} -{{- $family = .spec.family }} {{- break }} {{- end }} {{- end }} {{- $addresses := list }} {{- range (lookup "addresses" "" "").items }} -{{- if and (eq .spec.linkName $linkName) (eq .spec.family $family) (not (eq .spec.scope "host")) }} +{{- if and (ne $linkName "") (eq .spec.linkName $linkName) (eq (.spec.family | toString) "inet4") (not (eq .spec.scope "host")) }} {{- if not (hasPrefix (printf "%s/" $.Values.floatingIP) .spec.address) }} {{- $addresses = append $addresses .spec.address }} {{- end }} {{- end }} {{- end }} {{- toJson $addresses }} {{- end }}The added
(ne $linkName "")guard also closes a small defensive gap: when no IPv4 default route exists, the previous code matched any address with emptylinkName/familyfields (rare in practice, but possible with malformed mocks).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@charts/talm/templates/_helpers.tpl` around lines 102 - 121, The helper talm.discovered.default_addresses_by_gateway currently captures $family from a route match but that value is effectively always "inet4"; simplify by hardcoding the family check to "inet4" (remove the $family capture/assignment and use the literal "inet4" when filtering addresses) or, if you intentionally want to keep the indirection, add a brief comment above $family explaining it's retained for future IPv6 support; update the address-filtering condition (which currently compares .spec.family to $family) to compare to the literal "inet4" when hardcoding, and keep the existing (ne $linkName "") defensive check to avoid matching empty linkName cases.
156-216: 💤 Low valueFive helpers repeat the same default-route guard — consider extracting.
The clause
(eq .spec.dst "") (not (eq .spec.gateway "")) (eq .spec.table "main") (eq (.spec.family | toString) "inet4")is duplicated acrossdefault_link_name_by_gateway,default_link_address_by_gateway,default_link_bus_by_gateway,default_link_selector_by_gateway, anddefault_gateway. A future change (e.g. honoring route priority, supporting an alternate table) has to land in five places. Since all five helpers also fundamentally need the same matching route, one option is to compute the IPv4 default route'soutLinkNameandgatewayonce into a tiny helper and have the link-attribute helpers dispatch off that.Not blocking — Helm template factoring is awkward and the duplication is shallow — but worth flagging while the surface is fresh.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@charts/talm/templates/_helpers.tpl` around lines 156 - 216, Extract the repeated default-route guard into a single helper (e.g. "talm.discovered.ipv4_default_route") that runs the range over (lookup "routes" "" "").items and returns the matching route object (the one satisfying (eq .spec.dst "") (not (eq .spec.gateway "")) (eq .spec.table "main") (eq (.spec.family | toString) "inet4") ) or empty; then change the five helpers talm.discovered.default_link_name_by_gateway, talm.discovered.default_link_address_by_gateway, talm.discovered.default_link_bus_by_gateway, talm.discovered.default_link_selector_by_gateway and talm.discovered.default_gateway to call that new helper (use with/if to guard empty) and derive .spec.outLinkName/.spec.gateway or lookup "links" "" <outLinkName>.spec.hardwareAddr / .spec.busPath from the single returned route instead of repeating the guard logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@charts/talm/templates/_helpers.tpl`:
- Around line 102-121: The helper talm.discovered.default_addresses_by_gateway
currently captures $family from a route match but that value is effectively
always "inet4"; simplify by hardcoding the family check to "inet4" (remove the
$family capture/assignment and use the literal "inet4" when filtering addresses)
or, if you intentionally want to keep the indirection, add a brief comment above
$family explaining it's retained for future IPv6 support; update the
address-filtering condition (which currently compares .spec.family to $family)
to compare to the literal "inet4" when hardcoding, and keep the existing (ne
$linkName "") defensive check to avoid matching empty linkName cases.
- Around line 156-216: Extract the repeated default-route guard into a single
helper (e.g. "talm.discovered.ipv4_default_route") that runs the range over
(lookup "routes" "" "").items and returns the matching route object (the one
satisfying (eq .spec.dst "") (not (eq .spec.gateway "")) (eq .spec.table "main")
(eq (.spec.family | toString) "inet4") ) or empty; then change the five helpers
talm.discovered.default_link_name_by_gateway,
talm.discovered.default_link_address_by_gateway,
talm.discovered.default_link_bus_by_gateway,
talm.discovered.default_link_selector_by_gateway and
talm.discovered.default_gateway to call that new helper (use with/if to guard
empty) and derive .spec.outLinkName/.spec.gateway or lookup "links" ""
<outLinkName>.spec.hardwareAddr / .spec.busPath from the single returned route
instead of repeating the guard logic.
In `@pkg/engine/render_test.go`:
- Around line 3581-3788: dualStackNicLookup and dualStackTwoNicsLookup duplicate
boilerplate that appears in simpleNicLookup and secondaryNicLookup; refactor by
introducing a small fixture builder (e.g. a lookupBuilder type with fields
links, addresses, routes, nodeAddress, resolvers and a Build() method) and
update dualStackNicLookup, dualStackTwoNicsLookup (and optionally
simpleNicLookup/secondaryNicLookup) to call lookupBuilder.Build() and only
supply the differing pieces (routes, eth0/eth1 definitions) so the shared
linksList/addressesList/nodeDefault/resolvers and the switch resource dispatcher
are centralized in the builder.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 023f224c-0b70-48c6-81dc-08e0318f5244
📒 Files selected for processing (4)
charts/cozystack/values.yamlcharts/generic/values.yamlcharts/talm/templates/_helpers.tplpkg/engine/render_test.go
2 participants
What changed
All six
default_*_by_gatewayhelpers incharts/talm/templates/_helpers.tplnow filter routes tofamily == inet4. Previously they iteratedlookup "routes"and broke at the first default route, so on a dual-stack node where IPv6 was discovered first the chart would emit:gateway: <ipv6 link-local>paired with an IPv4 destination (network: 0.0.0.0/0on the legacy schema, or nonetwork:field on the typed RouteConfig schema where Talos derives family from the literal) — Talos rejects the malformed entry, Layer2 VIP and other routing features that depend on the chart-emitted route silently break.addresses:empty in the rendered LinkConfig because the helper inherited the IPv6 family from the first-iterated default route and the address filter then dropped every IPv4 entry on the link.LinkConfig name:would attach to the IPv6-default link while addresses/gateway described the IPv4-default link — neither NIC ended up correctly configured.Why
The IPv4-only filter is symmetric with the pre-existing
gateway_by_linkhelper, which already documents "IPv4-only by convention to avoid family/address mismatch on dual-stack nodes". The chart consumers all pair these helpers with IPv4 destinations, so widening the existing helpers to non-IPv4 routes was always inconsistent with how they're used.For IPv6-only setups: the helper now returns empty for
default_gateway(documented), and operators can pinadvertisedSubnetsexplicitly via values.yaml (the comment was extended to call this out).Tests
TestDefaultGatewayIsIPv4OnDualStack— helper-level pin ondefault_gatewaywith IPv6-first ordering.TestDefaultLinkHelpersFollowIPv4OnTwoNicDualStack— multi-NIC scenario: IPv4 and IPv6 default routes on different links.TestCozystackChartRendersIPv4GatewayOnDualStack— end-to-end through cozystack and generic charts on the v1.12 multi-doc schema, including a multi-NIC subtest.Closes #78.
Summary by CodeRabbit
Documentation
New Features
Improvements
Tests