fix(backupstrategy-controller): run backup clients on a package-owned image by androndo · Pull Request #4756 · cozystack/cozystack

Andrey Kolkov (androndo) · 2026-10-05T18:06:54Z

What this PR does

The Altinity (ClickHouse) and RabbitMQ backup strategy Pods ran platform-migrations: a 349 MB image carrying kubectl, helm, git, cozyhr and etcd-migrate, whose ENTRYPOINT is run-migrations.sh. Both pins were stamped from packages/core/platform/Makefile, so any migration change re-digested the backup clients too, and only the command: override kept the Pods from running platform migrations (#3420).

New packages/system/backupstrategy-controller/images/backup-http-client/: alpine:3.24 (digest-pinned, the same base the tree already uses) + curl + jq + ca-certificates, USER 65532:65532, no ENTRYPOINT. Everything is installed at build time, so the Pods still never run apk add and keep working air-gapped.
New image-backup-http-client target in the package's own Makefile, chained after image-redis-backup like the existing ones so only one yq -i writes values.yaml at a time. It stamps chBackupClientImage and rabbitmqBackupClientImage with the same ref.
Removed the cross-package stamps from packages/core/platform/Makefile, the WORKAROUND/TODO comments in the chart and the RabbitMQ example, and the stale "live violation" line in docs/agents/image-refs.md.
hack/image-pin-consistency.bats: dropped the platform-migrations-specific test, whose subject no longer exists. The general check (no repository pinned at more than one digest, computed from hack/promote-retag.sh --dry-run) still covers the two keys sharing one image.

Why one image for both strategies rather than ch-backup-client as the issue named it. The RabbitMQ strategy had the same reuse and the same cross-package stamp; leaving it would have kept the stamp alive. Its script needs curl --aws-sigv4 + ca-certificates, a subset of the ClickHouse one, so one image serves both. The two values keys stay separate so either strategy can move to its own image later.

Initial pin. The image does not exist in ghcr until CI builds it, so both keys are committed as backup-http-client:v1.6.0 without a digest, the same way redis-backup was introduced. PR CI rebuilds touched packages and overlays the fresh refs for e2e; release preparation's make image stamps tag+digest.

Image size: 6.9 MB vs 349 MB. Verified locally:

docker buildx build for linux/amd64,linux/arm64; in the image as uid 65532: curl 8.22 with --aws-sigv4, jq 1.8.2, date, seq, writes to /tmp (the RabbitMQ script's only write), TLS verification against the bundled CA store.
helm unittest (23/23), helm template renders both strategies with backup-http-client and no platform-migrations reference.
hack/cozytest.sh hack/image-pin-consistency.bats and hack/build-matrix_test.bats pass.

Not yet run end to end on a cluster; the e2e backup round-trips will exercise it.

Fixes #3420

Screenshots

N/A — no UI changes.

Downstream repositories

Walked the trigger map against the diff: it touches packages/system/backupstrategy-controller (new image, Makefile target, values comments/pins, template comments), packages/core/platform/Makefile (stamp removal only, no values.yaml key change), hack/image-pin-consistency.bats, docs/agents/image-refs.md and a comment in examples/backups/rabbitmq. No app package, schema, CRD, variant, asset, platform values key or hack/package.mk change. The website does not list first-party image repositories (searched for platform-migrations and redis-backup), so no mirror list needs the new one.

Release note

fix(backupstrategy-controller): the ClickHouse (Altinity) and RabbitMQ backup strategy Pods now run a dedicated `backup-http-client` image (alpine + curl + jq + ca-certificates, non-root, ~7 MB) instead of the 349 MB `platform-migrations` image. Air-gapped mirrors need the new `ghcr.io/cozystack/cozystack/backup-http-client` repository.

Summary by CodeRabbit

Improvements
- Altinity and RabbitMQ backups now use a dedicated backup HTTP client image, configured separately from the platform migrations image.
- The client image includes the tools required for backup operations and runs with a non-root identity.
- Both backup configurations receive the same digest-pinned image reference when the image is built.

coderabbitai · 2026-10-05T18:07:02Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: cozystack/cozystack/.coderabbit.yaml
Review profile: CHILL
Plan: Advanced
Run ID: 1fc9dd1e-5d5d-4722-b069-e7de449b48af

📥 Commits

Reviewing files that changed from the base of the PR and between 8f6b7e8 and 770f515.

📒 Files selected for processing (2)

hack/image-pin-consistency.bats
packages/system/backupstrategy-controller/values.yaml

🚧 Files skipped from review as they are similar to previous changes (1)

packages/system/backupstrategy-controller/values.yaml

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.

📝 Walkthrough

Walkthrough

The backup strategy controller now builds a dedicated backup HTTP client image and stamps its digest into both backup client image values. The platform Makefile no longer stamps those values. Related comments, image-pin tests, and documentation are updated.

Changes

Backup HTTP client image

Layer / File(s)	Summary
Build and stamp the dedicated image `packages/system/backupstrategy-controller/values.yaml`, `packages/system/backupstrategy-controller/images/backup-http-client/Dockerfile`, `packages/system/backupstrategy-controller/Makefile`, `packages/system/backupstrategy-controller/templates/*`, `examples/backups/rabbitmq/03-rabbitmq-strategy.yaml`	The controller package adds a dedicated image with `curl`, `jq`, and `ca-certificates`. Its Make target builds the image and stamps the digest into both backup client values. Related comments are updated; the example command remains unchanged.
Update image pin ownership `packages/core/platform/Makefile`, `hack/image-pin-consistency.bats`, `docs/agents/image-refs.md`	The platform Makefile stops stamping the backup client values. Image-pin test guidance and image-reference documentation no longer identify the former migration-image pins as live violations. The test comparing three migration-image values is removed.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant Makefile as backupstrategy-controller Makefile
  participant Image as backup-http-client image
  participant Values as backupstrategy-controller values.yaml
  Makefile->>Image: Build image and read digest
  Makefile->>Values: Set both backup client image values

Merge Risk: ⚪ Minimal · up to 770f5

The image and build flow match both backup strategies. Supported release paths stamp the image before use, and no material merge-blocking risk remains.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 41969

The dedicated, non-root image reduces unnecessary tooling and migration coupling without changing backup credentials or workflow commands. However, the committed defaults lose their digest pins, making reproducible and trustworthy image selection depend on building and stamping values before deployment.

Retained concerns

Medium · security · inferred: Both credential-bearing backup clients change from digest-pinned images to a tag-only default. Unless deployment first uses the stamped values, executable identity depends on registry tag contents and node caches rather than the reviewed digest. The normal build restores pins, but the committed chart defaults and retained duplicate-pin check do not enforce that prerequisite.

Security review details

Security Blast Radius

inferred — The shared defaults can affect every Altinity or RabbitMQ strategy workload using them. Each client receives application API credentials; RabbitMQ also receives backup-storage credentials. Namespace-prefixed object keys describe normal addressing, not an IAM boundary. Maximum S3 exposure cannot be bounded without the effective credential policies.

Security Findings and Attack Paths

inferred — If unstamped defaults are deployed, an actor able to replace the referenced registry tag could substitute client executables and access injected credentials or authorized backup data. Registry-write authority is a prerequisite; no ordinary tenant exploit or actual substitution was established. Digest-pinned base references resisted this tag-substitution path.

Trust Boundaries and Controls

observed — Secret references and target construction are unchanged. The scripts retain authenticated in-cluster HTTP API access and RabbitMQ's S3 SigV4 requests, without disabling certificate verification for HTTPS. The PR changes executable supply and OS identity, not the declared credential or endpoint authority.

Resilience and Maintainability Implications

observed — Removing the migration-image dependency narrows backup runtime contents and eliminates cross-package pin updates. The remaining duplicate-pin check detects multiple collected digests per repository, but its digest-only collector skips these new tag-only defaults rather than proving their executable identity.

Pre-merge checks | 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely describes the main change: replacing the shared image with a package-owned image for the backup strategy controller.
Linked Issues check		Issue `#3420` requires a package-owned minimal image, build-time installation of `curl`, `jq`, and CA certificates, a package Makefile target that stamps the image value, and removal of the cross-packag…
Out of Scope Changes check		The reviewed changes support issue `#3420`. The RabbitMQ extension, shared image reference, related comments, and generalized pin-consistency test all address the same image reuse and stamping problem. …
Docstring Coverage		Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 3 files. (1 skipped: 1 …

✨ Finishing Touches

📝 Generate docstrings

Commit to this branch
Create a new PR

🧪 Generate unit tests (beta)

Commit to this branch
Create a new PR

Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

IvanHunters

LGTM.

This swaps the ClickHouse/Altinity and RabbitMQ backup strategy Pods off the reused 349 MB platform-migrations image and onto a dedicated backup-http-client (alpine + curl + jq + ca-certificates, non-root 65532, ~7 MB) built by the package's own Makefile. It also removes the cross-package stamp in platform/Makefile that had left one key with no producer, frozen at v1.4.0-rc.2 (#3420). The design is clean: a single yq writes both keys to one digest, the target chains after image-redis-backup so the three yq -i writers never race under make -j, and both strategies render against the new ref with the command: override intact and no platform-migrations reference left in the rendered output.

Verified against a merge of current origin/main: helm unittest 23/23, image-pin-consistency and build-matrix bats green, both strategies render backup-http-client:v1.6.0 with zero platform-migrations refs, and the ClickHouse and RabbitMQ chainsaw suites each drive a real BackupJob and RestoreJob round-trip, so the new image is exercised in the E2E lane once the suite runs. A throwaway probe of the pinned alpine digest confirmed curl 8.22 with --aws-sigv4, jq 1.8.2, a writable /tmp, and pipefail under busybox ash, so the uid 65532 switch and the S3 transport both hold; the scripts write only to /tmp (RabbitMQ) or nothing (Altinity).

CI

The red Unit & controller tests on this head is not from this PR. Its three failures are internal/fluxcontract/snapshot_shape_test.go, the negated-assert audit flagging find's ! in select-e2e_test.bats:34, and the flux-install workflow contract. All three are main's own contracts broken by adjacent merges, already fixed on main by #4751 and three follow-up commits that landed after this branch's base. Merging current origin/main into this head makes all three pass (ran them locally: go test ok, both bats audits OK). A rebase clears the check. E2E (in-tree) was skipped only because it gates on that stale unit job.

Findings (all non-blocking, posted inline)

MINOR values.yaml:12: both keys committed as a bare tag that was never released and that make image will not reproduce; contradicts the invariant this PR edits in image-refs.md.
NIT values.yaml:41: comment reflow left one 135-column line in an otherwise ~84-column block.
NIT backup-http-client/Dockerfile:2: comment says both scripts parse JSON with jq, but the RabbitMQ script uses curl only.

Checked and correct

Bare-tag introduction is consistent with redis-backup and tolerated by the enumeration/promote suites (promote-rewrite-tags 17/0).
No orphaned platform-migrations consumer: platform/Makefile still stamps .migrations.image, the only remaining reference.
The dropped platform-migrations is pinned identically bats test loses no real coverage: exact tag-string equality is now guaranteed by one yq writing both keys, and the general duplicate-digest check still holds.
The previous image ran as root (no USER in platform-migrations), so the uid change is a hardening, not a regression; nothing in either script depends on root.

IvanHunters · 2026-10-06T08:00:52Z

+  # release promotion (hack/image-pin-consistency.bats). They are separate keys
+  # so either strategy can move to its own image later.
+  # DO NOT hand-edit: `make image` stamps both.
+  chBackupClientImage: "ghcr.io/cozystack/cozystack/backup-http-client:v1.6.0"


[MINOR] both keys committed as a bare tag that was never released and that make image will not reproduce

Both keys are committed as the bare tag backup-http-client:v1.6.0 with no digest. Unlike backupstrategy-controller:v1.6.0@sha256:... two lines up, this tag was never released (v1.6.0 predates the image) and make image in this tree stamps the dev version derived from git describe (v1.7.0-alpha.3-...), so the committed ref is unpullable as written. Release preparation and PR CI both stamp the real tag+digest before anything consumes the ref (overlay-main-images.sh classifies the *Image: keys as refs since #4257), and redis-backup:v1.6.0 was introduced the same way, so this is not a release or install defect. It does, though, contradict the invariant this same PR edits in docs/agents/image-refs.md ("A bare tag with no digest ... must not be committed"), and a dev applying the raw tree with helm template | kubectl apply (no build first) gets ImagePullBackOff on both the Altinity and RabbitMQ strategy Pods, where the previous platform-migrations pin resolved. Either note the "first commit of a new image" exception in that doc, or publish the image first and pin the digest.

IvanHunters · 2026-10-06T08:00:52Z

  # The Kafka metadata strategy carries no client image here: unlike the Altinity
-  # and RabbitMQ drivers (curl-only, so a shared platform-migrations image works),
-  # its driver runs the Kafka Admin-API CLI (kafka-topics / kafka-configs), which
+  # and RabbitMQ drivers, which need only curl and jq, its driver runs the Kafka Admin-API CLI (kafka-topics / kafka-configs), which


[NIT] comment reflow left one 135-column line in an otherwise ~84-column block

The Kafka comment reflow left this line at 135 columns while the rest of the block wraps at ~84. Cosmetic, but it is the only line in the file that breaks the wrap.

IvanHunters · 2026-10-06T08:00:52Z

@@ -0,0 +1,9 @@
+# backup-http-client runs the Altinity (ClickHouse) and RabbitMQ strategy Pods:
+# both scripts only drive an HTTP API and S3 with curl, parse JSON with jq, and


[NIT] comment says both scripts parse JSON with jq, but the RabbitMQ script uses curl only

"both scripts ... parse JSON with jq" holds only for the Altinity script; the RabbitMQ script (templates/strategy-rabbitmq-default.yaml) uses curl only, with no jq call (5 jq invocations in the Altinity script, 0 in the RabbitMQ one). Harmless, since jq is installed regardless, but the comment overstates what the RabbitMQ Pod needs.

Aleksei Sviridkin (lexfrei)

Andrey Kolkov (@androndo) NOT LGTM, but only because of the commit history. The image change itself is correct.

The second commit fixes comments that the first commit added: the Dockerfile header (only the Altinity script uses jq) and a rewrap in values.yaml. Its last paragraph walks through that diff, and with merge commits it lands in main as written. Please fold those two edits into d6b945a, so 8f6b7e8 keeps only the image-refs.md change and its first paragraph.

While there: the Kafka comment in values.yaml still says the Altinity and RabbitMQ drivers "need only curl and jq". The RabbitMQ script does not use jq, as the new Dockerfile header says.

What I checked on a trial merge with current main: helm unittest for the package, hack/image-pin-consistency.bats, hack/build-matrix_test.bats and hack/promote-rewrite-tags_test.bats pass. Both scripts already ran under busybox sh, and the date -u +FORMAT and seq calls work with busybox. The bare-tag keys are picked up by the *Image: branch in hack/overlay-main-images.sh.

The red E2E Tests is not caused by the code. clickhouse-2-backup-roundtrip failed on ImagePullBackOff: the e2e registry answers 403 Forbidden for the new backup-http-client repository. A new repository there is created private on first push, so a maintainer has to make it public before a rerun can pass. I still get the same 403 today with an anonymous pull token, while an existing repository in the same namespace accepts the same token.

… image The Altinity and RabbitMQ strategy Pods ran platform-migrations, a 349 MB image carrying kubectl, helm and etcd-migrate whose ENTRYPOINT is the migration runner, and whose pins were stamped from packages/core/platform so every migration change moved the backup clients too. Build images/backup-http-client (alpine + curl + jq + ca-certificates, non-root) from this package's Makefile and stamp both client keys from it. The cross-package stamp in packages/core/platform/Makefile and the platform-migrations-specific check in hack/image-pin-consistency.bats go away; the general one-digest-per-repository check still covers the two keys sharing one image. Fixes #3420 Assisted-by: LLM Signed-off-by: Andrey Kolkov <[email protected]>

docs/agents/image-refs.md forbade committing a bare tag without saying how a new image's first pin can carry a digest it does not have yet. Record the exception the backup client keys rely on: the bare tag is a placeholder the producing `make image` replaces, and an unbuilt tree pulls a tag that does not exist. Drop the securitygroup-controller example, which is pinned by digest now. Assisted-by: LLM Signed-off-by: Andrey Kolkov <[email protected]>

Aleksei Sviridkin (lexfrei)

Andrey Kolkov (@androndo) LGTM. The history and the Kafka comment are fixed. What still blocks E2E is on the maintainer side.

On a trial merge with current main (95c2d85), helm unittest for the package passes, and so do hack/image-pin-consistency.bats and hack/build-matrix_test.bats. Both keys are in packages/system/backupstrategy-controller/values.yaml, which the values.yaml glob in hack/lib/image-refs.sh covers.

The e2e registry still refuses an anonymous pull of backup-http-client. The anonymous token request is denied with "Anonymous users are only allowed read access on public repos", and the manifest fetch for this head's tag returns 403. redis-backup in the same namespace lists its tags with an anonymous token. A maintainer has to make the new repository public before the backup round-trip e2e can pass; nothing in the PR needs to change for that.

github-actions Bot added area/storage Issues or PRs related to storage (linstor, seaweedfs, bucket, velero, harbor) kind/bug Categorizes issue or PR as related to a bug size/L This PR changes 100-499 lines, ignoring generated files labels Oct 5, 2026

Andrey Kolkov (androndo) marked this pull request as ready for review October 5, 2026 18:08

Andrey Kolkov (androndo) requested review from IvanHunters, Kingdon Barrett (kingdonb), Andrei Kvapil (kvaps), Aleksei Sviridkin (lexfrei), Timofei Larkin (lllamnyp), mattia-eleuteri, myasnikovdaniil, Kirill Ilin (sircthulhu) and Timur Tukaev (tym83) as code owners October 5, 2026 18:09

IvanHunters previously approved these changes Oct 6, 2026

View reviewed changes

Andrey Kolkov (androndo) dismissed IvanHunters’s stale review via 8f6b7e8 October 6, 2026 08:41

Andrey Kolkov (androndo) force-pushed the fix/backup-client-image branch from 41969a8 to 8f6b7e8 Compare October 6, 2026 08:41

Aleksei Sviridkin (lexfrei) requested changes Oct 8, 2026

View reviewed changes

Andrey Kolkov (androndo) added 2 commits October 12, 2026 09:54

Andrey Kolkov (androndo) force-pushed the fix/backup-client-image branch from 8f6b7e8 to 770f515 Compare October 12, 2026 05:56

Aleksei Sviridkin (lexfrei) approved these changes Oct 12, 2026

View reviewed changes

Aleksei Sviridkin (lexfrei) enabled auto-merge October 12, 2026 06:04

Aleksei Sviridkin (lexfrei) merged commit 7c46fff into main Oct 12, 2026
21 checks passed

Aleksei Sviridkin (lexfrei) deleted the fix/backup-client-image branch October 12, 2026 09:38

Navigation Menu

Repository navigation

fix(backupstrategy-controller): run backup clients on a package-owned image - #4756

fix(backupstrategy-controller): run backup clients on a package-owned image#4756
Aleksei Sviridkin (lexfrei) merged 2 commits into
mainfrom
fix/backup-client-image

Andrey Kolkov (androndo) commented Oct 5, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Oct 5, 2026 •

edited

Loading

Walkthrough

Changes

Sequence Diagram(s)

Uh oh!

IvanHunters left a comment

Uh oh!

IvanHunters Oct 6, 2026

Uh oh!

IvanHunters Oct 6, 2026

Uh oh!

IvanHunters Oct 6, 2026

Uh oh!

Aleksei Sviridkin (lexfrei) left a comment

Uh oh!

Aleksei Sviridkin (lexfrei) left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		@@ -0,0 +1,9 @@
		# backup-http-client runs the Altinity (ClickHouse) and RabbitMQ strategy Pods:
		# both scripts only drive an HTTP API and S3 with curl, parse JSON with jq, and

Conversation

Andrey Kolkov (androndo) commented Oct 5, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR does

Screenshots

Downstream repositories

Release note

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Oct 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Uh oh!

IvanHunters left a comment

Choose a reason for hiding this comment

CI

Findings (all non-blocking, posted inline)

Checked and correct

Uh oh!

IvanHunters Oct 6, 2026

Choose a reason for hiding this comment

Uh oh!

IvanHunters Oct 6, 2026

Choose a reason for hiding this comment

Uh oh!

IvanHunters Oct 6, 2026

Choose a reason for hiding this comment

Uh oh!

Aleksei Sviridkin (lexfrei) left a comment

Choose a reason for hiding this comment

Uh oh!

Aleksei Sviridkin (lexfrei) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Andrey Kolkov (androndo) commented Oct 5, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Oct 5, 2026 •

edited

Loading