mirror of
https://github.com/cozystack/cozystack.git
synced 2026-11-08 21:14:56 +09:00
b099f8dde4eccfd892233731244bcaf68bd2ea4c
1161
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Andrey Kolkov
|
00943392cc |
fix(kafka): resolve a single Kafka image for backup e2e harness
STRIMZI_KAFKA_IMAGES is a newline-separated version=image map with one entry per supported Kafka version. The old 'sed | tr -d [:space:]' collapsed every line into one string, producing an invalid multi-image reference (quay.io/...3.8.0quay.io/...3.8.1...) that failed 'kubectl run' with 'invalid reference format' and broke kafka-2-backup-roundtrip. Split on whitespace, keep image lines, strip the version prefix and take the last (newest) image so a single valid reference is exported. Signed-off-by: Andrey Kolkov <andrey.kolkov@aenix.io> Assisted-by: LLM |
||
|
Andrey Kolkov
|
7842b88676 |
feat(kafka): add topic-data backup/restore example and e2e roundtrip
Add examples/backups/kafka, a worked backup/restore of a Cozystack-managed Kafka application's topic data via the generic Job backup strategy - the same pattern as examples/backups/nats, with no purpose-built backup image: a stock Strimzi Kafka image (kafka-*.sh CLI + bash/curl/tar) runs one shell script that branches on .Mode. Consistency is a frozen-end-offset cut: kafka-get-offsets --time -1 is read once per partition at backup start and the consumer drains only up to that offset, so anything produced during the run is excluded and every partition is captured as of the same instant. Restore recreates each topic with its original partition count and replays every partition file; keyed records reproduce into their original partition via the default partitioner. This is deliberately a DATA-only backup: topic configs, ACLs, SCRAM users and consumer-group offsets are not captured, and re-produced records get fresh offsets. The README spells out the scope and points at a volume-snapshot strategy for faithful, offset-preserving backups. Add kafka-2-backup-roundtrip to hack/e2e-chainsaw/kafka, driving the example run-all.sh as the harness (Bucket -> source Kafka + seeded topic -> BackupJob -> in-place restore -> to-copy restore + verify) so the test and the documented flow cannot drift, mirroring mariadb-2-backup-roundtrip. The image is resolved from the running operator so the backup Pod reuses the cached Kafka image. Verified end to end on a live cluster: freeze cut orders:0 [0,8), orders:1 [0,11), orders:2 [0,11) = 30 records; in-place and to-copy restores both return 30 with the original 8/11/11 partition distribution. Signed-off-by: Andrey Kolkov <andrey.kolkov@aenix.io> Assisted-by: LLM |
||
|
Aleksei Sviridkin
|
b1519acfe7 |
fix(e2e): cap the bucket S3 reads so the verify step honors its timeout (#4281)
## What this PR does Refs #4268 — a plain reference, not `Fixes`: the issue asks for a `script` op to be cut off at its declared ceiling, and that is Chainsaw behaviour this repo cannot change. Chainsaw runs the op's shell under Go's `exec.CommandContext`, whose default cancel SIGKILLs the shell PID alone and then blocks on the inherited stdout/stderr pipe until an orphaned `mc` closes it, so the op returns only when `mc` exits on its own rather than at the ceiling. This PR mitigates that where it bit — per-command caps so no descendant outlives its own budget — but the op-timeout gap itself lives in Chainsaw, so the link stays a reference rather than a `Fixes`. The sibling step that produced the 606s, `verify-every-bucket-takes-a-write` in the `bucket-1-capacity` Test, is on `main` and already carries its own per-call caps; this PR applies the same shape to `verify-s3-access`. On run 34948279363 the bucket suite's `verify-every-bucket-takes-a-write` step declared `timeout: 5m` but ran 606s and ended on `mc`'s own non-zero status rather than being cut at the ceiling. `verify-s3-access` in `hack/e2e-chainsaw/bucket/chainsaw-test.yaml` is the same-shaped step in this suite. This PR wraps every S3 (`mc`) and apiserver (`kubectl`) read in `verify-s3-access` in its own `timeout`, so a hung read dies at a known budget instead of holding the step open, and corrects the two places (`hack/e2e-chainsaw/.chainsaw.yaml` and `docs/agents/e2e-testing.md`) that explained the op ceiling with a mechanism that does not hold. It caps `verify-s3-access` for sharing the shape, not for producing the 606s number. ### Which of the issue's two candidate mechanisms holds The issue named two candidates and stated the log could not separate them. The Chainsaw v0.2.15 source separates them, and it is the second one, with the exact refinement the issue proposed. **Candidate 1 — "an operation `timeout` does not bound a `script` op" — is refuted.** The op timeout is applied and does reach the process: `pkg/runner/operations/script.go:24` copies the step's `timeout` into `Timeouts.Exec`, and `:38` wraps the op context with `ctx, cancel := context.WithTimeout(ctx, tc.Timeouts().Exec)` before calling `op.Exec(ctx, …)`. So a 5m step produces a real 300s deadline on the context the command runs under. **Candidate 2 — "it bounds it and kills the shell, but Chainsaw waits on a pipe a grandchild (`mc`) still holds open" — is confirmed.** In `pkg/engine/operations/script/operation.go` the command is built with `exec.CommandContext(ctx, shell, …)` (`:105`), and in `execute` its stdout/stderr are set to `*bytes.Buffer` (`:128`–`:129`, the buffers are `internal.CommandOutput` in `pkg/engine/operations/internal/command_output.go:12`–`13`) and run via `cmd.Run()` (`:130`). Two Go `os/exec` properties then produce the overrun: `exec.CommandContext`'s default cancel is `Process.Kill()` on the shell PID alone — not a process-group kill, and the operation sets no `SysProcAttr.Setpgid` — and because stdout/stderr are not `*os.File`, `os/exec` allocates OS pipes and copier goroutines, so `cmd.Wait()` blocks until those pipes reach EOF. EOF needs every inherited write-end closed. When the shell forks `mc`, `mc` inherits those fds; at 300s the deadline SIGKILLs the shell but `mc` is orphaned, keeps retrying the S3 PUT, and holds the pipe, so the op does not return until `mc` exits on its own at 606s. `cmd.Run()` then returns `signal: killed`, which is stored in the `$error` binding and never printed — only the `($error == null): true` check failure is logged — which is why grepping the run for `signal: killed` found nothing. **Empirical confirmation.** A standalone Go program reproduces it exactly. `exec.CommandContext` with a 1s timeout, `*bytes.Buffer` stdout/stderr, running `sh -c 'sleep 2 & sleep 4'` (a descendant that inherits the pipe): `cmd.Run()` returns after ~4s, not 1s, with `err=signal: killed` and `ctx.Err()=context deadline exceeded` — the ceiling fired at 1s, control came back only when the descendant closed the pipe. Redirecting the descendants' stdout/stderr away from the pipe (`… >/dev/null 2>&1`) returns at 1.00s. That is the whole mechanism: the ceiling cuts the shell but not a descendant that still holds the pipe. ### The fix Per-command caps, the remedy the issue itself points to ("every read carries its own cap") and the shape already used elsewhere in the suite. The bucket step now defines a `k()` wrapper (`timeout 30 kubectl`) and an `mc()` wrapper (`timeout 15 mc`) for the reads, and calls `timeout 45 mc cp` directly for the two PUTs; a hung read is killed at its own budget, releasing the inherited pipe promptly, so even when the op ceiling fires the residual is one inner cap rather than `mc`'s full retry exhaustion. The terminal precedents for these budgets are the redis suite's terminal `k()` (30s, `redis/chainsaw-test.yaml:298`) and the sibling `verify-every-bucket-takes-a-write` step's terminal `mc cp` (45s, `:390`, sized for a volume grow); the 15s cap on `mc` in `hack/e2e-chainsaw/_lib/backup-access-preflight.sh` is not a precedent, because it sits inside a `while :;` retry loop where a 124 costs one attempt, whereas here under `set -e` a 124 ends the step. The inner budgets — 2×30 for the two `k` reads + 2×45 for the two `mc cp` PUTs + at most 7×15 for the other `mc` calls (eight other call sites, but the two cleanup `mc rm` are mutually exclusive) + the 30s port-forward readiness wait = at most 285s — sit under the 5m op ceiling, which stays as a backstop, per the "op above the sum of inner budgets" rule in `docs/agents/e2e-testing.md`. The long-lived `kubectl port-forward` keeps no cap by design (it must persist for the whole step, and its stdout/stderr are already redirected to `/dev/null`, so it cannot hold the step's pipe). The `mc()` wrapper is non-recursive because `timeout` execs its argument through a PATH lookup that bypasses shell functions — verified in `dash`, `bash` and `sh`. The `catch` block's diagnostic read gets the same treatment — `timeout 30 kubectl` under a 1m op `timeout` — for the same pipe reason. ### The readonly deny probe fails on a timeout The readonly-upload probe used to branch on `mc` directly (`if mc cp …; then`), which suspends `set -e`, so a `timeout` there (exit 124) was indistinguishable from the access-denied non-zero the probe expects: a 45s hang on the deny path would exit 124, take the false branch, reach cleanup and exit 0 — a long hang turned green, the exact window this PR exists to bound. This is the first `ro-user` write in the step, so no earlier call proved that path healthy. The probe now captures the status: exit 0 is the readonly regression, 124 is a timed-out probe, both fail; any other non-zero is the expected denial. ### No unit surface; why the evidence is the proof This is an e2e-config and doc-correctness change. The embedded Chainsaw script is not extracted into a unit harness anywhere in the tree, so there is no red-to-green unit test to add without inventing a bespoke script-linter (which would be a new checker standing in for the fix). The regression is instead pinned by the mechanism proof above: the source citations show the ceiling cannot cut a descendant that holds the pipe, and the caps remove the only way a descendant outlives its budget. The deny-probe status handling and the pipe-vs-file secret reads were exercised against stub exit codes (0 → regression-fail, 124 → timeout-fail, other non-zero → expected denial) and the whole script passes `sh -n` and `bash -n`. ### Downstream repositories Walked the trigger map in `docs/agents/contributing.md` against the diff. The change touches only `hack/e2e-chainsaw/` suite content and `docs/agents/e2e-testing.md`; it moves or renames nothing under `hack/`, and touches no node-prerequisite or `package.mk` path. No downstream repository is reached. - [x] No downstream repository is affected by this change ### Release note ```release-note NONE ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved end-to-end S3 access verification so stalled Kubernetes API and S3 operations fail promptly. - Correctly distinguishes timed-out access checks from expected permission denials. - Preserves partial command output and allows subsequent test steps to continue after individual timeouts. - Applies separate time limits to bucket reads, uploads, and access checks for more predictable test behavior. - **Documentation** - Clarified operation timeout behavior and recommended per-command limits within the overall operation limit. - Documented timeout handling for orphaned processes and updated worst-case timing guidance. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
ec706c49b1 |
docs(e2e): correct the Chainsaw Test-ordering claim in etcd and clickhouse suites (#4277)
Fixes #4269 ## What `hack/e2e-chainsaw/etcd/chainsaw-test.yaml` explained its layout with a comment claiming Chainsaw executes Tests in **name order, not file order**, and used that claim to justify why teardown lives inside each scenario. The clickhouse suite had picked up the same claim. This removes the ordering claim from both — it is false at the pinned version — and re-grounds the etcd teardown justification in a reason that does not depend on order. ## Why the claim was wrong (evidence, Chainsaw v0.2.15 — the pinned version) At v0.2.15 Chainsaw guarantees **no order between Tests at all**: not file, not document, not name. Restating "name order" as "document order" is wrong for the same reason. Verified against the pinned source: - `pkg/runner/runner.go` — the runner registers each Test through `testing.T.Run` and the per-Test body calls `t.Parallel()` whenever `spec.concurrent` is nil or true. Neither suite sets `concurrent`, so both take that path. Go pauses those subtests and releases them together once the parent returns; they then race for the `-test.parallel` semaphore. - `pkg/runner/flags/flags.go` — `execution.parallel` maps only to `-test.parallel`, a cap on how many Tests run at a time, not an order. - `hack/e2e-chainsaw/.chainsaw.yaml` sets `execution.parallel: 1`, so Tests are serialised — one at a time — but not ordered. The E2E run on this PR's own head demonstrates it: discovery loaded the five selected Tests in document order, then ran `clickhouse-2` before `clickhouse-1` with both etcd Tests interleaved between the clickhouse pair. Serial, non-overlapping, and in an order that is neither file, document, nor name. ## The fix The teardown decision the false claim justified is still correct, for a reason that does not touch order. Each etcd scenario shares the singleton `etcd` release and its cluster-scoped Kamaji `DataStore`, runs with `skipDelete: true` (opting out of Chainsaw auto-cleanup), and v0.2.15 has **no test-level `finally`** — so a scenario is reclaimed only if it reclaims itself, and that is true of whichever Test runs last regardless of order. The etcd comment now says exactly this, with the ordering clause dropped rather than restated. The clickhouse header's ordering sentence is removed outright: the three Tests use distinct releases and share no state, so nothing there needed an ordering claim. This is a comment-correctness change with no runtime surface, so there is no unit test to add — the load-bearing claim was verified empirically against the pinned Chainsaw source, as above. A repo-wide grep confirms no other ordering claim survives in `hack/e2e-chainsaw/` or `docs/agents/e2e-testing.md`. Per `docs/agents/e2e-testing.md`: comment-only, no new retries, no traps, no assertion or teardown-timing changes. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Corrected end-to-end test comments to drop an incorrect test-ordering claim; Chainsaw v0.2.15 guarantees no order between Tests (not file, not document, not name). * Clarified that teardown behavior is independent of test execution order. * Documented the relationship between ClickHouse backup tests and the corresponding backup test suite. * No test behavior or user-facing functionality changed. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
89da4a121c |
fix(cdi): stop make update from deleting first-party templates (#4097)
## What this PR does `make update` in `packages/system/kubevirt-cdi` began with `rm -rf templates` and then refetched one file, `cdi-cr.yaml`. Nothing in that directory is purely vendored. Two of the three manifests, the upload-proxy Ingress and TLSRoute from the Gateway API work, are ours and no fetch recreates them, so the next CDI bump would have dropped upload-proxy exposure from the chart and still exited 0. The third is `cdi-cr.yaml` itself, which despite its name carries Helm templating, the `ExpandDisks` gate, a toleration and the two RBAC documents that preauthorize cloning from `cozy-public`; the fetch overwrites it, and removing the wipe does not change that. That recipe now opens with `mkdir -p templates` and deletes nothing, which is where `packages/system/kubevirt-operator` landed at the last KubeVirt bump. The wipe bought nothing there anyway: `wget -O` truncates the file it writes. `kubevirt-cdi-operator` has the same recipe shape, and the same edit there would be a regression rather than a fix. It applies two patches, and since #3920 moved them to `-p4` they resolve, so a hunk upstream has moved out from under leaves `templates/cdi-operator.yaml.rej`; its `awk -i inplace` step leaves `templates/cdi-operator.yaml.gawk.XXXXXX` if gawk dies abnormally. Helm renders every file under `templates/`, and I reproduced both: the reject fails the package with a YAML parse error, and the gawk temp renders a duplicate object at exit 0. The wipe is what takes both, so removing it for symmetry would trade a latent loss for a live one, and the quieter of the two is the one that survives. Deleting the wipe there wants the recipe restructured to transform outside `templates/` and move the result in once, which is what `kubevirt-operator` does today and what its own comment gives this exact reason for. That is a larger change than this one and belongs on its own; #4383 makes it. Meanwhile the scan below covers the package: I added a first-party file to its `templates/` and the guard reported it, so the day that package grows one, this fails rather than the file being lost. `hack/package-update-templates.bats` guards this across the tree rather than in these two packages. It scans every package Makefile and fails when a recipe that clears its own `templates/` leaves behind a tracked file it never names. Put the removal back into `kubevirt-cdi` and it goes red naming both upload-proxy files. A file counts as named only outside the Makefile's own `NAME` and `NAMESPACE`, however the assignment is written. The match is on the variable name rather than on a list of spellings: the tree already carries three of them, `export NAME=`, the bare `NAME=`, and `NAME := ` in `packages/core/testing`, and the hole is identical in each. Without that, a package called `foo-bar` satisfies the check for its own `templates/bar.yaml` through its identity alone. Nothing the scan reaches is exempted that way as the tree stands, so the exclusion is there for the first package that would be: rename the asset in `kubevirt-cdi-operator`'s recipe and put the removal back, and the guard reports the orphaned template only with the exclusion in place. The two variables are matched by name rather than by a `[A-Z_]+=` shape, because a shape that broad is one unspaced assignment away from swallowing a variable that does name templates, which is how `etcd-operator-crds` names all five of its CRDs. A control test runs the same helpers over a fixture broken the same way, over one whose name swallows a template stem, then over that fixture rewritten to spell the removal with a trailing slash, to take one file instead of the directory, and to drop the removal entirely. Both cases pass under `hack/cozytest.sh hack/package-update-templates.bats`. Two limits, both written into the file's header. The check fires on the consequence rather than the shape, so `rm -rf templates` in a package that does refetch everything stays green, which is what keeps `kubevirt-cdi-operator` green for as long as its recipe wipes. And it only looks at a recipe clearing its own directory: `cert-manager` clears `../cert-manager-crds/templates/*` across the package boundary, which needs the removal's target resolved rather than the wiping package's own, so that directory is unguarded. The other packages that clear their own `templates/` refetch all of it and are untouched here. Two more limits the header names. The scan reads the text of each package's own `update:` recipe, so a removal made elsewhere is out of range: in a prerequisite target, behind an include, or behind a recursive make. `capi-operator`, `clustersecret-operator`, `openbao` and `reloader` write `update: clean <name>-update` and take both recipes from `hack/package.mk`, which removes nothing under `templates/` today. And the glob stops at `packages/*/*/Makefile`. The package Makefiles off that depth are the group aggregators such as `packages/system/Makefile` and the vendored kamaji subcharts, and none of them declares `update`. One behaviour of the matcher worth naming, and deliberately not changed here. `wipe_command` evaluates `/^update:/` ahead of its terminator, so a second `update:` in the same Makefile re-arms the scan instead of ending it, and what that means depends on what sits between the two: adjacent or separated by a blank line, both recipes are read; separated by another target, only the first is. The direction that matters is a false positive rather than a miss. A Makefile whose first `update:` clears `templates/` and whose second does not would be reported by this scan, while make runs only the second, so the package fails over a recipe that never executes. No package in the tree defines `update:` twice today. Three more notes, all non-blocking and none acted on. The header says what the scan leaves uncovered is a recipe clearing someone else's `templates/`; that is the uncovered set for *deletion*, and `kubevirt-cdi` is green under the scan while still losing `cdi-cr.yaml`'s content to the in-place `wget -O` on every run, which the Makefile comment records but the header does not. The identity strip has a false-positive direction the header does not argue: a wiping package writing `templates/$(NAME).yaml` would be reported even though the recipe writes it, which no package does today. And the fixture stages `.gitkeep` with `git add --all`, so a developer whose global excludes list that name would get a red for environment rather than defect; `--force` would remove the dependency. The stem match is generous, and the helper's comment now names the two live shapes. A sibling name: `templates/crds.yaml` next to the existing `templates/crds-experimental.yaml` on `gateway-api-crds`. A mention from an unrelated context: `templates/sidecar.yaml` on `objectstorage-controller`, whose Makefile says `sidecar` across its image targets while its recipe clears `templates/` and writes only `templates/controller.yaml`. I fed both to the helper and both come back exempted; a stem the Makefile never mentions is still reported. The duplicate `test:` target in this Makefile is #4157 and stays there. It sits outside the hunks this diff touches, so removing it here would be reaching past what this change is about. One more note, not a defect. Where the comment points at `kubevirt-operator` as the shape that closes the transient-file class, `packages/system/kubevirt/Makefile` is the closer analogue for the `cdi-cr.yaml` half specifically: its `update` downloads nothing and re-applies the parameterization onto a hand-maintained CR with a per-marker self-check. And the failure message offers a third option, staging into a tmpdir the way `etcd-operator-crds` does, in a parenthetical that reads as a gloss on the option before it. What this PR does not fix, and what does. `make update` here still overwrites `templates/cdi-cr.yaml`, which the Makefile comment says plainly. A wholesale overwrite is loud: upstream's object carries neither `podResourceRequirements` nor `cloneStrategyOverride`, and `tests/importer-resources_test.yaml` and `tests/clone_strategy_test.yaml` pin both, so `helm unittest` goes red. The silent case is a partial hand reconciliation that keeps those two and drops something else, because nothing here pins the `ExpandDisks` gate, the `CriticalAddonsOnly` toleration, the `uploadProxyURL` override or the two RBAC documents that preauthorize tenant cloning from `cozy-public`. #4107, stacked on this branch, stops `update` writing the file at all and adds `tests/local_templates_test.yaml`, which pins those. One limit the header does not name, left alone here. The comment strip is anchored to the start of a line, so a trailing comment on a recipe line survives into the searched text and counts as naming the file it mentions. I confirmed it by feeding `wget ... -O templates/cdi-cr.yaml # local-extra.yaml is written by hand` through the filter. Nothing in the tree is written that way, and the header only claims comment lines are dropped, which is what it does. `sed 's/#.*//'` in place of the `grep -v` closes it if it ever matters. Closes #4028. ### Screenshots No UI change. ### Downstream repositories Walked the trigger map in `docs/agents/contributing.md` against the diff. Two entries come close: the website's "developer tooling (the package `Makefile`s)" line, and ccp's "move or rename anything under `hack/`". Neither fires. `make update` still behaves as the website's development page describes, nothing under `hack/` moved or was renamed, and the two files ccp's skills key on (`hack/package.mk`, `hack/common-envs.mk`) are untouched. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note ```release-note fix(cdi): `make update` in kubevirt-cdi no longer deletes its first-party templates ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Updated Kubernetes CDI package updates to preserve existing template files while refreshing downloaded manifests. - Prevented update operations from clearing first-party and vendored template content. - **Tests** - Added safeguards to detect package update recipes that could remove tracked templates without restoring them. - Added Helm unit test support for the Kubernetes CDI package. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
8d37e0b950 |
docs(hack): drop a false universal from the nats-rd secrets check (#4390)
## What this PR does The header of the nats-rd secrets check explained its assertions with a claim about every `-rd` package: none has a tests directory or the `test` target that the helm-unittest runner looks for. `postgres-rd` has both now, so the claim told the next reader that the helm-unittest route is closed for this class of package when it is open. The claim is removed. The assertions rest only on `nats-rd` itself having neither, which is still true, so they stay as they are. Moving them to helm-unittest is a separate question and is not part of this change. Comment-only change. Closes #4209 ### Screenshots No UI change. ### Downstream repositories - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note ```release-note docs(hack): remove a false claim about -rd package tests from the nats-rd secrets check comment. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Updated a test-file comment to clarify that the inlined ResourceDefinition chart lacks a `tests` directory and the test target used by the Helm unit test script. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
1071802e3f |
test(e2e): pin the release-history read against the upstream type (#3607)
## What this PR does
`hack/remediation-guard.bats` carried a test called "status.history
extraction pins HR v2 status.history shape". It wrote a HelmRelease
document containing the field names it expected, then read that same
document back with the same jsonpath through `yq`. What it established
is that `yq` can read a fixture the test itself composed.
Its own comment named the failure it was there to catch: a future Flux
release renames the field, the jsonpath matches nothing, the guard
reports no remediation cycle, and real cycles pass the e2e assertion. A
rename moves neither the fixture nor the expression, so the test stays
green through exactly that event. The comment also named where the truth
lives, `github.com/fluxcd/helm-controller/api/v2`, and then pinned a
copy of the author's expectations instead.
`internal/fluxcontract` supplies neither half. The expression is read
out of `hack/e2e-chainsaw/_lib/remediation-guard.sh` at test time, so it
is the one that ships rather than a copy. The object is a
`helmv2.HelmRelease` built from the upstream type at the version
`go.mod` holds, serialized and matched through
`k8s.io/client-go/util/jsonpath`, which is the engine kubectl's `-o
jsonpath` uses. A renamed or retyped field is then a compile error or a
failed match, and a guard that stops reading history fails the lookup
with a message saying so.
That is one test. Two more exist so that it reads the expression kubectl
actually receives, because the expression now lives in a named
assignment the test sources and the caller expands: between the value
checked here and the value that runs there is a shell variable, and each
step of that indirection can fail while the suite stays green. The first
follows the caller's own call: it reads the local handed to
`helmrelease_has_remediation_cycle`, requires that local to be assigned
exactly once, and requires that assignment to take the expansion of the
shared name rather than the bare name, which matters because kubectl
echoes a brace-less template back verbatim, so a read that lost its `$`
returns its own text, clears the script's empty-history check, and
matches no status. Starting from the call rather than from the text is
what makes the pin describe the read that runs: asking only whether a
well formed read exists somewhere in the file is satisfied by a second
assignment overwriting the same local, and by a read filling a local the
guard never consumes. The second holds that no shell library but the one
that owns the name assigns it, which does not follow from the first: an
assignment beside the read satisfies the first and still replaces the
value after this file has approved the one the library exported. It
covers every library in that directory rather than the caller alone,
because the caller sources three of them and the owning one first, so an
assignment in any of the others lands later and wins.
Those two match text with a pattern, and each carries both halves of its
contract. A refuse table lists the forms the pattern must reject, and an
accept table lists the forms it must keep accepting, one case per
freedom the code claims: for the call, the `if` prefix, the negation, an
expansion with or without braces; for the read, the spelling of `-o`,
the quoting of the substitution, an expansion with or without braces,
and the local's name, which stays free because the call and the read are
renamed together. Without the accept side those freedoms survive only
until someone tightens the pattern, and nothing reddens when they go.
Two boundaries, named rather than left to be found. The pin holds the
shape of the read, not the vocabulary of the answer: the guard greps for
`failed` and `uninstalled`, and the test supplies those two strings
itself, so a release status renamed upstream leaves the jsonpath
returning strings, the test green because it wrote them, and the shell
grep silently matching nothing. Those values come from Helm's release
status rather than the Flux type, so pinning them reaches past what this
change binds, and there is more than one way to do it. Second, a literal
history read that fills a local the guard never consumes is not caught,
and on its own that is harmless: the guard still runs the shared
expression, and the moment it stops doing so the read filling the
consumed local no longer matches and the caller test fails.
The caller-side half is a heuristic and is meant to be read as one. It
decides whether a piece of text is the read by matching it, and a
matcher that reasons about shell is wrong at some spelling by
construction, which is the same objection the file raises against
reading the library's assignment as text instead of sourcing it.
Sourcing is available for the library and not for the caller, so what is
left is to make the imprecision run one way: the count is of
occurrences, so an extra one reddens and a missed one cannot pass, and a
form outside the accepted set fails with the pattern printed rather than
quietly stopping asserting.
That direction is the whole argument for keeping it. A false red is a
broken build on a legitimate rewrite, and the failure names what it did
not recognise, so it costs a minute. A silent green is a guard that
asserts nothing for as long as nobody looks, which is the state this
branch exists to end. The ways the caller can break without anyone
noticing are worth that trade. A read that lost its `$` hands kubectl a
template with no braces, which comes back as the variable's own name; a
hand-written path that is valid but names the wrong field comes back as
that field's values. Both are non-empty, so the script's own
empty-history backstop stays quiet and the cycle grep matches nothing. A
deleted call is quieter still, since there is then no assertion to fail.
None of the three is visible to the library-side pin, which judges the
value the library hands out and never learns whether it reached kubectl.
What does fail loudly is an expression kubectl cannot use at all, and
that is the one case needing no pin.
The shape the fixed test now has already exists in this tree, in
`hack/seaweedfs-naming-audit.bats`, under "the audit's reconstruction
agrees with the chart helper it mirrors". That one also writes a
heredoc, which is what makes the pair worth comparing: it copies the
real `_naming.tpl` out of the chart, renders it through `helm`, and
checks the audit script's output against the render. The heredoc is a
harness for calling the real helper, not a record of what the author
expected it to say, and the two sides have different authors. A test
that writes both sides proves only that it is self-consistent.
Nothing is lost by the deletion. The removed test's tail fed extracted
statuses into `helmrelease_has_remediation_cycle`, and the seven tests
remaining in the bats file `hack/remediation-guard.bats` still cover
that, including `uninstalled then deployed still returns detected`. The
extraction half is now pinned more strictly than before: the old test
asserted only that the output was non-empty and contained one line, the
new one asserts the exact two bare tokens, so dropping the `{"\n"}`
separator also goes red.
CI runs it. `make test-controllers` is its own step in
`.github/workflows/pull-requests.yaml` and runs `go test ./internal/...
-count=1`, which picks the new package up without registration.
### Screenshots
Not a UI change.
### Downstream repositories
Walked the trigger map against the diff. It adds a Go test package under
`internal/`, deletes one test from `hack/remediation-guard.bats`, adds a
variable to `hack/e2e-chainsaw/_lib/remediation-guard.sh`, points the
kubectl invocation in `hack/e2e-chainsaw/_lib/run-kubernetes.sh` at that
variable, adds a sentence to `docs/agents/e2e-testing.md`, and records
`make test-controllers` in `docs/agents/overview.md`. No package is
added, renamed or removed, and nothing touches a `values.schema.json`, a
version enum, a `values.yaml` default, an `ApplicationDefinition`, a
CRD, a namespace, an annotation or a telemetry metric.
`hack/e2e-prepare-cluster.bats` is untouched, so the node contract that
`talm` and `ansible-cozystack` restate by hand does not move.
The closest call is `ccp`, whose skills anchor on files under `hack/`.
Its trigger is a move, a rename, or a make target that does something
different. This changes the contents of three files, not their names,
and no target behaves differently; `hack/package.mk` and
`hack/common-envs.mk` are untouched.
- [x] No downstream repository is affected by this change
- [ ] [cozystack/website](https://github.com/cozystack/website) -
follow-up:
- [ ]
[cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack)
- follow-up:
- [ ]
[cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack)
- follow-up:
- [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up:
- [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up:
- [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) -
follow-up:
- [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) -
follow-up:
- [ ]
[cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server)
- follow-up:
- [ ]
[cozystack/external-apps-example](https://github.com/cozystack/external-apps-example)
- follow-up:
- [ ] [cozystack/examples](https://github.com/cozystack/examples) -
follow-up:
### Release note
```release-note
NONE
```
|
||
|
Aleksei Sviridkin
|
90a1a947c5 |
feat(kube-ovn): bump kube-ovn to v1.15.26 (#4331)
## What this PR does Bumps kube-ovn from `v1.15.10` to `v1.15.26` (package `cozy-kubeovn` `0.38.0` -> `0.39.0`). The motivation is a cold-start failure mode in the OVN NB/SB raft leader checker. In `v1.15.10`, `checkDBClusterIntegrity` (`pkg/ovn_leader_checker/ovn.go:336`) called `os.Remove` on a healthy local raft DB (`ovn.go:367`) whenever a leader observed fewer members than expected. That count drops below the target transiently during cluster growth, a rolling restart, or recovery after a node address change, so the check could delete a valid DB and leave `ovn-central` unable to elect a leader. `v1.15.26` removes that function entirely, so the DB is no longer deleted. It is a removal of the faulty check, not a grace-period around it. The chart is re-vendored with `make update`, not hand-edited, so the templates match the image: - `charts/kube-ovn` is re-pulled from the `v1.15.26` tag and `patches/mtu.diff` is re-applied; the image build args are regenerated. - This brings the whole v1.15.26 chart: the reworked master-node affinity helpers, the widened kube-ovn CRD schema (`bandwidth.ingress`/`egress` from `integer` to int-or-string, `u2oFeatures` added), and the new `OVN_CENTRAL_MODE` (`cluster`/`single`) support with its `central-svc`, `central-pvc` and `central-hcp-rbac` templates. `OVN_CENTRAL_MODE` defaults to `cluster` (raft on hostPath), so an in-place upgrade keeps the existing topology. - Two vendored patches now included upstream in `v1.15.26` are dropped: `fix-keepvmip-portgroup.diff` (`hasAliveSiblingVMPod`, `pod.go:2437`) and `fix-vmim-scheduling-retry.diff` (scheduling requeue, `kubevirt.go:195`). `fix-vmim-target-pod-retry.diff` is kept: `v1.15.26` still returns nil at `kubevirt.go:215`. The image also picks up `kube-ovn-bfdd-supervisor`, which v1.15.26 adds (`makefiles/build.mk:33`) and upstream's `dist/images/Dockerfile:12` copies into `/kube-ovn`. `pkg/controller/vpc_egress_gateway.go:47` hardcodes that path for the `bfdd` container a `VpcEgressGateway` with `spec.bfd` gets, so a copy that stops at `kube-ovn-controller` ships a container that cannot exec. `hack/check-kubeovn-image-copies.sh` compares what our Dockerfile and upstream's copy into the image, `make update` runs it against the tag it pulls and `hack/check-kubeovn-image-copies.bats` runs it on every CI run, because `make update` otherwise rewrites only the ARG lines and the copy drifts at each bump. The `multus` commit moves the `kube-ovn-cni-plugins-checked-against` marker to `v1.15.26`. `CNI_PLUGINS_VERSION` is `v1.9.1` in kube-ovn's base image at both releases and in multus, so portmap and macvlan stay in lockstep; the marker is the only change. The image ref stays digest-pinned (`repository` + `tag@sha256`), which the promote-retag release contract requires for a cozystack-built image. The digest is carried over from `v1.15.10` as a placeholder and is refreshed to the real `v1.15.26` image by the release-candidate digest-vendoring step; no PR or main job rebuilds it. Verification: `helm unittest .` passes (22 tests), `helm template .` renders clean, the `cni-plugins-staging-contract` and `promote-retag` unit contracts are green, and the image-build `TestVMIMigrationSchedulingRetry` gate passes 6/6 on the patched v1.15.26 tree. > [!NOTE] > `start-db.sh` was substantially reworked upstream between `v1.15.10` and `v1.15.26`. The privileged fork E2E run installs into a sandbox and passes, but the in-tree E2E job is skipped on a fork PR, and a fresh install is not an upgrade. An in-place upgrade on a multi-node control plane (image rebuild plus OVN raft re-convergence) is still untested before merge. ### Screenshots N/A: no UI changes. ### Downstream repositories Walked the trigger map against the diff. The change is confined to `packages/system/kubeovn` (re-vendored chart, image build args, version pins) and a one-line marker in `packages/system/multus`. It does not add, rename or remove a package, does not touch platform values, variants, components, release assets, `ApplicationDefinition` semantics, the Talos version, or any hand-typed CRD schema in a downstream provider. No downstream trigger matches. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note ```release-note feat(kube-ovn): bump kube-ovn to v1.15.26, which removes the OVN leader-checker integrity check that could delete a healthy raft DB and prevent ovn-central from electing a leader after a node address change or rolling restart ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added hosted control-plane support for OVN Central with StatefulSet deployment, persistent storage, service exposure, and dedicated RBAC. - Added split-cluster and data-plane-only configuration with external OVN endpoints. - Added configurable TLS, leader election, service, and storage settings. - Enhanced CRDs with overlay-only routing and flexible bandwidth quantities. - **Bug Fixes** - Improved VM migration retries during scheduling and preserved connectivity during live migration. - Improved upgrade handling and service configuration validation. - **Updates** - Upgraded Kube-OVN components and images to version 1.15.26. - Documented required OVN IPsec directory preparation for read-only filesystems. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
0f21d06520 |
fix(dashboard): raise the console nginx header buffers for large OIDC tokens (#4376)
## What this PR does A user whose OIDC access token carries many group claims cannot use the console: nginx answers `400 request header or cookie too large` before the request reaches the apiserver proxy. The config baked into the console image kept the nginx defaults, 1k for `client_header_buffer_size` and four buffers of 8k for `large_client_header_buffers`, while the token arrives as one Cookie header several kilobytes long. A header field has to fit into a single buffer, so the per-buffer size is what decides this. Both directives are now 16k, at the top level of that config rather than inside the server block: a header field read before the `Host` header has selected the virtual server is measured against the default server's buffers, so the same lines inside a server block cover that server only while it stays the default one. Measured in the pinned nginx base image: a 12 KB cookie gets 400 on the old config and 200 on the new one, a 20 KB one still gets 400. `nginx -t` accepts the result. This hop sees more than the browser sent. The gatekeeper in front of it runs with `--pass-access-token=true` and `--pass-authorization-header=true`, so the request arrives carrying the `kc-access` cookie, an `Authorization: Bearer` header and `X-Forwarded-Access-Token`, three separately bounded header fields where the edge saw one. This is the console image only. The platform ingress-nginx controller also runs on the defaults, which is #2853. Worth reading that trace again before fixing it there: the controller logged `upstream_status 400`, so it forwarded the request and the 400 came back from behind the gatekeeper, which is this layer. The check in `hack/console-nginx-header-buffers.bats` reads the config out of the Containerfile heredoc and fails when either directive is missing, below 16k, or no longer ahead of the first server block. That config lives inside an image build, so no chart template and no helm-unittest case can reach it. `make unit-tests` picks the file up on its own. No image reference changes here. CI stamps the console digest. Closes #3113 ### Screenshots No UI change. This is the nginx config in front of the SPA. ### Downstream repositories The diff is the console image's nginx config plus one new `hack/*.bats` file. `cozystack-ui` is archived and the console lives in this repo, so a console change belongs in this PR. Nothing under `hack/` moved or was renamed, and no make target changed behaviour. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note ```release-note fix(dashboard): the console no longer answers `400 request header or cookie too large` to users whose OIDC access token carries many group claims ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Increased the console’s NGINX request-header buffer capacity to support larger header fields and request headers. * Improved handling of requests with headers that previously exceeded the default buffer limits. * **Tests** * Added automated checks to verify the console’s NGINX header-buffer settings remain correctly configured. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
cc602f0dca |
docs(hack): drop a false universal from the nats-rd secrets check
The justification for the two nats-rd assertions ended with a claim about every -rd package: none carries a tests directory or the test target the helm-unittest runner looks for. At least one does now, so a reader deciding where to put the next -rd assertion was told the helm-unittest route is closed when it is open. The assertions rest only on nats-rd lacking both, which still holds, so the claim goes and the assertions stay as they are. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
2f5625f0dc |
fix(cdi): stop make update from deleting first-party templates
The update recipe in kubevirt-cdi opened with `rm -rf templates` and then refetched one upstream manifest. The directory keeps two more files -- the upload-proxy Ingress and TLSRoute -- that are written by hand and never refetched, so the next CDI bump would drop upload-proxy exposure from the chart while `make update` still exited 0. The clean slate bought nothing either way: wget -O truncates the file it writes. The recipe now creates the directory with `mkdir -p` and leaves the rest of it alone, which is what kubevirt-operator moved to when it was last bumped. kubevirt-cdi-operator keeps its wipe. The same change there would be for symmetry rather than for a loss, and it is not free: both its patch calls resolve, so a hunk upstream has moved out from under leaves templates/cdi-operator.yaml.rej, and gawk's inplace edit leaves templates/cdi-operator.yaml.gawk.XXXXXX if it dies abnormally. Helm renders every file under templates/, so the first fails the package loudly and the second renders a duplicate object at exit 0. The wipe is what takes both. Removing it wants that recipe restructured to transform outside templates/ the way kubevirt-operator now does, which is its own change. Until then the scan below is what watches that package, with the limit the scan already documents: it reports a tracked template whose stem the Makefile never mentions, so a name that collides with something already written there, templates/ tolerations.yaml against the patches/tolerations.diff line, is exempted. It catches the loss it was written for, not every loss. The bats case scans every package Makefile rather than a pinned list, so a first-party template added under a recipe that clears templates/ fails the commit that adds it instead of vanishing at the next bump. A package's own NAME and NAMESPACE do not count as naming a file, because a package called foo-bar would otherwise vouch for its templates/bar.yaml with nothing writing it. A removal reaching into another package is outside the scan's range, and the header records the case in the tree that this leaves uncovered. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
97cb2781db |
test(e2e): pin the release-history read against the upstream type
The test that claimed to pin the HelmRelease v2 history shape wrote a HelmRelease document containing the field names it expected, then read that same document back with the same jsonpath. It established that yq can read a fixture the test itself composed. Its own comment described the failure it was meant to catch: a future Flux release renames the field, the jsonpath matches nothing, the guard reports no remediation cycle, and real cycles pass the e2e assertion. A rename moves neither the fixture nor the expression, so the test stays green through exactly that event. The comment also named where the truth lives, github.com/fluxcd/helm-controller/api/v2, and pinned a copy of the author's expectations instead. Replace it with a Go test that supplies neither half. The expression is read out of the shell library that ships it, and the object is built from the upstream type at the version go.mod holds, then serialized and matched the way kubectl does. A renamed or retyped field is a compile error or a failed match; a guard that stops reading history fails the lookup with a message saying so. Both directions were confirmed by mutation. Locating the expression by searching the file is not enough on its own. Inline in a long script, locating it means deciding which occurrences are code and which are prose, and every way of getting that wrong fails green: an example quoted near a broken read satisfies the pin while the guard reads nothing. So the expression moves into a named assignment in remediation-guard.sh, which the guard already sources, and the test sources that library and takes the value the shell is left holding. Deciding which text is the assignment is then not the test's problem: an `export`, an indent, a second assignment later in the file all resolve the way the guard will resolve them, because the thing resolving them is the shell. Sourcing borrows the environment along with the shell, so the snippet unsets the name before it sources. `go test` hands the child its own environment, and a shell that had exported this name would answer in place of a library that stopped assigning it: the value comes back non-empty, the empty check stays quiet, and the pin passes over a guard that reads nothing. That is this file's own subject reaching it through the environment rather than through the source. Moving the expression out of the call site puts a step between what is pinned and what runs, so the call site is checked too, and the check starts from the call rather than from the text: the local the guard is handed must be assigned exactly once, by an anchored assignment whose jsonpath is the expansion of the shared name. That a well-formed read exists somewhere in the file is a weaker claim than it reads as, since a second assignment to the same local, or a read filling another one, satisfies it while the guard runs a literal nothing here pins. The expansion and not the name: kubectl echoes a braceless template back verbatim, so a read that dropped the `$` would return its own text, clear the script's empty-history backstop, and carry no status the cycle check matches. The library is checked by sourcing rather than by reading, so an assignment the shell would run is resolved by the shell instead of by a guess about which text the shell would pick. Each rule is pinned by its own test, so a regression names which one it opened rather than only that extraction broke. How kubectl's printer treats an absent path is its own configuration rather than part of this contract, so it is left unasserted: guessing at it would put the expectation and the fixture back in the same hands. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
4475fb4c17 |
test(e2e): assert the System-mode OIDC uid claim mapping
The System lane read the rendered AuthenticationConfiguration for the issuer URL and the audience but not for the mapping that gives the user a UID. A configuration missing that mapping is still valid, so the tenant control plane starts and the suite passes while every OIDC user authenticates with an empty UID. The unit twin mocks the Secret without the mapping, so dropping the grep from the helper reddens it rather than leaving it unpinned. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
4bf95815a8 |
fix(kubernetes): render the aggregation-layer UID header for tenant OIDC (#3539)
## What this PR does Tenant clusters with `spec.oidc.mode: System` or `CustomConfig` authenticate users fine, but a UID does not survive the hop to an aggregated API server. The tenant kube-apiserver publishes `requestheader-uid-headers` into the `extension-apiserver-authentication` ConfigMap only when the flag is set, and the chart never set it. The aggregation layer still sends `X-Remote-Uid`, but the extension server trusts only the header names published in that ConfigMap, so it ignores the one it gets and the call arrives without a UID. The field documentation even told operators to add the flag by hand via `controlPlane.apiServer.extraArgs`. It forwards a UID rather than creating one. ServiceAccount tokens carry one, and for those the aggregation hop starts working. Users authenticated by `spec.oidc.mode: System` still have none, because the generated AuthenticationConfiguration maps `username` and `groups` only; giving them one means mapping a claim, which changes the identity of users who have none today, and is tracked separately. The chart now renders `--requestheader-uid-headers=X-Remote-Uid` on v1.32 and newer, and adds `--feature-gates=RemoteRequestHeaderUID=true` on v1.32, where the gate is still alpha and the apiserver rejects the flag without it. From v1.33 the gate is beta and on by default, so only the header flag is rendered. v1.31 gets nothing: the flag does not exist there, and writing it by hand stops the apiserver from starting. `--emulated-version` in the same passthrough moves that decision, because the gate resolves through the versioned spec at the emulated version. Rendering the gate on every version from v1.32 rather than on v1.32 alone was the alternative, and it would remove the migration step entirely, because an opt-out would then drop two entries instead of one and move the entry count by itself. It loses on what it leaves behind: a redundant gate term on every cluster, which stops the apiserver from starting on the day the gate is retired from the known list. The design also prefers rendering over failing for the same reason the fail branches are narrow, since a cluster stranded on a HelmRelease that no longer converges is a worse outcome than one that never gets the UID fix. The chart reads an operator entry the way the apiserver reads it: header list elements matched whole and untrimmed and folded as `EqualFold` folds them, gate names compared exactly, gate values resolved as `strconv.ParseBool` resolves them, `AllAlpha` and `AllBeta` blankets honoured, an empty value treated as no entry. On v1.32 an entry that says nothing about `RemoteRequestHeaderUID` gets a second one carrying the operator's own terms plus `RemoteRequestHeaderUID=true`. Carrying those terms into the merged copy is what makes it land correctly whether the tenant control plane collapses duplicate flags into the last one or hands both to the apiserver, which merges them in the order it reads them. Tests cover the render matrix over mode, version and operator input: every fail path with its exact message, the ownership and merge paths, duplicate entries in both orders, blanket gates, empty values, and case variants on the header name and on the gate value. The latest-version tenant lane creates its tenant with `spec.oidc.mode: System` and asserts the flag on the rendered KamajiControlPlane, so the tenant apiserver starts with the flag in CI. Nothing asserts the UID arriving at an aggregated apiserver; that needs an extension server inside the tenant and is not in this change. The layer where the count-keyed reset actually bites is the tenant Deployment's arguments, and asserting there needs a live tenant control plane, so it is not in this change either. The rule this follows is that a configuration which renders and boots today keeps rendering. Both flags are new, so an `extraArgs` shape reaching this code was rendering before it existed, and stranding a running tenant on a HelmRelease that no longer converges is a worse outcome than one that never gets the fix. A few shapes do stop rendering, and reaching any of them takes writing `--requestheader-uid-headers` into the passthrough by hand, attempting exactly what the chart now does and getting it wrong; in each the kube-apiserver fails option validation and never starts, so nothing that was working is lost. `docs/oidc-tenant.md` covers these shapes. One known limitation is in there and worth naming here: on v1.33 and newer, adding the documented opt-out is count-neutral in `extraArgs`, and so is removing it again, and on v1.33 and v1.34 adding or removing `--emulated-version=1.31`. The pinned Kamaji build rewrites a running apiserver's arguments only when that count changes and otherwise keeps every flag that has left the list, so either the header flag stays beside a now-disabled gate or the disabled gate stays beside the header flag, and the apiserver refuses to start. Changing the entry count in the same edit is the mitigation, and the fix belongs to the pin rather than to this chart, tracked in #3541. The paths that need no operator edit are clear: turning the feature on for a default tenant moves the count, and a tenant already carrying the opt-out renders byte-identically before and after. Out of scope: `mode: None` still publishes no UID header even though ServiceAccount tokens carry UIDs. That is pre-existing, and widening the condition changes the render for every default tenant. Tracked separately, along with the claim mapping above. Closes #3377 ### Screenshots Not a UI change. ### Downstream repositories - [x] No downstream repository is affected by this change Checked the trigger map against the diff: no package added or removed, no schema field, enum or default changed (description text only), no ApplicationDefinition surface touched, no hack/ layout moved. The managed-apps reference page picks up the new field description through release-time README regeneration. ### Release note ```release-note fix(kubernetes): tenant clusters with `spec.oidc.mode` set to `System` or `CustomConfig` now render `--requestheader-uid-headers=X-Remote-Uid` on the tenant kube-apiserver, plus the `RemoteRequestHeaderUID` feature gate on v1.32, so that an aggregated API server can read the caller's UID out of the request headers. Without that flag the tenant apiserver never publishes `requestheader-uid-headers`, and an aggregated API server ignores the UID header even for callers that have one, ServiceAccount tokens among them. It forwards a UID rather than creating one: users authenticated by `spec.oidc.mode: System` still have none, because the generated AuthenticationConfiguration maps `username` and `groups` only, and that is tracked separately. Adding the flag by hand through `controlPlane.apiServer.extraArgs` is no longer necessary, and an entry of your own is respected: the chart steps aside when yours already carries `X-Remote-Uid` or already enables the gate, merges `RemoteRequestHeaderUID=true` into a copy of a v1.32 `--feature-gates` entry that says nothing about the gate, and renders neither flag when you switch the gate off. The render fails where a `--requestheader-uid-headers` list of your own would stop the kube-apiserver from starting anyway: one that omits `X-Remote-Uid`, one with an empty element, and a list of your own while the gate is off. On v1.33 and newer there is a migration step, and it applies in both directions: the pinned Kamaji build rewrites a running apiserver's arguments only when the number of `extraArgs` entries changes, and otherwise keeps every flag that has left the list. Adding the documented opt-out keeps that number the same, so the header flag already on the Deployment stays beside the now-disabled gate. Removing the opt-out later keeps it the same too, so the disabled gate stays on the Deployment beside the header flag the chart renders again, although your spec no longer shows it. Adding or removing `--emulated-version=1.31` has the same shape both ways on v1.33 and v1.34. The apiserver refuses to start in each case, so change the entry count in the same edit. ``` |
||
|
Aleksei Sviridkin
|
7d53f7af22 |
fix(dashboard): raise the console nginx header buffers for large OIDC tokens
The nginx config baked into the console image kept the nginx defaults for client_header_buffer_size (1k) and large_client_header_buffers (4 8k). An OIDC access token carrying many group claims arrives as a single Cookie header of several kilobytes, which no longer fits into one buffer, so nginx answered 400 "request header or cookie too large" before the request reached the /apis, /api or /k8s/ upstream. Raise both to 16k, at the top level of that config rather than inside the server block: a header field read before the Host header has selected the virtual server is measured against the default server's buffers, so the same lines inside a server block cover that server only while it stays the default one. Measured against the pinned nginx base image, a 12 KB cookie gets 400 on the old config and 200 on the new one. The ceiling moves rather than disappears: a 20 KB cookie is still rejected. This is the console hop alone. The platform ingress-nginx controller in front of it runs on the same defaults, so where the dashboard is published through an Ingress a token large enough to exceed them is still rejected at the edge; where it is published through the gateway, whose edge has no per-header ceiling of this kind, the console was the hop returning the 400. Raising the controller is a separate change in a separate package. The accompanying check extracts the config from the Containerfile heredoc and fails if either directive is missing, below 16k, or no longer ahead of the first server block: the config lives inside an image build, where no chart template and no helm-unittest case can reach it. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
scooby87
|
7d07bbe3ce |
[kafka] Migrate from ZooKeeper to KRaft mode (#2352)
## Summary - Replace ZooKeeper-based Kafka deployments with KRaft (Kafka Raft) metadata mode - Existing ZooKeeper instances migrate automatically and in place via a Helm `pre-install,pre-upgrade` hook Job that drives Strimzi's ZK-to-KRaft migration state machine - New instances deploy directly in KRaft mode with broker and controller `KafkaNodePool`s - The node pools are owned by the migration hook, not by regular chart manifests (see "Why the pools are hook-managed") - Kafka version is user-configurable (v3.9 default, v3.8 available) - Strimzi operator is unchanged in this PR (stays at 0.45.1-rc1); 0.45.x is the last line that supports ZooKeeper→KRaft migration - ZooKeeper removed from the dashboard UI (ApplicationDefinition) ## Migration strategy Both fresh installs and ZooKeeper→KRaft migrations are driven by a single Helm hook Job (`templates/migration-hook.yaml`, `pre-install,pre-upgrade`) that owns the `KafkaNodePool`s. On every install and upgrade the Job: 1. Ensures both node pools exist (idempotent): a broker pool named exactly `kafka` for a migrating cluster — so Strimzi adopts the existing `<cluster>-kafka-N` brokers and their PVCs in place — or `b-<hash>` for a fresh cluster, plus a controller pool `c-<hash>`. The controller pool pins `strimzi.io/next-node-ids` above the broker range so the broker pool keeps node IDs `[0..N-1]` and Strimzi cannot hand the existing broker IDs to the fresh controllers. The shared `kafka` name is claimed atomically (via `kubectl create`) so only one migration per namespace can win it. 2. Inspects the Kafka CR: no CR → fresh install (pools ensured; the main apply then creates the KRaft Kafka CR, nothing to migrate); already `strimzi.io/kraft=enabled` or `status.kafkaMetadataState: KRaft` → done; any other read error → fail closed. Otherwise (a ZooKeeper cluster) it migrates. 3. Annotates `strimzi.io/node-pools=enabled` then `strimzi.io/kraft=migration`, polls `status.kafkaMetadataState` through `ZooKeeper → KRaftMigration → KRaftDualWriting → KRaftPostMigration → PreKRaft → KRaft`, and only once a safe state is reached flips `strimzi.io/kraft=enabled`. If it times out in an intermediate state it aborts without finalising, so Helm fails the upgrade and the next reconcile retries from where it left off. Data is migrated in place: brokers keep their node IDs and reuse their existing PVCs, nothing is copied. Migration is one-way — Strimzi has no supported KRaft→ZooKeeper rollback once complete, so lowering the `version` field or rolling the app back to a pre-KRaft chart is unsupported (take a backup first). ### Why the pools are hook-managed The hook must create the pools and walk the migration state machine *before* the main release apply. A resource a hook creates cannot also be a regular chart manifest across the ZK→KRaft boundary: Helm's main apply finds the pool live in the cluster but absent from the previous (ZooKeeper) release revision and aborts with `original object KafkaNodePool ... not found`, leaving the release `failed` even though the cluster migrated fine (reproduced under server-side apply, the helm-controller default). Managing the pools only from the hook avoids that, so `templates/kafkanodepools.yaml` and the deployed-version gate ConfigMap (`templates/version.yaml`) are removed and the pools are applied idempotently by the Job on every install and upgrade. ## Bug fixes included - ZooKeeper `metricsConfig` referenced wrong ConfigMap key (`kafka-metrics-config.yml` instead of `zookeeper-metrics-config.yml`) - ZooKeeper `storageClass` used `kafka.storageClass` instead of `zookeeper.storageClass` - WorkloadMonitor used `.Values.replicas` instead of `.Values.kafka.replicas` - `validateNameLength` now caps a Kafka app name at the KRaft controller pod-hostname budget (`<release>-c-<hash>-<id>` must fit 63 chars), instead of admitting names that later fail the chart render forever ## What's next A follow-up PR will upgrade Strimzi to 0.51.0 and Kafka to 4.2 once all instances are confirmed migrated to KRaft. That PR is already prepared on branch `KRaft-upgrade-strimzi-051`. ## Test plan - [x] `helm template` renders correctly for new KRaft instances (no ZooKeeper refs) - [x] `helm template` with custom values (1 replica, external, topics, v3.8) - [x] Version validation rejects unsupported versions via JSON schema - [x] helm-unittest: 86/86 across 9 suites - [x] `hack/kafka-migration-hook.bats`: 12/12 — the real hook script executed against a stub kubectl (resolution branches, fail-closed reads, atomic collision guard, finalize guard) - [x] Operator template renders with the unchanged Strimzi 0.45.1-rc1 - [x] Live migration on a disposable cluster under server-side apply: fresh install → healthy KRaft, release `deployed`; in-place migration of a 20000-message ZooKeeper cluster → release `deployed`, broker PVCs adopted in place (same UIDs), and the KRaft data byte-for-byte identical to the pre-migration ZooKeeper data (matching sha256 over every `partition:offset:key:value`) ### Release note ```release-note [kafka] Enable KRaft metadata mode with automatic, in-place ZooKeeper-to-KRaft migration. Kafka version is now configurable (v3.9, v3.8). The Strimzi operator is unchanged (0.45.1-rc1). ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added chart-level Kafka version selector (default v3.9) and KRaft-aware node pool rendering with controller storage sizing. * **Updates** * Default Kafka appVersion updated to 3.9.0; operator and image tag defaults aligned to 0.45.0. * Templates now omit ZooKeeper resources and metrics when KRaft is in use. * **Tests** * New Helm unit tests for Kafka CR, node pools, metrics, topics, dashboard mappings, and workload monitors. * **Documentation** * Values, schema, and README updated to document new version and controller storage options. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
187e4ce12b |
test(e2e): assert the tenant OIDC UID header on the control plane
The render matrix proves the chart emits --requestheader-uid-headers, but nothing checked that the flag survives the path a real tenant takes: aggregated API admission of spec.oidc, the HelmRelease, and the rendered KamajiControlPlane. Assert it in both OIDC helpers, next to the existing --authentication-config check on the same resource. Read the arg list once into a variable and grep the capture twice. The existing check consumed a pipeline, which is single-use, and a second kubectl call would double the management-cluster reads these helpers already make. Only the header flag is asserted. The chart also renders --feature-gates=RemoteRequestHeaderUID=true, but only on v1.32 where the gate is still alpha, while this lane takes the highest version in the map, which is above that. The header flag is the part that holds wherever the flag exists at all. The bats case for the missing flag runs the helper as a standalone subshell under set -e rather than as an if condition. Shells suppress errexit for a command whose status is being tested, and that reaches inside the subshell, so the if spelling reports success and pins nothing. The binding query next to these assertions piped kubectl straight into sort and so returned sort's status. Its callers capture it in a command substitution, where errexit does not reach, so a failed query read as an empty binding list. It now returns kubectl's failure itself. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
9efe78ce44 |
test(e2e): hold the tenant-cluster install wait to an ordering rule (#3802)
A test that creates a tenant `Kubernetes` cluster must not end while that release is still installing. The kind drops the install wait on its main resources, but the chart's blocking post-install hook still holds the install open, so a test that asserts and ends hands the rest of it to teardown. Teardown then reds on a step that tests nothing. Reading the cluster mid-install is fine. Ending mid-install is not. `hack/e2e-chainsaw/_lib/run-kubernetes.sh` already waits for the parent release before its tenant-cluster assertions, and that wait is older than this change. Nothing held it there. This adds the guard that does. The guard reads the script, not Chainsaw fixtures: the cluster comes from a heredoc, so a guard walking `apply.file` would find nothing to check and pass. It requires the CR, then a readiness wait on the parent release, then the assertions the test ends on. The wait has to block, and it has to be unconditional in the body that holds it. One more case holds this script to being the only creator of that kind, since a second one would be governed by no rule here. The ordering is read within one function body, so splitting `run_kubernetes_test` reds the unit lane until the rule is pointed at the new shape. Timeout length stays with review. The floor depends on where the wait sits, and the script text does not carry that. Which spellings each matcher takes, which rewrites it refuses instead of judging, and the blind spots it names are written in the guard's header next to the code, each with its mechanism and whether it fails loudly or quietly. Fixtures mark their own lines, so a probe resolves by name instead of by a line number that drifts. No runtime code here: the guard, its fixtures, and the conventions that state the rule. ```release-note NONE ``` |
||
|
IvanHunters
|
a4547a6fe0 |
test(kube-ovn): compare image copies against upstream's Dockerfile
packages/system/kubeovn/images/kubeovn/Dockerfile is a hand-kept copy of upstream's dist/images/Dockerfile, and `make update` rewrites only its ARG VERSION and ARG TAG. A binary upstream starts shipping therefore does not reach our image on its own, and nothing read the two files together: kube-ovn-bfdd-supervisor went missing that way with the whole suite green. hack/check-kubeovn-image-copies.sh normalizes the COPY instructions of both files and reports either side's surplus. It compares only the stages built FROM kubeovn/kube-ovn-base, because our builder stage clones and patches kube-ovn and its COPY lines have no upstream counterpart, and it treats an empty result as a broken check rather than agreement. `make update` now runs it against the tag it pulls, so the next bump stops on a mismatch, and hack/check-kubeovn-image-copies.bats runs it on every CI run against a verbatim copy of upstream's Dockerfile at the vendored appVersion. Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
myasnikovdaniil
|
41ef4a6437 |
fix(ci): teach the overlay to repair an unstamped images/*.tag ref (#4362)
## What `hack/overlay-main-images.sh` walks `images/*.tag`, but every branch of its line classifier needs either a YAML key or an `@sha256:`. A `.tag` file holds one bare reference and nothing else, so until a build stamps it the line has neither, the drift guard reads it as a non-ref change, and the committed reference survives. For a package onboarded with the `:v0.0.0` placeholder this repo already uses for a new image, that surviving reference is one no registry serves. The overlay is the only thing that would replace it for a PR that does not rebuild the package, so once such a package is on main every full-suite run that skips its build keeps the placeholder. The same placeholder inside a `values.yaml` repairs itself, because there it sits under an `image:` key the classifier recognises. The difference is the file shape, not the placeholder. ## Reproduction A fixture holding a committed bare tag against an artifact copy carrying a stamped one: ``` $ cd fixture/fakerepo && sh ../../hack/overlay-main-images.sh ../mainpkgs '[]' '' overlay: packages/system/x/values.yaml -> current-main drift (non-ref change) in packages/apps/x/images/x.tag -> keeping committed ref ``` Both files differ only in their reference. The `values.yaml` one is repaired; the `.tag` one is not. Adding any digest to the same placeholder makes it pass, which is what narrows this to the classifier rather than the file. ## Why it has not bitten yet 17 of the 19 `.tag` files in the tree carry `@sha256:` and pass on that branch alone. The only other digest-less one is `packages/extra/seaweedfs/images/seaweedfs-cosi-driver.tag`, and it survives only because it is third-party: `build-main` never rebuilds it, the artifact copy is byte-identical, and the classifier is never reached. The first digest-less first-party `.tag` is what turns the gap live, and one is coming in #3426. ## The fix `hack/promote-rewrite-tags.sh` already carries a second match branch for exactly this shape, with a comment saying why ("the second branch is what catches a bare `.tag` file, whose content has no YAML key at all"). This gives the overlay the same one, scoped to `*.tag` so an unkeyed line in a `values.yaml` still counts as drift, because only in a `.tag` file is a whole line known to be a reference and nothing else. Two tests in `hack/overlay-main-images_test.bats`: the unstamped placeholder is overlaid, and a `.tag` whose non-reference line differs still counts as drift, so the rule cannot quietly widen into "every line in a `.tag` file is a reference". Removing the fix fails the first one. ```release-note NONE ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Fixed image overlay handling for `.tag` files containing bare image references, allowing current references to be applied correctly. - Preserved drift detection when `.tag` files also contain unrelated, non-reference changes. - **Documentation** - Clarified how image-reference tooling handles bare references and package-tree processing. - **Tests** - Added coverage for bare-reference overlays and mixed reference/comment changes. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
c0ff783026 |
fix(hack): make bats unit tests pass on a BSD/macOS checkout (#4284)
## What this PR does Refs #4272 (Defects 1 and 2). On a clean checkout of `main`, `make unit-tests` is red out of the box on macOS for reasons that have nothing to do with the change under test: the BSD userland shipped with macOS differs from the GNU tools on the Linux CI runners, and CI never sees any of it. This PR closes the two worst of those, both scoped to `hack/`. **Defect 1 — the runner cannot parse a non-ASCII `@test` title.** `hack/cozytest.sh` rebuilds each test's function name by walking its title one character at a time with awk `substr()`. BSD awk aborts that walk on the first multibyte byte (`towc: multibyte conversion failure`) rather than returning it, so a file carrying a non-ASCII `@test` title never parses and not one of its tests runs — silently, because the suite only reports that the runner errored. Four `hack/*.bats` files carry such a title today. The fix pins `LC_ALL=C` on all three awk passes (title parse, run selection, and the exit-code scan of the captured log) so each reads the input byte-wise; the parse and selection passes must share the ctype or they would derive mismatched function names for one title. A new regression test, `hack/cozytest-nonascii-title.bats`, builds a multibyte-titled fixture at runtime and asserts the runner actually runs it; it is red against the pre-fix runner (mutation-verified) and green after. **Defect 2 — GNU-only `sed -i`.** `hack/nightly-mirror.sh`, `hack/promote-rewrite-tags.sh`, and the fixture builder in `hack/promote-rewrite-tags_test.bats` edit files in place with `sed -i`. BSD sed reads the argument after `-i` as a mandatory backup suffix, so `sed -i -e` and `sed -i -E` swallow the following flag as that suffix and the edit either errors or silently no-ops. Each is rerouted through a temp file, portable to both seds. The `nightly-mirror.sh` rewrite additionally relied on the GNU `\?` optional quantifier, which BSD BRE reads as a literal `?` so a bare host was never matched; it is replaced with the POSIX `\{0,1\}`, equivalent on both. All three suites are green on macOS after the change, and behaviour on GNU sed is unchanged. ### Scope This PR is deliberately scoped to Defects 1 and 2. Defect 3 from the issue — a GNU-only `date -d` in `hack/seaweedfs-naming-audit.sh` — is tracked separately as #4273 and is left untouched here. For transparency: with Defect 1 fixed, several `hack/*.bats` suites that previously never ran now run, and a handful still fail on macOS because they depend on GNU coreutils `timeout`, which base macOS lacks (`capture-dataplane`, `cozytest-capture-gate`, `run-kubernetes-join-timing_test`, `run-kubernetes-runner-cpu_test`, `runner-identity`). Those failures are pre-existing — identical before and after this change when the runner is swapped in place — and are a distinct defect from the two fixed here, so they are out of this PR's scope. A fully green `make unit-tests` on macOS additionally depends on #4273 and on that separate `timeout` gap. ### Verification All fixes were verified on a genuine macOS host (Darwin, BSD awk 20200816 / BSD sed). `hack/cozytest-nonascii-title.bats`, `hack/nightly-mirror_test.bats` (5 tests), and `hack/promote-rewrite-tags_test.bats` (17 tests) are green; the two files the issue reports as running zero tests (`overlay-main-images_test.bats`, `promote-gate-contract.bats`) now yield 13 and 15 `Test OK`, matching the issue's numbers. Each fix is mutation-proven: reverting it turns the corresponding suite red again. ### Downstream repositories The diff touches only internal `hack/` tooling — the bats test runner and two release scripts. No downstream repository vendors or restates these: `cozystack/ccp` and `cozystack/external-apps-example` gate on `hack/package.mk`, `hack/common-envs.mk` and `hack/update-crd.sh`, none of which this PR changes, and nothing renames or moves a file or alters what a make target does. - [x] No downstream repository is affected by this change ### Release note ```release-note fix(hack): make the bats unit-test runner parse non-ASCII `@test` titles under BSD awk, and route the in-place edits in `nightly-mirror.sh` and `promote-rewrite-tags.sh` through a temp file so they work under BSD sed — closing two of the host-tool gaps that make `make unit-tests` red on a clean macOS checkout ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved test runner compatibility with non-ASCII test titles and output. * Improved text-processing and file-rewrite behavior across BSD and GNU environments. * Preserved file permissions and metadata during automated rewrites. * Improved handling of host values and release tags across supported platforms. * **Tests** * Added coverage for non-ASCII test titles. * Updated rewrite tests for portable BSD and GNU tooling. * **Documentation** * Clarified supported BSD and GNU command-line tool compatibility. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Alexey Artamonov
|
0b9332877e |
test(kafka): drop the false-green ZK->KRaft migration chainsaw suite
The kafka-zk-to-kraft-migration suite did not exercise the pre-upgrade hook, it reimplemented a simplified flow beside it: a controller pool named "controller" instead of the hashed c-<hash>, no strimzi.io/next-node-ids pin, and a single broker so the controller-vs-broker node-id collision could never occur. That is exactly the defect the hook exists to prevent, so the suite stayed green on a revision whose in-place migration lost every message — reproduced by hand 2/2 on a live cluster. A suite that cannot fail on this code is worse than no suite: it reads as migration coverage and invites trust it has not earned, while costing ~380s on every Kafka PR and asserting only that a PVC object exists, never reading a record back. The migration hook's bats suite and the live upgrade runs are the real coverage; the Strimzi bump would delete this suite regardless. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Aleksei Sviridkin
|
1c88014661 |
fix(installer): keep node_modules out of the packages artifact (#4290)
## What this PR does `flux push artifact` walks the whole `packages/` tree and reads neither `.gitignore` nor `.sourceignore`, so a working copy where somebody ran `pnpm install` under the vendored console ships the console's dependency tree inside the bundle. Helm then refuses the dashboard chart on every cluster that pulls it, because one dependency file sits above the 5MB per-file limit, and the build itself reports nothing: ```text Could not load chart: decompressed chart file "dashboard/images/console/node_modules/.pnpm/@rolldown+binding-darwin-arm64@1.0.0-rc.15/node_modules/@rolldown/binding-darwin-arm64/rolldown-binding.darwin-arm64.node" is larger than the maximum file size 5242880 ``` Passing `--ignore-paths` replaces flux's built-in list rather than extending it, so the defaults are restated alongside `node_modules/`. `hack/check-packages-artifact-ignores.bats` holds both halves: dropping `node_modules/`, dropping one of the defaults, or declaring the variable without passing it to the recipe each turn the test red. Its second case re-reads the defaults out of the installed flux, so an upstream addition surfaces as a failure rather than as a pattern quietly missing from the artifact. Measured on a tree with the console's dependencies installed: the artifact goes from 80MB carrying 32397 `node_modules` entries to 6.7MB carrying none, with the dashboard chart still in it. ### Screenshots Not a UI change. ### Downstream repositories - [x] No downstream repository is affected by this change The two workflows that push the same artifact themselves (`e2e-fork.yaml`, `nightly.yaml`) run from a clean CI checkout, where the tree has no `node_modules` to pick up. ### Release note ```release-note fix(installer): the packages OCI artifact no longer picks up node_modules from a developer's working copy, which made the dashboard chart unloadable on every cluster that pulled the bundle. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Package artifacts now exclude unnecessary files, including version-control metadata, common media and archive formats, and dependency directories. * This helps prevent oversized files from exceeding Helm’s per-file size limit. * **Tests** * Added validation to ensure package artifact ignore patterns remain complete and aligned with the packaging tool’s defaults. * Optional checks report when the packaging tool is unavailable without failing the test suite. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Alexey Artamonov
|
ac92b6b95c |
fix(e2e): cap the bucket verify S3 reads so the step honors its timeout
A Chainsaw `script` op runs its shell under exec.CommandContext with stdout and stderr wired to in-process buffers. When the op timeout fires it SIGKILLs the shell process alone, not its descendants, and cmd.Wait() does not return until every process that inherited the pipe fds closes them. An mc that keeps retrying an S3 call after the shell is gone holds the pipe open, so the op runs to mc's own exhaustion instead of stopping at its ceiling. This is why the bucket suite's verify-every-bucket-takes-a-write step ran 606s under a 300s ceiling. verify-s3-access shares that shape, so wrap each S3 and apiserver read in its own `timeout`: a hung read dies at a known budget and the op timeout stays a backstop. The 30s kubectl cap matches the terminal `k` wrapper the redis suite carries. The two `mc cp` PUTs take 45s, the budget verify-every-bucket-takes-a-write already gives a write, because a working PUT can still wait while the master grows a volume for a freshly created bucket; 15s left too little headroom on that grow and could turn a slow-but-healthy write red on an unrelated PR. The 15s cap on the mc reads has no terminal precedent: the backup-access preflight caps `mc cp` at 15s too, but inside a retry loop where a 124 costs one attempt rather than the step, so it is not one. Land each secret's encoded value in a file before base64 -d so a timed-out kubectl read (exit 124) stops the step under set -e rather than masking as an empty-credential mc alias set further down. The readonly-upload deny probe branched on mc directly (if mc cp ...), which suspends set -e, so a timeout there (exit 124) was indistinguishable from the access-denied non-zero the probe expects: a hung deny path would exit 124, take the false branch, and reach a green cleanup, the exact window this change bounds. Capture the status instead: exit 0 is the readonly regression, 124 is a timed-out probe, both fail; any other non-zero is the expected denial. The suite's own catch read (the `kubectl get pods` in namespace cozy-objectstorage-controller) carried neither an op timeout nor an inner cap, so it inherits timeouts.exec (5m) and can hold the pipe the same way; give it a 30s inner cap and a 1m op timeout so the rule the rest of the file follows holds for it too. The .chainsaw.yaml notes still read the op ceiling as a reliable cut, the old SIGKILL as a de-facto cap and a smaller timeout as guaranteeing a kill; correct them, since the shell-only kill plus the pipe wait are why an inner cap on every read is required rather than optional. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Aleksei Sviridkin
|
92d2eede52 |
test(opensearch): separate the mount-plan rewrite from the roll onto it (#4279)
## What this is
`Refs #4259`. A diagnosis of the opensearch suite timeout plus the one
in-repo change it warrants: making the failing step say which of its two
candidates actually happened. It does **not** claim to fix the root
cause — see "What this does not do".
## What the run actually showed
The `verify-statefulset-rolled-onto-the-new-mount-plan` assert erred
with exactly two field failures, both under `status`: `(currentRevision
== updateRevision): false` and `updatedReplicas: field not found`. No
volume field appears in the error list, and `readyReplicas: 1` shows as
an unchanged line, so the pod was Ready at the deadline and nothing had
replaced it. Per e2e-testing.md ("Reading a failure diff: the actual
side is a projection"), the real cause is the error line, not the diff —
and a Chainsaw diff projects the live object onto the expected fields,
so the three `http-ca` / `http-cert` / `http-key` lines showing in the
diff are the expected tree, not proof the volumes are absent. Read that
way, the volume assertions matched: the three volumes were present on
the live template. The "volumes missing" reading is the projection trap
the doc warns against.
## Which candidate
Candidate (b): the template was rewritten onto the subPath mount plan,
and the operator has not restarted the pod onto it. This is consistent
with the source, traced against opensearch-k8s-operator v2.8.0 as
vendored:
- The chart renders the CR correctly — `generate` dropped, `secret` and
`caSecret` named — proven by
`packages/apps/opensearch/tests/tls_upgrade_test.yaml`.
- Given that CR, `pkg/reconcilers/tls.go` `handleHttp` appends exactly
the three volumes the step asserts (`http-ca` from
`opensearch-test.http-ca`, `http-key` and `http-cert` from
`opensearch-test-http-server`), and `pkg/reconcilers/cluster.go`
`reconcileNodeStatefulSet` writes that template onto the StatefulSet.
Three branches return ahead of that write — no node-pool config hash,
the `opensearch.role` recreate, and parallel recovery — but the
parallel-recovery gate is `ReadyReplicas < replicas-1`, unreachable at
one replica, so the write happens. The reconcilers that run ahead of the
cluster reconciler do not stop it either: the TLS reconciler runs first
and returns the admin-certificate path's result, empty in the ordinary
case, and `securityconfig.Reconcile` short-circuits to nil when the
security-config checksum is unchanged, which the move does not change.
So the operator does write the volumes.
- The nodePool StatefulSet is `OnDelete` (`pkg/builders/cluster.go` sets
`UpdateStrategy.Type: OnDelete` alongside `PodManagementPolicy:
OrderedReady`), so the StatefulSet controller never deletes the pod to
apply a template change: `podManagementPolicy` orders creation and
deletion during scaling, `updateStrategy` decides the update. The roll
is the operator's rolling-restart reconciler's to drive
(`pkg/reconcilers/rollingRestart.go`) — it deletes the pod itself. It
needs `status.Initialized`, it needs `ReadyReplicas == replicas` (a
Ready pod is a precondition, not the blocker), then it opens a client to
the OpenSearch API and calls `CheckClusterStatusForRestart`, which holds
the restart while the cluster is red; on yellow the gate now enables
shard allocation, waits for every data node to join, waits for shard
activity to settle, then proceeds (#4317 deleted the old
`.opensearch-observability` carve-out, and this suite builds the
operator with that patch). Any of those gates can sit indefinitely.
`status.currentRevision` never advances by itself under `OnDelete`; the
operator patches it once `updatedReplicas == replicas`, so the second
assert's own condition cannot go true unless that reconciler ran to the
end.
- The pod's startup probe is a TCP check on 9200
(`builders/cluster.go`), so "connection refused" is OpenSearch not
listening, not a probe-scheme mismatch. The plaintext `GET / HTTP/1.1`
from `curl/8.5.0` on the TLS listener is neither the startup/liveness
probe (TCP) nor the readiness probe (`curl -k https://localhost:9200`,
localhost, HTTPS) — it is external traffic to the LoadBalancer address,
a red herring for the stall.
## What this changes
The step now runs two asserts instead of one. The first waits for the
operator to rewrite the template onto the three volumes, on a
reconcile-scale budget (6m) — a failure here is the operator not writing
them (candidate a). The second waits for the roll to complete onto that
template, on the pod-lifetime budget (12m) — a failure here with the
first green is the roll not completing (candidate b): the operator never
restarting the pod, which the rolling-restart reconciler holds while
`CheckClusterStatusForRestart` reports the cluster red — on yellow the
gate enables shard allocation, waits for every data node to join, waits
for shard activity to settle, then proceeds — or a replacement that was
deleted but never came Ready. The second assert carries the three volume
projections and `observedGeneration == metadata.generation` as well, so
the roll is pinned to the template the first assert checked rather than
to whatever revision is current when the status converges. The suite
`catch` also captures the StatefulSet as YAML and as a describe, and the
operator's own log, because `updateRevision`, `currentRevision`,
`generation` and `updatedReplicas` are what separate the two halves —
describe alone omits them — and the restart decision is logged in the
operator, not in the cluster pods. The reconciler logs `Starting rolling
restart of the StatefulSet` (`rollingRestart.go:155`) once its early
gates (`Initialized`, ready-replica, update-pending) pass; it builds its
OpenSearch client just before, at `:138`, and that client pings the
cluster — so a run whose log lacks `Starting rolling restart` either had
an earlier gate hold it silently or had the client ping fail loudly, not
only the former, while that line present followed by a `Couldn't proceed
with rolling restart ... because <reason>` line names a health-stage
hold (the health gate is `CheckClusterStatusForRestart`, and it is not
the last: `PreparePodForDelete` follows it). The operator-log `tail` was
raised so it spans the whole assert window rather than a third of it.
## What this does not do
It does not fix the stall; #4317 does, and is already merged. The run
that captured this (`Run E2E tests`, job 104576486900) shows the
rolling-restart reconciler holding on health: the manager logged
`Starting rolling restart of the StatefulSet opensearch-test-nodes`
(`rollingRestart.go:155`, printed only after the `Initialized` and
ready-replica gates passed) and then, every ten seconds to the deadline,
`Couldn't proceed with rolling restart ... waiting for health to be
green` (`os_data_service.go:180`), which is reached only after
`GetFlatClusterSettings` returned 2xx (`os_client.go:224` makes anything
else an error). So the operator reached the cluster and the cluster
answered; health held the restart. #4259 established why, and the CA
move plays no part: on a single data node the bootstrap node leaving is
audited into a fresh `security-auditlog-*` index with one replica, that
replica has nowhere to go, the cluster stays yellow for good, and the
v2.8.0 restart gate opens only on green (or on a yellow
`.opensearch-observability`) so it never opens. #4317 backports
opensearch-project/opensearch-k8s-operator#1569 to fix that gate; this
PR is only the diagnostic split that names which half of the roll a
failure is. The split content itself is not the failure: on `49f3f059f`
both asserts passed, the second after 1m47s, and this change touches
only the step's comments and the operator-log tail, not the assertions.
With this change in place, a future occurrence names the half, and the
catch carries the roll's own fields and the operator log, so the
crust-gather can be read against a specific claim rather than an
ambiguous one.
### Release note
```release-note
NONE
```
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **Tests**
* Improved OpenSearch StatefulSet failure diagnostics by capturing YAML,
detailed resource descriptions, and the latest operator pod logs.
* Expanded rolling-restart verification to check volume projections,
template updates, rollout readiness, observed and metadata generations,
and revision consistency.
* Separated template validation from rollout completion checks with
dedicated six- and twelve-minute time limits.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
|
||
|
IvanHunters
|
c4f72325a6 |
fix(test): pass over the flux defaults check instead of aborting the suite
hack/cozytest.sh is not bats and defines no `skip`, so the guard for a missing flux killed the whole file with `skip: not found` and kept the unit job red. A red checks job also cancels finalize and e2e, so the recipe this suite exists to hold never ran in CI at all. The form the rest of the suite already uses returns 0 instead, as hack/release-changelog-behaviour.bats does for a missing node. Verified by mutation: with flux installed, dropping a pattern from FLUX_DEFAULT_IGNORES still turns the test red, so the early return does not make it vacuous. Assisted-by: LLM Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
scooby87
|
6677c24beb |
fix(ci): fail the trailer check on an empty commit range (#4278)
## What `hack/check-commit-trailers.sh` printed `Commit trailers OK: 0 commit(s) checked.` and exited 0 when the range it was given resolved to zero commits, so a run that examined nothing was indistinguishable from a clean run. An empty range now prints a message naming the empty case and exits 1. ## Why The documented pre-push recipe in `docs/agents/contributing.md` is `hack/check-commit-trailers.sh origin/main..HEAD`. The range is resolved in the process working directory, so an agent whose shell sits in a different checkout than the branch resolves it against the wrong `HEAD` and gets nothing — and the old green there was a green nobody could tell from a real one, defeating the check precisely when the checkout is misconfigured. The CI path is unaffected: `.github/workflows/pre-commit.yml` builds the range from a pull request's merge base and head SHA, which GitHub refuses to open with no commits between the two, and which goes empty only once the base has absorbed the head's commits and there is nothing left to merge, where a red trailer check is harmless. ## Test TDD, bats: the previously buggy `accepts an empty range` test is inverted into `fails on an empty range instead of reporting OK`, which asserts a non-zero exit, that the message names the empty case, and that the `Commit trailers OK` line is absent. Mutation-verified — removing the guard turns it RED, restoring it turns it GREEN. Full suite: 28 passing, 0 failing. ## Release note ```release-note Local runs of `hack/check-commit-trailers.sh` now fail on an empty commit range instead of reporting OK, so the documented pre-push check no longer passes silently when it examined nothing. ``` Fixes #4263 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Commit trailer checks now fail when the specified commit range contains no commits. - Empty ranges no longer report a misleading successful validation. - Error output clearly indicates that no commits were checked. - **Tests** - Added coverage confirming empty commit ranges return a failure status and appropriate diagnostic output. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
myasnikovdaniil
|
a1b32f2e5a |
ci(workflows): build Talos only when its sources changed (#4021)
Stacked on #4020, not independently mergeable. Nothing on the same-repo PR path consumes either half of what `build-talos` produces. `image-talos` pushes `${REGISTRY}/talos:pr-N-sha`, which no chart in the tree references and which is not even version-tagged here (`PUBLISH_VERSIONED` and `PUBLISH_FLOATING` are both 0). `image-matchbox` stamps `packages/extra/bootbox/images/matchbox.tag`, and that one line is the whole content of `pr-patch-fragment-talos` - checked against the artifact from a real run - but bootbox is opt-in and the E2E platform Package does not enable it, so no run pulls the image. Unstamped tag leaves installer artifact pointing at released matchbox, which is a valid ref nothing on this path resolves. So the job ran a privileged siderolabs imager build on every PR for two artifacts with no reader. Now it runs when the PR touched `packages/core/talos`, and `finalize` accepts a skipped result as well as a successful one. Without that second half skipping it would block finalize, and therefore e2e, on nearly every PR. Forks keep the unconditional build, and the reason is not about talos. `e2e-fork.yaml` publish job requires at least one OCI archive to exist, an invariant that holds today only because this job always ran, so a fork PR touching nothing under `packages/` would arrive with an empty export and fail a legitimate run. Teaching that guard what `plan` expected to build is work in the fork allowlist path and belongs in its own change. Saving is about 1.5-2 min of critical path. Main point is removing a privileged image build that nobody reads from every pull request. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **CI Improvements** - Pull request validation now runs Talos builds only when relevant changes require them, while retaining full validation for fork-based contributions. - Finalization correctly handles skipped Talos builds and preserves generated boot artifacts when builds succeed. - Changed-file detection now accounts for renames, deletions, and shared build inputs. - **Documentation** - Updated testing and build guidance to reflect the current validation behavior. - **Tests** - Added comprehensive coverage for build classification, workflow outputs, skipped jobs, and artifact handling. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Myasnikov Daniil
|
1118b340a7 |
fix(ci): teach the overlay to repair an unstamped images/*.tag ref
hack/overlay-main-images.sh walks images/*.tag, but every branch of its line classifier needs either a YAML key or an @sha256:. A .tag file holds one bare reference and nothing else, so until a build stamps it the line has neither, the drift guard reads it as a non-ref change, and the committed reference survives. For a package onboarded with the :v0.0.0 placeholder this repo already uses for a new image, that surviving reference is one no registry serves. The overlay is the only thing that would replace it for a PR that does not rebuild the package, so once such a package is on main every full-suite run that skips its build keeps the placeholder. The same placeholder inside a values.yaml repairs itself, because there it sits under an `image:` key the classifier recognises: the difference is the file shape, not the placeholder. The gap is latent rather than new. 17 of the 19 .tag files in the tree carry @sha256: and pass on that branch alone; the only other digest-less one, extra/seaweedfs/images/seaweedfs-cosi-driver.tag, is third-party, so build-main never rebuilds it, the artifact copy is byte-identical and the classifier is never reached. The first digest-less first-party .tag is what turns it live. hack/promote-rewrite-tags.sh already carries a second match branch for this shape and states why. This is that branch, scoped to *.tag so an unkeyed line in a values.yaml still counts as drift. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Alexey Artamonov
|
7fd8376ccb |
fix(ci): fail the trailer check on an empty commit range
The pre-push recipe resolves origin/main..HEAD in the process working directory, so an agent whose shell sits in a checkout at or behind the base resolves it to zero commits and the check printed "Commit trailers OK" with exit 0. A run that examined nothing was indistinguishable from a clean one, defeating the recipe exactly when the checkout is wrong. Treat an empty range as a named failure so the misconfiguration is loud; the CI path is unaffected because it builds the range from a pull request's merge base and head, which is empty only once nothing is left to merge. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Alexey Artamonov
|
8ef230f0bf |
Merge remote-tracking branch 'origin/main' into kafka-multi-instance
Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> # Conflicts: # docs/storage-immutability.md # packages/apps/kafka/tests/topics_test.yaml # packages/system/kafka-rd/cozyrds/kafka.yaml |
||
|
Aleksei Sviridkin
|
8799a4e4aa |
fix(cluster-api): backport live migration before host eviction (#4291)
## What this PR does CAPK v0.1.10 handles host eviction by draining and deleting the guest VMI. It also looks up the physical host name in the guest cluster, and a recreated guest can remain cordoned. Backport CAPK #374, #389 and #392 onto the current provider API and runtime, with separate bounded submission, active migration and fallback windows, and ordinary VMIM objects without a priority, a field the pinned `kubevirt.io/api` v1.5.0 does not carry. The submitted VMIM deadline survives reconciliation; expired, invalid or legacy marker values enter the existing fallback path. Add the manager build to the package pipeline, read its digest pin through the provider imageUrl override, and ship VMIM create permission in the compressed provider manifest. Source tests and four packaging checks pass, including failed-build pin preservation and the rendered ConfigMap payload. The regression tests include unprocessed submissions, legacy and invalid deadline values, repeated reconciles, and a fresh active migration window after submission expiry. The updated manager also cross-compiles for linux/amd64 with Go1.26.4. The previous OCI build predates the submission-deadline correction and does not validate an image containing this change. The updated source still needs the maintainer-owned image build described below. The checked-in pin still identifies the original upstream image, so installing this source chart directly does not enable the backport. The regular main and release builds now include this package, stamp the built image digest, and package that exact artifact; the release workflow also commits the generated pin. Pre-merge E2E validation needs a maintainer-owned build because fork publication only allows image repositories already present in the base tree. ### Downstream repositories - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note ```release-note fix(cluster-api): attempt live migration before drain/delete on External eviction, use the guest node name for fallback drain, and uncordon recreated guests. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added a Cluster API Provider KubeVirt image pinned to version 0.1.10. - Added migration-first workload eviction with fallback draining and node recovery. - Added permission to create virtual machine migration resources. - Included the provider image in builds and deployment templates. - **Bug Fixes** - Preserved migration deadlines across retries and improved recovery after interrupted or completed migrations. - Added a bounded wait for migrations before falling back to draining. - **Documentation** - Documented the provider image version, patches, build process, and release considerations. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
ee09e73343 |
fix(kafka): make topics[].config optional (#3935)
## What this PR does Makes `topics[].config` optional across the Kafka typed API, generated schema, documentation, and chart contract. The KafkaTopic template already omits an empty config, and Strimzi accepts the field as optional; the Cozystack schema was the layer incorrectly rejecting such topics. The Helm test renders a topic without config and asserts `spec.config` is absent. The Chainsaw fixture also omits config and waits for the resulting KafkaTopic to reach Ready. Go API compatibility: `Topic.Config` changes from `runtime.RawExtension` to `*runtime.RawExtension`, so Go callers constructing that field must pass a pointer. The Terraform provider intentionally does not model Kafka topic tuning (`config`), so this does not require a provider follow-up. Validation: - `helm unittest packages/apps/kafka`: 36/36 passed. - `go vet ./kafka` in `api/apps/v1alpha1`: clean; the package has no test files, so this shows it compiles. - root `make generate`, which owns `zz_generated.deepcopy.go`, and `make -C packages/apps/kafka generate`: no diff. ### Screenshots Not a UI change. ### Downstream repositories Walked the trigger map against all nine changed files. The provider's Kafka model deliberately excludes `topics[].config`; no downstream repository mirrors this nested field or its requiredness. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note ```release-note fix(kafka): allow topics to omit the optional Strimzi config map ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Kafka topic configuration is now optional. Topics can be created without explicitly providing configuration settings. - Topic configuration is omitted from serialized resources when not specified. - **Bug Fixes** - Improved handling of topic resources without configuration, including correct readiness validation and rendering. - **Documentation** - Updated Kafka configuration documentation and schemas to reflect that topic configuration is optional. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Alexey Artamonov
|
8fbec92411 |
test(hack): match rewrite diagnostics literally in the mirror test
The two assertions checking the dry-run host-rewrite diagnostic used `grep -q` with patterns carrying the registry hostnames `iad.ocir.io` and `ghcr.io`. The dots are BRE wildcards, so the assertions pass on a diagnostic whose host bytes are corrupted (`iadXocir.io` matches), which is exactly the failure the test exists to catch. Switch both to `grep -Fq` so the host strings are compared literally. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Alexey Artamonov
|
a9dd898684 |
fix(hack): correct the nightly-mirror dry-run diagnostic and BSD-sed comments
The --dry-run line advertised `sed -i 's|...|g'`, an idiom the real path stopped using once the edit moved through a temp file, and it printed only the first of the two expressions the rewrite applies -- understating what the run touches and re-describing a command the script no longer runs. Report the rewrite by intent instead, covering both the `<host>/` prefix and the bare-host scalar. Its test asserted the old `s|...|g` string, so it encoded the same stale idiom; retarget it at the new message and guard against `sed -i` reappearing in the plan. Also tighten two comments that overstate what BSD sed does: the `\?` note claimed the quantifier merely fails to match a bare host, but `["']\?` reads as "a quote then a literal ?" so the whole expression is dead on macOS, matching neither shape; and the `cat`-over-temp-file note now records that `cat` truncates before it writes, an interrupt-window trade taken to avoid `mv` stamping mktemp's 0600 onto rewritten files. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Alexey Artamonov
|
cb08472c98 |
fix(hack): make in-place sed edits portable to BSD sed
nightly-mirror.sh and promote-rewrite-tags.sh, and the fixture builder
in promote-rewrite-tags_test.bats, rewrite files with `sed -i`. BSD sed
reads the argument after -i as a mandatory backup suffix, so `sed -i -e`
and `sed -i -E` consume the following flag as that suffix: the edit
either errors or silently no-ops, leaving `make unit-tests` red on macOS
while CI stays green on GNU sed.
Route each edit through a temp file instead. The nightly-mirror rewrite
also relied on the GNU `\?` optional quantifier, which BSD BRE reads as a
literal `?` so a bare host never matched; replace it with the POSIX
`\{0,1\}`, equivalent on both seds.
Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
|
||
|
Alexey Artamonov
|
0473079448 |
fix(hack): parse non-ASCII @test titles under BSD awk
cozytest.sh rebuilds each test's function name by walking its @test title one character at a time with awk substr(). BSD awk aborts that walk on the first multibyte byte instead of returning it, so a file carrying a non-ASCII title never parses and none of its tests run -- silently, since the suite only reports that the runner errored. Four hack/*.bats files carry such a title today, and the gap is invisible to CI because GNU awk does not abort on the same input. Pin LC_ALL=C on all three awk passes -- the title parse, the run selection, and the exit-code scan of the captured log -- so each reads the input byte-wise. The parse and selection passes must share the ctype or they would derive mismatched function names for one title. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Alexey Artamonov
|
324a342e61 |
test(seaweedfs): make the BSD-date shim honor the audit's real format string
The two end-to-end tests that reproduce issue #4273 on a GNU runner drive the audit through _write_fake_date, a `date` shim that rejects the GNU `-d` spelling so the script must take its BSD `date -j -f` branch. But the shim discarded the format argument the script passed and recomputed the epoch with its own hardcoded `%Y-%m-%dT%H:%M:%S`. So the one string those tests exist to protect -- the audit's BSD format spelling, which is the whole of #4273 -- was never actually exercised: a regression in it left the tests green. The only test that hits a real BSD `date -j -f` short- circuits on the GNU branch on the CI runner, so the gap was invisible exactly where the gate runs. The shim now captures both the format and the value and uses the format the audit passed. On a BSD host it honors it through real `-j -f`, so a wrong format fails as production would; on a GNU-only runner, which has no `-j` to enforce a format, it falls back to `-d` only for the exact format the audit is supposed to pass, so a regressed spelling is not silently rescued and the tests turn red on the very runner they guard. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Alexey Artamonov
|
6aa4ca954a |
fix(seaweedfs): parse naming-audit timestamps portably across GNU and BSD date
The naming audit read PV and first_deployed timestamps with `date -u -d`, a spelling only GNU date accepts. On a host with BSD date the call failed, its stderr was discarded by the `2>/dev/null` on the same line, and `|| return 0` turned that host-tool failure into the audit's "no epoch" signal -- so every tenant read as MIXED with "direction cannot be established", as if the cluster lacked the evidence rather than the host lacking a readable clock. Since the audit gates a destructive runbook step, that indistinguishability is the defect. parse_epoch now tries the GNU spelling then the BSD one (`date -j -f`) and uses whichever the host speaks, so both call sites work on GNU and BSD hosts alike with no behaviour change for a valid timestamp on GNU. A timestamp that is present but readable by neither form is no longer folded into the absent-evidence branch: it is fatal and names the value, keeping a genuine parse failure loud and distinct from the legitimate "no bound PV age" case (unbound claim, NotFound PVC/PV) the audit conservatively degrades on. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Yan Bondarenko
|
0e4d6a305c |
test(cluster-api): declare the subshell EXIT traps in the CAPK bats suite
hack/bats-no-exit-trap.bats is an exact ratchet, and the new suite installs three traps inside explicit subshells without declaring them, so make unit-tests fails on the merge commit. The header now declares the count and says why the three are not debt. Assisted-by: LLM Signed-off-by: Yan Bondarenko <202671653+yankawai@users.noreply.github.com> |
||
|
Yan Bondarenko
|
c8f4906e93 |
fix(cluster-api): backport live migration before host eviction
Signed-off-by: Yan Bondarenko <202671653+yankawai@users.noreply.github.com> |
||
|
Yan Bondarenko
|
8ec76a3614 |
fix(kafka): make topics[].config optional
Signed-off-by: Yan Bondarenko <202671653+yankawai@users.noreply.github.com> |
||
|
Aleksei Sviridkin
|
88a7ea0e3c |
fix(kubeovn): retry VM migration setup after scheduling (#4253)
## What this PR does Kube-OVN can drop VM migration setup when a migration enters `Scheduling` before the target Pod is visible or the source or target node is known. The phase may remain unchanged afterward, leaving the target Pod waiting for network setup. Backport the final fix from kubeovn/kube-ovn#6834 to the pinned v1.15.10 image: return an error for the transient missing-node case so the existing rate-limited queue retries it. Keep the same-node behavior unchanged. That backport can be removed when updating to v1.15.14 or later. Add a separate retry guard for the target-Pod creation race. It returns an error when the informer cache has no target Pod yet, instead of forgetting the migration event. This guard is not part of #6834 and must remain until an equivalent fix is verified in the selected upstream version. Also include the existing kubeovn image target in the root build list. Without it, a kubeovn patch produces an empty PR build matrix and the image is omitted from the regular build and release paths. Validation: - All six regression cases pass on Linux/amd64 using the real controller workqueue. The new missing-target-Pod case fails without its guard and passes after the Pod appears in the informer cache, without changing the VMIM phase. The existing deferred-node, already-scheduled, and same-node cases continue to pass. - All 19 build-matrix tests pass; the full matrix includes kubeovn exactly once. - Both lint steps, pinned golangci-lint v2.10.1 and upstream `modernize -test -skipgenerated ./...`, pass for the Linux/amd64 target using Go 1.26.4. On macOS, modernize is built for the host and run with the Linux target environment, since `go tool` otherwise tries to execute a cross-compiled Linux binary. - All three packaged patches apply cleanly to pinned v1.15.10 with `git apply --check --whitespace=error`; the resulting source matches the tested tree. Formatting and source/build whitespace checks pass. The regression test covers controller queue behavior; the standard container E2E environment does not cover DRBD-backed live migration. The regular image build applies the backport and stamps the resulting digest before packaging. ### Screenshots Not applicable; no UI change. ### Downstream repositories - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: The change adds an existing image target to the build list; package layout, make target interfaces, chart values, APIs, and published image-reference formats stay the same. CCP discovers package image targets dynamically and does not carry the root build list. The website has no documented workaround for kubeovn/kube-ovn#6823 or #6834 to remove. ### Release note ```release-note fix(kubeovn): retry VM migration network setup when the target Pod or source/target scheduling information is not yet available, and include the Kube-OVN image in the regular build matrix. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved VM migration handling when source or destination node scheduling information is temporarily unavailable. - Migration events are now retried after scheduling completes instead of being dropped. - Preserved existing behavior when migration source and destination nodes are identical. - **Tests** - Added coverage for migration scheduling and retry scenarios, including partially assigned and same-node cases. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Alexey Artamonov
|
36bbc62445 |
test(opensearch): separate the mount-plan rewrite from the roll onto it
The verify-statefulset-rolled step folded two outcomes with two owners into one assert: that the operator rewrote the nodes template onto the subPath mount plan, and that the StatefulSet finished rolling onto it. When the roll stalled, the timeout could not say which half did not happen. The failure surfaced only as the status mismatch, while the volume projections in the diff read as absent even though they were present -- the projection trap e2e-testing.md documents, where the "actual" side of a Chainsaw diff is a projection and the real cause is the error line above it, not the diff. Split into two asserts so each candidate fails on its own line: the template rewrite on a reconcile-scale budget, the roll on the pod-lifetime one. The nodePool StatefulSet is OnDelete (opensearch-k8s-operator v2.8.0), so the roll is the operator's rolling-restart reconciler's to drive, not the StatefulSet controller's -- it deletes the pod itself, and currentRevision advances only once that reconciler patches it. A failure on the first assert is the operator not writing the volumes; a failure on the second with the first green is the roll not completing. The catch captures the StatefulSet as YAML and as a describe and the operator's own log, because updateRevision, currentRevision, generation and updatedReplicas are what separate the two halves -- describe omits them -- and the restart decision is logged in the operator, not in the cluster pods. The roll-gate comment ties each log signature to a v2.8.0 source line: the reconciler builds its OpenSearch client at rollingRestart.go:138, before it logs "Starting rolling restart" at :155, and that client pings the cluster, so the absence of the log line means an early gate held silently or the client ping failed loudly -- both, not just the first. The stall itself is fixed in #4317: a single-replica cluster is left permanently yellow by an index whose replica cannot be assigned. This change only makes the suite name which half of the roll a future failure stalled on. Refs #4259 Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> Assisted-by: LLM |
||
|
Timofei Larkin
|
c398e0e116 |
Merge branch 'main' into chore/drop-http-cache
main took migration 57 and moved migrations.targetVersion to 58 while this branch was open, so that line is the one conflict: 57 at the merge base, 58 on main, 59 here. Resolved as 59, which is what this branch's migration 58 requires, since run-migrations.sh loops up to targetVersion - 1. Merged rather than rebased because the branch carries commits that are not mine to rewrite. Signed-off-by: Timofei Larkin <lllamnyp@gmail.com> |
||
|
Timofei Larkin
|
2cbc02bd10 |
fix(platform): freeze HTTPCache on the artifact the cluster runs
Pinning the frozen source to a digest written into the migration picks one release for every cluster, and v1.6.0 is not even the last one that ships http-cache: v1.6.1 through v1.6.3 and v1.7.0-alpha.1 all do, and 1.6 patch releases may keep doing so. A cluster on any of those would be rolled back to an older chart by the upgrade that was meant to leave its instances alone, and an air-gapped mirror resolves the constant only if someone mirrored that exact manifest. The hook runs before the upgrade re-points cozystack-packages, so the live object still names the release the instances render from, in whatever registry the operator serves it from and with whatever pull secret and verification they configured. Reading the digest off it makes the freeze a no-op by construction and needs no knowledge of which tag was last. spec.ref.digest is used when the installer pinned one; a tag or semver ref goes through status.artifact.revision, whose digest half is the manifest digest. status.artifact.digest is the checksum of the downloaded tarball and cannot stand in. A source with neither has nothing to freeze on and the migration stops there. The copy also carries the no-delete label the platform chart puts on the original, because it is the only chart source those instances have left. The tenant grant travels with the freeze as well. cozy:tenant:admin:base enumerates the apps.cozystack.io resources a tenant owner may write, and an application that has left the catalog has no business in that list: the next sweep over it takes the entry as dead config, and the failure is silent and remote, a Forbidden for the one person who still needs to remove what they have. The chart drops httpcaches, and a cluster that freezes gets a ClusterRole under the tenant admin aggregation label instead, applied before the upgrade drops the rule so the access never lapses and carrying the verb set the chart had so nothing changes for the owner. A cluster with no instance is granted nothing. The Package delete on the no-instance path has to block. The operator reconciles off an informer cache, so a reconcile in flight against the pre-delete entry re-creates the release it owns, and with it the ApplicationDefinition and the Helm storage the following lines remove. Returning immediately made the ordering a property of the command log rather than of the cluster. The preset check's SIGPIPE note named http-cache-rd as its example of a large enum, and that chart goes away here; kubernetes-rd carries the same enums. Numbered 58 rather than 57: main already carries a migration 57, the SeaweedFS volume-size-limit one, and its targetVersion is already 58. The whole shape of this retirement was worked out here rather than anywhere reusable, so docs/deprecating-applications.md writes it down as a procedure: how to pick between migrating onto a successor and freezing, what the prune takes with it, and what the migration owes a cluster on each branch. The next application to go should not have to rediscover it. Assisted-by: LLM Signed-off-by: Timofei Larkin <lllamnyp@gmail.com> |
||
|
Yan Bondarenko
|
55a2918fcb |
fix(kubeovn): retry VM migration setup after scheduling
Signed-off-by: Yan Bondarenko <202671653+yankawai@users.noreply.github.com> |
||
|
Aleksei Sviridkin
|
d3e228fed8 |
test(opensearch): roll a single-node cluster held yellow by a replica
The roll onto the new mount plan timed out on some runs and passed on others with identical content. What differed was cluster health at the moment the listener moved: on one data node an index that asks for a replica stays yellow for good, and the operator restarts a pod only once its health gate opens. The security audit log creates such an index on its first audited event, and whether that had happened yet was down to timing. Create the index in the suite so the roll always has to get through a yellow cluster, and read the health back so the step cannot keep passing while it proves nothing. The operator logs that health held a restart back, never what the health was, and cluster health is not a Kubernetes object. The catch block now reads it from the node, together with the shard table and the allocation explanation. Signed-off-by: Aleksei Sviridkin <f@lex.la> Assisted-by: LLM |
||
|
Alexey Artamonov
|
353f37e4fe |
test(kafka): widen the fresh-create e2e cleanup budget for KRaft
The fresh-create Kafka suite relied on the shared 5m cleanup default, but a KRaft cluster tears down broker and controller node pools, the entity operator, and their PVCs — more work than a single ZooKeeper node. Under a contended CI run (concurrent Postgres/OpenSearch/Redis suites) that teardown overran 5m and failed the suite after every assertion had already passed, so cleanup latency, not the test, was deciding the verdict. Give this suite its own 8m cleanup timeout; the migration suite runs in its own namespace and is unaffected. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Andrey Kolkov
|
6e18ceef53 |
fix(kafka): fail closed when the migration hook cannot list Kafka CRs
The "free the kafka pool" gate counted other ZooKeeper clusters through a `kubectl ... | awk` pipeline under `set -eu` without `pipefail`, its kubectl error dropped by `2>/dev/null`. A failed read then counted zero and kept the shared pool, stranding any co-namespaced cluster still waiting for the name. Separate the read status from the count, mirroring the drain read below, and cover it with a bats case. Assisted-By: LLM Signed-off-by: Andrey Kolkov <androndo@gmail.com> |