fix(cozy-lib,harbor,mariadb): digest-pin cleanup-hook kubectl image and route it through cozy-lib.images-registry - #3171
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughHarbor and MariaDB cleanup hooks now render digest-pinned ChangesDigest-pinned cleanup images
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses reliability and security concerns in the Harbor and MariaDB cleanup hooks. By replacing hardcoded, mutable image tags with digest-pinned references and introducing a flexible registry routing helper, the changes ensure that cleanup jobs remain functional in air-gapped clusters and maintain consistent image resolution across different deployment environments. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a new helper template cozy-lib.image to build fully-qualified image references routed through the cluster images registry, and updates the cleanup hooks in harbor and mariadb to use this helper with digest-pinned images. It also adds corresponding tests to verify image routing and registry prefixing. The reviewer suggested trimming potential leading/trailing slashes from the image path and registry to prevent malformed image references with double slashes.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| {{- $image := index . 0 }} | ||
| {{- $ctx := index . 1 }} | ||
| {{- $registry := include "cozy-lib.images-registry" $ctx }} | ||
| {{- with $registry }}{{ . }}/{{ end }}{{ $image }} |
There was a problem hiding this comment.
To prevent potential malformed image references with double slashes (e.g., if images-registry is configured with a trailing slash or if the image path starts with a leading slash), it is safer to trim any trailing slash from the registry and any leading slash from the image path.
{{- $image := index . 0 | trimPrefix "/" }}
{{- $ctx := index . 1 }}
{{- $registry := include "cozy-lib.images-registry" $ctx | trimSuffix "/" }}
{{- with $registry }}{{ . }}/{{ end }}{{ $image }}
There was a problem hiding this comment.
Good catch — applied in a76c592. The cozy-lib.image helper now trimPrefix "/" on the image path and trimSuffix "/" on the registry, so a misconfigured images-registry with a trailing slash (e.g. registry.example.com/) no longer yields a double slash. Added a regression test in both the harbor and mariadb cleanup-hook suites asserting the rendered ref contains no //. Thanks!
57bd941 to
123be9e
Compare
…nd route it through cozy-lib.images-registry The post-delete cleanup Jobs in the harbor and mariadb charts hardcoded `docker.io/clastix/kubectl:v1.32` — a moving tag pulled directly from Docker Hub. This broke build reproducibility (the tag can be re-pushed), was a supply-chain risk, and could not be satisfied on air-gapped / mirrored clusters, where it failed at the worst moment: during uninstall, when the post-delete hook runs and stalls release teardown. Add a canonical `cozy-lib.image` helper that prefixes a registry-relative image path with `cozy-lib.images-registry` when set and returns it unchanged (no leading "/") when empty, so standard installs resolve from the default registry and mirrored installs resolve internally. Consume it from both cleanup hooks with the image digest-pinned to the immutable multi-arch index sha256:b9ef7d8dbe65bcc81a46c09b8dc7543103055021c4f43287bf59e92a8f4fe05c (the same digest already pinned for postgres-operator's webhook hook). Keep the pin fresh with a custom Renovate manager: the repo disables the helm-values manager (enabledManagers), and the ref is built at render time by the helper rather than being a Dockerfile/go.mod dependency, so the built-in managers cannot see it. A regex manager over the hook templates tracks clastix/kubectl by tag+digest — mirroring the existing extra/etcd manager — so the supply-chain pin does not silently age. Also harden `cozy-lib.images-registry` against a nil .Values._cluster so the helper never crashes the render — relevant precisely because the hook renders during teardown. Helm unit tests assert the rendered image (routed, digest-pinned, no leading slash) by pattern rather than an exact digest, so a Renovate bump keeps them green. Closes #3088 Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Алексей Артамонов <alexeyartamonov1987@gmail.com>
123be9e to
a76c592
Compare
IvanHunters
left a comment
There was a problem hiding this comment.
Review: LGTM with non-blocking notes
The digest-pin and the cozy-lib.image routing are correct across all three charts. Both chart unittests pass (harbor 16, mariadb 10).
Main note (accuracy of framing)
cozy-lib.images-registryreads.Values._cluster["images-registry"], but that key is never populated anywhere in the tree. Thecozystack-valuesemitter (packages/core/platform/templates/apps.yaml) emits ~40_clusterkeys andimages-registryis not one of them; there is no Go injector and no installer manifest that sets it. So on every real cluster today the registry resolves empty andcozy-lib.imagereturns the image unchanged. The routing is correct and future-proof, but it is currently dormant: air-gapped resolution is not achieved until a separate change populates_cluster.images-registry. Suggest softening the PR body / release-note to "establishes the routing" rather than implying present air-gapped capability.
Verified
helm templaterendersclastix/kubectl:v1.32@sha256:b9ef7d8…with empty registry (no leading slash) andregistry.example.com/clastix/kubectl:v1.32@sha256:b9ef7d8…with the registry set.- nil-
_clusterhardening ((.Values._cluster | default dict)) is necessary: the oldindex .Values._cluster "images-registry"fails withindex of untyped nilwhen_clusteris unset. - Bare
clastix/kubectl(nodocker.io/prefix) resolves todocker.io/clastix/kubectl, so the join produces<mirror>/clastix/kubectlrather than<mirror>/docker.io/clastix/kubectl. - Digest matches the pin already at
packages/system/postgres-operator/values.yaml. - Renovate manager
matchStringsrequires@sha256:andmanagerFilePatternsmatches both changed files.
Follow-ups (separate PR, as the body acknowledges)
- Migrate the remaining hardcoded
docker.io/clastix/kubectl:v1.32cleanup hooks to the same pattern (keycloak-configure, dashboard, bucket, qdrant, kubernetes, seaweedfs, gateway, clickhouse, tenant). - Some of those are not under a
templates/hooks/directory, so they are not caught by this PR's RenovatemanagerFilePatterns; broaden the pattern when migrating them or the pins will silently age.
Upgrade impact: none (hooks fire only on post-delete; rendered image is registry-identical to before plus an immutable digest). Fresh install: none negative.
What this PR does
Fixes #3088.
The post-delete cleanup Jobs in the harbor and mariadb charts hardcoded the cleanup image as
docker.io/clastix/kubectl:v1.32— a moving Docker Hub tag. This:v1.32is mutable and can be re-pushed to a different image;docker.iobypassed the cluster images registry, and it failed at the worst moment: during uninstall, when thepost-deletehook runs and a stuck Job stalls release teardown.Changes
cozy-lib.image(packages/library/cozy-lib/templates/_cozyconfig.tpl). Given a registry-relative image path and the global context, it prefixescozy-lib.images-registrywhen set (<registry>/<image>) and returns the image unchanged when empty — with no leading/, so standard installs still resolve from the default registry. This establishes the previously-undefined "how to reference a cluster image" pattern (images-registrywas defined but never consumed).clastix/kubectl:v1.32@sha256:b9ef7d8dbe65bcc81a46c09b8dc7543103055021c4f43287bf59e92a8f4fe05c(the immutable multi-arch OCI index for thev1.32tag; the same digest already pinned forpostgres-operator's webhook hook).enabledManagers), and the ref is assembled at render time by the helper rather than being a Dockerfile / go.mod dependency, so the built-in managers can't see it. Acustom.regexmanager over the hook templates tracksclastix/kubectlby tag+digest — mirroring the existingextra/etcdmanager — so the supply-chain pin doesn't silently age.cozy-lib.images-registryagainst a nil.Values._clusterso the helper never crashes the render — relevant precisely because this hook renders during teardown.Verification
helm unittestpasses for both charts (harbor: 16, mariadb: 10).helm templaterenders confirmed for both charts:clastix/kubectl:v1.32@sha256:b9ef…images-registry=registry.internal:5000→registry.internal:5000/clastix/kubectl:v1.32@sha256:b9ef…matchStringsregex verified to capturecurrentValue=v1.32and the digest from both hook templates.Note on the wider footprint
The same
docker.io/clastix/kubectl:v1.32hardcode is already merged onmainin several other cleanup hooks (bucket, qdrant, gateway, seaweedfs, kubernetes, tenant, dashboard, keycloak-configure) and appears in the open PRs #3170 (etcd), #3094 (monitoring), #3072 (clickhouse). This PR fixes harbor + mariadb per the issue and establishes thecozy-lib.image+ Renovate pattern; migrating the remaining hooks is a follow-up.Release note
🤖 Generated with Claude Code
Summary by CodeRabbit