mirror of
https://github.com/cozystack/cozystack.git
synced 2026-11-02 18:16:53 +09:00
648eb42cae3f12bd367102048fa89089fc40407a
297
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 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> |
||
|
Aleksei Sviridkin
|
41ff94eca9 |
fix(flux): tune source-watcher concurrency and CPU under load
source-watcher reconciles one ArtifactGenerator per PackageSource and builds the resulting ExternalArtifacts. On the management cluster that is ~100 artifact builds during install, and the platform HelmRelease blocks until all of them are Ready. It was the only Flux controller left untuned. In the bootstrap all-in-one pod it ran without an explicit --concurrent (binary default 10) and with the shared 100m CPU request; in the production FluxInstance it was missing from the --concurrent / CPU patch that source-, kustomize- and helm-controller already receive. Under load its per-artifact builds finish at staggered times, leaving a random tail of not-yet-Ready artifacts that times out the platform install. Raise source-watcher to --concurrent=20 in both deployments. In the bootstrap all-in-one pod also give it a 500m CPU request; the pod has no CPU limits, so the request is what drives its CPU share under contention. In the production FluxInstance add it to the shared reconciler patch, so it now receives the same --requeue-dependency=5s and 2000m/2048Mi limits the other tuned controllers already have. The flux-aio module exposes a resources override for the watcher (set via CUE) but no args override, so the bootstrap --concurrent bump is a post-render patch in the Makefile. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Алексей Артамонов
|
184324430f |
fix(controller): don't clamp pool resources the parent budget leaves unbounded
ComputePools derived a pool's Available with SubtractWithNonNegativeResult over the union of the budget and carve-out keys, so a bounded child carving a resource its parent does not bound surfaced that resource as 0 in the parent pool's Available. The controller then wrote a tenant-quota-allocated clamping the parent namespace to zero of a resource it never limited, blocking its workloads (observed on a cluster: a memory-bounded child froze its cpu-only parent's memory). Mask Available to the budget's resource names so only resources the pool root actually bounds are enforced; mirrors the fix already applied to Pool.Overcommitted. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Алексей Артамонов <alexeyartamonov1987@gmail.com> |
||
|
Алексей Артамонов
|
b0556cc753 |
fix(controller): ignore unbounded resources in tenant quota overcommit check
Pool.Overcommitted reported a resource as overcommitted whenever a sub-tenant carved out a resource the pool root does not bound. Admission explicitly allows a child to bound a resource its parent leaves unbounded, and an unbounded budget can never be overcommitted, so this produced a spurious QuotaOvercommitted warning event on every reconcile. Restrict the check to resources present in the pool budget. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Алексей Артамонов <alexeyartamonov1987@gmail.com> |
||
|
Andrei KvapilandClaude
|
55f1edda78 |
fix(controller): read tenant budget from rendered quota, skip inert pools
Take a tenant's declared budget from its chart-rendered tenant-quota ResourceQuota (.spec.hard) instead of the raw tenant.spec.resourceQuotas values. The chart expands the shorthand values into real quota resource names and applies the cluster's request/limit allocation ratios, so reading the rendered object keeps the controller-owned allocated quota in exactly the same key space — the two quotas compose correctly and the controller never replicates that cluster-configurable transformation. Also skip pools that need no tightening beyond the chart quota (a lone tenant with no sub-tenants and no carve-outs): the controller writes an allocated quota only when a budget is actually shared or carved up, so a single-tenant namespace is left exactly as the chart renders it. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io> |
||
|
Andrei KvapilandClaude
|
844deb170f |
feat(controller): enforce recursive tenant quotas at runtime
Add a tenant quota reconciler to cozystack-controller that realises the hierarchical (sub-tree) quota model at runtime. Each tenant's declared quota is the budget for its whole sub-tree: a child that declares its own quota carves a fixed slice out of the parent budget, while a child without a quota shares the parent's remaining pool. The controller maintains a per-namespace 'tenant-quota-allocated' ResourceQuota (Kubernetes enforces the most restrictive quota, so it binds without fighting Flux over the chart-rendered 'tenant-quota'), aggregating sub-tree usage so a pool's members collectively stay within the budget. A --tenant-quota-buffer-percent flag temporarily inflates pool budgets so workloads already over a freshly-introduced quota keep running during rollout; overcommitted pools emit a QuotaOvercommitted event. The model and the selector-spans-namespaces/aggregated-usage approach are adapted from OpenShift's ClusterResourceQuota controllers (openshift/api, library-go, cluster-policy-controller, apiserver-library-go), Apache-2.0, Copyright Red Hat, Inc. and the OpenShift Authors. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io> |
||
|
Aleksei Sviridkin
|
bf14a73b39 |
feat(api): add SecurityGroup network policy resource (sdn.cozystack.io) (#2922)
## What this PR does
Introduces a tenant-facing `SecurityGroup` resource in a new
`sdn.cozystack.io/v1alpha1` API group, served by the Cozystack
aggregated API server as a 1:1 projection of a `CiliumNetworkPolicy`. A
SecurityGroup attaches to one of the tenant's managed applications by
reference (`spec.targetRef: {apiGroup, kind, name}`) and declares
allow-list ingress/egress for that application. Tenants manage network
policy for their own apps entirely through
`securitygroups.sdn.cozystack.io`, without being granted any access to
the `cilium.io` API group.
The translation is synchronous and stateless — the API server converts
each SecurityGroup into a CiliumNetworkPolicy in the same namespace on
write and back on read; there is no controller. The backing policy's
`endpointSelector` is **derived** from `targetRef` (matched on the
application's lineage labels
`apps.cozystack.io/application.{group,kind,name}`), never copied from
tenant input, so the selector is machine-generated and can only ever
match the referenced application's own pods in the tenant's namespace.
Policies owned by this API carry an `sdn.cozystack.io/securitygroup`
marker label that the storage always re-asserts and hides from the view,
so it only ever surfaces or mutates its own policies and never touches
platform-managed network policies.
The value is self-service network policy on a Cozystack-native,
RBAC-scoped surface, without exposing Cilium's version-coupled CRDs to
tenants. Because the selector is derived from an authorized application
reference rather than free-form tenant input, the RBAC boundary is
structural, not selector-validated: a tenant cannot express network
policy against arbitrary or platform-owned pods. The full design —
selector derivation from lineage labels, the structural boundary, the
additive-policy reality (a SecurityGroup widens a managed application's
allowed traffic but cannot sever its platform-managed management plane,
and equally cannot tighten below the platform baseline), the in-tree
CiliumNetworkPolicy mirror that avoids a Cilium Go-module dependency,
RBAC, and deferred extensions (raw-pod targeting, cluster scope,
reusable groups) — is in `pkg/apis/sdn/DESIGN.md`.
Implements the attachment model from #1614.
### Release note
```release-note
feat(api): add the SecurityGroup resource (sdn.cozystack.io/v1alpha1) — a tenant-facing, namespace-scoped firewall that attaches to a managed application by reference and projects 1:1 onto a CiliumNetworkPolicy with a machine-derived endpoint selector, letting tenants manage their application's network policy without cilium.io access.
```
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **New Features**
* Added the `SecurityGroup` API resource (`sdn.cozystack.io` v1alpha1)
with a namespaced projection backed by Cilium network policies.
* Enabled the SDN `APIService` and wired SDN routing, scheme, and REST
support.
* Expanded RBAC to include SecurityGroup access and required Cilium
policy permissions.
* **Documentation**
* Added a design document describing projection behavior and rule
semantics.
* **Bug Fixes**
* Improved API rule validation coverage for missing list-type fields.
* **Tests**
* Added SDN round-trip, fuzzing, RBAC, and comprehensive REST
create/get/list/update/delete/watch/resource-version tests.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
|
||
|
Aleksei Sviridkin
|
31c15b282f |
feat(network): mechanism-agnostic LoadBalancer exposure (ExposureClass + ServiceExposure) (#3081)
## What this PR does Lays the foundation for migrating tenant LoadBalancer exposure off the deprecated `Service.spec.externalIPs` (KEP-5707) and, eventually, off MetalLB onto Cilium-native announcement — the mechanism-agnostic shape prescribed when the earlier chart-side approach (#2468) was reverted in #2603. It adds a new `network.cozystack.io/v1alpha1` API group and a controller that translates intent into whichever LoadBalancer backend the cluster admin configured: - `ExposureClass` (cluster-scoped, admin): binds a logical class name to a backend (`externalIPs` | `metallb` | `cilium` | `robotlb`) and its VIP address scope — the StorageClass/IngressClass pattern. - `ServiceExposure` (namespaced, tenant intent): "expose this Service via class X"; status reports the resolved backend and assigned IPs. - A `serviceexposure` controller renders the backend's pool + announcer once per class (MetalLB `IPAddressPool` + `L2Advertisement`; Cilium `CiliumLoadBalancerIPPool` + `CiliumL2AnnouncementPolicy`), scoped to the namespaces that opted in. Backends are pluggable behind a small Go interface, so adding BGP or another cloud LB is one implementation, not an API change. The controller never mutates the target Service — the chart still owns its shape. Platform wiring: `publishing.exposureClass.*` renders the admin `ExposureClass` and the host ingress publishes through it. The default (no class set, `externalIPs`) leaves every existing cluster's behaviour byte-for-byte unchanged; the MetalLB→Cilium announcer migration becomes a single `ExposureClass.spec.backend` change. This is the foundation only — Part of #2586. Converting per-app exposure, flipping Cilium `l2announcements` on, changing the default, and removing MetalLB are deferred to later phases. ### Release note ```release-note feat(network): add ExposureClass and ServiceExposure for mechanism-agnostic LoadBalancer exposure. Opt-in foundation; the default externalIPs publishing path is unchanged. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added new `ExposureClass` and `ServiceExposure` custom resources with support for multiple backends (externalIPs, MetalLB, Cilium, RobotLB), including optional L2 and address configuration. * Added Helm wiring and templates to enable service/ingress publishing via `ServiceExposure`, selecting a configured (or default) `ExposureClass`. * **Bug Fixes** * Improved reconciliation, finalizer-based cleanup, and status updates for readiness and assigned IP reporting, including safer handling when backends or managed resources change. * **Tests** * Added end-to-end and unit test coverage for backend rendering, readiness, and cleanup flows. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
881591d34d |
fix(securitygroup): log swallowed errors in the membership reconciler
The reconciler intentionally treats a malformed attachments annotation and a failed pod-mapping List as empty / no-op to stay resilient, but it swallowed both errors silently. That hides real defects: a bad annotation writer, or a transient API error that drops a pod update until the next periodic resync with no trace. Log both while preserving the resilient return behavior. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |