Skip to content

fix(network): correct multi-NIC discovery and add per-link helpers #127

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kvaps merged 9 commits into from
Apr 28, 2026
Merged

fix(network): correct multi-NIC discovery and add per-link helpers #127

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f8d4320
fix(helpers): make default_*_by_gateway return a single deterministic…
lexfrei Apr 16, 2026
9604b2a
fix(network): skip empty interface emission when no default link disc…
lexfrei Apr 16, 2026
07b1bfc
feat(helpers): add per-link discovery helpers for multi-NIC templates
lexfrei Apr 16, 2026
d461e81
test(engine): cover #108, #58, #125 regression scenarios
lexfrei Apr 16, 2026
c795703
fix(helpers): tighten per-link helper output (review feedback)
lexfrei Apr 16, 2026
14f4ab6
fix(helpers): address review round 2
lexfrei Apr 16, 2026
5fde4c7
chore: ignore .claude/worktrees
lexfrei Apr 16, 2026
fb12b68
chore(gitignore): remove contributor-specific entry
lexfrei Apr 27, 2026
0bfa594
fix(helpers): guard bus_by_link/mac_by_link against missing fields
lexfrei Apr 27, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions charts/cozystack/templates/_helpers.tpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,7 @@ nameservers:
[]
{{- end }}
{{- $defaultLinkName := include "talm.discovered.default_link_name_by_gateway" . }}
{{- if $defaultLinkName }}
{{- $isVlan := include "talm.discovered.is_vlan" $defaultLinkName }}
{{- $parentLinkName := "" }}
{{- if $isVlan }}
Expand Down Expand Up @@ -269,19 +270,21 @@ name: {{ .Values.floatingIP | quote }}
link: {{ $vipLinkName }}
{{- end }}
{{- end }}
{{- end }}

{{- /* Shared legacy network section for machine.network */ -}}
{{- define "talos.config.network.legacy" }}
network:
hostname: {{ include "talm.discovered.hostname" . | quote }}
nameservers: {{ include "talm.discovered.default_resolvers" . }}
{{- (include "talm.discovered.physical_links_info" .) | nindent 4 }}
interfaces:
{{- $existingInterfacesConfiguration := include "talm.discovered.existing_interfaces_configuration" . }}
{{- $defaultLinkName := include "talm.discovered.default_link_name_by_gateway" . }}
{{- if or $existingInterfacesConfiguration $defaultLinkName }}
interfaces:
{{- if $existingInterfacesConfiguration }}
{{- $existingInterfacesConfiguration | nindent 4 }}
{{- else }}
{{- $defaultLinkName := include "talm.discovered.default_link_name_by_gateway" . }}
{{- $isVlan := include "talm.discovered.is_vlan" $defaultLinkName }}
{{- $parentLinkName := "" }}
{{- if $isVlan }}
Expand Down Expand Up @@ -318,6 +321,7 @@ link: {{ $vipLinkName }}
{{- end }}
{{- end }}
{{- end }}
{{- end }}
{{- end }}

{{- define "talos.config.legacy" }}
Expand Down
8 changes: 6 additions & 2 deletions charts/generic/templates/_helpers.tpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ nameservers:
[]
{{- end }}
{{- $defaultLinkName := include "talm.discovered.default_link_name_by_gateway" . }}
{{- if $defaultLinkName }}
{{- $isVlan := include "talm.discovered.is_vlan" $defaultLinkName }}
{{- $parentLinkName := "" }}
{{- if $isVlan }}
Expand Down Expand Up @@ -192,19 +193,21 @@ name: {{ .Values.floatingIP | quote }}
link: {{ $vipLinkName }}
{{- end }}
{{- end }}
{{- end }}

{{- /* Shared legacy network section for machine.network */ -}}
{{- define "talos.config.network.legacy" }}
network:
hostname: {{ include "talm.discovered.hostname" . | quote }}
nameservers: {{ include "talm.discovered.default_resolvers" . }}
{{- (include "talm.discovered.physical_links_info" .) | nindent 4 }}
interfaces:
{{- $existingInterfacesConfiguration := include "talm.discovered.existing_interfaces_configuration" . }}
{{- $defaultLinkName := include "talm.discovered.default_link_name_by_gateway" . }}
{{- if or $existingInterfacesConfiguration $defaultLinkName }}
interfaces:
{{- if $existingInterfacesConfiguration }}
{{- $existingInterfacesConfiguration | nindent 4 }}
{{- else }}
{{- $defaultLinkName := include "talm.discovered.default_link_name_by_gateway" . }}
{{- $isVlan := include "talm.discovered.is_vlan" $defaultLinkName }}
{{- $parentLinkName := "" }}
{{- if $isVlan }}
Expand Down Expand Up @@ -241,6 +244,7 @@ link: {{ $vipLinkName }}
{{- end }}
{{- end }}
{{- end }}
{{- end }}
{{- end }}

{{- define "talos.config.legacy" }}
Expand Down
143 changes: 138 additions & 5 deletions charts/talm/templates/_helpers.tpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@
{{- if and (eq .spec.dst "") (not (eq .spec.gateway "")) (eq .spec.table "main") }}
{{- $linkName = .spec.outLinkName }}
{{- $family = .spec.family }}
{{- break }}
{{- end }}
{{- end }}
{{- $addresses := list }}
Expand Down Expand Up @@ -106,31 +107,34 @@

{{- define "talm.discovered.default_link_name_by_gateway" }}
{{- range (lookup "routes" "" "").items }}
{{- if and (eq .spec.dst "") (not (eq .spec.gateway "")) }}
{{- if and (eq .spec.dst "") (not (eq .spec.gateway "")) (eq .spec.table "main") }}
{{- .spec.outLinkName }}
{{- break }}
{{- end }}
{{- end }}
{{- end }}

{{- define "talm.discovered.default_link_address_by_gateway" }}
{{- range (lookup "routes" "" "").items }}
{{- if and (eq .spec.dst "") (not (eq .spec.gateway "")) }}
{{- if and (eq .spec.dst "") (not (eq .spec.gateway "")) (eq .spec.table "main") }}
{{- (lookup "links" "" .spec.outLinkName).spec.hardwareAddr }}
{{- break }}
{{- end }}
{{- end }}
{{- end }}

{{- define "talm.discovered.default_link_bus_by_gateway" }}
{{- range (lookup "routes" "" "").items }}
{{- if and (eq .spec.dst "") (not (eq .spec.gateway "")) }}
{{- (lookup "links" "" .spec.outLinkName).spec.hardwareAddr }}
{{- if and (eq .spec.dst "") (not (eq .spec.gateway "")) (eq .spec.table "main") }}
{{- (lookup "links" "" .spec.outLinkName).spec.busPath }}
{{- break }}
{{- end }}
{{- end }}
{{- end }}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

{{- define "talm.discovered.default_link_selector_by_gateway" }}
{{- range (lookup "routes" "" "").items }}
{{- if and (eq .spec.dst "") (not (eq .spec.gateway "")) }}
{{- if and (eq .spec.dst "") (not (eq .spec.gateway "")) (eq .spec.table "main") }}
{{- with (lookup "links" "" .spec.outLinkName) }}
busPath: {{ .spec.busPath }}
{{- break }}
Comment on lines +137 to 140
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

default_link_selector_by_gateway can still emit an empty busPath: selector.

with (lookup "links" "" .spec.outLinkName) is truthy for bond/VLAN links that have a spec but no busPath, so this helper renders busPath: with an empty value instead of returning nothing. That produces an invalid selector fragment for the “default route via virtual link” case. Guard the field itself here the same way bus_by_link and link_selector_by_name already do.

Suggested fix 
 {{- define "talm.discovered.default_link_selector_by_gateway" }}
 {{- range (lookup "routes" "" "").items }}
 {{- if and (eq .spec.dst "") (not (eq .spec.gateway "")) (eq .spec.table "main") }}
-{{- with (lookup "links" "" .spec.outLinkName) }}
-busPath: {{ .spec.busPath }}
-{{- break }}
-{{- end }}
+{{- $link := lookup "links" "" .spec.outLinkName }}
+{{- if and $link $link.spec $link.spec.busPath }}
+busPath: {{ $link.spec.busPath }}
+{{- end }}
+{{- break }}
 {{- end }}
 {{- end }}
 {{- end }}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
{{- if and (eq .spec.dst "") (not (eq .spec.gateway "")) (eq .spec.table "main") }}
{{- with (lookup "links" "" .spec.outLinkName) }}
busPath: {{ .spec.busPath }}
{{- break }}
{{- define "talm.discovered.default_link_selector_by_gateway" }}
{{- range (lookup "routes" "" "").items }}
{{- if and (eq .spec.dst "") (not (eq .spec.gateway "")) (eq .spec.table "main") }}
{{- $link := lookup "links" "" .spec.outLinkName }}
{{- if and $link $link.spec $link.spec.busPath }}
busPath: {{ $link.spec.busPath }}
{{- end }}
{{- break }}
{{- end }}
{{- end }}
{{- end }}
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@charts/talm/templates/_helpers.tpl` around lines 137 - 140, The helper
default_link_selector_by_gateway can emit an empty "busPath:" because it only
checks that lookup "links" returns an object, not that that object's
.spec.busPath is non-empty; update the template to only render the busPath line
when the looked-up link has a non-empty .spec.busPath (same guard style used in
bus_by_link and link_selector_by_name). Specifically, inside
default_link_selector_by_gateway where you call with (lookup "links" ""
.spec.outLinkName), add an additional conditional that checks .spec.busPath (or
use if and not (eq .spec.busPath "")) before emitting the busPath: {{
.spec.busPath }} line so the selector is omitted when busPath is empty.

Expand Down Expand Up @@ -266,3 +270,132 @@ vlans:
- vlanId: {{ $link.spec.vlan.vlanID }}
{{- end -}}
{{- end -}}

{{- /*
Multi-NIC discovery helpers (#125).

The `default_*_by_gateway` family above resolves only the link carrying the
default route (primary). Templates targeting nodes with secondary NICs
(storage links on a control-plane, second uplink, etc.) need to enumerate
every physical link and read its addresses/routes/MAC by name. The helpers
below are the by-name building blocks; existing default_*_by_gateway helpers
remain wrappers that resolve the primary link and call into these.
*/ -}}

{{- /* JSON list of physical link names (raw NICs only — not bond/vlan masters).
Renamed from `physical_links` to avoid collision with the older
`physical_links_info` helper, which renders YAML comments. */ -}}
{{- define "talm.discovered.physical_link_names" -}}
{{- $names := list -}}
{{- range (lookup "links" "" "").items -}}
{{- if and .spec.busPath (regexMatch "^(eno|eth|enp|enx|ens)" (.metadata.id | toString)) -}}
{{- $names = append $names .metadata.id -}}
{{- end -}}
{{- end -}}
{{- toJson $names -}}
{{- end -}}

{{- /* JSON list of every link a user template can configure: physical NICs
plus bond / vlan / bridge top-level links. */ -}}
{{- define "talm.discovered.configurable_link_names" -}}
{{- $names := list -}}
{{- range (lookup "links" "" "").items -}}
{{- $isPhysical := and .spec.busPath (regexMatch "^(eno|eth|enp|enx|ens)" (.metadata.id | toString)) -}}
{{- $isVirtual := has (.spec.kind | toString) (list "bond" "vlan" "bridge") -}}
{{- if or $isPhysical $isVirtual -}}
{{- $names = append $names .metadata.id -}}
{{- end -}}
{{- end -}}
{{- toJson $names -}}
{{- end -}}

{{- /* JSON list of CIDR addresses configured on the given link, excluding
kernel-managed scopes (host loopback, link-local, nowhere) and
addresses with no scope set at all. Both IPv4 and IPv6 globally-scoped
addresses are returned; the caller is responsible for filtering by
family or stripping VIPs if needed. */ -}}
{{- define "talm.discovered.addresses_by_link" -}}
{{- $linkName := . -}}
{{- $addresses := list -}}
{{- range (lookup "addresses" "" "").items -}}
{{- $hasScope := and .spec.scope (ne (.spec.scope | toString) "") -}}
{{- $skip := has (.spec.scope | toString) (list "host" "link" "nowhere") -}}
{{- if and (eq .spec.linkName $linkName) $hasScope (not $skip) -}}
{{- $addresses = append $addresses .spec.address -}}
{{- end -}}
{{- end -}}
{{- toJson $addresses -}}
{{- end -}}

{{- /* Scalar IPv4 gateway for the default route (dst="", main table) on the
given link. Empty if the link has no IPv4 default route. IPv4-only by
convention to avoid family/address mismatch on dual-stack nodes — add
a sibling helper for IPv6 if you need it. */ -}}
{{- define "talm.discovered.gateway_by_link" -}}
{{- $linkName := . -}}
{{- range (lookup "routes" "" "").items -}}
{{- if and (eq .spec.outLinkName $linkName) (eq .spec.dst "") (not (eq .spec.gateway "")) (eq .spec.table "main") (eq (.spec.family | toString) "inet4") -}}
{{- .spec.gateway -}}
{{- break -}}
{{- end -}}
{{- end -}}
{{- end -}}

{{- /* JSON list of non-default routes on the given link, across all
routing tables (the `table` field is included so callers can
filter — gateway_by_link narrows to table=main, this helper does
not). Each entry is a flat map {dst, gateway, family, table,
priority} so consumers can fromJsonArray + range + dig. Absent
fields are emitted as empty strings; present fields including the
integer 0 (e.g. priority: 0) round-trip through toString. */ -}}
{{- define "talm.discovered.routes_by_link" -}}
{{- $linkName := . -}}
{{- $routes := list -}}
{{- range (lookup "routes" "" "").items -}}
{{- if and (eq .spec.outLinkName $linkName) (not (eq .spec.dst "")) -}}
{{- $gateway := "" -}}
{{- if not (kindIs "invalid" .spec.gateway) -}}{{- $gateway = printf "%v" .spec.gateway -}}{{- end -}}
{{- $family := "" -}}
{{- if not (kindIs "invalid" .spec.family) -}}{{- $family = printf "%v" .spec.family -}}{{- end -}}
{{- $table := "" -}}
{{- if not (kindIs "invalid" .spec.table) -}}{{- $table = printf "%v" .spec.table -}}{{- end -}}
{{- $priority := "" -}}
{{- if not (kindIs "invalid" .spec.priority) -}}{{- $priority = printf "%v" .spec.priority -}}{{- end -}}
{{- $entry := dict "dst" .spec.dst "gateway" $gateway "family" $family "table" $table "priority" $priority -}}
{{- $routes = append $routes $entry -}}
{{- end -}}
{{- end -}}
{{- toJson $routes -}}
{{- end -}}
Comment on lines +351 to +369
Copy link
Copy Markdown
Contributor

@myasnikovdaniil myasnikovdaniil Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

routes_by_link returns priority as a string

{{- $priority := "" -}}
{{- if not (kindIs "invalid" .spec.priority) -}}{{- $priority = printf "%v" .spec.priority -}}{{- end -}}
{{- $entry := dict "dst" .spec.dst "gateway" $gateway "family" $family "table" $table "priority" $priority -}}

The result is "priority":"0" / "priority":"100" — string-typed in the JSON. Functional and documented in the helper comment, but downstream callers that want metric/priority ordering will have to atoi the value. If you'd rather keep it native:

{{- $entry := dict "dst" .spec.dst "gateway" $gateway "family" $family "table" $table -}}
{{- if not (kindIs "invalid" .spec.priority) -}}
{{- $entry = merge $entry (dict "priority" .spec.priority) -}}
{{- end -}}

Not a blocker; flagging because priority is the one field where the lossy stringification is most likely to bite a future consumer.

Copy link
Copy Markdown
Contributor Author

@lexfrei lexfrei Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Declining this one. The helper's invariant is "all fields are present strings; absent fields are empty strings", per the existing doc and pinned by the assertContains '"priority":"0"' assertion. Switching priority alone to native int introduces a heterogeneous shape — "string for dst, int for priority, empty string for absent fields" — which forces every consumer to add a type/presence check. The single-field atoi cost on the consumer side is real but bounded.

Happy to revisit if a downstream consumer surfaces concrete metric-ordering pain — at that point the better answer is probably a sibling helper that emits native types throughout, leaving this one as the string-uniform version.


{{- /* Scalar MAC address for the given link, or empty if the link is
missing or has no hardwareAddr (virtual links have spec but may
lack the field; test mocks may return {} for unknown ids). The
field guard prevents `nil | toString` from rendering "<nil>". */ -}}
{{- define "talm.discovered.mac_by_link" -}}
{{- $link := lookup "links" "" . -}}
{{- if and $link $link.spec $link.spec.hardwareAddr -}}
{{- $link.spec.hardwareAddr | toString -}}
{{- end -}}
{{- end -}}

{{- /* Scalar PCI / bus path for the given link, or empty if the link is
missing or has no busPath (virtual links such as bonds expose a
spec without a busPath). The field guard prevents
`nil | toString` from rendering "<nil>". */ -}}
{{- define "talm.discovered.bus_by_link" -}}
{{- $link := lookup "links" "" . -}}
{{- if and $link $link.spec $link.spec.busPath -}}
{{- $link.spec.busPath | toString -}}
{{- end -}}
{{- end -}}
Comment on lines +386 to +391
Copy link
Copy Markdown
Contributor

@myasnikovdaniil myasnikovdaniil Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bus_by_link / mac_by_link can emit <nil> for virtual links

{{- define "talm.discovered.bus_by_link" -}}
{{- $link := lookup "links" "" . -}}
{{- if and $link $link.spec -}}
{{- $link.spec.busPath | toString -}}
{{- end -}}
{{- end -}}

When the link exists with a spec but the specific field is absent — concretely, bus_by_link "bond0" on the secondary-NIC fixture, since virtual links don't have a busPath — $link.spec.busPath is nil and nil | toString renders as <nil>. The doc string promises "empty if the link or its spec is missing", but a present spec with a missing field falls through. The global assertNotContains "<nil>" in TestSecondaryNicHelpers doesn't exercise this exact call.

Suggested tighten:

{{- define "talm.discovered.bus_by_link" -}}
{{- $link := lookup "links" "" . -}}
{{- if and $link $link.spec $link.spec.busPath -}}
{{- $link.spec.busPath | toString -}}
{{- end -}}
{{- end -}}

Same shape for mac_by_link (guard on $link.spec.hardwareAddr).

Copy link
Copy Markdown
Contributor Author

@lexfrei lexfrei Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tightened both helpers in 28f37e0 — the if guard now extends to the specific field ($link.spec.busPath / $link.spec.hardwareAddr), so a present-spec / absent-field link returns empty rather than dereferencing through nil | toString. The bond0 fixture in secondaryNicLookup already had hardwareAddr but no busPath; added explicit bus_bond0 / mac_bond0 template inclusions to TestSecondaryNicHelpers so the regression is pinned directly rather than relying on the global assertNotContains "<nil>" sweep. Updated the doc comments to spell out the present-spec / absent-field case.


{{- /* YAML fragment `busPath: <path>` for use as a Talos deviceSelector by
link name. Prefer this over emitting `interface:` when you need the
config to be portable across renames (e.g. predictable network names). */ -}}
{{- define "talm.discovered.link_selector_by_name" -}}
{{- $link := lookup "links" "" . -}}
{{- if and $link $link.spec.busPath -}}
busPath: {{ $link.spec.busPath }}
{{- end -}}
{{- end -}}
Loading