Repository navigation
feat(platform, kubernetes): platform-wide defaults for tenant Talos worker settings - #4897
myasnikovdaniil wants to merge 3 commits into
Conversation
… workers An operator in a closed or rate-limited environment had no platform-wide way to send tenant worker image pulls through a mirror. The platform registries value fed a Secret that nothing read, and since the Talos rollover every worker pool had to repeat talos.registryMirrors by hand. Add kubernetesRegistries to the platform values and carry it through _cluster, so a pool that sets no mirrors of its own gets the platform's. Mirrors and the TLS settings of their endpoints are rendered as the RegistryMirrorConfig and RegistryTLSConfig documents that Talos 1.12 introduced in place of the deprecated machine.registries section, which the chart no longer renders. The switches on them are on only for a true value, so a quoted "false" cannot turn on skipFallback or skip TLS verification. Drop the retired registries value and its patch-containerd Secret. The platform values are copied into every tenant namespace and read by every pool, so the platform chart checks them where the operator writes them. Credentials, client keys and keys the chart does not read are refused, because the machine config is stored in the tenant namespace and they would be readable there. So is a mirror the nodes chart could not render, which would otherwise fail every pool that has none of its own. A pool that already sets talos.registryMirrors rolls once when it moves to this release, because the machine config text, and with it the TalosConfigTemplate name, changes. Pools without mirrors render exactly as before. Setting the platform mirrors rolls the pools that have none of their own, and setting the platform tls settings rolls every pool. Assisted-by: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
…rker pools An operator in a closed environment had to set talos.imageFactoryURL and talos.installerRepository on every KubernetesNodes by hand, and a pool created without them went to the public factory, which is the egress such environments block. The worker image catalog already lets the platform name the factory the golden images come from, but nothing made a new pool use it. Let the kubernetesWorkerImage platform value state the image source once. Its imageFactoryURL, its installerRepository, and whether the catalog package is enabled travel to cozystack-api through _cluster. The installer prefix is never derived from the factory URL, which only says where the OS image is fetched from: a file server or a caching mirror serves that and no installer. Both values are copied into the spec of every new pool, so the platform refuses a factory URL that is not a plain http(s) URL or carries a user name and password, whether or not the catalog is enabled, and an installer prefix that is not a registry path. When a KubernetesNodes is created, the API fills in the fields its spec leaves out: osImage.builtin so the pool clones a golden, the factory URL, and the installer prefix. A proxmox pool takes only the installer, because its render refuses osImage. The values are written into the spec on create and read nowhere else. The image source and the installer are hashed into names the MachineDeployment points at, so a default applied at render time, or on read, would re-image every existing pool the first time an operator set it. A pool that exists is never touched, and a field the spec already carries, even an empty one, is kept, which is how a pool opts out. A cluster that already runs the catalog package gets this without setting a new value: its new pools start on a golden, and one whose Talos version, schematic or storage class the catalog does not cover fails to render until it opts out with osImage.factory or the catalog gains the entry. Assisted-by: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
…mages The platform now writes the image source and the installer prefix into the spec of a new worker pool, and does nothing for a pool that already exists. An operator who wants an existing pool on a golden image has no document that says how, and the obvious attempts go wrong in ways that are easy to miss. Defaults are applied when a pool is read and written back by its first edit, so the pool already carries the public installer prefix explicitly and keeps it unless the same patch replaces it. And a pool that opted out of the golden, or pinned a factory version, carries osImage.factory: a merge patch that only adds osImage.builtin leaves both set, and the pool stops rendering with "set only one of". The same patch has to null the factory, and so does the way back. Put the procedure in the header of the worker image catalog, where an operator looks for everything about goldens, with the checks that make the pool render and the cost of the roll. Say in the pool's field docs that these values are filled in at creation and that an older pool keeps what it has. Assisted-by: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
github-actions
Bot
added
area/kubernetes
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 57.14% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 14 functions across 4 files. (14 skipped:… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: platform-wide defaults for tenant Talos worker settings. Linked Issues check The PR meets the coding requirements for [ #3950] and [#4006]. For [#3950], platform worker-image values are copied to the platform Secret, create-time API logic fills omittedKubernetesNodesfields,…Out of Scope Changes check The changes stay within the linked issue scope. Installer repository and catalog-based worker defaults support the platform-wide worker defaults in [ #3950]. Registry TLS handling, validation, document…
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 57.14% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 14 functions across 4 files. (14 skipped: 14 unsupported.)
- Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
-
📝 Generate docstrings 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
NOT LGTM
A sub-1.12 worker pool that already sets talos.registryMirrors renders today but fails outright after this change, which blocks the pool's whole HelmRelease on a default upgrade; a few smaller issues ride along.
Findings
- [MAJOR]
packages/apps/kubernetes-nodes/templates/_talosconfigtemplate.tpl:141, Talos-1.12 render gate breaks existing sub-1.12 pools and over-fires on empty platform config
- [MINOR]
packages/core/platform/templates/apps.yaml:55, mirror-endpoint guard lets a query-string credential into every tenant's Secret
- [MINOR]
pkg/registry/apps/application/rest_create_defaults.go:105, transient read of cozystack-values silently stores a pool on a different image source
- [NIT]
packages/core/platform/templates/apps.yaml:57, dead urlParse result; opaque panic on a URL that passes the regex but not net/url
Caveats
- Catalog stamping writes
osImage.builtin: {}into any create that omitsosImage, including pools whosetalos.version/schematicID/storageClassthe catalog does not carry: the create succeeds and the HelmRelease then fails to render. Documented in the PR body, so noted not blocked; consider skipping the stamp when the spec overrides those fields. - Pools built outside cozystack-api (
packages/extra/computeplane, migration 56) never receive the stamped defaults yet still pick up platform mirrors via_cluster; undocumented mismatch. - No e2e names
installerRepository/osImage/kubernetesRegistries(new-values-field-no-e2e); the RegistryMirrorConfig / RegistryTLSConfig documents are only checked offline, never on a booted node.
Recommended follow-ups
cozystack-pr-teston a dev stand: replay the opt-in roll paths and a sub-1.12 proxmox pool to confirm a worker boots pulling through a mirror and that the version gate behaves on the proxmox substrate.
| {{- $registryConfig := $clusterRegistries.config | default dict }} | ||
| {{- $registryDocuments := "" }} | ||
| {{- if or $registryMirrors $registryConfig }} | ||
| {{- if not (semverCompare ">=1.12.0-0" $talosVersion) }} |
There was a problem hiding this comment.
[MAJOR] Talos-1.12 render gate breaks existing sub-1.12 pools and over-fires on empty platform config
The gate {{- if or $registryMirrors $registryConfig }} then {{- if not (semverCompare ">=1.12.0-0" $talosVersion) }} fail turns a working upgrade into a failed render.
Proven with helm unittest on this chart (talos.version v1.11.5):
# merge-base 3a46360: pool with its own talos.registryMirrors RENDERS (emits machine.registries.mirrors)
helm unittest -f tests/<probe>.yaml packages/apps/kubernetes-nodes # PASS, matchRegex 'mirrors:'
# head 154c4ff: the same pool FAILS
# nodeGroup ... registry mirrors ... Talos has only since 1.12 ... resolves to Talos v1.11.5
- Upgrade regression. A pool pinned below Talos 1.12 that already set
talos.registryMirrorsworked (it renderedmachine.registries.mirrors, valid on 1.11). After this change its whole HelmRelease fails to render, so the pool stops reconciling at all (no scale, no version bump). The PR body's "a pool with its own talos.registryMirrors rolls once" is false for such a pool: it fails rather than rolls. Talos pins below 1.12 are the operator's call and pass unchecked (README), and air-gapped setups that use mirrors are exactly where an older pinned Talos is plausible. - Over-broad gate. The
failfires on the mere presence of$registryMirrors/$registryConfig, before any document is built. A platformkubernetesRegistries.configentry with an emptytlsproduces zero RegistryTLSConfig documents (theif gt (len $doc) 3guard drops it) yet still fails every sub-1.12 pool in every tenant, including pools that use no mirror. So enabling platform registry TLS silently breaks unrelated sub-1.12 pools cluster-wide, contradicting the values.yaml note that "nothing is rendered while both are empty, so a pool without mirrors keeps the machine config". - On substrate proxmox
$talosVersionistalos.version, the in-guest installer target only; the booted OS is the VM template's Talos. A sub-1.12 template with talos.version at the 1.13.6 default passes the gate, and the documents then reach a node that rejects them (needs a cluster to confirm the boot failure).
Fix direction: fail only when a document is actually produced for that pool, or keep emitting machine.registries for Talos below 1.12.
| {{- fail (printf "kubernetesRegistries.mirrors[%q].endpoints must hold URL strings, got %v" $host .) -}} | ||
| {{- end -}} | ||
| {{- if not (regexMatch "(?i)^https?://[^/@[:space:]]+(/[^[:space:]]*)?$" .) -}} | ||
| {{- fail (printf "kubernetesRegistries.mirrors[%q].endpoints entry %q must be an http(s) URL with no user name or password in it: the mirrors are copied into the cozystack-values Secret of every tenant." $host .) -}} |
There was a problem hiding this comment.
[MINOR] mirror-endpoint guard lets a query-string credential into every tenant's Secret
The guard regexMatch "(?i)^https?://[^/@[:space:]]+(/[^[:space:]]*)?$" exists to keep credentials out of the cozystack-values Secret copied into every tenant namespace, but it only blocks userinfo (user:pass@host). A credential in the query string passes: https://mirror.example.org?token=SECRET matches, because the host class [^/@[:space:]]+ accepts ?, = and the token, and so does an @ placed in the path. The sibling kubernetesWorkerImage.imageFactoryURL check (^https?://[^/]*@) has the same gap. A platform operator pointing a mirror at an authenticated pull-through endpoint with a query token leaks it to every tenant. Parse the URL and reject a non-empty query / userinfo (urlParse is already called one line below), or state in the message that endpoints must carry no secrets at all.
| secret := &corev1.Secret{} | ||
| key := client.ObjectKey{Namespace: platformValuesNamespace, Name: platformValuesSecret} | ||
| if err := r.c.Get(ctx, key, secret); err != nil { | ||
| klog.Warningf("Not applying worker pool defaults: cannot read Secret %s: %v", key, err) |
There was a problem hiding this comment.
[MINOR] transient read of cozystack-values silently stores a pool on a different image source
loadWorkerPoolDefaults returns (_, false) on ANY error reading or unmarshalling the cozystack-values Secret, and applyCreateDefaults then stores the KubernetesNodes with no defaults, logging only a klog warning the tenant never sees. A transient GET (apiserver blip, RBAC hiccup) or a momentarily malformed Secret therefore changes what is persisted: the pool is created on the HTTP image-factory path instead of osImage.builtin, and without the platform installer prefix. Because the image source is hashed into the MachineDeployment name, correcting it later re-images the pool. Distinguish a real NotFound (skip quietly, legitimate) from any other error (fail the create, or surface a warning to the caller) so a silently diverging default does not reach storage unnoticed.
| {{- if not (regexMatch "(?i)^https?://[^/@[:space:]]+(/[^[:space:]]*)?$" .) -}} | ||
| {{- fail (printf "kubernetesRegistries.mirrors[%q].endpoints entry %q must be an http(s) URL with no user name or password in it: the mirrors are copied into the cozystack-values Secret of every tenant." $host .) -}} | ||
| {{- end -}} | ||
| {{- $parsed := urlParse . -}} |
There was a problem hiding this comment.
[NIT] dead urlParse result; opaque panic on a URL that passes the regex but not net/url
{{- $parsed := urlParse . -}} assigns a variable that is never read. Its only effect is a side channel: urlParse panics on a URL that passes the regex above but is not parseable by net/url (for example http://[bad), surfacing as an opaque Helm panic rather than the legible fail the surrounding guards give. Either drop the dead line, or actually use it to validate and emit a readable fail on a parse error.
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
myasnikovdaniil NOT LGTM. The Talos 1.12 gate in the nodes chart is the one blocker, and I agree with Ivan on it.
The gate fails a pool when mirrors or TLS config are merely present, before any document is built. kubernetes.talosRegistryDocuments drops a TLS entry with neither ca nor insecureSkipVerify: true, and your test "a tls entry that sets nothing renders no document" pins that. A pool below 1.12 still fails on that entry. So a platform entry that renders nothing for anyone stops every sub-1.12 pool in every tenant, including pools that use no mirror. The fix that keeps your design is to build the documents first and fail only when $registryDocuments is non-empty. For a pool with its own talos.registryMirrors, the cleaner option is to keep rendering machine.registries below 1.12, since that is what main does today and it works there.
If you keep the refusal, the upgrade notes need to say so. The "What changes on upgrade" list in the PR body and the last paragraph of 4232739 both say a pool with its own talos.registryMirrors "rolls once". Below 1.12 that pool stops rendering instead, and the PR body becomes the merge commit message.
On Ivan's other points. The query-string credential and the dead urlParse are real. I don't think the transient-read case can happen: cozystack-api starts a cluster-wide Secret informer and waits for its sync before serving (pkg/apiserver/apiserver.go, mustGetInformers). r.c.Get is answered from that cache, so there is no network read to fail. What is left is NotFound, which is the legitimate skip, and a Secret the platform chart itself rendered wrong. His computeplane caveat holds: packages/extra/computeplane/templates/cluster.yaml renders the HelmRelease directly and never goes through Create.
Non-blocking:
- A pool can't opt out of the platform mirrors.
registryMirrorsdefaults to{}in the CRD, and{}falls through to the platform map, so "unset" and "empty" look the same. A pool that has to pull directly while the platform setsskipFallbackhas no way to do it. Worth one sentence in the field doc. packages/apps/kubernetes/values.yaml:365(and the schema inkubernetes-rd) still calls the parent cluster'stalos.registryMirrorsa "machine.registries.mirrorspassthrough". Nothing in the kubernetes templates reads it, and after this PR the description is wrong about the format too.
Checked on 154c4ff with a trial merge onto current main: the Go tests and both helm suites pass (245 and 348). Removing the absent checks, the proxmox exclusion, the applyCreateDefaults call in Create, the disabledPackages check, or swapping the pool/platform mirror precedence turns them red.
3 participants
What this PR does
Adds platform-wide defaults for tenant talos worker pools, closes #3950. It also removes the dead
registriesplatform value and thepatch-containerdSecret rendered from it, closes #4006.Worker image defaults
kubernetesWorkerImagenow sets what a newKubernetesNodesstarts with, not only the golden image catalog. On create cozystack-api writes into the spec every field it leaves out:osImage.builtin: {}(only when the catalog package is enabled, not for proxmox),talos.imageFactoryURLfromimageFactoryURL(not for proxmox) andtalos.installerRepositoryfrom the newinstallerRepository. A field the spec already carries is kept, even an empty one, soosImage.factory: {}opts a pool out.Installer prefix is not derived from
imageFactoryURL, because the factory url only says where the OS image is fetched from, and a file server or a caching mirror has no installer.Defaults go in at create only. Image source and installer are hashed into names the MachineDeployment points at, so a default applied at render or on read would roll every existing pool when an operator first sets it. Existing pools are not touched, moving one onto a golden image is a patch described in the header of the catalog values (it rolls that pool).
Registry mirrors
kubernetesRegistries.mirrorsandkubernetesRegistries.config(onlytls.caandtls.insecureSkipVerify) go to_cluster.kubernetes-registriesand into every worker pool asRegistryMirrorConfigandRegistryTLSConfigdocuments. The chart doesn't render deprecatedmachine.registriesanymore. Pool's owntalos.registryMirrorsreplaces the platformmirrorsas a whole,configalways applies. The documents need talos 1.12, render of an older pool fails with a message when mirrors or config are set.Credentials and client keys are refused at platform render because the machine config is readable in the tenant namespace. Mirrors with a wrong shape are refused there too, otherwise they would fail every pool that has no mirrors of its own.
What changes on upgrade
talos.registryMirrorsrolls once, machine config text and TalosConfigTemplate name changemirrorsrolls pools without own mirrors,configrolls every poolosImage.factory: {}or the catalog gets the entrykubernetesWorkerImage.imageFactoryURLhas to be a plain http(s) URL with no user name or password, platform render refuses anything else, also when the catalog package is off (before the key was dormant then). It is copied to every tenant namespace and into the spec of every new poolcozystack-valuesSecret make every release reconcile once (about 10 minutes on a small test stand, no worker rolls from it)Dashboard form is not covered. It may submit the full
talosblock, then nothing is stamped, I didn't look into it here.How it was checked
Chart tests for platform and kubernetes-nodes, go tests for the stamping,
make unit-testspasses. On a test stand with an earlier revision of this branch a new pool withoutosImagewas stored withbuiltin: {}and the factory and installer values, cloned the golden and its worker joined, a pool withosImage.factory: {}kept it, a pod pulled an image of a nonexistent registry host through the mirror withskipFallback, and the TalosConfigTemplate had nomachine.registries.RegistryTLSConfigis checked only with the talos 1.13 config loader, not on a real node.Screenshots
Downstream repositories
Release note
Summary by CodeRabbit