Skip to content

fix(chart): requests-proxy honours a pinned digest for imagePullPolicy (#552) - #570

Merged
shujaatTracebloc merged 1 commit into
developfrom
fix/552-controlplane-digest-pin
Aug 4, 2026
Merged

fix(chart): requests-proxy honours a pinned digest for imagePullPolicy (#552)#570
shujaatTracebloc merged 1 commit into
developfrom
fix/552-controlplane-digest-pin

Conversation

@shujaatTracebloc

@shujaatTraceblocshujaatTracebloc commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

What & why

Narrow, safe part of #552 (control-plane restart resilience). requests-proxyhardcoded imagePullPolicy: Always even though its image line already read images.requestsProxy.digest — so a pinned digest was silently ignored for the pull policy, and every pod restart re-pulled the image even when it was already cached in containerd. That's the same offline-restart fragility class as the incident, and it's an outright inconsistency bug (the other control-plane deployments are already digest-aware).

Change

client/templates/requests-proxy-deployment.yaml — pull policy now mirrors jobs-manager / pods-monitor / resource-monitor:

  • images.requestsProxy.digestsetrepo@digest + IfNotPresent (restart-safe offline)
  • images.requestsProxy.digestempty (default) → repo:tag + Always (unchanged default behaviour)

Verified by render: default → docker.io/tracebloc/jobs-manager:prod + Always; with a digest → …@sha256:… + IfNotPresent.

Tests

client/tests/requests_proxy_test.yaml+2 helm-unittest cases: default floats on the tag with Always; a set digest renders repo@digest + IfNotPresent. Suite green (13/13).

client/Chart.yaml 1.9.11 → 1.9.12 (chart-content change requires a bump).

Scope

This is deliberately only the requests-proxy bug. The broader offline-restart update-model change for the always-running images — jobs-manager/pods-monitor are updated by the image-refresh CronJob via rollout restart, which relies on Always, and resource-monitor isn't refreshed at all — is a real design decision (digest-pin à la ingestor.prodDigest, vs. digest-on-update in image-refresh) and is tracked in #569.

Part of #552.


Note

Low Risk
Narrow Helm template fix with unchanged default behaviour when no digest is set; covered by new unit tests.

Overview
requests-proxy no longer hardcodes imagePullPolicy: Always when images.requestsProxy.digest is set. Pull policy now matches jobs-manager, pods-monitor, and resource-monitor: pinned digest → repo@digest + IfNotPresent (restart without registry); no digest → tag + Always (unchanged default).

Helm unittest adds two cases for default vs digest-pinned rendering. Chart version 1.9.11 → 1.9.12.

Reviewed by Cursor Bugbot for commit 97611fa. Bugbot is set up for automated code reviews on this repo. Configure here.

#552)
requests-proxy hardcoded `imagePullPolicy: Always` while its image line already
read `images.requestsProxy.digest` — so a pinned digest was ignored for the pull
policy and every restart re-pulled the image even when it was already cached.
Make it digest-aware, matching jobs-manager / pods-monitor / resource-monitor:
digest set -> repo@digest + IfNotPresent (restart-safe offline)
digest empty -> repo:tag + Always (unchanged default)
+2 helm-unittest cases (default -> Always; digest -> repo@digest + IfNotPresent).
Chart 1.9.11 -> 1.9.12 (chart-content change requires a version bump).
Scope: this is the narrow, unambiguous part of #552. The broader control-plane
offline-restart update-model change (jobs-manager/pods-monitor rely on Always for
the image-refresh CronJob; resource-monitor too) is a deliberate design decision
tracked in #569.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@shujaatTraceblocshujaatTracebloc self-assigned this Aug 3, 2026
@shujaatTracebloc
shujaatTracebloc marked this pull request as ready for review August 4, 2026 07:51
@shujaatTracebloc
shujaatTracebloc requested review from aptracebloc and removed request for saadqbalAugust 4, 2026 07:56

@aptraceblocaptracebloc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shujaatTraceblocApproved. Clean parity fix. What I verified:

  • ✅ requests-proxy was the sole outlier. The other first-party floating-tag deployments already use the identical {{ if digest }}IfNotPresent{{ else }}Always{{ end }} idiom — jobs-manager (.Values.images.jobsManager.digest), pods-monitor, and resource-monitor all match. This brings requests-proxy into line. The two hardcoded-IfNotPresent templates (egress-proxy/squid, mysql-client) are correctly-pinned third-party images, not the same bug — so no other deployment carries this latent issue.
  • ✅ Live-settable, not inertimages.requestsProxy.digest is in values.schema.json and the unittest's set: exercises it end-to-end (renders repo@digest + IfNotPresent); the dig "requestsProxy" "digest" "" accessor is the safest null-nav form.
  • ✅ Chart version bumped 1.9.11 → 1.9.12 (correct Helm convention), and both branches (tag→Always, digest→IfNotPresent) are pinned by the new helm-unittest cases (CI-run).
  • No R8 manifest concern — this is chart territory, not the installer sub-scripts.

(Verification note: my local helm template --show-only didn't emit — a missing-value/path quirk, not a fix defect, since the template is ungated and the unittest renders it cleanly. Relied on the source-parity check + the CI unittest cases + the schema check.)

⚪ Nits (non-blocking)

  • The chart now has three idioms for the same digest null-navigation: dig … (this PR, safest), resource-monitor's default (dict) chaining, and jobs-manager's direct .Values.images.jobsManager.digest (which would error if images.jobsManager were absent). Not this PR's problem — it picked the safest — but they'd read more consistently converged.
  • Worth flagging for context: requests-proxy runs the jobs-manager image (shared, different command) pinned via its own requestsProxy.digest key, so it can be pinned independently of jobsManager.digest — and the fix correctly honors the proxy's own key.

Minimal, correct, closes a real "re-pull on every restart / not offline-safe when pinned" gap. Touches your code-owned client/templates/, so Asad's code-owner review is the backstop; this is the second set of eyes.

🤖 Generated with Claude Code

@shujaatTracebloc
shujaatTracebloc merged commit 5743028 into developAug 4, 2026
31 checks passed
@shujaatTracebloc
shujaatTracebloc deleted the fix/552-controlplane-digest-pin branch August 4, 2026 08:10
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@shujaatTracebloc@aptracebloc@LukasWodka