mirror of
https://github.com/cozystack/cozystack.git
synced 2026-11-06 03:57:52 +09:00
648eb42cae3f12bd367102048fa89089fc40407a
1842
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 myasnikovdaniil
|
741a5acc4a |
fix(opensearch-operator): use RollingUpdate maxSurge=0 instead of Recreate (#3319)
## What this PR does
Fixes a Helm-upgrade failure in the `opensearch-operator` chart that
blocks the new release-upgrade E2E lane (added in #3276).
Upgrading from a chart that shipped no explicit strategy (v1.5.x) to a
v1.6.0-rc build fails validation immediately, helm-controller retries
it, and the HelmRelease never becomes Ready:
```
Helm upgrade failed for release cozy-opensearch-operator/opensearch-operator:
Deployment.apps "opensearch-operator-controller-manager" is invalid:
spec.strategy.rollingUpdate: Forbidden: may not be specified when strategy type is Recreate
```
Observed in the ["Upgrade E2E Test" job of run
29480724982](https://github.com/cozystack/cozystack/actions/runs/29480724982)
— `UpgradeFailed ... (x19 over 11m)`, with the HelmRelease never
reaching Ready.
### Root cause
#3040 (commit
|
||
|
myasnikovdaniil
|
960d853706 |
feat(api): typed waitStrategy/healthCheckExprs for kstatus readiness (#2642) (#3273)
## What Adds typed `waitStrategy` + `healthCheckExprs` fields so a package/app can declare the Flux helm-controller v1.5.0 kstatus health gate for the custom resource it wraps — making `HelmRelease.Ready` mean "the CR is actually healthy" instead of "helm applied the CR". This is the operator/apiserver foundation of the kstatus epic (#2642); the API surface was reviewed and approved by the API owner in #3263. Commits (per the review's "isolated step-0" request): 1. `build(deps)` — the dependency bump (see below). 2. `feat(api)` — the fields, plumbing, tests, and regenerated code. 3. `fix(api)` — match the openapi code-generator pin to the 0.35 runtime (fixes an aggregated-apiserver crash the bump surfaced). 4. `fix(api)` — declare `OpenAPIModelName` on the apps types so SSA resolves catalog kinds. 5. `docs(api)` — note that `disableWait` silently defeats `healthCheckExprs`. ## API surface Typed fields on **both** HelmRelease-building paths: - `ApplicationDefinition.spec.release` (apiserver / catalog apps → `convertApplicationToHelmRelease`) - `ComponentInstall` (operator / platform packages → `buildHelmReleaseSpec`) ```go // WaitStrategy maps to HelmReleaseSpec.WaitStrategy.Name — a deliberate scalar // simplification of the upstream {name} object. One of poller|legacy. WaitStrategy string // HealthCheckExprs maps to HelmReleaseSpec.HealthCheckExprs (the upstream // kustomize.CustomHealthCheck type verbatim). HealthCheckExprs []kustomize.CustomHealthCheck ``` `config.ResolveWaitStrategy` is shared by both builders and **couples the default**: `healthCheckExprs` are only evaluated under the `poller` strategy, so when expressions are set and no strategy is given, the generated HelmRelease defaults to `poller`. A package that sets only `healthCheckExprs` is therefore self-contained, independent of the controller's global default. ## The dependency bump (commit 1) The `WaitStrategy` / `kustomize.CustomHealthCheck` / `HelmReleaseSpec.HealthCheckExprs` types do not exist below `helm-controller/api` v1.5.0 — the runtime is already v1.5.0 (shipped by #2602), so this closes a Go-types-vs-runtime skew. It is **not** an isolated bump: v1.5.x requires **k8s 0.35 + controller-runtime 0.23**, cascading the k8s stack 0.34 → 0.35 (and the go directive to 1.26). cozystack replaces `k8s.io/apimachinery` with a fork carrying the still-open upstream `ConvertToVersion` patch (k/k#135537). That single commit rebases cleanly onto apimachinery v0.35.0; published at `cozystack/apimachinery@release-1.35-pr135537`, and `go.mod` pins its pseudo-version. **The repo builds with zero source changes under the new stack.** > Maintainer note: please confirm the `release-1.35-pr135537` fork branch naming, or fold it into the `cozystack` branch as you prefer — the pin can be updated to match. ## Not included (conscious YAGNI) `disableWait` is intentionally omitted. The only known need (the kubernetes tenant-addon deadlock) was resolved child-side, and `waitStrategy: legacy` is not equivalent (legacy still waits Helm-v3-style; `disableWait` skips waiting entirely). Add it later when a concrete leaf needs a true no-gate escape hatch. ## Tests - `config.TestResolveWaitStrategy` — the couple-the-default logic (unset+exprs→poller; explicit legacy honored; unset+no-exprs→nil). - Operator + apiserver cases mirror each other (`TestBuildHelmReleaseSpecHealthCheckExprs` / `TestConvertApplicationToHelmRelease_HealthCheckExprs`). - `go build ./...` clean, `go vet` clean, `gofmt` clean, codegen regenerated and committed. The 4 backupstrategy CRDs regenerate with benign k8s-0.35 PodSpec description drift (DynamicResourceAllocation alpha→stable, toleration Lt/Gt, resizePolicy note) — no functional change. ## Follow-up Phase 3 (per-leaf adoption — postgres/kafka/mongodb/etc. declaring their CEL via these fields) builds on this. The full CRD→CEL catalog is maintained in the epic's readiness audit. Refs #2642 ```release-note feat(api): add `waitStrategy` and `healthCheckExprs` to ApplicationDefinition and ComponentInstall, so a HelmRelease reports Ready only once the custom resource it wraps is actually healthy ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added configurable CEL health-check expressions for application releases and package components. * Added `waitStrategy` support (`poller` and `legacy`) with automatic `poller` selection when health checks are configured and no strategy is set. * Propagated these settings into generated Helm releases, affecting readiness/wait behavior. * **Documentation** * Updated Application and PackageSource schemas to include `healthCheckExprs` and `waitStrategy`. * Extended related Kubernetes CRD documentation, including projected pod certificate annotations and workload reference fields. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Andrey Kolkov
|
e73ad68a85 |
fix(backups): support cnpg-operator with barman plugin (#3313)
<!-- Thank you for making a contribution! Here are some tips for you:
- Use Conventional Commits for the PR title: `type(scope): description`
- Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore
- Scopes are not an exhaustive list — pick the most specific scope for
the change and extend the list when a genuinely new area appears.
Examples:
- System components: dashboard, platform, operator, cilium, kube-ovn,
linstor, fluxcd, cluster-api
- Managed apps: postgres, mariadb, redis, kafka, clickhouse,
virtual-machine, kubernetes
- Development and maintenance: api, hack, tests, ci, docs, maintenance
- Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or
add a `BREAKING CHANGE:` footer
- If it's a work in progress, consider creating this PR as a draft.
- Don't hesistate to ask for opinion and review in the community chats,
even if it's still a draft.
- Add the label `backport` if it's a bugfix that needs to be backported
to a previous version.
-->
## What this PR does
Fixes #3300. Migrates CNPG Postgres backups off the deprecated native
`spec.backup.barmanObjectStore` (removed in CNPG 1.29; the
`barman-cloud-*` binaries it needs are absent from the `standard` image
variant keycloak-db pins, and the bare system-flavor tags apps/postgres
pins are deprecated upstream) onto the [CloudNativePG Barman Cloud
plugin](https://github.com/cloudnative-pg/plugin-barman-cloud), across
every Postgres backup surface: `apps/postgres`, `system/keycloak`, and
the `cozy-default-cnpg` BackupClass strategy driver. The chart renders
`spec.plugins` + a `barmancloud.cnpg.io/ObjectStore` CR; the
backup-controller driver SSA-applies an `ObjectStore` and patches
`spec.plugins`; `Backup`/`ScheduledBackup` use `method: plugin`. The
`postgres-operator` chart vendors and co-locates the
`plugin-barman-cloud` operator (with its cert-manager-issued mTLS) so
CNPG can discover it.
One platform-level change is required for the plugin to run and ships
with this PR (see the upgrade notes in
`docs/operations/backup-classes.md` and the release note below): the
CloudNativePG operator is bumped to 1.28.1 (also fixing an operator/CRD
version skew that broke all backups). The other prerequisite — the
LINSTOR scheduler admission webhook stripping
`initContainers[].restartPolicy` cluster-wide, which broke the plugin's
restartable sidecar — is already fixed on `main` (
|
||
|
Myasnikov Daniil
|
b16a51e60e |
test(seaweedfs): pin the naming guard's MultiZone zone-component classification
MultiZone (and Simple-with-pools) tenants render no plain `volume` component — extra/seaweedfs sets volume.enabled=false and the vendored chart emits one component per zone key with suffix `volume-<key>`, truncated at (62 - len(suffix)), which is SHORTER than the 56-char cut the guard's renamedVolumePrefix replays. The final review pass flagged this divergence as a guard bypass. For the supported instance the prefixes cannot diverge: the tenant module hardcodes the name `seaweedfs`, the fullname is 16 chars, and nothing truncates until a zone/pool key of ~40+ characters. Three new cases pin that supported path — legacy-only zones adopt, release-named-only zones refuse as class S, both generations refuse — since the suite previously had no zone shapes at all and the zone objects are the ONLY evidence on a MultiZone tenant. The divergence beyond that boundary (long instance names or absurd keys) is an accepted, documented limit, not a fix: decided in the 1.6 triage, recorded in _naming.tpl (both copies, kept in lockstep) and the runbook's Scope section. Instance names are fixed by the tenant module, so absurd keys are the only reachable trigger; revisit if instance naming is ever opened up. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
601f164c5f |
fix(seaweedfs): stop the naming guard guessing which volume generation holds the data
The guard told a class-S tenant from a D tenant by comparing PVC creationTimestamps,
on the premise that "PVCs are never recreated in place, so the older generation is
where the data was born". The premise is false, and this runbook is the counter-
example: Step 2's re-bind DELETES each release-named claim and recreates it under
the chart name against the same PV.
So a tenant interrupted part-way through Step 2 — after volume-0, before volume-1 —
has data1-seaweedfs-volume-0 created SECONDS ago holding real data, beside
data1-seaweedfs-system-volume-1 with the original timestamp also holding real data.
That is the exact inversion of the rule. Reproduced against the old template:
keeps its data on the release-named volume PVCs (created 2026-06-20T07:01:59Z,
OLDER than the chart-named data1-seaweedfs-volume-* claims created
2026-07-17T09:30:00Z) ... Remove the empty chart-named set ... (Step 2a, then Step 2)
Step 2a then runs `grep -E '^data1-seaweedfs-volume' | xargs kubectl delete pvc` —
deleting the claim Step 2 had just re-bound — and Step 2 had already restored the
PV's original reclaim policy, so on any Delete-policy StorageClass the volume and
the bytes go with it. The guard's own recovery path was a data-loss instruction.
Two more holes in the same classifier:
- The S-damaged branch was gated on $systemPVC, which requires the PVC name to
contain "seaweedfs". seaweedfs.componentName truncates the fullname to 56 chars
before appending -volume, so an instance name >= ~40 chars drops the chart name
off the PVC name — the very case fullname_override_named_test.yaml exists for.
Probed with `archive-of-quarterly-financial-statements-x1`: damaged + renamed set
Ready fell through to the D-split branch ("is running two live SeaweedFS sets ...
Stop the split"), sending the operator to quiesce the set holding ALL the data —
the exact misclassification the parent commit exists to remove. Damaged + not
Ready fired no branch at all and rendered through unguarded.
- readyReplicas == 0 was read as proof the duplicate never served. It is a
snapshot: a duplicate that crashed, was scaled down, or lost readiness AFTER
serving writes reads identically, and adoption strands whatever it wrote.
There is no sound signal here. Claim timestamps are mutable, StatefulSets are
recreated by the adoption hook, and liveness is a snapshot — nothing durable in the
object graph distinguishes "empty set a bad upgrade created" from "the data,
mid-re-bind". Rather than add a fourth heuristic, the guard now decides only what
is decidable:
legacy generation only (or nothing) -> render; adoption is correct
release-named generation only -> refuse, class S (Step 2)
BOTH generations -> refuse; an operator classifies
Both generation flags now OR the PVC and StatefulSet evidence, so the truncated
long-name tenant is seen via its label-matched StatefulSet.
The cost is bounded and lands only where it should. A duplicate exists only on a
tenant that passed through the broken 4.31 rename in 1.5.x — already damaged,
already needing an operator. A 1.4.x tenant upgrading straight to 1.6 never
renames, so it has one generation and is untouched. Confirmed on the stand:
tenant-l (1.4.5-era, never upgraded) is legacy-only and dry-runs clean; the four
both-generation tenants all already need an operator. What is lost is D-wedged
auto-adoption, which was resting on the readyReplicas assumption above.
Classification moves to the runbook, where a human has signals a template does not:
Step 1 now reads the ages of the PVs (durable — they survive the re-bind) via
.spec.volumeName instead of the claims, reports MIXED instead of picking, and walks
the operator through interrupted-Step-2 first, then strict age separation, then
verifying emptiness directly, then escalating rather than guessing. Step 2 stashes
the PV's original reclaim policy ON THE PV and leaves it Retain until Step 5
verifies the tenant, so a mis-step during recovery is survivable. Step 2a grows a
precondition that refuses unless every chart-named claim is bound to a strictly
newer PV.
Tests: 7 of 14 in the enforcing suite fail against the old template, including the
interrupted-Step-2 tenant and both truncated-long-name cases. hack/seaweedfs-guard-
parity.bats pins the two copies (system/ enforcing, extra/ sibling) against silent
drift — they cannot share a library, neither chart depends on cozy-lib, and "keep
these in sync" as a comment is the failure class this whole branch is about; it
also pins that creationTimestamp and readyReplicas never come back.
`make test` in both charts now runs helm template --is-upgrade and greps for the
refusal. The fail-closed canary — the headline safety property — had ZERO automated
coverage: guard_fail_closed_test.yaml passes with AND without the fix, because
helm-unittest 1.0.3 ignores release.isUpgrade. Verified the new target fails when
the canary is removed.
Review round 3 — the refusal was right; the justification was false and the
procedure that replaced the chart's judgement was broken on both axes.
THE COMMENT WAS WRONG. It said "nothing durable in the object graph distinguishes
the empty generation from the data mid-re-bind". A durable signal does exist, and
the reviewers were right to call that out: the Helm release history. Verified on
the stand — sh.helm.release.v1.<name>-system.v1 records the scheme the tenant was
BORN under (tenant-root/l/dsplit/named rev 1 render seaweedfs-master => pre-4.31;
tenant-fresh renders seaweedfs-system-master => 1.5.x), and info.first_deployed is
on every retained revision, so anchoring PV creationTimestamps against it survives
history pruning and the Step 2 re-bind (tenant-root: chart-named +10s vs
release-named +2084s; tenant-fresh: +5225s vs +6s).
BUT IT DOES NOT UNLOCK AUTO-ADOPTION, and that is the substantive disagreement
with the round-3 direction. Birth order answers "which generation is ORIGINAL".
Adoption needs "is the OTHER one EMPTY". Those come apart exactly where it
matters: D-wedged (duplicate never scheduled, safe to adopt) and D-split
(duplicate served writes, adopting strands them) are BOTH born pre-4.31 and are
therefore identical on every durable signal. Verified: tenant-root and
tenant-dsplit have the same rev-1 scheme and the same first_deployed deltas, yet
one duplicate served and the other did not. The only thing that ever separated
them was readyReplicas, which is a snapshot and not evidence. So the render keeps
refusing on presence, and the comment now says why — naming the signal, so nobody
re-derives the broken one.
The signal IS used where it belongs, in the audit.
- naming-guard.yaml / seaweedfs.yaml: match the release-named generation by
RECONSTRUCTING the name 4.31 gives this release's volume component
(templates/_naming.tpl, replaying seaweedfs.fullname + componentName), tested
FIRST because the prefixes nest. Fixes a blocker: an instance legitimately named
`seaweedfs-volume` renders seaweedfs-volume-system-volume and
data1-seaweedfs-volume-system-volume-0, which BOTH satisfy the chart-named
prefixes — a prefix-only guard read its live storage as legacy and would render
onto the empty chart-named claims, stranding the real generation. The parity
bats passed straight through it, because parity pins sameness, not correctness.
Reconstruction also removes the name-match blind spot at source: a truncated long
instance name now matches by PVC, not only by StatefulSet.
- hack/seaweedfs-naming-audit.sh + .bats: the classifier is a tested script now.
Two revisions of it shipped as an untested snippet inside the runbook and both
were wrong in ways that routed a live tenant into the step that strands its data:
a selector `^data1-(.*seaweedfs.*)-volume` that also matched the chart-named
claims, so the release-named range spanned BOTH generations, every tenant read as
"overlapping / mid-rebind", and a genuine S-damaged tenant was routed AWAY from
Step 2a into Step 2 — which deletes its data claim, fails to recreate it, then
deletes its StatefulSets; and a `grep seaweedfs` name match that reported
"L — nothing to do" for a long-named tenant the chart refuses. Both are covered.
The audit reports L/S/MIXED matching the guard, names the original generation
from both durable signals, detects MID-REBIND (both generations at
first_deployed), and states plainly that "original" is not "empty" and that
emptiness must be established from the duplicate's volume files before deleting.
A bats case renders _naming.tpl through helm and compares it to the script's
reconstruction, so the two implementations cannot drift.
- Runbook: Step 1 calls the script. Steps 3/4 were circular — Step 3 ended in an
upgrade the presence-gated guard refuses, and Step 4 told the operator to clear
duplicates AFTER an upgrade that cannot run until they are gone. Step 3 now
removes the duplicate BEFORE the upgrade and re-runs the audit to prove it; Step 4
starts from "every tenant reads L or S". D-wedged no longer claims to need
nothing. Step 3's "enumerate fids that resolve only on the duplicate" was not an
executable procedure and is now an explicit escalation: two masters allocate
volume IDs from independent sequences into one shared seaweedfs-db, so there is
no supported tool that reconciles two volume-ID spaces against one metadata
store. Step 2's re-bind restores the cleanup hook's labels on the recreated
claims, without which a recovered tenant leaks its volumes on a later delete.
Fixture corrected: data1-...-x1-system-volume-0 is not a name the chart renders;
componentName gives ...-x1-system-seaw-volume-0.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
|
||
|
Myasnikov Daniil
|
ae93356ed1 |
fix(seaweedfs): name cluster-scoped RBAC per namespace, not after the pinned fullname
ClusterRoles and ClusterRoleBindings are cluster-scoped: one object per name, cluster-wide. This chart installs once per tenant, so every cluster-scoped name it renders must be unique per install. Four of them were not. Upstream 4.31 switched them from global.serviceAccountName to seaweedfs.fullname. That is fatal in combination with the #3282 pin: fullnameOverride: seaweedfs is what adopts the running NAMESPACED workloads across the rename, so fullname is by construction the SAME string in every tenant. Every SeaweedFS instance in the fleet therefore renders seaweedfs-objectstorage-provisioner, seaweedfs-rw-cr and their bindings — one object, five claimants. NOT a #3282 regression. Rendering v1.4.5, v1.5.3 and main for the same tenants: v1.4.5 tenant-root-seaweedfs-* tenant-named-seaweedfs-* unique v1.5.3 seaweedfs-system-* foo-system-seaweedfs-* COLLIDES (identical for every default-named instance; a non-default instance name was the only thing that escaped) main seaweedfs-* seaweedfs-* COLLIDES (nothing escapes: the pin removes the last distinguishing input) So the collision shipped in 1.5.0 with the 4.31 bump; #3282 widens it from "every default-named instance" to "every instance". Observed live on the upgrade stand: one ClusterRole/seaweedfs-objectstorage-provisioner annotated to a single owning tenant, four other tenants' COSI provisioners left with no RBAC, and tenant-dsplit's helm history recording Upgrade failed: original object ClusterRole "seaweedfs-objectstorage-provisioner" not found The binding is the sharp end: its subject names a ServiceAccount in the release namespace, so the single live ClusterRoleBinding grants cluster-wide secrets RBAC to whichever tenant reconciled last, and silently revokes it from the rest. Fix: put the four names back on global.seaweedfs.serviceAccountName, which extra/seaweedfs already sets to <namespace>-seaweedfs and which cosi-provisioner-sa-name.patch already uses for the ServiceAccount and the binding's subject — the cluster-scoped names were simply left behind. Three of the four come out byte-identical to pre-4.31, so a 1.4.x tenant adopts them in place; the fourth (rw-crb) replaces upstream's username-shaped system:serviceaccount:<sa>:default and is pruned from the release's own manifest. The namespaced pin is untouched: workloads still render seaweedfs-master/-filer/ -volume, so #3282's adoption is intact. Uniqueness now rests on a value, so assert it rather than trust it: templates/cluster-scoped-rbac-guard.yaml refuses to render when the name would not carry the release namespace. It sits behind the same namespace canary as the naming guard, because system/seaweedfs's own values.yaml ships a placeholder that only extra/seaweedfs overrides — a client-side render must not trip on it. Tests fail on unfixed code: cluster_scoped_names_test asserts two tenants get two names, and unpatched both render seaweedfs-objectstorage-provisioner (5 failed, 0 passed). They test the vendored chart templates directly, which is where the rename happens. The guard includes seaweedfs.compat before reading. extra/seaweedfs sets the OLD flat key global.serviceAccountName, and the shim is what folds it into the canonical global.seaweedfs.serviceAccountName; without it the guard compares against this chart's placeholder default and refuses every real upgrade. Caught by helm upgrade --dry-run=server against the stand, not by unit tests — the mocks never carry the flat key. Verified live with --dry-run=server on the stand: tenant-l (installed pre-4.31, never upgraded) already owns tenant-l-seaweedfs-objectstorage-provisioner and tenant-l-seaweedfs-rw-cr, and the fixed chart renders exactly those — adopted in place, no ownership error. tenant-root renders tenant-root-seaweedfs-* and also dry-runs clean. Leftovers on the release-based names that a 1.5.x tenant passed through are inert once every tenant is upgraded (no release renders them any more) but are not always pruned, since more than one release claimed them; the runbook adds the sweep. Review round 2: - The uniqueness guard used `contains .Release.Namespace $sa`, a substring test that accepts namespace `tenant-a` for service account `tenant-ab-seaweedfs` — a different namespace's. Anchored to hasPrefix "<namespace>-". It still asserts the VALUE the names are built from, not the rendered names, so it cannot catch the chart being reverted to name them after the pinned fullname; cluster_scoped_names_test.yaml covers that axis and is what actually bites. - Added the case that would have caught the compat bug: every test set the CANONICAL global.seaweedfs.serviceAccountName, while production sets the OLD FLAT global.serviceAccountName that only seaweedfs.compat folds in. Deleting the include left the suite passing 5/5 while every real upgrade refused. The new case sets the flat key; with the include removed it now fails with the production error. - Runbook sweep missed the fourth pre-4.31 name: 4.31 renamed the master-rw binding from upstream's username-shaped system:serviceaccount:<sa>:default to <sa>-rw-crb, and the old one carries neither suffix nor a tenant prefix, so the grep could not see it (live on the stand as system:serviceaccount:tenant-l-seaweedfs:default). Also documents the rolling- upgrade window: Helm prunes by name without checking ownership, so the first tenant onto the fixed chart deletes shared objects a not-yet-upgraded tenant is still bound through — COSI provisioning 403s until they reconcile, S3 traffic unaffected. Review round 3: - hasPrefix "<namespace>-" was still too loose. It accepts `tenant-a-b-seaweedfs` for namespace `tenant-a` — which is ALSO the natural value for namespace `tenant-a-b`, so two namespaces would accept one string and collide on the very object this guard exists to keep unique. extra/seaweedfs sets exactly `<namespace>-seaweedfs`, so assert equality with it and nothing looser. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
8499b5f4a1 |
fix(seaweedfs): move the naming-migration guard into the render that actually renames
The class-S guard from #3282 lives in extra/seaweedfs, but that render is not in the path of a platform upgrade: the <name>-system HelmRelease pulls system/seaweedfs from a platform-managed ExternalArtifact, so bumping the platform upgrades the release directly from the new artifact and applies fullnameOverride: seaweedfs without extra/seaweedfs ever re-rendering. On a tenant installed fresh on 1.5.x that stood up an empty chart-named set beside the live release-named data, deleted the renamed s3 Service, and left S3 readable only because the empty set's pods cannot start. Worse, the damage flips the guard's own classification: creating the chart-named artifacts makes $legacyData true, so the guard then reports D-split and sends the operator to runbook Step 3 — which quiesces the renamed set, i.e. the one holding ALL the data. - Add the ENFORCING guard to system/seaweedfs (templates/naming-guard.yaml): the only render guaranteed to sit between a platform upgrade and the tenant's workloads. It also refuses to render if the fullnameOverride pin is ever lifted. - Tell S from D by PVC AGE when both generations exist: PVCs are never recreated in place (StatefulSets are, by the adoption hook), so the older volume-PVC generation is where the data was born. Renamed-older ⇒ class S (damaged variant, new Step 2a), never D-split. - Fail closed: the release namespace is a lookup canary; an upgrade that cannot see the cluster refuses instead of classifying blind. Client-side installs (CI lint/unittest) still render — they never touch live data. - Mirror the same classification in extra/seaweedfs so the operator-facing SeaweedFS application reports the correct class. - Tests now model the ExternalArtifact upgrade path in the chart it actually runs through (the old ones mocked lookups only in extra/seaweedfs, which is why this shipped green), including the damaged-S tenant observed live. - Runbook: add S-damaged audit classification (PVC age), Step 2a cleanup of the empty chart-named set, and the D-wedged anti-affinity stall workaround observed on 3-node clusters. helm-unittest 1.0.3 ignores release.isUpgrade, so the canary refusal is verified with 'helm template --is-upgrade' instead of a unit test. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
myasnikovdaniil
|
df0da594c0 |
fix(metrics-server): move the listener off the kubelet's port (#3361)
## What this PR does metrics-server is the third component serving on port 10250 — the kubelet's port — after the webhooks addressed in #3359 and #3360. It ships default-enabled in the platform bundle and is also installed into every tenant Kubernetes cluster with no value overrides, so a package-root override propagates to both. The hard defect here is different from the webhook cases. The chart has first-class `hostNetwork.enabled` support, documented upstream for clusters where the API server cannot reach pod IPs — and on the default port that mode cannot start at all: the kubelet already holds 10250, so the process dies with `bind: address already in use`. Upstream is aware (kubernetes-sigs/metrics-server#1026 carries the exact panic) and declined to change the chart default, advising hostNetwork users to set their own port. This override is that port, set ahead of need. The misleading-certificate angle of the sibling fixes mostly does not apply: the chart defaults `apiService.insecureSkipTLSVerify` to true, so a connection misrouted to a node would fail silently rather than with a wrong-certificate error — and with endpoints resolving to pod IPs such a misroute is not established to occur here at all. The comment states that as a hypothesis, not a fact. What is factual is the consumer set: `kubectl top`, tenant HPAs, and the console's cluster-usage page all read `metrics.k8s.io`; the shipped VPA does not — it is configured with Prometheus storage. The port is 4443 rather than the siblings' 10260, for a reason worth keeping: metrics-server is the one component of the three with supported hostNetwork, and parking it on 10260 would put a live host-network listener on exactly the port the webhook fixes rely on being silent — turning their honest connection refusals back into answered handshakes. 4443 is the value metrics-server's own manifests shipped through v0.6.4, before v0.7.0 moved them to 10250; it sits outside the 10248-10259 node-component block, and nothing in the tree binds it. No chart patch is needed: the Service, both probes and the ServiceMonitor already reference the port by name, so the rollout-gap that required a vendored patch in #3360 does not exist here. The package also gains the standard `test:` target it was missing. Not verified without a cluster: that the aggregated APIService stays Available through an upgrade, and that no supported substrate filters 4443 inbound on nodes — the tree configures no host firewall, but Talos and managed substrates are outside what a render can prove. There is no e2e coverage of `metrics.k8s.io` at all; a wrong guess here would surface as HPAs silently not scaling. ### Release note ```release-note fix(metrics-server): move the listener off port 10250, which the kubelet also holds. The default made hostNetwork mode unable to start at all, and the new port keeps node-port collisions out of the metrics path. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Configured the metrics server to listen on secure port 4443. * Updated service routing and health checks to use the named HTTPS port, improving reliability when port settings change. * Ensured pod networking does not rely on host networking. * **Tests** * Added automated chart tests covering container ports, service routing, and liveness/readiness probes. * Added a test command for running the chart test suite. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
myasnikovdaniil
|
8af4c1d600 |
fix(external-secrets): move the webhook off the kubelet's port (#3360)
## What this PR does The external-secrets webhook listens on port 10250 — the kubelet's port — with `failurePolicy: Fail`. This is the same trap being removed from cert-manager in #3359: a connection to the webhook Service that resolves to a node IP instead of the pod IP reaches the kubelet, which completes the handshake with its own node serving certificate, and the API server rejects the call with an x509 error that blames the webhook's PKI rather than the routing layer. The port collision does not cause the misroute; it turns a rare transient one into a confidently misattributed failure. The blast radius here is wider than admission alone: `crds.conversion.enabled` defaults to true and points every ExternalSecret CRD's conversion webhook at the same Service, so a misrouted connection also breaks CRD conversion — reads at non-storage versions, not just writes. The webhook moves to 10260, matching #3359. The controller-runtime default 9443 was considered and rejected: the etcd kube-rbac-proxy binds it on host network, which would reproduce exactly the same wrong-certificate failure with a different certificate. Nothing in the tree binds 10260. Unlike cert-manager, the upstream chart's Service targets the port numerically — `targetPort: {{ .Values.webhook.port }}` — and a numeric targetPort is copied into every endpoint verbatim rather than resolved per pod. With one replica, a port change would direct traffic at a port the still-running old pod does not serve while the replacement is not yet Ready, failing every admission and conversion call in the gap; surge-first rollout does not help because both pods sit behind the same number. A one-line vendored-chart patch points the Service at the `webhook` container-port name upstream already declares (the metrics port in the same Service is already named), which closes the gap: during the overlap each pod is routed on the port it actually serves. The patch is applied by `make update` with `--forward`, so an already-applied hunk fails loudly instead of being silently reversed. Upstream has already made the same change — merged as external-secrets/external-secrets#5108 and shipped since chart 0.19.1 — but the vendored chart here is 0.10.4, nine minor releases earlier, so the patch carries the fix until the chart is bumped past that point. The `--forward` guard is exactly what will flag that moment: on a bumped chart the patch fails loudly, signalling it should be dropped. external-secrets-operator ships disabled by default and is enabled per-cluster via `bundles.enabledPackages`, so this is defensive hardening for opt-in installs rather than a response to an observed incident. `metrics-server` is a third instance of the same port collision, left out here: it serves an APIService rather than an admission webhook, a lesser failure class deserving its own change. ### Release note ```release-note fix(external-secrets): move the webhook off port 10250, which the kubelet also serves, and make its Service target the port by name so the move rolls out without an admission gap ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved webhook Service routing by switching to the named `targetPort` (`webhook`) for pod traffic. * Configured the webhook listener to run on port **10260** to avoid prior connection/TLS certificate misrouting issues. * Preserved admission validation behavior and CRD conversion routing. * **Tests** * Added Helm chart regression tests covering webhook port wiring, networking settings, failure policy behavior, and CRD conversion. * **Chores** * Added Helm chart unit testing to the development workflow and extended the chart update process to apply the webhook routing adjustment. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Myasnikov Daniil
|
955f7ccaa6 |
fix(opensearch-operator): use RollingUpdate maxSurge=0 instead of Recreate
#3040 set the single-replica operator Deployment to strategy.type: Recreate to avoid two uncoordinated managers overlapping during a rollout while leader election is gated off. That breaks Helm upgrades of existing releases: the upgrade fails validation immediately, helm-controller retries it (x19 over 11m in Actions run 29480724982), and the HelmRelease never becomes Ready: Deployment.apps "opensearch-operator-controller-manager" is invalid: spec.strategy.rollingUpdate: Forbidden: may not be specified when strategy type is Recreate The trigger is server-side apply. A v1.5.x release shipped no strategy at all, so the apiserver defaulted spec.strategy.rollingUpdate to 25%/25%, and the helm-controller field manager never owned that block. helm-controller applies server-side by default (Install.ServerSideApply defaults to true, Upgrade.ServerSideApply to "auto"; the operator's package_reconciler.go sets neither), and SSA does not remove a field the applier never owned merely because the new intent omits it -- so sending type: Recreate leaves the defaulted rollingUpdate in place and the apiserver rejects the merged object. The client-side path is unaffected: DeploymentSpec.Strategy carries patchStrategy:"retainKeys", so a 3-way merge emits {"$retainKeys":["type"],"type":"Recreate"} and does clear rollingUpdate. That is why this surfaces through helm-controller's server-side apply but not a local 'helm upgrade'. Keep type: RollingUpdate and set rollingUpdate.maxSurge: 0 / maxUnavailable: 1 on the single-replica path instead. maxSurge: 0 caps total pods at replicas, so the old manager is scaled down before the new one is scaled up, rather than starting a surge pod while the old manager is still fully live as the default 25% would. It is not Recreate's exclusivity -- the deployment controller orders the ReplicaSet desired counts, not the pod lifecycles, so a draining old manager can still overlap the new pod -- but it keeps the type transition legal on every apply path, and recovers releases already on an rc build with Recreate. This matches the existing kube-ovn idiom (central-deploy.yaml). Also add a 'test' target to the package Makefile: hack/helm-unit-tests.sh runs a package's suite only when its Makefile defines one, so tests/leader_election_test.yaml had never executed. Multi-replica installs keep the default RollingUpdate (leader election on), unchanged. The change is carried in patches/leaderElection.diff so it survives 'make update'. Verified on kind v1.33.1: SSA of the v1.5.x manifest followed by SSA of the Recreate manifest reproduces the error, while the same pair applied client-side succeeds; SSA of the maxSurge:0 manifest succeeds both from v1.5.x and from a live Recreate Deployment. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Aleksei Sviridkin
|
aa2525a8d8 |
fix(metrics-server): move the listener off the kubelet's port
metrics-server scrapes every kubelet on 10250 and, by upstream default, serves its own aggregated API there too. Sharing that number makes hostNetwork unusable. The chart offers it for clusters whose apiserver cannot reach Pod IPs, but at the default port the process joins the node namespace where the kubelet already holds 10250 and exits with "bind: address already in use". Upstream has had this reported and kept the default anyway, on the view that hostNetwork is non-standard here and that those users can set a port themselves. It also decides how a misrouted connection would present. Were a connection to the Service to arrive at a node IP rather than a Pod IP it would reach the kubelet, and because the APIService sets insecureSkipTLSVerify the aggregation layer would never check whose certificate it got, leaving nothing in the error to name the wrong server. Nothing here establishes that such a misroute occurs; moving the port only makes one fail as a connection refusal rather than as a rejection from an unexpected daemon. The override gives up the firewall coverage 10250 buys on substrates permitting only 443 and 10250 to nodes. Cozystack's own do not filter it, but this package ships into every tenant cluster and no e2e exercises the metrics API. 4443 is the value upstream's manifests carried through v0.6.4, before v0.7.0 shipped 10250. It sits clear of the 10248-10259 block the node components occupy and nothing else in this repo binds it. The Service's targetPort and both probes reach the container by name, so the number moves without a rollout gap. The tests pin that alongside the value, since the vendored chart ships no schema that would reject an upstream rename of the key. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
a90fa38fcb |
fix(external-secrets): move the webhook off the kubelet's port
The chart defaults the validating webhook to port 10250, which the kubelet also serves on. When a connection to the webhook Service resolves to a node IP rather than a Pod IP it reaches the kubelet, which completes the TLS handshake with its node serving certificate, and the apiserver rejects the call with an x509 error naming the node instead of the webhook Service. The certificate is valid, just the wrong server's, so the failure reads as a PKI fault and hides the misroute behind it. Moving the listener to a port nothing else answers on turns the same misroute into a connection refusal, which names the real problem. This does not prevent the misroute. 9443, the controller-runtime default this chart would otherwise take, is disqualified: the etcd kube-rbac-proxy binds it on host-network, so it would swap one wrong certificate for another. Nothing in the tree binds 10260 in a node network namespace. It remains CloudControllerManagerWebhookPort in k8s.io/cloud-provider, so a host-network cloud-controller-manager added later has to keep clear of it. Point the Service at the "webhook" container-port name so the move is safe. Upstream renders targetPort from webhook.port as a number, which every endpoint shares regardless of what its Pod listens on, so changing the port alone would aim the Service at a port the outgoing Pod does not serve until the replacement passes readiness. That window rejects admission for externalsecrets, secretstores and clustersecretstores, and breaks CRD conversion, which routes through the same Service. No values-only setting closes it; a named targetPort resolves per-Pod and does. Add unit tests pinning the listener flag, the containerPort and the named targetPort. This chart's values.schema.json allows unknown keys, so a renamed upstream value would be silently ignored and the port would revert to 10250 with nothing else noticing. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
df85632c89 |
fix(cert-manager): move the webhook off the kubelet's port
The vendored chart defaults webhook.securePort to 10250, which is the port the kubelet serves on. Upstream picks that value deliberately, for GKE private clusters where apiservers may reach nodes only on 443 and 10250; it is free of consequence there and unsafe here. When a connection to the webhook Service resolves to a node IP instead of a Pod IP, it reaches the kubelet, which completes the TLS handshake with its node serving certificate. The apiserver then rejects every cert-manager admission call in the cluster with x509: certificate is valid for <node>, not cert-manager-webhook.cozy-cert-manager.svc and, because the webhook is failurePolicy: Fail, that stalls platform bootstrap. The certificate is valid; it is simply the wrong server's. The error names cert-manager and points away from the routing layer that actually failed, which makes a rare misroute expensive to diagnose. Serve on 10260 instead, the port upstream's troubleshooting guide prescribes for this exact error. It documents the failure on EKS Fargate, where a microVM forwards every port to the pod except the one its own kubelet already holds; the cause there is understood and ours is not, but the shape is the same, down to the certificate that comes back naming the node. What the fix needs of the port is that nothing answers on it in a node's network namespace, and nothing in the platform does. The same misroute is then refused at connect time, which is an honest and immediately diagnosable error. This does not prevent the misroute, whose cause is unestablished; it removes the collision that turns one into a cluster-wide failure attributed to the wrong component. The Service targets the container port by name, so it follows the change without an edit, and a rolling update resolves the old and new ports per Pod rather than dropping the endpoint. Tests pin the listener flag together with the advertised containerPort so the two cannot drift apart. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Timofei Larkin
|
7475491686 |
fix(keycloak): point HTTPRoute backends at the service port
The keycloak-http Service only exposes port 8080, but every HTTPRoute backendRef referenced port 80. The Gateway API resolves backendRefs against the Service's declared ports, so the route had no valid backend and envoy answered 503 for all Keycloak hostnames whenever gateway-enabled is true. The Ingress template is unaffected: it references the port by name (http). Signed-off-by: Timofei Larkin <lllamnyp@gmail.com> |
||
|
Andrey KolkovandClaude Fable 5
|
67db8f7da7 |
fix(backups): make the barman-cloud serverName stable across every flow
Two defects of the same class — the WAL-archive S3 prefix (serverName) changing or defaulting differently between components: Recovery default regression (review blocker): native CNPG defaulted an absent externalClusters serverName to the entry name (= bootstrap.oldName), but the barman-cloud plugin defaults it to the NEW cluster's name, so a legacy-flow restore with bootstrap.serverName unset would read the wrong S3 prefix (hard failure, or silently restoring another cluster's data). The chart now always emits serverName, defaulting to bootstrap.oldName. Live-cluster serverName flip (found by the gated postgres backup e2e on a real cluster): the chart renders spec.plugins without serverName (effective prefix = cluster name) while a strategy template may name a different one; the driver's ForceOwnership patch then flipped the prefix mid-stream, splitting the WAL archive across two prefixes — the WAL holding the backup's start was lost between them and the restore failed with "WAL not found". The driver now preserves the live Cluster's effective serverName (explicit parameter, or the cluster name when the plugin is attached without one) and only applies the strategy's value to a Cluster with no plugin attached; both charts pin serverName explicitly, and the example strategy no longer sets one. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey KolkovandClaude Opus 4.8
|
a1c135c2a1 |
fix(backups): omit empty retentionPolicy from the ObjectStore
Review follow-up (cozystack#3300, non-blocking): the barman-cloud ObjectStore CRD
validates spec.retentionPolicy against ^[1-9][0-9]*[dwm]$, so an operator who
explicitly sets backup.retentionPolicy="" would render retentionPolicy: "" and
the API server would reject the ObjectStore, breaking backup provisioning. Guard
the field in the apps/postgres and keycloak chart ObjectStores with
{{- if .Values.backup.retentionPolicy }} (the Go driver path is already safe via
omitempty). Add a helm-unittest asserting an empty retentionPolicy is omitted.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Andrey Kolkov <androndo@gmail.com>
|
||
|
Andrey KolkovandClaude Opus 4.8
|
5fbc90047d |
docs(apps/postgres): modernize backup annotations/README for the plugin flow
Review follow-up (cozystack#3300): the apps/postgres user-facing docs still described the removed native spec.backup.barmanObjectStore / archive_command model. Update the values.yaml @field/@typedef/@param annotations (useSystemBucket, schedule, S3CredentialsSecret, EndpointCA, endpointCA, the @param backup note, bootstrap.serverName) and the hand-written README Operations prose to the barman-cloud plugin + ObjectStore model, then regenerate values.schema.json, the README parameters table, the api types, and the postgres-rd ApplicationDefinition schema (which feed the dashboard form help + cozyrd schema). Field names, defaults, and the storageClass immutability marker are unchanged; only backup/ bootstrap description text changed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey KolkovandClaude Opus 4.8
|
2212367842 |
fix(backups): own driver-created ObjectStore + correct stale barman references
Review follow-ups (cozystack#3300): - Lifecycle/leak: applyClusterPluginBackup now sets an ownerReference from the driver-created barmancloud.cnpg.io/ObjectStore to its cnpg.io/Cluster, so Kubernetes garbage-collects it when the Cluster is deleted (tenant app teardown or the RestoreJob purge). In the platform useSystemBucket flow the chart does not render this ObjectStore, so previously it was orphaned in the tenant namespace. Drop the now-accurate-only 'delete' verb on objectstores from the driver RBAC (GC handles removal; the driver never deletes it) and correct the RBAC comment that falsely claimed the purge deletes it. - Diagnostics/comments: the RestoreJob WAL-archive-deadline failure message and several doc comments still told operators to check spec.backup.barmanObjectStore / externalClusters[].barmanObjectStore, fields no longer emitted after the migration. Point them at the barman-cloud plugin (spec.plugins + ObjectStore, externalClusters[].plugin) instead so they don't mislead during an incident. - Test: TestApplyClusterPluginBackup_PatchesExistingCluster now asserts the ObjectStore carries an ownerReference to the Cluster (kind/name/uid). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey KolkovandClaude Opus 4.8
|
39bd8623c7 |
fix(keycloak): back up keycloak-db via the barman-cloud plugin
The keycloak-db backup added in #3174 uses the in-tree native spec.backup.barmanObjectStore, whose barman-cloud-* binaries ship only in the CNPG system image variant; keycloak-db is pinned to the standard variant, so base backups fail with 'barman-cloud-backup: executable file not found'. The proposed workaround (#3306) switches keycloak-db to the deprecated system image variant when backups are enabled. Migrate keycloak-db to the barman-cloud plugin instead, matching the rest of this PR: render spec.plugins referencing a barmancloud.cnpg.io ObjectStore (the S3/barman config moves there) and a method=plugin ScheduledBackup, keeping the Cluster on the standard image (the plugin runs the barman tooling in a sidecar). This removes the need for the system-variant workaround entirely, so #3306 can be closed. cozystack#3300. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey Kolkov
|
13ff340277 |
fix(backups): migrate CNPG backups to the barman-cloud plugin
Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Aleksei Sviridkin
|
a5cabca73e |
fix(ouroboros): render controller rollout strategy in every mode (#3323)
## What this PR does Bumps the ouroboros chart and controller image to 0.8.1, which fixes a latent upgrade break in the controller Deployment's rollout strategy, and pins the fixed contract from the cozystack side. Up to chart 0.8.0 the controller Deployment rendered a rollout strategy **only** in `external-dns` mode, as `type: Recreate`, and rendered no strategy block at all in the other modes. The apiserver defaults a `rollingUpdate` block onto any Deployment created without one, so switching a live release into `external-dns` laid `Recreate` on top of that defaulted block, and the apiserver rejected the object: ``` spec.strategy.rollingUpdate: Forbidden: may not be specified when strategy `type` is 'Recreate' ``` 0.8.1 renders the same `RollingUpdate` block, with no surge, in every mode. The strategy no longer changes when `controller.mode` does, so the rejected combination cannot arise on any transition. Rendering `Recreate` unconditionally would not have worked: existing `coredns`-mode releases already carry the defaulted `rollingUpdate` block and would hit the identical rejection on their first upgrade. Always-`RollingUpdate` is upgrade-safe from both existing states, and rollback works for the same reason. **Reachability, stated plainly:** this is a latent break, not an active one. The host pins `coredns` and tenants pin `coredns-import`, neither of which rendered a strategy in 0.8.0, and no shipped configuration selects `external-dns`. But `controller.mode` is a documented, user-settable value that the tenant wrapper exposes through `valuesOverride`, so the transition is reachable by configuration rather than only in theory. **Why it went unnoticed:** the break only surfaces under server-side apply, which helm-controller uses by default. `DeploymentSpec.Strategy` carries `patchStrategy:"retainKeys"`, so a client-side strategic merge clears `rollingUpdate` on the way through and a plain `helm upgrade` never reproduces it. Only the upgrade breaks, never the install, so nothing catches it at render time either. **Why no surge, in every mode:** the controller has no leader-election in any mode and no serving path — no ports, no probes — so surging a second pod buys no availability anywhere. In `external-dns` mode it also removes a real hazard: that reconciler `Update`s DNSEndpoints with no conflict retry, so two overlapping reconcilers race on Get→Update. The `coredns` paths wrap Get→Update in `RetryOnConflict` and converge instead, which is why only `external-dns` rejects `replicaCount > 1`; that asymmetry is deliberate and is now called out in the template so it does not get tidied away later. The upstream comment is explicit that `maxSurge: 0` is *not* Recreate-equivalent exclusivity — the surge budget counts desired replicas, not live pods — and rests the argument on what is actually guaranteed: the old pod's SIGTERM precedes the new pod's creation, and SIGTERM cancels the reconciler context. The proxy Deployment is untouched and keeps its default rolling surge, as the data-path component. The fix was made upstream in `lexfrei/ouroboros` and released as v0.8.1 rather than patched into the vendored tree here, since `make update` regenerates `charts/` wholesale. The vendored chart in this PR is a verbatim `make update` pull of the published 0.8.1 artifact, byte-identical to the registry. Chart manifest digest and controller image digest are bumped in lockstep with the version, as the package Makefile requires. 0.8.1 also carries an upstream Renovate bump of the cleanup-hook kubectl image to 1.36.1 (verified to resolve, multi-arch). The `update` target additionally drops the empty digest-named directory that `helm pull` leaves beside the untarred chart when pulling by digest. Git ignores empty directories, which is exactly why it lingered unnoticed. ### Testing `make -C packages/system/ouroboros test` → 17/17 pass. The three new cases pin the rendered strategy on `coredns`, `coredns-import` and `external-dns`. `external-dns` is included deliberately: it is the only mode that ever rendered `Recreate`, so a future bump that re-gated the block would slip past a coredns-only suite. Verified by re-gating the bug surgically against the new suite — it fails on exactly that case, and passes without it. Upstream additionally tests this contract across every mode in its own suite. ### Screenshots Not applicable — no UI change. ### Downstream repositories Walked the trigger map in `docs/agents/contributing.md` against the diff file by file. The diff touches only `packages/system/ouroboros/`: the package Makefile's version and digest pins, the vendored chart (regenerated), the image pin in `values.yaml`, and the package's own tests. It adds, renames and removes no package under `packages/apps/` or `packages/extra/`; touches no `packages/core/platform/values.yaml` key, no platform variant, bundle or component; changes no `ApplicationDefinition`, no `values.schema.json`, no version enum, no `release.prefix`; touches nothing under `hack/`, no namespace, and no node prerequisite. No trigger matches. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note ```release-note fix(ouroboros): render the controller rollout strategy in every controller.mode. Up to chart 0.8.0 the strategy was rendered only in external-dns mode, as type: Recreate, so switching a live release into that mode applied Recreate on top of the rollingUpdate block the apiserver defaults onto a Deployment created without one, and the upgrade was rejected with "spec.strategy.rollingUpdate: Forbidden: may not be specified when strategy `type` is 'Recreate'". The rejection only occurs under server-side apply, which helm-controller uses by default. The strategy is now identical across all modes, so a mode switch changes no strategy field. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Updated Ouroboros to version 0.8.1 with refreshed, pinned images. * Improved controller rollouts using a consistent rolling-update strategy across supported modes. * Updated cleanup hook tooling to version 1.36.1. * **Bug Fixes** * Prevented leftover chart directories from accumulating during chart updates. * Improved deployment behavior by limiting rollout disruption while maintaining controller availability. * **Tests** * Added coverage validating rollout behavior across all supported controller modes. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
myasnikovdaniil
|
4a00b502d7 |
fix(seaweedfs): adopt legacy workloads in place across the 4.31 rename (#3282)
## What this PR does Fixes a v1.5.0 upgrade regression where a tenant ends up with **two** SeaweedFS workload sets. **Root cause.** v1.5.0 bumped the vendored SeaweedFS chart `4.0.405 → 4.31.0`. Upstream changed resource naming: before 4.31 the chart named workloads after the **chart** (`seaweedfs-*`, ignoring the release name), and 4.31 names them after the **release**. The data-plane release is `<name>-system`, so every StatefulSet wanted to become `seaweedfs-system-*`. StatefulSet names are immutable, so Helm could not rename in place — it stood up a second, empty set beside the running one while the data stayed on the original `data1-seaweedfs-volume-*` PVCs. Where the node count let both sets run, they shared one `seaweedfs-s3` Service (identical pod labels) and one `seaweedfs-db` filer metadata database, so the empty set answered reads and the two master sets mixed volume IDs from independent sequences into one metadata table. **Fix.** Pin `fullnameOverride` in `system/seaweedfs` values so the rendered names stop following the release name, which is the convention already used by `cozy-proxy`, `flux-operator`, `grafana-operator`, `linstor-scheduler` and `victoria-metrics-operator`. The names go back to what every pre-1.5 cluster already runs, so the upgrade adopts the running workloads **and their volumes** in place. No data migration, no platform migration. **The one case that cannot be adopted.** A tenant installed *fresh* on 1.5.x wrote its data under the renamed workloads, so its volumes are not the chart-named ones. Pinning the chart name there would rename the workloads *away* from that data and bring up an empty cluster beside it. Helm cannot move data between PVCs, so `extra/seaweedfs` refuses to render for such a tenant and points at the recovery runbook, which re-binds the existing PVs onto the adopted PVC names (no copying — the claim is renamed, the volume is not touched). The guard is fail-closed: it fires only when renamed volumes are present and no adopted ones are. Two naming subtleties the guard has to respect, both verified by rendering the shipped 1.5 chart: - Before 4.31 the chart ignored the release name, so an instance running under a non-default name (`SeaweedFS` is a user-creatable kind) *also* stored its data on `data1-seaweedfs-volume-*`. Such a tenant is adopted, not blocked. - On 4.31 the fullname helper appends the chart name when the release name does not contain it, and the component helper truncates to 56 characters. A fresh 1.5.x instance named `archive` therefore wrote to `data1-archive-system-seaweedfs-volume-*`, and a long name loses the chart name from its tail entirely. The guard matches volumes by shape and by the chart labels on the StatefulSet instead of reconstructing those names, so both are caught. **On the duplicate set.** Once the fix reconciles, the `seaweedfs-system-*` StatefulSets and Deployments are no longer part of the release and Helm removes them on its own. What it cannot remove — PVCs it did not template, and cert-manager Secrets that carry no owner reference — is inert, and the runbook clears it explicitly. A pre-upgrade migration was considered and dropped: it would duplicate what Helm already does, while its only unique action (deleting PVCs) cannot distinguish "the duplicate never served" from "it served and is currently down", and any transient delete failure under `set -e` would fail the platform's pre-upgrade hook and block delivery of this very fix. ## Testing `helm unittest` — `system/seaweedfs` 21 tests, `extra/seaweedfs` 17 tests. The guard's `lookup` branches are covered by mocking the Kubernetes provider (the pattern already used in `core/platform` and `system/cozystack-api`): adopted-only, renamed-only, both sets, zoned volumes, a non-default instance name, a truncated long name, and an unrelated `data1-*-volume-*` PVC that must not trip the guard. The s3-consumer suite now re-creates the name/fullname divergence explicitly, so it keeps guarding that regression regardless of the shipped default. The remaining regression test for this class of bug is an upgrade E2E (seed a pre-4.31 named set, upgrade, assert a single set with data intact) — no fresh-install E2E or `helm template` can reproduce an upgrade over pre-existing state. That belongs in the chainsaw upgrade lane and is a follow-up. ### Screenshots N/A — no UI changes. ### Release note ```release-note fix(seaweedfs): adopt existing SeaweedFS workloads and volumes in place when upgrading past the 4.31 chart rename, instead of standing up a second, empty cluster beside them ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added an audit & recovery runbook for SeaweedFS 4.31 rename impacts. * **Bug Fixes** * Stabilized SeaweedFS workload/PVC naming across upgrades, including non-default instance names. * Added preflight safeguards to prevent upgrade scenarios that could split or wedge existing data. * Updated post-delete cleanup to remove secrets from both legacy and current naming schemes. * **Tests** * Expanded Helm unit coverage for naming guards, cleanup behavior, and S3 service name consumers. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
0238231303 |
fix(linstor-scheduler): re-vendor to upstream 0.3.1 with native admission webhook (#3201)
## What this PR does Re-vendors the `linstor-scheduler` chart from upstream **0.3.1** (appVersion **v0.3.6**). Upstream now ships the admission webhook natively (contributed via [piraeusdatastore/helm-charts#94](https://github.com/piraeusdatastore/helm-charts/pull/94)), so the loose downstream admission templates — which round-tripped every Pod through an old `k8s.io/api` and silently stripped fields they did not know about, the `image` volume source and the native-sidecar `restartPolicy` among them, breaking KubeVirt containerDisk VMs and native sidecars cluster-wide — are dropped in favour of the upstream copy. - The admission webhook comes from the upstream chart now; the previous downstream copy and its patch are removed. `make update` pulls 0.3.1 and applies only `cozystack-scheduler-deployment.patch` (the scheduler pod component label and the distro-suffix-stripped kube-scheduler image tag, neither of which is upstream yet), so vendoring stays deterministic and reproducible. - The upstream admission Deployment keeps the same name but changes its immutable pod selector, so an in-place upgrade of an existing install would fail with `field is immutable`. Migration 52 deletes the old admission Deployment before the upgrade so the new one is recreated cleanly (`targetVersion` 52 → 53). - appVersion v0.3.6 also carries the webhook's TLS certificate hot-reload fix ([piraeusdatastore/linstor-scheduler-extender#22](https://github.com/piraeusdatastore/linstor-scheduler-extender/pull/22)): the webhook used to load its certificate once at startup, so after the mounted Secret was rotated it kept serving a stale certificate and — with `failurePolicy: Ignore` — silently stopped mutating until it was restarted. It now reloads on rotation, so no external cert-reload workaround is needed here. - Wrapper values keep `replicaCount: 2`, now that the admission Pods have their own PodDisruptionBudget and no longer share the scheduler's. - Adds a helm-unittest suite covering the admission resources, PodDisruptionBudget selector isolation, least-privilege RBAC, the TLS provisioning modes and their rejection paths, the scheduler image-tag strip, and backward compatibility with the webhook disabled. Closes #3198. ### Screenshots N/A — no UI changes. ### Downstream repositories I walked the trigger map against the diff, file by file. It touches `packages/system/linstor-scheduler/` and, under `packages/core/platform/`, only migration 52 and the `migrations.targetVersion` bump — an internal, release-managed counter, not a user-facing `spec.components.platform.values.*` key documented on the website. No trigger matches. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note ```release-note fix(linstor-scheduler): the LINSTOR scheduler admission webhook no longer strips unknown Pod fields (the `image` volume source and native-sidecar `restartPolicy`), which had broken KubeVirt containerDisk VMs and native sidecars, and it now reloads its TLS certificate after rotation instead of silently failing. The chart is re-vendored from upstream 0.3.1; an automatic upgrade migration replaces the admission Deployment whose immutable selector changed. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Expanded the optional Linstor Scheduler admission webhook with richer configuration, including automated `schedulerName` mutation for LINSTOR CSI-backed PVC Pods. * **Improvements** * Updated the Linstor Scheduler Helm chart to **0.3.1** (scheduler app **v0.3.6**), including updated admission webhook behavior, safer defaults, and **2 replicas**. * Platform migrations now advance to **version 53**, with upgrade-safe cleanup of the existing admission deployment. * **Documentation** * Updated Helm README and post-install notes with detailed guidance for admission webhook and TLS modes. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Andrei Kvapil
|
fbbe4af7df |
refactor(network): drop ExposureClass/ServiceExposure for native loadBalancerClass (#3218)
## What Removes the `network.cozystack.io` API group — `ExposureClass` + `ServiceExposure` (#3081) — and its reconciler, in favor of native `Service` `type: LoadBalancer` + `loadBalancerClass`. ## Why Full rationale and discussion in #3164. For **managed applications** the chart already owns the Service, so: - `type: LoadBalancer` + `loadBalancerClass` + an admin-provisioned pool covers vendor-neutral external exposure without a dedicated API group and controller; - the `externalIPs` (node-IP) default never went through `ServiceExposure` (its backend is a no-op) and is left unchanged; - the only current consumer, the host ingress, keeps rendering `type: LoadBalancer` and just drops the `ServiceExposure`; the admin provisions the address pool / announcer out of band, as the install docs already assume. `ServiceExposure`'s one unique feature — dynamic per-namespace scoping of a shared VIP pool — is a convenience the managed-app path does not need, and it does not justify a second tenant-networking API group next to `sdn.cozystack.io`. ## Changes - Delete `api/network/v1alpha1` and `internal/controller/serviceexposure` (+ CRD definitions, e2e bats, controller registration, RBAC). - Host ingress: drop the `ServiceExposure` template; add an optional `publishing.loadBalancerClass` that sets `spec.loadBalancerClass` on the host-ingress LoadBalancer Service. - The `externalIPs` path and default behavior are unchanged. ## Verification - `go build ./...` — green. - `helm unittest` — `core/platform` 72/72, `extra/ingress` 25/25 (incl. host-vs-tenant `loadBalancerClass` scoping and the immutable-field migration guard). ## Migration None required. `network.cozystack.io` (#3081) has never shipped in a release — it is on `main` only (not backported to `release-1.5`; absent from v1.4.x / v1.5.x), so no upgrading cluster has these CRDs and this removes it before it is ever released. Clusters that track `main` and explicitly set `publishing.exposureClass` (opt-in, only since 2026-06-30) should provision their MetalLB / Cilium address pool themselves and set `publishing.loadBalancerClass`; any orphaned `cozystack-<class>` pool can be deleted by hand. Default clusters (externalIPs path) are unaffected. ## Status **Draft / RFC for the #3164 discussion** — this removes an API group that landed only days ago, so nothing here should merge before the design call. cc @lexfrei (author of #3081) and @lllamnyp (#3164). Deferred: when Kubernetes actually removes `Service.spec.externalIPs`, the node-IP default migrates via a MetalLB `IPAddressPool` with no `L2Advertisement` (allocate-but-don't-announce) — a separate change. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added support for configuring the ingress host Service with a native `loadBalancerClass` value. * Expanded chart tests to verify class wiring and immutability behavior across install and upgrade scenarios. * **Bug Fixes** * Removed the legacy exposure-class/service-exposure flow and related controller behavior. * Updated templates and CRD packaging so the platform no longer renders or depends on those deprecated resources. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
f4aef73053 |
fix(linstor-scheduler): re-vendor to upstream 0.3.1 with native admission webhook
Re-vendor the linstor-scheduler chart from upstream 0.3.1 (appVersion v0.3.6), which carries the admission webhook binary that stopped stripping unknown Pod fields — the image VolumeSource and native-sidecar restartPolicy, which broke KubeVirt containerDisk VMs and native sidecars cluster-wide — and now reloads its TLS certificate on rotation instead of serving a stale one until restarted. The admission webhook ships in the upstream chart, so the previous downstream copy is dropped: `make update` pulls it directly and applies only cozystack-scheduler-deployment.patch (pod component label and a distro-suffix-stripped image tag), keeping the vendoring deterministic. Because the webhook now reloads its own certificate, the external cert-reload workaround is dropped. The upstream admission Deployment keeps the same name but changes its immutable pod selector, so an in-place upgrade of an existing install would fail. A migration deletes the old admission Deployment before the upgrade so the new one is recreated cleanly, and bumps the migration target version. Add a helm-unittest suite covering the admission resources, PDB selector isolation, least-privilege RBAC, the scheduler image tag, and backward compatibility. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
d542f66368 |
fix(ouroboros): render controller rollout strategy in every mode
Bump the chart and controller image to 0.8.1, which renders the controller Deployment's rollout strategy in every controller.mode instead of only in external-dns mode. Up to 0.8.0 the chart rendered a strategy only in external-dns mode, as type: Recreate, and nothing in the other modes. The apiserver defaults a rollingUpdate block onto a Deployment created without one, so switching a live release into external-dns laid Recreate on top of that defaulted block and the apiserver rejected the object: spec.strategy.rollingUpdate: Forbidden: may not be specified when strategy `type` is 'Recreate' The strategy is now identical across all modes, so no mode switch changes a strategy field at all. This is a latent break rather than an active one: the host pins coredns and tenants pin coredns-import, neither of which rendered a strategy in 0.8.0, and no shipped configuration selects external-dns. But controller.mode is user-settable and the chart supports the mode, so the transition is reachable by configuration. It surfaces only under server-side apply, which helm-controller uses by default, so no local helm upgrade reproduces it: a client-side strategic merge carries retainKeys on strategy and clears rollingUpdate on the way through. The rendered strategy is pinned on all three modes, external-dns included — it is the only mode that ever rendered Recreate, so a bump that re-gated the block would otherwise slip past a coredns-only suite. 0.8.1 also carries an upstream bump of the cleanup-hook kubectl image to 1.36.1. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Myasnikov Daniil
|
f1619215ff |
docs(api): note disableWait silently defeats healthCheckExprs
The field docs already say healthCheckExprs are only evaluated under the poller wait strategy, but that is only half the precedence rule. Upstream evaluates them only when the Helm action itself has wait enabled (helm-controller/api@v1.5.1 v2/helmrelease_types.go:193-199), so the release.cozystack.io/helm-install-disable-wait annotation makes them a silent no-op whatever waitStrategy says — no error, no warning, the release just reports Ready as before. This is reachable today: packages/system/kubernetes-rd already sets that annotation and is a plausible candidate for CEL expressions in phase 3. Document it on the ApplicationDefinition field (so it reaches the CRD description) and next to HelmInstallDisableWaitAnnotation. ComponentInstall has no disableWait, so its field doc is unchanged. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
96a9652fb1 |
feat(api): typed waitStrategy/healthCheckExprs for kstatus readiness
Add first-class fields so a package/app can declare the Flux helm-controller v1.5.0 kstatus health gate for the custom resource it wraps, so HelmRelease.Ready means "the CR is actually healthy" instead of "helm applied the CR" (#2642). API-owner approved in #3263. - api: WaitStrategy (scalar, maps to HelmReleaseSpec.WaitStrategy.Name — a deliberate simplification) + HealthCheckExprs ([]kustomize.CustomHealthCheck, the upstream type verbatim) on both ApplicationDefinition.spec.release (the apiserver path) and ComponentInstall (the operator path). - config.ResolveWaitStrategy: shared by both HelmRelease builders. Couples the default — healthCheckExprs imply the poller strategy when none is set, since exprs are only evaluated under poller. This removes the "must also set waitStrategy: poller" footgun and does not depend on the controller's global default. - thread through buildHelmReleaseSpec (operator) and convertApplicationToHelmRelease (apiserver); the apiserver loads the typed spec.release fields in start.go. - regenerate deepcopy, the ApplicationDefinition and PackageSource CRDs, and openapi. The backupstrategy CRDs also regenerate — benign k8s-0.35 PodSpec description drift, not a functional change. disableWait is intentionally not added (YAGNI): the only known need (the kubernetes tenant-addon deadlock) was resolved child-side, and waitStrategy: legacy is not equivalent (legacy still waits Helm-v3-style; disableWait skips waiting entirely). Add it later when a concrete leaf needs a true no-gate escape hatch. Refs #2642 Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
myasnikovdaniil
|
f6a03e93f8 |
fix(etcd): complete v1alpha2 transition for in-cluster 1.5→1.6 upgrades (#3270)
Consolidates the etcd `v1alpha2` transition fix for **in-cluster 1.5 → 1.6 upgrades** into a single PR, rebased on current `main`. Supersedes #3265 and #3261 — their commits are carried here (authorship preserved), so those PRs can be closed once this lands. Fresh installs use the new shapes directly and are unaffected, so fresh-install CI does not catch these; the 1.5 → 1.6 e2e upgrade path is the authoritative regression guard. ## What this PR does ### 1. Keep the legacy `etcd-headless` Service alive during adoption — `packages/extra/etcd` `etcd-migrate` adopts legacy clusters **in place**: the Pods keep their original `spec.subdomain: etcd-headless` and are dialed at `etcd-<i>.etcd-headless.<ns>.svc` until they roll onto the operator's native `<member>.etcd.<ns>.svc` domain. The v1alpha2 operator only creates the native `etcd` Service and the legacy `etcd-headless` Service is pruned, so those per-pod names stop resolving (`no such host`), `MemberList` fails, and `status.readyMembers` never populates — the `EtcdCluster` never goes `Ready` even though etcd is healthy and in quorum. We ship a chart-managed transitional headless `etcd-headless` Service (selector mirrors the operator's native `etcd` Service via `etcd-operator.cozystack.io/cluster`, `publishNotReadyAddresses: true`) — the DNS counterpart of the legacy `*.etcd-headless.<ns>.svc` SAN already kept for this window. Removable together with that SAN once members roll onto the native subdomain. ### 2. Survive the immutable controller-Deployment selector on upgrade — `packages/system/etcd-operator` (#3242) #2859 replaced the upstream etcd-operator chart with the cozystack-authored one, changing `Deployment.spec.selector.matchLabels`. `spec.selector` is immutable, so `helm upgrade` cannot patch the existing Deployment and the whole HelmRelease upgrade fails (`field is immutable`). A **pre-upgrade hook** (`templates/pre-upgrade-selector-fix.yaml`: ServiceAccount + Role + RoleBinding + Job) deletes the Deployment **only** when its live selector is the pre-1.6 one, so Helm recreates it cleanly. No-op when the selector already matches, never runs on fresh install. ### 3. Raise the operator's memory cold-start floor — `packages/system/etcd-operator` Steady-state working set is ~250Mi; the static `limits.memory: 128Mi` OOMKills a Pod that starts before the VPA admission webhook rewrites it. Raise the floor to `256Mi` (and VPA `minAllowed` to match) as defense in depth so the operator never depends on VPA timing to avoid crashing. ### 4. Make migration 50 (etcd adoption) robust in-cluster — `packages/core/platform/images/migrations/migrations/50` (was #3261) Exact server/peer cert-SAN match, Secret-gated wait on the adoption Secret, and in-cluster (IPv6-safe) kubeconfig handling, so the migration script drives the adoption reliably from inside the cluster. Covered by `hack/migration-50-etcd-adopt.bats`. ### 5. Hardening / review fixes (this PR's original scope) - **Hook `runAsUser: 65532`.** `pre-upgrade-selector-fix.yaml` set `runAsNonRoot: true` but no numeric `runAsUser`; `clastix/kubectl`'s image user is the non-numeric name `nonroot`, which the kubelet cannot verify against `runAsNonRoot` — so the hook Pod fails admission and silently blocks the very upgrade it exists to unblock. Adds `runAsUser: 65532`, matching the postgres-operator webhook-ready hook that runs the same image. - **Digest-pin the kubectl image.** The values comment claimed digest-pinning but shipped a floating `v1.32` tag. Reuses the digest postgres-operator vendors, adds the `renovate` annotation, and templates `repo:tag@digest`. - **Tests.** `etcd-operator/tests/selector-fix-hook_test.yaml` (hook wiring, weight ordering, namespaced least-privilege RBAC, numeric-non-root security context, digest-pinned image), `etcd-operator/tests/deployment_test.yaml` (256Mi cold-start floor), `extra/etcd/tests/etcd-cluster_test.yaml` (transitional `etcd-headless` Service). Each assertion was mutation-tested. ### 6. Derive the default S3 endpoint from the provisioned bucket — `packages/system/backupstrategy-controller` Derive the default S3 endpoint (and per-driver scheme / TLS / `secure_connection`) from the provisioned bucket Secret instead of requiring it to be hand-set, so etcd (and other) backup strategies get a working endpoint by default. Docs in `docs/operations/backup-classes.md`; covered by `tests/endpoint_form_test.yaml`. ### Verification - `helm unittest` green on current `main`: etcd-operator **18/18**, extra/etcd **18/18**, backupstrategy-controller **11/11**. - Behaviours 1 & 2 were reproduced and confirmed live on a 1.5.2 → 1.6.0-rc.1 adoption (3-node cluster) — recreating the `etcd-headless` Service took the adopted cluster to `readyMembers=3 / Available=True`. ```release-note fix(etcd): complete the v1alpha2 transition on in-cluster 1.5→1.6 upgrades — keep the legacy etcd-headless Service alive so adopted members stay resolvable, delete the pre-1.6 operator Deployment via a pre-upgrade hook to get past the immutable selector, raise the operator's memory floor so it does not OOM before the VPA scales it, and make the etcd adoption migration robust in-cluster. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Improved backup storage endpoint handling across supported backup drivers, including provisioned and external S3 storage. * Added compatibility support for legacy etcd pod discovery during migration. * Added an automated upgrade safeguard for etcd operator deployments. * **Bug Fixes** * Improved certificate SAN detection and etcd migration authentication. * Increased the etcd operator’s minimum startup memory to prevent early restarts. * **Documentation** * Clarified backup endpoint, TLS, and driver-specific behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Myasnikov DaniilandClaude Opus 4.8
|
4af5a82796 |
fix(etcd-operator,backups): address review nits — trim endpoint + hook resources
- backupstrategy-controller: trim() the decoded S3 endpoint before scheme stripping so a trailing newline in the Secret can't yield a malformed URL - etcd-operator: give the pre-upgrade selector-fix hook container resource requests/limits (matches postgres-operator webhook-ready), +unittest Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov DaniilandClaude Opus 4.8
|
e160a0dc87 |
fix(backups): derive default S3 endpoint from the provisioned bucket
The default backupStorage.endpoint is http://seaweedfs-s3...svc:8333, but Cozystack ships SeaweedFS with global.seaweedfs.enableSecurity=true, so its in-cluster S3 serves TLS on :8333 behind the self-signed "SeaweedFS CA". Every cozy-default backup (etcd/mariadb/velero/fdb/cnpg) therefore hits a TLS listener over plaintext and fails the handshake. This is fatal for the etcd v1alpha2 adoption migration: its mandatory pre-upgrade safety snapshot writes to this endpoint, so migration 50 hard-fails and blocks the 1.5->1.6 upgrade on any cluster with a legacy etcd. The Etcd Strategy S3 schema has no caCert/insecureSkipVerify field, so it cannot target the self-signed in-cluster endpoint at all. Add a "backupstrategy-controller.endpoint" helper that, for a provisioned bucket, resolves the endpoint from the COSI bucket's system credentials Secret (backupStorage.systemSecretName) — the external S3 ingress with an ACME cert, the same trusted endpoint COSI advertises and every backup operator can verify — and forces the https:// scheme. All Strategy CRs, the Velero BackupStorageLocation and the controller Deployment env are routed through it. It falls back to .Values.backupStorage.endpoint for external S3 (provisionBucket=false) and for offline `helm template`/unit renders and the pre-reconcile first install, where the Secret lookup returns nothing (Flux re-renders on spec.interval once the Secret exists). helm-unittest extended to assert every consumer picks up the resolved endpoint (CNPG/Etcd/Velero keep the full URL, MariaDB/FDB strip the scheme and derive their secure flag) plus the external-S3 verbatim fallback. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov DaniilandClaude Opus 4.8
|
ca4f99411f |
fix(etcd): harden v1alpha2 transition — hook runAsUser, digest-pin kubectl, tests
Addresses review on #3265: - pre-upgrade-selector-fix hook: add runAsUser: 65532. clastix/kubectl's image user is the non-numeric name `nonroot`, which the kubelet cannot verify against runAsNonRoot: true — so the Pod fails admission and the hook never runs, silently blocking the very 1.5->1.6 upgrade it exists to unblock. Matches postgres-operator's webhook-ready hook (same image). - Digest-pin the kubectl image (the comment already claimed digest-pinning but shipped a floating v1.32 tag). Reuse the digest postgres-operator already vendors, add the renovate annotation, and template repo:tag@digest. - Tests (both charts have CI helm-unittest suites, none previously covered these): - etcd-operator/tests/selector-fix-hook_test.yaml: hook wiring, namespaced least-privilege RBAC, numeric-non-root securityContext (guards the runAsUser regression above), digest-pinned image. - etcd-operator/tests/deployment_test.yaml: assert the 256Mi cold-start memory floor so it can't silently drop back to an OOMKilling value. - extra/etcd/tests/etcd-cluster_test.yaml: assert the transitional etcd-headless Service (headless, publishNotReadyAddresses, member selector, client/peer ports). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Andrey Kolkov
|
9daaa9ff5a |
fix(etcd): fix etcd-operator 1.5->1.6 upgrade (immutable selector + OOM floor)
Two defects that break the etcd-operator on an in-cluster 1.5 -> 1.6 upgrade (#2859 swapped the upstream chart for the cozystack-authored one): - #3242: the controller Deployment's spec.selector.matchLabels changed ({instance,name} -> {name,control-plane}). spec.selector is immutable, so `helm upgrade` cannot patch the existing Deployment and the whole HelmRelease upgrade fails ("field is immutable") -- the operator and the v1alpha2 CRDs it serves never come up, which blocks the etcd v1alpha2 adoption. Fresh installs use the new selector directly, so fresh-install CI does not catch it. Add a pre-upgrade hook that deletes the Deployment ONLY when its live selector is the pre-1.6 one (lacks control-plane=controller-manager), so Helm recreates it cleanly. No-op when the selector already matches (rc.1 -> later) and never runs on a fresh install (pre-upgrade only). Keeping the selector stable in the chart is not an option: rc.1 already shipped the new selector, so aligning it back would merely move the immutable break to rc.1 -> next. - Raise the manager's cold-start memory limit floor 128Mi -> 256Mi (and the VPA minAllowed to match). The steady-state working set is ~250Mi (the VPA's own recommendation); at 128Mi a Pod that starts before the VPA admission webhook rewrites it (e.g. a Deployment recreated out of band, or the webhook briefly unavailable during upgrade) OOMKills into a crash loop. Defense in depth so the operator never depends on VPA timing merely to avoid crashing; the VPA still scales it further under load up to maxAllowed. Verified live on a 1.5 -> 1.6 adoption: the delete-Deployment step plus the VPA re-applying 256Mi+ let the operator come up and the adoption complete. Refs: #3242, #3243 Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Ivan Okhotnikov
|
0164a6afa4 |
fix(monitoring): set groups_attribute_path=groups so allowed_groups actually works on Grafana v11.x
Verified end-to-end on dev3 with Grafana v11.6.15: OAuth flow reaches Grafana, id_token and userinfo response both carry the correct `groups` claim (`[cozystack-cluster-admin, tenant-root-super-admin]` for kvaps), `allowed_groups` in grafana.ini lists `tenant-root-super-admin`, and login still fails with `user not a member of one of the required groups`. Grafana debug log shows `Attributes: map[]` — the extracted user's Groups slice is empty. On v11.x+ Grafana does not auto-map the `groups` claim onto user.Groups unless `groups_attribute_path` is set explicitly (the field is documented as optional but the gate is a silent no-op without it — a claim-name-string is not enough). Fix: hard-wire `groups_attribute_path=groups` in the chart-forced System-mode block alongside `allowed_groups`. "groups" points at the top-level `groups` array from userinfo, matching the shape Keycloak's oidc-group-membership-mapper emits (verified on the same dev3 run). Two helm-unittest cases updated to assert `groups_attribute_path` lives next to `allowed_groups` in both the users-map-active and empty-users branches (rejecting the silent no-op regression). Docs also updated to name the requirement. Signed-off-by: Ivan Okhotnikov <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
b002c967ac |
fix(monitoring): close cross-tenant Grafana OIDC hole via tenant-group gate
Round-3 review from lllamnyp: any user in the shared `cozy` Keycloak
realm could sign into any tenant's Grafana. The audience mapper stops
replay of a minted token across releases, but does not gate who may
mint one — tenant-alice could complete the OAuth flow against
tenant-bob's Grafana. With `spec.oidc.users` non-empty the earlier
`allow_sign_up=false` fix mostly closed the door (though tenant-alice
still hit `error 401` after landing in Grafana, revealing the release
exists); with empty users under mode: System — a config the schema
permits and the docs describe as hands-off — the login succeeded and
minted a Viewer account for every cozy-realm identity.
The fix uses the KeycloakRealmGroups the tenant chart already
provisions (`<namespace>-{view,use,admin,super-admin}` in
packages/apps/tenant/templates/keycloakgroups.yaml) — no new
directory objects owned by the Monitoring release. Two chart edits:
1. oidc-keycloak.yaml: add the platform-managed realm-default `groups`
scope to `defaultClientScopes` explicitly. EDP's KeycloakClient
does NOT merge realm-level defaults into `defaultClientScopes`, so
the scope must be listed here or the `groups` claim would be
missing from the token.
2. grafana.yaml: in System mode, unconditionally emit
`allowed_groups: <ns>-view <ns>-use <ns>-admin <ns>-super-admin`
in `auth.generic_oauth` and add `groups` to `scopes`. Grafana
rejects any login whose token does not carry at least one of the
groups.
The gate is chart-owned and does not depend on the users-map — with
`users: []` tenant members still get a self-service Viewer through
`auto_assign_org_role`, cross-tenant users get 401 in Keycloak's OAuth
callback. Docs updated to reflect the two-layered contract
(tenant-membership gate + optional users-map).
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
|
||
|
IvanHunters
|
f64ba91872 |
fix(monitoring): do not render users-Job when spec.oidc.users is empty
Address PR#3176 review round-2 gap-2: the users-Job's prune pass deletes every non-admin Main-Org member whose email is not in the desired list. When the desired list is empty every helm-upgrade wiped the whole org. Three empty-users states used to fall into this trap: * customConfig.secretRef: users[] is forbidden by assertCustomSecretRefUsersEmpty, so the desired list was structurally always empty and the chart wiped an org whose auth.ini fragment the operator advertised as authoritative. * System with users[] unset: the operator opted into OIDC but not into the chart-managed users-map, yet the Job still pruned every Main-Org member on their behalf. * CustomConfig-inline with users[] unset: same shape as System. The mode=None guard (from an earlier round) already had the same shape; extend the render gate to cover the other empty-list doors — now the Job renders only when `mode != None AND users non-empty`. The trade-off: taking `users:` back to `[]` no longer prunes the last entry — matching the already-documented `System → None` posture — called out explicitly in the docs. Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
ff4ef2c189 |
fix(monitoring): force allow_sign_up=false and gate isolation dict on users-map
Address PR#3176 review round-2 gap-1: the docs promise "users not in spec.oidc.users get nothing", but Grafana's default allow_sign_up=true combined with the chart-forced skip_org_role_sync=true was minting a Viewer account for every cozy-realm identity that completed the OAuth flow, then reaping it on the next helm-upgrade prune pass. The chart-forced isolation dict (now three keys: skip_org_role_sync, oauth_allow_insecure_email_lookup, allow_sign_up=false) is applied only when spec.oidc.users is non-empty. A BYO-IdP operator who does not use the users-map gets none of the three forced on top of their inline config, matching the review's minor: forcing insecure_email_lookup against an issuer that may not verify emails is a needless account- linking surface, and allow_sign_up=false with no pre-provisioned accounts would lock everyone out. Tests split the two branches: users-active asserts all three keys are present and the operator cannot override them via CustomConfig-inline; users-empty asserts the auth.generic_oauth block is emitted without any of the isolation keys. Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
b2fd32385e |
fix(monitoring): rewrite users-Job shell in POSIX (alpine BusyBox compat)
CI E2E caught the Job container crash-looping within 20s of the first Start event. Root cause: the `alpine/k8s:1.33.4` image is Alpine-based (BusyBox coreutils + BusyBox grep), and the Job shell used four GNU-specific constructs that BusyBox does not implement: - `head --bytes=N` (BusyBox `head` only accepts `-c N`). - `tr --delete` (BusyBox `tr` only accepts `-d`). - `jq --raw-output0` (NUL-terminated output — jq 1.7+ feature; also paired with `read -r -d ''` which is a bash-ism, not POSIX `sh` and not honoured by BusyBox `ash`). - `grep --null-data --fixed-strings --line-regexp` (BusyBox `grep` supports `-F` and `-x` short forms but NOT `-z`/`--null-data`). Rewrite: - `head -c 24 /dev/urandom | base64 | tr -d '=+/' | head -c 32` — same random-32-char password, portable to BusyBox. - Emit desired emails and org membership one record per line (jq `--raw-output`) instead of NUL-terminated. Safe because the template-level `monitoring.oidc.assertUsersEmailShape` guard already rejects any email containing whitespace / control characters, so line-oriented iteration cannot get confused. - Read TSV membership records with `IFS="$(printf '\t')"` (POSIX- portable TAB) + plain `read -r uid ulogin uemail` — no need for `-d ''` once records are newline-terminated. - Match desired list with `grep -Fxq -e "$uemail" -e "$ulogin"` — short-form flags supported by both GNU and BusyBox `grep`. Syntax validated with both `sh -n` and `bash -n` on the extracted inline script. helm-unittest 34/34 still green (render shape is unchanged, only the shell literal is different). Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
1422495b88 |
fix(monitoring): do not render users-Job when mode=None (upgrade safety)
Caught by dev3 end-to-end test on the PR working tree
(cozystack-pr-test skill): the users-Job was rendered in every mode
including `None` on the rationale that a `mode: System → None`
switch should garbage-collect OIDC-provisioned local accounts. That
rationale ignored the upgrade path from the pre-PR chart: an
existing tenant that never had `spec.oidc` on their Monitoring CR
would receive `mode: None` on first render under the PR, the Job
would fire on every Flux reconcile of the outer HR, and its prune
pass would DELETE every non-admin Main-Org member — including users
the tenant added by hand through the Grafana UI on the pre-PR chart.
Silent breaking change with data loss.
Wrap the whole Job resource in `{{- if ne $mode "None" }} … {{- end }}`
so `mode: None` = "chart owns nothing in Grafana orgs, we don't
touch what's there". Operators who need to prune OIDC-provisioned
accounts after switching System | CustomConfig → None do it
themselves through the Grafana UI or admin API — same posture as
"we don't own the Keycloak directory objects" from the earlier
architectural change.
The `assertUsersPrologue` include stays outside the `if` so an
operator listing users in `mode: None` still gets the schema and
email-shape validation at render time — the Job body just doesn't
emit.
Test coverage:
- Flip the previous "mode=None still renders the users-Job so a
mode switch can prune stale members" case to
`mode=None does NOT render the users-Job (upgrade safety guard)`
as a regression lock.
- Add `mode=None with non-empty users still fails render` covering
the prologue-only-fires-on-schema-issue path.
Docs: rewrite the "Users and RBAC" section step 3 to name the
upgrade-safety motivation and the manual-cleanup handoff.
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
|
||
|
IvanHunters
|
c31a9f8ec3 |
fix(monitoring): promote oidc.users[].role to required + regenerate schema
The `OIDCUser` typedef in `packages/extra/monitoring/values.yaml`
declared only `@field {string} email`; `cozyvalues-gen` regenerated
`values.schema.json`, `README.md`, and the mirrored `openAPISchema`
on the `Monitoring` ApplicationDefinition WITHOUT a `role` field.
Consequences:
- The `@enum OIDCRole` declaration was orphan — referenced from
nowhere, so the schema's items object had `required: ["email"]`
and one property.
- A dashboard-driven CR would offer a form without any role picker;
a schema-conformant `{email}` submission would render into the
users-Job with `role: null`, `jq --raw-output` yields the string
`"null"`, the `POST /api/orgs/1/users` body carries
`"role":"null"`, Grafana returns HTTP 400, and the post-install
hook exhausts `backoffLimit: 6` — HelmRelease never becomes Ready.
- Unit + e2e tests all hardcoded `role: Admin` / `role: Viewer`, so
the contract mismatch never surfaced in CI.
Fix:
- Add `@field {OIDCRole} role - ...` to the `OIDCUser` typedef in
`packages/extra/monitoring/values.yaml` so cozyvalues-gen materialises
the field with enum + required.
- Regenerate `values.schema.json`, `README.md`, and mirrored
`openAPISchema` via `make generate`.
- Add `monitoring.oidc.assertUsersRoleShape` template-level guard
(same shape as `assertUsersEmailShape`) so a stale-schema CR or a
direct-helm invocation fails at render time with a legible error
instead of at hook execution with a Grafana 400.
- Consolidate all `spec.oidc.users` guards into a single
`monitoring.oidc.assertUsersPrologue` include and call it once from
`oidc-users-job.yaml`, ordering the incompat check
(`assertCustomSecretRefUsersEmpty`) before the per-entry role /
email regexes so operators see the more actionable message first.
Drop the duplicate include from `grafana.yaml` (Job template renders
in every mode anyway).
- Two new helm-unittest cases lock the regression:
`oidc.users entry missing role fails the render` and
`oidc.users entry with unknown role fails the render`.
- Point the previous `secretRef + users` incompat unit test at the
users-Job template since that's where the prologue now fires.
- `docs/oidc-grafana.md`: correct the failed-hook post-mortem window
claim — with `ttlSecondsAfterFinished: 3600` the Kubernetes TTL
controller reaps the Job + Pod 1h after the terminal condition,
not "until the next helm-upgrade".
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
|
||
|
IvanHunters
|
86e54f1296 |
docs(monitoring): align OIDC docs with the actual mode-agnostic contract
Previous docs claimed "the chart forces skip_org_role_sync = true regardless of mode" — a promise the code did not keep in CustomConfig mode. The chart is now fixed; sync the docs so the guarantee reads truthfully and the incompatibility surface is explicit: - `docs/oidc-grafana.md`: rewrite the CustomConfig section so both chart-forced settings are named explicitly, the `merge` semantics are documented (operator can't override), and the `secretRef` + `users` incompatibility is called out with an explicit failure path. - `system/monitoring/values.yaml`: expand the `config`/`secretRef` comments so operators reading the values doc see the merge behavior and the secretRef limitation before they hit the render-time fail. Add `enabled: "true"` to the "typical keys" list — Grafana defaults `enabled: false` and an inline map without it is inert. - `extra/monitoring`: regenerated values.schema.json + README.md via `make generate` (cozyvalues-gen + hack/update-crd.sh) to pick up the `oidc.users[].email` doc-string touch; mirrored openAPISchema on the ApplicationDefinition follows. Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
ad67dee95b |
test(monitoring): lock the CustomConfig chart-force contract in helm-unittest
Two blockers on the previous review cycle were "code says X but no test enforces X" — so future edits to `grafana.yaml` could silently break the contract again. Add three assertions that fail loudly if the merge behavior regresses: - `mode=CustomConfig inline preserves the operator's map + chart-forced settings` — asserts `skip_org_role_sync=true` and `oauth_allow_insecure_email_lookup=true` are both present alongside the operator's `client_id` / `client_secret` / `auth_url`. - `mode=CustomConfig inline — operator cannot override chart-forced OIDC settings` — sets both settings to "false" in the operator's map; asserts they render as "true" (proves `merge` argument order is correct). - `mode=CustomConfig secretRef + non-empty users fails the render (contract incompat)` — asserts the new `monitoring.oidc.assertCustomSecretRefUsersEmpty` helper's error message fires so operators cannot silently strand their users-Job assignments by combining incompatible fields. - `mode=CustomConfig secretRef + empty users renders successfully` — the compatible path. Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
26e5f06d27 |
fix(monitoring): chart-force skip_org_role_sync + email lookup in every OIDC mode
The chart previously injected the two settings the users-Job's
contract depends on — `skip_org_role_sync = true` and
`oauth_allow_insecure_email_lookup = true` — only inside the
`mode: System` branch of `grafana.yaml`. `values.yaml` and
`docs/oidc-grafana.md` both promised the settings would be forced
regardless of mode, but under `mode: CustomConfig` the operator's
inline `auth.generic_oauth` map was written verbatim, both
settings were absent, and the users-Job's role assignments were
silently stranded (no lookup by email → new Grafana account minted
at login) or transiently overwritten (no skip_org_role_sync → any
`role_attribute_path` or Grafana's `auto_assign_org_role: Viewer`
overwrote the Job's role on each login).
Fix: merge a `$chartForcedOidc` dict on top of both the generated
System block and the operator's inline map (`merge` wins for
overlapping keys, so the operator can NOT override the two
settings). CustomConfig secretRef mode cannot receive the merge
because the ini fragment is authoritative — the chart now hard-fails
render when both `customConfig.secretRef.name` and non-empty
`spec.oidc.users` are set, forcing the operator to either switch to
inline config or manage authorization themselves inside the ini.
Also:
- Align the inner HR `timeout` on `packages/extra/monitoring/templates/helmrelease.yaml`
from 10m to 15m so it matches the outer cozyrds
`release.cozystack.io/helm-install-timeout: "15m"` and does not
fail while the users-Job is still inside its
`activeDeadlineSeconds: 900`.
- Route the users-Job image through `cozy-lib.image` so
mirrored / air-gapped installations resolve it (matches the
pattern in `packages/apps/mariadb/templates/hooks/cleanup-pvc.yaml`).
- Add `spec.ttlSecondsAfterFinished: 3600` on the users-Job so a
succeeded Job + Pod are garbage-collected on the next reconcile
interval rather than lingering until the next hook activation
(which never fires on release uninstall).
- Harden the users-Job shell:
* emails read NUL-terminated (`jq --raw-output0`) so an address
containing whitespace cannot corrupt the `for email in ...`
loop's field splitting;
* `/api/users/lookup?loginOrEmail=<email>` URL-encoded via
`jq @uri` so `&`/`=`/`?` in the local part parse correctly on
the Grafana side;
* prune loop switched from pipe-delimited fields to TSV inside
NUL-terminated records, with `cut -f` for extraction.
- Add a template-level assertion (`monitoring.oidc.assertUsersEmailShape`)
rejecting emails that do not match a conservative regex —
cheaper failure mode than a 400 from Grafana.
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
|
||
|
IvanHunters
|
c493f2cb96 |
test(monitoring): rewrite OIDC unit tests around the users-map contract
Rewrites the helm-unittest coverage for the Phase-1 OIDC selector
against the new chart contract:
- Drop assertions for the three KeycloakRealmGroups and for
`role_attribute_path` / `allow_assign_grafana_admin` / the
`oidc.grafanaAdmin` values lever — none of those render anymore.
- Assert the System-mode Grafana config carries
`skip_org_role_sync = "true"` and
`oauth_allow_insecure_email_lookup = "true"` and NO
`role_attribute_path`.
- Cover the new users-Job template: helm-hook annotations, absence
of a ServiceAccount, `activeDeadlineSeconds`, admin-credentials
wiring from the chart-managed `grafana-admin-password` Secret,
and correct serialisation of `spec.oidc.users` into the
`DESIRED_USERS_JSON` env.
- Assert the users-Job also renders under `mode: None` so a
System -> None flip still gets a prune pass.
- Cover the defensive `.Values.oidc | default dict` paths — omitted
block and explicit `oidc: ~` — across the three OIDC templates.
- Tighten the client-secret regex to the actual base64-encoded
length (`^[A-Za-z0-9+/]{43}=$`) so shortened random secrets are
caught.
32 tests, all green under helm-unittest v1.0.3.
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
|
||
|
IvanHunters
|
0a65dd04a1 |
feat(monitoring): expose spec.oidc.users on the Monitoring CR
Extends the tenant-facing `Monitoring` CR with the new
`spec.oidc.users: [{email, role: Admin|Editor|Viewer}]` field
consumed by the users-Job introduced in the previous commit. Works
for both `System` and `CustomConfig` modes; ignored when
`mode: None`.
Regenerated `packages/extra/monitoring/values.schema.json`,
`README.md`, and the mirrored `openAPISchema` on the
`monitoring-rd` ApplicationDefinition through the standard
`make generate` (cozyvalues-gen + hack/update-crd.sh) pipeline.
Also refreshed the stale annotation comment on the CR that still
claimed the chart provisions three KeycloakRealmGroups.
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
|
||
|
IvanHunters
|
01d68904ea |
feat(monitoring): drop chart-owned Keycloak groups; app-side users-map + reconcile Job
Replace the group-based authorization model on the Phase-1 Grafana OIDC selector with an app-side users map, mirroring the tenant kube-apiserver sibling in cozystack/cozystack#3044. What is removed - The three chart-owned KeycloakRealmGroup objects in the cozy realm (`<clientId>-{admin,editor,viewer}`). A Helm release must not own directory objects — toggling `spec.oidc.mode: System -> None` or deleting the Monitoring release would Helm-delete the groups, and with them any membership curated out-of-band. Per-instance groups in the platform realm is exactly the design question left open at the end of cozystack/community#24; establishing it as precedent from a monitoring PR is out of scope. - Dash-separated group naming (`<namespace>-<release>-<role>`) was also collision-prone against tenant/subtenant paths, since both the namespace and release names can themselves contain dashes. - `role_attribute_path` in the Grafana `generic_oauth` block and the default-Viewer fallback, which handed org-Viewer to every cozy realm identity that logged in — cross-tenant metrics read across every opted-in Grafana. Also removed the `groups` client scope from the KeycloakClient defaults. - `monitoring.oidc.allowAssignGrafanaAdmin` helper and the internal `oidc.grafanaAdmin` values key: the feature was already deferred and the helper's release-name-only guard was tenant-spoofable. What replaces it - `spec.oidc.users: [{email, role: Admin|Editor|Viewer}]` on the Monitoring chart values. - A chart-owned post-install/post-upgrade Job (`templates/grafana/oidc-users-job.yaml`) that reconciles that list into Grafana's Main Org. via the admin API: pre-provisions a local Grafana account per listed email so the reconcile can set the org role before the operator's first OIDC login, adds the account to the org, PATCHes the role to converge across re-runs, and prunes every non-admin Main-Org member whose email is not in the list (handles both entry removal and `mode: None`). - `skip_org_role_sync = true` + `oauth_allow_insecure_email_lookup = true` in the generic_oauth block. The former stops a login from overwriting the Job's assignments; the latter binds the OIDC identity to the pre-provisioned local account. - Job talks to Grafana over the in-cluster Service and reads admin creds from the chart-managed `grafana-admin-password` Secret via `envFrom`: no ServiceAccount / Role / RoleBinding. `activeDeadlineSeconds: 900` caps a stuck Job so a broken Grafana cannot hold the release in `pending-upgrade`. Defensive rendering - `.Values.oidc | default dict` in the four templates that dereference the `oidc` block so `oidc: ~` or an omitted block no longer nil-derefs. - `oidc-client-secret.yaml` falls back to a fresh random when the existing Secret lookup succeeds but `data.client-secret` is empty. Rationale: cozystack/cozystack#3176 review from @lllamnyp (architectural CHANGES_REQUESTED points 1, 2, 3 + the default-Viewer leak). The three points stack: dropping the groups incidentally fixes the naming-collision and the default-Viewer leak, and the users-Job gives operators the same UX story as the tenant kube-apiserver PR across both sides of the platform. Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
Ivan Okhotnikov
|
aa63786acd |
docs(monitoring): sync oidc.grafanaAdmin values docstring with deferred-lever posture
Second-iteration cozy-review flagged this as a leftover from the platform-lever removal: `packages/system/monitoring/values.yaml` still described `grafanaAdmin` as "Set true for the platform Grafana release only ... the platform bundle passes this flag through Package.spec.components", which contradicts the deferred-lever reality already documented in the same commit's updates to `_helpers.tpl` and `docs/oidc-grafana.md`. Rewrite the docstring to match: chart-internal lever, defaults `false`, Phase 1 has no end-to-end wiring; kept as a values key so a future phase can add the wiring without touching the chart contract. Signed-off-by: Ivan Okhotnikov <xorokhotnikov@gmail.com> |
||
|
Ivan Okhotnikov
|
b46543e5f2 |
fix(monitoring): defer platform GrafanaAdmin lever to a future phase
The MAJOR finding from the cozy-review agent: the platform-side `oidc.grafanaAdmin: true` documented in the PR body and in docs/oidc-grafana.md never actually reached the platform Grafana's HelmRelease. `Package.spec.components["monitoring-system"].values.oidc.grafanaAdmin` was set in packages/core/platform/templates/bundles/system.yaml, but `monitoring-system` (and `monitoring`) have no `install:` block in packages/core/platform/sources/monitoring-application.yaml — only `monitoring-rd` does. PackageReconciler skips components without an `install:` block before ever reaching the assignment that copies `pkg.Spec.Components[].Values` onto `hr.Spec.Values` (internal/operator/package_reconciler.go around lines 197-201 and 292-293). The values-override dictionary was silently dropped, no HR carried `oidc.grafanaAdmin: true`, and the platform Grafana rendered with the chart default (`false`). The `bundles_monitoring_grafana_admin_test.yaml` unit test only asserted against the emitted Package CR spec — exactly the layer where the value still existed but was about to be discarded — so the broken propagation slipped past. Trim the broken promise from Phase 1. The chart-side helper (`monitoring.oidc.allowAssignGrafanaAdmin`) stays as-is so a future phase that routes the override through the tenant-root chart's monitoring HR values (or adds an `install:` block on the `monitoring-system` component) lands on a green chart-side contract. The helm-unittest that verifies chart-side emission stays too, but with a docstring calling out that end-to-end wiring is deferred. Everything else that promised the lever works — bundle line, docs, platform bundle test — is either removed or rewritten to match reality. Also picks up two MINOR findings from the same review: * Drop the misleading `secret-hash` annotation on the OIDC client-secret Secret. The comment claimed it "rotates the KeycloakClient CR and therefore the Grafana Deployment env when the client-secret changes", but nothing downstream watches an annotation on the Secret itself — no rotation is triggered. The `lookup`-based fallback already keeps the value stable across reconciles, so no rotation is even needed. Removing the dead annotation avoids planting a broken assumption for future readers. * Add `release.cozystack.io/helm-install-timeout: "15m"` on the monitoring cozyrds ApplicationDefinition. Under `mode: System` the chart now emits, on top of the existing Grafana + Alerta + VMCluster + CNPG stack, four additional Keycloak CRs whose reconciliation is asynchronous against an operator in `cozy-keycloak` (not the tenant namespace). Flux's default 5m install window was already tight for the pre-OIDC stack; 15m matches the precedent set by tenant-rd/kubernetes-rd cozyrds. Signed-off-by: Ivan Okhotnikov <xorokhotnikov@gmail.com> |
||
|
Ivan Okhotnikov
|
586e77c361 |
fix(monitoring): hard-fail System OIDC render on missing Keycloak CRD
`mode: System` previously wrapped every Keycloak resource in the `Capabilities.APIVersions.Has "v1.edp.epam.com/v1"` guard, so a cluster where the Keycloak operator CRDs were not yet registered silently dropped the whole Keycloak side while Grafana still came up with an `auth.generic_oauth` block pointing at a client that never got provisioned. Login broke with an opaque OIDC error and no chart-side signal that the platform was misconfigured. Turn the missing CRD into an actionable render error. A new `monitoring.oidc.assertSystemKeycloakCRD` helper fails with a message that points at `cozystack.keycloak-operator`, called from both `templates/grafana/oidc-keycloak.yaml` (replacing the silent capability guard) and `templates/grafana/grafana.yaml` (symmetric defense-in-depth so a future refactor of oidc-keycloak.yaml cannot regress the invariant). For the platform `monitoring-system` release the oidc variant's `dependsOn: cozystack.keycloak-operator` already prevents the race; for tenant releases the assertion converts the misconfiguration into an operator-facing error instead of a broken login flow. Signed-off-by: Ivan Okhotnikov <xorokhotnikov@gmail.com> |
||
|
Ivan Okhotnikov
|
e05ca91ff0 |
fix(monitoring): keep OIDC grafanaAdmin as a platform-only lever
The tenant-facing `Monitoring` CR exposed `spec.oidc.grafanaAdmin` via `packages/extra/monitoring/values.schema.json`, which the ApplicationDefinition mirrored into `openAPISchema`. A tenant with create/patch on `Monitoring` in their own namespace could set the flag on their release, promoting members of their `<clientId>-admin` Keycloak group to server-level `GrafanaAdmin` instead of the intended org-level Admin. Drop the field from the tenant surface. The platform bundle still flips it on for the `monitoring-system` component through `Package.spec.components["monitoring-system"].values.oidc.grafanaAdmin` (the platform Grafana chart continues to read the value verbatim in `packages/system/monitoring/values.yaml`); tenant Grafana releases inherit the chart default `false` and stay at org-level Admin. Signed-off-by: Ivan Okhotnikov <xorokhotnikov@gmail.com> |