mirror of
https://github.com/cozystack/cozystack.git
synced 2026-11-09 05:25:14 +09:00
fix/bats-runner-coverage
3348
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Myasnikov Daniil
|
3fbbcd1537 |
test(hack): fail when a bats file is run by no runner
Two suites sat in hack/e2e-apps/ for three weeks without executing once, and nothing in the tree could notice: the property "something runs this file" was described in prose in three comments and asserted nowhere. This adds the assertion. There are exactly two runners and that is a closed list by design -- the root Makefile's one-level, non-e2e glob, and the literal suite names in packages/core/testing/Makefile. A file matching neither is reported, as is the inverse: a name in the e2e Makefile with no file on disk, which today surfaces forty minutes into the e2e job, or wedges the bootstrap outright. A suite named through a make variable, the shape of the rule #2826 deleted, is reported as unresolvable rather than expanded, because a pattern rule nobody invokes is as dead as no rule. The Makefiles are parsed rather than asked. hack/common-envs.mk is included at Makefile:3 and runs `git remote add upstream` and `git fetch upstream --tags` in $(shell ...) at parse time, so `make -p` or `make -n` would have a unit test mutate git remotes and reach the network to answer a question about filenames. The cost is that the model can drift from what it models, so a second test pins the modelled lines verbatim -- that is the one direction which would otherwise widen the covered set in silence. No allowlist, deliberately: the off-switch is the .disabled suffix, which the scan stops seeing on its own. An in-test list of exempt paths would be the same artefact class as the runner that got deleted while its documentation stayed. That documentation is corrected here too. Makefile's comment claimed a new .bats file in hack/ is picked up automatically, which holds only at the top level and is the false claim this bug grew in. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
f6b30e2928 |
test(monitoring): pin the oidc values passthrough to the inner chart
The wrapper hands the whole of .Values to the inner chart verbatim, so that one line is the only path by which a tenant-set spec.oidc reaches the chart which renders it. Nothing pinned that hop: the inner chart's own suite sets its values directly, so a wrapper that dropped or renamed a key would still render a correct inner chart -- from values no tenant can actually set. Verified by mutation: replacing the passthrough with `omit .Values "oidc"` fails exactly this case's three assertions and leaves the other three cases in the file green. This is the one item from the deleted monitoring OIDC bats that live coverage was genuinely carrying and unit coverage was not, and it costs no cluster. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Aleksei Sviridkin
|
1c00ddb0f3 |
fix(platform): give node DaemonSets memory requests and limits (#3495)
## What this PR does Sets memory requests and limits on the node DaemonSets that had none. Companion to the namespace-wide LimitRange; the reasoning for why a limit specifically (and not a request, and not a `PriorityClass`) is in that PR and in `docs/operations/system-memory-limits.md`. Requests come from 14-day peak working set measured on a 3-node dev cluster. Limits are deliberately loose, sized as a runaway backstop rather than a fitted ceiling, for two reasons: several of these scale with cluster size in ways a small dev cluster understates (cilium with endpoint and policy count, fluent-bit with log volume, multus and kube-ovn with pods per node, velero node-agent during a backup), and undersizing converts a rare pressure-driven kill into a deterministic one. This repository already carries a scar from that failure mode, recorded in the `vertical-pod-autoscaler` values where a 110Mi limit OOM-killed the updater and cascaded into failed HelmReleases. Two DaemonSets are not chart-rendered and needed their own seam. `linstor-satellite` goes through the `LinstorSatelliteConfiguration` podTemplate, including the two plunger sidecars — every container in the pod needs a limit for the pod cgroup to get `memory.max`, so one limit-free sidecar would put the whole pod back in the victim set. `virt-handler` goes through KubeVirt `customizeComponents.patches`, because virt-operator owns that DaemonSet and chart values cannot reach it. `linstor-controller` is deliberately left limit-free. It is a JVM that must not be OOM-killed mid-operation, and `cluster_test.yaml` pins that as an invariant. The satellite is a Go binary with a bounded working set, so the opposite call applies there. Not covered, because upstream hardcodes the containers with no `resources` knob: the four frr-k8s `cp-*` init containers, the kube-ovn `hostpath-init` and `install-cni` init containers, and `cozy-proxy`, whose vendored chart exposes no resources value at all. `hami` and `kilo` are left alone as optional components with no usage measurements behind them. All of these still receive a limit from the namespace LimitRange, so they are not left exposed — they simply carry no request. Worth stating plainly: this PR does **not** grant OOM immunity on its own, because the init containers it cannot reach leave the pod cgroup without `memory.max`. That only closes with the LimitRange PR in place. ### Verification Every touched package renders and its helm-unittest suite passes. New assertions cover the metallb speaker and all five frr-k8s containers, the linstor satellite and plunger sidecars, and the KubeVirt patch. The metallb suite was mutation-tested: removing the frr-k8s reloader limit fails it. One gap. The KubeVirt patch shape was validated against the live CRD and pushed through the real update path with `kubectl patch --dry-run=server` (accepted; live object confirmed unchanged), but virt-operator actually reconciling the DaemonSet with those resources has **not** been observed, because that means restarting virt-handler fleet-wide. Worth confirming in e2e or on a dedicated stand before merge. ### Downstream repositories Walked the trigger map against the diff. No entry is triggered: this changes chart values and cozystack-owned templates under `packages/system/`, not the `cozy-proxy` label and annotation contract, not node prerequisites, not an app schema or version enum. Leaving the boxes empty rather than ticking "none affected", because the diff surfaced a real gap in a downstream repository even though nothing here forces a change there: the vendored `cozy-proxy` chart exposes no `resources` value, so its DaemonSet cannot be given one from this side at all. That wants an issue or PR in cozystack/cozy-proxy followed by a re-vendor. Flagging it for a maintainer decision rather than filing speculatively. ### Release note ```release-note fix(platform): node DaemonSets (cilium, metallb speaker and frr-k8s, kube-ovn, linstor-satellite, virt-handler, fluent-bit, node-exporter, multus, velero node-agent, kubevirt-csi-node) now declare memory requests and limits, so they are no longer selected as victims by the Talos userspace OOM handler when an unrelated workload drives node memory pressure. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Resource Management** - Added CPU and memory requests and limits across Cilium, KubeVirt CSI, KubeVirt, Linstor, MetalLB, monitoring agents, Multus, and Velero workloads. - Improved workload stability with defined memory ceilings and resource reservations. - **Multus** - Moved resources to the dedicated `cozy-multus` namespace and configured Cilium as the master CNI. - Added optional CNI plugin staging with enhanced security settings. - **Tests** - Added and expanded Helm tests validating resource configurations and Multus behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
22ce998890 |
fix(flux-shard-operator): repair sharded helm-controller crashloop behind an HTTP proxy (#3546)
## What this PR does Fixes #3544. `flux-shard-operator` clones the flux-aio `helm-controller` container into each `helm-controller-shard<i>` Deployment. It already sanitises the all-in-one localhost wiring, but keeps two things that stop the standalone shard from starting behind a corporate HTTP proxy: 1. **Inherited `HTTP_PROXY`/`HTTPS_PROXY`/`NO_PROXY`.** A standalone shard needs no external egress (source-controller fetches artifacts). Behind an unreachable proxy the controller makes a blocking startup HTTPS call that never completes, the manager never serves `/healthz`, and the liveness probe crashloops the pod forever. flux-aio survives only because it starts once and never restarts. On an affected cluster every HelmRelease carrying a `sharding.fluxcd.io/key` (150 of ~240) was frozen for days, silently. 2. **No `startupProbe`.** The clone inherits only a ~30s liveness window, so any slow start (large cache sync) is fatal and unrecoverable. This PR extends the sanitisation to drop the proxy env, and derives a `startupProbe` from the liveness handler with a generous budget so a slow-but-progressing start is not killed. Verified by an A/B on a live cluster (throwaway helm-controller with a non-matching selector): with the proxy env it hangs; without it, it starts cleanly (`Starting workers`). ### Downstream repositories - [x] No downstream repository is affected by this change Internal operator behavior only; no API/CRD/values contract change. ### Release note ```release-note fix(flux-shard-operator): sharded helm-controller no longer crashloops at startup behind an HTTP proxy; the cloned controller drops the inherited proxy env and gains a startupProbe, so HelmReleases assigned to shards keep reconciling in proxied environments. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Prevented inherited proxy settings, including uppercase and lowercase variants of `NO_PROXY`, from affecting Flux shard deployments. * Automatically adds a startup health check when only a liveness check is configured. * Preserved the liveness check’s handler and timeout while applying appropriate startup timing and failure settings. * **Documentation** * Documented startup health checks for slow helm-controller startup. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Myasnikov Daniil
|
4f9c20cac0 |
fix(backupstrategy-controller): gate the default MongoDB strategy on bootstrap
A fresh install of packages/system/backupstrategy-controller fails permanently and applies nothing: Helm install failed for release cozy-backup-controller/backupstrategy-controller resource mapping not found for name: "cozy-default-mongodb" no matches for kind "MongoDB" in version "strategy.backups.cozystack.io/v1alpha1" The chart ships its own strategy.backups.cozystack.io CRDs as ordinary templates (templates/crds.yaml globs definitions/*.yaml), and Helm resolves every document in a release manifest through the cluster's RESTMapper in Build() before it applies any of them. An ungated Strategy CR therefore has no mapping on the first install, Build() fails, and the release applies ZERO objects, including the CRDs that would have made the mapping resolve. Nothing recovers: every retry renders the same manifest, so this is permanent rather than a race, and Helm's kind sorter cannot help because it orders the apply, not the Build. The seven sibling strategies never hit this because each gates on a resolved $bucketName, which is empty until the COSI BucketClaim status is reconciled. That gate is documented as "the CR needs the bucket name", and it silently doubles as the CRD-ordering guard. MongoDB genuinely does not need the bucket name, so the gate was left off and the accidental protection went with it. Gate it too, for the ordering reason rather than the value, and record that reason in the template so the next bucket-independent strategy keeps the gate. Convergence is unchanged: DefaultObjectsGate already routes apps.cozystack.io/MongoDB to cozy-default-mongodb and forces a second revision once any routed object is missing, which is how the seven siblings materialise today. Verified by rendering both windows. Before: the bootstrap render emits one Strategy, MongoDB, alongside the eight not-yet-applied CRDs. After: it emits none, and with backupStorage.bucketNameOverride set it emits all eight. tests/rendering_test.yaml asserted count: 1 for MongoDB under a case named "bootstrap window (no resolvable bucket name)", so the defect was codified green. helm-unittest has no RESTMapper and cannot see it. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Aleksei Sviridkin
|
abd331b2c8 |
test(system): anchor index-addressed negative asserts to what they name (#3608)
## What this PR does
Some negative assertions in these suites can't fail for the reason their
test names give. `contains` and `notContains` compare list elements for
equality, so `--pinned-tenants=` never equals a rendered
`--pinned-tenants=tenant-a=shard0`, and `{name:
GF_AUTH_GENERIC_OAUTH_CLIENT_SECRET}` never matches an env entry
carrying a `valueFrom` beside the name. Others forbid one rendered value
under a name that claims the flag itself, so `notContains
--compatibility=cilium` leaves the kilo default render free to carry
`--compatibility=flannel` off the same values key.
Flag guards become `notMatchRegex` on `args[*]`, which matches per
element, so any value fails and so does the bare form. Mapping guards
get `any: true`. flux-shard-operator gets an `equal` on the whole
default arg list instead, and `--shard-cpu-limit=` is dropped because
that list covers it. The vendored ouroboros templates keep per-flag
guards: a whole-list pin there would go red on any upstream flag change,
under a test name about one flag.
Several of these address `containers[0]` with nothing pinning that
index, so a sidecar rendered ahead of the workload turns them green
while the flag is live on the real container. Those get an `equal` on
the container name first. A Cilium deny rule has no name, so that one
pins the selector plus the rule and `toPorts` counts. Where a positive
`contains` already sits on the same path it holds the index by itself
and gets no extra anchor.
Assertions of the same shape remain in files this PR doesn't touch,
including one in a package it does.
## Tests
Every dead guard was confirmed dead first: make the forbidden content
render, watch the old form stay green. `compatibility: flannel` in
kilo's values, `--gateway-api=true` in place of the bare flag,
`--proxy-fqdn` baked beside the composition flags, the OIDC client
secret in the `mode=None` grafana render. Each one turns the new
assertion red. Anchors checked the same way, with a sidecar rendered
ahead of the workload.
`hack/helm-unit-tests.sh` is green.
### Screenshots
None. This changes test files.
### Downstream repositories
Every changed file is under a `tests/` directory. No templates, no
values, no rendered output.
- [x] No downstream repository is affected by this change
- [ ] [cozystack/website](https://github.com/cozystack/website) -
follow-up:
- [ ]
[cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack)
- follow-up:
- [ ]
[cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack)
- follow-up:
- [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up:
- [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up:
- [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) -
follow-up:
- [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) -
follow-up:
- [ ]
[cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server)
- follow-up:
- [ ]
[cozystack/external-apps-example](https://github.com/cozystack/external-apps-example)
- follow-up:
- [ ] [cozystack/examples](https://github.com/cozystack/examples) -
follow-up:
### Release note
```release-note
NONE
```
|
||
|
Aleksei Sviridkin
|
9dc49de1c9 |
fix(apps): make the post-delete cleanup hooks report failure (#3593)
## What this PR does The qdrant and harbor post-delete cleanup hooks could not fail. Every delete was guarded by `|| echo "WARNING: ..." >&2`, neither script set `-e`, and both ended on an unconditional completion message, so the container exited 0 on every path. Helm read that as a successful hook, `helm.sh/hook-delete-policy: hook-succeeded` removed the Job together with the pod that held the warning, and an uninstall that reclaimed nothing looked exactly like one that had nothing to reclaim. Written up in #3566. Both hooks now count failures and set the exit code at the end. `set -e` would abort at the first failed delete and skip the rest of the sweep, which is the opposite of what is wanted: every remaining object should still be attempted and the outcome reported once. Already-gone objects stay a success, since every delete keeps `--ignore-not-found`. Each failure message names the object it left behind. `packages/apps/mariadb` already uses this shape. Both hooks also consumed `kubectl get` output directly: harbor by iterating `$(kubectl get ...)`, qdrant by piping it into `grep`, whose status the pipeline then reports. A failed listing arrived as an empty list, and the sweep reported success without having looked. Each listing is now captured before it is used. #3566 attributes that shape to harbor's two loops alone. qdrant has it too: `pvcs=$(kubectl get pvc ... | grep -E ... || true)` reports grep's status, so a `kubectl get` that failed left `$pvcs` empty and the hook announced "no data PVCs found; nothing to delete". Both instances are fixed here. ### The cost A hook that exits non-zero keeps its Job and its pods, because `hook-succeeded` does not fire for it. That is the point, the pods hold the log, but it means a failed uninstall now leaves objects behind where it previously left none. Nothing collects them on its own: `before-hook-creation` only fires when the hook runs again, and a `post-delete` hook runs only on an uninstall, so they stay until a release of that name is installed and uninstalled again, or until the namespace goes. The release itself is not stranded. Helm appends the post-delete hook error to its error list instead of returning on it, marks the release uninstalled, purges it from storage anyway, and only then reports `uninstallation completed with N error(s)`. The failure surfaces once and the app still goes away. Both Jobs get one retry instead of the Kubernetes default of six, and instead of none. Every delete is idempotent, so a second attempt re-issues an API call rather than re-running any logic, and one short backoff separates a transient apiserver error from a real one. These failures become visible for the first time with this change, and a signal that fires on noise stops being read, including on the day it is right. Six is the far end of the same mistake: ten minutes of backoff across seven pods outlasts the hook timeout, so Helm reports that timeout instead of the hook's own error. `packages/apps/clickhouse` and `packages/apps/mariadb` already exit non-zero from their cleanup hooks and set no retry limit, so a real failure in either spends the full backoff and reaches the operator as a timeout rather than as the error the hook wrote. Adding the exit code without the cap reproduces that. One limitation is deliberate. harbor's ACME sweep cannot tell "this kind is not served" from "I could not look". If cert-manager is torn down before the release, `kubectl get challenge` fails and the uninstall is reported failed with nothing to sweep. cert-manager is a hard dependency of the bundle harbor ships in, so the window is narrow, and a CRD-existence probe would add a second silent-skip path of its own: it would swallow a discovery failure the way this change stops swallowing a listing failure. ### Tests `hack/app-cleanup-hooks_test.bats` renders each hook's script out of its chart and runs it against a fake `kubectl`, because the property under test is the container's exit code and matching against manifest text cannot establish it. There are eight failure paths across the two hooks, six in harbor and two in qdrant, and each has a case that goes red when that one path alone is reverted to a swallowed warning. Also covered: a clean delete, and an object that is already gone in both its shapes, an empty listing and a `NotFound` on the delete itself. The fake `kubectl` models `--ignore-not-found` instead of exiting 0 unconditionally, so dropping that flag from a hook turns the already-gone cases red. Message assertions are anchored on the hook's own `ERROR: ` prefix, because the fake names the object in its own diagnostic too, and an unanchored match is satisfied by the fixture. Two manifest fields are pinned in each chart's existing helm-unittest suite, where manifest fields belong. The retry cap is one. The other is `helm.sh/hook-delete-policy`: adding `hook-failed` to that annotation hands the failed Job straight back to Helm to delete, which undoes the whole change. I checked that before adding the assert, and the reversal left both suites fully green. The qdrant Chainsaw suite explained its post-delete pod collector by quoting the guard string this change removes, and concluded that Helm always collects the pod away. That conclusion inverts here, so the note is rewritten, along with the sentence in the harbor suite that points at it. ### Screenshots Not a UI change. ### Downstream repositories I walked the trigger map in `docs/agents/contributing.md` against the diff, file by file. It touches two app chart templates and adds one file under `hack/`. No package added, renamed or removed, no `values.schema.json`, no `values.yaml` default, no version enum, no `ApplicationDefinition`, no namespace, no node prerequisite, and neither `hack/package.mk` nor `hack/update-crd.sh`. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note ```release-note fix(apps): the qdrant and harbor post-delete cleanup hooks now exit non-zero when a delete or a listing fails, instead of reporting success on every path. `helm uninstall` reports the failure rather than returning clean, and the hook Job and its pods stay in the namespace so the log survives to be read. Nothing collects them on its own: they remain until a release of that name is installed and uninstalled again, or until the namespace is removed. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved Harbor and Qdrant cleanup reliability by detecting listing and deletion failures. - Cleanup jobs now retry once and report failures with a non-zero status. - Cleanup continues processing remaining resources after individual failures. - Successful cleanup remains safe and repeatable, including when resources are already missing. - Harbor cleanup remains limited to resources associated with the configured Harbor host. - **Tests** - Added comprehensive coverage for successful, repeated, partial, and failed cleanup scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
d2551b6ea7 |
fix(keycloak): keep KeycloakRealmUser roles when spec.roles is unset (#3531)
<!-- Thank you for making a contribution! Here are some tips for you: - Use Conventional Commits for the PR title: `type(scope): description` - Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore - Scopes are not an exhaustive list — pick the most specific scope for the change and extend the list when a genuinely new area appears. Examples: - System components: dashboard, platform, operator, cilium, kube-ovn, linstor, fluxcd, cluster-api - Managed apps: postgres, mariadb, redis, kafka, clickhouse, virtual-machine, kubernetes - Development and maintenance: api, hack, tests, ci, docs, maintenance - Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or add a `BREAKING CHANGE:` footer - If it's a work in progress, consider creating this PR as a draft. - Don't hesistate to ask for opinion and review in the community chats, even if it's still a draft. - Add the label `backport` if it's a bugfix that needs to be backported to a previous version. --> ## What this PR does The vendored keycloak-operator image is built from a pinned upstream commit with `images/keycloak-operator/patches/*.diff` applied at build time. This patch is a backport of epam/edp-keycloak-operator#372 onto that pin, so it is dropped in the same commit that moves `COMMIT_REF` past the upstream merge. The bug is epam/edp-keycloak-operator#371. Under the default `full` strategy the operator treats `spec.roles` and `spec.clientRoles[].roles` as authoritative sets: after adding what is missing, it deletes every mapping the spec does not list. A resource that manages the user record and leaves those fields out therefore loses all of its realm roles on the first reconcile, `default-roles-<realm>` included, and with it `offline_access`, `uma_authorization` and the `account` client roles. The user keeps logging in and stops being able to do anything. Upstream fixes this in two halves that only work together. The controller returns early when the desired list is nil, so an omitted field means "not managed" while an explicit `roles: []` keeps its meaning of "remove every role". The json tags on `Roles` and `ClientRoles[].Roles` lose `omitempty`, because with it an explicit empty list is erased the first time the operator writes the object back, which adding its own finalizer already does, and comes back indistinguishable from an omitted field. Take that half away and the distinction holds only until the operator next touches the object. All eight files from the upstream PR are here. Six apply to the pin unchanged. `sync_user_roles.go` and its test are rewritten against the pinned context, where the generated client package is still named `keycloakv2` (upstream renamed it to `keycloakapi` after the pin, which is the only reason those two do not apply as-is). Tests match upstream: four controller cases for unset and empty realm and client roles, two in `api/v1` for the serialization round trip. Checked by extracting the pinned tarball, applying every `.diff` in the patches directory in the order the Dockerfile globs them, then `go build ./...` and `go test ./internal/controller/keycloakrealmuser/... ./api/...`, all green. Reverting each of the three conditions in turn fails exactly the tests that cover it, and nothing else. One caveat a reader deserves up front: this patch does not reach any cluster yet. No CI path rebuilds this package, so `hack/build-matrix.sh` returns an empty matrix for a change confined to it, and `values.yaml` keeps pointing at an image built by hand in March. The same happened to `9c5fbb69a` and `5b9b420ff`, which edited this Dockerfile and left the pin alone, so the running binary is already older than the tree says. That gap is tracked in https://github.com/cozystack/cozystack/issues/3416 and is deliberately not addressed here: the fix for it is a choice between adding these packages to the build list and dropping the in-repo build, and the first option wakes a known mirror gap for split-host references. Landing the patch now means it is in place the moment that is resolved. ### Screenshots <!-- REQUIRED for UI changes: attach screenshots or screen recordings demonstrating the visual impact of your changes. PRs with UI changes without screenshots will not be merged. --> Not a UI change. ### Downstream repositories <!-- Cozystack is upstream for repositories that are NOT kept in sync with it automatically, and nothing in CI compares the two sides, so a change here can break them silently. Which change forces what, and which file to touch, is the trigger map in docs/agents/contributing.md under "Downstream Repositories". Walk it before you tick anything. IF YOU ARE AN AI AGENT OR AN LLM, THIS IS PART OF THE TASK, NOT A FORM TO FILL IN: 1. Walk the trigger map against the actual diff of this PR, file by file. Do not decide from the PR title or from your memory of what you changed. 2. For every repository you tick, open the follow-up PR there and link it on the same line. A ticked box with no link claims work that does not exist, which is worse than an empty box. Search that repository for an open PR or issue covering it first, and link that instead of filing a duplicate. 3. When the follow-up is out of scope, or needs a decision that is not yours to make, open an issue in that repository instead, link that, and say so in the PR body. Do not push a speculative PR into another repository just to fill a line here. 4. Do not tick "No downstream repository is affected" to make the checklist go away. If you are unsure, leave every box empty and say so in the PR body, so a human decides. --> Walked the trigger map against the diff. It touches one build-time patch file under `packages/system/keycloak-operator/images/`, adds no package under `packages/apps/` or `packages/extra/`, and changes no values key, schema, enum, default, CRD, `ApplicationDefinition`, namespace, label, annotation, metric, release asset, variant or node prerequisite. Nothing in the map fires. - [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 <!-- Write a release note: - Explain what has changed internally and for users. - Start with the same `type(scope):` prefix as in the PR title - Follow the guidelines at https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md. --> ```release-note fix(keycloak): a `KeycloakRealmUser` that omits `spec.roles` or leaves `roles` out of a `spec.clientRoles` entry no longer has the matching Keycloak role mappings deleted under the default `full` reconciliation strategy. Users keep `default-roles-<realm>` and the `offline_access`, `uma_authorization` and `account` roles it grants. Setting an explicit empty list still removes every role. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved Keycloak role management for realm users and client roles. * Leaving a role list unspecified now preserves existing roles. * Explicitly providing an empty role list now removes all assigned roles. * **Documentation** * Updated API and resource documentation to clarify role-list behavior. * **Tests** * Added coverage for omitted, empty, managed, and unmanaged role configurations. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
3d4e8c07e9 |
test(tenant): anchor the operator-gate absence and refresh a spent reason (#3661)
## What this PR does Two tests in `packages/apps/tenant/tests/vmo_webhook_dependson_test.yaml` say the seaweedfs and gateway HelmReleases deliberately carry no operator gate, and assert that with `notExists: spec.dependsOn` and nothing else. A bare absence assert also passes when the template renders no document at all, so both stayed green with those two releases switched off entirely. With `seaweedfs: false, gateway: false` the suite still reported 5 of 5 passing; pinning `metadata.name` next to the absence turns the same run into 2 failures. An earlier check on these asserts injected a `dependsOn` and watched them redden. That proves they catch an added gate, which is the other direction. For a negative assert the direction that matters is the document going away, and that one was never covered. Pairing `equal: metadata.name` with the absence is not a new idiom introduced here. `no_upgrade_force_test.yaml`, in the same directory, already anchors all seven of its releases that way, and this brings the last two suites in the chart into line with it. Both suite headers also carried a reason for their `templates:` list that has expired: that rendering the whole chart fails because `keycloakgroups.yaml` indexes `.Values._cluster` unguarded. #3636 added the guard, so a no-values render of the chart now succeeds. The list is still load-bearing, for the reason `no_upgrade_force_test.yaml` already records — it keeps an unrelated template breaking out of this suite's result. Checked by adding a nil dereference to a template neither suite asserts on: both suites stay green with their lists and error without them. ### Screenshots Not a UI change. ### Downstream repositories Walked the trigger map against the diff. It touches two files, both under `packages/apps/tenant/tests/`, and no chart, template, values file or tooling. Nothing in the map keys on test files. - [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 ``` |
||
|
Myasnikov Daniil
|
c9ba7a3cca |
fix(platform): give node DaemonSets memory requests and limits
The Talos userspace OOM handler (v1.12+) ranks cgroups with
memory_max.hasValue() ? 0.0 :
{Besteffort: 1.0, Burstable: 0.5, ...}[class] * memory_current
and discards every cgroup scoring zero, so only a memory limit takes a
pod out of the victim set. Selection is decoupled from the trigger: a
tenant workload thrashing against its own limit drives node memory PSI,
and a limit-free system DaemonSet is killed for it. metallb-speaker and
linstor-satellite were the observed victims.
Set requests and limits on the node DaemonSets that had none. Requests
come from 14d peak working set measured on a 3-node dev cluster; limits
are deliberately loose, sized as a runaway backstop rather than a fitted
ceiling, because several of these scale with cluster size that a dev
cluster understates and because undersizing converts a rare
pressure-driven kill into a deterministic one. A limit costs nothing
until it is hit -- only the request reserves.
memory.max lands on the POD cgroup only when EVERY container in the pod
carries a limit, so sidecars and init containers are covered too; one
limit-free container returns the whole pod to the victim set.
Two DaemonSets are not chart-rendered and needed their own seam:
linstor-satellite via the LinstorSatelliteConfiguration podTemplate, and
virt-handler via KubeVirt customizeComponents.patches, since virt-operator
owns that DaemonSet. Note linstor-controller stays deliberately limit-free
-- it is a JVM that must not be OOM-killed mid-operation, and cluster_test
pins that.
Two things the multus manifest needed beyond the values change.
customize-deployment.patch is the source for that template, so the
limits are carried there rather than only in the generated file, which
`make update` would discard. And the reason kube-multus had no memory
limit was that this patch DELETED upstream's 50Mi rather than raising
it -- 50Mi being well under the 230Mi measured peak -- which is what
removed memory.max from the pod cgroup in the first place.
Not covered, because upstream hardcodes the containers with no resources
knob: the four frr-k8s cp-* init containers, the kube-ovn hostpath-init
and install-cni init containers, and cozy-proxy, whose vendored chart
exposes no resources value at all. hami and kilo are left alone as
optional components with no usage measurements behind them. All of these
still get a limit from the default LimitRange the operator maintains in
every system namespace.
Every limit is pinned by a helm-unittest assertion, and every assertion
was mutation-tested: changing the rendered value turns its suite red.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
|
||
|
Myasnikov Daniil
|
cf87201099 |
test(monitoring-agents): run this package's helm suite
hack/helm-unit-tests.sh discovers work by probing each package for a `test` Makefile target, so a package without one is skipped in silence however many suites sit under tests/. monitoring-agents had tests/fluent-bit_probes_test.yaml and no target, so that suite had never run once since it was written. It passes; the only thing missing was the two lines that reach it. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
IvanHunters
|
09dc7f47fb |
feat(backups): add a native MongoDB (psmdb) backup-strategy driver (#3562)
## What this PR does Adds a native MongoDB (Percona Server for MongoDB / psmdb) backup-strategy driver so `apps.cozystack.io/MongoDB` reaches feature parity with the CNPG, MariaDB and ClickHouse native drivers — logical backups, restore into a differently-named instance, and point-in-time recovery — all through the unified `BackupJob` / `RestoreJob` / `BackupClass` / `Plan` interface (closes #3545). The driver orchestrates the operator's `PerconaServerMongoDBBackup` / `PerconaServerMongoDBRestore` CRDs: - **BackupJob**: creates a one-shot `PerconaServerMongoDBBackup`, polls `status.state`, and persists the S3 destination + storage descriptor (by reference — Secret name only, never a raw credential) onto the Cozystack `Backup` artifact. - **RestoreJob**: drives `PerconaServerMongoDBRestore` via `backupSource` (read back from the source backup status, with a snapshot fallback for a reaped operator CR) so the same artifact restores in-place **or** into a differently-named instance. PITR is exposed as `spec.options.recoveryTime` (RFC3339), matching the CNPG driver's option exactly; unrecognised `spec.options` keys are surfaced as a `UnknownRestoreOption` Warning event rather than silently dropped. Because psmdb resolves the backup storage by name against the cluster's `spec.backup.storages`, the driver names the chart's storage (`s3-storage`) via the `cozy-default-mongodb` Strategy rather than injecting it, and surfaces a clear `Ready=False` precondition when a target has backups disabled instead of hanging until the deadline. **Wiring** - `api/backups/strategy/v1alpha1`: `MongoDB` strategy type + generated CRD/deepcopy. - `internal/backupcontroller`: `psmdbstrategy_controller` + `psmdbtypes` / `mongodbapp` partial-schema mirrors; BackupJob/RestoreJob dispatch, supported-kinds, cleanup-on-delete branch, and scheme registration. - `backupstrategy-controller`: `cozy-default` MongoDB route, `cozy-default-mongodb` Strategy CR, and `psmdb.percona.com` RBAC (get/list/watch, +create on backups/restores, no delete — the driver does not own the S3 archive). - `apps/mongodb`: a `backup.insecureSkipTLSVerify` value (psmdb's s3 storage has no CA-bundle field) so backups can target Cozystack's self-signed in-cluster seaweedfs. Default `false`, fully backward-compatible; regenerated schema/README/RD. **Tests + e2e** - Unit tests for the driver: applicationRef validation, ensure-idempotency (backup + restore), the backups-enabled/storage precondition gate, `backupSource` resolution (live-preferred / snapshot fallback / no-destination), snapshot round-trip, the producer-seam (`createMongoDBBackupArtifact`) driver-metadata, the cleanup-on-delete no-Velero-fall-through guard, and `recoveryTime` → psmdb-UTC mapping + unknown-option reporting. - `examples/backups/mongodb/` + a `mongodb-2-backup-roundtrip` Chainsaw suite that drives `run-all.sh` end-to-end (a sentinel document round-trips through S3 into a restored copy while the source stays untouched), per `docs/agents/e2e-testing.md`. - `docs/operations/backup-classes.md` updated (new route, the app-owns-the-storage model, and the `recoveryTime` PITR usage). Verified end-to-end on a live dev cluster against the platform-default `cozy-backups` bucket: backup via `cozy-default` reached the bucket and the Cozystack `Backup` went `Ready`, and a to-copy `RestoreJob` restored the sentinel into a differently-named instance with the source left intact. ### Screenshots N/A — no UI changes (controller, chart values, CRD, docs and tests only). ### Downstream repositories Walked the trigger map in `docs/agents/contributing.md` file by file against the diff: - **cozystack/website** — the only app-facing change is `packages/apps/mongodb` (a new `backup.insecureSkipTLSVerify` value + regenerated `README.md`). The managed-app reference page is regenerated from the package `README.md` by the release bot for apps already in the website `Makefile` (mongodb is one), so it refreshes automatically on the next tag — no manual follow-up. `docs/operations/backup-classes.md` is not a website-mirrored doc in the trigger map. - **cozystack/terraform-provider-cozystack** — the one candidate trigger ("a field added to an app's `values.schema.json`") lands inside the mongodb `backup` block, which the provider's `internal/provider/mongodb_schema.go` explicitly states it does **not** manage ("The deprecated backup block, the bootstrap restore block, and detailed shardingConfig are not managed"). No enum, default, `release.prefix`, Secret/Service name, or hand-typed CRD (`Plan`, `RestoreJob`) changed; the new `strategy.backups.cozystack.io/MongoDB` CRD is opaque to the provider. Not affected. - All other repositories — no matching trigger (no `hack/` layout or make-target change, no namespace rename, no `ApplicationDefinition` semantics change, no `packages/core/platform` values, no telemetry metric or `cozy-proxy` label rename). - [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 native MongoDB (psmdb) backup-strategy driver — logical backup to the cozy-default BackupClass, restore into a differently-named instance, and point-in-time recovery via spec.options.recoveryTime ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added MongoDB backup and restore support, including scheduled and on-demand backups, restores to new or existing clusters, and point-in-time recovery. * Added configurable S3 TLS certificate verification for MongoDB backups, defaulting to secure verification. * Added MongoDB support to the default backup configuration. * **Documentation** * Added MongoDB backup and restore examples, workflows, and operational guidance. * **Tests** * Added automated coverage for MongoDB backup, restore, cleanup, and end-to-end scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
c681ae1243 |
test(tenant): anchor the operator-gate absence and refresh a spent reason
The two "carries no operator gate" tests asserted only `notExists: spec.dependsOn`. A bare absence assert passes on a template that renders no document, so toggling `seaweedfs` and `gateway` off left both green while the releases they guard were gone. Measured: all 5 tests passed with the templates rendering nothing, and 2 of them fail once the name is pinned alongside the absence. An earlier check on these two asserts injected a `dependsOn` and watched them redden, which proves only that they catch an added gate. That is the other direction; the one that matters for a negative assert is the document disappearing. Both suite headers also justified their `templates:` list with a reason that has expired: keycloakgroups.yaml now guards its `.Values._cluster` index, so a no-values render of the whole chart succeeds. The list is still load-bearing, for the reason the sibling suite already records. It keeps an unrelated template out of this suite's result: with a nil dereference added outside the guard of a template neither suite asserts on, both suites stay green with their list and error without it. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
e9411d6b8c |
fix(apps): make the post-delete cleanup hooks report failure
The qdrant and harbor post-delete hooks guarded every delete with `|| echo "WARNING: ..." >&2`, set no `-e`, and ended on an unconditional completion message, so the container exited 0 on every path. Helm read that as a successful hook and the hook-succeeded delete policy removed the Job together with the pod that carried the warning, leaving an uninstall that reclaimed nothing indistinguishable from one that had nothing to reclaim. Count the failures instead and set the exit code at the end, so a failed delete does not skip the deletes that follow it. An object that is already gone stays a success: every delete keeps --ignore-not-found, which exits 0 in that case. This is the shape packages/apps/mariadb already uses for the same problem. Both hooks also consumed the output of `kubectl get` directly -- harbor by iterating it, qdrant by piping it into grep, whose status the pipeline then reports. A listing that failed therefore arrived as an empty list and the sweep was skipped rather than reported. Each listing is now captured before it is used, so a listing that failed and one that returned nothing are different events. A hook that exits non-zero keeps its Job and pods, which is the point: they hold the log. Give the Job one retry rather than the Kubernetes default of six or none at all. Every delete is idempotent, so a second attempt re-issues an API call rather than re-running any logic, and one short backoff is what separates a transient apiserver error from a real one -- these failures become visible for the first time here, and a signal that fires on noise stops being read, including on the day it is right. Six is the far end of the same mistake: ten minutes of backoff across seven pods outlasts the hook timeout, and Helm reports that timeout instead of the hook's own error. The qdrant Chainsaw suite explained its post-delete pod collector by quoting the guard string removed here and concluding the pod is always collected away. That is now backwards: a failed cleanup is exactly when the pod survives and the collector is the evidence. Rewrite the note, and the sentence in the harbor suite pointing at it. hack/app-cleanup-hooks_test.bats renders both scripts and runs them against a fake kubectl. There are eight failure paths across the two hooks -- six in harbor, two in qdrant -- and each has a case that goes red when that one path alone is reverted to a swallowed warning. Message assertions are anchored on the hook's own `ERROR: ` prefix, because the fake kubectl names the object in its own diagnostic too and an unanchored match is satisfied by the fixture. The helm-unittest suites pin the retry limit and the hook delete policy: adding hook-failed to that annotation hands the Job back to Helm to delete on failure and destroys the evidence this change exists to keep, and until these asserts that reversal left both suites green. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
5f8a820494 |
fix(keycloak): keep KeycloakRealmUser roles when spec.roles is unset
The operator reconciles user roles as an authoritative set. Under the default `full` strategy it deletes every realm role mapping missing from spec.roles, and every client role mapping missing from the matching spec.clientRoles entry. A consumer that manages only the user record and leaves those fields out therefore has all of its roles stripped on the first reconcile, `default-roles-<realm>` included, which costs the user offline_access, uma_authorization and the account client roles. Treat an omitted list as "not managed" and leave Keycloak untouched for that field, the same convention spec.identityProviders already uses. An empty but non-nil list keeps its meaning, so revoking every role is still possible with an explicit `roles: []`. Drop omitempty from the two json tags as well. Without that an explicit empty list is erased the first time the operator writes the object back, adding its own finalizer is enough, and returns indistinguishable from an omitted field, so the distinction does not survive a round trip. The patch mirrors epam/edp-keycloak-operator#372 file for file, so it can be dropped in one move once COMMIT_REF moves past the upstream merge. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
62204d4903 |
test(cozystack-controller): guard the Secret grant by effective permission
The suite's "does not grant a blanket wildcard verb on secrets" assert was a notContains over a rule object. helm-unittest compares every field it is handed by value equality, and `any: true` changes which fields are compared rather than how, so no spelling of that assert bounds what a verbs list may hold: `verbs: ["get", "*"]` satisfies all of them. Measured against the rendered ClusterRole, seven realistic spellings of a widened Secret grant left that assert green: a wildcard in resources, a wildcard in apiGroups, a "*" appended to the declared verb list, an extra field beside the verbs, deletecollection spelled out, and two where the grant is added as a separate rule. Those last two leave the whole suite green - appending apiGroups:['*'] resources:['*'] verbs:['*'] keeps the positive pin satisfied while the controller gains every verb on every Secret cluster-wide. Replace it with a bats guard over the rendered manifests. It unions the verbs that every ClusterRole and Role rule confers on core Secrets, expanding "*" on each of the three axes, and compares that union with the declared set. Equality rather than an upper bound: the controller is cluster-scoped, so a widening reaches every tenant namespace, and a narrowing breaks the two reconcilers that share the grant. The positive pin stays in tests/rbac_test.yaml. Value equality is the right primitive for pinning a rule as written and the wrong one for bounding what the rule may become. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
d73c102945 |
test(system): anchor index-addressed negative asserts to what they name
notContains passes whenever its path does not resolve, and also
whenever it resolves to something that simply lacks the forbidden
content. A different container at the same index qualifies, so these
guards go green the moment a sidecar is placed ahead of the workload
they were written for, while the flag or env var they forbid comes
back on the real container.
Assert the identity at that index first, in each affected case: the
container name for the Deployment-shaped ones, and for the Cilium
policy its selector, since a deny rule has no name. The selector
identifies that rule only while there is one rule to identify, so it
is paired with a length assertion, and with a second one on the
rule's toPorts, because that path carries two indexes and anchoring
the outer one alone leaves the inner free.
The anchor goes only where nothing else holds the index. A positive
contains on the same path fails on its own once a sidecar arrives
ahead of the workload, and an equal on the whole argument list fails
the same way, so neither carries a separate anchor. A negative does,
including the per-element form below: a wildcard resolving to the
sidecar's own arguments matches nothing and passes.
Several of the guards could not fail for the reason they name, and
the anchor alone would have left that hidden behind a comment saying
they were covered. contains and notContains compare elements for
equality, so a mapping is compared whole: `{name: GF_...}` never
matched an entry carrying a value beside the name, and `{port: 7473}`
would not match that port re-added with a protocol beside it, which
is how the neighbouring entry in the same list is written. Those get
`any: true`.
Equality against a rendered argument is the same failure worn
differently, and it arrives in three disguises. A prefix that never
equals an element: `--pinned-tenants=` never equalled
`--pinned-tenants=tenant-a=shard0`, and `--target-host=` never
equalled a rendered `--target-host=<value>`. One value out of many
under a name that claims the flag: `--compatibility=cilium` left the
kilo default render free to carry `--compatibility=flannel` off the
same values key, and the two `--proxy-fqdn` negatives forbade only
the FQDN the default cluster-domain produces. And a form that holds
by luck: `--gateway-api` still fails today only because that template
renders it bare, so an upstream switch to `--gateway-api=true` would
retire the guard in silence.
All of those become notMatchRegex over args[*], which matches the
flag name per element and fails on any value and on the bare form
alike. A length assertion would not have covered the prefix cases,
since an upstream bump that adds the flag while consolidating another
argument keeps the count. A wildcard that resolves to nothing still
passes, so the index anchors stay.
The whole argument list is a different instrument, answering what the
default invocation is rather than whether one flag is absent. It
fails on an argument added, removed or swapped instead of only on the
ones someone thought to enumerate, which is worth having where the
template is first-party and a failure is a deliberate change asking
to be read. flux-shard-operator gets it in `renders the default
Deployment shape`, replacing eleven individual contains that listed
exactly the same eleven elements; `--shard-cpu-limit=` was dead for
the prefix reason and is dropped rather than repaired, because that
list now covers it. The vendored ouroboros templates get no such pin:
upstream churn arrives there on its own schedule, and a whole-list
assertion would fail a test named for one flag for a reason that has
nothing to do with that flag.
Anchor every index on a path, not the outermost one. The Cilium
assertion reads spec.ingressDeny[0].toPorts[0].ports, and pinning the
rule list alone would still let the port return on a second toPorts
entry of the same rule.
Each guard was checked by rendering what it forbids onto the element
it names, and each anchor by rendering the shift it exists to catch.
The form is already used in this tree, in opensearch-operator and
dashboard. Assertions of the same shape remain in files this commit
does not touch, including one in a package it does, and stay out of
scope here.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Signed-off-by: Aleksei Sviridkin <3811295@gmail.com>
|
||
|
myasnikovdaniil
|
516be4abfe |
fix(cozystack-basics): normalize tenant hostname VAP comparisons with lowerAscii (#3208)
## What this PR does The Ingress, Gateway, and Route hostname `ValidatingAdmissionPolicy` objects compare tenant hostnames against the tenant apex with case-sensitive CEL. The resource-side host is always lowercase — the apiserver's hostname validation permits nothing else — but both apex operands can carry uppercase: the `namespace.cozystack.io/host` label value, which Kubernetes permits to carry uppercase and which the tenant chart writes verbatim from `tenant.spec.host`, and the templated `_cluster.root-host`, a free-form platform value that nothing validates for case. A mixed-case apex label wrongly denies a tenant its own legitimate lowercase in-apex hostname. An uppercase `_cluster.root-host` is the dangerous direction: the Ingress outside-root exclusion compares a lowercase host against the uppercase literal, never matches, so the exclusion stops excluding and the policy admits any non-wildcard host — every non-wildcard platform-apex name a tenant asks for, including a hostname belonging to another tenant and the root apex itself. Only the anti-wildcard gate still holds. This normalizes both operands of every hostname comparison with `.lowerAscii()` (host, apex label, and the templated root-host literal) across all three policies, so the match follows DNS case semantics. There is no behavior change in the default all-lowercase configuration. The Gateway policy's fail-open on an absent or empty host label is deliberately left as it was: that is the platform's one documented fail-open exception in `docs/security/threat-model.md`, so changing the posture is a security-model decision rather than a side effect of normalizing case. Its current shape is pinned by a test. Test coverage: - helm-unittest pins each comparison as a whole operand pair — equality and `endsWith`, across the rule-host and tls-host branches — so dropping the normalization from any single operand is caught. A pattern matching a lone `.lowerAscii()` would not: the apex appears four times in the Ingress expression, so stripping one operand still leaves three matches. - A rendering case with `_cluster.root-host: Example.ORG` pins the outside-root escape shut; it is red pre-fix. - e2e probes set a mixed-case apex on an isolated throwaway namespace and assert both halves of the match: a lowercase in-apex HTTPRoute and Ingress are still admitted (red pre-fix, green post-fix), while a foreign apex and a hostname ending with the apex without a dot boundary are still denied. Vacuous-pass guards re-read the persisted label and assert the probe apex really is mixed-case. Normalizing the apex at the write site — the tenant namespace template and `_cluster.root-host` — is tracked separately in #3328. It would not remove the need for this change: the host label is settable out-of-band by a trusted caller, so these policies must not assume the apex arrives normalized. Closes #3205. ### Screenshots N/A (no UI changes). ### Release note ```release-note fix(cozystack-basics): tenant hostname admission policies now compare against the tenant apex and the platform root apex case-insensitively. This closes a bypass where an uppercase `_cluster.root-host` made the Ingress policy's outside-root exclusion stop matching, so the policy admitted any non-wildcard host — including a hostname under the platform apex belonging to another tenant, and the root apex itself. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Hostname validation now consistently matches case-insensitively across Gateway, Ingress, HTTPRoute, and TLSRoute. * Mixed-case tenant and apex hostnames are accepted when they meet the intended domain boundaries. * Foreign domains and prefix-adjacent hostnames remain correctly rejected; validation behaves as documented when the tenant host is empty. * **Tests** * Expanded unit and e2e coverage for case-insensitive hostname rendering and boundary rejections across the supported resources. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Andrey KolkovandClaude Opus 4.8
|
dff56aa998 |
feat(backups): add a native MongoDB (psmdb) backup-strategy driver
Adds a MongoDB (Percona Server for MongoDB / psmdb) backup-strategy driver so `apps.cozystack.io/MongoDB` reaches feature parity with the CNPG, MariaDB and ClickHouse native drivers — logical backups, restore-to-differently-named instances, and point-in-time recovery through the unified BackupJob/RestoreJob/BackupClass/Plan interface (closes #3545). The driver orchestrates the operator's PerconaServerMongoDBBackup / PerconaServerMongoDBRestore CRDs: - BackupJob: creates a one-shot PerconaServerMongoDBBackup, polls status.state, and persists the S3 destination + storage descriptor (by-reference, no credentials) onto the Cozystack Backup artifact. - RestoreJob: drives PerconaServerMongoDBRestore via backupSource (read back from the source backup status, snapshot fallback) so the same artifact restores in-place or into a differently-named instance; PITR via spec.options.pitr (date/latest). Because psmdb resolves the backup storage by name against the cluster's spec.backup.storages, the driver names the chart's storage (s3-storage) via the cozy-default-mongodb Strategy rather than injecting it, and surfaces a clear Ready=False precondition when a target has backups disabled. Wiring: - api/backups/strategy/v1alpha1: MongoDB strategy type + generated CRD/deepcopy. - internal/backupcontroller: psmdbstrategy_controller + psmdbtypes/mongodbapp partial-schema mirrors; dispatch, supported-kinds and scheme registration. - backupstrategy-controller: cozy-default MongoDB route, cozy-default-mongodb Strategy CR, and psmdb.percona.com RBAC. - apps/mongodb: backup.insecureSkipTLSVerify knob (psmdb s3 storage has no CA-bundle field) so backups can target Cozystack's self-signed seaweedfs. Tests + e2e: - unit tests for the driver (validation, ensure-idempotency, precondition, backupSource resolution, snapshot round-trip, PITR option parsing). - examples/backups/mongodb + a mongodb-2-backup-roundtrip Chainsaw suite that drives run-all.sh end-to-end (sentinel document round-trips through S3 into a restored copy while the source stays untouched), per docs/agents/e2e-testing.md. - docs/operations/backup-classes.md updated. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Alexey Artamonov
|
2b5c85cde7 |
fix(clickhouse): assign DB_LIST so backup retention cleanup runs
The legacy per-instance backup script printed `echo DB_LIST=$(...)` instead of assigning DB_LIST, leaving it unset. The cleanup loop `for db in $DB_LIST` then ran zero iterations, so `restic forget` and `restic prune` never executed and snapshots accumulated in object storage indefinitely despite the configured cleanupStrategy. The Job still reported success, hiding the failure. Assign DB_LIST from the shuffled table list so the retention cleanup loop runs. Add a helm-unittest regression asserting the rendered backup.sh assigns DB_LIST and no longer contains the `echo DB_LIST=` typo. Fixes #3758 Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Andrey Kolkov
|
d0990299ed |
feat(clickhouse): let the backup sidecar trust a private S3 CA, unblocking the backup e2e round-trip (#3385)
## What this PR does Gives the ClickHouse `clickhouse-backup` sidecar a way to trust an S3 endpoint whose certificate is signed by a private CA, and uses that to make the ClickHouse backup/restore e2e round-trip actually run in CI. This actualizes #2600, whose feature code and example scripts have since landed on `main` independently, and whose remaining contribution — the e2e test — was still in the retired BATS format. #2600 can be closed once this merges. ### 1. `backup.endpointCA` on the ClickHouse chart The sidecar had **no TLS/CA surface at all**: bucket coordinates and credentials arrive as `S3_*` env, and nothing let it verify an endpoint signed by a private CA. Cozystack's own in-cluster SeaweedFS is exactly that case — it serves `:8333` behind the self-signed "SeaweedFS CA" — so a ClickHouse release could only ever back up to a publicly-trusted endpoint. MariaDB (`storage.s3.tls.caSecretKeyRef`) and Postgres (barman `endpointCA`) have had a CA field all along; this closes the gap for ClickHouse. `backup.endpointCA` takes `{name, key}`. When set, the chart mounts that Secret into the sidecar and points `SSL_CERT_DIR` at it. Empty — the default — renders no env, no mount and no volume, so existing releases are byte-for-byte unchanged. **Why `SSL_CERT_DIR` and not `AWS_CA_BUNDLE`.** Both are honoured by `altinity/clickhouse-backup:2.7.4`; I measured them against a self-signed endpoint and public AWS S3 in turn. Go reads `SSL_CERT_DIR` *in addition to* the system bundle, so both endpoints verify. `AWS_CA_BUNDLE` *replaces* the SDK's trust pool: the private endpoint verifies, and public S3 then fails `x509: certificate signed by unknown authority` — it would silently break a release that also talks to a public endpoint. `S3_DISABLE_CERT_VERIFICATION` was rejected outright, since it drops verification rather than trusting one more CA. ### 2. The e2e suite, and what it was really testing Two Tests added to the existing `hack/e2e-chainsaw/clickhouse/` suite (in name order, mirroring the `etcd`/`mariadb`/`postgres` shape — see the layout rationale below for why they live here rather than in a new directory): - **`clickhouse-1-backup-contracts`** (CI, `tenant-test`, ~98s): a backup-enabled ClickHouse asserted against the *running* cluster — the `clickhouse-backup` sidecar on the `ch-backup-api` `:7171` port, and the `<release>-backup-api-auth` Secret whose keys the sidecar's `API_USERNAME`/`API_PASSWORD` actually resolve `valueFrom`. Each is a silent break if a chart bump renames it. Needs no object store. - **`clickhouse-2-backup-roundtrip`** (CI, `tenant-root`): `Bucket` → `Altinity` strategy + `BackupClass` → source ClickHouse with a sentinel row → `BackupJob` → in-place `RestoreJob` → to-copy `RestoreJob` into a second instance, driving `examples/backups/clickhouse/run-all.sh` so test and docs cannot drift. The round-trip was originally gated behind `CLICKHOUSE_E2E_S3_ROUNDTRIP` and **skipped in CI** — its `PASS (1.01s)` meant the gate fired, not that a backup happened. `backup.endpointCA` removes the reason for the gate, so the suite now adopts the same three preconditions mariadb/postgres rely on: run in `tenant-root` (an isolated tenant's Cilium egress allowlist blocks its Pods from reaching `tenant-root`'s SeaweedFS), target the in-cluster endpoint instead of the `Bucket`'s external ingress URL (an unroutable placeholder in CI), and trust its certificate via the copied CA. It also pre-cleans with `cleanup.sh`, because `tenant-root` is shared and a leftover `Succeeded` `BackupJob` would satisfy the harness's wait without a backup ever running. Both backup Tests live in the existing `hack/e2e-chainsaw/clickhouse/` suite rather than a new suite directory, so no selector mapping is added: `hack/select-e2e.sh` already derives `clickhouse` both from `packages/apps/clickhouse/**` (the `*-application` rule) and from `examples/backups/clickhouse/**` (the harness path), and both paths therefore select the suite that actually runs the harness. That alignment is the reason for the layout — a separate `clickhouse-backup/` directory would have made an edit to the harness select a suite that cannot exercise it. `hack/select-e2e.sh` and `hack/select-install.sh` are otherwise untouched (the only change is a comment recording that invariant), and the existing `backup example harness edit selects its app suite` case in `hack/select-e2e_test.bats` covers the mapping. ### Verification - `helm unittest` — 17 pass, including 5 new `endpointCA` cases: the unset default renders nothing, the env/mount/volume wiring, a custom key still projected as `ca.crt`, composition with `useSystemBucket`, and `backup.enabled=false` renders nothing. - The CA-trust behaviour was measured against the real image, not assumed: without a CA knob the sidecar reports `x509: certificate signed by unknown authority`; with `SSL_CERT_DIR` the handshake succeeds. - `hack/select-e2e_test.bats` (16) and `hack/select-install_test.bats` (17) pass; `select-install.sh --validate` reports graph OK. - `make generate` is idempotent, so the pre-commit job's `git diff --exit-code` stays clean. - The contracts Test passed in CI on earlier runs of this branch. The round-trip only became CI-live with this work, so it still needs a green E2E run before merge — the branch has since been rebased onto current `main` to get one. ### Screenshots Not applicable — no UI changes. ### Downstream repositories Walked the trigger map against the diff, file by file. The load-bearing change is the new `backup.endpointCA` field in `packages/apps/clickhouse/values.yaml` / `values.schema.json`; everything else is `hack/e2e-chainsaw/`, `examples/backups/`, one docs paragraph, and regenerated artifacts, none of which appear in the map. - [ ] No downstream repository is affected by this change - [x] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: https://github.com/cozystack/terraform-provider-cozystack/issues/18 — "add, remove or rename a field in an app's `values.schema.json`" reaches the provider's hand-maintained schema, model and expand/flatten pair, so the new nested object is not surfaced until it is added there. Opened as an issue rather than a PR because the natural moment is alongside that repo's next `api/apps/v1alpha1` pin bump, which is a maintainer call. Not ticked, with reasons rather than silence: **cozystack/website** — the trigger is *adding, renaming or removing a package*, and this adds a field to an existing one. `clickhouse` is already in the website `Makefile`'s app list and its reference page is generated from this package's `README.md`, so the regenerated `backup.endpointCA` rows are carried by the existing release-tag automation with no manual PR. **cozystack/external-apps-example** — vendors `hack/package.mk` and `update-crd.sh`, neither of which this PR touches. **cozystack/ansible-cozystack**, **ccp**, **talm**, **cozyhr**, **cozy-proxy**, **cozystack-telemetry-server**, **examples** — no installer values, `hack/` layout, namespace, variant, node-prerequisite, annotation, label or metric in their trigger rows is touched here. ### Release note ```release-note feat(clickhouse): add `backup.endpointCA` so the clickhouse-backup sidecar can verify an S3 endpoint whose certificate is signed by a private CA, such as Cozystack's in-cluster SeaweedFS. Leave it unset for publicly-trusted endpoints; existing releases are unaffected. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added optional private S3 endpoint CA support for ClickHouse backup/restore sidecars via `backup.endpointCA`, including configurable Secret name and CA bundle key (default `ca.crt`). * Sidecars mount the CA and extend trust while leaving public endpoints unchanged. * **Documentation** * Updated ClickHouse chart docs, operational backup guidance, and backup/restore examples to cover CA setup, overrides, and cleanup. * **Tests** * Added helm-unittest and end-to-end coverage validating CA rendering/mounting behavior and ClickHouse backup/restore workflows. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
IvanHunters
|
cfaf2e086c |
fix(backupstrategy-controller): repair lookup-gated backup objects (#3524)
<!-- Thank you for making a contribution! Here are some tips for you: - Use Conventional Commits for the PR title: `type(scope): description` - Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore - Scopes are not an exhaustive list — pick the most specific scope for the change and extend the list when a genuinely new area appears. Examples: - System components: dashboard, platform, operator, cilium, kube-ovn, linstor, fluxcd, cluster-api - Managed apps: postgres, mariadb, redis, kafka, clickhouse, virtual-machine, kubernetes - Development and maintenance: api, hack, tests, ci, docs, maintenance - Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or add a `BREAKING CHANGE:` footer - If it's a work in progress, consider creating this PR as a draft. - Don't hesistate to ask for opinion and review in the community chats, even if it's still a draft. - Add the label `backport` if it's a bugfix that needs to be backported to a previous version. --> ## What this PR does Fixes #3518. Objects in `packages/system/backupstrategy-controller` and `packages/system/bucket` are gated on a Helm `lookup` of an object that is still being created. When the lookup is empty the object is skipped — and since helm-controller does not re-render a release whose chart and values did not change (drift detection is off on operator-generated HelmReleases), **the skip is permanent**, not a bootstrap window. On the reporter's cluster the install-timing window was 4 seconds and the objects were still missing months later: - the 7 default backup `Strategy` CRs and the Velero `BackupStorageLocation`, gated on `include "backupstrategy-controller.bucketName"` → a `lookup` of `BucketClaim`; - the `<bucket>-<user>-credentials` Secret (`packages/system/bucket/templates/user-credentials.yaml`), gated on a `lookup` of the COSI Secret. Only `BackupClass cozy-default` was present, because it renders unconditionally. Its own comment predicted this case while assuming "Flux re-renders the chart", which does not happen — the same trap `packages/core/platform/sources/cozystack-basics.yaml` already documents: *"Drift detection is off on operator-generated HelmReleases, so a capability gate in the templates would render the policy out at first install and never add it back."* Consequence: migration 50 (etcd adoption, v1.6.0) is fail-closed on the snapshot target. It resolves the target from the projected credentials Secret, which has no source without `<bucket>-<user>-credentials`, so its Job reboots forever and the cluster cannot be upgraded at all. ### 1. `backupstrategy-controller` reconciles the objects into existence New leader-elected `DefaultObjectsGate` runnable (`internal/backupcontroller/default_objects_gate.go`), modelled on the existing `SystemCredentialsProjector`. Every minute: - resolves the bucket name from the **projector's source Secret** (`parseSourceSecret`) — no new dependency, no `lookup`, and it covers the external-S3 path too. While it is unresolvable the gate does nothing: forcing then would only re-run the same empty lookup; - reads `BackupClass cozy-default` as the manifest of what must exist (its `strategyRef`s), plus the `cozy-default` BSL when `velero.bslEnabled`; - when any is missing, stamps `reconcile.fluxcd.io/forceAt` **and** `requestedAt` on its own HelmRelease. Both are required: `requestedAt` alone only requests a reconcile, which is a no-op for an unchanged release; `forceAt` is what triggers a real Helm upgrade, and it is only honoured together with `requestedAt`; - throttled to one forced upgrade per 5 minutes, and a no-op in the steady state. A `strategyRef` whose CRD is not installed is not counted as missing, so a missing CRD cannot drive an endless force loop. Two new metrics: `cozystack_backup_default_objects_missing{backupclass}` — the alert that would have caught this, since it is non-zero regardless of the HelmRelease's `Ready` condition — and `cozystack_backup_default_objects_force_reconciles_total`. RBAC delta is two lines: `patch` on `helmreleases` (`get` was already there) and `get` on `backupstoragelocations`. Both accesses are point Gets through the dynamic client, so no cluster-wide informer and no `list`/`watch`. Off switch: `backupStorage.reconcileDefaultObjects: false`. **Why not a render-time `fail` in this chart:** it would deadlock. `templates/bucket.yaml` in this same chart is the producer of the `Bucket` whose `BucketClaim` the lookup reads, so a failed render never applies it and the condition can never resolve. **Why not template the objects in Go:** their bodies are values-driven (endpoint, region, `forcePathStyle`, the Altinity strategy's whole PodTemplateSpec). Helm stays their single author; the gate only makes sure the render that produces them actually happens. **Why not `dependsOn`:** an edge guarantees the `Bucket` CR is *applied*, not that its claim is *reconciled*, so it does not close the race. ### 2. The gate also repairs the credentials Secret, one release earlier The `<bucket>-<user>-credentials` Secret is the same trap as the Strategy CRs, one release up the chain: `packages/system/bucket/templates/user-credentials.yaml` renders it behind a `lookup` of the COSI Secret, and skips it just as permanently. It is also the *first* domino — the projector reads it, so without it there is no `cozy-backups-creds`, no strategy, no Velero, and no migration 50. An earlier revision of this PR made that render `fail` so the release would retry and converge. [@IvanHunters pointed out](https://github.com/cozystack/cozystack/pull/3524#issuecomment-5214269078) that Helm cannot render a partial set, so the `fail` aborts the **whole** `<bucket>-system` release — the other users' Secrets and the bucket UI Deployment/Service/Ingress/HTTPRoute with them. One declared user whose `BucketAccess` never provisions would take down every other user of that bucket, and on an installed release park it in `Failed`, blocking every later upgrade. That was a real blast-radius regression on a universal path, and it is gone. The repair now lives in the gate, where it is per-object: - while the source Secret is absent, or carries no bucket name, the gate stamps `forceAt` + `requestedAt` on the bucket's `bucket-<name>-system` HelmRelease instead of doing nothing; - **no self-deadlock:** the `BucketClaim` and the `BucketAccess` whose COSI Secret that lookup reads are rendered unconditionally by the **parent** release (`packages/apps/bucket/templates/bucketclaim.yaml`), not by the one being forced; - **no RBAC delta:** `patch` on `helmreleases` is already cluster-scoped; - the two releases are throttled **independently**. They are forced in sequence on a bootstrap (credentials, then the objects the resolved bucket unblocks), so one shared timestamp would delay the second by a full `MinForceInterval` for no reason; - coordinates come from the chart under `provisionBucket`, and are omitted on external S3, where the Secret is admin-managed and no release renders it. The bucket chart keeps its partial render. Its comment now explains why the skip is not self-healing and why failing is the wrong lever, and a test renders the UI with *every* user unresolvable so a reintroduced `fail` fails the suite. **Scope this narrows, deliberately:** tenant buckets keep the pre-existing silent skip. The gate owns only the platform bucket, which is the cluster-breaking path. Generalising the repair to every bucket needs a controller that owns tenant buckets and is a separate change. Two more corrections from the same review: - `cozystack_backup_default_objects_missing` was only written on the happy path, so the state this PR exists to catch reported `0` and the documented alert never fired. The credentials Secret now counts in `missing`, so an absent or rotated-away Secret moves the gauge. On an API error the gauge is deliberately left alone rather than flapping, and a new `cozystack_backup_default_objects_check_errors_total` marks it stale; the doc pairs the two instead of overstating the gauge. `force_reconciles_total` gained `namespace`/`name`, since two releases can now be forced. - `forceHelmRelease` does a point `Get` and skips a release with `spec.suspend: true`. helm-controller ignores both annotations while suspended (`cozyhr suspend` sets exactly that), so the gate was re-stamping every `MinForceInterval` for the whole suspension and climbing the force counter — which the runbook attributes to a render that is not producing the objects. The skip is logged and not counted. ### 3. Corrected comments and docs `_helpers.tpl`, `backupclass-default.yaml`, `values.yaml` and `docs/operations/backup-classes.md` all claimed the skip self-heals on the HelmRelease interval, and the doc suggested a `flux reconcile` that does not re-render. They now say it does not, and why. The two-release manual recovery for already-affected clusters is documented, including the easy-to-miss part: the credentials Secret is rendered by the `bucket-<name>-system` release, not by `bucket-<name>`. ### Manual recovery for already-affected clusters Only needed on a version without the gate. A plain `flux reconcile helmrelease` does **nothing** — it does not re-render. ```bash # 0. Confirm the bucket name is resolvable first — forcing before that # just re-runs the same empty lookup. kubectl -n tenant-root get bucketclaim bucket-cozy-backups -o jsonpath='{.status.bucketName}' # 1. The credentials Secret. Rendered by the -system release, NOT by # bucket-cozy-backups — easy to miss, and the projector (hence every # strategy, Velero, and migration 50) has no source without it. ts=$(date -u +%Y-%m-%dT%H:%M:%SZ) kubectl -n tenant-root annotate helmrelease bucket-cozy-backups-system \ reconcile.fluxcd.io/forceAt="$ts" reconcile.fluxcd.io/requestedAt="$ts" --overwrite # 2. The Strategy CRs and the Velero BSL. ts=$(date -u +%Y-%m-%dT%H:%M:%SZ) kubectl -n cozy-backup-controller annotate helmrelease backupstrategy-controller \ reconcile.fluxcd.io/forceAt="$ts" reconcile.fluxcd.io/requestedAt="$ts" --overwrite ``` ### Validation - `go test ./internal/... -count=1` — pass, including 15 cases for the gate: force on missing, no-op in the steady state, throttling, unmapped kinds ignored, BSL skipped when disabled, patch failure surfaced without advancing the throttle, absent HelmRelease tolerated, plus force-on-absent-Secret, force-on-empty-bucket-name, external-S3 reports-but-does-not-force, independent throttles, suspend skip on each of the two releases, gauge written on the unresolved path, and errors counter on a failed check. - `make helm-unit-tests` — pass, with 2 new suites: 3 cases for the bucket chart (partial render preserved, and the UI still renders with every user unresolvable — the guard against reintroducing the `fail`), 7 for the gate's env + RBAC wiring. Those include that `.Release.Name` and `.Release.Namespace` match the HelmRelease the operator generates (`releaseName` becomes the HR's `metadata.name`, and no `spec.releaseName`/`targetNamespace` is set, so the Helm release name equals the HR name), that the bucket release coordinates track a renamed bucket together with `systemSecretName`, and that they are unwired on `provisionBucket: false`. - `make generate` — no diff. - `make go-unit-tests rd-presets-check test-check-readiness migrations-target-check` — pass. - `make bats-unit-tests` — every suite passes except `hack/migration-seaweedfs-db-adopt.bats`, which needs a Docker daemon not available in this environment ("docker is required: these tests run the migrations inside alpine"). Unrelated to this diff. **Not verified — please read this before approving.** There was still no live-cluster run. What that gap covers is smaller than it was: dropping the chart `fail` removed the universal behaviour change on every bucket's install path, so there is no longer a render-blind failure mode whose convergence has to be observed. What remains is one mechanism — `forceAt` + `requestedAt` on an unchanged release — applied to two releases instead of one, argued from read code rather than an observed install: `Strategy.Name = RetryOnFailure` on operator-generated HelmReleases (`internal/operator/package_reconciler.go`), and `bucketName = "bucket-" + claim.UID` in `packages/system/objectstorage-controller/images/objectstorage/patches/92-bucketclaim-propagate-ready.diff`. It is the same procedure as the manual recovery below, which has been run by hand on the reporter's cluster. Someone with a bootstrap cluster should still confirm that the gate fires within a minute or two of the bucket becoming ready, and that forcing `bucket-<name>-system` materialises the credentials Secret. ### Screenshots Not applicable: no UI change. This PR touches a controller, two system charts, and docs. <!-- REQUIRED for UI changes: attach screenshots or screen recordings demonstrating the visual impact of your changes. PRs with UI changes without screenshots will not be merged. --> ### Downstream repositories <!-- Cozystack is upstream for repositories that are NOT kept in sync with it automatically, and nothing in CI compares the two sides, so a change here can break them silently. Which change forces what, and which file to touch, is the trigger map in docs/agents/contributing.md under "Downstream Repositories". Walk it before you tick anything. IF YOU ARE AN AI AGENT OR AN LLM, THIS IS PART OF THE TASK, NOT A FORM TO FILL IN: 1. Walk the trigger map against the actual diff of this PR, file by file. Do not decide from the PR title or from your memory of what you changed. 2. For every repository you tick, open the follow-up PR there and link it on the same line. A ticked box with no link claims work that does not exist, which is worse than an empty box. Search that repository for an open PR or issue covering it first, and link that instead of filing a duplicate. 3. When the follow-up is out of scope, or needs a decision that is not yours to make, open an issue in that repository instead, link that, and say so in the PR body. Do not push a speculative PR into another repository just to fill a line here. 4. Do not tick "No downstream repository is affected" to make the checklist go away. If you are unsure, leave every box empty and say so in the PR body, so a human decides. --> Walked file by file against the diff of this PR. **`website` — affected, follow-up issue opened.** `content/en/docs/next/operations/services/backup-classes.md` mirrors this repo's `docs/operations/backup-classes.md` and carries the exact stale claim this PR corrects, plus a `flux reconcile helmrelease` recovery suggestion that has no effect. `operations/configuration/platform-package.md` enumerates the forwarded `backupStorage` keys, and `reconcileDefaultObjects` is admin-settable through `spec.components.platform.values.backupStorage` (the whole block is deep-merged), so that list becomes incomplete. Filed as an issue rather than a PR because the wording depends on the shape this PR lands in, and whether to touch the frozen `v1.5`/`v1.6` copies is a maintainer call. No existing website issue or PR covered it. **`terraform-provider-cozystack` — not affected.** Checked, not assumed: `packages/apps/**` is untouched by this diff (`git show --stat HEAD -- packages/apps/` is empty), so no app `values.schema.json`, no `values.yaml` default, no version enum, and no `ApplicationDefinition` `kind`/`plural`/`release.prefix` changed. `reconcileDefaultObjects` is a key on a **system** chart, which has no `values.schema.json` and is not modelled by the provider. (An earlier revision added `requireUserCredentials` to the `bucket` system chart; that is gone, and no key was added to `apps/bucket`, so the app API is untouched.) The `<bucket>-<user>-credentials` Secret the provider looks up by literal name is **not renamed** — this PR only changes whether it gets created, which if anything removes a case where the provider silently read null from a Secret that never existed. Everything else: no `hack/` layout or make-target change (`ccp`, `external-apps-example`, `cozyhr`), no `ApplicationDefinition` CRD change, no namespace rename, no node prerequisites in `hack/e2e-prepare-cluster.bats` (`talm`, `ansible-cozystack`), no `cozyhr.cozystack.io/values-files` annotation or chart-source-kind change, no `service-proxy-name` / `wholeIP` / `allowICMP` rename (`cozy-proxy`), no telemetry metric or label rename (`cozystack-telemetry-server` — the two metrics here are new, not renamed), no node or network requirement change (`examples`). - [ ] No downstream repository is affected by this change - [x] [cozystack/website](https://github.com/cozystack/website) - follow-up: https://github.com/cozystack/website/issues/639 - [ ] [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 <!-- Write a release note: - Explain what has changed internally and for users. - Start with the same `type(scope):` prefix as in the PR title - Follow the guidelines at https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md. --> ```release-note fix(backupstrategy-controller): the default backup Strategy CRs, the Velero BackupStorageLocation and the bucket user-credentials Secret were skipped permanently when their install-time Helm `lookup` came back empty, leaving clusters with a BackupClass but no strategies and blocking the v1.6.0 etcd migration. The controller now detects the missing objects and forces the Helm upgrade that creates them, including on the bucket release that renders the credentials Secret. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added automatic detection and recovery for missing default backup resources. - Missing resources trigger a throttled Helm upgrade once bucket configuration is available. - Added configuration to enable or disable default-object reconciliation. - Added validation requiring user credential Secrets during bucket installation by default, with an offline rendering option. - **Bug Fixes** - Improved recovery from incomplete backup and Velero storage configuration. - **Documentation** - Expanded operational guidance, monitoring metrics, alerts, and manual recovery steps. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
IvanHunters
|
980715250c |
fix(kube-ovn): reload kubeovn-webhook serving certificate on cert-manager renewal (#3557)
## What this PR does The `kube-ovn-webhook` loaded its TLS serving certificate once at process startup (`tls.LoadX509KeyPair` into a static `tls.Config.Certificates`) and never re-read it. cert-manager renews the backing Secret ahead of expiry, but a running pod kept serving the certificate it loaded at boot. Once that certificate expired — roughly one year after install, on any cluster whose webhook pods had not been restarted after the renewal — the pod served an expired certificate, the kube-apiserver's TLS call to the webhook failed verification, and because the `MutatingWebhookConfiguration` uses `failurePolicy: Fail`, every pod creation in tenant namespaces was rejected. In practice this blocks `virt-launcher` pods, so no VMI can start. This PR: - Serves the certificate through a reloading `tls.Config.GetCertificate` callback that re-reads the key pair when the mounted files change, and keeps serving the last good certificate if a reload fails (e.g. a torn read while the volume is being updated). cert-manager renewals are now honoured without a pod restart. - Widens the certificate `renewBefore` from `24h` to `720h`, so renewal happens well ahead of expiry instead of one day before. - Adds a regression test that fails against the previous static implementation (the server keeps presenting the old certificate after the files are replaced) and passes once the certificate is reloaded; verified non-vacuous by mutation. ### Screenshots Not applicable (no UI changes). ### Downstream repositories - [x] No downstream repository is affected by this change Walked the trigger map file-by-file against the diff: the change is confined to the `system/kubeovn-webhook` image and its cert-manager `Certificate` `renewBefore`; it touches no package under `apps/`/`extra/`, no `values.schema.json`, no app `values.yaml` default, no `ApplicationDefinition`, no platform values, no CRD, and no `hack/` tooling. ### Release note ```release-note fix(kube-ovn): kubeovn-webhook now reloads its serving certificate when cert-manager renews it, so the webhook no longer serves an expired certificate that blocked all pod creation (including VMIs) roughly one year after install ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Webhook TLS certificates now reload automatically when renewed, without requiring a server restart. * **Bug Fixes** * Certificate reload failures now fail the TLS handshake instead of serving an outdated certificate. * Added server timeouts to improve connection handling. * **Configuration** * Certificates are now renewed earlier to help prevent expiration-related service disruptions. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
f4d812cfb9 |
fix(velero): apply CRD updates on upgrade via CreateReplace
velero evolves its CRD schema between versions; v1.18 adds the Queued and ReadyToStart backup phases. Helm never upgrades CRDs shipped in crds/, the generated HelmRelease left spec.upgrade.crds at the Skip default, and the package disables the chart's upgrade-crds Job, so live CRDs stayed frozen at first-install state: the server upgrades, the apiserver rejects the new phases, and backups sit in New while the HelmRelease stays green. Opt the velero PackageSource into CreateReplace, consistent with mongodb-operator, add a platform test pinning the policy, and correct the stale Job-disable rationale in the package. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <3811295@gmail.com> |
||
|
Aleksei Sviridkin
|
ba15b3817a |
fix(cozystack-basics): normalize tenant hostname VAP comparisons with lowerAscii
The Ingress, Gateway, and Route hostname ValidatingAdmissionPolicies compare tenant hostnames against the tenant apex with case-sensitive CEL. The resource-side host is already lowercase — the apiserver's hostname validation permits nothing else — but the apex operand is the namespace.cozystack.io/host label value, which Kubernetes permits to carry uppercase, and the Ingress policy additionally compares against the templated root host, a free-form platform value that nothing validates for case. A mixed-case apex label wrongly denies a tenant its own legitimate lowercase in-apex hostname. An uppercase root host is the dangerous direction: the Ingress outside-root exclusion compares a lowercase host against the uppercase literal, never matches, so the exclusion stops excluding and the policy degenerates into admitting any non-wildcard host — every non-wildcard platform-apex name a tenant asks for, including the root apex itself. Only the anti-wildcard gate still holds. Wrap both operands of every comparison in .lowerAscii() so the match follows DNS case semantics — no behavior change for the all-lowercase default. Each policy reads a label it does not own: the namespace host label is settable out-of-band by a trusted caller, so none of them may assume the apex arrives normalized. The Gateway policy admits any listener hostname when the host label is absent or empty, unlike its two siblings. That is the platform's one documented fail-open exception, recorded in the threat model together with its compensating control, so changing the posture is a security-model decision rather than a side effect of normalizing case. This leaves the behavior untouched and pins its current shape in the tests. Covered by helm-unittest assertions pinning each comparison as a whole operand pair, so dropping the normalization from any single operand is caught; a rendering case that pins the uppercase-root-host escape shut; and e2e probes that set a mixed-case apex on an isolated namespace and assert both halves of the match — a lowercase in-apex HTTPRoute and Ingress are still admitted, while a foreign apex and a hostname that ends with the apex without a dot boundary are still denied. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
3ae1e10c92 |
docs(kubernetes): correct the registryMirrors field and README claims
State in the README that in-guest registry pulls are routable again via talos.registryMirrors, which knob covers which artifact (the mirror routes registry pulls; the worker OS disk image arrives as a raw HTTP artifact under talos.imageFactoryURL and no registry mirror touches it), and the limits: skipFallback is needed for air-gap, machine.registries.config is not passed through, and existing nodes keep their boot-time config until replaced. Trim the field doc comment. The regenerated schema, README tables and cozyrds openAPISchema follow from those sources. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
3a1292d682 |
fix(kubernetes): escape tenant values in the worker reconcile heredoc
The worker TalosConfigTemplate is applied from an unquoted `cat <<EOF | kubectl apply` heredoc, so every free-form value interpolated into it is subject to shell parameter expansion and command substitution at Job runtime. Escape backslash, dollar and backtick on talosVersion, installerRepository, schematicID and the registryMirrors block so each renders as a literal, and record the invariant above the data block in both charts: a new free-form field here must be escaped or render-time pattern-validated, or it reopens command injection into a host-cluster Pod. Escaping covers every byte by construction; the chain is a no-op for ordinary values, so the default render and its content-hash Job name are unchanged. Tests render both charts and run the heredoc through a real shell with hostile values, which a rendered-string regex cannot do, and the default-omits-mirrors assertion selects the Job by kind rather than by document index so it cannot pass against the wrong document. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
IvanHunters
|
58910c0edc |
feat(kubernetes): route tenant worker image pulls through a registry mirror
Add a talos.registryMirrors passthrough to the kubernetes and kubernetes-nodes
charts that renders into the worker TalosConfigTemplate machine.registries.mirrors,
so an operator can point a registry such as ghcr.io at an in-cluster pull-through
mirror and a worker boot no longer depends on live public egress. Default {}
keeps the rendered worker reconcile Job byte-identical to before, content-hash
Job name included. Free-form mirror values are shell-escaped for the unquoted
apply heredoc, covered by an execution-level bats test.
Also add the e2e ghcr-mirror (registry:2 pull-through plus a Cilium egress
policy) and wire it into the kubernetes chainsaw suites, so CI exercises the
mirror path and takes the flaky public ghcr.io kubelet-image pull off live
egress.
Refs #3513 (addresses the egress variant of the tenant-worker node-join
failures; the dominant variant is CI-runner CPU starvation, tracked in the
issue).
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
|
||
|
Aleksei Sviridkin
|
92ec49e64e |
fix(tenant): retry failed monitoring and gateway installs in place (#3633)
<!-- Thank you for making a contribution! Here are some tips for you: - Use Conventional Commits for the PR title: `type(scope): description` - Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore - Scopes are not an exhaustive list — pick the most specific scope for the change and extend the list when a genuinely new area appears. Examples: - System components: dashboard, platform, operator, cilium, kube-ovn, linstor, fluxcd, cluster-api - Managed apps: postgres, mariadb, redis, kafka, clickhouse, virtual-machine, kubernetes - Development and maintenance: api, hack, tests, ci, docs, maintenance - Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or add a `BREAKING CHANGE:` footer - If it's a work in progress, consider creating this PR as a draft. - Don't hesistate to ask for opinion and review in the community chats, even if it's still a draft. - Add the label `backport` if it's a bugfix that needs to be backported to a previous version. --> ## What this PR does The tenant chart renders monitoring and gateway with no install strategy, so they get Flux's default, and install remediation is an uninstall in every case Flux offers. Neither sets `spec.uninstall.disableHooks`, which defaults to false. So a release that missed nothing but a readiness budget gets uninstalled, and the app chart's `post-delete` cleanup Job runs against it. Those Jobs delete on purpose, and they are right to. The monitoring one removes PVCs labelled `apps.cozystack.io/application.name=<release>-system`. That label is not incidental either: the metrics and logs claim templates stamp it so the operator copies it onto the storage PVCs, and the Job selects on it to stay release-scoped. Both ends were built to meet. The result is that a slow first install deletes the tenant's metrics and logs, and nothing reconstructs them. The gateway Job deletes the `cozystack-acme-account` Secret. That key regenerates, so this one costs no data. It costs a re-registration against the ACME provider, and repeated cycles run into its rate limits, which can hold certificate issuance for the tenant down for hours. Smaller stake, same mechanism, and the two are not one class: only one of them owns storage. `RetryOnFailure` keeps the manifests applied and retries the failed release, so no uninstall happens and neither Job runs. Both actions carry the strategy because the controller performs that retry as an upgrade, so with install alone the retry's own failure falls through to upgrade remediation. The remediation blocks stay: an absent release consults install, an out-of-sync one consults upgrade, both by retry count rather than by strategy. Two things are traded away. A real upgrade failure now retries instead of rolling back, which gives up no stable safety net, since with `retries: -1` the default strategy was already rolling back and re-upgrading without end. And the uninstall was the only automatic clean slate, so an install wedged on a bad resource state retries against that state until an operator removes the release. For a release that owns data that is the right exchange. This removes the consequence rather than the trigger, and for monitoring the trigger is worth naming separately: the parent HelmRelease here waits `10m`, while the child `monitoring-system` release sets `15m` deliberately, with a comment explaining that 10m is too short for the OIDC users-Job inside its `activeDeadlineSeconds: 900`. The parent's wait covers the child, so the parent can fail while the child is behaving exactly as designed. That mismatch is untouched here and wants its own change. The chart's etcd and seaweedfs releases lose storage the same way and are outside this change. Beyond those four, computeplane, ingress and info stay on the default strategy, but their charts ship no destructive `post-delete` hook, so remediation costs them a wasteful teardown rather than data. Same shape as #3552 and #3581. Relates to #3624. ### Screenshots <!-- REQUIRED for UI changes: attach screenshots or screen recordings demonstrating the visual impact of your changes. PRs with UI changes without screenshots will not be merged. --> ### Downstream repositories <!-- Cozystack is upstream for repositories that are NOT kept in sync with it automatically, and nothing in CI compares the two sides, so a change here can break them silently. Which change forces what, and which file to touch, is the trigger map in docs/agents/contributing.md under "Downstream Repositories". Walk it before you tick anything. IF YOU ARE AN AI AGENT OR AN LLM, THIS IS PART OF THE TASK, NOT A FORM TO FILL IN: 1. Walk the trigger map against the actual diff of this PR, file by file. Do not decide from the PR title or from your memory of what you changed. 2. For every repository you tick, open the follow-up PR there and link it on the same line. A ticked box with no link claims work that does not exist, which is worse than an empty box. Search that repository for an open PR or issue covering it first, and link that instead of filing a duplicate. 3. When the follow-up is out of scope, or needs a decision that is not yours to make, open an issue in that repository instead, link that, and say so in the PR body. Do not push a speculative PR into another repository just to fill a line here. 4. Do not tick "No downstream repository is affected" to make the checklist go away. If you are unsure, leave every box empty and say so in the PR body, so a human decides. --> - [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 <!-- Write a release note: - Explain what has changed internally and for users. - Start with the same `type(scope):` prefix as in the PR title - Follow the guidelines at https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md. --> ```release-note fix(tenant): retry a failed monitoring or gateway install instead of uninstalling the release, so a slow first install no longer runs the cleanup hooks that delete the tenant's metrics and logs volumes or its ACME account key ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Reliability Improvements** * Monitoring and gateway deployments now automatically retry failed installs and upgrades every 30 seconds. * Existing unlimited remediation retries remain available, improving recovery from deployment issues. * **Validation** * Added automated coverage to verify retry behavior for both monitoring and gateway deployments. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
36f83fa9b7 |
fix(tenant): retry failed etcd and seaweedfs installs in place (#3630)
<!-- Thank you for making a contribution! Here are some tips for you: - Use Conventional Commits for the PR title: `type(scope): description` - Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore - Scopes are not an exhaustive list — pick the most specific scope for the change and extend the list when a genuinely new area appears. Examples: - System components: dashboard, platform, operator, cilium, kube-ovn, linstor, fluxcd, cluster-api - Managed apps: postgres, mariadb, redis, kafka, clickhouse, virtual-machine, kubernetes - Development and maintenance: api, hack, tests, ci, docs, maintenance - Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or add a `BREAKING CHANGE:` footer - If it's a work in progress, consider creating this PR as a draft. - Don't hesistate to ask for opinion and review in the community chats, even if it's still a draft. - Add the label `backport` if it's a bugfix that needs to be backported to a previous version. --> ## What this PR does The tenant chart renders the etcd and seaweedfs releases with no install strategy, so they get Flux's default, and install remediation is an uninstall in every case Flux offers. Neither release sets `spec.uninstall.disableHooks`, which defaults to false, so that uninstall runs the app chart's `post-delete` cleanup Job. Nothing unusual is configured anywhere along that chain: fail early, uninstall, hooks. Those Jobs delete data on purpose. The etcd one removes the `data-etcd-<ordinal>` PVCs by name, because the etcd-operator does not reclaim them when the EtcdCluster goes. The seaweedfs one removes the volume PVCs by label and explicitly reclaims the keep-protected CNPG Cluster holding the filer metadata, because nothing else would. Both are right for a real uninstall, and neither hook is at fault here. Remediation is what calls them, on a release that is only starting slowly. So an install that misses its budget does not restart. It destroys the datastore the tenant control planes keep their state in, or the object store's data together with the metadata database that indexes it, and the next attempt begins from nothing. That budget is not a comfortable margin. The seaweedfs release has `timeout: 10m`, and `hack/e2e-install-cozystack.bats` already records that on a loaded runner the tenant stack only starts creating pods around 9 to 10 minutes in. The window CI observes and the window that triggers this are the same window. `RetryOnFailure` keeps the manifests applied and retries the failed release as an upgrade. No uninstall happens, so neither cleanup Job runs and the install already under way carries on instead of starting over. Both actions carry the strategy. The controller runs the retry of a failed install as an upgrade, so with install alone the retry's own failure falls through to upgrade remediation. The remediation blocks stay: an absent release consults install, an out-of-sync one consults upgrade, both by retry count rather than by strategy. One trade is that a real upgrade failure retries instead of rolling back. That costs no stable safety net. With `retries: -1` and the default strategy, upgrade remediation was already rolling back and retrying without end, so the release never rested in the rolled-back state either. The other side of the trade is that the uninstall was also the only automatic clean slate. On a tenant holding no data yet, an install that wedges on a bad resource state now retries against that state until an operator removes the release. For a release that owns data that is the right exchange, because lost data does not come back and a wedged install does. This covers two of the tenant's data-owning releases. The monitoring release carries the same default and a cleanup Job that deletes its PVCs as well, and is left for its own change, as is the rest of the catalog, whose nested releases want the question asked one at a time. Same shape as #3552 and #3581. Relates to #3555 and #3576. ### Screenshots <!-- REQUIRED for UI changes: attach screenshots or screen recordings demonstrating the visual impact of your changes. PRs with UI changes without screenshots will not be merged. --> ### Downstream repositories <!-- Cozystack is upstream for repositories that are NOT kept in sync with it automatically, and nothing in CI compares the two sides, so a change here can break them silently. Which change forces what, and which file to touch, is the trigger map in docs/agents/contributing.md under "Downstream Repositories". Walk it before you tick anything. IF YOU ARE AN AI AGENT OR AN LLM, THIS IS PART OF THE TASK, NOT A FORM TO FILL IN: 1. Walk the trigger map against the actual diff of this PR, file by file. Do not decide from the PR title or from your memory of what you changed. 2. For every repository you tick, open the follow-up PR there and link it on the same line. A ticked box with no link claims work that does not exist, which is worse than an empty box. Search that repository for an open PR or issue covering it first, and link that instead of filing a duplicate. 3. When the follow-up is out of scope, or needs a decision that is not yours to make, open an issue in that repository instead, link that, and say so in the PR body. Do not push a speculative PR into another repository just to fill a line here. 4. Do not tick "No downstream repository is affected" to make the checklist go away. If you are unsure, leave every box empty and say so in the PR body, so a human decides. --> - [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 <!-- Write a release note: - Explain what has changed internally and for users. - Start with the same `type(scope):` prefix as in the PR title - Follow the guidelines at https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md. --> ```release-note fix(tenant): retry a failed etcd or seaweedfs install instead of uninstalling the release, so a slow first install no longer runs the cleanup hooks that delete the etcd data volumes, the SeaweedFS volumes and the filer metadata database ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Reliability Improvements** * Improved installation and upgrade recovery for etcd and SeaweedFS by automatically retrying failed operations every 30 seconds. * Preserved ongoing remediation attempts to help services recover without uninstalling or rolling back after failures. * **Tests** * Added coverage to verify retry behavior and remediation settings for both services. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
79ceaf1dc2 |
fix(harbor): retry the nested system install instead of uninstalling it (#3621)
<!-- Thank you for making a contribution! Here are some tips for you: - Use Conventional Commits for the PR title: `type(scope): description` - Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore - Scopes are not an exhaustive list — pick the most specific scope for the change and extend the list when a genuinely new area appears. Examples: - System components: dashboard, platform, operator, cilium, kube-ovn, linstor, fluxcd, cluster-api - Managed apps: postgres, mariadb, redis, kafka, clickhouse, virtual-machine, kubernetes - Development and maintenance: api, hack, tests, ci, docs, maintenance - Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or add a `BREAKING CHANGE:` footer - If it's a work in progress, consider creating this PR as a draft. - Don't hesistate to ask for opinion and review in the community chats, even if it's still a draft. - Add the label `backport` if it's a bugfix that needs to be backported to a previous version. --> ## What this PR does Harbor's app chart renders a nested HelmRelease that owns the whole workload: the Deployments, the inline RedisFailover and its volume. It had no install strategy, so it got Flux's default, and install remediation is an uninstall in every case Flux offers. A first install that misses its budget gets torn down, and the next one provisions the redis volume and runs the master election again from scratch. That happens on a loaded runner. harbor-core waits on redis, redis waits on its volume and the master election, and the nginx Deployment hits the default `progressDeadlineSeconds` of 600s. Flux reads that as a stalled resource and fails the install early. The e2e wait budget is 10m, the same as the deadline, so after a teardown the second attempt has less time than a first install needs. `RetryOnFailure` keeps the manifests applied and retries as an upgrade. Nothing is deleted, so the volume, the elected master and harbor-core's startup all survive, and the retry costs 30s instead of a full install. A Deployment clears `ProgressDeadlineExceeded` by itself once its pods go ready, so the retry lands as soon as the stack converges. This drops the teardown, not the deadline. The suite applies the release in one step and waits for it in later ones, so part of the budget is still left when the fail-early hits. That leftover is useless to a fresh install and usable to a retry that resumes. Whether the deadline or the budget also needs moving is a separate question, and an early deadline only becomes safe once a failed install stops being torn down. In production the win holds without any of that: a slow first install no longer deletes the Redis volume and the database. Both actions carry the strategy. The controller runs the retry of a failed install as an upgrade, so with install alone the retry's own failure falls through to upgrade remediation. The remediation blocks stay: an absent release consults install, an out-of-sync one consults upgrade, both by retry count rather than by strategy. It goes on the nested release, not the parent. The parent is built from the Application by the apiserver, which already stamps this strategy on both actions. The nested one owns the workload and the volume a remediation deletes. Same shape as the tenant CNI and CSI releases in #3552 and #3581, and the unit test follows theirs. Relates to #3445. ### Screenshots <!-- REQUIRED for UI changes: attach screenshots or screen recordings demonstrating the visual impact of your changes. PRs with UI changes without screenshots will not be merged. --> ### Downstream repositories <!-- Cozystack is upstream for repositories that are NOT kept in sync with it automatically, and nothing in CI compares the two sides, so a change here can break them silently. Which change forces what, and which file to touch, is the trigger map in docs/agents/contributing.md under "Downstream Repositories". Walk it before you tick anything. IF YOU ARE AN AI AGENT OR AN LLM, THIS IS PART OF THE TASK, NOT A FORM TO FILL IN: 1. Walk the trigger map against the actual diff of this PR, file by file. Do not decide from the PR title or from your memory of what you changed. 2. For every repository you tick, open the follow-up PR there and link it on the same line. A ticked box with no link claims work that does not exist, which is worse than an empty box. Search that repository for an open PR or issue covering it first, and link that instead of filing a duplicate. 3. When the follow-up is out of scope, or needs a decision that is not yours to make, open an issue in that repository instead, link that, and say so in the PR body. Do not push a speculative PR into another repository just to fill a line here. 4. Do not tick "No downstream repository is affected" to make the checklist go away. If you are unsure, leave every box empty and say so in the PR body, so a human decides. --> - [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 <!-- Write a release note: - Explain what has changed internally and for users. - Start with the same `type(scope):` prefix as in the PR title - Follow the guidelines at https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md. --> ```release-note fix(harbor): retry a failed Harbor install instead of uninstalling the release, so a slow first install no longer deletes the Redis volume and starts over ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Harbor installations and upgrades now automatically retry every 30 seconds when failures occur. * Existing unlimited remediation retries remain available for both operations. * **Tests** * Added automated coverage to verify Harbor retry behavior and remediation settings. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
d956536461 |
fix(tenant): render the chart without the platform's injected values (#3636)
<!-- Thank you for making a contribution! Here are some tips for you:
- Use Conventional Commits for the PR title: `type(scope): description`
- Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore
- Scopes are not an exhaustive list — pick the most specific scope for
the change and extend the list when a genuinely new area appears.
Examples:
- System components: dashboard, platform, operator, cilium, kube-ovn,
linstor, fluxcd, cluster-api
- Managed apps: postgres, mariadb, redis, kafka, clickhouse,
virtual-machine, kubernetes
- Development and maintenance: api, hack, tests, ci, docs, maintenance
- Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or
add a `BREAKING CHANGE:` footer
- If it's a work in progress, consider creating this PR as a draft.
- Don't hesistate to ask for opinion and review in the community chats,
even if it's still a draft.
- Add the label `backport` if it's a bugfix that needs to be backported
to a previous version.
-->
## What this PR does
`helm lint packages/apps/tenant` fails on main with three errors. This
removes two of the three; the branch name notwithstanding, lint still
exits 1 afterwards on the third, which is a repository-wide convention
question rather than a defect in this chart.
**The chart cannot be rendered outside a cluster at all.**
`templates/keycloakgroups.yaml` reads `index .Values._cluster
"oidc-enabled"`, and `_cluster` is injected by the platform, so it is
absent whenever the chart is rendered on its own. `index` on a nil map
is a hard error, not an empty result, so both `helm lint` and a plain
`helm template` die on line 1.
The idiom the neighbouring charts use for the same lookup does not
rescue this on its own, which is worth stating because it looks exactly
like a guard. `(index .Values._cluster "x") | default "y"` still fails:
`index` is evaluated first and errors on the nil map, so `default` never
receives a result to substitute; it only ever covered a key missing from
a map that exists.
The spelling alone does not decide whether a chart breaks, though.
`packages/system/cozystack-basics` uses it and lints clean, because line
1 of its `values.yaml` is `_cluster: {}`, which makes the map non-nil
before `index` runs. A chart is broken only when it neither declares the
empty map nor guards at the use site, and a lint sweep across the
catalog found 27 in that state. Guarding at the use site is the choice
here because it makes the template correct whatever `values.yaml`
contains, and because `_cluster` is platform-internal while this chart's
`values.yaml`, schema and README are generated together from
`api/apps/v1alpha1/tenant/types.go` as user-facing output. The guard has
to sit on the map rather than on the value it returns:
```
{{- $oidcEnabled := index (.Values._cluster | default dict) "oidc-enabled" }}
```
**This is behaviour-preserving, and that is checked rather than
asserted.** With `_cluster` present, `| default dict` returns it
untouched and the lookup is what it was; with it absent the lookup is
nil, the `eq` below is false, and the block stays unrendered, which is
what an unconfigured tenant should produce. Rendering before and after
gives byte-identical output:
| values | before | after |
| --- | --- | --- |
| `_cluster.oidc-enabled: "true"` | 4 KeycloakRealmGroups |
byte-identical |
| `_cluster.oidc-enabled: "false"` | no groups | byte-identical |
| no values at all | hard error | renders, no groups |
Worth recording how that comparison has to be run, because the obvious
way proves nothing. The KeycloakRealmGroup block sits behind
`.Capabilities.APIVersions.Has "v1.edp.epam.com/v1"`, which is false
under a plain `helm template`, so a first pass rendered the same output
in *both* the enabled and disabled cases, and a diff of those two would
have compared one empty result against another. The renders above pass
`--api-versions v1.edp.epam.com/v1` so the guarded branch actually
executes. Anyone proving neutrality on a capability-gated template needs
that flag or the proof is vacuous.
**`Chart.yaml` does not declare `cozy-lib`**, which is present as a
symlink under `charts/`, so lint reports missing chart metadata. It is
declared the way `packages/apps/qdrant` does it, that being the one
chart in the tree which declares it and does not carry this error.
Rendering is unchanged by the declaration, checked the same way as
above.
Two things that could plausibly have broken and did not, checked rather
than assumed: rendering from a packaged `helm package` artifact is
byte-identical, and the `charts/cozy-lib` symlink is still expanded into
the tgz with the dependency declared. And neither `hack/update-crd.sh`
nor the `fix-charts` target in `packages/apps/Makefile` reads the
`dependencies:` block, so nothing needs regenerating.
**The icon error is left in place on purpose, and the relative path is
load-bearing.** `helm lint` wants a URL with a scheme, and
`/logos/tenant.svg` has none. But `hack/update-crd.sh` consumes exactly
this form: it rewrites a leading slash to `./`, then reads that file off
disk and base64-encodes it into `.spec.dashboard.icon` of the generated
ApplicationDefinition. `packages/apps/tenant/logos/tenant.svg` is that
file. Give the chart a URL with a scheme and the generator's `[[ ! -f
"$ICON_PATH" ]]` check no longer finds anything to encode. So the
relative form is a requirement of the pipeline rather than a stylistic
convention, all 31 icon-carrying charts use it, and satisfying helm lint
here means changing the generator, not the charts. That is a
repository-wide decision and not this PR's.
`tests/keycloakgroups_oidc_test.yaml` pins the gate in all three
directions: four realm groups when `oidc-enabled` is `"true"`, none when
it is `"false"`, and none when the platform injected no `_cluster` at
all. The last case is the one the guard exists for, and it needs no
mocking, since `_cluster` is absent from `values.yaml` and "not
injected" is just the chart's default state. Against the unguarded
template that suite fails with `index of untyped nil`. The chart's other
seven suites pass identically with and without the fix, which is
precisely why the change needed its own.
Two limits of the new suite, stated rather than left to be found. It
pins how many documents render, not which: renaming the four groups or
breaking a `realmRef` would pass. And the template's second gate,
`.Capabilities.APIVersions.Has "v1.edp.epam.com/v1"`, is enabled for the
whole suite, so no case covers its absence. Both are worth closing on
the next touch of this file; neither weakens what the suite does pin,
which is that the chart renders at all when the platform injected
nothing.
A trap for anyone reproducing this by hand: the gate compares against
the string `"true"`, so `--set _cluster.oidc-enabled=true` passes a YAML
boolean and renders nothing, while `--set-string` renders the four
groups. The platform writes a genuine string, and it survives the
round-trip through a Secret because `toYaml` quotes it. Note also that
this chart's helpers reject a release name that does not begin with
`tenant-`, which will silently render nothing for either form.
One note in case a future change teaches the linter to inject the
platform's values, which would hide this class of error without fixing
it: the guard is still worth having, because it is what lets `helm
template packages/apps/tenant` succeed with no values file at all. A
chart that renders standalone can be inspected, diffed and unit-tested
without reproducing the platform around it.
### Screenshots
<!-- REQUIRED for UI changes: attach screenshots or screen recordings
demonstrating
the visual impact of your changes. PRs with UI changes without
screenshots will not be merged. -->
### Downstream repositories
<!--
Cozystack is upstream for repositories that are NOT kept in sync with it
automatically, and nothing in CI compares the two sides, so a change
here can break them silently.
Which change forces what, and which file to touch, is the trigger map in
docs/agents/contributing.md under "Downstream Repositories". Walk it
before you tick anything.
IF YOU ARE AN AI AGENT OR AN LLM, THIS IS PART OF THE TASK, NOT A FORM
TO FILL IN:
1. Walk the trigger map against the actual diff of this PR, file by
file. Do not decide from the PR title or from your memory of what you
changed.
2. For every repository you tick, open the follow-up PR there and link
it on the same line. A ticked box with no link claims work that does not
exist, which is worse than an empty box. Search that repository for an
open PR or issue covering it first, and link that instead of filing a
duplicate.
3. When the follow-up is out of scope, or needs a decision that is not
yours to make, open an issue in that repository instead, link that, and
say so in the PR body. Do not push a speculative PR into another
repository just to fill a line here.
4. Do not tick "No downstream repository is affected" to make the
checklist go away. If you are unsure, leave every box empty and say so
in the PR body, so a human decides.
-->
- [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
<!-- Write a release note:
- Explain what has changed internally and for users.
- Start with the same `type(scope):` prefix as in the PR title
- Follow the guidelines at
https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md.
-->
```release-note
fix(tenant): the tenant chart now renders without the values the platform injects, so `helm template` works on it standalone, and its `cozy-lib` dependency is declared in Chart.yaml; this clears two of the three `helm lint` errors it reported, the remaining one being the repository-wide relative icon URL
```
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
- **New Features**
- Added support for using the shared Cozy library in tenant deployments.
- **Bug Fixes**
- Improved tenant configuration rendering when cluster settings are
unavailable.
- OIDC-related Keycloak groups now render reliably when OIDC is enabled
and required platform capabilities are present.
- Preserved expected behavior when OIDC is disabled or configuration is
omitted.
- **Tests**
- Added coverage for enabled, disabled, and missing cluster
configuration scenarios.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
|
||
|
Aleksei Sviridkin
|
c1071dab9b |
feat(kubernetes): let a node group log the guest serial console (#3637)
## What this PR does Both tenant Kubernetes e2e suites keep missing the node-Ready budget with zero Nodes registered, and the time goes inside the guest, between the worker VMI reaching Running and Talos asking for its apid certificate. Nothing collected today reaches that window. The in-guest capture the harness already has drives `talosctl`, so it needs the apid the worker never got to, and the management side has neither a Node nor a certificate request to read. Four attempts at this failure have been designed from outside the guest; the most recent raised the budget from 12m to 18m and the same fingerprint came back at the new deadline. KubeVirt can stream a VM's guest serial console into a `guest-console-log` container beside virt-launcher, readable with `kubectl logs` and needing nothing from inside the guest — no talosctl, no client certificate, no helper Pod, no reachable apid. That is the one property this failure class demands, so this turns it on for the suite's worker node group and collects it into the diagnostic bundle when node-join fails, ahead of the `talosctl` capture that cannot describe the same runs. Reads start at the beginning of the stream rather than tailing it: a guest that repaints its console would push the boot output out of any `--tail` window, while `--limit-bytes` truncates from the far end instead. Every read is bounded, and the walk stops at a cap that records both counts, so the collector cannot become an unbounded term in a failure path that already runs several others under one deadline. A read that returns no console is not filed as a quiet guest. Three outcomes get three labels, because only one of them says anything about the guest: the read failed and returned nothing, the read succeeded and returned nothing, or it broke off part way. Each read's stderr is kept beside the capture rather than inside it — kubectl's own `container guest-console-log is not valid for pod` would otherwise fill the file, and a console that never started would read as one that had spoken. That case is the headline one here, so the Pod state and events that explain it are collected alongside, once for the whole selector. The switch is a new opt-in `logSerialConsole` on a node group, off by default. It renders nothing unless set to true, including when set to false, because the `KubevirtMachineTemplate` is content-hash named and a byte emitted for a group that did not ask for it renames the template and rolls every live worker VM on upgrade. I verified that by rendering the base tree and this branch: absent and explicit-false produce a byte-identical template name on both charts. Worker VMs come from two charts, `kubernetes` and `kubernetes-nodes`; both carry the field, and the golden-parity check between them gains a case that keeps their output identical. The passing path checks separately that the container actually attached, and runs that check last, after everything the suite exists to prove — a debugging aid should not preempt the assertions that cover the cluster. Without it the suite would first learn the setting was inert in the one run that needed the console, when it can no longer be recovered. Relates to #3513. ### The risk this carries, stated up front The platform turns serial console logging off cluster-wide in the KubeVirt CR. That setting is a mitigation added in February 2026 for kubevirt/kubevirt#15989, where the `guest-console-log` container kept virt-launcher from starting and left VMs in `PodInitializing`. A per-VM `logSerialConsole` takes precedence over it — `isSerialConsoleLogEnabled` reads the VMI field first and only then falls back to the cluster setting — which is what makes this work, and also makes this the first thing on the platform to go through that mitigation. The mitigation was written against KubeVirt v1.6.x; the platform now ships v1.8.4, and the upstream issue is still open with no reproduction reported either way on the newer version. So the first run of these suites is also the experiment, and it doubles as a revalidation of that February mitigation on the version actually shipping, which nobody has done. The two outcomes are distinguishable rather than ambiguous: kubevirt/kubevirt#15989 shows up as virt-launcher wedged in `PodInitializing` with the VMI never reaching Running, while the failure this instruments shows the VMI Running and zero certificate requests. A red run with the first signature is an answer, not noise — and the collector is built so that it says which one happened instead of reporting silence. The blast radius is small: the product field is absent by default and only the e2e node group sets it, so backing this out is one line. The revert criterion, stated before the run rather than after it. If worker virt-launcher Pods sit in `Init` on `guest-console-log` and no Node registers, kubevirt/kubevirt#15989 still reproduces on v1.8.4: drop the `logSerialConsole: true` line from the e2e node group, keep the chart field, and the upstream issue gains the reproduction on a current version that its thread does not have. If the Pods reach Running and the console logs carry boot output, the mitigation is a candidate for removal platform-wide, which is its own change. Either way the run answers something that was open before it. Dropping `virtualMachineOptions.disableSerialConsoleLog` outright was the alternative. It is a smaller diff and needs no values key, but it exposes every VM on the platform to a race nobody has re-tested, instead of one node group in one test. That is its own revalidation exercise and belongs in its own change, on evidence this one produces. ### Verification `make unit-tests` passes. The new bats suite runs green under both `bash` and a `dash` `/bin/sh`, which matters because `hack/cozytest.sh` is `#!/bin/sh` and an earlier revision of this branch carried a bash-only construct that failed silently there. Every test in it is killed by a specific mutation of the code it covers. Both charts' helm-unittest suites pass, the golden-parity script reports byte-identical output across all four cases including the new one, and `make generate` is idempotent at the repo root. Reviewed by Claude across several rounds and by an independent second reader; the review loop found and fixed real defects in this branch, including a green-path check that had become unable to report the one thing it existed for. ### Screenshots Not a UI change. ### Downstream repositories - [ ] 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: https://github.com/cozystack/terraform-provider-cozystack/issues/26 - [ ] [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(kubernetes): worker node groups accept `logSerialConsole`, which streams a worker VM's guest serial console into a `guest-console-log` container readable with `kubectl logs`. Off by default. It is the only way to observe a worker that stalls during boot before Talos apid answers, when no Node and no certificate request exist to diagnose from. Enabling it rolls the node group and overrides the platform's cluster-wide `disableSerialConsoleLog`. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added an optional `logSerialConsole` setting for Kubernetes worker node groups. * Streams worker VM guest serial-console output to a dedicated log container when enabled. * Configuration is disabled by default and available through Kubernetes application settings. * **Bug Fixes** * Improved worker failure diagnostics with bounded serial-console output and pod-state information. * Validates that enabled worker pools attach the expected console-log container. * **Documentation** * Documented configuration behavior, rollout effects, defaults, and timeout considerations. * **Tests** * Added coverage for rendering, attachment validation, capture limits, and failure scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
93ffb38a9b |
fix(cozy-lib): make the cozyconfig helpers survive uninjected values (#3647)
<!-- Thank you for making a contribution! Here are some tips for you:
- Use Conventional Commits for the PR title: `type(scope): description`
- Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore
- Scopes are not an exhaustive list — pick the most specific scope for
the change and extend the list when a genuinely new area appears.
Examples:
- System components: dashboard, platform, operator, cilium, kube-ovn,
linstor, fluxcd, cluster-api
- Managed apps: postgres, mariadb, redis, kafka, clickhouse,
virtual-machine, kubernetes
- Development and maintenance: api, hack, tests, ci, docs, maintenance
- Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or
add a `BREAKING CHANGE:` footer
- If it's a work in progress, consider creating this PR as a draft.
- Don't hesistate to ask for opinion and review in the community chats,
even if it's still a draft.
- Add the label `backport` if it's a bugfix that needs to be backported
to a previous version.
-->
## What this PR does
`_cluster` and `_namespace` reach a chart through valuesFrom on the
cozystack-values Secret, so they are absent whenever a chart is rendered
on its own: `helm lint`, a plain `helm template`, and any helm-unittest
suite that does not set them. Twelve of the helpers in `_cozyconfig.tpl`
read straight through the missing map and abort the render instead of
returning the empty default they document.
Two spellings, one cause. `index .Values._cluster "root-host"` raises
`index of untyped nil`; `.Values._namespace.host` raises `nil pointer
evaluating interface {}.host`. In both, the trailing `| default ""`
never runs, because it applies to the result and there is no result,
only an error. That default covered a key missing from a map that
exists, never a missing map.
The guard goes on the map, which is what `cozy-lib.images-registry` in
this same file already did:
```
(index (.Values._cluster | default dict) "root-host") | default ""
```
`index` on an empty map returns nil harmlessly, so the trailing default
applies as it always read, and with the map present `| default dict`
returns it untouched.
**This fixes no chart that is failing today.** The charts that cannot
render standalone read the injected values directly in their own
templates rather than through these helpers, so a change to the library
does not reach them; they are handled where they live. Worth being blunt
about the same point from the other side: twelve of the thirteen helpers
here currently have no callers anywhere in `packages/`. Only
`cozy-lib.images-registry` is reached, from `cozy-lib.image`, and it was
already guarded. The guard is still worth having on the rest, because
they are the library's public surface, they are what a chart author
copies when reaching for cluster configuration, and a broken spelling
propagates by being read. `cozy-lib.image` is called from four
templates, so these helpers do get adopted once they exist.
**Rendered output does not change, and that is measured rather than
argued.** All 34 charts that symlink cozy-lib were rendered before and
after, both with the injected values populated and with them set to
empty maps, and every result is byte-identical. That check needs a
control: the first comparison showed eight charts differing, which looks
like a regression in a library change. Rendering the *unchanged* tree
twice reproduces exactly those eight, because they generate passwords.
Subtracting the control leaves zero real differences.
The suite lives in `cozy-lib-tests`, which is where library behaviour is
pinned, since a library chart renders nothing of its own. One case
covers the uninjected state, which needs no mocking because neither key
appears in any values file, and one covers the injected state so the
guard cannot quietly change what a populated map returns. The injected
case pins `scheduling` and `branding` as whole YAML documents rather
than by substring: those two had their bodies restructured rather than
wrapped, and a substring assertion survives the loss of the `toYaml`
that produces the document, because the key name also appears in Go's
default map formatting. Against the unguarded helpers the first case
fails with `index of untyped nil`.
Three things left deliberately undone, so they are not mistaken for
oversights. The header comments in `_cozyconfig.tpl` say where these
values come from but not what the helpers now do when they are absent,
which is the contract this change creates and would be better stated
next to the helpers than only in the suite. `Chart.yaml` in
cozy-lib-tests still carries `name: quota` from when that chart covered
one helper family, so the runner prints `### Chart [ quota ]` for a
suite that now covers three. And the mixed state, one map injected and
the other not, has no case of its own; the two guards are independent so
composition covers it, and a third case would pin nothing new.
Relates to #3646.
### Screenshots
<!-- REQUIRED for UI changes: attach screenshots or screen recordings
demonstrating
the visual impact of your changes. PRs with UI changes without
screenshots will not be merged. -->
### Downstream repositories
<!--
Cozystack is upstream for repositories that are NOT kept in sync with it
automatically, and nothing in CI compares the two sides, so a change
here can break them silently.
Which change forces what, and which file to touch, is the trigger map in
docs/agents/contributing.md under "Downstream Repositories". Walk it
before you tick anything.
IF YOU ARE AN AI AGENT OR AN LLM, THIS IS PART OF THE TASK, NOT A FORM
TO FILL IN:
1. Walk the trigger map against the actual diff of this PR, file by
file. Do not decide from the PR title or from your memory of what you
changed.
2. For every repository you tick, open the follow-up PR there and link
it on the same line. A ticked box with no link claims work that does not
exist, which is worse than an empty box. Search that repository for an
open PR or issue covering it first, and link that instead of filing a
duplicate.
3. When the follow-up is out of scope, or needs a decision that is not
yours to make, open an issue in that repository instead, link that, and
say so in the PR body. Do not push a speculative PR into another
repository just to fill a line here.
4. Do not tick "No downstream repository is affected" to make the
checklist go away. If you are unsure, leave every box empty and say so
in the PR body, so a human decides.
-->
- [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
<!-- Write a release note:
- Explain what has changed internally and for users.
- Start with the same `type(scope):` prefix as in the PR title
- Follow the guidelines at
https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md.
-->
```release-note
fix(cozy-lib): the cozyconfig helpers now return their documented empty defaults when the platform has not injected `_cluster` / `_namespace`, instead of aborting the render, so a chart using them can be rendered and unit-tested on its own
```
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
- **Bug Fixes**
- Improved configuration handling when cluster or namespace settings are
missing, preventing template rendering errors.
- Preserved existing configuration values for scheduling, branding,
networking, registries, and component namespaces.
- **Tests**
- Added coverage for configurations with missing platform values.
- Added validation that injected cluster and namespace settings render
correctly.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
|
||
|
Aleksei Sviridkin
|
7b36a5d2f2 |
test(tenant): give these template lists their real reason (#3651)
## What this PR does Two helm-unittest suites in `packages/apps/tenant/tests/` said they restrict rendering because `namespace.yaml`'s `lookup` errors under helm-unittest. It does not. The lookup returns empty offline and the template renders, which `namespace.yaml`'s own header states and which `tests/namespace_ancestor_labels_test.yaml` depends on. I measured what each list actually does by removing it, and the two suites turned out to be different cases rather than two copies of one. In `cleanup_role_watch_test.yaml` the list is load-bearing, for a template the comment never mentioned. That suite sets no values, and `keycloakgroups.yaml` indexes `.Values._cluster` on its first line, so the whole-chart render fails with `index of untyped nil` and all 8 tests error. The comment now says that, and says it conditionally: a guard on that index is already in flight, and once it lands the list stops being load-bearing. I checked that too, by applying the guard and re-running with the list removed, which passes 8/8. In `vmo_webhook_dependson_test.yaml` the list is not load-bearing at all. That suite sets `_cluster`, the chart renders without the list, and all 5 tests pass. It stays only to scope the suite, and the comment says so instead of inventing a technical reason. Each comment now states what holds for its own file and nothing wider. ### Screenshots Not a UI change. ### Downstream repositories Walked the trigger map against the diff. It touches two files, both under `packages/apps/tenant/tests/`, and no chart, template, values file or tooling. Nothing in the map keys on test files. - [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 ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Clarified test-rendering rationale and suite-scoping comments. * Documented that rendering requires `_cluster` values and that template lists control test scope. * **Tests** * Updated explanatory comments only; test behavior and configuration remain unchanged. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
4740daa7be |
test(tenant, harbor, seaweedfs, api): anchor guards that pass on a missing document (#3639)
## What this PR does Five helm-unittest suites asserted things that stayed green when the thing they guard disappeared. No chart or template logic changes here — only the tests, and each fix is measured in both directions. **tenant `no_upgrade_force_test.yaml`.** Every test asserted `notExists: spec.upgrade.force` and nothing else. A `notExists` passes on a template that renders no document at all, so renaming the value that gates one of these releases leaves its test green while the release it guards is gone. Measured on `etcd.yaml`: with the gate renamed, the suite reported 6 passed and a sibling suite's `equal` was the only thing in the chart that noticed. Each test now pins the HelmRelease name alongside the absence. The same file also covers `computeplane.yaml` now. Seven templates in this chart render a HelmRelease and the list held six; the module landed after the suite was written. The invariant holds there today, so this closes an unguarded template rather than a live regression. That is the gap that produced this suite in the first place, when the first sweep missed `gateway.yaml`. Two rationales in that header were wrong and are corrected. The template list was said to be there because `namespace.yaml`'s `lookup` errors under helm-unittest; it does not, it returns empty offline and the template renders, which is what `namespace.yaml`'s own header says and what `namespace_ancestor_labels_test.yaml` relies on. Removing the whole list leaves the chart's suites passing, so the list stays for the reason that actually applies. And helm-controller does not deprecate `upgrade.force`: the CRD vendored at `internal/fluxinstall/manifests/fluxcd.yaml`, which is what the pinned Flux 2.8.x ships, describes it as a replacement strategy with no deprecation marker. `recreate` is the neighbouring field that carries one. **harbor, both suites.** Both addressed their subject by document index. An index is a position, so a template that gains a document ahead of the one under test moves every later assertion onto a different object. Measured by inserting one document at the head of each template: the cleanup suite reddens outright, while two of the four jobservice tests keep passing: the two `notExists` ones, satisfied against an object that never had the path. Every document these suites assert on is unique by kind within its template, so each test now names its subject. A selector that matches nothing fails loudly, including on the `notExists` tests. Indices are also easy to get wrong by hand: helm-unittest numbers documents in template source order while `helm template` prints them sorted by kind, so the rendered output is not the order being counted. **seaweedfs and cozystack-api.** Both forbade a rule by naming it in full, and helm-unittest compares every field it is handed by value equality, so one respelling walks past. For seaweedfs that is not theoretical: ungating the cnpg block and adding a verb to the rule is the same grant and more, and the guard stayed green while a Client instance held delete on another app's Postgres cluster. The Role is now bounded by shape. A Client renders exactly two rules, both in the core group, and the cnpg grant is gated between them, so ungating it adds a third whatever its spelling. For cozystack-api the count bound sits beside the existing named-rule assert rather than replacing it. Neither covers the other: `templates/rbac.yaml` carries no template directives, so the list is pinned at exactly sixteen rules from both sides, while a grant swapped in for an existing one leaves the count where it was and only the named-rule assert sees it. Measured on all three: append, delete, and swap. ### Screenshots Not a UI change. ### Downstream repositories Walked the trigger map against the diff. It touches five files, all under `packages/*/*/tests/`, and no chart, template, values file or tooling. Nothing in the map keys on test files. - [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 ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Tests** - Improved validation of rendered Kubernetes resources across Harbor, tenant, SeaweedFS, and system API configurations. - Tests now identify resources by kind and relevant properties, making checks more reliable when rendered document ordering changes. - Expanded coverage for tenant upgrade settings, compute plane resources, storage class propagation, cleanup permissions, and network policy access. - Added stricter assertions for expected resource counts, names, rules, and omitted settings. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
50c5e2c14c |
test(e2e): capture the tenant worker guest serial console
The tenant Kubernetes suites fail the node-Ready budget with zero Nodes registered, and the time is lost inside the guest between the worker VMI reaching Running and Talos requesting its apid certificate. Nothing collected today covers that window: the existing in-guest capture drives talosctl, so it needs the very apid the worker never reached, and on the management side there is neither a Node nor a certificate request to read. Turn on the guest console log for the suite's node group and collect each virt-launcher's guest-console-log container from the management cluster. Reads start at the beginning of the stream rather than tailing it, so a guest that repaints its console cannot push the boot output out of the window; every read is bounded, and the walk stops at a cap that records both counts, so the collector cannot become the unbounded term in a failure path that already runs several others under one deadline. A read that returns no console is not filed as a quiet guest. Three outcomes get three labels, because only one of them says anything about the guest: the read failed and returned nothing, the read succeeded and returned nothing, or it broke off part way. Each read's stderr is kept beside the capture rather than inside it, since kubectl's own "container is not valid for pod" would otherwise fill the file and a console that never started would read as one that had spoken. That case is the headline one here, so the Pod state and events that explain it are collected alongside, once for the whole selector. Enabling the console overrides a cluster-wide setting the platform holds for an open upstream defect, so the run is also an experiment, and its own failure mode is the one that hides best: the container wedges virt-launcher in Init, no worker boots, and the suite reports "fewer than 2 tenant nodes Ready" -- the same sentence the failure under study produces. Read as the known signature, the experiment's answer would be filed as the flake it was meant to explain. So the failure path names that case first, before any other diagnostic, and only when it is true. The passing path checks separately that the container attached at all, and reports the ratio it saw rather than a bare success, since the check is deliberately satisfied by any one Pod. It keeps three outcomes apart too: a read that did not answer, a selector that matched no Pod, and Pods that matched and carry nothing. It reads the Pod name alongside the container names so the last two cannot collapse, and both container lists so a future move of the container cannot fail a run over a diagnostic. It runs last, after everything the suite exists to prove, because a debugging aid must not preempt the assertions that cover the cluster. The capture runs before the talosctl one because it is the only of the two that can describe a worker which never reached apid, the shape this failure keeps taking. kubectl reports a warning and an empty result alike, on stderr with a zero status, so the exit status decides which artifact its output lands in. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
1cb51c92aa |
test(tenant): give these template lists their real reason
Both suites here said they restrict rendering because namespace.yaml's `lookup` errors under helm-unittest. It does not. The lookup returns empty offline and the template renders, which namespace.yaml's own header states and which tests/namespace_ancestor_labels_test.yaml depends on. Measured what each list actually does by removing it. In cleanup_role_watch_test.yaml the list is load-bearing, for a template the comment never mentioned: the suite sets no values and keycloakgroups.yaml indexes `.Values._cluster` on its first line, so the whole-chart render fails with "index of untyped nil". Stated conditionally, because a guard on that index is already in flight and would end it. In vmo_webhook_dependson_test.yaml the list is not load-bearing at all, because that suite sets `_cluster` and the chart renders without it. It stays only to scope the suite. Each comment now states what holds for its own file and nothing wider. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
myasnikovdaniil
|
7dca642894 |
fix(ci): restore e2e for fork PRs and close the merge-gate bypass (#3262)
## What this PR does Fixes #3257 — fork PRs never ran e2e, and the skipped required check let them merge anyway. Fork PRs get no secrets on `pull_request`, so `make image` pushed anonymously and died with `denied`. That failed every build, collapsed the `build → finalize → e2e` chain, and left the required **E2E Tests** job *skipped* — which branch protection treats as satisfied, so an approved fork PR could merge with e2e never having run. This implements **Option A** from the issue (privileged `workflow_run` split), pushing fork e2e images to the existing CI registry (OCIR): **Build primitive (`hack/common-envs.mk`)** - New `OCI_EXPORT_DIR` mode: build each image to a per-image OCI archive instead of pushing, and force `PUSH`/`LOAD` off. The digest is captured via `--metadata-file` regardless of output type, so the refs baked into `pr.patch` match what the privileged run later pushes. Threaded through the `image-tags` macro every package already uses — no per-package Makefile change. **`pull-requests.yaml` (unprivileged, fork branch)** - `build` / `build-talos` / `finalize` export images to OCI archives and upload them as artifacts instead of pushing → the red `denied` build wall disappears and the chain no longer collapses. - The in-tree e2e job is renamed off `"E2E Tests"` and guarded to same-repo PRs. A new `e2e-report` job concludes the required `"E2E Tests"` context as an **explicit check-run** (success on a docs-only PR or when e2e passed, failure otherwise). A skipped job can no longer satisfy the required check — this also closes the same hole for same-repo PRs whose build fails. **`e2e-fork.yaml` (new, privileged `workflow_run` from the default branch)** - Resolves the fork PR, opens a pending `"E2E Tests"` check-run, and fails-closed if the fork build failed. - Pushes the pre-built image archives to OCIR **by digest** and flux-pushes the digest-pinned packages tree. The OCIR token is live only for trusted binaries (`git`/`skopeo`/`flux`/`yq`) over data — never for `make` or any fork-authored script. - Runs the e2e suite against the pushed closure with **no push token** and concludes the check-run on the PR head SHA. Fork build code never receives registry credentials; the fork's images and test scripts run only in the credential-less, ephemeral e2e job — a stronger posture than a `pull_request_target` run that would hand fork code the secrets directly. ### Validation Local: buildx OCI-archive output + digest capture, macro expansion for single- and multi-image packages, non-fork push path unchanged, `actionlint` clean. Needs live validation (why this is a **draft**): `workflow_run` artifact download across runs; that the `"E2E Tests"` commit status satisfies the required context and keeps a fork PR non-mergeable until the privileged run concludes it; `refs/pull/<N>/merge` fetch + `git apply --3way`; `skopeo --preserve-digests` end-to-end. `workflow_run` only runs from the default branch, so the privileged half is exercised by mirroring the workflow onto a personal fork's default branch and opening a throwaway fork PR into it — not by merging first. _Since review_: the fork `e2e` job ran the pre-#2826 BATS app loop that the merged Chainsaw migration deleted (already broken); it now runs the same Chainsaw + TIA path as the in-tree job, with suite selection and the `full-e2e` override sourced from `resolve`'s trusted base-repo file list / labels (not a merge-ref diff). Publish job hardened per the non-blocking follow-ups: base-tree image allowlist, `skopeo copy --all`, pinned flux CLI. The required `E2E Tests` gate is now published as a commit **status** (default token + `statuses:write`), not a COZYSTACK_CI app check-run — the app lacked `checks:write` (`checks.create` 403'd) and a default-token check-run floated under a labeler suite; a commit status has neither problem, and forks can't post one (their `pull_request` token is read-only). Two review-bot findings fixed too: fork `OCI_EXPORT_DIR` is now absolute (a relative path misplaced archives under `make -C <pkg>` and every fork build would fail at upload), and the "≥1 archive" guard no longer no-ops under `nullglob`. ### Repo-admin prerequisites One repository setting lives outside this diff: 1. **Branch protection: keep `E2E Tests` required.** The gate is now a commit **status** named `E2E Tests` (posted with the default `GITHUB_TOKEN` — see `docs/agents/e2e-testing.md` §10), not a job or an app check-run. Branch protection already requires `E2E Tests` (currently the e2e job's name, renamed here to `E2E (in-tree)`), so the required context now resolves to the status — **no GitHub App, no `checks:write` grant, no app-id pin needed**. Just don't pin the context to a specific app. 2. **Actions → "Require approval for all outside collaborators".** Each fork run must be a deliberate maintainer click before it executes, since the fork's unprivileged run produces the artifacts the privileged `e2e-fork.yaml` consumes. ### Downstream repositories Walked the trigger map in `docs/agents/contributing.md` against the diff (7 files: two CI workflows, `docs/agents/e2e-testing.md`, `hack/common-envs.mk` + its bats suite, and two package `Makefile`s). The one trigger worth naming is "change developer tooling (`cozyvalues-gen`, `cozypkg`, the package `Makefile`s) → `development.md`" on the website. It does not fire here: `OCI_EXPORT_DIR` is a CI-only build mode that defaults off, and the local developer workflow is unchanged — `make image` still builds and pushes exactly as before, which the new default-path case in `hack/common-envs_test.bats` now pins. No chart values, no API surface, no app list, no platform variant, no Talos version bump and no release asset rename are touched. - [x] No downstream repository is affected by this change ### Release note ```release-note fix(ci): fork pull requests now run e2e via a privileged workflow, and a fork PR can no longer merge on a skipped (never-run) required E2E Tests check ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added a dedicated fork E2E workflow that runs after the base PR and reports results via the required **E2E Tests** commit status. - Fork PRs now export build images as OCI archive artifacts for later publishing. - Added a same-repo E2E reporting job and renamed the in-tree E2E flow to **E2E (in-tree)**. - **Bug Fixes** - Added tighter, fail-closed gating to ensure merge blocking when **E2E Tests** isn’t correctly finalized. - **Documentation** - Documented fork vs same-repo E2E split, OCI export behavior, and required fork security settings. - **Tests** - Added Bats coverage validating `OCI_EXPORT_DIR` and preventing unintended registry pushes. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
85fd592cc7 |
fix(cozy-lib): make the cozyconfig helpers survive uninjected values
`_cluster` and `_namespace` reach a chart through valuesFrom on the
cozystack-values Secret, so they are absent whenever a chart is rendered
on its own: `helm lint`, a plain `helm template`, and any helm-unittest
suite that does not set them. Twelve of the helpers in _cozyconfig.tpl
read straight through the missing map and abort the render rather than
returning their documented empty default.
Two spellings, one cause. `index .Values._cluster "root-host"` raises
`index of untyped nil`, and `.Values._namespace.host` raises
`nil pointer evaluating interface {}.host`. In both cases the trailing
`| default ""` never runs: it applies to the result, and there is no
result, only an error. That default covered a key missing from a map
that exists, never a missing map.
Guard the map instead, which is what `cozy-lib.images-registry` in this
same file already did:
(index (.Values._cluster | default dict) "root-host") | default ""
`index` on an empty map returns nil harmlessly, so the trailing default
applies as it always read. With the map present, `| default dict`
returns it untouched.
This changes no rendered output. Every one of the 34 charts that symlink
cozy-lib was rendered before and after, both with the injected values
populated and with them set to empty maps, and every result is
byte-identical. Eight charts differ between any two runs because they
generate passwords; rendering the unchanged tree twice reproduces
exactly those eight, which is how they are excluded.
What this does not do is fix a chart that fails today. The charts that
cannot render standalone read the injected values directly in their own
templates rather than through these helpers, so they are unaffected by a
change to the library and are being addressed where they live.
Twelve of the thirteen helpers here have no callers anywhere in
packages/ at present; only `cozy-lib.images-registry` is reached, from
`cozy-lib.image`, and it was already guarded. The guard is still worth
having on the rest: they are the library's public surface, they are what
a chart author copies when reaching for cluster configuration, and the
broken spelling propagates by being read. `cozy-lib.image` is called
from four templates, so these helpers do get adopted once they exist.
Add a suite in cozy-lib-tests, which is where library behaviour is
pinned because a library chart renders nothing of its own. One case
covers the uninjected state, which needs no mocking since neither key is
in values.yaml, and one covers the injected state so the guard cannot
quietly change what a populated map returns. The injected case includes
`scheduling` and `branding` specifically: those two are the helpers
whose body was restructured rather than wrapped, so they are the ones
whose present-map behaviour most needs pinning. Against the unguarded
helpers the first case fails with `index of untyped nil`.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
|
||
|
Aleksei Sviridkin
|
9d1d6808db |
test(seaweedfs,api): bound both Roles against a respelled rule
Each suite forbade a rule by naming it in full, and helm-unittest compares every field it is handed by value equality, so one respelling walks past. For seaweedfs that is not theoretical. Ungating the cnpg block and adding a verb to the rule (get,list,delete rather than get,delete) is the same grant and more, and the guard stayed green while a Client instance held delete on another app's Postgres cluster. Bound the Role by shape: a Client renders exactly two rules, both in the core group, and the cnpg grant is gated between them, so ungating it adds a third whatever its spelling. The rules themselves stay unpinned because the secret list is long and changes on its own schedule. For cozystack-api, add a count bound beside the existing spelling assert rather than in place of it. The two catch different things and neither covers the other: templates/rbac.yaml carries no template directives, so the list is pinned at exactly sixteen rules, while a grant swapped in for an existing one leaves the count where it was and only the named-rule assert sees it. The count is pinned from both sides — a dropped grant is the failure this suite is about, since the apiserver then returns forbidden and List silently drops the dropdown. Measured on all three: append, delete, and swap. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
a3c1408d08 |
test(harbor): select hook and release documents by kind, not by index
Both suites addressed their subject by document index. An index is a position, so a template that gains a document ahead of the one under test moves every later assertion onto a different object, and what happens then depends on the assertion: measured by inserting one document at the head of each template, the cleanup suite reddens outright, while two of the four jobservice tests keep passing because a `notExists` against an object that never had the path is satisfied for the wrong reason. Indices are also easy to get wrong by hand. helm-unittest numbers documents in template source order, while `helm template` prints them sorted by kind, so the rendered output is not the order being counted. Every document these suites assert on is unique by kind within its template, so each test now names its subject. A selector that matches nothing fails, including on the `notExists` tests, so the anchor holds in the direction that was silent before. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
eac7e1ea15 |
test(tenant): anchor the upgrade.force guard to its HelmRelease
Each test asserted only `notExists: spec.upgrade.force`, which passes on a template that renders no document at all. Renaming the value that gates one of these releases makes the template render nothing and leaves its test green: measured on etcd.yaml, where the guard stayed green while a sibling suite's `equal` was the only thing that noticed the release had vanished. Pin each HelmRelease name alongside the absence so the assertion has a document to be about. Cover computeplane.yaml. Seven templates in this chart emit a HelmRelease and the list held six; the module arrived after the suite was written. The invariant holds there today, so this closes an unguarded template rather than a live regression — which is the same gap that produced this suite, whose first pass missed gateway.yaml. Correct two claims in the header. The template restriction was said to be there because namespace.yaml's `lookup` errors under helm-unittest; it does not, the lookup returns empty offline and the template renders, which is what its own header says and what namespace_ancestor_labels_test.yaml relies on. The restriction stays, for the reason that actually applies: it keeps an unrelated template breaking out of this suite's result. helm-controller also does not deprecate upgrade.force. The CRD vendored at internal/fluxinstall/manifests/fluxcd.yaml, which is what the pinned Flux 2.8.x ships, describes it as a replacement strategy and carries no deprecation marker; `recreate` is the neighbouring field that does. Later upstream documentation adds that the field is ignored under server-side apply, and that sentence is not in the pinned CRD, so the comment says which is which. The reason to keep the flag off is client-side apply, where it replaces resources instead of patching them. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
6d403c3015 |
fix(tenant): retry failed etcd and seaweedfs installs in place
The tenant chart renders both releases with Flux's default install strategy, and install remediation is an uninstall in every case Flux offers. Neither release disables hooks on it, so a remediated install runs each chart's post-delete cleanup Job, and those Jobs delete data on purpose. The etcd one removes the data-etcd-<ordinal> PVCs by name, because the etcd-operator does not reclaim them when the EtcdCluster goes. The seaweedfs one removes the volume PVCs by label and explicitly reclaims the keep-protected CNPG Cluster holding the filer metadata, precisely because nothing else removes it. Both are correct for a real uninstall. Remediation is what calls them, on a release that is only starting slowly. So an install that misses its budget does not merely restart. It destroys the datastore the tenant control planes keep their state in, or the object store's data together with the metadata database that indexes it, and the next attempt begins from nothing. RetryOnFailure keeps the manifests applied and retries the failed release, so no uninstall happens and neither cleanup Job runs. Both actions carry the strategy because the controller performs that retry as an upgrade, so on install alone the retry's own failure falls through to upgrade remediation. The cost is that a real upgrade failure retries rather than rolls back, which gives up no stable safety net: with retries: -1 and the default strategy, upgrade remediation already rolled back and retried without end. The remediation blocks stay because an absent release consults install and an out-of-sync one consults upgrade, both by retry count rather than by strategy. The other side of the trade is that the uninstall was also the only automatic clean slate. On a tenant holding no data yet, an install that wedges on a bad resource state now retries against that state until an operator removes the release. For a release that owns data this is the right exchange, because lost data does not come back and a wedged install does. This covers two of the tenant's data-owning releases. The chart's monitoring release destroys its storage on a remediated install as well and is outside this change, as is the rest of the catalog, whose nested releases want the question asked one at a time. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
bd3f3b93d0 |
fix(tenant): render the chart without the platform's injected values
`helm lint packages/apps/tenant` fails, and two of its three errors are
the chart's own.
`templates/keycloakgroups.yaml` reads `index .Values._cluster
"oidc-enabled"`. `_cluster` is injected by the platform, so it is absent
whenever the chart is rendered on its own, and `index` on a nil map is a
hard error: `index of untyped nil`. The chart cannot be rendered or
linted outside a cluster at all.
The idiom the neighbouring charts use for the same lookup does not
rescue this on its own, which is worth stating because it looks like it
does. `(index .Values._cluster "x") | default "y"` still errors: `index`
evaluates first and fails on the nil map, so `default` never sees a
result to substitute; it only ever covered a key missing from a map that
exists.
The spelling alone does not decide whether a chart breaks, though.
`packages/system/cozystack-basics` uses it and lints clean, because line
1 of its values.yaml is `_cluster: {}`, which makes the map non-nil
before `index` runs. So a chart is broken only when it neither declares
the empty map nor guards at the use site. Guarding at the use site is
preferred here because it makes the template correct whatever
values.yaml contains, which is what lets the chart render standalone;
and `_cluster` is platform-internal, while this chart's values.yaml,
schema and README are generated together from
api/apps/v1alpha1/tenant/types.go as user-facing output.
The guard has to sit on the map rather than on the value it returns,
hence `index (.Values._cluster | default dict) "oidc-enabled"`.
That is behaviour-preserving rather than merely quieter. With `_cluster`
present, `| default dict` returns it untouched and the lookup is what it
was. With it absent the lookup is nil, the `eq` below is false, and the
block stays unrendered, which is what an unconfigured tenant should
produce. Rendered both ways before and after, with
`--api-versions v1.edp.epam.com/v1` so the capability-gated block
actually executes, the output is identical byte for byte: four
KeycloakRealmGroups when `oidc-enabled` is `"true"`, none when it is
`"false"`. Rendering with no values at all goes from a hard error to a
clean render with no groups.
`Chart.yaml` also does not declare `cozy-lib`, which is present as a
symlink under `charts/`, so lint reports missing chart metadata. Declare
it the way `packages/apps/qdrant` does, that being the one chart in the
tree that declares it and does not carry this error. Rendering is
unchanged by the declaration, checked the same way.
`tests/keycloakgroups_oidc_test.yaml` pins all three branches of the
gate: four realm groups when `oidc-enabled` is `"true"`, none when it is
`"false"`, and none when the platform injected no `_cluster` at all.
That third case is the one the guard exists for, and it needs no mock,
because `_cluster` is absent from values.yaml and so "not injected" is
simply the chart's own default state. Against the unguarded template the
suite fails with `index of untyped nil`; the chart's other seven suites
pass identically either way, which is what left the change uncovered.
The third error, `invalid icon URL '/logos/tenant.svg'`, is deliberately
left. Lint wants a URL with a scheme, and all 31 charts in the tree that
carry an icon use a relative path, none an absolute one. Changing this
one would make it the only chart with a different convention, and
choosing the URL a relative path should become is a decision about where
those assets are served from, not about lint.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
|
||
|
Aleksei Sviridkin
|
a27c341843 |
fix(tenant): retry failed monitoring and gateway installs in place
Both releases carry Flux's default install strategy, and install remediation is an uninstall in every case Flux offers. Neither disables hooks on it, so a remediated install runs the app chart's post-delete cleanup Job against a release whose only fault was missing a readiness budget. The monitoring Job deletes PVCs labelled apps.cozystack.io/application.name=<release>-system. That label is not incidental: the metrics and logs claim templates stamp it precisely so the operator copies it onto the storage PVCs, and the cleanup Job selects on it to stay release-scoped. A slow first install therefore deletes the tenant's metrics and logs, and nothing reconstructs them. The gateway Job deletes the cozystack-acme-account Secret. That key regenerates on the next install, so this one costs no data. It costs a re-registration against the ACME provider, and repeated cycles run into its rate limits, which can hold certificate issuance for the tenant down for hours. RetryOnFailure keeps the manifests applied and retries the failed release, so no uninstall happens and neither cleanup Job runs. Both actions carry the strategy because the controller performs that retry as an upgrade, so on install alone the retry's own failure falls through to upgrade remediation. Two things are traded away. A real upgrade failure now retries rather than rolls back, which gives up no stable safety net, since with retries: -1 and the default strategy upgrade remediation already rolled back and retried without end. And the uninstall was the only automatic clean slate, so an install wedged on a bad resource state retries against that state until an operator removes the release. The remediation blocks stay because an absent release consults install and an out-of-sync one consults upgrade, both by retry count rather than by strategy. The chart's etcd and seaweedfs releases lose storage the same way and are outside this change. Beyond those four, computeplane, ingress and info remain on the default strategy, but their charts ship no destructive post-delete hook, so remediation costs them a wasteful teardown rather than data. That is a different question from this one. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
c20d47a245 |
feat(kubernetes): let a node group log the guest serial console
A worker VM that stalls during boot before Talos apid answers is invisible to every diagnostic the platform has: no Node registers, no certificate request arrives, and talosctl needs the apid that never came up. The guest serial console does show that window, but KubeVirt attaches its guest-console-log container only when the VMI asks for it, and the platform turns console logging off cluster-wide. Add an opt-in per-node-group logSerialConsole that sets the KubeVirt field, which takes precedence over the cluster-wide setting. The key renders nothing unless true -- including when set to false -- because the KubevirtMachineTemplate is content-hash named, so a byte emitted for a group that did not ask for it renames the template and rolls every live worker VM. The description carries the two costs the field name does not show. Changing the value rolls the group it is set on, so the stuck VM whose console was wanted is replaced along with the rest and the setting buys visibility into the next boot rather than the current one. And the cluster-wide default it overrides exists because that same container has been seen holding virt-launcher in PodInitializing, which would take a group down rather than explain it. Worker VMs are emitted by two charts. Both carry the field, and the golden-parity check gains a case that keeps their output identical. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
f4dc5d4d3c |
fix(harbor): retry the nested system install instead of uninstalling it
The harbor app chart renders a nested HelmRelease that owns the whole workload: the harbor Deployments, the inline RedisFailover and its volume. It carried Flux's default install strategy, and install remediation is an uninstall in every case Flux offers, so a first install that went over budget was torn down between attempts and the next one re-provisioned the redis volume and re-ran the master election from zero. That is reachable on a loaded runner. harbor-core blocks on redis while redis waits for its volume and its master election, and the nginx Deployment reaches the default progressDeadlineSeconds of 600s, which Flux reads as a stalled resource and fails the install early. The e2e wait budget for the release is the same 600s, so one teardown cycle consumes all of it and the second attempt cannot finish however fast it converges. RetryOnFailure keeps the manifests applied and retries the failed release, so the convergence already under way continues instead of restarting. Both actions carry the strategy because the controller performs the retry of a failed install as an upgrade, so on install alone the retry's own failure falls through to upgrade remediation. The cost is that a real upgrade failure retries rather than rolls back, which gives up no stable safety net: with retries: -1 and the default strategy, upgrade remediation already rolled back and retried without end. The remediation blocks stay because an absent release consults install and an out-of-sync one consults upgrade, both by retry count rather than by strategy. The change lands on the nested release rather than on the parent: the parent is built from the Application by the apiserver, which already stamps this strategy on both actions, while the nested release is the one that owns the workload and the volume a remediation deletes. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
605030b5a7 |
test(backupstrategy-controller): anchor the metrics-absence guard to its container (#3605)
## What this PR does The `metrics.enabled=false` case in `deployment_test.yaml` guards against the `metrics.enable` typo regression by asserting that the metrics arg and port are absent. Both assertions address the controller by index, and `notContains` passes whenever its path resolves to something that lacks the forbidden content. A different container at that index qualifies, so a sidecar placed ahead of the controller leaves the guard green while the flag it exists to forbid comes back on the real container. This adds an `equal` on `containers[0].name` ahead of the two negatives. It fails when the index stops pointing at the controller, which is the case the negatives structurally cannot see. Verified rather than argued. Rendering a sidecar ahead of the controller against the current tests leaves the case green and reddens only the two neighbouring tests, which use positive `contains` and defend themselves; the guard that exists for this scenario is not among them. With the anchor, the case fails on the anchor. This is not a new convention. `packages/system/opensearch-operator/tests/leader_election_test.yaml` and `packages/system/dashboard/tests/gatekeeper_test.yaml` already anchor index-addressed negatives the same way, the second with the reasoning spelled out in a comment. This brings one more package in line with what the tree already does. ## What this PR does not do It does not switch the assertions to a name filter. helm-unittest resolves `containers[?(@.name=="...")]`, but `notContains` against a filter that matches nothing passes just as quietly, so that trade swaps "wrong container" for "no container" inside the same family of silent green. It does not touch the other two cases in the file. Both are positive `contains`, both went red under the same injection, so they anchor themselves. ## Tests The change is itself a test fix. Package suite is green: 11 tests, 3 suites. ### Screenshots None. This changes a test file. ### Downstream repositories Walked the trigger map against the diff. One helm-unittest file, nine added lines, no chart output changes. - [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 ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Improved deployment validation by confirming the correct container is targeted when metrics are disabled. * Added checks that metrics-related arguments and ports are absent in this configuration. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
0ba567dd73 |
fix(tenant): bound the pre-delete cleanup waits and the hook Job (#3598)
## What this PR does The tenant pre-delete hook deletes the tenant's HelmReleases in two waves with `kubectl delete --wait=true` and no `--timeout`. kubectl reads `--timeout=0` as "wait forever" and substitutes a week, so a wave hangs for as long as any child HelmRelease cannot finalize. helm then returns from the uninstall without deleting anything: the release stays in storage, flux keeps the finalizer, and the same teardown gets retried. The delete policy carries no `hook-failed`, so the waiting Job survives between attempts too. Each wave now gets 90s. Neither is guarded, on purpose. Nothing stands behind these deletes, so a wave that did not finish ends the run instead of reporting a cleanup that never happened. That also keeps the order the two waves exist for, since the tenant modules are what the applications run on. Whatever got deleted stays deleted, so the next attempt resumes from there. The Job gets `activeDeadlineSeconds: 270`. It bounds what the waits do not, since the DELETE calls carry kubectl's default `--request-timeout` of 0, and it sits under the 300s uninstall budget flux applies when the HelmRelease has no timeout of its own. That is the budget for tenants cozystack-api generates; `tenant-root` is the exception, a static release in `cozystack-basics` carrying `timeout: 15m0s`, and the ordering holds there too since 280 is under 900. The 30s gap is not cosmetic. The deadline runs from the Job's `.status.startTime` while helm's budget runs from before the Job is created, so at equal values helm gives up first, on a Job that is still running. That is the state this change is meant to end, and it would look fixed, because a number would be there. `terminationGracePeriodSeconds: 10` is part of the same arithmetic. Since 1.31 the Job controller holds back the terminal condition until every pod has finished terminating, so what helm waits for is the deadline plus the termination. At the 30s default that lands on 270+30, level with the budget, which is the gap the deadline was set to buy. That period gets spent in full, every time. The SIGTERM goes to PID 1, which is the shell: the script runs several commands, so the shell never execs into kubectl and stays PID 1 itself. A signal carrying its default disposition is discarded by the kernel when the target is PID 1, and a non-interactive shell installs no SIGTERM handler, so nothing acts on it. kubectl is a child and was not signalled at all. The pod ends on SIGKILL. Nothing in that script needs to finish gracefully, the DELETE calls are already sent and the process is only watching. The pod also gets resource requests, matching the sibling hook in `packages/apps/kubernetes`. Without them it is BestEffort and first to go under node pressure, which mattered less when a retry inside the Job absorbed an eviction. With a single attempt it does not. `backoffLimit: 0` and `restartPolicy: Never` go together, and neither implies the other. At the default limit of 6 the Job does not reach `Failed` inside the budget, so helm falls back on its own timeout with the same nameless "timed out waiting for the condition" as before. Under `OnFailure` the pod survives its own failure and the kubelet re-runs the container in place, and the Job controller finds out afterwards by summing `restartCount`. The script starts over, re-enters the applications wave that just expired, and gets cut down mid-run when the controller catches up. `Never` puts a failed pod in front of the controller directly. ## What this PR does not do No remedy behind the bounded waits. The `packages/apps/kubernetes` hook force-clears child finalizers when its own wait expires. That needs `patch` on HelmReleases in the Role, and it is only safe there because the whole tenant control plane is going away in the same run. Here a wave that expires ends the attempt. `packages/apps/kubernetes/templates/delete.yaml` is untouched. It does not make a doomed teardown converge. A tenant whose children cannot finalize still gets its uninstall retried. What changes is that each attempt ends in bounded time, instead of eating the whole uninstall budget and leaving a pod behind. 90s is a guess, and it is tight. The first wave deletes every application HelmRelease in the namespace, including kinds that are slow to tear down: a nested Tenant runs this same hook one level down before its release finalizes, and an app of kind Kubernetes tears a control plane down. Either one can outrun 90s. The bound is what is left once the Job deadline sits under helm's budget instead of level with it, split between the two waves. The 90s the deadline keeps back for scheduling and the image pull is not calibrated either, and I would rather say so than imply it was. `docker.io/clastix/kubectl:v1.32` comes from Docker Hub with no mirror in front of it, and this repo has measured Docker Hub pulls of a smaller image at about 1m42s in CI. So that remainder is roughly the observed worst case rather than comfortably above it. A cold pull that slow lands on the `activeDeadlineSeconds` path, which is the one that deletes the pod and leaves nothing to read. There is a band that pays for this, and I would rather name it than round it off. Neither shape above fits inside 300s today, so for them the bound costs nothing that works. But every generated child HelmRelease carries a wait strategy, so a wave inherits its slowest child, and a wave that settles in something like 95s does complete inside the budget today. After this change it expires instead, and that tenant trades one clean attempt for an extra flux retry. In wall clock that retry is about five minutes, since `helmrelease-interval` defaults to `5m`, so a tenant in that band takes noticeably longer to delete than it does now. I think that is the right side of the trade: an unbounded wave does not fail, it spends the whole budget and leaves helm reporting a timeout that names nothing. If the band turns out to be populated, the waits and the deadline move together, and the bats suite pins their ordering rather than their values. One caveat on the diagnostics, since the comments in the chart used to promise more than they deliver. The failing wave leaves its output in the failed pod, which `ttlSecondsAfterFinished: 300` then reaps while flux's uninstall backoff is still climbing. And on the `activeDeadlineSeconds` path there is no log at all, because the Job controller deletes the active pods when the deadline expires. That path is the backstop for a kubectl that never returns; the expected path is the wave's own `--timeout`, where the script exits on its own and the pod survives to be read. ## Tests `hack/tenant-pre-delete-hook.bats` renders the chart, pulls the hook script out of the Job and runs it against a stub `kubectl` on `PATH`. It covers a clean run reporting success, every delete reaching kubectl with a non-zero bound, the ordering of the four numbers, and a wave that gave up ending the run without going on to the next one. `--timeout` is implemented inside kubectl, so no stub can make that deadline actually elapse. The tests read the arguments that reached kubectl and inject the non-zero exit an expired delete produces. No EXIT traps. The ordering test reads the numbers out of the rendered Job instead of pinning them, so moving a value keeps it honest while a value that breaks the ordering still fails. An absent deadline or termination period is rejected explicitly, otherwise `null` satisfies the numeric comparison and a Job with no bound at all passes. The Job's own shape is pinned in the chart's helm-unittest suite, since the script cannot observe that about itself. Both suites are green locally, and I mutated each pinned value to confirm they fail for the right reason: dropping either `--timeout`, dropping `activeDeadlineSeconds`, dropping `terminationGracePeriodSeconds`, dropping the requests, `backoffLimit` back to 6, `restartPolicy` back to `OnFailure`, and a `|| echo` appended to the applications wave. Each one goes red in the test that claims that ground. Worth flagging so the check marks are not read as more than they are: `make bats-unit-tests` is red at the merge base, unrelated to this branch, and the loop it runs exits on the first failing file. `hack/cozyreport.bats` sorts ahead of this suite, so on CI the new bats file is not reached at all right now. It is not failing, it is not running. The helm-unittest cases are unaffected and do execute. ### Screenshots None. This changes a Job manifest and its tests, with no user-visible surface. ### Downstream repositories Walked the trigger map against the diff. The diff is one Job manifest inside the tenant chart, its helm-unittest suite, and a new `hack/*.bats` file. No package is added, renamed or removed; no `values.schema.json`, `values.yaml` default or version enum changes; no `ApplicationDefinition` semantics, namespace, variant or release asset changes; nothing under `hack/` is moved or renamed and no make target changes behaviour, since `bats-unit-tests` already globs `hack/*.bats`. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note ```release-note fix(tenant): bound the pre-delete cleanup hook's HelmRelease deletions and give its Job a wall-clock deadline, so a tenant whose children cannot finalize no longer leaves the hook waiting indefinitely and blocking the uninstall ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Reliability Improvements** * Tenant cleanup jobs now enforce execution deadlines and terminate gracefully. * Cleanup operations have bounded deletion timeouts and no automatic retries. * Failed cleanup steps now stop subsequent operations and correctly report failure. * Cleanup jobs now specify CPU and memory resource requirements. * **Tests** * Added comprehensive coverage for cleanup behavior, timeouts, failure handling, and job lifecycle settings. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |