chore(deps): migrate to Helm 4 and drop the cozystack/talos fork - #231
Conversation
The engine's contract tests are substring-based — they pin only the specific fields each one names, so a change that alters whitespace, key ordering, number formatting, document count, or any unasserted field renders green. Add byte-for-byte golden snapshots of the full rendered output across the schema matrix: cozystack and generic charts, controlplane and worker, legacy single-doc and multi-doc schemas, driven by a deterministic discovery fixture. Regenerate with TALM_UPDATE_GOLDEN=1; without it the test compares and fails on any drift, catching behavioral regressions the targeted assertions miss. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
talm consumes the Helm library only as a template engine — a vendored fork of pkg/engine plus the chart types, loader, and strvals — never its release or install machinery. The v4 changes around server-side apply, kstatus, and release storage therefore do not apply here. The swap is not purely mechanical. The Helm v4 module requires k8s.io/api v0.36, which drops autoscaling/v2beta2 that fluxcd/pkg/ssa v0.70 still imports, so the whole k8s.io stack moves to v0.36.2 and fluxcd/pkg/ssa to v0.77 (which is already on v0.36 and no longer needs the removed package). Chart value and file types moved to pkg/chart/common in v4; the engine fork and its tests reference them there, with CoalesceValues now under pkg/chart/common/util. The golden snapshots render byte-for-byte identical on v4, confirming the generated Talos machine-config is unchanged for users. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
📝 WalkthroughWalkthroughThe change reimplements native ChangesTalm-native TLS verification
Helm v4 rendering migration
Golden render coverage
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant TalmCommand
participant ClientBuilder
participant TalosConfig
participant TalosAPI
CLI->>TalmCommand: invoke native command with --skip-verify
TalmCommand->>ClientBuilder: request client and dial options
ClientBuilder->>TalosConfig: resolve context, credentials, and endpoints
ClientBuilder->>TalosAPI: connect with server verification disabled
TalosAPI-->>TalmCommand: execute action with resolved context metadata
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@docs/manual-test-plan.md`:
- Around line 1001-1010: Update the expected passthrough warning in the manual
test plan to include rotate-ca in the talm-native command list, matching
warnSkipVerifyUnsupported in talosctl_wrapper.go; leave the remaining expected
stderr text unchanged.
In `@pkg/commands/root.go`:
- Around line 71-73: Update the TLS configuration logic around configContext.Crt
and configContext.Key to distinguish an entirely absent certificate/key pair
from a partial pair. Return the existing empty TLS configuration only when both
are unset; when exactly one is set, return a clear configuration error before
the invalid-base64 and key-pair validation paths.
🪄 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: a4bc126f-e290-4c80-9afb-7fb5879fc17d
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (26)
.gitattributesdocs/manual-test-plan.mdgo.modmain.gopkg/commands/apply.gopkg/commands/root.gopkg/commands/rotate_ca_handler.gopkg/commands/skip_verify_test.gopkg/commands/talosctl_wrapper.gopkg/commands/template.gopkg/engine/contract_errors_test.gopkg/engine/engine.gopkg/engine/golden_test.gopkg/engine/helm/doc.gopkg/engine/helm/engine.gopkg/engine/helm/engine_test.gopkg/engine/helm/files.gopkg/engine/render_test.gopkg/engine/testdata/golden/cozystack-controlplane-legacy.golden.yamlpkg/engine/testdata/golden/cozystack-controlplane-multidoc.golden.yamlpkg/engine/testdata/golden/cozystack-worker-legacy.golden.yamlpkg/engine/testdata/golden/cozystack-worker-multidoc.golden.yamlpkg/engine/testdata/golden/generic-controlplane-legacy.golden.yamlpkg/engine/testdata/golden/generic-controlplane-multidoc.golden.yamlpkg/engine/testdata/golden/generic-worker-legacy.golden.yamlpkg/engine/testdata/golden/generic-worker-multidoc.golden.yaml
| if configContext.Crt == "" || configContext.Key == "" { | ||
| return tlsConfig, nil | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Inconsistent handling of a partial cert/key pair.
If only one of Crt/Key is set, this silently returns a TLS config with no client certificate instead of erroring — unlike the invalid-base64 and mismatched-keypair cases just below, which fail loudly. A talosconfig context with a typo'd/half-populated cert pair would silently lose client-cert auth and surface as an opaque downstream authorization failure instead of a clear config error.
🛡️ Proposed fix: treat "exactly one present" as an error too
- if configContext.Crt == "" || configContext.Key == "" {
+ if configContext.Crt == "" && configContext.Key == "" {
return tlsConfig, nil
}
+
+ if configContext.Crt == "" || configContext.Key == "" {
+ return nil, errors.New("talosconfig context has only one of client certificate/key set")
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if configContext.Crt == "" || configContext.Key == "" { | |
| return tlsConfig, nil | |
| } | |
| if configContext.Crt == "" && configContext.Key == "" { | |
| return tlsConfig, nil | |
| } | |
| if configContext.Crt == "" || configContext.Key == "" { | |
| return nil, errors.New("talosconfig context has only one of client certificate/key set") | |
| } |
🤖 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/commands/root.go` around lines 71 - 73, Update the TLS configuration
logic around configContext.Crt and configContext.Key to distinguish an entirely
absent certificate/key pair from a partial pair. Return the existing empty TLS
configuration only when both are unset; when exactly one is set, return a clear
configuration error before the invalid-base64 and key-pair validation paths.
The fork existed solely to carry the --skip-verify flag, which upstream declined (siderolabs/talos#12652) over the risk of leaking data to an actor impersonating the Talos API. Reimplement the flag locally in pkg/commands: a package-level SkipVerify bound to --skip-verify, and a WithClientSkipVerify that opens the talosconfig, builds a TLS config skipping server-certificate verification while preserving client-cert authentication, and dials the node directly. With the flag no longer depending on fork-only API, the replace directive is dropped and talm tracks stock upstream Talos v1.13.7. The rendered machine-config is unchanged — golden snapshots stay byte-identical; only the client-connection path is affected. Co-authored-by: Kirill Ilin <stitch14@yandex.ru> Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
Refresh the dependency tree with the latest compatible minor/patch versions across direct and indirect modules. siderolabs/go-talos-support is held at v0.2.1 (the version Talos v1.13.7 expects) because v0.3.0 removes bundle.WithTalosClient, which the Talos cluster package still calls — bumping it breaks the build. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
a8d04bc to
3612169
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/commands/root.go (1)
61-93: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPartial cert/key pair still silently ignored (unresolved from prior review).
If only one of
Crt/Keyis set, this returns an insecure-only TLS config with no client cert instead of erroring — inconsistent with the invalid-base64/mismatched-keypair cases below, which fail loudly. A half-populated talosconfig context silently drops client-cert auth instead of surfacing a clear error. This is the same gap flagged on a prior commit of this PR and hasn't been addressed;skip_verify_test.goalso has no test covering the "exactly one set" case, so the gap is currently unverified.🛡️ Proposed fix: treat "exactly one present" as an error
- if configContext.Crt == "" || configContext.Key == "" { + if configContext.Crt == "" && configContext.Key == "" { return tlsConfig, nil } + + if configContext.Crt == "" || configContext.Key == "" { + return nil, errors.New("talosconfig context has only one of client certificate/key set") + }🤖 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/commands/root.go` around lines 61 - 93, Update skipVerifyTLSConfig to reject partially populated client credentials: if exactly one of configContext.Crt or configContext.Key is set, return a clear error instead of returning the insecure-only TLS config. Preserve the existing no-credentials path when both are empty and certificate decoding/key-pair loading when both are present, and add coverage in skip_verify_test.go for the one-sided cases.
🧹 Nitpick comments (1)
pkg/commands/talosctl_wrapper.go (1)
282-282: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse Cobra’s configured stderr for the warning.
Passing
os.Stderrbypassescmd.SetErr(...)/cmd.ErrOrStderr(), so embedded callers and command-level tests cannot capture or redirect this warning. Passcmd.ErrOrStderr()instead.🤖 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/commands/talosctl_wrapper.go` at line 282, Update the warning call in the command execution flow to pass cmd.ErrOrStderr() instead of os.Stderr, ensuring warnSkipVerifyUnsupported respects Cobra’s configured error output while preserving the existing command name argument.
🤖 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.
Duplicate comments:
In `@pkg/commands/root.go`:
- Around line 61-93: Update skipVerifyTLSConfig to reject partially populated
client credentials: if exactly one of configContext.Crt or configContext.Key is
set, return a clear error instead of returning the insecure-only TLS config.
Preserve the existing no-credentials path when both are empty and certificate
decoding/key-pair loading when both are present, and add coverage in
skip_verify_test.go for the one-sided cases.
---
Nitpick comments:
In `@pkg/commands/talosctl_wrapper.go`:
- Line 282: Update the warning call in the command execution flow to pass
cmd.ErrOrStderr() instead of os.Stderr, ensuring warnSkipVerifyUnsupported
respects Cobra’s configured error output while preserving the existing command
name argument.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bdba6a9f-a6e8-4e85-b0a6-aa89a7bc1419
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
docs/manual-test-plan.mdgo.modmain.gopkg/commands/apply.gopkg/commands/root.gopkg/commands/rotate_ca_handler.gopkg/commands/skip_verify_test.gopkg/commands/talosctl_wrapper.gopkg/commands/template.go
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/commands/apply.go
- main.go
- pkg/commands/rotate_ca_handler.go
- docs/manual-test-plan.md
- pkg/commands/template.go
VerdictLGTM with non-blocking notes. Helm v3→v4 migration and the cozystack/talos fork drop are mechanically sound and independently verified end-to-end (build, vet, full test suite, lint, Findings[MINOR] The custom manager's regex ( [MINOR] The block allow-lists [MINOR]
[MINOR] Commit messages — the disclosed The PR body has an explicit "Breaking change: Caveats
Recommended follow-ups
|
|
On the scope question: the reduced set (
The passthroughs that lose it (
The only way the scope would be insufficient is a supported workflow where you must introspect ( |
2 participants
What
Migrates talm off the end-of-life Helm 3 library to Helm 4. The v4 dependency graph forces the rest: it drops the
cozystack/talosfork for stock upstream Talos v1.13.7, then refreshes the dependency tree. Four self-contained commits: golden snapshots (on v3), the v3-to-v4 swap, fork drop plus local--skip-verify, remaining dependency bumps.Why the fork drop rides along
Helm v4 requires
k8s.io/apiv0.36, which cascades across the whole build graph. The pinnedcozystack/talosfork was a January-2026 pre-v1.13 snapshot that can't coexist with that bump. The fork only ever existed to carry--skip-verify(upstream declined it in siderolabs/talos#12652), so I reimplemented that flag in talm's own client wrappers and moved to upstream v1.13.7. You can't take Helm 4 without also unpinning talos.Transparency
The golden snapshots prove the migration doesn't change what talm generates. They were committed on v3, the v4 commit doesn't touch them, and
TestGoldenRenderpasses byte-for-byte across the full matrix (cozystack and generic, controlplane and worker, multidoc and legacy). Rendered machine-config is identical before and after.Breaking change:
--skip-verifyon wrapped talosctl commandsUnder the fork,
--skip-verifywas injected at the library level and worked for every command, including passthroughs (talm get,talm health,talm dashboard). Those run upstream code that never reaches talm's client wrappers, so without the fork there is no way to inject it.--skip-verifynow works only for talm-native commands (apply, template, upgrade, rotate-ca). Passthroughs print an explicit warning and connect with full verification, instead of failing with an opaque TLS SAN error.Testing
go build,go vet, the full suite, and golangci-lint are all clean. The--skip-verifyreimplementation covers the TLS config, native routing, the no-nodes contract, config-context and endpoint threading, and the passthrough warning. Manual steps are indocs/manual-test-plan.mdD4. Reviewed by Opus. Docs for the--skip-verifyscope reduction will follow at cozystack/website.Summary by CodeRabbit
--skip-verifyTLS behavior for talm-native commands (apply, template, upgrade, rotate-ca).--skip-verifywith wrapped talosctl passthrough commands now warns and falls back to full TLS verification.--skip-verifyscenarios.--skip-verifyrouting/TLS coverage and new golden render snapshot validation.