ci-config: integrate tls-scanner into fips-check-node-scan in parallel#79625
ci-config: integrate tls-scanner into fips-check-node-scan in parallel#79625redhat-chai-bot wants to merge 2 commits into
Conversation
Modify the fips-check-node-scan step to launch the TLS scanner pod early and run it in parallel with the existing FIPS node scan. The TLS scanner deploys a privileged pod that enumerates all cluster pod TLS ports and validates cipher suites and protocol versions. Changes: - fips-check-node-scan-commands.sh: add start_tls_scanner() and collect_tls_scanner_results() functions that bracket the existing node scan logic. The scanner pod is launched before the node scan and its artifacts are collected after the node scan completes. - fips-check-node-scan-ref.yaml: add tls-scanner-tool dependency (PULL_SPEC_TLS_SCANNER_TOOL), increase timeout to 4h30m, update documentation. - openshift-release-main__nightly-4.22.yaml: add tls-scanner-tool base image from tls-scanner/tls-scanner. The TLS scanner exit code is intentionally ignored (informational only), matching the existing tls-scanner-run step behavior. If PULL_SPEC_TLS_SCANNER_TOOL is not set, the TLS scan is skipped gracefully.
|
@redhat-chai-bot: GitHub didn't allow me to request PR reviews from the following users: openshift/tls-scanner-maintainers. 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. |
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR adds parallel TLS scanner execution to the FIPS node scan workflow. A new base image entry registers the scanner container, the step definition declares the dependency and extends timeout to 4.5 hours, and the script implements scanner pod provisioning and asynchronous artifact collection across all exit paths. ChangesTLS Scanner Parallel Execution
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
openshift-ci
Bot
added
the
needs-ok-to-test
|
Hi @redhat-chai-bot. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: redhat-chai-bot 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 |
![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
🤖 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/fips-check/node-scan/fips-check-node-scan-commands.sh`:
- Around line 79-81: The script currently touches /results/scan.done then
unconditionally sleeps 120 (touch /results/scan.done and sleep 120), which can
let the container exit before the external collector runs oc cp; replace the
fixed sleep with a wait loop that keeps the container running until an external
signal file is present (e.g. replace sleep 120 with: while [ ! -f
/results/collect.done ]; do sleep 5; done) so the pod remains Running for oc cp;
ensure the collector writes /results/collect.done after copying so the loop can
exit.
- Around line 43-47: The startup currently aborts on early `oc` failures and
skips cleanup when the pod never becomes Ready; fix start_tls_scanner() by
making namespace creation best-effort: after running `oc create namespace ... |
oc apply -f -` set a new flag (e.g., TLS_SCANNER_NS_CREATED=true) immediately
even if later `oc` commands fail, avoid letting `oc` failures before this point
trigger an overall exit, and keep setting TLS_SCANNER_STARTED only when the pod
is Ready; add an EXIT trap that always runs cleanup to delete ${TLS_SCANNER_NS}
(and optionally remove the cluster-admin role and privileged SCC bindings)
regardless of TLS_SCANNER_STARTED, and update collect_tls_scanner_results() to
check TLS_SCANNER_NS_CREATED (not TLS_SCANNER_STARTED) to ensure namespace
deletion and RBAC/SCC rollback happen even if the scanner never became Ready.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 1d988ccb-de1e-4428-921c-abea843b7d38
📒 Files selected for processing (3)
ci-operator/config/openshift/release/openshift-release-main__nightly-4.22.yamlci-operator/step-registry/fips-check/node-scan/fips-check-node-scan-commands.shci-operator/step-registry/fips-check/node-scan/fips-check-node-scan-ref.yaml
1. Cleanup leak: Introduce TLS_SCANNER_NS_CREATED flag and an EXIT trap (cleanup_tls_scanner) that always deletes the namespace, regardless of whether the pod became Ready. Make namespace creation and RBAC/SCC grants best-effort so failures skip the TLS scan instead of aborting the entire step. 2. oc cp flakiness: Replace the fixed 'sleep 120' in the scanner pod with a wait loop for /results/collect.done. The collector signals the pod after oc cp completes, so the container stays Running for as long as needed and exits cleanly afterward.
|
[REHEARSALNOTIFIER]
A total of 2693 jobs have been affected by this change. The above listing is non-exhaustive and limited to 25 jobs. A full list of affected jobs can be found here Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
|
Closing in favor of a new approach: creating a dedicated release controller informing job using the existing tls-scanner-run step ref, per feedback from the requester. |
1 participant
Summary
Integrate the TLS scanner directly into the
fips-check-node-scanstep so it runs in parallel with the existing FIPS node scan on the same cluster.Supersedes #79624 (closed).
Changes
fips-check-node-scan-commands.shstart_tls_scanner()— launches a privileged TLS scanner pod (namespacetls-scanner, withcluster-adminandprivilegedSCC) before the existing node scan begins.collect_tls_scanner_results()— waits for the scanner to finish, copies artifacts (results.json,results.csv,junit_tls_scan.xml,scan.log) into${ARTIFACT_DIR}/tls-scanner/, and cleans up the namespace.PULL_SPEC_TLS_SCANNER_TOOLis not set, the TLS scan is skipped gracefully.tls-scanner-runstep behavior.fips-check-node-scan-ref.yamltls-scanner-tooldependency (PULL_SPEC_TLS_SCANNER_TOOLenv var).4h30mto accommodate the parallel TLS scan.openshift-release-main__nightly-4.22.yamltls-scanner-toolbase image (tls-scanner/tls-scanner:tls-scanner-tool).Context
Requested in Slack thread.
/cc @openshift/tls-scanner-maintainers