mirror of
https://github.com/cozystack/cozystack.git
synced 2026-11-02 18:16:53 +09:00
11bac54f931682dd94de4081bb9e8a7eef60bef1
1615
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Aleksei Sviridkin
|
c290dee945 |
fix(vm-instance)!: refuse resources that cannot size the VM
resources.cpu and resources.sockets are typed as quantities, but they land in domain.cpu.cores and domain.cpu.sockets, which KubeVirt declares as uint32. The chart cast them with int64, which turns 500m, 1.5 or 1e1 into 0 without an error, and KubeVirt boots a zero core count as one core. The user asked for half a core and got a one-core VM with nothing reporting it. The raw spelling is now decoded as YAML, the way an integer field reads it, and the render stops unless the result is a positive whole number that fits in uint32. A leading zero is refused, because YAML reads 010 as octal 8. The check keys on presence rather than truthiness, so an unquoted 0 is refused instead of being read as unset. Memory gets the same presence check. A quoted "0" was truthy and rendered requests.memory: "0", while an unquoted 0 was falsy and dropped, so one quantity took two paths. A zero or negative amount now stops the render in both spellings. BREAKING CHANGE: values that used to render now either fail or render differently. A cpu or sockets that does not decode to a plain whole number within uint32 (a millicore, suffixed such as 1k, fractional, zero, negative or leading-zero value), and a zero or negative memory, fail the next upgrade of the release until the value is fixed. This includes cpu or sockets set on its own beside an instanceType, which used to be ignored whatever it held. A spelling that the int64 cast truncated to 0 but that YAML decodes to a whole number, such as the exponent 1e1 or the trailing dot 2., used to boot one core and now sizes the VM with the count it spells, so such a VM changes size on upgrade. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
8c5126ccd9 |
fix(kubernetes): give System-mode tenant OIDC users a UID (#4379)
## What this PR does Users authenticated through `spec.oidc.mode: System` on a tenant Kubernetes cluster now get a UID. The AuthenticationConfiguration the chart renders mapped `username` and `groups` and nothing else, and with no `claimMappings.uid` entry the apiserver resolves such a user to an empty UID. The `X-Remote-Uid` header the tenant control plane publishes then reaches an aggregated API server carrying nothing. The mapping takes the `sub` claim. The username claim is `email`, and the platform's Keycloak realm turns on the UPDATE_EMAIL required action, so that value is one a user changes; `sub` is the account UUID and stays where it is. Users on a cluster that is already running gain a UID where they had none. Tenant RBAC does not notice, because a role binding matches a username or a group. Audit records on the tenant apiserver do carry `user.uid` now, `kubectl auth whoami` prints it, and an admission webhook or extension server inside the tenant that read the empty value reads the subject instead. `CustomConfig` mode stays as it was. An operator-supplied configuration is written verbatim, uid mapping or not, and a unit test pins that the chart adds none of its own. Closes #4166 ### Screenshots Not a UI change. ### Downstream repositories Walked the trigger map against the diff, which touches the tenant chart's OIDC template, its unit tests, `docs/oidc-tenant.md`, and the shared e2e runner library with its bats twin. `values.schema.json` does not change. No values key is added, renamed or removed, no default moves and the version enum is untouched, so the terraform-provider's hand-written schema and its expand/flatten pair stay correct. No package is added or renamed and no `ApplicationDefinition` changes, so the website's app lists and generated reference pages are unaffected, and its tenant OIDC page carries no claim about UIDs that this could make stale. The installer values, the platform values and the variant list are untouched, so the ansible role is unaffected. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [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 fix(kubernetes): users authenticated through `spec.oidc.mode: System` on a tenant Kubernetes cluster now carry a UID, taken from the issuer's `sub` claim, where they previously had an empty one. Audit records, `kubectl auth whoami` and the `X-Remote-Uid` header that reaches an aggregated API server inside the tenant all show it. RBAC is unaffected: a role binding never matches on a UID. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * System-mode OIDC authentication now provides a stable user identifier based on the account subject. * Existing clusters now expose this identifier in audit records, `kubectl auth whoami`, and to extension servers without changing tenant RBAC behavior. * Custom OIDC configurations continue using operator-defined UID mappings. * **Documentation** * Updated OIDC documentation to explain stable identifiers and their visibility. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
IvanHunters
|
ff0b851bbc |
test(vpn): add default render helm unit tests (#4347)
## What this PR does vpn was one of the eight user-facing apps in `packages/apps/` called out in #3631 as shipping with zero helm-unittest coverage. This adds a `tests/` suite pinning the default render and wires the standard `test: helm unittest .` target the sibling charts already use, following the same pattern as #4081, #4074 and #4093. Ran `helm unittest .` locally against the chart, it passes. `make show` needs a live cluster connection through `cozyhr`, which was not available in this environment, so I relied on `helm unittest` for local verification, same as the referenced sibling PRs. ### Screenshots Not applicable, test-only change. ### Downstream repositories Walked the trigger map in docs/agents/contributing.md against the diff: this only adds a test file and a Makefile target inside an existing package, it does not add, rename or remove a package, and it does not touch `values.yaml`, `values.schema.json`, `Chart.yaml` or `README.md`, so none of the listed triggers apply. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [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 test(vpn): add Helm unit test coverage for the default render ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Tests** - Added automated validation for the VPN deployment’s default configuration, including replica count, container image, service port, and generated resource naming. - **Chores** - Added a convenient command for running the VPN chart’s automated tests. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
e2464ea788 |
fix(kubernetes): give System-mode tenant OIDC users a UID
The AuthenticationConfiguration the chart renders mapped username and groups only, so upstream resolved every System-mode user to an empty UID and the X-Remote-Uid header the tenant apiserver publishes reached an aggregated API server carrying nothing. Map uid to the sub claim. The platform realm enables Keycloak's UPDATE_EMAIL required action, so the address the username claim carries is mutable and cannot serve as a stable identifier, while sub is the account UUID and survives a change of either. CustomConfig mode is untouched: an operator-supplied configuration is still written verbatim, uid mapping included or not. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
4bf95815a8 |
fix(kubernetes): render the aggregation-layer UID header for tenant OIDC (#3539)
## What this PR does Tenant clusters with `spec.oidc.mode: System` or `CustomConfig` authenticate users fine, but a UID does not survive the hop to an aggregated API server. The tenant kube-apiserver publishes `requestheader-uid-headers` into the `extension-apiserver-authentication` ConfigMap only when the flag is set, and the chart never set it. The aggregation layer still sends `X-Remote-Uid`, but the extension server trusts only the header names published in that ConfigMap, so it ignores the one it gets and the call arrives without a UID. The field documentation even told operators to add the flag by hand via `controlPlane.apiServer.extraArgs`. It forwards a UID rather than creating one. ServiceAccount tokens carry one, and for those the aggregation hop starts working. Users authenticated by `spec.oidc.mode: System` still have none, because the generated AuthenticationConfiguration maps `username` and `groups` only; giving them one means mapping a claim, which changes the identity of users who have none today, and is tracked separately. The chart now renders `--requestheader-uid-headers=X-Remote-Uid` on v1.32 and newer, and adds `--feature-gates=RemoteRequestHeaderUID=true` on v1.32, where the gate is still alpha and the apiserver rejects the flag without it. From v1.33 the gate is beta and on by default, so only the header flag is rendered. v1.31 gets nothing: the flag does not exist there, and writing it by hand stops the apiserver from starting. `--emulated-version` in the same passthrough moves that decision, because the gate resolves through the versioned spec at the emulated version. Rendering the gate on every version from v1.32 rather than on v1.32 alone was the alternative, and it would remove the migration step entirely, because an opt-out would then drop two entries instead of one and move the entry count by itself. It loses on what it leaves behind: a redundant gate term on every cluster, which stops the apiserver from starting on the day the gate is retired from the known list. The design also prefers rendering over failing for the same reason the fail branches are narrow, since a cluster stranded on a HelmRelease that no longer converges is a worse outcome than one that never gets the UID fix. The chart reads an operator entry the way the apiserver reads it: header list elements matched whole and untrimmed and folded as `EqualFold` folds them, gate names compared exactly, gate values resolved as `strconv.ParseBool` resolves them, `AllAlpha` and `AllBeta` blankets honoured, an empty value treated as no entry. On v1.32 an entry that says nothing about `RemoteRequestHeaderUID` gets a second one carrying the operator's own terms plus `RemoteRequestHeaderUID=true`. Carrying those terms into the merged copy is what makes it land correctly whether the tenant control plane collapses duplicate flags into the last one or hands both to the apiserver, which merges them in the order it reads them. Tests cover the render matrix over mode, version and operator input: every fail path with its exact message, the ownership and merge paths, duplicate entries in both orders, blanket gates, empty values, and case variants on the header name and on the gate value. The latest-version tenant lane creates its tenant with `spec.oidc.mode: System` and asserts the flag on the rendered KamajiControlPlane, so the tenant apiserver starts with the flag in CI. Nothing asserts the UID arriving at an aggregated apiserver; that needs an extension server inside the tenant and is not in this change. The layer where the count-keyed reset actually bites is the tenant Deployment's arguments, and asserting there needs a live tenant control plane, so it is not in this change either. The rule this follows is that a configuration which renders and boots today keeps rendering. Both flags are new, so an `extraArgs` shape reaching this code was rendering before it existed, and stranding a running tenant on a HelmRelease that no longer converges is a worse outcome than one that never gets the fix. A few shapes do stop rendering, and reaching any of them takes writing `--requestheader-uid-headers` into the passthrough by hand, attempting exactly what the chart now does and getting it wrong; in each the kube-apiserver fails option validation and never starts, so nothing that was working is lost. `docs/oidc-tenant.md` covers these shapes. One known limitation is in there and worth naming here: on v1.33 and newer, adding the documented opt-out is count-neutral in `extraArgs`, and so is removing it again, and on v1.33 and v1.34 adding or removing `--emulated-version=1.31`. The pinned Kamaji build rewrites a running apiserver's arguments only when that count changes and otherwise keeps every flag that has left the list, so either the header flag stays beside a now-disabled gate or the disabled gate stays beside the header flag, and the apiserver refuses to start. Changing the entry count in the same edit is the mitigation, and the fix belongs to the pin rather than to this chart, tracked in #3541. The paths that need no operator edit are clear: turning the feature on for a default tenant moves the count, and a tenant already carrying the opt-out renders byte-identically before and after. Out of scope: `mode: None` still publishes no UID header even though ServiceAccount tokens carry UIDs. That is pre-existing, and widening the condition changes the render for every default tenant. Tracked separately, along with the claim mapping above. Closes #3377 ### Screenshots Not a UI change. ### Downstream repositories - [x] No downstream repository is affected by this change Checked the trigger map against the diff: no package added or removed, no schema field, enum or default changed (description text only), no ApplicationDefinition surface touched, no hack/ layout moved. The managed-apps reference page picks up the new field description through release-time README regeneration. ### Release note ```release-note fix(kubernetes): tenant clusters with `spec.oidc.mode` set to `System` or `CustomConfig` now render `--requestheader-uid-headers=X-Remote-Uid` on the tenant kube-apiserver, plus the `RemoteRequestHeaderUID` feature gate on v1.32, so that an aggregated API server can read the caller's UID out of the request headers. Without that flag the tenant apiserver never publishes `requestheader-uid-headers`, and an aggregated API server ignores the UID header even for callers that have one, ServiceAccount tokens among them. It forwards a UID rather than creating one: users authenticated by `spec.oidc.mode: System` still have none, because the generated AuthenticationConfiguration maps `username` and `groups` only, and that is tracked separately. Adding the flag by hand through `controlPlane.apiServer.extraArgs` is no longer necessary, and an entry of your own is respected: the chart steps aside when yours already carries `X-Remote-Uid` or already enables the gate, merges `RemoteRequestHeaderUID=true` into a copy of a v1.32 `--feature-gates` entry that says nothing about the gate, and renders neither flag when you switch the gate off. The render fails where a `--requestheader-uid-headers` list of your own would stop the kube-apiserver from starting anyway: one that omits `X-Remote-Uid`, one with an empty element, and a list of your own while the gate is off. On v1.33 and newer there is a migration step, and it applies in both directions: the pinned Kamaji build rewrites a running apiserver's arguments only when the number of `extraArgs` entries changes, and otherwise keeps every flag that has left the list. Adding the documented opt-out keeps that number the same, so the header flag already on the Deployment stays beside the now-disabled gate. Removing the opt-out later keeps it the same too, so the disabled gate stays on the Deployment beside the header flag the chart renders again, although your spec no longer shows it. Adding or removing `--emulated-version=1.31` has the same shape both ways on v1.33 and v1.34. The apiserver refuses to start in each case, so change the entry count in the same edit. ``` |
||
|
Aleksei Sviridkin
|
12e64a99cd |
fix(rabbitmq): right-size the default resources preset to s1.nano (#3936)
## What this PR does
Raises the RabbitMQ default resource preset from `t1.nano` (128Mi
memory) to `s1.nano` (250m CPU / 512Mi memory).
A default-sized RabbitMQ 4.2 broker on Cozystack 1.6.1 was OOMKilled
with exit 137, reached 10 restarts, and never became Ready. A canary on
a larger preset reached 3/3 Ready with zero restarts; its working set
peaked at 125.8Mi. Against the 128Mi `t1.nano` ceiling that is 98% of
the limit in steady state, and cgroup accounting (working set plus page
cache) crosses it under load. The canary and its storage were removed
after validation.
### Sizing
`s1.nano` keeps the CPU of `t1.nano` (250m) and quadruples memory to
512Mi — a 4x margin over the observed 125.8Mi peak.
An earlier revision of this PR proposed `u1.nano` (1Gi). Review
correctly objected that 1Gi reserves 8x the observed working set with no
measured peak to justify it, and asked to either report that peak or
right-size the default. The peak is reported above, and the default is
right-sized to the smallest preset that clears the ceiling with a real
margin rather than to the first one that is comfortably large.
### Upgrade impact
cozystack-api applies OpenAPI schema defaults on the read path and
stores what the client sent: `Get` runs `applySpecDefaults` on its way
out (`pkg/registry/apps/application/rest.go:1232`) and the write puts
`app.Spec` into `HelmRelease.spec.values` (`rest.go:1652`). That splits
existing brokers in two.
**Brokers with no stored `resourcesPreset`** pick up the new chart
default on the next Helm reconcile. For them this upgrade is:
- a rolling restart of every replica, and
- a rise in guaranteed memory reservation from 128Mi to 512Mi per
replica, because the sanitize helper sets memory request equal to limit
— roughly 1.1Gi extra reserved per 3-replica instance.
**Brokers whose values went through a read-modify-write keep `t1.nano`
and are not resized.** `Update` opens with `r.Get` (`rest.go:489`), so a
GET-then-PUT — `kubectl edit` is the unambiguous case, and any client
that reads before writing behaves the same — freezes the schema default
of the moment into the stored values, and a stored value beats the chart
default. Those brokers are not restarted, not resized, and keep hitting
the ceiling this PR raises.
Nothing downstream can separate a deliberate `t1.nano` from one a round
trip baked in, so this is documented rather than migrated. After the
upgrade the affected brokers are the ones that still report the old
preset:
```
kubectl get rabbitmq -A -o custom-columns=NS:.metadata.namespace,NAME:.metadata.name,PRESET:.spec.resourcesPreset
```
The same command before the upgrade tells you nothing: the cozyrd
default is still `t1.nano` then, so the read path fills it in for both
groups. Afterwards, anything reading `t1.nano` carries a stored value;
setting `resourcesPreset: s1.nano` explicitly on the ones that should
move is the fix.
PVCs are untouched, so there is no data loss, but node headroom should
be checked before upgrading.
Also worth knowing when reading the parameter table: the preset does not
only apply to a fully omitted `resources`.
`cozy-lib.resources.defaultingSanitize` merges it key by key, so a
broker pinned to `resources: {cpu: 700m}` keeps its CPU and moves from
128Mi to 512Mi with everyone else. The field descriptions now say so,
and a test pins it.
The values, generated schema, ApplicationDefinition, typed API, and
README all carry the same default. Tests cover the new default, an
explicit legacy override, explicit resources, and a partially set
`resources` map inheriting the preset for what it leaves out.
Validation:
- `helm template` with the chart's `cozy-lib` dependency renders `limits
250m / 512Mi` and `requests 0.025 / 536870912` for the default; an
explicit `t1.nano` renders `128Mi / 134217728`; explicit `700m / 2Gi`
renders `0.07 / 2147483648`. The unit test asserts exactly these values.
- Provider counterpart: cozystack/terraform-provider-cozystack#34.
### Screenshots
Not a UI change.
### Downstream repositories
The default is restated by the Terraform provider, so the linked
provider PR updates it and documents the state-plan implication. No
other trigger-map entry matches this diff.
- [ ] 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:
https://github.com/cozystack/terraform-provider-cozystack/pull/34
- [ ]
[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:
### Release note
```release-note
fix(rabbitmq): raise the default resource preset from t1.nano to s1.nano so default-sized brokers are no longer OOMKilled. Brokers with no stored `resourcesPreset` are restarted on upgrade and their guaranteed memory reservation grows from 128Mi to 512Mi per replica; check node headroom before upgrading. Brokers whose values already pin `resourcesPreset`, including ones where a read-modify-write through the API froze the old default into them, keep their old size: list them after the upgrade with `kubectl get rabbitmq -A -o custom-columns=NAME:.metadata.name,PRESET:.spec.resourcesPreset` and set `resourcesPreset: s1.nano` explicitly on the ones that should move. The preset also fills any resource a partially set `resources` leaves out, so a broker pinned to cpu alone moves too. A tenant that opted into `resourceQuotas` and sits near its memory ceiling has the extra reservation rejected by quota admission, which shows up as a StatefulSet that never scales rather than as pods left Pending.
```
|
||
|
scooby87
|
7d07bbe3ce |
[kafka] Migrate from ZooKeeper to KRaft mode (#2352)
## Summary - Replace ZooKeeper-based Kafka deployments with KRaft (Kafka Raft) metadata mode - Existing ZooKeeper instances migrate automatically and in place via a Helm `pre-install,pre-upgrade` hook Job that drives Strimzi's ZK-to-KRaft migration state machine - New instances deploy directly in KRaft mode with broker and controller `KafkaNodePool`s - The node pools are owned by the migration hook, not by regular chart manifests (see "Why the pools are hook-managed") - Kafka version is user-configurable (v3.9 default, v3.8 available) - Strimzi operator is unchanged in this PR (stays at 0.45.1-rc1); 0.45.x is the last line that supports ZooKeeper→KRaft migration - ZooKeeper removed from the dashboard UI (ApplicationDefinition) ## Migration strategy Both fresh installs and ZooKeeper→KRaft migrations are driven by a single Helm hook Job (`templates/migration-hook.yaml`, `pre-install,pre-upgrade`) that owns the `KafkaNodePool`s. On every install and upgrade the Job: 1. Ensures both node pools exist (idempotent): a broker pool named exactly `kafka` for a migrating cluster — so Strimzi adopts the existing `<cluster>-kafka-N` brokers and their PVCs in place — or `b-<hash>` for a fresh cluster, plus a controller pool `c-<hash>`. The controller pool pins `strimzi.io/next-node-ids` above the broker range so the broker pool keeps node IDs `[0..N-1]` and Strimzi cannot hand the existing broker IDs to the fresh controllers. The shared `kafka` name is claimed atomically (via `kubectl create`) so only one migration per namespace can win it. 2. Inspects the Kafka CR: no CR → fresh install (pools ensured; the main apply then creates the KRaft Kafka CR, nothing to migrate); already `strimzi.io/kraft=enabled` or `status.kafkaMetadataState: KRaft` → done; any other read error → fail closed. Otherwise (a ZooKeeper cluster) it migrates. 3. Annotates `strimzi.io/node-pools=enabled` then `strimzi.io/kraft=migration`, polls `status.kafkaMetadataState` through `ZooKeeper → KRaftMigration → KRaftDualWriting → KRaftPostMigration → PreKRaft → KRaft`, and only once a safe state is reached flips `strimzi.io/kraft=enabled`. If it times out in an intermediate state it aborts without finalising, so Helm fails the upgrade and the next reconcile retries from where it left off. Data is migrated in place: brokers keep their node IDs and reuse their existing PVCs, nothing is copied. Migration is one-way — Strimzi has no supported KRaft→ZooKeeper rollback once complete, so lowering the `version` field or rolling the app back to a pre-KRaft chart is unsupported (take a backup first). ### Why the pools are hook-managed The hook must create the pools and walk the migration state machine *before* the main release apply. A resource a hook creates cannot also be a regular chart manifest across the ZK→KRaft boundary: Helm's main apply finds the pool live in the cluster but absent from the previous (ZooKeeper) release revision and aborts with `original object KafkaNodePool ... not found`, leaving the release `failed` even though the cluster migrated fine (reproduced under server-side apply, the helm-controller default). Managing the pools only from the hook avoids that, so `templates/kafkanodepools.yaml` and the deployed-version gate ConfigMap (`templates/version.yaml`) are removed and the pools are applied idempotently by the Job on every install and upgrade. ## Bug fixes included - ZooKeeper `metricsConfig` referenced wrong ConfigMap key (`kafka-metrics-config.yml` instead of `zookeeper-metrics-config.yml`) - ZooKeeper `storageClass` used `kafka.storageClass` instead of `zookeeper.storageClass` - WorkloadMonitor used `.Values.replicas` instead of `.Values.kafka.replicas` - `validateNameLength` now caps a Kafka app name at the KRaft controller pod-hostname budget (`<release>-c-<hash>-<id>` must fit 63 chars), instead of admitting names that later fail the chart render forever ## What's next A follow-up PR will upgrade Strimzi to 0.51.0 and Kafka to 4.2 once all instances are confirmed migrated to KRaft. That PR is already prepared on branch `KRaft-upgrade-strimzi-051`. ## Test plan - [x] `helm template` renders correctly for new KRaft instances (no ZooKeeper refs) - [x] `helm template` with custom values (1 replica, external, topics, v3.8) - [x] Version validation rejects unsupported versions via JSON schema - [x] helm-unittest: 86/86 across 9 suites - [x] `hack/kafka-migration-hook.bats`: 12/12 — the real hook script executed against a stub kubectl (resolution branches, fail-closed reads, atomic collision guard, finalize guard) - [x] Operator template renders with the unchanged Strimzi 0.45.1-rc1 - [x] Live migration on a disposable cluster under server-side apply: fresh install → healthy KRaft, release `deployed`; in-place migration of a 20000-message ZooKeeper cluster → release `deployed`, broker PVCs adopted in place (same UIDs), and the KRaft data byte-for-byte identical to the pre-migration ZooKeeper data (matching sha256 over every `partition:offset:key:value`) ### Release note ```release-note [kafka] Enable KRaft metadata mode with automatic, in-place ZooKeeper-to-KRaft migration. Kafka version is now configurable (v3.9, v3.8). The Strimzi operator is unchanged (0.45.1-rc1). ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added chart-level Kafka version selector (default v3.9) and KRaft-aware node pool rendering with controller storage sizing. * **Updates** * Default Kafka appVersion updated to 3.9.0; operator and image tag defaults aligned to 0.45.0. * Templates now omit ZooKeeper resources and metrics when KRaft is in use. * **Tests** * New Helm unit tests for Kafka CR, node pools, metrics, topics, dashboard mappings, and workload monitors. * **Documentation** * Values, schema, and README updated to document new version and controller storage options. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
f4e923aa82 |
docs(kubernetes): restate the apiServer extraArgs contract for OIDC
The extraArgs field documentation told operators to add --requestheader-uid-headers themselves, which the chart now renders whenever OIDC is on. Describe what it renders per version, when it defers to an operator entry, when it merges into one, and that it still refuses to render shapes the apiserver would not start on. The version field says it too: choosing v1.32 is what makes the chart render the feature gate next to extraArgs, and a reader of that field alone would otherwise not learn that the two interact. The package's hack/update-versions.sh writes that description itself on every make update, so it carries the same text, or the next version bump would drop the sentence again. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
d870b392fa |
fix(kubernetes): render the aggregation-layer UID header for tenant OIDC
A tenant kube-apiserver publishes requestheader-uid-headers into the extension-apiserver-authentication ConfigMap only when the flag is set, and the chart never set it. Without that key the request-header authenticator on an aggregated apiserver does not know which header carries the UID and ignores it, so a caller who has a UID is served without one. Operators had to add the flag by hand through the controlPlane.apiServer.extraArgs escape hatch. This forwards a UID the caller already has and does not create one. ServiceAccount tokens carry a UID and gain from it. OIDC users authenticated through spec.oidc.mode: System do not: the AuthenticationConfiguration the chart generates maps username and groups only, and with no claimMappings.uid upstream returns an empty UID (plugin/pkg/authenticator/token/oidc/oidc.go, getUID, read at v0.35.0), so there is nothing to carry. Mapping a claim would give those users a UID and change the identity of users who have none today, which is a separate decision and is tracked separately. The half read rather than run: that an existing UID is lost without the published key comes from the controller writing the ConfigMap and the request-header authenticator consuming it, not from a live cluster. That OIDC users have no UID is unambiguous in the source named above. Render --requestheader-uid-headers=X-Remote-Uid whenever OIDC is on, plus --feature-gates=RemoteRequestHeaderUID=true on v1.32, where the gate is still alpha and the apiserver rejects the flag without it. v1.31 gets nothing: the flag does not exist there and an unknown flag is fatal to the apiserver. The deciding version is the effective one, not the image tag. --emulated-version in the same passthrough moves it down, and the gate resolves at the emulated version through featureSpecAtEmulationVersion (featureSpecAtEmulationAndMinCompatVersion by v1.35), so an emulated 1.32 needs the gate line even on a v1.35 cluster, and below 1.32 the gate is PreAlpha and cannot be enabled at all, which leaves nothing safe to render. Both flags are new, so every extraArgs shape reaching this code was rendering before it existed, and turning one of those into a render error would strand a running tenant on a HelmRelease that stops converging while its control plane carries on. The chart therefore yields to an operator entry that already satisfies the requirement, renders nothing at all when the operator turned RemoteRequestHeaderUID off, and otherwise merges its gate term into a copy of the operator's --feature-gates value. Carrying that value into the merged entry decides the gate whether the tenant control plane collapses duplicate flags into the last one or hands both to the apiserver, which merges them in the order it reads them. Operator entries are read the way the apiserver reads them: header list elements matched whole and untrimmed, folded as EqualFold folds them, and refused when one is empty after trimming, as checkForWhiteSpaceOnly refuses it; gate names compared exactly; gate values resolved as strconv.ParseBool resolves them; blanket AllAlpha and AllBeta honoured; an empty value treated as no entry at all. Each flag is judged by its last entry carrying a value. The apiserver's CSV reader strips quotes the chart does not parse, so quotes on that entry make the chart render nothing, while quotes on an earlier entry the collapse drops do not. The render fails only where a uid-headers list of the operator's describes a control plane that already refuses to start: one that omits X-Remote-Uid, one with an element that is empty after trimming, and any such list while the gate is off. Withholding the chart's own flag rescues none of them, because the flag the apiserver rejects is the operator's. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Akanksha Trehun
|
422b6f65ea |
test(vpn): add default render helm unit tests
vpn was one of the remaining user-facing apps with no helm-unittest coverage. Signed-off-by: Akanksha Trehun <akankshatrehun@gmail.com> |
||
|
Yan Bondarenko
|
78f01d5542 |
docs(rabbitmq): point the preset section at the shared size matrix
The preset table in this README listed the seven legacy aliases only, and its CPU column disagreed with cozy-lib on every row, so a reader looking up what the new s1.nano default buys them found neither the name nor a correct number. Nine app READMEs already replaced that table with the two paragraphs that describe the <series>.<size> convention and link the full matrix in docs/operations/resource-presets.md; this README now carries the same text. Assisted-by: LLM Signed-off-by: Yan Bondarenko <202671653+yankawai@users.noreply.github.com> |
||
|
Yan Bondarenko
|
6d5666f7e6 |
fix(rabbitmq): describe the preset as filling what resources leaves unset
cozy-lib merges the preset under resources key by key, so a partially set resources map still takes the preset for everything it omits: with resources.cpu=700m alone the broker renders 700m of CPU against the preset's 512Mi of memory, and this upgrade moves that broker from 128Mi to 512Mi as well. The description said the preset applies when resources is omitted, and cozyvalues-gen copies it verbatim into values.schema.json, the README parameter table and the cozyrd openAPISchema the dashboard renders, so one sentence made the claim on three surfaces. The suite covered a fully omitted resources map and one that sets both cpu and memory, which is the one shape that does not exercise the merge. The new case pins it: reverting defaultingSanitize to replace the preset instead of merging over it turns that case red and leaves the other three green. Assisted-by: LLM Signed-off-by: Yan Bondarenko <202671653+yankawai@users.noreply.github.com> |
||
|
Yan Bondarenko
|
03394bd5fe |
chore(rabbitmq): drop the test target the rebase duplicated
main already carries a test target at the bottom of the Makefile; the rebase kept the earlier copy at the top and GNU make warns about the override. The Makefile matches main again. Assisted-by: LLM Signed-off-by: Yan Bondarenko <202671653+yankawai@users.noreply.github.com> |
||
|
Yan Bondarenko
|
cf749988a4 |
fix(rabbitmq): right-size the default resources preset to s1.nano
Signed-off-by: Yan Bondarenko <202671653+yankawai@users.noreply.github.com> Assisted-by: LLM |
||
|
Alexey Artamonov
|
bad152443e |
fix(kafka): correct the stale co-namespaced migration render error
The brokerPoolName render guard still told operators to "migrate one at a time or use separate namespaces" when a sibling cluster owned the shared "kafka" pool. That predates automatic name freeing: the migration hook now drains the first cluster off the shared pool with a Cruise Control rebalance and releases the name, and the waiting cluster's HelmRelease retries into it — every co-namespaced Kafka reaches KRaft, no manual action required. Reword the fail message and the helper comment to describe the transient wait-and-retry, and update the unit test's error pattern to match. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Alexey Artamonov
|
ff637ed9a2 |
fix(platform): validate the realm-name charset and regenerate kubernetes artifacts
The realm-name guard checked only the type and echoed the parsed value back, so its hint told an operator who wrote `on` to set realmName: "true" — the token YAML had already discarded. Drop the echo and add a slug-charset check: the name is spliced verbatim into URL path segments and a shell token, so a "/" or a space must be rejected at the source rather than split a URL or the hook script downstream. The description edit in the previous commit was hand-applied to the generated kubernetes README and never reached the Go types, leaving pre-commit red; regenerate both with cozyvalues-gen. Finish the oidc-tenant System-mode prose that still read cozy as a fixed name. Assisted-by: LLM Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Alexey Artamonov
|
949096694b |
fix(platform): reject non-string realm names and shell-quote the hook realm
A realmName that YAML resolves to an int or a bool (a bare 123, or a reserved word like on/off) was coerced with toString, silently renaming the realm to something the operator never typed (on -> "true"). Fail the render with a quoting hint at the source instead of surfacing it later as a broken issuer URL. The update-email hook Job interpolated the realm into its shell script unquoted, so a name carrying a space or a shell metacharacter could break or inject into the script; single-quote it. The Grafana and managed-cluster issuer helpers build the URL with printf %s straight off the channel value, so guard them with toString for the same non-string reason. Name the configured realm in the tenant kube-apiserver oidc.mode description and the Grafana identity-mode comment, which still read cozy as if it were fixed. Assisted-by: LLM Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Alexey Artamonov
|
a3df02df23 |
fix(kubernetes): name the configured realm in the OIDC denial message
The claimValidationRules message in the System-mode AuthenticationConfiguration hardcoded "in the cozy realm", so on a renamed realm it told a rejected user to look in a realm that no longer exists. Build it from the same realm the issuer uses (toString-guarded, as the derived issuer URL already is). Assisted-by: LLM Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Alexey Artamonov
|
44a9a8e735 |
feat(platform): make the Keycloak realm name configurable
The realm name "cozy" was hardcoded in keycloak-configure (which owns the realm, its clients and the cozystack-cluster-admin roles), in the dashboard and linstor-gui gatekeepers, in the info kubeconfig, and in the Grafana and managed-Kubernetes apiserver OIDC issuers. Operators could not rename the realm without patching vendored templates, which `make update` reverts. The apiserver OIDC issuer is already configurable via talm oidcIssuerUrl, so pointing it at a custom realm left every SSO component on "cozy" and broke login with an issuer mismatch. authentication.oidc.realmName (default "cozy") now rides the _cluster channel so every consumer resolves the same realm, and the default keycloakInternalUrl is derived from it so the realm is named in one place. Assisted-by: LLM Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Alexey Artamonov
|
8ef230f0bf |
Merge remote-tracking branch 'origin/main' into kafka-multi-instance
Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> # Conflicts: # docs/storage-immutability.md # packages/apps/kafka/tests/topics_test.yaml # packages/system/kafka-rd/cozyrds/kafka.yaml |
||
|
Aleksei Sviridkin
|
3219f7ee9a |
fix(backups): give the barman-cloud sidecar its own resources (#4220)
## What this PR does The barman-cloud plugin injects its sidecar with no resource requests or limits. What that means depends on the namespace. A tenant with `resourceQuotas` set ships a `LimitRange` that defaults containers to 128Mi (`packages/apps/tenant/templates/quota.yaml`), so there the sidecar inherits that default and is OOMKilled mid-backup. A namespace without a `LimitRange`, which is `cozy-keycloak` and any tenant that leaves `resourceQuotas` empty, gave the sidecar no requests and no limit at all, so nothing reserved memory for it and nothing bounded it. The failure on the tenant path is quiet from the control plane: the `ObjectStore` stays healthy, the `Cluster` stays `Ready`, and only the backup fails. Observed on a 1.6 cluster while backing up a 38 MB database: `lastState.terminated.reason: OOMKilled`, `exit 137`, four restarts on the sidecar, and a cgroup high-water mark of 254 MiB. `container_memory_working_set_bytes` peaked at only 64 MiB over the same window — it is sampled, and it misses the spike that actually triggers the kill, which is why the working-set number does not explain the failure on its own. Both paths that build an ObjectStore now carry resources: - `cozy-lib.barman.sidecarConfiguration`, used by `packages/apps/postgres` for the backup and recovery ObjectStores and by `packages/system/keycloak`; - `barmanSidecarConfiguration()` in `internal/backupcontroller/cnpgstrategy_controller.go`, which builds the platform's own ObjectStore on the `useSystemBucket=true` path. Requests are 100m/256Mi with a 1Gi memory limit on every caller: 256Mi holds the measured working set and 1Gi is four times the measurement, which is also the ceiling a caller without a LimitRange gets where it had none. No CPU limit is set, following the `entityOperator` precedent in `packages/apps/kafka/templates/kafka.yaml`: a throttled sidecar stalls WAL archiving instead of failing it, which is harder to notice than an outright failure. Measured CPU peak was 50 mCPU. Inside a tenant the limit is charged against the `ResourceQuota` `limits.memory` budget, 1Gi per instance pod; a 4Gi tenant spends a quarter of it per Postgres instance. The helper is renamed from `checksumSidecarConfiguration` to `sidecarConfiguration`, because it no longer carries only the checksum pin and its name and doc comment would otherwise be wrong. Tests cover both paths and both render sites: a new helm suite in `packages/apps/postgres`, extra assertions in the existing keycloak suite, an assertion in the controller test, and a deepcopy test for the new field. Each of them fails against the unfixed sources. One thing this PR deliberately does not do. This is the fifth time the 128Mi tenant default has been worked around per component — kafka and zookeeper presets (#2537), the kafka entity-operator (#2934), the cert-manager cainjector (#3199), and the note carried in the etcd-operator values. Whether the default itself should move looks like your call rather than something to fold into a fix for one sidecar, so it is left alone here. ### Screenshots Not a UI change. ### Downstream repositories Walked the trigger map in `docs/agents/contributing.md` against the diff, entry by entry. The change touches one cozy-lib helper, the two chart templates that include it, one Go function, and tests. It adds, renames or removes no package; changes no `values.yaml`, `values.schema.json`, `Chart.yaml` or `README.md`, so no version enum, default or generated artifact moves; changes no `ApplicationDefinition` semantics, no `release.prefix`, no output Secret or Service name; touches no CRD, no namespace, no `hack/` file, no telemetry metric or label, no `cozy-proxy` annotation, no node or network requirement, and no commit or PR convention. No entry matches. - [x] No downstream repository is affected by this change ### Release note ```release-note fix(backups): give the barman-cloud sidecar its own resources The barman-cloud sidecar was injected without resource requests or limits, so in tenant namespaces it inherited the 128Mi `LimitRange` default and was OOMKilled during backups while the `ObjectStore` and the `Cluster` both stayed healthy. It now requests 100m/256Mi and caps memory at 1Gi on the chart path and on the platform's Go path alike; in namespaces without a LimitRange the sidecar had no limit before and gets the same 1Gi ceiling. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added explicit CPU and memory resource settings for barman-cloud backup and recovery sidecars. - Backup sidecars request 100m CPU and 256Mi memory, with a 1Gi memory limit. - Recovery sidecars receive a 1Gi memory limit. - Preserved the S3 checksum configuration. - **Bug Fixes** - Prevented sidecars from inheriting unsuitable tenant resource limits. - Ensured resource settings are preserved when backup configuration is copied. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
ee09e73343 |
fix(kafka): make topics[].config optional (#3935)
## What this PR does Makes `topics[].config` optional across the Kafka typed API, generated schema, documentation, and chart contract. The KafkaTopic template already omits an empty config, and Strimzi accepts the field as optional; the Cozystack schema was the layer incorrectly rejecting such topics. The Helm test renders a topic without config and asserts `spec.config` is absent. The Chainsaw fixture also omits config and waits for the resulting KafkaTopic to reach Ready. Go API compatibility: `Topic.Config` changes from `runtime.RawExtension` to `*runtime.RawExtension`, so Go callers constructing that field must pass a pointer. The Terraform provider intentionally does not model Kafka topic tuning (`config`), so this does not require a provider follow-up. Validation: - `helm unittest packages/apps/kafka`: 36/36 passed. - `go vet ./kafka` in `api/apps/v1alpha1`: clean; the package has no test files, so this shows it compiles. - root `make generate`, which owns `zz_generated.deepcopy.go`, and `make -C packages/apps/kafka generate`: no diff. ### Screenshots Not a UI change. ### Downstream repositories Walked the trigger map against all nine changed files. The provider's Kafka model deliberately excludes `topics[].config`; no downstream repository mirrors this nested field or its requiredness. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [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: ### Release note ```release-note fix(kafka): allow topics to omit the optional Strimzi config map ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Kafka topic configuration is now optional. Topics can be created without explicitly providing configuration settings. - Topic configuration is omitted from serialized resources when not specified. - **Bug Fixes** - Improved handling of topic resources without configuration, including correct readiness validation and rendering. - **Documentation** - Updated Kafka configuration documentation and schemas to reflect that topic configuration is optional. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Yan Bondarenko
|
9fac0add84 |
docs(mongodb): describe the upgrade as a carry-over, not a rotation
The upgrade section still promised a password rotation and a regenerated credentials Secret. With the operator-copy fallback the chart reads the current passwords from internal-<release>-users and writes the same values, so nothing rotates and no session breaks. Assisted-by: LLM Signed-off-by: Yan Bondarenko <202671653+yankawai@users.noreply.github.com> |
||
|
Yan Bondarenko
|
0e17c6f65e |
test(mongodb): pin the reuse, fallback and ownership branches with lookup fixtures
Every test rendered with an empty lookup, so the branch that keeps a live password could be deleted with the suite green. Three fixtures through kubernetesProvider cover what a fresh render never reaches: a helm-owned users Secret is reused, a Secret the release does not own stays out of the manifest while its password still reaches the credentials, and the operator copy rebuilds a missing users Secret. Values with URI-reserved characters cover the userinfo encoding, and both Secrets are asserted Opaque again. Assisted-by: LLM Signed-off-by: Yan Bondarenko <202671653+yankawai@users.noreply.github.com> |
||
|
Yan Bondarenko
|
f0da352b30 |
fix(mongodb): refuse to invent a password for a users Secret the chart does not own
On the not-owned path a missing password key produced a generated password that reached <release>-credentials but never the users Secret the operator reads, so the dashboard advertised a password MongoDB never had. The render now fails and names the Secret and the key. The operator fills every key itself, so only a hand-made partial Secret reaches this branch. Assisted-by: LLM Signed-off-by: Yan Bondarenko <202671653+yankawai@users.noreply.github.com> |
||
|
Yan Bondarenko
|
348a65d18b |
docs(mongodb): keep the credentials paragraphs on one line
Two paragraphs under Credentials were hard-wrapped at 80 columns, the one place in this README that was; the rest follows the one-line-per-paragraph rule the repository asks for in markdown. Signed-off-by: Yan Bondarenko <202671653+yankawai@users.noreply.github.com> Assisted-by: LLM |
||
|
Yan Bondarenko
|
66a00f552e |
fix(mongodb): url-encode the username in the connection uri
Signed-off-by: Yan Bondarenko <202671653+yankawai@users.noreply.github.com> |
||
|
Yan Bondarenko
|
b4be956d7e |
fix(mongodb): fill the dashboard credentials on a first install
Signed-off-by: Yan Bondarenko <202671653+yankawai@users.noreply.github.com> |
||
|
Yan Bondarenko
|
4dc6065ec9 |
fix(backups): give the sidecar sizing its reason per caller
The Keycloak comment and the helper doc claimed the tenant LimitRange as the reason for the resources, but cozy-keycloak has no LimitRange and neither does a tenant that leaves resourceQuotas empty: there the sidecar had no requests and no limit at all, and the change gives it a 1Gi ceiling where there was none. The comments and both doc blocks now say what applies to each caller. No rendered object changes. Assisted-by: LLM Signed-off-by: Yan Bondarenko <202671653+yankawai@users.noreply.github.com> |
||
|
Yan Bondarenko
|
9214430bd8 |
fix(backups): give the barman-cloud sidecar its own resources
The barman-cloud plugin injects its sidecar without resource requests or limits. Tenant namespaces ship a LimitRange that defaults containers to 128Mi, so the sidecar inherits that default and is OOMKilled mid-backup: the ObjectStore stays healthy, the Cluster stays Ready, and only the backup fails. A cgroup high-water mark of 254 MiB on a 38 MB database shows the default cannot hold it. The shared cozy-lib helper and the Go path that builds the platform's own ObjectStore now request 100m/256Mi and cap memory at 1Gi. No CPU limit is set, matching the entityOperator precedent in the kafka chart: a throttled sidecar stalls WAL archiving instead of failing it. The helper is renamed from checksumSidecarConfiguration to sidecarConfiguration, since it no longer carries only the checksum pin. Signed-off-by: Yan Bondarenko <202671653+yankawai@users.noreply.github.com> |
||
|
Yan Bondarenko
|
8ec76a3614 |
fix(kafka): make topics[].config optional
Signed-off-by: Yan Bondarenko <202671653+yankawai@users.noreply.github.com> |
||
|
Aleksei Sviridkin
|
08a2e76707 |
fix(opensearch): remove the data PVCs when the application is deleted (#4136)
## What this PR does Fixes #4086. The OpenSearch operator provisions its data volumes through a StatefulSet, so the PVCs carry no ownerReference, and the `opensearch` app chart has no cleanup hook. Removing the application therefore leaves every data volume `Bound`, counting against the tenant's storage quota with nothing left in the namespace that could reclaim it. `clickhouse`, `kafka` and `harbor` remove their operator-owned PVCs in a post-delete hook. This adds a post-delete Job to `opensearch` that requests the deletion of the claims labelled `opster.io/opensearch-cluster=<release>` with `--ignore-not-found --wait=false` and exits non-zero when the request fails, with its own ServiceAccount, a Role limited to `persistentvolumeclaims` (`get`, `list`, `delete`), the `clastix/kubectl` image pinned by digest through `cozy-lib.image`, a non-root restricted securityContext, `activeDeadlineSeconds: 120` so a kubectl that never returns cannot hold the uninstall, and `before-hook-creation` alone on the Job so a failed run and its log survive; the ServiceAccount, Role and RoleBinding keep `hook-succeeded`. The delete does not wait for the claims to disappear, on purpose: OpenSearch data pods stop slowly, and a claim they still hold would keep a waiting hook running into helm-controller's 300s uninstall budget. The claims go once their pods have stopped, which is what the log says. This is the shape the monitoring, etcd and seaweedfs hooks use. Retention is not offered as a knob, consistent with the other charts; an opt-out can be added if the maintainers prefer one. `helm unittest`: 64/64 in the chart, including `tests/cleanup_pvc_test.yaml` (hook annotations and deadline, selector and exit path, digest pin and registry prefix, securityContext, Role rules, RoleBinding). `helm template` renders the Job, ServiceAccount, Role and RoleBinding next to the existing objects. ### Downstream repositories A new hook template and its tests only: no values, schema, README or defaults change. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [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 fix(opensearch): reclaim the data PVCs of a removed application with a post-delete hook ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Automatically cleans up orphaned OpenSearch persistent volume claims after a release is deleted. * Cleanup waits for associated pods to stop and uses restricted permissions and hardened security settings. * Supports optional container registries with pinned cleanup images. * **Tests** * Added coverage for cleanup behavior, hook configuration, security settings, registry overrides, and required access permissions. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Timofei Larkin
|
8f19910988 |
chore(platform)!: remove the HTTPCache application (#4200)
## What this PR does Removes the HTTPCache application. Telemetry says it is the least installed app we ship: 3 instances in June, July and August 2026, 4 so far in September, out of 1162-1418 clusters reporting per month. That is not zero, so I am not going to claim nobody uses it. The maintenance side is what does not hold up. `make update` in this package is broken. All eight sed lines in the `update` target patch `images/nginx/Dockerfile`, but the image lives in `images/nginx-cache/Dockerfile`, so the target fails on the first line. It is the only package under `packages/apps/` pointing at a Dockerfile that does not exist. The pins show what that cost us: ``` nginx 1.25.3 -> 1.31.5 ngx_cache_purge 2.5.3 -> 3.0.2 nginx-module-vts 0.2.2 -> 0.2.7 ip2proxy-c 4.1.2 -> 4.3.0 IP2Location-C 8.6.1 -> 8.7.0 ip2location-nginx 8.6.0 -> 8.7.0 ip2proxy-nginx 8.1.1 -> 8.2.0 51Degrees 3.2.21.1 -> 3.2.21.1 (frozen upstream) ``` So we ship an old nginx with third party C modules compiled into it, and nothing tests it: no chainsaw suite, no `helm unittest` target. I don't think we should keep carrying that for 4 instances. Removed: the chart in `packages/apps/http-cache`, its ResourceDefinition in `packages/system/http-cache-rd`, the values type in `api/apps/v1alpha1/httpcache`, the `cozystack.http-cache-application` PackageSource, its line in the `naas` bundle, the `httpcaches` entry in `cozy:tenant:admin:base`, the dashboard icon, and the entry in the root `Makefile` build list. `cozy:tenant:admin:base` enumerates the `apps.cozystack.io` resources a tenant owner may write, so `httpcaches` comes out of it with the rest of the application. A cluster that freezes gets the grant back from the migration instead, which keeps the chart's list tracking the catalog and leaves no entry behind for a later sweep to read as dead config. ### Existing installations A cluster that still runs an HTTPCache keeps it, served from a frozen source. A cluster that never created one keeps nothing. Without a migration an upgrade would break both. `templates/sources.yaml` is a bare glob with no keep annotation, so Helm prunes the http-cache PackageSource; the ArtifactGenerator is owned by it and goes too, and both ExternalArtifacts go with the generator. Those are the `chartRef` targets of `hr/http-cache-rd` and of every tenant `http-cache-<name>` release, so all of them would sit `Ready=False` for good and `HelmReleaseNotReady` would fire on every cluster with the naas bundle. Migration 58 runs as a pre-upgrade hook, before that prune, and branches on whether any instance exists. Its header comment carries the mechanism in full; this is the summary. If one does, it copies the packages `OCIRepository` and pins the copy to the manifest digest that object resolves at hook time, which is the release the instances are rendering from right then. `spec.ref.digest` is used when the installer pinned one; a tag or semver ref goes through `status.artifact.revision`, whose digest half is the manifest digest. `status.artifact.digest` is the checksum of the downloaded tarball and cannot stand in, and a source with neither aborts the migration rather than publishing a source that points nowhere. Reading the digest live rather than baking one into the script makes the freeze a no-op by construction and asks no question about which release was the last to ship the chart. The copy takes the url, the pull secret, the interval and any verification block from the live object, so an air-gapped or mirrored registry keeps serving exactly what it already served. It is written with no owner references and no Helm metadata, and with the `platform.cozystack.io/no-delete` label the platform chart puts on the original, because it becomes the only chart source those instances have. The PackageSource is then repointed at the copy, annotated `helm.sh/resource-policy: keep` and stripped of its release metadata, so neither Helm nor a later release adopts or prunes it. Helm re-reads that annotation off the live object when it prunes, which is what makes disowning from inside a pre-upgrade hook work. Names stay as they are, deliberately. The ApplicationDefinition inside that artifact hardcodes its `chartRef`, so a renamed PackageSource would leave every release pointing at an artifact nobody produces. Because the names hold, `hr/http-cache-rd` and every tenant release keep the `chartRef` they already have and nothing goes not ready. What changes is where the chart comes from: a pinned digest that no later release moves. The migration also applies `cozy:tenant:admin:http-cache-frozen`, a ClusterRole labelled `rbac.cozystack.io/aggregate-to-tenant-admin`, granting the verbs `cozy:tenant:admin:base` used to carry on `httpcaches`. The hook applies it before the upgrade drops the rule from the chart, so an owner's access never lapses and nothing changes for them. Without it a frozen instance would be visible and undeletable by the person who owns it: the wildcard over `apps.cozystack.io` lives in `cozy:tenant:base`, which aggregates into `cozy:tenant` and not into `cozy:tenant:admin`, `cozy:tenant:view:base` is read only, and no role in the tenant chain carries a verb on `helm.toolkit.fluxcd.io`. If no instance exists, the migration removes the platform side instead: the Package first and waited on, because the operator reconciles off an informer cache and re-creates the release it owns while that Package is still there, then the release, its ApplicationDefinition and its Helm storage. Nothing is pinned or granted on a cluster that never used the application. Every path is fail-closed. A failed fleet scan, a failed read of the source OCIRepository, a source with no digest to freeze on, a failed patch or a failed delete stops the migration before it stamps the version. The migration is numbered 58, because main already carries a 57 from the SeaweedFS volume-size-limit change and its `targetVersion` is already 58. #4287 wants 58 as well, so whichever of the two lands second renumbers and bumps `targetVersion` with it. main is merged in rather than rebased, because the branch carries commits that are not mine to rewrite; the one conflict was `migrations.targetVersion`, resolved as 59, one past this branch's migration number. ### The procedure, written down `docs/deprecating-applications.md` is new, and is the point of doing this one carefully. It covers how to choose between migrating instances onto a successor and freezing them, the chain that makes a bare chart deletion strand every surviving instance, how the freeze reads its digest off the cluster's own source, what the migration owes a cluster on each branch, and where the tenant RBAC grant goes. `AGENTS.md` points at it so the next retirement starts there rather than rediscovering it. ### If you run HTTPCache Nothing changes on upgrade. Your instance keeps running and stays manageable through the Cozystack API and the dashboard, and `kubectl get httpcaches` keeps working. What changes is that it is frozen. The chart now comes from the packages artifact your cluster was already running, pinned by digest, and gets no further updates from us, including security updates to the nginx build. There is no replacement in the catalog. Delete the HTTPCache object when you no longer need it, the way you always did; the migration keeps the tenant permission that lets you. ### Verification `helm template` on `packages/core/platform` before and after, for `isp-full`, `isp-full-generic` and `isp-hosted`. In all three the differences are the `http-cache` Package and PackageSource going away and `migrations.targetVersion` moving from 57 to 58, which on a cluster below 58 also renders the migration hook Job and its RBAC. The migration suite `hack/migration-58-http-cache-freeze.bats` covers both branches, the order of every step and each fail-closed path. Freezing on the tarball checksum instead of the manifest digest, dropping the `no-delete` label, dropping the `managed-by` null, dropping the tenant grant, returning from the Package delete without waiting, freezing unconditionally and granting on a cluster with no instance each turn the matching test red. `packages/system/cozystack-basics/tests/clusterroles-tenant-admin-aggregation_test.yaml` pins the aggregation label the migration's ClusterRole reaches tenant admins through, so renaming it fails in the chart rather than silently on the clusters that need the grant. Green: `helm unittest` for `packages/core/platform` and `packages/system/cozystack-basics`, `make migrations-target-check`, `hack/cozystack-version-stamp.bats`, `go test ./internal/controller/cacert/...`, `go build ./...` in the `api/apps/v1alpha1` module, the console `ServicesTab` and `sidebar-icons` suites with `tsc --noEmit`, `make rd-presets-check`, and the bats suites that enumerate packages and image refs. ### Screenshots The catalog entry and its nginx sidebar icon are gone. Nothing new to show. ### Downstream repositories Two repos are affected. No follow-up is open yet, so no box is ticked. `cozystack/website` carries `content/en/docs/*/networking/http-cache.md` in eight doc versions and lists `http-cache` in the hardcoded `NETWORKING` list in its Makefile. The `next` page and the Makefile entry are what need to go, released versions stay as history. `cozystack/terraform-provider-cozystack` has a `cozystack_httpcache` resource and data source. The resource keeps working after this: it is hand written against its own terraform-plugin-framework model and imports nothing from here. What breaks, once that repo bumps its `api/apps/v1alpha1` pin past this release, is `internal/provider/httpcache_model_test.go`, the per kind schema guard, which is the only file over there importing this module. The resource and the guard should go together. The other repos have no reference to the package. - [ ] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [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 chore(http-cache)!: remove the HTTPCache application from the catalog. New instances can no longer be created from the catalog and the `HTTPCache` values type is gone from the `api/apps/v1alpha1` Go module. A platform migration keeps existing instances working: on a cluster that runs one, the application is frozen on the packages artifact that cluster was already rendering from, pinned by digest, and stays manageable through the Cozystack API and the dashboard, but receives no further updates, including security updates to the nginx build. On a cluster with no instance the application is removed outright. There is no replacement in the catalog; delete the HTTPCache object when you no longer need it. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Removed** * Removed the HTTP Cache application and its deployment, configuration, dashboard metadata, and packaging from the platform. * The HTTP Cache application is no longer included in the NaaS bundle. * HTTP Cache resources and related API definitions are no longer available. * **Migration** * Existing HTTP Cache installations are frozen during migration; environments without instances are cleaned up. * Platform migration target advanced to version 58. * **Documentation** * Updated storage immutability documentation to reflect Valkey instead of HTTP Cache. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Yan Bondarenko
|
54ac8ed135 |
fix(opensearch): keep the PVC cleanup hook from holding the uninstall
kubectl delete waits for the claims to be gone unless told otherwise, and OpenSearch data pods stop slowly, so the post-delete hook could outlive helm-controller's 300s uninstall budget on a claim that would not release and leave the HelmRelease stuck uninstalling. The delete now runs with --wait=false, the shape the monitoring, etcd and seaweedfs hooks use: the deletion is requested and the claims go once their pods have stopped, which the log now says instead of "complete". The Job carries activeDeadlineSeconds 120 as the backstop for a kubectl that never returns and keeps only before-hook-creation, so a failed run and its log survive. The Role drops watch, which nothing needs once the wait is gone. Signed-off-by: Yan Bondarenko <202671653+yankawai@users.noreply.github.com> Assisted-by: LLM |
||
|
Yan Bondarenko
|
13be303fbb |
fix(opensearch): remove the data PVCs when the application is deleted
The OpenSearch operator creates its data volumes through a StatefulSet, so the PVCs carry no ownerReference, and the chart had no cleanup hook. Removing the application left every data volume Bound and counting against the tenant quota, unlike clickhouse, kafka and harbor, whose post-delete hooks reclaim the operator-owned storage. A post-delete hook in the same shape as the clickhouse one now deletes the PVCs labelled opster.io/opensearch-cluster=<release>, with a Role limited to persistentvolumeclaims and the kubectl image pinned by digest. Signed-off-by: Yan Bondarenko <202671653+yankawai@users.noreply.github.com> |
||
|
Aleksei Sviridkin
|
bf26ce9744 |
fix(apps): let the cleanup hooks watch the objects they delete (#4135)
## What this PR does Fixes #4085. `kubectl delete` waits for the object to disappear unless `--wait=false` is given, and that wait is a watch on the object. The post-delete hook Roles of `harbor`, `clickhouse`, `mariadb` and `qdrant` grant `get`, `list` and `delete` only, so every uninstall logs ``` Failed to watch *unstructured.Unstructured: persistentvolumeclaims "..." is forbidden: User "system:serviceaccount:<ns>:<release>-cleanup" cannot watch resource "persistentvolumeclaims" ``` the wait is skipped, and the hook prints `PVC cleanup complete` while the claims are still terminating. Every collection-scoped rule behind a delete in these four hooks now also carries `watch`: `persistentvolumeclaims` in all four; `certificates`, `challenges` and the solver `ingresses` in harbor. The four named Secrets the mariadb hook removes keep `get` and `delete` only: they carry no finalizer, so the delete never reaches the watch, and a `resourceNames` rule does not authorize a collection watch anyway. A real wait needs a bound. kubectl's default is a week, and a claim held by a pod that will not stop, or a stuck detach, would hold the hook until helm-controller's 300s uninstall budget ran out and leave the HelmRelease stuck uninstalling. The PVC deletes now run with `--wait=true --timeout=120s`, so the expiry is reported by the script with the claims still terminating named, and each Job carries `activeDeadlineSeconds: 270` with `terminationGracePeriodSeconds: 10`, the numbers `packages/apps/tenant/templates/cleanup-job.yaml` documents against that budget, as the backstop for a kubectl that never returns. `--wait=false` was not an option: it is exactly the report-complete-while-terminating behaviour this PR removes. The chart unit tests are updated and pass: harbor 21/21, clickhouse 24/24, mariadb 111/111, qdrant 104/104. The behavioural suite `hack/app-cleanup-hooks_test.bats` passes 11/11 against the changed scripts. The second observation in #4085, the harbor hook pod occasionally starting after its ServiceAccount is gone, is a hook-weight question and is not touched here. ### Downstream repositories Hook templates and their tests only: no values, schema, README or defaults change. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [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 fix(apps): let the harbor, clickhouse, mariadb and qdrant cleanup hooks wait for the deleted objects, with the wait bounded so a stuck claim cannot hang the uninstall ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Enhancements** - Cleanup operations for ClickHouse, Harbor, MariaDB, and Qdrant now wait for persistent volume claim deletion to complete, with a 120-second timeout. - Cleanup jobs are limited to 270 seconds and allow 10 seconds for graceful termination. - Cleanup permissions now support monitoring relevant resources, including persistent volume claims and, where applicable, certificates, challenges, and ingresses. - Named Secret access is limited to retrieval and deletion. - **Tests** - Updated chart validation to verify cleanup timing, deletion behavior, permissions, and restrictions on unrelated resources. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Timofei Larkin
|
c398e0e116 |
Merge branch 'main' into chore/drop-http-cache
main took migration 57 and moved migrations.targetVersion to 58 while this branch was open, so that line is the one conflict: 57 at the merge base, 58 on main, 59 here. Resolved as 59, which is what this branch's migration 58 requires, since run-migrations.sh loops up to targetVersion - 1. Merged rather than rebased because the branch carries commits that are not mine to rewrite. Signed-off-by: Timofei Larkin <lllamnyp@gmail.com> |
||
|
Yan Bondarenko
|
334f6adac6 |
fix(nats): merge config.merge.accounts with the generated accounts map
With users set the chart emitted accounts.A.users from the generated passwords and then appended .Values.config.merge verbatim. A tenant that also put an accounts block into config.merge, which is the only place to enable JetStream for the generated account, got the key twice in the rendered values and the release failed at post-render. The generated users are now merged into config.merge before it is rendered: the generated list wins for account A, everything else the tenant put under config.merge is kept, and a config.merge without users passes through unchanged. Signed-off-by: Yan Bondarenko <202671653+yankawai@users.noreply.github.com> |
||
|
Yan Bondarenko
|
9ee2a2375c |
fix(foundationdb): let tenant roles read the connection ConfigMap
The operator stores the cluster file in the <release>-config ConfigMap, but the dashboard resource map did not list it and no RBAC granted tenant subjects access to it, so a tenant could not retrieve the connection string for its own FoundationDB at all. Add the ConfigMap to the dashboard resource map and create a Role/RoleBinding scoped to exactly that ConfigMap (get/list/watch) for the tenant use-level subjects. Add helm unittest coverage. Signed-off-by: Yan Bondarenko <202671653+yankawai@users.noreply.github.com> |
||
|
Aleksei Sviridkin
|
bd3892253b |
fix(clickhouse): keep the Keeper name inside the StatefulSet volume-name limit (#4133)
## What this PR does Fixes #4083. clickhouse-operator names the Keeper config volume `chk-<chk>-deploy-confd-<cluster>-<shard>-<replica>`. With the CHK called `<release>-keeper` and `Release.Name` already prefixed with `clickhouse-`, an application name longer than 15 characters pushes that volume name past 63 characters. The StatefulSet is rejected on every reconcile, no Keeper pod appears, and the CHK still reports `Completed`; the only visible signal is the Keeper `WorkloadMonitor` staying at `operational: false`. `templates/_keeper.tpl` now computes the CHK name: `<release>-keeper` as before when it fits, otherwise the release name is truncated and a six-character hash of the release name is appended so two long names cannot collide. The budget accounts for the width of the replica index. `chkeeper.yaml` and the zookeeper `nodes` list in `clickhouse.yaml` both use the helper, so the hosts ClickHouse connects to always match the CHK the operator creates. The pod label, the `VMPodScrape` selector and the `WorkloadMonitor` stay on `<release>-keeper`. Names up to 15 characters render byte-for-byte as before. A release with a longer name never had a Keeper pod; on upgrade its pod-less CHK is replaced by the shortened one and the Keeper comes up. `helm unittest`: 30/30 in the chart, including the new `tests/keeper_name_test.yaml` (short name unchanged, the exact 63-character boundary, one character past it, a very long name, a two-digit replica index, no trailing dash after truncation). ### Downstream repositories Templates and tests only: no values, schema, README, defaults, `ApplicationDefinition` or tenant-facing Secret/Service names change. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [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 fix(clickhouse): shorten the Keeper name for application names longer than 15 characters so the Keeper StatefulSet can be created ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Fixed ClickHouse Keeper resource naming for long release names so generated names remain within Kubernetes’ 63-character limit. - Updated ClickHouse Keeper hostnames to consistently match the generated resource name, including when names are shortened. - Preserved identifying labels and monitoring selectors while applying the corrected Keeper naming behavior. - **Tests** - Added coverage for standard, boundary-length, and truncated Keeper names, including replica-specific naming scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
355630a5e7 |
fix(kubernetes): pin KubeVirt CSI sidecars to SIG Storage releases (#3934)
## What this PR does Replaces five mutable OpenShift `latest` image references (four unique sidecar images) in the KubeVirt CSI controller and tenant-node DaemonSet with versioned Kubernetes SIG Storage releases. This addresses the affected references from #3578 without claiming the other references in that issue. The selected controller versions match the sidecars already used by Cozystack's LINSTOR CSI deployment. Render tests pin every changed container name and image so a mutable tag cannot return silently. Validation: - `helm unittest packages/apps/kubernetes`: 247/247 passed. - `helm unittest packages/system/kubevirt-csi-node`: 6/6 passed. - Live MSK-prod guest-cluster canaries on 2026-08-20 and 2026-08-21 reproduced the failure with the stock chart: the `latest` sidecars exited on the old CPU with the x86-64-v3 glibc error and the controller stayed in CrashLoopBackOff. A disposable guest cluster with the pinned images reached a 7/7 Ready controller on 2026-08-19 and was removed after the check. ### Screenshots Not a UI change. ### Downstream repositories Walked the trigger map against the four changed template/test files. No package, schema, API, default, CRD, tooling contract, or downstream hardcoded value changes. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [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: ### Release note ```release-note fix(kubernetes): pin KubeVirt CSI sidecars to versioned SIG Storage images that run on pre-x86-64-v3 CPUs ``` |
||
|
Yan Bondarenko
|
f886f26ad4 |
fix(apps): bound the wait the cleanup hooks now perform
With watch granted, kubectl delete waits for the claims to be gone, and its default for that wait is a week. A claim held by a pod that will not stop, or a stuck detach, would hold the post-delete hook until helm-controller's 300s uninstall budget ran out and leave the HelmRelease stuck uninstalling. The PVC deletes in the clickhouse, harbor, mariadb and qdrant hooks now wait 120s at most and report the claims still terminating by name, and each Job carries activeDeadlineSeconds 270 with a 10s termination grace period, the numbers packages/apps/tenant/templates/cleanup-job.yaml documents against that budget, so a kubectl that never returns cannot hold the hook either. The mariadb Role no longer grants watch on the named Secrets: they carry no finalizer, so the delete never reaches the watch, and a resourceNames rule does not authorize a collection watch anyway. Signed-off-by: Yan Bondarenko <202671653+yankawai@users.noreply.github.com> Assisted-by: LLM |
||
|
IvanHunters
|
8a6fe02ee4 |
fix(bucket): stop the ACME cleanup hook from talking to localhost, and gate the class (#4289)
## What this PR does Fixes #4288, and closes the class behind it and #4276. `packages/apps/bucket/templates/hooks/cleanup-acme.yaml` passed `--request-timeout=30s` on all four of its kubectl calls. A non-zero value there makes kubectl skip its in-cluster credentials: client-go consults them only while the assembled config still equals the default, and the flag lands in that config, so every call went to kubectl's built-in default cluster and was refused. This package failed more quietly than kafka. Each call is guarded by `|| echo "WARNING: ..."` and the script sets no `-e`, so the hook exited 0 and printed `ACME cleanup complete.`, and `hook-succeeded` then deleted the Job holding those warnings. The ACME solver objects it exists to reap were left behind, with the only record removed moments after it was written. Its own suite matched the rendered command including the flag, so the broken form was pinned in place. The gate is the point of the change rather than a bonus. Two packages carried this, neither was visible in a rendered manifest, and both passed their unit suites. Nothing stops the next hook from reaching for the same flag, because it reads as ordinary defensive practice. `hack/check-hook-request-timeout.bats` fails any non-vendored template whose kubectl call carries a non-zero `--request-timeout`. The coarse part is stated in the file rather than left to be discovered: a template that names `KUBECONFIG` is skipped, because an explicit kubeconfig is not served by the in-cluster branch and the flag is harmless there. That is a per-file judgement, not per-command. It is what lets `apps/kubernetes` keep the flag on two calls that pass their own kubeconfig and stay green, and it is the first thing to tighten if a file ever mixes both styles. Checked in both directions rather than only on the clean tree: the gate passes as committed, fails if the flag is put back on any bucket call, fails if it is put back on the kafka call, and leaves the two `apps/kubernetes` files alone. It also carries a vacuity guard, because a collapsed glob would otherwise report success for having examined nothing. Bounding the work is still available where it belongs: `--timeout` for a watch, `activeDeadlineSeconds` for the Job, which this hook already had at 120s. **Depends on #4280** and is branched off it, so the gate is green here. #4280 removes the same flag from the kafka hook; merged before it, this check would be red on `main`. ### Screenshots Not a UI change. ### Downstream repositories Walked the trigger map against the diff. A hook's kubectl flags and a new `hack/*.bats` check; no values key, schema, API, default or naming contract moves, and nothing under `hack/` is moved or renamed, nor does any make target change what it does. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [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 fix(bucket): the ACME cleanup hook now reaches the apiserver instead of reporting success while doing nothing ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved ACME cleanup reliability by preventing cleanup commands from targeting an unavailable local Kubernetes endpoint. * Improved Kafka uninstall cleanup reliability by ensuring the pre-delete hook retains access to required credentials throughout execution. * Preserved existing execution time limits and warning-based failure handling for cleanup operations. * **Tests** * Added coverage to prevent regressions in Kubernetes cleanup command configuration and hook resource lifecycle behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
IvanHunters
|
3a43bcf391 |
docs(kafka): say why the hook's Job and its credentials are reaped differently
The policies are right and nothing here changes them. What is missing is the reason, and this file has now been changed in both directions inside a week by people reasoning from the annotations alone, so the reason is the part worth keeping. The Job carries no hook-succeeded because the script exits 0 on the degraded path as well as the clean one, and its log is the only record that a finalizer outlived the wait; ttlSecondsAfterFinished reaps it on a timescale a human can act within. The ServiceAccount, Role and RoleBinding have no such clock, and before-hook-creation fires on a next activation that after the last uninstall never comes, so without a terminal policy they sit in the tenant namespace for the life of the cluster still granting delete over every KafkaTopic in it. Reaping them cannot strand a running pod, which is the objection the symmetrical shape was written against: helm applies hook-succeeded only once the hook is terminal, and activeDeadlineSeconds ends the Job inside the uninstall hook timeout. hook-failed would decide nothing, because a failing hook already reaps the earlier successful ones under hook-succeeded. Assisted-by: LLM Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
coderabbitai[bot]
|
a295e29b8a | fix(kafka): clean up pre-delete hook ServiceAccount and RBAC on success | ||
|
Andrey Kolkov
|
6e18ceef53 |
fix(kafka): fail closed when the migration hook cannot list Kafka CRs
The "free the kafka pool" gate counted other ZooKeeper clusters through a `kubectl ... | awk` pipeline under `set -eu` without `pipefail`, its kubectl error dropped by `2>/dev/null`. A failed read then counted zero and kept the shared pool, stranding any co-namespaced cluster still waiting for the name. Separate the read status from the count, mirroring the drain read below, and cover it with a bats case. Assisted-By: LLM Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Alexey Artamonov
|
cd9f4d6a27 |
feat(kafka): auto-migrate every ZooKeeper Kafka in a namespace to KRaft
Extends the ZooKeeper->KRaft migration hook so that N Kafka clusters sharing one namespace all reach KRaft automatically, with no manual steps. This closes the last gap in the migration: previously only one cluster per namespace could migrate in place (the broker pool must be named exactly "kafka" to adopt the existing <cluster>-kafka-N brokers, and that name is namespace-unique), so a second co-namespaced ZooKeeper cluster hit the render guard and its HelmRelease never rendered again -- stuck on ZooKeeper with no way forward once a future Strimzi drops the migration. The clusters now serialize on the "kafka" name automatically. Whichever upgrade wins the name (claimed atomically via kubectl create) migrates in place; a cluster that loses fails its hook fast and its next reconcile retries. Once a migrated cluster sees other ZooKeeper clusters still waiting, it frees the name: it stands up its own release-hashed "b-<hash>" broker pool (node IDs pinned clear of the existing broker and controller ranges), enables Cruise Control, drains every partition off the "kafka"-pool brokers with a remove-brokers KafkaRebalance, deletes the drained pool, and removes Cruise Control again. The next waiting cluster's retry adopts the freed name; the last one to migrate keeps "kafka". Everything happens inside the one namespace -- the hook's Role is namespaced and it never reads or touches another. Robustness details learned from live runs: - The drain rebalance sets skipHardGoalCheck: true. A mandatory broker evacuation must not be blocked by Cruise Control's capacity hard goals; under a small resources preset the capacity model can briefly over-estimate CPU from noisy samples and reject the proposal with CpuCapacityGoal even when real CPU is near idle. - The drain wait refreshes the proposal (strimzi.io/rebalance=refresh) on any NotReady and keeps polling, rather than treating it as terminal: a just-enabled Cruise Control is unreachable for a minute or two, and its model settles as metric windows age out. This also makes the drain resumable -- a re-run refreshes a stale rebalance instead of reading its old failure forever. - Cruise Control is enabled with explicit resources (it is a JVM and a tenant LimitRange would OOM it at 128Mi), transiently -- kafka.yaml never sets spec.cruiseControl, so Helm's server-side apply does not fight the hook patching it on and off. Every phase is idempotent so a Job killed mid-drain (the drain scales with data size and can run for hours) resumes on retry; the Job deadline and the release's helm-install-timeout are raised accordingly. Verified on a live cluster across five runs of two co-namespaced ZooKeeper Kafkas migrating to KRaft: both reach KRaft automatically, the first drains onto its b-<hash> pool and frees the name, the second adopts "kafka" in place (broker node IDs and PVCs unchanged), and each cluster's data is byte-for-byte identical before and after. helm-unittest 87/87 and hack/kafka-migration-hook.bats 14/14 (the real hook script against a stub kubectl) cover the freeing decision, the atomic name claim, the drain's refresh/skip-hard-goal handling, and the "delete the pool only after the rebalance is Ready" guard. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Alexey Artamonov
|
cb7578bb31 |
fix(kafka): data-safe, Helm-clean ZooKeeper->KRaft migration
Two live-reproduced blockers in the in-place ZK->KRaft migration are
fixed, plus the concurrency guard behind them.
1. Data loss (node IDs). The migration created the broker and controller
KafkaNodePools together without pinning node IDs. On Strimzi 0.45 the
controller pool deterministically took the existing broker IDs
[0..N-1], so the brokers were recreated empty at [N..] and their data
PVCs were orphaned (reproduced 2/2 on dev9: 20000 messages -> 0,
partitions leaderless, migration wedged in KRaftMigration). Naming the
broker pool "kafka" is necessary but NOT sufficient: Strimzi adopts by
node id, not pool name. The controller pool now pins
strimzi.io/next-node-ids above the broker range, so the broker pool
keeps [0..N-1] and adopts the existing <cluster>-kafka-N brokers and
their data in place.
2. Release stuck "failed" (Helm lifecycle). The pre-upgrade hook creates
the pools, and the chart also declared them as regular manifests, so
Helm's main apply tried to reconcile a resource that exists in the
cluster but was absent from the previous (ZooKeeper) release revision
and aborted with "original object KafkaNodePool ... not found",
leaving the HelmRelease failed even though the cluster migrated fine
(reproduced 3/3 under server-side apply, the production
helm-controller path). This is unavoidable while the pools are Helm
main-manifest resources across the ZK->KRaft boundary, so the pools
are now managed entirely by the migration Job (a pre-install,
pre-upgrade hook) on every install and upgrade, and removed from the
regular manifests. The Job ensures both pools (creating or updating
them idempotently), then drives the migration state machine.
templates/kafkanodepools.yaml and the deployed-version gate ConfigMap
(templates/version.yaml) are removed; the broker pool name is resolved
per path by kafka.brokerPoolName ("b-<hash>" fresh, "kafka"
migrating), and the render-time name-length guard moves to the Kafka
CR template.
3. Concurrency (one migration per namespace). The collision guard read
the "kafka" pool owner and then applied, a non-atomic read-then-write
two concurrent per-namespace migrations could both pass. When the pool
is absent the hook now uses "kubectl create" as the atomic arbiter:
exactly one concurrent creator wins, the loser re-reads the owner and
aborts.
Verified live on dev9 under server-side apply: a fresh install comes up
KRaft with pools at [0,1,2]/[3,4,5] and the release is "deployed"; an
in-place migration of a 20000-message ZooKeeper cluster ends "deployed",
adopts the broker PVCs in place (same UIDs), and the KRaft data is
byte-for-byte identical to the pre-migration ZooKeeper data (matching
sha256 over every partition:offset:key:value). helm-unittest 86/86 and
hack/kafka-migration-hook.bats 12/12 (the real hook script against a stub
kubectl) cover the resolution branches, the fail-closed reads, the atomic
collision guard and the finalize guard.
Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
|
||
|
Alexey Artamonov
|
053ef16976 |
feat(kafka): allow multiple KRaft Kafka clusters per namespace
Fresh KRaft clusters no longer need to be alone in a namespace. Their node pools get short, release-hashed names — "b-<hash>" for brokers, "c-<hash>" for controllers — instead of the fixed "kafka"/"controller". Each release hashes differently, so the names are unique within the namespace (KafkaNodePool object names are namespace-unique) and any number of Kafka instances coexist. The hash keeps the pool name a fixed length: Strimzi derives every pod/PVC name as <cluster>-<pool>-<id> and the cluster name is already the release name, so embedding the release name in the pool name too would double it into every pod hostname and blow the 63-character DNS label limit for long release names. A render-time guard fails with a clear message when even the hashed controller node name would still exceed 63 characters. The fixed "kafka" name is still required to adopt an existing ZooKeeper cluster's <cluster>-kafka-N brokers and data in place during migration, so kafka.brokerPoolName resolves to "kafka" for a migrating/migrated cluster and "b-<hash>" for a fresh one. A second ZooKeeper->KRaft migration into a namespace that already holds a "kafka" pool owned by another cluster fails closed, both at render time and in the pre-upgrade hook (whose owner probe uses --ignore-not-found + an explicit exit-code check, so a transient kubectl error aborts rather than silently waiving the collision guard). This replaces the previous blanket "one Kafka per namespace" guard, which over-restricted the fresh case. The pool names are resolved from a live Helm lookup; that is safe because Helm propagates any non-NotFound read error and fails the whole render (pkg/engine/lookup_func.go), so a transient apiserver/RBAC miss retries rather than silently flipping a live cluster's pool name — an empty lookup means genuine absence or a never-applied client-side render. The migration hook and workload monitors follow the same naming. Tests: helm-unittest fixtures (via kubernetesProvider) cover every resolution branch and the fail-closed guards; hack/kafka-migration-hook.bats drives the migration Job's real shell script against a stub kubectl across the fresh / read-error / already-KRaft / owner-collision / happy-path / stuck / retry cases. Verified on a live cluster: in-place ZooKeeper->KRaft migration preserved all data and adopted the original broker PVCs, and two fresh clusters coexist with distinct pools, pods and PVCs. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Alexey Artamonov
|
1a68157564 |
fix(kafka): harden migration hook and isolate the migration e2e
Addresses the second review round on the ZK->KRaft migration. Migration hook: - Fully qualify the Strimzi resources as kafkas.kafka.strimzi.io / kafkanodepools.kafka.strimzi.io. The bare plural "kafkas" also exists in this chart's apps.cozystack.io group, so a bare name resolves by per-cluster discovery priority and can hit the tenant kind — which the hook Role does not grant, failing the pre-upgrade hook deterministically. - Classify NotFound structurally via `kubectl get --ignore-not-found` instead of substring-matching stderr, so an unrelated "... not found" error can no longer be mistaken for a fresh install and permanently skip a live ZooKeeper cluster. This also drops the jq dependency. - Treat a KRaft-native / already-migrated CR (annotated strimzi.io/kraft=enabled) as done, never driving it backwards into kraft=migration. - Keep only `before-hook-creation` on the Job's hook-delete-policy so a failed attempt's pod logs survive for a post-mortem until the next run. - Route the kubectl image through cozy-lib.image (cluster registry mirror) and pin it by digest (clastix/kubectl:v1.32@sha256:...), matching the other cozystack hooks, instead of an unpinned docker.io tag. Controller quorum: - Fix the controller node-pool replica count at creation instead of deriving it from kafka.replicas on every reconcile. Strimzi 0.45 cannot scale a controller pool, so a day-2 edit of kafka.replicas (e.g. 1 -> 3) must not flip the quorum; the helper now keeps the existing "controller" pool's replica count when it is present. Docs: - Document that migration to KRaft is one-way: neither a version-field downgrade nor a pre-KRaft chart rollback is supported once migrated (Strimzi has no KRaft->ZooKeeper rollback after completion). E2E: - Run the ZK->KRaft chainsaw migration in its own namespace instead of the shared tenant-test. It raw-applies Strimzi resources plus a namespace-wide LimitRange and leaves deleteClaim:false PVCs, which polluted the sibling kafka suite. Raise the seed readiness timeout for CI headroom. Validated against a live cluster: migration adopts the broker PVC in place and reaches KRaft under a 128Mi LimitRange, in isolation. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |