NO-ISSUE: Cache the etcd image at snapshot time so recert never needs a live pull secret

[eliorerz]

Summary

cmd_boot's recert step starts a standalone etcd container with --authfile /var/lib/kubelet/config.json — the golden snapshot's frozen copy of that file, which cmd_boot must never overwrite out-of-band (MCO owns it; see test_boot_pull_secret_not_written_to_node). If the credential baked into that frozen file is ever revoked or rotated independently of the live pull secret cmd_boot is given (e.g. a leaked registry credential getting rotated), every future boot from that snapshot fails at this exact pull, with no way to recover short of rebuilding the snapshot.

Observed in production: osac-installer#30327475992, reproduced on two different machines (osac-8, osac-9), same image digest — unauthorized: access to the requested resource is not authorized pulling quay.io/openshift-release-dev/ocp-v4.0-art-dev. Rotating the pull secret in Vault did not fix it, because it was never the credential recert actually uses.

Fix

RECERT_IMAGE and the nodeip-configuration image already avoid this problem by being cached into the VM's local podman store at snapshot-creation time (cmd_snapshot), so cmd_boot's podman run never needs the network or the authfile for them. This caches the etcd image the same way — once it's present locally, podman's default --pull=missing behavior skips the pull (and the credential check) entirely, regardless of whether the snapshot's baked-in pull secret is still valid.

Also added cache-etcd to the post-snapshot prune/cleanup command, matching cache-recert/cache-nodeip, so the new anchor container doesn't leak.

Existing snapshots still need to be rebuilt once for this to take effect — this only prevents the failure for snapshots created after this change merges.

Test plan

• Added test_snapshot_caches_etcd_image (mirrors test_snapshot_caches_recert_before_prune): asserts the etcd image is pulled and anchored before pruning
• Updated test_snapshot_skips_pull_when_image_already_cached to expect all three anchors (recert/nodeip/etcd) when images are already local
• Updated test_snapshot_prunes_container_images to assert the new cache-etcd anchor is cleaned up
• Full suite: python3 -m pytest test_cluster_tool.py — 158 passed, same 11 pre-existing failures as on main before this change (confirmed via git stash comparison — unrelated DNSMASQ_DIR environment issue, not caused by this PR)
• python3 -m py_compile cluster-tool test_cluster_tool.py — clean

Summary by CodeRabbit

• Bug Fixes
  • Snapshot creation now caches the detected etcd image alongside other required boot images.
  • Snapshot cleanup now removes the temporary etcd image cache container.
• Tests
  • Expanded coverage verifies etcd image caching, cleanup, and the correct operation order before image pruning.

[coderabbitai]

Walkthrough

Snapshot boot-image caching now includes the detected standalone etcd image through a cache-etcd anchor, and cleanup removes that anchor after image pruning. Tests verify caching, command ordering, and cleanup.

Changes

Snapshot etcd image caching

Layer / File(s)	Summary
Cache and clean up the etcd image cluster-tool, test_cluster_tool.py	cmd_snapshot caches the detected etcd image with cache_image, removes cache-etcd during pruning, and tests pull, anchor creation order, and cleanup.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

• osac-project/cluster-tool#32: Adds the existing boot-image anchor caching and pruning behavior that this change extends for etcd.

Suggested reviewers: omer-vishlitzky, danielerez

---

Caution

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

• Ignore

❌ Failed checks (1 error)

Check name	Status	Explanation	Resolution
No-Injection-Vectors	❌ Error	cache_image() shells out with unquoted etcd_image from the VM manifest in podman exists/pull/create, enabling shell injection if that string is malicious.	Quote image args with shlex.quote() or pass argv lists to run_vm; avoid building shell commands from manifest-derived strings.

✅ Passed checks (10 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
No-Hardcoded-Secrets	✅ Passed	No new hardcoded secrets found; added literals are image refs, anchors, and test text, with no API-key/token/password/private-key shapes.
No-Weak-Crypto	✅ Passed	No weak-crypto primitives or secret comparisons were added; the patch only caches etcd via podman and updates tests.
Container-Privileges	✅ Passed	No container/K8s manifest changes; the patch only adds cache-etcd caching/cleanup and test assertions, with no new privileged/root/host* settings introduced.
No-Sensitive-Data-In-Logs	✅ Passed	No new runtime logging was added; the change only caches etcd and updates tests/comments, with no secret/token/PII output introduced.
Ai-Attribution	✅ Passed	No AI-tool usage is mentioned in the PR or commit, and no Assisted-by/Generated-by/Co-Authored-By trailers were found.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main change: caching etcd at snapshot time to avoid live pull-secret usage during recert.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@cluster-tool`:
- Line 802: Validate the YAML-derived etcd_image at the call into cache_image
using a strict image-reference allow-list, rejecting unsafe values before any
SSH command executes. Update cache_image to shell-quote the validated image
wherever it builds podman image exists, podman pull, and podman create commands,
or use argv-based execution consistently throughout.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 6780a82a-57b8-4e69-82b7-7b3354c242fb

📥 Commits

Reviewing files that changed from the base of the PR and between 91c625a and c36f9d3.

📒 Files selected for processing (2)

• cluster-tool
• test_cluster_tool.py

[coderabbitai]

🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win

Validate and shell-quote the detected etcd image.

Line 802 passes YAML-derived data into cache_image, which interpolates it into podman image exists, podman pull, and podman create shell commands. A crafted image value could execute arbitrary commands in the SSH session.

Validate the image reference with a strict allow-list and use shell quoting or argv-based execution throughout cache_image.

As per path instructions, validate at trust boundaries with allow-lists and prevent command injection.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cluster-tool` at line 802, Validate the YAML-derived etcd_image at the call
into cache_image using a strict image-reference allow-list, rejecting unsafe
values before any SSH command executes. Update cache_image to shell-quote the
validated image wherever it builds podman image exists, podman pull, and podman
create commands, or use argv-based execution consistently throughout.

Source: Path instructions