mirror of
https://github.com/cozystack/cozystack.git
synced 2026-11-03 10:37:21 +09:00
feat/externalip-api
357
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Aleksei Sviridkin
|
d685fb30ec |
fix(gateway)!: require tenant routes to declare their hostnames (#3475)
## What this PR does The tenant route hostname policy skipped the shape it most needed to check. Its CEL opened with `!has(object.spec.hostnames) ||`, so an HTTPRoute or TLSRoute declaring no hostnames satisfied the first operand and got admitted with nothing compared against the tenant apex. An empty list passed too: `has()` is true on `[]`, and `all()` over an empty list is vacuously true. Such a route is not inert. Under Gateway API it inherits the hostnames of every listener it attaches to, and `parentRefs` can name a Gateway in another namespace, so it covers whatever that listener covers. Reported in #3467. The predicate now requires the field to be present and non-empty before validating it. Both policies rendered from that template share one CEL string, so HTTPRoute and TLSRoute get the change together, and both match `CREATE` and `UPDATE`, so an existing route that drops its hostnames is denied exactly as a new one is. The issue is titled after TLSRoute, but the reachable shape is mostly HTTPRoute. TLSRoute allows a hostname-less route only on the deprecated `v1alpha2`; at `v1` and `v1alpha3` the bundle this repo vendors marks `spec.hostnames` required and rejects an empty list on `minItems: 1`, while `v1alpha2` carries neither constraint. HTTPRoute `v1` carries neither either, so it accepts both shapes. All four are checkable in `packages/system/gateway-api-crds/templates/crds-experimental.yaml`. Checking that turned up a stale claim in the template header, which said TLSRoute had no `v1` in Gateway API 1.4 to 1.5 and used it to justify the rule naming only `v1alpha2`. Gateway API 1.5.1, which this repo vendors, serves TLSRoute at `v1` as the storage version. The rule still covers every served version because `matchConstraints.matchPolicy` defaults to `Equivalent`, and that default is now written down, because setting `matchPolicy: Exact` reads like a tightening and would silently stop the policy seeing `v1` and `v1alpha3` TLSRoutes. I corrected the text and left the coverage alone: the wrong sentence predates this branch, but my change is what makes the file contradict itself, so the text is mine to fix while widening version coverage is a separate job. A `ValidatingAdmissionPolicy` has no cross-object lookup, so it cannot resolve the parent Gateway to see which hostnames a route would inherit. Requiring the route to name its own is what keeps every admitted hostname comparable against the apex. That has a cost: a route relying on listener inheritance now has to spell its hostnames out, and a tenant that hand-writes one gets a denial where it used to get an admission. ### The controller change belongs in the same PR `renderHTTPRedirect` built the http-to-https redirect HTTPRoute with no hostnames on purpose, to catch every host on port 80. It lands in a `tenant-*` namespace, so the stricter policy denies it, `reconcileHTTPToHTTPSRedirect` returns that error, and since it runs before the status update the TenantGateway never reaches Ready. That redirect is what keeps app routes attaching by hostname from serving plaintext on port 80. Neither half of this change is safe alone. The redirect now names the tenant apex and its wildcard. Both entries are needed: a wildcard hostname is a suffix match over any number of labels, so `*.<apex>` covers subdomains at every depth but not the bare `<apex>`. `renderWildcardCertificate` already builds the same pair. Coverage narrows in one direction. A request on port 80 with a Host outside the tenant apex now falls through to a 404 instead of a 301. That narrowing costs no covered host: the port-80 listener admits routes only from the Gateway's own namespace and cert-manager's, and the tightened policy requires every route in the tenant namespace to declare its hostnames inside the apex, so any host published from this namespace already falls under the apex or its wildcard. An inheriting child tenant does not widen that set either. Its hostnames would reach port 443 through a listener on this same Gateway, and `cozystack-gateway-hostname-policy` compares every listener against the host label of the Gateway's own namespace, so a child apex outside that label never gets its listener: the Gateway write carrying it is refused at admission whole, in `reconcileGateway` and before the redirect is rendered. A child apex under this one passes that check and the wildcard already covers it. That holds at merge base as well, so it is not something this branch changes. Deriving hostnames from `spec.apex` makes that field load-bearing for admission, so it gains `MinLength=1`. An empty apex used to reconcile to Ready, since the redirect declared no hostnames at all; it would now render `""` and `"*."` and be rejected on the HTTPRoute hostname schema, surfacing as a validation error naming the route rather than the field. The renderer checks it too, because the CRD and the controller binary roll out separately and an older CRD in the cluster will not carry the bound. That schema bound is a breaking change to an existing field and the API review gate flags it as one, so here is what it actually invalidates. The chart that renders these CRs already fails on an empty `_namespace.host`, and in DNS-01 or existingSecret mode an empty apex already broke earlier, in `renderGateway`, on a listener with `hostname: "*."`. Nothing that works today becomes invalid. What the bound rejects is a TenantGateway written by hand, past the chart, with an empty apex, and I would rather that be refused at admission naming the field than at reconcile time in controller logs, which is why the bound sits in the CRD as well as the renderer. Naming those hostnames also meant excluding the redirect from `collectHostnameClaims`. A route that attaches to the Gateway and declares hostnames otherwise reads as an app publishing them, and each claimed hostname becomes an HTTPS listener named after its first label. The first label of `*.<apex>` is `*`, which the Gateway CRD's listener-name pattern rejects, so the Gateway write fails and the reconcile stops before anything downstream. The exclusion matches on namespace, name and controller ownership rather than on the labels the renderer stamps: those labels are writable by anyone who can create a route, and the claim set also drives conflict resolution, so a label-keyed exception would let any route opt out of being marked `Accepted=False` when it loses a hostname race. Name and ownership are each required and each is pinned by its own test, since ownership alone would exclude any other controller-owned route and name alone would extend the exception to a route planted under that name by someone else. ### Radius Only namespaces starting with `tenant-` are matched. The three first-party TLSRoutes the platform ships, `kubernetes-api` in `default` plus `cdi-uploadproxy` and `vm-exportproxy`, set hostnames explicitly and none sits in a `tenant-*` namespace, so nothing changes for them. I went through every producer of routes in the tree, in three places rather than one. No Helm template emits an absent or empty hostname list in any configuration a tenant can reach: the vendored openbao, harbor, opencost and flux-operator route templates can render one, but each is behind a flag that defaults off with no values path to turn it on, and openbao would fail CRD validation first because its default route apiVersion requires hostnames. Exactly one Go site builds a route, the redirect fixed here. And one e2e fixture did rely on the old behaviour: the Gateway suite's parentRef probe declared no hostnames and is applied into `tenant-test`, which the policy matches, so it now reads the apex from the namespace label and names a hostname under it, the same way the neighbouring cases in that suite already did. cert-manager also writes routes into tenant namespaces, since it creates the Challenge in the Certificate's own namespace and the HTTP-01 solver route beside it. That route declares the hostname it is validating, so the policy admits it. ### Related issues Two open issues have a text half that this PR happens to settle, and neither of them closes here. #3788 reports that the port-80 allow-list contains the namespace its own comment says it excludes; the comment and the README paragraph now describe what that filter actually does, while the behavioural exit it names stays open. #3789 reports that the TLSRoute rule names only the deprecated API version; that premise is now written down in the template header, including what has to change before a bundle drops `v1alpha2`, but the rule still names `v1alpha2` alone. #3787 covers the wildcard-hostname listener name that the apiserver rejects, which is what makes the claim-set exclusion here load-bearing rather than tidy. ### Screenshots N/A (no UI changes). ### Downstream repositories I walked the trigger map in `docs/agents/contributing.md` against the diff, file by file. No package is added, renamed or removed; no `values.yaml` key, `values.schema.json` field, version enum or default moves; no `kind` or `plural` changes; no namespace or release-asset name changes; nothing under `hack/` moves, is renamed, or changes what a make target does. One row deserves a sentence rather than a blanket denial, because the diff does change a CRD schema: `spec.apex` on `gateway.cozystack.io/v1alpha1 TenantGateway` gains a `minLength` bound, which lands in the generated `gateway.cozystack.io_tenantgateways.yaml`. The provider hand-types `Package` under `cozystack.io` and `Plan`/`RestoreJob` under `backups.cozystack.io`; `gateway.cozystack.io` is not among them, and a tightening on an existing field adds no new field for it to offer typed access to. The `packages/extra/gateway/README.md` edit sits in the `## Security model` section, below the `## Parameters` block that `cozyvalues-gen` regenerates, so it survives `make generate`. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note ```release-note fix(gateway)!: the tenant route hostname policy now requires an HTTPRoute or TLSRoute in a `tenant-*` namespace to declare a non-empty `spec.hostnames`. A route that omitted the field, or set it to an empty list, was previously admitted without any hostname being checked against the tenant apex, even though such a route inherits the hostnames of the listener it attaches to rather than matching nothing. A route in a `tenant-*` namespace relying on that inheritance is now rejected and has to name its hostnames. The controller-owned http-to-https redirect route names the tenant apex and its wildcard accordingly, so a request reaching port 80 with a Host outside the tenant apex now gets a 404 instead of a 301. `TenantGateway.spec.apex` additionally gains a `minLength: 1` bound, because the redirect derives its hostnames from that field; a TenantGateway written past the chart with an empty apex is now rejected at admission instead of failing later in the controller. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added edge-terminated gateway support with plain-HTTP listeners and TLS passthrough handling. - HTTP-to-HTTPS redirects now explicitly cover the tenant apex and subdomains. - Gateway configurations require a non-empty apex hostname. - HTTPRoute, TLSRoute, and GRPCRoute resources must declare hostnames. - **Bug Fixes** - Invalid hostname-less routes are now rejected consistently. - Controller-managed redirects no longer trigger unnecessary hostname claims, listeners, or certificates. - Switching certificate modes now cleans up obsolete ACME resources and route status. - **Documentation** - Updated security and gateway guidance for hostname validation, edge termination, route compatibility, and certificate handling. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
IvanHunters
|
8738063287 |
fix(operator): reject tar entries containing ".." before any path operation
safeJoin already rejected traversal and re-checked the joined path against destDir, but the guard sits behind a call boundary that CodeQL's go/zipslip barrier does not follow, so the extraction loop reported three high-severity arbitrary-write alerts. Reject the entry name up front instead, which is both the fail-closed spelling and the shape the analysis recognises. Extend the traversal test to cover a ".." buried mid-path, not just a leading one. Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
15be9329a1 |
refactor(marketplace): source-centric Tap resource, label-based identity
The aggregated Tap resource no longer derives a community.<org>.<repo> name from the connect URL. A connected repository is identified by its Flux source (tap-<org>-<repo>), and the PackageSource(s) it materializes keep their own declared names. Community is computed from the marketplace-tap label, and disconnect is guarded by that label instead of a name prefix. List also surfaces tap sources with no materialized PackageSource yet (a connect in progress, or one blocked by a collision), carrying the materializer error message, so a blocked tap is visible in the dashboard rather than silently never appearing. The community. prefix constant is removed. Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
1653da967b |
refactor(marketplace): drop artifact-prefix injection and reserved names
Without the community. rename, a tapped repository's cozyrds ApplicationDefinition references its component's artifact by the concrete <packagesource>-<variant>-<component> name again, so the operator no longer injects a cozystackArtifactPrefix value and the -rd chart renders its cozyrds asset with a plain Files.Get. cozypkg init scaffolds the concrete chartRef. Reserved name prefixes are removed: repositories choose their own PackageSource names, and a clash with a core component is caught on-cluster at tap time (the collision check) rather than by refusing prefixes offline. Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
95badea1c8 |
refactor(marketplace): identify taps by marker label, drop community rename
Tapped repositories keep their own declared PackageSource names instead of being renamed under the community. prefix. Tap-ness is now marked by the marketplace-tap label (and source annotation) on the OCIRepository and every materialized PackageSource, so untap, the Flux-source cleanup, and idempotent re-tap key on the marker rather than a name prefix. A declared name that clashes with an existing core component (or another tap) is rejected: the CLI tap fails fast before applying anything, and the operator materializer refuses to overwrite the foreign object, records the reason on the source (annotation and Event), and does not stamp the revision so a corrected artifact retries. The shared check lives in internal/marketplace/collision. Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
0c0609ad63 |
fix(marketplace): resolve community chartRef via injected artifact prefix
Fixes the rename-vs-chartRef break found on dev3: a community-tapped
PackageSource is renamed to community.<org>.<repo>, so the assembled artifact
name changes, but a repo's cozyrds ApplicationDefinition baked chartRef.name
under the author's original name and the ExternalArtifact it referenced no
longer existed, leaving every community-installed app permanently ArtifactFailed.
Implements @kvaps's option 2 (derive at the -rd component, keep platform charts
literal):
- naming.ArtifactPrefix exposes the per-(source,variant) artifact-name prefix.
- The Package reconciler injects it as the cozystackArtifactPrefix Helm value,
but ONLY for community.-prefixed sources, so platform HelmRelease values are
byte-for-byte unchanged (no churn, no platform chart edits).
- cozypkg init now scaffolds a community-style -rd chart: cozyrd.yaml renders the
cozyrds asset through tpl, and chartRef.name templates
'{{ .Values.cozystackArtifactPrefix }}-<app>', resolving to the real artifact
whatever the tap-time rename produces.
- validate skips the dangling-chartRef check for a templated name (it cannot be
cross-checked at authoring time; the operator resolves it at install).
Verified end-to-end: init -> validate is clean, and helm template with the
prefix value renders chartRef.name to the exact artifact the reconciler emits.
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
|
||
|
IvanHunters
|
950268cccc |
feat(marketplace): reject reserved PackageSource name prefixes
From @kvaps's review: a repository author could name a PackageSource 'cozystack.<x>' or 'community.<x>' and claim the platform or marketplace namespace. Until now validate only rejected an empty metadata.name and the forced rename in the materializer was the sole guard. - validate now flags 'cozystack.'- and 'community.'-prefixed PackageSource names as an error. The escape hatch --allow-reserved-names lives on the caller side only: the index gate (validate-entry.sh) never passes it, so a submitter cannot hand it to themselves. - cozypkg init refuses the same prefixes early with a neutral-name hint. - Document why the ClusterIP fail-open is safe: it falls back only on address resolution, and integrity stays enforced by the fail-closed digest check, so a wrong host yields a mismatch error rather than a content substitution. Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
b96a97e5c4 |
fix(marketplace): resolve tap artifact host to ClusterIP for the hostNetwork operator
The tap materializer fetches the Flux artifact from status.artifact.url, which is a cluster-DNS name (flux.<ns>.svc/...). cozystack-operator runs with hostNetwork: true and dnsPolicy: ClusterFirst, so it resolves against the host's DNS rather than CoreDNS and that name is NXDOMAIN — every dashboard-driven connect (API Create, which relies entirely on the materializer) then hangs in 'connecting' forever, while the CLI tap works only because it pulls via 'flux pull artifact' from the public registry. Resolve the artifact host to the backing Service's ClusterIP (which a hostNetwork pod reaches directly, the same way the in-cluster apiserver is reached via KUBERNETES_SERVICE_HOST) before fetching. This fails open: a non-cluster host, a missing Service, or a headless/empty ClusterIP falls back to the original URL, so clusters where DNS already works are unaffected. Services are readable through the operator's existing cluster-wide get/list/watch grant. Found on dev3 via cozystack-pr-test; the connect->materialize path never worked on a stock operator deployment before this. Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
c9accd6086 |
fix(marketplace): address maintainer review — url collisions, fail-closed digest, centralized prefix
From @kvaps's review of the untrusted-artifact path: - Refuse to silently retarget: two repositories that share an org/repo path on different registry hosts derive the same tap name. The API and CLI now reject a connect when an existing source points at a different spec.url, instead of blindly repointing the first at the second. - Fail closed on integrity: verifyAndExtract now errors on an absent digest (not just a mismatch), and the reconciler waits for status.artifact.digest, so a tap artifact is never materialized without a verified digest. - Centralize the load-bearing community. prefix in internal/marketplace/tapconst (removing two duplicate consts and several literals across the CLI, API, materializer, and dashboard) so the disconnect guard, the community badge, and the anti-shadowing guarantee cannot drift. - Split multi-document YAML with the apimachinery reader so a PackageSource whose values contain a separator-looking line is not fractured. (The short-name version-pin drop was already fixed in an earlier commit.) Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
3ce8279019 |
fix(marketplace): prune stale taps, preserve source state, scope RBAC, pull by digest
Address the remaining review findings: - The materializer now prunes PackageSources it materialized from an earlier revision that the current artifact no longer contains (including a single/multi rename), so a removed package leaves the catalog; installed Packages stay. - A repeat connect updates the fetched OCIRepository's spec, label and name annotation instead of replacing the object, so the operator's finalizer and materialized-revision annotation survive; covered by a test. - OCIRepository write verbs move from the cluster-wide cozystack-api ClusterRole to a namespaced Role in cozy-system (get/list/watch stay cluster-wide for the watch cache), since the API only manages tap sources there. - 'cozypkg validate --require-signature' now pulls the exact manifest digest cosign verified, closing the tag-mutation window between verify and pull. - safeJoin rejects any '..' path segment outright (defense in depth and to satisfy the Zip-Slip scanner) on top of the existing containment check. - Document why the index gate's merge/label steps need a workflow_run split for fork pull requests. Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
7b703f2859 |
fix(cozypkg): satisfy golangci-lint and derive a valid kind for hyphenated apps
Address the linters flagged in review: check the previously-ignored writer, Close, and RemoveAll return values (errcheck), lowercase an error string (ST1005), drop the embedded-field selector (QF1008), remove an unused const, and switch the materializer's post-finalizer requeue from the deprecated Result.Requeue to RequeueAfter. Also fix 'cozypkg init': capitalize now builds an upper CamelCase kind from a hyphenated --app (foo-bar -> FooBar) so the scaffolded ApplicationDefinition is not rejected by the apiserver at install time; covered by a test. Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
3643fceceb |
refactor(marketplace): fail-safe re-tap rollback and single-source artifact naming
Close the two non-blocking notes from the third review round: - objectExists now fails safe: it returns 'not existing' (eligible for rollback) only on a definite NotFound; any other Get error (RBAC, throttling, timeout) is treated as possibly-pre-existing, so an idempotent re-tap that hits a transient probe error cannot delete a pre-existing resource on rollback. Mirrors the fail-direction of sourceStillReferenced; covered by a new interceptor-based test. - The PackageSource reconciler now calls internal/marketplace/naming.ArtifactName instead of inlining the formula, so the artifact-name convention has a single definition shared by the producer, the marketplace backend, and the validator, and the parity test guards producer-vs-helper. Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
1fbceee10e |
fix(marketplace): operator RBAC for tap materialization and dot-safe artifact naming
Second review round found the write-path RBAC fix was only half applied and the extracted naming helper diverged from its producer: - The tap materializer runs as the cozystack-controller ServiceAccount, whose ClusterRole granted source.toolkit.fluxcd.io/ocirepositories only get/list/watch. Its finalizer and materialized-revision writes (r.Update on the OCIRepository) were therefore Forbidden, so a dashboard-connected tap never materialized. Grant the controller update/patch on ocirepositories. - naming.ArtifactName replaced dots only in the PackageSource name, but the reconciler replaces them in the variant and component too, so a tapped repo with a dotted variant or component name dropped its packages from the catalog and tripped a false appdef-dangling warning. Replace dots in all three segments and add a direct parity test with dotted names. - Pin cosign-installer and the cosign release in the index gate so the signature verifier is reproducible. - Add a unit test for the tap rollback objectExists predicate. Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
be03900262 |
refactor(marketplace): address review — shared naming, tap rollback, pinned gate, trust docs
- Extract the per-component artifact-name convention into internal/marketplace/naming and have the validator and the Tap backend delegate to it, so the component-to-ApplicationDefinition linkage cannot drift across copies. - cozypkg tap only rolls back resources it actually created, so an idempotent re-tap no longer deletes a pre-existing OCIRepository/PackageSource when a later apply fails. - Pin cozypkg and yq in the index publication gate so the auto-merge decision runs on a reproducible toolchain. - Document the trust model explicitly: tap validates structure but does not verify signatures; verification is enforced by the index CI gate (pinned to the recorded cosign identity) and optionally by Flux OCIRepository verification at pull time. Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
e61d2c7b53 |
feat(marketplace): connect a community tap from the API with async materialization
Add server-side connect to complete the marketplace write path. Creating a Tap records the intent only: the API validates the oci:// reference and creates a labeled Flux OCIRepository (with an optional pull-credential secretRef), never blocking the request on a registry pull, and returns a Tap whose status shows materialization pending. The connect inputs (url, tag, secretRef) are write-only and are never echoed back or returned on read. A new tap materializer reconciler in the operator watches community-tap OCIRepositories, and once source-controller has pulled the artifact, fetches the tarball over the Flux artifact URL (digest-verified, size-bounded, path-traversal guarded), parses the PackageSource(s) it carries, and materializes them under the community. prefix with their sourceRef repointed at the tap's source. A finalizer cleans up the materialized PackageSources when the source is removed, leaving installed Packages in place. The shared label/annotation keys live in internal/marketplace/tapconst so the API and the operator cannot drift on the connect-to-materialize handoff. Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
Aleksei Sviridkin
|
18d02d813a |
fix(cacert): make the comments match the reconciler (#4064)
## What this PR does Comments in the CA-extraction package claim guards the code does not have. No behaviour change; one test is renamed to match the property it pins. - `ManagedLabel` said the reconciler only ever writes over a Secret carrying that label. It does not. `isOurProjection` decides that from the Secret type and the owner reference, and never looks at the label, which any writer in the namespace can strip. - `managedByCozystackLabel` said the reconciler never writes it. It deletes it whenever the selectors digest changes, which is what gets the projection re-admitted by the lineage webhook. - `projectionSuffix` said the dotted canonical name cannot collide with an operator-owned object. A release the apps API created cannot carry a dot, since application names are DNS-1035 labels and `release.prefix` is restricted to `^[a-z0-9-]*$`. The guarantee stops there: the controller reads the release from the `helm.toolkit.fluxcd.io/name` label, where a dot is legal, so a hand-written HelmRelease sits outside it. On the validated path the dot can only come from the suffix an operator or chart appends, which nothing validates at all, and since a canonical name ends in `.tenant-ca` and carries that one dot, a suffix ending in `.tenant-ca` is the only one that reaches a canonical name. The comment says all of that now. - The test pinning that name claimed the property for any engine suffix, in its doc and in its own name, while its loop samples only dot-free ones. It is now `TestProjectionNameCannotCollideWithADotFreeEngineSuffix`, and its doc says what the loop covers. ### Screenshots Not a UI change. ### Downstream repositories Comments and one test name in two Go files. No package, values key, schema field, CRD, metric, release asset or `hack/` target changes, so nothing in the trigger map fires. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note ```release-note NONE ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Clarified certificate reconciliation behavior, including label handling, projection naming, and source certificate scenarios. * Documented naming guarantees and their API boundaries more precisely. * **Tests** * Updated test descriptions and failure messaging to clarify coverage for dot-free engine suffixes. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
4b2507b7a1 |
docs(gateway): align gateway prose with the code
The comment on the shared CEL string said what the hostname requirement is but not what it is worth per kind. Two of the three schemas leave spec.hostnames optional at every version the vendored bundle serves, so for HTTPRoute and GRPCRoute this policy is the only rejection a hostname-less route meets; tlsroutes requires the field at v1 and v1alpha3 and not at v1alpha2, so there the policy closes the gap for as long as v1alpha2 is served. State that where the requirement is stated. The comment on the port-80 allowedRoutes filter credited the cert-manager namespace entry for letting ACME complete, which the paragraph above it retracts: the solver route lives in the tenant namespace, and that is what keeps ACME working. The same sentence claimed the filter keeps app routes off port 80, and the Layer 1 paragraph of the gateway README claimed it too. It does not. A tenant that owns its Gateway is its own gateway namespace, so its app routes are admitted by this very filter; what it excludes is cozy-* and inheriting children. Both copies now say that. Nothing said why naming the apex on the redirect costs no coverage, which is the question a reader has once the route stops matching every host. It holds because of the other half of this change: the port-80 listener admits routes only from the tenant namespace and cert-manager's, and the tightened route policy requires every route there to declare hostnames inside the apex. An inheriting child does not widen that set either, and the comment now says so at the point the hostnames are chosen: the gateway hostname policy compares every listener against the host label of the Gateway's own namespace, so a child apex outside it never gets its listener, the Gateway write carrying it being refused at admission whole, in reconcileGateway and before this route is rendered, while a child apex under this one passes that check and the wildcard already covers it. The empty-apex guard in renderHTTPRedirect, and the test pinning it, both read as though the guard is what spares an operator an unreadable schema error. It is, but the condition stated for it named the number of attached routes, which does not enter into it. In HTTP-01 the listener set is built from route hostnames alone, so reconcileGateway never reads the apex ahead of the guard however many routes attach. DNS-01 and existingSecret render an apex listener, and any tlsPassthroughServices entry builds <service>.<apex>, so in those the operator meets the equivalent error against the Gateway first. State that condition once, at the guard, and let the test defer to it rather than carry a second copy that can drift; the test stays green either way because the fake client does not validate against the CRD schema. The e2e suite explained omitting the TLSRoute empty-list probe by the v1 schema rejecting an empty list. Both halves of that are true and the conclusion does not follow: the probe runs at v1alpha2, which marks spec.hostnames neither required nor minItems, so an empty list reaches admission there exactly as an absent field does. What makes the omission safe is that all three policies render one byte-identical CEL string, so the HTTPRoute pair proves that term already. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
b9ecb2e39f |
fix(tenantgateway): name the apex on the http-to-https redirect route
The controller-owned redirect route carried no spec.hostnames, relying on the port-80 listener to match every host. Name the tenant apex and its wildcard instead. A route in a tenant namespace has to declare hostnames inside its own apex to pass the route-hostname admission policy, and a route that names none has nothing to compare against that apex. Naming them also states the coverage the route wants rather than inheriting whatever its listener happens to carry. Both hostnames are required: a wildcard is a suffix match over any number of labels, so "*.<apex>" covers every subdomain at every depth but not the bare "<apex>". Coverage narrows in one direction worth stating: a request arriving on port 80 with a Host outside the tenant apex now falls through to a 404 instead of a 301. Skip this route when collecting hostname claims. Before it carried hostnames it contributed nothing to that set, and letting its hostnames through instead provisions listeners and certificates for them, which fails. The check matches on name and controller ownership, not on the labels the renderer stamps, since those are writable by anyone who can create a route. Deriving hostnames from spec.apex makes that field load-bearing for admission, so it gains MinLength=1 and a matching check in the renderer for clusters whose CRD is older than the binary. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
773d8dabce |
fix(gateway): withdraw TLSRoute acceptance when the mode drops passthrough
Edge terminates TLS at the class provider, so its Gateway carries plain HTTP listeners only and a route pinned to a passthrough section has nothing left to attach to. The api, vm-exportproxy and cdi-uploadproxy endpoints of publishing.exposedServices are the ones that pin one. Hostname claims are empty for every whole-apex mode, so the status pass ran over an empty ref set and left the Accepted=True written while the tenant ran HTTP-01 standing on a route the switch had just unpublished. Whoever went looking for the dead endpoint read that condition as proof the route was fine. The rule this settles on is that the controller owns a condition only where it judges. Edge judges every attached route by the absence of the listener it needs and writes Accepted=False with Gateway API's NoMatchingParent, the reason defined for a parentRef whose port or sectionName matches no listener. The other whole-apex modes render those listeners and serve them but collect no claims, so they hold no opinion: they drop the entry rather than leave a withdrawal standing on a listener that is serving, naming a class provider that no longer terminates anything. HTTP-01 collects claims and has already judged every route this pass could speak about. A route declaring no hostnames is judged by none of them, because claims are gathered inside a loop over spec.hostnames and TLSRoute v1alpha2 sets no minItems on that field. Withdrawing one would write a condition with no path back, since HTTP-01 renders the listener again and judges only the routes that produced claims, so both directions skip it. The tests read the controller name from the constant the controller stamps rather than restating it, so a helper cannot go looking for a name that is no longer written and report absence it never checked. The switch already deletes the redirect route, the Issuer and the wildcard Certificate it invalidates. The statuses it owns were the one thing left behind. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
5976ddf02f |
fix(gateway): normalise the GatewayClass name and refuse a blank one
The class lists arrive over the values channel comma-joined, so every reader trims each entry as it splits them. The scalar those entries are compared against was not trimmed anywhere. A class name carrying a stray space therefore failed `has` against its own padded self, and the two readers of that scalar disagreed about the same class: the gateway chart put the tenant on an edge-terminated class, while the cluster-wide ACME issuers still pinned their solver to a listener an edge Gateway does not render. That stops every HTTP-01 certificate in the cluster from issuing, with nothing in any status naming the cause. Normalise once at the single writer so both readers see a clean value, and keep a defensive trim at each reader. The tenant's own choice does not travel that channel — it comes from tenant.spec.gatewayClass — so it is trimmed where the gateway chart reads it. Unset and whitespace-only are different events and no longer share an outcome. Unset means the choice was not made, and falling back to the bundled class stays its documented behaviour. Whitespace-only means a choice was made and is garbage, in the field that decides where TLS terminates: resolving it quietly would take a tenant the operator meant to put behind an edge terminator and publish it directly, with a real certificate on a real address. Both the platform default and the tenant's own choice now refuse it by name. The kind check beside that refusal is part of the same fix, not a tidy up. Through Helm's values path an unquoted numeric scalar arrives as a float64, and the whitespace check compares it against a string, so `className: 123` dies at that comparison — one line before the trim — with an error naming neither the key nor the file. Refusing whitespace without checking the kind first would therefore have made the numeric case worse than it was. `kindIs "string"` is the right test because it never has to name the type that actually arrived. The order of the guard's steps is part of the guard. The value is read before anything normalises it, so a default that fires on any falsy value cannot launder 0 or false into the empty string and resolve them to the bundled class ahead of the kind check. Unset is the one case that still falls back, and each of those four inputs is pinned, so a normalisation drifting back ahead of a check fails rather than quietly covering only the loudest bad input. Also correct a cleanup log line: the wildcard Certificate is deleted for every mode that is not DNS-01, so naming HTTP-01 in that message is wrong for existingSecret and for edge alike. And state the listener name invariant plainly: the cap is 253, not the 63 of a DNS label, and the longest name the builders produce is 84. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <3811295@gmail.com> |
||
|
Aleksei Sviridkin
|
7851969448 |
docs(gateway): document GatewayClass selection and edge TLS termination
An operator setting up a class whose provider terminates TLS upstream had no page answering their questions: which platform values bound the choice, what a tenant writes to make it, what a move between classes does to certificates and to the replicated wildcard Secret, and how the mechanism fails when a configured class name matches nothing installed. Add docs/gateway-classes.md covering that surface. Describe the mode and its consequences in the chart README alongside the other three, including the one constraint stated nowhere else: the publishing tenant cannot select a class whose TLS termination differs from the platform default, because the cluster-wide ACME issuers derive their solver pin from that default alone. State the blast radius where it actually applies. The endpoints an edge Gateway stops publishing exist only where those routes are rendered at all, and both pages say what the transition does to the route conditions the controller owns, so a reader is not left inferring it from the code. The security model names the selector each listener really carries. The port-80 listener selects on the built-in kubernetes.io/metadata.name label, which kube-apiserver writes and a namespace cannot forge; the HTTPS and TLS-passthrough listeners select on namespace.cozystack.io/gateway, written by the tenant chart and the controller, so that guarantee does not extend to them. What closes the exposure is that Namespace is cluster-scoped with no tenant verb on it, and that the route-hostname policies bound what a labelled namespace could publish. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <3811295@gmail.com> |
||
|
Aleksei Sviridkin
|
57c51f58ee |
feat(gateway): stop replicating the wildcard key into edge namespaces
The wildcard-secret controller replicates the operator's TLS Secret, private key included, into every namespace that owns a termination point, and a namespace owning a Gateway counted as one without further question. A Gateway whose class ends TLS at its provider's edge terminates nothing locally, so the replica has no consumer there. Read certMode off the tenant's own TenantGateway rather than a cluster-wide value, because the class is a per-tenant choice. Keep the key whenever the answer is not a confident no: a namespace that also owns an ingress controller still terminates locally, a namespace holding more than one TenantGateway keeps it while any of them is not at the edge, and an unreadable list or a namespace whose TenantGateway has not appeared yet keeps it too, so a transient API error never withdraws a key a Gateway may still be serving. Watch TenantGateway, so the withdrawal follows the class change rather than waiting for the periodic resync. The watched set is a value the tests assert on, because the controller-runtime builder exposes nothing about what it watched: a dropped watch would otherwise leave both suites green while the withdrawal quietly degraded to that resync, leaving the private key in a namespace that stopped terminating TLS. Pruning the replica is not the same as leaving the namespace key-free. The Certificate objects go, but the Secrets they issued stay, as they do on any other mode transition. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <3811295@gmail.com> |
||
|
Aleksei Sviridkin
|
2ef0e8d3a1 |
feat(gateway): add an edge cert mode for upstream-terminated classes
Some GatewayClasses have their provider terminate TLS in front of the Gateway and reach it over plain HTTP. On such a class the platform issued origin certificates nobody serves, and where ACME could not complete, the Gateway's listeners never became ready. Add certMode=edge to TenantGateway. The controller renders the apex, its wildcard and every inheriting child apex as plain HTTP listeners carrying the same label-based allowedRoutes as the HTTPS listeners of the other modes, so application HTTPRoutes attach by hostname unchanged. It mints no Issuer and no Certificate, and removes the http-to-https redirect route, because the edge performs that redirect itself. TLSPassthroughServices are not rendered: an HTTP-only edge cannot carry a TLS listener, which leaves the endpoints relying on one unpublished through such a Gateway. The hostname-less port-80 listener is left out. It carries the ACME challenge and the redirect in the other modes and has neither here, so it would only be a catch-all admitting any hostname. Leaving it out also keeps a hostname-pinned listener at index 0, which is what Cilium's first-listener-wins namespace check reads before the fix in cilium/cilium#45693. Moving into the mode garbage-collects the Issuer and Certificate objects the controller owned. The TLS Secrets they issued stay behind: cert-manager runs with enableCertificateOwnerRef false, so a Certificate does not own the Secret it issued into. Every mode transition already behaves that way. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <3811295@gmail.com> |
||
|
Aleksei Sviridkin
|
a2cb47c961 |
feat(gateway): update the CRD bundle to Gateway API v1.6.1 and gate GRPCRoute hostnames (#3872)
## What this PR does Bumps the vendored Gateway API CRD bundle in `packages/system/gateway-api-crds` from v1.5.1 to v1.6.1. The package Makefile pins the upstream ref, so the change is that pin plus the re-rendered `templates/crds-experimental.yaml`. The bundle version is not only bookkeeping: it is a compatibility signal consumers read at runtime. Every CRD in the bundle carries a `gateway.networking.k8s.io/bundle-version` annotation, and a Gateway API controller can read it off the `gatewayclasses` CRD to decide whether the installed bundle matches the one it was built against. Controllers that do this typically accept any patch release but require an exact major and minor match, and report `SupportedVersion=False` with reason `UnsupportedVersion` on their GatewayClass when the minor differs, a status condition that surfaces the skew rather than a functional break. The Cloudflare Tunnel Gateway API controller proposed for the platform in #3858 is built against Gateway API v1.6.1 and performs exactly that comparison, so against the v1.5.1 bundle shipped today its GatewayClass reports the bundle unsupported, and with this bump it reports supported. Neither change depends on the other landing first: this PR only moves the bundle, and the check lives entirely on the consumer side. One consequence worth stating plainly, since it will outlive this PR: an exact-minor check makes the shipped bundle version a coupled pin for any such controller. A future bump to v1.7.x flips the same condition back to `UnsupportedVersion` until that controller is rebuilt against the newer bundle, so the two want to move together. The bundle still emits the same twelve CustomResourceDefinitions and the same two `safe-upgrades.gateway.networking.k8s.io` admission objects, and every CRD still carries the `helm.sh/resource-policy: keep` stamp the Makefile's post-processing step adds. One of the policy's two CEL rules is rewritten, though, and it changes what a cluster accepts. That has its own section below. Two kinds change their version surface: TCPRoute and UDPRoute gain a served `v1` that takes over as the storage version, and their `v1alpha2` stays served with a deprecation marker. Every other kind keeps exactly the versions it served at v1.5.1. That matters because the Cilium operator resolves GatewayClass, Gateway, HTTPRoute and GRPCRoute in `v1`, ReferenceGrant in `v1beta1` and TLSRoute in `v1alpha2` before it enables its Gateway API controller; all six are still served by the v1.6.1 bundle. TLSRoute in particular keeps `v1alpha2`, which the tenant gateway controller, the platform TLSRoute templates and the tenant hostname admission policy all address. Schema changes are small and land on fields nothing in this repository sets. `sessionPersistence.idleTimeout` is gone from HTTPRoute, GRPCRoute and XBackendTrafficPolicy, removed upstream under GEP-1619 because no implementation ever implemented it and HTTP cookies have no native idle-timeout mechanism, so no working behaviour is lost even though the field disappears from the schema; HTTPRoute gains a lower bound on `retry.attempts` and now rejects duplicate `retry.codes`; ReferenceGrant requires `spec` at the root. In the other direction, Gateway infrastructure annotations and frontend CA certificate references go from a limit of 8 to 16, and TLSRoute hostnames from 16 to 1024. No CRD schema loses or changes a CEL validation rule, and the six added belong exclusively to the new TCPRoute and UDPRoute `v1` schemas. ### The safe-upgrades admission policy accepts a different set of bundles The experimental kustomization ships a `ValidatingAdmissionPolicy` and its binding, both named `safe-upgrades.gateway.networking.k8s.io`, next to the twelve CRDs. The binding is `validationActions: [Deny]` with `failurePolicy: Fail`, matching CREATE and UPDATE on `apiextensions.k8s.io/v1 customresourcedefinitions` cluster-wide, and Cozystack installs it in two places: on the host through `cozystack.gateway-api-crds`, and inside every guest cluster that sets `addons.gatewayAPI.enabled`. Its second rule reads the `gateway.networking.k8s.io/bundle-version` annotation off an incoming CRD. At v1.5.1 that rule was a blacklist: reject anything matching `v1.[0-4].\d+` or containing `v0`, allow the rest. At v1.6.1 it is a whitelist: allow `v0.0.0-dev` outright, otherwise require the annotation to start with `v1.` and not match `^v1\.[0-4](\.|$)`. Evaluating both expressions over a spread of annotation values: | bundle-version | v1.5.1 policy | v1.6.1 policy | | --- | --- | --- | | `v1.6.1`, `v1.5.1`, `v1.5.0-dev`, `v1.10.0` | allow | allow | | `v1.4.0`, `v1.4.10`, `v1.0.0` | reject | reject | | `v0.0.0-dev` | reject | **allow** | | `v2.0.0` | allow | **reject** | | `1.6.1` (no leading `v`) | allow | **reject** | | `v1.4` (no patch) | allow | **reject** | | `main`, empty string | allow | **reject** | Nothing in this repository trips the new rejections: every CRD the bundle ships is stamped `v1.6.1`. What is exposed is an operator applying a Gateway API CRD set from somewhere else on a cluster that carries the policy. A fork, a vendor build or a nightly whose annotation reads `1.6.1` or `v2.0.0` used to go in and is now denied with `reason: Invalid`, naming a policy the operator may not know is installed, and whatever HelmRelease owns those CRDs then sits un-Ready. Uninstalling the policy is the escape hatch, and the denial message says so. The same hunk fixes an upstream inconsistency: at v1.5.1 the policy pair was stamped `v1.5.0-dev` while its twelve CRDs said `v1.5.1`. At v1.6.1 all fourteen documents agree. The Gateway and HTTPRoute objects the tenant gateway controller renders were serialized across all three certificate modes and checked against both the v1.5.1 and the v1.6.1 schemas; the two runs are identical, so the bump does not change what the controller can create. `go.mod` still pins `sigs.k8s.io/gateway-api` v1.4.1 and this PR deliberately leaves it there. The CRD bundle and the Go client types have been on separate versions since before this change, and nothing the controller emits depends on the gap. One thing to know before the next bump: from v1.6 on, the two things upstream calls the experimental bundle disagree. This package follows `config/crd/experimental`, the kustomization, which emits 12 CRDs at v1.6.1; the published `experimental-install.yaml` for the same tag emits 13, the extra one being `xbackends.gateway.networking.x-k8s.io`, new in v1.6 and not listed in the kustomization. At v1.5.1 the two agreed, so there was nothing to choose between. Nothing in this repository references XBackend and no vendored CRD depends on it, so following the kustomization costs nothing today, but whoever bumps next inherits the choice, and it should be made deliberately rather than by whichever command gets typed. The second commit corrects a comment this bump invalidated. It claimed TLSRoute stays at `v1alpha2` through Gateway API v1.5 and that the promotion to `v1` was still upstream-only; TLSRoute has been served at `v1` since v1.5.0, so the claim was already wrong before this bump and the version range it named is now wrong too. The third commit closes a gap that correction exposed. `cozystack-route-hostname-policy-tls` named `v1alpha2` alone while the bundle serves `tlsroutes` at `v1`, `v1alpha2` and `v1alpha3`: three served versions against one named. The gap is not new and this bump neither creates nor widens it: the v1.5.1 bundle and the v1.6.1 one serve the same three versions, with the same two marked deprecated. Coverage of the two unnamed versions rested entirely on request conversion, because `matchPolicy` is unset on both rules in that file and so defaults to `Equivalent`, which rewrites a request submitted under any served version into the one the rule names before the policy sees it. That is why there is no bypass to demonstrate, and equally why the arrangement is brittle: the conversion has nothing left to convert into once no named version is served, and a rule that selects nothing is not caught by `failurePolicy: Fail`, because nothing failed. The rule now names all three, which is the shape the HTTPRoute rule above it already used, and `hack/route-hostname-policy-version-coverage.bats` reads the served versions out of the vendored bundle and fails when a policy does not cover every one of them, so a future bundle move cannot outrun the list silently, in either direction. That guard carries negative fixtures rather than only watching a correct tree pass. Three of them drive `assert_covers` itself, with the two readers redefined in a subshell, against an omitted served version, an empty served set and an empty named set. Each requires the specific complaint text rather than merely a non-zero exit, so a helper renamed out from under it fails for the right reason instead of reading as a successful rejection. A fourth drives the comparison helper directly. The fourth commit pins the vendored bundle version in the CRD package's unit suite. Nothing outside the package Makefile named a Gateway API version, so re-running `make update` against a different ref landed green: twelve CRDs, every one stamped, document count unchanged, suite passing. The new case reads the `gateway.networking.k8s.io/bundle-version` annotation off every rendered CRD and fails when it is not the version the Makefile pins. The fifth commit extends the version-coverage guard to `cozystack-gateway-hostname-policy`, which names the `gateways` versions literally the same way the two route policies do, and whose version set lives in the vendored bundle and moves on `make update` there. It matched the served set already; the guard now says so on every run instead of leaving one of the three hostname policies unpinned. Two notes for whoever reads the release: the TCPRoute and UDPRoute storage version moves from `v1alpha2` to `v1`, and while nothing in this repository ships either kind and the CRDs declare no conversion block, an upgraded cluster accumulates both entries in `status.storedVersions` — inert today, load-bearing only if a later bundle stops serving `v1alpha2`. The sixth commit gives GRPCRoute the same hostname rule the other two route kinds have. GRPCRoute carries `spec.hostnames` and it can reach a tenant Gateway: the port-443 listeners pin `allowedRoutes.kinds` to HTTPRoute and TLSRoute, but the port-80 listener leaves `kinds` unset, and an unset list resolves to the kinds the listener protocol supports, which for HTTP includes GRPCRoute. A GRPCRoute in a tenant namespace could claim any hostname on the plaintext listener with nothing checking it. Tenants hold no `gateway.networking.k8s.io` RBAC in Cozystack, so this is the same defence-in-depth against a chart bug that the other two rules are, not a tenant-user control. The rule is the HTTPRoute rule with `grpcroutes` in place of `httproutes` and the one version the bundle serves for it, and the coverage guard pins that list to the bundle like the others. Several places in the tree enumerate which route kinds the hostname policies cover, and all of them named two: the port-443 kinds invariant in the tenant gateway controller and the test that pins it, `packages/extra/gateway/README.md`, and the threat model together with the self-assessment. They name three now. The port-443 kinds set also carries an accurate reason now. cilium#45559 governs only that every port-443 listener carries the same set, not what is in it, so the contents are a least-privilege choice: nothing the platform ships needs gRPC, TCP or UDP routing on port 443. The README had also described HTTPS and TLS-passthrough listeners as carrying different kinds, where the controller gives both the same one. One more commit makes the suite's existing assertions bite. A ValidatingAdmissionPolicyBinding whose `policyName` points at another policy renders without complaint and leaves its own policy inert, and the suite pinned `validationActions` on the bindings while never pinning `policyName` at all, so pointing the GRPCRoute binding at the HTTPRoute policy kept every case green with the GRPCRoute control doing nothing. All three bindings now pin the policy they name. In the same file, `uncovered()` assigned to the variable names its caller reads back when printing a mismatch, so a call site with the arguments swapped would have printed a diagnostic naming the wrong side. Two things are named rather than made. The version-coverage guard collects `apiVersions` from every `resourceRules` entry mentioning the resource without also matching on `apiGroups` or `operations`, so a rule in another group, or one scoped to a single verb, would donate its versions to the set coverage is measured against; the chart's own suite pins the group and the resource on the rule it checks, which closes that a layer below. And `packages/system/cozystack-api/templates/api-tlsroute.yaml` renders `gateway.networking.k8s.io/v1alpha2` while the bundle serves `v1` as storage and marks `v1alpha2` deprecated, but the identical pin sits in the kubevirt and kubevirt-cdi TLSRoute templates, and the pinned `sigs.k8s.io/gateway-api` carries no `apis/v1` TLSRoute type for the controller side to follow, so moving one of the three here would leave the tree less consistent than leaving all three. Closes #3789. ### Screenshots Not a UI change. ### Downstream repositories Walked the trigger map in `docs/agents/contributing.md` against the diff, file by file: neither `packages/system/gateway-api-crds/Makefile` nor its rendered CRD template matches any entry. Leaving every box empty rather than ticking the "no downstream repository" line, because the docs site carries a Gateway API version statement that is already inaccurate and needs a decision that is not mine: `content/en/docs/next/networking/gateway-api.md` and its v1.5 and v1.6 copies say Gateway API v1.5 ships TLSRoute at `v1alpha2` and that the graduation to `v1` is still upstream-only, when v1.5.0 already shipped TLSRoute `v1` as the storage version. ### Release note ```release-note chore(gateway-api-crds): update the vendored Gateway API CRD bundle to v1.6.1. TCPRoute and UDPRoute gain a served v1 that becomes their storage version, with v1alpha2 still served and now deprecated; every other kind keeps the versions it served at v1.5.1. Upstream removed `sessionPersistence.idleTimeout` from HTTPRoute, GRPCRoute and XBackendTrafficPolicy; the field is not set by anything Cozystack renders, but a hand-written route that carries it loses it silently on its next write, with no admission error. The bundle's `safe-upgrades.gateway.networking.k8s.io` ValidatingAdmissionPolicy changes which CRDs it accepts. Cozystack installs it cluster-wide on the host and in every guest cluster with `addons.gatewayAPI.enabled`. Its `gateway.networking.k8s.io/bundle-version` rule was a blacklist on two patterns and is now a whitelist on the `v1.` prefix with a `v0.0.0-dev` escape hatch. A bundle annotated `v0.0.0-dev` is now accepted where it was denied. A bundle annotated `v2.0.0`, `1.6.1` without the leading `v`, `main`, `v1.4` without a patch, or carrying an empty annotation is now denied where it was accepted. Nothing Cozystack ships is affected, since every CRD in the bundle is stamped `v1.6.1`, but applying a Gateway API CRD set from a fork, a vendor build or a nightly on such a cluster can now fail admission with `reason: Invalid`. Uninstalling the policy remains the documented escape hatch. fix(cozystack-basics): the tenant TLSRoute hostname admission policy now names every served TLSRoute version (`v1`, `v1alpha2`, `v1alpha3`) instead of `v1alpha2` alone. Coverage of the other two rested on request conversion, which holds only while a named version stays served, and a rule left selecting nothing is not caught by `failurePolicy: Fail`. feat(cozystack-basics): GRPCRoute hostnames in `tenant-*` namespaces are now gated by the same admission policy that gates HTTPRoute and TLSRoute. A tenant Gateway's port-80 listener leaves `allowedRoutes.kinds` unset, which admits GRPCRoute by protocol default, so a GRPCRoute could claim any hostname on the plaintext listener unchecked. Tenants hold no `gateway.networking.k8s.io` RBAC, so this is defence-in-depth against a chart bug rather than a tenant-user control. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added hostname admission validation for `GRPCRoute` resources. - Expanded `TLSRoute` hostname validation to support all served Gateway API versions. - Ensured route hostname policies consistently cover HTTP, TLS, and gRPC routes. - **Security** - Updated listener policies to restrict port-443 routes to supported HTTP and TLS route types. - Clarified fail-closed hostname validation behavior across Gateway, Route, and Ingress resources. - **Documentation** - Updated security and threat-model documentation to include GRPCRoute handling and validation coverage. - **Maintenance** - Updated the bundled Gateway API CRDs to version 1.6.1. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
fb740db0b0 |
docs(security): separate the fail-open Gateway-listener policy from the fail-closed route policies
The self-assessment summarised the whole hostname policy set as fail-closed, while its own table row states that the Gateway-listener policy passes a Gateway whose namespace carries no host label. Say which policies are which. The port-443 kinds comment in the reconciler test also tied the GRPCRoute admission rule to the port-80 listener; the rule matches the kind wherever it attaches, and the port-443 exclusion is a least-privilege choice, not the reason the rule exists. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
af19aa933c |
docs(security): record GRPCRoute in the route hostname policy set
The port-443 kinds set still excludes GRPCRoute, TCPRoute and UDPRoute. cilium#45559 governs only that every port-443 listener carries the same set, not what is in it, so the contents are a least-privilege choice: nothing the platform ships needs gRPC, TCP or UDP routing on port 443. TCPRoute and UDPRoute additionally carry no hostname and no admission rule gates them. The gateway README described HTTPS and TLS-passthrough listeners as carrying different kinds. They carry the same one, which is what cilium#45559 requires. The threat model and the self-assessment enumerate which route kinds the hostname policies cover, and both listed two. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
9ea4a961ec |
fix(cacert): make the comments match the reconciler
Comments in the CA-extraction package described guards the code does not have. ManagedLabel: the anti-clobber decision is made by isOurProjection, from the Secret type and an owner reference back to the sentinel, not from the marker, which no decision keys on beyond the drift check. managedByCozystackLabel: the reconciler does mutate it. It deletes the label on a selectors-digest change, which is what hands the projection back to the lineage webhook for a fresh verdict. projectionSuffix: the projection name is kept clear of engine CA Secrets by a dot, and a release name cannot carry one, but the suffix an operator appends can and nothing validates it. A suffix ending in ".tenant-ca" is the only one that reaches a canonical name. TestProjectionNameCannotCollideWithADotFreeEngineSuffix: both its doc and its name claimed the property for any engine suffix while the loop samples only dot-free ones, which is the same overstatement one file over. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Andrey Kolkov
|
e94fcf8219 |
feat(backups): RabbitMQ definitions backup via a dedicated strategy driver (#3990)
## What this PR does Adds backup/restore support for the managed `RabbitMQ` application. RabbitMQ's cluster operator ships no backup CRD and RabbitMQ has no consistent online export of message payloads, so this backs up broker **definitions** (vhosts, users, permissions, queues, exchanges, bindings, policies, parameters) over the management HTTP API (`GET`/`POST /api/definitions`) and stores them in the system bucket. Message payloads and point-in-time recovery are intentionally out of scope; a full-data backup would be a Velero volume snapshot, which is a separate strategy. It is delivered as a **first-class `strategy.backups.cozystack.io/Rabbitmq` driver**, following the Altinity pattern (a dedicated driver that renders a one-shot Job for an HTTP-API engine): - `reconcileRabbitmq` / `reconcileRabbitmqRestore` render the strategy's `PodTemplateSpec` — `curl` `GET`/`POST /api/definitions`, pushing/pulling the JSON to S3 with `curl --aws-sigv4` — as a `batch/v1.Job`, covering both restore flows: in-place (into the source broker) and to-copy (into a separate, differently-named RabbitMQ in the same namespace, authenticating with the target's own default-user credentials and reading the source's object key). - A **Go-read precondition** gates each run on the `RabbitmqCluster` reporting `AllReplicasReady` (read through a small `rabbitmqtypes` partial type), so a not-yet-ready broker surfaces a precise `Ready=False` wait on the BackupJob/RestoreJob rather than a Job that starts and fails against an unreachable management API. - The produced `Backup` records the stored object's location on `status.artifact.uri`, rendered from the strategy's `spec.artifactURITemplate`. The `Backup` CRD has no status subresource, so this persists on Create, mirroring the Etcd driver. Dispatch is wired in both controllers (plus `supportedBackupStrategyKinds`), `rabbitmqtypes` is registered in the scheme, and RBAC grants read on `rabbitmqclusters.rabbitmq.com` (the Job itself is already covered by the existing `batch/jobs` grant). It ships `cozy-default-rabbitmq` (a `$bucketName`-gated `Rabbitmq` strategy) and routes `apps.cozystack.io/RabbitMQ` to it in the `cozy-default` BackupClass, which also enrols it in the `DefaultObjectsGate`. The RabbitMQ application chart is left untouched — it had no prior backup wiring, and the strategy carries the S3 coordinates itself. Unit tests cover the driver's pure helpers (applicationRef validation, cluster-name derivation, the readiness-message builder, artifact-URI rendering, parameter round-trip) and the reconcile precondition gate (defers with `Ready=False` and no Job until the cluster is ready, then proceeds). A runnable example under `examples/backups/rabbitmq/` and an honest-roundtrip Chainsaw e2e under `hack/e2e-chainsaw/rabbitmq/` seed a sentinel vhost, back it up, drop it, and assert it round-trips through object storage on both restore flows. The Job pod reuses the existing `platform-migrations` image (curl + ca-certificates) under a new `backupStrategyController.rabbitmqBackupClientImage` key, stamped in lockstep with `chBackupClientImage` in `packages/core/platform/Makefile` so the repository stays pinned at a single digest (`hack/image-pin-consistency.bats`). A dedicated slim image built by this package is a follow-up. **Validation:** `go build`/`go vet`/`gofmt`/`go test ./internal/backupcontroller/... ./internal/template/...`, deepcopy + Rabbitmq CRD generation, `helm template` proving the CRD-ordering gate both on and off, `helm lint`, and `helm unittest` (18/18). The backup + in-place + to-copy roundtrip was verified end-to-end on a dev cluster (a real management-API export and a real S3 `--aws-sigv4` upload/download against SeaweedFS), so the driver's core contract is exercised on a real cluster; the in-CI Chainsaw run is the remaining automated evidence. The object-cleanup, terminating-namespace teardown and readiness-deadline paths added during review are unit-tested (including `Reconcile`-level finalizer retention and the terminating-namespace release) but not yet cluster-exercised. (Note found while testing: RabbitMQ 4.2 is OOMKilled on the `nano` preset, so the example/e2e use `c1.small`.) ### Screenshots Not applicable — no UI changes. ### Downstream repositories Walked the trigger map against the diff. This is an additive backup strategy plus a new strategy CRD; no downstream repository is mechanically forced by the change. A documentation page for RabbitMQ backup on [cozystack/website](https://github.com/cozystack/website) would be a reasonable follow-up, but it is not required by this diff — leaving the boxes unticked for a human to decide rather than ticking "no downstream" to clear the checklist. ### Release note ```release-note feat(backups): add RabbitMQ definitions backup/restore via a dedicated strategy.backups.cozystack.io/Rabbitmq driver; it gates each run on the RabbitmqCluster being ready and records the stored object on Backup.status.artifact.uri. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added RabbitMQ backup and restore support for definitions, including vhosts, users, queues, exchanges, bindings, policies, and related settings. * Added in-place and restore-to-copy workflows with readiness checks. * Added scheduled and on-demand backup examples. * Integrated RabbitMQ backups with the default backup configuration. * Backups store definitions only; message payloads and point-in-time recovery are not included. * **Documentation** * Added a complete RabbitMQ backup and restore demonstration guide. * **Tests** * Added end-to-end coverage for readiness and backup/restore round trips. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Andrey KolkovandClaude Opus 4.8
|
ba32df69c0 |
feat(backups): add RabbitMQ definitions backup via a dedicated strategy driver
RabbitMQ's cluster operator ships no backup CRD and RabbitMQ has no
consistent online export of message payloads, so back up broker
DEFINITIONS (vhosts, users, permissions, queues, exchanges, bindings,
policies, parameters) over the management HTTP API and store them in the
system bucket. Message payloads and PITR are out of scope - a full-data
backup is a Velero volume snapshot, a separate strategy.
Add a first-class strategy.backups.cozystack.io/Rabbitmq driver, following
the Altinity pattern (a dedicated driver that renders a one-shot Job for an
HTTP-API engine):
- reconcileRabbitmq / reconcileRabbitmqRestore render the strategy's
PodTemplateSpec (curl GET/POST /api/definitions, push/pull to S3 with
--aws-sigv4) as a batch/v1.Job, for both in-place and to-copy restore.
- A Go-read precondition gates each run on the RabbitmqCluster reporting
AllReplicasReady (via the rabbitmqtypes partial type), surfacing a
precise Ready=False wait instead of a Job that starts and fails.
- The produced Backup records the object location on status.artifact.uri
(rendered from spec.artifactURITemplate; the Backup CRD has no status
subresource, so it persists on Create, as in the Etcd driver).
Wire dispatch in both controllers + supportedBackupStrategyKinds, register
rabbitmqtypes in the scheme, and grant read on rabbitmqclusters. Ship
cozy-default-rabbitmq (a $bucketName-gated Rabbitmq strategy) and route
apps.cozystack.io/RabbitMQ to it in the cozy-default BackupClass. The
application chart is untouched.
Add unit tests for the driver's pure helpers and the precondition gate, a
runnable example, and an honest-roundtrip Chainsaw e2e that seeds a
sentinel vhost, backs it up, drops it, and asserts it round-trips through
object storage on both restore flows.
The Job pod reuses the platform-migrations image (curl + ca-certificates)
under a new rabbitmqBackupClientImage key, stamped in lockstep with
chBackupClientImage so the repository stays pinned at one digest; a
dedicated slim image is a follow-up.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Andrey Kolkov <androndo@gmail.com>
|
||
|
Andrey KolkovandClaude Opus 4.8
|
5bcdb8cfb8 |
fix(postgres): let in-place restore re-bootstrap instead of wedging
An in-place RestoreJob (targetApplicationRef omitted) re-bootstraps the
Postgres app under its own name. On stock v1.6.2 it never comes back up,
for two independent reasons in the same restore patch - both surface only
in place, because a to-copy restore targets a different app.
1. Archive-vs-recovery serverName collision. The driver set the restored
cluster's WAL-archive serverName equal to the recovery source
(bootstrap.serverName = bootstrap.oldName = sourceServerName) while the
chart pinned the archiver serverName to the release name. In place those
coincide, so the cluster archived onto the source's own WAL prefix and
barman-cloud-check-wal-archive refused to start ("Expected empty
archive"). Fixed by giving the restored cluster a fresh
bootstrap.newServerName, keyed by the RestoreJob UID (unique per
restore, stable across reconciles); the chart archives there while
externalClusters still recovers from bootstrap.serverName. This is what
the manual CNPG strategies hit (serverName defaults to the cluster
name); the cozy-default flow dodged it (serverName is <ns>-<app>).
2. useSystemBucket guard. The restore populates explicit S3 coordinates
(destinationPath / s3CredentialsSecret / endpointCA) but left
backup.useSystemBucket=true, so the chart's "bootstrap.enabled is
incompatible with useSystemBucket" guard failed the HelmRelease upgrade
and the cluster never re-rendered. This is what the cozy-default
(system-bucket) flow hits on an in-place restore. Fixed by clearing
useSystemBucket in the restore patch, since the app is now on the
explicit-creds path. The driver's app view (postgresapp.Backup) gains
the field, without omitempty so a false survives the merge patch.
Both are exercised only by an in-place restore; the postgres backup e2e
drove only to-copy, so neither surfaced in CI. A step 50 in-place restore
now guards reason 1 (manual-strategy flow); reason 2 is covered by a Go
unit assertion (a system-bucket in-place e2e needs the platform bucket in
CI and is left as a follow-up).
Signed-off-by: Andrey Kolkov <androndo@gmail.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
Aleksei Sviridkin
|
3d076099a2 |
docs(postgres): name the trust anchor as the tenant actually sees it
The README told a tenant to fetch <release>.tenant-ca, but the release name carries the postgres- prefix the application definition applies, so the object is postgres-<name>.tenant-ca. Following the old text returned NotFound. Also drop two comments promising a companion ValidatingAdmissionPolicy that pins sentinel writes to helm-controller. No such policy ships: the proposal for it was rejected because authorization by username duplicates RBAC in a more brittle form. The RBAC boundary is the whole control. Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
22ce998890 |
fix(flux-shard-operator): repair sharded helm-controller crashloop behind an HTTP proxy (#3546)
## What this PR does Fixes #3544. `flux-shard-operator` clones the flux-aio `helm-controller` container into each `helm-controller-shard<i>` Deployment. It already sanitises the all-in-one localhost wiring, but keeps two things that stop the standalone shard from starting behind a corporate HTTP proxy: 1. **Inherited `HTTP_PROXY`/`HTTPS_PROXY`/`NO_PROXY`.** A standalone shard needs no external egress (source-controller fetches artifacts). Behind an unreachable proxy the controller makes a blocking startup HTTPS call that never completes, the manager never serves `/healthz`, and the liveness probe crashloops the pod forever. flux-aio survives only because it starts once and never restarts. On an affected cluster every HelmRelease carrying a `sharding.fluxcd.io/key` (150 of ~240) was frozen for days, silently. 2. **No `startupProbe`.** The clone inherits only a ~30s liveness window, so any slow start (large cache sync) is fatal and unrecoverable. This PR extends the sanitisation to drop the proxy env, and derives a `startupProbe` from the liveness handler with a generous budget so a slow-but-progressing start is not killed. Verified by an A/B on a live cluster (throwaway helm-controller with a non-matching selector): with the proxy env it hangs; without it, it starts cleanly (`Starting workers`). ### Downstream repositories - [x] No downstream repository is affected by this change Internal operator behavior only; no API/CRD/values contract change. ### Release note ```release-note fix(flux-shard-operator): sharded helm-controller no longer crashloops at startup behind an HTTP proxy; the cloned controller drops the inherited proxy env and gains a startupProbe, so HelmReleases assigned to shards keep reconciling in proxied environments. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Prevented inherited proxy settings, including uppercase and lowercase variants of `NO_PROXY`, from affecting Flux shard deployments. * Automatically adds a startup health check when only a liveness check is configured. * Preserved the liveness check’s handler and timeout while applying appropriate startup timing and failure settings. * **Documentation** * Documented startup health checks for slow helm-controller startup. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Andrey Kolkov
|
b30c7f4272 |
fix(backups): requeue MongoDB restore on a transient source-CR read error
resolveMongoDBBackupSource folded two failure classes into one error return:
a transient API read error on the live PerconaServerMongoDBBackup CR (cache
not yet synced, apiserver hiccup) and the terminal no-restorable-destination
case. The RestoreJob caller turned every error into markRestoreJobFailed, so a
retryable read error failed a restore that would have succeeded on the next
reconcile. The backup path already treats the symmetric live-CR read error as
transient (return ctrl.Result{}, err).
Wrap the transient read error in a new errTransientRestoreSource sentinel and
requeue on it at the caller; only a genuinely unresolvable source stays
terminal. Adds a non-vacuous interceptor-driven test.
Addresses the MINOR robustness note from PR review.
Signed-off-by: Andrey Kolkov <androndo@gmail.com>
|
||
|
Andrey Kolkov
|
b97c443a16 |
fix(backups): harden MongoDB restore auth + PITR option handling from review
Address the two blocking findings in PR #3562: - Restore into a differently-named target now authenticates with the TARGET cluster's own S3 credentials instead of the source's. The backupSource carried the source release's -s3-creds Secret, which is deleted together with the source app — the disaster-recovery case backups exist for — leaving a to-copy restore unable to reach S3. The restore replays into the target (gated on backup.enabled just above), whose own -s3-creds Secret is present and grants access to the same bucket; only the credential reference is swapped, bucket/prefix/endpoint still come from the source archive. Falls back to the source reference when the target declares no discoverable s3 credentials (in-place restore resolves to the same Secret, so it is a no-op there). - A non-string recoveryTime (spec.options is free-form runtime.RawExtension, so {"recoveryTime": 20260805} passes admission) no longer silently degrades a PITR request to a plain snapshot restore reported Succeeded. When the typed decode fails and the blob carries a recoveryTime key, the RestoreJob now fails terminally — the same guarantee pitrSpec already makes for a malformed RFC3339 string. Minor findings also addressed: - BackupJob tolerates a not-yet-present MongoDB app CR with the same bounded grace the psmdb-CR existence gate uses, instead of failing terminally on a GitOps single-commit ordering race. - examples: the to-copy target writes into its OWN S3 prefix (mongodb-target/) rather than the source's, so the two clusters' oplog and backup metadata no longer interleave under shared keys (both run replset rs0). - examples: wait_psmdb_ready no longer treats a transient state=error as terminal — the cluster state is recomputed each reconcile and can blip through error during replset init before converging to ready. Unit tests cover the target-credential selector (preferred/default/sole storage) and the load-bearing recoveryTime-key detection. Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey KolkovandClaude Opus 4.8
|
0d4a3e3134 |
refactor(backups): make MongoDB PITR option uniform with CNPG + warn on unknown restore options
PITR: replace the bespoke spec.options.pitr{type,date} blob with a single
spec.options.recoveryTime RFC3339 string, matching the CNPG strategy's option
exactly (CNPGRestoreOptions.RecoveryTime). The driver converts recoveryTime to
the psmdb Restore's pitr{type:date,date} in the operator's "YYYY-MM-DD HH:MM:SS"
UTC format. An empty recoveryTime restores the snapshot as-taken (documented
difference from CNPG, whose empty value means latest WAL — a psmdb logical
backup is already consistent). A non-RFC3339 recoveryTime fails terminally.
Unknown options: parseMongoDBRestoreOptions now also returns the set of
spec.options keys the driver does not recognise; reconcileMongoDBRestore emits a
UnknownRestoreOption Warning event per unknown key (and logs it) instead of
silently ignoring it — so a "recoverytime" typo no longer silently degrades a
PITR restore to a snapshot restore. (BackupJob carries no spec.options blob, so
there is no backup-path equivalent; the only backup-time input is the free-form
BackupClass parameters used as template vars.)
Docs + examples updated to recoveryTime; tests cover the RFC3339→UTC mapping,
empty/invalid recoveryTime, and unknown-key reporting.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Andrey Kolkov <androndo@gmail.com>
|
||
|
Andrey KolkovandClaude Opus 4.8
|
540bb94f58 |
test(backups): cover createMongoDBBackupArtifact producer seam
Address branch-review blocker: add TestCreateMongoDBBackupArtifact_ArtifactShape, mirroring the MariaDB sibling. createMongoDBBackupArtifact writes the driver-metadata keys (backup-name/namespace/destination), the Status.Artifact URI, and the persisted snapshot that the restore path reads back — the producer half of a seam previously exercised only end-to-end. A key-name drift between this producer and resolveMongoDBBackupSource would otherwise pass every unit test and surface only in e2e. The test asserts the metadata keys, phase, artifact URI, and that the snapshot decodes to a usable by-reference backupSource; a second case pins the empty-destination path (nil Artifact, no destination key). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey KolkovandClaude Opus 4.8
|
bc5e47c64f |
fix(backups): add explicit MongoDB cleanup-on-delete branch + its guard test
Address branch-review recommendation: give the MongoDB strategy an explicit case in BackupReconciler.cleanupOnDelete, matching CNPG / Altinity / MariaDB / FoundationDB. A deleted MongoDB Backup artifact previously fell through to the Velero default (a safe no-op — MongoDB driverMetadata never carries velero.io/backup-name), but the explicit branch documents the "we do not own the psmdb S3 archive" contract and guards the seam against a future shared helper incidentally stamping velero.io/backup-name onto MongoDB metadata. Ships with TestBackupCleanup_MongoDB_DoesNotFallThroughToVelero, mirroring the one-test-per-branch convention its three siblings follow: it seeds the velero metadata keys + a matching velero.io/Backup and asserts no DeleteBackupRequest is synthesised and the Velero Backup survives — so deleting the branch makes the test fail, which is what makes the guard self-verifying. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey KolkovandClaude Opus 4.8
|
dff56aa998 |
feat(backups): add a native MongoDB (psmdb) backup-strategy driver
Adds a MongoDB (Percona Server for MongoDB / psmdb) backup-strategy driver so `apps.cozystack.io/MongoDB` reaches feature parity with the CNPG, MariaDB and ClickHouse native drivers — logical backups, restore-to-differently-named instances, and point-in-time recovery through the unified BackupJob/RestoreJob/BackupClass/Plan interface (closes #3545). The driver orchestrates the operator's PerconaServerMongoDBBackup / PerconaServerMongoDBRestore CRDs: - BackupJob: creates a one-shot PerconaServerMongoDBBackup, polls status.state, and persists the S3 destination + storage descriptor (by-reference, no credentials) onto the Cozystack Backup artifact. - RestoreJob: drives PerconaServerMongoDBRestore via backupSource (read back from the source backup status, snapshot fallback) so the same artifact restores in-place or into a differently-named instance; PITR via spec.options.pitr (date/latest). Because psmdb resolves the backup storage by name against the cluster's spec.backup.storages, the driver names the chart's storage (s3-storage) via the cozy-default-mongodb Strategy rather than injecting it, and surfaces a clear Ready=False precondition when a target has backups disabled. Wiring: - api/backups/strategy/v1alpha1: MongoDB strategy type + generated CRD/deepcopy. - internal/backupcontroller: psmdbstrategy_controller + psmdbtypes/mongodbapp partial-schema mirrors; dispatch, supported-kinds and scheme registration. - backupstrategy-controller: cozy-default MongoDB route, cozy-default-mongodb Strategy CR, and psmdb.percona.com RBAC. - apps/mongodb: backup.insecureSkipTLSVerify knob (psmdb s3 storage has no CA-bundle field) so backups can target Cozystack's self-signed seaweedfs. Tests + e2e: - unit tests for the driver (validation, ensure-idempotency, precondition, backupSource resolution, snapshot round-trip, PITR option parsing). - examples/backups/mongodb + a mongodb-2-backup-roundtrip Chainsaw suite that drives run-all.sh end-to-end (sentinel document round-trips through S3 into a restored copy while the source stays untouched), per docs/agents/e2e-testing.md. - docs/operations/backup-classes.md updated. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Aleksei Sviridkin
|
ec6a835251 |
fix(manifestutil): report one message when the CRD wait is cancelled (#3742)
<!-- Thank you for making a contribution! Here are some tips for you: - Use Conventional Commits for the PR title: `type(scope): description` - Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore - Scopes are not an exhaustive list — pick the most specific scope for the change and extend the list when a genuinely new area appears. Examples: - System components: dashboard, platform, operator, cilium, kube-ovn, linstor, fluxcd, cluster-api - Managed apps: postgres, mariadb, redis, kafka, clickhouse, virtual-machine, kubernetes - Development and maintenance: api, hack, tests, ci, docs, maintenance - Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or add a `BREAKING CHANGE:` footer - If it's a work in progress, consider creating this PR as a draft. - Don't hesistate to ask for opinion and review in the community chats, even if it's still a draft. - Add the label `backport` if it's a bugfix that needs to be backported to a previous version. --> ## What this PR does `WaitForCRDsEstablished` reported a cancelled context two different ways. The check at the top of the poll loop returned a message with no CRD name, the `select` at the bottom named the CRD the poll had stopped on. When a tick and the deadline come ready in the same instant, `select` picks between the ready cases at random, so one cancellation reached the operator either way. Both callers wrap this as `CRDs not established after apply`, so the CRD name is what makes that log line actionable, and whether it is there was a coin toss. `TestWaitForCRDsEstablished_timeout` asserts on the name, which is where this surfaced: its 2s deadline is an exact multiple of the 500ms ticker, so the two channels come ready together on the fourth tick and the assertion failed at random. The message is now built in one place and returned from both exits, so the two are indistinguishable by construction rather than by inspection. The name is carried across iterations, because a cancellation noticed at the top of the loop has no poll of its own to take one from, and it starts as the first CRD of the set for a wait cancelled before any poll observed anything. That start value is a stand-in rather than an observation, and the comment on it says so; the alternative, an empty name and a second phrasing for it, would leave the loop-top message with nothing to assert against and no test able to catch its return to the unnamed form. Deleting the top check instead, so there is literally one exit, is the smaller diff and it moves the name to the head of the list in the very interleaving this PR is about. A client refuses `Get` on a dead context at the first name in the list whichever CRD is actually lagging, so a poll entered on a cancelled context renames the report to that first name. That premise is about the production client, `client.New` at `cmd/cozystack-operator/main.go:176`, uncached, so the request is rejected before it is issued, and it is worth saying that nothing in the test surface pins it: replacing the `ctx` passed to `Get` with `context.Background()` survives the whole package, and the fixtures that need a context-honouring client check `ctx.Err()` by hand. That line predates this change and is untouched by it. Measured with a client that honours the context: with the check, a wait that re-enters the loop after the deadline keeps the name its last poll settled on; without it, the report moves to the head of the list. The check closes that window and no more than that. A context that dies part-way through a poll still records the first `Get` to fail, exactly as merge base does, because this branch cannot tell a refused request from a CRD status. That is unchanged here and written up separately. Four tests cover the ways this wait can end on a cancelled context: before any poll has observed the set, cut off inside a poll, cancelled while waiting for the next tick, and re-entering the loop on a context that died during that wait. Two of them are red on merge base and are the regression pins for the defect above, both deterministically rather than by repetition: merge base answers a wait that starts cancelled, and a wait that re-enters the loop cancelled, with the unnamed message every time. Its loop-top exit is a `select` whose `Done` case is ready, and a ready case always beats `default`. The middle two are green on merge base, which gets those cases right; they are here because each catches a specific way of breaking the fix. Every one of the four is the sole killer of at least one mutation, which is the property worth having: it says none of them is redundant. They compare the whole message rather than searching it for a name, because a second exit that happens to carry a name is still the defect being fixed here, and a test that only looks for the name cannot see it. The re-entry test needs the bottom `select` to take the tick while cancellation is already pending, which against a real context is the same coin toss the bug rides on. The double it uses hands out an open `Done` for the single read that `select` makes in that iteration and a closed one for every read after, so the tick wins by construction and the next iteration sees a context cancelled through both `Done` and `Err`, the way a real one is. That pairing is guaranteed rather than incidental: `cancelCtx.Err` reads `<-c.Done()` before returning a non-nil error, so a double that reported cancellation through `Err` alone would be modelling a state no context can be in, and would pin the shape of the check rather than its behaviour. One property of that double is worth knowing before editing the loop: it hands out its open and closed channels per read, so a `Done()` read added inside the poll would consume the open one and leave the bottom `select` a closed channel, quietly turning this into a second bottom-exit test that still passes. The failure direction there is false-green, which is the worse half, and the cheap guard is a lower bound on elapsed time inside that test, a run that leaves through the loop top spends a ticker interval getting there, one that leaves at the bottom of the first iteration does not. Not done here, because the test is correct today and provably kills the mutation it exists for, so the hazard is conditional on a future edit. The timeout test does reproduce locally under load: 6 failures across 400 runs on the unfixed code while I was measuring, spread over five batches, the last of which saw none. #3719 reports that it does not reproduce locally, so this is worth writing down before someone spends an evening re-testing that. That spread is also why the number is an order of magnitude and not a rate. `TestInstall_crdNotEstablished` in `internal/crdinstall` has the same 2s deadline against the same 500ms ticker. It asserts on the `CRDs not established` wrapper the caller adds, and both messages carried that wrapper, so this coin flip could not reach it and nothing here touches it. I left the 2s deadline where it is. Moving it off the tick boundary would have hidden the coin flip and left the second message in place. The new tests catch a reintroduced second message deterministically, which is what that job now rests on rather than the timeout test's roughly one-in-sixty. ### Screenshots <!-- REQUIRED for UI changes: attach screenshots or screen recordings demonstrating the visual impact of your changes. PRs with UI changes without screenshots will not be merged. --> ### Downstream repositories <!-- Cozystack is upstream for repositories that are NOT kept in sync with it automatically, and nothing in CI compares the two sides, so a change here can break them silently. Which change forces what, and which file to touch, is the trigger map in docs/agents/contributing.md under "Downstream Repositories". Walk it before you tick anything. IF YOU ARE AN AI AGENT OR AN LLM, THIS IS PART OF THE TASK, NOT A FORM TO FILL IN: 1. Walk the trigger map against the actual diff of this PR, file by file. Do not decide from the PR title or from your memory of what you changed. 2. For every repository you tick, open the follow-up PR there and link it on the same line. A ticked box with no link claims work that does not exist, which is worse than an empty box. Search that repository for an open PR or issue covering it first, and link that instead of filing a duplicate. 3. When the follow-up is out of scope, or needs a decision that is not yours to make, open an issue in that repository instead, link that, and say so in the PR body. Do not push a speculative PR into another repository just to fill a line here. 4. Do not tick "No downstream repository is affected" to make the checklist go away. If you are unsure, leave every box empty and say so in the PR body, so a human decides. --> - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note <!-- Write a release note: - Explain what has changed internally and for users. - Start with the same `type(scope):` prefix as in the PR title - Follow the guidelines at https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md. --> ```release-note fix(manifestutil): a CRD wait cut short by a cancelled context now reports one error naming the CRD it was waiting on, where before the name was present or missing depending on which of two exits the scheduler happened to pick ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved cancellation handling while waiting for custom resources to become established. * Cancelled operations now stop before issuing unnecessary requests and identify the affected resource. * Timeout errors remain distinguishable from cancellation errors. * **Tests** * Added coverage for cancellation before, during, after, and between polling attempts. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
5747747087 |
fix(manifestutil): report one message when the CRD wait is cancelled
WaitForCRDsEstablished reported a cancelled context two different ways. The check at the top of the poll loop returned a message with no CRD name; the select at the bottom named the CRD the poll had stopped on. Which one ran was left to the scheduler: when the deadline and a tick come ready in the same instant, a select picks between the ready cases at random, so one cancellation reached the operator either way. Build the message in one place and return it from both exits, so the two are indistinguishable by construction. The name is carried across iterations, since a cancellation noticed at the top of the loop has no poll of its own to take one from, and starts as the first CRD of the set for a wait cancelled before any poll observed anything. Keeping the check at the top is what makes carrying the name worthwhile. A client refuses Get on a dead context at the first name in the list whichever CRD is actually lagging, so a loop that re-entered and polled anyway would overwrite the name its last poll settled on. The check closes that window and no more than that: a context dying part-way through a poll still records the first Get to fail, because this branch cannot tell a refused request from a CRD status. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <3811295@gmail.com> |
||
|
IvanHunters
|
66df3b20e0 |
docs(flux-shard-operator): drop unproven mechanism from proxy comments
Name all six proxy env spellings in the sanitisation list, and replace the unestablished 'external HTTPS call' / 'flux-aio does not restart' claims with the grounded in-cluster mechanism: after the KUBERNETES_SERVICE_HOST drop above, the shard talks to the management apiserver over the kubelet-injected ClusterIP, which NO_PROXY=.svc does not match, so that startup call is what stalls behind an unreachable proxy. Comment-only; no behaviour change. Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
a10fcb3e4a |
test(flux-shard-operator): assert full startupProbe contract and reject alias regression
Model the source liveness probe as flux-aio actually ships it (a bare httpGet with no timing fields) and tighten the startupProbe assertions: - assert the complete normalised budget (delay/period/success/failure) instead of only FailureThreshold, so a later PeriodSeconds regression cannot silently restore a short crashloop window; - assert TimeoutSeconds is inherited from the source probe rather than a hardcoded value, so forcing it stricter is caught; - assert the startupProbe is a DeepCopy of liveness, not an alias, which would otherwise stamp the 30-failure startup budget onto liveness and produce a never-failing liveness probe. The alias assertion is non-vacuous: it fails when the DeepCopy is regressed to a plain alias. Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
036a31b550 |
docs(flux-shard-operator): record proxy-env drop and startupProbe in sanitisation list
The BuildShardDeployment doc comment and the operator README are the only record of how far the cloned shard container has diverged from flux-aio. Add the two steps this fix introduced (dropping the inherited corporate-proxy env and deriving a startupProbe) so the list stays the authoritative account and does not invite re-adding what was deliberately removed. Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
Mattia Eleuteri
|
f859b710a4 |
fix(backupstrategy-controller): repair the bucket credentials Secret from the gate, not a chart fail
Addresses the four findings on #3524. **The `fail` in the bucket chart (MAJOR).** The <bucket>-<user>-credentials Secret was made to `fail` the render while its COSI lookup was empty, so the release would retry and converge instead of skipping the Secret permanently. Helm cannot render a partial set, so that fail aborts the WHOLE <bucket>-system release: the other users' Secrets, and the bucket UI Deployment/Service/Ingress/HTTPRoute with them. One declared user whose BucketAccess never provisions takes down every other user of that bucket, and on an already-installed release parks it in Failed, blocking every later upgrade. **No per-bucket escape (MAJOR).** Rather than add one, remove the need for it. The gate already forces the release that renders the lookup-gated Strategy CRs; the credentials Secret is the same trap one release earlier, and the gate reads that Secret to resolve the bucket name in the first place. So while it is absent, or carries no bucket name, force the bucket's <bucket>-system release instead of doing nothing. That cannot deadlock its own precondition: the BucketClaim and the BucketAccess whose COSI Secret the lookup reads are rendered unconditionally by the parent release, not by the one being forced. The two releases are throttled independently — they are forced in sequence on a bootstrap, so a shared timestamp would delay the second by a full MinForceInterval for no reason. Coordinates come from the chart under provisionBucket, and are omitted on external S3 where the Secret is admin-managed and no release renders it. No RBAC delta: patch on helmreleases is already cluster-scoped. The chart goes back to a partial render, with the comment now explaining why the skip is not self-healing and why failing is the wrong lever, and a test that renders the UI with every user unresolvable so a reintroduced `fail` fails the suite. Tenant buckets keep their pre-existing behaviour; generalising the repair to every bucket needs a controller that owns them and is tracked separately. **Stale gauge (MINOR).** cozystack_backup_default_objects_missing was only written on the happy path, so the state the gate exists to catch reported 0 and the documented alert never fired. It is now written on the unresolved path too — the credentials Secret counts as one of the objects the default backups depend on, which is what it always was. On an API error the gauge is deliberately left alone rather than flapping, so a new cozystack_backup_default_objects_check_errors_total marks it stale, and the doc pairs the two instead of overstating the gauge. **Suspended releases (MINOR).** forceHelmRelease now does a point Get and skips a release with spec.suspend: true. helm-controller ignores forceAt and requestedAt while suspended (`cozyhr suspend` sets exactly that), so the gate was re-stamping every MinForceInterval for the whole suspension and climbing the force counter — which the runbook attributes to a render that is not producing the objects, the wrong diagnosis. The skip is logged and not counted, so a climbing counter keeps its documented meaning. Reported-by: IvanHunters <xorokhotnikov@gmail.com> Signed-off-by: Mattia Eleuteri <mattia@hidora.io> Assisted-By: Claude <noreply@anthropic.com> |
||
|
IvanHunters
|
8b006391c2 |
test(backupstrategy-controller): mirror all six cozy-default routes in the gate fixture
The gate reads the BackupClass as the manifest of what must exist, but the Go fixture listed only CNPG, Etcd and the two Velero routes while the shipped backupclass-default.yaml also routes MariaDB and Altinity. Add both to the fixture, and to the RESTMapper and list-kinds derived from it, so the expected-object set the tests exercise equals the real route set. Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
bf152ba864 |
fix(backupstrategy-controller): resolve the default BSL through the RESTMapper
The BSL existence check used a hardcoded GVR and the dynamic client, whose Get returns a plain 404 (IsNotFound) for an unserved API group. The IsNoMatchError branch meant to skip an absent Velero API was therefore dead code, and if Velero were uninstalled after bootstrap while bslEnabled=true the BSL would be counted missing and force a Helm upgrade forever. Route the lookup through the RESTMapper like the strategy loop, so an absent velero.io API is a NoMatch we skip. A regression test fails without the change. Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
aa3accd128 |
fix(backupstrategy-controller): stop the default-objects gate looping when velero.bslEnabled=false
The cozy-default BackupClass routes VMInstance/VMDisk to the Velero Strategy CRs unconditionally, but the chart gates those CRs on velero.bslEnabled. With the BSL disabled they never render, so the gate counted them as permanently missing and forced a Helm upgrade every MinForceInterval forever, climbing force_reconciles_total and pinning the missing gauge. Skip Velero-kind strategyRefs when the BSL is disabled (empty VeleroNamespace), the same flag that gates their render. The regression test now omits the manually-created Velero objects so it fails without the skip. Correct the values.yaml comment that wrongly claimed the Strategy CRs still ship in this mode. Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
affdd70e9d |
fix(flux-shard-operator): inherit probe timeout, correct comments
Review follow-ups: - Do not force the cloned startupProbe TimeoutSeconds to 1: keep the value inherited from the liveness handler, otherwise a future flux-aio with a larger liveness timeout would get a stricter startup probe and could recreate the crashloop. Covered by a regression guard (liveness timeout 5s must be inherited). - Soften the proxy-strip rationale to the A/B-confirmed mechanism (blocking external HTTPS call at startup) and note the remote spec.kubeConfig exception that does not apply to cozystack in-cluster guest apiservers. - Correct the test fixture comment: the proxy env is operator/patch-level, not installer-injected (the installer injects only KUBERNETES_SERVICE_*). Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
d44c87d939 |
fix(flux-shard-operator): add startupProbe to cloned helm-controller
The cloned shard Deployment inherits only a liveness probe (~30s window, no startupProbe), so a controller still syncing caches on startup is killed before it serves /healthz and never recovers. Derive a startupProbe from the liveness handler with a generous budget so a slow start is not fatal, while liveness still catches a wedged running pod. Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
c475faae0f |
fix(flux-shard-operator): strip proxy env from cloned helm-controller
A sharded helm-controller inherits HTTP_PROXY/HTTPS_PROXY/NO_PROXY from the flux-aio helm-controller container. A standalone shard needs no external egress (source-controller fetches artifacts), so behind an unreachable corporate proxy it makes a blocking startup HTTPS call that never completes, the manager never serves /healthz, and the liveness probe crashloops the pod forever, freezing every HelmRelease carrying a sharding key. flux-aio itself survives only because it starts once and never restarts. Drop the proxy env (and the now-pointless NO_PROXY) when cloning, alongside the existing localhost/KubePrism sanitisation, so the shard talks to the in-cluster apiserver directly. Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |