Add automated snapshot build/validate/publish pipeline - #38
Add automated snapshot build/validate/publish pipeline#38omer-vishlitzky wants to merge 6 commits into
Conversation
Upload Python scripts that automate the full OSAC snapshot flavor pipeline: boot base SNO, install OSAC components via Helm, validate cluster health, strip credentials, snapshot VM, and push to OCI registry. Scripts support a SNAPSHOT_TAG env var for CI candidate tagging — when set, the push step uses this tag instead of the flavor name. Jira: OSAC-3352
On every merge to main, rebuild all snapshot flavors (base SNO, VMaaS, CaaS) with updated cluster-tool code, validate each via E2E tests, and publish validated snapshots to the OCI registry. Base SNO validation calls osac-test-infra full-install E2E reusable workflows (VMaaS + CaaS suites) against the candidate snapshot. VMaaS/CaaS validation boots from the local flavor, runs refresh-after-snapshot, then the matching E2E suite. Candidate snapshots are pushed under a temporary tag and promoted to the production tag only after validation passes. Also supports manual dispatch for rebuilding individual VMaaS/CaaS flavors with a configurable base flavor image. Jira: OSAC-3353, OSAC-854, OSAC-868, OSAC-3354, OSAC-3355, OSAC-3357 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: osac-project/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds shared Python snapshot orchestration, base/CaaS/VMaaS entrypoints, kubelet configuration, and a GitHub Actions workflow that builds, validates, promotes, and cleans up SNO, VMaaS, and CaaS snapshots. ChangesSnapshot pipeline
Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: ⚪ Minimal · up to No actionable merge-blocking risk remains based on the supplied evidence; the PR is merge-ready after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant Workflow as Rebuild Snapshots workflow
participant ClusterTool as cluster-tool
participant Scripts as Snapshot scripts
participant E2E as VMaaS and CaaS E2E workflows
participant Registry as Snapshot registry
Workflow->>ClusterTool: Configure and boot build cluster
Workflow->>Scripts: Run snapshot creation
Scripts->>Registry: Publish candidate image
Workflow->>E2E: Validate candidate image
E2E-->>Workflow: Return validation results
Workflow->>Registry: Promote successful image
Workflow->>ClusterTool: Destroy temporary clusters
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (3 errors, 2 warnings)
✅ Passed checks (6 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (7)
README.md (1)
395-403: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument
SNAPSHOT_TAGandFLAVOR_NAME.Both variables exist in the code and both are set by
.github/workflows/rebuild-snapshots.yaml, but neither appears in this table.SNAPSHOT_TAGis how the workflow publishes candidate tags instead of production tags, so an operator reproducing a CI run needs it.📝 Proposed addition
| `SOURCE` | *(required for base)* | Clone ID of the running SNO instance | | `KUBECONFIG` | *(required for base)* | Path to kubeconfig | +| `FLAVOR_NAME` | `sno-4-22` | Snapshot name (base script only) | +| `SNAPSHOT_TAG` | *(flavor name)* | Registry tag for the push step |🤖 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 `@README.md` around lines 395 - 403, Update the README environment-variable table to document both SNAPSHOT_TAG and FLAVOR_NAME, including their defaults or required status and concise descriptions consistent with the existing entries; ensure SNAPSHOT_TAG’s role in publishing candidate snapshot tags is clear for CI reproduction..github/workflows/rebuild-snapshots.yaml (3)
134-142: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPin the reusable workflows and register the custom runner label.
Two points on this block and its twin at Line 147:
@mainis a mutable ref. These reusable workflows decide whether a snapshot reaches the production tag, so a change inosac-test-infrasilently changes the gate. Pin them by commit SHA, in the same way theactions/checkoutandhashicorp/vault-actionsteps are pinned.- actionlint reports
clustertool-snapshotas an unknown label at Lines 33, 161, 202, and 380. Declare it inactionlint.yamlunderself-hosted-runner.labelsto silence the four errors.As per path instructions: "Pin actions by full SHA, not tag".
🤖 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 @.github/workflows/rebuild-snapshots.yaml around lines 134 - 142, The reusable workflow references in both blocks should be pinned to immutable full commit SHAs instead of `@main`; update the e2e-vmaas-full-install workflow references while preserving their inputs. Also update actionlint.yaml under self-hosted-runner.labels to register clustertool-snapshot.Sources: Path instructions, Linters/SAST tools
368-375: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDelete the candidate tags after promotion.
The cleanup step removes clones, the local podman image, and temp files, but no step removes
${REGISTRY}:vmaas-4-22-candidate-<run_id>from the registry. Every push and every dispatch leaves one multi-gigabyte tag behind, in this job and inbuild-and-validate-caasat Line 546, plussno-4-22-candidate-<run_id>frombuild-base-sno. Add askopeo deletefor the candidate tag in thealways()cleanup, or configure a retention policy on the quay repository for the*-candidate-*tag pattern.🤖 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 @.github/workflows/rebuild-snapshots.yaml around lines 368 - 375, Add registry cleanup to the always-running Cleanup step by deleting the promoted vmaas-4-22 candidate tag with skopeo, using the same registry, run ID, and authentication context as the push. Apply equivalent cleanup to the candidate tags produced by build-and-validate-caas and build-base-sno, including the vmaas-4-22 and sno-4-22 tag variants.
73-77: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winValidate the merged auth file before use.
If either Vault secret lacks an
authskey,jqemits{"auths":null}and exits 0. The failure then surfaces much later as an opaque registry authentication error during push. Add a check after the merge, for examplejq -e '.auths | length > 0' "${HOME}/.config/containers/auth.json" > /dev/null. The same block repeats at Lines 251-255 and Lines 429-433.🤖 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 @.github/workflows/rebuild-snapshots.yaml around lines 73 - 77, Validate the merged auth file immediately after each jq merge in the rebuild workflow, including the repeated blocks near the other merge commands. Use jq in exit-status mode to require a non-empty .auths object, redirecting normal output, and fail the workflow before chmod or registry operations when validation fails.scripts/snapshot_lib.py (3)
314-337: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReplace the fixed sleeps with polling.
The three
time.sleepcalls add 7 minutes to every run and still do not guarantee readiness. Poll the target condition with a deadline instead, for exampleoc rollout statusfor the network operator DaemonSet andoc wait --for=condition=...for the Helm-installed workloads. The health checks at step 5 already express the desired end state, so a sharedwait_until(predicate, timeout)helper can replace the sleeps.🤖 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 `@scripts/snapshot_lib.py` around lines 314 - 337, The fixed time.sleep calls in the setup flow should be replaced with deadline-based polling. Add or reuse a shared wait_until(predicate, timeout) helper, use rollout/status and condition checks for OVN, the MachineConfigPool, and Helm-installed workloads, and preserve the existing readiness checks and timeout behavior while removing all three fixed sleeps.
202-211: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winGuard the index-based CSV patch with
testops.The patch paths use array indices from an earlier read. If OLM rewrites
spec.install.spec.deploymentsbetween the read and the patch, the wrong deployment is scaled to zero. JSON Patch supports atestop, so the API server rejects the patch when the index no longer holds the expected name.♻️ Proposed refactor
patch: list[dict] = [ - {"op": "replace", - "path": f"/spec/install/spec/deployments/{i}/spec/replicas", - "value": 0} - for i in range(len(deploys)) + op + for i, d in enumerate(deploys) + for op in ( + {"op": "test", + "path": f"/spec/install/spec/deployments/{i}/name", + "value": d["name"]}, + {"op": "replace", + "path": f"/spec/install/spec/deployments/{i}/spec/replicas", + "value": 0}, + ) ]🤖 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 `@scripts/snapshot_lib.py` around lines 202 - 211, Update the patch construction around csv_data and deploys to prepend a JSON Patch test operation for each indexed deployment, validating that its current name matches the name read from that deployment before the existing replicas replace operation. Keep the index-based replace paths and zero-replica behavior unchanged, and ensure the API rejects the patch when an index no longer contains the expected deployment.
95-99: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRaise exceptions instead of calling
sys.exitin library code.
snapshot_lib.pyis imported by three entrypoints.sys.exit(1)here and at Lines 119, 130, 197, 243, and 248 terminates the caller from inside a helper. Raise a dedicated exception (for exampleSnapshotError) and let each entrypoint decide the exit code. This also makes the health checks testable.🤖 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 `@scripts/snapshot_lib.py` around lines 95 - 99, Replace the sys.exit calls in snapshot_lib.py, including the failure paths around the health checks and the referenced lines, with a dedicated SnapshotError exception. Define or reuse SnapshotError consistently, include the existing failure context in each raised exception, and update the three entrypoints to catch it and choose the appropriate exit code.
🤖 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 @.github/workflows/rebuild-snapshots.yaml:
- Around line 200-203: The flavor validation jobs build against stale or
partially rewritten base images and can concurrently corrupt shared server
state. Update build-and-validate-vmaas and build-and-validate-caas to depend on
promote-base-sno for push runs, ensure each pulls the promoted base tag before
building, and assign each job a distinct server-scoped concurrency group so
their state.json usage is serialized; preserve dispatch-specific behavior.
- Around line 39-40: Disable checkout credential persistence for every checkout
step: add persist-credentials: false to the Checkout step in
.github/workflows/rebuild-snapshots.yaml lines 39-40 and to all three checkout
steps in the build-and-validate-vmaas block at lines 205-218 and the
build-and-validate-caas block at lines 383-396.
- Around line 191-197: Add a root-owned permission hardening step after the sudo
cp in the snapshot registry authentication setup: update the commands near
skopeo copy to run sudo chmod 600 on /root/.config/containers/auth.json,
matching the existing permission behavior used elsewhere.
- Around line 44-50: Mask ROLE_ID with the GitHub Actions add-mask command
before writing it to GITHUB_OUTPUT, matching the existing SECRET_ID handling.
Apply this change to all three repeated AppRole credential blocks, including the
copies near the other referenced sections, and preserve the existing output
names and vault-action inputs.
- Around line 284-295: The workflow interpolates the untrusted base flavor image
directly into shell commands. In .github/workflows/rebuild-snapshots.yaml lines
284-295, update the “Determine base flavor” and “Pull base flavor (dispatch)”
steps to pass the input through env BASE_FLAVOR_IMAGE, use the quoted variable
in shell commands, and validate it with the specified allow-list before sudo
./cluster-tool pull; apply the identical change in lines 462-473 for the CaaS
job.
In `@scripts/snapshot_base.py`:
- Around line 40-52: Consolidate the duplicated pull-secret stripping logic: in
scripts/snapshot_base.py lines 40-52, delete the local
strip_cluster_pull_secret() and import the shared function from snapshot_lib; in
scripts/snapshot_lib.py lines 232-243, expose strip_cluster_pull_secret(), add
--ignore-not-found to its pull-secret delete operation, and update
strip_credentials() to call it.
In `@scripts/snapshot_lib.py`:
- Around line 353-355: Post-change MCO waits can succeed against the previous
Updated state; add a shared wait_for_mcp_rollout(pool) helper in
scripts/snapshot_lib.py that first waits for Updating=True, then waits for
Updated, and call it at scripts/snapshot_lib.py:353-355 after strip_credentials,
scripts/snapshot_lib.py:321-324 after KUBELETCONFIG apply (removing the 180s
sleep), scripts/snapshot_base.py:71-73 after strip_cluster_pull_secret, and
scripts/snapshot_base.py:63-66 after kubeletconfig apply.
- Around line 106-115: Update the crash-check loop in the pod status processing
to inspect both status.containerStatuses and status.initContainerStatuses. Apply
the existing restart-count and waiting-reason checks to init containers as well,
while preserving the current crashing message format and handling missing status
fields safely.
---
Nitpick comments:
In @.github/workflows/rebuild-snapshots.yaml:
- Around line 134-142: The reusable workflow references in both blocks should be
pinned to immutable full commit SHAs instead of `@main`; update the
e2e-vmaas-full-install workflow references while preserving their inputs. Also
update actionlint.yaml under self-hosted-runner.labels to register
clustertool-snapshot.
- Around line 368-375: Add registry cleanup to the always-running Cleanup step
by deleting the promoted vmaas-4-22 candidate tag with skopeo, using the same
registry, run ID, and authentication context as the push. Apply equivalent
cleanup to the candidate tags produced by build-and-validate-caas and
build-base-sno, including the vmaas-4-22 and sno-4-22 tag variants.
- Around line 73-77: Validate the merged auth file immediately after each jq
merge in the rebuild workflow, including the repeated blocks near the other
merge commands. Use jq in exit-status mode to require a non-empty .auths object,
redirecting normal output, and fail the workflow before chmod or registry
operations when validation fails.
In `@README.md`:
- Around line 395-403: Update the README environment-variable table to document
both SNAPSHOT_TAG and FLAVOR_NAME, including their defaults or required status
and concise descriptions consistent with the existing entries; ensure
SNAPSHOT_TAG’s role in publishing candidate snapshot tags is clear for CI
reproduction.
In `@scripts/snapshot_lib.py`:
- Around line 314-337: The fixed time.sleep calls in the setup flow should be
replaced with deadline-based polling. Add or reuse a shared
wait_until(predicate, timeout) helper, use rollout/status and condition checks
for OVN, the MachineConfigPool, and Helm-installed workloads, and preserve the
existing readiness checks and timeout behavior while removing all three fixed
sleeps.
- Around line 202-211: Update the patch construction around csv_data and deploys
to prepend a JSON Patch test operation for each indexed deployment, validating
that its current name matches the name read from that deployment before the
existing replicas replace operation. Keep the index-based replace paths and
zero-replica behavior unchanged, and ensure the API rejects the patch when an
index no longer contains the expected deployment.
- Around line 95-99: Replace the sys.exit calls in snapshot_lib.py, including
the failure paths around the health checks and the referenced lines, with a
dedicated SnapshotError exception. Define or reuse SnapshotError consistently,
include the existing failure context in each raised exception, and update the
three entrypoints to catch it and choose the appropriate exit code.
🪄 Autofix
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: CHILL
Plan: Pro Plus
Run ID: d91fee09-7056-458c-83f3-6bff68c3d84a
📒 Files selected for processing (7)
.github/workflows/rebuild-snapshots.yamlREADME.mdscripts/kubeletconfig.yamlscripts/snapshot_base.pyscripts/snapshot_caas.pyscripts/snapshot_lib.pyscripts/snapshot_vmaas.py
| run: | | ||
| APPROLE_DIR="${HOME}/.vault-server/.approle" | ||
| ROLE_ID="$(cat "${APPROLE_DIR}/role-id")" | ||
| SECRET_ID="$(cat "${APPROLE_DIR}/secret-id")" | ||
| echo "role-id=${ROLE_ID}" >> "$GITHUB_OUTPUT" | ||
| echo "::add-mask::${SECRET_ID}" | ||
| echo "secret-id=${SECRET_ID}" >> "$GITHUB_OUTPUT" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Mask the AppRole role ID as well.
The step masks SECRET_ID but writes ROLE_ID unmasked into $GITHUB_OUTPUT. The role ID is half of the AppRole credential, and it is then interpolated into the vault-action inputs. Mask both halves. The same block repeats at Lines 166-172 and Lines 400-406, and the three copies are a good candidate for a small composite action.
🔒 Proposed fix
ROLE_ID="$(cat "${APPROLE_DIR}/role-id")"
SECRET_ID="$(cat "${APPROLE_DIR}/secret-id")"
+ echo "::add-mask::${ROLE_ID}"
echo "role-id=${ROLE_ID}" >> "$GITHUB_OUTPUT"
echo "::add-mask::${SECRET_ID}"
echo "secret-id=${SECRET_ID}" >> "$GITHUB_OUTPUT"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| run: | | |
| APPROLE_DIR="${HOME}/.vault-server/.approle" | |
| ROLE_ID="$(cat "${APPROLE_DIR}/role-id")" | |
| SECRET_ID="$(cat "${APPROLE_DIR}/secret-id")" | |
| echo "role-id=${ROLE_ID}" >> "$GITHUB_OUTPUT" | |
| echo "::add-mask::${SECRET_ID}" | |
| echo "secret-id=${SECRET_ID}" >> "$GITHUB_OUTPUT" | |
| run: | | |
| APPROLE_DIR="${HOME}/.vault-server/.approle" | |
| ROLE_ID="$(cat "${APPROLE_DIR}/role-id")" | |
| SECRET_ID="$(cat "${APPROLE_DIR}/secret-id")" | |
| echo "::add-mask::${ROLE_ID}" | |
| echo "role-id=${ROLE_ID}" >> "$GITHUB_OUTPUT" | |
| echo "::add-mask::${SECRET_ID}" | |
| echo "secret-id=${SECRET_ID}" >> "$GITHUB_OUTPUT" |
🤖 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 @.github/workflows/rebuild-snapshots.yaml around lines 44 - 50, Mask ROLE_ID
with the GitHub Actions add-mask command before writing it to GITHUB_OUTPUT,
matching the existing SECRET_ID handling. Apply this change to all three
repeated AppRole credential blocks, including the copies near the other
referenced sections, and preserve the existing output names and vault-action
inputs.
Source: Path instructions
| mkdir -p "${HOME}/.config/containers" | ||
| printf '%s' "${SNAPSHOT_REGISTRY_AUTH}" > "${HOME}/.config/containers/auth.json" | ||
| chmod 600 "${HOME}/.config/containers/auth.json" | ||
| sudo mkdir -p /root/.config/containers | ||
| sudo cp "${HOME}/.config/containers/auth.json" /root/.config/containers/auth.json | ||
| skopeo copy "docker://${CANDIDATE_IMAGE}" \ | ||
| "docker://${REGISTRY}:sno-4-22" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Set the permissions on the root copy of auth.json.
Lines 257-258 and Lines 435-436 follow the sudo cp with sudo chmod 600. This job omits it, so the registry credential lands in /root/.config/containers/auth.json with the umask default. Add the same sudo chmod 600 here.
🔒 Proposed fix
sudo mkdir -p /root/.config/containers
sudo cp "${HOME}/.config/containers/auth.json" /root/.config/containers/auth.json
+ sudo chmod 600 /root/.config/containers/auth.json
skopeo copy "docker://${CANDIDATE_IMAGE}" \📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| mkdir -p "${HOME}/.config/containers" | |
| printf '%s' "${SNAPSHOT_REGISTRY_AUTH}" > "${HOME}/.config/containers/auth.json" | |
| chmod 600 "${HOME}/.config/containers/auth.json" | |
| sudo mkdir -p /root/.config/containers | |
| sudo cp "${HOME}/.config/containers/auth.json" /root/.config/containers/auth.json | |
| skopeo copy "docker://${CANDIDATE_IMAGE}" \ | |
| "docker://${REGISTRY}:sno-4-22" | |
| mkdir -p "${HOME}/.config/containers" | |
| printf '%s' "${SNAPSHOT_REGISTRY_AUTH}" > "${HOME}/.config/containers/auth.json" | |
| chmod 600 "${HOME}/.config/containers/auth.json" | |
| sudo mkdir -p /root/.config/containers | |
| sudo cp "${HOME}/.config/containers/auth.json" /root/.config/containers/auth.json | |
| sudo chmod 600 /root/.config/containers/auth.json | |
| skopeo copy "docker://${CANDIDATE_IMAGE}" \ | |
| "docker://${REGISTRY}:sno-4-22" |
🤖 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 @.github/workflows/rebuild-snapshots.yaml around lines 191 - 197, Add a
root-owned permission hardening step after the sudo cp in the snapshot registry
authentication setup: update the commands near skopeo copy to run sudo chmod 600
on /root/.config/containers/auth.json, matching the existing permission behavior
used elsewhere.
| build-and-validate-vmaas: | ||
| if: github.event_name == 'push' || inputs.flavor == 'vmaas' | ||
| runs-on: [self-hosted, clustertool-snapshot] | ||
| timeout-minutes: 180 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Order the flavor jobs after base promotion, and serialize the shared runner.
On a push run, build-and-validate-vmaas and build-and-validate-caas declare no needs, so three jobs start at the same time on the same [self-hosted, clustertool-snapshot] runner and the same baremetal server. Two consequences follow:
build-base-snobootsrebuild-baseand, throughscripts/snapshot_base.pyLines 76-77, deletes and rewrites the localsno-4-22flavor. The flavor jobs boot from that same localsno-4-22at that moment. The base flavor can disappear mid-boot.- The "Pull base flavor (dispatch)" step is skipped on
push, so the flavor jobs never consume the candidate thatpromote-base-snopublishes. Each push validates VMaaS and CaaS against the previous base image, which defeats the promotion gate.
Add needs: promote-base-sno to both flavor jobs for the push path, pull the promoted base tag before building, and give each job a distinct concurrency group tied to the server so parallel runs do not share state.json. The same change applies to build-and-validate-caas at Line 378.
🧰 Tools
🪛 actionlint (1.7.12)
[error] 202-202: label "clustertool-snapshot" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2025-vs2026", "windows-2022", "windows-11-arm", "ubuntu-slim", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "macos-latest", "macos-latest-xlarge", "macos-latest-large", "macos-26-intel", "macos-26-xlarge", "macos-26-large", "macos-26", "macos-15-intel", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xlarge", "macos-14-large", "macos-14", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
🤖 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 @.github/workflows/rebuild-snapshots.yaml around lines 200 - 203, The flavor
validation jobs build against stale or partially rewritten base images and can
concurrently corrupt shared server state. Update build-and-validate-vmaas and
build-and-validate-caas to depend on promote-base-sno for push runs, ensure each
pulls the promoted base tag before building, and assign each job a distinct
server-scoped concurrency group so their state.json usage is serialized;
preserve dispatch-specific behavior.
| if [[ "${{ github.event_name }}" == "workflow_dispatch" ]]; then | ||
| IMAGE="${{ inputs.base-flavor-image }}" | ||
| echo "flavor=${IMAGE##*:}" >> "$GITHUB_OUTPUT" | ||
| else | ||
| echo "flavor=sno-4-22" >> "$GITHUB_OUTPUT" | ||
| fi | ||
|
|
||
| - name: Pull base flavor (dispatch) | ||
| if: github.event_name == 'workflow_dispatch' | ||
| run: | | ||
| IMAGE="${{ inputs.base-flavor-image }}" | ||
| sudo ./cluster-tool pull "${IMAGE##*:}" --registry "${IMAGE%:*}" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
inputs.base-flavor-image is expanded into shell commands. Four run blocks interpolate ${{ inputs.base-flavor-image }} directly into bash. GitHub substitutes the value before bash parses the script, so a dispatch value containing shell metacharacters executes on the runner, and one of those blocks runs under sudo. The runner is self-hosted and persistent, so the blast radius covers the cluster-tool state and the registry credentials in /root/.config/containers. Pass the value through env: and quote the variable, then validate it against an allow-list such as ^[A-Za-z0-9._/-]+:[A-Za-z0-9._-]+$.
.github/workflows/rebuild-snapshots.yaml#L284-L295: move the input intoenv: BASE_FLAVOR_IMAGEfor the "Determine base flavor" and "Pull base flavor (dispatch)" steps, reference"${BASE_FLAVOR_IMAGE}", and reject values that fail the allow-list before thesudo ./cluster-tool pullcall..github/workflows/rebuild-snapshots.yaml#L462-L473: apply the identicalenv:indirection and validation in the CaaS job.
As per path instructions: "Agentic CI actions: audit for prompt injection via issue/PR title/body flowing into LLM prompts" and the injection-prevention rule "Validate at trust boundaries with allow-lists, not deny-lists".
🧰 Tools
🪛 zizmor (1.29.0)
[error] 285-285: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[error] 294-294: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
📍 Affects 1 file
.github/workflows/rebuild-snapshots.yaml#L284-L295(this comment).github/workflows/rebuild-snapshots.yaml#L462-L473
🤖 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 @.github/workflows/rebuild-snapshots.yaml around lines 284 - 295, The
workflow interpolates the untrusted base flavor image directly into shell
commands. In .github/workflows/rebuild-snapshots.yaml lines 284-295, update the
“Determine base flavor” and “Pull base flavor (dispatch)” steps to pass the
input through env BASE_FLAVOR_IMAGE, use the quoted variable in shell commands,
and validate it with the specified allow-list before sudo ./cluster-tool pull;
apply the identical change in lines 462-473 for the CaaS job.
Sources: Path instructions, Linters/SAST tools
| def strip_cluster_pull_secret(): | ||
| """Strip only the cluster pull-secret (no OSAC namespace secrets).""" | ||
| print(" Stripping cluster pull-secret auths...") | ||
| oc("delete", "secret", "pull-secret", "-n", "openshift-config") | ||
| oc("create", "secret", "generic", "pull-secret", "-n", "openshift-config", | ||
| '--from-literal=.dockerconfigjson={"auths":{}}', | ||
| "--type=kubernetes.io/dockerconfigjson") | ||
|
|
||
| ps = oc_json("get", "secret", "pull-secret", "-n", "openshift-config") | ||
| auths = json.loads(base64.b64decode(ps["data"][".dockerconfigjson"])) | ||
| if auths.get("auths"): | ||
| sys.exit(f"ERROR: pull-secret still has auths: {list(auths['auths'].keys())}") | ||
| print(" Pull-secret stripped and verified empty") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
The pull-secret strip logic is duplicated, and both copies fail on re-runs. strip_cluster_pull_secret and the middle of strip_credentials perform the identical delete, recreate, decode, and verify sequence. Both call oc delete secret pull-secret -n openshift-config without --ignore-not-found, unlike the neighbouring deletes in strip_credentials, so a re-run after a partial failure aborts on a secret that is already gone.
scripts/snapshot_base.py#L40-L52: delete this function and import a sharedstrip_cluster_pull_secretfromsnapshot_lib.scripts/snapshot_lib.py#L232-L243: extract these lines into a publicstrip_cluster_pull_secret()and add--ignore-not-foundto the delete, then call it fromstrip_credentials.
📍 Affects 2 files
scripts/snapshot_base.py#L40-L52(this comment)scripts/snapshot_lib.py#L232-L243
🤖 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 `@scripts/snapshot_base.py` around lines 40 - 52, Consolidate the duplicated
pull-secret stripping logic: in scripts/snapshot_base.py lines 40-52, delete the
local strip_cluster_pull_secret() and import the shared function from
snapshot_lib; in scripts/snapshot_lib.py lines 232-243, expose
strip_cluster_pull_secret(), add --ignore-not-found to its pull-secret delete
operation, and update strip_credentials() to call it.
| for pod in data["items"]: | ||
| name: str = pod["metadata"]["name"] | ||
| for cs in pod.get("status", {}).get("containerStatuses", []): | ||
| restarts: int = cs.get("restartCount", 0) | ||
| if restarts > 3: | ||
| crashing.append(f" {name}/{cs['name']}: {restarts} restarts") | ||
| waiting: dict = cs.get("state", {}).get("waiting", {}) | ||
| reason: str = waiting.get("reason", "") | ||
| if reason in ("CrashLoopBackOff", "ImagePullBackOff", "ErrImagePull"): | ||
| crashing.append(f" {name}/{cs['name']}: {reason}") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Include init container statuses in the crash check.
A pod stuck in Init:CrashLoopBackOff or Init:ImagePullBackOff reports the failure in status.initContainerStatuses, not containerStatuses. This check passes such a pod, and the snapshot then captures a broken cluster.
🐛 Proposed fix
for pod in data["items"]:
name: str = pod["metadata"]["name"]
- for cs in pod.get("status", {}).get("containerStatuses", []):
+ status: dict = pod.get("status", {})
+ statuses = (
+ status.get("initContainerStatuses", [])
+ + status.get("containerStatuses", [])
+ )
+ for cs in statuses:
restarts: int = cs.get("restartCount", 0)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for pod in data["items"]: | |
| name: str = pod["metadata"]["name"] | |
| for cs in pod.get("status", {}).get("containerStatuses", []): | |
| restarts: int = cs.get("restartCount", 0) | |
| if restarts > 3: | |
| crashing.append(f" {name}/{cs['name']}: {restarts} restarts") | |
| waiting: dict = cs.get("state", {}).get("waiting", {}) | |
| reason: str = waiting.get("reason", "") | |
| if reason in ("CrashLoopBackOff", "ImagePullBackOff", "ErrImagePull"): | |
| crashing.append(f" {name}/{cs['name']}: {reason}") | |
| for pod in data["items"]: | |
| name: str = pod["metadata"]["name"] | |
| status: dict = pod.get("status", {}) | |
| statuses = ( | |
| status.get("initContainerStatuses", []) | |
| status.get("containerStatuses", []) | |
| ) | |
| for cs in statuses: | |
| restarts: int = cs.get("restartCount", 0) | |
| if restarts > 3: | |
| crashing.append(f" {name}/{cs['name']}: {restarts} restarts") | |
| waiting: dict = cs.get("state", {}).get("waiting", {}) | |
| reason: str = waiting.get("reason", "") | |
| if reason in ("CrashLoopBackOff", "ImagePullBackOff", "ErrImagePull"): | |
| crashing.append(f" {name}/{cs['name']}: {reason}") |
🤖 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 `@scripts/snapshot_lib.py` around lines 106 - 115, Update the crash-check loop
in the pod status processing to inspect both status.containerStatuses and
status.initContainerStatuses. Apply the existing restart-count and
waiting-reason checks to init containers as well, while preserving the current
crashing message format and handling missing status fields safely.
| print("[8/10] Waiting for MCO to propagate empty pull-secret to node...") | ||
| oc("wait", "--for=condition=Updated", "mcp/master", "--timeout=600s") | ||
| print(" MCO rollout complete") |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
The mcp/master waits can return before the MachineConfig rollout starts. Every one of these sites changes a resource that triggers a new rendered MachineConfig, then immediately waits for mcp/master condition=Updated. At that instant the pool still reports Updated=True from the previous rollout, so the wait succeeds without the node ever draining. Step 8 then snapshots a node that still holds the original pull-secret, which is exactly the credential leak the strip step exists to prevent.
Wait for the pool to leave the updated state first, then wait for it to return. For example: oc wait --for=condition=Updating=True mcp/master --timeout=120s followed by the existing --for=condition=Updated wait, or compare mcp/master status.configuration.name against the value captured before the change.
scripts/snapshot_lib.py#L353-L355: gate the post-strip_credentialswait on the rollout actually starting before declaring "MCO rollout complete".scripts/snapshot_lib.py#L321-L324: apply the same two-phase wait afteroc apply -f KUBELETCONFIG, and drop the 180s sleep once the wait is reliable.scripts/snapshot_base.py#L71-L73: apply the same two-phase wait afterstrip_cluster_pull_secret.scripts/snapshot_base.py#L63-L66: apply the same two-phase wait after the kubeletconfig apply.
Extract one wait_for_mcp_rollout(pool) helper in scripts/snapshot_lib.py and call it from all four sites.
📍 Affects 2 files
scripts/snapshot_lib.py#L353-L355(this comment)scripts/snapshot_lib.py#L321-L324scripts/snapshot_base.py#L71-L73scripts/snapshot_base.py#L63-L66
🤖 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 `@scripts/snapshot_lib.py` around lines 353 - 355, Post-change MCO waits can
succeed against the previous Updated state; add a shared
wait_for_mcp_rollout(pool) helper in scripts/snapshot_lib.py that first waits
for Updating=True, then waits for Updated, and call it at
scripts/snapshot_lib.py:353-355 after strip_credentials,
scripts/snapshot_lib.py:321-324 after KUBELETCONFIG apply (removing the 180s
sleep), scripts/snapshot_base.py:71-73 after strip_cluster_pull_secret, and
scripts/snapshot_base.py:63-66 after kubeletconfig apply.
Add 30s sleep after kubeletconfig apply in snapshot_base.py to give the MCO time to observe the change before waiting for Updated. Add persist-credentials: false to all checkout steps to prevent GitHub token persistence on self-hosted runners.
On push, build-and-validate-vmaas and build-and-validate-caas now wait for promote-base-sno to succeed before starting. This ensures derivative flavors always build on a validated base snapshot. On dispatch, promote-base-sno is skipped and the dependency is satisfied automatically.
cluster-tool pull takes a positional OCI image ref, not --registry. Snapshot scripts need sudo for libvirt operations. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 participant
Summary
scripts/that automate the full pipeline: boot base SNO, install OSAC components via Helm, validate health, strip credentials, snapshot VM, and push to OCI registryrebuild-snapshots.yamlworkflow that on every merge to main rebuilds all snapshot flavors (base SNO, VMaaS, CaaS) with updated cluster-tool code, validates each via E2E tests, and publishes to quay.ioJira: OSAC-3352, OSAC-3353, OSAC-854, OSAC-868, OSAC-3354, OSAC-3355, OSAC-3357
Supersedes: #34, #35, #36
Test plan
rebuild-snapshotsvia workflow_dispatch for VMaaS flavorquay.io/osac-project/cluster-flavorsSummary by CodeRabbit