feat(charts): add preset value knobs (#226-#230) - #232
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
📝 WalkthroughWalkthroughAdds time servers, registry settings, control-plane arguments, multiple VIPs, preserved interfaces, declarative extra links, and stricter validation across both chart variants. Apply-time reference checking now recognizes links created within the same configuration. ChangesConfiguration rendering
Network rendering
Apply-time validation
Documentation and validation plan
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Values
participant HelmTemplates
participant TalosDiscovery
participant ApplyValidation
Values->>HelmTemplates: Provide registry, VIP, and network settings
TalosDiscovery->>HelmTemplates: Supply existing links and addresses
HelmTemplates->>HelmTemplates: Render typed network documents
HelmTemplates->>ApplyValidation: Emit created-link and link references
ApplyValidation->>TalosDiscovery: Compare references with snapshot links
ApplyValidation-->>HelmTemplates: Accept created links or report blockers
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
b098772 to
e3477ff
Compare
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
pkg/engine/render_test.go (1)
6866-6898: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: parameterize the returned template to avoid duplicating
renderCozystackWith.
renderCozystackWorkerWithis a near-verbatim copy ofrenderCozystackWith, differing only in the returned map key. A shared helper taking the template path would remove the drift risk between the two.🤖 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 6866 - 6898, The duplicated rendering logic in renderCozystackWorkerWith should be consolidated with renderCozystackWith. Extract or update a shared helper to accept the desired rendered template key, have both functions reuse it, and preserve each function’s existing return key and behavior.
🤖 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.
Inline comments:
In `@charts/generic/templates/_helpers.tpl`:
- Around line 123-132: Prevent empty apiServer keys from rendering when no
arguments exist: in charts/generic/templates/_helpers.tpl lines 123-132, place
apiServer: inside the existing extraApiServerArgs with block; in
charts/cozystack/templates/_helpers.tpl lines 256-278, move apiServer: inside
the existing if or $oidcSet .Values.extraApiServerArgs guard. Preserve rendering
when the guarded content is present.
In `@docs/manual-test-plan.md`:
- Around line 639-646: Update the collision scenario in the manual test plan so
the selected preserved interface device is returned by discovery and reliably
triggers the expected “collides with a discovered link” error; alternatively,
replace the expected error with the actual preserved-link collision message.
Keep the non-collision and other refusal scenarios unchanged.
In `@README.md`:
- Around line 177-181: Clarify the README statement about VIP removal to specify
that stripping applies only during typed per-link document rebuilds. Update the
nearby preserveExisting explanation consistently, noting that verbatim interface
preservation bypasses this stripping while VIP and registry documents remain
unaffected.
- Around line 181-184: Remove the blank lines between the contiguous blockquote
paragraphs in README.md, keeping the version compatibility text directly
adjacent to the preceding network behavior text so markdownlint MD028 passes.
---
Nitpick comments:
In `@pkg/engine/render_test.go`:
- Around line 6866-6898: The duplicated rendering logic in
renderCozystackWorkerWith should be consolidated with renderCozystackWith.
Extract or update a shared helper to accept the desired rendered template key,
have both functions reuse it, and preserve each function’s existing return key
and behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 48bd51c0-60a4-4154-a78d-3280c84252ef
📒 Files selected for processing (16)
README.mdcharts/cozystack/templates/_helpers.tplcharts/cozystack/values.yamlcharts/generic/templates/_helpers.tplcharts/generic/values.yamlcharts/talm/templates/_helpers.tpldocs/manual-test-plan.mdpkg/applycheck/refs.gopkg/applycheck/validate.gopkg/applycheck/validate_test.gopkg/engine/contract_cluster_test.gopkg/engine/contract_errors_test.gopkg/engine/contract_machine_test.gopkg/engine/contract_network_legacy_test.gopkg/engine/contract_network_multidoc_test.gopkg/engine/render_test.go
| apiServer: | ||
| {{- with .Values.extraApiServerArgs }} | ||
| extraArgs: | ||
| {{- range $k, $v := . }} | ||
| {{- if kindIs "invalid" $v }} | ||
| {{- fail (printf "values.yaml: extraApiServerArgs.%s has no value. A bare `key:` renders as the literal string \"<nil>\" onto the component command line; give it a value or drop the key." $k) }} | ||
| {{- end }} | ||
| {{ $k }}: {{ $v | toString | quote }} | ||
| {{- end }} | ||
| {{- end }} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Empty apiServer: key emitted for control-plane renders without extra args (both charts). Shared root cause: the apiServer: map key is rendered unconditionally, so with the default empty extraApiServerArgs (and no OIDC on cozystack) a bare apiServer: line is produced, adding output that wasn't there before and breaking the byte-identical-for-defaults invariant.
charts/generic/templates/_helpers.tpl#L123-L132: wrapapiServer:in{{- with .Values.extraApiServerArgs }} ... {{- end }}, matching the guardedcontrollerManager(L100) andscheduler(L113) blocks.charts/cozystack/templates/_helpers.tpl#L256-L278: move theapiServer:line inside the{{- if or $oidcSet .Values.extraApiServerArgs }}guard on L258 so the key only appears when it has content.
📍 Affects 2 files
charts/generic/templates/_helpers.tpl#L123-L132(this comment)charts/cozystack/templates/_helpers.tpl#L256-L278
🤖 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/generic/templates/_helpers.tpl` around lines 123 - 132, Prevent empty
apiServer keys from rendering when no arguments exist: in
charts/generic/templates/_helpers.tpl lines 123-132, place apiServer: inside the
existing extraApiServerArgs with block; in
charts/cozystack/templates/_helpers.tpl lines 256-278, move apiServer: inside
the existing if or $oidcSet .Values.extraApiServerArgs guard. Preserve rendering
when the guarded content is present.
Andrei Kvapil (kvaps)
left a comment
Andrei Kvapil (kvaps)
left a comment
There was a problem hiding this comment.
Reviewed by checking out the branch and verifying each claim in the description independently rather than relying on the added tests.
Verification
"a stock render is byte-identical to before" — confirmed, with room to spare. I rendered a 240-cell matrix (3 discovery fixtures × 2 charts × 2 templates × 4 Talos versions × 5 value sets) on both the merge base and the branch head and diffed them: identical. The fixtures included a rich topology (physical primary + bond with two slaves + VLAN child + bridge with a port, a link-local address, a non-default route, jumbo MTU) and the value sets covered floatingIP, vipLink, oidcIssuerUrl and allocateNodeCIDRs: false. The committed golden files are untouched and TestGoldenRender passes across both presets × controlplane/worker × legacy/multi-doc. Moving the hardcoded docker.io mirror into values.yaml is handled carefully, and the deep-merge caveat ({} keeps the default, null clears it) is both documented and true — I checked that registryMirrors: null emits no registry documents at all.
Component-arg quoting — confirmed. Integers, floats and booleans all come out quoted; legitimate values survive intact (A=true,B=false stays whole, an empty string does not become null, yes does not become a boolean). Key ordering is deterministic.
Apply-gate relaxation — confirmed as bounded. I exercised WalkRefs + ValidateRefs directly. A bond → VLAN → VIP chain created entirely by the config validates clean, while a typoed VLANConfig.parent, a dangling Layer2VIPConfig.link, a bond slave absent from the node, and a VIP pointing at a near-miss VLAN name all still block. Because the created-link union runs in its own pass before validation, document order does not matter. LinkConfig still validates its own name, and the WireguardConfig exception is described accurately in the README.
"Anything Talos would reject fails fast" — mostly. Every case listed in the description does fail with a clear message on both schemas, and each has a contract test. A few gaps are below.
go build, go vet, gofmt -l, go test ./... and golangci-lint run are all clean. The 97 new contract tests assert on literal rendered output rather than restating the template expression, and cover the negative path for every guard — no skips, no TODOs.
Please fix
registryTLS keys are emitted unquoted, unlike registryMirrors. charts/talm/templates/_helpers.tpl:366 (multi-doc) and :399 (legacy) interpolate the key raw, while the mirror name goes through | toYaml at :356 and :389 for exactly this reason. An IPv6 endpoint host produces invalid YAML on both schemas:
# registryTLS: { "[::1]:5000": { insecureSkipVerify: true } }
kind: RegistryTLSConfig
name: [::1]:5000 # yaml: did not find expected node content [::1]:5000: # same failure in machine.registries.config
tls:Neither document parses. A DNS host:port key survives only because it happens to contain no ": ". The same | toYaml treatment in both places fixes it.
Worth considering
An extraLinks bond can enslave a link the chart is also emitting a config for. At charts/talm/templates/_helpers.tpl:1375-1400, bond.interfaces are not checked against $configurableLinks, $existingLinkNames or $emittedNames — only the entry's own interface name is. Given a node whose discovered primary is eth0:
network:
extraLinks:
- interface: bond1
bond: { interfaces: [eth0], mode: 802.3ad }
addresses: [203.0.113.10/24]the render emits both:
kind: LinkConfig
name: eth0
addresses:
- address: 192.168.201.10/24
routes:
- gateway: 192.168.201.1
---
kind: BondConfig
name: bond1
links:
- eth0The discovery path deliberately filters this shape out — configurable_link_names (:736-747) excludes anything with a slaveKind so a slave never gets a standalone document alongside its master. The declarative path has no equivalent check, and "move the primary NIC into a bond" is one of the main reasons to reach for extraLinks. The same applies under preserveExisting: a preserved interface emitted verbatim in machine.network.interfaces can simultaneously become a bond slave. Given how thorough the rest of the guard set is, this reads more like an oversight than a decision — but if it is intentional, a note in the extraLinks values documentation would help.
Prefix length is not validated in address and route guards. talm.guard.addresses_not_vip (:428-434) and talm.render.link_routes_mtu (:452-459) check for the presence of / and validate the IP portion, but not the prefix itself. 203.0.113.10/33, 203.0.113.10/abc and 203.0.113.10/ all render successfully and reach the node, where Talos fails at decode with a message that does not name the document — the exact failure mode these guards exist to prevent. cidrPrefixLen is already registered and used elsewhere in the same file (:644, :779) and returns -1 on a parse failure, so ge (cidrPrefixLen $address) 0 would close both.
mtu is not validated at all (:470-472). mtu: "jumbo", mtu: -5 and mtu: 99999 are emitted verbatim; mtu: 0 is silently dropped as falsy.
Non-scalar extraArgs values leak a Go string. The guard catches only kindIs "invalid", so a nested map renders as foo: "map[a:b]" onto the component command line. Affects charts/cozystack/templates/_helpers.tpl:238,254,277 and charts/generic/templates/_helpers.tpl:110,120,130.
Nits
pkg/applycheck/refs.go:249-252— thehandleListOnlydocstring still says it emits "only the list-valued slaves/ports of the doc, not the doc's own.name", but the function now callsappendCreatedReffirst. Stale by exactly the change this PR makes.charts/talm/templates/_helpers.tpl:610-613— the comment justifies the dict-mutation pattern by stating thatrangeintroduces a new scope so$var = ...does not propagate. In Gotext/template,=assigns to the existing variable and does survive iterations; the same file relies on that at:781,:1005and:1374. Either drop the rationale or bring$bestin line with the surrounding style.- Duplicate addresses across two
extraLinksentries, and anextraLinksaddress matching another link's discovered address, are not caught. Probably harmless, but they sit just outside the "anything Talos would reject" line. Layer2VIPConfigfor a VIP on anextraLinksbond is emitted before theBondConfigthat creates it. Documents are a set, so this is fine today.timeServersaccepts a map and renders it as-is underservers:.
Size
The +3105 is 1936 lines of tests plus 156 of docs; the logic itself is around 740 lines. Splitting would create artificial ordering between PRs since the guards are shared across knobs ($vipIPs is needed by both vips and extraLinks; talm.guard.registries serves both schemas), so I would keep it as one change.
myasnikovdaniil
left a comment
myasnikovdaniil
left a comment
There was a problem hiding this comment.
NOT LGTM — two input-dependent rendering paths in the new knobs emit a config that is either unparseable YAML or double-declares a VIP.
Business context: Five preset knobs (#226–#230) so a node's Talos config comes from values.yaml instead of a forked template — NTP, control-plane extraArgs, registry mirrors/TLS, multiple VIPs, and declarative network topology.
Blockers
B1: registryTLS host rendered unquoted → invalid YAML
File: charts/talm/templates/_helpers.tpl:366 (multidoc), :399 (legacy)
Issue: A bracketed IPv6 endpoint host emits name: [fd00::1]:5000, where [ opens a YAML flow sequence.
Evidence: Rendered both schemas with registryTLS: {"[fd00::1]:5000": {insecureSkipVerify: true}} and parsed the result:
kind: RegistryTLSConfig
name: [fd00::1]:5000 -> yaml: line 99: did not find expected key
config:
[fd00::1]:5000: -> yaml: line 65: did not find expected key
tls:
insecureSkipVerify: true
The sibling registryMirrors path already solved exactly this at :354 with name: {{ $name | toYaml }}, and its comment explains why. registryTLS does not. A plain host:port (no brackets) parses fine — only the bracketed form breaks.
Impact: talm template emits a document that neither talm nor the node can parse — worse than the fail-fast this PR promises elsewhere.
Fix: {{ $host | toYaml }} at both sites, matching :354.
B2: Non-canonical IPv6 VIP emitted as both a static address and a Layer2VIPConfig
File: charts/talm/templates/_helpers.tpl:1073
Issue: The strip compares hasPrefix (printf "%s/" .) $addr on raw strings. Talos reports addresses canonically (netip.Prefix.String()), so a VIP spelled 2001:0DB8::5 never matches the discovered 2001:db8::5/64.
Evidence: With a fixture carrying 2001:db8::5/64 on eth0 and vips: [{link: eth0, ip: "2001:0DB8::5"}], the render contains both 2001:db8::5/64 under LinkConfig.addresses and kind: Layer2VIPConfig. A control render with the canonical spelling strips correctly, so the mechanism itself works — only the string comparison is wrong. The same gap pre-exists for floatingIP on main (:164, :807); this PR rewrites that loop and extends it to vips, so one fix covers both.
Impact: Exactly the failure the code's own comment says the strip prevents — "leaves the leader and the followers disagreeing about who owns it". Breaks VIP failover.
Fix: Canonicalize every VIP (and the discovered address) once, before the dedup check, the addresses_not_vip guard, and this strip.
Non-blocking follow-ups
-
CIDR prefix length unvalidated —
_helpers.tpl:432and:457checkcontains "/"and then runipIsValidon the text before the slash only.203.0.113.10/999as anextraLinksaddress, and198.51.100.0/fooas a route destination, both render straight through with no error. ThecidrPrefixLenhelper already exists (pkg/engine/helm/engine.go:265) and would close both paths. -
Fractional
vlanId—_helpers.tpl:1449range-checks$vlan.vlanId | int, truncating7.5to7and passing, but:1469emits the original value: the rendered output isvlanID: 7.5with document nameeth0.7.5. Talos rejects that at decode time. Worth checking the value is an integer before the range check. -
LinkAliasConfig%daliases —pkg/applycheck/refs.go:211registers.nameliterally. Machinery v1.13.7config/types/network/link_alias.go:59-63documentsnet%dexpanding tonet0,net1, … per matched link. So the literalnet%denterslinkSetwhile the names that will actually exist do not: anet%dreference passes the gate, and a legitimatenet0reference still blocks. This also makes the README's new claim thatWireguardConfigis the sole exemption slightly overstated. -
Bare
apiServer:—charts/generic/templates/_helpers.tpl:123emits a valueless key whencertSANsis[](the default) andextraApiServerArgsis empty. This is pre-existing (apiServer:is unconditional on main at line 76), so it is not a regression from this PR and the golden tests correctly show defaults unchanged — but the block is open here and it is a one-line fix. -
Commit granularity — five independent features land in one 3,105-line commit. Split per issue (#226–#230) would make
git bisectand a per-feature revert possible. -
Operator docs — the PR notes site docs as a follow-up; there is no open PR on the docs repo yet. Worth a tracking issue so the values reference (legacy vs v1.12+ split for
vips/registryTLS, the multi-doc-only restriction onnetwork.extraLinks, and thepreserveExistingescape hatch) does not get lost.
Bot findings triaged
- Empty
apiServer:breaking byte-identical defaults — not a regression;apiServer:is already unconditional on main at line 76. Retained as follow-up 4 on its cosmetic merits only. docs/manual-test-plan.md:646setup/error-message mismatch — valid.- README VIP-stripping wording contradicting itself — valid, and B2 makes the wording moot until fixed.
- README MD028 blank lines inside the blockquote — valid, lint-only.
Verified as sound
go test ./... passes locally across all packages, matching CI. Golden renders confirm defaults are byte-identical over both presets × controlplane/worker × legacy/multi-doc. The two applycheck dispatch maps are genuinely disjoint — WireguardConfig appears only in multidocNetAddrHandlers, as the comment claims. mergeMaps (pkg/engine/engine.go:1795) does assign nil, so the "null the key out" instruction in values.yaml is accurate. The 228 test functions across the five contract files do cover accept and reject paths; B1 and B2 are cases those tests never reach, not a testing-convention gap.
| --- | ||
| apiVersion: v1alpha1 | ||
| kind: RegistryTLSConfig | ||
| name: {{ $host }} |
There was a problem hiding this comment.
B1 (blocker). A bracketed IPv6 endpoint host renders as name: [fd00::1]:5000, and [ opens a YAML flow sequence — the document no longer parses (yaml: did not find expected key). Verified by rendering registryTLS: {"[fd00::1]:5000": {insecureSkipVerify: true}} and feeding the output to a YAML decoder.
registryMirrors at line 354 already handles this with name: {{ $name | toYaml }} and a comment about the * fallback key. Same treatment needed here: {{ $host | toYaml }}.
(Plain host:port without brackets parses fine, so only the bracketed form is affected.)
| {{- with .Values.registryTLS }} | ||
| config: | ||
| {{- range $host, $cfg := . }} | ||
| {{ $host }}: |
There was a problem hiding this comment.
B1 (blocker), legacy half. Same unquoted $host as line 366, this time as a mapping key:
config:
[fd00::1]:5000:
tls:
insecureSkipVerify: true
Parsing this fails with yaml: line 65: did not find expected key. {{ $host | toYaml }} fixes it here too.
| {{- $addr := . }} | ||
| {{- $isVip := false }} | ||
| {{- range $vipIPs }} | ||
| {{- if hasPrefix (printf "%s/" .) $addr }} |
There was a problem hiding this comment.
B2 (blocker). This compares raw strings, but Talos reports discovered addresses canonically via netip.Prefix.String(). A VIP spelled 2001:0DB8::5 therefore never matches the discovered 2001:db8::5/64, so the strip misses it.
Verified: fixture with 2001:db8::5/64 on eth0 plus vips: [{link: eth0, ip: "2001:0DB8::5"}] renders the address both under LinkConfig.addresses and as a Layer2VIPConfig. A control render using the canonical spelling strips correctly, so the mechanism works — only the comparison is spelling-sensitive.
That is the exact condition the comment above warns about ("leaves the leader and the followers disagreeing about who owns it"). The same gap pre-exists for floatingIP on main (lines 164 and 807), so canonicalizing once — before the dedup check, the addresses_not_vip guard, and this strip — covers both values.
| {{- fail (printf "talm: address %q on network.extraLinks %q has no prefix length. Talos parses addresses as CIDR, so write it as 203.0.113.10/24." (. | toString) $name) }} | ||
| {{- end }} | ||
| {{- $ip := (splitList "/" (. | toString)) | first }} | ||
| {{- if not (ipIsValid $ip) }} |
There was a problem hiding this comment.
ipIsValid runs on the text before the slash only, so the prefix length is never checked. 203.0.113.10/999 renders straight through to the emitted document with no error — Talos then rejects it at decode time with a message that does not name this document, which is the outcome the surrounding guards exist to avoid.
cidrPrefixLen already exists (pkg/engine/helm/engine.go:265) and validates the whole CIDR. The route-destination check at line 457 has the same partial-validation shape (198.51.100.0/foo also passes).
| {{- fail (printf "talm: network.extraLinks entry %q declares a VLAN with no vlanId. VLANConfig requires vlanID; add vlanId to the vlans entry." ($extra.interface | toString)) }} | ||
| {{- end }} | ||
| {{- if or (lt ($vlan.vlanId | int) 1) (gt ($vlan.vlanId | int) 4094) }} | ||
| {{- fail (printf "talm: network.extraLinks entry %q declares vlanId %v, outside the valid range. Talos requires vlanID between 1 and 4094." ($extra.interface | toString) $vlan.vlanId) }} |
There was a problem hiding this comment.
$vlan.vlanId | int truncates, so vlanId: 7.5 becomes 7 and passes this range check — but line 1469 emits the original value. The render output is vlanID: 7.5 with document name eth0.7.5, which Talos rejects while decoding. Worth asserting the value is an integer before range-checking it.
| // absent here: it belongs to the parallel net-addr walker, and the | ||
| // two dispatch maps must stay disjoint or a kind gets double-walked. | ||
| "DummyLinkConfig": handleCreatorOnly, | ||
| "LinkAliasConfig": handleCreatorOnly, |
There was a problem hiding this comment.
LinkAliasConfig.name supports a format verb: machinery v1.13.7 config/types/network/link_alias.go:59-63 documents net%d expanding to net0, net1, … one per matched link, with the selector allowed to match multiple links in that case.
So appendCreatedRef registers the literal net%d into linkSet while the names that will actually exist are never registered. Net effect is inverted from the intent: a VLANConfig.parent: net%d reference passes the gate even though no such link will ever exist, and a legitimate parent: net0 still blocks on first apply.
This also makes the README's new sentence — that WireguardConfig is the one exception which does not register its name — narrower than stated.
e3477ff to
171afd8
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/engine/render_test.go (1)
7040-7072: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: fold into
renderCozystackWithvia a template-name parameter. The two helpers differ only in the returned output key, so the endpoint defaulting and lookup save/restore logic is now duplicated.🤖 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 7040 - 7072, Refactor renderCozystackWith and renderCozystackWorkerWith to share their common setup and rendering logic, parameterizing only the template output key (or otherwise reusing one helper). Preserve each helper’s existing returned template while removing duplicated endpoint defaulting and helmEngine.LookupFunc save/restore behavior.pkg/engine/contract_cluster_test.go (1)
728-744: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueName the chart in the failure path. Both presets run through the same anonymous loop, so a failure doesn't say which chart broke.
♻️ Optional: table with names
- for _, render := range []func(*testing.T, func(string, string, string) (map[string]any, error), map[string]any, ...string) error{ - renderCozystackExpectError, - renderGenericExpectError, - } { - err := render(t, simpleNicLookup(), map[string]any{ - "timeServers": map[string]any{"primary": "time.cloudflare.com"}, - }) - if err == nil { - t.Fatal("expected a fail-fast for a timeServers mapping") - } - - if !strings.Contains(err.Error(), "timeServers") { - t.Errorf("error should name the field, got %v", err) - } - } + for name, render := range map[string]func(*testing.T, func(string, string, string) (map[string]any, error), map[string]any, ...string) error{ + "cozystack": renderCozystackExpectError, + "generic": renderGenericExpectError, + } { + t.Run(name, func(t *testing.T) { + err := render(t, simpleNicLookup(), map[string]any{ + "timeServers": map[string]any{"primary": "time.cloudflare.com"}, + }) + if err == nil { + t.Fatal("expected a fail-fast for a timeServers mapping") + } + + if !strings.Contains(err.Error(), "timeServers") { + t.Errorf("error should name the field, got %v", err) + } + }) + }🤖 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/contract_cluster_test.go` around lines 728 - 744, Update TestContract_Cluster_TimeServersMapping_Fails to associate each renderer with its chart/preset name and include that name in failure messages, so failures identify whether renderCozystackExpectError or renderGenericExpectError failed while preserving the existing timeServers assertion.
🤖 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 `@pkg/engine/contract_cluster_test.go`:
- Around line 728-744: Update TestContract_Cluster_TimeServersMapping_Fails to
associate each renderer with its chart/preset name and include that name in
failure messages, so failures identify whether renderCozystackExpectError or
renderGenericExpectError failed while preserving the existing timeServers
assertion.
In `@pkg/engine/render_test.go`:
- Around line 7040-7072: Refactor renderCozystackWith and
renderCozystackWorkerWith to share their common setup and rendering logic,
parameterizing only the template output key (or otherwise reusing one helper).
Preserve each helper’s existing returned template while removing duplicated
endpoint defaulting and helmEngine.LookupFunc save/restore behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5ac9f87a-4030-445c-9453-df97d242a38f
📒 Files selected for processing (17)
README.mdcharts/cozystack/templates/_helpers.tplcharts/cozystack/values.yamlcharts/generic/templates/_helpers.tplcharts/generic/values.yamlcharts/talm/templates/_helpers.tpldocs/manual-test-plan.mdpkg/applycheck/refs.gopkg/applycheck/validate.gopkg/applycheck/validate_test.gopkg/engine/contract_cluster_test.gopkg/engine/contract_errors_test.gopkg/engine/contract_machine_test.gopkg/engine/contract_network_legacy_test.gopkg/engine/contract_network_multidoc_test.gopkg/engine/helm/engine.gopkg/engine/render_test.go
🚧 Files skipped from review as they are similar to previous changes (8)
- charts/cozystack/values.yaml
- pkg/applycheck/refs.go
- pkg/engine/contract_errors_test.go
- docs/manual-test-plan.md
- pkg/engine/contract_network_legacy_test.go
- charts/generic/values.yaml
- charts/talm/templates/_helpers.tpl
- pkg/engine/contract_machine_test.go
myasnikovdaniil
left a comment
myasnikovdaniil
left a comment
There was a problem hiding this comment.
NOT LGTM — both original blockers are fixed; one new blocker came in with the fix round.
Previous findings — all resolved
registryTLShost unquoted —{{ $host | toYaml }}now applied at_helpers.tpl:405and:442. Verified: a bracketed IPv6 endpoint host renders asname: '[fd00::1]:5000'and both schemas parse.- Non-canonical IPv6 VIP — the new
ipCanonicaltemplate helper normalises both sides. Verified:vips[].ip: 2001:0DB8::5against a discovered2001:db8::5/64now strips correctly and emits a singleLayer2VIPConfig; two spellings of one address are also caught as a duplicate. - CIDR prefix length —
cidrPrefixLen < 0now checked onextraLinksaddresses and on route destinations.203.0.113.10/999and198.51.100.0/fooare both refused, with the document named. - Fractional
vlanId— digits-only check now runs before the range check;vlanId: 7.5is refused. LinkAliasConfig%dpatterns —unionCreatedLinkscontributes the pattern's prefix and admits references matching prefix+digits.aliasPatternPrefixmirrors machinery's ownValidateexactly (strings.Cuton%, rejectingsuffix != "d"and an empty prefix), so the accepted shape matches what the node will accept.- Bare
apiServer:— declining this with a rationale comment and a pinning test is a fine answer for a cosmetic pre-existing artifact.
The additional hardening in this round (mtu digits + kernel 68-65535 bounds, the timeServers shape guard, and folding six copied extraArgs loops into talm.render.component_args with map/slice rejection) all render and refuse as intended in local checks.
Blocker
B3: bond-slave guard prescribes a remediation that cannot work
File: charts/talm/templates/_helpers.tpl:1167-1168
Issue: The guard refuses to enslave a link that discovery reports carrying addresses or the default route, and its message offers two ways out:
Move those addresses and routes onto the bond entry, or enslave a link that carries neither.
The condition at :1167 tests only $slaveAddresses and $defaultLinkName — both derived from discovery. It never reads $extra.addresses. The first remediation therefore has no effect.
Evidence: With eth0 carrying 192.168.201.10/24 and the default route via 192.168.201.1, and an extraLinks entry declaring bond0 over [eth0, eth1] with that same address and gateway moved onto the bond entry, the render still fails with the identical message. Bonding an idle NIC (no addresses, not the default-route link) succeeds, so the guard is otherwise working as designed.
The guard's own comment states the intended contract — "They have to say so by putting the addresses on the bond" — so this is the implementation diverging from the stated design rather than a deliberate restriction.
TestContract_NetworkMultidoc_ExtraLinksBondSlaveWithAddresses_Fails (pkg/engine/contract_network_multidoc_test.go:2605) declares addresses on the bond, i.e. follows the prescribed remediation, and asserts failure — so the suite currently pins the misleading guidance instead of catching it.
Impact: enslaving an already-addressed NIC is unreachable through network.extraLinks. That is the headline scenario in #226 — bond0 as 802.3ad over two NICs carrying the default route — so the most common bonding migration cannot be expressed, and the error text sends the operator down a path that does not resolve it.
Fix: either accept the case when the bond entry declares addresses (matches the comment's stated intent), or drop the first clause from the message and narrow the test to pin only the genuine restriction. A test covering the accepted path — slave addresses moved onto the bond, render succeeds — would close the gap either way.
| {{- if has $slaveName $configurableLinks }} | ||
| {{- $slaveAddresses := fromJsonArray (include "talm.discovered.addresses_by_link" $slaveName) }} | ||
| {{- if or $slaveAddresses (eq $slaveName $defaultLinkName) }} | ||
| {{- fail (printf "talm: network.extraLinks bond %q enslaves %q, which discovery reports carrying its own addresses or the default route. A bond slave holds no addressing of its own, so this apply would take the node's connectivity away without saying where it goes. Move those addresses and routes onto the bond entry, or enslave a link that carries neither." ($extra.interface | toString) $slaveName) }} |
There was a problem hiding this comment.
B3 (blocker). The message offers "Move those addresses and routes onto the bond entry" as a way out, but the condition on line 1167 tests only $slaveAddresses and $defaultLinkName — both from discovery. It never reads $extra.addresses, so that remediation cannot unblock the render.
Verified: eth0 carrying 192.168.201.10/24 plus the default route via 192.168.201.1, and an extraLinks entry declaring bond0 over [eth0, eth1] with that same address and gateway moved onto the bond entry — the render still fails with this exact message. Bonding an idle NIC works, so the rest of the guard behaves as designed.
The comment above states the intent ("They have to say so by putting the addresses on the bond"), so the code is diverging from the stated design. This currently makes enslaving an already-addressed NIC unreachable — the bond0-over-two-NICs-carrying-the-default-route case from #226.
Either honour the intent (accept the case when the bond declares addresses), or drop the first clause so the message only offers the remediation that works.
| // fails instead of silently dropping them. Filtering the slave's document | ||
| // would take the node's addressing away without saying where it went, and | ||
| // the render cannot know whether the operator meant to move it. | ||
| func TestContract_NetworkMultidoc_ExtraLinksBondSlaveWithAddresses_Fails(t *testing.T) { |
There was a problem hiding this comment.
This test declares addresses: [203.0.113.10/24] on the bond — following the remediation the guard's error message prescribes — and asserts that it still fails. So the contract being pinned here is the misleading guidance rather than the genuine restriction.
Worth pairing with a positive case: slave addresses moved onto the bond entry, render succeeds. That test would have surfaced the mismatch, and it is the coverage that decides which way B3 gets fixed.
171afd8 to
3f158c9
Compare
myasnikovdaniil
left a comment
myasnikovdaniil
left a comment
There was a problem hiding this comment.
LGTM — every blocker from the previous rounds is resolved and independently verified; two narrow edges in the new bond-slave guard are left as follow-ups.
B3 resolved
The guard is now two independent checks (_helpers.tpl:1168 and :1171) instead of one, and both messages interpolate the discovered values so the operator is told which addresses and which gateway to restate. talm.discovered.gateway_by_link, used by the second message, is the pre-existing helper at :870.
Verified against a fixture where eth0 carries 192.168.201.10/24 plus the default route via 192.168.201.1:
- Declaring
bond0over[eth0, eth1]with that address and gateway restated on the bond entry now renders, and neither slave gets aLinkConfigof its own. - Omitting them still refuses, now naming
carrying its own addresses (192.168.201.10/24). - Bonding an idle NIC continues to work.
TestContract_NetworkMultidoc_ExtraLinksBondSlaveWithAddresses_Fails now enslaves eth1 — which carries 10.0.0.5/24 and is not the default-route link — so it exercises the addresses branch rather than pinning the old contradiction. The two added tests cover the accepted path and the forgot-the-route case, and the manual-test-plan section flags the bond migration as worth applying on real hardware, which is the right call given the render cannot prove the node keeps its route across the switch.
Local go test ./... is clean, including the golden renders — stock output is still byte-identical to before for both presets across controlplane/worker and legacy/multi-doc.
Non-blocking follow-ups
-
The default-route check can be satisfied without a default route (
_helpers.tpl:1171). The condition tests only that$extra.routesis non-empty, so a destination-scoped route passes it. Withroutes: [{gateway: 192.168.201.1, destination: 172.16.0.0/12}]on a bond over the default-route link, the render is accepted and the emittedBondConfigcarries only that scoped route — no default route anywhere. The guard's purpose is "so the node keeps its way out", and this satisfies the check without achieving it. Requiring at least one route with nodestinationwould close it. Non-blocking because it takes an operator actively writing a destination while omitting the default; the render does emit exactly what it was given. -
A slave's non-default static route is dropped without a word. A link carrying a scoped route (e.g.
10.0.0.0/24 via 10.0.0.1) has it on its own document; enslaving that link drops the document and the route, and the guard does not look at anything beyond addresses and the default route. Lower severity than losing connectivity, and arguably outside the guard's stated scope, but it is the same class of silent loss the guard exists to prevent — worth either extending the check or naming the limitation in the values documentation.
Earlier findings, all confirmed fixed
registryTLS host quoting on both schemas; IPv6 VIP canonicalisation via the new ipCanonical helper, covering the strip, the duplicate check and the extraLinks guard; full-CIDR prefix validation on addresses and route destinations; the integer check on vlanId ahead of the range check; and LinkAliasConfig %d patterns registering as a prefix so net0 resolves while the literal net%d does not — with the accepted pattern shape matching machinery's own Validate. The bare apiServer: nit was declined with a rationale and a pinning test, which is a reasonable answer for a pre-existing cosmetic artifact.
The hardening added along the way — mtu digits plus the kernel 68-65535 bounds, the timeServers shape guard, and folding six duplicated extraArgs loops into talm.render.component_args with map/slice rejection — all behaves as documented in local checks.
| {{- if and $slaveAddresses (not $extra.addresses) }} | ||
| {{- fail (printf "talm: network.extraLinks bond %q enslaves %q, which discovery reports carrying its own addresses (%s). A bond slave holds no addressing of its own, so this apply would take the node's connectivity away without saying where it goes. Put those addresses on the bond entry, or enslave a link that carries none." ($extra.interface | toString) $slaveName (join ", " $slaveAddresses)) }} | ||
| {{- end }} | ||
| {{- if and (eq $slaveName $defaultLinkName) (not $extra.routes) }} |
There was a problem hiding this comment.
Follow-up, non-blocking. This tests only that $extra.routes is non-empty, so a route that is not a default route satisfies it.
With routes: [{gateway: 192.168.201.1, destination: 172.16.0.0/12}] on a bond over the default-route link, the render is accepted and the emitted BondConfig is:
routes:
- gateway: 192.168.201.1
destination: 172.16.0.0/12No default route anywhere — the node comes back up without a way off its subnet, which is the outcome the message below is written to prevent.
Requiring at least one entry with no destination would close it. Not blocking: it takes an operator actively writing a destination while omitting the default, and the render faithfully emits what it was given.
| {{- range $linkName := $configurableLinks }} | ||
| {{- $link := lookup "links" "" $linkName }} | ||
| {{- if $link }} | ||
| {{- if and $link (not (has $linkName $bondSlaveNames)) }} |
There was a problem hiding this comment.
Follow-up, non-blocking. Filtering the slave here also drops any non-default static route it carried, and the guard above only inspects addresses and the default route.
Concretely: a link carrying 10.0.0.0/24 via 10.0.0.1 on its own document loses that route when enslaved, with no refusal and no warning. Less severe than losing connectivity, and arguably outside this guard's stated scope — but it is the same class of silent loss, so either extending the check to scoped routes or naming the limitation in the network.extraLinks values documentation would round it out.
Expose operator-visible values on the cozystack, generic, and talm charts
so a node's Talos config can be described from values.yaml instead of
forking the template:
- timeServers: machine.time.servers
- extra{ApiServer,ControllerManager,Scheduler}Args: passthrough
control-plane component args, coerced to strings (Talos wants
map[string]string) and guarded against colliding with preset built-ins
- registryMirrors: declarative machine registry mirrors (the default
docker.io -> mirror.gcr.io is preserved)
- registryTLS: TLS posture per registry endpoint host, keyed by endpoint
hostname rather than mirror name so a self-signed pull-through cache can
be trusted without touching the mirror list
- vips: multiple Layer2 VIPs, each pinned to a link
- network.preserveExisting: emit the running interfaces verbatim
- network.extraLinks: declare bonds, VLANs, and extra addresses, with the
same bond tuning the discovery path reconstructs (xmitHashPolicy,
lacpRate, miimon, updelay, downdelay) plus mtu and routes on both a link
and its VLAN children, so an externally-routed tagged uplink is
expressible without a template fork
Every knob defaults empty so a stock render stays byte-identical, pinned
by TestGoldenRender. Each input Talos would reject fails fast at render
with a hinted message: extraArgs colliding with a built-in, a duplicate or
dangling VIP, a VIP on a link already in the running config, a VLAN on a
missing parent, an empty bond, a route with no gateway, a malformed
registry entry, or a registryTLS entry that leaves its security posture
unstated. Contract tests cover the happy and rejected paths across the
legacy and multi-doc schemas for both presets.
An extraLinks bond slave gets no document of its own, matching the filter
discovery applies once the bond exists. Moving an already-addressed NIC
into a bond works once the entry restates everything that link was
carrying — its addresses, a destination-less route where it held the
default one, and a matching route per static destination. Anything left
behind fails the render, naming what to restate, rather than costing the
node its connectivity or a reachable subnet. VIP addresses are compared
canonically, so a VIP spelled differently from what the node reports is
still stripped from the static addresses rather than shipping twice.
pkg/applycheck learns which links a config creates, so a VLANConfig.parent,
BondConfig.links[] or Layer2VIPConfig.link pointing at one of them resolves
instead of blocking the first apply — without it a declared bond is
unusable. A LinkAliasConfig name ending in %d is registered as a pattern,
because Talos expands it into one alias per matched link.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
5e8be43
3f158c9 to
5e8be43
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/engine/contract_network_multidoc_test.go (1)
2383-2561: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winError-path cases here drop
advertisedSubnets, unlike the accept-path tests.Most of these rely solely on a message substring to prove the intended guard fired. For cases whose assertion is just an interface name (Line 2620
"eth1", Line 2763"eth9"), an unrelated render failure caused by the missing base values would satisfy the assertion. Consider includingadvertisedSubnetsconsistently in the failure cases, or asserting on a guard-specific phrase.🤖 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/contract_network_multidoc_test.go` around lines 2383 - 2561, Add the required advertisedSubnets fixture to the affected error-path inputs in the network multidoc contract tests, matching the accept-path setup so failures reach the intended validation guard. In particular, update cases such as TestContract_NetworkMultidoc_ExtraLinksAddressIsVipAcrossSpellings_Fails and other eth1/eth9 assertions, or strengthen their checks to require a guard-specific error phrase rather than only an interface name.
🤖 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 `@pkg/engine/contract_network_multidoc_test.go`:
- Around line 2383-2561: Add the required advertisedSubnets fixture to the
affected error-path inputs in the network multidoc contract tests, matching the
accept-path setup so failures reach the intended validation guard. In
particular, update cases such as
TestContract_NetworkMultidoc_ExtraLinksAddressIsVipAcrossSpellings_Fails and
other eth1/eth9 assertions, or strengthen their checks to require a
guard-specific error phrase rather than only an interface name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a79d8c7c-0fea-4d31-8bb8-5c484ac060f2
📒 Files selected for processing (17)
README.mdcharts/cozystack/templates/_helpers.tplcharts/cozystack/values.yamlcharts/generic/templates/_helpers.tplcharts/generic/values.yamlcharts/talm/templates/_helpers.tpldocs/manual-test-plan.mdpkg/applycheck/refs.gopkg/applycheck/validate.gopkg/applycheck/validate_test.gopkg/engine/contract_cluster_test.gopkg/engine/contract_errors_test.gopkg/engine/contract_machine_test.gopkg/engine/contract_network_legacy_test.gopkg/engine/contract_network_multidoc_test.gopkg/engine/helm/engine.gopkg/engine/render_test.go
🚧 Files skipped from review as they are similar to previous changes (15)
- pkg/engine/helm/engine.go
- charts/generic/values.yaml
- pkg/engine/contract_errors_test.go
- pkg/applycheck/validate.go
- pkg/applycheck/refs.go
- pkg/engine/render_test.go
- charts/generic/templates/_helpers.tpl
- pkg/applycheck/validate_test.go
- charts/cozystack/values.yaml
- docs/manual-test-plan.md
- charts/cozystack/templates/_helpers.tpl
- pkg/engine/contract_cluster_test.go
- pkg/engine/contract_network_legacy_test.go
- pkg/engine/contract_machine_test.go
- charts/talm/templates/_helpers.tpl
3 participants
New values so a node's Talos config can come from
values.yamlinstead of a template fork:timeServerssetsmachine.time.serversextraApiServerArgs/extraControllerManagerArgs/extraSchedulerArgspass through control-plane component argsregistryMirrorsdeclares machine registry mirrorsregistryTLSsets TLS posture per registry endpoint host (a private CA, or skipping verification for a self-signed cache)vipsgives multiple Layer2 VIPs, each pinned to a linknetwork.preserveExistingkeeps the running interfaces verbatimnetwork.extraLinksdeclares bonds, VLANs, and extra addresses, with bond tuning, mtu and routesCovers #226, #227, #228, #229, #230.
Every knob defaults to empty, so a stock render is byte-identical to before.
TestGoldenRenderpins that across both presets, control-plane and worker, legacy and multi-doc. Nothing changes for existing users until they opt in.Anything Talos would reject fails fast at render with a hinted message instead of shipping a broken config: an
extraArgskey colliding with a preset built-in, a duplicate or dangling VIP, a VIP on a link already in the running interfaces, a VLAN on a missing parent, an empty or mode-less bond, a route without a gateway, a malformed registry entry. Component args aremap[string]stringon the Talos side, so values are quoted to keep an unquoted numeric from becoming a YAML int.The apply gate learned that a link this config creates (a bond or VLAN from
extraLinks) satisfies references to it from sibling documents, so an extraLinks bond with a VLAN and VIP on top passes on first apply instead of blocking.Contract tests cover the accepted and rejected path for every knob, on both schemas and both presets.
docs/manual-test-plan.mdgets a matching section.One follow-up, not in this PR: operator docs on cozystack/website for the new values.
Summary by CodeRabbit
New Features
timeServers, componentextra*Args,registryMirrors, andregistryTLS.vips, and improved Talos v1.12+ multi-document network output.network.preserveExistingandnetwork.extraLinksfor richer declarative topology (including bonds/VLANs/addresses/routes).Bug Fixes
Documentation
Tests