ROX-29003: Add ACS smoke test#423
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds ACS smoke-test automation in GitHub Actions and Bash: workflows derive inputs and run cluster-based smoke tests, while shared helpers and a test script handle install, upgrade, polling, reset, cleanup, and reporting. ChangesACS smoke test automation
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
.github/workflows/auto-smoke-test.yml (1)
1-115: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAdd an explicit
permissions:block.No
permissions:is set for this workflow, soderive-inputsruns with default (often broad)GITHUB_TOKENscope, and the flagged token is what's used for thegit diff/checkout in this job.As per static analysis hints (zizmor `excessive-permissions`, lines 1-115, 26-104).🔒 Proposed fix
name: "Auto Smoke Test" + +permissions: + contents: read on:🤖 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/auto-smoke-test.yml around lines 1 - 115, Add an explicit permissions block for this workflow so the default GITHUB_TOKEN is not over-scoped. Update the workflow near the top of the file and ensure the derive-inputs job only has the minimum access needed for actions/checkout and the git diff logic, rather than relying on the broad default token permissions. Use the workflow-level permissions setting to lock this down.Source: Linters/SAST tools
.github/workflows/acs-smoke-test.yml (1)
1-43: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAdd an explicit
permissions:block.No
permissions:is declared, so the workflow runs with the (often broad) defaultGITHUB_TOKENscope for every job, includingrun-smoke-testswhich also holdsGH_TOKEN: ${{ github.token }}in env. Scope this down to least privilege (e.g.contents: read), overriding per-job if a step genuinely needs more.🔒 Proposed fix
name: "ACS Smoke Test" + +permissions: + contents: read on:🤖 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/acs-smoke-test.yml around lines 1 - 43, Add an explicit permissions block to the ACS Smoke Test workflow so the default GITHUB_TOKEN scope is restricted to least privilege instead of inheriting broad access. Update the workflow-level configuration near the on/workflow_call and env sections, and keep any job-specific elevation only where a step truly needs it; the run-smoke-tests job should continue using GH_TOKEN from github.token but with the workflow permissions narrowed, such as contents: read.smoke-test/upgrade-latest-test.sh (1)
32-42: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winGraceful-skip claim doesn't hold —
wait_for_cataloghard-exits the whole script.The comment says a catalog timeout should degrade gracefully, but
wait_for_catalogis invoked directly in theifcondition (not in a subshell). Sincewait_for_catalogcallsfail(), which doesexit 1(lib.shline 11), a timeout kills the entire script instead of just falling through to theelsebranch. The&&short-circuit never gets a chance to evaluate to false because the process exits first.Wrap the call in a subshell so its internal
exitonly terminates the subshell, letting theif/elselogic work as intended:🐛 Proposed fix
-if wait_for_catalog "redhat-operators" "openshift-marketplace" 180 && +if ( wait_for_catalog "redhat-operators" "openshift-marketplace" 180 ) && latest=$(get_latest_official_minor "$ACS_MAJOR") && [[ -n "$latest" ]]; then🤖 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 `@smoke-test/upgrade-latest-test.sh` around lines 32 - 42, The graceful-skip path in `upgrade-latest-test.sh` does not work because `wait_for_catalog` can call `fail()` and exit the whole script before the `if` can fall through. Update the `if` condition around `wait_for_catalog`, `get_latest_official_minor`, and the `LATEST_MINOR` assignment so the catalog check runs in a subshell and only affects the condition result. Keep the existing `info`/`warn` branches, but ensure a timeout or catalog failure does not terminate the script and instead reaches the upgrade-skip `else` path.
🧹 Nitpick comments (3)
.github/workflows/auto-smoke-test.yml (2)
34-37: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSet
persist-credentials: falseon checkout.
fetch-depth: 0checkout persists theGITHUB_TOKENcredential by default; this job doesn't push anything, so disable persistence for defense-in-depth.As per static analysis hints (zizmor `artipacked`, lines 34-37).🔒 Proposed fix
- name: Check out code uses: actions/checkout@v7 with: fetch-depth: 0 + persist-credentials: false🤖 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/auto-smoke-test.yml around lines 34 - 37, The checkout step in the workflow currently leaves the default GitHub token credentials persisted, which is unnecessary for this read-only job. Update the actions/checkout usage in the “Check out code” step to disable credential persistence by setting persist-credentials to false alongside the existing fetch-depth setting. This is the only change needed in the checkout configuration.Source: Linters/SAST tools
106-114: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider scoping
secrets: inheritto only what's needed.
secrets: inheritpasses every repo/org secret to the called workflow, which only needsGCP_RELEASE_AUTOMATION_SA. Passing it explicitly viasecrets:would narrow the blast radius if this workflow is ever triggered from less-trusted contexts.
As per static analysis hints (zizmorsecrets-inherit, line 110).🤖 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/auto-smoke-test.yml around lines 106 - 114, The reusable workflow invocation in smoke-test currently uses secrets: inherit, which exposes all available secrets to the called workflow. Replace it with an explicit secrets mapping that passes only the secret actually needed by acs-smoke-test.yml, using the smoke-test job’s uses block and secrets configuration to narrow access.Source: Linters/SAST tools
.github/workflows/acs-smoke-test.yml (1)
71-72: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider
persist-credentials: falseon checkout.The container job checks out the repo but never pushes; persisting the token on disk is unnecessary residual exposure, especially combined with the missing
permissions:block above.🔒 Proposed fix
- name: Check out code uses: actions/checkout@v7 + with: + persist-credentials: false🤖 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/acs-smoke-test.yml around lines 71 - 72, The checkout step in the ACS smoke test workflow leaves Git credentials on disk even though the container job does not push; update the `actions/checkout` usage to disable credential persistence, and make sure the workflow’s job-level `permissions` are explicitly scoped as tightly as possible. Use the `Check out code` step in `acs-smoke-test.yml` and the surrounding job definition to apply the security hardening consistently.
🤖 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/auto-smoke-test.yml:
- Around line 72-105: The Determine operator-index image step is interpolating
inputs.operator-index-image-override directly inside the shell script, which
should be avoided. Move the override value into an env entry for this step and
read it from the environment in the run block instead of embedding the input in
the script. Keep the existing logic in the image step and update the
operator-index-image-override check to use the env-backed value so the rest of
the loop and GITHUB_OUTPUT handling stays unchanged.
- Around line 54-58: Update the NEW_VERSIONS extraction in the auto-smoke-test
workflow so it only matches indented bundle version entries, not any added key
containing version:. The current grep in the bundle diff parsing can
accidentally pick up oldest_supported_version lines; tighten the pattern in the
version-filtering pipeline (the shell snippet that uses git diff, grep, sed, and
sort) to match only lines with leading indentation before version so
support-policy updates are ignored.
- Around line 39-70: The Determine ACS version step is interpolating user/input
and GitHub context directly into the shell script, so move the acs-version
override and base ref values into env variables instead of embedding `${{
inputs.acs-version-override }}` and `${{ github.base_ref }}` in the run block.
Update the workflow step to read those values inside the script as
`$ACS_VERSION_OVERRIDE` and `$BASE_REF`, and keep the existing logic in the
version-determination flow unchanged.
---
Outside diff comments:
In @.github/workflows/acs-smoke-test.yml:
- Around line 1-43: Add an explicit permissions block to the ACS Smoke Test
workflow so the default GITHUB_TOKEN scope is restricted to least privilege
instead of inheriting broad access. Update the workflow-level configuration near
the on/workflow_call and env sections, and keep any job-specific elevation only
where a step truly needs it; the run-smoke-tests job should continue using
GH_TOKEN from github.token but with the workflow permissions narrowed, such as
contents: read.
In @.github/workflows/auto-smoke-test.yml:
- Around line 1-115: Add an explicit permissions block for this workflow so the
default GITHUB_TOKEN is not over-scoped. Update the workflow near the top of the
file and ensure the derive-inputs job only has the minimum access needed for
actions/checkout and the git diff logic, rather than relying on the broad
default token permissions. Use the workflow-level permissions setting to lock
this down.
In `@smoke-test/upgrade-latest-test.sh`:
- Around line 32-42: The graceful-skip path in `upgrade-latest-test.sh` does not
work because `wait_for_catalog` can call `fail()` and exit the whole script
before the `if` can fall through. Update the `if` condition around
`wait_for_catalog`, `get_latest_official_minor`, and the `LATEST_MINOR`
assignment so the catalog check runs in a subshell and only affects the
condition result. Keep the existing `info`/`warn` branches, but ensure a timeout
or catalog failure does not terminate the script and instead reaches the
upgrade-skip `else` path.
---
Nitpick comments:
In @.github/workflows/acs-smoke-test.yml:
- Around line 71-72: The checkout step in the ACS smoke test workflow leaves Git
credentials on disk even though the container job does not push; update the
`actions/checkout` usage to disable credential persistence, and make sure the
workflow’s job-level `permissions` are explicitly scoped as tightly as possible.
Use the `Check out code` step in `acs-smoke-test.yml` and the surrounding job
definition to apply the security hardening consistently.
In @.github/workflows/auto-smoke-test.yml:
- Around line 34-37: The checkout step in the workflow currently leaves the
default GitHub token credentials persisted, which is unnecessary for this
read-only job. Update the actions/checkout usage in the “Check out code” step to
disable credential persistence by setting persist-credentials to false alongside
the existing fetch-depth setting. This is the only change needed in the checkout
configuration.
- Around line 106-114: The reusable workflow invocation in smoke-test currently
uses secrets: inherit, which exposes all available secrets to the called
workflow. Replace it with an explicit secrets mapping that passes only the
secret actually needed by acs-smoke-test.yml, using the smoke-test job’s uses
block and secrets configuration to narrow access.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: e72ae90a-9989-401c-a338-0777f2c3706a
📒 Files selected for processing (4)
.github/workflows/acs-smoke-test.yml.github/workflows/auto-smoke-test.ymlsmoke-test/lib.shsmoke-test/upgrade-latest-test.sh
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/workflows/auto-smoke-test.yml (1)
57-58: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
github.base_refstill interpolated directly into shell.The
acs-version-overrideandoperator-index-image-overrideinterpolations were correctly moved intoenv:, butgithub.base_refat Line 58 is still expanded directly inside therun:script — the exact concern raised previously and still flagged by static analysis.🔒 Proposed fix
- name: Determine ACS version id: version env: ACS_VERSION_OVERRIDE: ${{ inputs.acs-version-override }} OPERATOR_INDEX_IMAGE_OVERRIDE: ${{ inputs.operator-index-image-override }} + BASE_REF: ${{ github.base_ref }} run: | ... if [ "${{ github.event_name }}" = "pull_request" ]; then - DIFF_BASE="origin/${{ github.base_ref }}" + DIFF_BASE="origin/$BASE_REF"🤖 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/auto-smoke-test.yml around lines 57 - 58, The pull_request branch logic in the auto-smoke-test workflow still interpolates github.base_ref directly inside the run script, so move that value into env like the other overrides and reference the environment variable from the shell instead. Update the DIFF_BASE assignment in the workflow job that contains the pull_request check so the shell only reads a safe env var rather than expanding github.base_ref inline.Source: Linters/SAST tools
🤖 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.
Duplicate comments:
In @.github/workflows/auto-smoke-test.yml:
- Around line 57-58: The pull_request branch logic in the auto-smoke-test
workflow still interpolates github.base_ref directly inside the run script, so
move that value into env like the other overrides and reference the environment
variable from the shell instead. Update the DIFF_BASE assignment in the workflow
job that contains the pull_request check so the shell only reads a safe env var
rather than expanding github.base_ref inline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 92759a7c-bf3e-4980-a585-8e3df4768a04
📒 Files selected for processing (1)
.github/workflows/auto-smoke-test.yml
1 participant
Add an automated ACS smoke test running on GHA for PRs which add a new ACS version(s) to the bundles.yaml file (e.g. #427).
GH actions files:
acs-smoke-test.yml- setups infra cluster and triggers smoke test for the provided operator-index image and ACS version.This workflow is tested in this PR 🟢
auto-smoke-test- derives ACS version(s) from diff of PR which updatesbundles.yaml. Also it waits for the Konflux pipeline success check and once it's green and checks the operator-index image. I intentionally decided to wait for check status instead of waiting for image because it allows fail faster (check failed). Then it builds a test matrix of ACS version(s) and operator-index image (the image is the same no matter how many ACS versions was added to the bundles.yaml file).This workflow is NOT tested in this PR
Smoke test
Each smoke test runs on the single infra cluster.
The test consists of two scenarios: