mirror of
https://github.com/cozystack/cozystack.git
synced 2026-11-03 10:37:21 +09:00
ci/shellcheck-gate
305
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Timofei Larkin
|
95e4faf68e |
feat(workloadmonitor): bucket metrics endpoint override
The controller discovers where to query SeaweedFS bucket size metrics via the namespace.cozystack.io/monitoring label, which assumes the monitoring stack that scrapes SeaweedFS runs in the same cluster. Some deployments host SeaweedFS and its metrics in a separate cluster, so add a --seaweedfs-metrics-endpoint flag to cozystack-controller (and a matching cozystackController.seaweedfsMetricsEndpoint chart value): an explicit base URL of a Prometheus-compatible query API, with scheme, host, optional port and path prefix; /api/v1/query is appended. Basic auth may be embedded as URL userinfo. The flag is validated and normalized at startup, so a malformed URL fails the rollout rather than the queries. Empty keeps label-based discovery unchanged. Bucket sizes feed billing, so metrics failures no longer collapse to "no size": previously any query failure yielded an empty result map and the reconciler overwrote Workload resources with it, silently wiping the recorded sizes. The reconciler now retains the last known s3-storage-bytes and s3-physical-storage-bytes values, emits a BucketMetricsUnavailable warning event on the WorkloadMonitor, and returns the error from Reconcile so it lands in the reconcile error metrics. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Timofei Larkin <lllamnyp@gmail.com> |
||
|
Aleksei Sviridkin
|
6ecb36ffc7 |
feat(cozystack-controller): extract application CA into a key-free tenant Secret (#3407)
## What this PR does Adds an engine-agnostic controller that publishes a key-free `<release>.tenant-ca` Secret (only `ca.crt`) for a managed application, so a tenant can verify the application's TLS without ever seeing a private key. The source is declared, not guessed: a chart renders a namespaced `TenantProjection` sentinel (group `internal.cozystack.io`) naming the CA Secret to lift; the controller watches the sentinel, extracts `ca.crt`, and writes the projection owner-referenced to the sentinel, so garbage collection is native. The projection carries the `internal.cozystack.io/tenant-ca` label the lineage webhook turns into tenant visibility. Postgres is the first consumer. The trust boundary is RBAC: no tenant role grants any verb on `internal.cozystack.io`, so a tenant cannot forge a sentinel. A companion `ValidatingAdmissionPolicy` that pins the writer to helm-controller ships separately as defence in depth. This is the controller-and-CRD slice, split out from #3299 for focused review. It is the only piece required for the feature to work; the render-time helper, the writer policy, the capability-gate, and the release-prefix validation land as their own PRs. ### Downstream repositories Walked the trigger map against the diff. This adds an internal CRD and controller and a sentinel to the Postgres chart; no app is added or renamed and no `values.schema.json` changes, so the typed provider and Ansible are unaffected. The one to confirm is the Postgres README edit — if the website mirrors it, it needs a refresh. - [ ] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: ### Release note ```release-note feat(cozystack-controller): publish a key-free per-application CA trust anchor (`<release>.tenant-ca`) that tenants read to verify managed-application TLS, declared through a new `TenantProjection` CRD. Postgres is the first consumer. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added tenant-facing CA certificate projections for PostgreSQL deployments. * Tenant secrets now provide only the CA certificate, excluding private keys. * Added support for configuring and monitoring tenant CA projections. * **Documentation** * Updated PostgreSQL TLS instructions to retrieve certificates through tenant secrets. * **Bug Fixes** * Improved protection against invalid certificates, secret collisions, and accidental exposure of private key material. * Added validation to ensure projected certificates remain synchronized with their sources. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Andrey Kolkov
|
8aeedde088 |
feat(backups): support PostgreSQL point-in-time recovery (PITR) (#3383)
## What this PR does Makes PostgreSQL point-in-time recovery (PITR) a genuinely supported, tested capability for the new `backups.cozystack.io` API. Fixes #2774. Was **stacked on #3313** (`fix/3300-cnpg-barman-plugin`) for the `recoveryTime` → CNPG `bootstrap.recovery.recoveryTarget.targetTime` plumbing; #3313 has since merged and this branch is rebased on `main`. This PR adds the reliability + verification layer #2774 asked for. ### Changes - **Precise failure reason for an unreachable target.** The restore deadline (`spec.options.restoreTimeoutSeconds`, default 30m) stays the sole authority for "this restore is stuck" — the client waits the full window either way. What changes is the *diagnosis*: once the deadline has elapsed, if `recoveryTime` was set and the recovery Cluster never went healthy, the CNPG driver reads the `full-recovery` pod logs and — when several distinct attempts exited with PostgreSQL's `FATAL: recovery ended before configured recovery target was reached` — fails the RestoreJob with `RecoveryConverged=False` / `Ready=False` (reason `RecoveryTargetUnreachable`) and an actionable message, instead of a bare generic timeout. Timing is unchanged; only the reason/message differ. (For a faster rejection, set a shorter `restoreTimeoutSeconds`.) Design points: - **We deliberately do NOT trip early off the FATAL.** A valid near-now target emits that same FATAL transiently — repeatedly, for many attempts, on a slow archiver — before it converges, so any earlier trip would wrongly reject a recoverable restore (observed twice in CI). The FATAL is used only to *explain* a failure the deadline has already declared. - **Health wins unconditionally** — the deadline/classification check precedes the healthy check, but a recovery that converges within the window reaches success and is never classified as failed (a live dev7 restore did exactly this). - **Counting distinct FATAL-ing attempts** (not pod failures) tolerates a one-off archive-lag miss on a near-now target and ignores transient recovery-pod crashes (a node blip / brief API-server unreachability never emits that FATAL). - Adds the `pods/log` RBAC grant the log read needs (distinct subresource from `pods`); a `Forbidden` is logged loudly and surfaced in the failure message rather than silently disabling the classification. - **e2e** (`examples/backups/postgres/run-all.sh`, wired into the Chainsaw `postgres-2-backup-roundtrip` suite): - Step 45 — write `before`, capture server timestamp (µs precision), write `after`, restore to that timestamp, assert `before` present and `after` absent. - Step 46 — a `recoveryTime` an hour ahead must fail with reason `RecoveryTargetUnreachable` once the deadline elapses (asserts the *reason*, so it proves the classification fired, not just that a timeout occurred). - Chainsaw budget raised 25m → 40m to cover three sequential restores + the post-deadline classification. - **Docs + example**: uncommented `45-restorejob-pitr.yaml`; a `docs/operations/backup-classes.md` PITR section (recoverable window, discovering earliest/latest restorable time from the backup catalog since `firstRecoverabilityPoint` is unreliable under the plugin, archive-lag tolerance, idempotency under GitOps). ### Verification - `go build` / `go vet` clean; unit tests pass (`logIndicatesRecoveryTargetUnreachable`, `recoveryUnreachableFromLogs`, `recoveryPodsToInspect`, nil-Clientset guard, a reconcile-level success-path test asserting `Succeeded` + `RecoveryConverged=True`, and — added per review — `TestReconcileCNPGRestore_DeadlineClassifiesRecoveryTargetUnreachable`, a reconcile-level test driving the deadline branch through all three sub-paths: FATAL found → reason `RecoveryTargetUnreachable`, FATAL absent → generic timeout, `Forbidden` log read → generic timeout + missing-`pods/log`-RBAC note. It fails if the call-site reason is reverted to the generic one). - Reviewed via adversarial branch-review; fixes include a critical missing-RBAC dead-on-arrival, a classification false-positive on transient recovery-pod crashes, and a sub-second e2e flake. - **dev7**: backup + WAL archiving + recovery convergence verified live on the #3313 build. The load-bearing assumption of the classification — CNPG **accumulates** failed `full-recovery` pods rather than reusing one — was confirmed (observed 7 simultaneous failed recovery pods). A fully-green live step-46 run was blocked by intermittent COSI bucket-provisioning flakiness on dev7; the negative path is covered by the CI Chainsaw e2e. ### Downstream repositories Walked the trigger map in `docs/agents/contributing.md` against the diff. The only user-facing surface is the new `docs/operations/backup-classes.md` PITR section; the website keeps a hand-mirrored copy per docs version. - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: PITR docs mirror warranted (not yet opened; same pending mirror as #3313's backup-classes changes) - [x] No other downstream repository is affected by this change ```release-note feat(backups): PostgreSQL RestoreJob now supports point-in-time recovery (PITR). Set spec.options.recoveryTime (RFC3339) to restore to an exact instant. A recoveryTime past the latest archived WAL still runs to the restore deadline (spec.options.restoreTimeoutSeconds, default 30m), but the failure is now reported with the precise reason RecoveryTargetUnreachable and an actionable message instead of a generic timeout; set a shorter restoreTimeoutSeconds for a faster rejection. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added PostgreSQL point-in-time recovery (PITR) support via RFC3339 `recoveryTime`. * Introduced a PITR RestoreJob example, plus helper variables and an unreachable-target scenario. * **Bug Fixes** * Improved restore deadline handling with deterministic, log-based fail-fast classification for unreachable recovery targets, including clearer conditions. * **Documentation** * Added detailed PITR operational documentation (recovery windows, failure behavior, and GitOps/idempotent behavior). * Expanded README and example comments for PITR verification and failure expectations. * **Tests** * Extended unit tests and end-to-end chainsaw workflow to cover PITR success and unreachable failures. * **Chores** * Updated RBAC to allow pod log reads; refined cleanup and run scripts for additional RestoreJob artifacts. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Andrey KolkovandClaude Opus 4.8
|
b574e2176f |
fix(backups): cover Go-driven system-bucket ObjectStore + share the checksum helper
Address review on the S3 request-checksum fix: - Extend the fix to the platform-managed useSystemBucket=true flow, whose ObjectStore is built in Go by applyClusterPluginBackup (SSA-applied), not chart-rendered. Add InstanceSidecarConfiguration to cnpgtypes.ObjectStoreSpec and set AWS_REQUEST_CHECKSUM_CALCULATION=when_required on it via barmanSidecarConfiguration(). The platform's own default system bucket is SeaweedFS, a non-AWS S3 gateway of the same class as Ceph RGW. Unit test in cnpgstrategy_controller_test.go asserts the applied ObjectStore carries it. - Factor the duplicated instanceSidecarConfiguration block (3x across two charts) into a shared cozy-lib helper (cozy-lib.barman.checksumSidecarConfiguration). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Andrey Kolkov <andrey.kolkov@aenix.io> |
||
|
Andrey Kolkov
|
ad3f8b2487 |
fix(backups): check cluster health before the restore deadline (PITR)
Addresses review CHANGES_REQUESTED on #3383. CRITICAL: reconcileCNPGRestore checked the restore deadline BEFORE the cluster-health check, so a reconcile that first observes "deadline elapsed" AFTER the recovery Cluster already reached a healthy state (a controller restart or stalled workqueue spanning restoreTimeoutSeconds) would mark an already-converged restore Failed / RecoveryTargetUnreachable. A false Failed can then escalate to data loss: a resubmitted RestoreJob's purge guard treats the recovered Cluster as a stale leftover and deletes it + its PVCs. Fix: check hasRecovery + cnpgClusterHealthy first and return Succeeded unconditionally when healthy, regardless of how late the reconcile runs; only a not-yet-healthy restore reaches the deadline/classification branch. Adds TestReconcileCNPGRestore_HealthyPastDeadlineSucceeds (StartedAt 72h past the deadline + healthy Cluster + recoveryTime set; the injected readPodLog fails the test if the classification path is entered, proving health short-circuits). MINOR: handle the deferred Close error on the pod-log reader (errcheck). Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey KolkovandClaude Opus 4.8
|
b5d03f889b |
test(backups): reconcile-level test for the PITR deadline classification
Review of #3383 found the RecoveryTargetUnreachable classification branch in reconcileCNPGRestore was only covered through the pure helpers and recoveryTargetUnreachable directly, never through reconcileCNPGRestore itself — a mutation swapping the call-site reason back to the generic RestoreFailed kept the suite green. Add TestReconcileCNPGRestore_DeadlineClassifiesRecoveryTargetUnreachable: it backdates StartedAt past the restore deadline with a recoveryTime set and drives the real reconcile through the readPodLog seam, asserting all three deadline sub-paths — FATAL present (reason RecoveryTargetUnreachable on both RecoveryConverged and Ready), FATAL absent (generic timeout, no RecoveryTargetUnreachable), and a Forbidden log read (generic timeout + the missing-pods/log-RBAC note). Verified it fails when the call-site reason is reverted to the generic RestoreFailed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Andrey Kolkov <andrey.kolkov@aenix.io> |
||
|
Andrey KolkovandClaude Opus 4.8
|
6a1e2d0d06 |
feat(backups): support PostgreSQL point-in-time recovery (PITR)
Makes RestoreJob.spec.options.recoveryTime a genuinely supported, tested capability for the new backups.cozystack.io API (issue #2774). The recoveryTime -> CNPG bootstrap.recovery.recoveryTarget.targetTime plumbing already existed on the barman-cloud-plugin branch; this adds the reliability and verification layer the issue asked for: - Fail-fast for an unreachable target: when a RestoreJob has recoveryTime set and the recovery Cluster is not healthy, the driver reads the CNPG full-recovery pod logs and, once several distinct attempts have exited with PostgreSQL's "recovery ended before configured recovery target was reached" FATAL, fails the RestoreJob with conditions RecoveryConverged=False / Ready=False (reason RecoveryTargetUnreachable) instead of hanging to the restore deadline. Health is checked first, so a converged recovery always wins; counting distinct FATAL-ing attempts (not pod failures) tolerates a transient archive-lag miss on a near-now target and ignores transient recovery-pod crashes. Adds the pods/log RBAC grant the log read requires. - e2e (examples/backups/postgres/run-all.sh, wired into the Chainsaw postgres-2-backup-roundtrip suite): step 45 writes a before marker, captures the server timestamp, writes an after marker, restores to that timestamp and asserts the before row survived and the after row did not; step 46 submits a recoveryTime an hour ahead and asserts it fails fast with reason RecoveryTargetUnreachable. - Working, uncommented example (45-restorejob-pitr.yaml) and a docs section (docs/operations/backup-classes.md) covering the recoverable window, how to discover the earliest/latest restorable time from the backup catalog, the archive-lag tolerance, and idempotency under GitOps. Refs #2774 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Aleksei Sviridkin
|
37a91f98b8 |
feat(cozystack-controller): extract application CA into a key-free tenant Secret
Every catalog engine issues a per-release CA, but the object holding ca.crt usually holds a private key beside it (cert-manager's tls.key, CloudNativePG's ca.key), so granting a tenant read on it hands over key material too. Add a controller that publishes the trust anchor as one canonical, key-free Secret per release, "<release>.tenant-ca", holding only ca.crt. A chart declares the anchor by rendering a namespaced TenantProjection sentinel (internal.cozystack.io/v1alpha1) that names the source Secret and key. The controller rebuilds ca.crt from the parsed certificate DER rather than copying the source, refuses any value carrying private key material, and owns the projection by owner reference so a foreign Secret at the same name is never overwritten. No tenant RBAC grants any verb on internal.cozystack.io; a ValidatingAdmissionPolicy pins writes to helm-controller as defence in depth. The controller does not grant tenant read access — the lineage webhook does, from the ApplicationDefinition's spec.secrets. This side stamps the engine-agnostic internal.cozystack.io/tenant-ca label and forces a re-admission when those selectors change. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
myasnikovdaniil
|
960d853706 |
feat(api): typed waitStrategy/healthCheckExprs for kstatus readiness (#2642) (#3273)
## What Adds typed `waitStrategy` + `healthCheckExprs` fields so a package/app can declare the Flux helm-controller v1.5.0 kstatus health gate for the custom resource it wraps — making `HelmRelease.Ready` mean "the CR is actually healthy" instead of "helm applied the CR". This is the operator/apiserver foundation of the kstatus epic (#2642); the API surface was reviewed and approved by the API owner in #3263. Commits (per the review's "isolated step-0" request): 1. `build(deps)` — the dependency bump (see below). 2. `feat(api)` — the fields, plumbing, tests, and regenerated code. 3. `fix(api)` — match the openapi code-generator pin to the 0.35 runtime (fixes an aggregated-apiserver crash the bump surfaced). 4. `fix(api)` — declare `OpenAPIModelName` on the apps types so SSA resolves catalog kinds. 5. `docs(api)` — note that `disableWait` silently defeats `healthCheckExprs`. ## API surface Typed fields on **both** HelmRelease-building paths: - `ApplicationDefinition.spec.release` (apiserver / catalog apps → `convertApplicationToHelmRelease`) - `ComponentInstall` (operator / platform packages → `buildHelmReleaseSpec`) ```go // WaitStrategy maps to HelmReleaseSpec.WaitStrategy.Name — a deliberate scalar // simplification of the upstream {name} object. One of poller|legacy. WaitStrategy string // HealthCheckExprs maps to HelmReleaseSpec.HealthCheckExprs (the upstream // kustomize.CustomHealthCheck type verbatim). HealthCheckExprs []kustomize.CustomHealthCheck ``` `config.ResolveWaitStrategy` is shared by both builders and **couples the default**: `healthCheckExprs` are only evaluated under the `poller` strategy, so when expressions are set and no strategy is given, the generated HelmRelease defaults to `poller`. A package that sets only `healthCheckExprs` is therefore self-contained, independent of the controller's global default. ## The dependency bump (commit 1) The `WaitStrategy` / `kustomize.CustomHealthCheck` / `HelmReleaseSpec.HealthCheckExprs` types do not exist below `helm-controller/api` v1.5.0 — the runtime is already v1.5.0 (shipped by #2602), so this closes a Go-types-vs-runtime skew. It is **not** an isolated bump: v1.5.x requires **k8s 0.35 + controller-runtime 0.23**, cascading the k8s stack 0.34 → 0.35 (and the go directive to 1.26). cozystack replaces `k8s.io/apimachinery` with a fork carrying the still-open upstream `ConvertToVersion` patch (k/k#135537). That single commit rebases cleanly onto apimachinery v0.35.0; published at `cozystack/apimachinery@release-1.35-pr135537`, and `go.mod` pins its pseudo-version. **The repo builds with zero source changes under the new stack.** > Maintainer note: please confirm the `release-1.35-pr135537` fork branch naming, or fold it into the `cozystack` branch as you prefer — the pin can be updated to match. ## Not included (conscious YAGNI) `disableWait` is intentionally omitted. The only known need (the kubernetes tenant-addon deadlock) was resolved child-side, and `waitStrategy: legacy` is not equivalent (legacy still waits Helm-v3-style; `disableWait` skips waiting entirely). Add it later when a concrete leaf needs a true no-gate escape hatch. ## Tests - `config.TestResolveWaitStrategy` — the couple-the-default logic (unset+exprs→poller; explicit legacy honored; unset+no-exprs→nil). - Operator + apiserver cases mirror each other (`TestBuildHelmReleaseSpecHealthCheckExprs` / `TestConvertApplicationToHelmRelease_HealthCheckExprs`). - `go build ./...` clean, `go vet` clean, `gofmt` clean, codegen regenerated and committed. The 4 backupstrategy CRDs regenerate with benign k8s-0.35 PodSpec description drift (DynamicResourceAllocation alpha→stable, toleration Lt/Gt, resizePolicy note) — no functional change. ## Follow-up Phase 3 (per-leaf adoption — postgres/kafka/mongodb/etc. declaring their CEL via these fields) builds on this. The full CRD→CEL catalog is maintained in the epic's readiness audit. Refs #2642 ```release-note feat(api): add `waitStrategy` and `healthCheckExprs` to ApplicationDefinition and ComponentInstall, so a HelmRelease reports Ready only once the custom resource it wraps is actually healthy ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added configurable CEL health-check expressions for application releases and package components. * Added `waitStrategy` support (`poller` and `legacy`) with automatic `poller` selection when health checks are configured and no strategy is set. * Propagated these settings into generated Helm releases, affecting readiness/wait behavior. * **Documentation** * Updated Application and PackageSource schemas to include `healthCheckExprs` and `waitStrategy`. * Extended related Kubernetes CRD documentation, including projected pod certificate annotations and workload reference fields. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Andrey KolkovandClaude Fable 5
|
67db8f7da7 |
fix(backups): make the barman-cloud serverName stable across every flow
Two defects of the same class — the WAL-archive S3 prefix (serverName) changing or defaulting differently between components: Recovery default regression (review blocker): native CNPG defaulted an absent externalClusters serverName to the entry name (= bootstrap.oldName), but the barman-cloud plugin defaults it to the NEW cluster's name, so a legacy-flow restore with bootstrap.serverName unset would read the wrong S3 prefix (hard failure, or silently restoring another cluster's data). The chart now always emits serverName, defaulting to bootstrap.oldName. Live-cluster serverName flip (found by the gated postgres backup e2e on a real cluster): the chart renders spec.plugins without serverName (effective prefix = cluster name) while a strategy template may name a different one; the driver's ForceOwnership patch then flipped the prefix mid-stream, splitting the WAL archive across two prefixes — the WAL holding the backup's start was lost between them and the restore failed with "WAL not found". The driver now preserves the live Cluster's effective serverName (explicit parameter, or the cluster name when the plugin is attached without one) and only applies the strategy's value to a Cluster with no plugin attached; both charts pin serverName explicitly, and the example strategy no longer sets one. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey KolkovandClaude Opus 4.8
|
268ec29856 |
test(backups): assert driver-initiated Backup uses method=plugin
Review follow-up (cozystack#3300): ensureCNPGBackup stamps spec.method=plugin + spec.pluginConfiguration on the postgresql.cnpg.io/Backup it creates — the field that routes a driver-initiated run through the barman-cloud plugin. If it regressed to the default (barmanObjectStore) method, CNPG would run the legacy method against a Cluster that no longer has one and every platform backup would fail. Add TestEnsureCNPGBackup_UsesPluginMethod asserting method, plugin name, and cluster ref. (The chart ScheduledBackup path was already covered; the driver path was not.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey KolkovandClaude Opus 4.8
|
2212367842 |
fix(backups): own driver-created ObjectStore + correct stale barman references
Review follow-ups (cozystack#3300): - Lifecycle/leak: applyClusterPluginBackup now sets an ownerReference from the driver-created barmancloud.cnpg.io/ObjectStore to its cnpg.io/Cluster, so Kubernetes garbage-collects it when the Cluster is deleted (tenant app teardown or the RestoreJob purge). In the platform useSystemBucket flow the chart does not render this ObjectStore, so previously it was orphaned in the tenant namespace. Drop the now-accurate-only 'delete' verb on objectstores from the driver RBAC (GC handles removal; the driver never deletes it) and correct the RBAC comment that falsely claimed the purge deletes it. - Diagnostics/comments: the RestoreJob WAL-archive-deadline failure message and several doc comments still told operators to check spec.backup.barmanObjectStore / externalClusters[].barmanObjectStore, fields no longer emitted after the migration. Point them at the barman-cloud plugin (spec.plugins + ObjectStore, externalClusters[].plugin) instead so they don't mislead during an incident. - Test: TestApplyClusterPluginBackup_PatchesExistingCluster now asserts the ObjectStore carries an ownerReference to the Cluster (kind/name/uid). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey Kolkov
|
13ff340277 |
fix(backups): migrate CNPG backups to the barman-cloud plugin
Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrei Kvapil
|
fbbe4af7df |
refactor(network): drop ExposureClass/ServiceExposure for native loadBalancerClass (#3218)
## What Removes the `network.cozystack.io` API group — `ExposureClass` + `ServiceExposure` (#3081) — and its reconciler, in favor of native `Service` `type: LoadBalancer` + `loadBalancerClass`. ## Why Full rationale and discussion in #3164. For **managed applications** the chart already owns the Service, so: - `type: LoadBalancer` + `loadBalancerClass` + an admin-provisioned pool covers vendor-neutral external exposure without a dedicated API group and controller; - the `externalIPs` (node-IP) default never went through `ServiceExposure` (its backend is a no-op) and is left unchanged; - the only current consumer, the host ingress, keeps rendering `type: LoadBalancer` and just drops the `ServiceExposure`; the admin provisions the address pool / announcer out of band, as the install docs already assume. `ServiceExposure`'s one unique feature — dynamic per-namespace scoping of a shared VIP pool — is a convenience the managed-app path does not need, and it does not justify a second tenant-networking API group next to `sdn.cozystack.io`. ## Changes - Delete `api/network/v1alpha1` and `internal/controller/serviceexposure` (+ CRD definitions, e2e bats, controller registration, RBAC). - Host ingress: drop the `ServiceExposure` template; add an optional `publishing.loadBalancerClass` that sets `spec.loadBalancerClass` on the host-ingress LoadBalancer Service. - The `externalIPs` path and default behavior are unchanged. ## Verification - `go build ./...` — green. - `helm unittest` — `core/platform` 72/72, `extra/ingress` 25/25 (incl. host-vs-tenant `loadBalancerClass` scoping and the immutable-field migration guard). ## Migration None required. `network.cozystack.io` (#3081) has never shipped in a release — it is on `main` only (not backported to `release-1.5`; absent from v1.4.x / v1.5.x), so no upgrading cluster has these CRDs and this removes it before it is ever released. Clusters that track `main` and explicitly set `publishing.exposureClass` (opt-in, only since 2026-06-30) should provision their MetalLB / Cilium address pool themselves and set `publishing.loadBalancerClass`; any orphaned `cozystack-<class>` pool can be deleted by hand. Default clusters (externalIPs path) are unaffected. ## Status **Draft / RFC for the #3164 discussion** — this removes an API group that landed only days ago, so nothing here should merge before the design call. cc @lexfrei (author of #3081) and @lllamnyp (#3164). Deferred: when Kubernetes actually removes `Service.spec.externalIPs`, the node-IP default migrates via a MetalLB `IPAddressPool` with no `L2Advertisement` (allocate-but-don't-announce) — a separate change. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added support for configuring the ingress host Service with a native `loadBalancerClass` value. * Expanded chart tests to verify class wiring and immutability behavior across install and upgrade scenarios. * **Bug Fixes** * Removed the legacy exposure-class/service-exposure flow and related controller behavior. * Updated templates and CRD packaging so the platform no longer renders or depends on those deprecated resources. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Myasnikov Daniil
|
96a9652fb1 |
feat(api): typed waitStrategy/healthCheckExprs for kstatus readiness
Add first-class fields so a package/app can declare the Flux helm-controller v1.5.0 kstatus health gate for the custom resource it wraps, so HelmRelease.Ready means "the CR is actually healthy" instead of "helm applied the CR" (#2642). API-owner approved in #3263. - api: WaitStrategy (scalar, maps to HelmReleaseSpec.WaitStrategy.Name — a deliberate simplification) + HealthCheckExprs ([]kustomize.CustomHealthCheck, the upstream type verbatim) on both ApplicationDefinition.spec.release (the apiserver path) and ComponentInstall (the operator path). - config.ResolveWaitStrategy: shared by both HelmRelease builders. Couples the default — healthCheckExprs imply the poller strategy when none is set, since exprs are only evaluated under poller. This removes the "must also set waitStrategy: poller" footgun and does not depend on the controller's global default. - thread through buildHelmReleaseSpec (operator) and convertApplicationToHelmRelease (apiserver); the apiserver loads the typed spec.release fields in start.go. - regenerate deepcopy, the ApplicationDefinition and PackageSource CRDs, and openapi. The backupstrategy CRDs also regenerate — benign k8s-0.35 PodSpec description drift, not a functional change. disableWait is intentionally not added (YAGNI): the only known need (the kubernetes tenant-addon deadlock) was resolved child-side, and waitStrategy: legacy is not equivalent (legacy still waits Helm-v3-style; disableWait skips waiting entirely). Add it later when a concrete leaf needs a true no-gate escape hatch. Refs #2642 Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Ivan Okhotnikov
|
a8494d874f |
test(backups/postgres): reconcile-level repeat-restore regression + conservative freshness tie
Address review feedback on the repeat in-place restore fix: - Make the freshness discriminator strict (creationTimestamp > StartedAt) so an exact tie resolves to "not fresh" and the caller purges, matching the function's stated conservative default; a fresh Cluster is always created well after StartedAt so this never misclassifies the status-write-race path. - Document why a wall-clock timestamp comparison was chosen over recording the purged Cluster's UID on the TargetPurged condition (the UID approach leans on the same status write the race path assumes can fail). - Add a reconcile-level regression test that drives reconcileCNPGRestore end to end with a fake client: a stale leftover recovery Cluster from a prior restore is purged, and a Cluster this restore just re-created is not re-purged. This covers the actual call site where #3311 lived, not just the pure helpers. Signed-off-by: Ivan Okhotnikov <xorokhotnikov@gmail.com> |
||
|
Ivan Okhotnikov
|
cf1b6ba28e |
fix(backups/postgres): purge stale recovery cluster on repeat in-place restore
A repeat in-place PostgreSQL RestoreJob into an already-restored target silently no-opped: the destructive purge was skipped because the live Cluster still carried spec.bootstrap.recovery left over from the previous restore, and the re-purge guard mistook that for a freshly-recovered Cluster produced by this restore's own purge. The job then reported Succeeded while the PVC, disk, and data were never touched. Tell the two apart by the Cluster's creationTimestamp relative to the RestoreJob's StartedAt: only a recovery Cluster created at/after StartedAt is treated as this restore's own and skipped; one that predates StartedAt is leftover from an earlier completed restore and is purged so the restore re-bootstraps from the backup. Signed-off-by: Ivan Okhotnikov <xorokhotnikov@gmail.com> |
||
|
Aleksei Sviridkin
|
249f7b5a5e |
feat(platform, ingress): propagate operator wildcard certificate to per-tenant termination points (#2990)
## What this PR does The operator-provided wildcard certificate (`publishing.certificates.wildcardSecretName`) currently serves the root tenant only: everything is same-namespace, so the root ingress controller and Gateway read the operator's TLS Secret directly. Child tenants run their own ingress controller / Gateway in their own namespace under a default-deny policy and cannot read that Secret across namespaces, so they fall back to per-host ACME. This propagates the operator wildcard to per-tenant termination points by replicating the Secret into each tenant namespace that owns one, keeping the root-MVP invariant intact: the wildcard is deliberately distributed to tenant namespaces, but no tenant RBAC is widened — every consumer reads only its own-namespace copy. A new controller in `cozystack-controller` does the replication with no extra operator input. It reads the same platform values channel the consumers read (`cozy-system/cozystack-values`): the wildcard Secret name from `_cluster.wildcard-secret-name` and the publishing namespace from `_cluster.expose-ingress`. It mirrors that Secret into every tenant namespace whose `namespace.cozystack.io/ingress` or `namespace.cozystack.io/gateway` label equals its own name — i.e. the namespace runs its own controller / Gateway. Because the source is derived from the same value that makes the consumers reference it, the replica is created whenever the consumers expect it — no manual labelling and no upgrade hazard where a child controller references a Secret that nothing creates. Replicas are refreshed on rotation and pruned when a namespace stops terminating TLS. Teardown of every replica happens on exactly one trigger: clearing `wildcardSecretName` (explicit disable). A source that is merely absent or mistyped, or a transiently-missing values channel, leaves existing replicas in place, so a brief gap, a delete+recreate rotation, or a misconfigured publishing namespace never drops tenant TLS. The platform controller's own ServiceAccount gains Secret writes; tenant RBAC is untouched. The controller does not cache every Secret in the cluster: the manager's Secret informer is scoped to managed replicas and the values channel only, and the operator source / foreign collisions are read through the uncached APIReader. The source is not watched (its name is dynamic), so an in-place rotation is picked up by a bounded periodic resync while the publishing tenant serves the source with no lag. On the consumer side, each tenant ingress controller now sets `--default-ssl-certificate` to the wildcard Secret in its own namespace (`.Release.Namespace/<name>`), so a child tenant serves the replica instead of minting per-host ACME. The Gateway path already renders `certMode: existingSecret` with a same-namespace `wildcardSecretRef` for every tenant, so it consumes the replica with no chart change. ### Why not clustersecret-operator The issue proposed the shipped `clustersecret-operator`. It materializes inline template data into selected namespaces; it cannot mirror an existing source Secret, and it is not installed in any default bundle. Driving it would mean embedding the certificate and private key as literal data in a cluster-scoped object rendered from a Helm `lookup` — that broadcasts key material the design keeps off any shared channel, and a `lookup` is invisible to the Flux helm-controller digest so it would not re-render on rotation. Its namespace selector also cannot express "the namespace whose owner label equals its own name", so it would over-replicate the key into every tenant namespace. A small dedicated reconciler targets exactly the termination namespaces and keeps the key off the values channel. ### Coverage caveat Replication delivers the certificate bytes, not SAN coverage: a single `*.<root-host>` does not match `*.<child-apex>`. Operators serving child-tenant subdomains supply a certificate whose SANs cover those apexes; the docs spell this out. The replica carries the wildcard private key into each terminating tenant namespace — the same exposure as the per-host ACME Secret it replaces, now shared across tenants. ### Tests - Go fake-client tests for the reconciler: termination-only replication (ingress-owning and gateway-owning namespaces only, never inheriting ones, never over the source), copy markers, rotation, prune on namespace demotion, the non-destructive source-deletion contract, replica self-heal after out-of-band deletion, the publishing-namespace self-replication guard, disable-via-config teardown, source rename, foreign-Secret collision safety (no overwrite, no requeue, does not block siblings), transient-error requeue, non-TLS source ignored, absent-channel keeps replicas, publishing-namespace mismatch keeps and requeues, the cache-scope pin, the enqueue-wiring map, an `ownsTerminationPoint` table, and the active-only requeue contract. - helm-unittest for the ingress `default-ssl-certificate` wiring (including a child-tenant controller pointing at its own-namespace replica) and for the cozystack-controller ClusterRole's new Secret-write rule. ### Screenshots N/A — no UI changes. ### Release note ```release-note feat(platform): the operator-provided wildcard TLS certificate (`publishing.certificates.wildcardSecretName`) is now served by per-tenant ingress controllers and Gateways automatically — the platform controller replicates it into each tenant namespace that terminates TLS, with no extra operator input and no cross-namespace Secret read ``` Closes #2820 Part of #2811 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **New Features** * Wildcard TLS Secret now replicates from the platform to tenant namespaces that terminate TLS via their own ingress controller or Gateway, including child-tenant scenarios. * **Documentation** * Clarified configuration, timing, and SAN matching requirements for wildcard replication. * Updated gateway and ingress-nginx default certificate documentation for replica mirroring behavior. * **Bug Fixes** * Improved handling for missing/invalid source secrets (no unnecessary pruning) and safe collision behavior. * **Tests** * Added comprehensive reconciliation/replication test coverage. * Added Helm unittest target for the controller chart. * **Chores** * Updated controller RBAC to support wildcard TLS Secret replication. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Myasnikov DaniilandClaude Fable 5
|
9297e51170 |
Merge origin/main into feat/e2e-chainsaw-example
Reconcile the Chainsaw e2e migration with ~500 commits of main drift: - keep main's run-kubernetes.sh helpers (kubectl_wait_retry, tenant drain, LINSTOR pool-free wait, SC-fallback probe) under the branch's _lib/ rename with the Chainsaw finally/snapshot deltas re-applied - resolve modify/delete on 10 drifted hack/e2e-apps/*.bats as deleted; their post-branch assertion drift is re-ported into the Chainsaw suites in follow-up commits - drop the 4 new main-side bats (kubernetes-oidc-*, securitygroup, serviceexposure) from _lib/ relocation; ported as Chainsaw suites in follow-up commits - docs/agents/e2e-testing.md: merge main's plan-job TIA wording with the Chainsaw suite phrasing Assisted-By: Claude <noreply@anthropic.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
myasnikovdaniil
|
48a3cfd341 |
Merge branch 'main' into ci/scorecard-hardening
Signed-off-by: myasnikovdaniil <60174387+myasnikovdaniil@users.noreply.github.com> |
||
|
Andrei Kvapil
|
3dbc7d3662 |
refactor(network): drop ExposureClass/ServiceExposure for native loadBalancerClass
Remove the network.cozystack.io API group (ExposureClass + ServiceExposure) and its reconciler. Managed-application charts own their Service, so native type: LoadBalancer + loadBalancerClass + an admin-provisioned pool covers external exposure without a dedicated API group and controller. The host ingress -- the only consumer -- keeps rendering type: LoadBalancer and gains an optional publishing.loadBalancerClass to pick the LB controller; the externalIPs (node-IP) default path is unchanged. Rationale in cozystack#3164. Assisted-By: Claude Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io> |
||
|
Myasnikov DaniilandClaude Opus 4.8
|
1f0c1949e7 |
test(flux-shard-operator): add fuzz targets for shard config parsers
Native Go fuzzing over the flag parsers in config.go, asserting the
invariants that matter rather than just no-panic:
- ParseShardIndex round-trips with ShardName (accepted input is the canonical
name of the returned index; index is non-negative).
- ShardName round-trips back through ParseShardIndex for every valid index.
- ParseShardCount: a non-auto success yields count >= 1; auto yields 0.
- ParsePinnedTenants: every entry in a success has a non-empty key mapped to a
valid shard name.
Adds an OSS-Fuzz-detectable fuzzing signal for the OpenSSF Scorecard Fuzzing
check while covering parsers that sit on the sharding hot path.
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Assisted-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
||
|
Timofei Larkin
|
0f55a150ba |
fix(ci): catch templated CRDs and APIService-only groups in the API gate (#3184)
## What this PR does Closes two coverage gaps found while shadow-observing the API-owner-review gate (#3167) against real PRs in the first hours after merge: 1. **CRD discovery was gated on directory name** (`crds/`, `definitions/`, `manifests/`), so a CRD shipped inside a chart's `templates/` dir — a valid, existing convention when the CRD is wrapped in a `{{- if }}` guard — was invisible to the gate. This was not hypothetical: `#3149` (cozyplane) adds four brand-new CRDs (`VPC`, `VPCBinding`, `VPCPeering`, `Port`) exactly this way, and none of them tripped a review requirement. 2. **Groups served only by an external aggregated apiserver had no representation at all.** The same `#3149` also registers `sdn.cozystack.io` via a `kind: APIService` pointing at a prebuilt image built outside this repository — no CRD, no vendored Go types, nothing to diff. ### Fix - CRD/APIService discovery is now content-based: every `.yaml`/`.yml` file under `packages/` and `internal/` is a candidate, and whether it's kept depends on whether it parses into the right `kind` with a first-party group — not which directory it sits in. - Both parsers now tolerate a Helm directive line (`{{- if }}`, `{{- end }}`, a comment block) sharing a `---`-delimited document with otherwise-valid YAML, rather than dropping the whole document. This matters because the closing `{{- end }}` of a wrapping conditional has no `---` before it, so it lands in the same document as the manifest it closes. - Added a fourth resource source, `SourceAPIService`, for `kind: APIService` manifests registering a first-party group. Like the existing static Go-backed groups, these carry no checked-in schema, so they participate only in new-group / new-resource / removal detection — never a fabricated breaking-change diff against a schema this repo can't see. Verified against the real `feat/cozyplane` branch: the gate now reports all four new CRDs, plus the removal of the old `securitygroups`/`APIService` registration for the same group — a full API-surface migration that was previously invisible end to end. ### Release note ```release-note fix(ci): the API-owner-review gate now discovers CRDs and APIService registrations by content instead of directory name, closing a gap where a CRD or an externally-served aggregated API group could ship without triggering the required review. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Discovery now includes APIService registrations, enabling detection of groups/versions exposed only via aggregated APIService manifests (including merging versions from multiple templates). * **Bug Fixes** * Improved YAML discovery to tolerate mixed/templated multi-document streams: non-CRD/non-APIService documents no longer stop discovery, and embedded template directives are handled more reliably. * File processing is more resilient during snapshot loading (includes a safety cap on YAML size). * **Tests** * Added coverage for APIService discovery, templated neighbor documents, and correct exclusion of third-party manifests. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Timofei Larkin
|
50f8548ba9 |
fix(ci): stop double-parsing every file in the API gate's content walk
branch-review on #3184 measured a real regression: switching CRD/ APIService discovery to content-based scanning made every file in packages/+internal a candidate, and each was split into documents and fully unmarshaled twice — once per parser — inflating a full snapshot load from ~0.5s to ~3.4s. Split each file's documents once and probe each document's kind once before deciding which shape to try, so the common case (a document that is neither a CRD nor an APIService — the overwhelming majority of what the wider walk now sees) pays for at most one parse attempt instead of two. Add a raw substring fast-reject, at both the per-document and whole-file level, ahead of any YAML parsing: neither kind this package cares about can appear without its own name appearing as a literal substring first. Also fixes two smaller gaps flagged in the same review: an untrusted PR head checkout could point discovery at an arbitrarily large file with no size guard (capped at 8 MiB), and a group's merged APIService resource only recorded the first contributing file's path, dropping the others from the report. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Timofei Larkin <lllamnyp@gmail.com> |
||
|
Timofei Larkin
|
985f17b7be |
fix(ci): catch templated CRDs and APIService-only groups in the API gate
Shadow-observation of the merged gate found a real miss: a CRD shipped
inside a chart's templates/ dir (guarded by a Helm {{- if }}) was
invisible because discovery was gated on directory name (crds/,
definitions/, manifests/), and a group served only by an external
aggregated apiserver (kind: APIService, no CRD, no vendored Go types)
had no representation at all.
Switch CRD/APIService discovery from directory-name to content
sniffing, and tolerate Helm directive lines sharing a document with
otherwise-valid YAML (the closing `{{- end }}` of a wrapping
conditional has no leading `---`, so it lands in the same document as
the manifest). Add a fourth source, SourceAPIService, reusing the
existing empty-schema mechanism so a first-party group's first
APIService appearance requires review without pretending to diff a
schema that lives outside this repository.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
|
||
|
Andrey Kolkov
|
cc5d688e15 |
feat(platform): migrate to etcd-operator v1alpha2
Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Ivan Okhotnikov
|
0d98e27981 |
fix(operator): hold final backoff window before give-up, note upstream version skew
Address lexfrei's non-blocking follow-ups on the LGTM approval: Follow-up #1 — give-up preempted the final backoff. `decideRecovery` returned `recoveryActionGiveUp` as soon as `attempts >= maxRecoveryAttempts`, without checking whether the final-force backoff window had elapsed. The Owns()-watch re-fire from the N-th force-drift's own writes triggered a reconcile milliseconds after the force, so the effective time-to-give-up was ~7.5m (30s+60s+2m+4m) — not the ~11.5m documented on the constants block. The final 4-minute response window the last nudge was supposed to grant source-watcher was never actually granted. Fix: extend the exhaustion branch to hold `Wait` until `now - lastRecoveryAt >= backoffFor(maxRecoveryAttempts)` before switching to GiveUp. Now the documented ~11.5m budget matches the observed behaviour, and the last force gets a real chance to succeed (Owns watch → source-watcher Ready=True copy-through) before we surface SourceWatcherStalled. Tests: * `TestDecideRecovery` gains a case pinning `Wait` when attempts=maxRecoveryAttempts but the final backoff has not yet elapsed (the exact re-fire shape the previous behaviour tripped on). * `TestUpdateStatus_OwnMarkerAttemptsExhaustedButFinalBackoffPending` drives the full updateStatus path against the same shape, asserting the PackageSource stays in Unknown/AwaitingSourceWatcherRecovery and the RequeueAfter equals the remaining backoff, NOT zero (GiveUp). Follow-up #3 — upstream version-skew note. The Go module dependency tracks `fluxcd/source-watcher/api/v2 v2.0.3` (CRD types package only), but the deployed binary this workaround targets is pinned to v2.1.0 at `internal/fluxinstall/manifests/fluxcd.yaml`. Comments referencing `drift.go:52` / `controller.go:164/254` describe the deployed binary, not the compiled API package. Documented on the constants block so a future flux bump re-verifies the anchors AND the watch predicate / drift-detection heuristics, which are the load-bearing upstream assumptions. Follow-up #2 (envtest over full Reconcile→Owns→updateStatus cycle) is architectural and out of scope for this PR — noted for a follow-up. Signed-off-by: Ivan Okhotnikov <xorokhotnikov@gmail.com> |
||
|
Ivan Okhotnikov
|
37421d2fec |
docs(operator): honest JSON-merge-patch tradeoff, clarify asymmetric OG check, warn on partial force-drift
Self-review nits raised in a skeptical pass over the current tip: * The status-patch docstring in forceArtifactGeneratorDrift described the JSON-merge-patch full-array-replace on `status.conditions[]` as "net-neutral". That was too optimistic: if source-watcher writes a concurrent Reconciling condition between our Get and our Patch, our patch does transiently wipe it. source-watcher restores it on its next reconcile (which our requestedAt bump is about to trigger), but downstream tooling watching for Reconciling can observe the momentary absence. Rewrite the comment to name that trade-off explicitly and point at server-side apply as the fuller fix if the operator ever migrates to SSA globally. * The unified own-marker/progressing/stuck gate in updateStatus is asymmetric on ObservedGeneration — isProgressingInRecovery checks it, isOwnMarker does not. That asymmetry is deliberate (a stale own-marker after a spec edit must still route to the state machine to avoid leaking the synthetic reason to the PackageSource — exactly the B2 regression), not an oversight. Add a comment explaining the reasoning so a future edit doesn't "symmetrise" the check and re-open B2. * forceArtifactGeneratorDrift's step-2 (metadata patch) failure was silent when it happened after a successful step-1 status patch. In that state source-watcher has been nudged, but the tracking annotations never persisted, so the next reconcile reads attempts=0 and re-forces from scratch instead of accumulating toward SourceWatcherStalled — a persistent metadata-write-only apiserver malfunction would loop forever. Log the failure loudly with the intended attempt count so operators can spot this specific mode from cozystack-operator logs. Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io> |
||
|
Ivan Okhotnikov
|
596427deb3 |
docs(operator): scrub stale awaitSourceWatcherResponse references from comments and test names
Self-review after the previous commit removed the awaitSourceWatcherResponse helper (routing unified through maybeRecoverArtifactGenerator's state machine via a single own-marker / progressing-in-recovery / stuck gate). Four comments and two test names still referenced the now-deleted symbol — misleading anyone tracing the architecture. * maybeRecoverArtifactGenerator docstring: rewrote the trade-off paragraph to describe the unified gate and Wait branch instead of the removed helper. * TestMaybeRecoverArtifactGenerator_DoesNotResetCounterOnFreshTransition comment: same rewrite. * TestUpdateStatus_OwnMarkerRoutesToAwait renamed to TestUpdateStatus_OwnMarkerHoldsTrackingViaStateMachine + comment rewritten to describe the state-machine Wait branch. * TestUpdateStatus_ProgressingDuringRecoveryRoutesToAwait renamed to TestUpdateStatus_ProgressingDuringRecoveryHoldsTracking + comment rewritten in parallel. Pure doc / naming; no code or test-body changes. Tests still pass with -race, vet clean, golangci-lint 0 issues. Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io> |
||
|
Ivan Okhotnikov
|
7b08fcdca9 |
fix(operator): unify recovery gating through state machine, close source-watcher-unresponsive dormancy loop
Self-review on 467b788b8 caught a MAJOR bug introduced by the separate awaitSourceWatcherResponse helper: if source-watcher never responds to a forced drift (network partition, missing predicate, dead pod), our own reasonRecoveryForced marker keeps reflecting back via Owns() forever, and the wait-forever helper schedules a 1-second requeue tight loop without ever advancing the retry state machine. SourceWatcherStalled give-up was unreachable. Fix: remove awaitSourceWatcherResponse and unify all three recovery entry conditions — our own marker, source-watcher's Progressing during rebuild, and the classic first-time stuck detection — through a single gate that routes to maybeRecoverArtifactGenerator. The state machine's decideRecovery is the single arbiter of wait/force/give-up, and its Wait branch is bounded by the same backoff schedule as Force. When backoff elapses on a still-reflecting own-marker the machine escalates to another force (bumps attempts, re-stamps requestedAt); when attempts exhaust it surfaces SourceWatcherStalled honestly. The own-marker filter also no longer requires attempts>0, so an operator crash between step 1 (status patch) and step 2 (metadata patch) of forceArtifactGeneratorDrift is still routed through the state machine on the next reconcile rather than leaking the synthetic marker onto the PackageSource. Also address the non-blocking notes from the same self-review: * Docstring on maybeRecoverArtifactGenerator was still describing the removed reset-counter branch and the "requestedAt alone is a no-op" framing. Rewrite to explain the current shape: status write triggers the drift signal, requestedAt bump triggers the reconcile event — both are required, neither alone works. * TestForceArtifactGeneratorDrift_MetadataPatchFailure_RollbackAnnotations now asserts reconcile.fluxcd.io/requestedAt is also rolled back on the metadata-patch failure branch (symmetric with the tracking annotations). * TestUpdateStatus_OwnMarkerRoutesToAwait asserts an exact 25s RequeueAfter (backoffFor(1)=30s minus 5s elapsed) so a silent regression in the backoff arithmetic is caught. New regression tests for the source-watcher-unresponsive path: * TestUpdateStatus_OwnMarkerStale_TriggersRetry: with attempts=1 and lastRecoveryAt older than backoffFor(1), the reconcile must escalate through the Force branch and bump attempts to 2 rather than idle in a tight requeue loop. * TestUpdateStatus_OwnMarkerAttemptsExhausted_GivesUp: with attempts=maxRecoveryAttempts and the marker still reflecting back, the reconcile must set PackageSource Ready=False / SourceWatcherStalled instead of extending the loop indefinitely. Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io> |
||
|
Ivan Okhotnikov
|
934ca46a36 |
fix(operator): bump requestedAt with force-drift, filter own writes, remove reset-counter loop
Address lexfrei's three blockers on the force-drift approach — as
committed, the recovery driver did not function on a live cluster.
**B1**: `forceArtifactGeneratorDrift` wrote `Ready=False` on the AG's
status subresource but did not touch its metadata annotations.
source-watcher v2.1.0 registers its ArtifactGenerator watch with
`predicate.Or(GenerationChangedPredicate, ReconcileRequestedPredicate)`
— a status-only patch changes neither the generation nor
`reconcile.fluxcd.io/requestedAt`, so it does NOT enqueue a
source-watcher reconcile. The AG's periodic self-requeue is hardcoded
to `time.Hour`, well outside the 15m HR install timeout. Fix: also
stamp `reconcile.fluxcd.io/requestedAt` in the metadata patch so the
ReconcileRequestedPredicate fires and source-watcher's next reconcile
picks up the `IsFalse(Ready)` drift signal.
**B2**: `Owns(&ArtifactGenerator{})` re-fires this reconciler on the
force-drift's own writes. Both the status patch (Ready=False, reason
`SourceWatcherRecoveryForced`) and the metadata patch enqueue a
PackageSource reconcile. Blindly running `clearRecoveryTracking` on
those re-entrant fires nuked the attempts counter — the give-up
budget was unreachable, and the synthetic marker leaked to
`PackageSource.status.conditions[Ready]`. Fix: in `updateStatus`,
detect our own reason and route into a new `awaitSourceWatcherResponse`
helper that holds tracking and reports Unknown /
AwaitingSourceWatcherRecovery on the PS. The same helper covers the
Progressing/Unknown write source-watcher issues during rebuild once
tracking is present (attempts > 0) — otherwise the transient
Progressing would trigger the same premature tracking clear.
**B3**: The attempt-counter reset keyed on
`Ready.LastTransitionTime > lastRecoveryAt`. That inequality is true
on every source-watcher response during recovery (the Progressing
write is always newer than our force), so under the very race this
driver targets the counter would reset every cycle and
`recoveryActionGiveUp` was unreachable. Fix: remove the reset. The
canonical counter reset is the natural `clearRecoveryTracking` on the
not-stuck path (Ready=True from source-watcher, or a real upstream
Ready=False), and the HA/leader-election requirement already
contains the "missed recovery event on operator restart" edge case
the reset was trying to paper over.
Extract `reasonRecoveryForced` to a named constant so both the
writer (forceArtifactGeneratorDrift) and the reader (updateStatus'
own-marker filter) use the same symbol.
New tests pin all three fixes:
- ForceArtifactGeneratorDrift_Success now asserts the
reconcile.fluxcd.io/requestedAt annotation is set (B1).
- UpdateStatus_OwnMarkerRoutesToAwait asserts we do NOT clear
tracking and do NOT copy the synthetic Ready=False through when we
see our own marker (B2).
- UpdateStatus_ProgressingDuringRecoveryRoutesToAwait covers the
second Owns() re-entrancy path — source-watcher's Progressing
write during rebuild (B2).
- UpdateStatus_ReadyTrueClearsTracking pins that the natural not-
stuck path IS the canonical counter reset after B3.
- The old ResetsCounterOnFreshTransition test is replaced with
DoesNotResetCounterOnFreshTransition, asserting the opposite
behaviour that closes B3.
Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io>
|
||
|
Ivan Okhotnikov
|
f64081b232 |
fix(operator): schedule follow-up reconcile when Ready is Unknown within grace
E2E on dev3 caught a dormancy bug that renders the whole recovery driver inactive in practice: when updateStatus copies Ready=Unknown across but the AG's LastTransitionTime is still within stuckGracePeriod, artifactGeneratorStuck returns false and we fall through to the copy path with no RequeueAfter set. If the reconcile happens to fire in the [stuck_start, stuck_start+30s) window — a routine case since operator restarts touch every PackageSource — the controller then sleeps waiting for an AG-status change that will never come (that IS the fluxcd/pkg#934 lost-write race), up to the 10h informer resync. Observed on dev3: `cozystack.networking` stuck at Ready=Unknown for 9+ minutes with zero recovery attempts. Two reconciles fired at stuck_start+23s and +24s — both correctly detected "not yet stuck" — and then nothing until observation ended. Upstream OCIRepository revision was unchanged throughout, ruling out a coincidental source update as the cause. Fix: whenever we copy Ready=Unknown to the PackageSource (or set a synthetic Unknown because no Ready condition exists yet), compute the remaining grace period against the AG's LastTransitionTime (or CreationTimestamp for the missing-Ready branch) and schedule a RequeueAfter at that expiry. A 1s floor prevents a zero-delay tight loop when the anchor is right at or past grace expiry. Extracted the delay computation to `agFollowUpDelay` for direct unit testing. Adds three tests: the follow-up scheduling on Ready=Unknown in-grace, the same on the missing-Ready branch, and a table test on agFollowUpDelay covering the floor edge cases. Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io> |
||
|
Ivan Okhotnikov
|
4df18df5de |
test(operator): pin maybeRecoverArtifactGenerator branches, tighten wait assertions, polish comments
Address the fourth-pass self-review's non-blocking findings:
* Add TestMaybeRecoverArtifactGenerator_{ForceBranch, WaitBranch,
GiveUpBranch, ResetsCounterOnFreshTransition}. The three switch
outcomes of the driver (force / wait / give-up) and the reset-counter
guard on a fresh Ready.LastTransitionTime transition were previously
only covered indirectly through their pure-function components. Now
each outcome is asserted end-to-end against a fake client with a
status subresource, verifying the Ready condition, tracking
annotations, RequeueAfter, and — for the wait branch — that no AG
writes are issued at all.
* Tighten TestDecideRecovery's wait-branch assertions from a loose lower
bound to exact equality. decideRecovery is deterministic
(needed - elapsed) so an exact assertion catches subtle backoff
regressions a lower bound would miss (coderabbit's inline nit).
* Rename the local var `wantWaitMin` to `wantWait` for the tighter
assertion shape.
* Fix two stale identifier references in comments
(maybeRequeueArtifactGenerator, AwaitingSourceWatcherRequeue) left
over from the rename pass.
* Add a docstring paragraph on forceArtifactGeneratorDrift explaining
why the JSON-merge-patch full-array-replace on
`.status.conditions[]` is net-neutral in this workaround's context.
Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io>
|
||
|
Ivan Okhotnikov
|
7c3af85e58 |
fix(operator): force source-watcher drift via status patch, not requeue bump
Rework the fluxcd/pkg#934 recovery driver after myasnikovdaniil's B1 finding: bumping reconcile.fluxcd.io/requestedAt cannot recover a stuck ArtifactGenerator because source-watcher v2.1.0's reconcile is drift-gated. detectDrift (artifactgenerator_drift.go:52) returns NoDriftDetected for exactly the stuck signature we target — Inventory populated, ObservedSourcesDigest set, Ready=Unknown on the current generation — so the ReconcileRequestedPredicate fires a reconcile that lands on the no-drift early-return at controller.go:164 and MarkTrue at :254 is never reached. Every attempt in the previous shape was a no-op. New mechanism: force drift by patching Ready=False on the AG's status subresource. detectDrift picks this up as NotReady drift, source-watcher takes the drifted branch, and either MarkTrue (on healthy rebuild) or MarkFalse with a real reason (on genuine failure) replaces our synthetic False. Owns() watch fires this reconciler to copy the resolved condition through. Same bounded-retry harness (30s / 60s / 2m / 4m / 4m budget inside the 15m HR install timeout, SourceWatcherStalled surfaced on exhaustion), same annotation-based state on the AG, same rollback contract on Patch failure — only the action itself changed. Additional fixes bundled in: * Reset attempt counter when the AG's Ready.LastTransitionTime is newer than our lastRecoveryAt, so a fresh stall episode after a previous give-up gets a fresh budget rather than an immediate re-give-up (myasnikovdaniil's follow-up #3). * Propagate updateStatus errors from Reconcile instead of swallowing them, so controller-runtime's exponential backoff retries transient failures on force-drift / status-update writes rather than dropping them silently (coderabbit's inline finding). * forceArtifactGeneratorDrift documents the two-subresource shape (status patch first, metadata patch second) and the partial-failure semantics. * New tests cover the status-patch success / status-patch failure / metadata-patch failure paths, all with fake client + interceptor. The successful case asserts Ready=False actually lands in the apiserver via WithStatusSubresource. * All annotation / action / decision / helper names renamed from *Requeue* to *Recovery* to match the new semantics. Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io> |
||
|
Ivan Okhotnikov
|
4c3099d6f2 |
test(operator): pin overwrite-rollback contract, minor polish
Address the fourth-pass self-review's non-blocking NITs: * Add TestBumpArtifactGeneratorRequeue_PatchFailure_RollbackRestoresPriorBump to cover the subtle case the two existing rollback tests miss: when the tracking annotations were already set from a prior bump, a failed Patch must restore the PRIOR values, not delete the keys. A refactor that naively swapped rollback for `delete(ag.Annotations, key)` would pass the nil-rollback tests but silently regress this contract. * Rename the cloneAnnotations subtest from "deep copy" to "independent copy" — the code documents shallow copy semantics and the misnomer was the only thing pointing readers at the wrong mental model. * Hoist `now.UTC().Format(time.RFC3339Nano)` to a local so the two timestamp annotations use one formatted value instead of two identical Format calls. Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io> |
||
|
Ivan Okhotnikov
|
28834ca874 |
test(operator): cover rollback contract, deletion guard, and clone helper for the requeue driver
Address the third-pass self-review's blocker: the mutation contract on bumpArtifactGeneratorRequeue / clearRequeueTracking, the DeletionTimestamp early-return in Reconcile, and the nil-preserving cloneAnnotations helper had no regression tests, so a future refactor could silently break the contracts documented in comments. New fake-client + interceptor-based tests: * TestCloneAnnotations — pins the "nil in / nil out" sentinel behaviour and mutation-independence of the shallow copy. * TestBumpArtifactGeneratorRequeue_Success — asserts the persisted state matches the caller's in-memory `ag.Annotations` on success, including that reconcile.fluxcd.io/requestedAt is actually written (previously never asserted anywhere). * TestBumpArtifactGeneratorRequeue_PatchFailure_Rollback and its PreservesUnrelated variant — drive the Patch-failure branch via an interceptor client and assert both that our tracking annotations are rolled back and that pre-existing unrelated annotations survive. * TestClearRequeueTracking_Noop — asserts we don't issue a no-op Patch on every healthy reconcile when there are no tracking annotations to clear. * TestClearRequeueTracking_PatchFailure_Rollback — same rollback contract as bump, on the clear path. * TestReconcile_DeletionTimestamp_EarlyReturn — asserts a PackageSource with a deletionTimestamp does not receive any writes and does not schedule a RequeueAfter. Doc tightening in the same commit: * Reword clearRequeueTracking's comment on the flux annotation semantics — standard flux controllers do not clear reconcile.fluxcd.io/requestedAt, they only advance .status.lastHandledReconcileAt via the ReconcileRequestedPredicate. * Note controller-runtime v0.15+'s default RecoverPanic behaviour where the retry-driver's unhandled-action panic is raised, so a future SetupWithManager flip that disables it is easier to spot. * Explain why cloneAnnotations' shallow copy is sound (annotation values are immutable strings). Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io> |
||
|
Ivan Okhotnikov
|
c3f9125163 |
fix(operator): skip deleted PackageSource, restore ag mutation contract, document HA + timing rationale
Address second-pass self-review findings on the bounded-requeue driver: * Skip reconcile early when the PackageSource has a deletionTimestamp so the retry driver stops writing "AwaitingSourceWatcherRequeue" onto an object about to be garbage collected. ownerReference cascade handles the ArtifactGenerator cleanup. * Fix the mutation contract on bumpArtifactGeneratorRequeue and clearRequeueTracking: the previous deep-copy shape left the caller's `ag` desynchronised from the apiserver on success. Now mutations land on `ag` itself with a rollback to the pre-call annotation snapshot on Patch failure — so success reflects persisted state and failure does not leak unpersisted state to subsequent code paths. * Convert the "unreachable" default in maybeRequeueArtifactGenerator to an explicit panic so a future requeueAction added without a matching case fails loudly instead of freezing the Ready condition silently. * Document the timing constants' rationale (grace period matches the observed <10s source-watcher settle time on dev3; total ~11.5m budget keeps SourceWatcherStalled inside the 15m HR install timeout) and note the operator must run with leader election when replicas > 1 so the annotation-based retry state does not race. Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io> |
||
|
Ivan Okhotnikov
|
c5845db221 |
refactor(operator): thread single now through reconcile, harden patches, tighten status
Address self-review nits on the bounded-requeue driver: * Sample `now := time.Now()` once at Reconcile entry and thread it into `updateStatus`, `artifactGeneratorStuck` and `maybeRequeueArtifactGenerator` so every time-sensitive check inside a reconcile agrees on the same wall-clock reading. * Set `ObservedGeneration` in the three early Ready-condition branches (`SourceRefNotSet`, `ArtifactGeneratorNotFound`, `ArtifactGeneratorNotReady`) for parity with the retry-driver branches. * Patch the ArtifactGenerator via a deep copy in `bumpArtifactGeneratorRequeue` and `clearRequeueTracking` so a failed Patch no longer leaves the caller's in-memory `ag` desynchronised from the apiserver. * Document the retry driver's dependency on source-watcher's ReconcileRequestedPredicate on ArtifactGenerator, so a future upstream change dropping the predicate is easier to trace. Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io> |
||
|
Ivan Okhotnikov
|
ff314797e9 |
refactor(operator): drive source-watcher via bounded requeue instead of synthesising PackageSource Ready
Replaces the Ready=True synthesis for PackageSources whose ArtifactGenerator hit the fluxcd/pkg#934 patch.Helper split-write race with a bounded retry driver: on detected stall, bump reconcile.fluxcd.io/requestedAt with exponential backoff (30s to 4m cap) up to 5 attempts, then surface as PackageSource Ready=False with reason SourceWatcherStalled. The previous approach could mask a genuine regeneration failure if the same patch.Helper race that the workaround exists for also swallowed a real Ready=False write, and it permanently reinterpreted PackageSource.Ready semantics from "current revision processed" to "valid consumable artifacts exist". Requeue keeps the CRD contract intact and, on stall exhaustion, produces an honest failure signal an operator can act on instead of a false-positive True. Retry state (attempts + last-bump timestamp) lives on the AG as cozystack.io/source-watcher-* annotations so it survives operator restarts. Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io> |
||
|
Ivan Okhotnikov
|
88d62612fe |
docs(operator): correct meta.SetStatusCondition rewrite semantics in comment
review-branch skill flagged that the comment claimed meta.SetStatusCondition rewrites LastTransitionTime on Status OR Reason change. Actually only Status change triggers a LastTransitionTime rewrite — Reason and Message are updated in place, LastTransitionTime is preserved. Outcome (repeated identical calls are no-op-y on persisted state) is unchanged; just tightening the technical claim to match apimachinery. Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io> |
||
|
Ivan Okhotnikov
|
122584045b |
docs(operator): link workaround to fluxcd/pkg#934 (existing upstream issue)
Reviewer nit was to file an upstream issue so the workaround does not silently outlive its cause. Checked first — fluxcd/pkg already has one open: #934 \"patch.Helper creates race conditions in downstream controllers\" (May 2025). Same root cause we traced: patch.Helper writes .status and .status.conditions as separate apiserver requests, so an etcd blip between the two leaves the object half-persisted. The upstream issue is broader than just source-watcher — every fluxcd downstream controller consuming fluxcd/pkg's runtime patch helper is affected. Update the TODO(remove) marker on our workaround to point at that issue and note that both source-watcher v2.1.0 and v2.2.x (flux-aio latest) pull the same fluxcd/pkg runtime, so bumping source-watcher alone does not remove the need for this synthesis. Compile + unit tests unchanged. Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io> |
||
|
Ivan Okhotnikov
|
2fcf0bf5be |
fix(operator): document PackageSource Ready contract change + address review nits
kvaps and lexfrei confirmed the diagnosis by tracing source-watcher
v2.1.0 (same code paths in v2.2.x / flux-aio latest, so a simple
version bump does not remove the need for this workaround):
* fluxcd's patch.Helper patches .status and .status.conditions as
SEPARATE apiserver requests, so an etcd timeout during first-
install load can land between them — leaving Inventory /
ObservedSourcesDigest persisted while the Ready-condition write
is lost.
* On the next reconcile the !hasDrifted branch returns before
touching conditions. The deferred summarizeStatus still runs and
adjusts the Reconciling condition, but it never promotes a
stuck Unknown Ready to True — that promotion only happens on the
successful hasDrifted path.
Two substantive changes prompted by the reviews:
1. Document the semantic change the workaround introduces on
PackageSource.status.conditions[Ready]. fluxcd/pkg/runtime/
conditions.Set writes condition.ObservedGeneration =
ag.Generation on EVERY condition mutation including the
Progressing/Unknown mark at the start of a rebuild. And
ArtifactGeneratorStatus does not expose an object-level
ObservedGeneration (only ReconcileRequestStatus is inlined).
So Generation-matching cannot distinguish 'current revision
fully processed' from 'rebuild in progress, previous
Inventory/digest still persisted' — meaning the predicate can
(and does) fire during a content-only regeneration (new OCI
revision, same .metadata.generation).
Net effect: PackageSource Ready no longer dips through Unknown
while a new revision is being processed; it now means 'valid
consumable artifacts exist' rather than 'the current revision
has been processed'. Kvaps flagged this as the better contract
for install/upgrade ordering (no InProgress flapping, and a
genuine regeneration failure still surfaces as Ready=False on
the successful hasDrifted path). Document it inline instead of
implying the workaround only fires in the stuck-status window.
2. Reworked the predicate signature to take the pre-resolved
Ready condition as a parameter. The caller was already doing
the FindStatusCondition lookup for the pass-through path, so
the helper no longer duplicates it (kvaps' nit).
Also refreshed the pre-existing comments:
* The 'early-exits before touching status again' step is now
correctly worded as 'returns before touching conditions again;
the deferred summarizeStatus never promotes a stuck Unknown
Ready to True'.
* Bug attribution now names the code paths ('!hasDrifted branch',
'summarizeStatus deferred'), notes that v2.1.0 and v2.2.x
('flux-aio latest') both ship the affected code, and carries a
TODO(remove once upstream is fixed) marker so the workaround
cannot silently outlive its cause.
Tests: existing 8 subtests updated to construct the ready condition
manually so the call matches the new signature, plus one new
subtest locking in the intentional 'content-only regeneration
predicate fires' behaviour so a future refactor that tightens the
predicate back to 'current revision processed' — which would
re-introduce the flapping — fails loudly. 9/9 pass.
Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io>
|
||
|
Ivan Okhotnikov
|
a03e879316 |
fix(operator): require Ready.ObservedGeneration == ag.Generation before synthesising
Own-audit found the first pass had a spec-update race: if a user
edited PackageSource.spec.variants on a running cluster, cozystack
would bump the derived ArtifactGenerator to Generation=2, and
between "source-watcher started reconciling gen=2" and "source-
watcher wrote new Inventory + ObservedSourcesDigest for gen=2" the
ArtifactGenerator still exposes the previous generation's
Inventory / ObservedSourcesDigest AND Ready=Unknown (Progressing).
The workaround would synthesise Ready=True on stale artifacts, and
downstream Flux would consume the previous-generation
ExternalArtifacts as if the new spec had converged.
Tighten the predicate to require the Ready condition's
ObservedGeneration to match ag.Generation. Only when source-watcher
has actually processed the current spec (which happens strictly
after it writes the current-generation Inventory + Digest) does the
Ready condition carry ObservedGeneration == ag.Generation. That is
exactly the window where Inventory / Digest are guaranteed current-
spec AND only the status patch on Ready itself failed — the actual
upstream bug this workaround exists to paper over.
Consequence: if Ready is missing entirely we return false now
(was true). We cannot verify ObservedGeneration on an absent
condition, and the missing-Ready state is indistinguishable from
mid-reconcile-of-a-newer-generation. On the next reconcile source-
watcher will write at least a Progressing condition, at which point
the current-spec Generation check will be evaluable and the
workaround will kick in if — and only if — Inventory / Digest also
match the current spec.
Tests updated:
* changed 'no Ready condition' case from want=true to want=false
(reflecting the tighter invariant);
* added a case where Ready=Unknown carries an OLD Generation
(want=false — do not synthesise on stale artifacts);
* stamped ObservedGeneration on every existing condition-bearing
case so they exercise the matching-Generation branch;
* introduced a small agWithGen helper to keep the fixtures
readable now that Generation is load-bearing.
All 8 subtests pass; existing operator suite unaffected.
Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io>
|
||
|
Ivan Okhotnikov
|
73e88e8ff9 |
fix(operator): synthesise PackageSource Ready when source-watcher status-patch stalls
Symptom cozystack-platform HelmRelease times out at 15m on fresh install with a single PackageSource stuck in InProgress. Snapshot from #3178 CI run 28587656430: PackageSource cozystack.vm-disk-application Ready: Unknown, reason: Progressing lastTransitionTime: 2026-07-02T13:39:26Z (snapshot generated 30+ minutes later — no state change) ArtifactGenerator cozystack.vm-disk-application status.inventory: 2 items with sha256 digests status.observedSourcesDigest: populated status.conditions[Ready]: Unknown, reason: Progressing status.conditions[Reconciling]: True, reason: Progressing ExternalArtifacts are produced. Downstream Flux consumes them and its HelmReleases install fine. But PackageSourceReconciler faithfully copies the Unknown Ready condition through, so cozystack-platform (which waits on PackageSource.Ready=True) never converges. Root cause Traced in fluxcd/source-watcher:v2.1.0 log for the affected ArtifactGenerator: 13:39:28.475 ExternalArtifact vm-disk reconciled ✅ 13:39:28.641 ExternalArtifact vm-disk-rd reconciled ✅ 13:39:45.719 ❌ failed to update status: etcdserver: request timed out 13:39:45.720 ❌ Reconciler error (etcd timeout) 13:39:46.485 ✅ No drift detected, 2 artifact(s) up to date ——— nothing else for this ArtifactGenerator ——— Under first-install etcd pressure the patchStatus call times out. The reconciler returns the etcd error; controller-runtime prints the "returned both a result and non-nil error, RequeueAfter ignored" warning and requeues. The next reconcile hits the "No drift detected" short-circuit (artifacts are already up to date) and early-exits before touching status. Result: the Ready condition is stuck Unknown until either the process restarts or something forces a state change on the ArtifactGenerator. Neither happens in a real install run. This is an upstream bug in source-watcher's reconcile loop. It has been papered over once already at the runtime-tuning level in #3163 (source-watcher concurrency + CPU), but a bad etcd second during install still lands us here. Cozystack cannot afford to wait for an upstream release cycle to unblock every install. Fix PackageSourceReconciler.updateStatus now checks two invariants before falling back to copying the (potentially stuck) upstream Ready condition: 1. ArtifactGenerator.status.Inventory is non-empty 2. ArtifactGenerator.status.ObservedSourcesDigest is set Both fields are only written by source-watcher after the artifacts have actually been produced — status.Inventory carries the ExternalArtifact digests downstream Flux is already consuming. If BOTH are present AND the upstream Ready condition is still Unknown (or absent), the PackageSource Ready condition is synthesised as True with a distinctive reason `ArtifactsGeneratedAwaitingUpstreamStatus` and a message that names the upstream bug and the observable state it relied on. If the upstream Ready ever catches up (True or False) the Owns() watch re-fires the reconciler and the real condition overwrites the synthetic one — True stays True, False surfaces the real failure. Failures are NEVER masked: the synthesis path only triggers when Ready is Unknown or missing. Safety * Never synthesises True over a real False → upstream failure is surfaced unchanged. * Requires BOTH Inventory non-empty AND ObservedSourcesDigest set → a fresh ArtifactGenerator that has literally not started work cannot be prematurely marked Ready. * ObservedGeneration is stamped so a spec change re-invalidates the synthesis until the next observable-artifacts pass. * Reason string is greppable so operators can audit which PackageSources came up via the workaround. Tests Added TestArtifactGeneratorObservablyReady with 7 cases covering every branch of the predicate: - fresh generator: false - inventory only: false - digest only: false - both set + Unknown Ready: true (the actual bug) - both set + no Ready cond: true (same window pre-stamp) - both set + Ready=True: false (pass real condition) - both set + Ready=False: false (never mask failure) All pass; existing operator unit tests are unaffected. Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io> |
||
|
Aleksei Sviridkin
|
9aa045fc30 |
feat(ci): gate sizeable API changes behind API-owner review (#3167)
## What this PR does Adds a CI gate that requires an approving review from a designated API owner (`lllamnyp` or `kvaps`) when a pull request makes a **sizeable** change to the Cozystack API surface. Sizeable means one of: 1. A new API group. 2. A new resource. 3. A breaking change to an existing resource's schema. 4. Removal of a resource or an entire API group. Detection is fully deterministic — no LLM or network calls, so it runs on every PR. ### How it works - **`cmd/api-gate` + `internal/apigate`** — a small Go tool that loads the API surface from two checkouts (merge base and PR head) and classifies the delta. Every resource's schema is normalized to a parsed OpenAPIv3Schema; the tool only unwraps it from its two container formats and reads identity (group/kind/plural) from the wrapper. | Surface | Files scanned | Schema source | | --- | --- | --- | | Apps (`apps.cozystack.io`) | `packages/system/*-rd/cozyrds/*.yaml` | `spec.application.openAPISchema` | | Typed groups (`cozystack.io`, `backups.*`, `network.*`, `gateway.*`) | every `*.cozystack.io` CRD discovered under a `crds/` or `definitions/` directory in `packages/` and `internal/` | served `spec.versions[].schema.openAPIV3Schema` | | Static Go-backed (`core.*`, `sdn.*`) | `pkg/apiserver/apiserver.go` storage registrations | none — new/removed group / resource only | CRDs are found by location + content rather than a fixed file list, so a first-party CRD that moves (e.g. into a chart's `crds/` dir) stays covered. Only `served` CRD versions count as live API surface, so the standard deprecation step (`served: true → false`) registers as a removed version. An empty snapshot is treated as an error, never a silent pass. - **Breaking classification is semantic, not textual.** Additive changes (new optional field, new enum value, widened type, relaxed or removed constraint, description/default edits) pass. Flagged as breaking: removed fields, type narrowing (including adding a type to a previously-unconstrained field), enum-value removal, newly-required fields, added/tightened `pattern`/min/max/length constraints, `additionalProperties` restricted to `false`, added `x-kubernetes-validations` (CEL) rules, and removed served versions. - **`.github/workflows/api-review-gate.yaml`** builds the tool, diffs the PR head against its true **merge base**, and — when the change is sizeable — requires an `APPROVED` review from an API owner. Only each reviewer's *latest* review *on the current head commit* counts, so a stale or superseded approval no longer satisfies the gate. It re-runs on `pull_request_review` so an approval flips the check without a new push. The token is scoped to the approval step alone (`persist-credentials: false` on checkout) so PR-controlled build steps can't read it. The allowlist lives in one place (`API_OWNERS`). The job runs on every PR (no path filter) so it can safely be marked a required check. ### Follow-up needed after merge Mark **"Require API owner review for sizeable API changes"** as a required status check in `main` branch protection for the gate to block merges. ### Tests Table-driven unit tests cover the schema-diff rules, the classifier (new group / new resource / breaking, including the double-count and pure-removal edge cases), all three parsers, and an on-disk end-to-end snapshot test. Verified against the real repo tree: identical trees pass; injected new-group/new-resource/breaking changes are all caught; an additive-only edit passes. ### Release note ```release-note feat(ci): require a review from an API owner (lllamnyp or kvaps) when a PR introduces a new API group, a new resource, or a breaking change to an existing API schema ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added an API review gate workflow that flags “sizeable” API changes and requires at least one approving API-owner review. * Introduced an API-gate CLI to compare API snapshots, classify changes (new/removed resources and groups, plus breaking schema updates), and generate readable reports. * Enhanced snapshot parsing and deterministic reporting across multiple input sources. * **Tests** * Added comprehensive unit tests covering parsing, snapshot loading, schema diff detection, and classification/report output behavior. * **Chores** * Updated API gate ownership rules for the new workflow and related tooling. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Timofei Larkin
|
dc615664a3 |
fix(ci): flag constraints added to open maps/arrays
childSchemas now recurses into items/additionalProperties whenever head defines a schema, diffing against an empty base when base had none. This catches constraining a previously-unconstrained array element or map value (base absent/true -> schema), while still treating additionalProperties:false -> schema and schema removal as safe relaxations. Also tighten the first-party group check to cozystack.io and its dot-delimited subdomains so a lookalike like fakecozystack.io is not gated. Both covered by tests. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Timofei Larkin <lllamnyp@gmail.com> |
||
|
Timofei Larkin
|
f3e55d2be4 |
fix(ci): harden API gate against tampering and schema gaps
Scope the gate's own files (cmd/api-gate, internal/apigate, the workflow) to the API owners in CODEOWNERS, and compile the detector from the trusted base checkout when present (head fallback only during bootstrap), so a PR cannot neuter the gate by editing its own detector code. Detect two more breaking vectors: removing nullable:true (rejects null) and adding a oneOf/allOf/not composition constraint. Both covered by regression tests. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Timofei Larkin <lllamnyp@gmail.com> |
||
|
Timofei Larkin
|
dec840e938 |
fix(ci): address review of the API gate
Fix schema-diff correctness: guard nil head on removed child schemas, flag adding a type to a previously-unconstrained field, stop reporting a head becoming unconstrained as narrowing, detect additionalProperties:false, and only recurse items/additionalProperties when both sides carry a schema. Gate resource and group removal as new categories. Filter CRD versions to served ones so the deprecation flow surfaces as a removed version, and flag added x-kubernetes-validations (CEL) rules. Discover first-party cozystack.io CRDs by walking crds/ and definitions/ dirs so a relocated CRD (e.g. cozystack-scheduler) is covered, and error on an empty snapshot. Surface unmapped apiserver storage prefixes and allow hyphens in plurals; drop Markdown from text-mode reports. Workflow: diff against the true merge base, count only each reviewer's latest review on the current head commit, and scope the token to the approval step with persist-credentials:false. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Timofei Larkin <lllamnyp@gmail.com> |
||
|
Timofei Larkin
|
fa871249c6 |
feat(ci): gate sizeable API changes behind API-owner review
Add a deterministic api-gate tool (cmd/api-gate, internal/apigate) and a workflow that require an approving review from an API owner (lllamnyp or kvaps) when a PR introduces a new API group, a new resource, or a breaking schema change to an existing resource. Detection diffs the checked-in OpenAPIv3 schemas (apps cozyrds + CRD manifests) and the apiserver storage registrations; no network or LLM calls. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Timofei Larkin <lllamnyp@gmail.com> |
||
|
Aleksei Sviridkin
|
c841c70866 |
feat(operator): replicate operator wildcard certificate to tenant namespaces
The operator-provided wildcard MVP serves the root tenant only: the root ingress controller and Gateway read a TLS Secret from their own namespace. Child tenants run their own ingress controller / Gateway in a separate, default-deny namespace and cannot read that Secret cross-namespace. Add a controller that mirrors the operator wildcard Secret into every tenant namespace that owns a TLS termination point (its namespace.cozystack.io/ingress or .../gateway label equals its own name). The source is identified by name from the platform values channel (_cluster.wildcard-secret-name) with no extra operator input; replicas carry a managed-by label and refresh on rotation. An individual replica is removed when its namespace stops terminating TLS; every replica is torn down only on an explicit disable (clearing wildcard-secret-name). A missing or mistyped source, or a transiently-absent values channel, keeps existing replicas, so tenant TLS never drops on a transient gap. The controller's Secret cache is scoped to managed replicas and the values channel, not every cluster Secret. Only the platform controller's ServiceAccount gains Secret writes — no tenant RBAC is widened, and every consumer reads only its own-namespace copy. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |