oran-o2ims: add tls scanner - #82154
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:
WalkthroughThe ORAN O2IMS CI configuration adds a TLS scanner image and optional validation job. The scanner now accepts an existing namespace through ChangesORAN O2IMS TLS validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant CIJob
participant OperatorBundle
participant OpenShiftAPI
CIJob->>OperatorBundle: Install operator-bundle in oran-o2ims
CIJob->>OpenShiftAPI: Wait for Inventory and discover annotated services
OpenShiftAPI-->>CIJob: Return pod selectors and TLS secret names
CIJob->>OpenShiftAPI: Wait for selected pods to become Ready
CIJob->>OpenShiftAPI: Verify referenced TLS secrets
CIJob->>CIJob: Run tls-13 and tls-scanner-run
🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 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 |
openshift-ci
Bot
added
the
needs-ok-to-test
|
Hi @rauhersu. 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. |
|
@donpenney , may you run an ok-to-test ? |
|
/ok-to-test |
openshift-ci
Bot
added
ok-to-test
|
/retest |
1 similar comment
|
/retest |
|
/test ci-operator-config |
|
/test generated-config |
|
/pj-rehearse auto-ack |
|
@rauhersu: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
openshift-merge-bot
Bot
added
the
rehearsals-ack
|
The new TLS scanner - Example for this execution: After this PR merges, then on any PR to openshift-kni/oran-o2ims, someone can trigger it with @Missxiaoguo @donpenney , I would need a lgtm+approve if you agree with these changes. |
|
Since one of my PRs is referenced here - if you are looking for TLS Adherence test this setup won't do. If you're looking for plain compliance check it's fine. I later found out that I missed it in the AWS tls scan job referenced here and had to amend it to get the scanner check TLS adherence which is what I was looking for. In other words |
|
Need to assess the new @RomanBednar comment. Thanks, Roman ! /hold |
openshift-ci
Bot
added
the
do-not-merge/hold
a6c6c37 to
3ce5266
Compare
openshift-merge-bot
Bot
removed
the
rehearsals-ack
|
/pj-rehearse auto-ack |
|
@rauhersu: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
3ce5266 to
0d5f3d0
Compare
|
/retest |
|
/pj-rehearse abort |
|
@rauhersu: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
alegacy
left a comment
alegacy
left a comment
There was a problem hiding this comment.
[Comment authored by Claude]
TLS Scanner CI Review
The overall approach is solid — on-demand + periodic TLS scanning with PQC checks is valuable. The SCANNER_NAMESPACE extension to the shared step-registry ref is backward compatible and well-designed.
A few issues to address before merging, detailed in inline comments below.
Issue not addressable inline: cluster-admin granted to default SA, never cleaned up
In tls-scanner-run-commands.sh line 154:
oc adm policy add-cluster-role-to-user cluster-admin -z default -n "${NAMESPACE}"When SCANNER_NAMESPACE=oran-o2ims, this grants cluster-admin to the default service account in the oran-o2ims namespace. The cleanup function (line 119) only deletes the scanner pod — it never removes the ClusterRoleBinding. Every pod using the default SA in that namespace gains cluster-admin for the remaining lifetime of the cluster.
In CI ephemeral clusters this is mitigated by cluster teardown, but it's still a risky pattern — the binding is live during the entire scan phase. Consider:
- Creating a dedicated scanner SA instead of using
default - Removing the
ClusterRoleBindingin the cleanup trap
CodeRabbit flagged this as well and it appears unaddressed in the latest commits.
| TLS_13_TLS_ADHERENCE_POLICY: StrictAllComponents | ||
| test: | ||
| - ref: optional-operators-operator-sdk | ||
| - as: wait-for-server-pods |
There was a problem hiding this comment.
Medium: ~65-line inline script duplicated 4 times
This wait-for-server-pods commands block is copy-pasted identically across:
- This on-demand job
- The periodic job below (line ~220)
release-4.22.yamlon-demand jobrelease-4.22.yamlperiodic job
Any bug fix (like adding the missing exit 1 from the other comments) would need to be applied in 4 places. Consider extracting this into a step-registry ref (e.g., oran-o2ims-wait-for-tls-pods-ref.yaml) that all 4 jobs reference.
There was a problem hiding this comment.
[Comment authored by Claude]
Agreed. Tracked as CNF-26477 and addressed in a follow-up PR: #83194, which extracts the script into a step-registry ref at oran-o2ims/wait-for-tls-pods. We'd prefer not to block this PR on the refactoring — the error handling fixes are already applied to all 4 copies here, and the dedup PR depends on this one merging first.
|
Addressing comments /hold |
openshift-ci
Bot
added
the
do-not-merge/hold
- Fail the step if the Inventory CR is not created within 5 minutes instead of silently falling through to the scanner - Fail the step if any TLS service pod does not become ready - Fail the step if any TLS serving cert secret is missing - Switch pipe-based while loops to here-strings to avoid subshell scoping issues with failure tracking Applies to all 4 wait-for-server-pods script copies (on-demand and periodic jobs for main and release-4.22). Script deduplication tracked in CNF-26477. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Raúl Hernández <rauherna@redhat.com>
|
/pj-rehearse auto-ack |
|
@rauhersu: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
openshift-merge-bot
Bot
removed
the
rehearsals-ack
|
[REHEARSALNOTIFIER]
A total of 135 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: |
|
/lgtm |
|
Ready for review again /unhold |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
@rauhersu: 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. |
openshift-merge-bot
Bot
added
the
rehearsals-ack
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alegacy, rauhersu, smith-xyz The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
/test config |
1 similar comment
|
/test config |
355ce76
into
openshift:main
6 participants
Summary
Adds TLS scanner CI integration for
openshift-kni/oran-o2imson main and release-4.22 branches.The
install-bundle-tls-scanjob (on-demand) andinstall-bundle-tls-scan-periodicjob (cron-scheduled):ipi-awsworkflow.operator-sdk run bundlevia theoptional-operators-operator-sdkstep-registry ref.InventoryCR and all TLS-serving pods to become ready.TLSAdherence: StrictAllComponentsvia thetls-13step.tls-scanner-runstep to scan all TLS endpoints in theoran-o2imsnamespace.PQC_CHECK=true).On-demand jobs
Configured as
always_run: falseandoptional: true. Triggered with/test install-bundle-tls-scanon PRs to the respective branches.Periodic jobs
0 3 * * 5)0 3 * * 1)Changes
ci-operator/config/openshift-kni/oran-o2ims/openshift-kni-oran-o2ims-main.yaml: Addedtls-scanner-toolbase image,install-bundle-tls-scan(on-demand) andinstall-bundle-tls-scan-periodictest definitions with PQC check enabled.ci-operator/config/openshift-kni/oran-o2ims/openshift-kni-oran-o2ims-release-4.22.yaml: Same additions for the release-4.22 branch.ci-operator/jobs/: Auto-generated presubmit and periodic job files bymake update.ci-operator/step-registry/tls/scanner/run/tls-scanner-run-ref.yaml: AddedSCANNER_NAMESPACEenv var to support deploying the scanner into an existing namespace.ci-operator/step-registry/tls/scanner/run/tls-scanner-run-commands.sh: AddedSCANNER_NAMESPACElogic.References