mirror of
https://github.com/cozystack/cozystack.git
synced 2026-10-30 16:47:20 +09:00
11bac54f931682dd94de4081bb9e8a7eef60bef1
191
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
|
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> |
||
|
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
|
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
|
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
|
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 |
||
|
Yan Bondarenko
|
8ec76a3614 |
fix(kafka): make topics[].config optional
Signed-off-by: Yan Bondarenko <202671653+yankawai@users.noreply.github.com> |
||
|
Alexey Artamonov
|
710cdd18bb |
fix(kafka): address KRaft-migration review — broker pool adoption, hook safety, tests
Addresses the branch review on the ZK->KRaft migration path. CRITICAL — broker node pool adoption. Strimzi derives broker/PVC names as <cluster>-<pool>-<id>; a pre-node-pool ZooKeeper cluster has brokers <cluster>-kafka-N, so the adopting broker pool must be named exactly "kafka" or the operator provisions fresh empty brokers and orphans the tenant's data. A new kafka.brokerPoolName helper resolves the broker pool to "kafka" for an existing (migrating) cluster and keeps the release-scoped "<release>-broker" for fresh installs, so multiple Kafka instances still coexist in one namespace. The migration hook creates the pool as "kafka" and fails closed if a "kafka" pool already belongs to another cluster (node pool names are namespace-unique, so in-place migration of co-namespaced ZK clusters is unsupported). Wired through kafkanodepools.yaml, workloadmonitor.yaml and the hook. MAJOR — migration robustness. - Raise the HelmRelease timeout: the migration Job can run tens of minutes but the release timeout defaults to 10m, so helm-controller aborted and thrashed the pre-upgrade hook. Add release.cozystack.io/helm-install-timeout=45m on the kafka ApplicationDefinition, above the Job's activeDeadlineSeconds (2400s). - Idempotent migration: do not re-issue kraft=migration once the cluster is past the ZooKeeper phase (an invalid backward transition Strimzi rejects on retry). - Fail closed on the decision gates: distinguish a genuine NotFound (skip) from any other kubectl error (abort), and never treat an empty kafkaMetadataState on an existing CR as "migrated". MINOR — version downgrade documented as unsupported; integer (not lexical) compare in the version gate; deleteClaim/PVC-retention and the one-per-namespace migration limitation documented in the README; docs/storage-immutability.md updated for KRaft. Tests — helm-unittest coverage for the broker pool name, the hook guards and the idempotent/fail-closed logic; a bats check for the helm-install-timeout budget ordering; and a Chainsaw ZK->KRaft migration suite that seeds a ZooKeeper cluster, drives the migration, and asserts the original broker PVC survives (not recreated) and the cluster reaches KRaft. The migration sequence and the Chainsaw assertions were validated against a live cluster. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Matthieu
|
42db2ca141 |
feat(kafka): migrate from ZooKeeper to KRaft mode with automatic migration
Replace ZooKeeper-based Kafka with KRaft (Kafka Raft) metadata mode. Strimzi 0.45 is the last line that still supports ZooKeeper, so this prepares the transition before ZooKeeper support is removed upstream. Chart changes: - The Kafka CR always renders strimzi.io/kraft=enabled with node pools and no spec.zookeeper block. Storage, replicas and resources move to separate broker + controller KafkaNodePools (a homogeneous separate-pool layout works for both fresh installs and migrations; Strimzi 0.45 does not support migration into a combined pool). - Kafka version is now user-configurable via the standard files/versions.yaml pattern (v3.9 default, v3.8 available); appVersion bumped to 3.9.0. - WorkloadMonitors track the broker and controller pools (no zookeeper monitor); the dashboard RBAC grants the controller monitor and drops the zookeeper one. Migration of existing ZooKeeper clusters is handled by a Helm pre-upgrade Job (templates/migration-hook.yaml) gated by a <release>-kafka-deployed-version ConfigMap, following the established seaweedfs/etcd hook pattern. The Job creates the broker + controller pools with Helm ownership labels, drives the Strimzi ZK->KRaft state machine (node-pools=enabled, kraft=migration -> kraft=enabled once KRaftPostMigration is reached) and is skipped once the ConfigMap records version >= "1". Testing: - Helm unit tests cover the Kafka CR, node pools, migration hook, metrics, topics, version map, dashboard RBAC and workload monitors. - The e2e suite is ported to the current Chainsaw framework (hack/e2e-chainsaw/kafka): it asserts the KRaft topology (both node pools, kafkaMetadataState=KRaft), the absence of ZooKeeper, and bootstrap endpoints. Rebased onto current main: TLS listener support, entity-operator resource guards, x-cozystack-options schema annotations and the WorkloadMonitor replica-path fix are all preserved and reconciled with KRaft. Signed-off-by: Matthieu <matthieu@hidora.com> Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Andrei Kvapil
|
d7c0332d5e |
chore(dashboard,api): drop the HTTPCache catalog entry and values type
The dashboard no longer registers a brand icon for the kind, and the ServicesTab fixture moves to TCPBalancer, which the catalog still ships. An instance that survives the removal still renders, on the generic sidebar icon. api/apps/v1alpha1/httpcache goes too. types.go is generated by values-gen out of packages/apps/http-cache, which is gone, so the file could never be regenerated, and no Go file in this repository imports it. Surviving instances do not depend on it: the aggregated API serves the schema recorded in the in-cluster ApplicationDefinition. The terraform provider's cozystack_httpcache resource is hand-written and does not import it either; only its per-kind schema-guard test does, and that goes away with the resource. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com> |
||
|
Aleksei Sviridkin
|
cecb4b11d3 |
feat(opensearch)!: issue HTTP and Dashboards certificates from cert-manager (#2682)
## What this PR does The OpenSearch HTTP API (9200) and Dashboards (5601) already served TLS, from the operator's own private CA and with a SAN built from `cluster.local` while the credentials Secret advertised the release on the platform domain, so nothing a client was handed could verify. Both certificates now come from a per-release cert-manager CA in the tenant namespace, and the CA reaches tenants as a key-free trust anchor. - `tls.issuer` selects who issues the HTTP server certificate, `cert-manager` or `operator`, and is named for the issuer because TLS is served under both. Unset resolves from `external` (cert-manager for externally published services, the operator for cluster-internal); an explicit value always wins. - Renders a self-contained cert-manager chain in the tenant namespace: a self-signed Issuer, a CA Certificate, a CA Issuer, and two leaf Certificates, one for the HTTP API and one for Dashboards, each with the SANs of its own Service and the external hostname when `external: true`. Leaf keys are written as PKCS8, since the JVM loader rejects the PKCS1/SEC1 encoding cert-manager emits by default, and the CA subject is pinned so the operator's signature check accepts the chain. - The CA Secret is `<release>.http-ca`. The dot separates the chart-chosen suffix from release names, so no other release can produce that name; the two leaf Secrets stay hyphenated because none of the chart's suffixes is a tail of another. The operator's own transport CA remains at `<release>-ca` and is not touched. - Transport mTLS (9300) stays operator-managed (`transport.generate: true`, `perNode: true`). The security plugin requires it and the chart cannot turn it off. - `hack/e2e-chainsaw/opensearch/` runs the move on a cluster: it installs the app on the operator's HTTP listener, removes `tls.issuer` so it resolves from `external`, and asserts the cert-manager chain, the CR dropping `http.generate`, the admin certificate reissued under the chart CA, the StatefulSet rolling onto the three subPath mounts, the tenant anchor, and the certificate the endpoint actually presents to a client holding that anchor. ### Sysctl carrier and single-node rolls The E2E suite for this PR was red because the chart set `spec.general.setVMMaxMapCount: true`, which makes the operator prepend a privileged `init-sysctl` container to every OpenSearch pod. Tenant namespaces run at the cluster's default PodSecurity level, baseline on Talos, so admission refused the container and the StatefulSet never produced a pod (#4131). The carrier for the sysctl is now on main: #4152 added a DaemonSet to the `opensearch-operator` package that raises `vm.max_map_count` to a floor of 262144 on every node, from a namespace the platform labels privileged. This branch is rebased onto that main and sets the flag to an explicit `false`, with a template comment saying why and a unit test pinning it, so a release no longer asks the operator for a container admission refuses. The value stays with the DaemonSet until the node contract carries it (#4144); the DaemonSet's own template comment, which said the chart still asks, now says it does not. Three more things came out of the same pass. The chart asked the operator to drain data nodes before every rolling restart, and with a single data node the drain never finishes: at v2.8.0 `PreparePodForDelete` excludes the node from allocation and then waits for it to hold no shards, which the only data node can never satisfy, so a one-replica release could not roll for a version bump or for the certificate move this PR relies on. The chart now drains only when a second data node exists, pinned both ways in the unit tests. The chart maps an empty `storageClass` to `replicated` at one replica, and the PR lane runs LINSTOR without DRBD and creates only `local`, so the E2E fixture takes its class from the `storageClass` value the harness passes with `--set-string` from `COZY_E2E_STORAGE_CLASS`, the way the vminstance fixtures do; on the nightly QEMU lane that still resolves to `replicated`. And the external Dashboards Service selected labels no pod carries. `pkg/builders/dashboards.go` at v2.8.0 stamps `opensearch.cluster.dashboards: <cluster name>` on the Deployment, its selector and the pod template, and nothing else; the chart asked for `opster.io/opensearch-cluster` together with `app.kubernetes.io/component: dashboards`, so the LoadBalancer came up with no endpoints and no error to show for it. It selects the label the operator sets now. ### securityadmin cross-trust, still blocked on the DNS base Chart-managed HTTP TLS previously broke the operator's securityadmin reconciliation: the admin certificate was signed by the transport CA while the HTTP listener trusted the cert-manager CA, so the two never verified against each other and users and roles silently never applied. This chart puts one CA under both sides, handing the HTTP CA to the operator through `spec.security.tls.http.caSecret` so that it signs the admin certificate with the CA the listener trusts. That closes the CA mismatch. The other half, the job resolving a name the operator built from `cluster.local` on a cluster that does not use it, was closed separately and is on main, so the security configuration applies once this lands. No operator bump is required; the API exists in the pinned version. That path is version-gated upstream at OpenSearch 2.0.0: below it the operator falls back to the transport CA and the mismatch returns with no error anywhere. The chart therefore refuses chart-managed HTTP TLS on `version: v1` with an explicit render-time failure that names the mechanism and offers the alternatives, rather than letting it fail silently at runtime. ### Trust anchor for tenants The HTTP server's private key is not exposed to tenants. The chart renders a `TenantProjection` sentinel that names `<release>.http-ca` as its source; the CA-extraction controller publishes a `ca.crt`-only copy as `<release>.tenant-ca`, and the ResourceDefinition selects it with the engine-agnostic `internal.cozystack.io/tenant-ca` label. The sentinel is gated on the same condition as the chain, so a plaintext release renders no sentinel instead of one stuck at `SourceNotFound`. Tenants get what they need to verify the server and nothing more. ## Known limitations These are stated rather than left to be discovered: 1. **securityadmin needed both trust and reachability, and both are closed now.** This branch put one CA under the admin certificate and the HTTP listener. The other half was the operator resolving `<svc>.<ns>.svc.<DNS_BASE>` with `DNS_BASE` left at the vendored `cluster.local`, so the job waited on a name that never resolved whichever CA had signed; the platform now renders that value from `networking.clusterDomain` and it is on main, which is what unblocks this branch. 2. **The e2e suite reaches the move, not the chart bump that triggers it.** The suite above arrives at the chart-managed chain by removing `tls.issuer` from a running release, which leaves exactly the values an upgraded release arrives with. The one step it cannot run is the one where the previous chart version wrote the CR, because there is no second chart version to install from in this tree. Everything downstream of that write is exercised on a cluster. 3. **A reissue does not reach a running pod.** Neither service reloads its certificate, and cert-manager reissues on any change to `external`, the tenant host or the cluster domain, so an ordinary edit leaves the pods serving material that no longer matches their own names with no error, event or condition to show for it. Completing such a change is manual. So is removing the CA private key cert-manager leaves behind when TLS is turned off again, since the platform runs with `enableCertificateOwnerRef: false` and deleting a `Certificate` does not delete its `Secret`. `packages/apps/opensearch/README.md` carries both, with the commands. ### Downstream repositories `packages/apps/opensearch/values.schema.json` gains the `tls` object, so `cozystack/terraform-provider-cozystack` needs the field modelled for `cozystack_opensearch`; tracked in cozystack/terraform-provider-cozystack#41. ### Upgrade impact An existing `external: true` release with `tls.issuer` unset serves operator-issued HTTP TLS today and moves to chart-managed TLS on its first reconcile after this lands: the cluster rolls once and is re-anchored on a new per-release CA, so any client pinned to the operator's CA has to be repointed at the key-free `<release>.tenant-ca`. Setting `tls.issuer: operator` before the upgrade keeps that half as it is. Releases that are not external are untouched. Every existing release rolls once regardless of `tls.issuer` and `external`, because the pod template loses the operator's `init-sysctl` container; on a single-replica release that roll is the first one the operator can complete, since draining the only data node used to block every rolling restart. The published external-dns name changes on the same upgrade whatever `tls.issuer` says, because the Services are gated on `external` alone: they move from `<release>.<namespace>.<cluster-domain>`, an in-cluster name that never resolved outside the cluster, to `<release>.<tenant-host>` and `<release>-dashboards.<tenant-host>`. external-dns is configured upsert-only here, so the old record is not withdrawn and keeps pointing at the LoadBalancer until it is removed from the zone by hand. That cleanup is per zone rather than per release and nothing enumerates the records, so the README shows how to derive the list from the cluster. ### Release note ```release-note feat(opensearch)!: the OpenSearch HTTP API and Dashboards already served TLS, from the operator's own private CA and with a SAN built from cluster.local while the credentials Secret advertised the release on the platform domain; both certificates now come from a per-release cert-manager CA whose anchor the platform publishes into the tenant as <release>.tenant-ca. Any client that worked against the old endpoint was skipping verification, because no hostname it was handed could have matched that SAN. Existing external releases move from the operator's CA to the chart's on their first reconcile, and their published external-dns name moves from the in-cluster domain to <release>.<tenant-host> with the old record left in place; set tls.issuer: operator beforehand to opt out of the CA move. With dashboards.enabled the application name is limited to 41 characters, and to 32 when external is on as well; past either bound the chart fails the render and withholds every object in the release. A release already above a bound is not broken by the guard, it is told why it has not been upgrading: the Dashboards Service the operator builds from that name exceeds the 63-character limit for a DNS label, the operator returns that failure on every pass, and the reconcilers queued behind Dashboards, which are upgrade and restart and snapshot repository, have not run since the release was created. Set dashboards.enabled to false to drop the bound. The external Dashboards Service also selected labels the operator does not put on Dashboards pods, so it stood up with no endpoints; it selects opensearch.cluster.dashboards now. The drain of data nodes is now requested only with the data role on and more than two replicas, since at two the operator gives up on a full drain itself. The chart stops asking the operator for its privileged init-sysctl container, since the opensearch-operator package now sets vm.max_map_count from a DaemonSet, so every existing release rolls once on upgrade; single-replica releases no longer ask the operator to drain their only data node, which used to block every rolling restart ``` |
||
|
Arsolitt
|
85fa60a33f |
feat(opensearch)!: issue the HTTP API certificate from cert-manager
The HTTP listener and Dashboards ran on certificates the operator minted itself, so a client outside the cluster had no anchor it could obtain through the platform and no way to verify the endpoint it was handed. Issue the HTTP chain from cert-manager instead, and give the operator the same CA through spec.security.tls.http.caSecret so it signs the securityadmin admin certificate with it. Without that the admin certificate would keep the operator's own CA while the listener trusted cert-manager's, and securityadmin could no longer authenticate to apply users, roles and audit policy. tls.issuer names the authority rather than a switch, because the listener serves TLS under either value and only the signer changes. It is tri-state: an explicit value is obeyed, an absent one follows external, because a release nobody publishes has no third party to prove anything to. The cert-manager issuer needs OpenSearch 2.0.0 or newer, since the operator honours the HTTP CA only from that version and silently signs with its transport CA below it. An explicit choice under that floor fails the render and one resolved from external falls back to the operator, so asking for a verifiable certificate never quietly yields less than was asked, and turning on external access never breaks an upgrade. BREAKING CHANGE: an externally published release on OpenSearch 2.0.0 or newer serves the HTTP API from a cert-manager CA rather than the operator's own. A client pinned to the operator CA must move to the trust anchor the platform projects into the tenant. BREAKING CHANGE: the external-dns name of an externally published release moves from <release>.<namespace>.<cluster-domain> to <release>.<tenant-host>, and the Dashboards name with it. This half is gated on external alone and does not depend on the issuer. external-dns runs upsert-only here, so the old record stays in the zone until it is removed by hand. BREAKING CHANGE: with dashboards enabled the application name is limited to 41 characters, and to 32 when external access is also on. A release above either limit now fails to render rather than converging. Such a release was already stalled: the Dashboards Service built from its name exceeds the 63-character DNS label limit, and the operator returns that failure ahead of its upgrade, restart and snapshot-repository reconcilers, so it has not upgraded or rolled since it was created. Set dashboards.enabled to false to converge again, or recreate the application under a shorter name. Assisted-by: LLM Signed-off-by: Arsolitt <arsolitt@gmail.com> [lexfrei: reworded subject and body for the issuer field rename] Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
4b6b1dc2da |
fix(vm-instance): let complete resources replace the instance type (#4109)
## What this PR does Closes #2909. The matcher keyed off `instanceType`, the domain sizing keyed off `resources`, and neither side looked at the other, so the chart could render both. KubeVirt refuses that, and the two halves of its rule are different shapes: `validateCPU` conflicts on `domain.cpu.cores` and `domain.cpu.sockets` only when they are non-zero, while `validateMemory` conflicts on `domain.resources.requests.memory` whenever the key is present. `values.yaml` ships `instanceType: "u1.medium"`, so clearing the field in the dashboard just falls back to that default. Fill in cpu, sockets and memory after clearing it and you get a VM the webhook rejects, naming fields the form gives you no way to clear. Resources that size the VM completely now suppress the matcher and size it directly. `packages/apps/kubernetes-nodes` already does this for its worker VMs. Beside an instance type, sizing that is only partial stops the render, naming what sized the VM and what did not; with no instance type in play there is nothing to conflict with, so that half renders as it does today. Letting resources win unconditionally would silently drop the instance type for a VM that sets memory alone, which is a different bug in the same place. Completeness is decided from what `domainResources` emitted, never from which keys the user set, and all three fields go through that one reading. The two readings come apart in more than one place. `resources.cpu` on its own emits nothing, so it conflicts with nothing and goes on rendering a plain instancetype-sized VM. cpu and sockets pass through `int64`, which yields 0 for a quantity the schema accepts, so `cpu: 500m` beside a whole sockets and a memory would read as complete from the values, drop the matcher, and hand KubeVirt a `cores: 0` the user never asked for. And a `resources` block nulled outright leaves `.Values.resources` nil rather than the chart's `{}`, so a raw field read is a render-time nil pointer where the emitted object is merely empty. The existence check and the pre-upgrade update hook read the same effective value. Keying the `lookup` on the raw one would fail a resource-sized render over an instance type it never references, which is the same confusing error this fixes. Reading `.Values.instanceType` in the hook would see the leftover value, skip the removal patch, and leave `spec.instancetype.revisionName` on a VM Helm had just given explicit domain resources. ### What changes in the rendered output | `instanceType` | `resources` | Before | After | | --- | --- | --- | --- | | set | unset | matcher only | unchanged | | unset | cpu, sockets and memory | domain cpu and resources | unchanged | | set | cpu, sockets and memory | matcher plus domain sizing, refused by KubeVirt | domain sizing, no matcher | | set | any subset with memory, or cpu with sockets | matcher plus a conflicting domain field, refused by KubeVirt | render stops, message names both sides | | set | cpu alone, or sockets alone | matcher only | unchanged | | set | cpu and sockets that both truncate to zero | matcher plus `cores: 0` | unchanged | | unset | anything short of all three | already refused by the existing check | unchanged | The rows that stay unchanged are deliberate. `virtual-machine.domainResources` writes `domain.cpu` only when cpu and sockets arrive together, so cpu alone emits no override at all. A `cores: 0` beside a matcher is not a conflict either, because `validateCPU` guards on non-zero, so KubeVirt admits it and the instance type sizes the VM. Both shapes render today and go on rendering. `cpuModel` is left alone for a related reason: it writes `domain.cpu.model`, which KubeVirt flags only when the instance type declares a model of its own, and no instance type shipped in `packages/system/kubevirt-instancetypes` declares one. ### Why no working release breaks Every shape that now fails at render was already being rejected by the KubeVirt validating webhook at admission, so it could not have produced a running VM in the first place. The failure moves from an opaque admission error to a render error that names the fields. Nothing that worked stops working. The unchanged rows above are measured. Each non-failing shape in the table, plus firmware on and off, an explicitly nulled `resources` block and the three update-hook branches, was rendered against a mocked cluster on both sides and compared document by document. Across 14 value shapes and 78 rendered documents, 77 come out byte-identical. The one that differs is the row the table marks as changed, which is also what the harness was checked against first, so it is known to be able to fail. ### Live-VM impact This changes a rendered VM spec, so an upgrade can resize or restart a running VM. Adding a complete `resources` block to a VM currently sized by an instance type drops its matcher, and the pre-upgrade hook patches `spec.instancetype` off the live object and writes the domain resources in its place. Cozystack runs KubeVirt with `vmRolloutStrategy: LiveUpdate` (`packages/system/kubevirt/templates/kubevirt-cr.yaml:125`), so KubeVirt applies what it can to the running VMI and marks the rest `RestartRequired`; I have not measured which half a given resize lands in, so plan the upgrade as if it restarts. Dropping the matcher drops the whole instance type, sizing and everything else it carries. Of the 69 shipped cluster instancetypes, 36 declare `dedicatedCPUPlacement`, 36 declare `hugepages` and 26 declare both. 20 declare NUMA passthrough, 8 declare `realtime`. A VM moved from `rt1.medium` to explicit resources loses those characteristics along with the type. Before this change that combination was rejected outright, so the VM never existed; now it is admitted, and the field help says so. ### Tests `tests/instancetype_resources_test.yaml` covers matcher only, resources only, both complete, each failing partial shape, the shapes that emit nothing, both cpu and sockets truncating to zero, `cpuModel` beside a matcher and beside complete resources, a stale instance type with and without complete resources, an explicit `instanceType: null`, an explicitly nulled `resources` block, and every branch of the update hook's instancetype selector, in both directions plus the two no-op cases. 24 cases in this package, 2127 across `make helm-unit-tests`, all green. Every case pinning new behaviour was checked against a reverted template to confirm it goes red; the rest pin behaviour this change deliberately leaves alone. A ten-mutation ladder over the helper, `vm.yaml` and the update hook leaves no survivor. The memory branch took the most work. Swapping its emitted read for a raw `.Values.resources.memory` looks like an equivalent mutant, since `domainResources` emits `resources.requests.memory` under the same truthiness condition. It is not: a `resources` block nulled outright leaves `.Values.resources` nil, and the raw read dereferences it. The case pinning that shape kills the mutant. The update-hook cases assert the `kubectl patch` command the Job will run, which is as far as a render-level test reaches. They do not show that KubeVirt accepts the resulting object, that helm-controller converges on a live VM whose `spec.instancetype` also carries a `revisionName` written by virt-controller, or which half of a resize lands as a live update rather than `RestartRequired`. That needs a cluster. The `dig` calls survive the shapes that reach them. `values.schema.json` rejects a non-object `resources` before render; Helm coalescing turns a nulled one into a nil `.Values.resources`, which digs to an empty object; and for any other non-map intermediate, current Helm turns the underlying sprig type assertion into a render error rather than a panic. ### Interaction with the firmware block `firmware` (UEFI/BIOS selection, Secure Boot, persistent EFI NVRAM) landed in this chart in the meantime, and it does not touch the sizing question. `domainResources` does not read it, so it takes no part in the completeness decision. KubeVirt's instancetype spec carries neither firmware nor `domain.features`, so nothing an instance type declares can conflict with what the firmware block renders. The boot mode a VM inherits when the block is empty comes from the preference, and the preference is not what gets dropped when complete resources replace the matcher. Both firmware states are covered on each side by the byte-identity comparison above. `README.md`, `values.schema.json`, `api/apps/v1alpha1/vminstance/types.go` and the ApplicationDefinition all come out of `make generate` in the package, not out of a hand merge. Re-running it on the result produces no further diff, and the root `make generate` reports no drift on top of it. ### On the ticket's second proposal #2909 also asks whether the non-empty `u1.medium` default belongs in the chart at all. That requirement is met, and not by removing the default. The default was a problem because a schema-generated form gave no way to say "no instance type": clearing the field falls back to `u1.medium`. Filling cpu, sockets and memory now says it directly, so the obstacle the ticket names is gone while the default stays. Dropping the default on top of that would change the rendered spec of every release that never set the field, with nothing left to gain. ### Named improvements, not done here The failure message names the fields that did not size the VM, computed from what was emitted, so for `cpu: 4` with no `sockets` it names `resources.cpu` even though the user set it. The message states the pair rule that explains this, but naming the actually-missing field first would read better. The two tests that pin `domain.cpu.cores: 0` document the pre-existing `int64` truncation in prose rather than through a `FIXME` reference, so a later reader has to read the comment to tell a contract from a bug we chose not to fix. Both are wording; neither changes behaviour. `resources.cpu`, `sockets` and `memory` are checked for shape and not for value, so `memory: "0"` and `-1Gi` pass the schema and fail further down. Both forms fail before and after this change and only the place of the failure moves, so that belongs with [#4104](https://github.com/cozystack/cozystack/issues/4104) rather than here. The new template comments run to about 46 lines against about 28 lines of logic. All of it is why rather than what and all of it is accurate, but the helper's third paragraph restates the commit body, and two sentences would carry that weight. The seven-line block in `vm.yaml` is the one worth keeping: swapping that gate to the effective value survives the entire suite, so what the block records is an equivalence no test can pin, and trimming it would delete the only record of it. The `instanceType` description runs to about 370 characters and reaches the dashboard as tooltip help, through `values.schema.json` and the ApplicationDefinition. That is long for a tooltip. It is accurate as it stands, and rewriting accurate text for length is a fresh unmeasured claim in the surface with the most readers, so it is not done here: the place for the detail about what a dropped instance type takes with it is the chart README, leaving the field help carrying the precedence rule alone. Choosing a `cx1.*` type and then giving it complete resources drops what the type guaranteed along with the type, and nothing says so at runtime. That is a consequence of the precedence rule rather than a defect in it, and the field help states it. A runtime signal would need the chart to compare what the type declares against what `resources` sets, which is its own task. Defaulting `sockets` to 1 would let a user size a VM from cpu and memory alone and would remove the commonest way to land in the partial case. It is more than the ticket asked for and it changes the rendered spec of stored releases that set cpu and memory without sockets, so it needs its own call on compatibility. ### Not changed Two pre-existing quirks stay as they are, because both reach past the conflict this fixes. `resources` fields are read for truthiness, so an explicit `0` reads as unset, in `domainResources` and in the existing "Either instanceType or resources" check alike. And with no instance type in play, a fractional `resources.cpu` still truncates to `cores: 0` and renders a VM sized by nothing, with no error. The new guard covers only the half where a matcher is present. The tests pin both as they stand, which is what keeps working releases rendering. ### Screenshots No dashboard code is touched. The only user-visible surface is two field descriptions the dashboard renders as help text, regenerated into `values.schema.json` and the ApplicationDefinition. `instanceType` now reads "Ignored when `resources` sizes the VM, which needs whole-number cpu and sockets plus memory; the whole type is dropped then, so any dedicated CPU placement, hugepages, NUMA or realtime settings it carries go with it. While this field is set, a `resources` block that sizes only part of the VM is rejected outright instead of ignored, so complete it or clear it." `resources` states the same rule from its own side. ### Downstream repositories - [ ] 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: I walked the trigger map against the diff and no listed trigger fires: no package added, renamed or removed; no `values.schema.json` field added, renamed or removed; no default changed; nothing under `hack/` moved; `templates/service.yaml` and its `wholeIP` and `allowICMP` annotations untouched. Two repositories do carry prose this makes incomplete. I left every box empty rather than push follow-ups nobody agreed to, so a maintainer decides. `cozystack/website`: `content/en/docs/next/virtualization/resources.md` opens with "Each virtual machine has these two configuration settings" and lists only `instanceType` and `instanceProfile`. The page is hand-written, so the release-time bot that regenerates app reference pages from `README.md` will not reach it. `cozystack/terraform-provider-cozystack`: `internal/provider/vminstance_schema.go` describes `instance_type` as "Virtual machine instance type." Its `kubernetes_schema.go` and `kubernetes_nodes_schema.go` already spell out the equivalent precedence rule for their own charts. No schema field or default changed here, so its new-field test guard has nothing to catch. ### Release note ```release-note fix(vm-instance): a complete `resources` block (whole-number cpu and sockets, plus memory) now sizes the VM directly and suppresses the `instanceType` matcher, instead of rendering both and having KubeVirt refuse the VM. Beside an `instanceType`, a `resources` block that sizes the VM only in part now stops the render with a message naming both sides, where it previously produced a VM the admission webhook rejected. Note that suppressing the matcher drops the whole instance type, including any dedicated CPU placement, hugepages, NUMA or realtime settings it carries. A release whose values already carry both converts on the next platform upgrade rather than at a moment the operator picks: the pre-upgrade hook strips `spec.instancetype` off the live VM and writes the domain sizing in its place, so plan that upgrade as if the VM restarts. ``` |
||
|
scooby87
|
afc003c464 |
feat(postgres): database read-replica autoscaling via KEDA (#3954)
## What this PR does Adds automatic horizontal autoscaling of a PostgreSQL database's **read replicas** in response to load, implementing the merged design proposal [Database Horizontal Autoscaler](https://github.com/cozystack/community/tree/main/design-proposals/database-horizontal-autoscaling) (community#44). The mechanism is entirely stock — no bespoke operator, no new CRD: - **KEDA as an optional platform component** (`packages/system/keda`, wired into `core/platform` as a source + system-bundle entry). It claims the `external.metrics.k8s.io` APIService that nothing served before. - **`cozy-lib.keda.scaledObject` helper** (`packages/library/cozy-lib/templates/_keda.tpl`) renders a KEDA `ScaledObject`, fail-closed on missing/invalid arguments. - **`packages/apps/postgres` autoscaling** — an `autoscaling` values block (`enabled`, `minReplicas`, `maxReplicas`, `metric`, `target`, `maxReplicationLagSeconds`, `dryRun`, `transition`) validated by `values.schema.json`; under active autoscaling the chart renders a `ScaledObject` and seeds `spec.instances` to a constant floor (`max(replicas, effectiveMin)`), while the postgres release is forced to client-side (two-way) Helm apply so the seed is a merge no-op on upgrade and the KEDA-managed HPA — the runtime writer via the CNPG `Cluster` scale subresource — is not reverted. - **Single-value metric (§1)** — one PromQL query returns `Σ(read load over standbys) + target`, so a stock HPA computes `desired = ceil((Σ+target)/target) = 1 + ceil(Σ/target) = primary + desiredRead`, with no per-pod emission. - **The four static brakes (§5)** — quorum floor `max(minReplicas, maxSyncReplicas+1, 2)` (quorum wins over `maxReplicas`), scale-down pacing (`Pods/1/600s`), a replication-lag clamp folded into the query (freeze both directions while lag exceeds the threshold and the primary is writing, with `max_over_time` hysteresis), and dry-run (render paused). - **Two-phase enable/disable migration** via a `transition` sub-flag: phase 1 stages `spec.instances` to the live count and brings KEDA up with its HPA paused in both directions; flipping to phase 2 hands the runtime count to KEDA, and because the seed re-renders the same count the flip is a merge no-op with no dip. - **Observability** — a database-autoscaling Grafana dashboard and PrometheusRule alerts. **Live validation (dev cluster, CNPG 1.30.0).** The full path works end-to-end: the CNPG `Cluster` scale subresource serves `status.selector`, KEDA renders the HPA, the metric reaches it (`ScalingActive=ValidMetricFound`, `TARGETS 151/150`, `desiredReplicas=2 = 1 primary + 1 replica` — the §1 arithmetic), and the lag-clamp query returns the base value on live VictoriaMetrics. Two packaging fixes were needed and are included: KEDA must run on the cozystack `cozy.local` cluster domain (the upstream `cluster.local` default makes the metrics gRPC target NXDOMAIN and every HPA hangs on `FailedGetExternalMetric`), and the operator's client-go throttle is raised so a single-replica operator does not lose its leader lease under scale-subresource polling. Implements #3953. ### Downstream repositories Walked the trigger map in `docs/agents/contributing.md` against the diff. Two repositories are affected; both are follow-ups to open once this lands (and after its dependency #3951 merges and the values surface is final), left unticked here for a maintainer to confirm rather than claimed with a speculative link: - **cozystack/terraform-provider-cozystack** — this adds the nested `autoscaling` block to `packages/apps/postgres/values.schema.json`. The provider hand-models each app's fields (schema + model + expand/flatten), so it needs the new block to expose it in Terraform. - **cozystack/website** — KEDA is a new platform component (`guides/platform-stack`), and the postgres reference page gains the `autoscaling` params. The managed-app reference is regenerated from `README.md` by the release bot; the platform-stack component list is hand-maintained. - [ ] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: to open (new platform component + postgres autoscaling params) - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: to open (new `autoscaling` block in postgres values.schema.json) ### Depends on - #3951 — bump CloudNativePG to 1.30.0. The HPA can only drive the CNPG `Cluster` scale subresource on CNPG ≥ 1.28.4, where it serves `status.selector`; without it the HPA fails with `InvalidSelector`. This PR should not merge before #3951. ### Release note ```release-note feat(postgres): add optional horizontal autoscaling of read replicas via KEDA, configured through the new `autoscaling` values block; KEDA ships as a new optional platform component. Requires CloudNativePG >= 1.28.4 for the Cluster scale subresource selector. Note: every Postgres release now applies client-side (`release.cozystack.io/helm-server-side-apply: "false"`) so the autoscaler's live `/scale` count is not reverted by Helm; a kind-wide side effect is that chart-rendered fields on Postgres releases are no longer re-asserted by server-side-apply drift correction (driftDetection is unset), a documented trade-off with a per-instance apply strategy as follow-up. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added configurable PostgreSQL read-replica autoscaling with KEDA. * Added CPU and read-connection metrics, replica bounds, replication-lag safeguards, dry-run mode, and transition handling. * Added optional KEDA platform support, Grafana autoscaling dashboard, and Prometheus alerts. * **Documentation** * Clarified integer thresholds, prerequisites, configuration, and transition guidance. * **Bug Fixes** * Improved validation and actionable errors for unavailable KEDA or invalid settings. * **Tests** * Expanded coverage for autoscaling, observability, package selection, and validation. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
59731429a8 |
fix(vm-instance): let complete resources replace the instance type
KubeVirt refuses a VirtualMachine that carries an instancetype matcher and explicit domain sizing at once. validateCPU in pkg/instancetype/apply conflicts on domain.cpu.cores and domain.cpu.sockets when they are non-zero, and validateMemory conflicts on domain.resources.requests.memory whenever the key is present. The chart rendered both, because the matcher keyed off instanceType while the domain sizing keyed off resources, and neither side looked at the other. values.yaml ships instanceType "u1.medium", so clearing that field in the dashboard only falls back to the default. A user who then fills in cpu, sockets and memory gets a VM the webhook rejects, with an error naming fields the form gives them no way to clear. Resources that size the VM completely now replace the matcher, the same way the kubernetes-nodes chart sizes its worker VMs. Beside an instance type, sizing that is only partial stops the render, because there is then no safe reading: dropping the matcher would size the VM from fields the user never finished, and keeping it either loses to the webhook or silently discards what was typed. With no instance type in play there is nothing to conflict with, so that half is left as it renders today. Completeness is decided from what domainResources emitted, not from which keys the user set, and the two readings come apart. resources.cpu on its own emits nothing, so it conflicts with nothing and goes on rendering a plain instancetype-sized VM. cpu and sockets pass through int64, which yields 0 for a quantity the schema accepts, so a cpu of 500m beside a whole sockets and a memory would read as complete from the values, drop the matcher, and hand KubeVirt a cores of 0 the user never asked for. And a resources block nulled outright leaves .Values.resources nil, where a raw field read is a render-time nil pointer and the emitted object is merely empty. The existence check and the update hook read the same effective value. Keying the lookup on the raw one would fail a resource-sized render over an instance type it never references, which is the same confusing error this fixes. Reading .Values in the hook would see the leftover instanceType, skip the removal patch, and leave spec.instancetype.revisionName on a VM Helm had just given explicit domain resources. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Alexey Artamonov
|
678d22f209 |
docs(postgres): move the serverAddress namespace-pin note out of a template comment
Closes IvanHunters' MINOR on scaledobject.yaml:97. The API-owner-gate marker was a Go template comment, stripped at render, so the ownership question and the pin's residual surface were recorded where nobody reads them again. State the constraint on the serverAddress field doc (propagated into values.schema.json, README and the cozyrds openAPISchema) including the accepted residual: tenant-root is always in the allow list because the derived default points there, so a tenant can still aim the operator at a root Service as a blind in-cluster probe (no response body reaches the tenant), which the egress NetworkPolicy does not narrow. The API-owner sign-off is tracked on the PR, not in a stripped comment. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Alexey Artamonov
|
07f1daa22d |
docs(postgres): correct the replicas "restored on disable" claim
Closes IvanHunters' MINOR on values.yaml:58. The replicas description (propagated into values.schema.json, README and the cozyrds openAPISchema) said the count is "restored on disable". Under client-side apply it is not: the two-way merge writes instances only when the rendered value changes, so a cluster KEDA grew past replicas stays there after autoscaling.enabled: false with no autoscaler left to reduce it — the sibling dryRun doc already states the true rule, so the two contradicted each other. Reword both the @param and the @typedef to say the live count is not auto-restored and that the operator lowers replicas to reclaim the grown capacity; regenerated the derived artifacts. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Andrei Kvapil
|
54621bc046 |
feat(migration): tenant self-service VM import from VMware vSphere
Hidora's customers are leaving VMware now, and a tenant has to be able to import their own machines without a platform administrator. The chart approach could not express that: a tenant can neither create the Secret holding vCenter credentials nor name the VDDK image, so both had to come from somewhere they cannot reach. Two CRDs in a new `forklift.cozystack.io` group, split by lifecycle. A VMImportSource is a long-lived connection -- type, endpoint, credentials, readiness -- and a VMImportTask is the one-shot operation that names machines and produces VMDisks and VMInstances. The outputs carry no owner reference back to the task, so deleting a finished import leaves the machines alone. Konveyor Forklift does the transfer underneath; the controller renders its objects, mirrors its verdicts, and hands each finished volume into a VMDisk without copying it. What a live vCenter migration forced out, and no amount of source reading would have: - A guest copied verbatim keeps the drivers it had under VMware and no virtio, so on a virtio disk it does not fail the import -- it succeeds and then does not boot. Imported disks go on SATA. - VDDK connects straight to the ESXi host at the address vCenter advertises, which is routinely unreachable, and on the first cluster collided with the Service CIDR. `spec.hosts` redirects per host. - Firmware must be carried across or a UEFI guest imports as BIOS and never boots (stacked on #3002). - Deleting a task mid-transfer orphaned its DataVolume; a finalizer now removes the engine's volumes and leaves the outputs. The console gains a Migration section: connections are editable, and the VM field is a picker built from a machine list the controller publishes, since the aggregated API cannot reach the inventory and should not hold a source's credentials to try. Two general mechanisms fall out of that -- an option source may take an argument, and one named in a CRD annotation may reference a sibling field. Design: cozystack/community#62. Docs: cozystack/website#673. Signed-off-by: Andrei Kvapil <kvapss@gmail.com> |
||
|
Aleksei Sviridkin
|
d000d02b99 |
feat(rabbitmq): add TLS support for AMQPS and management HTTPS (#2683)
## What this PR does Adds TLS for AMQP and the management API, and hands the tenant a key-free trust anchor instead of a private key. - A chart-authored cert-manager chain issues the broker certificate. The chart names its CA Secret in a `TenantProjection` sentinel; the CA-extraction controller reads `ca.crt` from that Secret and writes a key-free projection, stamping `internal.cozystack.io/tenant-ca` on the projection it owns. That label is what the ResourceDefinition selects. Nothing labels the CA Secret itself, which holds the CA private key and is excluded by name. - The leaf `<release>-tls` carries the broker's private key and reaches no tenant. Neither the dashboard Role nor the ResourceDefinition grants it, and the ResourceDefinition excludes it by name as well, so a label mis-stamped onto it later still does not get through. - SAN coverage now lists every resolvable form explicitly, including the bare `*.<release>-nodes` wildcard that the operator uses for `RABBITMQ_NODENAME`. ## TLS is opt-in here, unlike the sibling charts `tls.enabled` defaults to `false` and is not inferred from `external`. That diverges from nats, qdrant and kafka, which infer it, and the divergence is deliberate for a reason specific to this operator. Setting `spec.tls.secretName` makes the cluster operator rewrite the `port` key in the `<release>-default-user` Secret from 5672 to 5671, on the mere fact of TLS and regardless of whether the plaintext listener is still open. That Secret is the Service Binding the chart hands the tenant. A client with a hardcoded `:5672` keeps working. A client that reads the endpoint from the binding, which is the documented way, gets moved to the TLS port and must then speak TLS and be given a CA to verify against, neither of which it was configured for. Inferring TLS from `external` would fire all of that on an ordinary upgrade, with no configuration change, against exactly the clients that followed the documentation. Whether the other engines should follow is an epic-level question rather than this PR's to answer; for nats it is raised in #3348. ## The plaintext ports are still published, and this PR says so There is no operator-supported way to publish the TLS ports alone. `spec.override.service.spec.ports` is a strategic merge keyed on `port` against defaults the operator rebuilds every reconcile, so it adds and mutates entries but never subtracts one, and a TLS-only port list there would read as a guarantee while changing nothing. The chart renders no service override at all and says why at the callsite. `tls.disableNonTLSListeners` is offered as an explicit opt-in, and it refuses to render rather than failing silently in three situations. - Together with `users` or `vhosts`. The flag forces messaging-topology-operator onto TLS regardless of its own configuration, after which it verifies the broker against the system certificate pool, where a per-release CA cannot be. Every topology object bound to the release would stop reconciling, silently: already-created users keep working while password rotation and new users simply never apply. There is no working route left for it, so the chart refuses instead. Managing those objects as hand-written CRs is not a way out, because the operator takes TLS from the `RabbitmqCluster` and not from whoever created the CR, so manual objects break identically and without the render-time refusal. `rabbitmqctl` or a definitions import are. - While topology objects are still live in the namespace. See the next section. - With TLS off, where no listener of either kind would remain. Erlang distribution (25672) and epmd (4369) stay plaintext under every setting; the flag does not touch them, so `disableNonTLSListeners: true` is not "TLS everywhere". A third route, closing AMQP at the broker through `spec.rabbitmq.additionalConfig` while leaving the operator on plaintext management, has been suggested and is plausible, but the operator's sources are not vendored here and it was not verified. It is recorded as an open question rather than promised. ## Closing the listeners takes two upgrades, and the chart enforces it Clearing `users` and `vhosts` and setting `tls.disableNonTLSListeners: true` in one edit is the natural thing to try, and it wedges the objects being removed. The values-level refusal cannot catch it, because by then both maps are empty. Helm applies every object in the target release before pruning the ones the release dropped, so the broker closes plaintext and loses the `rabbitmq.com/operator-connection-uri` annotation while the topology objects still hold `deletion.finalizers.<plural>.rabbitmq.com`. Their operator then has to reach a management API it can only get to over a certificate it verifies against the system trust pool. The delete never completes, and the upgrade reports success over objects left in `Terminating`, with the users still live in the broker and `kubectl delete namespace` on that tenant hanging. The chart now reads the namespace rather than the values. When the flag is being introduced and any topology object is still there, the render refuses and names them, gives the two-upgrade order and the `kubectl get` that confirms nothing bound to this release is left, and says how to unwedge a release that is already stuck. The check keys on the live `RabbitmqCluster` as well, so it applies only to the transition: once the listeners are closed, an object a tenant creates afterwards is theirs to clean up rather than something that stops the release rendering. Eleven unit cases cover the firing and the silent paths. The set is wider than the three kinds this chart creates. All twelve kinds `messaging-topology-operator` drives through its shared `TopologyReconciler` carry that finalizer and share one deletion path, so a `Queue` or a `Shovel` written by hand against the release wedges exactly the same way, and a runbook step that checks only users, vhosts and permissions says the namespace is clear when it is not. The list is derived in one place and read from both the lookup and the `kubectl get` the refusal prints, and a unit test holds it against the operator's own CRDs so a bump cannot introduce a kind the guard has never heard of. `SuperStream` is the one CRD left out: it runs on its own reconciler with no finalizer, and the objects it owns are already on the list. An object that binds through `connectionSecret` instead of naming the cluster stays outside the guard, which the comment says. ## Breaking for metrics scrapers Enabling TLS moves the metrics endpoint from 15692 to 15691. A ServiceMonitor selecting the port by the name `prometheus` silently stops matching and the instance's metrics disappear with no error anywhere. Nothing in this repository scrapes RabbitMQ, so only user-defined monitoring is affected, silently, which is why it is stated here. ## Known limitations 1. Turning TLS back off leaves the raw CA material behind. The anchor itself is collected: the projection carries an owner reference to the `TenantProjection` sentinel, and Helm deletes the sentinel along with the rest of the chain. What survives is `<release>-ca` and `<release>-tls`, because cert-manager runs with `enableCertificateOwnerRef: false` and does not delete the Secrets it issued, so the README says to remove them by hand. #3349 asks the same question of the label-driven leg of the contract, which this chart does not use. 2. No e2e coverage for the TLS path. The one-upgrade wedge above is the case that most deserves it: install with TLS and users, upgrade to no users plus the flag, and assert the `User` is actually gone rather than merely marked for deletion. A template test cannot reach that. On certificate renewal, the leaf carries `duration: 8760h` and `renewBefore: 720h`, so cert-manager rewrites the Secret around day 335. The cluster operator mounts it as a projected volume with no `subPath`, so the kubelet refreshes the files in place, and the operator does not watch a user-supplied TLS Secret, so nothing rolls the pods. The last hop, the broker picking the new file up, rests on Erlang's `ssl_pem_cache` re-reading by mtime and has only been read from sources rather than exercised against a live broker. The merged qdrant chart ships the same duration and `rotationPolicy`, so this is a platform-wide property rather than something this PR introduces. ### Downstream repositories `packages/apps/rabbitmq/values.schema.json` gains the `tls` object, so `cozystack/terraform-provider-cozystack` needs the field modelled for `cozystack_rabbitmq`; tracked in cozystack/terraform-provider-cozystack#42. ### Release note ```release-note feat(rabbitmq): opt-in TLS for AMQP and the management API, with the CA published to tenants as a key-free trust anchor. Enabling TLS moves the metrics port from 15692 to 15691 (ServiceMonitors selecting the port named `prometheus` must be updated) and moves the `port` value in the `<release>-default-user` binding Secret from 5672 to 5671. Plaintext AMQP and management ports remain published; `tls.disableNonTLSListeners` closes them but also drops in-cluster plaintext clients, and must be enabled in a separate upgrade after the topology objects are gone. ``` |
||
|
myasnikovdaniil
|
94a202bc5a |
test(e2e): run the merge-gating lanes on Talos containers (#4020)
Replaces QEMU substrate with Talos containers on both lanes that gate a merge. srv1-srv3 run as containers, so tenant worker sits at L2 instead of L3. Measured in CI on commits where both substrates ran the same tree: | | qemu | container | |---|---|---| | `kubernetes-latest` / `-previous` | 1143-2040s, red on every vgif-absent runner | 466-809s, green in both strata | | whole job | | 24-49 min faster | Almost all of it is in Chainsaw. Install takes about half an hour either way. Node-join soft-red gate goes with it. It was a prosthetic for nested virt: a worker that registers in minutes elsewhere could miss any deadline this test can afford, and #3513 established the discriminator is host `kvm_amd` `vgif`, not anything about the product. Remove the nesting level and there is nothing left to tolerate, so `hack/e2e-node-join-soft-red.sh`, the marker and the `soft_red` outputs are gone from all four workflows. ### What moves off the per-PR path DRBD, and with it `replicated` StorageClass, its `Immediate` binding mode, tenant StorageClass propagation (applies only to remotely-accessible classes), and cozystack talos node image with its extensions. None of it is lost - `nightly.yaml` and `e2e-tag.yaml` still run the suite on QEMU, so all four keep nightly and release-candidate coverage. Live migration is not on that list because nothing tests it today anyway. One more belongs on the list and was missing from it: the nocloud disk build. `build-talos` no longer runs `make -C packages/core/talos talos-nocloud`, so `images/talos/profiles/nocloud.yaml` is compiled by `nightly.yaml` and by the tag build's `make assets` rather than by anything a PR runs, and `promote-rc.yaml` still hard-fails a promotion whose rc release has no `nocloud-amd64.raw.xz`. Nothing in that chain breaks, because the tag build supplies the asset as before. What a PR no longer catches is narrow: it still compiles `installer.yaml` through the same imager, and the two profiles are generated by the same script and differ only in `platform`, `kind`, `imageOptions` and `outFormat`. ### Product fixes Four, all found by measurement while building the lane, each on its own commit: The linstor `drbd.enabled` gate. Without it drbd-logger sidecar exits, satellite is never Ready, piraeus registers zero nodes and every PVC hangs Pending behind a cluster that looks healthy. linstor `--strict-topology`. Without it external-provisioner passes every topology segment as `requisite`, so a node-pinned volume can be provisioned away from its pod and the pod is then unschedulable forever with nothing erroring anywhere. kubevirt-cdi importer resources are configurable now. Default restates CDI's own values exactly so production behaviour does not move, and the lane raises the ceiling through its own Package. vm-disk binds standalone disks immediately. A VMDisk on `local` silently never populated, while the same disk on `replicated` did. ### Where the run stands The lane has gone green twice, most recently on `cc70cef7b` where `E2E (in-tree)` took 2h3m against its 215m cap with 45 Chainsaw tests passing. The measurements quoted elsewhere in this description come from the first of those runs, on a commit that has since been rebased away: the two tenant-Kubernetes suites came in at 569.72s (`kubernetes-latest`) and 470.19s (`kubernetes-previous`) against a 67m operation. Both runs answer the four earlier reds, all of which were the lua-protobuf segfault from #4012 once `vminstance`/`vmdisk` were fixed on the branch. ghcr.io mirror removal is carried here too and is the same change as #4007, whichever lands first makes the other a no-op. ### What review changed `drbd.enabled=false` now refuses to render together with `talos.enabled=false`. It removed the logger sidecar only, while the two DRBD initContainers piraeus contributes unconditionally are deleted by the Talos configuration alone, so that combination produced a satellite that can never reach Ready. `isp-full-generic` sets `talos.enabled=false`, so it was reachable and not theoretical. vm-disk keeps unconditional immediate binding, a standalone disk has no consumer to wait for. What it costs on a node-pinned class is in the chart README now, and the annotation is pinned per source type and on the existing-DataVolume path where `helm upgrade` retrofits it. The rest is the harness holding its own promises: HelmRelease gate reads are bounded and its minimum-count half is pinned by a test that fails without it, two Chainsaw ops sit above the deadlines they contain, the fallback-StorageClass render restores `replicated` when it fails, container lane compose project and pool files are per-checkout, and three comments that said the opposite of the code now say what is true (QEMU is still wired into nightly and e2e-tag, the lane replaces two Packages and not one, the argument-count assertion cannot see upstream drift). ### Release note ```release-note fix(linstor): CSI provisioning is constrained to the node the scheduler chose (`--strict-topology`), and the DRBD sidecar can be switched off with `drbd.enabled` on substrates whose kernel cannot run DRBD. On an existing cluster the new default re-templates `spec.csiController.podTemplate`, so piraeus rolls `linstor-csi-controller` once during the upgrade. `drbd.enabled=false` requires `talos.enabled=true` and the chart now refuses the other combination at render time, because only the Talos satellite configuration removes the DRBD initContainers. fix(kubevirt-cdi): CDI worker pod resources are configurable through `importerResources`, which reaches the importer, the uploader and the host-assisted cloner. The default restates CDI's own built-in values exactly, so an install that sets nothing behaves as before. fix(vm-disk): every disk requests immediate binding, not only `upload` sources, so a standalone disk on a WaitForFirstConsumer StorageClass is populated instead of waiting for a consumer that may never arrive. On a node-pinned class such as `local` the volume then binds where CDI's worker pod was scheduled rather than where the VM will run; see the chart README before choosing such a class for VM disks. The default `replicated` is unaffected. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added container-based end-to-end testing with local storage support. * Added optional backup-access preflight checks for database backup examples. * Added configurable CDI worker resources and LINSTOR storage settings. * Added configurable immediate storage binding for VM disks. * Added stronger readiness checks, diagnostics, and parallel test execution. * **Bug Fixes** * Improved cleanup failure reporting, capacity validation, and missing-image detection. * Improved FoundationDB health verification and storage placement behavior. * **Removed** * Removed GHCR mirror support and soft-red node-join tolerance. * Removed standalone OIDC render test suites. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Myasnikov Daniil
|
a115745c69 |
feat(vm-disk): let the operator defer binding to the consumer
Immediate binding for every source fixed a real bug: a standalone disk on a WaitForFirstConsumer class has no consumer to wait for, so it never populated. It also took away the path that is correct for the other case. For a disk a VMInstance consumes, KubeVirt renders a temporary launcher pod carrying the VM's own affinity and nodeSelector, so the volume binds where the VM can actually run; with the annotation always present the CDI worker becomes the first consumer instead and the volume lands where that pod happened to be scheduled. On a node-pinned class the VM is then unschedulable against a node-affinity conflict, and storageClass is immutable, so the only exit is deleting the disk. The two cases want opposite defaults and only the operator knows which one a given disk is. bindImmediately defaults to true, so nothing moves for existing disks or for anyone on `replicated`, which binds Immediate either way. Both branches are pinned in tests, including the existing-DataVolume path, which renders its own annotation block. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> Assisted-By: LLM |
||
|
Alexey Artamonov
|
c693ed36fb |
fix(postgres): force client-side apply so the autoscaler seed survives
Under active read-replica autoscaling the chart seeds spec.instances at a constant max(replicas, effectiveMin) rather than omitting it (an omitted field defaults to 1, which the CNPG quorum webhook rejects, and — under server-side apply — the phase-2 omit prunes the field whenever KEDA has not yet written /scale, collapsing an idle-at-floor cluster to 1 and shedding its replica PVCs). Keeping the constant only works if Flux does not force-own and revert it. The postgres ApplicationDefinition therefore carries release.cozystack.io/helm-server-side-apply: "false", so the tenant HelmRelease applies client-side: helm-controller patches the CNPG Cluster from the previous-vs-new rendered manifest without consulting live state, the unchanged constant yields no patch, and KEDA's higher live /scale count is preserved. The constant also equals the disabled/dryRun/transition render (instances: replicas) whenever replicas sits at/above the floor, so those flips are patch-free too. Correct the merge-semantics comments across db.yaml, _autoscaling.tpl and values.yaml to describe the two-way previous-vs-new patch precisely (not a "three-way no-op"), and document the residual floor-change hazard: editing replicas / minReplicas / quorum.maxSyncReplicas moves the seed, so that one reconcile does patch spec.instances to the new seed. Add autoscaling.serverAddress so a monitoring:true tenant can point the ScaledObject at a vmselect that carries both the CNPG metrics and kube_pod_labels. Regenerated README/schema/types/cozyrds. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Alexey Artamonov
|
826f08972f |
fix(apps/postgres): make autoscaler fail-safe, dip-free, and observable
Address review findings on the KEDA read-replica autoscaler where the
implementation diverged from the merged design proposal.
Fail-safe read load. Gate the `or vector(0)` floor on the existence of ANY CNPG
instance pod (`count(kube_pod_labels{...instance_role=~".+"}) > 0`) — primary or
replica — so the query is empty ONLY when no CNPG pod labels exist at all (a
broken kube-state-metrics pipeline), not during initial provisioning when the
sole replica-side pod is CNPG's join Job (jobRole, not instanceRole). Also pin
`ignoreNullValues: "false"` on the trigger: KEDA's prometheus scaler defaults it
true, turning an EMPTY result into metric 0 (not an error) — the HPA would then
desire 0 and clamp to the floor, i.e. silently scale a broken-pipeline cluster
DOWN. With it false an empty result is a scaler error, so the HPA HOLDS and
DatabaseAutoscalerScalerErrors fires; gating on any instance role keeps a healthy
cluster (including a long basebackup) from tripping that alert.
Constant seed = max(replicas, effectiveMin), so enable/dryRun/transition flips
are a merge no-op with no dip and never undercut the quorum floor.
Fail closed when the quorum floor exceeds maxReplicas (was: effectiveMax silently
raised both bounds, unbounded for a large maxSyncReplicas).
Dry-run / transition keep the HPA (unidirectional pause pair, not the HPA-deleting
full pause) but both SelectPolicy=Disabled, so desired normalizes to current — the
recommendation is read from the read-load metric, not a diverging desired count.
Observability. DatabaseAutoscalerAtMax keys on desired >= maxReplicas, NOT the
ScalingLimited condition (which is also true at the floor, so it fired on every
idle cluster with wrong "raise maxReplicas" advice). Dashboard read-load panels
are per-cluster (label_cnpg_io_cluster) with a CPU panel for ReadCPUUtilization
and the exact clamp in descriptions; ScaleStuck's Pending intersection is scoped
to CNPG pods; alerts.yaml narrative comments moved to Go-template comments so they
do not ship into the PrometheusRule.
Guard the seeding invariant: TestBuildHelmReleaseSpecNoForceOrDriftDetection.
Regenerated README/schema/types.
Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
|
||
|
Alexey Artamonov
|
6e3da63586 |
fix(postgres): guard the disable-wedge, role-scope the freeze count, and harden edges
Address the remaining IvanHunters review points on #3954: - [MAJOR] Disabling autoscaling wedged the HelmRelease under synchronous quorum. An autoscaled + maxSyncReplicas>=1 cluster runs with instances seeded from effectiveMin while replicas stays at its default, so flipping autoscaling off rendered instances: <replicas> next to the quorum and CNPG's webhook rejected it ("maxSyncReplicas must be lower than the number of instances"). Add a symmetric render-time guard on the static-count path (covers off/dryRun/ transition): fail with an actionable message when replicas <= maxSyncReplicas. - [MINOR] Role-scope the freeze brake's currentInstances count (label_cnpg_io_instance_role=~".+") so lingering CNPG join/bootstrap Job pods (cnpg.io/cluster but jobRole, not instanceRole) don't inflate it and nudge desired up while the brake is meant to hold it. - [MINOR] Default maxReplicationLagSeconds to 0 (brake opt-in): only the non-braking path is validated live, so ship the validated default and let the freeze be enabled explicitly until its branch is exercised under load. - [MINOR] Document the floor-change and non-quorum-disable dips (same three-way- merge hazard) and the pin-replicas-first mitigation. - [NIT] Coerce the cozy-lib helper's `paused` (eq "true") so a caller passing the string "false" does not pause the ScaledObject. - [NIT] Trim the keda.clusterDomain injection comment so it no longer ships ~8 verbatim lines into the cozystack-values Secret on every cluster. Regenerate types/schema/README/postgres-rd. New tests cover the disable-wedge guard, the role-scoped freeze count, and the string-"false" paused coercion. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Alexey Artamonov
|
842a59a957 |
fix(postgres): correct the migration-dip docs and pin the KEDA metricType
Two review findings: - The enablement note claimed the two-phase `transition` flow avoids the count dip when enabling autoscaling on an existing cluster. It does not: helm-controller upgrades with Helm's three-way merge (not SSA), which keys off the old-vs-new manifest diff, not KEDA's live /scale value. At phase 2 the chart stops rendering the staged static count, so the field is deleted (no quorum -> CNPG defaults to 1, a brief collapse) or changed (quorum -> rebased to effectiveMin), then the warm KEDA restores it. Rewrite the docs to state the flow reduces (KEDA is pre-warmed, no cold-start) but does not eliminate the dip, and spell out the suspend + scale-subresource pin that actually makes the handoff dip-free. This is the SSA-vs-three-way-merge handoff the design proposal left for the PoC to settle. - Pin metricType: AverageValue on the Prometheus trigger in cozy-lib.keda.scaledObject. The desired = ceil(query / threshold) identity holds only for an AverageValue external metric; it was relying on KEDA's default. Make it explicit and assert it so a re-vendor or default change cannot silently break the arithmetic. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Alexey Artamonov
|
610bcb5df9 |
fix(postgres): seed spec.instances at the quorum floor under autoscaling
Under active autoscaling the chart previously omitted .spec.instances entirely,
letting the KEDA HPA own it via /scale. But CNPG defaults an absent
.spec.instances to 1, and its admission webhook rejects a Cluster where
maxSyncReplicas >= instances ("maxSyncReplicas must be lower than the number of
instances"). So a fresh database created with autoscaling.enabled: true AND
synchronous quorum (maxSyncReplicas >= 1) was rejected at admission before KEDA
could ever scale it to effectiveMin (= maxSyncReplicas+1) — the whole tenant
HelmRelease failed. The default maxSyncReplicas: 0 path was unaffected, which is
why it went unnoticed. Verified live: a rendered Cluster with maxSyncReplicas: 1
and an omitted instances is rejected by the dev-cluster webhook; with the seeded
floor it is admitted.
Render instances: effectiveMin under active autoscaling instead of omitting it,
so a fresh Cluster starts at a valid, quorum-satisfying count. Because the chart
emits the same constant floor on every reconcile, Helm's three-way merge is a
no-op on the field and never reverts KEDA's higher live value, so the HPA still
owns the runtime count. Update the enablement docs (a new DB can enable directly
even with synchronous replication; existing DBs above the floor rebase to it
unless the two-phase transition flow is used) and add render tests for the fresh
create + maxSyncReplicas>=1 case.
Addresses the fresh-create-with-quorum admission gap found in branch review.
Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
|
||
|
Alexey Artamonov
|
284f5e5543 |
fix(postgres): fail closed when autoscaling is enabled without KEDA, guard target
autoscaling.enabled is a tenant-facing knob, but KEDA is an optional platform package that is not installed by default. A tenant enabling autoscaling on a cluster without KEDA would render a ScaledObject against an unknown kind and the whole database HelmRelease would fail with an opaque `no matches for kind "ScaledObject"`. Guard the ScaledObject template on `.Capabilities.APIVersions.Has "keda.sh/v1alpha1"` and stop with an actionable message instead. Skipping the render is deliberately NOT an option: db.yaml already drops the static .spec.instances under active autoscaling, so a missing ScaledObject would let CNPG collapse the cluster to 1 instance. Document the KEDA prerequisite on the `enabled` field (README/schema). Also reject a non-positive autoscaling.target at render time: it is a plain integer now, but the desired-replica formula divides the read load by it, so 0 is undefined and negatives invert scaling. cozyvalues-gen has no minimum directive, so the bound is a template guard. Add unittests for both guards (a dedicated suite renders without the KEDA CRD so the fail-closed path is exercised; the main suite now advertises keda.sh/v1alpha1 via capabilities). Addresses IvanHunters (KEDA-absent) and CodeRabbit (non-positive target) review feedback on #3954. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Alexey Artamonov
|
f8fc2d99bf |
fix(postgres): make autoscaling.target an integer and drop the dead root metric
Two review findings on the generated API: - autoscaling.target was a resource.Quantity, which accepts suffixed values like `150m`. The chart splices target straight into the PromQL query and the KEDA threshold, where `150m` means 150 minutes to PromQL and is not a usable KEDA float threshold. Retype it as a dimensionless integer so the schema rejects suffixed input - helm unittest's schema validation now enforces it, and the existing autoscaling suite exercises the integer contract. - ConfigSpec carried a root `metric` field (and spec.metric in the postgres-rd schema) that nothing reads - the chart consumes only autoscaling.metric - so setting it silently did nothing. Remove the dead root param, keeping the ReadMetric enum that autoscaling.metric references. Regenerate types.go, zz_generated.deepcopy.go, values.schema.json, README.md and the postgres-rd schema; update the autoscaling tests to the integer target. Addresses CodeRabbit review feedback on #3954. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Alexey Artamonov
|
d4c76fadb4 |
feat(postgres): two-phase autoscaling migration and read-CPU metric
Make enabling autoscaling on an existing database count-preserving, and add a second read-load signal: - autoscaling.transition (migration phase 1): keep .spec.instances rendered as a floor (from replicas, which the operator stages to the live count) AND pause the ScaledObject, so KEDA stands up without contending with Flux for the field. Flip transition off (phase 2) to drop the static field and hand the count to KEDA — suspend the HelmRelease across that apply, since helm-controller uses Helm's three-way merge (not SSA) and would otherwise race CNPG's default-to-1. The procedure is documented on the values field. - autoscaling.metric (ReadConnections | ReadCPUUtilization): the query helper emits either the active-connections sum or the postgres-container CPU sum, both + target. active = enabled && !dryRun && !transition; the ScaledObject is paused whenever dryRun or transition is set. Covered by helm unittest (phase-1 holds the staged count + pauses; CPU metric query) and regenerated schema/README/Go types/postgres-rd. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Alexey Artamonov
|
0335276b61 |
feat(postgres): render a KEDA ScaledObject for read-replica autoscaling
Implement the KEDA-based read-replica autoscaler for postgres per the merged design proposal (cozystack/community#44): - cozy-lib.keda.scaledObject: engine-agnostic helper rendering a KEDA ScaledObject (scale target, effective bounds, prometheus trigger, scale-down behavior, optional pause), fail-closed on bad input. - postgres: an `autoscaling` values block (enabled / minReplicas / maxReplicas / target / maxReplicationLagSeconds / dryRun). Under active autoscaling the chart drops the static .spec.instances so Flux and the KEDA-managed HPA no longer contend, and renders a ScaledObject whose minReplicaCount folds in the synchronous-quorum floor (max(minReplicas, maxSyncReplicas+1, 2)); when the floor exceeds maxReplicas, quorum wins. dry-run keeps the static count and pauses the ScaledObject. The WorkloadMonitor tracks the quorum floor instead of a stale static value. Covered by helm unittest (cozy-lib helper: valid render, pause, three fail-closed guards; postgres: instances dropped, quorum-floored bounds, quorum-over-max, dry-run pause). Regenerated values.schema.json, README, Go types and postgres-rd. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Aleksei Sviridkin
|
3a369fed99 |
feat(redis): add opt-in TLS for Redis and Sentinel (#2729)
## What this PR does `tls.enabled` puts Redis and Sentinel on a TLS-only listener with a per-release cert-manager chain, and hands the tenant the CA certificate as `redis-<name>.tenant-ca`. Off unless set. redis-operator v3.3.5 has no TLS support. The work is ported upstream in freshworks-oss/redis-operator#85, which is open and not in a release, so this carries it as three patches over the v3.3.5 tarball. `images/redis-operator/patches/tls.diff` is the source half, cut from that PR's commit `2ad2ed8d1`, which the patch header pins; it is applied by the glob the image Dockerfile already runs, after the Dockerfile strips the tarball's `manifests`, `charts`, `example` and `.github`, so the image builds from source alone and the upstream tests that read the CRD YAML skip instead of failing against the stale v3.3.5 copy. `patches/crd-tls.diff` adds `spec.tls` to the vendored CRD, and with it the CEL rule that fixes `tls.enabled` at creation; without the schema block the apiserver prunes the field on admission and the patched binary waits for something it never sees, and the rule cannot exist without the block it references. `patches/rbac-tls.diff` grants Create, Get and Update on Secrets, so the operator can publish the CA-only Secret it addresses by name. That is the whole delta, and it is narrower than the ported operator's own chart asks for: those three are the only Secret calls in the patched source, and cert-manager Certificates get no grant at all, because the operator only touches one under `spec.tls.certManager` and this chart pins `certificateSecret` mode instead. One change in `tls.diff` is not TLS and takes effect with TLS off, named in the patch header: `MakeSlaveOfWithPort` connects to the replica on its own port rather than on the master's. It changes a dial, not a pod template, so shipping the rebuilt image rolls nothing; the plaintext pod templates stay byte-identical to v3.3.5. The image build registration this branch used to carry (`make -C packages/system/redis-operator image` in the root `build:` target) landed on main with the Valkey service (#3380), so the rebuilt operator image is produced by CI from main as well; the digest in `values.yaml` is still refreshed by release preparation, not here. The chart patches re-apply at the end of `update:`, the same way reloader, grafana-operator, ingress-nginx, seaweedfs and kamaji carry theirs. Otherwise `rm -rf charts` takes TLS back out on the next `make update` and nothing fails. `make update` reproduces the committed tree byte for byte. TLS is chosen when the instance is created. The config gets `port 0` and the listener moves to `tls-port`, so TLS replaces plaintext rather than running beside it, and a running instance cannot change mode: a replica restarted onto the TLS listener cannot replicate from a master still on plaintext, and Sentinels already on TLS would fail over to a stale replica. The RedisFailover schema in the carried CRD patch rejects a change of `tls.enabled`, which on an existing instance surfaces as a failed HelmRelease upgrade while the instance keeps serving what it did; the README points at creating a new instance and migrating. That is also why TLS is not inferred from `external`: an externally reachable instance is the one most likely to have clients an upgrade must not drop. Existing releases keep their plaintext listener and are not restarted by the rebuilt operator image. The tenant gets `ca.crt` and nothing else, through `tenantsecrets`. The chart's own Secrets take a dot between the release and the suffix (`<release>.ca-tls`, `<release>.tls`, `<release>.ca-cert`): the suffixes are dot-free, so the split is unique and release `foo`'s CA Secret can no longer be release `foo-ca`'s leaf, the collision the hyphenated form had. The chart renders a `TenantProjection` pointing at `<release>.ca-tls`, and the CA-extraction controller re-encodes the one named key from the parsed certificate, so nothing that is not a certificate gets through. Same path as postgres, mongodb, nats and qdrant. No Secret of this release is granted to the tenant by name beyond the generated password, and that includes `<release>.ca-cert`, the CA-only copy the operator publishes for itself. It holds no key today, but the operator has Create and Update on Secrets cluster-wide, so what sits at that name is the operator's call rather than the platform's, and RBAC cannot filter by key. `hack/check-redis-rd-secrets.bats` pins the whole rendered Secret surface instead of a list of known-bad names, because an include with no `resourceNames` and no `matchLabels` restricts nothing. The chainsaw suite for redis gains two cases. A TLS fixture asserts the cert-manager chain is issued, the TLS-probed Redis replicas and Sentinel come up, the operator publishes its CA-only Secret under the widened ClusterRole, the tenant receives exactly `ca.crt` through `tenantsecrets` while the key-bearing Secrets stay off that path, and a `redis-cli --tls` PING through the service name completes with the same CA. A second case applies `tls.enabled` to a running plaintext release and asserts the refusal: the HelmRelease upgrade fails with the schema's message, the StatefulSet keeps its revision with both replicas Ready, plaintext still answers and no TLS listener appears. Certificate renewal rolls the pods from inside the operator. Redis and Sentinel read the certificate once at startup, so the patched operator stamps a hash of the TLS Secret's `tls.crt` and `ca.crt` into the pod template of both the Redis StatefulSet and the Sentinel Deployment on every reconcile, as `redis-failover.freshworks.com/tls-secret-hash`. A renewal changes the hash; the operator's own roller replaces the Redis pods one at a time and the Deployment controller rolls Sentinel. README names the fallback: if the roll cannot run, the deadline is the old certificate's expiry, past which the operator's own TLS connections fail along with the clients. `tls.authClients` maps to `tls-auth-clients` and defaults to `no`. The platform mints no client certificate for the tenant and the CA key is out of reach, so `yes` is only usable when the client certificate comes from whoever also controls the CA. Changing the value on a running release restarts the Redis and Sentinel pods: Redis reads the directive from its config file only at startup, so the operator folds the effective value into the certificate hash that drives the rollout, and the change lands when it is made rather than at the next unrelated restart. Follow-ups in the carried operator patch go upstream first, to freshworks-oss/redis-operator#85 or a successor there, and are picked up here afterwards; the carried patch does not grow Go that upstream has not seen. `tlsConfigFor` fetches the Secret on every call, tens of GETs per reconcile. The renewal roll is static analysis so far: the live check, `cmctl renew` on a running release while watching the `rfr-*` and `rfs-*` pods roll, is still owed and belongs with the e2e follow-up. Two facts about the shipped image rather than the patch. `packages/system/redis-operator/values.yaml` keeps the digest of the image built before the patch until release preparation rebuilds it, so on `main` between this merge and the next release-prep, `tls.enabled` renders the chain and `spec.tls` while the running operator ignores them and serves plaintext. And the pinned v3.3.5 tarball carries GO-2026-5026 in `golang.org/x/net` v0.53.0, reached through `PodService.UpdatePodLabels`; the port neither introduces nor fixes it, upstream `master` is already on v0.55.0, so it lands with the next tag bump rather than here. Outside the patch: `cozyvalues-gen` renders `{}` in the README Value column for `tls.authClients`: a child property without a default inherits the parent object's `{}`, and `make generate` reproduces it byte for byte, so it is a generator bug rather than something to hand-edit here. ### Screenshots No UI changes. ### Downstream repositories - [ ] 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: cozystack/terraform-provider-cozystack#37 - [ ] [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: I walked the trigger map against the diff file by file. terraform-provider-cozystack is reached, tracked as cozystack/terraform-provider-cozystack#37 rather than a speculative PR: `values.schema.json` gains a `tls` object, and the provider hand-writes a schema, model and expand/flatten pair per Kind. It already models `tls` for postgres, kafka, nats and qdrant, not for redis. Redis needs different wording from those four, whose descriptions say to omit the block to follow `external`, which is the one thing redis does not do. Nothing else is reached. The website regenerates this package's reference page from `README.md` on a stable tag, so the README changes need no PR there. No package added, renamed or removed. `packages/core/platform/values.yaml`, variant and bundle composition and release asset names are untouched, and so are `hack/package.mk`, `hack/common-envs.mk` and `hack/update-crd.sh`, which is what ccp and external-apps-example read. The two new files under `hack/` are bats unit tests. No node prerequisite, installer value, telemetry metric or cozy-proxy label changes. ### Release note ```release-note feat(redis): add opt-in TLS for Redis and Sentinel. Set `tls.enabled: true` to serve both on a TLS-only listener backed by a per-release cert-manager chain; the tenant reads the CA certificate as the `<release>.tenant-ca` tenant secret. TLS is off unless set and is not inferred from `external`, so existing releases keep serving plaintext and are not restarted by the rebuilt operator image. TLS is chosen at creation: a running instance cannot switch modes, the change is rejected and surfaces as a failed HelmRelease upgrade, so add TLS by creating a new instance and migrating to it. Requires cert-manager, which the platform's `system` bundle installs on every variant. A renewed certificate rolls the Redis and Sentinel pods through the operator. TLS support is carried as a downstream patch over redis-operator v3.3.5 until freshworks-oss/redis-operator#85 lands in a release. ``` |
||
|
Aleksei Sviridkin
|
d800526696 |
feat(mariadb): add TLS support via cert-manager (#2680)
## What this PR does Gives MariaDB a chart-managed certificate chain, makes TLS enforcement explicit, and hands the tenant a key-free trust anchor instead of a private key. - `tls.issuer` selects who issues the server certificate: `operator` (the CA mariadb-operator manages itself, the default and today's behaviour) or `cert-manager` (the instance gets its own CA, and this is what covers the external hostname). The field is named for the issuer rather than for on/off because TLS is served under both — unlike the similarly spelled field in some other charts, which really is a switch. - `tls.required` refuses plaintext connections. It is opt-in and defaults to false: enforcing TLS before the tenant can obtain the CA would be backwards, and a platform upgrade must not sever connections that work today. - Neither issuer is inferred from `external`. Existing instances keep the CA they have and change nothing on upgrade. - The tenant no longer receives `<release>-ca-tls` or `<release>-tls` — the CA private key and the server private key. It receives the operator's `ca.crt`-only bundle, which works today, and the ResourceDefinition additionally declares that bundle as the source for the CA-extraction controller. ### Fixes along the way - The TLS backup path could never have worked: it passed `--ssl-mode=VERIFY_IDENTITY`, a MySQL-8 flag the MariaDB client rejects during argument parsing. Replaced with `--ssl-ca` + `--ssl-verify-server-cert`, and the test now asserts the dialect rather than the presence of a substring. - The leaf certificate carried `clientAuth`. Because `REQUIRE X509` accepts any certificate signed by a trusted CA, the server's own key was usable as a client credential. Removed. - The CA and server private keys survived deletion of the instance: cert-manager runs with `enableCertificateOwnerRef: false`, so removing the `Certificate` left the Secrets behind. Both are now cleaned up by the existing post-delete hook. - The Secrets are labelled for the operator's watch predicate, without which a renewed leaf would not reach the pods until a full resync. ### Why the include carries an exclude list The trust-anchor selector matches by label with no name constraint, so the enumerable dangerous set is excluded explicitly — both issuers' private keys, the internal credentials, the backup keys, and the metrics configuration, which the operator writes unconditionally with credentials in plain text for a user holding `SELECT` on everything. The backstop is bounded and the comment says so: matching is by exact name with no globs, so a Secret introduced by a future operator version, or a transient object created mid-renewal, stays in scope of the label-only include. The primary defence remains that the controller stamps only its own projection. ## Known limitations 1. **Merge is blocked on #3299 — and the blocking is mechanical, not just procedural.** The CRD on `main` does not carry `spec.caCert`, and under Flux's Server-Side Apply the API server rejects a field the schema does not declare (`field not declared in schema`); under the older client-side apply the same field was silently dropped, so installing this branch without #3299 fails outright. An earlier revision of this description claimed the field would be silently pruned; that was wrong. The tenant still gets a usable trust anchor once merged in order, through the operator's bundle. 2. **The deprecated backup CronJob cannot start at all**, for reasons that predate this branch: its `secretKeyRef` names a Secret and key the chart never renders. That is filed separately as #3347 and deliberately not fixed here, since repairing it would start scheduled dumps on production instances from a TLS change. The tests that pin the backup TLS flags therefore pin a path that does not currently execute — the suite header says so. 3. **The operator's `clusterName` is hardcoded**, so on a cluster with a non-default domain the SANs and the name the operator dials diverge. Pre-existing, raised separately. 4. **No e2e coverage for the managed path.** Unit coverage pins the rendered chain, the SAN set, the exclusion set and the guards; the end-to-end handshake is not asserted in CI. ### On review coverage Two independent review passes were run repeatedly against this branch — seven rounds, converging from a security finding, through comment accuracy, to guard strength, and finally to nothing new. A third, model-based pass reviewed the branch through `6cb1bd236` and its findings were fixed; it could not be run against the final revision because of an external account quota lasting until 25 July. The delta since that revision is substantial and includes runtime code, not only tests and docs. Recording this so the coverage is not overstated: if the quota returns before this is ready to merge, that pass is worth running again. ### Downstream repositories `packages/apps/mariadb/values.schema.json` gains the `tls` object, so `cozystack/terraform-provider-cozystack` needs the field modelled for `cozystack_mariadb`; tracked in cozystack/terraform-provider-cozystack#40. ### Release note ```release-note feat(mariadb): chart-managed TLS via cert-manager, opt-in enforcement, and the CA published to tenants as a key-free trust anchor ``` |
||
|
Aleksei Sviridkin
|
487cafac3e |
feat(platform): let a tenant pick its Gateway class from an allowlist
Tenant Gateways were pinned to the bundled Cilium class. Give the platform a default in gateway.className, the set a tenant may choose from in gateway.tenantSelectableClasses, and the set whose provider terminates TLS upstream of the Gateway in gateway.edgeTerminatedClasses. A tenant picks its own with tenant.spec.gatewayClass, read only when it owns a Gateway; an inheriting tenant runs on its ancestor's. The selectable set is an allowlist rather than free choice because Tenant is a tenant-writable object while a GatewayClass is cluster-scoped: without it a tenant could attach its Gateway to a class an admin keeps for internal-only traffic. Naming anything outside the set fails that tenant's own gateway release and reaches no other tenant. Where TLS ends is a property of the class, so the gateway chart derives certMode from whichever class the tenant landed on instead of taking it as a separate input. The two therefore cannot be configured out of step. The publishing tenant cannot cross that line by itself. The cluster-wide ACME issuers point their HTTP-01 solver at its Gateway and are rendered from the platform default alone, so a mismatch would pin the solver to a listener that Gateway does not have. Those issuers now drop the sectionName pin when the default class is edge-terminated, letting the challenge attach by hostname instead. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <3811295@gmail.com> |
||
|
Aleksei Sviridkin
|
fcab5dbb07 |
docs(rabbitmq): carry the TLS consequences to where the toggle is rendered
cozyvalues-gen propagates the @field line and nothing under it, so the description reaching values.schema.json, the README table and the dashboard form said only that TLS turns on, while the continuation lines holding the breaking part of the change stayed in values.yaml. Fold the listener, binding-port and metrics-port moves into the @field line, and leave the continuation with the part that is genuinely about the struct. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
36ee8a2608 |
feat(rabbitmq): make TLS opt-in instead of following external
tls.enabled inherited the value of external when unset, so enabling external access also enabled TLS. That is not free on an existing instance: setting spec.tls.secretName makes the operator rewrite the port key of the <release>-default-user Secret from 5672 to 5671, and that Secret is the Service Binding handed to the tenant. Every client resolving its endpoint from the binding — the documented way — would move onto TLS on upgrade and have to verify a per-release CA it has no key-free way to obtain. Resolve unset, null and false to false instead, so TLS is only ever on because someone asked for it. enabled becomes a plain bool now that there is no third state to express. Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
d668dfbd6a |
feat(rabbitmq): add opt-in switch to close the plaintext listeners
TLS so far only added encrypted endpoints; nothing closed the unencrypted ones, and the chart had no way to express the intent. tls.disableNonTLSListeners gives the operator that switch, defaulting to false so an upgrade never closes a listener a client is already using — the choice to break plaintext has to be made deliberately. Its reach is worth stating plainly, because the natural reading is wrong in both directions. It writes `listeners.tcp = none` into rabbitmq.conf, which is broker level: the socket is not opened on any interface, so in-cluster clients lose plaintext at the same instant external ones do, and there is no configuration that closes it outside while keeping it inside. In the other direction it is not "TLS everywhere" either — Erlang distribution on 25672 and epmd on 4369 ride the headless Service, which the flag never touches, so inter-node traffic stays in the clear regardless. The flag also cannot coexist with chart-managed users. messaging-topology-operator derives its connection as TLSEnabled() && (!connectUsingHTTP || DisableNonTLSListeners()), so the flag forces TLS on that side whatever the operator's own configuration says, and it verifies the broker against x509.SystemCertPool(), which cannot hold a per-release CA. The connection-uri annotation is no escape, since it would point at the port the flag just closed — so that annotation is now omitted when the flag is set, rather than advertising a closed port. Every User, Vhost and Permission would silently stop reconciling. Rendering fails instead, naming the conflict, and the flag stays usable for an instance whose users and vhosts are managed outside the chart. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Arsolitt
|
ae0ff9e577 |
chore(rabbitmq): regenerate codegen deepcopy artifacts
Signed-off-by: Arsolitt <arsolitt@gmail.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Arsolitt
|
8007b899e5 |
feat(rabbitmq): add TLS schema and generated artifacts
Extend RabbitMQ values schema with TLS configuration options, update Go API types, regenerate CRD artifacts, and add TLS fields to the rabbitmq-rd operator config. Signed-off-by: Arsolitt <arsolitt@gmail.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
5a9c3dd79f |
feat(mariadb): name the TLS field for the issuer instead of for on/off
tls.enabled read as a switch and was not one — TLS is served under every value, and what the field actually chose was which authority issues the server certificate. The same key means on/off in other charts here, so a tenant carrying that habit across would have read this exactly backwards. Replace it with tls.issuer: operator | cert-manager, defaulting to operator. The value now states the behaviour instead of needing prose to undo the name: operator is what runs today, so the fact that nothing changes on upgrade is visible in the default rather than argued in a comment. Being an enum, a typo is rejected by the schema, which a bool could not do. The explanation the old name required is gone with it — the helper docblock and the CR comment lose the paragraphs that existed only to say this was not a switch, and the README states the split in a table. Not marked breaking: the field is introduced by this series and does not exist on the target branch, so there is no released spelling to migrate from. Breaking is measured against what is merged, not against the intermediate history of the branch. Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
2e4ed43b88 |
docs(mariadb): shorten the TLS field descriptions to fit where they render
These strings flow into the ApplicationDefinition schema and become the help text beside the fields in the console. They had grown into paragraphs of rationale — 704 and 613 characters against 48 for the neighbouring external — which is the wrong length for a form label and duplicated what the README already explains at the right length. Say what the field does and that it is opt-in; the reasoning stays in the README, which is where a reader goes for it. Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
d2d85cb634 |
fix(mariadb): let a missing CA bundle fail the backup instead of stopping it
The bundle volume was a required mount. The operator reconciles it only after certificates settle, so on first install with managed TLS there is a window where it does not exist, and a required mount leaves the pod in ContainerCreating rather than failing. With concurrencyPolicy: Forbid that one stuck pod blocks every subsequent backup for as long as it hangs. Marking it optional turns that into a run that fails with a message naming the missing file, retries, and lets the schedule proceed. Correct the reason given for enforcement defaulting to false while here. It said the tenant had nothing to verify the server against, but this same change grants the tenant read access to the bundle, so the anchor is reachable. The default is still right — enforcement refuses clients that have not moved yet, which is a decision to make rather than something an upgrade does — but that is the reason, and it was stated in five places, all fed from the values annotations. Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
e1cc5e5284 |
fix(mariadb): make managed TLS opt-in instead of deriving it from external
tls.enabled defaulted to the value of external, so every existing external instance would have moved from the operator's CA to a chart-issued one on the next platform upgrade, without its owner doing anything. Clients pinning the operator's ca.crt fail chain validation the moment the new certificate is served, and external instances are exactly the population where a copied ca.crt is most likely. The usual argument for applying something on upgrade does not hold here: taking over the CA does not raise the security floor. These instances already serve TLS under the operator's CA, and what changes is which authority issues it — an architectural move toward the platform trust contract, not protection against a threat. There is no security gain to weigh the breakage against, so it becomes a choice. Rendered for an existing external instance, the result is now identical to what the operator already applies on its own: TLS on, enforcement off, no chart-issued certificates. Nothing happens on upgrade. The default flips as an announced change together with the published trust anchor, the same way tls.required does. Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
18215895fd |
fix(mariadb): apply TLS enforcement regardless of who owns the CA
tls.required was only emitted when the chart managed the certificates, so setting it on an instance using the operator's own CA was accepted by the schema and then dropped on the way to the CR — no enforcement, no error, no warning. For a setting whose entire purpose is refusing unprotected connections, silently ignoring it is the worst available outcome. The coupling was artificial: in the operator API required sits beside enabled with no dependency on either certificate reference, and enforcement works against operator-issued certificates just as well. Emit it in every configuration and let it mean what it says. Also loosen the two negative guards in the ApplicationDefinition check, which required a literal closing quote and exact brace spacing and so would have missed an unquoted key-bearing source. Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
d9c4d614c5 |
fix(mariadb): make TLS enforcement opt-in
Enforcement has to follow the trust anchor, not lead it. The platform does not yet publish a key-free CA to the tenant, so requiring TLS today tells clients they must encrypt while giving them nothing to verify the server against — protection on paper, unverified connections in practice. It also cannot be opted out of in time. Enforcement would arrive with a platform upgrade rather than a user action, and the escape hatch lives in a field this change is the first to introduce, so an operator has no way to set it beforehand. The only available order was upgrade, break, then repair. Default required to false and let enforcement be chosen. Managed TLS still issues the per-instance CA and certificate, so the path to verified connections is in place; what is deferred is refusing the connections that predate it. The default flips to true as an announced change once the published anchor is universal, and the field descriptions say so rather than leaving it implied. This does not change any running instance: enforcement was never applied to one. The chart previously sent no enforcement at all unless it managed the certificates, and managing them is itself opt-in, so an existing instance keeps the operator's unenforced default either way. Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
4339eb4631 |
test(mariadb): pin the ApplicationDefinition trust-anchor invariants
The ApplicationDefinition is where a mistake decides whether a private key can reach a tenant, and it is the one file in this change the chart test suite cannot reach: helm-unittest only covers rendered templates, and the *-rd packages expose no test target. Add a shell check in the style of the existing kafka one, which the Makefile discovers automatically. It pins that the CA source is the operator's key-free bundle rather than either key-bearing Secret, that the tenant selector is the specific label rather than an empty selector that would match every Secret in the namespace, and that no key-bearing Secret is listed by name. Type required as a pointer while here. As a plain bool it rendered a default of false in the generated documentation while the description promised true, and the zero value could not represent an explicit false for consumers going through the generated struct — which is the value the documented migration path depends on. Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
c6678ea053 |
fix(mariadb): stop advertising enforcement and trust the tenant cannot reach
Three corrections to what the rendered objects claim. The schema defaulted tls.required to true unconditionally, so every CR carried required: true even where the chart renders no enforcement at all. The operator reads an absent required as not-enforced, so anyone reading the CR was told plaintext is refused on an instance that accepts it. Drop the schema default and let the template supply the managed default, which keeps enforcement on for managed instances and silent for the rest. The tenant grant for the CA bundle was gated on TLS being managed, but the operator reconciles that bundle whenever TLS is on, which for this chart is always. Ungate it: the default instance serves TLS, so leaving the tenant unable to fetch the CA meant it could not verify the server it was talking to. Reword the claim that TLS cannot be switched off. The operator does honour an explicit disable; what is true is narrower, that omitting the field is not how you select it and this chart always asks for TLS. The text is what a reader relies on when deciding whether a connection is protected, so it has to describe the chart rather than invent an operator invariant. Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
6c1bcb4ff8 |
feat(mariadb): declare the trust anchor and stop claiming TLS can be off
Two corrections to what the chart tells the platform and the user. The tls values never controlled whether TLS runs. The operator substitutes an enabled TLS spec whenever the field is absent, so every instance has always served TLS, and a chart that omits the block only hides that. Render it explicitly and let the values select what they actually select: who owns the CA, and whether TLS is enforced. Rendering the resolved value instead would be the one change that genuinely turns TLS off on instances that have it today, which is not something to ship silently. The field descriptions said the opposite and are corrected with it. Publication of the trust anchor moves to a declaration on the ApplicationDefinition, naming the operator's <release>-ca-bundle. That Secret holds ca.crt alone and carries the previous CA across a rollover, so the publication path never runs next to a private key at all — better than labelling the chart's own CA Secret, which does hold one. The labels on that Secret are dropped rather than left as a second, redundant path. The tenant keeps direct read access to the bundle so the feature is usable before the extraction controller exists; that grant is key-free and comes out once the published anchor is universal. Also set a commonName on the leaf, which cert-manager otherwise leaves empty, breaking clients that verify the hostname against the subject rather than the SAN list. The CA for external instances changes hands here, from the operator's to one issued for the instance. A client verifying against <release>-ca-bundle follows that automatically, because the bundle carries both CAs; a client that pinned a copied ca.crt has to re-read it. Enforcement is a separate, opt-in setting and is not changed by this commit. Signed-off-by: Aleksei Sviridkin <f@lex.la> |