mirror of
https://github.com/cozystack/cozystack.git
synced 2026-11-04 02:57:27 +09:00
feat/ca-extraction-controller
3176
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Aleksei Sviridkin
|
a121bec10b |
fix(controller): resolve an empty sourceSecretName to a condition
An entry with an empty sourceSecretName sent the reconciler to Get a Secret with an empty name. The apiserver answers that with "resource name may not be empty" — not NotFound — so it escaped the not-found branch, was returned from Reconcile, and wedged the sentinel on exponential backoff. The error path returns before any status write, so no Ready condition was ever recorded: the silent retry the sentinel was introduced to make visible. Pin sourceSecretName to MinLength=1 so a live apiserver rejects it at admission, and guard the reconcile so an object admitted by an older CRD resolves to Ready=False/SourceInvalid instead of an error. Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
d558920c84 |
docs(cozystack-basics): distinguish the two VAP capability-gate cases
The dependsOn comment warned that a capability gate renders the policy out at first install and never re-adds it, which read as contradicting the .Capabilities.APIVersions.Has guard the policy templates now carry. They do not conflict: the warning is about gating on the matched types' CRDs, which may not exist yet at first install, while the template guard names the ValidatingAdmission Policy API itself — GA on every supported cluster, so always present. Spell out that a maybe-absent matched-type CRD needs dependsOn ordering while an always-present core API can be gated in the template. Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
9ac06cd1fc |
fix(controller): withdraw the CA projection and refuse contested releases
Two gaps in the sentinel-to-canonical-name lifecycle: Withdrawal — the reconciler published each declared entry but never deleted one. A sentinel that keeps existing with its CACert entry removed (a chart gating it on tls.enabled, say) left the old <release>.tenant-ca served forever, because owner-reference GC only fires when the whole sentinel is pruned. A sentinel that reconciles with zero CACert entries now deletes the projection at the canonical name, but only when it still carries that sentinel's owner reference, so a foreign Secret or another sentinel's projection is never touched. Contention — two sentinels in one namespace with the same release label both resolve to the single canonical name. Whichever reconciled first would publish an arbitrary CA and the two could flap ownership of one projection. Refuse both, mirroring the more-than-one-CACert-entry refusal, so nothing is published until the declaration lives on exactly one sentinel. The ClusterRole comments claimed the reconciler never deletes a Secret; corrected to describe the owner-scoped withdrawal delete. Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
93b496be95 |
feat(cozystack-basics): pin the TenantProjection writer to flux
Replace the publish-ca-cert label-leg admission control with a policy on the sentinel. A ValidatingAdmissionPolicy restricts TenantProjection CREATE and UPDATE to the flux ServiceAccount helm-controller runs as, so a tenant (who has no RBAC on internal.cozystack.io) cannot forge a declaration that publishes a CA of their choosing. DELETE stays ungated for owner-ref GC. cozystack runs its flux controllers under one shared cozy-fluxcd:flux ServiceAccount, so the pin admits any flux controller — a deliberate platform-tier widening, since only helm-controller renders charts and no tenant reaches this group. The literal is guarded by a bats check on its shape, name and namespace. Remove the label-leg policy, its unit test, its pin check and its chainsaw suite — the sentinel model retires that leg. Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
4503538f91 |
feat(postgres): render a TenantProjection for the CA trust anchor
Postgres is the first consumer of the sentinel model. The chart renders a TenantProjection named <release>-ca declaring CNPG's <release>-ca as the CACert source; the CA-extraction controller lifts ca.crt alone into the tenant-readable <release>.tenant-ca projection and owns it through the sentinel. Drop the spec.caCert declaration leg from the ApplicationDefinition API and from the postgres definition — the sentinel replaces it. The tenant-ca include selector that makes the projection tenant-visible is unchanged. Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
b41b6cce89 |
feat(controller): discover the CA source through a TenantProjection sentinel
Drive the CA-extraction reconciler off the namespaced TenantProjection sentinel a chart renders, replacing the publish-ca-cert label leg and the cluster-scoped spec.caCert declaration leg. The sentinel names the source Secret and key; the projected trust anchor takes an owner reference to the sentinel, so the lineage webhook resolves the tenant through sentinel -> HelmRelease and garbage collection prunes the projection when the chart stops rendering the sentinel. The source Secret is watched as metadata only, so the informer holds no private key material, and the one source actually read goes through the uncached reader. The sentinel's Ready status makes a missing or unpopulated source visible with kubectl instead of a silent requeue. The write-path guard, canonical-name contract, adoption rules and selectors-digest re-admission are unchanged. Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
564e1d95d9 |
feat(api): add TenantProjection CRD for declared tenant-visible projections
Introduce the namespaced internal.cozystack.io/v1alpha1 group and its TenantProjection kind. A chart renders this sentinel to declare which of an application's Secrets should be published to its tenant as a key-free copy; the controller reconciles the projected Secret from it. No tenant RBAC role grants any verb on internal.cozystack.io, so the sentinel cannot be forged from a tenant namespace. Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
d2944051c9 |
fix(cozystack-basics): gate ValidatingAdmissionPolicies on API availability
The four VAP policies in this chart (publish-ca-cert-writer plus the route, gateway, and ingress hostname policies) rendered unconditionally, so a cluster whose apiserver does not serve the ValidatingAdmissionPolicy API would receive an unrenderable resource and fail the cozystack-basics install or upgrade. Wrap all four in the same .Capabilities.APIVersions.Has guard the platform deletion-protection template already uses, and cover both the served and unavailable cases in helm-unittest. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
7e7cf7a377 |
docs(cozystack-basics): record the source/destination gating asymmetry
The writer policy gates the CA-source surface but not the destination surface — the <release>.tenant-ca projection a tenant reads. Document why that is accepted rather than closed: a platform-tier writer that could forge a projection already has an ungated path through the declared-leg source, so it is unequal depth rather than an escalation; a destination gate could not be a clean single-writer pin because the tenant-ca label has two legitimate producers (this controller and helm-controller); and the surgical fix is a UID check in lineage owner resolution, tracked as separate follow-up work. Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
c106a28026 |
feat(api): constrain the application release prefix to dot-free characters
The CA-extraction controller projects a tenant's trust anchor to "<release>.tenant-ca" and relies on the dot being a separator no release name can contain: release names are "<prefix><app name>", app names are DNS-1035 labels with no dot, so the projection name is unambiguous only while the prefix is dot-free too. That property was asserted in the controller but never enforced on the field it depends on. Add a validation pattern restricting the prefix to lowercase DNS-1123 characters, which excludes the dot by construction. Every shipped application definition already conforms. Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
82c959ac4d |
fix(cozy-lib): anchor the CA trust-anchor guard to the whole value
The caCertSecret helper emits ca.crt verbatim but only checked that a certificate block appeared SOMEWHERE in the value. Bytes before the first block or after the last one — a human-readable preamble, or a raw DER/JWK key that carries no PEM private-key header and so slips the key guard — therefore passed the check and were published to the tenant inside the trust anchor. Anchor the certificate regex to the entire value, so it matches only one or more complete blocks surrounded by whitespace and refuses anything else. This mirrors the CA-extraction controller, which reaches the same result by rebuilding the projection from the parsed blocks. Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
e8631ae920 |
feat(cozystack-basics): restrict CA-source Secret writes to cert-manager
The CA-extraction controller's label-driven leg treats a core Secret labelled internal.cozystack.io/publish-ca-cert as a trust-anchor source and lifts its ca.crt into the tenant-readable projection. That label is the whole attribution on that leg and sits on an ordinary Secret, so without a platform-side control any principal that can write a Secret in the namespace could forge a source or rewrite an existing one's ca.crt and swap the trust anchor a tenant is handed as vouched. Add a ValidatingAdmissionPolicy that allows a Secret carrying that label to be created or updated only by cert-manager's controller ServiceAccount, the sole legitimate writer of these Secrets. It covers both the forge-new (CREATE) and tamper-existing (UPDATE, including a relabel-away evasion via the old object) shapes, and by keying on the publish-ca-cert label it never touches the declared-leg source, the projection, or any unlabelled Secret. The allowed identity is pinned as a literal to the shipped cert-manager package's controller ServiceAccount; a comment marks it as the fragile seam that must move if cert-manager is repackaged. Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
f2c2abfe88 |
feat(postgres): publish the CA trust anchor to tenants
Declare CNPG's operator-created CA Secret as the postgres trust-anchor
source (spec.caCert names {{ .release }}-ca), and add the generic
internal.cozystack.io/tenant-ca selector to spec.secrets so the key-free
<release>.tenant-ca projection the extraction controller publishes becomes
readable by the tenant.
postgres is the first converged engine, and the hardest: CNPG creates the
CA Secret itself, key-bearing, asynchronously, and offers no way to label
it, so it exercises the name-driven leg end to end. The test drives the
shipped ApplicationDefinition rather than a fixture, so the wiring and the
controller cannot drift apart unseen: a CNPG-shaped key-bearing
<release>-ca appears, and the projection must be produced key-free AND
selected for tenant visibility by the shipped selector.
Signed-off-by: Aleksei Sviridkin <f@lex.la>
|
||
|
Aleksei Sviridkin
|
689e493ccf |
feat(cozy-lib): converge the trust-anchor helper on the canonical anchor
The CA-extraction controller publishes every engine's trust anchor as an Opaque "<release>.tenant-ca" Secret labelled internal.cozystack.io/tenant-ca. That label is the single, engine-agnostic selector an ApplicationDefinition puts in spec.secrets, and it is what ultimately reaches the tenant. This helper is the other producer of that same object, for charts that already hold the CA PEM at render time. It agreed with neither half of the contract: it documented "<release>-ca-cert" as the name, and it stamped no tenant-ca label at all. Both are now fixed, and both mattered. The name "<release>-ca-cert" is not merely a second choice, it is unusable: Percona Server for MongoDB creates a Secret of exactly that name itself and puts a private key in it, so a chart rendering a trust anchor there would be writing over live key material. Without the tenant-ca label, the two producers would converge on the name but not on the label. A helper-rendered anchor would be missed by the generic selector, the lineage webhook would mark it tenantresource=false, and the tenant would be locked out of the very Secret the helper exists to publish — while a controller-projected one, of the same name, worked. Also record what the tenantresource label does and does not do. The lineage webhook recomputes it from the definition's spec.secrets selectors on every admission and overwrites whatever the chart wrote, so rendering it is necessary but not sufficient for tenant read access. Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
d7232831f4 |
feat(cozystack-controller): run the CA-extraction reconciler
Wire the reconciler into the platform controller manager and grant its ServiceAccount the Secret verbs the projection needs: create/update/patch to publish the trust anchor, and delete to withdraw it when an application stops publishing one. Read was already covered by the catch-all rule, and no tenant RBAC is widened. Give the reconciler its own dedicated, scoped Secret cache rather than narrowing the manager's shared cache. Only labelled CA sources are held in that informer, so a cluster-wide Secret watch never caches every Secret, and so every private key, in the cluster; the operator-owned name-driven sources and the projections are read through the uncached API reader instead. A private cache also keeps this scoping independent of any other controller on the same manager that needs to scope Secrets differently. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
5da3fab457 |
feat(api): declare where an application's CA lives
The trust-anchor contract is a label on the CA Secret, but two engines cannot honour it. CloudNativePG builds its PKI Secrets without applying spec.inheritedMetadata, so the chart's labels never reach <release>-ca, and Percona Server for MongoDB exposes no secretTemplate at all and reconciles an out-of-band patch away. Add an optional ApplicationDefinition.spec.caCert declaring, per engine, the name of the Secret its operator creates and the key inside it that holds the certificate. Registering an engine is then data, not code. The declaration only names an object the platform already reads; nothing a tenant can read is widened by it, and a labelled Secret still takes precedence where an engine can label its own output. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
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` (
|
||
|
Andrey KolkovandClaude Opus 4.8
|
9c3bae012a |
fix(kubernetes): re-pin deleted centos:stream9 digest for kubevirt-csi-driver
quay.io/centos/centos:stream9 is a rolling tag; upstream deletes the pinned
digest whenever it publishes a new stream9 build. The digest re-pinned in
|
||
|
Myasnikov Daniil
|
8355f3e36f |
docs(seaweedfs): correct the stale "D-wedged is adopted" comments
The naming guard now refuses every two-generation state, but two comments in extra/seaweedfs still described a D-wedged duplicate (zero ready replicas) as safe to adopt and rendering through. readyReplicas is a snapshot, not proof the duplicate never served, so it cannot be told apart from D-split and the guard refuses it too. Align the state summary in the seaweedfs template comment and the test header with the presence-only refusal behaviour. No classifier change -- comments only. Address review feedback from coderabbitai on packages/extra/seaweedfs/tests/fullname_override_test.yaml. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
a099b7e086 |
fix(seaweedfs): fail the cleanup Job on a database-owner lookup error
The post-delete cleanup hook read Cluster/seaweedfs-db's Helm owner with `|| true`, so a Forbidden, a timeout or any API error collapsed to an empty owner, took the "nothing to reclaim" branch, and let the Job complete -- permanently orphaning the keep-protected Cluster the hook exists to reclaim, with nothing else that will ever collect it. Only a genuine NotFound now counts as absence; every other error prints the failure and exits non-zero so the Job retries. This matches the fail-closed handling the delete path already had. Add a regression assertion that the ownership lookup no longer swallows failures. Address review feedback from coderabbitai on packages/extra/seaweedfs/templates/hooks/cleanup.yaml. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
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
|
47e24f01d1 |
fix(platform): hand seaweedfs-db over for every instance name, not just the default
Migration 43 performs the v1.5.0 db-split hand-over: it re-owns Cluster/seaweedfs-db from <name>-system to the new <name>-db release and stamps helm.sh/resource-policy: keep, so the <name>-system upgrade — whose post-split chart no longer renders the Cluster — does not prune it as a removed resource. It compared the owning release name against the literal "seaweedfs-system". SeaweedFS is a user-creatable kind, so an instance named `foo` is owned by `foo-system` and was silently skipped: no re-own, no keep. The <name>-system upgrade then deleted the Cluster, CNPG took its PVC with it, and the filer metadata — the index for every object in that tenant's S3 — was destroyed. Data loss, not an outage: the volume PVCs still hold the bytes, nothing can find them. Verified live on the upgrade stand, which is a natural control group. All five tenants sit in the SAME failed-upgrade loop (<name>-system last-deployed is rev 1, a pre-split revision whose manifest still contains the Cluster, so every retry recomputes the same deletion): tenant-root/dsplit/l instance `seaweedfs` owner=seaweedfs-db keep Cluster alive tenant-named instance `foo` no Cluster at all Cluster GONE The only difference is that migration 43 fired for the default-named instances and skipped `foo`. That also answers whether `keep` is sufficient: three tenants survive the identical prune loop with it. Correction to the reported mechanism: this is the 1.4->1.5 db split (PR #2601, v1.5.0), not a 1.5->1.6 change — v1.5.3 and main are identical on every db-split file. And the split does protect the object; it just only protected the instances someone happened to name `seaweedfs`. - lib/seaweedfs-db-adopt.sh: shared hand-over matching the `-system` SUFFIX, so `foo-system` -> `foo-db` exactly as `seaweedfs-system` -> `seaweedfs-db`. Idempotent, and refuses a release named literally `-system` rather than annotating an owner of `-db` that no release would claim. - Migration 43 sources it: covers clusters upgrading from before 43 (1.4.x). - Migration 53 (new) re-runs it: migrations never re-run, so any cluster already at >= 44 ran the hardcoded version and is still exposed. As a pre-upgrade hook it lands before <name>-system re-renders, closing the window on THIS upgrade. This is the one that matters for 1.5.x -> 1.6. - targetVersion 53 -> 54. Tests fail on unfixed code: the non-default-name case produces no ANNOTATE at all against the hardcoded 43, while the default-name case passes — which is precisely the bug's shape. They drive the real migration scripts against a fake kubectl, following hack/migration-50-etcd-adopt.bats. LIMIT, stated plainly: a Cluster that is already deleted cannot be recovered by either migration — there is nothing left to re-annotate and the PVC went with it. Such a tenant needs seaweedfs-db restored from a backup. Runbook Step 0 adds the ownership audit that distinguishes at-risk from already-lost, and records that <name>-db can report Ready while its Cluster is gone (it rendered fine; a later <name>-system prune removed it and Flux has not re-checked) — so the audit trusts `kubectl get cluster`, not the HelmRelease status. Review round 2 — two ways this still lost the database: - OWNERSHIP IS NOT SAFETY. The helper skipped any Cluster already owned by <name>-db, treating ownership as proof the hand-over was done. It is not: where the hand-over was skipped, <name>-system prunes the Cluster and <name>-db RECREATES it under its own ownership with NO keep, while <name>-system's prune baseline still lists it — so the next reconcile deletes it again. That is the observed delete/recreate loop, and the skip walked straight past it. Verified on the stand: tenant-l and tenant-root are seaweedfs-db-owned AND their <name>-system deployed revision (rev 1) still contains the Cluster, so only keep saves them; tenant-fresh is seaweedfs-db-owned without keep but was installed after the split, so its baseline never had one. Telling those apart needs the release's deployed manifest, which this script cannot read cheaply or reliably. The costs are asymmetric — a needless keep leaves an orphan on delete (now reclaimed by the cleanup hook), a missing one loses the database — so keep is stamped on every Cluster owned by either side of the split. - FAIL OPEN, THEN STAMP. `set -euo pipefail` does not abort on a failing command substitution in a `for` word-list: the loop ran zero times, the script continued, and stamp_cozystack_version ran anyway. Migrations never re-run, so one throttle, RBAC hiccup, or not-yet-established CNPG apiservice behind the pre-upgrade hook permanently left every at-risk tenant exposed with no later migration to catch them. Reproduced: against the old helper a failing fleet scan still logged STAMP 54. Now every kubectl failure is fatal EXCEPT the two that genuinely mean "nothing to do" — the CNPG resource type not being served (the fail-open that IS load-bearing: a cluster without CNPG must still upgrade) and a Cluster vanishing between scan and read. An unreadable annotation is no longer indistinguishable from an absent one, and the unowned case now warns instead of passing silently. - ORPHAN ON DELETE. `keep` is permanent (removal deferred to the 1.7 batch migrations) and also survives the <name>-db release's own uninstall, so `kubectl delete seaweedfs <name>` left the CNPG Cluster, its Postgres pods and its PVCs behind — the cleanup hook selects app.kubernetes.io/instance=<name>-system, which the db PVCs (cnpg.io/cluster=seaweedfs-db) do not carry. Migration 53 widened that from pre-1.5 default-named tenants to every instance name, so the hook now deletes the Cluster explicitly; its PVCs carry ownerReferences to it and follow. - Tests assert the stamped VERSION (44 / 54), not a bare "STAMP": a wrong number would loop run-migrations.sh forever and the old assertion could not tell. Review round 3 — the reclaim introduced a data-loss path of its own: - DELETING ONE APP DELETED ANOTHER APP'S DATABASE. The db chart hardcodes the Cluster name `seaweedfs-db` for every instance, and the reclaim was scoped by that name in the namespace rather than by what the release owns. templates/seaweedfs-db.yaml gates the <name>-db HelmRelease on `topology != "Client"`; the cleanup hook had no such gate. Verified by render: `helm template s3-remote . --set topology=Client` emits ZERO s3-remote-db HelmReleases yet still emitted the Role granting delete on clusters/seaweedfs-db AND `kubectl delete cluster.postgresql.cnpg.io -n tenant-root seaweedfs-db`. So in a namespace running a server instance plus a Client, deleting the CLIENT destroyed the SERVER's filer metadata — CNPG takes the PVC via ownerReferences — for an app the operator never touched. Reachable the same way via a second Simple instance, whose <name>-db loses Helm's ownership check but which still installs and still carries the hook. main's hook was instance-scoped; this was new, and mine. Now gated exactly as seaweedfs-db.yaml is, and the Job re-checks meta.helm.sh/release-name == <release>-db at run time before deleting. The gate stops the permission being granted where it can never be legitimate; the runtime check stops the delete where it can. - `|| echo` turned any API/authz/admission failure into exit zero, so backoffLimit never retried and hook-delete-policy removed the only cleanup Job while the keep-protected Cluster stayed orphaned. A failed reclaim now fails the Job. - TEST SHELL. The migrations run under /bin/sh = busybox ash (the image is FROM alpine, run-migrations.sh is #!/bin/sh), but the tests drove them with bash — so the fail-closed guarantees were asserted in a shell that never runs them, which matters more than usual given the bug they cover was a fail-open. The scripts are now invoked via `sh` and the fake kubectl is POSIX sh. Verified directly against the production base image (busybox 1.37.0 / alpine:3.24): `set -o pipefail` is supported, every script passes `ash -n`, and the failing-fleet-scan path exits non-zero without stamping under real ash. The recipe is in the bats header. 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
|
6338b0b2fd |
fix(migrations): derive the etcd-adoption snapshot target from the projected bucket creds (#3335)
## What this PR does
Migration 50 takes a mandatory S3 safety snapshot before adopting live
etcd onto the v1alpha2 operator. It is a **`pre-upgrade` hook**, so
every chart-rendered resource it reads still belongs to the version
being upgraded **from**. It resolved the snapshot target from the live
`cozy-default-etcd` Strategy CR — and on a v1.5.x cluster that CR cannot
be trusted to describe a reachable target. The platform upgrade is gated
behind the snapshot, so nothing can self-heal first: the corrected
`backupstrategy-controller.endpoint` helper only reconciles **after**
the upgrade this hook blocks.
### Two failure modes, both a hard block
**1. The Strategy CR does not exist.** `strategy-etcd-default.yaml` is
guarded by `{{- if $bucketName -}}`, whose helper `lookup`s the
BucketClaim's `.status.bucketName`. That BucketClaim is created by the
*same chart*, so on first render the lookup is empty and the Strategy is
silently skipped. Helm `lookup` only runs at install/upgrade time, so a
cluster that reached 1.5.x in one hop never grows the CR on reconcile —
a forced reconcile does not produce it.
**2. The Strategy CR exists carrying
`http://seaweedfs-s3.tenant-root.svc.cozy.local:8333`** (the static
v1.5.x `backupStorage.endpoint` default). Cozystack ships SeaweedFS with
`enableSecurity=true`, so `:8333` is a **TLS listener** — plaintext
against it fails the handshake and every snapshot dies. Fixing the
*scheme* is not sufficient: as `backupstrategy-controller.endpoint`
documents, the Etcd Strategy's S3 schema has no
`caCert`/`insecureSkipVerify`, so the self-signed in-cluster endpoint is
unusable at **any** scheme. A trusted-cert endpoint is required.
## The fix
Take the coordinates from `cozy-backups-creds` instead. The projector
writes it from the **COSI-provisioned bucket's system credentials** —
bucket provisioning, not chart values — so it describes the live bucket
regardless of which version rendered the charts. It carries `endpoint`,
`bucketName`, `region` and `forcePathStyle` alongside the AWS keys, and
`resolve_platform_backup_args` already read this Secret to validate
those keys, so preferring it adds no new dependency.
Both modes disappear: mode 1 because the CR is no longer needed, mode 2
because its endpoint is ignored.
### Classifying COSI vs. external S3
The projected endpoint is a bare host fronted by the always-TLS S3
ingress, so on the platform-managed path we force `https://` — exactly
what `backupstrategy-controller.endpoint` does. But the admin-managed
external-S3 case (`provisionBucket: false`) must keep
`.Values.backupStorage.endpoint` verbatim, since it may legitimately be
plaintext against a private store. The two therefore have to be told
apart.
**Endpoint presence cannot do that.** `ProjectBackupCredentials`
substitutes `BACKUP_STORAGE_ENDPOINT` when the source Secret is silent
and then *fails loud* rather than projecting an endpoint-less Secret, so
a projected `endpoint` key is **always** present and **always**
scheme-stripped — for external S3 too. Classifying on its absence would
force `https://` onto a plaintext external store and never consult the
CR, which is a regression against a supported, documented configuration.
Instead the hook asks whether the bucket the credentials name is one
**COSI actually provisioned**: it matches the projected `bucketName`
against `.status.bucketName` of any BucketClaim. That is keyed on data
rather than names, so it is independent of `backupStorage.namespace` /
`.bucketName` / `bucketNameOverride` — all supported Package-CR
overrides the hook cannot see — and a claim-name lookup would
misclassify a renamed bucket as external.
Resolution is then:
| cluster state | endpoint | other coordinates |
|---|---|---|
| a **live** BucketClaim claims the bucket | projected host, forced
`https://` | projected Secret |
| **no** claim claims the bucket (external S3) | Strategy CR, scheme
verbatim | Strategy CR |
| only a **Terminating** claim claims it | refuse | — |
| BucketClaim API unreadable | refuse | — |
On the external path every coordinate comes from the CR, never a
Secret/CR hybrid: `strategy-etcd-default.yaml` pairs `bucket` with
`credentialsSecretRef: cozy-backups-creds`, so CR coordinates plus the
projected credentials is exactly the pairing the cluster's real etcd
BackupJobs already use.
### Deliberate behaviour changes
Two states now refuse where `main` proceeded. Both are cases where the
classifier cannot read its input, and adopting live etcd is
irreversible, so it fails closed with an actionable message rather than
guessing:
- **A Terminating BucketClaim on the projected bucket.** Reachable via
COSI → external reusing a retained bucket name, where the
`bucketclaim-protection` finalizer wedges the corpse permanently. Both
readings are reachable and guessing wrong in either direction also ends
in a block — just with a confusing TLS/handshake error instead of a
precise one. Remedy: let COSI reap the claim, or clear its finalizer,
then re-run (migration 50 is idempotent).
- **A sustained BucketClaim API read failure** on a cluster that has
COSI installed. A genuinely absent CRD is still a definitive, zero-cost
"external" answer and costs no retries.
### The escape hatch is now reachable
The script has always supported `ETCD_ADOPT_SKIP_BACKUP=1` for clusters
that intentionally have no backup storage, but `migration-hook.yaml`
passed no such variable, so the failure paths advised an action the
platform made impossible. It is now plumbed as
`migrations.etcdAdoptSkipBackup`, an explicit opt-in defaulting to off,
and the operator-facing messages name that knob rather than a variable
nobody can set. It also unblocks the two refuse states above.
The template normalises to the exact string the script matches: a bare
bool renders `true`, which the script's `= "1"` test would accept into
the Job spec and then silently ignore — the worst outcome for a valve
reached for under duress. Anything unrecognised resolves to `"0"`, so
taking the hatch requires an affirmative spelling and never a typo. A
Platform Package predating the key renders cleanly as off.
The script's own check was widened alongside it to accept `1|true|yes`
case-insensitively, so a hand-run image honours what an operator
actually types. That is a behaviour change beyond plumbing, and it has a
cost worth naming: if the template's normalisation were ever dropped, a
raw bool would previously have failed safe (snapshot taken) and now
fails unsafe (snapshot skipped). The rendered value is pinned by a unit
test to prevent that.
`migration-hook.yaml` had no test coverage at all before this: the Job
renders only when a `lookup` of `cozystack-version` reports a version
below target, and `lookup` returns nil under `helm template`, so lint
and CI never rendered the Job. That is structurally why an unreachable
escape hatch shipped unnoticed. The new suite mocks the ConfigMap so the
Job is rendered and its env asserted by value.
## Tests
The bats fixture modelled `cozy-backups-creds` as carrying only the AWS
keys, and test 1 asserted the broken plaintext endpoint **as expected
output** — written against the implementation rather than the
requirement, against a mock that did not match a real cluster, which is
why this shipped green. The fixture now models what the projector and
COSI really produce.
Added, each verified to fail before the corresponding fix and pass
after:
- platform path derives `https://` from the projected coordinates, and
the in-cluster host never appears
- absent Strategy CR still resolves (mode 1)
- external S3 keeps the CR endpoint's scheme verbatim
- external S3 takes **every** coordinate from the CR, never a Secret/CR
hybrid
- overridden bucket coordinates still classify as COSI
- a large multi-tenant claim list still classifies correctly (the match
must not depend on list size or position)
- a Terminating claim on the projected bucket refuses, and never scales
or adopts
- an unreadable BucketClaim CRD / list refuses
- a live claim wins over an unrelated Terminating one
- COSI keeps the projected bucket even when the CR names another
- the skip-backup hatch is honoured, and an unrecognised value does not
skip
Suite: 15 → 28, plus a new helm-unittest suite covering the hook's
rendered env (chart: 81 → 88 tests).
## Validation
Reproduced end-to-end on a throwaway 3-node stand (fresh v1.4.5 → v1.5.3
→ this branch's build). Before: mode 1, version stamp stuck,
`cozy-backups` Bucket Ready, `backupstrategy-controller` Ready, creds
projected, **zero** Strategy CRs. After: the stamp advanced and the hook
completed, with endpoint, bucket, region and path-style all derived from
the projected Secret and no Strategy CR in existence. Failure was safe
in both cases — it refuses before any mutation: etcd stayed 3/3 Running,
the legacy CR un-adopted, no scale-down. The derived `https://` endpoint
reproduces the 1.6 chart helper's own output byte-for-byte, and the
projected `bucketName` equals the BucketClaim's `.status.bucketName` on
a live stand.
## Follow-ups this PR deliberately does not do
- **Fix the `if $bucketName` lookup race** in
`strategy-etcd-default.yaml`, so the Strategy exists at all on affected
clusters. This PR makes migration 50 independent of it, but the missing
CR likely means **default etcd backups are silently broken on 1.5.x
clusters**, upgrade or not — the backup e2e is `.disabled`, so CI cannot
see it. Possibly related to #3256. Worth its own issue.
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **New Features**
* Backup destination for migration 50 adoption can now derive from
projected credentials when available.
* Added `migrations.etcdAdoptSkipBackup` to control whether the
migration job skips pre-adoption snapshot safety checks.
* **Bug Fixes**
* Improved COSI vs external S3 classification and endpoint
normalization; clearer failures when destination can’t be resolved.
* `ETCD_ADOPT_SKIP_BACKUP` now honors true/false and rejects
unrecognized values (does not skip by accident).
* **Tests**
* Expanded end-to-end coverage for projected vs external S3 behavior,
precedence rules, and non-destructive failure scenarios.
* Added migration hook tests validating `ETCD_ADOPT_SKIP_BACKUP` env-var
mapping and defaulting.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
|
||
|
myasnikovdaniil
|
1d2418e4c4 |
fix(mariadb): derive my.cnf sizing from the instance's own resources (#3345)
## What this PR does The chart shipped a my.cnf with sizing hardcoded for a large dedicated host — `innodb_buffer_pool_size=60G`, `innodb_log_file_size=4096M`, `key_buffer_size=1024M`, `max_connections=4096` — while the default preset `t1.nano` gives the pod 250m CPU and 128Mi of memory, and the default volume is 10Gi. Nothing connected the two, so the defaults asked a 128Mi container for a 60G buffer pool and made every first boot preallocate a redo log worth 40% of the volume. That is not only a slow start. On the current default, MariaDB 10.6 is OOM-killed before it finishes checking its configuration, and on 11.8 the server refuses to start at higher limits with `Cannot map innodb_buffer_pool_size_max`. It survives in practice only because a large node's overcommit heuristics allow the mapping and a small database never touches most of it. Sizing now derives from the instance's own limits through the same helper that fills `resources` on the CR, so both `resourcesPreset` and an explicit `resources` block work. The buffer pool takes half the memory limit, rounded down to a 128 MiB multiple because InnoDB on the 10.6 line allocates in chunks of that size and rounds requests up — a naive value would hand the pod a pool larger than its own limit. Half rather than the conventional 70-80% because that advice assumes a dedicated host where the remainder is page cache and overshoot means swapping; under a cgroup limit the same budget also has to cover per-connection buffers, temporary tables, the key cache and mysqld's own heap, and overshoot means the kernel kills the process. The redo log follows the smaller of the pool and 5% of the volume, and the remaining allocations scale from memory or CPU with floors at the server's own defaults. `innodb_buffer_pool_instances` and `innodb_thread_concurrency` are removed rather than scaled: every shipped server version logs that they were removed and now do nothing, so this only drops a startup warning. Settings that describe durability or disk behaviour rather than instance size — `innodb_flush_log_at_trx_commit`, `innodb_flush_method`, `innodb_io_capacity`, `innodb_fast_shutdown` — are deliberately left alone, since the chart cannot derive them from a pod's resources. ### Upgrade safety An existing instance has a 4096M redo log and will get a smaller one. Shrinking it was verified on every shipped server version — 10.6.24, 10.11.15, 11.4.9 and 11.8.5 — after both a clean shutdown and a SIGKILL with writes still unflushed. In all cases the server replayed crash recovery first and resized the log afterwards, with data intact by checksum and `CHECK TABLE ... EXTENDED`. The ConfigMap carries no operator watch label, so a running healthy instance keeps its current sizing until its next restart; an instance already crash-looping on the old values picks the new ones up when it restarts. ### Release note ```release-note fix(mariadb): size my.cnf from the instance's own CPU, memory and volume instead of hardcoded values that assumed a large dedicated host. Instances on small presets previously advertised `max_connections=4096`, which they could not serve; the limit now scales with memory and starts at the server default of 151. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - MariaDB server settings now scale automatically based on CPU, memory, storage, and configured resource limits. - Improved buffer pool, redo log, connection, cache, temporary table, and thread sizing across instance presets. - Large instances can use buffer pools beyond the previous fixed limit. - **Bug Fixes** - Improved sizing for small-volume deployments. - Removed obsolete MariaDB configuration options for improved version compatibility. - Explicit resource settings now take precedence over preset values. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
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 --> |
||
|
myasnikovdaniil
|
3affec9920 |
fix(cert-manager): move the webhook off the kubelet's port (#3359)
## What this PR does The cert-manager webhook listens on port 10250 — the kubelet's port. When a connection to the webhook Service resolves to a node IP rather than the pod IP, it reaches the kubelet, which answers the TLS handshake with its own node serving certificate. The API server then rejects every cert-manager admission call in the cluster: ```text failed calling webhook "webhook.cert-manager.io": ... x509: certificate is valid for srv3, not cert-manager-webhook.cozy-cert-manager.svc ``` The certificate is valid — it is simply the kubelet's, issued for the node name. That signature is what separates this from the cainjector failure mode, which reports `certificate signed by unknown authority`; that possibility was checked and ruled out on the run where this was observed. Here it blocked the whole platform bootstrap and pointed the operator at cert-manager rather than at the routing layer. The value comes straight from the vendored chart, where upstream chose it deliberately for a different constraint: on GKE private clusters the API server may only reach nodes on 443 and 10250. cozystack did not override it. Moving the webhook to 10260 is upstream's own remedy for this exact error. The cert-manager troubleshooting guide documents the identical signature — under a different cause, EKS Fargate, where each microVM runs a kubelet that already holds 10250 — and prescribes `--set webhook.securePort=10260`. This does not fix the misroute, whose root cause is still unestablished; the run carries no dataplane state, so candidates such as a stale service map during bootstrap remain unproven. What it removes is the trap that turns a rare, transient misroute into a cluster-wide failure attributed to the wrong component. With the collision gone, the same misroute surfaces as a connection refusal. The override lives in the package-root values file, which `make update` leaves alone, so it survives re-vendoring. ### Upgrade safety The rendered webhook Deployment has one replica and no explicit strategy, so the defaults apply: `maxUnavailable` 0 and `maxSurge` 1. The replacement Pod must become Ready before the old one is removed. The Service targets a named port rather than a number, so during the overlap the EndpointSlice controller routes to each Pod on the port it actually serves. There is no window with zero ready endpoints, and no maintenance step is required. Had the Service pinned the port numerically, this upgrade itself would have broken cluster-wide admission. ### Not addressed here `external-secrets-operator` runs its webhook on 10250 with the same `failurePolicy: Fail`, so it carries the identical exposure and deserves the same treatment; `metrics-server` also uses 10250 but is an APIService rather than an admission webhook. Both are left out to keep this change to one component. Closes #3355 ### Release note ```release-note fix(cert-manager): move the admission webhook off port 10250, which the kubelet also serves. A connection misrouted to a node answered with the kubelet's certificate, failing every cert-manager admission call cluster-wide with a misleading x509 error. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Configured the cert-manager webhook to use secure port 10260. * Added coverage to verify secure port configuration, HTTPS service routing, and disabled host networking. * **Documentation** * Added configuration comments explaining the secure port selection and its role in avoiding service routing conflicts. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
myasnikovdaniil
|
2830ad977c |
chore(packages): drop duplicate test targets (#3352)
## What this PR does Three package Makefiles — `apps/mariadb`, `apps/kafka` and `extra/monitoring` — declared an identical `test` target twice, so every invocation printed: ```text Makefile:19: warning: overriding commands for target `test' Makefile:7: warning: ignoring old commands for target `test' ``` Make names the lines in that warning itself: it was already running the second recipe and discarding the first, so the earlier declaration was dead in each case and removing it changes nothing but the noise. All six recipes were `helm unittest .`. Which copy is dead is not the same across the three, so this is not a mechanical edit. In `monitoring` the `.PHONY: test` line sits above the *dead* recipe rather than the live one; since `.PHONY` applies to the target name wherever it is declared, the declaration stays and only the recipe beneath it goes. Default goals are unaffected: `hack/package.mk` sets `.DEFAULT_GOAL=help` and is included ahead of any target in these files, so `test` was never selected by first-target-wins. Confirmed with `make -p --dry-run` before and after, and each package's tests still run — 11, 35 and 9 respectively, with no warnings left. ### Release note ```release-note NONE ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Streamlined build workflows for MariaDB, Kafka, and Monitoring applications. * Removed redundant test target definitions while preserving the existing Helm unit test capability. * Other generation, update, and image-building workflows remain unchanged. <!-- 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> |
||
|
Myasnikov Daniil
|
efa85a4f61 |
feat(platform): make the etcd-adoption skip-backup hatch reachable
Migration 50 has honoured ETCD_ADOPT_SKIP_BACKUP since it was written, and its failure paths tell operators to "re-run with ETCD_ADOPT_SKIP_BACKUP=1". But migration-hook.yaml passes only NAMESPACE/CURRENT_VERSION/ TARGET_VERSION, and the Job is a Helm hook the chart re-renders, so there has never been a way to set it. The script advertised an escape the platform made impossible. The endpoint-classification work landing alongside this makes that worse: it adds refuse-rather-than-guess paths, so more clusters can now legitimately stop here. Plumb it through migrations.etcdAdoptSkipBackup, default false. Skipping the pre-adoption snapshot rewires ownership of LIVE etcd storage with no way back, so it stays opt-in and deliberate — the flag is the last resort for a cluster that has nothing to fix, not the first response to a refusal. Render "1"/"0" rather than the bare bool. The script matches the literal string "1", so a bool would render "true", sail through the Job spec looking correct, and be ignored — a safety valve that is set but silently disregarded is worse than one that was never offered, because the operator believes they opted in. The value is normalised through a string, so a bool, a quoted "true" out of the Package CR's Values JSON, or a numeric 1 all land on the same answer, while nil or garbage resolves to "0": taking this hatch must require an affirmative spelling, never a typo. Resolved with `dig` so a Platform Package predating the key renders "0" instead of failing the render of the entire platform chart and bricking every upgrade. Widen the script's own check to accept 1/true/yes as well. The template guarantees the GitOps path, but an operator running this image by hand will reasonably type "true", and that is exactly the moment they are already stuck. The two layers cover independent failure modes. Emit the var unconditionally, including the "0" default, so the rendered Job states the cluster's safety posture outright rather than leaving an operator to diff values to find out whether a snapshot was skipped. Point the failure messages at the values path that actually exists, and describe it honestly: editing the Platform Package and letting Flux re-render is a real operation, not a one-liner, and the flag has to be reverted afterwards or the next etcd migration skips its snapshot too. Fixing the condition the migration names remains the recommended path — the migration is idempotent, so re-running after a fix is safe. templates/migration-hook.yaml had no coverage at all, because the Job only renders when a lookup of the cozystack-version ConfigMap reports a version below targetVersion, and lookup is nil under helm template. The new suite mocks that ConfigMap and asserts the RENDERED VALUE — enabled, disabled, key absent, stringified true, garbage, and the pre-existing three env vars — rather than merely that the variable exists, which is what let the trap above go unnoticed in the first place. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
0ba5337526 |
fix(migrations): key the etcd-adoption endpoint scheme off the claimed bucket
Deriving the safety-snapshot target from the projected cozy-backups-creds Secret is right, but detecting the admin-managed external-S3 case (provisionBucket: false) by "the projected endpoint is absent" is not: that state does not exist. The projector substitutes BACKUP_STORAGE_ENDPOINT and fails ReasonSourceMalformed when both sources are empty, so a projected Secret always carries a non-empty, scheme-stripped endpoint. So for provisionBucket: false against a plaintext external S3 the projected endpoint is a bare host, the script saw it non-empty, forced https:// and never consulted the Strategy CR — a regression against main, which read the CR verbatim and passed http:// through. Being a pre-upgrade hook with no ETCD_ADOPT_SKIP_BACKUP plumbed through migration-hook.yaml, that is a total upgrade block with no operator escape. Classify on data instead: match the projected bucketName against the .status.bucketName of any BucketClaim. The COSI driver assigns that name and the projector republishes exactly it (packages/system/bucket user-credentials.yaml reads BucketInfo spec.bucketName), so a match means the creds describe a COSI bucket => the host is the always-TLS S3 ingress => force https, as the 1.6 chart helper does. No match means external S3. Keying on data rather than on the claim's name and namespace matters twice. backupStorage.namespace/.bucketName are supported Package-CR overrides that this hook cannot see, so a name lookup would miss a renamed bucket and fall back to the v1.5.x plaintext CR — the original P0. And a BucketClaim wedged Terminating (its cosi bucketclaim-protection finalizer outlives an uninstalled COSI controller) keeps answering a name lookup while its status names some other bucket. Split the two paths by which source is authoritative. On COSI the projected coordinates win: the bucket is the COSI-assigned name the CR can only reproduce through a live lookup that may never have run. On external S3 the Strategy CR wins WHOLESALE, not just for the endpoint — the CR renders backupStorage.* and points at this same cozy-backups-creds, so CR coordinates plus these credentials is exactly what the cluster's own BackupJobs already use, and the bucket an operator will look in. Keeping the Secret's bucket while taking the CR's endpoint assembled a pairing nothing else produces: the projector copies bucketName straight from the admin-managed source Secret and never from backupStorage.bucketName, so the two can disagree outright and the snapshot could land in a bucket the platform never writes. Relying on the CR is safe on this path alone, because bucketName short-circuits to values before the BucketClaim lookup that races on the COSI path, so the CR is guaranteed present. Match with a single jq `any`, never `jq -r | grep -q`: grep exits on the first match and SIGPIPEs jq, and pipefail reports jq's 141 rather than grep's 0, so a MATCH returns as a failure and the cluster is misclassified external. It is a race on the list outgrowing one stdio write (~8KB) with only a last-line match safe, so it strikes exactly the big multi-tenant clusters that can least afford it, and backoffLimit 3 can decide differently on each retry. Measured with the match first, 20 runs each: 300 claims went 0/20 correct (rc=141 every time) and 2000 likewise, while `any` is 20/20 at 3, 300 and 2000. Refuse rather than guess when the answer is not knowable. Three states qualify: the BucketClaim API is unreadable; the list is not interpretable (no producer emits that, but without a shape gate it reads as "no claim" = external = the plaintext P0, silently); or the bucket is claimed ONLY by a Terminating claim. That last one is ambiguous — an admin who moved COSI -> external S3 reusing the bucket name, versus a still-COSI cluster whose claim is mid-recreate — and both readings fail as the same confusing handshake error when wrong, so a guess buys no reliability while reading it as "external" specifically resurrects the plaintext-:8333 P0. A live claim still wins over an unrelated Terminating one, so healthy clusters are unaffected. Each refusal names the condition and the operator action; the migration is idempotent, so re-running after resolving it is the remedy. Behaviour change worth noting: a cluster on external S3 that still has COSI installed now refuses on a sustained BucketClaim-API failure where main would have proceeded off the Strategy CR. That is the intended trade — main proceeded only because it never had to tell the two apart — but it does mean an apiserver outage can now block this hook where it previously did not. ETCD_ADOPT_SKIP_BACKUP is still not plumbed through migration-hook.yaml, so the messages deliberately point at resolving the condition rather than at an escape hatch the operator cannot reach. Force the scheme only onto a non-empty host: an absent endpoint must degrade to the CR as main did, not to a literal "https://" that would pass the final non-empty guard and reach etcd-migrate as a destination. The old test modelled external S3 by dropping the projected endpoint entirely — a state the projector cannot produce — so it green-lit the regression. It now models what the projector really writes, and the suite pins the stale-claim, overridden-coordinates, empty-endpoint, 300-claim-multi-tenant, Terminating-match, hybrid-coordinate and unreadable/uninterpretable API cases. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
9ceccd2b07 |
fix(migrations): derive the etcd-adoption snapshot target from the projected bucket creds
Migration 50 is a pre-upgrade hook, so every chart-rendered resource it reads still belongs to the version being upgraded FROM. It read the snapshot target from the live cozy-default-etcd Strategy CR, which on a v1.5.x cluster is broken in one of two ways — and the platform upgrade is gated behind this snapshot, so neither can self-heal first: 1. Absent. strategy-etcd-default.yaml is guarded by `if $bucketName`, whose helper looks up the BucketClaim status. That BucketClaim is created by the same chart, so on first render the lookup is empty and the Strategy is silently skipped. Helm lookup only runs at install/upgrade time, so a cluster that reached 1.5.x in one hop never grows the CR on reconcile. 2. Present, carrying the static v1.5.x default http://seaweedfs-s3.tenant-root.svc.cozy.local:8333. Cozystack ships SeaweedFS with enableSecurity=true, so :8333 is a TLS listener and plaintext against it fails the handshake. Fixing the scheme alone is not enough: the Etcd Strategy's S3 schema has no caCert/insecureSkipVerify, so the self-signed in-cluster endpoint is unusable at any scheme and a trusted-cert endpoint is required. Either way migration 50 hard-fails and blocks the upgrade, and the documented ETCD_ADOPT_SKIP_BACKUP escape hatch cannot be reached — migration-hook.yaml passes only NAMESPACE/CURRENT_VERSION/TARGET_VERSION. Take the coordinates from cozy-backups-creds instead. The projector writes it from the COSI-provisioned bucket's system credentials — bucket provisioning, not chart values — so it describes the live bucket regardless of which version rendered the charts, and it carries endpoint, bucketName, region and forcePathStyle alongside the AWS keys. resolve_platform_backup_args already read this Secret to validate those keys, so preferring it adds no new dependency. The endpoint is a bare host fronted by the always-TLS S3 ingress, so force https://, exactly as the 1.6 chart helper backupstrategy-controller.endpoint does. The Strategy CR remains the fallback for the admin-managed external-S3 case (provisionBucket: false), where .Values.backupStorage.endpoint is authoritative and may legitimately be plaintext against a private store; that path is detected by the absence of a projected endpoint and its scheme is preserved verbatim. The bats fixture modelled cozy-backups-creds as carrying only the AWS keys, and test 1 asserted the broken plaintext endpoint as expected output — the test was written against the implementation rather than the requirement, which is why the defect shipped green. The fixture now carries the coordinates the real projected Secret has, test 1 asserts the derived https endpoint, and two cases are added: an absent Strategy CR still resolving, and external S3 keeping the CR endpoint verbatim. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
myasnikovdaniil
|
2642965026 |
fix(mariadb): widen startup probe budget so bootstrap cannot be killed (#3344)
## What this PR does The operator builds the MariaDB startup probe without setting `failureThreshold`, so it falls back to the Kubernetes default of 3. The kubelet only starts probing after `initialDelaySeconds` and kills the container on the Nth consecutive failure, so the budget is `initialDelay + (failureThreshold - 1) * period` — with the operator's 20s delay and 10s period that is 40 seconds, exactly the budget of the liveness probe the startup probe exists to defer. Bootstrapping a fresh datadir does not reliably finish inside 40 seconds under a constrained CPU limit or slow replicated storage, and overrunning it is not a slow start that recovers on the next attempt. The kubelet kills the container after `mariadb-install-db` has created the system tables but before the entrypoint's temporary server applies the root grant. The restarted entrypoint keys `DATABASE_ALREADY_EXISTS` off that directory, skips initialization for good, and the server comes up with the install-time root account — local only and passwordless — instead of the credentials the operator holds. Every probe from then on fails with `Access denied for user 'root'`, the pod crash-loops permanently, and the install times out on a MariaDB that can never become ready. Raising `failureThreshold` to 30 lifts the first-boot budget to 310 seconds. Only the threshold is set, so the operator keeps its own probe handler — the agent HTTP endpoint when replication is enabled, the exec check otherwise. Liveness and readiness keep the operator defaults and stay suppressed until the startup probe first succeeds, so a server that hangs after a successful start is still caught as quickly as before. The defect is still present in the latest upstream operator release, so a version bump does not resolve it. The e2e suite needed a matching adjustment. The chart hands the operator a `MariaDB` CR and ships no workload of its own, so the HelmRelease goes Ready as soon as helm applies the manifests and never waits for a pod — the gate that actually covers first boot is the Endpoints assert, which sat at 3m, below the budget the probe now grants. That one moves to 10m; the HelmRelease ceilings are left as they were. ### Release note ```release-note fix(mariadb): widen the startup probe budget so a slow first boot cannot leave the datadir permanently unusable ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved MariaDB startup reliability by allowing more time for initialization before startup checks fail. * Reduced the risk of containers being terminated during first-time bootstrap. * **Tests** * Added coverage for startup probe timing across single-replica and replicated deployments. * Verified that liveness and readiness probe defaults remain unchanged. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
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> |
||
|
Aleksei Sviridkin
|
055c8c755c |
chore(packages): drop duplicate test targets
Three package Makefiles declared an identical `test` target twice, so every invocation printed "overriding commands for target 'test'" and "ignoring old commands". Make names the lines in that warning: it was already running the second recipe and discarding the first, so the first declaration was dead in each case and removing it changes nothing but the noise. Which copy is dead is not the same in all three, so the edit is not mechanical. In monitoring the `.PHONY: test` line sits above the dead recipe rather than the live one; `.PHONY` applies to the target name wherever it is declared, so the declaration stays and only the recipe under it goes. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
ba2623f31d |
chore(mariadb): drop the duplicate test target
The package Makefile declared an identical `test` target twice, so every invocation printed "overriding commands for target 'test'" and "ignoring old commands". Make already ran the second recipe and discarded the first, so removing the first is behaviour-preserving. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
4f9ff665da |
fix(mariadb): derive my.cnf sizing from the instance's own resources
The shipped my.cnf hardcoded the tuning of a large bare-metal host: a 60G buffer pool, a 4096M redo log, a 1024M MyISAM key cache, 4096 connections and 24 pool threads. The chart defaults to t1.nano, which is 250m of CPU and 128Mi of memory with a 10Gi volume, so by default a 128Mi container was asked to map 60G of buffer pool and preallocate a redo log worth 40% of its data volume. That is not merely wasteful. With the Linux default heuristic overcommit the 60G mapping is refused outright on a node without the memory to back it, and mariadbd exits with "Cannot map innodb_buffer_pool_size_max"; where the mapping is permitted, the eagerly allocated key cache alone exceeds a small instance's limit and the pod is OOM killed during startup. A nano instance only survives on a node large enough for the mapping to be granted and with a working set small enough never to touch it. Every value that is a memory allocation or a thread count now derives from the memory and CPU limits resolved by the same cozy-lib helper that fills the MariaDB CR's resource stanza, so an explicit resources block and a preset both flow through. Settings that encode durability or storage behaviour rather than size -- the flush method, flush_log_at_trx_commit, io_capacity, fast_shutdown -- are left alone, because the chart cannot infer them from the pod. The buffer pool takes half of the memory limit. The usual advice to give InnoDB 70-80% of RAM assumes a dedicated host where the rest is page cache and overshooting merely swaps; under a cgroup limit the same budget must also cover per-connection buffers, temp tables, the key cache and mysqld's own heap, and overshooting is a kill rather than a slowdown. Sizes above 128 MiB are rounded down to a whole number of 128 MiB chunks. On the 10.6 line InnoDB allocates the pool in 128 MiB chunks and rounds a request up, which would hand the pod more pool than its limit allows; the newer lines either autosize the chunk or ignore it outright, so there the rounding only costs a little pool on a limit that is at least 256 MiB and not a multiple of it -- the rounding applies to half the limit, and below one whole chunk it does not fire at all -- and buys one effective size across every version the chart ships. Most caps are the previously shipped value, so an instance large enough to reach one is sized as it is today, and most floors are the server's own default, so a small instance is not configured more restrictively than an untuned MariaDB. Three clamps deliberately break that symmetry. The buffer pool has no cap: pinning it to the old 60G would keep the largest instances sized for a host they do not run on, so an instance with 128Gi or more of memory is the one case where a value goes up rather than down, to half of a limit it already has. The MyISAM key cache goes the other way, capped at 128M against the 1024M shipped and floored at 8M against a 128M default, because it is allocated eagerly at startup and MariaDB's own system tables are Aria rather than MyISAM, so on a small instance it is close to dead weight charged against the limit. The IO threads are the third, floored at 2 against a default of 4, because they should follow the CPU limit rather than the host. The one user-visible narrowing is max_connections on small instances, which falls from 4096 to the stock default of 151 -- a limit those instances could not have served anyway, and one that now rises automatically as the instance is sized up. Changing the redo log size is safe on every version this chart ships. InnoDB replays the existing log during crash recovery and only then rewrites it at the new size. This was verified on all four shipped versions -- 10.6.24, 10.11.15, 11.4.9 and 11.8.5 -- after both a clean shutdown and a SIGKILL mid-write, with the data intact afterwards. The size is also bounded by a twentieth of the data volume, so first boot no longer preallocates a large part of it -- below roughly 2Gi of volume the 96M floor wins instead and the server's own default is what lands. The ConfigMap is not labelled k8s.mariadb.com/watch, so the operator does not roll an instance when it changes. That is left as it was. An instance crashlooping on the old config picks the new one up as soon as the kubelet restarts it, which is the case this fixes, while a healthy instance keeps its current sizing until it next restarts for some other reason. Rolling every tenant database on a chart upgrade would be the larger risk. innodb_buffer_pool_instances and innodb_thread_concurrency are dropped rather than scaled. Both were removed from the server in 10.6, below the oldest version this chart ships, and each start logged "was removed. It does nothing now and exists only for compatibility with old my.cnf files". Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
bbed4892c9 |
fix(mariadb): widen startup probe budget so bootstrap cannot be killed
The operator builds the MariaDB startup probe without setting failureThreshold, so it falls back to the Kubernetes default of 3. The kubelet only starts probing once initialDelaySeconds has elapsed and kills the container on the Nth consecutive failure, so with the operator's initialDelaySeconds of 20 and periodSeconds of 10 the budget is 20 + (3 - 1) * 10 = 40 seconds -- exactly what the liveness probe allows, which leaves the startup probe deferring nothing. Bootstrapping a fresh datadir does not reliably fit in 40 seconds under a constrained CPU limit or slow replicated storage. Overrunning the budget is not a slow start that recovers on the next attempt: the kubelet kills the container after mariadb-install-db has created the system tables but before the entrypoint's temporary server applies the root grant. The restarted entrypoint keys DATABASE_ALREADY_EXISTS off that directory and skips initialization for good, so the server comes up with the install-time root account -- local only and passwordless -- rather than the credentials the operator holds. Every probe from then on fails with "Access denied for user 'root'", so the pod crash-loops permanently with no path back and the install times out waiting for a MariaDB that can never become ready. Raise failureThreshold to 30, giving first boot 310 seconds. Only the threshold is set, so the operator keeps its own probe handler -- the agent HTTP endpoint when replication is enabled, the exec check otherwise -- and the liveness and readiness probes keep their defaults, so a server that hangs after a successful start is still caught as quickly as before. 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
|
572492c276 |
docs(backups): scope the barman-binaries claim, drop the stale linstor webhook note
The db.yaml plugins-block comment still carried the over-generalized claim that the pinned postgresql image variants no longer ship the barman-cloud-* binaries; mirror the scoped wording from docs/operations/backup-classes.md (only the standard variant keycloak-db pins lacks them; this chart's bare system-flavor tags still ship them but the flavor is deprecated upstream). After the rebase onto main this PR no longer touches linstor-scheduler: main re-vendored it to upstream chart 0.3.1 / extender v0.3.6, which keeps the admission webhook and stops stripping unknown pod fields (including initContainers[].restartPolicy the barman sidecar needs). The upgrade note claiming the webhook is removed and locality scheduling becomes advisory is rewritten as a hold-back warning. Also: afterwards -> afterward (American English, review nit). Assisted-By: Claude <noreply@anthropic.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey KolkovandClaude Fable 5
|
0b584a1e6e |
chore(postgres): regenerate README after the bootstrap.serverName description
make -C packages/apps/postgres generate re-pads the Bootstrap parameters table for the new bootstrap.serverName row; the committed README still carried the old padding and kept the pre-commit generate check red. Assisted-By: Claude <noreply@anthropic.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Andrey Kolkov <androndo@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>
|