Give each SecurityGroup its own membership label
(securitygroup.sdn.cozystack.io/<name>) and select it from the backing
CiliumNetworkPolicy, instead of deriving the endpointSelector from a single
app's lineage labels. spec.targetRef becomes spec.attachments, so one
SecurityGroup can apply to several applications at once.
Peers move from free-form fromEndpoints/toEndpoints selectors to
fromApp/toApp (resolved to lineage labels) and fromSG/toSG (resolved to
another group's membership label, a reference the Cilium dataplane resolves
live). Attachments are persisted in a storage-owned CiliumNetworkPolicy
annotation, round-tripped and hidden from the SecurityGroup view like the
marker label. Validation now covers attachments and app/SG peers and rejects
names that collide with reserved Cilium entities.
The membership labels are maintained by a separate controller added in a
follow-up commit; this change covers the API types and the
aggregated-apiserver projection only.
BREAKING CHANGE: SecurityGroup spec.targetRef is replaced by
spec.attachments, and ingress/egress peers use fromApp/fromSG/toApp/toSG
instead of fromEndpoints/toEndpoints. The feature is unreleased, so no
migration is required.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
The access-check helpers returned plain fmt.Errorf values, which the
apiserver coerces to 500 InternalError regardless of the actual failure.
A request without user identity now surfaces as 401 Unauthorized, and a
RoleBinding list failure as 500 InternalError with a typed Status body.
Get also repackages a NotFound from the backing Namespace lookup so the
Status body reports the tenantnamespaces resource instead of namespaces.
The Watch goroutine already logs access-check failures and skips the
event, so no untyped error escapes to the client from that path.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
- talos-reconcile-job: emit a `---` separator inside the nodeGroups
range so a tenant with multiple node groups renders valid
multi-document YAML instead of two Jobs concatenated into one doc
- talos-reconcile + bootstrap-token jobs: add a restricted
securityContext (drop ALL caps, no privilege escalation, read-only
root fs) and CPU/memory requests+limits; drop the unused management
SA token mount on the bootstrap-token job, which only talks to the
tenant apiserver via the mounted admin kubeconfig
- migration 44: list Helm-managed resources by the
app.kubernetes.io/managed-by=Helm label, not meta.helm.sh/release-name
(an annotation, so it never matched as a --selector — the keep
annotation was silently never applied)
- cluster.yaml: fail fast on a malformed nodeHealthCheck.maxUnhealthy
rather than silently coercing it to 0 via `| int`
- config: make helm-install-disable-wait parsing genuinely
case-insensitive (EqualFold + trim) to match its documented contract
- capi-providers bootstrap/cpprovider Makefiles: guard the
patches/*.diff loop against an empty glob
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Parse a new release.cozystack.io/helm-install-disable-wait annotation in start.go and apply it as Install/Upgrade.DisableWait in rest.go, so helm-controller no longer blocks post-install hooks on in-tenant addon readiness. This breaks the reconcile-hook chicken-and-egg where the hook that creates the worker TalosConfigTemplate was gated behind addons that need workers to schedule. Covered by a config table test.
Co-authored-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Rewrite the SecurityGroup design doc for the targetRef projection:
selector derivation from lineage labels, the structural (not
selector-validated) RBAC boundary, and the additive-policy reality —
a SecurityGroup can only widen a managed application's allowed traffic,
never sever its platform-managed management plane, and equally cannot
tighten below the platform baseline. Records the deliberate caveats:
raw non-application pods are not targetable, and a targetRef to a
non-existent application matches zero pods rather than erroring.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Update the REST storage fixtures and e2e bats to the targetRef shape, and
add cases for the new behaviour: the derived endpointSelector matchLabels,
the apiGroup default to apps.cozystack.io, an empty-targetRef rejection,
and invalid-label-value rejection (over-length or malformed kind/name/
apiGroup). The e2e asserts the dotted-key lineage labels serialise and are
accepted by the real cilium.io/v2 CRD, and that the view reconstructs the
targetRef.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Replace the tenant-authored endpointSelector on SecurityGroup with a
targetRef that references a managed application by {apiGroup, kind, name}.
The backing CiliumNetworkPolicy's endpointSelector is now derived by the
API server from the application's lineage labels
(apps.cozystack.io/application.{group,kind,name}) instead of 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.
The in-tree CiliumNetworkPolicy mirror gets its own spec type carrying a
concrete endpointSelector; the ingress/egress rule lists still project
1:1. Reads reconstruct the targetRef from the derived matchLabels, which
is lossless because validation caps each component at a valid label value
below the lineage webhook's truncation threshold.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
validateCIDRs rejected a bare IP (10.0.0.1) because net.ParseCIDR
requires a prefix, but Cilium's CIDR.sanitize accepts a bare IP as a
single-host CIDR. That made the validator stricter than Cilium and
blocked a legitimate single-host fromCIDR/toCIDR rule, contrary to the
design goal of rejecting only specs Cilium would silently discard.
Mirror Cilium by falling back to a plain-address parse, and add a
regression test for bare-IP CIDR acceptance.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
The resourceVersion de-dup filter dropped ADDED events whose RV was <=
the requested startingRV. During a sendInitialEvents replay the backing
WatchList intentionally re-emits existing objects as initial ADDED
events with RV <= startingRV, so the filter broke the initial-state
replay. Skip the de-dup while sendInitialEvents is set; it still applies
when resuming a plain watch from a resourceVersion. Add a regression
test covering sendInitialEvents with a non-zero starting RV.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Enforce in the REST storage that Create persists into the request
namespace and that Update/force-create targets the request name,
rejecting a conflicting metadata.namespace or metadata.name with a
BadRequest. The aggregated apiserver already rejects these before the
storage is reached, but binding them here keeps the storage
self-defending against a cross-namespace or off-target write regardless
of the calling path. Add regression tests that drive the storage
directly to lock the contract.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Describe the implemented model: SecurityGroup as a synchronous 1:1
projection of a marker-labelled CiliumNetworkPolicy served by the
aggregated API server, the marker invariant the storage enforces on every
write, the RBAC boundary it provides, the in-tree mirror that avoids the
Cilium module dependency, and the deferred attachment / cluster-scope /
validation extensions.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Co-authored-by: Timofei Larkin <lllamnyp@gmail.com>
Install the sdn.cozystack.io API group and serve the securitygroups
resource from the SecurityGroup REST storage, register the static
SecurityGroup kinds, add the group to the server codec, and register the
in-tree CiliumNetworkPolicy mirror with the client scheme.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Co-authored-by: Timofei Larkin <lllamnyp@gmail.com>
The aggregated API server translates each SecurityGroup into a single
CiliumNetworkPolicy in the same namespace and back. Policies owned by the
SecurityGroup API are marked with the sdn.cozystack.io/securitygroup label,
which the storage always re-asserts on every write and hides from the
SecurityGroup view, so it only ever surfaces or mutates its own policies,
leaves platform-managed policies untouched, and cannot be orphaned by a
tenant overwriting the marker through spec labels. Mutating verbs run the
genericapiserver admission hooks, and updates replace labels/annotations
rather than merging them, matching Kubernetes PUT semantics.
To avoid importing the full Cilium module — which pins a Kubernetes
version incompatible with this project's apimachinery fork — the backing
object is a minimal in-tree CiliumNetworkPolicy mirror that reuses
SecurityGroupSpec as its spec body, making the projection a near-identity
translation in both directions.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Co-authored-by: Timofei Larkin <lllamnyp@gmail.com>
Introduce the sdn.cozystack.io/v1alpha1 API group with the SecurityGroup
resource. SecurityGroup is a namespace-scoped, tenant-facing firewall
object whose spec mirrors the subset of the CiliumNetworkPolicy rule that
the abstraction exposes (endpointSelector, ingress, egress, ports, CIDR,
FQDN), so the resource can be projected 1:1 onto a CiliumNetworkPolicy
without granting tenants direct access to the cilium.io API group.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Co-authored-by: Timofei Larkin <lllamnyp@gmail.com>
## 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>
release.cozystack.io/helm-install-timeout sets both Install.Timeout and
Upgrade.Timeout to one value. Add release.cozystack.io/helm-upgrade-timeout
so an ApplicationDefinition can override only Upgrade.Timeout — letting a
kind keep a short install budget but a longer upgrade budget (or vice
versa). When set it wins over the value the install annotation would
otherwise apply to the upgrade side.
- config: add HelmUpgradeTimeoutAnnotation + ReleaseConfig.HelmUpgradeTimeout;
generalize ParseHelmInstallTimeoutAnnotation to ParseHelmTimeoutAnnotation
since it now parses both annotations (identical Flux unit validation).
- server: parse the new annotation at startup with the same loud-fail
behavior; update --helmrelease-upgrade-timeout help.
- rest: apply HelmUpgradeTimeout after the install-annotation copy so the
upgrade-specific value wins; restore the accurate carrier comment.
- tests: cover the upgrade-only override, the asymmetric precedence, and
the install-sets-both regression guard.
Behavior is unchanged for every existing ApplicationDefinition (none set
the new annotation today).
Address review feedback from coderabbitai on rest.go.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
The ParsePositiveDuration table exercised all error inputs but did not
distinguish which branch fired. Add an errMatch field so the table
pins "must be > 0" for zero/negative and "invalid duration" for
malformed/empty inputs, mirroring TestParseHelmInstallTimeoutAnnotation
and keeping the two parsers' diagnostics from drifting.
Address review feedback from coderabbitai on config_test.go.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
HelmReleaseMaxHistory carried omitempty, so a value of 0 (which Helm
treats as "unlimited") would be dropped on marshal and become
indistinguishable from an unset field. The runtime path (start.go
always populates it from --helmrelease-max-history) is safe today, but
drop omitempty so the explicit "unlimited" intent survives if
ReleaseConfig is ever serialized.
Address review feedback from coderabbitai on config.go.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
gofmt -w on the two touched test files: the case-table struct fields in
rest_helmrelease_spec_test.go and main_test.go were misaligned, and
main_test.go carried a trailing blank line. CI runs no gofmt gate
(pre-commit only runs make generate), so this would otherwise land
unformatted.
Address review feedback from lexfrei.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
The convertApplicationToHelmRelease comment listed only kubernetes-rd
as carrying release.cozystack.io/helm-install-timeout, dropping
tenant-rd. tenant-rd/cozyrds/tenant.yaml sets the 15m annotation for
the seaweedfs-db CNPG bootstrap whose first reconcile exceeds the flux
default. Restore the accurate two-carrier list and rationale.
Address review feedback from lexfrei on rest.go.
Assisted-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>
Production change:
- convertApplicationToHelmRelease now sets Install/Upgrade.Timeout
unconditionally (matching cozystack-operator's buildHelmReleaseSpec).
The previous "stay nil if zero" branch was a leaky test shim — it kept
fixtures with empty ReleaseConfig{} green at the cost of api/operator
drift.
Test changes:
- newRESTForTimeout seeds the new HelmRelease* globals with
production-shaped defaults so the unset-annotation case exercises
"global default applies" rather than "field stays nil".
- TestConvertApplicationToHelmRelease_PerAppTimeoutOverridesGlobal is
table-driven and now covers asymmetric globals (install != upgrade)
with and without per-app override — guards against a regression that
only assigns one side under override.
- New start_test.go: table-driven validator coverage for each
malformed/non-positive duration flag, the negative MaxHistory branch,
the MaxHistory=0 unlimited branch, asymmetric timeout acceptance, and
a smoke test that NewCozyServerOptions defaults validate.
Refactor:
- Extracted parseAndValidateHelmReleaseFlags from Complete() so the
flag-validation portion is unit-testable without kubernetes I/O.
Documentation:
- Reworded --helmrelease-install/upgrade-timeout help so admins reading
--help see the dual-purpose annotation explicitly (one annotation
overrides both timeouts together).
- Dropped PR-#2509 references and "preserving the pre-#2509 behaviour
for tests" comments — kept the technical rationale (Strategy.Name
vs Remediation.Retries:-1 + 5m Interval coupling) on its own merits.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
convertApplicationToHelmRelease now builds the HelmRelease spec from
ReleaseConfig instead of hardcoding values, bringing it in lockstep
with cozystack-operator's PackageReconciler.buildHelmReleaseSpec
(PR #2509). The drift between the two HelmRelease-generating paths
was the rationale for this follow-up.
Changes to the generated spec:
- Spec.Interval reads ReleaseConfig.HelmReleaseInterval (was
hardcoded 5m).
- Spec.MaxHistory set as &maxHistory pointer from
ReleaseConfig.HelmReleaseMaxHistory; 0 survives as pointer-to-0
(unlimited per Helm semantics) - mirrors the operator's
TestBuildHelmReleaseSpecZeroMaxHistory.
- Install.Strategy{Name: RetryOnFailure, RetryInterval} replaces
Install.Remediation{Retries: -1}. Same for Upgrade.Strategy.
Functionally equivalent to "retry forever, never remediate" for
the api-side path (which also never relied on remediation
firing) but decouples failed-install retry timing from
Spec.Interval.
- Install.Remediation / Upgrade.Remediation stay nil:
helm-controller's XValidation rejects RetryOnFailure +
RetryInterval alongside a Remediation entry, so reintroducing
Retries:-1 "for safety" would silently break every HR.
Install/Upgrade.Timeout precedence:
- per-Application HelmInstallTimeout (annotation-driven) wins
when non-zero (preserves the kubernetes-rd opt-in path);
- otherwise HelmReleaseInstallTimeout / HelmReleaseUpgradeTimeout
globals apply;
- when both are zero, the field stays nil and flux defaults
apply (preserves the legacy code path for tests that
hand-construct ReleaseConfig without populating the new globals).
Tests:
- rest_timeout_test.go: Remediation.Retries == -1 assertions
replaced with Strategy.Name == RetryOnFailure +
Remediation == nil, matching the operator's spec test.
- rest_helmrelease_spec_test.go: new file mirroring
TestBuildHelmReleaseSpec / TestBuildHelmReleaseSpecZeroMaxHistory
from internal/operator/package_reconciler_test.go, plus a third
case asserting per-Application HelmInstallTimeout overrides the
global Install/Upgrade timeout defaults.
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Wire five flags into cozystack-api that mirror the cozystack-operator
flags introduced in PR #2509:
--helmrelease-interval (default 5m)
--helmrelease-retry-interval (default 30s)
--helmrelease-install-timeout (default 10m)
--helmrelease-upgrade-timeout (default 10m)
--helmrelease-max-history (default 5)
CozyServerOptions.Complete() validates each duration via
config.ParsePositiveDuration so a misconfigured operator restarts
loudly instead of waiting until the first Application is created,
and rejects MaxHistory < 0 (0 is valid: unlimited per Helm
semantics, matching the operator).
Validated values are written into every ReleaseConfig built from
the ApplicationDefinitionList. The per-Application
HelmInstallTimeout (annotation-driven) still wins over the new
global Install/Upgrade timeout defaults; this commit only plumbs
the values - the consumer in convertApplicationToHelmRelease lands
in the next commit.
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Lift parsePositiveDuration from cmd/cozystack-operator/main.go into
pkg/config so cozystack-api can reuse the same startup validator
without duplication. Operator main.go now imports and delegates;
the table-driven test moves to pkg/config/config_test.go.
Extend ReleaseConfig with the five HelmRelease generation knobs
the cozystack-operator already exposes via flags (PR #2509):
HelmReleaseInterval, HelmReleaseRetryInterval,
HelmReleaseInstallTimeout, HelmReleaseUpgradeTimeout,
HelmReleaseMaxHistory. The existing per-Application
HelmInstallTimeout (set from the
release.cozystack.io/helm-install-timeout annotation) keeps its
override semantics - it still wins over the new global
Install/Upgrade timeout defaults when set.
No behaviour change in this commit; the new fields are wired up in
follow-up commits.
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
REST.Update wraps the HelmRelease write in retry.RetryOnConflict so a stale
resourceVersion (the helm-controller racing the status subresource) is
refreshed and retried instead of surfacing a 409. Add a unit test using a fake
client whose interceptor returns one Conflict on the first Update and succeeds
on the second, asserting Update returns nil and writes exactly twice.
Addresses non-blocking follow-up 3 on #2948.
Assisted-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
The aggregated API server translates app CR updates (Etcd, Postgres, ...)
into a read-convert-update of the backing HelmRelease. Flux's
helm-controller continuously writes the HelmRelease status, which shares
the object's resourceVersion, so a CR update that lands while a prior
reconcile is still in flight reads a resourceVersion that goes stale
before the Update and the request fails with a 409 Conflict
("the object has been modified; please apply your changes to the latest
version and try again").
The HelmRelease spec is fully derived from the Application the caller
just applied, so a stale-resourceVersion conflict is never a real spec
conflict here. Wrap the Update in retry.RetryOnConflict, refreshing the
live resourceVersion on each conflict before retrying. The no-conflict
path is unchanged, and this mirrors the existing RetryOnConflict usage in
workloadmonitor_controller.go.
Surfaced by the etcd.bats e2e test, which applies the same singleton
Etcd resource three times back-to-back and intermittently failed the
third apply with the 409.
Assisted-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
(cherry picked from commit 76410599de)
## What this PR does
`cozystack-api` published the free-form `.spec` of aggregated
`apps.cozystack.io` resources (Tenant, Application, …) as the boolean
`additionalProperties: true`. The Kubernetes ValidatingAdmissionPolicy
**status type-checker** run by `kube-controller-manager`
nil-dereferences when it recurses into that node
(`k8s.io/apiserver/pkg/cel/openapi.isExtension`). As soon as a VAP
matches one of these resources — `cozystack-tenant-host-policy` matches
`tenants` — KCM `CrashLoopBackOff`s on every control-plane node, stalls
reconciliation, and times out the install (`kubectl wait hr --all` never
completes). With the 3× install retry removed, a single occurrence fails
E2E outright.
This emits `x-kubernetes-preserve-unknown-fields: true` instead, in
`pkg/cmd/server/openapi.go::patchSpec` (both the schemaless and
custom-schema branches, covering the OpenAPI v2 and v3 post-processors).
It is semantically equivalent — "arbitrary fields allowed" — and the
type-checker handles it safely. All managed-app custom schemas already
use the safe **object-form** `additionalProperties`, so `patchSpec` was
the only source of the boolean form.
### Regression test
`TestPatchedSpecDoesNotPanicVAPTypeChecker` reproduces the exact crash
through `k8s.io/apiserver/pkg/cel/openapi.SchemaDeclType` — the entry
point KCM's `validatingadmissionpolicystatus` controller uses. It
**panics on the pre-fix code and passes after the fix**. Companion
structural tests assert the published spec carries
`x-kubernetes-preserve-unknown-fields: true` and never the boolean
`additionalProperties: true`, and that genuine object-form maps are left
untouched.
Verified against the pristine vendored `k8s.io/apiserver@v0.34.1` (the
version cozystack-api builds against), which reproduces the panic, and
previously confirmed end-to-end on `kindest/node:v1.33.1`.
### Upstream
The underlying nil-dereference is a Kubernetes KCM bug — the VAP status
type-checker should treat `additionalProperties: true` as dynamic rather
than panic. A separate upstream report is being prepared; cozystack
fixes the symptom immediately with the schema change above.
Fixes#2863
### Release note
```release-note
fix(api): publish the free-form resource `.spec` as `x-kubernetes-preserve-unknown-fields` instead of `additionalProperties: true`, fixing a cluster-wide `kube-controller-manager` CrashLoopBackOff (nil-pointer panic in the ValidatingAdmissionPolicy status type-checker) that stalled installs.
```
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **Bug Fixes**
* Ensured published OpenAPI schemas no longer contain boolean-form
additionalProperties anywhere; top-level and nested schemas now use
preserve-unknown-fields for free-form specs, preventing
validation/runtime issues.
* **Tests**
* Added regression tests that feed varied untrusted schemas and verify
boolean additionalProperties are removed and published schemas remain
valid for tooling.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Address review feedback from gemini-code-assist on pkg/cmd/server/openapi.go:
markOpenObject and sanitizeForV2 initialized s.Extensions with the generic
map[string]any{}; use the named spec.Extensions{} type instead, matching the
kube-openapi types and the generated definitions in zz_generated.openapi.go.
No behavioral change — spec.Extensions is defined as map[string]interface{}.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Address review feedback from lexfrei and coderabbitai on
pkg/cmd/server/openapi_test.go:
TestPatchSpecSanitizesUserSuppliedBooleanAdditionalProperties only exercised the
top-level, properties, items, and allOf positions, leaving the one remaining
type-checker-reachable sanitizer branch untested: a boolean additionalProperties
nested inside an object-form additionalProperties. Deleting that recursion line
kept the whole suite green while the input panicked SchemaDeclType again (#2863).
Add the nested-under-additionalproperties case (the reachable blocker, verified
red->green by mutation) plus anyOf/oneOf/not cases (defense-in-depth: those
branches are recursed by the sanitizer but never traversed by SchemaDeclType, so
the hasBooleanAdditionalProperties assertion guards them). Rewrite the test
docstring to distinguish the type-checker-reachable cases from the
defense-in-depth ones, replacing the inaccurate "each case panics SchemaDeclType"
note.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
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>
Addresses review feedback on #2863. The previous fix only converted the open
spec wrapper cozystack-api injects, so a user-supplied ApplicationDefinition
openAPISchema carrying a boolean additionalProperties — declared explicitly at
the top level, or nested under properties/items/allOf/anyOf/oneOf — was
republished verbatim and re-triggered the kube-controller-manager VAP
type-checker crash. openAPISchema is untrusted input (external-apps and
operator-authored definitions flow through unvalidated), so close the whole
class rather than only the schemas cozystack ships.
Recursively rewrite every boolean-form additionalProperties (nil inner schema):
additionalProperties:true becomes x-kubernetes-preserve-unknown-fields:true,
additionalProperties:false is dropped. Both JSON forms carry a nil inner schema
and crash the type-checker identically; object-form maps are recursed into and
left intact. Also publish the schemaless spec from a fresh schema so no
inherited $ref or properties leak into the open object.
Regression tests cover top-level true/false and nested (properties, items,
allOf) boolean forms, asserting the published schema carries no boolean
additionalProperties node and does not panic SchemaDeclType.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
cozystack-api published the free-form ".spec" of aggregated apps.cozystack.io
resources (Tenant, Application, ...) as the boolean `additionalProperties: true`.
The Kubernetes ValidatingAdmissionPolicy status type-checker run by
kube-controller-manager nil-dereferences when it recurses into that node, so as
soon as a VAP matches one of these resources (cozystack-tenant-host-policy on
tenants) KCM CrashLoopBackOffs cluster-wide and stalls reconciliation, failing
cold installs once the 3x install retry was removed.
Emit `x-kubernetes-preserve-unknown-fields: true` instead. It is semantically
equivalent ("arbitrary fields allowed") and the type-checker handles it safely.
All app custom schemas already use the safe object-form additionalProperties,
so patchSpec was the only source of the boolean form.
Adds a regression test that reproduces the exact crash through
k8s.io/apiserver/pkg/cel/openapi.SchemaDeclType (the entry point KCM uses): it
panics on the pre-fix code and passes after the fix.
Fixes#2863
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
The Watch goroutine never closed the events channel it owns, and
ProxyWatcher.Stop does not close the channel it proxies. When the
backing watcher ended, consumers ranging over ResultChan blocked until
their own timeout instead of seeing end-of-stream. Closing the channel
on goroutine exit matches the watch.Interface contract and the pattern
already used by the tenantmodule and application registries.
Goroutine-termination tests now assert the result channel closes.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Four goroutine-termination tests duplicated the same IsStopped polling
loop; waitForFakeWatcherStop keeps the deadline logic in one place.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Manual runtime.NewScheme setups in the List/Get error-path tests silently
discarded AddToScheme errors; the shared helper fails the test instead.
Renamed from newWatchTestScheme since it now serves both test files.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Cover the per-event filtering paths of REST.Watch directly instead of
only transitively through the bookmark test: authorization filtering via
RoleBindings, privileged groups (system:masters, cozystack-cluster-admin),
non-tenant namespace and foreign object drops, backing bookmark
forwarding, field and label selector filtering, missing user rejection,
access-check error resilience, ADDED deduplication by resourceVersion,
terminating bookmark flush on backing watcher close, and goroutine
shutdown on Stop and context cancellation.
Also close the remaining gaps on the sibling read paths sharing the same
authorization logic: List/filterAccessible (per-user filtering, privileged
groups, missing user, RoleBindings outside the tenant set, backing list
errors) and the Get error branches.
Branches the controller-runtime fake client cannot drive (backing
bookmarks, non-Namespace objects, chosen resourceVersions, backing watch
errors) are exercised through a stub client.WithWatch backed by a
test-owned FakeWatcher.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
## What this PR does
While preparing for my CozySummit talk of last week (creating commits
from CozySummit edits with
[gitops-reverser](https://github.com/ConfigButler/gitops-reverser)) I
noticed warnings on my console:
```
Warning: event bookmark expired err="reflector.go:343: hasn't received required bookmark event marking the end of initial events stream, received last event 59.998946102s ago"
The warning fires on every reflector restart and continuously while the cache is unsynced from the WatchList perspective.
```
I've been searching a bit better and it turns out that `TenantSecret`,
`TenantModule`, and `TenantNamespace` (group `core.cozystack.io`) all
ignore `ListOptions.SendInitialEvents`. Interestingly enough
`apps.cozystack.io/Application` already does implement this in the right
way.
Under the [WatchList / streaming-list
protocol](https://kubernetes.io/docs/reference/using-api/api-concepts/#streaming-lists)
(client-go's `WatchListClient`, default-on since v1.35), clients expect
the initial `ADDED` events to be terminated by a
[`Bookmark`](https://kubernetes.io/docs/reference/using-api/api-concepts/#watch-bookmarks)
annotated with `k8s.io/initial-events-end: "true"`. These three
resources only forwarded the backing watcher's bookmarks (which lack the
annotation), so the marker was never sent and client-go reflectors never
reached `HasSynced` — looping on `reflector.go:343: hasn't received
required bookmark event marking the end of initial events stream`.
**Fix:** extracted the contract into a shared
`registry.InitialEventsBookmarker` helper (`pkg/registry/utils.go`) and
wired it into all four `Watch` implementations. It annotates the first
backing bookmark as the terminating marker, with fallbacks to emit it
before the first live event and on watcher close. The annotation reuses
the upstream `metav1.InitialEventsAnnotationKey` constant rather than a
string literal.
The full bookmark machinery in `k8s.io/apiserver/pkg/storage/cacher`
isn't reusable here: it's internal to the watch-cache layer and only
fires for cacher-backed storage, whereas these resources translate a
controller-runtime watch in custom REST storage that bypasses the
cacher.
`Application` is refactored onto the helper with its `WorkloadMonitor`
event-draining preserved. Adds unit tests for the helper and watch tests
for each core resource (`ADDED` → annotated `Bookmark` → live event).
### Screenshots
N/A — no UI changes.
### Release note
```release-note
fix(api): core.cozystack.io resources (TenantSecret, TenantModule, TenantNamespace) now emit the k8s.io/initial-events-end bookmark for SendInitialEvents=true (WatchList) watches, so client-go informers reach HasSynced instead of looping on the missing-bookmark warning.
```
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **Bug Fixes**
* Watch endpoints reliably emit the "initial-events-end" bookmark after
initial events, improving watch/list contract compliance and client
reflector synchronization.
* **Refactor**
* Watch implementations consolidated around a centralized initial-events
bookmarker and stop-aware event forwarding for consistent bookmark
behavior.
* **New Features**
* Introduced a reusable initial-events bookmarker to coordinate bookmark
emission and resourceVersion observation across translated watch
streams.
* **Tests**
* Added unit and integration tests validating bookmark emission,
ordering, resourceVersion seeding/observation, and close-time behavior.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
The aggregated apiserver resources in group core.cozystack.io
(TenantSecret, TenantModule, TenantNamespace) ignored the
SendInitialEvents field of ListOptions. Clients using the WatchList /
streaming-list protocol (client-go's WatchListClient feature, on by
default since v1.35) never received the required
k8s.io/initial-events-end bookmark, so their reflectors stayed stuck in
the initial-stream phase and logged "hasn't received required bookmark
event marking the end of initial events stream" every ~10s, never
reaching HasSynced.
apps.cozystack.io/Application already implemented this contract inline.
Extract that logic into a shared registry.InitialEventsBookmarker helper
(pkg/registry/utils.go) and wire it into all four translating Watch
implementations: after the initial ADDED events, the first backing
bookmark is annotated as the initial-events-end marker, with fallbacks
that emit it before the first live event and on watcher close. Application
is refactored onto the shared helper with its WorkloadMonitor
pending-event draining preserved.
Adds unit tests for the bookmarker and watch tests for each of the three
core resources asserting ADDED -> annotated Bookmark -> live event.
```release-note
[apiserver] core.cozystack.io resources (TenantSecret, TenantModule,
TenantNamespace) now emit the k8s.io/initial-events-end bookmark for
SendInitialEvents=true (WatchList) watches, so client-go informers reach
HasSynced instead of looping on the missing-bookmark warning.
```
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Simon Koudijs <simonkoudijs@gmail.com>
instancetype/instanceprofile replaced the static vm-instance enum with live
dropdowns sourced from instancetype.kubevirt.io. A wrong group/version would
make those dropdowns silently empty in every cluster (List tolerates the
error) — the failure mode the appkind GVR guard already catches. Pin both
GVRs to the served API.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
The one-shot option watch emitted the current state once then blocked on
ctx.Done(). If the initial List failed (e.g. every provider forbidden by a
broken RBAC config) it blocked silently, leaving the client with an empty
stream indistinguishable from "no options". Log the error so an operator can
tell the two apart.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
The gpu, image, storagepool, storageclass and vmdisk providers carry the real
parsing logic (whitelist-intersect-allocatable, prefix strip, nested pool
walk, default-class marking, size label) but had no tests — and List tolerates
a provider error by skipping it, so a logic bug ships as a silently empty or
wrong dropdown rather than a failure. Add table tests against the fake dynamic
client for each, raising option-package coverage from 39% to 77%.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
The core fuzzer was migrated from gofuzz to randfill, but the apps fuzzer was
left on gofuzz. TestRoundTripTypes runs both fuzzers (apps first), so
registering a gofuzz.Continue func against the randfill-based roundtrip harness
panics ("customFuncs' second argument must be a randfill.Continue").
Migrate the apps fuzzer the same way (randfill.Continue / FillNoCustom). With
no gofuzz consumers left, go mod tidy drops the dependency. TestRoundTripTypes
now passes.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
The Option REST deliberately makes List tolerate a single failing provider
(it logs and skips it, so a partial RBAC config drops one dropdown instead
of breaking the whole list) while Get surfaces the error as a 500. Neither
path was covered. Add tests that inject a failing provider and assert List
keeps the healthy sources and omits the failing one, and that Get returns an
InternalError.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
The appkind option provider queried applicationdefinitions in the
apps.cozystack.io group, but the ApplicationDefinition CRD is served from
cozystack.io. The provider listed a resource that does not exist, so the
appkind source resolved empty on every cluster (tolerated and dropped by
List, 500 on Get), and the backup form fields that reference it never
populated.
Point the provider at cozystack.io, which the existing cozystack.io read
grant already covers. Add a guard test pinning the CRD group, since the
existing fake-client test is built from the same GVR and cannot catch a
wrong group.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Add three dropdown sources to the Option provider registry so the
dashboard's backup forms can use the unified x-cozystack-options
mechanism instead of injecting client-side enum arrays:
- plan / backup: namespaced, list Plans / Backups in the tenant namespace
- appkind: cluster-scoped, lists application kinds from ApplicationDefinitions
Annotate the backups.cozystack.io CRD schemas (Plan, BackupJob, Backup,
RestoreJob) with x-cozystack-options on backupClassName (backupclass),
applicationRef.kind / targetApplicationRef.kind (appkind), planRef.name
(plan) and backupRef.name (backup). This also gives the previously
unused backupclass source its first consumer.
Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io>
Introduce a read-only, namespaced virtual resource
core.cozystack.io/v1alpha1 Option that serves named lists of dropdown
options for the dashboard. Each object (one per source: gpu,
instancetype, instanceprofile, network, image, storagepool,
storageclass, vmdisk, backupclass) is computed on read by an in-process
provider using a privileged dynamic client, so tenants need no direct
access to Nodes, the KubeVirt CR, cluster instancetypes, etc.
Tenants get read access via the aggregated cozy:tenant roles; the
apiserver ServiceAccount gains read on the underlying sources.
Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io>
The roundtrip test for the core API group panicked (Filler.Funcs:
customFuncs' second argument must be a randfill.Continue) because the
fuzzer still used github.com/google/gofuzz while apimachinery switched
to sigs.k8s.io/randfill. Port the fuzz functions to randfill.Continue
/ FillNoCustom.
Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io>
tenant-rd now carries release.cozystack.io/helm-install-timeout alongside
kubernetes-rd (its parent chart bootstraps the seaweedfs-db CNPG cluster, whose
first reconcile exceeds flux's default wait budget). Update the comment that
claimed only kubernetes-rd carries it.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Aleksei Sviridkin