Go 1.26's extended new() builtin takes an expression, so new(true) and
new(false) replace ptr.To for the owner reference's Controller and
BlockOwnerDeletion flags. ptr.Deref, which has no builtin equivalent,
stays.
Signed-off-by: Aleksei Sviridkin <f@lex.la>
A key-free Secret already sitting at the canonical trust-anchor name is
left untouched and reported Ready. But tenant visibility comes from the
internal.cozystack.io/tenant-ca label, and an engine-owned Secret at that
name carries none, so the lineage webhook marks it tenantresource=false
and the tenant is locked out while the controller reports "published".
Add the missing label to the canonical-name contract deviations so the
warning names it, and collect all deviations rather than only the first
so a single warning describes every way the object falls short.
Signed-off-by: Aleksei Sviridkin <f@lex.la>
The ApplicationDefinition mapping fans out to every TenantProjection in
the cluster on every definition event, including Flux's periodic no-op
re-apply of the many *-rd definitions. The selectors-digest drift check
already suppresses the resulting writes, but not the load: each enqueued
reconcile still issues several uncached reads.
Gate the watch with a predicate that delivers an update only when
spec.secrets — the field the selectors digest is computed from — changed,
while letting creates and deletes through. Comparing the same field the
digest digests keeps the gate and the digest from drifting apart.
Signed-off-by: Aleksei Sviridkin <f@lex.la>
The package doc claimed there is no withdrawal logic and that a retired
declaration is only ever a deleted sentinel. withdrawProjection has
since become the second mechanism: it deletes a projection when the
sentinel outlives its CACert entry, which owner-reference garbage
collection never reaps because the owner still exists. State both paths.
Signed-off-by: Aleksei Sviridkin <f@lex.la>
An entry with an empty sourceSecretName sent the reconciler to Get a
Secret with an empty name. The apiserver answers that with "resource
name may not be empty" — not NotFound — so it escaped the not-found
branch, was returned from Reconcile, and wedged the sentinel on
exponential backoff. The error path returns before any status write, so
no Ready condition was ever recorded: the silent retry the sentinel was
introduced to make visible.
Pin sourceSecretName to MinLength=1 so a live apiserver rejects it at
admission, and guard the reconcile so an object admitted by an older CRD
resolves to Ready=False/SourceInvalid instead of an error.
Signed-off-by: Aleksei Sviridkin <f@lex.la>
withdrawProjection recognised its projection by an exact owner-reference
match while adoption uses a name match, so the two disagreed about what
counts as "ours" — and withdrawal was the stricter, which is backwards:
"safe to overwrite" is a stronger claim than "safe to delete".
Two projections a name match adopts and rewrites were read as foreign
by the exact match and left in place: one carrying a previous
incarnation's UID (delete-and-recreate under the same name), which
owner-reference garbage collection never reaps once the CACert entry is
gone, so the retired anchor stayed tenant-readable forever; and one
whose BlockOwnerDeletion flag had drifted, the very drift the adoption
path normalizes. Decide ownership with isOurProjection so both paths
answer the question the same way.
Signed-off-by: Aleksei Sviridkin <f@lex.la>
The selectors digest kept only the first 8 bytes of the SHA-256. A collision
skips the re-admission the doc calls load-bearing — the revocation direction,
where a tenant would silently keep read access after a definition stopped
selecting the anchor — and the annotation has no length limit, so store the full
hash. Also correct the withdraw-branch comment: MinItems=1 prevents a
single-entry sentinel from emptying its list, so the branch is reached when a
future non-CACert entry replaces the CACert one, not by gating a lone entry off.
Signed-off-by: Aleksei Sviridkin <f@lex.la>
The two-sentinel refusal returned without a RequeueAfter. NoRelease and
MultipleCACert can do that because their fix edits the same sentinel, which the
For() watch delivers; a contest is cleared by deleting the SIBLING sentinel, and
nothing enqueues this one when a sibling changes. So after an operator removed
the duplicate, the surviving sentinel withheld its anchor until the informer's
global resync — hours. Return RequeueAfter: resyncInterval on the contest branch
so recovery rides the resync backstop, and clarify that an anchor an earlier
uncontested reconcile already published is left in place rather than withdrawn.
Signed-off-by: Aleksei Sviridkin <f@lex.la>
The projection carries two annotations whose keys still named a publish-ca-cert
discovery leg that no longer exists in the tree. Rename them to
internal.cozystack.io/ca-cert-source and internal.cozystack.io/ca-cert-selectors
while the API is unshipped; both stay functional (the selectors digest drives
the re-admission drift check, the source ref is a drift input and a traceability
record).
Resolving the ApplicationDefinition matched on Kind alone from a List of
unspecified order. Gate the match on the release's group label too, so a release
stamped with an unexpected group cannot resolve a same-kind definition by
accident.
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Two gaps in the sentinel-to-canonical-name lifecycle:
Withdrawal — the reconciler published each declared entry but never deleted
one. A sentinel that keeps existing with its CACert entry removed (a chart
gating it on tls.enabled, say) left the old <release>.tenant-ca served forever,
because owner-reference GC only fires when the whole sentinel is pruned. A
sentinel that reconciles with zero CACert entries now deletes the projection at
the canonical name, but only when it still carries that sentinel's owner
reference, so a foreign Secret or another sentinel's projection is never touched.
Contention — two sentinels in one namespace with the same release label both
resolve to the single canonical name. Whichever reconciled first would publish
an arbitrary CA and the two could flap ownership of one projection. Refuse both,
mirroring the more-than-one-CACert-entry refusal, so nothing is published until
the declaration lives on exactly one sentinel.
The ClusterRole comments claimed the reconciler never deletes a Secret; corrected
to describe the owner-scoped withdrawal delete.
Signed-off-by: Aleksei Sviridkin <f@lex.la>
The only watches on the projection Secret were the source-Secret index (keyed on
spec.projections.sourceSecretName) and the sentinel itself. The projection is
named <release>.tenant-ca, which is no sentinel's source, so an in-place rewrite
of its ca.crt was corrected only by the five-minute resync — a window in which a
forged trust anchor is served.
Map the projection back to its owning sentinel through the controller owner
reference it carries, on the same dedicated metadata cache the source watch uses.
Owns(&corev1.Secret{}) would register against the manager cache, whose Secret
informer is label-scoped to the WildcardSecret replicas, so a projection carrying
no wildcard label would never be delivered — it would compile and pass a fake
client yet never fire in a real cluster.
Signed-off-by: Aleksei Sviridkin <f@lex.la>
isOurProjection gated adoption on the ManagedLabel marker plus the Opaque
type. The marker is an ordinary label any Secret writer in the namespace can
strip: stripping it off the genuine projection made the controller disown its
own object, refuse it as a collision, and keep serving whatever bytes the same
actor wrote under ca.crt — a trust anchor stuck forged and never self-healing.
Key adoption on the owner reference back to this sentinel instead, matched by
name so a delete-and-recreate under the same name is still re-homed. Forging
the reference only causes an adopt-and-overwrite with the genuine key-free CA,
the self-healing direction; a Secret with no such reference is a stranger and
is refused as a collision, never overwritten. The Opaque type stays as a cheap
sanity guard but is no longer load-bearing — the update path is a
read-modify-write that never writes the immutable Type.
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Rewrite the cacert unit suite for the TenantProjection model: the
happy-path projection, the Ready=False/SourceNotFound status a missing
source now surfaces, ownership solely by the sentinel, the write-path
guards driven through a real source (private key and non-certificate
refused), the canonical-name cases, and the forged-projection heal. The
pure write-path guard tests — including the DER-reassembly guarantee —
carry over unchanged.
Replace the cacert chainsaw suite: it probes the writer policy against
the live API server (RBAC denies a tenant, the VAP denies a
non-flux writer, flux is allowed) and drives the controller end to end
(a flux-created sentinel reports SourceNotFound, then lifts a real CA
into a key-free, sentinel-owned projection). Drop the declared-leg
wiring test.
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Drive the CA-extraction reconciler off the namespaced TenantProjection
sentinel a chart renders, replacing the publish-ca-cert label leg and
the cluster-scoped spec.caCert declaration leg. The sentinel names the
source Secret and key; the projected trust anchor takes an owner
reference to the sentinel, so the lineage webhook resolves the tenant
through sentinel -> HelmRelease and garbage collection prunes the
projection when the chart stops rendering the sentinel.
The source Secret is watched as metadata only, so the informer holds no
private key material, and the one source actually read goes through the
uncached reader. The sentinel's Ready status makes a missing or
unpopulated source visible with kubectl instead of a silent requeue.
The write-path guard, canonical-name contract, adoption rules and
selectors-digest re-admission are unchanged.
Signed-off-by: Aleksei Sviridkin <f@lex.la>
releaseOwnerRef pins BlockOwnerDeletion to false so a trust anchor never
holds up the teardown of the application it belongs to, but the ownership
drift check compared every owner-reference field except that one. A sole
owner reference whose only drift was BlockOwnerDeletion flipped to true
therefore passed as already-correct and was never re-homed, leaving a
projection that blocks a foreground deletion of its HelmRelease — the
opposite of what the flag is set for.
Include BlockOwnerDeletion in the owner comparison so the write path
replaces a drifted reference and restores the flag to false.
Signed-off-by: Aleksei Sviridkin <f@lex.la>
The write-path guard validated the CA value as a certificate chain but
then published a clone of the original input. pem.Decode silently skips
bytes before the first PEM block and between blocks, and the private-key
header guard only matches a "-----BEGIN ... PRIVATE KEY-----" line, so a
raw DER or JWK private key placed ahead of or between real certificates
passes both checks and is copied verbatim into the tenant-readable
projection.
Assemble the projection from the re-encoded certificate DER instead of
cloning the input, so preamble and interstitial key material are
structurally unreachable. The empty-value, wrong-type, non-parseable and
trailing-byte rejections are unchanged.
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Declare CNPG's operator-created CA Secret as the postgres trust-anchor
source (spec.caCert names {{ .release }}-ca), and add the generic
internal.cozystack.io/tenant-ca selector to spec.secrets so the key-free
<release>.tenant-ca projection the extraction controller publishes becomes
readable by the tenant.
postgres is the first converged engine, and the hardest: CNPG creates the
CA Secret itself, key-bearing, asynchronously, and offers no way to label
it, so it exercises the name-driven leg end to end. The test drives the
shipped ApplicationDefinition rather than a fixture, so the wiring and the
controller cannot drift apart unseen: a CNPG-shaped key-bearing
<release>-ca appears, and the projection must be produced key-free AND
selected for tenant visibility by the shipped selector.
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Every engine in the catalog issues a per-release CA, but the object that
holds ca.crt almost always holds a private key next to it, so any RBAC
path that hands a tenant the trust anchor hands over key material too.
Add a controller that publishes, for each application release, a
canonical Opaque Secret <release>.tenant-ca holding only ca.crt. That
name is claimed by none of the operators the platform ships, which
matters: <release>-ca-cert, the obvious candidate, is already taken and
means opposite things across engines — a key-free CA on one, a
key-BEARING Secret on another — so projecting onto it could only be done
by overwriting live key material.
The certificate is found two ways. A Secret labelled
internal.cozystack.io/publish-ca-cert is how a chart opts in, and it is
watched, so a rotation propagates at once. For the engines whose operator
owns the CA Secret and will not label it, the application's caCert
declaration names that Secret, which is read by name and re-read on a
resync. The two legs differ only in how the source is found — extraction,
sanitization, ownership and collision handling are one shared write path,
because forking it is where a key-leak bug would hide.
Exactly one whitelisted key is ever copied, and every byte written is
re-checked for a PEM private-key header first: on the name-driven leg the
source is key-bearing by construction. The projection is owner-referenced
to the release, so deleting the application collects it with no prune
logic, and a Secret the controller did not create is never overwritten —
a Secret carrying key material on the canonical name is refused with a
Warning Event rather than adopted.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
## 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 -->
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>
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>
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>
## 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 -->
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>
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>
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>
## 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 -->
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>
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>
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>
## 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 -->
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
Aleksei Sviridkin