mirror of
https://github.com/cozystack/cozystack.git
synced 2026-11-06 03:57:52 +09:00
Aleksei Sviridkin
339cffe70f
## What this PR does BREAKING CHANGE: a Kubernetes application whose `controlPlane.apiServer.extraArgs` names a flag the tenant control plane now owns fails to render until that entry is removed, except that an `--enable-admission-plugins=` entry is moved into the new `controlPlane.apiServer.admissionControllers` and a `--kubelet-preferred-address-types=` entry into the new `controlPlane.kubelet.preferredAddressTypes`. Closes #3541. Updates the Kamaji build this repo ships and rewrites the part of the tenant Kubernetes chart whose correctness rested on the old one. The new pin is `26.9.4-edge`, which is what `make update` picks. Two upstream changes reach us. The tenant control plane no longer folds the kube-apiserver argument list into a map keyed by flag name, so entries supplied through the control plane spec arrive at the apiserver verbatim, duplicates included. And the reset that rewrites those arguments from scratch keys on a hash of the list rather than on its length, so swapping one flag for another no longer strands the old one on the running Deployment. The OIDC block in the tenant chart read only the last entry of each flag. That was right while earlier entries were dropped before the apiserver saw them, and it is wrong now. An operator whose `extraArgs` carried `--feature-gates=RemoteRequestHeaderUID=false` followed by an unrelated `--feature-gates` entry got `--requestheader-uid-headers=X-Remote-Uid` rendered next to a gate the apiserver has switched off, and the control plane would not start. A blanket `AllBeta=false` has the same shape, and so does an `--emulated-version` entry naming kube followed by one naming another component. The chart now reads every entry with the accumulation each flag performs. Occurrences of `--requestheader-uid-headers` append into one list, so where a list of the operator's own omits `X-Remote-Uid` the chart adds its own element beside it instead of refusing to render: the union is what the apiserver validates, and their header keeps working. Feature-gate terms collect per component and are applied in one pass, so the chart carries only its own term, placed last where it wins. Naming the kube component both bare and `kube:`-prefixed now fails the render, because the apiserver refuses that configuration and nothing the chart renders repairs it. `--emulated-version` accumulates too, and two entries naming kube stop the apiserver outright, so the chart renders nothing rather than pick a version that will not exist. The same upstream change narrows what `extraArgs` can do, and the chart now refuses the part that stopped working. Kamaji 26.9.4-edge sets a list of kube-apiserver flags itself and drops any `extraArgs` entry that names one before the apiserver sees the list, where `26.3.6-edge` merged `extraArgs` last and let the entry win. A tenant with `--enable-admission-plugins=NodeRestriction,AlwaysPullImages` loses both plugins on the first reconcile after the upgrade, with nothing to say so. The drop happens on the running control plane whatever the chart renders, so the chart cannot prevent it; what it can do is refuse to render such an entry, in every `oidc.mode`, and name it. The list mirrors `buildKubeAPIServerCommand` at the pinned tag for the control plane this chart renders (an etcd DataStore, konnectivity on, no `dataStoreOverrides`), and the `controlPlane.apiServer.extraArgs` description in the chart README carries it. A flag the control plane only defaults, such as `--authorization-mode` or `--requestheader-group-headers`, is still yours to set. `--enable-admission-plugins` gets a replacement: the new `controlPlane.apiServer.admissionControllers` passes a list through to the KamajiControlPlane `spec.admissionControllers`, which the control plane renders into that flag, and the application API checks each name against the enum the KamajiControlPlane CRD declares. The list is rendered only when non-empty, so a tenant that sets nothing renders exactly as before, and a non-empty list replaces the control plane's default set, as the `extraArgs` entry did under the previous build. `--kubelet-preferred-address-types` gets the same treatment: the list the chart used to hardcode on the KamajiControlPlane is now `controlPlane.kubelet.preferredAddressTypes`, which renders that list when left empty. Neither entry is refused where it can be moved: an `--enable-admission-plugins=` or `--kubelet-preferred-address-types=` entry is moved into its field while the field is empty, on every render, and left out of the rendered `extraArgs`, so a tenant carrying one keeps its setting across the Kamaji update with no edit and no ordering to get right. The entry is refused beside a non-empty field, when it names nothing, and in the two-entry spelling. A separate defect in the same package is fixed here. The TenantControlPlane schema reaches existing clusters through the `kamaji-crds` subchart's templated CRDs, which every release upgrade applies; `crds/` holds the copies a fresh install lays down first. Directory-based renders lost that schema entirely, `helm template`, `lint` and `package` alike, because the parent `.helmignore` matched the subchart's `hack/` directory at any depth and that is where the templates keep the schema they read with `.Files.Get`. Helm honours `.helmignore` for a directory and skips it for an archive, so the cluster never saw the difference and no local check could catch a schema defect. The patterns are anchored to the chart root and a unit test asserts the rendered CRDs carry their versions. The bump is not cleanly reversible, and that is the one thing to know before rolling it out. The args annotation holds a checksum once the new build has written it, and the old build reads that annotation with `strconv.Atoi`. The parse fails and `resetKubeAPIServerFlags` returns early, ahead of both the args reset and the annotation write that would put a number back. So after a rollback to `26.3.6-edge` the count-keyed reset is disabled on every TenantControlPlane: a later removal of an `extraArgs` entry no longer clears the flag from the running Deployment, and it stays that way until someone writes a number into `kube-apiserver.kamaji.clastix.io/args` by hand. Findings from a live upgrade on a dev cluster, none of them caused by the diff. The DNS-service-IP check moved out of a CEL rule on the CRD into the admission webhook (`internal/webhook/handlers/tcp_dns.go`), so it now depends on that webhook being reachable where the apiserver used to enforce it by itself. The manager's readiness probe targets port 8081 while the webhook serves on 9443, so a Ready pod does not prove 9443 accepts connections; with `failurePolicy: Fail` on the TenantControlPlane webhook, that window rejects writes rather than letting them through unvalidated. The readiness probe this build adds to kube-scheduler and kube-controller-manager survives a rollback, since the old build neither sets nor clears those two; that one is benign. Seven limits worth naming rather than leaving to be found. The mixed-spelling refusal quotes the entry carrying each spelling, so one entry holding both, `--feature-gates=Foo=true,kube:Bar=true`, is quoted twice in its own message; the refusal is right and the message reads oddly. The unit test that pins the rendered CRD schemas asserts on a count, so a CRD missing from the render altogether fails on an empty comparison rather than naming the CRD that went missing. There is one window during the upgrade itself: the chart used to fold an operator's own `--feature-gates` terms into the entry it rendered, because the old build would otherwise drop them, and it now renders only its own term. Nothing orders a tenant release after the Kamaji operator has rolled, so a `v1.32` tenant that sets gate terms of its own with OIDC on can lose them until the new operator reconciles it, which it then does on its own. The same window takes a `--requestheader-uid-headers` list of the operator's own that lacks `X-Remote-Uid`: the chart now renders its own entry beside it instead of refusing, and the old operator keeps one value per flag name, the chart's, so the operator's header is dropped until the new operator reconciles. Which entry an error message quotes is pinned only where one entry is eligible. Three variables choose between equally eligible entries, the gate dictionary taking the last setting for a name and the uid-headers ones the first, and no test distinguishes first from last when two entries qualify, so a future edit could make a message name the wrong entry of the two without a test noticing. What the chart renders and what it refuses is pinned exhaustively; it is the provenance in three message strings that is not. The block is deliberately inconsistent about two shapes that stop the apiserver equally hard: two `--emulated-version` entries naming kube make it withhold silently, while a kube-prefixed and a bare gate spelling in the same configuration make it refuse the render. The refusal is right where the apiserver rejects the configuration whatever the chart does; the withholding is right where the chart cannot read a version it must render against. They are two rules, not one, and a third that covers both is fair to ask for. Moving the refusal out from under those conditions so it fires everywhere is the obvious third rule and it is not free: the apiserver refuses the mix on `v1.31` too, but from `k8s.io/apiserver/pkg/util/version` rather than from the component-base `componentGlobalsRegistry.Set()` the message names, so hoisting it means rewriting that sentence as well. Reading a flag and its value written as two entries would be better than withholding for it, and is a change of behaviour rather than a correction, so it is not here. And two sentences about `--emulated-version` in `docs/oidc-tenant.md` sit thirty lines apart and read as a retraction in sequence, though each is true at its own layer: `toVersionMap` accepts an element naming a foreign component, and the registry rejects it later. What this leaves open, by name. The guard for a valueless flag at the end of `extraArgs` covers `--requestheader-uid-headers`, `--feature-gates` and `--emulated-version` only; any other flag that takes a value swallows the appended `--authentication-config` the same way, which is an input mistake the chart does not look for. The managed-flag refusal matches names as written, as Kamaji does: an underscore spelling such as `--secure_port` passes both, and the apiserver normalises it to `--secure-port`, where the operator's value wins as the later one, the same as before this change. Rendering `--authentication-config` ahead of the operator's entries would close that class for OIDC, since only the UID flags would then follow them; it changes the rendered order, and so rolls every OIDC tenant that has `extraArgs` of its own, and belongs in a separate change after the OIDC floor move. One pre-existing comment needs checking: the note above `extraArgs` in `templates/cluster.yaml` says that omitting the field would leave the live array untouched under JSON merge patch, which reads against Helm's `createPatch`, where a key the previous manifest carried and the new one does not is patched to null; what that comment was guarding against is still to be verified. The KamajiControlPlane CRD accepts only the plugin names it lists, so a plugin outside that list that the apiserver leaves off by default can no longer be enabled; that limit is Kamaji's, since it owns the flag and restricts the field. A name moved out of an `extraArgs` entry is not checked against that list by the chart, so one outside it fails when helm-controller applies the KamajiControlPlane, with an error that names it, rather than at render; the chart keeps no copy of the list to check it with. A repeated address type in a moved entry is dropped, keeping the first place it appears, because the field is a set. With `oidc.mode: None` the chart does not read `extraArgs` for the OIDC flags at all, and two shapes regress with the bump there: a kube feature-gate term written both bare and `kube:`-prefixed across entries, and two `--emulated-version` entries naming kube. The old build kept one entry per flag, the new one passes both, and the apiserver refuses both at startup. Next to a flag and value written as two entries the chart keeps the refusals that hold whatever the hidden value is, but it does not read positions: a hidden `--feature-gates` term keeps the opt-out refusal off even when it sits before the explicit opt-out and so cannot undo it. What the live run could not exercise: the konnectivity datapath, because the stand had no tenant workers, and `--requestheader-uid-headers`, which comes from the tenant Kubernetes chart and no image swap reaches. Both are covered by render and unit tests only. One shape the old collapse was quietly repairing, with OIDC on, needs a guard now that it is gone. A bare `--requestheader-uid-headers` in `extraArgs` used to be overwritten by the entry of the same name the chart renders, because the collapse kept one value per flag name; the same held for a bare `--feature-gates` on `v1.32`, where the chart renders a gate entry too. A bare `--emulated-version`, or a bare `--feature-gates` on any other version, had no entry of the chart's to be overwritten by and was emitted bare in sorted position, where it took the next flag as its value, so it never worked. Without it those entries reach the apiserver as written, and the apiserver gives a valueless long flag the next argument whatever that argument looks like. What follows the operator's last entry is the `--authentication-config` the chart appends, so the flag swallows it and the AuthenticationConfiguration is never applied. On `--requestheader-uid-headers` the path is accepted as a header name and the control plane comes up with OIDC silently off, failing every login for that tenant with nothing in the cluster to say why; the other two reject it and the apiserver exits. The chart refuses to render that shape and says which entry to fix. Two things in the image build fall out of the bump. The datastore deletion fix carried here as a patch landed upstream verbatim, so its patch no longer applies, and one failing patch fails the whole apply of the directory. The Go module overlay is obsolete for the opposite reason: this tag already carries `golang.org/x/net`, `golang.org/x/crypto` and `go.opentelemetry.io/otel/sdk` above the versions the overlay forced, so keeping it would downgrade all three. What I checked: `helm unittest` passes at 307 cases for the package, with the OIDC suite carrying every shape above, and the image builder stage builds end to end at the new tag with the one remaining patch. Kamaji's own support for Kubernetes 1.36 and 1.37 does not move the tenant version enum here; that is a separate update of the `kubernetes` package. No image reference in `packages/system/kamaji/values.yaml` is touched, since CI stamps that digest. ### Screenshots No UI changes. ### Downstream repositories I walked the trigger map file by file against the diff. The chart templates, the vendored Kamaji chart, the image build and the docs reach nothing downstream. The one generated file that reaches downstream is `packages/apps/kubernetes/values.schema.json`, which gains `controlPlane.apiServer.admissionControllers` and `controlPlane.kubelet.preferredAddressTypes` and a changed `extraArgs` description; both new fields are optional with defaults, so a stored Kubernetes object needs no change, no field is removed or renamed and no version enum moves. The new fields are the one trigger the map fires on: the Terraform provider keeps a hand-written schema. `packages/apps/kubernetes/README.md` feeds the website's managed-apps reference pages, which a bot regenerates on a stable tag. - [ ] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [x] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: model `controlPlane.apiServer.admissionControllers` and `controlPlane.kubelet.preferredAddressTypes` on the kubernetes resource (schema, model, expand/flatten). - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note ```release-note feat(kamaji): update Kamaji to 26.9.4-edge. Tenant kube-apiserver arguments are now derived from a hash of the control plane's extraArgs instead of from its length, so on the first reconcile after the upgrade every tenant control plane rewrites those arguments from scratch and rolls once. The rebuilt list also orders them differently, Kamaji's own flags sorted first and the operator's kept verbatim after them, so the roll happens whether or not the tenant sets any extraArgs. A tenant with OIDC enabled takes a second roll when the chart's own flags land. Rolling the Kamaji image back to an earlier build does not restore the old behaviour: the older code reads that annotation as a number, gives up when it finds a checksum, and from then on never clears a flag you remove from extraArgs, on any TenantControlPlane, until the annotation is set back to a number by hand. The tenant Kubernetes chart reads every extraArgs entry the way the apiserver accumulates them: a --requestheader-uid-headers list that omits X-Remote-Uid now gets the chart's own element beside it instead of failing the render, a feature gate set in an earlier entry is honoured, and naming the kube feature-gates component both bare and kube:-prefixed fails the render, because the apiserver refuses that configuration outright. Writing --feature-gates or --emulated-version as a flag and a value in two separate entries now keeps the chart from rendering UID flags on its own, because an entry the chart cannot read may hide an opt-out; a --requestheader-uid-headers list of your own still gets X-Remote-Uid beside it, and write --flag=value to get the rest back. Action required for tenants that pass their own apiserver flags: Kamaji now sets a list of flags itself and drops a controlPlane.apiServer.extraArgs entry naming one, where the previous build let that entry win, and the Kubernetes application now refuses to render such an entry, except for the two it moves into a field. A tenant carrying one of the others stops reconciling after the upgrade, and no later change to that tenant applies, until the entry is removed; the entry no longer has an effect either way. The list is in the description of controlPlane.apiServer.extraArgs. An --enable-admission-plugins=value entry is moved into the new controlPlane.apiServer.admissionControllers, a list that replaces the control plane's default set as the entry did, and a --kubelet-preferred-address-types=value entry into the new controlPlane.kubelet.preferredAddressTypes, while that field is empty; nothing has to be edited for them. Before upgrading, list the affected entries with: kubectl get kuberneteses.apps.cozystack.io --all-namespaces --output json | jq --raw-output '["--advertise-address","--client-ca-file","--enable-admission-plugins","--service-cluster-ip-range","--kubelet-client-certificate","--kubelet-client-key","--kubelet-preferred-address-types","--proxy-client-cert-file","--proxy-client-key-file","--requestheader-allowed-names","--requestheader-client-ca-file","--secure-port","--service-account-key-file","--service-account-signing-key-file","--tls-cert-file","--tls-private-key-file","--etcd-servers","--etcd-cafile","--etcd-certfile","--etcd-keyfile","--etcd-prefix","--etcd-compaction-interval","--egress-selector-config-file"] as $owned | .items[] | .metadata as $m | (.spec.controlPlane.apiServer.extraArgs // [])[] | select((split("=")[0]) as $f | $owned | index($f)) | "\($m.namespace)/\($m.name): \(.)"' Entries it lists for --enable-admission-plugins or --kubelet-preferred-address-types need a change only when written as two entries, when they name nothing, or next to a non-empty field of their own. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Configure tenant control-plane admission controllers and kubelet preferred address types. - Choose a cert-manager issuer for webhook certificates; self-signed certificates remain the default. - **Bug Fixes** - Improved OIDC argument handling for repeated, split, quoted, and conflicting settings, with clearer render-time errors. - DataStores are recognized as unused when no tenant control planes reference them. - **Validation** - DataStore endpoints are checked for length, count, and IPv6 address formatting. - Chart-managed tenant control-plane arguments are rejected when supplied through `extraArgs`. - **Documentation** - Updated Kubernetes OIDC and certificate issuer guidance. <!-- end of auto-generated comment: release notes by coderabbit.ai -->