mirror of
https://github.com/cozystack/cozystack.git
synced 2026-10-28 23:57:08 +09:00
648eb42cae3f12bd367102048fa89089fc40407a
5044
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Myasnikov Daniil
|
648eb42cae |
test(ci): make the image-ref guards see split-host and unparseable files
Both round-2 reviewers independently landed on the same two test-side
holes, and both are the guard being blind in exactly the way the bug it
guards was.
The completeness oracle derived its expected set from a grep for a
contiguous ghcr.io/cozystack/cozystack/...@sha256: string, so it could
not see any split-host shape: cilium, keycloak-operator, kubeovn, the OCI
artifact and seven shape-3 refs, 12 of 43 first-party refs in all. That
includes keycloak-operator, which the test's own rationale names as the
motivating profile. The fixture-discovery grep in the same file had
already been widened for this exact reason; the fix was never carried to
the oracle.
The oracle now applies the same shape knowledge to an independently
discovered file list, which is the axis that actually drifts -- what each
consumer disagreed about historically is which FILES it visits, not how a
ref is spelled inside one. To keep that sharing safe, shape coverage is
now pinned by its own test, so deleting a shape still fails something
even though both sides of the comparison would move together.
Sharing required splitting collect_refs_from_file out of
collect_image_refs. Behaviour is unchanged: same 67 refs collected before
and after, multus and the packages artifact both still present.
A YAML oracle cannot cover everything, though. A reviewer planted a
split-host ref in a Helm template, which yq cannot parse at all, and it
survived every test. A file-level marker check now backstops that: any
file carrying a first-party marker must be enumerated or explicitly
allowlisted. Two files are allowlisted today, each with its reason -- the
talos-csr-signer Dockerfile placeholder, and the documented capi gzip
gap. This also closes the secondary weakness that comparing on repo@digest
masks an unenumerated occurrence when the same image sits at an
enumerated path, which is live for that capi file.
Second hole: the postcondition's key-position branch had no coverage.
Replacing the whole pattern with the naive "${RC}@sha256:" -- the
narrowing rejected because cilium keeps tag: and digest: under separate
keys -- left all 22 tests green, so the branch that makes the split shape
visible was asserted by nothing. Added a cilium-shaped fixture at an
unenumerated path.
Both previously-surviving mutations are now caught. The oracle was also
pre-filtered by registry marker before parsing, cutting it from 32s to 5s.
Assisted-By: Claude <noreply@anthropic.com>
Assisted-By: GPT-5 <noreply@openai.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
|
||
|
Myasnikov Daniil
|
9bd8c17971 |
fix(ci): shape-filter the promote postcondition and close test gaps
Review of the previous commit found the postcondition itself could abort
a release. It grepped the bare "X.Y.Z-rc.N" substring across the tree, and
three live files carry such strings without being image refs:
kubevirt-csi-driver's go.sum pins github.com/golang/protobuf v1.4.0-rc.2
-- a version cozystack actually cut, so promoting v1.4.0 would have
failed on a Go checksum line -- metallb_test.yaml names v1.5.0-rc.2 in a
comment, and the console pnpm-lock.yaml carries rolldown@1.0.0-rc.15. The
remediation the error message gave would have made things worse: declaring
go.sum as a ref source would sed a checksum file.
Match the version only in image-reference position now: after an
image/repository/tag key with no intervening '#', or immediately before an
@sha256 digest. Narrowing it to "${RC}@sha256:" alone would have been
wrong -- cilium keeps `tag: v1.5.0` and `digest:` under separate keys, so
the version is not always adjacent to the digest. Verified against four
real rc versions that previously aborted.
The suite also only tested the fail-closed direction, which is why that
defect survived, and the collector had no completeness guard at all: a
first-party ref added at an unenumerated path on a `latest@` tag was
mirrored by nothing and passed all eighteen tests. Since the mirror's
failure mode is the worst of the three -- the published nightly points at
the private build registry -- that gap mattered most where coverage was
thinnest.
Added: a fail-open test using the three real rc versions above; a
collector completeness diff that discovers refs by content and fails on
any the enumeration misses; coverage for the OCI-artifact shape, whose
deletion previously left every suite green; and an unreadable-file test,
since a read error was being treated as "no match" and silently skipped.
The component-versioned test was vacuous -- its rc version shared no
substring with the kamaji tag it claimed to protect, so it passed by
construction. It now uses an rc version sharing kamaji's 0.19.0 prefix,
which a loosely-anchored implementation would corrupt.
Round-trip fixture discovery matched a combined 'ghcr.io/cozystack/
cozystack/' prefix, which skips keycloak-operator (host in a sibling
registry: key) and kubeovn (host in global.registry.address). It now
matches 'cozystack/cozystack' and stamps split `tag:` keys too.
Also documented two pre-existing gaps rather than half-fixing them: the
nightly host rewrite is a contiguous-substring replace and cannot reach a
split host, and a ref inside a gzip is invisible to every tool including
the postcondition, which skips binaries. Both are host-dimension defects;
the version dimension this change owns is unaffected, because the version
always lives in a tag key regardless of where the host sits.
Assisted-By: Claude <noreply@anthropic.com>
Assisted-By: GPT-5 <noreply@openai.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
|
||
|
Myasnikov Daniil
|
09323fff13 |
fix(ci): cover every image-ref storage shape in promote, retag and mirror
Promotion rewrote the rc version substring across the depth-2 package values.yaml plus packages/apps/kubernetes/images/*.tag alone, on the premise that the kubernetes app was the only one whose .tag files carry the cozystack version. Nine other .tag files do, and system/multus seds its ref straight into a vendored upstream manifest. v1.6.0 was therefore staged with 33 of 55 first-party refs still reading v1.6.0-rc.4. promote-retag.sh and nightly-mirror.sh had the same blind spot from the other direction: both scanned only the depth-2 values.yaml, so those images were never retagged to the stable version (30 refs selected before, 42 after) and never mirrored to the public registry for a nightly. A retag miss merely leaves an image short of a tag, since the digest still resolves inside one registry; a mirror miss is worse, because the host rewrite walks the same file list and a ref that is neither mirrored nor rewritten leaves the published tree pointing at the private build registry. The three call sites each carried their own idea of where a ref can live, which is how they drifted apart. Move that knowledge into hack/lib/image-refs.sh as the single enumeration -- image_ref_files() for the files, collect_image_refs() for the refs inside them -- and have all three source it, so a new storage location is declared once and reaches every consumer. The rewrite itself moves out of promote-rc.yaml into hack/promote-rewrite-tags.sh. Being workflow-inline is why this was only observable by cutting a release: there was nothing to unit test. It now also asserts its own postcondition, scanning wider than it rewrites and failing the promotion when any ref survives in a location the enumeration does not know about, rather than shipping it quietly. Component-versioned images (kamaji's v0.19.0-cozystack.N) and third-party pass-through images do not ride the cozystack version line and are deliberately left alone; a test pins that so a future fix here cannot over-reach. capi-providers-cpprovider is a documented known gap: its ref is also embedded in a gzipped copy that the chart actually ships, and rewriting only the readable copy would diverge the two. docs/agents/image-refs.md records the contract -- three tag classes, three storage shapes, the invariants, and which consumer does what. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
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` (
|
||
|
myasnikovdaniil
|
cc66e22952 |
fix(ci): mirror and retag images whose digest is embedded in the values tag (#3375)
## What's broken
`collect_refs` in both `hack/nightly-mirror.sh` and
`hack/promote-retag.sh` recognises three values shapes. The one most
package Makefiles actually write is not among them:
```yaml
image:
repository: <registry>/linstor-csi
tag: main@sha256:<digest> # digest embedded in the tag
```
Package Makefiles set `.image.tag` to `"$(IMAGE_TAG)@$(digest)"` in a
single `yq` call rather than maintaining a separate `digest` key. That
matches neither the `{repository, tag, digest}` rule (no `digest` key)
nor, usefully, the single-string rule — that one matches the bare tag
value, which carries no repository, so `ref_repo()` reduces
`main@sha256:...` to the repository `main` and the ownership filter
drops it.
Being dropped is silent. Both scripts then report success having simply
never seen those refs.
The same emit-then-drop hits a second class, found in review: refs whose
registry host does not live inside `repository` at all.
`keycloak-operator` splits it into a sibling `registry` key, and
`kubeovn` keeps it in the document-level `global.registry.address` with
a bare `repository: kubeovn`. Both are built and pushed to `$REGISTRY`
by cozystack — `kubeovn` by the `cozystack/kubeovn-chart` wrapper repo,
whose own `make image` writes all three keys — and both were emitted
host-less and discarded as third-party.
## Impact
**Nightly (`nightly-mirror.sh`) — this is the one that broke.** The
image is never copied to GHCR, *and* the unconditional host rewrite at
the end of the script still repoints its repository at the destination
registry. The result is a published reference to an image that was never
pushed there.
Confirmed against the `:nightly` tag on GHCR — the two packages using a
recognised shape are present, and eight images across six packages using
this shape are absent:
| shape | packages | `:nightly` |
|---|---|---|
| `{repository,tag,digest}` | cilium | present |
| single-string full ref | cozystack-operator | present |
| `{repository, tag:<t>@sha256}` | kamaji, kilo, linstor-csi,
piraeus-server, linstor-gui, metallb-controller, metallb-speaker,
redis-operator | **absent** |
This surfaced as the first full nightly run failing at install: the
`e2e-image-prepuller` DaemonSet never became ready (0/3, 10-minute
timeout) because `linstor-csi` and `piraeus-server` 404 on GHCR. The
prepuller only templates kubeovn/linstor/cert-manager, so it exposed two
of the eight; the other six would have failed later at HelmRelease
install.
**Promotion (`promote-retag.sh`) — same blind spot, milder
consequence.** This retags within a single registry, so the digests
already exist and digest-pinned installs still resolve — there is no
dangling reference and no 404. What is missing is the `:<version>` tag
(and `:latest` under `MOVE_LATEST=1`) for those images, so a promoted
release does not carry the tags it claims to.
The registry shows how long that has been true for the two host-split
images. `keycloak-operator` carries exactly one tag, `latest`, though
the package has shipped since 2024-11. `kubeovn`'s newest
cozystack-versioned tag is `v1.14.11-v0.38.8`, from before the 1.0
versioning switch — while `cilium`, which uses an already-recognised
shape, carries `v1.5.0` through `v1.6.0-rc.2`. Neither has received a
tag from any 1.x release.
Measured against the committed tree, the dry-run selects **19 unique
repositories before this change and 29 after** — the eight digest-in-tag
images plus `keycloak-operator` and `kubeovn`, nothing lost.
### What the nightly does *not* recover
Four of the ten are not in the root `Makefile`'s `build` target: `kilo`,
`redis-operator`, `keycloak-operator` and `kubeovn`. Their baked refs
keep the committed GHCR host, so the `SRC_REGISTRY` filter still drops
them and their `:nightly` tags remain absent after this PR. Installs are
unaffected — those digests already resolve on GHCR. The nightly recovers
the six OCIR-built images; promotion recovers all ten.
## Why this was invisible
Latent since `nightly-mirror.sh` was added — the nightly had never
completed a full run, so its mirror step was never exercised end to end.
Both test suites were green throughout. `nightly-mirror_test.bats`'s
fixture covered the single-string, `{repository,tag,digest}` and
platformSource shapes — a layout only cilium actually uses — and omitted
the dominant one. `promote-retag_test.bats` asserted "at least one
cozystack-owned image is selected", which is satisfied by the shapes
that already worked and cannot detect an omission.
Two comments actively pointed the wrong way, and are corrected here. The
header presented its list as the shapes the build writes, which reads as
exhaustive; it now says the list is empirical, names what an audit found
outside it, and says to re-run that audit when a package is added. The
ownership-filter comment named `kilo`, `kube-ovn` and
`keycloak-operator` as third parties "not pushed to `$REGISTRY`" — all
three are pushed there, and that wrong comment is part of why the
missing tags went unnoticed.
Both fixtures are extended here. The promotion test names concrete
packages rather than asserting a count or a generic pattern, since
either would drift back to proving nothing.
## Type guards
Both digest-in-tag rules check `.tag` is a string before `test()`.
Without it a non-string tag aborts the whole expression (`cannot match
with !!int, can only match strings`), and since these invocations
swallow stderr and status, that abort would silently discard **every**
ref of this shape in the same file — reintroducing this very bug in any
file that happens to carry an unquoted `tag: 1.24` next to a cozystack
image. Rule 1 is immune because its `select(tag == "!!str")` already
precedes its `test()`.
`registry`, `repository` and `digest` are guarded too, against a
narrower failure. Those keys are concatenated rather than `test()`ed,
and yq coerces int, bool, null and seq on `+` — each merely yields a ref
the ownership filter discards. Only a `!!map` aborts, raising `!!str ()
cannot be added to a !!map`, which takes the file's whole expression
with it. So that guard is insurance against a nested-map typo in a
vendored chart, not against unquoted scalars. It is a `select()`, so an
offending map drops only itself and its neighbours still resolve.
No non-string value of either kind exists in the tree today, so both are
prevention rather than an active fix.
## Testing
- Both suites pass: `nightly-mirror_test.bats` 2/2,
`promote-retag_test.bats` 4/4.
- Every new assertion was mutation-tested — removing the rule under test
turns it red. Verified individually for the shape-3 registry join, the
shape-2 registry join, the `global.registry.address` rule, and the
`repository` type guard. Also verified that a rule matching the bare tag
instead of the repository (the subtle wrong fix) fails, which is why the
assertions check the full source ref rather than only the destination.
- `promote-retag.sh --dry-run` against the real committed tree: 19 → 29
unique repositories, no losses.
- An audit of every `@sha256:` digest under `packages/*/*/values.yaml`
against the selector output accounts for all 35: 30 are selected, and
the remaining five are third-party
(`ghcr.io/lexfrei/{kuberture,ouroboros}`, `docker.io/clastix/kubectl`,
`ghcr.io/kvaps/...`) or sit outside `$REGISTRY`
(`ghcr.io/cozystack/ingress-nginx-with-protobuf-exporter/*`).
- `shellcheck` clean on both scripts at default severity and `-S style`.
## Ordering note for reviewers
Worth landing before the next release candidate is cut, rather than
treating this as routine CI cleanup.
`promote-rc.yaml` builds the release branch from the rc's staging
branch, and `pull-requests-release.yaml` runs `./hack/promote-retag.sh`
from that merged tree — so the copy of the script that executes at
promotion is the one frozen into the rc tag's tree, not whatever is on
the default branch at promotion time. An rc cut before this merges will
carry the old script and silently skip those retags when promoted.
The nightly half is also what unblocks the full-suite e2e lane, which
cannot currently publish a working install closure to GHCR.
### Downstream repositories
Walked the trigger map in `docs/agents/contributing.md` against the
diff: it touches only `hack/nightly-mirror.sh`, `hack/promote-retag.sh`
and their two `.bats` suites. No `packages/`, no chart, no
`values.schema.json`, no `ApplicationDefinition`, no CRD or RBAC. The
`cozystack/ccp` trigger covers moving or renaming files under `hack/`
and changing what a make target does; neither applies — the anchor files
it gates on (`hack/package.mk`, `hack/common-envs.mk`) are untouched and
no make target changes behaviour.
- [x] No downstream repository is affected by this change
- [ ] [cozystack/website](https://github.com/cozystack/website) -
follow-up:
- [ ]
[cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack)
- follow-up:
- [ ]
[cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack)
- follow-up:
- [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up:
- [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up:
- [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) -
follow-up:
- [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) -
follow-up:
- [ ]
[cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server)
- follow-up:
- [ ]
[cozystack/external-apps-example](https://github.com/cozystack/external-apps-example)
- follow-up:
- [ ] [cozystack/examples](https://github.com/cozystack/examples) -
follow-up:
### Release note
```release-note
fix(ci): mirror and retag every cozystack-owned image, including those whose digest is embedded in the values `tag` and those whose registry host is stored outside `repository`. Ten images across eight packages were silently skipped: the nightly published references that 404 at install, and promoted releases did not carry the `:<version>` tags they claim.
```
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **Bug Fixes**
* Improved nightly mirroring and retag promotion to recognize
digest-pinned image references across more YAML value shapes, including
digests embedded in the `tag` field and optional registry wrappers.
* Expanded support for chart-global image definitions and safer parsing
to avoid silently skipped references.
* Strengthened selection logic to exclude third-party images from copy
plans.
* **Tests**
* Expanded dry-run coverage with additional fixture cases for split
maps, `tag`-embedded digests, numeric-tag variants, and more “owned”
image assertions.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
v1.6.0-rc.4
|
||
|
myasnikovdaniil
|
1b777d8077 |
fix(kubernetes): re-pin deleted centos:stream9 digest for kubevirt-csi-driver (#3386)
## What this PR does `quay.io/centos/centos:stream9` is a **rolling tag** — upstream deletes the previously-pinned digest whenever it publishes a new `stream9` build. The digest re-pinned in |
||
|
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
|
292f8ec65a |
fix(ci): type-guard the concatenated keys in the ref selectors
Address non-blocking review feedback from lexfrei on hack/nightly-mirror.sh:81 and hack/promote-retag.sh:84. `registry`, `repository` and `digest` are joined into a ref by string concatenation with no type guard, so a non-string value could abort the whole per-file expression. Because collect_refs swallows both stderr and status, that abort silently discards every co-located owned ref — the same per-file blast radius the `tag` guard already prevents. The mechanism differs from `tag`'s, though, and the narrower reading is worth recording. These keys are never fed to test(), so the "cannot match with !!int" abort does not apply to them: yq coerces int, bool, null and seq on `+`, and each of those merely yields a ref the ownership filter discards. Only a !!map actually aborts, raising "!!str () cannot be added to a !!map". The guard is therefore narrow insurance against a nested-map typo in a vendored chart, not protection against unquoted scalars. Implemented as select() rather than a test(), so an offending map drops only itself and its neighbours in the same file still resolve. The nightly fixture gains a file pairing a map-typed `repository` with an owned ref, asserting the latter is still mirrored; removing the guard turns it red. Selection over the committed tree is unchanged at 30 repositories. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
acd88bdd00 |
fix(ci): collect image refs whose host is outside repository
Address review feedback from lexfrei and IvanHunters on
hack/promote-retag.sh:84 and hack/nightly-mirror.sh:81.
The shape-3 rule assumed the registry host lives inside `repository`.
Two cozystack-built images do not satisfy that and were emitted
host-less, so the ownership filter dropped them — the same silent
emit-then-drop this PR fixes for the other shapes:
- keycloak-operator splits the host into a sibling `registry` key
({registry: ghcr.io, repository: cozystack/cozystack/keycloak-
operator, tag: latest@sha256:...}), and its own Makefile writes
exactly that layout.
- kubeovn keeps the host in the document-level
global.registry.address with a bare `repository: kubeovn`. It is
built and pushed to $REGISTRY by the cozystack/kubeovn-chart
wrapper, whose `make image` writes all three keys.
Neither has received a 1.x release tag: on GHCR keycloak-operator
carries only `latest`, and kubeovn's newest cozystack-versioned tag is
v1.14.11-v0.38.8, from before the 1.0 versioning switch — while cilium,
which uses an already-covered shape, has v1.5.0 through v1.6.0-rc.2.
Shapes 2 and 3 now rejoin an optional `registry` sibling, and a new
shape-4 rule handles the chart-global layout. An audit of every
@sha256: digest under packages/*/*/values.yaml against the selector
output confirms the remaining five misses are third-party or sit
outside $REGISTRY. Dry-run over the committed tree: 28 -> 30
repositories, exactly these two gained, nothing lost.
Also correct two comments that overstated coverage. The header no
longer presents its list as the shapes the build is limited to — the
list is empirical and charts are vendored — and the ownership-filter
comment no longer names kilo, kube-ovn and keycloak-operator as
third parties not pushed to $REGISTRY. All three are pushed there, and
that wrong comment is part of why the missing tags went unnoticed.
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
|
||
|
Andrey KolkovandClaude Fable 5
|
b897b53107 |
test(postgres): fix S3 CA secret name in backup roundtrip e2e
The backup roundtrip e2e copied the seaweedfs S3 CA from a Secret named seaweedfs-system-ca-cert in tenant-root, a name captured off a dev cluster. On current main the seaweedfs chart pins fullnameOverride: seaweedfs, so the cert-manager CA Certificate's secret is seaweedfs-ca-cert; the old name is absent and the first CI run of the e2e failed at step 05a with 'secrets "seaweedfs-system-ca-cert" not found'. Default S3_CA_SECRET to seaweedfs-ca-cert and, when that Secret is absent, auto-discover the seaweedfs CA Certificate (the seaweedfs-labelled one with spec.isCA=true) and read its secretName, so a future fullnameOverride change does not silently break the copy again. Fail with a clear message when neither resolves. README and chainsaw comment updated to match. Assisted-By: Claude <noreply@anthropic.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Myasnikov Daniil
|
770314b3cb |
fix(ci): retag images whose digest is embedded in the values tag
promote-retag.sh carries the same collect_refs blind spot the previous commit fixes in nightly-mirror.sh, and nightly-mirror.sh's own comment asserted the two were "identical to" each other -- which is how the gap stayed invisible once one of them was fixed. Measured against the committed tree, the dry-run selects 19 repositories before this change and 27 after. The eight it was missing are exactly the set that broke the nightly: kamaji, kilo, linstor-csi, piraeus-server, linstor-gui, metallb-controller, metallb-speaker, redis-operator. Nothing is lost from the previous selection. The consequence differs from the nightly case and is milder. This retag is within a single registry, so the digests already exist and every digest-pinned install still resolves -- there is no dangling reference and no 404. What is missing is the :<version> tag (and :latest under MOVE_LATEST=1) for those eight images, so a promoted release does not carry the tags it claims to, and anything resolving a cozystack component by version tag rather than by digest does not find it. Nothing in the promotion detects this: the script reports success having simply never seen those refs. The existing test could not catch it. It asserts "at least one cozystack-owned image is selected" and that nothing third-party leaks in -- both satisfied by the shapes that already worked, which is why eight omissions sat under a green suite. It now names concrete shape-3 packages, since a count or a generic pattern would drift back to proving nothing. Verified the assertion fails against the unfixed script. Same type guard on `.tag` as the previous commit, for the same reason: without it an unquoted numeric tag anywhere in a file silently discards every shape-3 ref in that file, and the swallowed stderr leaves no trace. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
06278ac18b |
fix(ci): mirror images whose digest is embedded in the values tag
The nightly's full-suite e2e failed at install: the e2e-image-prepuller
DaemonSet never became ready, because
ghcr.io/cozystack/cozystack/linstor-csi@sha256:2f314cb9... and
.../piraeus-server@sha256:39fdacec... both 404 on GHCR. They are not there
because nightly-mirror.sh never copied them.
collect_refs handled three values shapes, and the one most package Makefiles
actually write is not among them: a split map whose digest is embedded in the
tag, {repository, tag: <tag>@sha256:<digest>}, produced by a single yq call
setting .image.tag to "$(IMAGE_TAG)@$(digest)". It matches neither the
{repository, tag, digest} rule (no `digest` key) nor, usefully, the
single-string rule -- that one matches the bare tag value, which carries no
repository, so ref_repo() reduces "main@sha256:..." to the repository "main"
and the SRC_REGISTRY case drops it.
Being dropped there is silent, and the host rewrite at the end of the script
runs over every depth-2 values.yaml regardless of what was mirrored. So the
image was never copied AND its repository was still repointed at
DST_REGISTRY, leaving a reference to an image that was never pushed there --
exactly the dangling-ref failure the comment above that rewrite warns about,
reached by a path it did not anticipate. The 404 then surfaces at pull time,
one job later, as a DaemonSet rollout timeout that names no image.
This is not limited to linstor. Checking the :nightly tag on GHCR, the two
packages using a recognised shape (cilium, cozystack-operator) are present
and eight images across six packages using this shape are absent: kamaji,
kilo, linstor-csi, piraeus-server, linstor-gui, metallb-controller,
metallb-speaker, redis-operator. The prepuller only templates
kubeovn/linstor/cert-manager, so it surfaced two of the eight; the rest would
have failed later at HelmRelease install.
Latent since the script was added in
|
||
|
myasnikovdaniil
|
f5227ee60f |
fix(seaweedfs): close the 4.31 rename fallout on the 1.5.x→1.6 upgrade path (#3339)
## What this PR does Cozystack v1.5.0 bumped the vendored SeaweedFS chart 4.0.405 → 4.31.0, which renamed every workload after the Helm release (`<name>-system-*`). StatefulSet names are immutable, so upgrades through 1.5.x stood up a second, empty set beside the running one instead of renaming. #3282 pinned `fullnameOverride: seaweedfs` so 1.6 adopts running workloads in place — this PR closes the remaining holes on that adoption path, found by driving a disposable 3-node cluster through a real v1.4.5 → v1.5.3 → main upgrade with SeaweedFS tenants planted in every reachable state. - **The naming guard moves into `packages/system/seaweedfs`** — the render a platform upgrade actually re-renders: the `<name>-system` HelmRelease pulls this chart from a platform-managed ExternalArtifact, so the previous guard in `extra/seaweedfs` was never in the path. Upgrading a fresh-1.5.x tenant therefore created an empty chart-named set beside its live data and flipped the guard's own legacy-data signal. `extra/` keeps a sibling copy for operator visibility; a bats parity suite pins the two detection blocks byte-identical. - **The guard refuses instead of guessing when both naming generations exist.** Nothing durable distinguishes a duplicate that never served from one that served and crashed: claim timestamps invert during the recovery runbook's own re-bind, `readyReplicas: 0` is a snapshot, and Helm birth order answers "which generation is original", not "is the other one empty". Exactly one generation present is decidable, and the render proceeds (or refuses as class S) on its own. - **Cluster-scoped RBAC is named per namespace again.** 4.31 named four cluster-scoped objects after the release — identical for every tenant — so all tenants collided on one ClusterRole/ClusterRoleBinding and only the last-reconciled tenant's COSI provisioner kept its RBAC. Names return to `global.seaweedfs.serviceAccountName`; three of four are byte-identical to pre-4.31 and adopt in place. - **The `seaweedfs-db` hand-over runs for every instance name.** Migration 43 compared the owning release against the literal `seaweedfs-system`, so an instance named `foo` was skipped and its CNPG Cluster — the filer metadata for every object in that tenant's S3 — was pruned on the next reconcile, PVC included. The comparison now matches the `-system` suffix (shared `lib/seaweedfs-db-adopt.sh`), and new migration 53 re-runs the hand-over for clusters already past 43, before anything re-renders. - **`hack/seaweedfs-naming-audit.sh` + `docs/operations/seaweedfs-431-rename-recovery.md`** — what the guard's refusal points operators at: read-only classification (`L` / `S` / `MIXED`, naming the candidate duplicate from relative PV vintage, never a clock window) and the recovery procedures. **Scope.** The supported SeaweedFS deployment is the tenant module, which hardcodes the instance name (`packages/apps/tenant/templates/seaweedfs.yaml`); a tenant only enables or disables it. The runbook and its selectors are scoped to that name; instances created directly against the API under other names are classified by the audit but routed to escalation. Zone/pool keys of ~40 or more characters fall outside the guard's reconstruction — an accepted limit, recorded in `_naming.tpl` and the runbook. **Upgrade impact.** 1.4.x → 1.6: no manual steps — one generation, adopted in place. 1.5.x → 1.6 with SeaweedFS: migration 53 protects every `seaweedfs-db` first; then the upgrade **refuses** for any tenant holding both naming generations until the operator resolves the duplicate. This is deliberate — which generation holds the data is not decidable from inside a render, and guessing wrong destroys it. Release notes should present the refusal as expected behavior. **Testing.** 55 chart unit tests across both packages (including new MultiZone zone-component guard cases — the suite previously had no zone shapes), 11 audit bats, 8 guard-parity bats, migration bats. Validated end-to-end on a disposable 3-node cluster driven v1.4.5 → v1.5.3 → main with five tenants covering: never-saw-4.31 (renders untouched), fresh-1.5.x (refused as class S), wedged duplicate, split duplicate, and a non-default-named instance (its database survives only with this fix). The audit classifies all five correctly, and its two independent signals — revision-1 birth scheme and relative PV vintage — agree on every MIXED tenant. Related: #3282 (fullnameOverride pin), #3335 (etcd adoption backup gate — separate, also required for the 1.5.x→1.6 path). ### Screenshots Not a UI change. ### Downstream repositories - [x] No downstream repository is affected by this change Walked the trigger map against the diff: no package added/renamed under `packages/{apps,extra}/`, the `packages/core/platform/values.yaml` change is only the migrations `targetVersion` bump (no `spec.components.platform.values.*` key changes), no variant/bundle/component changes, no asset renames, no ApplicationDefinition semantic changes. ### Release note ```release-note fix(seaweedfs): the 1.6 upgrade no longer renames a SeaweedFS instance away from its data. The naming guard now runs in the chart a platform upgrade actually re-renders and refuses when both pre- and post-4.31 naming generations exist; hack/seaweedfs-naming-audit.sh and docs/operations/seaweedfs-431-rename-recovery.md guide recovery, and the refusal is expected for tenants that passed through 1.5.x. Cluster-scoped COSI RBAC is named per namespace again (the 4.31 release-based names collided across tenants), and the seaweedfs-db hand-over runs for every instance name — previously an instance not named `seaweedfs` had its filer metadata database pruned on upgrade; new migration 53 repairs clusters that already ran the old hand-over. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added fail-closed upgrade protections for SeaweedFS naming migrations (mixed/damaged classification), including safer behavior when cluster visibility is limited. * Added cluster-scoped RBAC uniqueness safeguards to prevent cross-tenant name collisions during rendering. * Improved SeaweedFS database adoption/repair migrations and strengthened post-delete cleanup ownership checks. * **Bug Fixes** * Hardened SeaweedFS 4.31 rename recovery and PV rebind flow, including reclaim policy preservation and long/non-default instance-name edge cases. * **Documentation** * Expanded the SeaweedFS 4.31 rename-recovery runbook with clarified auditing, verification, and escalation. * **Tests** * Added/expanded integration and Helm rendering tests for the above scenarios and refusal/fail-closed behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai -->v1.6.0-rc.3 |
||
|
myasnikovdaniil
|
8020656725 |
fix(ci): resolve nightly packages digest without image-specific skopeo ops (#3332)
## What this PR does
The Nightly workflow has **never completed a run**. Every scheduled run
since `NIGHTLY_ENABLED` was set has failed in `mirror` at "Resolve
source commit and version", ~80s in, with:
```
level=fatal msg="unsupported image-specific operation on artifact with type \"application/vnd.cncf.flux.config.v1+json\""
```
Nothing has ever been published — `cozy-installer:nightly` and
`cozystack-packages:nightly` both 404 on GHCR, and `cozystack-nocloud`
does not exist.
This fixes that failure and the next two behind it.
### 1. The digest lookup could never have worked
`cozystack-packages` is a **Flux OCI artifact** (config mediaType
`application/vnd.cncf.flux.config.v1+json`), not a container image. A
plain `skopeo inspect` parses the config blob as an image config and
fatals on it, so `skopeo inspect --format '{{.Digest}}'` was never
viable against this ref — the `--raw` call on the very next line already
acknowledged this.
Replaced with `skopeo inspect --raw` + `skopeo manifest-digest`, which
hashes the manifest bytes — exactly what the registry addresses the
artifact by, with no image-specific parsing.
This also collapses two registry fetches into one: the digest and the
`org.opencontainers.image.revision` annotation now come from the **same
bytes**, which strengthens the race the original comment describes —
there is no longer any window between pinning the tag and reading it.
### 2. `flux pull artifact` rejects a non-existent output path (×2)
`flux pull artifact --output _baked` fails with `invalid output path
"_baked": no such file or directory`. `_baked` is neither tracked nor
created by anything. `pull-requests.yaml` does `rm -rf` + `mkdir -p`
first; `nightly.yaml` omitted it. Since the workflow always died at step
1, this was never reached.
Same bug for `_rw` in the e2e staging step. Both now create the
directory first.
## Verification
- Reproduced the original fatal against the live artifact with skopeo
1.22.2.
- Ran the fixed resolve step end-to-end against the real registry:
returns the digest the registry reports, and a `sha` that matches the
current `main` HEAD.
- Reproduced the `_baked` failure and confirmed the fix extracts the
tree.
- `actionlint` clean; `zizmor --offline` clean; `shellcheck` clean on
changed lines.
Also checked and **not** changed, each disproven rather than assumed:
- `skopeo copy --multi-arch all` on a Flux artifact — works; `copy` is
mediaType-agnostic.
- The `awk -F@ '/artifact successfully pushed/'` digest capture —
matches current flux CLI output, and is the same idiom
`packages/core/installer/Makefile` already uses in production.
- `nightly-mirror.sh`'s `inspect --format Digest` — all mirrored refs
there are genuine image manifests, so it is correct as written.
- `make -C packages/core/talos talos-nocloud` — exists and is CI-proven
via `tags.yaml`.
## Honest scope
This fixes the `mirror` job's blockers. It does **not** promise a green
nightly on the first run: every step after `mirror` is still unproven,
having never executed. The most likely next wall is `build-disk` —
`cozystack-nocloud` is a brand-new package, so the first `oras push`
creates it, and the follow-up `oras tag` can hit `403 permission_denied`
until the package is linked (the risk the workflow already documents
in-line). The first-ever full e2e run may well surface its own issues.
Reviewing that follow-on separately seems better than growing this PR.
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **Bug Fixes**
* Improved nightly artifact mirroring and staging reliability.
* Prevented artifact pull failures by ensuring required output
directories are created before use.
* Enhanced retrieval of artifact version and source revision metadata.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
|
||
|
myasnikovdaniil
|
a15206502f |
ci(labeler): map cert-manager, external-secrets and metrics-server scopes (#3362)
## What this PR does PRs titled with the `cert-manager`, `external-secrets` or `metrics-server` scopes landed on `area/uncategorized` because the labeler's scope map had no entry for them — all three surfaced at once in the current port-collision series (#3359, #3360, #3361). metrics-server feeds the monitoring pipeline, so it maps to `area/monitoring`; cert-manager and external-secrets are platform services, so they map to `area/platform`. ### Release note ```release-note NONE ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Chores** - Improved pull request labeling automation. - Pull requests involving metrics, certificate management, and external secrets are now categorized more accurately with the appropriate area labels. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Myasnikov Daniil
|
bd770b2288 |
fix(hack): make the seaweedfs naming audit POSIX sh
hack/seaweedfs-naming-audit.bats sources the audit script at top level,
and cozytest.sh sources the converted test file into its own /bin/sh --
dash 0.5.12 on the Ubuntu CI runner. dash has no `pipefail`, so
`set -uo pipefail` aborted the runner before the first @test and took
the whole file down with it:
hack/cozytest.sh: 48: set: Illegal option -o pipefail
The bash shebang could not prevent this: `.` executes the file's
contents in the caller's shell, so the shebang is only a comment there.
The script was already POSIX apart from those two lines -- no arrays,
no [[, no local, no bashisms at all -- so the script becomes POSIX sh
rather than the runner becoming bash. That keeps the shell under test
and the shell at run time the same one, instead of testing under dash
while shipping under bash.
Dropping `pipefail` changes no behaviour: the script never sets -e and
never reads a pipeline's exit status, reporting failure as empty output
checked with [ -n ... ].
Verified under dash (ubuntu:24.04): 11/11 tests pass, and the script
runs clean against a live cluster, classifying five seaweedfs tenants
with the revision-1 scheme and the PV ages agreeing on every MIXED
case. shellcheck -s sh reports nothing.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
|
||
|
Myasnikov Daniil
|
9b82d1bf36 |
test(seaweedfs): run the db-adopt migrations in the shell they ship with
Address review feedback from IvanHunters on packages/core/platform/images/migrations/migrations/43:23: The suite invoked the migrations as `sh "$MIG_DIR/<n>"`. On the CI runner /bin/sh is dash, which has no `set -o pipefail`, so both scripts aborted on their `set` line with "set: Illegal option -o pipefail" (exit 2) before doing any work, and the job went red. Production was unaffected — the migrations image is FROM alpine, so /bin/sh there is busybox ash — which is exactly why checking the runtime image missed it. Rather than change a shebang or drop pipefail, run the migrations the way run-migrations.sh does: by path, inside the image's own pinned alpine base, read out of the migrations Dockerfile so the interpreter under test cannot drift from the one they ship on. pipefail is load-bearing here — lib/cozystack-version.sh pipes the rendered manifest into `kubectl apply`, so without it a failed render stamps the version from empty input — and a shell that lacks it can prove nothing about that path. run_migration() returns the container's status explicitly: cozytest.sh's awk generator rewrites every bare `}` in column 0 into `return 0` + `}`, so a helper that falls off its own end reports success no matter what it ran, and every fail-closed assertion would have passed vacuously. 15/15 pass. Verified the interpreter under test is BusyBox v1.37.0 ash with pipefail supported, and that a fail-open mutation in lib/seaweedfs-db-adopt.sh turns the suite red. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
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
|
b4f98af940 |
docs(seaweedfs): fail closed in the rename runbook's reclaim-policy handling
The 1.5.x->1.6 rename recovery runbook deletes each volume PVC after switching its PV to Retain, but the switch was never gated on success. With no `set -e`, a failed `kubectl patch ... Retain` left the PV at Delete and the following `delete pvc` took the data with the claim. Gate the delete on both the annotation record and the Retain patch succeeding, aborting the re-bind loop otherwise, so no PVC is deleted while its PV is unprotected. In the Step 5 restore loop, chain the annotation removal after the restore patch with `&&` so a failed patch keeps the saved policy for a retry. Address review feedback from coderabbitai on docs/operations/seaweedfs-431-rename-recovery.md. 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
|
95fcc76902 |
docs(seaweedfs): scope the rename runbook to the supported instance name
The tenant module is the supported way to run SeaweedFS and it hardcodes the instance name: packages/apps/tenant/templates/seaweedfs.yaml creates the HelmRelease as `seaweedfs`, and a tenant can only enable or disable it. Every shell selector in this runbook assumes that name, so say so up front, and route instances created directly against the API under other names to escalation instead of through loops whose name filters provably cannot see truncated claims. The ~40-char zone/pool key limit of the guard's reconstruction is called out as an accepted limit for default-named instances too. Two content fixes ride along, both for findings the final review pass raised: - The emptiness check told the operator to kubectl exec the candidate's own volume pod — which for a D-wedged duplicate can never start; that is the definition of the class, so the check was unexecutable exactly where it matters most and every D-wedged tenant funneled to escalation. Mount each bound candidate claim read-only in a scratch pod instead (a Pending claim has no PV and is empty by construction). The weed shell cross-check stays on the authoritative master, which is running. - Step 1's description of the audit now matches the audit itself: direction is decided by the relative strictly-newer PV rule (the same precondition Step 2a enforces), and an interrupted re-bind reads as overlapping vintages with no candidate — not by a clock window anchored on first_deployed. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
24bcb0bb69 |
fix(seaweedfs): decide MIXED direction by relative PV vintage, not a clock window
The audit named the candidate duplicate by measuring each generation's oldest PV against the release's first_deployed, with an absolute 120-second window for the mid-rebind case. The shipped StorageClasses are WaitForFirstConsumer, so PVs are provisioned at pod-SCHEDULE time — on a cold cluster minutes after first_deployed — and a tenant interrupted mid-way through Step 2's re-bind (both generations sitting on original-vintage PVs) fell out of the window and into a branch decided by which volume index happened to provision first. The advice printed there routes the operator to a step that deletes the claims Step 2 had not yet re-bound. Replace the window with the relative rule runbook Step 2a already enforces: a generation is the candidate duplicate only when EVERY one of its bound PVs is strictly newer than every bound PV of the other generation. Overlapping or tying vintages name no candidate — finish Step 2 or escalate. first_deployed is still printed, as context only. The direction decision is now a pure function (classify_mixed_direction) so the bats suite drives it directly: clean duplicate both ways, interrupted re-bind, tie, and a no-clock case pinning the WaitForFirstConsumer regression. 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
|
aa707f17b5 |
test(etcd): stop losing the metrics probe's output to the attach race (#3350)
## What this PR does The etcd suite's metrics contract failed in CI with `metrics endpoint did not return 200 over http on the 'metrics' port; got: If you don't see a command prompt, try pressing enter.` The probe runs a one-shot curl Pod and reads a `code=<status>` marker back off its output via `kubectl run --rm --restart=Never --attach`. `--attach` only establishes its connection once the Pod reaches Running, so a container that exits in milliseconds — which is what curl against a Pod IP does — writes its stdout into the void, and `--rm` then destroys the Pod carrying the log that still held it. The step reports the metrics endpoint as broken on a cluster that was serving it correctly throughout. The evidence that this was output loss rather than an endpoint failure is that the captured output contained no `code=` at all. A genuine connection failure would still have printed `code=000` from `-w`, plus curl's own error text from `-fsS`, and both were being captured. Meanwhile the probe Pod's events show a clean run, all three etcd members were up without restarts, and every preceding assert in the suite passed. The fix drops `--rm`, re-reads the Pod log when the attach stream carried no marker, and deletes the Pod explicitly afterwards. This is the same treatment `hack/e2e-chainsaw/_lib/talos-image-cache.sh` already applies to the identical race — that probe documents it inline; the etcd one was simply never given it. A `--pod-running-timeout` is set for the same reason it is there: the probe image is used nowhere else in the tree, so it pays a cold pull on every run and kubectl's one-minute default was a second way to end up with no marker. The recovery cannot turn a real failure green. The marker encodes the outcome, so a `code=503` recovered from the log still fails the step; an attach stream that already carried a marker is never second-guessed; and the pre-run delete blocks, so a stale Pod's log cannot be read in place of this run's. Verified by running the probe's exact curl invocation against a 503 and against a dead port: the first yields `code=503`, the second `code=000` plus curl's error, and both fail. The probe still runs exactly once — this is output recovery, not a retry. Helper extracted to `hack/e2e-chainsaw/_lib/etcd-probe.sh` with unit coverage in `hack/etcd-probe_test.bats`. ### Release note ```release-note NONE ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved etcd metrics probe reliability by recovering probe results when attached output is incomplete. * Preserved meaningful probe and Kubernetes error details, including non-success status codes. * Improved probe pod timing and cleanup to reduce intermittent end-to-end test failures. * **Tests** * Added coverage for successful recovery, failed probes, missing output, and log retrieval errors. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
myasnikovdaniil
|
dc3929373b |
test(kubernetes): size the e2e tenant apiserver to the chart default (#3351)
## What this PR does The `kubernetes-previous` suite failed with `timed out waiting for the condition on helmreleases/kubernetes-test-previous-version-csi`, and the reported cause looked like a dependency problem — `csi` waiting on `vsnap-crd`, `coredns` waiting on `cilium`. It was neither. The cluster snapshot taken at failure shows the tenant `kube-apiserver` containers terminated with exit code 137 within about two minutes of each other: one killed for failing its liveness probe as reclaim stalled it, the other OOMKilled outright, both against `limits.memory: 512Mi`. The e2e fixture pinned the tenant control plane to the legacy `small` preset — 1 CPU and 512Mi — while the chart ships `c1.medium`, 2 CPU and 2Gi, as the default for this field. A two-node tenant cluster running the full addon set opens enough watches to cross 512Mi once the workers join, so the apiserver restarts, and every in-flight HelmRelease that is polling a DaemonSet rollout burns its entire ten-minute budget while the control plane is unavailable. That is why two unrelated releases, `csi` and later `ingress-nginx`, failed with the same signature while the Deployment-based releases that had installed earlier succeeded. The `DependencyNotReady` lines are aggregated first-occurrence events — `vsnap-crd` had in fact installed successfully nine minutes before cilium did. This is a defect in the fixture rather than in the product: users on defaults get four times the memory. The default was raised deliberately in v0.40.4, "to ensure more reliable operation under higher workloads and prevent resource constraints"; the fixture was left behind on an unexplained downward override. Setting it to the chart default makes the test exercise a control plane sized the way a real one is. No timeout is widened and no retry is added, since either would hide the cause rather than remove it. The sibling OIDC fixtures keep the smaller preset, correctly — they run a single replica with no workers, where this failure mode cannot occur. Suites are executed serially, so the additional memory applies to one tenant cluster at a time. What this does not prove is that 2Gi is empirically sufficient; that is the value the chart itself ships, so if it still proves too small, the finding belongs to the product default rather than to this fixture. ### Release note ```release-note NONE ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Updated the Kubernetes test environment’s control plane resource allocation to improve reliability and reduce out-of-memory errors and timeouts. * **Documentation** * Added guidance explaining the resource selection and previous failure conditions. <!-- 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
|
0f4a2715ba |
ci(labeler): map cert-manager, external-secrets and metrics-server scopes
PRs scoped to these three components fell through to area/uncategorized because the scope map had no entry for them. metrics-server serves the monitoring pipeline; cert-manager and external-secrets are platform services. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
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> |
||
|
Andrei Kvapil
|
7e206e2986 |
fix(keycloak): point HTTPRoute backends at the service port (#3235)
## Summary
With `gateway-enabled: "true"`, every Keycloak hostname answers 503 from
envoy. The `keycloak-http` Service exposes a single port, 8080:
```yaml
ports:
- name: http
port: 8080
```
but all three HTTPRoute backendRefs in
`packages/system/keycloak/templates/httproute.yaml` reference `port:
80`. Gateway API resolves a backendRef against the Service's declared
ports, so no backend resolves and the route serves 503. The Ingress
template is unaffected because it references the port by name (`http`).
Observed live on the freedom dev cluster: all Keycloak hosts returned
503 via the Gateway; hand-patching the route's backend port to 8080
restored service.
## Change
Point the three backendRefs (public route, its no-adminHost variant, and
the admin route) at port 8080.
Verified with `helm template -s templates/httproute.yaml` (with and
without `ingress.adminHost`): both routes render with `port: 8080`.
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **Bug Fixes**
* Updated Keycloak HTTP routing so both the main and admin routes
forward traffic to the correct backend service port (from 80 to 8080),
improving reliability and access.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
|
||
|
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
|
44b221d772 |
test(kubernetes): size the e2e tenant apiserver to the chart default
The tenant Kubernetes e2e pinned controlPlane.apiServer.resourcesPreset to the legacy "small" preset, capping the apiserver at 1 CPU / 512Mi -- a quarter of the memory the chart itself ships as the default (c1.medium, 2 CPU / 2Gi). 512Mi does not hold a two-node tenant cluster running the full addon set. Once the workers join, cilium, coredns, metrics-server, csi, ingress-nginx and VPA all establish watches at roughly the same moment, and the apiserver crosses the limit: one replica is killed for failing its liveness probe as reclaim stalls it, the other is OOMKilled outright. Both replicas restart within about two minutes of each other, so any tenant HelmRelease that is mid-install and waiting on a DaemonSet rollout observes 'InProgress' for its entire timeout and fails, then re-enters a fresh install on remediation. Which release loses is a matter of timing rather than of the release, which is why the failure moved between the csi, ingress-nginx and cilium releases and looked like an unrelated dependency problem each time. Use the chart default so the test exercises a control plane sized the way a real one is, instead of one held at a quarter of the shipped default. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |