mirror of
https://github.com/cozystack/cozystack.git
synced 2026-11-06 03:57:52 +09:00
feat/harbor-multiarch
449
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
|
|
084ac47237 |
feat(build): build published images for amd64 and arm64 by default
PLATFORM defaulted to empty, so buildx built for the host alone. On an arm64 workstation `make image` published an arm64-only digest, which amd64 nodes then fail to pull; the cilium pin shipped exactly that until it was rebuilt by hand. A build now targets linux/amd64 and linux/arm64 unless PLATFORM is set, and the digest a package stamps is the index digest. LOAD=1 keeps building for the host only. The classic docker image store cannot load a multi-platform index, and loading is the documented local workflow. This changes local pushes: a `make image` on the default docker driver without the containerd image store now fails with "Multi-platform build is not supported for the docker driver", where it used to build for the host. Such a build needs a docker-container builder (BUILDER), or LOAD=1 or an explicit PLATFORM for a single architecture. talos and testing pin linux/amd64 with `override`, placed before the include because common-envs.mk expands BUILDX_ARGS immediately. The Talos assets are amd64: matchbox bakes the kernel and initramfs into its image, and the e2e sandbox boots the disk under qemu-system-x86_64. An arm64 variant of either would still serve or boot amd64 files. CI pins linux/amd64 on every image build. Nothing there sets up QEMU, fork builds export a single-platform archive, and tags.yaml builds on the default docker driver, so the default would break or emulate those jobs. Native arm64 builds belong in a leg of their own. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
|
368d00b5d4 |
test(e2e): remove unused monitoring suites and audit BATS runners (#4019)
## What this PR does I removed the two monitoring OIDC BATS suites that had no runner after the Chainsaw migration, and brought the runner audit and wrapper OIDC test from [#3778](https://github.com/cozystack/cozystack/pull/3778). Also added checks for audience mapper and `OIDC_MODE=System`. The named `KeycloakRealmGroup` test documents coverage the document-count and kind assertions already provided. Runner audit reports unused BATS suites, missing files in runner paths, and unresolved Make variables (including `$(E2E_SUITE)`). Live Monitoring API admission, Grafana operator reconciliation and mode transitions are not tested here. ### Validation - `make -j4 unit-tests SHELL=/usr/bin/dash` passed. - Monitoring Helm tests: 53 system-chart tests and 10 wrapper-chart tests passed. - Runner audit: 12 cases passed under dash and Bats 1.14.0. The full shell syntax and BATS sweep passed under dash too. - Three chart mutations and seven runner mutations failed as expected. ### Downstream repositories - [x] No downstream repository is affected by this change ### Release note ```release-note NONE ``` |
||
|
|
03e2da5805 |
feat(postgres,mariadb)!: drop plaintext user passwords (#4078)
## What this PR does Removes the plaintext `users[].password` field from the `apps/postgres` and `apps/mariadb` charts (and the mariadb `root` account), so managed-database passwords can no longer be pinned in tenant values or committed to git. Passwords are always chart-generated into the `<release>-credentials` Secret and preserved across reconciles via `lookup`, matching the model `redis`/`valkey` already use. This closes the "User passwords" item of the portal upstream epic (aenix-org/cozyportal#1060). The generated password never lands in the init-script Secret or the SQL it runs: the postgres init-job reads each user's password from the mounted `<release>-credentials` Secret at run time and applies it through a psql variable (`ALTER ROLE … WITH PASSWORD :'pw'`), so it does not linger in the init-script manifest a tenant can read. It does live in cleartext `stringData` in the `<release>-credentials` Secret itself, which is a chart manifest and therefore kept in Helm release history for `MaxHistory` revisions like any chart-generated secret — that residue is exactly why rotation cannot be built on the chart (see below). A `password` or a `passwordRotation` key left over in a tenant's values is accepted-but-inert and draws an admission warning, so an operator editing it is told it has no effect rather than getting a silent 200. Backup/restore into a copy keeps the target's chart-managed credentials working: the CNPG driver clears `bootstrap.enabled` once recovery is healthy so the init-job re-applies the generated passwords onto the recovered roles, and the MariaDB driver excludes `mysql.global_priv` from the dump (`ignoreGlobalPriv`) for backups captured from this release onward, so restoring one of those into a copy does not clobber the target's chart-managed users or `root`. Excluding the grant table has a flip side, and it applies to every MariaDB restore, not only a to-copy one: only accounts declared through `User` CRs (the chart's `users` map) are captured, so an account created out of band directly in MySQL is in no backup and a restore brings back its data and `mysql.db` grants but not the account itself. This is also a backup-side fix only: a restore from a backup taken **before** this change still replays the source's `mysql.global_priv` and overwrites the target's users and `root`, and `root` then needs an out-of-band reset (the operator applies `rootPasswordSecretKeyRef` only at datadir bootstrap, so there is no in-band path). See `docs/operations/backup-classes.md` for the convergence window, the out-of-band-account limitation, and this old-backup caveat. Password **rotation** is deliberately **not** in this PR. A chart-rendered credentials Secret keeps its cleartext in Helm release history for `MaxHistory` revisions, so a rotation built on it cannot revoke a leaked password; rotation is being reworked as a controller that owns the Secret — design proposal in cozystack/community#72. Migration is seamless: existing releases keep their current passwords on upgrade (the value already lives in `<release>-credentials`, which `lookup` re-reads and preserves); a `password` still present in a tenant's values is ignored, so no existing HelmRelease fails validation. Verification: `helm unittest` passes for both charts (postgres 87, mariadb 126, including render-time username guards that reject any name k8s would later reject as an invalid Secret key or User resource name); `go build ./...`, `go vet`, the backup-controller tests and the apps-apiserver tests are green; `make generate` was run with the CI-pinned `cozyvalues-gen` v1.6.0 so `values.schema.json`, the READMEs, the API types, and the `cozyrds` ApplicationDefinitions are all in sync (the branch was rebased onto `main`, which reconciled cleanly with the read-replica autoscaling feature that landed there). ### Screenshots N/A — no UI change. ### Downstream repositories - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: affected — `content/en/docs/next/cozystack-api/go-types.md` is hand-written and still shows `Password:` in the Go examples, and the managed-app reference pages regenerate from these chart READMEs, so both document `users[name].password` until refreshed (not yet filed). - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: affected — the provider hand-maintains the postgres/mariadb schemas and per-field defaults; this PR removes `users[].password`. A follow-up issue/PR is needed there (not yet filed). `terraform-provider-cozystack` and `website` are reached by this diff; both boxes are left unticked because the follow-ups are not yet filed. Same-repo future work this change suggests: the controller-owned password rotation (design in cozystack/community#72), and extending the "passwords only in the Secret" model to the other engines that still accept plaintext (`clickhouse`, `mongodb`, `nats`, `opensearch`, `rabbitmq`, `vpn`). ### Release note ```release-note feat(postgres,mariadb)!: passwords for managed PostgreSQL and MariaDB users can no longer be set via `users[].password` — they are always auto-generated and stored only in the `<release>-credentials` Secret, never rendered into the init-script or the SQL it runs. Existing releases keep their current passwords on upgrade; a `password` (or a `passwordRotation`) left in values is ignored with an admission warning. Password rotation is deferred to a follow-up controller design (cozystack/community#72). MariaDB backups now exclude the `mysql.global_priv` grant table (`ignoreGlobalPriv`), so a restore recreates only accounts declared through the chart's `users` map — an account created out of band directly in MySQL is not captured and must be recreated after a restore. ``` |
||
|
|
0cb37dba9e |
docs(release): add changelog for v1.6.3 (#4077)
This PR adds the changelog for release `v1.6.3`.
✅ Changelog has been automatically generated in
`docs/changelogs/v1.6.3.md`.
|
||
|
|
564f540f46 |
fix(kubernetes): render the OIDC UID header only from v1.33 (#4407)
## What this PR does Stacked on #4398; review the diff against `fix/kamaji-pin-hash-reset`. The tenant Kubernetes chart now renders `--requestheader-uid-headers=X-Remote-Uid` only from an effective Kubernetes version of v1.33, where `RemoteRequestHeaderUID` is beta and on by default. It no longer renders a `--feature-gates` term of its own on any version. On v1.32 it used to add a term enabling that gate, which is alpha there, whenever it could read the operator's entries and they left the gate unset; no release tag contains that path, so dropping it now changes nothing a user has run. The chart still reads `--emulated-version`, because emulation moves the effective version: `--emulated-version=1.32` on a v1.35 tenant puts the gate back to alpha-and-off, and the header flag is not rendered there. It still reads the operator's `--feature-gates` entries for an opt-out (`RemoteRequestHeaderUID=false`, or `AllBeta=false` with no explicit `=true`), so the header flag never lands next to a disabled gate. `AllAlpha` is no longer read, since it says nothing about a beta gate. An unquoted `--emulated-version=value` entry whose kube version has a non-zero patch component now fails the render and names the entry. The rule mirrors upstream rather than the regex in #4396: `toVersionMap` parses each value with `version.Parse` and refuses only `ver.Patch() != 0` (`staging/src/k8s.io/component-base/compatibility/registry.go` lines 320-326 at v1.35.0, the same code in `staging/src/k8s.io/apiserver/pkg/util/version/registry.go` at v1.31.0). `version.Parse` takes `^\s*v?([0-9]+(?:\.[0-9]+)*)(.*)*$` (`versionMatchRE` in `k8s.io/apimachinery/pkg/util/version/version.go`, identical at v0.31.1 and v0.35.0). So `1.32`, `v1.32`, `1.32-rc`, `1.32.0` and `1.32.0-rc` all start an apiserver emulating 1.32, and `1.32.4` and `1.32.4-rc` do not. A `^[0-9]+\.[0-9]+$` check would have refused the first four. The chart extracts the numeric run with the same pattern, reads the minor from it, and fails only on a non-zero third component. A quoted value is still left unread. Two deliberate narrowings: - The chart no longer refuses a mix of bare and `kube:`-prefixed `--feature-gates` terms. Once the chart writes no gate term, the spelling of the operator's terms no longer concerns anything it renders. The apiserver still refuses the mix (`component-base/compatibility/registry.go:405` at v1.35.0), and the chart now passes it through like the other fatal gate values it already leaves to the apiserver: unknown names, unparseable values, quoted terms. - On v1.32 the chart now neither renders nor judges the UID flags, as it already did on v1.31. An operator's own `--requestheader-uid-headers` entry without the gate reaches an apiserver that refuses it, which is how every released chart version behaved. A cluster built from `main` that already runs a v1.32 tenant with OIDC on loses the chart's `--requestheader-uid-headers` and gate term on the next render; no release carried them. That holds only while no release is cut from `main` before this merges; a release that carries the v1.32 gate term first would turn this into a breaking change for v1.32 tenants, whose migration is to write both flags into `extraArgs` themselves. An element such as `kube=1.32=x` is now split at its first `=` only, the way `toVersionMap` splits it with `SplitN(compVer, "=", 2)`. The chart used to skip that element, which the apiserver reads as 1.32, and so rendered the header flag against an emulated 1.32. That gap predates this branch. Rebased onto #4398 at `fb2e57c3b`, which reads a two-element `--feature-gates` and `--emulated-version` separately. That behaviour carries over unchanged under the v1.33 floor: behind a hidden `--emulated-version` only an explicit `RemoteRequestHeaderUID=false` is refused beside a header entry, while a blanket `AllBeta=false` waits for a reading, because a lower emulated version can make the gate alpha again; nothing is refused as an opt-out behind a hidden `--feature-gates` value; the empty-element refusal applies behind both; and behind either one `X-Remote-Uid` is added only beside a uid-headers list of the operator's. The README breaking-changes note from #4398 now says the chart refuses neither the mixed gate spelling nor two kube emulated versions. In `docs/oidc-tenant.md` the paragraphs that list refusals now name the managed-flag move and refusal from #4398 and no longer carry a count, and the two-element paragraph now says what the chart renders behind a hidden value. Tests: `helm unittest` goes from 308 on #4398 to 298. 26 cases are deleted. 25 of them pinned the v1.32 gate term, `AllAlpha` handling or the mixed-spelling refusal. One emulated a version below 1.32, which the below-floor emulation case now covers. v1.32 cases that tested parsing rather than the gate are retargeted to v1.34. Sixteen cases are added: the patch-level refusal, the version parsing below the floor and above it (on v1.35 emulating `1.33-rc`, `v1.33`, `1.33.0` and `kube=1.33=x`, where only a correct minor renders the header), the first-`=` split, a pin that a mixed gate spelling now passes through, emulation landing exactly on 1.33, and a v1.32 opt-out beside a header entry passing through untouched. Coverage notes, not applied: no case pins that the patch-level refusal stays off with `spec.oidc.mode: None`, where the block that holds it does not run. The case "splits a component element at its first equals sign only" pins that the kube element of `kube=1.32=x` is read. It does not pin the split itself, because the version pattern ignores what follows the numbers either way. The `extraArgs` field description still calls the field the escape hatch for apiserver flags unrelated to OIDC. It is unchanged here, and it stays accurate for the v1.32 advice in the guide, because the UID flags belong to the aggregation layer rather than to OIDC. Optional, not applied: on v1.32 the chart could refuse an operator's `--requestheader-uid-headers` entry that comes without the gate, since the gate is off by default there and the apiserver will not start. The same skip also drops two refusals the branch history had at 1.32, a header entry beside `RemoteRequestHeaderUID=false` and a header list ending in a comma; both now reach an apiserver that refuses them. It stays out so that v1.32 matches v1.31 and the released charts, where the chart judges none of these flags. Not applied: for a value with a zero-prefixed major such as `01.32.4`, the patch refusal suggests `01.32`. The apiserver refuses both values over the leading zero, so the configuration fails either way and only the suggested fix is off. Why the patch refusal stays while the mixed-spelling refusal goes: the patch refusal is about `--emulated-version`, which the chart reads itself to decide what to render. The mixed spelling concerns `--feature-gates`, which the chart no longer writes to. Not split out: the `--emulated-version` parsing and the patch-level refusal share the fix commit with the floor move, because both change how the same value is read. The change also removes a comment from `hack/e2e-chainsaw/_lib/run-kubernetes.sh` that became false, so select-e2e runs the full suite for this PR. Default path: renders of the parent and this branch through `tests/values/common.yaml` and `tests/values/oidc-system.yaml` at v1.31 to v1.35 are byte-identical, once the randomly generated Kamaji token fields are masked, except for OIDC System at v1.32, which loses exactly the two UID lines. ### Screenshots Not a 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 only field that changes is the description of `version`, which returns to its earlier wording. The website's kubernetes reference page is regenerated from the README on the next tag, and the Terraform provider keeps its own copy of that description, which already reads "Kubernetes major.minor version to deploy.". Closes #4373 Closes #4396 ### Release note ```release-note fix(kubernetes): the tenant chart renders the aggregation-layer UID header flag with OIDC on only from an effective Kubernetes v1.33, and never renders a --feature-gates term of its own. v1.32 tenants render no UID flags, which matches every released version, so nothing regresses. This supersedes the v1.32 clause of the #3539 release note. An unquoted --emulated-version entry with a patch-level kube version such as 1.32.4, which the kube-apiserver refuses, now fails the render and names the entry. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * OIDC-enabled clusters on Kubernetes 1.33 and later now receive the UID request-header setting without an added feature-gate flag. * Chart rendering now checks Kubernetes and Talos version compatibility and prevents rendering when worker deployments do not meet ownership or retention requirements. * **Bug Fixes** * Emulated Kubernetes versions with non-zero patch components now fail with a clear error; supported version formats are recognized more consistently. * **Documentation** * Updated version and OIDC guidance to reflect current chart behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
|
339cffe70f |
feat(kamaji)!: update to 26.9.4-edge and read every apiServer extraArgs entry (#4398)
## What this PR does BREAKING CHANGE: a Kubernetes application whose `controlPlane.apiServer.extraArgs` names a flag the tenant control plane now owns fails to render until that entry is removed, except that an `--enable-admission-plugins=` entry is moved into the new `controlPlane.apiServer.admissionControllers` and a `--kubelet-preferred-address-types=` entry into the new `controlPlane.kubelet.preferredAddressTypes`. Closes #3541. Updates the Kamaji build this repo ships and rewrites the part of the tenant Kubernetes chart whose correctness rested on the old one. The new pin is `26.9.4-edge`, which is what `make update` picks. Two upstream changes reach us. The tenant control plane no longer folds the kube-apiserver argument list into a map keyed by flag name, so entries supplied through the control plane spec arrive at the apiserver verbatim, duplicates included. And the reset that rewrites those arguments from scratch keys on a hash of the list rather than on its length, so swapping one flag for another no longer strands the old one on the running Deployment. The OIDC block in the tenant chart read only the last entry of each flag. That was right while earlier entries were dropped before the apiserver saw them, and it is wrong now. An operator whose `extraArgs` carried `--feature-gates=RemoteRequestHeaderUID=false` followed by an unrelated `--feature-gates` entry got `--requestheader-uid-headers=X-Remote-Uid` rendered next to a gate the apiserver has switched off, and the control plane would not start. A blanket `AllBeta=false` has the same shape, and so does an `--emulated-version` entry naming kube followed by one naming another component. The chart now reads every entry with the accumulation each flag performs. Occurrences of `--requestheader-uid-headers` append into one list, so where a list of the operator's own omits `X-Remote-Uid` the chart adds its own element beside it instead of refusing to render: the union is what the apiserver validates, and their header keeps working. Feature-gate terms collect per component and are applied in one pass, so the chart carries only its own term, placed last where it wins. Naming the kube component both bare and `kube:`-prefixed now fails the render, because the apiserver refuses that configuration and nothing the chart renders repairs it. `--emulated-version` accumulates too, and two entries naming kube stop the apiserver outright, so the chart renders nothing rather than pick a version that will not exist. The same upstream change narrows what `extraArgs` can do, and the chart now refuses the part that stopped working. Kamaji 26.9.4-edge sets a list of kube-apiserver flags itself and drops any `extraArgs` entry that names one before the apiserver sees the list, where `26.3.6-edge` merged `extraArgs` last and let the entry win. A tenant with `--enable-admission-plugins=NodeRestriction,AlwaysPullImages` loses both plugins on the first reconcile after the upgrade, with nothing to say so. The drop happens on the running control plane whatever the chart renders, so the chart cannot prevent it; what it can do is refuse to render such an entry, in every `oidc.mode`, and name it. The list mirrors `buildKubeAPIServerCommand` at the pinned tag for the control plane this chart renders (an etcd DataStore, konnectivity on, no `dataStoreOverrides`), and the `controlPlane.apiServer.extraArgs` description in the chart README carries it. A flag the control plane only defaults, such as `--authorization-mode` or `--requestheader-group-headers`, is still yours to set. `--enable-admission-plugins` gets a replacement: the new `controlPlane.apiServer.admissionControllers` passes a list through to the KamajiControlPlane `spec.admissionControllers`, which the control plane renders into that flag, and the application API checks each name against the enum the KamajiControlPlane CRD declares. The list is rendered only when non-empty, so a tenant that sets nothing renders exactly as before, and a non-empty list replaces the control plane's default set, as the `extraArgs` entry did under the previous build. `--kubelet-preferred-address-types` gets the same treatment: the list the chart used to hardcode on the KamajiControlPlane is now `controlPlane.kubelet.preferredAddressTypes`, which renders that list when left empty. Neither entry is refused where it can be moved: an `--enable-admission-plugins=` or `--kubelet-preferred-address-types=` entry is moved into its field while the field is empty, on every render, and left out of the rendered `extraArgs`, so a tenant carrying one keeps its setting across the Kamaji update with no edit and no ordering to get right. The entry is refused beside a non-empty field, when it names nothing, and in the two-entry spelling. A separate defect in the same package is fixed here. The TenantControlPlane schema reaches existing clusters through the `kamaji-crds` subchart's templated CRDs, which every release upgrade applies; `crds/` holds the copies a fresh install lays down first. Directory-based renders lost that schema entirely, `helm template`, `lint` and `package` alike, because the parent `.helmignore` matched the subchart's `hack/` directory at any depth and that is where the templates keep the schema they read with `.Files.Get`. Helm honours `.helmignore` for a directory and skips it for an archive, so the cluster never saw the difference and no local check could catch a schema defect. The patterns are anchored to the chart root and a unit test asserts the rendered CRDs carry their versions. The bump is not cleanly reversible, and that is the one thing to know before rolling it out. The args annotation holds a checksum once the new build has written it, and the old build reads that annotation with `strconv.Atoi`. The parse fails and `resetKubeAPIServerFlags` returns early, ahead of both the args reset and the annotation write that would put a number back. So after a rollback to `26.3.6-edge` the count-keyed reset is disabled on every TenantControlPlane: a later removal of an `extraArgs` entry no longer clears the flag from the running Deployment, and it stays that way until someone writes a number into `kube-apiserver.kamaji.clastix.io/args` by hand. Findings from a live upgrade on a dev cluster, none of them caused by the diff. The DNS-service-IP check moved out of a CEL rule on the CRD into the admission webhook (`internal/webhook/handlers/tcp_dns.go`), so it now depends on that webhook being reachable where the apiserver used to enforce it by itself. The manager's readiness probe targets port 8081 while the webhook serves on 9443, so a Ready pod does not prove 9443 accepts connections; with `failurePolicy: Fail` on the TenantControlPlane webhook, that window rejects writes rather than letting them through unvalidated. The readiness probe this build adds to kube-scheduler and kube-controller-manager survives a rollback, since the old build neither sets nor clears those two; that one is benign. Seven limits worth naming rather than leaving to be found. The mixed-spelling refusal quotes the entry carrying each spelling, so one entry holding both, `--feature-gates=Foo=true,kube:Bar=true`, is quoted twice in its own message; the refusal is right and the message reads oddly. The unit test that pins the rendered CRD schemas asserts on a count, so a CRD missing from the render altogether fails on an empty comparison rather than naming the CRD that went missing. There is one window during the upgrade itself: the chart used to fold an operator's own `--feature-gates` terms into the entry it rendered, because the old build would otherwise drop them, and it now renders only its own term. Nothing orders a tenant release after the Kamaji operator has rolled, so a `v1.32` tenant that sets gate terms of its own with OIDC on can lose them until the new operator reconciles it, which it then does on its own. The same window takes a `--requestheader-uid-headers` list of the operator's own that lacks `X-Remote-Uid`: the chart now renders its own entry beside it instead of refusing, and the old operator keeps one value per flag name, the chart's, so the operator's header is dropped until the new operator reconciles. Which entry an error message quotes is pinned only where one entry is eligible. Three variables choose between equally eligible entries, the gate dictionary taking the last setting for a name and the uid-headers ones the first, and no test distinguishes first from last when two entries qualify, so a future edit could make a message name the wrong entry of the two without a test noticing. What the chart renders and what it refuses is pinned exhaustively; it is the provenance in three message strings that is not. The block is deliberately inconsistent about two shapes that stop the apiserver equally hard: two `--emulated-version` entries naming kube make it withhold silently, while a kube-prefixed and a bare gate spelling in the same configuration make it refuse the render. The refusal is right where the apiserver rejects the configuration whatever the chart does; the withholding is right where the chart cannot read a version it must render against. They are two rules, not one, and a third that covers both is fair to ask for. Moving the refusal out from under those conditions so it fires everywhere is the obvious third rule and it is not free: the apiserver refuses the mix on `v1.31` too, but from `k8s.io/apiserver/pkg/util/version` rather than from the component-base `componentGlobalsRegistry.Set()` the message names, so hoisting it means rewriting that sentence as well. Reading a flag and its value written as two entries would be better than withholding for it, and is a change of behaviour rather than a correction, so it is not here. And two sentences about `--emulated-version` in `docs/oidc-tenant.md` sit thirty lines apart and read as a retraction in sequence, though each is true at its own layer: `toVersionMap` accepts an element naming a foreign component, and the registry rejects it later. What this leaves open, by name. The guard for a valueless flag at the end of `extraArgs` covers `--requestheader-uid-headers`, `--feature-gates` and `--emulated-version` only; any other flag that takes a value swallows the appended `--authentication-config` the same way, which is an input mistake the chart does not look for. The managed-flag refusal matches names as written, as Kamaji does: an underscore spelling such as `--secure_port` passes both, and the apiserver normalises it to `--secure-port`, where the operator's value wins as the later one, the same as before this change. Rendering `--authentication-config` ahead of the operator's entries would close that class for OIDC, since only the UID flags would then follow them; it changes the rendered order, and so rolls every OIDC tenant that has `extraArgs` of its own, and belongs in a separate change after the OIDC floor move. One pre-existing comment needs checking: the note above `extraArgs` in `templates/cluster.yaml` says that omitting the field would leave the live array untouched under JSON merge patch, which reads against Helm's `createPatch`, where a key the previous manifest carried and the new one does not is patched to null; what that comment was guarding against is still to be verified. The KamajiControlPlane CRD accepts only the plugin names it lists, so a plugin outside that list that the apiserver leaves off by default can no longer be enabled; that limit is Kamaji's, since it owns the flag and restricts the field. A name moved out of an `extraArgs` entry is not checked against that list by the chart, so one outside it fails when helm-controller applies the KamajiControlPlane, with an error that names it, rather than at render; the chart keeps no copy of the list to check it with. A repeated address type in a moved entry is dropped, keeping the first place it appears, because the field is a set. With `oidc.mode: None` the chart does not read `extraArgs` for the OIDC flags at all, and two shapes regress with the bump there: a kube feature-gate term written both bare and `kube:`-prefixed across entries, and two `--emulated-version` entries naming kube. The old build kept one entry per flag, the new one passes both, and the apiserver refuses both at startup. Next to a flag and value written as two entries the chart keeps the refusals that hold whatever the hidden value is, but it does not read positions: a hidden `--feature-gates` term keeps the opt-out refusal off even when it sits before the explicit opt-out and so cannot undo it. What the live run could not exercise: the konnectivity datapath, because the stand had no tenant workers, and `--requestheader-uid-headers`, which comes from the tenant Kubernetes chart and no image swap reaches. Both are covered by render and unit tests only. One shape the old collapse was quietly repairing, with OIDC on, needs a guard now that it is gone. A bare `--requestheader-uid-headers` in `extraArgs` used to be overwritten by the entry of the same name the chart renders, because the collapse kept one value per flag name; the same held for a bare `--feature-gates` on `v1.32`, where the chart renders a gate entry too. A bare `--emulated-version`, or a bare `--feature-gates` on any other version, had no entry of the chart's to be overwritten by and was emitted bare in sorted position, where it took the next flag as its value, so it never worked. Without it those entries reach the apiserver as written, and the apiserver gives a valueless long flag the next argument whatever that argument looks like. What follows the operator's last entry is the `--authentication-config` the chart appends, so the flag swallows it and the AuthenticationConfiguration is never applied. On `--requestheader-uid-headers` the path is accepted as a header name and the control plane comes up with OIDC silently off, failing every login for that tenant with nothing in the cluster to say why; the other two reject it and the apiserver exits. The chart refuses to render that shape and says which entry to fix. Two things in the image build fall out of the bump. The datastore deletion fix carried here as a patch landed upstream verbatim, so its patch no longer applies, and one failing patch fails the whole apply of the directory. The Go module overlay is obsolete for the opposite reason: this tag already carries `golang.org/x/net`, `golang.org/x/crypto` and `go.opentelemetry.io/otel/sdk` above the versions the overlay forced, so keeping it would downgrade all three. What I checked: `helm unittest` passes at 307 cases for the package, with the OIDC suite carrying every shape above, and the image builder stage builds end to end at the new tag with the one remaining patch. Kamaji's own support for Kubernetes 1.36 and 1.37 does not move the tenant version enum here; that is a separate update of the `kubernetes` package. No image reference in `packages/system/kamaji/values.yaml` is touched, since CI stamps that digest. ### Screenshots No UI changes. ### Downstream repositories I walked the trigger map file by file against the diff. The chart templates, the vendored Kamaji chart, the image build and the docs reach nothing downstream. The one generated file that reaches downstream is `packages/apps/kubernetes/values.schema.json`, which gains `controlPlane.apiServer.admissionControllers` and `controlPlane.kubelet.preferredAddressTypes` and a changed `extraArgs` description; both new fields are optional with defaults, so a stored Kubernetes object needs no change, no field is removed or renamed and no version enum moves. The new fields are the one trigger the map fires on: the Terraform provider keeps a hand-written schema. `packages/apps/kubernetes/README.md` feeds the website's managed-apps reference pages, which a bot regenerates on a stable tag. - [ ] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [x] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: model `controlPlane.apiServer.admissionControllers` and `controlPlane.kubelet.preferredAddressTypes` on the kubernetes resource (schema, model, expand/flatten). - [ ] [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(kamaji): update Kamaji to 26.9.4-edge. Tenant kube-apiserver arguments are now derived from a hash of the control plane's extraArgs instead of from its length, so on the first reconcile after the upgrade every tenant control plane rewrites those arguments from scratch and rolls once. The rebuilt list also orders them differently, Kamaji's own flags sorted first and the operator's kept verbatim after them, so the roll happens whether or not the tenant sets any extraArgs. A tenant with OIDC enabled takes a second roll when the chart's own flags land. Rolling the Kamaji image back to an earlier build does not restore the old behaviour: the older code reads that annotation as a number, gives up when it finds a checksum, and from then on never clears a flag you remove from extraArgs, on any TenantControlPlane, until the annotation is set back to a number by hand. The tenant Kubernetes chart reads every extraArgs entry the way the apiserver accumulates them: a --requestheader-uid-headers list that omits X-Remote-Uid now gets the chart's own element beside it instead of failing the render, a feature gate set in an earlier entry is honoured, and naming the kube feature-gates component both bare and kube:-prefixed fails the render, because the apiserver refuses that configuration outright. Writing --feature-gates or --emulated-version as a flag and a value in two separate entries now keeps the chart from rendering UID flags on its own, because an entry the chart cannot read may hide an opt-out; a --requestheader-uid-headers list of your own still gets X-Remote-Uid beside it, and write --flag=value to get the rest back. Action required for tenants that pass their own apiserver flags: Kamaji now sets a list of flags itself and drops a controlPlane.apiServer.extraArgs entry naming one, where the previous build let that entry win, and the Kubernetes application now refuses to render such an entry, except for the two it moves into a field. A tenant carrying one of the others stops reconciling after the upgrade, and no later change to that tenant applies, until the entry is removed; the entry no longer has an effect either way. The list is in the description of controlPlane.apiServer.extraArgs. An --enable-admission-plugins=value entry is moved into the new controlPlane.apiServer.admissionControllers, a list that replaces the control plane's default set as the entry did, and a --kubelet-preferred-address-types=value entry into the new controlPlane.kubelet.preferredAddressTypes, while that field is empty; nothing has to be edited for them. Before upgrading, list the affected entries with: kubectl get kuberneteses.apps.cozystack.io --all-namespaces --output json | jq --raw-output '["--advertise-address","--client-ca-file","--enable-admission-plugins","--service-cluster-ip-range","--kubelet-client-certificate","--kubelet-client-key","--kubelet-preferred-address-types","--proxy-client-cert-file","--proxy-client-key-file","--requestheader-allowed-names","--requestheader-client-ca-file","--secure-port","--service-account-key-file","--service-account-signing-key-file","--tls-cert-file","--tls-private-key-file","--etcd-servers","--etcd-cafile","--etcd-certfile","--etcd-keyfile","--etcd-prefix","--etcd-compaction-interval","--egress-selector-config-file"] as $owned | .items[] | .metadata as $m | (.spec.controlPlane.apiServer.extraArgs // [])[] | select((split("=")[0]) as $f | $owned | index($f)) | "\($m.namespace)/\($m.name): \(.)"' Entries it lists for --enable-admission-plugins or --kubelet-preferred-address-types need a change only when written as two entries, when they name nothing, or next to a non-empty field of their own. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Configure tenant control-plane admission controllers and kubelet preferred address types. - Choose a cert-manager issuer for webhook certificates; self-signed certificates remain the default. - **Bug Fixes** - Improved OIDC argument handling for repeated, split, quoted, and conflicting settings, with clearer render-time errors. - DataStores are recognized as unused when no tenant control planes reference them. - **Validation** - DataStore endpoints are checked for length, count, and IPv6 address formatting. - Chart-managed tenant control-plane arguments are rejected when supplied through `extraArgs`. - **Documentation** - Updated Kubernetes OIDC and certificate issuer guidance. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
|
ce74fdeea9 |
docs(backups): describe restored credentials and pre-upgrade residue
Say that a restored copy converges its application passwords shortly after recovery, that MariaDB backups leave out the grant table, and that dropping the password field does not rewrite copies made before the upgrade: old Backup snapshots and Helm revisions still hold the password that stays live. The credentials projector comment counts only the engines that still take a password from values. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
|
a92e89e969 |
docs(kubernetes): describe the OIDC UID header from v1.33
The tenant OIDC guide, the version field description and the e2e helper comment still described a --feature-gates term rendered on v1.32. Left as they were, they would tell an operator that the chart switches that alpha gate on, which it no longer does. The guide now says what a v1.32 tenant has to write itself to get the UID, and which --emulated-version values fail the render, so an operator who meets that refusal finds it explained. The breaking-changes note in the README said the chart refuses the mixed gate spelling while OIDC is on; with that refusal gone it would promise a render error that no longer comes. The version description returns to its earlier wording in values.yaml and in the update-versions.sh template, because that template rewrites the line on every version bump and would otherwise bring the old text back. make generate carries it into the schema, README, API types and ApplicationDefinition. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
|
4e9f83ac0d |
fix(kubernetes)!: move or refuse extraArgs flags the control plane owns
From 26.9.4-edge the tenant control plane sets a list of kube-apiserver flags itself and drops any extraArgs entry naming one before the apiserver sees the list; 26.3.6-edge let such an entry win. An existing tenant that set, for example, its own admission plugins through extraArgs would lose them on the first reconcile after the upgrade with nothing to say so, and a flag written with its value in a separate entry leaves that value behind as a positional argument the apiserver refuses. Two of those flags have a KamajiControlPlane field the control plane renders them from. An --enable-admission-plugins=value or --kubelet-preferred-address-types=value entry is moved into that field while the field is empty, and left out of the rendered extraArgs, so the tenant keeps its setting across the upgrade without an edit. The move is computed from the values on every render; the last entry wins, as it did while the control plane kept one entry per flag name, and a repeated address type is dropped because that field is a set. The post-upgrade Job that patches extraArgs onto the KamajiControlPlane reads the same list. Every other entry naming an owned flag is refused at render time in every OIDC mode, since the chart cannot stop the drop and the refusal is where the operator learns of it. So are the two moved flags beside a non-empty field, with a value that names nothing or is quoted, and in the two-entry spelling. The list mirrors the Kamaji builder for the shape of control plane this chart renders, and the extraArgs description names the same flags. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
|
88264216b7 |
docs(kubernetes): restate the OIDC UID flags for the merged reading
The migration note told operators to move the extraArgs entry count in the same edit so the running control plane would rewrite its apiserver arguments; the control plane compares a hash of the list now, so any change already forces the rebuild and no second edit is owed in either direction. The paragraphs describing a value that survives only as the last entry of its flag, and an empty value reaching the apiserver as a bare flag, describe a build that is no longer shipped. What replaces them is the accumulation each flag actually performs: a uid-headers list the apiserver validates as the union of every entry, feature gates settled across entries with the last term winning and one spelling of the kube component throughout, and an emulated version that two entries cannot both name. One contract the earlier text sold is narrowed to what the chart keeps. The render checks run only where the chart renders something, so the three readings it refuses to guess at switch them off as well, and a shape that would otherwise be refused passes through to a control plane that does not start. A flag and value written as two entries is a narrower case, and the page says which checks hold next to one. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
|
c55ab268e2 |
test(monitoring): remove orphaned OIDC BATS suites
The two pre-Chainsaw monitoring OIDC suites have had no invocation site since the app suite migrated to Chainsaw. Remove them instead of carrying dead tests that force the selector to run every live suite. Keep the existing selector and capture guards for the historical path shape, and update their comments now that the directory is empty. Retain a named chart assertion for the no-KeycloakRealmGroup ownership invariant; the existing count and kind assertions already enforce the same behavior. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
|
180030be72 |
fix(api): refuse only what a name declaration actually breaks
Three cases refused creates of a kind, or were documented as refused, without the declaration being at fault. A schema that is not a JSON object was reported as an invalid x-cozystack-name, although such a schema has no declaration to blame, and the spec schema's own loader already reports it. It now declares nothing, so creates keep working and a kind with a legacy cap keeps it. Any unknown key inside a declaration took the kind offline, so a generator adding a title or an example would do the same. Keywords that constrain nothing, the JSON Schema annotations and x- extensions, are now ignored. A constraint keyword the server does not implement is still refused, because accepting it would drop it silently. The docs said a declaration no name satisfies is refused. That holds for lengths only: a pattern is checked to compile, not to match anything. The docs now say so. The parser moves to pkg/apis/apps/validation so the API review gate can reuse it rather than keep a copy of its rules. Assisted-by: LLM Signed-off-by: Timofei Larkin <lllamnyp@gmail.com> |
||
|
|
146e06e75e |
feat(api): take Application name limits from the application's own schema (#4367)
## What this PR does
The aggregated API server capped two kinds' names by knowing what their
charts render. A `Kubernetes` cluster name was held to 32 characters
because every worker pool is a `KubernetesNodes` release named
`kubernetes-nodes-<name>-<pool>`, and the default `md0` pool has to fit
the 53-character Helm release name limit. A `Kafka` name was held to 44
because the KRaft controller pod `kafka-<name>-c-<hash>-<id>` has to fit
the 63-character DNS-1123 label limit. The arithmetic is right, but it
is the charts' knowledge living in the platform: the next application
with a sub-resource budget needs another `if r.kindName == ...` branch,
and an application served from outside this repository — the whole point
of `ApplicationDefinition` — cannot get one at all.
An application now declares the limits on its own name in its own
schema, and the server applies whatever it finds there for whatever kind
the definition describes. The chart's `values.yaml` carries a `## @name`
annotation, `cozyvalues-gen` v1.7.0 publishes it at the root of
`values.schema.json` as `x-cozystack-name`, and `hack/update-crd.sh`
carries it into the `ApplicationDefinition`:
```yaml
## @name {string} - Cluster name. Every worker pool is a separate KubernetesNodes release named `kubernetes-nodes-<name>-<pool>`, so the name must leave room for the default `md0` pool inside the 53-character Helm release name limit.
## @maxLength 32
```
```json
"x-cozystack-name": {
"type": "string",
"description": "Cluster name. Every worker pool is a separate KubernetesNodes release named `kubernetes-nodes-<name>-<pool>`, so the name must leave room for the default `md0` pool inside the 53-character Helm release name limit.",
"maxLength": 32
}
```
`maxLength`, `minLength` and `pattern` are honoured; `pattern` is
unanchored, as JSON Schema specifies. A declaration only tightens:
`prefix + name` must still fit the 53-character Helm limit. A rejected
name is answered with the chart's own description, or with "limit
declared by the <Kind> application" when the chart gives none.
The schema is untrusted input — definitions from other repositories are
written by hand — so a declaration that cannot be enforced as written
(not an object, a type error, an unknown keyword such as a misspelled
`maxLenght`, `maxLength` below 1, a negative `minLength`, `minLength`
above `maxLength` or above the Helm budget, a pattern that is not RE2)
refuses every create of that kind with `configuration error: the <Kind>
ApplicationDefinition declares an invalid x-cozystack-name: …`, and logs
the same at start-up. Dropping the declaration would admit names the
chart cannot render; applying it would reject every name and blame the
user. Other kinds are unaffected, and existing objects of the affected
kind can still be read, updated and deleted.
An upgrade brings `cozystack-api` up before `kubernetes-rd` and
`kafka-rd` re-render (`kubernetes-rd` depends on `cozystack-engine`), so
the new server can briefly read a definition that predates its
declaration. For exactly that case — a definition that declares nothing
— the two previous caps stay in `legacyNameCaps` as a floor, to be
removed once no supported upgrade starts from such a definition. That is
the only per-kind knowledge left in the name checks.
Also in this PR:
- The `pre-commit` job pins `cozyvalues-gen` v1.7.0, the first release
that understands `@name`
([cozystack/cozyvalues-gen#33](https://github.com/cozystack/cozyvalues-gen/pull/33)).
- The API review gate now diffs the root `x-cozystack-name` of every
definition as `metadata.name`, so a chart that lowers its cap or adds a
pattern is reported like any other tightened field. On this PR it
reports the Kubernetes and Kafka caps as added; their effective limits
are unchanged.
- The migration 56 comment that pointed at the removed
`maxKubernetesClusterName` now points at the chart's declaration.
Not in scope, because none of it is a static limit a chart can declare:
the `Tenant` name pattern in `pkg/apis/apps/validation`, the computed
tenant-namespace length check (it depends on the parent namespace at
request time), the tenant quota hierarchy, and the Phase 2 removed-field
warning for `Kubernetes`.
### How it is tested
`TestNameCapComesFromTheShippedSchema` reads the shipped `kubernetes`
and `kafka` ApplicationDefinitions off disk, builds storage through
`NewREST` as the server does, and checks the cap and the chart's
explanation; it then rewrites `maxLength` inside the shipped schema and
checks the limit moves with it, which a cap held in code would not.
`TestInvalidNameSchemaFailsClosed` covers each malformed declaration
through the same path, `TestLegacyNameCapsCoverUndeclaredDefinitions`
pins the upgrade floor and that a declaration replaces it, and
`TestClassifyNameDeclaration` covers the gate.
### Downstream repositories
Two repositories are affected and neither follow-up is filed yet, so no
box is ticked.
- `cozystack/website` — `content/en/docs/next/development.md` documents
the `values.yaml` annotations and needs `@name`;
`content/en/docs/next/cozystack-api/application-definitions.md`
describes `openAPISchema` and needs `x-cozystack-name`.
- `cozystack/external-apps-example` — it pins no `cozyvalues-gen`
version and regenerates against whatever is current; nothing in it
declares a name constraint, so it is unaffected until it chooses to.
- [ ] 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(api): Application name limits now come from the application's own schema instead of being hardcoded per kind in the aggregated API server. A chart declares them with a `## @name` annotation in `values.yaml`, which cozyvalues-gen v1.7.0 publishes as `x-cozystack-name`; the API server enforces `maxLength`, `minLength` and `pattern` on `metadata.name` for any kind, answers a rejected name with the chart's own explanation, and refuses creates of a kind whose declaration it cannot enforce with a configuration error. The Kubernetes (32) and Kafka (44) name caps are unchanged in effect and are now declared by their charts.
```
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **New Features**
* Applications can declare minimum and maximum name lengths and
formatting patterns.
* Kubernetes cluster names are limited to 32 characters, and Kafka
cluster names to 44 characters, to meet generated resource naming
limits.
* Names that violate declared constraints are rejected with an
explanatory message. Tighter name constraints are also reported as
breaking changes.
* Declared maximum lengths cannot exceed the platform’s existing Helm
release-name budget.
* **Documentation**
* Added guidance on declaring and enforcing chart-specific resource-name
limits.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
|
||
|
|
1f30ac4598 |
fix(tests): let the mariadb waits cover a first boot (#3769)
## What this PR does Closes #3741 Several waits for a database HelmRelease to become Ready gave up before the release could: the mariadb, mongodb, rabbitmq and clickhouse backup walkthroughs waited 300 seconds, and both mariadb Chainsaw suites gave their HelmRelease-ready assert 6 minutes. A generated release carries a 10-minute install timeout (`pkg/cmd/server/start.go:91`), so a failed install was reported as a bare timeout, because the release had not yet recorded why it failed, and for mariadb a first boot the startup probe was still willing to wait for failed the harness instead. A wait on such a release has two floors and has to beat the larger. The first is the app's own first boot: cozystack-api leaves `spec.waitStrategy` unset and helm-controller defaults it to `poller`, so the install waits through kstatus, and kstatus maps the operator CR's `Ready=False` onto `InProgress`. For mariadb that boot may take the chart's 310-second startup budget (`startupProbe.failureThreshold: 30` in `packages/apps/mariadb/templates/mariadb.yaml` with the operator's probe defaults of 20 and 10); mongodb, rabbitmq and clickhouse set no startup probe. The second is the install timeout. At or below it the waiter is the one that expires, because its clock starts when the step runs and the install's only once the Application has been reconciled and the chart artifact fetched. The install timeout binds in every case here, so the walkthrough waits go to 660 seconds and the mariadb asserts to 11 minutes. Raising a wait inside a walkthrough spends the Chainsaw op that runs the script, and an op that runs out SIGKILLs the process group and loses the script's error. Each op therefore grows by exactly what the waits it reaches gained: mariadb's round-trip from 30 to 42 minutes, rabbitmq's from 30 to 42, mongodb's from 30 to 36 (its round-trip runs with `SKIP_RESTORE`, so only the source wait counts), and clickhouse's from 60 to 72, which still sits above the sum of every inner ceiling. That bounds a failure at a single wait, not a run that is slow at every stage and then fails late; bounding that needs a phase budget, not a larger literal. postgres keeps its 360-second waits and every walkthrough keeps its 300-second bucket wait: whether a suite pays the extra minutes on every red run is a per-suite call. The mariadb endpoint asserts keep their job. A freshly applied CR carries no conditions and kstatus reads it as current, so the install wait can end before any pod is up; an endpoint address closes that window. That is also why the `wait_for_field` on the CR stays in the walkthrough. `wait_hr_ready` now dumps every condition and the release history on timeout and on `Stalled=True`, instead of the single Ready message a scheduled retry can overwrite before anyone reads it. It also stops printing one `(none recorded)` line for different answers: a release that never appeared, a release deleted during the wait, a lookup kubectl could not answer, a failed history read and an empty history each say so, with kubectl's own stderr kept, and a release seen earlier still gets its dump when only the last lookup failed. The change goes to every byte-identical copy of the helper, in `examples/backups/{clickhouse,mariadb,mongodb,postgres,rabbitmq}/00-helpers.sh`, and they stay identical. `hack/mariadb-first-boot-waits.bats` pins the mariadb budgets against the tree. It derives both floors from their sources instead of pinning literals, so raising `failureThreshold` past the install timeout, or changing the install-timeout default, reds it; each source has a test that refuses to proceed when it cannot be read, including the ApplicationDefinition, whose two override annotations would each invalidate the derivation. It holds the round-trip op at a fixed remainder over the ceilings up to and including the source wait, bucket stage and S3 preflight included, and at a fixed slack once both mariadb waits are paid, so raising a wait without the op reds. The extractors are also tested against fixtures, because the tree cannot tell a correct extractor from a broken one. Beyond mariadb, the file runs every exit branch of `wait_hr_ready` in each copy against a stubbed kubectl, and pins the copies byte-identical. The population is every definition in the tree found by grep, minus two named exceptions held to still differ, so a new copy joins without editing the test and an exception whose copy converges goes red: - `examples/backups/redis/00-helpers.sh` is written in the compact one-line style of the rest of that file, so converging it rewrites its shape rather than applying the shared body. - `examples/backups/kafka-metadata/00-helpers.sh` is a helper of its own with the same diagnostic gap on timeout (it prints no reason at all), and closing that is a change to kafka-metadata. `docs/agents/e2e-testing.md` replaces the reviewer checklist's 5-6m standard and its list of exceptions, which the tree had outgrown, with the floor a wait has to beat and the rule that a longer wait carries its reason in-line. The derivation gets a section of its own, opening with the rule in one line. For a release an app chart renders itself the floor is `spec.install.timeout`, then `spec.timeout`, then helm-controller's 5m default, and the nested `<name>-system` releases never take cozystack-api's 10m. The harbor comments name the floor each of their asserts sits against, and the opensearch comment stops citing the removed standard. Adding `hack/mariadb-first-boot-waits.bats` does not escalate the E2E selection: `hack/[^/]+\.bats$` sits in `inert_config_pattern` in `hack/select-e2e.sh`, and `full_suite_pattern` escalates only `hack/e2e-*.bats`. On this changeset `hack/select-e2e.sh` selects `clickhouse harbor mariadb mongodb opensearch postgres rabbitmq`. What this costs is paid only on a failing run; the raised numbers are ceilings. Summing the timeouts stated on each suite's own steps, mariadb goes from 92m to 114m, clickhouse from 116m to 128m, rabbitmq from 67m to 79m and mongodb from 95m to 101m, before the untimed steps that fall back to the defaults in `.chainsaw.yaml`. clickhouse is the most expensive suite by that sum. All of them compete for the 215m `timeout-minutes` of the whole `e2e` job, which also covers image download, sandbox preparation and the Cozystack install, and a job cancelled at the cap loses both its log and `cozyreport.tgz`. This does not fix the failure that led here. In the run that prompted the investigation the mariadb replica never opened port 3306 at all, spending its whole 310-second budget inside `mariadb-install-db` before being killed, and no wait budget would have saved it. That defect is #3744. What this fixes is a second, independent defect found while reading the same code. One thing here is argued rather than measured: the mechanism is checked against helm-controller and kstatus, and every number is derived from the tree, but no run ID pins that it is what any particular red run hit. Both floors justify 660s whatever the cause was, so being wrong about the cause leaves slack rather than breakage. ### Not in this PR - This PR retires the mariadb, mongodb, rabbitmq and clickhouse walkthrough waits and the two mariadb asserts. The rest is tracked in #4404: the other Chainsaw HR-Ready asserts under their floor, the harbor asserts, the 300-second bucket waits, the postgres waits, and the redis and kafka-metadata diagnostic gap. - The clickhouse round-trip comment says every wait fails fast, `wait_hr_ready` on `Stalled=True`, "so a genuinely broken backup reports in seconds". A release whose install is still running is neither Ready nor Stalled, so that holds for a stalled release and not for an install that has not finished. The sentence predates this change. - `hack/mariadb-first-boot-waits.bats` is large for what it guards (two floors, two ratchets on the op, and the helper identity pin), with many fixture tests on its own extractors. Trimming it to that core is a separate decision. - Two comments in that file state facts about the mariadb suite that are true today and that nothing checks: three script ops carry a minute timeout, and the only one ahead of the round-trip is the 8m pre-clean. - The mariadb op comment says a bound on the whole flow "needs a phase budget (docs/agents/e2e-testing.md)"; the doc describes it as bounding the collection phase as a phase (`COZY_DIAG_PHASE_BUDGET`), so the pointer would resolve faster if it named that bullet. - Cosmetic: the behavioural test's loop body is not indented under its `for`, one line of the bats header runs past the width of the rest, and the checklist's sentence under the example assert ("what this block demonstrates least") reads awkwardly. ### Screenshots Not a UI change. ### Downstream repositories Walked the trigger map in `docs/agents/contributing.md` against the diff, file by file, for all eighteen files. The five `examples/backups/*/00-helpers.sh` files change the diagnostics of `wait_hr_ready`, and the mariadb, mongodb, rabbitmq and clickhouse walkthrough scripts change wait budgets; the manifests the walkthroughs apply and the flows they document are unchanged, so no website page restates anything that moved. Four Chainsaw suites (mariadb, clickhouse, mongodb, rabbitmq) change timeouts and comments, and the harbor and opensearch suites change comments only. `docs/agents/e2e-testing.md` changes the reviewer checklist entry and adds the sizing section. `hack/mariadb-first-boot-waits.bats` is a new file picked up by the existing `BATS_UNIT_FILES` wildcard. `hack/package.mk`, `hack/common-envs.mk` and `hack/update-crd.sh` are untouched, and no `values.schema.json`, `values.yaml` default, version enum, `ApplicationDefinition`, CRD, namespace or release asset is touched. - [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(tests): the MariaDB, MongoDB, RabbitMQ and ClickHouse backup examples and the MariaDB Chainsaw suites now wait for a HelmRelease past the release's own install timeout, so a failed install reports its own reason instead of a bare timeout, and the examples' HelmRelease wait now tells a release that never appeared, one deleted mid-wait and a kubectl that could not answer apart. ``` |
||
|
|
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 --> |
||
|
|
b96c822bde |
docs(agents): give the HR-Ready budget derivation its own section
The checklist entry had grown into a derivation and buried the rule it states. The rule stays in the checklist and the derivation moves to a section of its own. The derivation under-budgeted in two ways. It stated the 10m install timeout as universal, while releases an app chart renders itself take their template's spec.install.timeout, then its spec.timeout, or helm-controller's 5m where neither is set. And it restated which kinds kstatus gates on, which is easy to get wrong, so it now points at the upstream table instead. The harbor comments now name the floor each of their asserts actually sits against, and the opensearch comment stops citing a 5-6m standard the checklist no longer states. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <3811295@gmail.com> |
||
|
|
a3911811c7 |
feat(api): take Application name limits from the application's schema
The API server capped Kubernetes and Kafka names by knowing what those two charts render: worker pool releases under a 17-character prefix, controller pods with a 13-character suffix. That is the charts' knowledge sitting in the platform, and it does not generalise. The next application with a sub-resource budget needs another branch on Kind, and an application served from outside this repository cannot get one at all. An application now declares the limits on its own name in its schema, under x-cozystack-name, and the server applies whatever it finds there for whatever kind the ApplicationDefinition describes. A declaration only tightens: prefix + name must still fit the 53-character Helm release name limit. A rejected name is answered with the chart's own explanation of the limit. The schema is untrusted input, and definitions from other repositories are written by hand. A declaration that cannot be enforced as written, whether an unknown keyword, a wrong type, a limit no name satisfies or a pattern Go cannot compile, refuses creates of that kind with a configuration error. Dropping it would admit names the chart cannot render; applying it would reject every name and blame the user. An upgrade brings cozystack-api up before the definitions it serves are re-rendered, so for a while the new server can read a Kubernetes or Kafka definition that predates its declaration. The two old caps stay as a floor for exactly that case, a definition that declares nothing, until no supported upgrade starts from one. Assisted-by: LLM Signed-off-by: Timofei Larkin <lllamnyp@gmail.com> |
||
|
|
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
```
|
||
|
|
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> |
||
|
|
cbf297fa65 |
docs(kubernetes): describe the UID System-mode OIDC users carry
The aggregation-layer section said System-mode OIDC users have no UID for the published header to carry, and that mapping a claim for them was somebody else's change. Both are now false: the chart maps the sub claim. An operator upgrading a running cluster needs to know the identity of those users gained a field. Tenant RBAC never matched on it, but audit records and kubectl auth whoami show it, and an in-tenant webhook or extension server that read the empty value now reads the subject. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
|
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. ``` |
||
|
|
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 --> |
||
|
|
97832f3a40 |
docs(kubernetes): explain the aggregation-layer UID hop for tenant OIDC
A tenant cluster with OIDC on authenticates users fine while every call through an aggregated API server reaches it without the caller's UID, and nothing in the operator guide connected the two. Describe the hop, what the chart now renders per Kubernetes version to make it work, and how a hand-written flag in the extraArgs passthrough interacts with it. The migration step is described in both directions. The pinned tenant control plane rewrites a running apiserver's arguments only when their count changes and otherwise keeps every flag that has left extraArgs, so removing an opt-out or an --emulated-version entry strands a flag exactly as adding one does, and the operator can no longer see it in their own spec. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
|
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 ``` |
||
|
|
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 --> |
||
|
|
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> |
||
|
|
e32353e839 |
docs(agents): correct the labeler's trigger list (#4090)
## What this PR does The PR Title Auto-Labeling section still lists `edited` among the events `.github/workflows/pr-labeler.yaml` runs on. That trigger was dropped in #3211 — the workflow carries the note at the top of its `on:` block, and the v1.6.0 changelog records the change — so the document and the workflow have disagreed since. I hit it the way the document's intended reader would. Three PRs opened with GitHub's branch-derived titles, corrected afterwards to Conventional Commits, and the labels did not follow: `area/uncategorized` on all three and no `kind/feature`, because nothing re-ran. The second paragraph this PR adds is that consequence, which is the part a contributor actually needs: a corrected title does not re-label until the next push or a reopen, and since the labeler only ever adds, the `area/uncategorized` left by the first title stays until a human removes it. ### Screenshots Not applicable — no UI change. ### Downstream repositories I walked the trigger map in `docs/agents/contributing.md` against this diff. The only file it touches is that document, and the two triggers pointing at it are narrower than this change: `cozystack/community` restates the **commit convention** from it (types, scopes, sign-off, the `Assisted-by: LLM` trailer) and the **PR template and downstream checklist**, and `cozystack/ccp` gates its `package-bump` skill on the same commit convention. Neither restates the labeler's event list, which is what changes here. Nothing else in the map reaches a docs-only change. - [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 NONE ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Documentation** - Clarified when pull request title auto-labeling runs. - Documented that title edits do not trigger relabeling. - Explained that existing labels remain until a push or reopen, while additional uncategorized labels require manual removal. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
|
6cd78b0e30 |
feat(platform): make the Keycloak realm name configurable (#4237)
## What this PR does Fixes #4216. The Keycloak realm name `cozy` was hardcoded across every SSO component, so operators could not rename the realm without patching vendored templates that `make update` reverts. The kube-apiserver OIDC issuer is already configurable out-of-band via talm `oidcIssuerUrl`, which made the mismatch worse: pointing the apiserver at a custom realm left the dashboard, linstor-gui and the generated kubeconfig on `cozy`, so SSO broke with an issuer mismatch. This adds a single `authentication.oidc.realmName` (default `cozy`) to the platform Package values and threads it through the `_cluster` channel so every consumer resolves the same realm: keycloak-configure (realm, clients and `cozystack-cluster-admin` roles), the dashboard and linstor-gui gatekeepers (issuer/login/logout URLs), the info kubeconfig, and — beyond the components listed in the issue — the Grafana and managed-Kubernetes apiserver OIDC issuers, which carried the same hardcoded `/realms/cozy` in their `_helpers.tpl` and would otherwise have left SSO half-broken. `keycloakInternalUrl` is now derived from `realmName` when left unset, so the realm is named in one place. Its default changed from the literal in-cluster URL to `null` with a documented tri-state: `null` derives `http://keycloak-http.cozy-keycloak.svc:8080/realms/<realmName>`; `""` keeps the existing escape hatch that restores external OIDC discovery; any explicit value is used verbatim. A stock cluster renders the byte-identical `keycloak-internal-url` it did before, so existing clusters are unaffected on upgrade. The Kubernetes CR object names (`keycloakrealm-cozy`, `keycloak-cozy`) and the `realmRef` that clients point at are internal resource names, not the realm's technical name, and stay fixed — the EDP operator resolves clients into the renamed realm through the unchanged `realmRef`. Covered by helm-unittest across all seven affected packages, written test-first (red → green, mutation-verified): default stays `cozy`, a custom realm reaches every URL, the derived internal URL follows `realmName`, the `""` escape hatch survives a custom realm, and a numeric/reserved realm name stays a string. `packages/extra/info` had no unit-test target, so one was added. ### Downstream repositories `cozystack/website` needs a follow-up: `content/en/docs/next/operations/configuration/platform-package.md` is a hand-written table of the `spec.components.platform.values.*` keys and does not yet list `authentication.oidc.realmName`. Leaving the box unticked for a maintainer to action rather than filing a speculative PR. - [ ] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: document `authentication.oidc.realmName` in `platform-package.md` - [ ] [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(platform): the Keycloak realm name is now configurable via `authentication.oidc.realmName` (default `cozy`), threaded consistently through the apiserver OIDC issuer, dashboard, linstor-gui, Grafana, the managed-Kubernetes apiserver and the generated kubeconfig. Set it to the same realm as the talm `oidcIssuerUrl`. The `authentication.oidc.keycloakInternalUrl` default changed from the literal in-cluster URL to `null`, which now derives `http://keycloak-http.cozy-keycloak.svc:8080/realms/<realmName>`; clusters that leave it unset render the identical URL as before, and `""` still selects external OIDC discovery. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added configurable Keycloak realm support across OIDC integrations, including Kubernetes, dashboards, monitoring, kubeconfig, and authentication services. * The realm defaults to `cozy` and can be customized through platform authentication settings. * Internal Keycloak URLs are derived from the configured realm when not explicitly provided. * Added realm-name validation and support for explicitly disabling derived internal URLs. * **Documentation** * Updated OIDC guidance to describe configurable realm behavior and defaults. * **Tests** * Added coverage for custom realms, defaults, validation, and generated OIDC URLs. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
|
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 --> |
||
|
|
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> |
||
|
|
ff637ed9a2 |
fix(platform): validate the realm-name charset and regenerate kubernetes artifacts
The realm-name guard checked only the type and echoed the parsed value back, so its hint told an operator who wrote `on` to set realmName: "true" — the token YAML had already discarded. Drop the echo and add a slug-charset check: the name is spliced verbatim into URL path segments and a shell token, so a "/" or a space must be rejected at the source rather than split a URL or the hook script downstream. The description edit in the previous commit was hand-applied to the generated kubernetes README and never reached the Go types, leaving pre-commit red; regenerate both with cozyvalues-gen. Finish the oidc-tenant System-mode prose that still read cozy as a fixed name. Assisted-by: LLM Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
|
80d32b3ab7 |
fix(keycloak): complete realm-name coverage and guard non-string names
The enable-update-email hook Job in keycloak-configure still pinned REALM=cozy, so on a custom realmName it waited on a realm the chart never provisions and the cold install timed out — the one seventh consumer left behind, and the only value nobody could set without breaking the install. A bare-integer or YAML-reserved realm name reached the printf %s that builds the derived keycloakInternalUrl as a Go int, rendering a "%!s(...)" format error into the URL; coerce the name to a string once at the source. Document that renaming the realm on a running cluster provisions a fresh empty realm and orphans the accounts in the old one, and note in the OIDC tenant and grafana guides that the realm name is now the configurable default. info/Makefile carried NAME=etcd, so cozyhr drove the wrong release from that package; correct it while touching the file. Assisted-by: LLM Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
|
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 |
||
|
|
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> |
||
|
|
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> |
||
|
|
710cdd18bb |
fix(kafka): address KRaft-migration review — broker pool adoption, hook safety, tests
Addresses the branch review on the ZK->KRaft migration path. CRITICAL — broker node pool adoption. Strimzi derives broker/PVC names as <cluster>-<pool>-<id>; a pre-node-pool ZooKeeper cluster has brokers <cluster>-kafka-N, so the adopting broker pool must be named exactly "kafka" or the operator provisions fresh empty brokers and orphans the tenant's data. A new kafka.brokerPoolName helper resolves the broker pool to "kafka" for an existing (migrating) cluster and keeps the release-scoped "<release>-broker" for fresh installs, so multiple Kafka instances still coexist in one namespace. The migration hook creates the pool as "kafka" and fails closed if a "kafka" pool already belongs to another cluster (node pool names are namespace-unique, so in-place migration of co-namespaced ZK clusters is unsupported). Wired through kafkanodepools.yaml, workloadmonitor.yaml and the hook. MAJOR — migration robustness. - Raise the HelmRelease timeout: the migration Job can run tens of minutes but the release timeout defaults to 10m, so helm-controller aborted and thrashed the pre-upgrade hook. Add release.cozystack.io/helm-install-timeout=45m on the kafka ApplicationDefinition, above the Job's activeDeadlineSeconds (2400s). - Idempotent migration: do not re-issue kraft=migration once the cluster is past the ZooKeeper phase (an invalid backward transition Strimzi rejects on retry). - Fail closed on the decision gates: distinguish a genuine NotFound (skip) from any other kubectl error (abort), and never treat an empty kafkaMetadataState on an existing CR as "migrated". MINOR — version downgrade documented as unsupported; integer (not lexical) compare in the version gate; deleteClaim/PVC-retention and the one-per-namespace migration limitation documented in the README; docs/storage-immutability.md updated for KRaft. Tests — helm-unittest coverage for the broker pool name, the hook guards and the idempotent/fail-closed logic; a bats check for the helm-install-timeout budget ordering; and a Chainsaw ZK->KRaft migration suite that seeds a ZooKeeper cluster, drives the migration, and asserts the original broker PVC survives (not recreated) and the cluster reaches KRaft. The migration sequence and the Chainsaw assertions were validated against a live cluster. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
|
a2db2aeb8c |
fix(seaweedfs): raise the number of buckets a volume server can serve
A volume server started with -max 0 takes a slot count derived from disk rather than a fixed one, replication 001 spends a slot on each of the two servers per logical volume, and a collection claims COPY_2 volumes whenever it grows. So the bucket ceiling is set by the product of the size limit and COPY_2, and moving either number alone moves it. The mechanism is upstream in MaybeAdjustVolumeMax (weed/storage/store.go) and findVolumeCount (weed/topology/volume_growth.go). Do not divide these out of the nominal PVC size, because the count follows the space the filesystem makes available and not the volume's advertised size: the shipped 10Gi volume PVC was seen to give nine slots per server at the upstream 1000 and 19 at 500, which puts 400 at 23 to 24 by the same division, or eight collections against two. The span is the observation's own, since the free space behind 19 at 500 fits more than one filesystem geometry and those divide differently at 400; eight either way, because a collection that cannot get a whole batch of COPY_2 keeps the volumes it did get and writes into them. The division itself is measured: on a 1 GB image the master reports max as the filesystem's available space over the limit in every run, 25 slots and nine collections at the 40 and 3 that scale to this pair. Two is what the platform's own cozy-backups bucket and a single tenant bucket already use. Past that every S3 PUT to a new bucket times out while the disk still reads as nearly empty, and no COSI object reports anything wrong, because the claim and the access are both granted. 400 and 3 make the budget 1200 MB per collection. Splitting the reduction across both numbers rather than taking it out of the size limit alone keeps the cost off the curve that grows: open descriptors track stored bytes divided by the limit, so a 10 TiB server keeps roughly 52k of them at this limit against 21k upstream, where moving the size limit alone would have put it at 105k. A .dat and an .idx stay open per volume; the .vif beside them is written and closed. The ceiling reached is the same either way. Both figures describe the shipped replicationFactor of 2. The master picks its growth count from the replication the filer asks for, so at 1 and 3 copies it reads COPY_1 and COPY_3 instead and this COPY_2 does not apply; lowering the size limit still raises the ceiling there. The size limit was out of an operator's reach: it lived in the data-plane chart, which is pulled by ExternalArtifact from a component that declares no install section, so a Package-level values override for it is dropped before any merge. The app chart now declares master.volumeSizeLimitMB and sends it on every release, and 400 is set there. The data-plane chart keeps upstream's 1000 as its fallback on purpose. A platform upgrade can re-render that release from its artifact before the parent has sent anything new, and a lower fallback would then reach the master first and drop the limit ahead of an operator's own setting. COPY_2 is sent by nothing and lives in the data-plane chart, so it does land in that window, where a grow costs 3000 MB against the 6000 MB it cost before. The app chart also writes maxVolumes into dataDirs on every release, and because dataDirs is a list Helm replaces the data-plane chart's copy wholesale, so the zero that makes the slot count auto-derived at all comes from there. Losing it drops the server to the subchart's fixed seven without failing any render. The field is bounded at both ends, because either end takes an instance down. Zero is not a smaller limit but an off switch: the volume server leaves its slot calculation untouched at zero, keeps reporting no capacity, and nothing is ever grown. Past 30000 the master does not start at all. The bound sits in the values schema rather than in a template guard, so Helm refuses the value before any template runs. The apps API does not: it reads that schema only for defaults, so it accepts a value out of range, and the release then fails to render. An explicit null gets past both bounds, because Helm drops the key before it validates, and would render an empty -volumeSizeLimitMB= that the master exits on at start, measured on the shipped image, so the template requires the value. No bound against the disk goes with it, though the field invites one. A limit at or above the volume's own size does not break an instance: MaybeAdjustVolumeMax floors the slot count at one, measured on 4.31 with a 255 MiB disk against a 1000 MB limit, which reported Max 1 and served a write it then read back. Such a server is capped at a single slot rather than dead, so refusing it at render would be a capacity policy that stops a working release on every reconcile, not only on the upgrade. The field's description carries the relation instead, where the value is chosen. The growth count is deliberately not exposed beside the limit. The master picks which of its four growth counts to apply from the replication the filer requests, and the filer takes that from replicationFactor, so a field wired to COPY_2 would do nothing at 1 or 3 copies while still appearing to be a setting. A knob that is silently inert for two of the three supported values is worse than no knob. Two further prices. A collection absorbs the budget before it needs another grow, so growth requests arrive five times as often as at the upstream pair. A collection also starts with half as many writable volumes, and a volume server takes the writes into each volume one at a time (syncWrite holds that volume's dataFileAccessLock), so a heavily parallel writer spreads over fewer volumes until the collection grows again; that holds for instances kept at 1000 too. And lowering the limit on an instance that already holds data carries the other one: its volumes between the new limit and the old go oversized on the next heartbeat and leave the master's writable set, and each affected collection needs one successful grow before its next write lands. Nothing is lost and the volumes stay readable; the room left inside them for further writes is given up and does not return as capacity. Those grows compete for one pool, and the pool is smaller than it looks. An oversized volume is charged no headroom, while one below the limit is charged all of its unfilled room; a grow then claims a whole COPY_2 batch and takes as much of it as is there, and the volumes it creates are empty and charge their headroom straight back. Measured on a local SeaweedFS 4.31 setup: two oversized collections with two free slots between them, the first to write took both, and the second stayed refused with the disk still reporting a quarter of itself free. Collections that need no grow are not free either, since their volumes below the new limit were created six at a time and each can hold back a whole limit. Also measured there, with the limit and batch scaled down: a server with free space for one batch per collection refused the grow its one affected collection needed, because a second, nearly empty collection held the room, and the same free space without it let the grow through. The minFreeSpacePercent reserve only comes into it on a server whose writable volumes have all passed the limit, where nothing is charged headroom: from 8000 MiB of filesystem upward, a 10Gi PVC measuring 9979, five percent is at least 400 MB, and such a server then refuses a grow only once its disk is down to the reserve. Everywhere else the headroom charge decides, and a refusal can arrive with the disk far from full whatever the reserve. Deleting a collection gives the room back a slot per volume, however little it stored, since the headroom charged to those volumes leaves with them. So, on a variant that runs platform migrations, the upgrade does not change the limit on an existing instance unless its operator already set one, which the previous chart accepted and ignored. Platform migration 57 writes master.volumeSizeLimitMB: 1000 into every existing SeaweedFS instance that has not chosen a limit, and 400 applies only to instances created from here on. It leaves alone only an instance whose master is not an object, which no v1 chart renders, and names that one in its log. The default variant runs no platform migrations, so a cluster there that installed SeaweedFS by hand gets no pin, and the document below gives the same patch to apply before upgrading. A chosen limit is a whole number the new chart accepts; anything else counts as not chosen and is replaced with 1000, which is what the previous chart ran in its place, since it ignored the key and the new chart would refuse to render it. A valid value already set is kept, and since the previous chart ignored it, the upgrade is when it starts to apply; the document below lists the ones that change the limit ahead of it. Raised values are listed as well: existing buckets keep writing, but every volume below the new limit is charged up to it, and on a local SeaweedFS 4.31 setup seven nearly empty volumes left no free slot once the limit went up tenfold, so a new bucket was refused. The value goes into the application's HelmRelease, whose spec.values is the application spec, for an instance created through the API and for one the tenant module renders alike. The tenant module never renders spec.values on that release, and a Helm upgrade leaves a key alone that neither manifest carries: measured with Helm 4.3.0 on client-side, server-side and forced server-side apply, and read in createPatch at the v4.1.1 that helm-controller v1.5.0 runs. Where the migration runs, it is a pre-upgrade hook of the platform release, and the cozystack-packages source every application chart comes from is a manifest of that same release, applied only after its hooks; so no instance meets the new chart unstamped, and a failed migration stops the platform upgrade before that source moves. That holds while the platform source is pinned by digest, as it is out of the box; one that follows a tag, a semver range or a branch lets source-controller move cozystack-packages on its own schedule. Until cozystack-packages moves, each patched release keeps its old chart, which ignores the key, since no v1 release reads it or closes the master object in its schema. An instance kept at 1000 still gains, because at a replication factor of 2 a grow now claims three volumes rather than six, so it has room for roughly twice as many new buckets. Lowering an existing instance is left to its operator, and docs/operations/seaweedfs-lowering-the-volume-size-limit.md is the check for it. It lists each server's volume files and adds one limit for every volume below the limit and a batch for every collection with no volume at or below 90 percent of it, since the master grows such a collection on its own schedule without a write; measured the same way, a collection with every volume in that band and no writes got two new volumes per server within one pass. Where each disk type is served by as many volume servers as the replication factor, as the shipped two at a factor of 2 are, each collection's volumes sit on every server of that type, and that bounds the claim from above without assuming how the collections grew. The master places a new volume on any server of the requested disk type with room rather than where its collection already lives, whichever group or pool that server belongs to, and it reads an empty disk type as hdd; under MultiZone it also grows each zone on its own. So the document keeps zoned instances, and any disk type served by more servers than copies, at 1000. BREAKING CHANGE: on the default variant, which runs no platform migrations, set master.volumeSizeLimitMB to 1000 before upgrading on every SeaweedFS instance that has no valid value and whose master is an object, with the step in docs/operations/seaweedfs-lowering-the-volume-size-limit.md. On the variants that run platform migrations, migration 57 does this. If the step is skipped, every such instance drops to 400 on the upgrade: each of its volumes at or above 400 MB leaves the master's writable set, and a bucket whose volumes have all crossed that line takes no writes until it can grow. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
|
f9ea0da3de |
feat(backups): Kafka topic-metadata backup via a dedicated strategy driver (#4000)
## What this PR does Adds a dedicated `strategy.backups.cozystack.io/Kafka` Go driver that backs up the **topic metadata** of a Cozystack-managed `Kafka` application and wires it into the platform `cozy-default` flow — the counterpart to the app-specific drivers (CNPG, MariaDB, Altinity, MongoDB). It is a typed variant of the generic Job strategy: the controller renders a one-shot batch Job that talks to the Kafka Admin API, and adds on top an applicationRef Kind gate, a `Ready` precondition on the Strimzi `Kafka` cluster, and a per-run object key via `.BackupName` so distinct runs never overwrite each other. What it captures: every non-internal topic's partition count, replication factor and non-default configs (via `kafka-topics --describe`), stored as one small object per run in the system bucket; restore recreates them with `kafka-topics --create` + `kafka-configs --alter`. Deliberately **out of scope**: message payloads (a volume-snapshot strategy's job), consumer-group offsets (a log position, only meaningful with a byte-identical data restore), ACLs, quotas, and `KafkaUser`s. No PITR. Restored topics are created directly through the Admin API, so on the target they are unmanaged (no `KafkaTopic` CR) — the accepted tradeoff of an Admin-API-only driver. Both restore flows are implemented: **in-place** recreates dropped topics into the source, and **to-copy** applies the source's topics onto a freshly-bootstrapped empty `Kafka` (the "bootstrap a cluster, then apply the metadata state" flow). Contents: - Driver: `api/backups/strategy/v1alpha1/kafka_types.go`, `internal/backupcontroller/kafkastrategy_controller.go` (+ unit tests for its pure helpers), `internal/backupcontroller/kafkatypes/` (Strimzi `Kafka` readiness gate), dispatch cases in both controllers + `supportedBackupStrategyKinds()`, scheme registration, generated CRD + deepcopy. - Chart: `strategy-kafka-default.yaml` (gated on the bucket name), a `cozy-default` binding, read RBAC on `kafka.strimzi.io/kafkas`, and the Strimzi Kafka client image value. - Docs: a Kafka section in `docs/operations/backup-classes.md`. - Example + e2e: `examples/backups/kafka-metadata/` and `hack/e2e-chainsaw/kafka-metadata/` (`kafka-3-metadata-roundtrip`), an honest round-trip that proves a per-run `retention.ms` sentinel survives both restores. The original `packages/apps/kafka` chart is left untouched (it has no legacy backup mechanism). Validated: `go build ./...`, `go vet`, `go test ./internal/backupcontroller/...` (pure helpers plus a reconcile Kind-gate test and the RestoreJob cleanup regression test), `helm unittest` on the package (which now renders `strategy-kafka-default.yaml` and asserts its kind/name/`artifactURITemplate`), the `select-install`/`select-e2e` bats suites, `helm template` of the strategy CR, `bash -n` of the rendered script, and `make generate` (clean). The end-to-end round-trip (`kafka-3-metadata-roundtrip`) is enabled and mapped like the sibling backup suites, so CI builds this branch's controller and runs it live against a fresh sandbox; it has not been merged-and-run before, so that first live run must go green before merge. ### Screenshots N/A — no UI changes. ### Downstream repositories Walked the trigger map in `docs/agents/contributing.md` against the diff. This PR adds a new `strategy.backups.cozystack.io/Kafka` CRD, a Go driver, chart wiring, docs and examples. It does not add or rename a `packages/apps`/`packages/extra` package, does not change any `ApplicationDefinition`, `values.schema.json`, version enum, `release.prefix`, output Secret/Service, the `Package`/`Plan`/`RestoreJob` CRDs the provider types by hand, any `hack/*.mk` anchor or make-target behaviour, node prerequisites, labels/annotations, or telemetry metrics. A brand-new `strategy.backups.cozystack.io` CRD travels to the Terraform provider as opaque JSON (it types only `Plan`/`RestoreJob`/`Package`), so it keeps working untouched. None of the downstream triggers match. - [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 feat(backups): add a Kafka topic-metadata backup strategy bound by cozy-default — backs up topic definitions and configs (partitions, replication factor, non-default configs) via the Kafka Admin API, with in-place and to-copy restore; message data, consumer offsets, ACLs, quotas and KafkaUsers are out of scope ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added Kafka topic-metadata backup and restore support, excluding message data. - Supports ad-hoc and scheduled backups, in-place restoration, and restoration to a separate Kafka cluster. - Recreates missing topics and restores compatible configurations while validating partition and replication-factor constraints. - Waits for Kafka readiness and cleans up backup artifacts after completion. - Backup and restore jobs now enforce configurable deadlines to prevent stalled operations. - **Documentation** - Added guidance covering Kafka metadata backups, restoration limitations, and S3 configuration. - **Tests** - Added end-to-end coverage for backup, restoration, validation, and cleanup scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
|
d7c0332d5e |
chore(dashboard,api): drop the HTTPCache catalog entry and values type
The dashboard no longer registers a brand icon for the kind, and the ServicesTab fixture moves to TCPBalancer, which the catalog still ships. An instance that survives the removal still renders, on the generic sidebar icon. api/apps/v1alpha1/httpcache goes too. types.go is generated by values-gen out of packages/apps/http-cache, which is gone, so the file could never be regenerated, and no Go file in this repository imports it. Surviving instances do not depend on it: the aggregated API serves the schema recorded in the in-cluster ApplicationDefinition. The terraform provider's cozystack_httpcache resource is hand-written and does not import it either; only its per-kind schema-guard test does, and that goes away with the resource. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com> |
||
|
|
b3b280ea73 |
feat(gateway)!: add TLS-passthrough listeners to TenantGateway (#3342)
## What this PR does Phase 1 of external database exposure via Gateway API TLS-passthrough (#2815): adds a `tlsPassthroughListeners` field to `TenantGateway` and renders one layer-4 `mode: Passthrough` listener per entry, on a native database port with a per-engine subdomain hostname, alongside the Gateway's terminate listeners. No engine is wired: no TLSRoute, no certificate or CA changes. The existing layer-7 `tlsPassthroughServices` field gains a 62-entry count bound, an item length and a DNS-1123 item pattern, and a repeated entry now fails the reconcile with `Ready=False` instead of rendering the same listener twice. `spec.apex` gains a 253-character bound and nothing else: the CEL rules concatenate it, so the install-time cost estimator refuses the CRD without a declared bound. It gains no format pattern, and that is not a claim that a mixed-case host works. Such a host reaches the field verbatim from a namespace label, the renderer then composes listener hostnames Gateway API refuses, and that tenant has no Gateway either way; what a pattern would change is only that the write is refused, which fails this HelmRelease where the same fault costs the one Gateway. An apex no listener hostname can sit within is reported by the controller instead, and only when `tlsPassthroughListeners` is in use. The constraints and what they do to objects that already exist are described further down. The bound on an entry is what the schema can state on its own. An entry also becomes a listener name and a hostname built from the apex, so the controller checks both composed values against the 253 characters Gateway API allows and names whichever one overflows. Validation rejects duplicate listener names or ports, ports reserved by the platform Gateway (80 and 443), out-of-range ports, invalid DNS-1123 names, and hostnames outside the tenant apex (mirroring the Gateway hostname admission policy so an invalid entry cannot make the whole Gateway rejected). The SNI-overlap rule refuses two passthrough listeners that answer the same name on different ports. That is what [cilium#42898](https://github.com/cilium/cilium/issues/42898) reports: only one of the two routes ever works. It does not extend to a terminate listener and a passthrough listener sharing a name across ports. No failure is reported for that shape, and Cilium counts a terminate/passthrough pair as conflicting only when they share a port. The per-engine subdomain hostname scheme follows the phase-1 gate. The flat-hostname alternative (pending the Cilium listener-isolation fix targeting 1.20) is a later scheduling decision that does not change this API field. One shape differs from #2815 on purpose. The ticket has `hostname` optional with a `*.<name>.<apex>` default; here it is required. Deriving the SNI from the listener name hides the one value an operator has to get right, and the two drift apart the moment the naming scheme changes. Found while checking that pairing, unrelated to this field: a stock `dns01` gateway already renders `tls-api` (passthrough, 443) beside `https` (terminate, 443, `*.<apex>`), one port with overlapping SNI, which is the shape Cilium 1.19.6 marks `Conflicted`. That belongs to the Cilium bump and is recorded on #3343. Also named rather than fixed: the controller-side rules on this field stop the whole reconcile when they refuse a spec. `renderGateway` returns an error, so the Gateway is not updated at all, and the tenant sees `Ready=False` with `ReconcileError` while new app listeners and every certificate the controller would issue or amend wait behind it. Certificates already issued go on renewing, since cert-manager drives that from the objects already in the cluster and an aborted reconcile writes nothing to them. Dropping only the conflicting listener and reporting it on status would be the better shape, but that changes the error model of the whole renderer rather than one rule, so it is out of scope here. The current behaviour is at least observable: `Ready=False` is a signal, not silent drift. On HTTP-01, a hostname a passthrough listener answers earns no HTTPS-terminate listener once a TLSRoute claiming that hostname can be served on the listener, matched by SNI overlap so a wildcard entry reaches a name beneath it as readily as the name written in the spec, and the field itself is accepted under certMode http01 alone. That refusal is stated as the set of modes that can carry a passthrough listener rather than the set that cannot, because a mode added to the enum would otherwise be admitted by default: dns01 and existingSecret serve the whole apex from one wildcard terminate listener the pinned Cilium cannot keep apart from a passthrough listener beneath it, and edge ends TLS at the class provider and renders no TLS listener for one to sit beside. The route half of that condition is load-bearing rather than incidental. On the pinned v1.19.5, tlsPassthroughFilterChains walks a listener's routes and skips one with no backends, so a declared passthrough listener nothing attaches to contributes no filter chain and matches no ClientHello, while the terminate listener's chain is built from its own Secret and carries the name alone. Keying the withdrawal on the declaration would therefore take a name that is being served and hand it to a listener that forwards nowhere, and that is the ordinary shape rather than a corner: the chart puts api, vm-exportproxy and cdi-uploadproxy on every tenant Gateway, while there is one platform TLSRoute per name cluster-wide and each names one tenant's Gateway as its parent, so every other tenant renders all three entries with a route on none. The backend half is that same rule reached by a different road: the chain is built per route out of the backends that survived resolution, so a route that attached and forwards nowhere puts no chain on the name either, a backendRef naming a Service that does not exist yet because manifests applied out of order, one deleted since, a name with a typo, or one in another namespace with no ReferenceGrant, and without that half an ordinary backend mistake takes a live endpoint offline while Cilium builds nothing to replace it. Resolution asks what v1.19.5 asks and no more: the reference allowed, the Service present, and neither its ports nor its endpoints read, because the translator reads neither. Services and ReferenceGrants are watched alongside routes, so both halves are deferrals rather than exemptions and the withdrawal lands on the pass that sees the missing piece arrive. The withdrawal reaches HTTP-01 alone, because `collectHostnameClaims` returns nothing under the other two modes and they render no per-hostname listener to withdraw. A TLSRoute that declares no `spec.hostnames` claims the hostname of each rendered passthrough listener its parentRef selects, because that is the name Cilium serves it on: `ComputeHosts` at v1.19.5 substitutes the listener hostname for a route that names none, so such a route takes the name over exactly as one that spells it out does. The tenant-namespace hostname policy requires a route there to name its hostnames, so this shape reaches a Gateway from the system namespaces where the platform routes live, or from an administrator, not from a tenant chart. A claim broader than the listener's counts the same way from the other side: a TLSRoute claiming `*.<apex>` is served on `api.<apex>` rather than on the wildcard, because `ComputeHosts` returns the listener's own hostname for a route hostname covering it, so a claim covering a reserved name is judged as a claim on that name while the route keeps one entry under the key its owner wrote. Both restrictions are tied to the Cilium pin, currently v1.19.5. That version translates the Gateway into a single Envoy listener whose filter chains match on SNI without the port, so a terminate chain and a passthrough chain carrying one name collide regardless of which Gateway port declared them; upstream states the consequence above NeedsCrossProtocolSplit in operator/pkg/model/model.go as of v1.19.6, the release that starts splitting Envoy listeners per port when NeedsPerPortListeners holds. That is a disjunction rather than a blanket change, and the passthrough side of every one of its three clauses is counted from attached routes, so a listener with no TLSRoute behind it contributes to none of them: the split arrives with the route, not with the bump. The same single-listener translation has a consequence that is not a restriction: the declared port is where the listener is published, not a bound on reach, because the backend answers its hostname's SNI on every port the Gateway exposes. Firewalling a tenant address down to 80 and 443 does not put a database out of reach; the hostname is the access surface, and the field's own doc and the chart README now say so. Both restrictions are validation-only, and they lift together on the pin plus a configuration the split actually covers rather than on the bump alone. A route whose hostname is withdrawn this way is told so under this controller's own name: Accepted=False with `NoMatchingListenerHostname`, and a message naming the passthrough hostname that answers it, which for a wildcard entry the route cannot derive on its own. It matters most there. A wildcard entry can withdraw the listener from several published names beneath it at once, each one a TLSRoute claims and can be served on, and the certificates are garbage-collected with them, and a rollback is cheaper than that sounds: cert-manager here runs with `enableCertificateOwnerRef: false`, so deleting a Certificate leaves its Secret behind and a re-created Certificate adopts a still-valid one rather than placing an order. Before this the only signal was Cilium's own RouteParentStatus entry under a different controllerName. A TLSRoute whose hostname no passthrough listener answers is told so as well, on the same condition and for the same reason: before this change that hostname got a terminate listener Cilium refused with ResolvedRefs=False, so withdrawing the listener would otherwise have removed the only object saying the route went nowhere. Two things stay named rather than fixed. A `tlsPassthroughListeners` hostname of `*.<apex>` is refused only indirectly: the overlap rule rejects it while `tlsPassthroughServices` carries its defaults, because the wildcard covers api.<apex>, so emptying that list is enough to let it through, and the terminate listener then goes from every published name a TLSRoute claims under the wildcard. The README states this; refusing the apex and its wildcard on their own terms is a one-line rule worth adding when the field is wired up to chart values. A wildcard hostname on an attached HTTPRoute used to render a listener name the shipped Gateway CRD refuses, `https-*-<hash>`, which failed the whole Gateway write while the README said the shape worked. Listener and Certificate names now spell a leading `*` as `wildcard`, with the hash still covering the whole hostname, and a test reads both the listener-name and the hostname patterns off the shipped CRD bundle and renders wildcards through them; every name that was already admissible is pinned byte for byte, so nothing is renamed on upgrade. Under http01 an HTTPRoute wildcard claim is refused before the race and the overlap are read, with `NoMatchingListenerHostname`, because the HTTP-01 challenge issues no wildcard certificate and a terminate listener could never have carried the name. A route status write that fails no longer leaves the route carrying its previous condition: `updateRouteStatuses` and the whole-apex pass collect every failed write and return them after the loop, so the reconcile fails, the TenantGateway reports `Ready=False` with `ReconcileError` and controller-runtime requeues, while the loop still runs to the end so one route's conflict does not hold the others back. Conditions this controller wrote under http01 are retracted when the tenant moves to dns01, existingSecret or edge, for HTTPRoutes and hostname-less TLSRoutes alike, so a refusal does not outlive the mode that produced it. And a TLSRoute claiming a wildcard passthrough entry itself leaves the names beneath the wildcard alone, since Envoy ranks an exact server name above the wildcard and nothing collides. One thing this does not cover yet: Accepted is computed from the hostname race and the withdrawals above, so a route is still told it is fine when an HTTPRoute holds the name and the TLSRoute claiming it cannot attach to the terminate listener that answers it. That one predates this change. Two neighbours of it no longer fall in the gap. A `parentRef` port that names no listener answering the hostname is compared alongside the `sectionName`, since Gateway API requires every key a route pins itself with to match the listener it selects. An HTTPRoute's `parentRef.port` is judged the same way: the terminate listener sits on 443, so any other pinned port earns no listener and no certificate through that parentRef and reads the port-mismatch cause; port 80, or `sectionName: http`, from a namespace the plain-HTTP listener admits (the publishing tenant and cozy-cert-manager) is served plain with no terminate listener and no refusal, from any other namespace it is refused with `NotAllowedByListeners`, and a `sectionName` and a port that select different listeners select none and read `NoMatchingParent`. A plain-served route stays in the hostname race on the same footing as a terminating one, in every recount as well as the first count. And a `sectionName` naming a `tls-<name>` listener this Gateway does not render now reports `Accepted=False` with `NoMatchingParent`, Gateway API's reason for a parentRef matching no listener: the controller renders every listener in that namespace, so an absent name there is a fact rather than a guess, while a `sectionName` naming the plain-HTTP listener is judged like port 80 below, and any other name outside the prefix is written Accepted like an attached route, with the class controller's own status entry answering for the listener it named. The namespace case is off that list. Native-port listeners admit the publishing tenant alone, and a TLSRoute elsewhere on one of their hostnames is refused with `NotAllowedByListeners`. An HTTPRoute that pins itself by `sectionName` to a rendered `tls-<name>` passthrough listener is refused with the same reason: that listener forwards the stream it matched by SNI and terminates nothing, so it takes TLSRoute alone by protocol and cannot serve the route whatever answers its hostname. Its claim also leaves the hostname race before ownership is ranked, so the TLSRoute that can be served is not told it lost to a route that could never attach. Cilium does not report this refusal at the pin. The port-443 passthrough listeners name HTTPRoute in `allowedRoutes.kinds` to keep the port-443 set uniform (cilium#45559), the Gateway reports that entry back as `ResolvedRefs=False/InvalidRouteKinds` on the listener, and Cilium's route-side kind check reads a listener's kinds against every route on the Gateway rather than against the routes that named it, so it reports the route Accepted and the condition this controller writes is the only refusal the route gets. A hostname whose every claimant is refused this way earns no HTTPS-terminate listener and no certificate, where before it earned both. The refusal is decided per route rather than per hostname, which matters once a claim overlaps more than one reserved name: a wildcard covering both a port-443 service hostname and a native-port one is attachable through the first and refused through the second, so the answer comes from the listener the `parentRef` names, its own hostname and its own attach set. The row that pinned a `sectionName` naming no rendered listener as a known gap now pins the verdict instead. On the port-443 listeners the two namespace sets agree by construction, since `collectHostnameClaims` and the listeners' allowedRoutes selector run off the same list. And the justification for the pin is written out at every site that depends on it rather than once, so the day the pin moves they all have to move together and the one that does not is a false statement no diff can catch. That is not hypothetical here: three claims of exactly that shape were wrong in this branch and were caught by reading the source they described rather than by any diff, so the volume of restated reasoning is itself what produces them. One canonical block with references to it would carry the same knowledge with one place to update, and rewriting the narrative under review would be a documentation change rather than a fix, so it is tracked on #3842 instead. The gateway chainsaw suite now covers the withdrawal, which it did not before: a tenant owning its own Gateway renders the shipped passthrough entries with nothing routed to them, an HTTPRoute on api.<apex> keeps its terminate listener, its certificate and Accepted=True there, and attaching a TLSRoute to the same listener, pointed at a Service in the tenant's own namespace, since a route that forwards nowhere is the case the first phase already measures, withdraws all three and moves the route to NoMatchingListenerHostname. A third phase patches in a native-port entry and reads the per-listener readiness flag, which is the claim no test against a fake client can settle. The first two phases are one measurement: withdrawing on demand is what dates the earlier assertion to settled state rather than to a withdrawal that had not arrived yet. The `Require API owner review for sizeable API changes` check is red on purpose and does not clear with a code change. A new field plus tightened validation on already-shipped fields counts as a sizeable API change, so `.github/workflows/api-review-gate.yaml` holds the PR until an API owner approves it. The gate counts each owner's latest review on the current head commit, so the approval has to land on the head as pushed and any later push invalidates it; the job re-runs on review submission, so no push is needed to flip it. Reading that red as "not ready" is the mistake this paragraph exists to prevent. Part of #2815. ### Release note ```release-note feat(gateway): TenantGateway supports layer-4 TLS-passthrough listeners on native database ports for SNI-based external exposure. On HTTP-01 gateways a hostname a TLS-passthrough listener answers, including any name beneath a wildcard entry, no longer earns an HTTPS-terminate listener or a Gateway-issued certificate once a TLSRoute claiming that hostname can be served on the listener and forwards to a backend that resolves; a hostname claimed only by a TLSRoute stops earning one too. A TLSRoute with no spec.hostnames counts as claiming the hostname of each passthrough listener its parentRef selects, and one whose hostname covers a listener's counts as claiming that listener's name, both being the name Cilium serves it on. A declared passthrough listener with no route on it changes nothing, because it emits no filter chain and the terminate listener is what answers the name, and neither does a route whose backendRefs resolve to nothing, a Service that does not exist, or one in another namespace with no ReferenceGrant, since such a route emits no filter chain either. The controller watches Services and ReferenceGrants, so the withdrawal it defers lands once the backend appears. So the tenant whose Gateway the platform TLSRoutes name, tenant-root unless the cluster's expose-ingress says otherwise, sheds the terminate listeners and certificates for api, vm-exportproxy and cdi-uploadproxy on upgrade (those TLSRoutes ship with the cozystack-api, kubevirt and kubevirt-cdi packages, each rendered only where the Gateway integration is enabled and the service is among the platform's exposed services), and a tenant that renders the same three entries with no route on them keeps what it has. The shed certificates leave their TLS Secrets behind: cert-manager here ships enableCertificateOwnerRef: false, so each Secret stays in the namespace with no owner, nothing renewing it and nothing deleting it. That cuts both ways, as stale Secrets to remove by hand and as a cheap way back, since a re-created Certificate adopts a still-valid Secret instead of placing an order. A route the controller renders no listener it can attach to now carries Accepted=False from this controller instead of Accepted=True. Five shapes report NoMatchingListenerHostname and the message tells them apart: the HTTPRoute whose hostname a passthrough listener answers, the TLSRoute with no passthrough listener behind it, the route whose parentRef names a listener that answers a different hostname, the route whose parentRef names a port no listener answering its hostname is published on, and the HTTPRoute claiming a wildcard hostname under http01, which the HTTP-01 challenge cannot certify. A route pinned by sectionName to a tls-<name> listener the Gateway does not render carries NoMatchingParent instead, which is what Gateway API calls a parentRef matching no listener, and so does an HTTPRoute whose sectionName and parentRef port select different listeners. A TLSRoute outside the publishing tenant's namespace claiming a native-port hostname carries NotAllowedByListeners instead, since those listeners admit the tenant alone. So does an HTTPRoute pinned by sectionName to a rendered tls-<name> passthrough listener, which cannot serve its kind, and an HTTPRoute selecting the plain-HTTP listener from a namespace it does not admit; a refused route also stops counting in the hostname race, so a hostname whose every claimant is refused earns no terminate listener and no certificate. Listener and Certificate names derived from a wildcard hostname are admissible now, names that were already admissible do not change, a failed route-status write fails the reconcile instead of being logged away, and conditions written under http01 are retracted when certMode leaves it. The dns01 and existingSecret cert modes shed nothing, because neither renders per-hostname listeners at all, and tlsPassthroughListeners is accepted under http01 alone: those two serve the whole apex from one wildcard terminate listener, and edge terminates TLS upstream and renders no TLS listener a passthrough entry could sit beside. The withdrawal does not take a working path away: on the pinned Cilium both port-443 filter chains already carry these hostnames and nothing on the objects says which one answers a connection, so a race that was previously undetermined and unobservable now resolves the same way every time. Treat this as a change of serving path rather than the removal of something idle: today each of those three hostnames has a terminate listener and a passthrough listener on port 443, both filter chains carry the SNI, and nothing on the objects says which one answers a connection. Gateway API calls for both listeners to report Conflicted; the pinned v1.19.5 has no Conflicted condition at all, so there is nothing to read, and you cannot tell from the cluster whether api.<apex> is terminated by the Gateway or passed through. Afterwards it is passed through, always. Read the Gateway's listener conditions before upgrading to see what those listeners were serving. Before upgrading, confirm the management apiserver serving certificate carries api.<apex> in its SANs, because once the ACME certificate is withdrawn that is the only certificate left on the name; a cluster that came from the ingress-nginx path already meets that, a cluster served by the Gateway's ACME certificate since it moved does not necessarily and loses the name until the SAN is added. If something depends on the terminate path for one of those names, the package README lists three ways forward and what each costs. Moving the consumer to another hostname under the apex is the only one that gives up nothing. ``` ### Downstream repositories Walked the trigger map in `docs/agents/contributing.md` against this diff: `api/gateway/v1alpha1/{tenantgateway_types.go,zz_generated.deepcopy.go}`, `internal/controller/tenantgateway/{reconciler,renderers,conflict,mapper,status,backends,reconciler_test,crdcel_test,mapper_test,backends_test}.go`, `internal/controller/tenantgateway/rbac_test.go`, `cmd/cozystack-controller/main.go`, `hack/e2e-chainsaw/gateway/chainsaw-test.yaml`, `packages/system/cozystack-controller/definitions/gateway.cozystack.io_tenantgateways.yaml`, `packages/system/cozystack-controller/templates/rbac.yaml`, `packages/core/platform/values.yaml`, and `packages/extra/gateway/README.md`. No trigger fires, and the two repositories worth ruling out explicitly are below rather than waved off. `cozystack/website`: no package under `packages/apps/` or `packages/extra/` is added, renamed or removed. `packages/extra/gateway/README.md` is edited in place. The docs generator regenerates a reference page from a package README only for the apps hardcoded in the website `Makefile`, and `gateway` is in none of `APPS`, `K8S`, `VMS`, `NETWORKING` or `SERVICES`, so there is no generated page for this package to drift out of step with. That gap is the same shape the trigger map already records for `packages/apps/opensearch` and `packages/apps/bucket`; it predates this change and is neither created nor widened here. None of the other website triggers apply: `packages/core/platform/values.yaml` changes only in a comment, which now says that api, vm-exportproxy and cdi-uploadproxy render a TLSRoute rather than an HTTPRoute, with no key added, moved or given a new default, no platform variant or component moves, no release asset is renamed, `ApplicationDefinition` semantics are unchanged, there is no Talos bump and no developer-tooling change. `cozystack/terraform-provider-cozystack`: this adds an API field, so the field-level triggers are the ones to check, and they are keyed to app charts rather than to this CRD. No app is added under `packages/apps/`; no `values.schema.json` gains, loses or renames a field; no version enum, `values.yaml` default, `kind`, `plural`, `release.prefix` or output Secret/Service name changes. `TenantGateway` is not among the CRDs the provider types by hand. Those are `Package` under `cozystack.io` and `Plan`/`RestoreJob` under `backups.cozystack.io`, and the provider carries no reference to `gateway.cozystack.io` at all. It therefore falls under the map's last bullet: the spec travels as opaque JSON, so the provider keeps working and simply offers no typed access to `tlsPassthroughListeners`, which is the position it already holds for every other field it does not model, and not a break. The provider's new-top-level-field test guard is scoped to `api/apps/v1alpha1`, which this diff does not touch. `cozystack/ansible-cozystack`: nothing in reach. `packages/core/installer/values.yaml` and the `cozy-system`/PSA contract are untouched, no platform variant is added or renamed, none of the `packages/core/platform/values.yaml` keys the role sets are involved (the file changes in a comment alone), no object the role waits on is renamed, and `hack/e2e-prepare-cluster.bats` node prerequisites are unchanged. The schema constraints this diff adds to already-shipped fields all reach objects that already exist, and every one of them is a kind the apiserver ratchets. `spec.apex` gains a 253-character limit (no pattern: a mixed-case apex is admitted and refused by the controller only when passthrough listeners are declared), and `spec.tlsPassthroughServices` gains a DNS-1123 item pattern and a length bound. A stored value that violates one still passes any write that leaves it unchanged, so the rule bites only once the value itself changes. A repeated `tlsPassthroughServices` entry is reported by the controller as `Ready=False` rather than refused at admission, and the reason is not the one this section used to give. Ratcheting does reach a list-type error: `customresource.ValidateUpdate` validates an incoming object's list-type semantics only when the stored object passes them, so a stored duplicate is ratcheted through rather than blocking every write. What makes the marker the wrong tool is the grain of that ratchet, which is the whole object rather than the field, so one stored duplicate would switch list-type checking off for every list on the `TenantGateway` at once and say nothing about it. Documented in `packages/extra/gateway/README.md`, and pinned by `TestTightenedConstraintsOnExistingObjects` together with `TestListTypeSetWouldNotRefuseAStoredDuplicate`, which compiles a copy of the shipped schema carrying the marker to show what the apiserver actually does with one. Shipped defaults conform and the chart already fails on an empty host, so this is a no-op in practice. - [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: <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added layer-4 TLS passthrough listeners (`tlsPassthroughListeners`) with native port + SNI hostname, rendered as passthrough listeners and limited to TLSRoute attachment. * **Bug Fixes** * Added stricter admission-time and controller validation (reserved ports 80/443, distinct listener ports, tenant-apex hostname rules, and collision prevention with passthrough services). * Improved listener rendering safety: prevents exceeding the Gateway listener cap and ensures AllowedRoutes are isolated per listener. * **Documentation** * Updated gateway docs with listener-cap considerations and layer-4 passthrough usage details. * **Tests** * Added CRD/CEL and reconciliation coverage for valid/invalid configurations and rendering behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
|
5fe595d4c0 |
test(e2e): hold the tenant-cluster install wait to an ordering rule
A test that creates a tenant Kubernetes cluster cannot END while that release is still installing. A Helm release cannot be uninstalled mid-install, and for this kind the install can outlive the fast assertions: the kind carries helm-install-disable-wait, which drops the wait on the main resources but not on the chart's blocking post-install hook. A test that asserts and ends hands the remainder of the install to teardown, where it is reported red on a step that tests nothing. The rule is about ending, not about touching the cluster at all. The hook polls for the admin kubeconfig in the modes that have one, and that Secret is what the test needs to reach the tenant API, so bringing up a worker or waiting on a child release happens while the parent is still installing. Reading mid-install is fine. run-kubernetes.sh already does the right thing: it waits for the parent release to reach Ready before the tenant-cluster assertions. Nothing held it there. This adds the guard that does, and states the rule where the e2e conventions live. The guard reads the script rather than Chainsaw fixtures, because no fixture applies this kind and a guard walking apply.file would pass by having nothing to check. A case asserts that this stays the only script creating it, and the sweep behind it keys on the apiVersion and kind together so that an indented apply.resource block counts as a creator too. It reads scripts and manifests only, and that rule is pre-emptive rather than a response to what the root holds today: a manifest quoted in prose or parked in a `.disabled` file creates no cluster, and this repository writes the first shape in docs that lie outside the root. The rules it holds the script to -- what counts as a wait with teeth, where in the body it has to stand, what may not switch errexit off above it, and which call shapes count as an end-assertion -- are in the file's header beside the code that enforces them, each with the spellings it accepts and what it cannot see. How to size the budget is derived once in the e2e conventions. Nothing is added at the wait itself: hack/e2e-chainsaw/_lib is shared by every Chainsaw suite, so a comment there escalates this change from no e2e suites to the full suite, and the guard already reds on the move it would have warned about. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
|
38d6c4cf58 |
fix(backups): bound Kafka metadata run at the Job layer, not a controller wall clock
A controller-side running-Job deadline was enforced in reconcileKafka / reconcileKafkaRestore: at kafkaDefaultBackupDeadline from StartedAt the BackupJob/RestoreJob was marked Failed. But the batch Job carried no activeDeadlineSeconds and markBackupJobFailed only writes status, so the Job kept running, finished, and its final `s3 PUT` uploaded the object — while the terminal phase meant no Backup was ever created and cleanup (keyed on Backup deletion) never ran. Every timed-out run thus orphaned a uniquely-keyed object, and a cluster whose export legitimately exceeds the deadline could never produce a usable backup. The shipped docs also claimed the run "keeps the BackupJob Running ... rather than failing on a wall clock", which the deadline made false. Move the bound onto the Job as activeDeadlineSeconds (setKafkaJobDeadline): Kubernetes fails an unschedulable or wedged Job as DeadlineExceeded and kills its pod before the upload, so a timed-out run lands in the JobFailed branch cleanly and leaves no object behind. Make it operator-tunable with the `backupTimeout` BackupClass parameter (a Go duration), which round-trips through the Backup so a restore is bounded by the same value; the readiness-precondition wait honours it too. Correct the docs. Also requeue on a transient broker-image List error instead of failing the whole run: resolveKafkaBrokerImage now wraps a sentinel (errNoKafkaBroker) so only a Ready cluster with no listable broker is terminal. Tests: pin the deadline on the built Job, the backupTimeout override, a JobFailed run marking the owner Failed, and the transient-List requeue. Signed-off-by: Andrey Kolkov <androndo@gmail.com> Assisted-by: LLM |
||
|
|
14c6af122e |
fix(backups): restore Kafka S3 port workaround and hold the script with a test
- Regression fix: an earlier change dropped the curl --connect-to port workaround along with -k, but curl 7.76.1 (the image's) 403s on ANY ported endpoint, which is the shape backupStorage.endpoint ships (:8333) and what provisionBucket:false and s3.ingress.enabled:false produce. Restore --connect-to (sign a portless URL, redirect to the real port), keeping -k off so TLS is still verified. IPv6-safe host/port split, path prefix preserved. The port-less ingress passed; the ported case is the one that regressed. - Correct the S3 envelope claim in values.yaml and backup-classes.md: the 403 is decided by the port, not a body-hash the backend recomputes, and --connect-to handles it on SeaweedFS and MinIO alike. - Hold the script fixes at the layer the contract lives at. Externalise the driver to files/kafka-backup.sh (embedded verbatim via .Files.Get, render byte-identical) and add hack/kafka-backup-script.bats, which EXECUTES it against stubbed kafka CLIs + curl and asserts behaviour: \Q literal match on the colliding pair, --connect-to with no -k on a ported endpoint, fail-closed --list, comma-value bracket grouping, RF-divergence abort, and the cleanup delete's HTTP handling. Not a template grep. - Pin the Backup-delete dispatch and the readiness deadline in Go: TestCleanupOnDelete_Kafka_RoutesToKafkaCleanup and TestReconcileKafka_FailsAfterReadinessDeadline. - Docs: config restore is additive (no --delete-config); export time scales with topic count and the Job has no wall clock (as its siblings). Signed-off-by: Andrey Kolkov <androndo@gmail.com> Assisted-by: LLM |
||
|
|
0270650237 |
fix(backups): harden Kafka metadata S3 handling and own the artifact
- Drop `-k`: mirror the RabbitMQ S3 layer (ARTIFACT_URI as the single source of truth, forcePathStyle honoured, whole base URL kept) so curl --aws-sigv4 verifies TLS against the ACME endpoint the helper returns. The derived endpoint is default-port, so the curl 7.76.1 --connect-to port workaround is gone too; this also fixes the IPv6 and path-prefix mangling in the old host-splitting. - Own the artifact: a cleanup mode in the script plus cleanupKafkaBackup wired into BackupReconciler.cleanupOnDelete delete a deleted Backup's object (best-effort, waited on) instead of orphaning it, matching the RabbitMQ precedent. Kafka has the identical own-your-object property. - Restore probes existence with --list (fail closed), so an unreachable broker aborts instead of being misread as "topic absent" and created. - Hold the fixes with tests at the right level: a colliding topic pair (audit.events / audit-events, distinct partition counts) in the e2e so a revert of the \Q...\E literal-match breaks the round-trip, and Go tests pinning the scope stamp and the cleanup Job. - Correct the docs/comments: endpoint-shape row, the "cached on nodes" claim, and the narrower curl 7.76.1 S3 envelope (empty-body payload hash) that makes a small modern-curl client image the right follow-up. Signed-off-by: Andrey Kolkov <androndo@gmail.com> Assisted-by: LLM |
||
|
|
405eb25095 |
fix(backups): harden Kafka metadata backup
- Fail closed on topic listing: set -o pipefail, capture kafka-topics --list --exclude-internal exit separately, and abort on failure so an unreachable broker can no longer produce an empty backup reported as Succeeded. An empty list after a successful call stays legitimate. - Check each kafka-topics --describe exit and validate the parsed partition count / replication factor before writing a record, so a timed-out describe cannot write a corrupt orders\t\t\t line. - Store topic configs one per line with key and value in separate tab-delimited fields, sourced from kafka-configs --describe (one config per line), so a value containing a comma (cleanup.policy=compact,delete) round-trips; on restore each config is applied with the value bracketed only when it contains a comma. - Detect topic divergence on restore instead of a silent --if-not-exists no-op: grow partitions with --alter when the backup asks for more, and fail loudly (naming the topic and both values) when it asks for fewer partitions or a different replication factor. A restore that cannot reach the recorded state no longer reports Succeeded. - Add KafkaStrategyKind to the RestoreJob cleanup-dispatch no-op group so deleting a Kafka RestoreJob does not run a pointless Velero cleanup; add a regression test alongside the siblings, plus a reconcile Kind-gate test and a rendering test that actually renders the strategy template. - Wire the kafka-metadata Chainsaw suite into both selectors (select-install suite_to_source, select-e2e src_to_suites) so editing the kafka app or the suite selects it and the bats unit tests pass. - Pin the client image to the operator's default broker version (kafka 3.9.1) with a digest, and align the example default; fix the values comment. Point the RabbitMQ comment references (which shipped in the CRD description) at the Job strategy actually mirrored, and trim the in-pod comments that rendered into the manifest. - Document the Strimzi Topic Operator caveat: for a topic owned by a KafkaTopic CR the operator reconciles its config from the CR, so an in-place config restore is durable only for out-of-band topics. Signed-off-by: Andrey Kolkov <androndo@gmail.com> Assisted-by: LLM |
||
|
|
a410a3f48b |
feat(backups): Kafka topic-metadata backup via a dedicated strategy driver
Add a strategy.backups.cozystack.io/Kafka driver that backs up the topic metadata of a Cozystack-managed Kafka application — partition counts, replication factors and non-default topic configs — through the Kafka Admin API, and restores it with kafka-topics --create + kafka-configs --alter. Modelled on the Job/Rabbitmq strategies: the driver renders a one-shot batch Job, gates on the Strimzi Kafka cluster reporting Ready, and keys each backup by .BackupName so runs do not overwrite each other. Scope is deliberately narrow: message payloads, consumer-group offsets, ACLs, quotas and KafkaUsers are out of scope, and there is no PITR. Restored topics are created directly (unmanaged on the target) — the accepted tradeoff of an Admin-API-only driver. Both restore flows are implemented: in-place recreates dropped topics into the source, to-copy applies the source's topics onto a freshly-bootstrapped empty cluster. Wire it into cozy-default (strategy-kafka-default.yaml gated on the bucket name, a BackupClass binding, read RBAC on kafka.strimzi.io/kafkas for the readiness gate, and the Strimzi Kafka client image), document it in backup-classes.md, and add an honest round-trip example plus Chainsaw e2e (examples/backups/kafka-metadata, kafka-3-metadata-roundtrip) that proves a per-run retention.ms sentinel survives both restores. Unit tests cover the driver's pure helpers (application-ref validation, readiness verdict, parameter round-trip, per-run artifact-URI keying). Not yet verified on a live cluster. Signed-off-by: Andrey Kolkov <androndo@gmail.com> Assisted-by: LLM |
||
|
|
ae5b626c8f |
docs(gateway): describe the passthrough listeners and what they shed
The chart README gains the native-port field, the three limitations it carries and where each comes from, the one that is a phase-1 narrowing rather than a consequence of the Cilium pin, and the three ways forward for anything that depended on a terminate listener the change takes away. The withdrawal is stated as what it is, a change of serving path rather than the removal of something idle, because on the pinned version both filter chains carry the SNI today and nothing on the objects says which one answers. The layers section is brought back in step with the listeners: which label each listener class selects on, that the attach set is the whole inheriting tenant subtree rather than the tenant plus the admin list, and the route conditions an operator reads, including the one that says the sectionName itself names nothing. The platform values comment on gateway.enabled said every exposed service renders an HTTPRoute, which the chart value the withdrawal reads contradicts. It now separates them: the services whose TLS the Gateway terminates get an HTTPRoute, while api, vm-exportproxy and cdi-uploadproxy get a TLSRoute pinned to their passthrough listener, one each from the packages that own those services. The overlap paragraph no longer describes an HTTPRoute wildcard as keeping its terminate listener. Such a claim is refused under HTTP-01, which issues no wildcard certificate, and the paragraph says so and what the render used to do with it. The route-condition list gains that shape and the port shapes an HTTPRoute now reaches, and the per-app naming bullets carry the hash suffix the rendered names have. Two sentences that were false against the code are corrected: a TLSRoute on a wildcard entry itself leaves the names beneath it their terminate listeners, and an unusable apex is refused by the containment rule at admission before the controller's own check is reached. The edge section says what happens to the entries the HTTP-01 pass wrote once the mode moves. The NoMatchingParent sentence now separates the two kinds: an HTTPRoute pinned to an absent passthrough section is refused whatever answers its hostname, while a TLSRoute is judged where a passthrough listener answers it. A sectionName outside the passthrough prefix is written Accepted like any attached route rather than left without a verdict, and the text says so; the port-80 listener's refusal joins the NotAllowedByListeners shapes and the name-and-port disagreement the NoMatchingParent ones. The attachedNamespaces comment described the list as reaching every listener, which the native-port ones do not: they select on kubernetes.io/metadata.name and name the publishing tenant alone, so a namespace listed there cannot attach to a database port. It also credited the wrong label with keeping the list honest; what a listed entry gets is the namespace.cozystack.io/gateway label, and what keeps a tenant from stamping it on itself is that no tenant role carries patch on Namespace. The class reference is brought along. Its certificate-mode table promised one Certificate per published hostname under http01, which a hostname served by a passthrough listener no longer earns, and its edge table said every attached TLSRoute is refused, where only a route declaring hostnames is and the rest have this controller's entry removed instead. The refusal-shape list is corrected against the code, without a count of its own so the next correction does not have to move one. A route the plain-HTTP listener turns away on its namespace selector was listed under NoMatchingListenerHostname as well as under NotAllowedByListeners; the controller reports the second. The port shape standing beside it swept port 80 back in by naming every port but 443, where a route selecting the plain-HTTP listener is answered by the namespace shapes and never reaches the port refusal at all. And a TLSRoute pinned to an absent section is said to carry NoMatchingListenerHostname where no passthrough listener answers its hostname, which holds only while no HTTPRoute claims that hostname: with one there a terminate listener stands on the name, and the route is judged like any other attached route. The wildcard shape gains the same qualifier. A wildcard claim is refused because no terminate listener can carry a certificate HTTP-01 will not issue, so the refusal reaches a route that wanted a terminate listener and not one that asked for the plain-HTTP listener, which carries no hostname and serves the claim there. The Helm merge paragraph named the two-way path as what happens to a custom resource Helm has no Go type for. That path is one of two in createPatch, the version deciding which runs, and the version comes from helm-controller rather than from this tree, so the claim was not checkable where it was written. It now names both and says the outcome it rests on holds either way. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
|
1f04e31ed3 |
feat(backups): add Redis backup/restore strategy driver (#3992)
## What this PR does Adds backup/restore support for the `Redis` app (spotahome `RedisFailover`) via a dedicated `strategy.backups.cozystack.io/Redis` strategy controller. The spotahome operator ships no backup CRD, so the driver follows the generic-script-Job pattern (a specialisation of the Job/Altinity drivers): it runs a one-shot Job that dumps an RDB snapshot from the current master to the system bucket and replays it on restore. - **Backup**: discover the current master via the operator's sentinel Service (`rfs-<name>`, master `mymaster`) → `redis-cli --rdb` → `curl --aws-sigv4` PUT to `s3://<bucket>/<namespace>/<app>/dump.rdb`. - **Restore**: GET the object → convert RDB → RESP (`hdt3213/rdb`) → `FLUSHALL` + `redis-cli --pipe` into the target's master. Both **in-place** and **to-copy** are implemented; a to-copy restore reads the source's prefix via `.Backup.ApplicationRef.Name` while writing into a differently-named target. - **PITR**: not supported — RDB is a discrete snapshot with no continuous log to replay to an arbitrary time (the etcd-like case). The driver is a specialisation of the Job/Altinity script-Job flow with a `Redis` applicationRef gate and an app-readiness precondition (surfaces a precise `Ready=False` instead of a silent hang; blocks only on an explicit `Ready=False` so a missing condition never wedges backups). Strategy parameters round-trip through the Backup's `DriverMetadata`; there is no `underlyingResources` snapshot (the Job pattern re-renders on restore). ### System bucket Coordinates and credentials reach the Job as env `secretKeyRef` from the projected `cozy-backups-creds` (the ClickHouse/Altinity delivery model), so **the app chart is untouched** — no `backup.useSystemBucket`, no new RBAC (`batch/jobs` and `apps.cozystack.io/*` are already granted). The bare-host `endpoint` gets `https://` prepended when unscheme'd; against a self-signed endpoint the strategy verifies with the CA from `backupStorage.endpointCASecretName` when set, otherwise against the image's system CA store; skip-verify is a separate explicit `backupStorage.insecureSkipTLSVerify` opt-in, never a fallback. The `cozy-default-redis` strategy is gated on `$bucketName` (CRD-ordering) and bound in the `cozy-default` BackupClass. ### Changes - `api/backups/strategy/v1alpha1/redis_types.go` + generated CRD and deepcopy. - `internal/backupcontroller/redisstrategy_controller.go` (+ unit tests) wired into both dispatch switches, `supportedBackupStrategyKinds`, and the no-op restore-cleanup case. - `cozy-default-redis` strategy template + `cozy-default` BackupClass route + helm rendering-gate coverage. - `images/redis-backup` (redis-cli + curl + RDB→RESP converter) + Makefile target + `backupStrategyController.redisBackupClientImage`. - `examples/backups/redis/` — an honest marker round-trip for both restore flows. ### Validation `go build`/`vet`, `go test ./internal/backupcontroller/... ./api/backups/...`, `make generate` (clean), and `helm unittest` (10/10) all pass. Not yet cluster-verified. ## Downstream None. ```release-note Add backup and restore support for the Redis application (spotahome RedisFailover) via a new Redis backup strategy, wired into the cozy-default BackupClass. Supports in-place and to-copy restore; PITR is not supported. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added Redis backup and restore support using RDB snapshots in S3-compatible storage. * Supports restoring data in place or to a separate Redis instance. * Added configuration for Redis backup images, custom HTTPS certificate authorities, and TLS verification. * Added a complete Redis backup and restore demonstration with automated setup, verification, and cleanup. * **Documentation** * Documented Redis storage, credentials, endpoints, TLS, and restore behavior. * Added administrator and tenant scenario guides. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
|
975ea6acb7 |
ci(workflows): gate Talos builds on their inputs
Classify the rename-blind changed-path list against direct Talos sources and shared build inputs. This lets same-repository pull requests skip the heavy build while removals, renames, and build configuration changes still select it. Fork builds remain unconditional. Preserve the matchbox digest produced by a successful Talos build when finalize overlays base-branch image references. When the job is skipped, bootbox continues to receive the current base reference. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
|
774f788016 |
ci(agents): check commit trailers on every pull request (#4103)
## What this PR does `docs/agents/contributing.md` makes a few properties of a commit message blocking at review: the attribution trailer takes the value `LLM` and nothing else and appears once, no message carries a session trailer or a link to a transcript, no line is a generation byline naming the tool. Nothing checked any of it, so the only reader was a human scrolling `git log`. That reader is expensive and misses things. One open PR carried eight commits naming a model in the trailer, another four, all found by hand after the work was done, each one costing a review round and a rebase. `hack/check-commit-trailers.sh` decides all of it from the text, and the `Commit trailers` job in `.github/workflows/pre-commit.yml` runs it over the commits between the merge base and the head. Over all 6062 commits on `main` it rejects 1300, every one a true positive and no prose false-fires: 1289 attribution trailers naming a model, 24 `Generated with <tool>` bylines, 15 commits carrying two trailers, two `claude.ai/code/session_...` links. ### Where it lives Next to the other per-PR text lint in `pre-commit.yml`. The unit-test job in `pull-requests.yaml` was the other candidate and is wrong for this: it is gated on the diff touching code, so a docs-only PR skips it, and a trailer is wrong or right whatever files the commit changed. The range is `merge-base..head`, recomputed every run, so a rebase or a force-push is read as it now is. It never reaches the history on `main`, roughly a quarter of which carries the old form. ### Decisions worth arguing with It fails rather than warns. A warning in a job nobody watches guards nothing. It is not a required check, and that part is deliberate. Branch protection on `main` requires `pre-commit` and `E2E Tests`, and `Commit trailers` is a new context, so a PR can still merge with it red. 77 of the 200 open PRs carry at least one commit this rejects, so requiring it today turns those red at their next push. Flipping it is a branch-protection change once the open set has drained, and somebody else's call. Fork PRs are checked like any other. The job runs on `pull_request` with a read-only token and reads commit messages as data. A merge commit with no trailer of its own passes. Nothing is required of a commit, only what is written is judged, so a merge commit that does carry a bad trailer fails like the rest. Backport branches are exempt. The backport action cherry-picks merged commits verbatim, so those messages are history and not the author's to rewrite. The exemption matches the generated name `backport-<pr>-to-<branch>`, which is the only signal a `pull_request` event carries, since the label sits on the original PR. So it is a convention, not something enforced. The checker is read from the base branch instead of the checked-out PR. A `pull_request` event takes the workflow definition from the merge of base and head, so the job shows up on every open PR the moment it lands, including the ones whose head predates the script, where reading it from the head is exit 127 and a red check saying nothing about trailers. It also keeps the checker out of reach of the PR being measured, though the job around it is not. Two shapes are left to the reviewer. A model named in `Co-authored-by:` needs a list of model names to tell it from a person, and such a list either misses next month's model or fires on somebody's colleague; most of the 771 such lines on `main` name a model, so it would be deciding a lot of PRs. A `Generated-by:` trailer has never appeared here at all. The byline rule matches a capital `G` for a related reason: case-insensitively, one line in 25 on `main` is an ordinary sentence that wrapped onto "generated with cozyvalues-gen". ### Known costs `hack/check-commit-trailers.sh` matches `hack/[^/]+\.sh$` in `select-e2e.sh`'s `full_suite_pattern`, so this PR and any later one touching the script escalates to the full e2e suite. That pattern's own comment scopes it to the e2e harness, which this script is not, and `inert_config_pattern` already carries `hack/[^/]+\.bats$`. Escalation is fail-safe so it is cost rather than breakage, and reclassifying it wants its own review. The step drops the explicit `git fetch origin "$BASE_REF"` that `api-review-gate.yaml` keeps before its own `merge-base`. `fetch-depth: 0` already fetches `+refs/heads/*:refs/remotes/origin/*`, and if that were ever wrong the failure is a loud `merge-base` error under `set -euo pipefail`, not a silent pass. ### Screenshots Not a UI change. ### Downstream repositories Walked the trigger map against the diff. The two entries that could fire are `cozystack/ccp` and `cozystack/community`, both keyed on a change to the commit convention itself. The convention text is untouched here, only its enforcement is new, so both copies stay accurate. `ccp`'s `package-bump` skill already writes `Assisted-by: LLM`, which this check accepts. - [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 ci(agents): a pull request now fails when one of its commits carries an attribution trailer whose value is not `LLM`, a second attribution trailer, a session trailer or transcript link, or a `Generated with <tool>` byline. The check reads only the commits the pull request adds; backport branches are exempt. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Pull requests now include automated checks for commit attribution trailers, session links, and generation bylines. * Backport branches are excluded from this validation. * **Documentation** * Added guidance describing the commit-message checks, accepted attribution formats, exceptions, and how to run the validation locally. * **Tests** * Added coverage for valid and invalid commit-message formats, range handling, merge commits, and exempted content. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |