mirror of
https://github.com/cozystack/cozystack.git
synced 2026-10-28 07:37:49 +09:00
648eb42cae3f12bd367102048fa89089fc40407a
638
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
|
960d853706 |
feat(api): typed waitStrategy/healthCheckExprs for kstatus readiness (#2642) (#3273)
## What Adds typed `waitStrategy` + `healthCheckExprs` fields so a package/app can declare the Flux helm-controller v1.5.0 kstatus health gate for the custom resource it wraps — making `HelmRelease.Ready` mean "the CR is actually healthy" instead of "helm applied the CR". This is the operator/apiserver foundation of the kstatus epic (#2642); the API surface was reviewed and approved by the API owner in #3263. Commits (per the review's "isolated step-0" request): 1. `build(deps)` — the dependency bump (see below). 2. `feat(api)` — the fields, plumbing, tests, and regenerated code. 3. `fix(api)` — match the openapi code-generator pin to the 0.35 runtime (fixes an aggregated-apiserver crash the bump surfaced). 4. `fix(api)` — declare `OpenAPIModelName` on the apps types so SSA resolves catalog kinds. 5. `docs(api)` — note that `disableWait` silently defeats `healthCheckExprs`. ## API surface Typed fields on **both** HelmRelease-building paths: - `ApplicationDefinition.spec.release` (apiserver / catalog apps → `convertApplicationToHelmRelease`) - `ComponentInstall` (operator / platform packages → `buildHelmReleaseSpec`) ```go // WaitStrategy maps to HelmReleaseSpec.WaitStrategy.Name — a deliberate scalar // simplification of the upstream {name} object. One of poller|legacy. WaitStrategy string // HealthCheckExprs maps to HelmReleaseSpec.HealthCheckExprs (the upstream // kustomize.CustomHealthCheck type verbatim). HealthCheckExprs []kustomize.CustomHealthCheck ``` `config.ResolveWaitStrategy` is shared by both builders and **couples the default**: `healthCheckExprs` are only evaluated under the `poller` strategy, so when expressions are set and no strategy is given, the generated HelmRelease defaults to `poller`. A package that sets only `healthCheckExprs` is therefore self-contained, independent of the controller's global default. ## The dependency bump (commit 1) The `WaitStrategy` / `kustomize.CustomHealthCheck` / `HelmReleaseSpec.HealthCheckExprs` types do not exist below `helm-controller/api` v1.5.0 — the runtime is already v1.5.0 (shipped by #2602), so this closes a Go-types-vs-runtime skew. It is **not** an isolated bump: v1.5.x requires **k8s 0.35 + controller-runtime 0.23**, cascading the k8s stack 0.34 → 0.35 (and the go directive to 1.26). cozystack replaces `k8s.io/apimachinery` with a fork carrying the still-open upstream `ConvertToVersion` patch (k/k#135537). That single commit rebases cleanly onto apimachinery v0.35.0; published at `cozystack/apimachinery@release-1.35-pr135537`, and `go.mod` pins its pseudo-version. **The repo builds with zero source changes under the new stack.** > Maintainer note: please confirm the `release-1.35-pr135537` fork branch naming, or fold it into the `cozystack` branch as you prefer — the pin can be updated to match. ## Not included (conscious YAGNI) `disableWait` is intentionally omitted. The only known need (the kubernetes tenant-addon deadlock) was resolved child-side, and `waitStrategy: legacy` is not equivalent (legacy still waits Helm-v3-style; `disableWait` skips waiting entirely). Add it later when a concrete leaf needs a true no-gate escape hatch. ## Tests - `config.TestResolveWaitStrategy` — the couple-the-default logic (unset+exprs→poller; explicit legacy honored; unset+no-exprs→nil). - Operator + apiserver cases mirror each other (`TestBuildHelmReleaseSpecHealthCheckExprs` / `TestConvertApplicationToHelmRelease_HealthCheckExprs`). - `go build ./...` clean, `go vet` clean, `gofmt` clean, codegen regenerated and committed. The 4 backupstrategy CRDs regenerate with benign k8s-0.35 PodSpec description drift (DynamicResourceAllocation alpha→stable, toleration Lt/Gt, resizePolicy note) — no functional change. ## Follow-up Phase 3 (per-leaf adoption — postgres/kafka/mongodb/etc. declaring their CEL via these fields) builds on this. The full CRD→CEL catalog is maintained in the epic's readiness audit. Refs #2642 ```release-note feat(api): add `waitStrategy` and `healthCheckExprs` to ApplicationDefinition and ComponentInstall, so a HelmRelease reports Ready only once the custom resource it wraps is actually healthy ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added configurable CEL health-check expressions for application releases and package components. * Added `waitStrategy` support (`poller` and `legacy`) with automatic `poller` selection when health checks are configured and no strategy is set. * Propagated these settings into generated Helm releases, affecting readiness/wait behavior. * **Documentation** * Updated Application and PackageSource schemas to include `healthCheckExprs` and `waitStrategy`. * Extended related Kubernetes CRD documentation, including projected pod certificate annotations and workload reference fields. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Andrey Kolkov
|
e73ad68a85 |
fix(backups): support cnpg-operator with barman plugin (#3313)
<!-- Thank you for making a contribution! Here are some tips for you:
- Use Conventional Commits for the PR title: `type(scope): description`
- Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore
- Scopes are not an exhaustive list — pick the most specific scope for
the change and extend the list when a genuinely new area appears.
Examples:
- System components: dashboard, platform, operator, cilium, kube-ovn,
linstor, fluxcd, cluster-api
- Managed apps: postgres, mariadb, redis, kafka, clickhouse,
virtual-machine, kubernetes
- Development and maintenance: api, hack, tests, ci, docs, maintenance
- Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or
add a `BREAKING CHANGE:` footer
- If it's a work in progress, consider creating this PR as a draft.
- Don't hesistate to ask for opinion and review in the community chats,
even if it's still a draft.
- Add the label `backport` if it's a bugfix that needs to be backported
to a previous version.
-->
## What this PR does
Fixes #3300. Migrates CNPG Postgres backups off the deprecated native
`spec.backup.barmanObjectStore` (removed in CNPG 1.29; the
`barman-cloud-*` binaries it needs are absent from the `standard` image
variant keycloak-db pins, and the bare system-flavor tags apps/postgres
pins are deprecated upstream) onto the [CloudNativePG Barman Cloud
plugin](https://github.com/cloudnative-pg/plugin-barman-cloud), across
every Postgres backup surface: `apps/postgres`, `system/keycloak`, and
the `cozy-default-cnpg` BackupClass strategy driver. The chart renders
`spec.plugins` + a `barmancloud.cnpg.io/ObjectStore` CR; the
backup-controller driver SSA-applies an `ObjectStore` and patches
`spec.plugins`; `Backup`/`ScheduledBackup` use `method: plugin`. The
`postgres-operator` chart vendors and co-locates the
`plugin-barman-cloud` operator (with its cert-manager-issued mTLS) so
CNPG can discover it.
One platform-level change is required for the plugin to run and ships
with this PR (see the upgrade notes in
`docs/operations/backup-classes.md` and the release note below): the
CloudNativePG operator is bumped to 1.28.1 (also fixing an operator/CRD
version skew that broke all backups). The other prerequisite — the
LINSTOR scheduler admission webhook stripping
`initContainers[].restartPolicy` cluster-wide, which broke the plugin's
restartable sidecar — is already fixed on `main` (
|
||
|
Myasnikov Daniil
|
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
|
||
|
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
|
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> |
||
|
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
|
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 --> |
||
|
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> |
||
|
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> |
||
|
Aleksei Sviridkin
|
3a82ed43bc |
test(etcd): stop losing the metrics probe's output to the attach race
The metrics contract runs a one-shot curl Pod and reads a `code=<status>` marker back through `kubectl run --attach`. Attach connects only after the Pod reaches Running, so a container that exits before the connection is up writes its stdout into the void: the step then sees only kubectl's own "If you don't see a command prompt, try pressing enter." and reports the metrics endpoint as broken. curl against a Pod IP finishes in milliseconds, so losing that race is routine, and `--rm` compounded it by deleting the Pod the instant it exited, taking the log that still held the real output. The result was a false negative on a healthy cluster, non-deterministic in the way that reads as a flake because whether attach wins depends on Pod startup timing rather than on anything under test. Drop `--rm`, keep the Pod until its result is in hand, and re-read the log when the attach stream carried no marker. The recovery cannot mask a real failure: the marker encodes the outcome, so a recovered `code=503` stays a failure, and a stream that already carried a marker is never second-guessed. The pre-run delete stays blocking so the re-read cannot find a previous incarnation's marker, and the post-run delete is guarded so a transient API error cannot abort the step with the result in hand but unreported. Pin `--pod-running-timeout` at 90s rather than leaning on kubectl's 1m default. curlimages/curl is not otherwise used on these nodes, so the probe pays a cold pull, and a pull that outruns the budget produces the same markerless output this change exists to explain. The resolver is split into a sourced helper so the recovery is unit-tested without a cluster; the kubectl orchestration around it stays covered by the e2e run. The image-cache probe carries the same recovery inline for the same race. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
1e4a152291 |
test(mariadb): give first boot the readiness budget the probe grants
The suite allowed 3 minutes for a MariaDB endpoint address to appear, which was set when a replica had 40 seconds to finish its first boot. The startup probe now grants 310 seconds, so a healthy install can legitimately take longer than the gate allows and would be failed while the probe is still willing to wait. The endpoint assert is the gate that matters here. The chart hands the operator a MariaDB custom resource and ships no built-in workload, so helm has nothing to wait on and the HelmRelease reports Ready as soon as the manifests are applied, long before a server exists. An endpoint address, by contrast, appears only once a replica has passed its startup and then its readiness probe, so that assert is what actually covers bootstrap. Raise it to 10m in both the replicated and the single-replica test. One ready address satisfies the assert, so it has to clear a single startup budget rather than one per replica, plus the operator reconcile, PVC bind and image pull that precede it. The HelmRelease ceilings stay at 6m, since nothing about them gates on the server coming up. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Andrey KolkovandClaude Fable 5
|
8282c17d08 |
test(sdn): scope the new validate test's comment to what it proves
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey KolkovandClaude Fable 5
|
bbc0103949 |
test(sdn): pin the fail-closed suite-mapping contract in validate
Scrub the last "-" mention from the --validate usage doc (the escape no longer exists, so the header asserted the opposite of the behavior), and add the negative bats case the tightened branch was missing: a suite whose hardcoded mapping names a PackageSource absent from the graph must fail validation with the "maps to ..., not a PackageSource" error. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey KolkovandClaude Fable 5
|
4e9a40d811 |
test(sdn): drop the now-unreachable "-" mapping from suite_to_source
With the serviceexposure case gone no suite returns "-", and the only executable coverage of that branch was the bats test removed alongside it. Keeping an untestable dead branch for a hypothetical future suite is not worth it — re-adding the three lines with the next core-platform suite is cheaper, and validate/closure both simplify. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey KolkovandClaude Fable 5
|
bb1127c767 |
test(sdn): drop the serviceexposure chainsaw suite left after the feature removal
Commit |
||
|
Andrey KolkovandClaude Fable 5
|
50fb3c87c2 |
test(postgres): run the backup round-trip e2e in CI under TIA
Ungate postgres-2-backup-roundtrip: instead of an opt-in env flag it now runs whenever TIA selects the postgres suite (postgres changes, and every release cut), like the other chainsaw tests. The blocker was network placement, not capability: the S3 client is the barman-cloud sidecar inside the Postgres Pod, and the isolated e2e tenant's Cilium egress allowlist blocks it from tenant-root's seaweedfs — so the Test runs in tenant-root (same-tenant egress) against the in-cluster S3 endpoint, trusting its self-signed cert via the copied seaweedfs-system-ca-cert. This is exactly the recipe validated end to end on a real cluster. Also map examples/backups/<app>/ edits to that app's suite in hack/select-e2e.sh (the round-trip executes those scripts as its harness; previously such edits were silently ignored), with bats coverage for both the mapped and the no-suite case. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey KolkovandClaude Opus 4.8
|
bdc0b564e2 |
test(postgres): add gated barman-cloud backup/restore e2e round-trip
Port examples/backups/postgres into an automated e2e, following the etcd backup precedent: the numbered example manifests become a runnable harness (00-helpers.sh, run-all.sh, cleanup.sh) that a human reads and the test drives, so the documented flow and the test can never drift. run-all.sh provisions a Bucket, deploys the source Postgres (writing a sentinel row), derives the two Secrets the barman-cloud ObjectStore references from the Bucket's BucketInfo and seaweedfs' CA, submits an ad-hoc BackupJob and waits for Succeeded, then restores to a copy and asserts the sentinel round-tripped through S3 — the in-cluster witness that a plugin backup is both taken and restorable end to end. A second Test document (postgres-2-backup-roundtrip) in the existing hack/e2e-chainsaw/postgres suite drives run-all.sh, gated out of CI behind POSTGRES_E2E_S3_ROUNDTRIP=1 (kind has no reachable, CA-trusted S3 endpoint, and a tenant Pod cannot cross the Cilium egress policy to tenant-root's seaweedfs). Placing it in the postgres suite (rather than a new dir) keeps it triggered by packages/apps/postgres changes under Test Impact Analysis. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey KolkovandClaude Opus 4.8
|
604e79437b |
docs(backups): disclose CNPG-bump + LINSTOR scheduler change; fix stale e2e fixture
Review follow-up (cozystack#3300): - docs/operations/backup-classes.md: add an operator-facing upgrade note for the two platform-wide changes this migration carries — the CloudNativePG operator bump to 1.28.1 (with the CRD-skew fix and the Helm-CRD-upgrade caveat) and the LINSTOR scheduler admission-webhook removal, which makes data-locality scheduling advisory cluster-wide (spec.schedulerName no longer auto-assigned; opt in via explicit schedulerName). These affect more than Postgres backups and were previously only noted in a chart values.yaml comment. - hack/e2e-chainsaw/postgres/postgres.yaml: drop stale restic-era backup fields (s3Region, s3Bucket, cleanupStrategy, resticPassword) that no longer exist in the Postgres backup schema; use current fields (destinationPath, endpointURL, retentionPolicy, 6-field schedule). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrei Kvapil
|
fbbe4af7df |
refactor(network): drop ExposureClass/ServiceExposure for native loadBalancerClass (#3218)
## What Removes the `network.cozystack.io` API group — `ExposureClass` + `ServiceExposure` (#3081) — and its reconciler, in favor of native `Service` `type: LoadBalancer` + `loadBalancerClass`. ## Why Full rationale and discussion in #3164. For **managed applications** the chart already owns the Service, so: - `type: LoadBalancer` + `loadBalancerClass` + an admin-provisioned pool covers vendor-neutral external exposure without a dedicated API group and controller; - the `externalIPs` (node-IP) default never went through `ServiceExposure` (its backend is a no-op) and is left unchanged; - the only current consumer, the host ingress, keeps rendering `type: LoadBalancer` and just drops the `ServiceExposure`; the admin provisions the address pool / announcer out of band, as the install docs already assume. `ServiceExposure`'s one unique feature — dynamic per-namespace scoping of a shared VIP pool — is a convenience the managed-app path does not need, and it does not justify a second tenant-networking API group next to `sdn.cozystack.io`. ## Changes - Delete `api/network/v1alpha1` and `internal/controller/serviceexposure` (+ CRD definitions, e2e bats, controller registration, RBAC). - Host ingress: drop the `ServiceExposure` template; add an optional `publishing.loadBalancerClass` that sets `spec.loadBalancerClass` on the host-ingress LoadBalancer Service. - The `externalIPs` path and default behavior are unchanged. ## Verification - `go build ./...` — green. - `helm unittest` — `core/platform` 72/72, `extra/ingress` 25/25 (incl. host-vs-tenant `loadBalancerClass` scoping and the immutable-field migration guard). ## Migration None required. `network.cozystack.io` (#3081) has never shipped in a release — it is on `main` only (not backported to `release-1.5`; absent from v1.4.x / v1.5.x), so no upgrading cluster has these CRDs and this removes it before it is ever released. Clusters that track `main` and explicitly set `publishing.exposureClass` (opt-in, only since 2026-06-30) should provision their MetalLB / Cilium address pool themselves and set `publishing.loadBalancerClass`; any orphaned `cozystack-<class>` pool can be deleted by hand. Default clusters (externalIPs path) are unaffected. ## Status **Draft / RFC for the #3164 discussion** — this removes an API group that landed only days ago, so nothing here should merge before the design call. cc @lexfrei (author of #3081) and @lllamnyp (#3164). Deferred: when Kubernetes actually removes `Service.spec.externalIPs`, the node-IP default migrates via a MetalLB `IPAddressPool` with no `L2Advertisement` (allocate-but-don't-announce) — a separate change. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added support for configuring the ingress host Service with a native `loadBalancerClass` value. * Expanded chart tests to verify class wiring and immutability behavior across install and upgrade scenarios. * **Bug Fixes** * Removed the legacy exposure-class/service-exposure flow and related controller behavior. * Updated templates and CRD packaging so the platform no longer renders or depends on those deprecated resources. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Myasnikov Daniil
|
1ea4eb452c |
fix(api): regenerate OpenAPI with code-generator v0.35.0
This PR bumps the runtime k8s libraries to 0.35 (k8s.io/apiserver,
kube-openapi 20250910) but hack/update-codegen.sh still pinned the
openapi code-generator at v0.34.1.
The 0.34.1 openapi-gen keys the version.Info definition by its Go
import path ("k8s.io/apimachinery/pkg/version.Info"), whereas
kube-openapi 0.35 now resolves that model via the new OpenAPIModelNamer
interface — version.Info{}.OpenAPIModelName() ->
"io.k8s.apimachinery.pkg.version.Info". The key mismatch makes the
aggregated apiserver fatal while building its root OpenAPI spec
("cannot find model definition for io.k8s.apimachinery.pkg.version.Info"
-> klog.Fatalf, exit 255), crashlooping cozystack-api. That takes down
the apps.cozystack.io aggregated API, so the Bucket kind can no longer
be resolved and the backupstrategy-controller HelmRelease install fails,
reddening E2E.
Bump the code-generator pin to v0.35.0 (matching the runtime) and commit
the regenerated artifacts. No hand edits to generated files.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
|
||
|
myasnikovdaniil
|
f6a03e93f8 |
fix(etcd): complete v1alpha2 transition for in-cluster 1.5→1.6 upgrades (#3270)
Consolidates the etcd `v1alpha2` transition fix for **in-cluster 1.5 → 1.6 upgrades** into a single PR, rebased on current `main`. Supersedes #3265 and #3261 — their commits are carried here (authorship preserved), so those PRs can be closed once this lands. Fresh installs use the new shapes directly and are unaffected, so fresh-install CI does not catch these; the 1.5 → 1.6 e2e upgrade path is the authoritative regression guard. ## What this PR does ### 1. Keep the legacy `etcd-headless` Service alive during adoption — `packages/extra/etcd` `etcd-migrate` adopts legacy clusters **in place**: the Pods keep their original `spec.subdomain: etcd-headless` and are dialed at `etcd-<i>.etcd-headless.<ns>.svc` until they roll onto the operator's native `<member>.etcd.<ns>.svc` domain. The v1alpha2 operator only creates the native `etcd` Service and the legacy `etcd-headless` Service is pruned, so those per-pod names stop resolving (`no such host`), `MemberList` fails, and `status.readyMembers` never populates — the `EtcdCluster` never goes `Ready` even though etcd is healthy and in quorum. We ship a chart-managed transitional headless `etcd-headless` Service (selector mirrors the operator's native `etcd` Service via `etcd-operator.cozystack.io/cluster`, `publishNotReadyAddresses: true`) — the DNS counterpart of the legacy `*.etcd-headless.<ns>.svc` SAN already kept for this window. Removable together with that SAN once members roll onto the native subdomain. ### 2. Survive the immutable controller-Deployment selector on upgrade — `packages/system/etcd-operator` (#3242) #2859 replaced the upstream etcd-operator chart with the cozystack-authored one, changing `Deployment.spec.selector.matchLabels`. `spec.selector` is immutable, so `helm upgrade` cannot patch the existing Deployment and the whole HelmRelease upgrade fails (`field is immutable`). A **pre-upgrade hook** (`templates/pre-upgrade-selector-fix.yaml`: ServiceAccount + Role + RoleBinding + Job) deletes the Deployment **only** when its live selector is the pre-1.6 one, so Helm recreates it cleanly. No-op when the selector already matches, never runs on fresh install. ### 3. Raise the operator's memory cold-start floor — `packages/system/etcd-operator` Steady-state working set is ~250Mi; the static `limits.memory: 128Mi` OOMKills a Pod that starts before the VPA admission webhook rewrites it. Raise the floor to `256Mi` (and VPA `minAllowed` to match) as defense in depth so the operator never depends on VPA timing to avoid crashing. ### 4. Make migration 50 (etcd adoption) robust in-cluster — `packages/core/platform/images/migrations/migrations/50` (was #3261) Exact server/peer cert-SAN match, Secret-gated wait on the adoption Secret, and in-cluster (IPv6-safe) kubeconfig handling, so the migration script drives the adoption reliably from inside the cluster. Covered by `hack/migration-50-etcd-adopt.bats`. ### 5. Hardening / review fixes (this PR's original scope) - **Hook `runAsUser: 65532`.** `pre-upgrade-selector-fix.yaml` set `runAsNonRoot: true` but no numeric `runAsUser`; `clastix/kubectl`'s image user is the non-numeric name `nonroot`, which the kubelet cannot verify against `runAsNonRoot` — so the hook Pod fails admission and silently blocks the very upgrade it exists to unblock. Adds `runAsUser: 65532`, matching the postgres-operator webhook-ready hook that runs the same image. - **Digest-pin the kubectl image.** The values comment claimed digest-pinning but shipped a floating `v1.32` tag. Reuses the digest postgres-operator vendors, adds the `renovate` annotation, and templates `repo:tag@digest`. - **Tests.** `etcd-operator/tests/selector-fix-hook_test.yaml` (hook wiring, weight ordering, namespaced least-privilege RBAC, numeric-non-root security context, digest-pinned image), `etcd-operator/tests/deployment_test.yaml` (256Mi cold-start floor), `extra/etcd/tests/etcd-cluster_test.yaml` (transitional `etcd-headless` Service). Each assertion was mutation-tested. ### 6. Derive the default S3 endpoint from the provisioned bucket — `packages/system/backupstrategy-controller` Derive the default S3 endpoint (and per-driver scheme / TLS / `secure_connection`) from the provisioned bucket Secret instead of requiring it to be hand-set, so etcd (and other) backup strategies get a working endpoint by default. Docs in `docs/operations/backup-classes.md`; covered by `tests/endpoint_form_test.yaml`. ### Verification - `helm unittest` green on current `main`: etcd-operator **18/18**, extra/etcd **18/18**, backupstrategy-controller **11/11**. - Behaviours 1 & 2 were reproduced and confirmed live on a 1.5.2 → 1.6.0-rc.1 adoption (3-node cluster) — recreating the `etcd-headless` Service took the adopted cluster to `readyMembers=3 / Available=True`. ```release-note fix(etcd): complete the v1alpha2 transition on in-cluster 1.5→1.6 upgrades — keep the legacy etcd-headless Service alive so adopted members stay resolvable, delete the pre-1.6 operator Deployment via a pre-upgrade hook to get past the immutable selector, raise the operator's memory floor so it does not OOM before the VPA scales it, and make the etcd adoption migration robust in-cluster. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Improved backup storage endpoint handling across supported backup drivers, including provisioned and external S3 storage. * Added compatibility support for legacy etcd pod discovery during migration. * Added an automated upgrade safeguard for etcd operator deployments. * **Bug Fixes** * Improved certificate SAN detection and etcd migration authentication. * Increased the etcd operator’s minimum startup memory to prevent early restarts. * **Documentation** * Clarified backup endpoint, TLS, and driver-specific behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
IvanHunters
|
3fb8ec5b0e |
feat(monitoring): per-instance OIDC selector for the Grafana instance (Phase 1) (#3176)
<!-- Follow-up to cozystack/cozystack#3044 — same design family, different subject. --> ## What this PR does Adds a Phase-1 OIDC selector to the Grafana instance shipped by `packages/system/monitoring/`, mirroring the shape of the tenant kube-apiserver PR ([cozystack/cozystack#3044](https://github.com/cozystack/cozystack/pull/3044)) so that operators can use one mental model for both. Rationale in [cozystack/community#24](https://github.com/cozystack/community/pull/24) — per-cluster (here: per-Monitoring-release) audience isolation on the flat `cozy` realm, no per-tenant realm. Authorization is **app-side** via a chart-owned `users:` map reconciled by a post-install/post-upgrade Job into Grafana org membership. The chart does NOT own Keycloak directory objects (no `KeycloakRealmGroup`s) and does NOT rely on Grafana's `role_attribute_path` — same design shape as #3044's ClusterRoleBinding reconcile, adapted for Grafana orgs. See `docs/oidc-grafana.md` for the full rationale. The `Monitoring` CR grows a new field: ```yaml spec: oidc: mode: None | System | CustomConfig # default None customConfig: # only for CustomConfig config: {} # inline grafana.ini keys, or... secretRef: name: "" # ...an operator-managed Secret users: # optional; ignored in mode=None - email: alice@example.com role: Admin | Editor | Viewer ``` ### Modes - **`None`** (default) — no OIDC; `admin_user` / `admin_password` Secret is the only auth path. Existing releases render byte-identical. - **`System`** — chart provisions, in the release namespace of the Monitoring CR: - `KeycloakClient` (`public: false`, confidential, `secret` sourced from a chart-owned Secret via the EDP `$<secret>:<key>` reference; `redirectUris` locked to `https://grafana.<host>/login/generic_oauth`). - `KeycloakClientScope` with an `oidc-audience-mapper` pinning `id_token.aud` to the per-release clientId — the isolation primitive. - Persistent Kubernetes Secret carrying the confidential `client-secret` (random on first install, preserved via `lookup`+fallback, same pattern as `packages/system/dashboard`). - Grafana CR's `spec.config.auth.generic_oauth` section wired to the `cozy` realm issuer + per-release audience scope, with two chart-forced settings the users-Job depends on: `skip_org_role_sync: "true"` (login never overwrites the Job's org-role assignments) and `oauth_allow_insecure_email_lookup: "true"` (OIDC identity binds to the pre-provisioned local account by email). - `GF_AUTH_GENERIC_OAUTH_CLIENT_SECRET` env on the Grafana Deployment sourced from the Secret. - Post-install / post-upgrade **users-Job** reconciling `spec.oidc.users` into Grafana's Main Org.: pre-provisions a local account per listed email, `POST /api/orgs/1/users` + `PATCH /api/orgs/1/users/{userId}` to converge role changes, prunes every non-admin Main-Org member whose email is not in the list. - Server-level `GrafanaAdmin` promotion (`allow_assign_grafana_admin`) is out of scope for Phase 1. Every Grafana instance (platform and tenant) caps at org-level `Admin`. - **`CustomConfig`** — tenant supplies the whole `[auth.generic_oauth]` payload; `cozy` is not in the path. Two mutually exclusive paths: - `customConfig.config: {...}` — inline map of `grafana.ini` keys. The chart **merges** the two chart-forced settings (`skip_org_role_sync`, `oauth_allow_insecure_email_lookup`) on top so the users-Job contract holds. The operator can NOT override those two (merge wins). - `customConfig.secretRef.name: ...` — operator-owned Secret with an `auth.ini` fragment; the chart mounts it under `/etc/grafana/oidc` and wires `GF_PATHS_CUSTOM_INI`, and does NOT emit `auth.generic_oauth` under `spec.config` (the ini fragment is authoritative). Because the chart cannot inject the two settings into a mounted ini, `spec.oidc.users` is NOT supported in this branch — the chart hard-fails render on that combination. ### Fail-close guards - `mode: System` without `_cluster.oidc-enabled == "true"` (the platform-level `authentication.oidc.enabled` flag) → hard-fail with an actionable message pointing at the platform values. - `mode: System` on a cluster where the EDP Keycloak operator CRDs (`v1.edp.epam.com/v1`) are not registered → hard-fail (symmetric on `oidc-keycloak.yaml` and `grafana.yaml`). - `mode: CustomConfig` with both or neither of `config` / `secretRef.name` → hard-fail. - `mode: CustomConfig` with `secretRef.name` AND non-empty `users` → hard-fail (chart cannot inject the two forced settings into an operator-mounted ini; forcing an explicit choice avoids silent contract loss). - `spec.oidc.users[]` entry with missing / unknown `role` or malformed `email` → hard-fail at render time so the users-Job never fires with a body that would 400 on Grafana's admin API. ### Break-glass posture `admin_user` / `admin_password` and `disable_login_form: false` are unchanged in every mode. Locking the form off under `mode: System` is a documented follow-up hardening (see `docs/oidc-grafana.md`). ### What's NOT in this PR (documented as out of scope) - Per-tenant Keycloak realms (Phase 2 territory, tracked in community#24). - Full-logout through Keycloak's end-session endpoint (`backend-logout-url` and matching Keycloak client attribute). - CEL `claimValidationRules` gate on `email_verified` — layered guarantees are described in `docs/oidc-grafana.md`. - Multi-issuer composition on one Monitoring release (mutually exclusive modes). - Server-level `GrafanaAdmin` promotion. ### Docs - New `docs/oidc-grafana.md` — full operator guide (parallel to `docs/oidc-tenant.md`). - Website page follows in a paired PR against cozystack/website (`docs/monitoring-oidc-authentication` branch, under `content/en/docs/next/operations/services/monitoring/oidc-authentication.md`). ### Tests - 33 `helm-unittest` cases in `tests/oidc_test.yaml` + `tests/oidc_crd_missing_test.yaml` cover: all three modes; defensive nil paths (`oidc: ~`); System-mode Keycloak client + audience scope shape; CustomConfig chart-forced merge (positive assertion) AND `mode=CustomConfig inline — operator cannot override chart-forced OIDC settings` (regression guard); CustomConfig secretRef + `users` incompat fail-fast; `spec.oidc.users[].role` missing/unknown fail-fast; users-Job shape (hook annotations, RBAC-free, activeDeadlineSeconds, `ttlSecondsAfterFinished: 3600`, admin credential wiring); no chart-owned `KeycloakRealmGroup`s in any mode; no `role_attribute_path` in Grafana config. - Render-side bats in `hack/e2e-apps/monitoring-oidc-{system,customconfig}.bats` — mirror the tenant kube-apiserver's e2e shape (no live browser flow — deferred to a follow-up integration suite, same as #3044). Asserts on live cluster: users-Job appears with `DESIRED_USERS_JSON` populated, Grafana config carries `skip_org_role_sync=true` and no `role_attribute_path`, no chart-owned `KeycloakRealmGroup`s land in `cozy`. ### Screenshots N/A — no UI change on default values; the visual delta is Grafana's own "Sign in with Keycloak" button appearing under the login form when a release opts in. ### Release note ```release-note feat(monitoring): Each `Monitoring` CR can now opt its Grafana instance into OIDC via a flat selector — `spec.oidc.mode: System | CustomConfig | None` (default `None`). `System` trusts the platform `cozy` realm via a per-instance confidential Keycloak client + audience binding. Authorization is app-side: `spec.oidc.users: [{email, role: Admin|Editor|Viewer}]` is reconciled into Grafana's Main Org. by a chart-owned post-install/post-upgrade Job (pre-provision → org add → PATCH role → prune stale). `CustomConfig` accepts a tenant-supplied `[auth.generic_oauth]` payload — inline as a map (chart merges the two contract-critical settings `skip_org_role_sync` + `oauth_allow_insecure_email_lookup` on top) or as a Secret with an `auth.ini` key (in which case `spec.oidc.users` is not supported and the chart fails render on that combination). Server-level `GrafanaAdmin` promotion is out of scope for Phase 1. The `admin_user`/`admin_password` Secret stays a documented break-glass path in every mode. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added OIDC authentication for Monitoring Grafana with `None`, platform-managed `System`, and tenant-controlled `CustomConfig` modes. * System mode provisions platform-backed OIDC for each instance (including persistent client credentials) and can optionally promote matching users to server-level `GrafanaAdmin`. * CustomConfig supports inline OAuth settings or Secret-backed `auth.ini` (with mutual exclusivity validation). |
||
|
Andrey Kolkov
|
47d239c519 |
fix(platform): address review on migration 50 (exact SAN match, Secret-gated wait, IPv6 kubeconfig)
Review feedback on #3261 (gemini-code-assist, coderabbitai, myasnikovdaniil): - Exact SAN match. _san_present read the Certificate SANs as {.spec.dnsNames} (a bracketed JSON blob that `tr ','` cannot split) and matched with a substring `grep -qF`, so a wildcard that is a substring of a longer SAN (e.g. *.etcd.<ns>.svc inside *.etcd.<ns>.svc.cluster.local) could false-positive and skip the re-issue. Read {.spec.dnsNames[*]} (space separated) and match exactly with `grep -qxF`, matching the Secret-annotation branch. - Wait on the re-issued Secret, not the patched spec. The post-patch wait loop called _san_present, which returns as soon as the Certificate spec.dnsNames contains the wildcard -- but the patch just added it there, so the loop broke on the first iteration before cert-manager re-issued the Secret, and the "re-issued" log printed unconfirmed. Add _secret_has_san (Secret alt-names annotation only, same empty-read retry + exact match) and gate the wait on it. - IPv6-safe kubeconfig server. The synthesized kubeconfig used https://$KUBERNETES_SERVICE_HOST:$KUBERNETES_SERVICE_PORT, which is an invalid URL on an IPv6-only cluster (a bare IPv6 host must be bracketed). Use https://kubernetes.default.svc -- IP-family-agnostic, validated by the mounted SA CA, and the same server URL kubectl synthesizes in-cluster. Tests (the review blocker): extend the bats suite (and its fake kubectl) with two cases pinning these contracts -- a superstring dnsName must NOT skip the re-issue patch, and etcd-migrate must be invoked with a synthesized in-cluster kubeconfig whose server is kubernetes.default.svc (not the bare IPv6 host), authenticating via the SA token file + CA. Both the kubeconfig path and the SA directory are now env-overridable (ETCD_MIGRATE_KUBECONFIG, ETCD_ADOPT_SA_DIR) so the synthesis is exercisable off-cluster; production defaults are unchanged. Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
myasnikovdaniil
|
215bd430a9 |
test(e2e): diff-driven install selector select-install.sh (#3279)
## What this PR does Adds `hack/select-install.sh` — the install-side companion to `hack/select-e2e.sh`. Where `select-e2e.sh` picks *which* Chainsaw suites run for a diff (a reverse dependency walk), `select-install.sh` picks *what must be installed* for those suites: the forward `dependsOn` closure over the `PackageSource` graph. Given the suite names `select-e2e.sh` emits, it maps each back to its `*-application` `PackageSource` and walks `spec.variants[].dependsOn` forward to the transitive closure — the minimal set of packages to enable instead of provisioning the full `isp-full` platform. A `postgres` chart change, for example, resolves to 9 of 97 sources. Design notes: - Reuses the real `PackageSource` graph in `packages/core/platform/sources/` — no parallel dependency-declaration files to keep in sync. - Unlike `select-e2e.sh`'s reverse walk, the forward walk **keeps** the `cozystack.cozystack-engine` edge: for test *selection* it is noise (it would fan every app out to every other), but for *install* it is a genuine prerequisite (the app's `*-rd` HelmRelease needs the engine to register the ApplicationDefinition CRD). Because that edge lives only on `*-application` sources, a suite backed by a **system** package (e.g. `securitygroup`, whose `SecurityGroup` in `sdn.cozystack.io` is served by the aggregated `cozystack-api` — a `cozystack-engine` component) can't reach the engine via the walk, so the engine is **seeded explicitly for any non-empty closure** (and a graph missing it is a hard error). - `--validate` mode checks the whole graph — every `dependsOn` target resolves to a real `PackageSource` (reachability) and there are no dependency cycles — **and** the suite mapping: every `hack/e2e-chainsaw/*/` suite resolves to a real `PackageSource` or to `-` (a missing suites dir is itself a failure). The graph lives next to the packages and can't drift from the suite dirs; the hand-maintained mapping is the only part that can, so this is the cheap CI guard. - Suite→source mapping follows the `<suite>-application` convention with a bare-`<suite>` fallback, plus explicit cases for the irregular names (`kubernetes`/`vminstance`/`securitygroup`) and for core-platform features that have no separately-enabled package (`serviceexposure`, whose `ExposureClass`/`ServiceExposure` CRDs live in `packages/core/platform`). A suite may reference other app kinds by selector without standing them up (e.g. `securitygroup`'s `SecurityGroup` names `Postgres`/`Kubernetes` as selectors only) — only the primary package is mapped here; the forward closure and the seeded engine cover its prerequisites, and full multi-package modelling is left to the `test-minimal` follow-up. A suite with no known mapping is a **hard error** (fail closed), not a silent skip. Includes a 17-case unit suite (`hack/select-install_test.bats`) covering single-app closure, transitivity, multi-suite union, stdin input, the kubernetes/irregular/core-platform suite mappings, the `securitygroup` engine-seed regression, empty input, fail-closed unmapped suites (all reported, no partial output), the missing-engine hard error, and the `--validate` failure modes (dangling dep, cycle, unmapped suite dir, missing suites dir). It is auto-discovered by `make unit-tests` (`BATS_UNIT_FILES` globs `hack/*.bats`). Verified end-to-end against real git diffs across six path types (app chart, shared operator, docs-only, per-suite Chainsaw edit, library, shared e2e helper): a `postgres` chart change scopes the install to 9 of 97 sources; the full-suite fallback to 39 of 97. This is Track A of #2847 (diff-driven & scenario-based E2E); tracked in #3278. Consuming the output — a `test-minimal` platform variant/bundle plus wiring into the `pull-requests.yaml` install step — is a deliberate follow-up, since it depends on a platform-variant design decision. ### Screenshots Not applicable — CI/tooling-only change. ### Release note ```release-note test(e2e): add hack/select-install.sh to compute the minimal package install set for a diff-scoped E2E run (forward PackageSource dependency closure), with a --validate graph-consistency mode ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added a tool to select the minimal package set required for one or more end-to-end test suites. * Supports combining multiple suites, resolving transitive dependencies, and handling unmapped or empty suite selections. * Added validation for missing dependencies and circular dependency definitions. * **Tests** * Added coverage for package selection, dependency resolution, suite mappings, empty selections, and validation failures. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
IvanHunters
|
3776c447b9 |
test(e2e): poll users-Job DESIRED_USERS_JSON for target email, not just Job existence
CI E2E showed the users-Job assertion returning `DESIRED_USERS_JSON:
[]` — but the CR body had `users: [{email: e2e-admin@example.com,
role: Admin}]`. Root cause: `hook-delete-policy: before-hook-creation`
keeps the previous hook Job around across helm upgrades, so `until
kubectl get job` matches a stale Job from the previous customconfig-
mode test where users may have been unset (the secretRef swap CR had
no `users:` field, so the schema default `[]` propagated to the
prior Job's DESIRED_USERS_JSON). The System test then grabbed that
stale Job before the helm upgrade for the new CR completed and swapped
in the fresh Job.
Fix: poll on the target env value itself — the new Job with the
correct users list is the first Job whose DESIRED_USERS_JSON string
contains `e2e-admin@example.com`. 600s window matches the inner HR's
15m timeout with room for a slow reconcile.
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
|
||
|
IvanHunters
|
7f9e1a8bf6 |
test(e2e): poll the reconciled Grafana CR field instead of racing on CR existence
CI run 28794778739 caught two race conditions in the render-side Monitoring OIDC bats: - `Grafana CR carries auth.generic_oauth pointing at the cozy realm` (monitoring-oidc-system.bats:86) failed with `enabled=` empty after a 0-second wait. The prior `until kubectl get grafana grafana` only proved the CR EXISTS — it did not prove grafana-operator had reconciled the mode=System values onto it. When the CR from the previous `Monitoring CR accepts spec.oidc.mode=System` test was already there but the operator hadn't yet flipped `.spec.config.auth .generic_oauth.enabled` to "true", the jsonpath returned empty and `[ "" = "true" ]` short-circuited the whole test. - `secretRef variant mounts operator Secret under /etc/grafana/oidc` (monitoring-oidc-customconfig.bats:119) timed out at 180s waiting for the volumeMount swap from inline → ini. On a warm sandbox the full inner-HR upgrade + grafana-operator reconcile cycle can legitimately take >3m — the timeout was too tight. Fix: replace `until kubectl get grafana` with `until [ "$(kubectl ... jsonpath=<target-field>)" = <expected> ]` on every subsequent assertion, and bump the secretRef-swap window to 600s (well inside the inner HR's 15m timeout). Same pattern used for the "no role_attribute_path" and "operator payload verbatim" assertions so the whole suite is race-free. Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
123616a1d0 |
test(e2e): swap KeycloakRealmGroup assertions for the users-Job in bats
Aligns the render-side e2e bats with the users-map contract: - Feed a `spec.oidc.users:` entry to each mode's Monitoring CR so the reconcile Job has real input to act on. - Assert the users-Job appears in the release namespace with the desired `DESIRED_USERS_JSON` env and the expected `OIDC_MODE`. - Assert the Grafana CR renders with `skip_org_role_sync = "true"` and `oauth_allow_insecure_email_lookup = "true"` and NO `role_attribute_path`. - Flip the three KeycloakRealmGroup existence assertions to negative checks: chart-owned realm groups must NOT be present in either System or CustomConfig mode. Does not add a full OIDC login flow through Keycloak — that stays deferred to a follow-up integration suite, same posture as the tenant kube-apiserver's kubernetes-oidc-system.bats. Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
Ivan Okhotnikov
|
5f7ed89799 |
fix(e2e): work around EDP schema-default strip + kubectl delete/apply race
Two follow-up bats fixes uncovered by the first live e2e pass on the
singleton-aligned monitoring OIDC tests:
1. monitoring-oidc-system, test 4 (KeycloakClient assertion). The EDP
Keycloak operator's KeycloakClient CRD strips `spec.public: false`
on write because false is the schema default. The chart emits
`public: false` explicitly (confidentiality guard), but neither
managedFields nor the resulting spec ever carries the field, so
the assertion `[ "${public}" = "false" ]` grepped an empty string
and tripped. Assert `spec.clientAuthenticatorType == "client-secret"`
instead — the operator persists it, and it is the direct signal
for "confidential client" (which is what the `public: false`
assertion was testing for).
2. monitoring-oidc-customconfig, test 4 (secretRef variant). The
original flow was delete-then-recreate the Monitoring CR to swap
inline `customConfig.config` for `customConfig.secretRef.name`.
`kubectl apply` immediately after delete raced the API server's
grace-period cache: apply's GET-then-decide phase saw the CR
during grace, chose the patch path, and the patch hit NotFound
once the delete propagated. Drop the delete entirely — cozystack-
operator reconciles the CR mode change on the same object and
Flux re-renders the chart with the new values.
Add an explicit wait for the target state (Grafana CR's
`oidc-custom-ini` volumeMount present AND `spec.config.auth.generic_oauth`
absent) before the assertions, since the CR update triggers an
asynchronous chart re-render and the immediate get would still
see the inline auth.generic_oauth block from @test 1.
Signed-off-by: Ivan Okhotnikov <xorokhotnikov@gmail.com>
|
||
|
Ivan Okhotnikov
|
cb04caecee |
fix(e2e): align monitoring OIDC bats with chart singleton naming
The extra/monitoring chart enforces `.Release.Name == "monitoring"`
in templates/check-release-name.yaml (the tenant Monitoring is a
per-namespace singleton). Naming the Monitoring CR anything else
makes the operator materialise an outer HelmRelease with that name
and the chart hard-fails at render with
execution error at (monitoring/templates/check-release-name.yaml:2:4):
The name of the release MUST BE monitoring
Rename the CR to `monitoring` in both files. Rework every derived
name to match the actual reality of the chart pipeline instead of
the invented `monitoring-<TEST_NAME>` prefix the tests polled for:
* outer HR = ${CR_NAME} = monitoring
* inner HR = ${CR_NAME}-system = monitoring-system
(rendered by extra/monitoring/templates/helmrelease.yaml as
`metadata.name: {{ .Release.Name }}-system`)
* clientId = ${namespace}-${inner Release.Name}
= tenant-test-monitoring-system
* scope = ${clientId}-audience
* groups = ${clientId}-{admin,editor,viewer}
* client-secret Secret = ${inner Release.Name}-oidc-client
= monitoring-system-oidc-client
The BYO Secret name in the customconfig test is chart-independent
(the tenant provides it via customConfig.secretRef.name), so it is
kept short and no longer tied to CR_NAME.
The wait sequence in the first @test of each file is also split
into two probes: outer HR appears first (from the operator), then
inner HR (from the extra/monitoring chart's helmrelease.yaml), then
the Grafana CR. Under the previous incorrect single-probe wait
(`hr monitoring-${TEST_NAME}`) the timeout tripped after 60s
because that HR never existed under any name.
Signed-off-by: Ivan Okhotnikov <xorokhotnikov@gmail.com>
|
||
|
Ivan Okhotnikov
|
9ff05b2f32 |
test(e2e): port monitoring OIDC bats to cozytest lifecycle conventions
Mirrors the same port applied to kubernetes-oidc-*.bats in the tenant
kube-apiserver OIDC series: cozytest.sh (the e2e runner) is not real
bats, so setup()/teardown() are never invoked and any state they
established (TEST_NAME) is unset by the time an @test body runs,
tripping cozytest.sh's `set -eu` at the first "${TEST_NAME}"
expansion with `TEST_NAME: parameter not set`.
Lift TEST_NAME to file scope as a stable constant, register cleanup
via cozy_cleanup() so cozytest.sh runs it at suite exit and on the
first failing test, and call cleanup_mon() inline at the top of the
first @test for per-run isolation. Fixes the two E2E failures on
this PR: `Monitoring CR accepts spec.oidc.mode=CustomConfig with
inline config` and `Monitoring CR accepts spec.oidc.mode=System`.
Signed-off-by: Ivan Okhotnikov <xorokhotnikov@gmail.com>
|
||
|
Ivan Okhotnikov
|
fe2c606f79 |
test(e2e): add Phase-1 OIDC render-side bats for Monitoring
Two bats files mirroring the shape of the tenant kube-apiserver PR's
kubernetes-oidc-{system,customconfig}.bats:
- monitoring-oidc-system.bats — asserts the Monitoring CR admits the
new spec.oidc.mode: System field; the chart renders the Grafana CR
with an auth.generic_oauth section pointing at the cozy realm; a
32-char client-secret Secret is persisted; the KeycloakClient (with
public: false), KeycloakClientScope (audience mapper), and three
KeycloakRealmGroups (-admin, -editor, -viewer) land in cozy; and
the Grafana Deployment injects GF_AUTH_GENERIC_OAUTH_CLIENT_SECRET
from the Secret.
- monitoring-oidc-customconfig.bats — asserts the CR admits inline
CustomConfig; the operator's payload lands on spec.config verbatim
with no cozy realm in the auth_url; NO KeycloakClient / Scope /
Groups are created in cozy; and the secretRef variant mounts the
operator Secret under /etc/grafana/oidc without emitting
auth.generic_oauth under spec.config.
Neither drives a real browser login through Keycloak — that is
deferred to the follow-up integration suite, same posture as the
tenant kube-apiserver's Phase-1 tests.
Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io>
|
||
|
Myasnikov Daniil
|
1f084cecc8 |
test(e2e): harden select-install.sh per review
Address the review blockers on the diff-driven install selector: - B1: seed cozystack.cozystack-engine for any non-empty closure. Suites backed by a system package (e.g. securitygroup, which applies a sdn.cozystack.io resource served by the aggregated cozystack-api) carry no dependsOn edge to the engine, so the forward walk could never reach it and the emitted set could not run the suite. Hard-error if the engine source is absent from the graph. - B2: an unmapped suite is now a hard error in closure mode (every bad suite reported in one run, no partial output) instead of failing open. --validate additionally asserts every hack/e2e-chainsaw suite resolves to a real PackageSource or "-", and fails when the suites dir is missing. Also correct the inaccurate "securitygroup also stands up a Kubernetes app" comment (its fixtures reference Postgres/Kubernetes as selectors only), note the variant-union over-selection in build_forward_deps, drop the unnecessary source copies from the tests, and add coverage for stdin input, the engine seed, fail-closed mapping, the --validate suite guard, and the missing-suites-dir / missing-engine failure modes. Assisted-By: Claude <noreply@anthropic.com> Assisted-By: GPT-5 <noreply@openai.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Aleksei Sviridkin
|
6b1ad96093 |
fix(e2e): gate LINSTOR node-list probe on a ready controller endpoint (#3250)
## What this PR does The E2E `Install Cozystack` step runs `hack/e2e-post-install-prep.sh` in the background, which waits for 3 LINSTOR satellites to come Online by dialing the `linstor-controller` Service (`linstor+ssl://linstor-controller:3371`) from the controller pod. That probe was gated only by a one-shot `kubectl wait deployment/linstor-controller --for=condition=available`, which goes stale. The controller carries `reloader.stakater.com/auto=true` and mounts the cert-manager-issued API-TLS Secret (`packages/system/linstor/templates/cluster.yaml`), so when cert-manager (re)writes that Secret during bring-up, reloader rolls the single-replica controller. The roll drops the Service to zero ready endpoints *after* `Available` already passed, and a dial into that window fails with `[Errno 113] No route to host` (EHOSTUNREACH — the state of a ClusterIP whose Service has no ready backend). The blind `until` loop then churned the CLI on that error, tolerating a real un-Ready-controller race instead of gating on it. This gates every probe on the `linstor-controller` Service actually having a ready endpoint (a non-empty `Endpoints .subsets[].addresses`), so the CLI only dials a routable controller and a mid-loop reload re-waits for the endpoint instead of erroring. The `Online == 3` satellite-convergence check stays the real readiness assertion, and the dedicated 300s budget is preserved from the previous `timeout 300`; on timeout the loop now dumps the controller endpoints and pods for diagnostics before failing, per the E2E diagnostics convention. Evidence for the mechanism: the same single `Error: Unable to connect to linstor+ssl://linstor-controller:3371: [Errno 113] No route to host` line appears exactly once per install across multiple runs and is immediately recovered by the retry — consistent with one reloader-driven controller roll during initial bring-up, not a persistent outage. Scoped to the LINSTOR readiness path in `hack/e2e-post-install-prep.sh` only; no change to install ordering, dependencies, or timeouts. ### Release note ```release-note NONE ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved the LINSTOR startup readiness check to wait more reliably for the service to become reachable before verifying node status. * Made the node-online verification more resilient by handling transient connectivity errors and enforcing a clearer timeout. * Added extra diagnostic output when readiness does not complete in time, helping with troubleshooting failed installs. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
myasnikovdaniil
|
6c9edf5038 |
fix(platform): route OIDC backend calls through the in-cluster Keycloak by default (#3289)
## What this PR does Defaults `authentication.oidc.keycloakInternalUrl` to `http://keycloak-http.cozy-keycloak.svc:8080/realms/cozy` instead of an empty string. With the empty default, oauth2-proxy performed OIDC discovery against the external Keycloak hostname. That request resolves through public DNS and lands on the root ingress controller, which does not serve yet on a fresh install: the root tenant's IngressClass only exists once `tenant-root` is ready, and `tenant-root` is still reconciling while the dashboard starts. Discovery has nothing to talk to, the gatekeeper fails, and the dashboard never comes up — with no obvious diagnosis for a first-time installer. The bundled Keycloak is the only supported provider (the external hostname is templated as `keycloak.<host>` and the realm is fixed), so backend-to-backend calls have no reason to leave the cluster. The new default is the same Service address `keycloak-configure` already uses for its own backend calls. The dashboard and linstor-gui gatekeepers then run with `--skip-oidc-discovery` and send token, jwks, userinfo and logout requests straight to the Service, while browser-facing redirects stay on the external host. Both gatekeeper templates already branch on the value being non-empty, so neither needed a change. The value stays overridable for a Keycloak that does not live at the standard in-cluster address, and setting it back to `""` restores the previous external-discovery behaviour. Token validation is unaffected: Keycloak pins `KC_HOSTNAME` to the external URL, so tokens redeemed over the internal Service still carry the external `iss` and match the unchanged `--oidc-issuer-url`. ### Upgrade behaviour Existing OIDC clusters that never set `keycloakInternalUrl` pick up the new default on upgrade: their gatekeepers roll once and move to the internal path. Clusters that set the value explicitly — including to `""` — keep exactly what they configured. ### Security posture The backend leg (token redemption, which carries the gatekeeper's Keycloak client secret, plus jwks/userinfo/logout) now travels as plaintext HTTP between pods rather than HTTPS to the ingress. This is stated in the `values.yaml` comment rather than left implicit. The `keycloak-http` Service has no TLS listener, so in-cluster HTTPS is not an available alternative, and `keycloak-configure` already sends Keycloak *admin* credentials over that identical address — anyone positioned to read this traffic can already reach a strictly more powerful secret. The externally reachable front door (browser to oauth2-proxy) is unchanged. ### Testing helm-unittest coverage across all three affected charts: the default landing on the `_cluster` channel, both gatekeepers rendering the in-cluster backend URLs while keeping browser URLs external, an explicit override winning, the empty-string escape hatch still rendering the external-discovery shape, and OIDC-disabled clusters (now the common case of "OIDC off, internal URL set") ignoring the value entirely. The E2E OIDC case previously applied this exact URL as a manual workaround. It now sets only `oidc.enabled` and asserts the consequence: it waits for the `auth-proxy` container to appear in the gatekeeper Deployment, then gates on `kubectl rollout status`. Readiness alone would prove nothing there — the HelmRelease is still `Ready` from the token-proxy install, and with `replicas: 1` and `maxUnavailable: 25%` (which rounds down to 0) the Deployment keeps the old ReplicaSet `Available` while a new pod crashloops. Only the rollout catches a regression back to external discovery. Follow-up: the OIDC documentation in the website repo still frames `keycloakInternalUrl` as an optional tweak "for self-signed certificates or restricted external access". It is now the default path and needs a separate docs PR. Closes #3288 ### Screenshots No UI changes. ### Release note ```release-note fix(platform): `authentication.oidc.keycloakInternalUrl` now defaults to the in-cluster Keycloak Service (`http://keycloak-http.cozy-keycloak.svc:8080/realms/cozy`). OIDC backend calls from the dashboard and linstor-gui gatekeepers no longer leave the cluster, so enabling OIDC on a fresh install no longer leaves the dashboard down while the root ingress is not yet serving. Existing clusters that never set the value pick up the new default and roll their gatekeepers once. Set the value explicitly to point at a non-standard Keycloak placement, or to an empty string to restore external OIDC discovery. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - OIDC-enabled applications now use the internal Keycloak service for backend authentication by default, improving reliability and avoiding unnecessary external discovery. - Browser redirects and login flows continue using the external Keycloak address. - Operators can provide a custom internal Keycloak URL or set it to an empty value to restore external OIDC discovery. - **Bug Fixes** - Dashboard and Linstor GUI authentication now correctly apply internal Keycloak settings only when OIDC is enabled. - Dashboard upgrades now wait for authentication components to become ready before completing. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
44507d2cd0 |
docs(agents): add downstream repository checklist to the PR template
Cozystack is upstream for repositories that nothing keeps in sync with it, and no CI job compares the two sides, so a change here breaks them in silence. Only two narrow paths are automated, and both fire on a release rather than on a PR. It has already happened. The website docs generator works from an app list hardcoded in its own Makefile, and packages/apps/opensearch was never added to it, so an application we ship has no reference page at all. The Terraform provider still offers a Kubernetes version this repo removed. The plugin repo still sends operators to hack/ scripts that were deleted, one of them rewritten as a Go command. Add a Downstream Repositories trigger map to the contributing guide, naming per repository which change here forces a change over there and which file to touch, and a checklist in the PR template that links to it. The template addresses AI agents directly: they author a large share of PRs and will otherwise tick a box without opening the follow-up it claims. The coupling is wider than the obvious ones, and mostly invisible. talm restates the node contract, overlapping ours but already diverged, so it must be diffed rather than overwritten. cozyhr reads an annotation the operator writes and silently merges the wrong values if it is renamed. cozy-proxy is vendored in from its own repo, yet consumes labels produced here and quietly stops handling external VM services if they move. The telemetry server queries metric names emitted here, and a rename just empties its dashboards. None of these fail loudly. Guard the map with a unit test, because a map about silent drift must not drift silently itself. It pins every in-repo path the map cites to a file that exists, pins the map and the checklist to the same repository list, and pins the workflow carve-out below to the map's location, to the job that reads the flag, and to the output that carries it: cutting any one of those three leaves the other two looking wired up. The header states plainly what the suite cannot check, so a green tick is not mistaken for a correct map: whether a coupling is real, and whether it still holds over there, stays a human's job. Editing the map alone is a docs-only PR, which skips the unit tests and would exempt the map from its own guard. Rather than widen the docs-only rule, which also gates the Talos image build, the plan step exports a separate flag that only the unit-test job reads: a docs PR now runs the tests and still builds nothing. It detects the map by its old path too, since a rename is reported as the new path alone. Document that gh pr create --body and --body-file replace the body wholesale, which would otherwise drop the checklist from every CLI-authored PR. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
6f5889816b |
feat(ci): guard migrations.targetVersion against silent skip (#2771)
## Summary - Add `hack/check-migrations-target.sh` that fails when the latest numbered file under `packages/core/platform/images/migrations/migrations/` is not matched by a bumped `migrations.targetVersion` in `packages/core/platform/values.yaml`. - Wire it into `unit-tests` via a new `migrations-target-check` Makefile target so PR CI catches the regression mechanically. `run-migrations.sh:34` iterates `seq $CURRENT_VERSION $((TARGET_VERSION - 1))`, so a migration whose number equals the configured `targetVersion` is silently skipped. The cost is state corruption on upgrade — a class of bug worth eliminating with a 10-line check rather than relying on reviewer vigilance. ## Test plan - [x] `make migrations-target-check` passes on `main` HEAD (`targetVersion=43`, latest migration `42`, `43 >= 43`). - [x] Synthetic failure: `touch packages/core/platform/images/migrations/migrations/99` makes the script exit 1 with a clear error pointing at `packages/core/platform/values.yaml` and the required bump. - [x] Tested on macOS (BSD `ls` + mikefarah `yq`). ```release-note NONE ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Added a CI validation step to unit tests to ensure `migrations.targetVersion` is kept in sync with newly added database migrations, preventing upgrade scripts from skipping new migration steps. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
a4bbc74b61 |
feat(agents): add md-no-hardwrap hook enforcing the prose rule
An ~80-column wrap is correct in Go comments and commit bodies, where coding agents spend most of their time, and the reflex leaks from there into markdown files and GitHub bodies. A documented rule alone does not survive that reflex, so check it mechanically at the moment of writing. The hook refuses a Write/Edit/MultiEdit that would add a hardwrapped paragraph, list item or blockquote to a markdown file, and a gh command that would publish one to GitHub via --body, --body-file, a heredoc, or an api body field. Judging what an edit ADDS is the whole difficulty. Comparing the text of each wrapped block refuses a typo fix inside a paragraph that was already wrapped; counting the blocks lets a file be rewritten as entirely fresh hardwrapped prose so long as the total does not rise. So the hook asks about the break rather than the block: a soft break is the edit's doing only when both lines it joins are lines the edit wrote. Whatever it cannot read with certainty, it allows -- a body the shell assembles at runtime, an unsplittable command line, a path that is not a regular file, a --body-file the same command has yet to write. A guard that blocks legitimate work gets switched off, and then it guards nothing. Vendored charts, published changelogs and Go templates are out of scope entirely, so hack/upload-releasenotes.sh keeps publishing its historical release notes. Line breaks are only rejected inside prose. Code blocks, tables, HTML, front matter, link reference definitions, footnotes, badge stacks and explicit hard breaks -- two trailing spaces or a trailing backslash, which render as <br> -- keep their breaks, because reflowing those would change the rendered output. A heredoc is attributed to the command that opened it, so a YAML manifest piped to kubectl is not read as a PR body. The hook and its registration are tracked by name so contributors get them on clone, while personal agent state stays ignored; personal overrides belong in .claude/settings.local.json. hack/md-no-hardwrap.bats covers both surfaces with 60 cases and make unit-tests picks it up automatically. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |