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
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.
|
/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
|
/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. |
When the scanner pod fails (e.g. TLS compliance violations), the script exits before reaching the trap unregistration at the end of run_tls_scan. The EXIT trap fires the cleanup function, but bash unwinds the function call stack before running the trap handler. Since NAMESPACE and OWNS_NAMESPACE were declared local to run_tls_scan, they no longer exist in the trap handler's scope, causing a fatal "unbound variable" error under set -o nounset. Fix by removing the local qualifier from both variables. They are safe as globals: the non-HyperShift path calls the function once, and the HyperShift path runs each invocation in a subshell that isolates variable state. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Raúl Hernández <rauherna@redhat.com>
bf2e28c to
e42884d
Compare
|
[REHEARSALNOTIFIER]
A total of 102 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 infrastructure configuration (ci-operator/config/openshift-kni/oran-o2ims/openshift-kni-oran-o2ims-main.yaml).tls-scanner-toolbase image entry used by the TLS scanning workflow.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-13workflow step before executingtls-scanner-run.tls-scanner-run-ref.yamladdsSCANNER_NAMESPACE(empty by default); when set, the scanner uses the provided namespace rather than a dedicatedtls-scannernamespace.tls-scanner-run-commands.shderives the scannerNAMESPACEfromSCANNER_NAMESPACEand disables “owns namespace” behavior wheneverSCANNER_NAMESPACEis explicitly set.