mirror of
https://github.com/cozystack/cozystack.git
synced 2026-11-04 02:57:27 +09:00
feat/endpoint-attachments
1097
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Timofei Larkin
|
1d3ab0bb4c |
feat(api): add EndpointAttachment to attach addresses to app endpoints
A managed application's tenant-facing Services are its endpoints, and the only way to publish one externally has been the chart boolean `external`, which publishes one chart-chosen endpoint on one address from no particular pool through five different render paths. EndpointAttachment attaches one address, minted from an IPAddressClass or bound from a reserved IPAddressClaim, to one tenant-facing Service by rendering an additive LoadBalancer Service that mirrors the endpoint's selector and ports. The mechanism therefore needs no per-engine render knowledge and never touches the application's own objects, and a reserved address survives every attachment and moves between applications. The endpoint check requires both the lineage labels and the tenantresource marker: lineage alone also covers operator-internal Services. Identity is by owner reference throughout, so a Service carrying the ownership label without the reference is never adopted, and a same-name recreated application does not inherit the previous incarnation's attachments. The VM datapath modes render the cozy-proxy Service contract unchanged and are gated on a platform declaration rather than CNI detection: a proxy skips a Service delegated to a name it does not claim while the load balancer still attracts the address. Exclusivity is keyed on the backend pod in both modes because cozy-proxy's egress map holds one address per pod, and a whole-IP attachment with several ready backends keeps its address dark instead of picking one. The e2e suite is parked as disabled until the address substrate is packaged; the unit tests carry the hold/move and identity proofs. The kind lives in local.sdn.cozystack.io, the CRD-served half of the sdn family the address substrate and cozyplane share, so stock ResourceQuota counts keep working; the proposal text is being corrected from cozystack.io to match. Assisted-by: LLM Signed-off-by: Timofei Larkin <lllamnyp@gmail.com> |
||
|
Aleksei Sviridkin
|
d3e228fed8 |
test(opensearch): roll a single-node cluster held yellow by a replica
The roll onto the new mount plan timed out on some runs and passed on others with identical content. What differed was cluster health at the moment the listener moved: on one data node an index that asks for a replica stays yellow for good, and the operator restarts a pod only once its health gate opens. The security audit log creates such an index on its first audited event, and whether that had happened yet was down to timing. Create the index in the suite so the roll always has to get through a yellow cluster, and read the health back so the step cannot keep passing while it proves nothing. The operator logs that health held a restart back, never what the health was, and cluster health is not a Kubernetes object. The catch block now reads it from the node, together with the shard table and the allocation explanation. Signed-off-by: Aleksei Sviridkin <f@lex.la> Assisted-by: LLM |
||
|
IvanHunters
|
3615958240 |
fix(bucket): stop the ACME cleanup hook from talking to localhost, and gate the class
The hook's four kubectl calls carried --request-timeout=30s. A non-zero value there makes kubectl skip its in-cluster credentials: client-go consults them only while the assembled config still equals the default, and the flag lands in that config, so every call went to kubectl's built-in default cluster and was refused. This package failed more quietly than kafka did. Each call is guarded by `|| echo WARNING` and the script sets no -e, so the hook exited 0 and printed "ACME cleanup complete."; hook-succeeded then deleted the Job that held those warnings. The ACME solver objects it exists to reap were left behind, and the only record went with the Job. Its own suite matched the rendered command including the flag, so the broken form was pinned. The gate is the point of this change. Two packages had it, neither was visible in a rendered manifest, and nothing stops the next hook from reaching for the same flag, since it reads as ordinary defensive practice. hack/check-hook-request-timeout.bats fails any non-vendored template whose kubectl call carries a non-zero --request-timeout, skipping files that name KUBECONFIG because an explicit kubeconfig is not served by the in-cluster branch. That per-file judgement is the coarse part and is written down in the file rather than left to be inferred: apps/kubernetes keeps the flag on two calls that pass their own kubeconfig, and stays green. Bounding the work is still available where it belongs: --timeout for a watch, activeDeadlineSeconds for the Job, which is what this hook already had at 120s. Assisted-by: LLM Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
6818795588 |
fix(seaweedfs): raise the bucket ceiling by sizing volumes smaller (#4251)
## What this PR does Raises how many buckets a SeaweedFS instance can serve, by lowering the master's `volumeSizeLimitMB` from 1000 to 400 and its two-copy growth count from 6 to 3. The size limit is exposed on the app chart, where an operator can reach it. Existing instances stay at 1000 unless they already name a limit, so for them the upgrade changes only the growth count. A platform migration does that where migrations run; on the `default` variant it is a manual step before upgrading. A unit test pins each constant separately, and an e2e case creates several buckets and requires each to take a write. Late in a full e2e run every S3 `PUT` starts timing out and a backup suite fails its access preflight. It reads like a credentials or COSI problem and is not one: the instance has no volume slots left, so the master cannot grow a volume for a newly created bucket. The disk is still almost entirely free, which is what makes it look like a flake. The COSI objects say nothing either way, the claim is ready and the access is granted. ### The ceiling A volume server started with `-max 0` takes a slot count derived from disk rather than a fixed one, replication `001` spends a slot on each of the two servers per logical volume, and a collection claims `COPY_2` volumes each time it grows. So the ceiling is set by the product of the size limit and `COPY_2`, and moving either number alone moves it. The mechanism is upstream in `MaybeAdjustVolumeMax` (`weed/storage/store.go`) and `findVolumeCount` (`weed/topology/volume_growth.go`); what matters for choosing the numbers is only that upshot. Do not divide the slot figures out of the nominal PVC size, because what the server counts is the space the filesystem makes available and not the volume's advertised size. At the upstream 1000 and 6 a 10Gi volume PVC gives nine slots per server and two collections, which the platform's own `cozy-backups` bucket and a single tenant bucket already use. That same PVC was seen to give nineteen slots at 500, which puts 400 at twenty-three to twenty-four by the same division, or eight collections. The span is the observation's own: the free space behind nineteen at 500 fits more than one filesystem geometry, and those divide differently at 400. Eight either way, because a collection that cannot get a whole batch of `COPY_2` keeps the volumes it did get and writes into them, so the count rounds up rather than down. Every figure here is for the shipped `replicationFactor: 2`. The master picks which of its four growth counts to apply from the replication the filer asks for, and the filer takes that from `replicationFactor`, so at 1 or 3 copies it reads `COPY_1` or `COPY_3` and the `COPY_2` this PR sets does not apply. Lowering the size limit still raises the ceiling there, since it is the divisor whatever the replication is. I measured this instead of only deriving it, on the real `weed` 4.31 binary: one master, two volume servers with `-max 0`, each on its own 1 GB filesystem image, replication `001`, then one `assign` per fresh collection until the master refused. Scaled 10:1 against the 10Gi PVC, so the limits scale with it. | `volumeSizeLimitMB` | `COPY_2` | slots reported | collections served | | --- | --- | --- | --- | | 100 | 6 | 10 | 2 | | 100 | 3 | 10 | 4 | | 50 | 3 | 20 | 7 | | 40 | 3 | 25 | 9 | | 25 | 6 | 40 | 7 | The fourth row is the pair this PR ships. The refusal arrives as `No writable volumes and no free volumes left`, with `volumes == max` on both servers and the disk nearly empty. The reported `max` matched the space the filesystem made available over the limit in every run, which is the rule the PVC figures above are divided with. Those runs sit on a 1 GB image, so their counts are the rule's, not the PVC's: a 10Gi volume has its own filesystem geometry and divides to its own numbers. ### Why both constants move The last two rows are the argument: lowering the size limit and lowering the growth count reach the same ceiling. They do not cost the same. Open descriptors scale with stored bytes rather than with the PVC, so at 400 a 10 TiB server keeps roughly 52k of them against 21k upstream, where taking the whole reduction out of the size limit would have put it at 105k, well past a 65536 `nofile` limit where one is set. Two stay open per volume, a `.dat` and an `.idx`; the `.vif` beside them is written and closed. That cost lands hardest on large instances, which never had a ceiling problem in the first place: at any of these limits a multi-TiB PVC has slots to spare. Lowering the growth count costs no descriptors at all. It costs write spread, since a bucket then spreads its writes over three volumes rather than six, and that cost is flat, it does not grow with the instance. So the reduction is split across both. Neither number is a new bet: 3 is what upstream already uses for three-copy replication. One price is shared by both routes. A collection absorbs `volumeSizeLimitMB × COPY_2` before it needs another grow, so growth requests arrive five times as often as they did at 6000 MB. A collection also starts with half as many writable volumes at `replicationFactor: 2`, and a volume server takes the writes into each volume one at a time, so a heavily parallel writer spreads over fewer volumes until the collection grows again. That holds for instances kept at 1000 too. ### On upgrade On a variant that runs platform migrations, the upgrade does not change the limit on an existing instance unless its operator already set one. Platform migration 57 writes `master.volumeSizeLimitMB: 1000` into every existing SeaweedFS instance that has not chosen a limit, except one whose `master` is not an object (below), so 400 applies only to instances created after this release. The `default` variant runs no platform migrations. A cluster there that installed SeaweedFS by hand has to apply the same patch before upgrading, and the operations guide has the command. Both guide steps end with a `complete` line. `jq` prints it only after reading the whole list, and the pinning loop stops at the first failed patch before it, so a run without that line is incomplete, whatever else it printed. A chosen limit is a whole number from 1 to 30000 and is kept. Anything else counts as not chosen and becomes 1000: the previous chart ignored the key and ran 1000 in its place, and the new schema would refuse it. A kept value other than 1000 takes effect with the upgrade itself. A lower one can strand buckets and a higher one takes the room new buckets grow into, so the guide has a query that lists those instances before upgrading. An instance kept at 1000 still gains: at `replicationFactor: 2` a grow now claims three volumes instead of six, so it has room for about twice as many new buckets. Three shapes get their own line in the Job log, with what they hold. A non-object `spec.values` is pinned too. helm-controller v1.5.0 ignores values it cannot read as a map and renders the chart defaults, so that instance runs at 1000 today and would drop to 400. The merge patch replaces the unreadable value, and nothing that applies is lost. A non-object `master` is left alone. Every v1 schema already refuses it, so that release does not render today, and the log says to set the limit in the same edit that repairs it. A `valuesFrom` with entries besides the `cozystack-values` Secret is named and pinned anyway. helm-controller merges `spec.values` over those entries, so a limit set there does not apply, unless it is set through `targetPath`, which can still win depending on the entries before it. The guide steps print the same three. The value goes into the application's HelmRelease, whose `spec.values` is the application spec, so it covers an instance created through the API and one the tenant module renders alike, the platform's own in `tenant-root` included. The tenant module renders that release without `spec.values`, which a new tenant chart test pins, and a Helm upgrade leaves alone a key that neither the previous nor the new manifest carries: I measured that on a kind cluster with the real HelmRelease CRD and Helm 4.3.0, on client-side, server-side and forced server-side apply, and read `createPatch` at the v4.1.1 that helm-controller v1.5.0 runs. Where migrations run, the order comes from the platform release itself. The migration is its pre-upgrade hook, and the `cozystack-packages` source every application chart is built from is a manifest of that same release, applied only after the hooks finish, so no instance meets the new chart unstamped and a failed migration stops the platform upgrade before that source moves. That holds while the platform source is pinned by digest, as it is out of the box; a source that follows a tag, a semver range or a branch lets source-controller move `cozystack-packages` on its own schedule, and an instance there can run at 400 until the migration has run. Until `cozystack-packages` moves, each patched release still runs its old chart, which ignores the key: no v1 release reads it, and none closes the `master` object in its values schema. The migration's test runs the script under the migrations image's shell against a fake `kubectl` that refuses any selector but the one cozystack-api lists SeaweedFS with, because an empty fleet is a legitimate answer and a mistyped selector would otherwise pass while pinning nothing. The same test runs the two guide steps, found by a marker comment above each, against the same fake and checks that they select the same releases as the migration. Lowering an existing instance is left to its operator, because it is not free. Its volumes between the new limit and the old go oversized on the next heartbeat and leave the master's writable set, and each affected collection needs one successful grow before its next write lands. Nothing is lost and the volumes stay readable. Those grows compete for one pool, and the budget is per collection rather than per server. An oversized volume is charged no headroom while one below the limit is charged all of its unfilled room, so the slots open to a stranded collection are free space less that room, divided by the limit; a grow then claims a whole `COPY_2` batch and takes as much of it as is there, and the volumes it creates are empty and charge their headroom straight back. Measured on the rig: two oversized collections with two free slots between them, and the first to write took both, leaving the second refused afterwards with the disk still reporting a quarter of itself free. Collections that keep writing are not free either: they were grown six at a time, and each of their volumes below 400 MB can hold back a whole 400 MB. Measured at a scaled pair, a server with free space for one batch per collection refused the only grow it needed, because a second, nearly empty collection held the room, and the same free space without that collection let the grow through. The master also grows, on its own schedule and with no write, any collection with no writable volume at or below 90 percent of the limit, which at 400 is 360 MB; measured at the same scaled pair, a collection with every volume in that band got two new volumes per server within one pass while a roomy one was left alone. So the precondition on every volume server is a batch for each collection with no volume at or below that line plus 400 MB for each volume still below the limit, and no volume size exempts an instance from it, since a larger volume carries more buckets and the budget scales with them. The `minFreeSpacePercent` reserve only comes into it on a server whose writable volumes have all passed the limit, since nothing there is charged headroom. It is five percent of what the filesystem reports rather than of the PVC's nominal size, so `All >= 8000 MiB` is where it reaches one 400 MB limit; a 10Gi PVC measures 9979 MiB and clears that, and such a server then refuses a grow only once its disk is down to the reserve. Everywhere else the headroom charge decides, and a refusal can come with the disk far from full whatever the reserve. Deleting a collection gives back a slot for each of its volumes, however little it stored, since the headroom charged to them leaves with them; only a server already at its reserve, where every volume is read-only and charged nothing, gets back no more than the bytes. Deleted objects return room only through a vacuum. The room left inside those volumes for further writes is given up and does not return as capacity, since such a volume is charged no headroom while its own bytes still come out of the budget. Lowering the growth count takes nothing away by contrast: no path removes a volume from the writable set for being surplus to the batch size, so an existing collection keeps what it has and converges as those volumes fill. [`docs/operations/seaweedfs-lowering-the-volume-size-limit.md`](https://github.com/cozystack/cozystack/blob/main/docs/operations/seaweedfs-lowering-the-volume-size-limit.md) has the check for it, and the way back. The ceiling cannot come out lower at any supported `replicationFactor`, which is the part worth stating plainly. The divisor halves, and the space the slot count is derived from grows at the same time, because a volume past the new limit stops being charged headroom against it. `COPY_1` and `COPY_3` are untouched, so at 1 and 3 copies the limit alone still moves the ceiling upward. ### Reaching the value Neither constant was reachable by an operator. The data-plane chart is pulled by `ExternalArtifact`, and the component publishing it declares no `install` section, so the reconciler drops a `Package`-level values override for it before any merge happens. Editing the chart was the only route, on a value whose right setting depends on the PVC size the operator chose. `master.volumeSizeLimitMB` is on the app chart now, with a schema entry. Worth being precise about which file the two numbers end up living in, because exposing one of them moves it. The app chart's HelmRelease sends the size limit on every release, so `packages/extra/seaweedfs/values.yaml` holds the 400 installs run with, and `packages/system/seaweedfs/values.yaml` keeps upstream's 1000 as the fallback a render without it gets. That fallback is deliberate: a platform upgrade can re-render the data-plane release from its artifact before the parent has sent anything new, and a lower value there would then reach the master first and drop the limit ahead of an operator's own setting. Nothing sends the growth count, so the data-plane chart's value is the live one for that half, and it is the part that does land in that window, where a grow costs 3000 MB against the 6000 MB it cost before. The budget the pair produces is written out next to the growth count, with a pointer from the size limit to it, so the two do not drift apart. The field is bounded at both ends, because either end takes an instance down. Zero is not a smaller limit but an off switch: the volume server leaves its slot calculation untouched at zero, keeps reporting no capacity, and nothing is ever grown. Past 30000 the master does not start at all, exiting on `volumeSizeLimitMB should be smaller than 30000`. The bound lives in the values schema, through the generator's `@minimum` and `@maximum` annotations, so Helm refuses the value before any template runs. The apps API reads that schema only for defaults, so it accepts a value out of range, and the release then fails to render. A case pins each edge along with the value just outside it. An explicit null gets past both bounds, because Helm drops a null key before it validates, and would reach the master as an empty `-volumeSizeLimitMB=` that the shipped image exits on at start, so the template requires the value and a case pins that as well. There is no second bound against the disk, and not for want of trying: a limit at or above the volume's own size does not break an instance. `MaybeAdjustVolumeMax` floors the slot count at one, and a 255 MiB disk against a 1000 MB limit reported `Max 1`, took an assign for a fresh collection and served back what was written to it. Such a server is capped at a single slot rather than dead, so a render refusal would stop a working release on every reconcile. The field description says instead that a value close to or above `volume.size` leaves each volume server a single slot. The growth count is deliberately not exposed beside the limit. A field wired to `COPY_2` would do nothing at 1 or 3 copies, for the reason above, while still looking like a setting, and a knob that is silently inert for two of the three supported values is worse than no knob. Making it track the replication factor is a feature with its own matrix, and it would rest on a path where the master's own default replication is separately broken. The size limit has no such dependency, so it is the one that ships. ### Picking a value 400 is sized for the shipped `volume.size: 10Gi`, which is where the slot and collection figures above come from. Going up, it holds until the two descriptors per volume meet a 65536 `nofile` limit, roughly 12.5 TiB per volume server. Going down, an instance needs 1600 MB of filesystem per volume server to serve a second collection at all, since the first claims the opening three slots. ### What the tests cover The existing bucket case creates one bucket, which sits inside the ceiling at any sizing, so it could not tell a working ceiling from an exhausted one. The new e2e case creates three beside the platform's own backup bucket and requires each to accept an object, which is the only signal the exhaustion produces. Each `mc` call there carries its own timeout, since a hung PUT is exactly that signal and the step timeout alone would not end it. It was observed red at the old sizing in [run 34948279363](https://github.com/cozystack/cozystack/actions/runs/34948279363), which ran the case against the unchanged constants: the pre-existing single-bucket case passed in 18s beside it, and the new one failed on the write to the second bucket after the first succeeded. Three rather than two so the count clears the ceiling without depending on how many slots the unnamed default collection holds, which the suite cannot observe. The unit tests pin each constant on its own, because losing one is a silent capacity regression rather than something a render would catch. The app-chart test asserts an overridden size limit as well as the default, since asserting the default alone passes just as well against a template that ignores the value and writes the number as a literal. It also pins both ends of the accepted range and the first value outside each, the `-max 0` zero on all four render paths it is written to, and the 10Gi volume default the slot figures above are measured for, which nothing else guarded. The data-plane suite pins the five percent free-space reserve as well, since the 8000 MiB threshold above is computed from it and nothing else reads it. The capacity case also reads the master's slot accounting in its `catch`. A grow failure in the master log does not say whether the shipped limit is too coarse for the PVC or an earlier collection in the same run is still holding the budget, and the per-server `Max` against `Volumes` does. Closes #4239, closes #4245, closes #4248 ### Screenshots No UI change. ### Downstream repositories Walked against the diff. The new field lands on `packages/extra/seaweedfs`, and the provider models bucket, generic, qdrant, redis and tenant, with no SeaweedFS resource among them, so there is no schema or expand/flatten pair over there for it to drift from. No package is added, renamed or removed, so the website app lists are untouched. - [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 fix(seaweedfs): raise how many buckets a new SeaweedFS instance can serve on the default 10Gi volume PVC from two to eight at `replicationFactor: 2`, by defaulting the master volume size limit to 400 MB instead of 1000 and lowering the two-copy growth count from 6 to 3, and expose the limit as `master.volumeSizeLimitMB` on the app chart. **Action required on the `default` variant only:** it runs no platform migrations, so before upgrading, set `master.volumeSizeLimitMB: 1000` on every SeaweedFS instance that has no valid value and whose `master` is an object, with the step in [`docs/operations/seaweedfs-lowering-the-volume-size-limit.md`](https://github.com/cozystack/cozystack/blob/main/docs/operations/seaweedfs-lowering-the-volume-size-limit.md). If this is skipped, every such instance drops to 400 on the upgrade, and a bucket whose volumes have all reached 400 MB takes no writes until it can grow. On the variants that run platform migrations, migration 57 does this: it sets `master.volumeSizeLimitMB: 1000` on every instance that has not chosen a value, except one whose `master` is not an object, which no v1 chart renders and which it names in its Job's log instead, so nothing changes for an instance unless it already names a different value, which the previous chart accepted and ignored and this one applies; the same guide lists such instances before the upgrade, and the migration names them in its Job's log. An existing value that is not a whole number from 1 to 30000 is replaced with 1000, which is what the previous chart applied in its place. Instances at 1000 still get roughly twice the room for new buckets from the lower growth count. The upgrade rolls every SeaweedFS master StatefulSet once, since the growth count is part of the master's environment, and an instance that names a value other than 1000 rolls a second time when that value arrives. The pin lives in the instance's spec, so a tool that replaces the whole resource from a manifest without `master.volumeSizeLimitMB` removes it and the instance takes 400; add the field to such manifests before upgrading. Lowering the limit on an instance that already holds data takes every volume at or above the new value out of the master's writable set, and raising it takes the room new buckets grow into, so the same guide describes the free-space check to run first and how to set it back. ``` |
||
|
IvanHunters
|
c983390b39 |
ci(e2e): surface overlay drift and remove both causes of it (#4261)
## What this PR does Six commits against one script: one reports a silent degradation, three remove causes of it, two repair the tests and the reasons recorded beside them. **Why it matters.** A file whose config differs from the current-main artifact beyond its image lines keeps its committed ref. That is the right safe answer, and it is not a neutral one: the component then runs its last RELEASE image in a lane whose job is to exercise the tree under review, and the only trace is one line among hundreds. Seven packages took that path on the run behind #4257. One of them, `backupstrategy-controller`, served an image from v1.6.0 (22 July) against a tree carrying four backup strategy drivers added after it, so five of that run's seven chainsaw failures came from there, and three unrelated pull requests each spent a review round establishing that the red lane was not theirs. After this PR that list is empty. Against the public artifact at its own revision, with the tree checked out at that same revision: ```text before drift=5 overlaid=29 core/platform, cozystack-api, dashboard, kubeovn-webhook, linstor-gui after drift=0 overlaid=34 ``` **Naming the drifted packages.** One GitHub annotation after the existing tally. Which files get overlaid does not change, and a run with no drift emits nothing. **Cause one: a suffixed image key.** The classifier accepted a key only when it was exactly `image`, `repository`, `registry`, `tag` or `digest`. A chart rendering several images names the extras after what they are for, so `backupstrategy-controller` carries `chBackupClientImage`, `rabbitmqBackupClientImage` and `redisBackupClientImage`. The first two passed only through the `@sha256:` branch; the third is pinned by tag alone, so it read as a config change and cost the whole file its overlay. The fix accepts a prefix before `image`, and two things keep it narrow. A letter is required before `image`, which a key beginning with it has nothing to supply, so `imagePullPolicy` and `imagePullSecrets` stay config; the trailing colon is not what does that, and they match neither with it nor without. The value must also hold a repository path, which keeps out operator-settable knobs named after an image: `vddkImage: ""` in `core/platform` and `migration-controller` is configuration the build never stamps, and absorbing it would be the opposite of what this script is for. **Cause two: a blank line the build removes itself.** `yq -i` re-emits the document when it stamps a digest, and a blank line separating two top-level keys does not come back. So a file the PR never touched differs from the artifact on a line carrying no configuration at all, and loses its overlay for it: ```text $ diff repo/system/cozystack-api/values.yaml artifact/system/cozystack-api/values.yaml 4d3 < 6c5 < image: ...cozystack-api:v1.6.0@sha256:c4d374a9... --- > image: ...cozystack-api:main@sha256:2fb87cf8... ``` Running the stamping assignment over the committed file reproduces that diff exactly, which is the second signal behind the diagnosis rather than the first one twice. Blank lines are now discounted before the test; a line with content still counts, so a genuine config difference keeps the committed ref as before. This accounted for all five remaining packages, and it retires the explanation offered in #4265, which ruled `yq` reformatting out. That was wrong: the round-trip check behind it did not cover the blank line. `cozystack-api` and `dashboard` are not peripheral here. An e2e result about either was a result about the last release rather than about `main`. **Repairing the tests.** `hack/bats-no-exit-trap.bats` audits every `.bats` file under `hack` against the debt its own header declares, and this file was installing three traps beyond it. An EXIT trap inside an `@test` body replaces the one the bats binary installs, so a failing test prints no TAP line at all and the suite reads as green. The answer is to stop installing the handler rather than to raise the number, so those tests clean up at the end of the body instead. ### Tests Eight cases in `hack/overlay-main-images_test.bats`, each held by a mutation of the line it describes: - drift is summarised with the package names and the count; suppressing the annotation or dropping the names reddens the suite - a clean run emits no annotation - a suffixed image key pinned by tag alone overlays instead of drifting; reverting the classifier reddens it - a key merely STARTING with `image` is still config - an `*Image` key whose value is operator configuration rather than a path still drifts, so `vddkImage` is not absorbed - two drifted files in one package count as one package, and the name carries no `/images` suffix - a blank line lost to `yq` is not a config change; removing the filter reddens it - a real config line alongside a blank line still counts as drift; widening the filter to drop every line reddens this one and two others The suite's plan and its `ok` count agree at 21, which is the reconciliation the trap audit exists to protect. Green under the `bats` binary and under `hack/cozytest.sh`, which is what `make bats-unit-tests` runs and the only one of the two whose result decides the gate. ### What this does not close `redisBackupClientImage` is still pinned by tag with no digest, and the tag it names does not exist: ```text $ curl .../cozystack/redis-backup/tags/list {"tags":["nightly","0.0.0-nightly.20260911...","0.0.0-nightly.20260912...", ...]} ``` Only nightly tags are published, so `redis-backup:v1.6.0` resolves to nothing. Pinning it needs a published release tag first, which is #4264 rather than this PR. Making the overlay line-wise, so image lines transfer even when other lines genuinely differ, would go further still. It trades away the guarantee the script is built on, that a file comes wholly from one source, and that guarantee is deliberate and commented: a maintainer decision rather than a quiet change from a reviewer. ### Screenshots Not a UI change. ### Downstream repositories - [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 * **New Features** * GitHub Actions now warns when image overlay processing detects configuration drift. * Warnings list affected package names in sorted, de-duplicated form. * Image references with alphabetic prefixes are recognized during overlay processing. * Blank-line-only differences no longer trigger drift warnings. * **Tests** * Added coverage for image references, similarly named configuration keys, blank-line differences, and multi-package drift reporting. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
a2db2aeb8c |
fix(seaweedfs): raise the number of buckets a volume server can serve
A volume server started with -max 0 takes a slot count derived from disk rather than a fixed one, replication 001 spends a slot on each of the two servers per logical volume, and a collection claims COPY_2 volumes whenever it grows. So the bucket ceiling is set by the product of the size limit and COPY_2, and moving either number alone moves it. The mechanism is upstream in MaybeAdjustVolumeMax (weed/storage/store.go) and findVolumeCount (weed/topology/volume_growth.go). Do not divide these out of the nominal PVC size, because the count follows the space the filesystem makes available and not the volume's advertised size: the shipped 10Gi volume PVC was seen to give nine slots per server at the upstream 1000 and 19 at 500, which puts 400 at 23 to 24 by the same division, or eight collections against two. The span is the observation's own, since the free space behind 19 at 500 fits more than one filesystem geometry and those divide differently at 400; eight either way, because a collection that cannot get a whole batch of COPY_2 keeps the volumes it did get and writes into them. The division itself is measured: on a 1 GB image the master reports max as the filesystem's available space over the limit in every run, 25 slots and nine collections at the 40 and 3 that scale to this pair. Two is what the platform's own cozy-backups bucket and a single tenant bucket already use. Past that every S3 PUT to a new bucket times out while the disk still reads as nearly empty, and no COSI object reports anything wrong, because the claim and the access are both granted. 400 and 3 make the budget 1200 MB per collection. Splitting the reduction across both numbers rather than taking it out of the size limit alone keeps the cost off the curve that grows: open descriptors track stored bytes divided by the limit, so a 10 TiB server keeps roughly 52k of them at this limit against 21k upstream, where moving the size limit alone would have put it at 105k. A .dat and an .idx stay open per volume; the .vif beside them is written and closed. The ceiling reached is the same either way. Both figures describe the shipped replicationFactor of 2. The master picks its growth count from the replication the filer asks for, so at 1 and 3 copies it reads COPY_1 and COPY_3 instead and this COPY_2 does not apply; lowering the size limit still raises the ceiling there. The size limit was out of an operator's reach: it lived in the data-plane chart, which is pulled by ExternalArtifact from a component that declares no install section, so a Package-level values override for it is dropped before any merge. The app chart now declares master.volumeSizeLimitMB and sends it on every release, and 400 is set there. The data-plane chart keeps upstream's 1000 as its fallback on purpose. A platform upgrade can re-render that release from its artifact before the parent has sent anything new, and a lower fallback would then reach the master first and drop the limit ahead of an operator's own setting. COPY_2 is sent by nothing and lives in the data-plane chart, so it does land in that window, where a grow costs 3000 MB against the 6000 MB it cost before. The app chart also writes maxVolumes into dataDirs on every release, and because dataDirs is a list Helm replaces the data-plane chart's copy wholesale, so the zero that makes the slot count auto-derived at all comes from there. Losing it drops the server to the subchart's fixed seven without failing any render. The field is bounded at both ends, because either end takes an instance down. Zero is not a smaller limit but an off switch: the volume server leaves its slot calculation untouched at zero, keeps reporting no capacity, and nothing is ever grown. Past 30000 the master does not start at all. The bound sits in the values schema rather than in a template guard, so Helm refuses the value before any template runs. The apps API does not: it reads that schema only for defaults, so it accepts a value out of range, and the release then fails to render. An explicit null gets past both bounds, because Helm drops the key before it validates, and would render an empty -volumeSizeLimitMB= that the master exits on at start, measured on the shipped image, so the template requires the value. No bound against the disk goes with it, though the field invites one. A limit at or above the volume's own size does not break an instance: MaybeAdjustVolumeMax floors the slot count at one, measured on 4.31 with a 255 MiB disk against a 1000 MB limit, which reported Max 1 and served a write it then read back. Such a server is capped at a single slot rather than dead, so refusing it at render would be a capacity policy that stops a working release on every reconcile, not only on the upgrade. The field's description carries the relation instead, where the value is chosen. The growth count is deliberately not exposed beside the limit. The master picks which of its four growth counts to apply from the replication the filer requests, and the filer takes that from replicationFactor, so a field wired to COPY_2 would do nothing at 1 or 3 copies while still appearing to be a setting. A knob that is silently inert for two of the three supported values is worse than no knob. Two further prices. A collection absorbs the budget before it needs another grow, so growth requests arrive five times as often as at the upstream pair. A collection also starts with half as many writable volumes, and a volume server takes the writes into each volume one at a time (syncWrite holds that volume's dataFileAccessLock), so a heavily parallel writer spreads over fewer volumes until the collection grows again; that holds for instances kept at 1000 too. And lowering the limit on an instance that already holds data carries the other one: its volumes between the new limit and the old go oversized on the next heartbeat and leave the master's writable set, and each affected collection needs one successful grow before its next write lands. Nothing is lost and the volumes stay readable; the room left inside them for further writes is given up and does not return as capacity. Those grows compete for one pool, and the pool is smaller than it looks. An oversized volume is charged no headroom, while one below the limit is charged all of its unfilled room; a grow then claims a whole COPY_2 batch and takes as much of it as is there, and the volumes it creates are empty and charge their headroom straight back. Measured on a local SeaweedFS 4.31 setup: two oversized collections with two free slots between them, the first to write took both, and the second stayed refused with the disk still reporting a quarter of itself free. Collections that need no grow are not free either, since their volumes below the new limit were created six at a time and each can hold back a whole limit. Also measured there, with the limit and batch scaled down: a server with free space for one batch per collection refused the grow its one affected collection needed, because a second, nearly empty collection held the room, and the same free space without it let the grow through. The minFreeSpacePercent reserve only comes into it on a server whose writable volumes have all passed the limit, where nothing is charged headroom: from 8000 MiB of filesystem upward, a 10Gi PVC measuring 9979, five percent is at least 400 MB, and such a server then refuses a grow only once its disk is down to the reserve. Everywhere else the headroom charge decides, and a refusal can arrive with the disk far from full whatever the reserve. Deleting a collection gives the room back a slot per volume, however little it stored, since the headroom charged to those volumes leaves with them. So, on a variant that runs platform migrations, the upgrade does not change the limit on an existing instance unless its operator already set one, which the previous chart accepted and ignored. Platform migration 57 writes master.volumeSizeLimitMB: 1000 into every existing SeaweedFS instance that has not chosen a limit, and 400 applies only to instances created from here on. It leaves alone only an instance whose master is not an object, which no v1 chart renders, and names that one in its log. The default variant runs no platform migrations, so a cluster there that installed SeaweedFS by hand gets no pin, and the document below gives the same patch to apply before upgrading. A chosen limit is a whole number the new chart accepts; anything else counts as not chosen and is replaced with 1000, which is what the previous chart ran in its place, since it ignored the key and the new chart would refuse to render it. A valid value already set is kept, and since the previous chart ignored it, the upgrade is when it starts to apply; the document below lists the ones that change the limit ahead of it. Raised values are listed as well: existing buckets keep writing, but every volume below the new limit is charged up to it, and on a local SeaweedFS 4.31 setup seven nearly empty volumes left no free slot once the limit went up tenfold, so a new bucket was refused. The value goes into the application's HelmRelease, whose spec.values is the application spec, for an instance created through the API and for one the tenant module renders alike. The tenant module never renders spec.values on that release, and a Helm upgrade leaves a key alone that neither manifest carries: measured with Helm 4.3.0 on client-side, server-side and forced server-side apply, and read in createPatch at the v4.1.1 that helm-controller v1.5.0 runs. Where the migration runs, it is a pre-upgrade hook of the platform release, and the cozystack-packages source every application chart comes from is a manifest of that same release, applied only after its hooks; so no instance meets the new chart unstamped, and a failed migration stops the platform upgrade before that source moves. That holds while the platform source is pinned by digest, as it is out of the box; one that follows a tag, a semver range or a branch lets source-controller move cozystack-packages on its own schedule. Until cozystack-packages moves, each patched release keeps its old chart, which ignores the key, since no v1 release reads it or closes the master object in its schema. An instance kept at 1000 still gains, because at a replication factor of 2 a grow now claims three volumes rather than six, so it has room for roughly twice as many new buckets. Lowering an existing instance is left to its operator, and docs/operations/seaweedfs-lowering-the-volume-size-limit.md is the check for it. It lists each server's volume files and adds one limit for every volume below the limit and a batch for every collection with no volume at or below 90 percent of it, since the master grows such a collection on its own schedule without a write; measured the same way, a collection with every volume in that band and no writes got two new volumes per server within one pass. Where each disk type is served by as many volume servers as the replication factor, as the shipped two at a factor of 2 are, each collection's volumes sit on every server of that type, and that bounds the claim from above without assuming how the collections grew. The master places a new volume on any server of the requested disk type with room rather than where its collection already lives, whichever group or pool that server belongs to, and it reads an empty disk type as hdd; under MultiZone it also grows each zone on its own. So the document keeps zoned instances, and any disk type served by more servers than copies, at 1000. BREAKING CHANGE: on the default variant, which runs no platform migrations, set master.volumeSizeLimitMB to 1000 before upgrading on every SeaweedFS instance that has no valid value and whose master is an object, with the step in docs/operations/seaweedfs-lowering-the-volume-size-limit.md. On the variants that run platform migrations, migration 57 does this. If the step is skipped, every such instance drops to 400 on the upgrade: each of its volumes at or above 400 MB leaves the master's writable set, and a bucket whose volumes have all crossed that line takes no writes until it can grow. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
c038cf25d3 |
test(seaweedfs): cover the bucket count a volume server can serve
A volume server started with -max 0 derives the slots it has left from the free space on its disk, less the room its unfilled volumes still claim, divided by the master's volume size limit, and each collection claims a batch of those slots on every volume server the replication spans. The number of buckets an instance can hold therefore has a ceiling well below what the free bytes suggest, and crossing it leaves every COSI object reporting ready while the S3 PUT times out. The existing case creates one bucket, which sits inside that ceiling at any sizing, so nothing here told a working ceiling from an exhausted one. This case creates three beside the platform's own backup bucket and requires each to accept an object, which is the only signal the exhaustion produces. Three rather than two because the count has to clear the ceiling whether or not cozy-backups and the unnamed default collection have grown yet, which this suite cannot observe. BucketAccess reconciles separately from the claim it belongs to, and the write step reads the Secret that access produces, so readiness of the release and the claim is not enough to enter it. Without the access assertions the step can fail on a missing Secret, which looks the same in the report as the capacity failure it is here to measure. The catch reads the master's slot accounting as well as its log. A grow failure in the log does not say whether the shipped limit is too coarse for the PVC or an earlier collection in the same run still holds the budget, and the per-server Max against Volumes does. The read runs inside the master, whose image ships curl, so it needs nothing on the host and adds no second fixed port to the reason this suite cannot run in parallel. That reason now covers two cases rather than one, which the shared config says. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
IvanHunters
|
b99db147a7 |
test(ci): isolate the commit-trailer fixture from global tag signing (#4271)
## What this PR does `make_repo` in `hack/check-commit-trailers.bats` silences `commit.gpgsign` in the fixture repository but leaves `tag.gpgsign` alone. Anyone who signs tags has that setting globally, it reaches the fixture, `git tag start` becomes an annotated tag, and git refuses it for want of a message. The helper opens all 27 tests, so the whole suite dies before a single test reaches its body, and `make bats-unit-tests` takes `make unit-tests` down with it. Details and the reproduction are in #4270. The first commit is that one line, next to the one that is already there. With it the suite went 27/27 on a machine where the global setting is set, and was unchanged on one where it is not. I ran it both ways. CI never saw this. The `Commit trailers` job runs the checker against explicit SHAs and does not touch the bats file, and a fresh runner has no global git configuration to leak in. The failure only lands on the local `make unit-tests` that `docs/agents/contributing.md` asks for before pushing, which is the worst place for it: an unexplained red in a file the contributor did not touch. That is also how the guard went missing in the first place, and it would go missing again the same way, so the second commit adds `make_repo holds against a hostile global configuration`. None of the three `git config` lines in `make_repo` is exercised on a runner: delete any of them and CI stays green. The test supplies the hostile configuration itself rather than waiting for a contributor to have one, so each guard is held by the suite. Pulling them one at a time gives three distinct reds: `tag.gpgsign` gives `fatal: no tag message?`, `commit.gpgsign` gives `gpg failed to sign the data`, and `core.hooksPath` lets a global `commit-msg` hook rewrite the message and fails the assertion on it. The suite is 28/28 both with the global settings and without, so a runner sees no change. `hack/release-changelog-behaviour.bats` has gaps of the same kind in its own fixture, which sets `commit.gpgsign` and nothing else. The missing `tag.gpgsign` is inert there because it creates no tag. The missing `core.hooksPath` is not: a global `pre-commit` hook that exits non-zero kills every `git commit` it makes. I left both alone rather than widen this diff, and noted them in the issue. ### Screenshots Not a UI change. ### Downstream repositories Walked the trigger map against the diff. The only `hack/`-related trigger is `cozystack/ccp`, and it fires on moving or renaming something under `hack/` or changing what a make target does. This adds a line inside an existing file; the filename and the make target behave as before, except that the target stops failing. - [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 - **Bug Fixes** - Repository fixtures now remain reliable despite conflicting global Git settings, including commit and tag signing, failing signing tools, and commit-message hooks. - Fixture setup preserves lightweight tags and the original seed commit message, ensuring consistent repository initialization across environments. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
IvanHunters
|
379c763fc3 |
test(ci): drop an inert key from the fixture's hostile configuration
tag.forceSignAnnotated governs annotated tags and `git tag <name>` creates none, so with tag.gpgsign unset the tag still resolves to a commit object. The key cannot make the new test fail and guards nothing the fixture could lose; carrying it implied a fourth guard that does not exist. Assisted-by: LLM Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
ef3bfd71d3 |
docs(ci): record what actually narrows the overlay key branch
Two comments credited the trailing colon with keeping imagePullPolicy and imagePullSecrets out of the key branch. It does not: the branch requires a letter before image, which a key beginning with image has nothing to supply, so neither matches with the colon or without it, even when the value carries a slash. The colon earns its place elsewhere, as the point the value scan starts from, and removing it reddens the suffixed-key test rather than the pull-policy one. A reader acts on the recorded reason, so a wrong one costs more than none. Refer to that test by name rather than by ordinal: the number comes from the runner's output order and moves when anything is inserted above it. While here, give the pull-policy test a failure message about pull policies: it read 'a PR-edited package was overlaid', which belongs to a different case. Assisted-by: LLM Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
609a014cc4 |
fix(ci): make the clean-run overlay test able to fail, and narrow the key branch
`! grep -q …` is a pipeline preceded by `!`, which POSIX exempts from set -e, and hack/cozytest.sh appends `return 0` to every converted test function, so a failed negation was discarded twice over. That line was the only assertion in 'no annotation when nothing drifted': mutating the drift>0 guard to `true` left the suite green under both runners. Replace all three negations with an explicit if/exit; the mutation now reddens test 19. The prefixed *Image branch matched any key ending in image, including operator-settable ones the build never stamps: vddkImage (core/platform, migration-controller) defaults to "" and a change there was absorbed from the artifact in silence, the opposite of what this script is for. Require the value to hold a repository path, which a ref always does and "" does not. The annotation counted ref-bearing files while naming deduplicated dirnames, so one package with a drifted values.yaml and a drifted images/*.tag read as two, and named <pkg>/images, which is not a package. Count the named list instead. Assisted-by: LLM Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
e9f666bc25 |
fix(ci): stop a blank line lost to yq from costing a package its overlay
The overlay keeps a package on its committed release ref when the artifact copy
differs from the repo on any line that is not image-reference-bearing. A blank
line is such a line, and the build produces one on its own: yq -i re-emits the
document when it stamps a digest, and a blank line separating two top-level keys
does not come back.
So a file the PR never touched differs from the artifact on a line carrying no
configuration, and loses its overlay for it. That was the whole of the remaining
drift list. Against the public nightly artifact at its own revision, with the
tree checked out at that same revision:
before drift=5 overlaid=29 core/platform, cozystack-api, dashboard,
kubeovn-webhook, linstor-gui
after drift=0 overlaid=34
Each of the five differed from the artifact by one blank line and its own ref,
nothing else, which is reproducible locally by running the stamping assignment
over the committed file. cozystack-api and dashboard are not peripheral: an e2e
result about either was a result about the last release rather than about main.
Discount blank lines before the test. A line with content still counts, so a
real config difference keeps the committed ref as before.
Assisted-by: LLM
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
|
||
|
IvanHunters
|
80a41e4e6f |
test(ci): stop the overlay tests installing EXIT traps, and pin the key branch
hack/bats-no-exit-trap.bats audits every .bats file under hack against the debt its own header declares, and this file was installing three traps beyond it. An EXIT trap inside an @test body replaces the one the bats binary installs for its own bookkeeping, so a test that then fails prints no TAP line at all and the suite reads as green. That is the reason the count is exact rather than a ceiling, and the reason the answer here is to stop installing the handler rather than to raise the number. Clean up at the end of the body instead: both runners set -e, so on failure the cleanup is unreachable and the scratch directory survives for inspection. The key branch had nothing holding its shape. A test now pins each half: that a suffixed key such as clientImage is read as a ref, and that a key merely beginning with image is not. Assisted-by: LLM Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
8ac99b67cc |
fix(ci): read a suffixed image key as a ref, not a config change
The classifier accepted a key only when it was exactly image, repository, registry, tag or digest. A chart that renders several images names the extras after what they are for, so backupstrategy-controller carries chBackupClientImage, rabbitmqBackupClientImage and redisBackupClientImage. The first two reach the classifier through the @sha256: branch; the third is pinned by tag alone, so it read as a config change and cost the WHOLE file its overlay. That package therefore kept its committed release ref in every lane that did not rebuild it, and served an image from v1.6.0 (22 July) against a tree carrying four backup strategy drivers added after it. Five of the seven chainsaw failures behind #4257 trace to that, across three unrelated pull requests. Accept a prefix before image in the key branch. Assisted-by: LLM Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
618ab58482 |
ci(e2e): name the packages that kept release refs instead of current main
A file whose config differs from the artifact beyond its image lines keeps the committed ref. That is the safe answer, and it is not a neutral one: the component then runs its last release image in a lane whose job is to exercise the tree under review, and the only trace is one line among hundreds of overlay lines. Six packages took that path on a recent run, backupstrategy-controller among them, which is why four backup round-trip suites refused Kinds the tree supports and three unrelated pull requests each spent a round establishing the red lane was not theirs (#4257). Summarise the drifted packages in one GitHub annotation. Nothing about which files are overlaid changes; a run with no drift emits nothing. Assisted-by: LLM Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
80ab355d66 |
test(ci): hold the fixture's config guards from CI
make_repo silences commit.gpgsign, tag.gpgsign and core.hooksPath because a contributor's global configuration reaches a fresh repository. A runner carries no such configuration, so none of the three is exercised there: the suite stays green with any of them deleted, and the failure lands only on the machine of whoever signs their commits or tags. That is how tag.gpgsign came to be missing without anything going red. The new test supplies the hostile configuration itself, so removing a guard now fails in CI rather than on a contributor's first local run. Assisted-by: LLM Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
5a777e9cfc |
test(ci): isolate the commit-trailer fixture from global tag signing
make_repo silences commit.gpgsign in the fixture repository but not tag.gpgsign, so a contributor who signs tags has that setting reach the fixture. git tag start then becomes an annotated tag, git refuses it for want of a message, and since the helper opens all 27 tests the suite fails before a single one reaches its body, taking make unit-tests with it. CI never saw it: the Commit trailers job runs the checker against explicit SHAs and does not touch this file, and a fresh runner has no global configuration to leak. The failure only ever lands on the local run that contributing.md asks for before pushing. Assisted-by: LLM Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
637098f222 |
test(kafka): anchor the release selector the hook suites assert on
Both suites matched "strimzi.io/cluster=kafka-test" as a bare substring, and that is a prefix of the sibling release kafka-test2 and of kafka-test-topics. Rendering the hook with either selector left both suites green, so neither pinned the scoping they exist to pin. Anchor on the leading "-l " and on the trailing delimiter; both variants now red in both suites, and the six earlier hook mutations still red exactly where they did. Drop the notMatchRegex on a leading slash. It cannot fail on its own: the anchored image pattern above rejects a leading slash first, and an empty _cluster.images-registry renders no slash to begin with. Its rationale moves into that pattern's comment. Record why activeDeadlineSeconds and the 180s wait have to move together. A deadline that fires mid-wait kills the Job, and with backoffLimit: 0 the hook then returns non-zero, which is the wedged uninstall the deliberate exit 0 on the timeout branch exists to avoid. Assisted-by: LLM Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
415aa82409 |
test(kafka): drive the hook suite through the runner CI actually uses
The suite used bats' run helper. make bats-unit-tests does not invoke the bats binary: it runs hack/cozytest.sh over every hack/*.bats, and that runner implements @test and little else, so the file died at 'run: command not found' and took make unit-tests with it. Under bats it was 5/5, which is where the 5/5 in the previous commit came from. Save $? by hand instead, the shape app-cleanup-hooks_test.bats already uses; verified under both runners. The fake kubectl no longer defaults DELETE_RC to 0. An unset value made every failure-path assertion vacuous; now it exits 64 and says so. Cover both RESTMapper wordings for an absent CRD rather than just the first. Widen the image pattern to v[0-9]+\.[0-9]+(\.[0-9]+)? like harbor and mariadb: pinning v1\.32 reintroduced the breakage the digest was made pattern-based to avoid, one Renovate bump later. Assert the _cluster.images-registry override and the no-leading-slash default, neither of which was covered: the hook is the one image in this chart whose pull failure surfaces at uninstall. Assisted-by: LLM Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
3326e0fb9e |
test(kafka): run the pre-delete hook against a fake kubectl
The hook's exit code is the whole contract: a non-zero one returns from Uninstall.Run before any release resource goes, keeping the topic operator alive; a zero one lets Helm remove it. delete_test.yaml matches manifest text, so it stayed green when the timeout branch was put back to exit 1, when the CRD-absent branch was, and when the selector was widened past this release. Add a bats suite in the shape hack/app-cleanup-hooks_test.bats already uses for qdrant and harbor: render the script, run it against a fake kubectl, pin the exit code of each branch. Each of the three mutations above now turns it red. Drop hook-succeeded from the delete policy and reclaim the Job with ttlSecondsAfterFinished instead. The script exits 0 on the degraded path too, where topics still hold the finalizer, so deleting on success removed the only record of that seconds after it was written. Assert both in delete_test.yaml. Pin the hook image by shape rather than by literal digest: Renovate rewrites digests under packages/*/templates/hooks/ and nothing rewrites tests/, so the literal turns the next bump red for no defect. Assisted-by: LLM Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
Yan Bondarenko
|
9d2fddd4e6 |
test(kafka): assert release topics exist before the deletion check
The post-delete absence assertions could pass vacuously if the two release topics were never created. Asserting both topics first makes the E2E prove that the pre-delete cleanup actually removed them. Signed-off-by: Yan Bondarenko <202671653+yankawai@users.noreply.github.com> |
||
|
Yan Bondarenko
|
44ba13b527 |
fix(kafka): delete release topics before the topic operator is removed
Signed-off-by: Yan Bondarenko <202671653+yankawai@users.noreply.github.com> |
||
|
Aleksei Sviridkin
|
070dcb1e30 |
fix(e2e): wait for the redis replica link instead of sampling it once (#4252)
## What this PR does The `verify-tenant-ca-projection` step in the redis suite read `INFO replication` from both pods once and failed the run when a replica reported `role:slave` without `master_link_status:up`. That value is still in motion at that point, so one sampling decided the result. Redis reports the link up only once a replica reaches `REPL_STATE_CONNECTED`. The operator's readiness script admits a replica on two other predicates: `master_host` being off `127.0.0.1`, and `master_sync_in_progress` not being `1`. `master_host` is set the moment `REPLICAOF` is issued, ahead of the link coming up, and `master_sync_in_progress` is `1` only during the transfer phase, so every handshake state between the two is ready and unlinked. A dropped link re-enters the same window. `readyReplicas: 2` therefore establishes nothing about the link, and the comment on the step calling the check deterministic was wrong. The step now polls the topology to a bounded deadline and fails only if it does not settle, reporting each pod's observed state and the elapsed time. Nothing models the link declaratively: the RedisFailover CRD defines no status at all. So the poll carries the `TODO(e2e-replace-fixed-timeouts):` marker the e2e conventions require for a wait with no object behind it. What the step does now is wait for convergence where it used to sample once. No claim about flake rate follows from that. A green run says nothing about a failure that shows up in a minority of runs. Shell embedded in a chainsaw step has no unit-test surface in this repository, so this change is exercised by the suite and nothing else. Two pre-existing diagnostics defects in the same file turned up while reading it, tracked in #4246 and #4250. Neither is touched here. The first one matters a little more after this change: the poll runs its read path about a dozen times per step instead of twice. Closes #4238 ### Screenshots Not a UI change. ### Downstream repositories - [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: I walked the trigger map against the diff rather than the title. The change edits one file under `hack/e2e-chainsaw/`, moves and renames nothing, and alters no make target, so none of the listed triggers fire. The node-prerequisite trigger that reaches `ansible-cozystack` and `talm` is `hack/e2e-prepare-cluster.bats`, which this does not touch. ### Release note ```release-note fix(e2e): the redis TLS suite waits for the replica link to come up instead of reading it once, so a link that is still establishing no longer fails the run ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Tests** - Improved Redis TLS topology validation by allowing replication status time to stabilize before verification. - Extended the test operation timeout to reduce failures caused by slow environment startup or synchronization. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
999a8f5456 |
fix(e2e): keep backup preflight failure evidence across attempts (#4242)
## What this PR does The S3 access preflight that runs before a backup roundtrip prints its port-forward capture only when the port-forward never comes up. On the failure that actually happens, a healthy port-forward and an S3 roundtrip that never completes, that capture is written and then thrown away. The mc log is truncated at the top of every attempt on top of that, so what reaches the job log is the tail of the last attempt with nothing in it to say which stage died. One real run left a single line of evidence, an upload progress line, which reads as an upload that finished and a download that hung. The upload was what failed. The timeout path now prints both logs, the mc log accumulates across attempts, and every attempt records which of PUT, GET, compare or DELETE it reached. That is enough to tell a broken channel to S3 from S3 refusing the operation, and to see how many attempts ran and how each one ended. The port-forward log goes in whole rather than filtered. `kubectl port-forward` writes one `Handling connection for <port>` line per TCP connection, and whether those lines are there at all is the answer to whether mc ever reached the forwarder, so dropping them to cut noise would delete the signal this change exists to produce. Diagnosability only. Nothing changes about what the preflight passes or fails. Closes #4240 ### Screenshots No UI change. ### Downstream repositories Walked the trigger map in `docs/agents/contributing.md` against the diff. Nothing under `hack/` is moved or renamed, no make target changes behaviour, and the node prerequisites the Ansible collection and talm mirror are untouched. - [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 fix(e2e): a backup access preflight that times out now prints the port-forward log and the outcome of every attempt, not just the tail of the last one ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Backup access preflight checks now validate the complete upload, download, comparison, and cleanup sequence. - Retry results identify the specific operation that failed and preserve failure details across attempts, improving troubleshooting. - **Tests** - Added coverage for reporting the first failed upload. - Added timeout-path coverage for readiness checks, repeated operation failures, and reporting each affected stage. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
4878250a1d |
fix(e2e): wait for the redis replica link instead of sampling it once
The redis-tls topology check read INFO replication from both pods once and failed the suite when a replica reported role:slave without master_link_status:up. That value is legitimately in motion there. Redis reports the link up only in REPL_STATE_CONNECTED, while the operator's readiness script admits a replica on two different predicates -- master_host off 127.0.0.1, and sync not in the transfer phase -- so every handshake state between the two is ready and unlinked, and a dropped link re-enters that window. Ready replicas carry no evidence about the link at all, which is more than the step's own comment allowed for and makes its claim of determinism wrong. Poll the topology to a 60s deadline instead, and on expiry report each pod's observed state along with how long it waited. Nothing models the link declaratively -- the RedisFailover CRD defines no status, so there is no condition to assert on and the poll carries the marker the e2e conventions require. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Timofei Larkin
|
ae7a7115f5 |
fix(harbor-rd): carry the release prefix in the resource selectors (#4170)
<!-- 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 `kind/backport` if it's a bugfix that needs to be
backported to a previous version.
-->
## What this PR does
`harbor` declares `release.prefix: "harbor-"` and then selects its
Secret, Service and Ingress without it, so all three `resourceNames`
selectors match nothing. This corrects the three names and adds a guard
so the next omission fails a test instead of passing silently.
### Why they never matched
Release names are `<prefix><app name>`, but the lineage webhook
evaluates selectors against `{kind, name, namespace}`, where `name` is
the custom resource's name and carries no prefix. Every definition
therefore spells the prefix out itself, and the others do — `postgres-{{
.name }}-credentials`, `bucket-{{ .name }}-credentials`, and so on.
`harbor` is the only one that does not.
A selector that matches nothing raises no error: the webhook labels the
object `internal.cozystack.io/tenantresource=false` and continues, which
is indistinguishable from a selector that legitimately has nothing to
match. That is why this survived.
### What actually changes at runtime
Close to nothing, and that is the point — the manifest was wrong, not
the behaviour:
- the Secret already reaches tenants, because #2541 worked around this
exact mismatch by stamping `apps.cozystack.io/user-secret: "true"` on it
and matching by label. That workaround is deliberately left in place;
- the Service and Ingress labels flip from `false` to `true`, which
nothing consumes: `spec.services` and `spec.ingresses` are read only by
the webhook that computes the label, and the label's only consumer is
the TenantSecret registry, which handles Secrets. The dashboard reaches
both through the per-release Role, which names them correctly.
So this closes a latent defect before it bites, rather than fixing an
outage.
### The guard
`hack/check-rd-release-prefix.bats` asserts that every `resourceNames`
entry in `packages/system/*-rd/cozyrds/*.yaml` contains `<prefix>{{
.name }}`. `hack/*.bats` is auto-discovered by `BATS_UNIT_FILES`, so it
runs under `make unit-tests` with no registration.
The assertion is "contains", not "starts with", because operator-created
objects legitimately carry leading words of their own: mongodb excludes
`internal-mongodb-{{ .name }}-users`, which is correct and stays green.
Definitions with an empty prefix are skipped, since `hack/update-crd.sh`
generates the definitions for `packages/extra` charts with `PREFIX=""`.
A selector that is correct and still fails the check can opt out on its
own with a trailing `# cozy-lint:ignore rd-release-prefix -- <reason>`
comment. A directive without a reason does not count.
Verified by mutation: the guard fails on the three harbor selectors
before the fix and passes after. Across the rest of the tree `harbor`
was the only violation.
### What this PR does not do
It does not settle whether the prefix belongs in the template context
instead. That would remove the duplication, but it requires migrating
the definitions that spell the prefix out today, or they would double
it. The guard is the cheaper half and closes the class either way.
### 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:
- [ ] [cozystack/community](https://github.com/cozystack/community) -
follow-up:
Trigger map walked against the diff, which touches
`packages/system/harbor-rd/cozyrds/harbor.yaml` and adds
`hack/check-rd-release-prefix.bats`. The two nearest entries were
checked and neither fires:
- **terraform-provider-cozystack** lists "change `release.prefix`, or
rename an output Secret or Service". Neither happens here:
`release.prefix` is untouched and no object is renamed. The provider
concatenates the prefix itself to derive names, so it was already
computing the correct ones — it is this definition's selectors that were
wrong.
- **ccp** lists "rename `packages/system/<name>-rd/cozyrds/<name>.yaml`"
and "move or rename anything under `hack/`". The file is edited, not
renamed, and the new bats file is an addition; no make target changes
behaviour.
**website** would fire on a change to `ApplicationDefinition` semantics.
Semantics are unchanged — one definition's data is corrected — and
`content/en/docs/next/cozystack-api/application-definitions.md`
documents neither `resourceNames` nor the prefix rule, so there is
nothing there to bring in step.
### 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(harbor-rd): the ApplicationDefinition selectors for Harbor's Secret, Service and Ingress now carry the release prefix, so they match the objects the chart creates. No user-visible change — the Secret already reached tenants through a label-based workaround, and nothing consumes the Service and Ingress labels.
```
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
- **Bug Fixes**
- Updated Harbor resource naming patterns to consistently include the
`harbor-` prefix for credentials, services, and ingress resources.
- Improved alignment between Harbor resource names and their configured
release prefixes.
- **Tests**
- Added validation to detect missing release prefixes in secret,
service, and ingress resource selectors across system application
definitions.
- Validation reports all affected selectors and supports documented
exceptions where necessary.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
|
||
|
Aleksei Sviridkin
|
65c183e311 |
fix(e2e): keep backup preflight failure evidence across attempts
The backup access preflight printed its port-forward capture only when the port-forward never came up, and truncated its mc log at the top of every attempt. On the path that fails in practice, a healthy port-forward and an S3 round trip that never completes, the output was the tail of the last attempt with nothing to say which stage it died at, which reads as a successful upload followed by a hung download. Print the port-forward capture alongside the mc log on the timeout path, append the mc log across attempts, and record which of PUT, GET, compare or DELETE each attempt reached. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Andrei Kvapil
|
91112027a5 |
test(hack): let one selector opt out of the release-prefix guard
A repo-wide name check will eventually meet a selector that is correct and still fails it, such as an object named on the operator's terms. The only escape so far was an empty prefix, which exempts a whole definition, and a guard with no narrower way out is one people learn to route around. A trailing "cozy-lint:ignore rd-release-prefix -- <reason>" comment now exempts that one entry, and a directive without a reason does not count, so a bare suppression is never the easy path. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com> |
||
|
Aleksei Sviridkin
|
f9ea0da3de |
feat(backups): Kafka topic-metadata backup via a dedicated strategy driver (#4000)
## What this PR does Adds a dedicated `strategy.backups.cozystack.io/Kafka` Go driver that backs up the **topic metadata** of a Cozystack-managed `Kafka` application and wires it into the platform `cozy-default` flow — the counterpart to the app-specific drivers (CNPG, MariaDB, Altinity, MongoDB). It is a typed variant of the generic Job strategy: the controller renders a one-shot batch Job that talks to the Kafka Admin API, and adds on top an applicationRef Kind gate, a `Ready` precondition on the Strimzi `Kafka` cluster, and a per-run object key via `.BackupName` so distinct runs never overwrite each other. What it captures: every non-internal topic's partition count, replication factor and non-default configs (via `kafka-topics --describe`), stored as one small object per run in the system bucket; restore recreates them with `kafka-topics --create` + `kafka-configs --alter`. Deliberately **out of scope**: message payloads (a volume-snapshot strategy's job), consumer-group offsets (a log position, only meaningful with a byte-identical data restore), ACLs, quotas, and `KafkaUser`s. No PITR. Restored topics are created directly through the Admin API, so on the target they are unmanaged (no `KafkaTopic` CR) — the accepted tradeoff of an Admin-API-only driver. Both restore flows are implemented: **in-place** recreates dropped topics into the source, and **to-copy** applies the source's topics onto a freshly-bootstrapped empty `Kafka` (the "bootstrap a cluster, then apply the metadata state" flow). Contents: - Driver: `api/backups/strategy/v1alpha1/kafka_types.go`, `internal/backupcontroller/kafkastrategy_controller.go` (+ unit tests for its pure helpers), `internal/backupcontroller/kafkatypes/` (Strimzi `Kafka` readiness gate), dispatch cases in both controllers + `supportedBackupStrategyKinds()`, scheme registration, generated CRD + deepcopy. - Chart: `strategy-kafka-default.yaml` (gated on the bucket name), a `cozy-default` binding, read RBAC on `kafka.strimzi.io/kafkas`, and the Strimzi Kafka client image value. - Docs: a Kafka section in `docs/operations/backup-classes.md`. - Example + e2e: `examples/backups/kafka-metadata/` and `hack/e2e-chainsaw/kafka-metadata/` (`kafka-3-metadata-roundtrip`), an honest round-trip that proves a per-run `retention.ms` sentinel survives both restores. The original `packages/apps/kafka` chart is left untouched (it has no legacy backup mechanism). Validated: `go build ./...`, `go vet`, `go test ./internal/backupcontroller/...` (pure helpers plus a reconcile Kind-gate test and the RestoreJob cleanup regression test), `helm unittest` on the package (which now renders `strategy-kafka-default.yaml` and asserts its kind/name/`artifactURITemplate`), the `select-install`/`select-e2e` bats suites, `helm template` of the strategy CR, `bash -n` of the rendered script, and `make generate` (clean). The end-to-end round-trip (`kafka-3-metadata-roundtrip`) is enabled and mapped like the sibling backup suites, so CI builds this branch's controller and runs it live against a fresh sandbox; it has not been merged-and-run before, so that first live run must go green before merge. ### Screenshots N/A — no UI changes. ### Downstream repositories Walked the trigger map in `docs/agents/contributing.md` against the diff. This PR adds a new `strategy.backups.cozystack.io/Kafka` CRD, a Go driver, chart wiring, docs and examples. It does not add or rename a `packages/apps`/`packages/extra` package, does not change any `ApplicationDefinition`, `values.schema.json`, version enum, `release.prefix`, output Secret/Service, the `Package`/`Plan`/`RestoreJob` CRDs the provider types by hand, any `hack/*.mk` anchor or make-target behaviour, node prerequisites, labels/annotations, or telemetry metrics. A brand-new `strategy.backups.cozystack.io` CRD travels to the Terraform provider as opaque JSON (it types only `Plan`/`RestoreJob`/`Package`), so it keeps working untouched. None of the downstream triggers match. - [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 feat(backups): add a Kafka topic-metadata backup strategy bound by cozy-default — backs up topic definitions and configs (partitions, replication factor, non-default configs) via the Kafka Admin API, with in-place and to-copy restore; message data, consumer offsets, ACLs, quotas and KafkaUsers are out of scope ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added Kafka topic-metadata backup and restore support, excluding message data. - Supports ad-hoc and scheduled backups, in-place restoration, and restoration to a separate Kafka cluster. - Recreates missing topics and restores compatible configurations while validating partition and replication-factor constraints. - Waits for Kafka readiness and cleans up backup artifacts after completion. - Backup and restore jobs now enforce configurable deadlines to prevent stalled operations. - **Documentation** - Added guidance covering Kafka metadata backups, restoration limitations, and S3 configuration. - **Tests** - Added end-to-end coverage for backup, restoration, validation, and cleanup scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Andrey Kolkov
|
deb4916f94 |
test(backups): pin the restore-side Kafka --describe literal-topic guard
The restore path wraps the topic in \Q...\E on both the --describe and the --alter calls so a name with a regex-special character cannot match a colliding sibling. Only the --alter guard was pinned: the colliding pair in the fixture gave both topics the same partition count and replication factor, so stripping \Q on the --describe read the sibling's identical numbers and nothing downstream diverged. Give the sibling a distinct, larger partition count. A stripped --describe \Q now matches it first (glob order places desc.pay-events before desc.pay.events), reads 9 > the backup's 7, and aborts with "cannot decrease partitions" so no alter is recorded and the assertion reddens. Also point the suite's scratch file at a per-test path via METADATA_FILE so runs cannot read back a prior test's records. Signed-off-by: Andrey Kolkov <androndo@gmail.com> Assisted-by: LLM |
||
|
Andrei Kvapil
|
13549b5cfa |
test(hack): keep only what the code cannot say in the prefix guard header
The header retold both commit messages at several times their length. What stays is what nothing else records: why the check is "contains" rather than a prefix match, so tightening it does not break the nine selectors with their own leading words, and why an empty prefix is skipped. The latter now names the actual source of those definitions, since they live under packages/system and are generated from charts in packages/extra. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com> |
||
|
Andrei Kvapil
|
50f4834edc |
test(hack): run the release-prefix guard under cozytest.sh with yq
make unit-tests executes hack/*.bats through hack/cozytest.sh, which has no `run` helper, so the guard aborted before inspecting a single selector. It also depended on python3 and PyYAML, neither of which the build dependencies or the unit-test job provide. Walk the definitions with yq instead, as the neighbouring rd checks do, and fail when zero selectors were inspected so a broken glob or query cannot pass. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com> |
||
|
Aleksei Sviridkin
|
b3b280ea73 |
feat(gateway)!: add TLS-passthrough listeners to TenantGateway (#3342)
## What this PR does Phase 1 of external database exposure via Gateway API TLS-passthrough (#2815): adds a `tlsPassthroughListeners` field to `TenantGateway` and renders one layer-4 `mode: Passthrough` listener per entry, on a native database port with a per-engine subdomain hostname, alongside the Gateway's terminate listeners. No engine is wired: no TLSRoute, no certificate or CA changes. The existing layer-7 `tlsPassthroughServices` field gains a 62-entry count bound, an item length and a DNS-1123 item pattern, and a repeated entry now fails the reconcile with `Ready=False` instead of rendering the same listener twice. `spec.apex` gains a 253-character bound and nothing else: the CEL rules concatenate it, so the install-time cost estimator refuses the CRD without a declared bound. It gains no format pattern, and that is not a claim that a mixed-case host works. Such a host reaches the field verbatim from a namespace label, the renderer then composes listener hostnames Gateway API refuses, and that tenant has no Gateway either way; what a pattern would change is only that the write is refused, which fails this HelmRelease where the same fault costs the one Gateway. An apex no listener hostname can sit within is reported by the controller instead, and only when `tlsPassthroughListeners` is in use. The constraints and what they do to objects that already exist are described further down. The bound on an entry is what the schema can state on its own. An entry also becomes a listener name and a hostname built from the apex, so the controller checks both composed values against the 253 characters Gateway API allows and names whichever one overflows. Validation rejects duplicate listener names or ports, ports reserved by the platform Gateway (80 and 443), out-of-range ports, invalid DNS-1123 names, and hostnames outside the tenant apex (mirroring the Gateway hostname admission policy so an invalid entry cannot make the whole Gateway rejected). The SNI-overlap rule refuses two passthrough listeners that answer the same name on different ports. That is what [cilium#42898](https://github.com/cilium/cilium/issues/42898) reports: only one of the two routes ever works. It does not extend to a terminate listener and a passthrough listener sharing a name across ports. No failure is reported for that shape, and Cilium counts a terminate/passthrough pair as conflicting only when they share a port. The per-engine subdomain hostname scheme follows the phase-1 gate. The flat-hostname alternative (pending the Cilium listener-isolation fix targeting 1.20) is a later scheduling decision that does not change this API field. One shape differs from #2815 on purpose. The ticket has `hostname` optional with a `*.<name>.<apex>` default; here it is required. Deriving the SNI from the listener name hides the one value an operator has to get right, and the two drift apart the moment the naming scheme changes. Found while checking that pairing, unrelated to this field: a stock `dns01` gateway already renders `tls-api` (passthrough, 443) beside `https` (terminate, 443, `*.<apex>`), one port with overlapping SNI, which is the shape Cilium 1.19.6 marks `Conflicted`. That belongs to the Cilium bump and is recorded on #3343. Also named rather than fixed: the controller-side rules on this field stop the whole reconcile when they refuse a spec. `renderGateway` returns an error, so the Gateway is not updated at all, and the tenant sees `Ready=False` with `ReconcileError` while new app listeners and every certificate the controller would issue or amend wait behind it. Certificates already issued go on renewing, since cert-manager drives that from the objects already in the cluster and an aborted reconcile writes nothing to them. Dropping only the conflicting listener and reporting it on status would be the better shape, but that changes the error model of the whole renderer rather than one rule, so it is out of scope here. The current behaviour is at least observable: `Ready=False` is a signal, not silent drift. On HTTP-01, a hostname a passthrough listener answers earns no HTTPS-terminate listener once a TLSRoute claiming that hostname can be served on the listener, matched by SNI overlap so a wildcard entry reaches a name beneath it as readily as the name written in the spec, and the field itself is accepted under certMode http01 alone. That refusal is stated as the set of modes that can carry a passthrough listener rather than the set that cannot, because a mode added to the enum would otherwise be admitted by default: dns01 and existingSecret serve the whole apex from one wildcard terminate listener the pinned Cilium cannot keep apart from a passthrough listener beneath it, and edge ends TLS at the class provider and renders no TLS listener for one to sit beside. The route half of that condition is load-bearing rather than incidental. On the pinned v1.19.5, tlsPassthroughFilterChains walks a listener's routes and skips one with no backends, so a declared passthrough listener nothing attaches to contributes no filter chain and matches no ClientHello, while the terminate listener's chain is built from its own Secret and carries the name alone. Keying the withdrawal on the declaration would therefore take a name that is being served and hand it to a listener that forwards nowhere, and that is the ordinary shape rather than a corner: the chart puts api, vm-exportproxy and cdi-uploadproxy on every tenant Gateway, while there is one platform TLSRoute per name cluster-wide and each names one tenant's Gateway as its parent, so every other tenant renders all three entries with a route on none. The backend half is that same rule reached by a different road: the chain is built per route out of the backends that survived resolution, so a route that attached and forwards nowhere puts no chain on the name either, a backendRef naming a Service that does not exist yet because manifests applied out of order, one deleted since, a name with a typo, or one in another namespace with no ReferenceGrant, and without that half an ordinary backend mistake takes a live endpoint offline while Cilium builds nothing to replace it. Resolution asks what v1.19.5 asks and no more: the reference allowed, the Service present, and neither its ports nor its endpoints read, because the translator reads neither. Services and ReferenceGrants are watched alongside routes, so both halves are deferrals rather than exemptions and the withdrawal lands on the pass that sees the missing piece arrive. The withdrawal reaches HTTP-01 alone, because `collectHostnameClaims` returns nothing under the other two modes and they render no per-hostname listener to withdraw. A TLSRoute that declares no `spec.hostnames` claims the hostname of each rendered passthrough listener its parentRef selects, because that is the name Cilium serves it on: `ComputeHosts` at v1.19.5 substitutes the listener hostname for a route that names none, so such a route takes the name over exactly as one that spells it out does. The tenant-namespace hostname policy requires a route there to name its hostnames, so this shape reaches a Gateway from the system namespaces where the platform routes live, or from an administrator, not from a tenant chart. A claim broader than the listener's counts the same way from the other side: a TLSRoute claiming `*.<apex>` is served on `api.<apex>` rather than on the wildcard, because `ComputeHosts` returns the listener's own hostname for a route hostname covering it, so a claim covering a reserved name is judged as a claim on that name while the route keeps one entry under the key its owner wrote. Both restrictions are tied to the Cilium pin, currently v1.19.5. That version translates the Gateway into a single Envoy listener whose filter chains match on SNI without the port, so a terminate chain and a passthrough chain carrying one name collide regardless of which Gateway port declared them; upstream states the consequence above NeedsCrossProtocolSplit in operator/pkg/model/model.go as of v1.19.6, the release that starts splitting Envoy listeners per port when NeedsPerPortListeners holds. That is a disjunction rather than a blanket change, and the passthrough side of every one of its three clauses is counted from attached routes, so a listener with no TLSRoute behind it contributes to none of them: the split arrives with the route, not with the bump. The same single-listener translation has a consequence that is not a restriction: the declared port is where the listener is published, not a bound on reach, because the backend answers its hostname's SNI on every port the Gateway exposes. Firewalling a tenant address down to 80 and 443 does not put a database out of reach; the hostname is the access surface, and the field's own doc and the chart README now say so. Both restrictions are validation-only, and they lift together on the pin plus a configuration the split actually covers rather than on the bump alone. A route whose hostname is withdrawn this way is told so under this controller's own name: Accepted=False with `NoMatchingListenerHostname`, and a message naming the passthrough hostname that answers it, which for a wildcard entry the route cannot derive on its own. It matters most there. A wildcard entry can withdraw the listener from several published names beneath it at once, each one a TLSRoute claims and can be served on, and the certificates are garbage-collected with them, and a rollback is cheaper than that sounds: cert-manager here runs with `enableCertificateOwnerRef: false`, so deleting a Certificate leaves its Secret behind and a re-created Certificate adopts a still-valid one rather than placing an order. Before this the only signal was Cilium's own RouteParentStatus entry under a different controllerName. A TLSRoute whose hostname no passthrough listener answers is told so as well, on the same condition and for the same reason: before this change that hostname got a terminate listener Cilium refused with ResolvedRefs=False, so withdrawing the listener would otherwise have removed the only object saying the route went nowhere. Two things stay named rather than fixed. A `tlsPassthroughListeners` hostname of `*.<apex>` is refused only indirectly: the overlap rule rejects it while `tlsPassthroughServices` carries its defaults, because the wildcard covers api.<apex>, so emptying that list is enough to let it through, and the terminate listener then goes from every published name a TLSRoute claims under the wildcard. The README states this; refusing the apex and its wildcard on their own terms is a one-line rule worth adding when the field is wired up to chart values. A wildcard hostname on an attached HTTPRoute used to render a listener name the shipped Gateway CRD refuses, `https-*-<hash>`, which failed the whole Gateway write while the README said the shape worked. Listener and Certificate names now spell a leading `*` as `wildcard`, with the hash still covering the whole hostname, and a test reads both the listener-name and the hostname patterns off the shipped CRD bundle and renders wildcards through them; every name that was already admissible is pinned byte for byte, so nothing is renamed on upgrade. Under http01 an HTTPRoute wildcard claim is refused before the race and the overlap are read, with `NoMatchingListenerHostname`, because the HTTP-01 challenge issues no wildcard certificate and a terminate listener could never have carried the name. A route status write that fails no longer leaves the route carrying its previous condition: `updateRouteStatuses` and the whole-apex pass collect every failed write and return them after the loop, so the reconcile fails, the TenantGateway reports `Ready=False` with `ReconcileError` and controller-runtime requeues, while the loop still runs to the end so one route's conflict does not hold the others back. Conditions this controller wrote under http01 are retracted when the tenant moves to dns01, existingSecret or edge, for HTTPRoutes and hostname-less TLSRoutes alike, so a refusal does not outlive the mode that produced it. And a TLSRoute claiming a wildcard passthrough entry itself leaves the names beneath the wildcard alone, since Envoy ranks an exact server name above the wildcard and nothing collides. One thing this does not cover yet: Accepted is computed from the hostname race and the withdrawals above, so a route is still told it is fine when an HTTPRoute holds the name and the TLSRoute claiming it cannot attach to the terminate listener that answers it. That one predates this change. Two neighbours of it no longer fall in the gap. A `parentRef` port that names no listener answering the hostname is compared alongside the `sectionName`, since Gateway API requires every key a route pins itself with to match the listener it selects. An HTTPRoute's `parentRef.port` is judged the same way: the terminate listener sits on 443, so any other pinned port earns no listener and no certificate through that parentRef and reads the port-mismatch cause; port 80, or `sectionName: http`, from a namespace the plain-HTTP listener admits (the publishing tenant and cozy-cert-manager) is served plain with no terminate listener and no refusal, from any other namespace it is refused with `NotAllowedByListeners`, and a `sectionName` and a port that select different listeners select none and read `NoMatchingParent`. A plain-served route stays in the hostname race on the same footing as a terminating one, in every recount as well as the first count. And a `sectionName` naming a `tls-<name>` listener this Gateway does not render now reports `Accepted=False` with `NoMatchingParent`, Gateway API's reason for a parentRef matching no listener: the controller renders every listener in that namespace, so an absent name there is a fact rather than a guess, while a `sectionName` naming the plain-HTTP listener is judged like port 80 below, and any other name outside the prefix is written Accepted like an attached route, with the class controller's own status entry answering for the listener it named. The namespace case is off that list. Native-port listeners admit the publishing tenant alone, and a TLSRoute elsewhere on one of their hostnames is refused with `NotAllowedByListeners`. An HTTPRoute that pins itself by `sectionName` to a rendered `tls-<name>` passthrough listener is refused with the same reason: that listener forwards the stream it matched by SNI and terminates nothing, so it takes TLSRoute alone by protocol and cannot serve the route whatever answers its hostname. Its claim also leaves the hostname race before ownership is ranked, so the TLSRoute that can be served is not told it lost to a route that could never attach. Cilium does not report this refusal at the pin. The port-443 passthrough listeners name HTTPRoute in `allowedRoutes.kinds` to keep the port-443 set uniform (cilium#45559), the Gateway reports that entry back as `ResolvedRefs=False/InvalidRouteKinds` on the listener, and Cilium's route-side kind check reads a listener's kinds against every route on the Gateway rather than against the routes that named it, so it reports the route Accepted and the condition this controller writes is the only refusal the route gets. A hostname whose every claimant is refused this way earns no HTTPS-terminate listener and no certificate, where before it earned both. The refusal is decided per route rather than per hostname, which matters once a claim overlaps more than one reserved name: a wildcard covering both a port-443 service hostname and a native-port one is attachable through the first and refused through the second, so the answer comes from the listener the `parentRef` names, its own hostname and its own attach set. The row that pinned a `sectionName` naming no rendered listener as a known gap now pins the verdict instead. On the port-443 listeners the two namespace sets agree by construction, since `collectHostnameClaims` and the listeners' allowedRoutes selector run off the same list. And the justification for the pin is written out at every site that depends on it rather than once, so the day the pin moves they all have to move together and the one that does not is a false statement no diff can catch. That is not hypothetical here: three claims of exactly that shape were wrong in this branch and were caught by reading the source they described rather than by any diff, so the volume of restated reasoning is itself what produces them. One canonical block with references to it would carry the same knowledge with one place to update, and rewriting the narrative under review would be a documentation change rather than a fix, so it is tracked on #3842 instead. The gateway chainsaw suite now covers the withdrawal, which it did not before: a tenant owning its own Gateway renders the shipped passthrough entries with nothing routed to them, an HTTPRoute on api.<apex> keeps its terminate listener, its certificate and Accepted=True there, and attaching a TLSRoute to the same listener, pointed at a Service in the tenant's own namespace, since a route that forwards nowhere is the case the first phase already measures, withdraws all three and moves the route to NoMatchingListenerHostname. A third phase patches in a native-port entry and reads the per-listener readiness flag, which is the claim no test against a fake client can settle. The first two phases are one measurement: withdrawing on demand is what dates the earlier assertion to settled state rather than to a withdrawal that had not arrived yet. The `Require API owner review for sizeable API changes` check is red on purpose and does not clear with a code change. A new field plus tightened validation on already-shipped fields counts as a sizeable API change, so `.github/workflows/api-review-gate.yaml` holds the PR until an API owner approves it. The gate counts each owner's latest review on the current head commit, so the approval has to land on the head as pushed and any later push invalidates it; the job re-runs on review submission, so no push is needed to flip it. Reading that red as "not ready" is the mistake this paragraph exists to prevent. Part of #2815. ### Release note ```release-note feat(gateway): TenantGateway supports layer-4 TLS-passthrough listeners on native database ports for SNI-based external exposure. On HTTP-01 gateways a hostname a TLS-passthrough listener answers, including any name beneath a wildcard entry, no longer earns an HTTPS-terminate listener or a Gateway-issued certificate once a TLSRoute claiming that hostname can be served on the listener and forwards to a backend that resolves; a hostname claimed only by a TLSRoute stops earning one too. A TLSRoute with no spec.hostnames counts as claiming the hostname of each passthrough listener its parentRef selects, and one whose hostname covers a listener's counts as claiming that listener's name, both being the name Cilium serves it on. A declared passthrough listener with no route on it changes nothing, because it emits no filter chain and the terminate listener is what answers the name, and neither does a route whose backendRefs resolve to nothing, a Service that does not exist, or one in another namespace with no ReferenceGrant, since such a route emits no filter chain either. The controller watches Services and ReferenceGrants, so the withdrawal it defers lands once the backend appears. So the tenant whose Gateway the platform TLSRoutes name, tenant-root unless the cluster's expose-ingress says otherwise, sheds the terminate listeners and certificates for api, vm-exportproxy and cdi-uploadproxy on upgrade (those TLSRoutes ship with the cozystack-api, kubevirt and kubevirt-cdi packages, each rendered only where the Gateway integration is enabled and the service is among the platform's exposed services), and a tenant that renders the same three entries with no route on them keeps what it has. The shed certificates leave their TLS Secrets behind: cert-manager here ships enableCertificateOwnerRef: false, so each Secret stays in the namespace with no owner, nothing renewing it and nothing deleting it. That cuts both ways, as stale Secrets to remove by hand and as a cheap way back, since a re-created Certificate adopts a still-valid Secret instead of placing an order. A route the controller renders no listener it can attach to now carries Accepted=False from this controller instead of Accepted=True. Five shapes report NoMatchingListenerHostname and the message tells them apart: the HTTPRoute whose hostname a passthrough listener answers, the TLSRoute with no passthrough listener behind it, the route whose parentRef names a listener that answers a different hostname, the route whose parentRef names a port no listener answering its hostname is published on, and the HTTPRoute claiming a wildcard hostname under http01, which the HTTP-01 challenge cannot certify. A route pinned by sectionName to a tls-<name> listener the Gateway does not render carries NoMatchingParent instead, which is what Gateway API calls a parentRef matching no listener, and so does an HTTPRoute whose sectionName and parentRef port select different listeners. A TLSRoute outside the publishing tenant's namespace claiming a native-port hostname carries NotAllowedByListeners instead, since those listeners admit the tenant alone. So does an HTTPRoute pinned by sectionName to a rendered tls-<name> passthrough listener, which cannot serve its kind, and an HTTPRoute selecting the plain-HTTP listener from a namespace it does not admit; a refused route also stops counting in the hostname race, so a hostname whose every claimant is refused earns no terminate listener and no certificate. Listener and Certificate names derived from a wildcard hostname are admissible now, names that were already admissible do not change, a failed route-status write fails the reconcile instead of being logged away, and conditions written under http01 are retracted when certMode leaves it. The dns01 and existingSecret cert modes shed nothing, because neither renders per-hostname listeners at all, and tlsPassthroughListeners is accepted under http01 alone: those two serve the whole apex from one wildcard terminate listener, and edge terminates TLS upstream and renders no TLS listener a passthrough entry could sit beside. The withdrawal does not take a working path away: on the pinned Cilium both port-443 filter chains already carry these hostnames and nothing on the objects says which one answers a connection, so a race that was previously undetermined and unobservable now resolves the same way every time. Treat this as a change of serving path rather than the removal of something idle: today each of those three hostnames has a terminate listener and a passthrough listener on port 443, both filter chains carry the SNI, and nothing on the objects says which one answers a connection. Gateway API calls for both listeners to report Conflicted; the pinned v1.19.5 has no Conflicted condition at all, so there is nothing to read, and you cannot tell from the cluster whether api.<apex> is terminated by the Gateway or passed through. Afterwards it is passed through, always. Read the Gateway's listener conditions before upgrading to see what those listeners were serving. Before upgrading, confirm the management apiserver serving certificate carries api.<apex> in its SANs, because once the ACME certificate is withdrawn that is the only certificate left on the name; a cluster that came from the ingress-nginx path already meets that, a cluster served by the Gateway's ACME certificate since it moved does not necessarily and loses the name until the SAN is added. If something depends on the terminate path for one of those names, the package README lists three ways forward and what each costs. Moving the consumer to another hostname under the apex is the only one that gives up nothing. ``` ### Downstream repositories Walked the trigger map in `docs/agents/contributing.md` against this diff: `api/gateway/v1alpha1/{tenantgateway_types.go,zz_generated.deepcopy.go}`, `internal/controller/tenantgateway/{reconciler,renderers,conflict,mapper,status,backends,reconciler_test,crdcel_test,mapper_test,backends_test}.go`, `internal/controller/tenantgateway/rbac_test.go`, `cmd/cozystack-controller/main.go`, `hack/e2e-chainsaw/gateway/chainsaw-test.yaml`, `packages/system/cozystack-controller/definitions/gateway.cozystack.io_tenantgateways.yaml`, `packages/system/cozystack-controller/templates/rbac.yaml`, `packages/core/platform/values.yaml`, and `packages/extra/gateway/README.md`. No trigger fires, and the two repositories worth ruling out explicitly are below rather than waved off. `cozystack/website`: no package under `packages/apps/` or `packages/extra/` is added, renamed or removed. `packages/extra/gateway/README.md` is edited in place. The docs generator regenerates a reference page from a package README only for the apps hardcoded in the website `Makefile`, and `gateway` is in none of `APPS`, `K8S`, `VMS`, `NETWORKING` or `SERVICES`, so there is no generated page for this package to drift out of step with. That gap is the same shape the trigger map already records for `packages/apps/opensearch` and `packages/apps/bucket`; it predates this change and is neither created nor widened here. None of the other website triggers apply: `packages/core/platform/values.yaml` changes only in a comment, which now says that api, vm-exportproxy and cdi-uploadproxy render a TLSRoute rather than an HTTPRoute, with no key added, moved or given a new default, no platform variant or component moves, no release asset is renamed, `ApplicationDefinition` semantics are unchanged, there is no Talos bump and no developer-tooling change. `cozystack/terraform-provider-cozystack`: this adds an API field, so the field-level triggers are the ones to check, and they are keyed to app charts rather than to this CRD. No app is added under `packages/apps/`; no `values.schema.json` gains, loses or renames a field; no version enum, `values.yaml` default, `kind`, `plural`, `release.prefix` or output Secret/Service name changes. `TenantGateway` is not among the CRDs the provider types by hand. Those are `Package` under `cozystack.io` and `Plan`/`RestoreJob` under `backups.cozystack.io`, and the provider carries no reference to `gateway.cozystack.io` at all. It therefore falls under the map's last bullet: the spec travels as opaque JSON, so the provider keeps working and simply offers no typed access to `tlsPassthroughListeners`, which is the position it already holds for every other field it does not model, and not a break. The provider's new-top-level-field test guard is scoped to `api/apps/v1alpha1`, which this diff does not touch. `cozystack/ansible-cozystack`: nothing in reach. `packages/core/installer/values.yaml` and the `cozy-system`/PSA contract are untouched, no platform variant is added or renamed, none of the `packages/core/platform/values.yaml` keys the role sets are involved (the file changes in a comment alone), no object the role waits on is renamed, and `hack/e2e-prepare-cluster.bats` node prerequisites are unchanged. The schema constraints this diff adds to already-shipped fields all reach objects that already exist, and every one of them is a kind the apiserver ratchets. `spec.apex` gains a 253-character limit (no pattern: a mixed-case apex is admitted and refused by the controller only when passthrough listeners are declared), and `spec.tlsPassthroughServices` gains a DNS-1123 item pattern and a length bound. A stored value that violates one still passes any write that leaves it unchanged, so the rule bites only once the value itself changes. A repeated `tlsPassthroughServices` entry is reported by the controller as `Ready=False` rather than refused at admission, and the reason is not the one this section used to give. Ratcheting does reach a list-type error: `customresource.ValidateUpdate` validates an incoming object's list-type semantics only when the stored object passes them, so a stored duplicate is ratcheted through rather than blocking every write. What makes the marker the wrong tool is the grain of that ratchet, which is the whole object rather than the field, so one stored duplicate would switch list-type checking off for every list on the `TenantGateway` at once and say nothing about it. Documented in `packages/extra/gateway/README.md`, and pinned by `TestTightenedConstraintsOnExistingObjects` together with `TestListTypeSetWouldNotRefuseAStoredDuplicate`, which compiles a copy of the shipped schema carrying the marker to show what the apiserver actually does with one. Shipped defaults conform and the chart already fails on an empty host, so this is a no-op in practice. - [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: <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added layer-4 TLS passthrough listeners (`tlsPassthroughListeners`) with native port + SNI hostname, rendered as passthrough listeners and limited to TLSRoute attachment. * **Bug Fixes** * Added stricter admission-time and controller validation (reserved ports 80/443, distinct listener ports, tenant-apex hostname rules, and collision prevention with passthrough services). * Improved listener rendering safety: prevents exceeding the Gateway listener cap and ensures AllowedRoutes are isolated per listener. * **Documentation** * Updated gateway docs with listener-cap considerations and layer-4 passthrough usage details. * **Tests** * Added CRD/CEL and reconciliation coverage for valid/invalid configurations and rendering behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
cecb4b11d3 |
feat(opensearch)!: issue HTTP and Dashboards certificates from cert-manager (#2682)
## What this PR does The OpenSearch HTTP API (9200) and Dashboards (5601) already served TLS, from the operator's own private CA and with a SAN built from `cluster.local` while the credentials Secret advertised the release on the platform domain, so nothing a client was handed could verify. Both certificates now come from a per-release cert-manager CA in the tenant namespace, and the CA reaches tenants as a key-free trust anchor. - `tls.issuer` selects who issues the HTTP server certificate, `cert-manager` or `operator`, and is named for the issuer because TLS is served under both. Unset resolves from `external` (cert-manager for externally published services, the operator for cluster-internal); an explicit value always wins. - Renders a self-contained cert-manager chain in the tenant namespace: a self-signed Issuer, a CA Certificate, a CA Issuer, and two leaf Certificates, one for the HTTP API and one for Dashboards, each with the SANs of its own Service and the external hostname when `external: true`. Leaf keys are written as PKCS8, since the JVM loader rejects the PKCS1/SEC1 encoding cert-manager emits by default, and the CA subject is pinned so the operator's signature check accepts the chain. - The CA Secret is `<release>.http-ca`. The dot separates the chart-chosen suffix from release names, so no other release can produce that name; the two leaf Secrets stay hyphenated because none of the chart's suffixes is a tail of another. The operator's own transport CA remains at `<release>-ca` and is not touched. - Transport mTLS (9300) stays operator-managed (`transport.generate: true`, `perNode: true`). The security plugin requires it and the chart cannot turn it off. - `hack/e2e-chainsaw/opensearch/` runs the move on a cluster: it installs the app on the operator's HTTP listener, removes `tls.issuer` so it resolves from `external`, and asserts the cert-manager chain, the CR dropping `http.generate`, the admin certificate reissued under the chart CA, the StatefulSet rolling onto the three subPath mounts, the tenant anchor, and the certificate the endpoint actually presents to a client holding that anchor. ### Sysctl carrier and single-node rolls The E2E suite for this PR was red because the chart set `spec.general.setVMMaxMapCount: true`, which makes the operator prepend a privileged `init-sysctl` container to every OpenSearch pod. Tenant namespaces run at the cluster's default PodSecurity level, baseline on Talos, so admission refused the container and the StatefulSet never produced a pod (#4131). The carrier for the sysctl is now on main: #4152 added a DaemonSet to the `opensearch-operator` package that raises `vm.max_map_count` to a floor of 262144 on every node, from a namespace the platform labels privileged. This branch is rebased onto that main and sets the flag to an explicit `false`, with a template comment saying why and a unit test pinning it, so a release no longer asks the operator for a container admission refuses. The value stays with the DaemonSet until the node contract carries it (#4144); the DaemonSet's own template comment, which said the chart still asks, now says it does not. Three more things came out of the same pass. The chart asked the operator to drain data nodes before every rolling restart, and with a single data node the drain never finishes: at v2.8.0 `PreparePodForDelete` excludes the node from allocation and then waits for it to hold no shards, which the only data node can never satisfy, so a one-replica release could not roll for a version bump or for the certificate move this PR relies on. The chart now drains only when a second data node exists, pinned both ways in the unit tests. The chart maps an empty `storageClass` to `replicated` at one replica, and the PR lane runs LINSTOR without DRBD and creates only `local`, so the E2E fixture takes its class from the `storageClass` value the harness passes with `--set-string` from `COZY_E2E_STORAGE_CLASS`, the way the vminstance fixtures do; on the nightly QEMU lane that still resolves to `replicated`. And the external Dashboards Service selected labels no pod carries. `pkg/builders/dashboards.go` at v2.8.0 stamps `opensearch.cluster.dashboards: <cluster name>` on the Deployment, its selector and the pod template, and nothing else; the chart asked for `opster.io/opensearch-cluster` together with `app.kubernetes.io/component: dashboards`, so the LoadBalancer came up with no endpoints and no error to show for it. It selects the label the operator sets now. ### securityadmin cross-trust, still blocked on the DNS base Chart-managed HTTP TLS previously broke the operator's securityadmin reconciliation: the admin certificate was signed by the transport CA while the HTTP listener trusted the cert-manager CA, so the two never verified against each other and users and roles silently never applied. This chart puts one CA under both sides, handing the HTTP CA to the operator through `spec.security.tls.http.caSecret` so that it signs the admin certificate with the CA the listener trusts. That closes the CA mismatch. The other half, the job resolving a name the operator built from `cluster.local` on a cluster that does not use it, was closed separately and is on main, so the security configuration applies once this lands. No operator bump is required; the API exists in the pinned version. That path is version-gated upstream at OpenSearch 2.0.0: below it the operator falls back to the transport CA and the mismatch returns with no error anywhere. The chart therefore refuses chart-managed HTTP TLS on `version: v1` with an explicit render-time failure that names the mechanism and offers the alternatives, rather than letting it fail silently at runtime. ### Trust anchor for tenants The HTTP server's private key is not exposed to tenants. The chart renders a `TenantProjection` sentinel that names `<release>.http-ca` as its source; the CA-extraction controller publishes a `ca.crt`-only copy as `<release>.tenant-ca`, and the ResourceDefinition selects it with the engine-agnostic `internal.cozystack.io/tenant-ca` label. The sentinel is gated on the same condition as the chain, so a plaintext release renders no sentinel instead of one stuck at `SourceNotFound`. Tenants get what they need to verify the server and nothing more. ## Known limitations These are stated rather than left to be discovered: 1. **securityadmin needed both trust and reachability, and both are closed now.** This branch put one CA under the admin certificate and the HTTP listener. The other half was the operator resolving `<svc>.<ns>.svc.<DNS_BASE>` with `DNS_BASE` left at the vendored `cluster.local`, so the job waited on a name that never resolved whichever CA had signed; the platform now renders that value from `networking.clusterDomain` and it is on main, which is what unblocks this branch. 2. **The e2e suite reaches the move, not the chart bump that triggers it.** The suite above arrives at the chart-managed chain by removing `tls.issuer` from a running release, which leaves exactly the values an upgraded release arrives with. The one step it cannot run is the one where the previous chart version wrote the CR, because there is no second chart version to install from in this tree. Everything downstream of that write is exercised on a cluster. 3. **A reissue does not reach a running pod.** Neither service reloads its certificate, and cert-manager reissues on any change to `external`, the tenant host or the cluster domain, so an ordinary edit leaves the pods serving material that no longer matches their own names with no error, event or condition to show for it. Completing such a change is manual. So is removing the CA private key cert-manager leaves behind when TLS is turned off again, since the platform runs with `enableCertificateOwnerRef: false` and deleting a `Certificate` does not delete its `Secret`. `packages/apps/opensearch/README.md` carries both, with the commands. ### Downstream repositories `packages/apps/opensearch/values.schema.json` gains the `tls` object, so `cozystack/terraform-provider-cozystack` needs the field modelled for `cozystack_opensearch`; tracked in cozystack/terraform-provider-cozystack#41. ### Upgrade impact An existing `external: true` release with `tls.issuer` unset serves operator-issued HTTP TLS today and moves to chart-managed TLS on its first reconcile after this lands: the cluster rolls once and is re-anchored on a new per-release CA, so any client pinned to the operator's CA has to be repointed at the key-free `<release>.tenant-ca`. Setting `tls.issuer: operator` before the upgrade keeps that half as it is. Releases that are not external are untouched. Every existing release rolls once regardless of `tls.issuer` and `external`, because the pod template loses the operator's `init-sysctl` container; on a single-replica release that roll is the first one the operator can complete, since draining the only data node used to block every rolling restart. The published external-dns name changes on the same upgrade whatever `tls.issuer` says, because the Services are gated on `external` alone: they move from `<release>.<namespace>.<cluster-domain>`, an in-cluster name that never resolved outside the cluster, to `<release>.<tenant-host>` and `<release>-dashboards.<tenant-host>`. external-dns is configured upsert-only here, so the old record is not withdrawn and keeps pointing at the LoadBalancer until it is removed from the zone by hand. That cleanup is per zone rather than per release and nothing enumerates the records, so the README shows how to derive the list from the cluster. ### Release note ```release-note feat(opensearch)!: the OpenSearch HTTP API and Dashboards already served TLS, from the operator's own private CA and with a SAN built from cluster.local while the credentials Secret advertised the release on the platform domain; both certificates now come from a per-release cert-manager CA whose anchor the platform publishes into the tenant as <release>.tenant-ca. Any client that worked against the old endpoint was skipping verification, because no hostname it was handed could have matched that SAN. Existing external releases move from the operator's CA to the chart's on their first reconcile, and their published external-dns name moves from the in-cluster domain to <release>.<tenant-host> with the old record left in place; set tls.issuer: operator beforehand to opt out of the CA move. With dashboards.enabled the application name is limited to 41 characters, and to 32 when external is on as well; past either bound the chart fails the render and withholds every object in the release. A release already above a bound is not broken by the guard, it is told why it has not been upgrading: the Dashboards Service the operator builds from that name exceeds the 63-character limit for a DNS label, the operator returns that failure on every pass, and the reconcilers queued behind Dashboards, which are upgrade and restart and snapshot repository, have not run since the release was created. Set dashboards.enabled to false to drop the bound. The external Dashboards Service also selected labels the operator does not put on Dashboards pods, so it stood up with no endpoints; it selects opensearch.cluster.dashboards now. The drain of data nodes is now requested only with the data role on and more than two replicas, since at two the operator gives up on a full drain itself. The chart stops asking the operator for its privileged init-sysctl container, since the opensearch-operator package now sets vm.max_map_count from a DaemonSet, so every existing release rolls once on upgrade; single-replica releases no longer ask the operator to drain their only data node, which used to block every rolling restart ``` |
||
|
scooby87
|
080c649da0 |
fix(hack): avoid a SIGPIPE false-negative in the resourcesPreset check (#4204)
## Summary `hack/check-rd-presets.sh` intermittently fails the "Unit & controller tests" CI job with a false `resourcesPreset enum missing: <preset>` for a preset that is actually present — most visibly on `http-cache-rd`, and on PRs that have nothing to do with that package. ## Root cause The check tests each expected preset with `printf '%s\n' "$enums" | grep -Fqx -- "$want"`. When grep finds the preset it exits immediately and closes the pipe, so `printf` is killed by SIGPIPE (exit 141) while it still has data to write. The script runs under `set -o pipefail`, so that 141 becomes the pipeline's status and `if ! <pipeline>` records a present preset as missing. It is a race with the pipe buffer, so it fires intermittently and mostly on the larger enums (`http-cache-rd`'s schema carries two 47-entry enums). The CI log shows the tell: `check-rd-presets.sh: line 39: printf: write error: Broken pipe` immediately followed by `FAIL: ... resourcesPreset enum missing: s1.micro`. ## Fix Feed grep from a here-string instead of a pipe — no second process, no SIGPIPE, no pipefail interaction. Deterministic reproduction (target at the front of a 100k-line list, under `set -euo pipefail`): the `printf | grep -Fqx` form reports the target as missing; the here-string form finds it. After the change `bash hack/check-rd-presets.sh` passes green. ### Release note ```release-note NONE ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved preset validation to prevent false errors when configured presets are present. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
19adffb515 |
test(opensearch): exercise the TLS rollout on a cluster
No suite created an OpenSearchCluster anywhere in the tree, so the whole class of failure where the chart renders correctly and the cluster then refuses the workload had no coverage at all. Rendering proves the manifests; only a cluster proves that securityadmin authenticates against the CA the chart handed the operator and that the security configuration actually applies. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
f3c28e91c4 |
test(opensearch): pin the trust anchor, the name guard and the upgrade
Most of what this chart decides about TLS is invisible in a rendered manifest until a cluster disagrees with it. The upgrade case is the sharpest: a release that already runs operator-managed HTTP TLS must keep working when the chart takes the CA over, and nothing about the rendered output says whether it will. The negative assertions carry as much as the positive ones. The trust anchor must stay out of the resource map, since a name grant there would outlive the platform's verdict on it, and a null tls value must leave the chart rendering rather than aborting the whole release. Both are properties that fail silently and only a test states them. The bats guard covers what helm-unittest cannot see: which Secrets the opensearch-rd definition exposes, so a key-bearing one cannot be added to that surface without the guard going red. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Arsolitt
|
85fa60a33f |
feat(opensearch)!: issue the HTTP API certificate from cert-manager
The HTTP listener and Dashboards ran on certificates the operator minted itself, so a client outside the cluster had no anchor it could obtain through the platform and no way to verify the endpoint it was handed. Issue the HTTP chain from cert-manager instead, and give the operator the same CA through spec.security.tls.http.caSecret so it signs the securityadmin admin certificate with it. Without that the admin certificate would keep the operator's own CA while the listener trusted cert-manager's, and securityadmin could no longer authenticate to apply users, roles and audit policy. tls.issuer names the authority rather than a switch, because the listener serves TLS under either value and only the signer changes. It is tri-state: an explicit value is obeyed, an absent one follows external, because a release nobody publishes has no third party to prove anything to. The cert-manager issuer needs OpenSearch 2.0.0 or newer, since the operator honours the HTTP CA only from that version and silently signs with its transport CA below it. An explicit choice under that floor fails the render and one resolved from external falls back to the operator, so asking for a verifiable certificate never quietly yields less than was asked, and turning on external access never breaks an upgrade. BREAKING CHANGE: an externally published release on OpenSearch 2.0.0 or newer serves the HTTP API from a cert-manager CA rather than the operator's own. A client pinned to the operator CA must move to the trust anchor the platform projects into the tenant. BREAKING CHANGE: the external-dns name of an externally published release moves from <release>.<namespace>.<cluster-domain> to <release>.<tenant-host>, and the Dashboards name with it. This half is gated on external alone and does not depend on the issuer. external-dns runs upsert-only here, so the old record stays in the zone until it is removed by hand. BREAKING CHANGE: with dashboards enabled the application name is limited to 41 characters, and to 32 when external access is also on. A release above either limit now fails to render rather than converging. Such a release was already stalled: the Dashboards Service built from its name exceeds the 63-character DNS label limit, and the operator returns that failure ahead of its upgrade, restart and snapshot-repository reconcilers, so it has not upgraded or rolled since it was created. Set dashboards.enabled to false to converge again, or recreate the application under a shorter name. Assisted-by: LLM Signed-off-by: Arsolitt <arsolitt@gmail.com> [lexfrei: reworded subject and body for the issuer field rename] Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Andrey Kolkov
|
c3d1015df0 |
test(backups): pin the non-numeric recorded replication factor guard
The Kafka restore path rejects a recorded replication factor that is not numeric, but only the partition-count guard had its own test. Dropping both guards reddened that test, so the pair looked covered, while dropping the replication-factor guard alone left the suite green. Add a case that drives the create path (topic absent) with a valid recorded partition count and a non-numeric replication factor, so the guard is pinned on its own and a future edit that removes it fails loudly. Signed-off-by: Andrey Kolkov <androndo@gmail.com> Assisted-by: LLM |
||
|
Andrey Kolkov
|
c5a8a39bcd |
docs(backups): name the demo BackupClass in the Kafka flow headers
The run-all.sh flow header and the chainsaw harness header both said the ad-hoc BackupJob runs against the platform cozy-default BackupClass. It routes to this demo's kafka-metadata BackupClass instead, which maps Kafka to the kafka-strategy-default strategy that run-all.sh derives from the shipped cozy-default-kafka at run time. The numbered manifests already described this; only the two summary headers overstated the one piece of wiring the demo exists to exercise. Reword them to match. Signed-off-by: Andrey Kolkov <androndo@gmail.com> Assisted-by: LLM |
||
|
Andrey Kolkov
|
2cb2b459fa |
fix(backups): reject a non-numeric recorded partition count on Kafka restore
The restore path validated only the shape parsed from the live cluster,
not the partition count and replication factor read from the stored
object. A non-numeric recorded partition count made `[ "${lp}" -lt
"${parts}" ]` and its `elif` exit 2; an `if` condition is exempt from
errexit, so both read as false, the topic was skipped, and the loop still
printed its per-topic success line - so the RestoreJob reached Succeeded
without ever touching the topic. A well-formed but wrong-content object,
which curl -f cannot catch, must fail loudly like the unrecognised-record
guard.
Validate the recorded parts and rf are numeric before the comparisons and
--create, with a bats case that reddens when the guard is dropped.
Also correct a stale to-copy example comment: the strategy reads the exact
object the source backup recorded in status.artifact.uri (injected as
ARTIFACT_URI), not a key derived from .Backup.ApplicationRef.
Signed-off-by: Andrey Kolkov <androndo@gmail.com>
Assisted-by: LLM
|
||
|
Andrey Kolkov
|
cca3b08733 |
test(backups): pin the Kafka restore path and fail fast on a missing artifact
The restore half of the driver - the half that mutates a live Kafka cluster - had no test reaching its happy path, so three reversions of it left the suite green. - Add restore reconcile tests mirroring the Rabbitmq driver this was copied from: ToCopy (pins the targetApplicationRef.Name merge - a to-copy restore must resolve the TARGET, not write the source's topics back into the source), NoRecordedArtifactURI, TargetClusterNotReady and TargetNotFound. Verified non-vacuous: neutralising the name merge reddens ToCopy, and reverting the fail-fast ordering below reddens NoRecordedArtifactURI. - Fail fast on a Backup with no status.artifact.uri: move the check ahead of the readiness gate so it terminates immediately instead of waiting out the whole deadline and then reporting a misleading "timed out waiting for Kafka cluster to become Ready" (reconcileRabbitmqRestore checks it first; same order here). - Add bats cases for the fail-closed guards that no test reddened: MODE validation, the s3:// URI prefix, cleanup treating 404 as success, an unparsable backup/live describe, a kafka-configs describe error, and an unrecognised record kind (plus a CFG_FAIL knob in the configs stub). The describe-parse guards abort via `set -euo pipefail` at the `X=$(describe_field ...)` assignment, so these pin the fail-closed behaviour; the explicit guards remain as belt-and-braces. Signed-off-by: Andrey Kolkov <androndo@gmail.com> Assisted-by: LLM |
||
|
Andrey Kolkov
|
4925a84c26 |
fix(backups): pin the restore-side guards and bound a running Kafka Job
- delete_topic waited with `grep -qx` (BRE), so deleting `audit.events` matched the live `audit-events` and spun to its timeout — the e2e failed at step 25 on every run that reached it. Use `grep -qxF`. - The two -k/--connect-to guards were written as `! grep`, which cannot fail under hack/cozytest.sh (it wraps each @test in `set -e` + a trailing `return 0`, and bash exempts a `!`-negated command from errexit). Rewrite them as positive `if grep ...; then return 1; fi`; verified the -k reinstatement now reddens the suite under cozytest. - Cover the restore-side literal-topic guard that was unpinned at every level: a bats case runs the existing-topic branch with a colliding sibling and asserts the `--alter` argv carries `\Q…\E`, and run-all.sh now leaves `audit.events` live so the e2e exercises that branch instead of only `--create`. Add bats cases for the partition-shrink refusal, the `-f` upload guard, and `--exclude-internal`. - Bound a running Job: reconcileKafka / reconcileKafkaRestore now fail the BackupJob/RestoreJob once kafkaDefaultBackupDeadline elapses in the Job-running branch (buildJobStrategyBatchJob sets no activeDeadlineSeconds), so an unschedulable pod no longer pins it in Running forever. Pinned by TestReconcileKafka_FailsWhenJobNeverCompletes. - Fail closed on an unknown record kind and an unknown MODE, and validate the live partition/RF parsed on restore (an empty value silently skipped reconciliation). Correct the stale "image pinned in the strategy" comment in the example helpers and the chainsaw test (the controller now resolves the target broker's image as .ClientImage). Signed-off-by: Andrey Kolkov <androndo@gmail.com> Assisted-by: LLM |
||
|
Andrey Kolkov
|
ec26f1171d |
fix(backups): make Kafka metadata e2e reach an in-cluster S3 endpoint
The e2e drove run-all.sh against the shipped cozy-default-kafka strategy, whose endpoint is the platform's EXTERNAL S3 ingress. In CI that host (s3.example.org) is an unroutable placeholder with no issued cert, so the backup Pod died on `curl: (6) Could not resolve host` and the BackupJob went Failed — the chainsaw round-trip failed there, not in the driver. Mirror the rabbitmq e2e model, but without duplicating the 150-line driver script: run-all.sh now derives a demo strategy from the shipped cozy-default-kafka at run time (jq-patch: override S3_ENDPOINT, and when an endpoint override is given, mount the copied self-signed seaweedfs CA and point CURL_CA_BUNDLE at it), applies a demo BackupClass, and routes the BackupJob/Plan to it. The chainsaw run sets S3_ENDPOINT=https://seaweedfs-s3.tenant-root.svc:8333 (the .svc FQDN the seaweedfs cert SAN covers). No -k; TLS is verified against the CA. With no override (a real cluster whose external endpoint resolves and uses a public CA) the shipped endpoint is reused and no CA is mounted, since CURL_CA_BUNDLE replaces the system bundle rather than extending it. The shipped cozy-default-kafka strategy is untouched. jq derivation verified on a sample in both CA modes; example scripts pass bash -n. Signed-off-by: Andrey Kolkov <androndo@gmail.com> Assisted-by: LLM |
||
|
Andrey Kolkov
|
06e16d5f47 |
fix(backups): make the Kafka bats run under cozytest.sh (CI runner)
The suite was written for upstream bats and passed there, but CI runs hack/cozytest.sh, which knows only @test + bash: it never calls setup() and provides no $BATS_TEST_TMPDIR / run / $status / $output. So setup() never ran and the first test aborted with "STATE: parameter not set" under set -u. Move the stub setup into an init_stubs helper each test calls, drop the bats builtins for plain exit-code and $(...) assertions, and mktemp per test instead of $BATS_TEST_TMPDIR. Verified with hack/cozytest.sh (11/11), the runner make bats-unit-tests uses. Signed-off-by: Andrey Kolkov <androndo@gmail.com> Assisted-by: LLM |
||
|
Andrey Kolkov
|
aaadc70a08 |
docs(backups): drop a decaying line-count from the Kafka bats header
The comment cited "the 160-line driver script"; a count about the file decays on the next edit and nothing pins it. State the invariant instead. Signed-off-by: Andrey Kolkov <androndo@gmail.com> Assisted-by: LLM |
||
|
Andrey Kolkov
|
ebb3118694 |
fix(backups): key Kafka --connect-to off the request host in both S3 modes
The port workaround built --connect-to's HOST1 from ${host}, but the
virtual-hosted branch (forcePathStyle:false) sends to ${bucket}.${host}.
curl matches HOST1 against the request host, so on a virtual-hosted AND
non-default-port endpoint the redirect never fired and the request hit the
wrong port. Derive HOST1 from the host the URL actually uses; the real
connection target stays ${host}:${port}. Path-style is unaffected. Add a
bats case for the virtual-hosted ported mode, which the suite did not cover.
Signed-off-by: Andrey Kolkov <androndo@gmail.com>
Assisted-by: LLM
|
||
|
Andrey Kolkov
|
14c6af122e |
fix(backups): restore Kafka S3 port workaround and hold the script with a test
- Regression fix: an earlier change dropped the curl --connect-to port workaround along with -k, but curl 7.76.1 (the image's) 403s on ANY ported endpoint, which is the shape backupStorage.endpoint ships (:8333) and what provisionBucket:false and s3.ingress.enabled:false produce. Restore --connect-to (sign a portless URL, redirect to the real port), keeping -k off so TLS is still verified. IPv6-safe host/port split, path prefix preserved. The port-less ingress passed; the ported case is the one that regressed. - Correct the S3 envelope claim in values.yaml and backup-classes.md: the 403 is decided by the port, not a body-hash the backend recomputes, and --connect-to handles it on SeaweedFS and MinIO alike. - Hold the script fixes at the layer the contract lives at. Externalise the driver to files/kafka-backup.sh (embedded verbatim via .Files.Get, render byte-identical) and add hack/kafka-backup-script.bats, which EXECUTES it against stubbed kafka CLIs + curl and asserts behaviour: \Q literal match on the colliding pair, --connect-to with no -k on a ported endpoint, fail-closed --list, comma-value bracket grouping, RF-divergence abort, and the cleanup delete's HTTP handling. Not a template grep. - Pin the Backup-delete dispatch and the readiness deadline in Go: TestCleanupOnDelete_Kafka_RoutesToKafkaCleanup and TestReconcileKafka_FailsAfterReadinessDeadline. - Docs: config restore is additive (no --delete-config); export time scales with topic count and the Job has no wall clock (as its siblings). Signed-off-by: Andrey Kolkov <androndo@gmail.com> Assisted-by: LLM |
||
|
Andrey Kolkov
|
0270650237 |
fix(backups): harden Kafka metadata S3 handling and own the artifact
- Drop `-k`: mirror the RabbitMQ S3 layer (ARTIFACT_URI as the single source of truth, forcePathStyle honoured, whole base URL kept) so curl --aws-sigv4 verifies TLS against the ACME endpoint the helper returns. The derived endpoint is default-port, so the curl 7.76.1 --connect-to port workaround is gone too; this also fixes the IPv6 and path-prefix mangling in the old host-splitting. - Own the artifact: a cleanup mode in the script plus cleanupKafkaBackup wired into BackupReconciler.cleanupOnDelete delete a deleted Backup's object (best-effort, waited on) instead of orphaning it, matching the RabbitMQ precedent. Kafka has the identical own-your-object property. - Restore probes existence with --list (fail closed), so an unreachable broker aborts instead of being misread as "topic absent" and created. - Hold the fixes with tests at the right level: a colliding topic pair (audit.events / audit-events, distinct partition counts) in the e2e so a revert of the \Q...\E literal-match breaks the round-trip, and Go tests pinning the scope stamp and the cleanup Job. - Correct the docs/comments: endpoint-shape row, the "cached on nodes" claim, and the narrower curl 7.76.1 S3 envelope (empty-body payload hash) that makes a small modern-curl client image the right follow-up. Signed-off-by: Andrey Kolkov <androndo@gmail.com> Assisted-by: LLM |
||
|
Andrey Kolkov
|
405eb25095 |
fix(backups): harden Kafka metadata backup
- Fail closed on topic listing: set -o pipefail, capture kafka-topics --list --exclude-internal exit separately, and abort on failure so an unreachable broker can no longer produce an empty backup reported as Succeeded. An empty list after a successful call stays legitimate. - Check each kafka-topics --describe exit and validate the parsed partition count / replication factor before writing a record, so a timed-out describe cannot write a corrupt orders\t\t\t line. - Store topic configs one per line with key and value in separate tab-delimited fields, sourced from kafka-configs --describe (one config per line), so a value containing a comma (cleanup.policy=compact,delete) round-trips; on restore each config is applied with the value bracketed only when it contains a comma. - Detect topic divergence on restore instead of a silent --if-not-exists no-op: grow partitions with --alter when the backup asks for more, and fail loudly (naming the topic and both values) when it asks for fewer partitions or a different replication factor. A restore that cannot reach the recorded state no longer reports Succeeded. - Add KafkaStrategyKind to the RestoreJob cleanup-dispatch no-op group so deleting a Kafka RestoreJob does not run a pointless Velero cleanup; add a regression test alongside the siblings, plus a reconcile Kind-gate test and a rendering test that actually renders the strategy template. - Wire the kafka-metadata Chainsaw suite into both selectors (select-install suite_to_source, select-e2e src_to_suites) so editing the kafka app or the suite selects it and the bats unit tests pass. - Pin the client image to the operator's default broker version (kafka 3.9.1) with a digest, and align the example default; fix the values comment. Point the RabbitMQ comment references (which shipped in the CRD description) at the Job strategy actually mirrored, and trim the in-pod comments that rendered into the manifest. - Document the Strimzi Topic Operator caveat: for a topic owned by a KafkaTopic CR the operator reconciles its config from the CR, so an in-place config restore is durable only for out-of-band topics. Signed-off-by: Andrey Kolkov <androndo@gmail.com> Assisted-by: LLM |