INTEROP-9337: Harden OPP upgrade script and re-add rhacs-operator - #82920
INTEROP-9337: Harden OPP upgrade script and re-add rhacs-operator#82920amp-rh wants to merge 4 commits into
Conversation
|
@amp-rh: GitHub didn't allow me to request PR reviews from the following users: mpruitt. Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
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:
WalkthroughThe PR updates OPP preflight reporting, upgrade control, cluster health validation, and RHACS operator configuration. It replaces ChangesOPP upgrade flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Mergeability Score: 🟡 Moderate · up to The scripts improve upgrade and preflight behavior, but the current revision can report healthy conditions when health queries fail and may expose proxy or registry credentials through command tracing. These bounded correctness and security risks should be fixed or explicitly accepted before merge. Suggested labels: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 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: 2
🧹 Nitpick comments (1)
ci-operator/step-registry/interop/opp/upgrade/interop-opp-upgrade-commands.sh (1)
18-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument both tracing-disabled blocks.
Add a comment before each
set +x. State why the following operation must not run under tracing.
ci-operator/step-registry/interop/opp/upgrade/interop-opp-upgrade-commands.sh#L18-L20: Document that proxy configuration can contain sensitive proxy values.ci-operator/step-registry/interop/opp/upgrade/interop-opp-upgrade-commands.sh#L72-L74: Document why registry login must not be traced.As per coding guidelines, “keep the tracing-disabled scope minimal, add clear comments when disabling tracing.”
🤖 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 `@ci-operator/step-registry/interop/opp/upgrade/interop-opp-upgrade-commands.sh` around lines 18 - 20, Add a clear comment before the tracing-disabled block at ci-operator/step-registry/interop/opp/upgrade/interop-opp-upgrade-commands.sh lines 18-20 explaining that sourced proxy configuration may contain sensitive values; also add a comment before lines 72-74 explaining why registry login credentials or related data must not be traced. Keep each set +x/set -x scope minimal and make no other changes.Source: Coding guidelines
🤖 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
`@ci-operator/step-registry/interop/opp/upgrade/interop-opp-upgrade-commands.sh`:
- Around line 129-130: Make both required oc queries fail the step instead of
masking errors: in AdminAck, remove the fallback that converts the admin-gates
ConfigMap query failure into an empty gates result, and return failure when the
query cannot complete; in ValidateOppOperators, ensure the oc get pods command
failure is treated as validation failure rather than an empty healthy pod list.
Apply these changes at interop-opp-upgrade-commands.sh lines 129-130 and
393-394.
- Line 70: Update the trap declaration around DebugOnExit so EXIT and TERM use
separate handlers. Preserve the existing EXIT behavior, but make the TERM
handler set exitCode to 143, invoke DebugOnExit, and explicitly exit so
interrupted upgrade commands do not resume after wait.
---
Nitpick comments:
In
`@ci-operator/step-registry/interop/opp/upgrade/interop-opp-upgrade-commands.sh`:
- Around line 18-20: Add a clear comment before the tracing-disabled block at
ci-operator/step-registry/interop/opp/upgrade/interop-opp-upgrade-commands.sh
lines 18-20 explaining that sourced proxy configuration may contain sensitive
values; also add a comment before lines 72-74 explaining why registry login
credentials or related data must not be traced. Keep each set +x/set -x scope
minimal and make no other changes.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: e47e5193-c022-4e64-ac5a-a2903ca81f0a
📒 Files selected for processing (2)
ci-operator/config/stolostron/policy-collection/stolostron-policy-collection-main__ocp5.0-upgrade.yamlci-operator/step-registry/interop/opp/upgrade/interop-opp-upgrade-commands.sh
|
/pj-rehearse auto-ack |
|
@amp-rh: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
00b0992 to
3fca514
Compare
amp-rh
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@amp-rh: This pull request references INTEROP-9337 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/pj-rehearse auto-ack |
|
@amp-rh: |
|
@amp-rh: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: amp-rh The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
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
`@ci-operator/step-registry/interop/opp/preflight/interop-opp-preflight-commands.sh`:
- Around line 275-288: Update the alert-query parsing in the preflight command
so query failures are not converted into an empty alert list. Validate the
response status and require the expected payload shape before processing; report
unavailable queries separately, skip only malformed individual alerts, and
preserve valid firing alert names. Remove the broad exception suppression and
adjust the surrounding `|| true`/“No critical alerts firing” flow so it cannot
claim no alerts when the query failed.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 8394267f-74fc-4daa-bc6d-6bc3063a3a3e
📒 Files selected for processing (3)
ci-operator/config/stolostron/policy-collection/stolostron-policy-collection-main__ocp5.0-upgrade.yamlci-operator/step-registry/interop/opp/preflight/interop-opp-preflight-commands.shci-operator/step-registry/interop/opp/upgrade/interop-opp-upgrade-commands.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- ci-operator/step-registry/interop/opp/upgrade/interop-opp-upgrade-commands.sh
|
/pj-rehearse |
|
@amp-rh: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
@amp-rh: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
PR Readiness Review — Ready for OCP Team ReviewVerified this PR against INTEROP-9337 acceptance criteria and the parent epic INTEROP-8942 (Phase 4 — CI infrastructure hardening). Acceptance Criteria VerificationAll 6 outstanding AC items from INTEROP-9337 are addressed by this PR:
Technical Assessment
Notes
This PR is ready for OCP team review. AI-generated. Review for accuracy. |
|
Chai Bot Review Summary (Gate 2: Approach Validation) Chai Bot reviewed the PR. Verdict: Approved with no findings. |
88fe065 to
e0953a0
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
e0953a0 to
ab77bc1
Compare
There was a problem hiding this comment.
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
`@ci-operator/step-registry/interop/opp/upgrade/interop-opp-upgrade-commands.sh`:
- Line 2: Update the script’s shell options from set -eux -o pipefail to set
-euo pipefail to disable global xtrace. Replace status-only : commands with a
printf-based logger, remove redundant tracing toggles, and ensure raw oc error
output is not logged; document any remaining intentionally protected tracing
scope.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: a716536e-8320-4363-8631-5f3aa3448433
📒 Files selected for processing (4)
ci-operator/config/stolostron/policy-collection/stolostron-policy-collection-main__ocp4.22-upgrade.yamlci-operator/config/stolostron/policy-collection/stolostron-policy-collection-main__ocp5.0-upgrade.yamlci-operator/step-registry/interop/opp/preflight/interop-opp-preflight-commands.shci-operator/step-registry/interop/opp/upgrade/interop-opp-upgrade-commands.sh
🚧 Files skipped from review as they are similar to previous changes (3)
- ci-operator/config/stolostron/policy-collection/stolostron-policy-collection-main__ocp4.22-upgrade.yaml
- ci-operator/step-registry/interop/opp/preflight/interop-opp-preflight-commands.sh
- ci-operator/config/stolostron/policy-collection/stolostron-policy-collection-main__ocp5.0-upgrade.yaml
| @@ -1,5 +1,5 @@ | |||
| #!/bin/bash | |||
| set -euxo pipefail | |||
| set -eux -o pipefail | |||
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
bash -c '
set -x
captured="$(printf "%s\n" "https://cluster.example:6443")"
: "captured=${captured}"
' 2>&1Repository: openshift/release
Length of output: 283
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="ci-operator/step-registry/interop/opp/upgrade/interop-opp-upgrade-commands.sh"
printf '%s\n' '--- relevant source ---'
cat -n "$file" | sed -n '1,30p;65,85p;120,145p'
printf '%s\n' '--- tracing and status patterns ---'
rg -n 'set [+-]x|oc registry login|2>&1|:[[:space:]]|AdminAck|gates' "$file"Repository: openshift/release
Length of output: 9015
Disable global xtrace before running this step.
set -eux -o pipefail traces expanded values. When oc ... 2>&1 fails, ${gates} is inserted into a traced : command, so the error can expose cluster or proxy endpoints.
Start with set -euo pipefail. Replace status : commands with a printf-based logger. Do not log raw oc errors. Remove redundant tracing toggles, and document any remaining protected tracing scope.
🤖 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
`@ci-operator/step-registry/interop/opp/upgrade/interop-opp-upgrade-commands.sh`
at line 2, Update the script’s shell options from set -eux -o pipefail to set
-euo pipefail to disable global xtrace. Replace status-only : commands with a
printf-based logger, remove redundant tracing toggles, and ensure raw oc error
output is not logged; document any remaining intentionally protected tracing
scope.
Source: Coding guidelines
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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
`@ci-operator/step-registry/interop/opp/preflight/interop-opp-preflight-commands.sh`:
- Around line 14-16: Add a short comment before the set +x at
ci-operator/step-registry/interop/opp/preflight/interop-opp-preflight-commands.sh
lines 14-16 explaining that sourcing proxy-conf.sh may define proxy credentials;
also add a short comment before the set +x at lines 375-377 explaining that
registry login processes credentials. Keep both tracing-disabled scopes minimal
and preserve the corresponding set -x restoration.
- Around line 238-248: Update the ClusterOperator health check around
unhealthyCo so a failed oc get co command increments failed and records the
query failure before evaluating whether unhealthyCo is empty. Remove the
unconditional failure suppression from the command substitution, and ensure the
“all healthy” branch runs only when the query succeeds.
Apply the same fix in
`@ci-operator/step-registry/interop/opp/preflight/interop-opp-preflight-commands.sh`
around lines 266 - 292: The alert-query failure has the same error-suppression
pattern and remediation.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: f0936dc6-16ab-405f-be05-03d4fbe3d5db
📒 Files selected for processing (4)
ci-operator/config/stolostron/policy-collection/stolostron-policy-collection-main__ocp4.22-upgrade.yamlci-operator/config/stolostron/policy-collection/stolostron-policy-collection-main__ocp5.0-upgrade.yamlci-operator/step-registry/interop/opp/preflight/interop-opp-preflight-commands.shci-operator/step-registry/interop/opp/upgrade/interop-opp-upgrade-commands.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- ci-operator/config/stolostron/policy-collection/stolostron-policy-collection-main__ocp5.0-upgrade.yaml
- ci-operator/config/stolostron/policy-collection/stolostron-policy-collection-main__ocp4.22-upgrade.yaml
| set +x | ||
| source "${SHARED_DIR}/proxy-conf.sh" | ||
| set -x |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document both tracing-disabled scopes.
Add a short comment before each set +x. This identifies why tracing is disabled and protects these sensitive boundaries during future edits.
ci-operator/step-registry/interop/opp/preflight/interop-opp-preflight-commands.sh#L14-L16: State thatproxy-conf.shcan define proxy credentials.ci-operator/step-registry/interop/opp/preflight/interop-opp-preflight-commands.sh#L375-L377: State that registry login can process credentials.
As per coding guidelines, “keep the tracing-disabled scope minimal, add clear comments when disabling tracing.” Based on learnings, xtrace requires careful handling when a script processes credentials.
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 15-15: Not following: ./proxy-conf.sh was not specified as input (see shellcheck -x).
(SC1091)
📍 Affects 1 file
ci-operator/step-registry/interop/opp/preflight/interop-opp-preflight-commands.sh#L14-L16(this comment)ci-operator/step-registry/interop/opp/preflight/interop-opp-preflight-commands.sh#L375-L377
🤖 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
`@ci-operator/step-registry/interop/opp/preflight/interop-opp-preflight-commands.sh`
around lines 14 - 16, Add a short comment before the set +x at
ci-operator/step-registry/interop/opp/preflight/interop-opp-preflight-commands.sh
lines 14-16 explaining that sourcing proxy-conf.sh may define proxy credentials;
also add a short comment before the set +x at lines 375-377 explaining that
registry login processes credentials. Keep both tracing-disabled scopes minimal
and preserve the corresponding set -x restoration.
Sources: Coding guidelines, Learnings
|
/cc @sg-rh |
- Install rhacs-operator into openshift-operators namespace (global AllNamespaces OperatorGroup) instead of custom namespace. rhacs-operator does not support OwnNamespace InstallModeType. - Re-add rhacs-operator to 5.0 upgrade config (available since July 31). - Set OPP_OPERATORS explicitly for 5.0 (excludes odf-operator, not yet in catalog).
- Replace all jq calls with go-template, jsonpath, and awk (jq is not available in the CI base image). - Wrap proxy-conf.sh sourcing in set +x/set -x to prevent credential leakage in CI logs. - Fix trap handler: separate EXIT and TERM with brace-group form for errexit safety and correct exit code propagation. - Replace eval+statCmd with direct conditional execution. - Add StabilizeCluster failure diagnostics. - Use typeset -g instead of export for script-scoped variables.
- Add set -eux -o pipefail + shopt -s inherit_errexit for xtrace and strict error handling. - Add function keyword to all declarations (typeset scoping). - Fix TERM trap double-fire with separate EXIT/TERM brace-group handlers. - Add proxy-conf.sh sourcing with set +x privacy guards. - Replace jq with python3 for JSON manipulation. - Preserve health-query failures instead of reporting "all healthy" when the underlying oc command fails. - Validate alert API response status before processing. - Convert echo progress lines to : markers (xtrace-friendly).
567773a to
d3c8405
Compare
Detect and report when the initial oc get machineconfigpools command fails instead of silently falling through to "all 0 MCPs healthy". Reuse the captured output for the machine count mismatch check.
|
[REHEARSALNOTIFIER]
Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
3 participants
TL;DR
Fixes rhacs-operator install failures on OPP 4.22/5.0 upgrade jobs and hardens both CI scripts for reliability. 3 commits, 4 files, no new dependencies.
What changed
openshift-operatorsnamespace (global AllNamespaces OperatorGroup) instead of custom namespace that fails withUnsupportedOperatorGroupOPP_OPERATORSenv var (odf-operator excluded until ~Nov 2026)inherit_errexit; validate alert API responses; use:markers for xtrace-friendly outputCore files (review these first)
ci-operator/config/.../stolostron-policy-collection-main__ocp4.22-upgrade.yamlci-operator/config/.../stolostron-policy-collection-main__ocp5.0-upgrade.yamlStart with the 2 YAML configs (3 net lines changed). This is the only behavioral change: rhacs-operator namespace moves to
openshift-operatorsand gets re-added to 5.0.Mechanical/hardening files
ci-operator/step-registry/interop/opp/upgrade/interop-opp-upgrade-commands.shci-operator/step-registry/interop/opp/preflight/interop-opp-preflight-commands.shNo behavior change in these files beyond replacing unavailable
jqwith available alternatives and improving error handling.Risk areas
rhacs-operatornamespace toopenshift-operators. This matches the operator's supported InstallModeType (AllNamespacesonly) and is the same pattern quay-operator uses.jqnode/CO filtering with go-template equivalents. Logic is identical; output format may differ slightly in debug diagnostics (no downstream consumers).Testing
oc get packagemanifest rhacs-operator -o jsonpath='{.status.channels[*].currentCSVDesc.installModes}'shellcheck(CI gate validates this)Root cause
rhacs-operator does not support OwnNamespace InstallModeType. Installing into a custom namespace causes OLM to fail with
UnsupportedOperatorGroup. Fix: useopenshift-operators(global AllNamespaces OperatorGroup), matching quay-operator's pattern.Context
Jira
INTEROP-9337