Repository navigation
feat(kubernetes-nodes): let CDI trust a self-hosted Talos image factory - #3797
Conversation
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk | ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title clearly summarizes the main change: allowing CDI to trust a self-hosted Talos image factory. |
| Linked Issues check | ✅ Passed | Issue #3796 requests optional PEM CA support for worker-image imports from a self-hosted Talos Image Factory. The chart adds talos.imageFactoryCA, validates certificate-only PEM input, renders a rel… |
| Out of Scope Changes check | ✅ Passed | The shared cozy-lib.tls.validateCACert helper and caCertSecret delegation support the PEM validation required by #3796. The schema, API type, README, and kubernetes-nodes-rd updates expose and d… |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1… |
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
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 help to get the list of available commands.
github-actions
Bot
added
size/L
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
Fedor Batonogov (@batonogov) NOT LGTM, but only because main moved under this PR. The need is real: certConfigMap is still not rendered by any app on main, and #3796 is accepted.
The worker DataVolume is no longer in the kubernetes app. On current main packages/apps/kubernetes/templates/cluster.yaml has no source.http at all, and the import now lives in packages/apps/kubernetes-nodes/templates/nodegroup.yaml, which has its own talos.imageFactoryURL. That is also why the branch conflicts in _helpers.tpl, cluster.yaml and the RD. The field, the ConfigMap and the tests need to move to kubernetes-nodes, otherwise the CA would sit in values that no longer reach the import.
While moving it, please don't copy the PEM guards a second time. The two regexes are the same as in cozy-lib.tls.caCertSecret, and a security check kept in two copies will drift. A small validate-only helper in cozy-lib that takes the field name for the error message, used by both, is the shape I'd take. The doc comment above the helper can then lose the part addressed to maintainers.
The commit has Assisted-By: Claude <[email protected]>. The project takes exactly Assisted-by: LLM, no model or vendor name.
CI on this head is from 2026-08-13 and 2026-09-12, before the fork CI lane was fixed on 2026-09-27, so the red there is not caused by the diff. The push after the rebase will get a fresh run.
5bb2dd5 to
9bb77e2
Compare
github-actions
Bot
added
size/XL
|
Reworked on current main per the review: the field, the ConfigMap and the tests moved to The five workflow runs on this head sit in |
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
Fedor Batonogov (@batonogov) NOT LGTM, but only because the PR description and title still describe the old version. The code itself is ready.
The three points from my previous review are closed on 9bb77e2. The field and the certConfigMap reference moved to kubernetes-nodes, the PEM checks live once in cozy-lib.tls.validateCACert, and the trailer is Assisted-by: LLM with a matching sign-off.
I checked it on a trial merge with current main. Helm unit tests for kubernetes-nodes and cozy-lib-tests pass, and make generate leaves no diff. With the field unset, helm template output is byte-identical to main, so existing pools keep their KubevirtMachineTemplate name. I also tried three mutations: dropping the certConfigMap line, narrowing the private-key regex to BEGIN PRIVATE KEY, and removing the \A/\z anchors. Each one turns at least one test red. The Kubernetes app no longer renders worker pools, so setting the field on a KubernetesNodes app is the normal path and nothing is missing on the parent side.
The PR body becomes the merge commit message, so it has to match the code that merges. Right now it says:
- the value goes on the
kuberneteschart andtemplates/cluster.yamlrenderedurlalone. The change is inkubernetes-nodes,templates/nodegroup.yaml. - the guards are kept local and restate
caCertSecret, with an offer to extract them. They are extracted now. - "11 cases" and "216 passed". The test file has 13 cases now, and the package has 198 tests after the rebase.
- the regenerated files are
api/apps/v1alpha1/kubernetes/types.goand thekubernetes-rdCRD. They arekubernetesnodes/types.goandkubernetes-nodes-rd. - the downstream note points at
packages/apps/kubernetes/values.schema.json, and the release note starts withfeat(kubernetes).
The title has the same problem. The commit says feat(kubernetes-nodes), the title says feat(kubernetes), and the labeler takes the area/* label from the title scope. The terraform issue cozystack/terraform-provider-cozystack#31 also asks for the field on the Kubernetes resource, while the provider already has a kubernetes_nodes model where it belongs.
Not blocking: the field description could say that setting or clearing it rolls the pool once. certConfigMap is part of the DataVolume that is hashed into the KubevirtMachineTemplate name, and in my render the name changed from -c59638 to -59d103 when I set a CA. Rotating the PEM later rolls nothing, because the ConfigMap name stays the same. The neighbouring fields already carry a "Changing it rolls the pool" sentence.
Aleksei Sviridkin (lexfrei)
changed the title
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
Fedor Batonogov (@batonogov) I updated the title and body to match the rework. The code was ready already, so LGTM.
Head branch was pushed to by a user without write access
9bb77e2 to
5e88212
Compare
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
Fedor Batonogov (@batonogov) LGTM again. The last push is only a rebase onto current main. The diff is the same as the one I approved, except the regenerated schema line in kubernetes-nodes-rd. That line now also has what main added to the chart in the meantime, and it still matches values.schema.json. I ran helm unittest on a trial merge with main for kubernetes-nodes, cozy-lib-tests and redis (the other caCertSecret user), all green.
talos.imageFactoryURL is documented for self-hosted factories and mirrors in air-gapped, rate-limited or flaky-egress environments. Such an endpoint is normally served with a certificate from an internal CA, and CDI — which trusts only its system store — refuses the import with "x509: certificate signed by unknown authority". The DataVolume never completes, no worker joins, and the release sits in install until it times out. DataVolume.spec.source.http.certConfigMap exists for exactly this, but the chart rendered source.http.url alone and no value reached the field. Add an optional talos.imageFactoryCA taking the CA in PEM form. When set, the chart renders it into a ConfigMap in the release namespace — CDI requires certConfigMap to sit alongside the DataVolume — and references it from the worker DataVolume on the HTTP import path; a pool cloning a golden image via osImage.builtin builds no source.http and renders neither. When empty, nothing is rendered and the output is byte-for-byte what it was. CDI appends the bundle to the system pool rather than replacing it, so a release pulling from a publicly-trusted endpoint keeps verifying either way. The value is emitted verbatim into a ConfigMap readable by anything that can read the namespace, so the render fails closed on private key material and on anything that is not complete PEM certificate blocks. Those guards live in a new validate-only cozy-lib.tls.validateCACert helper that takes the caller's field name for its messages, and cozy-lib.tls.caCertSecret is refactored onto it, so the check exists in exactly one place. Assisted-by: LLM Signed-off-by: Fedor Batonogov <[email protected]>
5e88212 to
68cd102
Compare
2 participants
What this PR does
Adds
talos.imageFactoryCAto thekubernetes-nodeschart, so CDI can verify a Talos Image Factory served with a certificate from an internal CA. Fixes #3796.The gap
talos.imageFactoryURLis documented for self-hosted factories and mirrors in air-gapped, rate-limited or flaky-egress environments. Such an endpoint is normally served by the organisation's own CA, and CDI, which trusts only its system store, refuses the import withx509: certificate signed by unknown authority. The DataVolume never completes and no worker joins.DataVolume.spec.source.http.certConfigMapexists for exactly this, but the worker DataVolume intemplates/nodegroup.yamlrendersurlalone and no value reaches that field.The change
One optional value on the existing
talosgroup of aKubernetesNodespool:When set on an HTTP import path (the default and
osImage.factory), the chart renders a ConfigMap<release>-talos-factory-cain the release namespace and points the DataVolume'ssource.http.certConfigMapat it. A pool onosImage.builtinclones a golden image, builds no HTTP source and gets no ConfigMap. Empty, the default, renders nothing:helm templatewith the field unset is byte-identical tomain, so existing pools keep their KubevirtMachineTemplate name. Setting the CA changes that name, so the pool rolls once.CDI appends the bundle to the system pool rather than replacing it (
x509.SystemCertPool()followed byAppendCertsFromPEMinpkg/importer/http-datasource.go), so a publicly trusted endpoint keeps verifying.The PEM is taken inline rather than as a ConfigMap name because
certConfigMapmust live in the DataVolume's namespace, and the dashboard offers no way to create that object by hand.Fail-closed guards
The value lands verbatim in a ConfigMap that anything able to read the namespace can read, so it must be complete PEM certificate blocks and nothing else, and must never carry private key material. That includes the realistic accident of
cat ca.pem key.pempasted into the field. The rules now live in one shared helper,cozy-lib.tls.validateCACert, whichcozy-lib.tls.caCertSecretalso calls instead of keeping its own copy. The regexes are unchanged; the failure message names the caller's field.Testing
packages/apps/kubernetes-nodes/tests/talos_image_factory_ca_test.yaml, 13 cases: unset renders nothing, set renders the ConfigMap and a matchingcertConfigMap, whitespace-only counts as unset, multi-certificate chains, the builtin path, and each guard on the input that trips it.packages/tests/cozy-lib-tests/tests/tls_validatecacert_test.yaml, 6 cases for the shared helper, plus the existingcaCertSecrettests updated for the new message.make generatein the package: regeneratedvalues.schema.json,README.md,api/apps/v1alpha1/kubernetesnodes/types.goand thekubernetes-nodes-rddefinition are committed.Not yet exercised end to end against a live internal factory.
Not in this PR
packages/apps/vm-disk/templates/dv.yamlrenderssource.http.urlwith the same gap. Left for a follow-up if this shape is accepted.Screenshots
Not a UI change.
Downstream repositories
kubernetes_nodesmodel)Walked the trigger map against the diff:
packages/apps/kubernetes-nodes/values.schema.json, which the trigger map names directly. Filed as an issue there.README.md, which is regenerated and committed here.Release note
Summary by CodeRabbit