mirror of
https://github.com/cozystack/cozystack.git
synced 2026-11-03 18:47:04 +09:00
fix/bats-runner-coverage
326
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 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> |
||
|
Mattia Eleuteri
|
e85501e9f5 |
fix(backupstrategy-controller): bound each gate check and tolerate an absent Secret
Two defects found in review of the periodic recovery loop. checkAndLog passed the manager's root context straight into Check, which makes several sequential API calls. That context is only cancelled at shutdown, so an API server stalling on any one of them blocked the call indefinitely. The ticker loop is sequential, so one stuck call stopped every later check for the life of the pod: the recovery this gate exists to provide went silent, leaving a stale gauge and no further log line, with only a restart to bring it back. Each check now runs under a timeout of half the Period, capped at 30s, so it can never overlap the next tick. Check also propagated a NotFound on the projector's source Secret, although the very next branch treats an empty bucket name as an expected no-op. Those are the same bootstrap state seen a moment apart, so the absent Secret logged a check failure on every tick of the whole bootstrap window for a condition the doc comment frames as graceful. Both are pinned by tests that fail against the previous behaviour: one reverts to an error on the absent Secret, the other to a timeout equal to Period. Signed-off-by: Mattia Eleuteri <mattia@hidora.io> |
||
|
Mattia Eleuteri
|
51ad226706 |
fix(backupstrategy-controller): repair lookup-gated backup objects
The default Strategy CRs and the Velero BackupStorageLocation are Helm-templated behind a `lookup` of the BucketClaim the same chart creates, and the <bucket>-<user>-credentials Secret behind a `lookup` of the COSI Secret. When those lookups are empty at install time the objects were silently skipped, and helm-controller does not re-render a release whose chart and values did not change (drift detection is off on operator-generated HelmReleases), so the skip was permanent: clusters ran for months with only BackupClass cozy-default and no Strategy CRs at all. That also fail-closes the pre-adoption snapshot in the v1.6.0 etcd migration, which reads the projected credentials Secret. Add a DefaultObjectsGate runnable to backupstrategy-controller. Once the bucket name is resolvable from the projector's source Secret, it checks that every object cozy-default routes to exists and forces one real Helm upgrade (reconcile.fluxcd.io/forceAt + requestedAt) when any is missing. Helm remains the objects' only author; the gate only makes sure the render that produces them actually happens. It cannot be a render-time `fail` in this chart: the chart is the producer of the Bucket its own lookup reads, so a failed render would deadlock the condition. The bucket chart's user-credentials template CAN fail safely — the BucketAccess that produces the COSI Secret belongs to the parent release and that release retries forever — so make it fail loudly instead of skipping, with requireUserCredentials=false as the offline-render escape. Also correct the chart and doc comments that claimed Flux re-renders on its interval, and document the two-release manual recovery for clusters already affected. Signed-off-by: Mattia Eleuteri <mattia@hidora.io> |
||
|
Timofei Larkin
|
95e4faf68e |
feat(workloadmonitor): bucket metrics endpoint override
The controller discovers where to query SeaweedFS bucket size metrics via the namespace.cozystack.io/monitoring label, which assumes the monitoring stack that scrapes SeaweedFS runs in the same cluster. Some deployments host SeaweedFS and its metrics in a separate cluster, so add a --seaweedfs-metrics-endpoint flag to cozystack-controller (and a matching cozystackController.seaweedfsMetricsEndpoint chart value): an explicit base URL of a Prometheus-compatible query API, with scheme, host, optional port and path prefix; /api/v1/query is appended. Basic auth may be embedded as URL userinfo. The flag is validated and normalized at startup, so a malformed URL fails the rollout rather than the queries. Empty keeps label-based discovery unchanged. Bucket sizes feed billing, so metrics failures no longer collapse to "no size": previously any query failure yielded an empty result map and the reconciler overwrote Workload resources with it, silently wiping the recorded sizes. The reconciler now retains the last known s3-storage-bytes and s3-physical-storage-bytes values, emits a BucketMetricsUnavailable warning event on the WorkloadMonitor, and returns the error from Reconcile so it lands in the reconcile error metrics. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Timofei Larkin <lllamnyp@gmail.com> |
||
|
Aleksei Sviridkin
|
6ecb36ffc7 |
feat(cozystack-controller): extract application CA into a key-free tenant Secret (#3407)
## What this PR does Adds an engine-agnostic controller that publishes a key-free `<release>.tenant-ca` Secret (only `ca.crt`) for a managed application, so a tenant can verify the application's TLS without ever seeing a private key. The source is declared, not guessed: a chart renders a namespaced `TenantProjection` sentinel (group `internal.cozystack.io`) naming the CA Secret to lift; the controller watches the sentinel, extracts `ca.crt`, and writes the projection owner-referenced to the sentinel, so garbage collection is native. The projection carries the `internal.cozystack.io/tenant-ca` label the lineage webhook turns into tenant visibility. Postgres is the first consumer. The trust boundary is RBAC: no tenant role grants any verb on `internal.cozystack.io`, so a tenant cannot forge a sentinel. A companion `ValidatingAdmissionPolicy` that pins the writer to helm-controller ships separately as defence in depth. This is the controller-and-CRD slice, split out from #3299 for focused review. It is the only piece required for the feature to work; the render-time helper, the writer policy, the capability-gate, and the release-prefix validation land as their own PRs. ### Downstream repositories Walked the trigger map against the diff. This adds an internal CRD and controller and a sentinel to the Postgres chart; no app is added or renamed and no `values.schema.json` changes, so the typed provider and Ansible are unaffected. The one to confirm is the Postgres README edit — if the website mirrors it, it needs a refresh. - [ ] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: ### Release note ```release-note feat(cozystack-controller): publish a key-free per-application CA trust anchor (`<release>.tenant-ca`) that tenants read to verify managed-application TLS, declared through a new `TenantProjection` CRD. Postgres is the first consumer. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added tenant-facing CA certificate projections for PostgreSQL deployments. * Tenant secrets now provide only the CA certificate, excluding private keys. * Added support for configuring and monitoring tenant CA projections. * **Documentation** * Updated PostgreSQL TLS instructions to retrieve certificates through tenant secrets. * **Bug Fixes** * Improved protection against invalid certificates, secret collisions, and accidental exposure of private key material. * Added validation to ensure projected certificates remain synchronized with their sources. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Andrey Kolkov
|
8aeedde088 |
feat(backups): support PostgreSQL point-in-time recovery (PITR) (#3383)
## What this PR does Makes PostgreSQL point-in-time recovery (PITR) a genuinely supported, tested capability for the new `backups.cozystack.io` API. Fixes #2774. Was **stacked on #3313** (`fix/3300-cnpg-barman-plugin`) for the `recoveryTime` → CNPG `bootstrap.recovery.recoveryTarget.targetTime` plumbing; #3313 has since merged and this branch is rebased on `main`. This PR adds the reliability + verification layer #2774 asked for. ### Changes - **Precise failure reason for an unreachable target.** The restore deadline (`spec.options.restoreTimeoutSeconds`, default 30m) stays the sole authority for "this restore is stuck" — the client waits the full window either way. What changes is the *diagnosis*: once the deadline has elapsed, if `recoveryTime` was set and the recovery Cluster never went healthy, the CNPG driver reads the `full-recovery` pod logs and — when several distinct attempts exited with PostgreSQL's `FATAL: recovery ended before configured recovery target was reached` — fails the RestoreJob with `RecoveryConverged=False` / `Ready=False` (reason `RecoveryTargetUnreachable`) and an actionable message, instead of a bare generic timeout. Timing is unchanged; only the reason/message differ. (For a faster rejection, set a shorter `restoreTimeoutSeconds`.) Design points: - **We deliberately do NOT trip early off the FATAL.** A valid near-now target emits that same FATAL transiently — repeatedly, for many attempts, on a slow archiver — before it converges, so any earlier trip would wrongly reject a recoverable restore (observed twice in CI). The FATAL is used only to *explain* a failure the deadline has already declared. - **Health wins unconditionally** — the deadline/classification check precedes the healthy check, but a recovery that converges within the window reaches success and is never classified as failed (a live dev7 restore did exactly this). - **Counting distinct FATAL-ing attempts** (not pod failures) tolerates a one-off archive-lag miss on a near-now target and ignores transient recovery-pod crashes (a node blip / brief API-server unreachability never emits that FATAL). - Adds the `pods/log` RBAC grant the log read needs (distinct subresource from `pods`); a `Forbidden` is logged loudly and surfaced in the failure message rather than silently disabling the classification. - **e2e** (`examples/backups/postgres/run-all.sh`, wired into the Chainsaw `postgres-2-backup-roundtrip` suite): - Step 45 — write `before`, capture server timestamp (µs precision), write `after`, restore to that timestamp, assert `before` present and `after` absent. - Step 46 — a `recoveryTime` an hour ahead must fail with reason `RecoveryTargetUnreachable` once the deadline elapses (asserts the *reason*, so it proves the classification fired, not just that a timeout occurred). - Chainsaw budget raised 25m → 40m to cover three sequential restores + the post-deadline classification. - **Docs + example**: uncommented `45-restorejob-pitr.yaml`; a `docs/operations/backup-classes.md` PITR section (recoverable window, discovering earliest/latest restorable time from the backup catalog since `firstRecoverabilityPoint` is unreliable under the plugin, archive-lag tolerance, idempotency under GitOps). ### Verification - `go build` / `go vet` clean; unit tests pass (`logIndicatesRecoveryTargetUnreachable`, `recoveryUnreachableFromLogs`, `recoveryPodsToInspect`, nil-Clientset guard, a reconcile-level success-path test asserting `Succeeded` + `RecoveryConverged=True`, and — added per review — `TestReconcileCNPGRestore_DeadlineClassifiesRecoveryTargetUnreachable`, a reconcile-level test driving the deadline branch through all three sub-paths: FATAL found → reason `RecoveryTargetUnreachable`, FATAL absent → generic timeout, `Forbidden` log read → generic timeout + missing-`pods/log`-RBAC note. It fails if the call-site reason is reverted to the generic one). - Reviewed via adversarial branch-review; fixes include a critical missing-RBAC dead-on-arrival, a classification false-positive on transient recovery-pod crashes, and a sub-second e2e flake. - **dev7**: backup + WAL archiving + recovery convergence verified live on the #3313 build. The load-bearing assumption of the classification — CNPG **accumulates** failed `full-recovery` pods rather than reusing one — was confirmed (observed 7 simultaneous failed recovery pods). A fully-green live step-46 run was blocked by intermittent COSI bucket-provisioning flakiness on dev7; the negative path is covered by the CI Chainsaw e2e. ### Downstream repositories Walked the trigger map in `docs/agents/contributing.md` against the diff. The only user-facing surface is the new `docs/operations/backup-classes.md` PITR section; the website keeps a hand-mirrored copy per docs version. - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: PITR docs mirror warranted (not yet opened; same pending mirror as #3313's backup-classes changes) - [x] No other downstream repository is affected by this change ```release-note feat(backups): PostgreSQL RestoreJob now supports point-in-time recovery (PITR). Set spec.options.recoveryTime (RFC3339) to restore to an exact instant. A recoveryTime past the latest archived WAL still runs to the restore deadline (spec.options.restoreTimeoutSeconds, default 30m), but the failure is now reported with the precise reason RecoveryTargetUnreachable and an actionable message instead of a generic timeout; set a shorter restoreTimeoutSeconds for a faster rejection. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added PostgreSQL point-in-time recovery (PITR) support via RFC3339 `recoveryTime`. * Introduced a PITR RestoreJob example, plus helper variables and an unreachable-target scenario. * **Bug Fixes** * Improved restore deadline handling with deterministic, log-based fail-fast classification for unreachable recovery targets, including clearer conditions. * **Documentation** * Added detailed PITR operational documentation (recovery windows, failure behavior, and GitOps/idempotent behavior). * Expanded README and example comments for PITR verification and failure expectations. * **Tests** * Extended unit tests and end-to-end chainsaw workflow to cover PITR success and unreachable failures. * **Chores** * Updated RBAC to allow pod log reads; refined cleanup and run scripts for additional RestoreJob artifacts. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Andrey KolkovandClaude Opus 4.8
|
b574e2176f |
fix(backups): cover Go-driven system-bucket ObjectStore + share the checksum helper
Address review on the S3 request-checksum fix: - Extend the fix to the platform-managed useSystemBucket=true flow, whose ObjectStore is built in Go by applyClusterPluginBackup (SSA-applied), not chart-rendered. Add InstanceSidecarConfiguration to cnpgtypes.ObjectStoreSpec and set AWS_REQUEST_CHECKSUM_CALCULATION=when_required on it via barmanSidecarConfiguration(). The platform's own default system bucket is SeaweedFS, a non-AWS S3 gateway of the same class as Ceph RGW. Unit test in cnpgstrategy_controller_test.go asserts the applied ObjectStore carries it. - Factor the duplicated instanceSidecarConfiguration block (3x across two charts) into a shared cozy-lib helper (cozy-lib.barman.checksumSidecarConfiguration). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Andrey Kolkov <andrey.kolkov@aenix.io> |
||
|
Andrey Kolkov
|
ad3f8b2487 |
fix(backups): check cluster health before the restore deadline (PITR)
Addresses review CHANGES_REQUESTED on #3383. CRITICAL: reconcileCNPGRestore checked the restore deadline BEFORE the cluster-health check, so a reconcile that first observes "deadline elapsed" AFTER the recovery Cluster already reached a healthy state (a controller restart or stalled workqueue spanning restoreTimeoutSeconds) would mark an already-converged restore Failed / RecoveryTargetUnreachable. A false Failed can then escalate to data loss: a resubmitted RestoreJob's purge guard treats the recovered Cluster as a stale leftover and deletes it + its PVCs. Fix: check hasRecovery + cnpgClusterHealthy first and return Succeeded unconditionally when healthy, regardless of how late the reconcile runs; only a not-yet-healthy restore reaches the deadline/classification branch. Adds TestReconcileCNPGRestore_HealthyPastDeadlineSucceeds (StartedAt 72h past the deadline + healthy Cluster + recoveryTime set; the injected readPodLog fails the test if the classification path is entered, proving health short-circuits). MINOR: handle the deferred Close error on the pod-log reader (errcheck). Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey KolkovandClaude Opus 4.8
|
b5d03f889b |
test(backups): reconcile-level test for the PITR deadline classification
Review of #3383 found the RecoveryTargetUnreachable classification branch in reconcileCNPGRestore was only covered through the pure helpers and recoveryTargetUnreachable directly, never through reconcileCNPGRestore itself — a mutation swapping the call-site reason back to the generic RestoreFailed kept the suite green. Add TestReconcileCNPGRestore_DeadlineClassifiesRecoveryTargetUnreachable: it backdates StartedAt past the restore deadline with a recoveryTime set and drives the real reconcile through the readPodLog seam, asserting all three deadline sub-paths — FATAL present (reason RecoveryTargetUnreachable on both RecoveryConverged and Ready), FATAL absent (generic timeout, no RecoveryTargetUnreachable), and a Forbidden log read (generic timeout + the missing-pods/log-RBAC note). Verified it fails when the call-site reason is reverted to the generic RestoreFailed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Andrey Kolkov <andrey.kolkov@aenix.io> |
||
|
Andrey KolkovandClaude Opus 4.8
|
6a1e2d0d06 |
feat(backups): support PostgreSQL point-in-time recovery (PITR)
Makes RestoreJob.spec.options.recoveryTime a genuinely supported, tested capability for the new backups.cozystack.io API (issue #2774). The recoveryTime -> CNPG bootstrap.recovery.recoveryTarget.targetTime plumbing already existed on the barman-cloud-plugin branch; this adds the reliability and verification layer the issue asked for: - Fail-fast for an unreachable target: when a RestoreJob has recoveryTime set and the recovery Cluster is not healthy, the driver reads the CNPG full-recovery pod logs and, once several distinct attempts have exited with PostgreSQL's "recovery ended before configured recovery target was reached" FATAL, fails the RestoreJob with conditions RecoveryConverged=False / Ready=False (reason RecoveryTargetUnreachable) instead of hanging to the restore deadline. Health is checked first, so a converged recovery always wins; counting distinct FATAL-ing attempts (not pod failures) tolerates a transient archive-lag miss on a near-now target and ignores transient recovery-pod crashes. Adds the pods/log RBAC grant the log read requires. - e2e (examples/backups/postgres/run-all.sh, wired into the Chainsaw postgres-2-backup-roundtrip suite): step 45 writes a before marker, captures the server timestamp, writes an after marker, restores to that timestamp and asserts the before row survived and the after row did not; step 46 submits a recoveryTime an hour ahead and asserts it fails fast with reason RecoveryTargetUnreachable. - Working, uncommented example (45-restorejob-pitr.yaml) and a docs section (docs/operations/backup-classes.md) covering the recoverable window, how to discover the earliest/latest restorable time from the backup catalog, the archive-lag tolerance, and idempotency under GitOps. Refs #2774 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Aleksei Sviridkin
|
37a91f98b8 |
feat(cozystack-controller): extract application CA into a key-free tenant Secret
Every catalog engine issues a per-release CA, but the object holding ca.crt usually holds a private key beside it (cert-manager's tls.key, CloudNativePG's ca.key), so granting a tenant read on it hands over key material too. Add a controller that publishes the trust anchor as one canonical, key-free Secret per release, "<release>.tenant-ca", holding only ca.crt. A chart declares the anchor by rendering a namespaced TenantProjection sentinel (internal.cozystack.io/v1alpha1) that names the source Secret and key. The controller rebuilds ca.crt from the parsed certificate DER rather than copying the source, refuses any value carrying private key material, and owns the projection by owner reference so a foreign Secret at the same name is never overwritten. No tenant RBAC grants any verb on internal.cozystack.io; a ValidatingAdmissionPolicy pins writes to helm-controller as defence in depth. The controller does not grant tenant read access — the lineage webhook does, from the ApplicationDefinition's spec.secrets. This side stamps the engine-agnostic internal.cozystack.io/tenant-ca label and forces a re-admission when those selectors change. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
myasnikovdaniil
|
960d853706 |
feat(api): typed waitStrategy/healthCheckExprs for kstatus readiness (#2642) (#3273)
## What Adds typed `waitStrategy` + `healthCheckExprs` fields so a package/app can declare the Flux helm-controller v1.5.0 kstatus health gate for the custom resource it wraps — making `HelmRelease.Ready` mean "the CR is actually healthy" instead of "helm applied the CR". This is the operator/apiserver foundation of the kstatus epic (#2642); the API surface was reviewed and approved by the API owner in #3263. Commits (per the review's "isolated step-0" request): 1. `build(deps)` — the dependency bump (see below). 2. `feat(api)` — the fields, plumbing, tests, and regenerated code. 3. `fix(api)` — match the openapi code-generator pin to the 0.35 runtime (fixes an aggregated-apiserver crash the bump surfaced). 4. `fix(api)` — declare `OpenAPIModelName` on the apps types so SSA resolves catalog kinds. 5. `docs(api)` — note that `disableWait` silently defeats `healthCheckExprs`. ## API surface Typed fields on **both** HelmRelease-building paths: - `ApplicationDefinition.spec.release` (apiserver / catalog apps → `convertApplicationToHelmRelease`) - `ComponentInstall` (operator / platform packages → `buildHelmReleaseSpec`) ```go // WaitStrategy maps to HelmReleaseSpec.WaitStrategy.Name — a deliberate scalar // simplification of the upstream {name} object. One of poller|legacy. WaitStrategy string // HealthCheckExprs maps to HelmReleaseSpec.HealthCheckExprs (the upstream // kustomize.CustomHealthCheck type verbatim). HealthCheckExprs []kustomize.CustomHealthCheck ``` `config.ResolveWaitStrategy` is shared by both builders and **couples the default**: `healthCheckExprs` are only evaluated under the `poller` strategy, so when expressions are set and no strategy is given, the generated HelmRelease defaults to `poller`. A package that sets only `healthCheckExprs` is therefore self-contained, independent of the controller's global default. ## The dependency bump (commit 1) The `WaitStrategy` / `kustomize.CustomHealthCheck` / `HelmReleaseSpec.HealthCheckExprs` types do not exist below `helm-controller/api` v1.5.0 — the runtime is already v1.5.0 (shipped by #2602), so this closes a Go-types-vs-runtime skew. It is **not** an isolated bump: v1.5.x requires **k8s 0.35 + controller-runtime 0.23**, cascading the k8s stack 0.34 → 0.35 (and the go directive to 1.26). cozystack replaces `k8s.io/apimachinery` with a fork carrying the still-open upstream `ConvertToVersion` patch (k/k#135537). That single commit rebases cleanly onto apimachinery v0.35.0; published at `cozystack/apimachinery@release-1.35-pr135537`, and `go.mod` pins its pseudo-version. **The repo builds with zero source changes under the new stack.** > Maintainer note: please confirm the `release-1.35-pr135537` fork branch naming, or fold it into the `cozystack` branch as you prefer — the pin can be updated to match. ## Not included (conscious YAGNI) `disableWait` is intentionally omitted. The only known need (the kubernetes tenant-addon deadlock) was resolved child-side, and `waitStrategy: legacy` is not equivalent (legacy still waits Helm-v3-style; `disableWait` skips waiting entirely). Add it later when a concrete leaf needs a true no-gate escape hatch. ## Tests - `config.TestResolveWaitStrategy` — the couple-the-default logic (unset+exprs→poller; explicit legacy honored; unset+no-exprs→nil). - Operator + apiserver cases mirror each other (`TestBuildHelmReleaseSpecHealthCheckExprs` / `TestConvertApplicationToHelmRelease_HealthCheckExprs`). - `go build ./...` clean, `go vet` clean, `gofmt` clean, codegen regenerated and committed. The 4 backupstrategy CRDs regenerate with benign k8s-0.35 PodSpec description drift (DynamicResourceAllocation alpha→stable, toleration Lt/Gt, resizePolicy note) — no functional change. ## Follow-up Phase 3 (per-leaf adoption — postgres/kafka/mongodb/etc. declaring their CEL via these fields) builds on this. The full CRD→CEL catalog is maintained in the epic's readiness audit. Refs #2642 ```release-note feat(api): add `waitStrategy` and `healthCheckExprs` to ApplicationDefinition and ComponentInstall, so a HelmRelease reports Ready only once the custom resource it wraps is actually healthy ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added configurable CEL health-check expressions for application releases and package components. * Added `waitStrategy` support (`poller` and `legacy`) with automatic `poller` selection when health checks are configured and no strategy is set. * Propagated these settings into generated Helm releases, affecting readiness/wait behavior. * **Documentation** * Updated Application and PackageSource schemas to include `healthCheckExprs` and `waitStrategy`. * Extended related Kubernetes CRD documentation, including projected pod certificate annotations and workload reference fields. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Andrey KolkovandClaude Fable 5
|
67db8f7da7 |
fix(backups): make the barman-cloud serverName stable across every flow
Two defects of the same class — the WAL-archive S3 prefix (serverName) changing or defaulting differently between components: Recovery default regression (review blocker): native CNPG defaulted an absent externalClusters serverName to the entry name (= bootstrap.oldName), but the barman-cloud plugin defaults it to the NEW cluster's name, so a legacy-flow restore with bootstrap.serverName unset would read the wrong S3 prefix (hard failure, or silently restoring another cluster's data). The chart now always emits serverName, defaulting to bootstrap.oldName. Live-cluster serverName flip (found by the gated postgres backup e2e on a real cluster): the chart renders spec.plugins without serverName (effective prefix = cluster name) while a strategy template may name a different one; the driver's ForceOwnership patch then flipped the prefix mid-stream, splitting the WAL archive across two prefixes — the WAL holding the backup's start was lost between them and the restore failed with "WAL not found". The driver now preserves the live Cluster's effective serverName (explicit parameter, or the cluster name when the plugin is attached without one) and only applies the strategy's value to a Cluster with no plugin attached; both charts pin serverName explicitly, and the example strategy no longer sets one. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey KolkovandClaude Opus 4.8
|
268ec29856 |
test(backups): assert driver-initiated Backup uses method=plugin
Review follow-up (cozystack#3300): ensureCNPGBackup stamps spec.method=plugin + spec.pluginConfiguration on the postgresql.cnpg.io/Backup it creates — the field that routes a driver-initiated run through the barman-cloud plugin. If it regressed to the default (barmanObjectStore) method, CNPG would run the legacy method against a Cluster that no longer has one and every platform backup would fail. Add TestEnsureCNPGBackup_UsesPluginMethod asserting method, plugin name, and cluster ref. (The chart ScheduledBackup path was already covered; the driver path was not.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey KolkovandClaude Opus 4.8
|
2212367842 |
fix(backups): own driver-created ObjectStore + correct stale barman references
Review follow-ups (cozystack#3300): - Lifecycle/leak: applyClusterPluginBackup now sets an ownerReference from the driver-created barmancloud.cnpg.io/ObjectStore to its cnpg.io/Cluster, so Kubernetes garbage-collects it when the Cluster is deleted (tenant app teardown or the RestoreJob purge). In the platform useSystemBucket flow the chart does not render this ObjectStore, so previously it was orphaned in the tenant namespace. Drop the now-accurate-only 'delete' verb on objectstores from the driver RBAC (GC handles removal; the driver never deletes it) and correct the RBAC comment that falsely claimed the purge deletes it. - Diagnostics/comments: the RestoreJob WAL-archive-deadline failure message and several doc comments still told operators to check spec.backup.barmanObjectStore / externalClusters[].barmanObjectStore, fields no longer emitted after the migration. Point them at the barman-cloud plugin (spec.plugins + ObjectStore, externalClusters[].plugin) instead so they don't mislead during an incident. - Test: TestApplyClusterPluginBackup_PatchesExistingCluster now asserts the ObjectStore carries an ownerReference to the Cluster (kind/name/uid). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey Kolkov
|
13ff340277 |
fix(backups): migrate CNPG backups to the barman-cloud plugin
Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrei Kvapil
|
fbbe4af7df |
refactor(network): drop ExposureClass/ServiceExposure for native loadBalancerClass (#3218)
## What Removes the `network.cozystack.io` API group — `ExposureClass` + `ServiceExposure` (#3081) — and its reconciler, in favor of native `Service` `type: LoadBalancer` + `loadBalancerClass`. ## Why Full rationale and discussion in #3164. For **managed applications** the chart already owns the Service, so: - `type: LoadBalancer` + `loadBalancerClass` + an admin-provisioned pool covers vendor-neutral external exposure without a dedicated API group and controller; - the `externalIPs` (node-IP) default never went through `ServiceExposure` (its backend is a no-op) and is left unchanged; - the only current consumer, the host ingress, keeps rendering `type: LoadBalancer` and just drops the `ServiceExposure`; the admin provisions the address pool / announcer out of band, as the install docs already assume. `ServiceExposure`'s one unique feature — dynamic per-namespace scoping of a shared VIP pool — is a convenience the managed-app path does not need, and it does not justify a second tenant-networking API group next to `sdn.cozystack.io`. ## Changes - Delete `api/network/v1alpha1` and `internal/controller/serviceexposure` (+ CRD definitions, e2e bats, controller registration, RBAC). - Host ingress: drop the `ServiceExposure` template; add an optional `publishing.loadBalancerClass` that sets `spec.loadBalancerClass` on the host-ingress LoadBalancer Service. - The `externalIPs` path and default behavior are unchanged. ## Verification - `go build ./...` — green. - `helm unittest` — `core/platform` 72/72, `extra/ingress` 25/25 (incl. host-vs-tenant `loadBalancerClass` scoping and the immutable-field migration guard). ## Migration None required. `network.cozystack.io` (#3081) has never shipped in a release — it is on `main` only (not backported to `release-1.5`; absent from v1.4.x / v1.5.x), so no upgrading cluster has these CRDs and this removes it before it is ever released. Clusters that track `main` and explicitly set `publishing.exposureClass` (opt-in, only since 2026-06-30) should provision their MetalLB / Cilium address pool themselves and set `publishing.loadBalancerClass`; any orphaned `cozystack-<class>` pool can be deleted by hand. Default clusters (externalIPs path) are unaffected. ## Status **Draft / RFC for the #3164 discussion** — this removes an API group that landed only days ago, so nothing here should merge before the design call. cc @lexfrei (author of #3081) and @lllamnyp (#3164). Deferred: when Kubernetes actually removes `Service.spec.externalIPs`, the node-IP default migrates via a MetalLB `IPAddressPool` with no `L2Advertisement` (allocate-but-don't-announce) — a separate change. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added support for configuring the ingress host Service with a native `loadBalancerClass` value. * Expanded chart tests to verify class wiring and immutability behavior across install and upgrade scenarios. * **Bug Fixes** * Removed the legacy exposure-class/service-exposure flow and related controller behavior. * Updated templates and CRD packaging so the platform no longer renders or depends on those deprecated resources. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Myasnikov Daniil
|
96a9652fb1 |
feat(api): typed waitStrategy/healthCheckExprs for kstatus readiness
Add first-class fields so a package/app can declare the Flux helm-controller v1.5.0 kstatus health gate for the custom resource it wraps, so HelmRelease.Ready means "the CR is actually healthy" instead of "helm applied the CR" (#2642). API-owner approved in #3263. - api: WaitStrategy (scalar, maps to HelmReleaseSpec.WaitStrategy.Name — a deliberate simplification) + HealthCheckExprs ([]kustomize.CustomHealthCheck, the upstream type verbatim) on both ApplicationDefinition.spec.release (the apiserver path) and ComponentInstall (the operator path). - config.ResolveWaitStrategy: shared by both HelmRelease builders. Couples the default — healthCheckExprs imply the poller strategy when none is set, since exprs are only evaluated under poller. This removes the "must also set waitStrategy: poller" footgun and does not depend on the controller's global default. - thread through buildHelmReleaseSpec (operator) and convertApplicationToHelmRelease (apiserver); the apiserver loads the typed spec.release fields in start.go. - regenerate deepcopy, the ApplicationDefinition and PackageSource CRDs, and openapi. The backupstrategy CRDs also regenerate — benign k8s-0.35 PodSpec description drift, not a functional change. disableWait is intentionally not added (YAGNI): the only known need (the kubernetes tenant-addon deadlock) was resolved child-side, and waitStrategy: legacy is not equivalent (legacy still waits Helm-v3-style; disableWait skips waiting entirely). Add it later when a concrete leaf needs a true no-gate escape hatch. Refs #2642 Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Ivan Okhotnikov
|
a8494d874f |
test(backups/postgres): reconcile-level repeat-restore regression + conservative freshness tie
Address review feedback on the repeat in-place restore fix: - Make the freshness discriminator strict (creationTimestamp > StartedAt) so an exact tie resolves to "not fresh" and the caller purges, matching the function's stated conservative default; a fresh Cluster is always created well after StartedAt so this never misclassifies the status-write-race path. - Document why a wall-clock timestamp comparison was chosen over recording the purged Cluster's UID on the TargetPurged condition (the UID approach leans on the same status write the race path assumes can fail). - Add a reconcile-level regression test that drives reconcileCNPGRestore end to end with a fake client: a stale leftover recovery Cluster from a prior restore is purged, and a Cluster this restore just re-created is not re-purged. This covers the actual call site where #3311 lived, not just the pure helpers. Signed-off-by: Ivan Okhotnikov <xorokhotnikov@gmail.com> |
||
|
Ivan Okhotnikov
|
cf1b6ba28e |
fix(backups/postgres): purge stale recovery cluster on repeat in-place restore
A repeat in-place PostgreSQL RestoreJob into an already-restored target silently no-opped: the destructive purge was skipped because the live Cluster still carried spec.bootstrap.recovery left over from the previous restore, and the re-purge guard mistook that for a freshly-recovered Cluster produced by this restore's own purge. The job then reported Succeeded while the PVC, disk, and data were never touched. Tell the two apart by the Cluster's creationTimestamp relative to the RestoreJob's StartedAt: only a recovery Cluster created at/after StartedAt is treated as this restore's own and skipped; one that predates StartedAt is leftover from an earlier completed restore and is purged so the restore re-bootstraps from the backup. Signed-off-by: Ivan Okhotnikov <xorokhotnikov@gmail.com> |
||
|
Aleksei Sviridkin
|
249f7b5a5e |
feat(platform, ingress): propagate operator wildcard certificate to per-tenant termination points (#2990)
## What this PR does The operator-provided wildcard certificate (`publishing.certificates.wildcardSecretName`) currently serves the root tenant only: everything is same-namespace, so the root ingress controller and Gateway read the operator's TLS Secret directly. Child tenants run their own ingress controller / Gateway in their own namespace under a default-deny policy and cannot read that Secret across namespaces, so they fall back to per-host ACME. This propagates the operator wildcard to per-tenant termination points by replicating the Secret into each tenant namespace that owns one, keeping the root-MVP invariant intact: the wildcard is deliberately distributed to tenant namespaces, but no tenant RBAC is widened — every consumer reads only its own-namespace copy. A new controller in `cozystack-controller` does the replication with no extra operator input. It reads the same platform values channel the consumers read (`cozy-system/cozystack-values`): the wildcard Secret name from `_cluster.wildcard-secret-name` and the publishing namespace from `_cluster.expose-ingress`. It mirrors that Secret into every tenant namespace whose `namespace.cozystack.io/ingress` or `namespace.cozystack.io/gateway` label equals its own name — i.e. the namespace runs its own controller / Gateway. Because the source is derived from the same value that makes the consumers reference it, the replica is created whenever the consumers expect it — no manual labelling and no upgrade hazard where a child controller references a Secret that nothing creates. Replicas are refreshed on rotation and pruned when a namespace stops terminating TLS. Teardown of every replica happens on exactly one trigger: clearing `wildcardSecretName` (explicit disable). A source that is merely absent or mistyped, or a transiently-missing values channel, leaves existing replicas in place, so a brief gap, a delete+recreate rotation, or a misconfigured publishing namespace never drops tenant TLS. The platform controller's own ServiceAccount gains Secret writes; tenant RBAC is untouched. The controller does not cache every Secret in the cluster: the manager's Secret informer is scoped to managed replicas and the values channel only, and the operator source / foreign collisions are read through the uncached APIReader. The source is not watched (its name is dynamic), so an in-place rotation is picked up by a bounded periodic resync while the publishing tenant serves the source with no lag. On the consumer side, each tenant ingress controller now sets `--default-ssl-certificate` to the wildcard Secret in its own namespace (`.Release.Namespace/<name>`), so a child tenant serves the replica instead of minting per-host ACME. The Gateway path already renders `certMode: existingSecret` with a same-namespace `wildcardSecretRef` for every tenant, so it consumes the replica with no chart change. ### Why not clustersecret-operator The issue proposed the shipped `clustersecret-operator`. It materializes inline template data into selected namespaces; it cannot mirror an existing source Secret, and it is not installed in any default bundle. Driving it would mean embedding the certificate and private key as literal data in a cluster-scoped object rendered from a Helm `lookup` — that broadcasts key material the design keeps off any shared channel, and a `lookup` is invisible to the Flux helm-controller digest so it would not re-render on rotation. Its namespace selector also cannot express "the namespace whose owner label equals its own name", so it would over-replicate the key into every tenant namespace. A small dedicated reconciler targets exactly the termination namespaces and keeps the key off the values channel. ### Coverage caveat Replication delivers the certificate bytes, not SAN coverage: a single `*.<root-host>` does not match `*.<child-apex>`. Operators serving child-tenant subdomains supply a certificate whose SANs cover those apexes; the docs spell this out. The replica carries the wildcard private key into each terminating tenant namespace — the same exposure as the per-host ACME Secret it replaces, now shared across tenants. ### Tests - Go fake-client tests for the reconciler: termination-only replication (ingress-owning and gateway-owning namespaces only, never inheriting ones, never over the source), copy markers, rotation, prune on namespace demotion, the non-destructive source-deletion contract, replica self-heal after out-of-band deletion, the publishing-namespace self-replication guard, disable-via-config teardown, source rename, foreign-Secret collision safety (no overwrite, no requeue, does not block siblings), transient-error requeue, non-TLS source ignored, absent-channel keeps replicas, publishing-namespace mismatch keeps and requeues, the cache-scope pin, the enqueue-wiring map, an `ownsTerminationPoint` table, and the active-only requeue contract. - helm-unittest for the ingress `default-ssl-certificate` wiring (including a child-tenant controller pointing at its own-namespace replica) and for the cozystack-controller ClusterRole's new Secret-write rule. ### Screenshots N/A — no UI changes. ### Release note ```release-note feat(platform): the operator-provided wildcard TLS certificate (`publishing.certificates.wildcardSecretName`) is now served by per-tenant ingress controllers and Gateways automatically — the platform controller replicates it into each tenant namespace that terminates TLS, with no extra operator input and no cross-namespace Secret read ``` Closes #2820 Part of #2811 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **New Features** * Wildcard TLS Secret now replicates from the platform to tenant namespaces that terminate TLS via their own ingress controller or Gateway, including child-tenant scenarios. * **Documentation** * Clarified configuration, timing, and SAN matching requirements for wildcard replication. * Updated gateway and ingress-nginx default certificate documentation for replica mirroring behavior. * **Bug Fixes** * Improved handling for missing/invalid source secrets (no unnecessary pruning) and safe collision behavior. * **Tests** * Added comprehensive reconciliation/replication test coverage. * Added Helm unittest target for the controller chart. * **Chores** * Updated controller RBAC to support wildcard TLS Secret replication. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Myasnikov DaniilandClaude Fable 5
|
9297e51170 |
Merge origin/main into feat/e2e-chainsaw-example
Reconcile the Chainsaw e2e migration with ~500 commits of main drift: - keep main's run-kubernetes.sh helpers (kubectl_wait_retry, tenant drain, LINSTOR pool-free wait, SC-fallback probe) under the branch's _lib/ rename with the Chainsaw finally/snapshot deltas re-applied - resolve modify/delete on 10 drifted hack/e2e-apps/*.bats as deleted; their post-branch assertion drift is re-ported into the Chainsaw suites in follow-up commits - drop the 4 new main-side bats (kubernetes-oidc-*, securitygroup, serviceexposure) from _lib/ relocation; ported as Chainsaw suites in follow-up commits - docs/agents/e2e-testing.md: merge main's plan-job TIA wording with the Chainsaw suite phrasing Assisted-By: Claude <noreply@anthropic.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
myasnikovdaniil
|
48a3cfd341 |
Merge branch 'main' into ci/scorecard-hardening
Signed-off-by: myasnikovdaniil <60174387+myasnikovdaniil@users.noreply.github.com> |
||
|
Andrei Kvapil
|
3dbc7d3662 |
refactor(network): drop ExposureClass/ServiceExposure for native loadBalancerClass
Remove the network.cozystack.io API group (ExposureClass + ServiceExposure) and its reconciler. Managed-application charts own their Service, so native type: LoadBalancer + loadBalancerClass + an admin-provisioned pool covers external exposure without a dedicated API group and controller. The host ingress -- the only consumer -- keeps rendering type: LoadBalancer and gains an optional publishing.loadBalancerClass to pick the LB controller; the externalIPs (node-IP) default path is unchanged. Rationale in cozystack#3164. Assisted-By: Claude Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io> |
||
|
Myasnikov DaniilandClaude Opus 4.8
|
1f0c1949e7 |
test(flux-shard-operator): add fuzz targets for shard config parsers
Native Go fuzzing over the flag parsers in config.go, asserting the
invariants that matter rather than just no-panic:
- ParseShardIndex round-trips with ShardName (accepted input is the canonical
name of the returned index; index is non-negative).
- ShardName round-trips back through ParseShardIndex for every valid index.
- ParseShardCount: a non-auto success yields count >= 1; auto yields 0.
- ParsePinnedTenants: every entry in a success has a non-empty key mapped to a
valid shard name.
Adds an OSS-Fuzz-detectable fuzzing signal for the OpenSSF Scorecard Fuzzing
check while covering parsers that sit on the sharding hot path.
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Assisted-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
||
|
Timofei Larkin
|
0f55a150ba |
fix(ci): catch templated CRDs and APIService-only groups in the API gate (#3184)
## What this PR does Closes two coverage gaps found while shadow-observing the API-owner-review gate (#3167) against real PRs in the first hours after merge: 1. **CRD discovery was gated on directory name** (`crds/`, `definitions/`, `manifests/`), so a CRD shipped inside a chart's `templates/` dir — a valid, existing convention when the CRD is wrapped in a `{{- if }}` guard — was invisible to the gate. This was not hypothetical: `#3149` (cozyplane) adds four brand-new CRDs (`VPC`, `VPCBinding`, `VPCPeering`, `Port`) exactly this way, and none of them tripped a review requirement. 2. **Groups served only by an external aggregated apiserver had no representation at all.** The same `#3149` also registers `sdn.cozystack.io` via a `kind: APIService` pointing at a prebuilt image built outside this repository — no CRD, no vendored Go types, nothing to diff. ### Fix - CRD/APIService discovery is now content-based: every `.yaml`/`.yml` file under `packages/` and `internal/` is a candidate, and whether it's kept depends on whether it parses into the right `kind` with a first-party group — not which directory it sits in. - Both parsers now tolerate a Helm directive line (`{{- if }}`, `{{- end }}`, a comment block) sharing a `---`-delimited document with otherwise-valid YAML, rather than dropping the whole document. This matters because the closing `{{- end }}` of a wrapping conditional has no `---` before it, so it lands in the same document as the manifest it closes. - Added a fourth resource source, `SourceAPIService`, for `kind: APIService` manifests registering a first-party group. Like the existing static Go-backed groups, these carry no checked-in schema, so they participate only in new-group / new-resource / removal detection — never a fabricated breaking-change diff against a schema this repo can't see. Verified against the real `feat/cozyplane` branch: the gate now reports all four new CRDs, plus the removal of the old `securitygroups`/`APIService` registration for the same group — a full API-surface migration that was previously invisible end to end. ### Release note ```release-note fix(ci): the API-owner-review gate now discovers CRDs and APIService registrations by content instead of directory name, closing a gap where a CRD or an externally-served aggregated API group could ship without triggering the required review. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Discovery now includes APIService registrations, enabling detection of groups/versions exposed only via aggregated APIService manifests (including merging versions from multiple templates). * **Bug Fixes** * Improved YAML discovery to tolerate mixed/templated multi-document streams: non-CRD/non-APIService documents no longer stop discovery, and embedded template directives are handled more reliably. * File processing is more resilient during snapshot loading (includes a safety cap on YAML size). * **Tests** * Added coverage for APIService discovery, templated neighbor documents, and correct exclusion of third-party manifests. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Timofei Larkin
|
50f8548ba9 |
fix(ci): stop double-parsing every file in the API gate's content walk
branch-review on #3184 measured a real regression: switching CRD/ APIService discovery to content-based scanning made every file in packages/+internal a candidate, and each was split into documents and fully unmarshaled twice — once per parser — inflating a full snapshot load from ~0.5s to ~3.4s. Split each file's documents once and probe each document's kind once before deciding which shape to try, so the common case (a document that is neither a CRD nor an APIService — the overwhelming majority of what the wider walk now sees) pays for at most one parse attempt instead of two. Add a raw substring fast-reject, at both the per-document and whole-file level, ahead of any YAML parsing: neither kind this package cares about can appear without its own name appearing as a literal substring first. Also fixes two smaller gaps flagged in the same review: an untrusted PR head checkout could point discovery at an arbitrarily large file with no size guard (capped at 8 MiB), and a group's merged APIService resource only recorded the first contributing file's path, dropping the others from the report. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Timofei Larkin <lllamnyp@gmail.com> |
||
|
Timofei Larkin
|
985f17b7be |
fix(ci): catch templated CRDs and APIService-only groups in the API gate
Shadow-observation of the merged gate found a real miss: a CRD shipped
inside a chart's templates/ dir (guarded by a Helm {{- if }}) was
invisible because discovery was gated on directory name (crds/,
definitions/, manifests/), and a group served only by an external
aggregated apiserver (kind: APIService, no CRD, no vendored Go types)
had no representation at all.
Switch CRD/APIService discovery from directory-name to content
sniffing, and tolerate Helm directive lines sharing a document with
otherwise-valid YAML (the closing `{{- end }}` of a wrapping
conditional has no leading `---`, so it lands in the same document as
the manifest). Add a fourth source, SourceAPIService, reusing the
existing empty-schema mechanism so a first-party group's first
APIService appearance requires review without pretending to diff a
schema that lives outside this repository.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
|
||
|
Andrey Kolkov
|
cc5d688e15 |
feat(platform): migrate to etcd-operator v1alpha2
Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Ivan Okhotnikov
|
0d98e27981 |
fix(operator): hold final backoff window before give-up, note upstream version skew
Address lexfrei's non-blocking follow-ups on the LGTM approval: Follow-up #1 — give-up preempted the final backoff. `decideRecovery` returned `recoveryActionGiveUp` as soon as `attempts >= maxRecoveryAttempts`, without checking whether the final-force backoff window had elapsed. The Owns()-watch re-fire from the N-th force-drift's own writes triggered a reconcile milliseconds after the force, so the effective time-to-give-up was ~7.5m (30s+60s+2m+4m) — not the ~11.5m documented on the constants block. The final 4-minute response window the last nudge was supposed to grant source-watcher was never actually granted. Fix: extend the exhaustion branch to hold `Wait` until `now - lastRecoveryAt >= backoffFor(maxRecoveryAttempts)` before switching to GiveUp. Now the documented ~11.5m budget matches the observed behaviour, and the last force gets a real chance to succeed (Owns watch → source-watcher Ready=True copy-through) before we surface SourceWatcherStalled. Tests: * `TestDecideRecovery` gains a case pinning `Wait` when attempts=maxRecoveryAttempts but the final backoff has not yet elapsed (the exact re-fire shape the previous behaviour tripped on). * `TestUpdateStatus_OwnMarkerAttemptsExhaustedButFinalBackoffPending` drives the full updateStatus path against the same shape, asserting the PackageSource stays in Unknown/AwaitingSourceWatcherRecovery and the RequeueAfter equals the remaining backoff, NOT zero (GiveUp). Follow-up #3 — upstream version-skew note. The Go module dependency tracks `fluxcd/source-watcher/api/v2 v2.0.3` (CRD types package only), but the deployed binary this workaround targets is pinned to v2.1.0 at `internal/fluxinstall/manifests/fluxcd.yaml`. Comments referencing `drift.go:52` / `controller.go:164/254` describe the deployed binary, not the compiled API package. Documented on the constants block so a future flux bump re-verifies the anchors AND the watch predicate / drift-detection heuristics, which are the load-bearing upstream assumptions. Follow-up #2 (envtest over full Reconcile→Owns→updateStatus cycle) is architectural and out of scope for this PR — noted for a follow-up. Signed-off-by: Ivan Okhotnikov <xorokhotnikov@gmail.com> |
||
|
Ivan Okhotnikov
|
37421d2fec |
docs(operator): honest JSON-merge-patch tradeoff, clarify asymmetric OG check, warn on partial force-drift
Self-review nits raised in a skeptical pass over the current tip: * The status-patch docstring in forceArtifactGeneratorDrift described the JSON-merge-patch full-array-replace on `status.conditions[]` as "net-neutral". That was too optimistic: if source-watcher writes a concurrent Reconciling condition between our Get and our Patch, our patch does transiently wipe it. source-watcher restores it on its next reconcile (which our requestedAt bump is about to trigger), but downstream tooling watching for Reconciling can observe the momentary absence. Rewrite the comment to name that trade-off explicitly and point at server-side apply as the fuller fix if the operator ever migrates to SSA globally. * The unified own-marker/progressing/stuck gate in updateStatus is asymmetric on ObservedGeneration — isProgressingInRecovery checks it, isOwnMarker does not. That asymmetry is deliberate (a stale own-marker after a spec edit must still route to the state machine to avoid leaking the synthetic reason to the PackageSource — exactly the B2 regression), not an oversight. Add a comment explaining the reasoning so a future edit doesn't "symmetrise" the check and re-open B2. * forceArtifactGeneratorDrift's step-2 (metadata patch) failure was silent when it happened after a successful step-1 status patch. In that state source-watcher has been nudged, but the tracking annotations never persisted, so the next reconcile reads attempts=0 and re-forces from scratch instead of accumulating toward SourceWatcherStalled — a persistent metadata-write-only apiserver malfunction would loop forever. Log the failure loudly with the intended attempt count so operators can spot this specific mode from cozystack-operator logs. Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io> |
||
|
Ivan Okhotnikov
|
596427deb3 |
docs(operator): scrub stale awaitSourceWatcherResponse references from comments and test names
Self-review after the previous commit removed the awaitSourceWatcherResponse helper (routing unified through maybeRecoverArtifactGenerator's state machine via a single own-marker / progressing-in-recovery / stuck gate). Four comments and two test names still referenced the now-deleted symbol — misleading anyone tracing the architecture. * maybeRecoverArtifactGenerator docstring: rewrote the trade-off paragraph to describe the unified gate and Wait branch instead of the removed helper. * TestMaybeRecoverArtifactGenerator_DoesNotResetCounterOnFreshTransition comment: same rewrite. * TestUpdateStatus_OwnMarkerRoutesToAwait renamed to TestUpdateStatus_OwnMarkerHoldsTrackingViaStateMachine + comment rewritten to describe the state-machine Wait branch. * TestUpdateStatus_ProgressingDuringRecoveryRoutesToAwait renamed to TestUpdateStatus_ProgressingDuringRecoveryHoldsTracking + comment rewritten in parallel. Pure doc / naming; no code or test-body changes. Tests still pass with -race, vet clean, golangci-lint 0 issues. Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io> |