mirror of
https://github.com/cozystack/cozystack.git
synced 2026-11-08 21:14:56 +09:00
542ab899c2dff4df64eaf660293f70d901ec8607
3367
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 IvanHunters
|
542ab899c2 |
fix(kubernetes): break bootstrap-token Job out of Helm hook to unblock fresh installs
The bootstrap-token-tenant Job was wired as a Helm post-install/post-upgrade hook. post-* hooks fire only after Helm's wait phase resolves, and the tenant kubernetes release wait blocks on MachineDeployment Ready, which blocks on nodes registering, which needs the kubelet CSR path — for which the very bootstrap-token this Job produces has to already exist in the tenant kube-system. Fresh installs and any HR reconcile that finds MD in a non-Ready state deadlock; HR.upgradeFailures climbs while post-upgrade hooks never get their turn to run. pre-install/pre-upgrade would not help either: they fire before non-hook templates apply, so <release>-admin-kubeconfig (materialised by Kamaji after KamajiControlPlane is applied) is not yet available. Move the ServiceAccount and Job to the main install phase, matching what talos-reconcile already does in this chart. Helm applies them together with the KamajiControlPlane and talos-secrets; the Job Pod stays pending until Kamaji publishes the admin kubeconfig Secret (backoffLimit covers the retry window) and then completes before MachineDeployment can converge. Helm's wait treats the Job as another resource to reach terminal state and does not gate it on MD. Extend the existing template test to pin the absence of helm.sh/hook on both the ServiceAccount and the Job, so this cannot regress silently. Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
Aleksei Sviridkin
|
0b8837e552 |
fix(telemetry): make the documented opt-out cover both collectors (#3791)
## What this PR does Telemetry is reported by two components — `cozystack-operator` (cluster facts) and `cozystack-controller` (application counts) — but only the operator had a reachable switch. The documented opt-out sets `cozystackOperator.disableTelemetry` on the `cozy-installer` chart. That chart deploys the operator; it cannot reach a package the platform deploys. `packages/system/cozystack-controller` has its own independent `disableTelemetry` key that nothing propagated to, so after an operator had followed the published procedure the controller kept posting `cozy_application_count` every 15 minutes. Nothing failed, and nothing indicated the opt-out was partial. Two changes: - `packages/core/platform` gains a `telemetry.disabled` key and threads it into the `cozystack-controller` component of the `cozystack.cozystack-engine` Package — the same mechanism the bundle already uses for cilium, multus and linstor values. - `packages/core/installer/values.yaml` declares `cozystackOperator.disableTelemetry`. It was only ever referenced from the template, so the `--set` target the documentation names did not appear in the chart's own values. Both keys default to `false`. Nobody who has not opted out sees a behaviour change. The opt-out remains two steps, because the two collectors are installed by two different charts. Collapsing it into one knob would mean the operator writing the value into the platform Package it does not own, which is a design change rather than a fix. `helm unittest` on the platform chart: 137 passed (133 before, plus the four added here). `helm lint` clean on both charts. ### Screenshots Not a UI change. ### Downstream repositories Trigger map walked against the diff. `packages/core/platform/values.yaml` changed, which the map routes to the website's hand-written platform-package table; the same change also makes the published opt-out procedure wrong, so the docs follow-up covers both. - [ ] No downstream repository is affected by this change - [x] [cozystack/website](https://github.com/cozystack/website) - follow-up: https://github.com/cozystack/website/pull/651 - [ ] [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(telemetry): the documented telemetry opt-out now covers both collectors. Setting `cozystackOperator.disableTelemetry=true` alone left `cozystack-controller` reporting application counts; the platform gains a `telemetry.disabled` key that silences it. Both default to false. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added a platform telemetry opt-out setting, disabled by default. * Added a separate operator telemetry opt-out setting, also disabled by default. * Applying the platform setting now disables telemetry reporting for the relevant controller. * **Bug Fixes** * Ensured telemetry settings are correctly applied across supported platform variants while preserving existing configuration overrides. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
4b50a3ceb4 |
fix(ingress-nginx): pull the protobuf-exporter from the project namespace (#3783)
## What this PR does
Points the `protobuf-exporter` sidecar at the project's registry.
The sidecar was still pulled from `ghcr.io/kvaps/...` while the
controller beside it — built from the same repository, pinned to the
same `v1.11.2` — already came from `ghcr.io/cozystack/...`. The
repository moved to the organisation some time ago and the image
reference did not follow. It is the last thing in the tree that ships a
Cozystack-owned component out of an individual's namespace, which is a
Required item on the Incubation checklist ("All project metadata and
resources are vendor-neutral").
The image is the same bytes: `v1.11.2` was copied into the organisation
namespace with the manifest digest preserved, so `sha256:6d9235a9…` is
unchanged here. Nothing is rebuilt and nothing is upgraded.
The version is deliberately not bumped. `v1.11.5` exists in the
organisation namespace, but the tag tracks the upstream ingress-nginx
version and the controller sits on `v1.11.2`; bumping only the sidecar
would split the pair.
The comment in `hack/promote-retag.sh` loses its `ghcr.io/kvaps/...`
entry, because that path now falls under the bullet below it — a
cozystack-org repository outside `$REGISTRY`
(`ghcr.io/cozystack/cozystack`). Selector behaviour is unchanged;
`hack/promote-retag_test.bats` passes, 11/11.
### Downstream repositories
Walked the trigger map in `docs/agents/contributing.md` against the
diff. The two files touched are a package `values.yaml` image reference
and a comment in `hack/promote-retag.sh` — no trigger matches. In
particular the `ccp` trigger is a move, a rename, or a change to what a
make target does; this is neither, and its anchors (`hack/package.mk`,
`hack/common-envs.mk`) are untouched.
- [x] No downstream repository is affected by this change
### Release note
```release-note
fix(ingress-nginx): the protobuf-exporter sidecar is now pulled from ghcr.io/cozystack instead of a personal namespace. Same image and digest; no version change.
```
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **Chores**
* Updated the protobuf exporter image source to the new container
registry location.
* Refreshed image promotion guidance to remove an outdated third-party
registry example.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
|
||
|
Aleksei Sviridkin
|
3710541278 |
fix(kubernetes): validate MachineHealthCheck condition timeouts at render time
The CAPI webhook validates nodeStartupTimeout with a 30s floor but does
not validate unhealthyConditions timeouts at all, so a zero or negative
readyUnknownTimeout or readyFalseTimeout applied and remediated a
Machine the moment its Ready condition flipped. Route both fields
through the same render-time duration guard as maxNodeProvisionTime,
extracted into a kubernetes.positiveDuration helper in each chart.
The guard now also accepts the canonical form a duration serializes
back to ("20m0s", "1h0m0s"): metav1.Duration prints that shape, so a
value copied from kubectl output previously failed the render. The
first segment must still be positive and later segments may be zero,
which keeps every accepted string a positive duration.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <3811295@gmail.com>
|
||
|
Aleksei Sviridkin
|
b32ca5cbb3 |
chore(kubernetes): drop the force-delete flag that blocks partial cleanup (#3899)
## What this PR does Removes `--force-delete-unregistered-nodes=true` from the tenant cluster-autoscaler Deployment. On the clusterapi provider the flag cannot do what its name promises: the provider's `ForceDeleteNodes` returns `cloudprovider.ErrNotImplemented`, the autoscaler falls back to the ordinary `DeleteNodes` path, and that path enforces the node group's minimum size. The one behavior the flag does change is harmful here: it skips the step that trims a long-unregistered delete batch down to the group's headroom above min size, so a batch larger than the headroom is refused whole where the default would trim it and delete what fits. Context: #3513. A regression test renders the Deployment, proves the args list is live with a positive anchor on `--cloud-provider=clusterapi`, and asserts the flag is absent, so reintroducing it fails the suite. ### Downstream repositories - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note ```release-note chore(kubernetes): drop the cluster-autoscaler force-delete-unregistered-nodes flag; the clusterapi provider does not implement force deletion, and the flag only made batch removal of long-unregistered machines all-or-nothing ``` |
||
|
Aleksei Sviridkin
|
245bca3366 |
chore(kubernetes): drop the force-delete flag that blocks partial cleanup
`--force-delete-unregistered-nodes` reads as an override that lets the autoscaler remove a long-unregistered machine below its node group's min size. On the clusterapi provider it grants nothing of the sort: force deletion is not implemented there, the call falls back to the ordinary delete path, and that path refuses to take a group below its min size. The flag never bought the safety valve its name suggests. What it does buy is a behaviour nobody asked for. With the flag set the autoscaler skips trimming the delete batch down to the group's headroom above min size, and the ordinary delete path then refuses a batch larger than that headroom outright instead of removing what it can. A group of three machines above a min size of one loses two long-unregistered machines with the flag unset, and none with it set. Removing it restores the documented default and takes away a signal that advertised a protection the provider does not offer. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <3811295@gmail.com> |
||
|
Aleksei Sviridkin
|
ddb15ebc6a |
fix(kubernetes): widen the worker node startup budget to 20m
nodeStartupTimeout is the window a Machine has to produce a Node at all, measured from the later of the Machine's creation and its InfrastructureReady transition. What has to fit inside it is the guest boot and the kubelet registering with the tenant apiserver, and on a host under load neither is quick. At 10m a worker that was still booting was deleted before it ever joined, and its replacement started the same race from the beginning. Also record what a percentage maxUnhealthy does at small group sizes, because the interaction is not what the field name suggests: the value is scaled against the machines the check covers and rounded down, and remediation proceeds only while the unhealthy count is at or below the result. At the default "50%" a group of one never remediates, and a group of two remediates one unhealthy machine but not two. The golden-parity gate pinned the child chart's MachineHealthCheck values to the parent's defaults as inputs, which made a default moved on both sides fail there for the wrong reason. Drop the pins so the comparison is between the two charts' defaults, which is the contract it exists to hold, and pin the defaults themselves in each chart's own unit tests instead. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <3811295@gmail.com> |
||
|
Aleksei Sviridkin
|
33701800d6 |
fix(kubernetes): make the worker MachineHealthCheck timeouts configurable
The two unhealthyConditions timeouts were fixed in the template at 30s for `Ready=Unknown` and 300s for `Ready=False`, and both are too tight for a worker joining on a loaded host. `Ready=Unknown` is what the node lifecycle controller writes when a kubelet stops reporting, on a grace period of its own, so 30s on top of that deleted the VM of a guest that had merely stopped being scheduled for a stretch, before it had a chance to answer again. And `Ready=False` is the condition a registered Node holds until its CNI is up, so 300s was the entire budget a first boot got for the CNI rollout to reach it -- a worker deleted mid-rollout takes the rollout with it and its replacement starts over. Move both to values, defaulting to 5m and 10m. The remediation of a worker that is genuinely dead is correspondingly later, which costs less than it appears to: Pods leave an unreachable Node on the node lifecycle controller's taint-based eviction and the toleration that carries, not on either of these timers. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <3811295@gmail.com> |
||
|
Aleksei Sviridkin
|
83f0eef645 |
fix(kubernetes): give the autoscaler the real worker join time
The cluster-autoscaler treats a machine as long-unregistered once its own max-node-provision-time elapses, and a worker that has not registered by then stops counting towards its node group's upcoming capacity. On a host slow enough that the Talos image import, the guest boot and the CNI rollout do not fit inside that window, the timer fires on machines that were still joining: the group reads as short of capacity, so the autoscaler asks for replacements, and the extra provisioning load makes the join it was waiting for slower still. Render the per-node-group override the autoscaler reads off the MachineDeployment, sourced from a new maxNodeProvisionTime value that defaults to 30m. The duration is checked at render time because the autoscaler discards one it cannot parse and falls back to its own default without failing, and because the values schema types the field as a string and so accepts every malformed duration. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <3811295@gmail.com> |
||
|
Aleksei Sviridkin
|
b76cb2091c |
test(e2e): record the runner VM shape and region in the e2e report
An e2e lane asks for a runner class, not for a machine. A flex shape is filled from whatever capacity the pool has at the time, so one lane can be served by two processor generations, and the region is a preference with a fallback. None of that reached the report, so a red run and a green one could not be compared by the hardware they ran on. The read is taken on the runner VM, before the sandbox container is created, because the metadata service answers on a link-local address the container has no guaranteed route to: an "unreachable" answered from in there would look exactly like a capture that had tried. It is wired into the sandbox apply target rather than into a workflow step, so every lane picks it up through `make prepare-env` on both the passing and the failing path, and cozyreport folds the file into the artifact from inside without going near the endpoint. The keys are fetched one at a time and the root instance document never is. That document carries the instance and compartment OCIDs, the image OCID, the display name, the hostname and the user metadata bag, and the report is a public artifact, so asking per key makes that property something the wire settles rather than something a filter has to be trusted for. It also matches where the shape sizing is published: it has its own subpath and is not in the root document at all. From the sizing object only the numbers are kept, so the rule holds for keys nobody has seen yet, and an answer that is not one scalar is named rather than printed. Every read carries a wall-clock ceiling, since a read that never returns costs the step its whole budget without reaching any exit status, and the first key doubles as the probe so a machine that is not an OCI instance pays one ceiling rather than one per key. Nothing here is fatal: this runs inside the one retried step in the job, so a runner with no metadata service leaves an "imds unreachable" line and an exit status of 0. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <3811295@gmail.com> |
||
|
myasnikovdaniil
|
420c316e5c |
refactor(dashboard): validate the schema form before page-level checks (#3116)
## What this PR does Re-applies cozystack/cozystack-ui#26 (by @lexfrei) onto the in-tree console, after the UI was vendored into the monorepo (#2963) and the standalone repo is being archived. Part of the cozystack-ui retirement tracked in #3097. In the four backup create pages (Backup, BackupJob, BackupPlan, BackupRestoreJob), the RJSF `SchemaForm.validate()` call now runs right after the name check and before the page-level required-field alerts. The submit button lives outside RJSF and bypasses its validation, so each page triggers `validate()` explicitly; it previously ran only after the manual alert checks, so a schema-required field left empty exited via an alert without RJSF ever rendering its inline errors. Validating first makes inline errors show consistently; the page-level alerts then cover only page-only requirements (application/backup reference, strategy, timestamp). Scope is the validation-ordering change only — the original PR's VMDisk/StorageClass parts were obsoleted by the `DynamicOptionsWidget` refactor (#30) already present in the vendored snapshot. ## Verification - Includes the original per-page regression tests. - `pnpm typecheck` clean; full console suite green (317 passed). Original PR: cozystack/cozystack-ui#26 — credited via `Co-authored-by`. ```release-note NONE ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved submission validation across backup creation, backup job, backup plan, and backup restore job pages. * Inline required-field errors now appear before page-level alerts. * Invalid submissions are prevented from sending requests or triggering unnecessary alerts. * **Tests** * Expanded coverage for validation order and submission blocking. * Improved form test scenarios for invalid and missing data. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
3d076099a2 |
docs(postgres): name the trust anchor as the tenant actually sees it
The README told a tenant to fetch <release>.tenant-ca, but the release name carries the postgres- prefix the application definition applies, so the object is postgres-<name>.tenant-ca. Following the old text returned NotFound. Also drop two comments promising a companion ValidatingAdmissionPolicy that pins sentinel writes to helm-controller. No such policy ships: the proposal for it was rejected because authorization by username duplicates RBAC in a more brittle form. The RBAC boundary is the whole control. Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
3cb131785c |
fix(kubernetes): settle what a pod CPU quantity of zero or less means
Both podCpuLimit and podCpuRequest were rendered under `with`, which skips a zero as readily as an absent field, and their guards were keyed the same way. The generated schema behind them admits a leading minus and a zero as readily as a sane value, so rather than patch whichever edge turned up, the whole value domain is settled in one place. Presence, not truth, is the predicate everywhere either field is read. A negative value is refused: it never reaches a running Pod. A zero request on its own is allowed and reserves nothing. A zero request beside a non-zero limit is refused: KubeVirt's defaulter tests the request with IsZero, which an explicit zero satisfies exactly as an absent key does, so it overwrites the zero with the limit and the worker asks the scheduler for its whole ceiling. That is the outcome podCpuRequest exists to prevent, reached silently and reported nowhere, which is why it is refused rather than written down. A zero limit removes the ceiling rather than being the tightest one -- the kubelet turns a zero CPU limit into no CFS quota -- but it reaches a Pod only paired with a zero request. KubeVirt keeps deriving a request from the vCPU count when the VM declares none, the defaulter that would have copied the limit across does not fire because the limit is itself zero, and API server validation refuses a request above a limit that exists. Zero is a limit that exists. So a lone zero limit is refused too. Both fields document the same rules and each class has a test on both charts. A group that sets neither field renders the bytes it rendered before, so the content-hash name of the worker VM template does not move. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <3811295@gmail.com> |
||
|
Aleksei Sviridkin
|
a6770d0f1b |
fix(cozy-lib): convert micro and nano quantities instead of zero
cozy-lib.resources.toFloat knew every suffix of the Kubernetes quantity grammar except `u` and `n`. A suffix it does not recognise is not an error: the unit letter stays on the string, `float64` cannot parse the result and yields zero silently, and the caller divides or compares that zero as if it were the value. The node group CPU request guard is one such caller. A request of 100u above a limit of 50u compared zero against zero and rendered, so the pair reached API server validation on the worker's Pod instead of failing where the values were written. Pinned by a suite over the whole suffix grammar, one value per form, because the failure has no error to notice it by. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <3811295@gmail.com> |
||
|
Aleksei Sviridkin
|
1b8340298e |
feat(kubernetes): let a node group set the CPU request of its worker VMs
Adds nodeGroups.<name>.podCpuRequest, rendering into domain.resources.requests.cpu on the worker VM, and the same field on the kubernetes-nodes chart, which is the other half of a byte-identity contract rather than a separate feature surface. podCpuLimit on its own cannot raise the ceiling of a worker without raising what that worker asks the scheduler for, because KubeVirt copies a declared CPU limit into the request when the VM declares none. The request is the half that has to hold still, so the pair can now be moved apart. Both fields conflict with instancetype sizing for the same reason and in the same upstream check, so the request gets the guard the limit already had. A request above its own limit fails the render too, rather than reaching API server validation on a Pod two objects away from the CR that named the values; the quantities are compared as numbers, so 500m under a ceiling of 1 is accepted where a lexical guard would refuse it. A node group that sets neither field renders the bytes it rendered before, which the content-hash name of the KubevirtMachineTemplate depends on, and the render-parity gate gains a case that compares the pair across the two charts. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <3811295@gmail.com> |
||
|
Aleksei Sviridkin
|
669c3e60ab |
docs(kubernetes): correct what podCpuLimit does to the CPU request
The field was documented as moving the ceiling alone and leaving the CPU request at the value KubeVirt derives from the guest vCPU count and the cluster CPU allocation ratio. KubeVirt does the opposite: its VMI defaulter copies a declared CPU limit into the CPU request whenever the VM declares no request of its own, so a node group that sets only the limit asks the scheduler for its whole ceiling, and a ceiling the nodes cannot spare leaves the workers Pending on Insufficient cpu. The same paragraph promised a floor that does not exist either. A limit below the derived request was said to leave the container with a request above its limit; the copy makes the two equal instead, whatever the limit is. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <3811295@gmail.com> |
||
|
Aleksei Sviridkin
|
78b9bc3e6e |
feat(kubernetes-nodes): mirror the worker pod CPU limit into the pool chart
The split-out pool chart renders the same worker VM as the monolithic one, and tests/render-parity.sh exists to prove it renders it byte-identically, because the KubevirtMachineTemplate is content-hash named and any divergence rolls every live worker VM. A field that only one of the two charts knows breaks that quietly rather than loudly: a pool adopted into a kubernetes-nodes release would render without the CPU limit it was given, the hash would change, and the ceiling would fall back to the vCPU count with nothing reporting it. So the field, its render-time guard against combining it with an instancetype, and its tests arrive here in the shape the parent chart has them, and the parity gate gains a case that compares the two renders with the limit set. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <3811295@gmail.com> |
||
|
Aleksei Sviridkin
|
a666b9b3bd |
feat(kubernetes): let a node group set the CPU limit of its worker VMs
The chart had no way to give a worker VM a CPU ceiling different from its vCPU count. `resources.cpu` sizes the guest, and the pod side is filled in by KubeVirt: with a ResourceQuota limiting CPU in the namespace, which every tenant carrying resourceQuotas has, it derives limits.cpu from the guest vCPU count. The QEMU emulator thread and the IO threads then come out of the same allowance as the vCPU threads, which is tight for a nested guest that pays emulation overhead. Declaring the limit on the VM is what stops KubeVirt deriving one, and it is the only way out that does not make things worse: dropping cpu from the tenant resourceQuotas would let the LimitRange apply its 250m default instead, four times tighter than what it replaced. The limit is usable only on a group sized by resources, because KubeVirt's instancetype applier reports domain.resources.limits.cpu as a conflict rather than resolving it, and a VM carrying both is refused by the admission webhook with nothing on the CR to say why. Sizing by resources is what drops the instancetype from the VM, so the chart fails the render for a group that kept its instancetype, beside the existing guard for the same conflict class one field over. The field renders no CPU request, so the request stays derived from the vCPU count and the cluster CPU allocation ratio, and the scheduler weight of a worker does not move with its ceiling. It renders nothing at all when unset, byte for byte: the enclosing KubevirtMachineTemplate is named by a hash of its content, so anything else there would roll every live worker VM on upgrade. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <3811295@gmail.com> |
||
|
Myasnikov DaniilandAleksei Sviridkin
|
6181493d4f |
refactor(dashboard): validate the schema form before page-level checks
In the four backup create pages (Backup, BackupJob, BackupPlan, BackupRestoreJob), run the RJSF SchemaForm.validate() call right after the name check and before the page-level required-field alerts. The submit button lives outside RJSF and bypasses its validation, so each page triggers validate() explicitly; it previously ran only after the manual alert checks, so a schema-required field left empty exited via an alert without RJSF ever rendering its inline errors. Validating first makes inline errors show consistently; the page-level alerts then cover only page-only requirements. Re-applied from cozystack/cozystack-ui#26 onto the in-tree console after the UI was vendored (#2963); the source repo is being archived. Scope is the validation-ordering change only — the original PR's VMDisk/StorageClass parts were obsoleted by the DynamicOptionsWidget refactor (#30) already in the vendored snapshot. Includes the original per-page tests. Co-authored-by: Aleksei Sviridkin <1366514+lexfrei@users.noreply.github.com> Assisted-By: Claude <noreply@anthropic.com> 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 --> |
||
|
Andrei Kvapil
|
c3cead8f64 |
fix(telemetry): make the documented opt-out cover both collectors
Telemetry is reported by two components, cozystack-operator (cluster facts) and cozystack-controller (application counts), but only the operator had a reachable switch. The documented opt-out sets cozystackOperator.disableTelemetry on the cozy-installer chart, which cannot reach a package the platform deploys, so cozystack-controller kept reporting after an operator had followed the procedure — silently, with nothing to indicate the opt-out was partial. The platform now exposes telemetry.disabled and threads it into the cozystack-controller component of the cozystack-engine Package, the same way the bundle already threads cilium, multus and linstor values. The installer key is also declared in values.yaml: it was only ever referenced from the template, so the documented --set target did not appear in the chart's own values. Both keys stay false by default; this changes no behaviour for anyone who has not opted out. Assisted-By: Claude Signed-off-by: Andrei Kvapil <kvapss@gmail.com> |
||
|
Andrei Kvapil
|
1fc6dd4234 |
fix(ingress-nginx): pull the protobuf-exporter from the project namespace
The sidecar was still referenced from an individual maintainer's namespace while the controller beside it, built from the same repository and pinned to the same version, already came from the project's. The repository moved to the cozystack organisation some time ago; the image reference did not follow. Same image: the digest is unchanged, and v1.11.2 is kept so the sidecar stays aligned with the controller, whose tag tracks the upstream ingress-nginx version. Assisted-By: Claude Signed-off-by: Andrei Kvapil <kvapss@gmail.com> |
||
|
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 --> |