oran-o2ims: add tls scanner - #82154
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
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 service selectors and TLS secret names
CIJob->>OpenShiftAPI: Wait for selected pods to become Ready
CIJob->>OpenShiftAPI: Verify referenced TLS secrets
🚥 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
Signed-off-by: Raul Hernandez <rauherna@redhat.com>
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 |
- Add a wait-for-server-pods inline step to the install-bundle-tls-scan test that dynamically discovers TLS-serving services via the service.beta.openshift.io/serving-cert-secret-name annotation, waits for their backing pods to become Ready, and verifies that service-ca TLS secrets exist before the tls-scanner runs. This avoids hardcoding server names. - Move operator.substitutions under the bundle item where it belongs, making the pullspec override explicitly scoped to the operator-bundle.
3ce5266 to
0d5f3d0
Compare
|
/retest |
|
/pj-rehearse auto-ack |
|
@rauhersu: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/retest |
|
/pj-rehearse auto-ack |
|
@rauhersu: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse pull-ci-openshift-kni-oran-o2ims-main-install-bundle-tls-scan |
|
@rauhersu: 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: rauhersu 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ci-operator/config/openshift-kni/oran-o2ims/openshift-kni-oran-o2ims-main.yaml (1)
121-128: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFail the job when prerequisites or TLS secrets are missing.
The Inventory wait can time out, pod readiness failures are logged as warnings, and missing service-ca secrets are also warnings. The step therefore proceeds—and may succeed—even when the operator is not ready or TLS material is absent. Track these failures and
exit 1after reporting diagnostics.Also applies to: 154-176
🤖 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/config/openshift-kni/oran-o2ims/openshift-kni-oran-o2ims-main.yaml` around lines 121 - 128, Update the deployment readiness checks around the Inventory wait loop and the pod/service-ca validation blocks to track whether each prerequisite succeeds, retain the existing diagnostic output, and exit 1 after the checks when Inventory times out, pods are not ready, or required TLS secrets are missing. Ensure the workflow cannot proceed successfully when any of these conditions fails.
🤖 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/tls/scanner/run/tls-scanner-run-commands.sh`:
- Around line 9-16: Track namespace ownership from whether SCANNER_NAMESPACE was
explicitly provided, rather than comparing NAMESPACE to "tls-scanner", so an
explicit tls-scanner value is treated as existing. Update the scan resources and
cleanup logic to use unique per-scan pod and RBAC names, ensuring deletion only
targets resources created by this scan and never a fixed shared pod.
- Around line 9-16: Update the scanner setup and cleanup flow in
tls-scanner-run-commands.sh so SCANNER_NAMESPACE never grants cluster-admin to
the target namespace’s default service account. Create and use a dedicated
scanner service account, bind only the required temporary permissions to it, and
ensure the corresponding binding and service account are removed during cleanup.
---
Outside diff comments:
In
`@ci-operator/config/openshift-kni/oran-o2ims/openshift-kni-oran-o2ims-main.yaml`:
- Around line 121-128: Update the deployment readiness checks around the
Inventory wait loop and the pod/service-ca validation blocks to track whether
each prerequisite succeeds, retain the existing diagnostic output, and exit 1
after the checks when Inventory times out, pods are not ready, or required TLS
secrets are missing. Ensure the workflow cannot proceed successfully when any of
these conditions fails.
🪄 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: Pro Plus
Run ID: b806d116-31a5-4666-9280-ae0200145acb
📒 Files selected for processing (3)
ci-operator/config/openshift-kni/oran-o2ims/openshift-kni-oran-o2ims-main.yamlci-operator/step-registry/tls/scanner/run/tls-scanner-run-commands.shci-operator/step-registry/tls/scanner/run/tls-scanner-run-ref.yaml
| local NAMESPACE="${SCANNER_NAMESPACE:-tls-scanner}" | ||
| local OWNS_NAMESPACE=true | ||
| # When SCANNER_NAMESPACE is set, deploy into an existing namespace (do not | ||
| # create/delete it). This allows the scanner to satisfy same-namespace | ||
| # NetworkPolicy rules on the target pods. | ||
| if [[ "${NAMESPACE}" != "tls-scanner" ]]; then | ||
| OWNS_NAMESPACE=false | ||
| fi |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Scope cleanup to resources owned by this scan.
Existing-namespace mode deletes the fixed pod/tls-scanner, which can collide with an unrelated workload or another concurrent scan. Also, SCANNER_NAMESPACE=tls-scanner is incorrectly treated as an owned namespace because ownership is inferred from the name. Track ownership explicitly and use unique pod/RBAC names.
🤖 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/tls/scanner/run/tls-scanner-run-commands.sh` around
lines 9 - 16, Track namespace ownership from whether SCANNER_NAMESPACE was
explicitly provided, rather than comparing NAMESPACE to "tls-scanner", so an
explicit tls-scanner value is treated as existing. Update the scan resources and
cleanup logic to use unique per-scan pod and RBAC names, ensuring deletion only
targets resources created by this scan and never a fixed shared pod.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not grant cluster-admin to the target namespace’s default service account.
When SCANNER_NAMESPACE is set, Line 145 still binds cluster-admin to default; the binding remains after cleanup, and any workload using that service account gains cluster-admin privileges. Use a dedicated scanner service account with narrowly scoped permissions and remove its temporary binding during cleanup.
🤖 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/tls/scanner/run/tls-scanner-run-commands.sh` around
lines 9 - 16, Update the scanner setup and cleanup flow in
tls-scanner-run-commands.sh so SCANNER_NAMESPACE never grants cluster-admin to
the target namespace’s default service account. Create and use a dedicated
scanner service account, bind only the required temporary permissions to it, and
ensure the corresponding binding and service account are removed during cleanup.
|
I am experimenting with a SCANNER_NAMESPACE env var, added to the tls-scanner-run ref, allowing the scanner pod to be deployed into the target namespace. This pattern already exists in the ref for HyperShift management-cluster scans (OWNS_NAMESPACE=false). Previous to these changes, the scanner was producing false positives for our oran-o2ims: the TLS scanner runs in a dedicated tls-scanner namespace, but oran-o2ims pods have NetworkPolicies that only allow same-namespace ingress. The scanner discovers listening ports via oc exec into each pod (which bypasses NetworkPolicy), but the TLS handshake probe runs from the scanner pod itself and gets blocked by the target pods' NetworkPolicies. I will analyse logs again when I have a successful execution with these new changes and will ask tls scanner folks to review them too. |
…space The scanner always runs in a dedicated tls-scanner namespace. When scanning pods with NetworkPolicies restricting ingress to same-namespace traffic (e.g. oran-o2ims servers), the TLS handshake probes are blocked, producing false NO_TLS results. Add a SCANNER_NAMESPACE env var that deploys the scanner pod into an existing namespace instead, reusing the OWNS_NAMESPACE=false pattern already used by the HyperShift management-cluster path. Set SCANNER_NAMESPACE=oran-o2ims in the install-bundle-tls-scan test so the scanner satisfies the oran-o2ims NetworkPolicy rules.
80dafa9 to
c1f60d9
Compare
|
[REHEARSALNOTIFIER]
A total of 80 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: |
|
/pj-rehearse auto-ack |
|
@rauhersu: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
@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. |
3 participants
Summary
Adds TLS scanner CI integration for
openshift-kni/oran-o2ims.The new
install-bundle-tls-scanjob:ipi-awsworkflow.operator-sdk run bundlevia theoptional-operators-operator-sdkstep-registry ref.InventoryCR on startup, which triggers the reconciler to deploy all server pods.tls-scanner-runstep-registry ref to scan all TLS endpoints in theoran-o2imsnamespace.The job is configured as
always_run: falseandoptional: true, so it does not run automatically on every PR and does not block merges. It can be triggered on demand with/test install-bundle-tls-scan.Changes
ci-operator/config/openshift-kni/oran-o2ims/openshift-kni-oran-o2ims-main.yaml: Addedtls-scanner-toolbase image and theinstall-bundle-tls-scantest definition.ci-operator/jobs/openshift-kni/oran-o2ims/openshift-kni-oran-o2ims-main-presubmits.yaml: Auto-generated bymake jobs.References
Summary by CodeRabbit
Adds optional TLS scanner CI integration for the
openshift-kni/oran-o2imscomponent in the OpenShift CI configuration (ci-operator/config/openshift-kni/oran-o2ims/openshift-kni-oran-o2ims-main.yaml).tls-scanner-toolbase image entry used by the TLS scanning step.install-bundle-tls-scan(always_run: false,optional: true, triggerable via/test install-bundle-tls-scan) that:oran-o2imsnamespace (viaoperator-sdk run bundle),inventory/defaultCR,service.beta.openshift.io/serving-cert-secret-name, derives pod label selectors from each Service’sspec.selector, and waits up to 5 minutes for matching pods to becomeReady(logging pod status/events on timeout),oran-o2imsnamespace,tls-scanner-run/workflow: ipi-aws) against the discovered endpoints.tls-scanner-run-ref.yamladdsSCANNER_NAMESPACE(defaults to empty).tls-scanner-run-commands.shderives the scannerNAMESPACEfromSCANNER_NAMESPACE(defaulting totls-scanner), and disables “owns namespace” behavior wheneverSCANNER_NAMESPACEis explicitly set—so the scanner can targetoran-o2imsinstead of creating/using a separate namespace.