refactor(chart): area/chart cleanup batch (factor multi-doc, scope-filter symmetry, int() drop)#208
Conversation
cidrPrefixLen (the engine-provided helper in pkg/engine/helm) already returns Go int. Two call sites in charts/talm/templates/_helpers.tpl wrapped it in Sprig's int(): - addresses_by_link, line 151 - default_addresses_by_gateway, line 564 The third caller (link_name_for_address, line 429) uses the bare form. Dropping the no-op wrappers brings all three sites into one style and removes the implicit suggestion that cidrPrefixLen might return a non-int. Existing engine contract tests round-trip the render byte-for- byte, so no new test is needed — a regression would surface as diff in the existing render-output fixtures. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
…where too The legacy v1.11 helper `talm.discovered.default_addresses_by_gateway` filtered only the `host` scope, while the v1.12 multi-doc sibling `talm.discovered.addresses_by_link` rejects the full `host` / `link` / `nowhere` triple (the kernel-managed scopes that should never land in node config). Real Talos COSI always sets scope=link for the 169.254/16 link-local range, so the divergence was latent — but a future COSI schema bump that produced link-local entries on the default-gateway-bearing link would have leaked them verbatim into the legacy `machine.network.interfaces[].addresses` block while the v1.12 path correctly dropped them. Hoist the same `$skipScopes := list "host" "link" "nowhere"` the v1.12 helper uses and apply `has (.spec.scope | toString) $skipScopes` symmetrically. The two helpers now share one scope-filter rule. Pinned by TestContract_NetworkLegacy_DefaultAddressesFilterLinkScope with a new linkScopedAddressOnDefaultGatewayLookup fixture — a realistic Hetzner topology with a link-scoped 169.254/16 sandwiched between two global-scope siblings on the default- route-bearing link. Asserts the link-scoped entry is absent from the rendered output AND a global-scope sibling on the same link is still present (the filter must be scope-selective, not "drop everything on this link"). Verified regression-pin: stashed the chart fix, test failed at the expected assertion; restored, test passed. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
…m helper
charts/cozystack/templates/_helpers.tpl and
charts/generic/templates/_helpers.tpl each carried a ~309-line
talos.config.network.multidoc body that was byte-identical
modulo six lines of prose drift in one comment. Every change to
the multi-doc renderer (BondConfig / VLANConfig / BridgeConfig
emission, address scope filtering, longest-prefix VIP-link
selection, malformed-CIDR guard, floatingIP type coercion) had
to be applied twice. The mirror tests under
TestContract_NetworkMultidoc_Generic_* and
TestContract_NetworkLegacy_Generic_* caught drift heuristically
but never structurally.
Extracts the body into talm.config.network.multidoc on the
shared talm library chart (already a subchart of both
cozystack and generic via charts/<name>/charts/talm symlink).
Both chart-specific talos.config.network.multidoc defines
shrink to a one-line forward:
{{- define "talos.config.network.multidoc" }}
{{- include "talm.config.network.multidoc" . }}
{{- end }}
cozystack-specific RegistryMirrorConfig and the chart-specific
talos.config.cluster / talos.config.machine.common defines stay
where they are — only the per-link body moves. The Generic mirror
tests stay as defensive coverage but become structural rather
than load-bearing: a single source of truth for the per-link
emission logic guarantees byte-identical behavior across both
chart presets by construction.
Net change:
charts/cozystack/templates/_helpers.tpl: 571 -> 265 lines
charts/generic/templates/_helpers.tpl: 482 -> 176 lines
charts/talm/templates/_helpers.tpl: 702 -> 1019 lines
Full engine test suite stays green; both cozystack and generic
mirror tests pass byte-for-byte against the new shared define.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
✨ Finishing Touches🧪 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 |
lexfrei
added
kind/cleanup
![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 centralizes the Talos v1.12+ multi-document network configuration logic into a shared helper within the talm chart, reducing duplication across the cozystack and generic charts. It also synchronizes address filtering between the legacy v1.11 and v1.12 paths by excluding additional kernel-managed scopes and includes updated tests and documentation. Feedback suggests adding a scope presence check to the legacy address helper to ensure full consistency with the v1.12 implementation.
| {{- $address := .spec.address | toString }} | ||
| {{- $validCidr := ge (int (cidrPrefixLen $address)) 0 }} | ||
| {{- if and (eq .spec.linkName $linkName) (eq .spec.family $family) (not (eq .spec.scope "host")) $validCidr }} | ||
| {{- $validCidr := ge (cidrPrefixLen $address) 0 }} | ||
| {{- if and (eq .spec.linkName $linkName) (eq .spec.family $family) (not (has (.spec.scope | toString) $skipScopes)) $validCidr }} |
There was a problem hiding this comment.
The legacy default_addresses_by_gateway helper is missing the scope presence check that its v1.12 counterparts (addresses_by_link and link_name_for_address) use. To ensure full symmetry and avoid leaking addresses with missing or empty scopes into the legacy config, please add the $hasScope check here as well. This also makes the logic identical to the other helpers as intended by this refactor.
{{- $address := .spec.address | toString }}
{{- $hasScope := and .spec.scope (ne (.spec.scope | toString) "") }}
{{- $skip := has (.spec.scope | toString) $skipScopes }}
{{- $validCidr := ge (cidrPrefixLen $address) 0 }}
{{- if and (eq .spec.linkName $linkName) (eq .spec.family $family) $hasScope (not $skip) $validCidr }}
1 participant
Closes the area/chart cleanup queue (#165 + #166 + #167) in one cohesive pass.
Commits
chore(chart): drop redundant int() wrapper around cidrPrefixLen calls(#166)cidrPrefixLenis a Gointalready; theint(...)wrappers at two call sites incharts/talm/templates/_helpers.tplwere no-ops. Drops them so all three call sites ofcidrPrefixLenuse the bare form consistently. Pure mechanical, byte-identical render verified by existing fixtures.fix(chart): legacy default_addresses_by_gateway drops scope=link / nowhere too(#167)The legacy v1.11
talm.discovered.default_addresses_by_gatewayfiltered only thehostscope, while the v1.12 siblingtalm.discovered.addresses_by_linkrejects the fullhost/link/nowheretriple. Hoist the same$skipScopes := list "host" "link" "nowhere"into the legacy helper and applyhas (.spec.scope | toString) $skipScopessymmetrically. Real Talos COSI uses scope=link for 169.254/16, so the divergence was latent — but a future COSI schema bump that produced link-local entries on the default-gateway-bearing link would have leaked them verbatim into the legacymachine.network.interfaces[].addressesblock.Pinned by
TestContract_NetworkLegacy_DefaultAddressesFilterLinkScopewith a newlinkScopedAddressOnDefaultGatewayLookupfixture: a link-scoped 169.254 entry sandwiched between two global-scope siblings; asserts the link-scoped address is absent AND the global-scope sibling is still present (filter is scope-selective). Regression-pin verified — stash the chart fix, test fails; restore, test passes.refactor(chart): factor v1.12 multi-doc per-link body into shared talm helper(#165)charts/cozystack/templates/_helpers.tplandcharts/generic/templates/_helpers.tpleach carried a ~309-linetalos.config.network.multidocbody that was byte-identical modulo six lines of prose drift. Every change to the multi-doc renderer had to be applied twice; the*_Generic_*mirror tests caught drift heuristically but never structurally.Extracts the body into
talm.config.network.multidocon the shared talm library chart (already a subchart of both via symlinkcharts/<name>/charts/talm). Both chart-specific defines collapse to one-line forwarders.Net change:
charts/cozystack/templates/_helpers.tpl: 571 → 265 linescharts/generic/templates/_helpers.tpl: 482 → 176 linescharts/talm/templates/_helpers.tpl: 702 → 1019 linesExisting mirror tests stay green byte-for-byte against the shared define.
Test plan
docs/manual-test-plan.mdadds B6. Scope-filter symmetry across v1.11 and v1.12 renders — pinned-talosVersion render check for both schema paths against a 169.254-bearing topology.Validation
go build ./...cleango test ./... -count=1all greengolangci-lint run ./...0 issuestalm template -f nodes/node0.yaml+talm apply --dry-run -f nodes/node0.yamlproduce identical drift preview to pre-refactor output (3 update / 4 unchanged), proving the per-link factor-out is byte-identical end-to-end against a real cluster.