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>
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>
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>
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>
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>
## 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 -->
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>
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>
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>
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>
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>
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>
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>
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>
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>
## 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 -->
## 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 -->
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>
Add the serviceexposure controller to the cozystack-controller binary. It
resolves a ServiceExposure's ExposureClass to a backend and renders the
backend-specific pool and announcer CRs, then reports the assigned IPs in
status.
Backends are pluggable behind a small interface, one per file:
- externalIPs: renders nothing (the node owns the address) — preserves the
historical default behaviour.
- metallb: IPAddressPool + L2Advertisement in cozy-metallb, scoped to the
target Service's namespace.
- cilium: CiliumLoadBalancerIPPool (cilium.io/v2) + CiliumL2AnnouncementPolicy
(cilium.io/v2alpha1), scoped to the Service via serviceSelector.
- robotlb: renders nothing (the cloud LB owns allocation and announcement).
The rendered CRs belong to foreign API groups not in the Go scheme, so they
are emitted as unstructured objects. They carry no OwnerReference (the
Cilium CRs are cluster-scoped and the MetalLB CRs live in a different
namespace than the owning ServiceExposure), so ownership is tracked via a
management label plus owner annotations, cleanup is driven by a finalizer,
and the controller refuses to take over an identically-named resource it
does not manage. The controller never mutates the target Service, leaving
its shape to the chart that renders it.
Grant the controller write access to metallb.io and cilium.io pool /
announcer resources.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Add a controller that keeps each SecurityGroup's membership label
(securitygroup.sdn.cozystack.io/<name>) in sync with its attachments: it
stamps the label onto the pods of every attached application and removes it
when an application is detached or the SecurityGroup is deleted. The backing
CiliumNetworkPolicy's endpointSelector matches that label, so this is what
makes a SecurityGroup apply to its members.
The controller watches the marked backing policies and managed-app pods. It
resolves each attachment through the lineage labels the lineage webhook
already stamps — never the attachment list directly — and only ever labels
pods in the SecurityGroup's own namespace, using single-key merge patches so
it cannot clobber another group's label or a pod's lineage labels. A finalizer
on the backing policy guarantees the labels are stripped before the policy is
removed. Informer caches are bounded to managed pods and SecurityGroup-owned
policies.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
## What this PR does
Implements the tenant Flux sharding design from #2817: a new
`flux-shard-operator` system package that spreads tenant HelmReleases
across multiple helm-controller shards, so a noisy tenant (e.g. a
HelmRelease stuck in infinite remediation) cannot degrade the others.
- **Placement controller** owns the tenant→shard assignment: the unit of
placement is the tenant (all of its HelmReleases share one shard label),
distribution is greedy least-loaded weighted by HelmRelease count, with
pinned tenants, threshold-gated rebalance, paced moves, and never moving
deleting tenants. Watches are metadata-only, so the controller does not
decode the helm-controller status-patch firehose. Assignments are
recorded on tenant namespaces; HelmRelease labels remain the source of
truth on restarts.
- **Shard runtime** is provisioned by the operator: `shardCount`
helm-controller Deployments cloned from the flux-aio `flux` Deployment
and sanitised, so the image and feature-gates stay version-synced
automatically. flux-aio itself is unchanged and keeps owning everything
without a shard key.
- **CREATE-only mutating webhook** (`failurePolicy: Ignore`) stamps the
tenant's shard onto every HelmRelease at admission, so each one is born
on the correct shard regardless of creation path; any miss degrades
gracefully to the placement controller.
- The hand-rolled `flux-tenants` Deployment is retired: the operator
drains it (backfill `tenants` → `shard<i>`) and deletes it once empty;
migration 44 removes a leftover only under the same guard.
Ships with `shardCount: auto` (the default): the operator sizes the
shard count from the tenant HelmRelease count — `K = clamp(ceil(H/100),
1, min(16, T))` with hysteresis (scale up past 120 HR/shard, down under
60) — so small clusters stay at one shard (≈ today's behaviour) and
large fleets shard out automatically. An integer value pins the count
explicitly.
One deviation from the design doc: the webhook/controller scope is the
`sharding.fluxcd.io/key` label rather than
`internal.cozystack.io/tenantmodule` — every tenant HelmRelease is born
with `key=tenants` (stamped by ApplicationDefinition `release.labels`
and chart templates), so the webhook rewrites it and the placement
controller continuously relabels stragglers; `tenantmodule` covers only
the 6 tenant chart modules.
### Screenshots
Not a UI change.
### Release note
```release-note
feat(fluxcd): tenant HelmReleases are now reconciled by operator-managed helm-controller shards; the flux-shard-operator balances tenants across `shardCount` instances and retires the legacy flux-tenants deployment
```
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **New Features**
* Introduced **flux-shard-operator** to shard helm-controller workloads
with deterministic tenant placement, optional rebalance controls, and
autosized shard count.
* Added a **CREATE-only mutating webhook** to stamp the correct shard
label on tenant HelmReleases.
* **Bug Fixes**
* Application updates now preserve existing **flux-shard** label
assignments to prevent unwanted shard re-stamping.
* **Chores**
* Added platform packaging for the new component, including an image
build step.
* Removed the legacy tenants deployment and added a migration to retire
it safely.
* **Tests**
* Expanded unit and chart tests for placement, provisioning, webhook
behavior, and configuration parsing.
* **Documentation**
* Added operator documentation describing configuration and metrics.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Resolve go.mod conflict: keep main's go 1.26.4 and golang.org/x/* bumps, and keep gomodules.xyz/jsonpatch/v2 v2.4.0 as a direct dependency (imported by the flux-shard-operator admission webhook) instead of main's indirect entry. go mod tidy leaves go.sum consistent.
Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
BuildShardDeployment clones the flux-aio pod spec, which carries a required podAntiAffinity on app.kubernetes.io/name=flux. Inherited onto a shard pod it forbids scheduling on the flux-aio node, so on a single schedulable node every shard stays Pending and the tenant plane stalls (no ready shard -> assignments deferred -> HelmReleases stuck on key=tenants).
Drop the podAntiAffinity during sanitisation while keeping any benign nodeAffinity (e.g. os=linux). The fixture now carries flux-aio's affinity and TestBuildShardDeployment asserts the strip.
Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
The XValidation justification was factually wrong: the deployed
helm-controller CRD has no rule rejecting a Strategy alongside a
Remediation entry (only .retryInterval is gated on Strategy.Name), so
the claimed "silently break every HR" safety net does not exist.
Reword the comment in all three test files to the real reason
Remediation stays nil: retries are driven by a single
Strategy.RetryInterval path, matching cozystack-operator's
PackageReconciler.buildHelmReleaseSpec.
Address review feedback from lexfrei on rest_helmrelease_spec_test.go,
rest_timeout_test.go and internal/operator/package_reconciler_test.go.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Replacing the whole resources block dropped cloned values the override
did not name (e.g. the inherited cpu limit when only a memory limit is
configured), contradicting the documented inherit-when-empty semantics.
Also document the intentional provisioner-vs-placement sizing
asymmetry.
Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io>
ComputePlacement ignores pins to non-existent shards, but rebalance
treated any pinned tenant as sticky, so an invalid pin blocked expected
moves. Honor only pins placement itself honors.
Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io>
A failed patch no longer refreshes the rebalance cooldown or inflates
the moves counter, so retries are not delayed and not double-counted.
Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io>
The Update handler rebuilds the HelmRelease from the Application, which
re-stamped sharding.fluxcd.io/key with the ApplicationDefinition default
and bounced the HelmRelease off its operator-assigned shard on every
update. Carry the live label over from the current HelmRelease instead.
Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io>
Operators who already hold a wildcard TLS certificate (purchased, or
issued by a corporate CA) had no way to make a tenant Gateway serve
under it: the only cert modes were http01 (per-listener ACME) and
dns01 (controller-minted wildcard via ACME).
Add a third mode, existingSecret, that references a pre-existing TLS
Secret in the TenantGateway's own namespace via spec.wildcardSecretRef.
In this mode the controller mints no ACME Issuer and no Certificate;
the wildcard, apex, and per-child-apex HTTPS listeners reference the
supplied Secret directly. A missing wildcardSecretRef fails fast with
Ready=False rather than producing a Gateway pointing at a nonexistent
Secret. Switching into existingSecret from http01/dns01 reclaims the
now-unused Issuer and Certificates so no ACME machinery is left behind.
The non-cert listeners (http->https redirect, TLS passthrough) are
unaffected and continue to render.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
myasnikovdaniil