feat(config): migrate to Talos v1.12 multi-document config format#116
feat(config): migrate to Talos v1.12 multi-document config format#116
Conversation
![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 introduces support for the Talos v1.12+ multi-document configuration format in the cozystack and generic charts, while maintaining legacy support for older versions. The rendering engine is updated to pass the Talos version into the template context, and comprehensive integration tests are added to verify both output formats. Review feedback identifies several schema mismatches in the new multi-document resources, including incorrect field names for BondConfig and VLANConfig, and incorrect list structures for ResolverConfig, LinkConfig, and RegistryMirrorConfig. Additionally, a safety check is needed for the lookup function in BondConfig generation to prevent template rendering errors.
| nameservers: | ||
| {{- $resolvers := include "talm.discovered.default_resolvers" . }} | ||
| {{- if $resolvers }} | ||
| {{- range fromJsonArray $resolvers }} | ||
| - address: {{ . | quote }} | ||
| {{- end }} | ||
| {{- else }} | ||
| [] | ||
| {{- end }} |
There was a problem hiding this comment.
In Talos v1.12+, the ResolverConfig resource expects nameservers to be a list of strings (IP addresses), not a list of objects with an address key.
nameservers:
{{- $resolvers := include "talm.discovered.default_resolvers" . }}
{{- if $resolvers }}
{{- range fromJsonArray $resolvers }}
- {{ . | quote }}
{{- end }}
{{- else }}
[]
{{- end }}
There was a problem hiding this comment.
The current format is correct per the Go type definitions.
ResolverConfigV1Alpha1 has ResolverNameservers []NameserverConfig with yaml tag nameservers,omitempty, where NameserverConfig has Address Addr with yaml tag address.
Source: resolver.go:63 and resolver.go:81
The official test data (testdata/resolverconfig.yaml) confirms the format:
nameservers:
- address: 10.0.0.1
- address: 2001:4860:4860::8888| {{- if $isBondInterface }} | ||
| {{- $link := lookup "links" "" $interfaceName }} | ||
| {{- $bondMaster := $link.spec.bondMaster }} | ||
| {{- $slaves := fromJsonArray (include "talm.discovered.bond_slaves" $link.spec.index) }} | ||
| --- | ||
| apiVersion: v1alpha1 | ||
| kind: BondConfig | ||
| name: {{ $interfaceName }} | ||
| links: | ||
| {{- range $slaves }} | ||
| - {{ . }} | ||
| {{- end }} | ||
| bondMode: {{ $bondMaster.mode }} | ||
| {{- if $bondMaster.xmitHashPolicy }} | ||
| xmitHashPolicy: {{ $bondMaster.xmitHashPolicy }} | ||
| {{- end }} | ||
| {{- if $bondMaster.lacpRate }} | ||
| lacpRate: {{ $bondMaster.lacpRate }} | ||
| {{- end }} | ||
| {{- if $bondMaster.miimon }} | ||
| miimon: {{ $bondMaster.miimon }} | ||
| {{- end }} | ||
| {{- if $bondMaster.updelay }} | ||
| updelay: {{ $bondMaster.updelay }} | ||
| {{- end }} | ||
| {{- if $bondMaster.downdelay }} | ||
| downdelay: {{ $bondMaster.downdelay }} | ||
| {{- end }} | ||
| {{- if not $isVlan }} | ||
| addresses: | ||
| {{- range fromJsonArray (include "talm.discovered.default_addresses_by_gateway" .) }} | ||
| - address: {{ . }} | ||
| {{- end }} | ||
| routes: | ||
| - gateway: {{ include "talm.discovered.default_gateway" . }} | ||
| {{- end }} | ||
| {{- end }} |
There was a problem hiding this comment.
There are several schema and safety issues in the BondConfig generation:
- The
lookupfunction can return an empty map if the resource is not found. Accessing$link.specwithout checking if it exists will cause a template rendering error. - The field for bond mode is
mode, notbondMode. - The
addressesfield expects a list of strings. - The
routesfield entries require anetworkfield (e.g.,0.0.0.0/0).
{{- if $isBondInterface }}
{{- $link := lookup "links" "" $interfaceName }}
{{- if $link }}
{{- $bondMaster := $link.spec.bondMaster }}
{{- $slaves := fromJsonArray (include "talm.discovered.bond_slaves" $link.spec.index) }}
---
apiVersion: v1alpha1
kind: BondConfig
name: {{ $interfaceName }}
links:
{{- range $slaves }}
- {{ . }}
{{- end }}
mode: {{ $bondMaster.mode }}
{{- if $bondMaster.xmitHashPolicy }}
xmitHashPolicy: {{ $bondMaster.xmitHashPolicy }}
{{- end }}
{{- if $bondMaster.lacpRate }}
lacpRate: {{ $bondMaster.lacpRate }}
{{- end }}
{{- if $bondMaster.miimon }}
miimon: {{ $bondMaster.miimon }}
{{- end }}
{{- if $bondMaster.updelay }}
updelay: {{ $bondMaster.updelay }}
{{- end }}
{{- if $bondMaster.downdelay }}
downdelay: {{ $bondMaster.downdelay }}
{{- end }}
{{- if not $isVlan }}
addresses:
{{- range fromJsonArray (include "talm.discovered.default_addresses_by_gateway" .) }}
- {{ . }}
{{- end }}
routes:
- network: 0.0.0.0/0
gateway: {{ include "talm.discovered.default_gateway" . }}
{{- end }}
{{- end }}
{{- end }}
There was a problem hiding this comment.
Schema claims are incorrect, but the lookup safety point is valid.
bondMode is correct — BondConfigV1Alpha1 has BondMode *nethelpers.BondMode with yaml tag bondMode,omitempty (bond.go:96). Test data confirms: bondMode: 802.3ad.
addresses format is correct — AddressConfig has AddressAddress netip.Prefix with yaml tag address (link.go:122). Format is - address: IP/prefix, not bare strings.
routes without destination is valid — RouteDestination has yaml tag destination,omitempty (NOT network). The field is optional: "If not specified, a default route will be created for the address family of the gateway." (link.go:133-135)
Lookup safety — $isBondInterface calls talm.discovered.is_bond which already performs lookup "links" "" $interfaceName and checks $link.spec.kind == "bond". If it returned true, the resource is guaranteed to exist. Nevertheless, adding a guard is reasonable defensive coding. Will fix.
| apiVersion: v1alpha1 | ||
| kind: VLANConfig | ||
| name: {{ $defaultLinkName }} | ||
| vlanID: {{ include "talm.discovered.vlan_id" $defaultLinkName }} | ||
| parent: {{ $interfaceName }} | ||
| addresses: | ||
| {{- range fromJsonArray (include "talm.discovered.default_addresses_by_gateway" .) }} | ||
| - address: {{ . }} | ||
| {{- end }} | ||
| routes: | ||
| - gateway: {{ include "talm.discovered.default_gateway" . }} |
There was a problem hiding this comment.
The VLANConfig resource has schema mismatches:
- The field name is
vlanId, notvlanID. addressesshould be a list of strings.routesentries require anetworkfield.
kind: VLANConfig
name: {{ $defaultLinkName }}
vlanId: {{ include "talm.discovered.vlan_id" $defaultLinkName }}
parent: {{ $interfaceName }}
addresses:
{{- range fromJsonArray (include "talm.discovered.default_addresses_by_gateway" .) }}
- {{ . }}
{{- end }}
routes:
- network: 0.0.0.0/0
gateway: {{ include "talm.discovered.default_gateway" . }}
There was a problem hiding this comment.
All three claims are incorrect.
vlanID is correct — VLANConfigV1Alpha1 has VLANIDConfig uint16 with yaml tag vlanID,omitempty (vlan.go:69). Test data confirms: vlanID: 2.
addresses format is correct — uses AddressConfig with address key, same as LinkConfig/BondConfig.
routes without destination is valid — RouteDestination is omitempty, not required.
| kind: LinkConfig | ||
| name: {{ $interfaceName }} | ||
| addresses: | ||
| {{- range fromJsonArray (include "talm.discovered.default_addresses_by_gateway" .) }} | ||
| - address: {{ . }} | ||
| {{- end }} | ||
| routes: | ||
| - gateway: {{ include "talm.discovered.default_gateway" . }} |
There was a problem hiding this comment.
The LinkConfig resource expects addresses to be a list of strings and routes to include a network field.
kind: LinkConfig
name: {{ $interfaceName }}
addresses:
{{- range fromJsonArray (include "talm.discovered.default_addresses_by_gateway" .) }}
- {{ . }}
{{- end }}
routes:
- network: 0.0.0.0/0
gateway: {{ include "talm.discovered.default_gateway" . }}
There was a problem hiding this comment.
Both claims are incorrect.
addresses uses AddressConfig with yaml tag address — format is - address: IP/prefix, not bare strings (link.go:98,122).
routes — RouteDestination has yaml tag destination,omitempty. The destination field is optional, not required. There is no network field in RouteConfig. A route with only gateway creates a default route.
| kind: Layer2VIPConfig | ||
| name: {{ .Values.floatingIP | quote }} | ||
| link: {{ $interfaceName }} |
There was a problem hiding this comment.
The Layer2VIPConfig resource uses the ip field for the VIP address, not the name field.
kind: Layer2VIPConfig
ip: {{ .Values.floatingIP | quote }}
link: {{ $interfaceName }}
There was a problem hiding this comment.
Incorrect. The VIP IP address is stored in the name field.
Layer2VIPConfigV1Alpha1 has MetaName string with yaml tag name (layer2_vip.go:65). The VIP() method parses this field as an IP address (layer2_vip.go:128-131). Validation at line 111 confirms MetaName must be a valid IP.
Test data confirms: name: 1.2.3.4. There is no ip field in this type.
| endpoints: | ||
| - url: https://mirror.gcr.io |
There was a problem hiding this comment.
The RegistryMirrorConfig resource expects endpoints to be a list of strings (URLs), not a list of objects with a url key.
endpoints:
- https://mirror.gcr.io
There was a problem hiding this comment.
Incorrect. Endpoints are objects, not strings.
RegistryMirrorConfigV1Alpha1 has RegistryEndpoints []RegistryEndpoint with yaml tag endpoints,omitempty. RegistryEndpoint has EndpointURL meta.URL with yaml tag url AND EndpointOverridePath *bool with yaml tag overridePath,omitempty (registry_mirror.go:69,87,90).
Endpoints must be objects because they support overridePath. Test data confirms:
endpoints:
- url: https://my-private-registry.local:5000
- url: http://my-harbor/v2/registry-k8s.io/
overridePath: true| nameservers: | ||
| {{- $resolvers := include "talm.discovered.default_resolvers" . }} | ||
| {{- if $resolvers }} | ||
| {{- range fromJsonArray $resolvers }} | ||
| - address: {{ . | quote }} | ||
| {{- end }} | ||
| {{- else }} | ||
| [] | ||
| {{- end }} |
There was a problem hiding this comment.
The ResolverConfig resource expects nameservers to be a list of strings.
nameservers:
{{- $resolvers := include "talm.discovered.default_resolvers" . }}
{{- if $resolvers }}
{{- range fromJsonArray $resolvers }}
- {{ . | quote }}
{{- end }}
{{- else }}
[]
{{- end }}
There was a problem hiding this comment.
Same as above — the format is correct. NameserverConfig has Address Addr with yaml tag address. See resolver.go:63,81.
| {{- if $isBondInterface }} | ||
| {{- $link := lookup "links" "" $interfaceName }} | ||
| {{- $bondMaster := $link.spec.bondMaster }} | ||
| {{- $slaves := fromJsonArray (include "talm.discovered.bond_slaves" $link.spec.index) }} | ||
| --- | ||
| apiVersion: v1alpha1 | ||
| kind: BondConfig | ||
| name: {{ $interfaceName }} | ||
| links: | ||
| {{- range $slaves }} | ||
| - {{ . }} | ||
| {{- end }} | ||
| bondMode: {{ $bondMaster.mode }} | ||
| {{- if $bondMaster.xmitHashPolicy }} | ||
| xmitHashPolicy: {{ $bondMaster.xmitHashPolicy }} | ||
| {{- end }} | ||
| {{- if $bondMaster.lacpRate }} | ||
| lacpRate: {{ $bondMaster.lacpRate }} | ||
| {{- end }} | ||
| {{- if $bondMaster.miimon }} | ||
| miimon: {{ $bondMaster.miimon }} | ||
| {{- end }} | ||
| {{- if $bondMaster.updelay }} | ||
| updelay: {{ $bondMaster.updelay }} | ||
| {{- end }} | ||
| {{- if $bondMaster.downdelay }} | ||
| downdelay: {{ $bondMaster.downdelay }} | ||
| {{- end }} | ||
| {{- if not $isVlan }} | ||
| addresses: | ||
| {{- range fromJsonArray (include "talm.discovered.default_addresses_by_gateway" .) }} | ||
| - address: {{ . }} | ||
| {{- end }} | ||
| routes: | ||
| - gateway: {{ include "talm.discovered.default_gateway" . }} | ||
| {{- end }} | ||
| {{- end }} |
There was a problem hiding this comment.
The BondConfig generation has several schema and safety issues: missing lookup result check, incorrect mode field name, and incorrect addresses/routes structure.
{{- if $isBondInterface }}
{{- $link := lookup "links" "" $interfaceName }}
{{- if $link }}
{{- $bondMaster := $link.spec.bondMaster }}
{{- $slaves := fromJsonArray (include "talm.discovered.bond_slaves" $link.spec.index) }}
---
apiVersion: v1alpha1
kind: BondConfig
name: {{ $interfaceName }}
links:
{{- range $slaves }}
- {{ . }}
{{- end }}
mode: {{ $bondMaster.mode }}
{{- if $bondMaster.xmitHashPolicy }}
xmitHashPolicy: {{ $bondMaster.xmitHashPolicy }}
{{- end }}
{{- if $bondMaster.lacpRate }}
lacpRate: {{ $bondMaster.lacpRate }}
{{- end }}
{{- if $bondMaster.miimon }}
miimon: {{ $bondMaster.miimon }}
{{- end }}
{{- if $bondMaster.updelay }}
updelay: {{ $bondMaster.updelay }}
{{- end }}
{{- if $bondMaster.downdelay }}
downdelay: {{ $bondMaster.downdelay }}
{{- end }}
{{- if not $isVlan }}
addresses:
{{- range fromJsonArray (include "talm.discovered.default_addresses_by_gateway" .) }}
- {{ . }}
{{- end }}
routes:
- network: 0.0.0.0/0
gateway: {{ include "talm.discovered.default_gateway" . }}
{{- end }}
{{- end }}
{{- end }}
There was a problem hiding this comment.
Schema claims are incorrect (see replies on the cozystack counterpart). The lookup safety point is valid though — adding a guard. Fixed.
| apiVersion: v1alpha1 | ||
| kind: VLANConfig | ||
| name: {{ $defaultLinkName }} | ||
| vlanID: {{ include "talm.discovered.vlan_id" $defaultLinkName }} | ||
| parent: {{ $interfaceName }} | ||
| addresses: | ||
| {{- range fromJsonArray (include "talm.discovered.default_addresses_by_gateway" .) }} | ||
| - address: {{ . }} | ||
| {{- end }} | ||
| routes: | ||
| - gateway: {{ include "talm.discovered.default_gateway" . }} |
There was a problem hiding this comment.
The VLANConfig resource expects vlanId (not vlanID), a list of strings for addresses, and a network field in routes.
kind: VLANConfig
name: {{ $defaultLinkName }}
vlanId: {{ include "talm.discovered.vlan_id" $defaultLinkName }}
parent: {{ $interfaceName }}
addresses:
{{- range fromJsonArray (include "talm.discovered.default_addresses_by_gateway" .) }}
- {{ . }}
{{- end }}
routes:
- network: 0.0.0.0/0
gateway: {{ include "talm.discovered.default_gateway" . }}
There was a problem hiding this comment.
All claims incorrect — see replies on the cozystack counterpart. vlanID is correct yaml tag, addresses uses AddressConfig objects, routes.destination is optional.
📝 WalkthroughWalkthroughTemplates and engine rendering were updated to support Talos v1.12+ multi-document config output, with version-gated template branches (multi-doc vs legacy), injected Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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
force-pushed
the
feat/talos-v1.12-multidoc-config
branch
from
3fd6504 to
52eb508
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/engine/render_test.go (1)
298-323: Consider restoringLookupFuncafter modification for test isolation.This test and
TestMultiDocCozystack_NrHugepagesmodify the package-levelhelmEngine.LookupFuncwithout restoring it. While safe for sequential tests, consider adding cleanup for future-proofing if tests are parallelized:🔧 Suggested cleanup pattern
func TestLegacyCozystack_NrHugepages(t *testing.T) { // Test nr_hugepages is rendered correctly in legacy format + origLookup := helmEngine.LookupFunc + defer func() { helmEngine.LookupFunc = origLookup }() + helmEngine.LookupFunc = func(string, string, string) (map[string]any, error) { return map[string]any{}, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/engine/render_test.go` around lines 298 - 323, This test (and TestMultiDocCozystack_NrHugepages) mutates the package-level helmEngine.LookupFunc without restoring it; save the original (e.g., orig := helmEngine.LookupFunc) before assigning the stub and defer restoring it (defer func() { helmEngine.LookupFunc = orig }()) so the package state is reset after TestLegacyCozystack_NrHugepages and the other test, ensuring isolation when tests run in parallel.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/engine/render_test.go`:
- Around line 298-323: This test (and TestMultiDocCozystack_NrHugepages) mutates
the package-level helmEngine.LookupFunc without restoring it; save the original
(e.g., orig := helmEngine.LookupFunc) before assigning the stub and defer
restoring it (defer func() { helmEngine.LookupFunc = orig }()) so the package
state is reset after TestLegacyCozystack_NrHugepages and the other test,
ensuring isolation when tests run in parallel.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c75fd86c-523c-49ff-893e-105b7bf84efb
📒 Files selected for processing (8)
README.mdcharts/cozystack/templates/_helpers.tplcharts/generic/templates/_helpers.tplpkg/engine/engine.gopkg/engine/engine_test.gopkg/engine/helm/engine.gopkg/engine/helm/engine_test.gopkg/engine/render_test.go
✅ Files skipped from review due to trivial changes (3)
- README.md
- pkg/engine/engine_test.go
- charts/cozystack/templates/_helpers.tpl
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
myasnikovdaniil
left a comment
myasnikovdaniil
left a comment
There was a problem hiding this comment.
Bug
Malformed TalosVersion causes opaque template crash
A non-semver TalosVersion (e.g. latest, foobar) passes the and .TalosVersion truthiness check but semverCompare returns an error, producing:
error calling semverCompare: invalid semantic version
The clean validation via ParseContractFromVersion in engine.go runs after template rendering, so it never gets a chance to produce a user-friendly error. Custom templates that don't use semverCompare reach the later validation and get a better message: invalid talos-version: error parsing version "vlatest".
Suggestion: Validate TalosVersion before eng.Render() — e.g. call semver.NewVersion(opts.TalosVersion) early in Render() and return a clean error.
Test coverage gaps
BondConfig and VLANConfig paths are never exercised
The offline LookupFunc always returns empty maps, so is_bond/is_vlan always evaluate to false. No test ever emits a BondConfig or VLANConfig document. The hasLinkConfig || hasBondConfig assertion always passes via LinkConfig. Consider adding tests with a custom LookupFunc returning mock bond/vlan resources.
Layer2VIPConfig is never positively asserted
The cozystack defaults include floatingIP: 192.168.100.10, so Layer2VIPConfig is present in the controlplane multi-doc output — but no test checks for it. All four Layer2VIPConfig references are assertNotContains. Adding assertContains(t, output, "kind: Layer2VIPConfig") to TestMultiDocCozystack_ControlPlane would close this gap.
Nits
README note could be more precise — lists all 7 document types as always-present, but BondConfig/VLANConfig/Layer2VIPConfig are conditional on topology, and RegistryMirrorConfig is cozystack-only. A qualifier like "depending on network topology and chart preset" would help.
Comment on talos.config.machine.common (cozystack) omits nodeLabels — says "type, kubelet, sysctls, kernel, certSANs, files, install" but the template also renders nodeLabels for controlplane nodes.
Add support for the Talos v1.12 multi-document configuration format. When TalosVersion is set to v1.12 or later (via Chart.yaml or --talos-version CLI flag), templates generate separate YAML documents instead of the deprecated monolithic machine.network and machine.registries fields. Closes #100 Engine: - Pass TalosVersion through the Helm values map into the template rendering context (available as .TalosVersion), avoiding global mutable state - Remove unused ctx and chrt parameters from applyPatchesAndRenderConfig Chart templates (cozystack + generic): - Split talos.config into shared sub-templates to eliminate duplication: talos.config.machine.common, talos.config.cluster, talos.config.network.legacy, talos.config.network.multidoc - Version dispatcher selects legacy or multi-doc format based on semverCompare - New v1.12 document types generated: HostnameConfig, ResolverConfig, LinkConfig, BondConfig, VLANConfig, RegistryMirrorConfig, Layer2VIPConfig Bug fix: - Fix nr_hugepages rendering inside with block (was producing empty string for non-zero values due to .Values being inaccessible when dot is rebound) Backward compatibility: - Legacy format is fully preserved when TalosVersion is empty or < v1.12 - All legacy regression tests pass unchanged Tests: - isTalosConfigPatch and extractExtraDocuments tests for all 7 new document types - Offline rendering tests for both charts x both roles x both formats - Version edge cases: pre-release, two-component version strings - Concurrent render test with race detector - nr_hugepages rendering test for both legacy and multi-doc paths - Regression test from #119 for offline lookup producing empty interface Code style: - Replace interface{} with any across engine package - Use maps.Copy instead of range+assign loops - Use range over int for simple counting loops - Use strings.Repeat instead of += concatenation Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
Call ParseContractFromVersion early in Render() so malformed values (e.g. "latest", "foobar") surface a user-friendly "invalid talos-version" error instead of the opaque "semverCompare: invalid semantic version" that otherwise escapes from deep inside the Helm engine template evaluation. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
…onfig - TestRenderInvalidTalosVersion: exercises the new early validation. - TestMultiDocCozystack_BondTopology / TestMultiDocGeneric_BondTopology: assert BondConfig emission with mock links/routes/addresses lookups. - TestMultiDocCozystack_VlanOnBondTopology / TestMultiDocGeneric_VlanOnBondTopology: assert BondConfig + VLANConfig combined emission when the default route goes through a VLAN. - TestMultiDocCozystack_ControlPlane: add positive assert for Layer2VIPConfig since cozystack defaults set floatingIP. - TestLegacy/MultiDocCozystack_NrHugepages: restore the package-level helmEngine.LookupFunc via t.Cleanup for test isolation. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
- README: clarify that not all multi-doc types are always present — HostnameConfig/ResolverConfig and one network interface document are always emitted, Layer2VIPConfig depends on floatingIP on controlplane, and RegistryMirrorConfig is cozystack-only. - charts/cozystack _helpers.tpl: the talos.config.machine.common helper also renders nodeLabels for controlplane nodes — update the comment. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
lexfrei
force-pushed
the
feat/talos-v1.12-multidoc-config
branch
from
52eb508 to
7f9957b
Compare
|
Addressed review feedback. Force-pushed on rebase onto main. Changes since last review🐛 Bug — invalid 📊 Coverage — BondConfig / VLANConfig
📊 Coverage — Layer2VIPConfig positive assert 🧹 Test isolation 📝 README 📝 Comment fix 🔒 Lookup safety (already in place) Gemini feedbackAll schema claims from the bot-review (bare-string addresses, Tests: |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
pkg/engine/render_test.go (1)
36-39: Restore the global lookup hook after each helper call.
renderChartTemplateoverwriteshelmEngine.LookupFuncand leaves the package-global in that state. That makes later tests order-dependent and blocks safe parallelization of this suite.♻️ Suggested cleanup
func renderChartTemplate(t *testing.T, chartPath string, templateFile string, talosVersion ...string) string { t.Helper() + origLookup := helmEngine.LookupFunc + t.Cleanup(func() { helmEngine.LookupFunc = origLookup }) + // Reset to offline mode helmEngine.LookupFunc = func(string, string, string) (map[string]any, error) { return map[string]any{}, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/engine/render_test.go` around lines 36 - 39, renderChartTemplate currently overwrites the package-global helmEngine.LookupFunc and leaves it modified, making tests order-dependent; fix this by saving the original value of helmEngine.LookupFunc at the start of renderChartTemplate, then restore it (preferably via defer) after the helper finishes so the global is returned to its previous state; update renderChartTemplate to perform the assignment to helmEngine.LookupFunc for the offline stub only for the duration of the call and restore the saved original LookupFunc before returning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@charts/cozystack/templates/_helpers.tpl`:
- Around line 127-131: The multi-doc helper "talos.config.network.multidoc"
currently reconstructs network config for v1.12+ and ignores
talm.discovered.existing_interfaces_configuration; update the helper to preserve
that value by emitting it as a named document (e.g.,
"existing_interfaces_configuration") alongside the LinkConfig/BondConfig docs
when talm.discovered.existing_interfaces_configuration is present, or
alternatively add an explicit fast-fail validation at the top of
talos.config.network.multidoc that returns an error if
talm.discovered.existing_interfaces_configuration is set (to avoid silent data
loss when TalosVersion becomes v1.12+). Ensure the change references the helper
"talos.config.network.multidoc" and the variable
talm.discovered.existing_interfaces_configuration so callers get the preserved
data or a clear failure.
- Around line 224-230: The Layer2VIPConfig currently sets link to $interfaceName
which has been rewritten to the parent link in the VLAN path and moves the VIP
off the VLAN; update the template so Layer2VIPConfig.link uses the VLAN-emitted
interface variable $defaultLinkName instead of $interfaceName (keep the
surrounding condition using .Values.floatingIP and .MachineType and the resource
kind Layer2VIPConfig intact) so the VIP remains attached to the configured VLAN.
In `@charts/generic/templates/_helpers.tpl`:
- Around line 147-153: The Layer2VIPConfig block is attaching the VIP to
$interfaceName which has been rewritten to the parent link; replace the link
reference so the VIP stays on the VLAN by using $defaultLinkName instead of
$interfaceName in the link field of the Layer2VIPConfig (the conditional stays
the same: check .Values.floatingIP and .MachineType), ensuring the VIP is
emitted on the VLAN interface; update the template where Layer2VIPConfig's link:
is set to reference $defaultLinkName.
---
Nitpick comments:
In `@pkg/engine/render_test.go`:
- Around line 36-39: renderChartTemplate currently overwrites the package-global
helmEngine.LookupFunc and leaves it modified, making tests order-dependent; fix
this by saving the original value of helmEngine.LookupFunc at the start of
renderChartTemplate, then restore it (preferably via defer) after the helper
finishes so the global is returned to its previous state; update
renderChartTemplate to perform the assignment to helmEngine.LookupFunc for the
offline stub only for the duration of the call and restore the saved original
LookupFunc before returning.
🪄 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
Run ID: c1b72b25-39ca-4df0-bed9-306a4d0524dc
📒 Files selected for processing (8)
README.mdcharts/cozystack/templates/_helpers.tplcharts/generic/templates/_helpers.tplpkg/engine/engine.gopkg/engine/engine_test.gopkg/engine/helm/engine.gopkg/engine/helm/engine_test.gopkg/engine/render_test.go
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/engine/helm/engine.go
- pkg/engine/engine.go
- pkg/engine/engine_test.go
| {{- define "talos.config.network.multidoc" }} | ||
| {{- /* Multi-doc format always reconstructs network config from discovery resources. | ||
| existing_interfaces_configuration is not used here because v1.12 nodes store | ||
| network config in separate documents (LinkConfig, BondConfig, etc.), not in | ||
| the legacy machine.network.interfaces field. */ -}} |
There was a problem hiding this comment.
Preserve existing_interfaces_configuration in the multi-doc branch.
The legacy path consumes talm.discovered.existing_interfaces_configuration, but the v1.12 path ignores it and always rebuilds from the default-route interface. That silently drops any non-default interface state as soon as TalosVersion flips to v1.12+. Please translate that helper output into named docs here, or fail fast when it is present.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@charts/cozystack/templates/_helpers.tpl` around lines 127 - 131, The
multi-doc helper "talos.config.network.multidoc" currently reconstructs network
config for v1.12+ and ignores talm.discovered.existing_interfaces_configuration;
update the helper to preserve that value by emitting it as a named document
(e.g., "existing_interfaces_configuration") alongside the LinkConfig/BondConfig
docs when talm.discovered.existing_interfaces_configuration is present, or
alternatively add an explicit fast-fail validation at the top of
talos.config.network.multidoc that returns an error if
talm.discovered.existing_interfaces_configuration is set (to avoid silent data
loss when TalosVersion becomes v1.12+). Ensure the change references the helper
"talos.config.network.multidoc" and the variable
talm.discovered.existing_interfaces_configuration so callers get the preserved
data or a clear failure.
There was a problem hiding this comment.
Intentional. talm.discovered.existing_interfaces_configuration emits a legacy machine.network.interfaces YAML fragment, which has no safe 1:1 translation to the v1.12 multi-doc resources (LinkConfig / BondConfig / VLANConfig / Layer2VIPConfig). Rather than silently producing a partial or incorrect translation, the multidoc path rebuilds the required documents from the discovery resources directly; the comment at the top of talos.config.network.multidoc documents this. Users who rely on custom legacy interface state should keep machine.talosVersion on v1.11.x until they migrate their overrides to the new resources.
There was a problem hiding this comment.
@lexfrei, understood — that's a sound design rationale. Silently emitting a partial or incorrectly translated fragment would be far more dangerous than a clean rebuild from discovery resources, and the existing comment in the template documents the deliberate omission clearly. I'll note this for future reviews.
(ʕ •ᴥ•ʔ)≡
✏️ Learnings added
Learnt from: lexfrei
Repo: cozystack/talm PR: 116
File: charts/cozystack/templates/_helpers.tpl:127-131
Timestamp: 2026-04-14T15:31:55.570Z
Learning: In `charts/cozystack/templates/_helpers.tpl` and `charts/generic/templates/_helpers.tpl`, the `talos.config.network.multidoc` template intentionally ignores `talm.discovered.existing_interfaces_configuration`. That helper emits a legacy `machine.network.interfaces` YAML fragment that has no safe 1:1 translation to Talos v1.12 multi-doc resources (LinkConfig / BondConfig / VLANConfig / Layer2VIPConfig). The multidoc path rebuilds all required network documents directly from discovery resources instead. Users relying on custom legacy interface state must stay on Talos v1.11.x until they migrate their overrides to the new v1.12 resource types.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: lexfrei
Repo: cozystack/talm PR: 113
File: pkg/commands/apply.go:116-136
Timestamp: 2026-04-02T08:59:42.636Z
Learning: In the talm project (pkg/commands/apply.go and pkg/commands/template.go), when a node file (e.g. nodes/node0.yaml) is passed via `-f`, it serves only as a modeline container providing metadata (nodes, endpoints, template paths). The actual cluster configuration is produced entirely by Helm templates + values.yaml. The configFile is intentionally NOT passed as a patch to engine.Render. This matches templateWithFiles behavior in template.go and is equivalent to the two-step `talm template -f node.yaml | talm apply -f -` pattern.
In the VLAN branch of talos.config.network.multidoc, $interfaceName is rewritten to the parent link, while addresses/routes are emitted on the VLAN link ($defaultLinkName). Using $interfaceName for Layer2VIPConfig.link moved the VIP off the configured VLAN. Select $defaultLinkName when the default link is a VLAN so the VIP stays attached to the VLAN interface, matching the legacy path behavior. Address review feedback from coderabbitai on charts/cozystack/templates/_helpers.tpl:229. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
… path Mirror of the cozystack fix: when the default link is a VLAN, $interfaceName is rewritten to the parent link while the VLAN addresses/routes are emitted on $defaultLinkName. Use $defaultLinkName for Layer2VIPConfig.link in that branch so the VIP stays on the VLAN. Address review feedback from coderabbitai on charts/generic/templates/_helpers.tpl:152. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@charts/cozystack/templates/_helpers.tpl`:
- Around line 140-148: The template currently treats a non-empty JSON string
"[]" as truthy and ranges over fromJsonArray $resolvers which yields nothing,
leaving an empty "nameservers:" key; change the logic to parse the JSON into an
array first and check its length before rendering: call fromJsonArray once into
a variable (e.g. $arr := fromJsonArray $resolvers), then if gt (len $arr) 0
range $arr to render each "- address: {{ . | quote }}", else render an explicit
empty YAML list " []". Ensure you reference the include
"talm.discovered.default_resolvers", the $resolvers variable, and the
fromJsonArray result variable (e.g. $arr) in the updated condition.
🪄 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
Run ID: 40f39f20-84f4-4bcb-b68f-aca27e849ffb
📒 Files selected for processing (2)
charts/cozystack/templates/_helpers.tplcharts/generic/templates/_helpers.tpl
🚧 Files skipped from review as they are similar to previous changes (1)
- charts/generic/templates/_helpers.tpl
| nameservers: | ||
| {{- $resolvers := include "talm.discovered.default_resolvers" . }} | ||
| {{- if $resolvers }} | ||
| {{- range fromJsonArray $resolvers }} | ||
| - address: {{ . | quote }} | ||
| {{- end }} | ||
| {{- else }} | ||
| [] | ||
| {{- end }} |
There was a problem hiding this comment.
Minor edge case: empty nameservers output.
If talm.discovered.default_resolvers returns an empty JSON array [], the condition if $resolvers is true (non-empty string), but range fromJsonArray $resolvers produces nothing, resulting in:
nameservers:This is consistent with the generic template and likely acceptable, but verify Talos tolerates an empty nameservers: key without a value.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@charts/cozystack/templates/_helpers.tpl` around lines 140 - 148, The template
currently treats a non-empty JSON string "[]" as truthy and ranges over
fromJsonArray $resolvers which yields nothing, leaving an empty "nameservers:"
key; change the logic to parse the JSON into an array first and check its length
before rendering: call fromJsonArray once into a variable (e.g. $arr :=
fromJsonArray $resolvers), then if gt (len $arr) 0 range $arr to render each "-
address: {{ . | quote }}", else render an explicit empty YAML list " []".
Ensure you reference the include "talm.discovered.default_resolvers", the
$resolvers variable, and the fromJsonArray result variable (e.g. $arr) in the
updated condition.
myasnikovdaniil
left a comment
myasnikovdaniil
left a comment
There was a problem hiding this comment.
Re-reviewed. All items from the first review are addressed:
- Malformed TalosVersion crash → fixed (early validation in
Render()) - BondConfig/VLANConfig test coverage → fixed (
bondTopologyLookup+vlanOnBondTopologyLookup) - Layer2VIPConfig positive assertion → fixed
- README note precision → fixed
- machine.common comment → fixed
- Layer2VIP attached to wrong link in VLAN path → fixed (
$vipLinkName)
Tests pass clean, including with -race. Schema matches actual Talos Go types. LGTM.
3 participants
Summary
Add support for the Talos v1.12 multi-document configuration format. When
TalosVersionis set to v1.12 or later (viaChart.yamlor--talos-versionCLI flag), templates generate separate YAML documents instead of the deprecated monolithicmachine.networkandmachine.registriesfields.Closes #100
Changes
Engine
TalosVersionthrough the Helm values map into the template rendering context (available as.TalosVersion), avoiding global mutable stateChart templates (cozystack + generic)
talos.configinto shared sub-templates to eliminate duplication:talos.config.machine.common,talos.config.cluster,talos.config.network.legacy,talos.config.network.multidocsemverCompareHostnameConfig,ResolverConfig,LinkConfig,BondConfig,VLANConfig,RegistryMirrorConfig,Layer2VIPConfigBug fix
nr_hugepagesrendering insidewithblock (was producing empty string for non-zero values due to.Valuesbeing inaccessible when dot is rebound)Backward compatibility
TalosVersionis empty or < v1.12Tests
isTalosConfigPatchandextractExtraDocumentstests for all 7 new document typesnr_hugepagesrendering test for both legacy and multi-doc pathsSummary by CodeRabbit
Documentation
Refactor
Tests