Adding TLS scanner job for hypershift#80813
Conversation
|
Skipping CI for Draft Pull Request. |
|
/pj-rehearse periodic-ci-openshift-tls-scanner-main-periodic-hypershift-tls |
|
@richardsonnick: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
WalkthroughAdds a periodic CI job ( ChangesHyperShift TLS Scanning Pipeline
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: richardsonnick 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/tls/scanner/run/tls-scanner-run-commands.sh`:
- Line 7: Remove the unconditional `unset SCAN_NAMESPACE` command on line 7 to
preserve any caller-provided namespace configuration. The current implementation
discards user-configured namespace scoping which changes the scan behavior to
encompass all namespaces. Apply the same fix to the similar occurrences around
lines 42-47 where the same pattern of unsetting SCAN_NAMESPACE appears. Only
unset the variable if it was not explicitly provided by the caller, or remove
the unset commands entirely to respect the caller's configuration.
🪄 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: 6f6be6d9-58b7-4845-9c3b-b38d294369b9
⛔ Files ignored due to path filters (1)
ci-operator/jobs/openshift/tls-scanner/openshift-tls-scanner-main-periodics.yamlis excluded by!ci-operator/jobs/**
📒 Files selected for processing (14)
ci-operator/config/openshift/tls-scanner/openshift-tls-scanner-main.yamlci-operator/step-registry/hypershift/modern-tls/OWNERSci-operator/step-registry/hypershift/modern-tls/hypershift-modern-tls-commands.shci-operator/step-registry/hypershift/modern-tls/hypershift-modern-tls-ref.metadata.jsonci-operator/step-registry/hypershift/modern-tls/hypershift-modern-tls-ref.yamlci-operator/step-registry/tls/scanner/hypershift-aws/OWNERSci-operator/step-registry/tls/scanner/hypershift-aws/tls-scanner-hypershift-aws-workflow.metadata.jsonci-operator/step-registry/tls/scanner/hypershift-aws/tls-scanner-hypershift-aws-workflow.yamlci-operator/step-registry/tls/scanner/hypershift-run/OWNERSci-operator/step-registry/tls/scanner/hypershift-run/tls-scanner-hypershift-run-commands.shci-operator/step-registry/tls/scanner/hypershift-run/tls-scanner-hypershift-run-ref.metadata.jsonci-operator/step-registry/tls/scanner/hypershift-run/tls-scanner-hypershift-run-ref.yamlci-operator/step-registry/tls/scanner/run/tls-scanner-run-commands.shci-operator/step-registry/tls/scanner/run/tls-scanner-run-ref.yaml
ada4144 to
dc0c1be
Compare
|
/pj-rehearse periodic-ci-openshift-tls-scanner-main-periodic-hypershift-tls |
|
@richardsonnick: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ci-operator/step-registry/tls/scanner/run/tls-scanner-run-commands.sh (1)
126-132: 💤 Low valueUnused variable
PRIVILEGED_CONTAINER.The variable is assigned at line 129 but never referenced in the pod spec. The securityContext is set entirely via
SECURITY_CONTEXT_YAMLwhich already includesprivileged: trueinline at line 130.♻️ Remove unused variable
else HOST_NETWORK="true" HOST_PID="true" - PRIVILEGED_CONTAINER="true" SECURITY_CONTEXT_YAML=" privileged: true runAsUser: 0" fi🤖 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 126 - 132, Remove the unused assignment of the PRIVILEGED_CONTAINER variable in the else block. This variable is assigned at line 129 but is never referenced anywhere in the pod spec, and the privileged mode functionality is already provided by the SECURITY_CONTEXT_YAML variable which contains privileged: true. Keep the other variable assignments for HOST_NETWORK, HOST_PID, and SECURITY_CONTEXT_YAML as they are still needed.Source: Linters/SAST tools
🤖 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.
Nitpick comments:
In `@ci-operator/step-registry/tls/scanner/run/tls-scanner-run-commands.sh`:
- Around line 126-132: Remove the unused assignment of the PRIVILEGED_CONTAINER
variable in the else block. This variable is assigned at line 129 but is never
referenced anywhere in the pod spec, and the privileged mode functionality is
already provided by the SECURITY_CONTEXT_YAML variable which contains
privileged: true. Keep the other variable assignments for HOST_NETWORK,
HOST_PID, and SECURITY_CONTEXT_YAML as they are still needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: e342ea11-4a72-4805-97ea-14e6401b528b
⛔ Files ignored due to path filters (1)
ci-operator/jobs/openshift/tls-scanner/openshift-tls-scanner-main-periodics.yamlis excluded by!ci-operator/jobs/**
📒 Files selected for processing (14)
ci-operator/config/openshift/tls-scanner/openshift-tls-scanner-main.yamlci-operator/step-registry/hypershift/modern-tls/OWNERSci-operator/step-registry/hypershift/modern-tls/hypershift-modern-tls-commands.shci-operator/step-registry/hypershift/modern-tls/hypershift-modern-tls-ref.metadata.jsonci-operator/step-registry/hypershift/modern-tls/hypershift-modern-tls-ref.yamlci-operator/step-registry/tls/scanner/hypershift-aws/OWNERSci-operator/step-registry/tls/scanner/hypershift-aws/tls-scanner-hypershift-aws-workflow.metadata.jsonci-operator/step-registry/tls/scanner/hypershift-aws/tls-scanner-hypershift-aws-workflow.yamlci-operator/step-registry/tls/scanner/hypershift-run/OWNERSci-operator/step-registry/tls/scanner/hypershift-run/tls-scanner-hypershift-run-commands.shci-operator/step-registry/tls/scanner/hypershift-run/tls-scanner-hypershift-run-ref.metadata.jsonci-operator/step-registry/tls/scanner/hypershift-run/tls-scanner-hypershift-run-ref.yamlci-operator/step-registry/tls/scanner/run/tls-scanner-run-commands.shci-operator/step-registry/tls/scanner/run/tls-scanner-run-ref.yaml
✅ Files skipped from review due to trivial changes (4)
- ci-operator/step-registry/tls/scanner/hypershift-aws/tls-scanner-hypershift-aws-workflow.metadata.json
- ci-operator/step-registry/tls/scanner/hypershift-aws/OWNERS
- ci-operator/step-registry/tls/scanner/hypershift-run/tls-scanner-hypershift-run-ref.metadata.json
- ci-operator/step-registry/hypershift/modern-tls/OWNERS
🚧 Files skipped from review as they are similar to previous changes (9)
- ci-operator/step-registry/tls/scanner/hypershift-run/tls-scanner-hypershift-run-commands.sh
- ci-operator/step-registry/tls/scanner/hypershift-aws/tls-scanner-hypershift-aws-workflow.yaml
- ci-operator/step-registry/hypershift/modern-tls/hypershift-modern-tls-ref.metadata.json
- ci-operator/step-registry/tls/scanner/hypershift-run/OWNERS
- ci-operator/step-registry/tls/scanner/hypershift-run/tls-scanner-hypershift-run-ref.yaml
- ci-operator/config/openshift/tls-scanner/openshift-tls-scanner-main.yaml
- ci-operator/step-registry/tls/scanner/run/tls-scanner-run-ref.yaml
- ci-operator/step-registry/hypershift/modern-tls/hypershift-modern-tls-ref.yaml
- ci-operator/step-registry/hypershift/modern-tls/hypershift-modern-tls-commands.sh
dc0c1be to
51b6e5b
Compare
|
/pj-rehearse periodic-ci-openshift-tls-scanner-main-periodic-hypershift-tls |
|
@richardsonnick: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ci-operator/step-registry/tls/scanner/run/tls-scanner-run-commands.sh (1)
129-129: 💤 Low valueRemove unused
PRIVILEGED_CONTAINERvariable.This variable is assigned but never referenced. The pod's security configuration is fully handled by
SECURITY_CONTEXT_YAML. Theprivileged: truesetting is embedded directly in that YAML string at line 130, making this variable dead code.Suggested fix
else HOST_NETWORK="true" HOST_PID="true" - PRIVILEGED_CONTAINER="true" SECURITY_CONTEXT_YAML=" privileged: true runAsUser: 0" fi🤖 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` at line 129, Remove the unused PRIVILEGED_CONTAINER variable assignment. The variable is assigned a value but never referenced anywhere in the script, and its functionality is already covered by the SECURITY_CONTEXT_YAML variable which contains the privileged: true setting. Delete the line containing PRIVILEGED_CONTAINER="true" to clean up the dead code.Source: Linters/SAST tools
🤖 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/hypershift-run/tls-scanner-hypershift-run-ref.yaml`:
- Around line 7-11: There is a mismatch between the default value declared for
the TLS_SCANNER_RUN_HYPERSHIFT parameter in the ref file and the default being
used in the commands script. The TLS_SCANNER_RUN_HYPERSHIFT parameter is
declared with default: "true" in the ref file, but the
tls-scanner-hypershift-run-commands.sh script uses
${TLS_SCANNER_RUN_HYPERSHIFT:-false} which defaults to "false" when the variable
is not set. To fix this, either change the ref file's default value from "true"
to "false" to match the script's actual default, or update the script's
parameter expansion from :-false to :-true to align with the ref file
declaration. Choose whichever approach aligns with the intended behavior for
dual-cluster scanning.
---
Nitpick comments:
In `@ci-operator/step-registry/tls/scanner/run/tls-scanner-run-commands.sh`:
- Line 129: Remove the unused PRIVILEGED_CONTAINER variable assignment. The
variable is assigned a value but never referenced anywhere in the script, and
its functionality is already covered by the SECURITY_CONTEXT_YAML variable which
contains the privileged: true setting. Delete the line containing
PRIVILEGED_CONTAINER="true" to clean up the dead code.
🪄 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: 523fb901-f975-486f-bd7c-2b3b2d96e146
⛔ Files ignored due to path filters (1)
ci-operator/jobs/openshift/tls-scanner/openshift-tls-scanner-main-periodics.yamlis excluded by!ci-operator/jobs/**
📒 Files selected for processing (14)
ci-operator/config/openshift/tls-scanner/openshift-tls-scanner-main.yamlci-operator/step-registry/hypershift/modern-tls/OWNERSci-operator/step-registry/hypershift/modern-tls/hypershift-modern-tls-commands.shci-operator/step-registry/hypershift/modern-tls/hypershift-modern-tls-ref.metadata.jsonci-operator/step-registry/hypershift/modern-tls/hypershift-modern-tls-ref.yamlci-operator/step-registry/tls/scanner/hypershift-aws/OWNERSci-operator/step-registry/tls/scanner/hypershift-aws/tls-scanner-hypershift-aws-workflow.metadata.jsonci-operator/step-registry/tls/scanner/hypershift-aws/tls-scanner-hypershift-aws-workflow.yamlci-operator/step-registry/tls/scanner/hypershift-run/OWNERSci-operator/step-registry/tls/scanner/hypershift-run/tls-scanner-hypershift-run-commands.shci-operator/step-registry/tls/scanner/hypershift-run/tls-scanner-hypershift-run-ref.metadata.jsonci-operator/step-registry/tls/scanner/hypershift-run/tls-scanner-hypershift-run-ref.yamlci-operator/step-registry/tls/scanner/run/tls-scanner-run-commands.shci-operator/step-registry/tls/scanner/run/tls-scanner-run-ref.yaml
✅ Files skipped from review due to trivial changes (7)
- ci-operator/step-registry/hypershift/modern-tls/OWNERS
- ci-operator/step-registry/tls/scanner/hypershift-aws/tls-scanner-hypershift-aws-workflow.metadata.json
- ci-operator/step-registry/hypershift/modern-tls/hypershift-modern-tls-ref.metadata.json
- ci-operator/step-registry/tls/scanner/hypershift-run/tls-scanner-hypershift-run-ref.metadata.json
- ci-operator/step-registry/tls/scanner/hypershift-aws/OWNERS
- ci-operator/step-registry/tls/scanner/hypershift-run/OWNERS
- ci-operator/step-registry/tls/scanner/hypershift-run/tls-scanner-hypershift-run-commands.sh
🚧 Files skipped from review as they are similar to previous changes (5)
- ci-operator/step-registry/tls/scanner/run/tls-scanner-run-ref.yaml
- ci-operator/step-registry/tls/scanner/hypershift-aws/tls-scanner-hypershift-aws-workflow.yaml
- ci-operator/step-registry/hypershift/modern-tls/hypershift-modern-tls-ref.yaml
- ci-operator/config/openshift/tls-scanner/openshift-tls-scanner-main.yaml
- ci-operator/step-registry/hypershift/modern-tls/hypershift-modern-tls-commands.sh
|
At a glance it looks like the he compliance check is working. I'll check this again from my desktop (working from my phone) |
51b6e5b to
ccf01a8
Compare
|
/pj-rehearse periodic-ci-openshift-tls-scanner-main-periodic-hypershift-tls |
|
This run should fix spyglass junit output and the early exit before guest cluster scan |
|
[REHEARSALNOTIFIER]
A total of 62 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: |
|
@richardsonnick: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ci-operator/step-registry/tls/scanner/run/tls-scanner-run-commands.sh (1)
126-132: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueRemove unused
PRIVILEGED_CONTAINERvariable.
PRIVILEGED_CONTAINERis set but never referenced—the privileged flag is already embedded inSECURITY_CONTEXT_YAML. Shellcheck SC2034 correctly flags this as unused.Suggested fix
else HOST_NETWORK="true" HOST_PID="true" - PRIVILEGED_CONTAINER="true" SECURITY_CONTEXT_YAML=" privileged: true runAsUser: 0" fi🤖 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 126 - 132, The `PRIVILEGED_CONTAINER` variable is being set but never used anywhere in the script—the privileged container configuration is already included in the `SECURITY_CONTEXT_YAML` variable. Remove the line that assigns `PRIVILEGED_CONTAINER="true"` from the else block to eliminate the unused variable warning flagged by Shellcheck SC2034. The privileged flag embedded in `SECURITY_CONTEXT_YAML` provides all the necessary security context configuration.Source: Linters/SAST tools
🤖 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.
Nitpick comments:
In `@ci-operator/step-registry/tls/scanner/run/tls-scanner-run-commands.sh`:
- Around line 126-132: The `PRIVILEGED_CONTAINER` variable is being set but
never used anywhere in the script—the privileged container configuration is
already included in the `SECURITY_CONTEXT_YAML` variable. Remove the line that
assigns `PRIVILEGED_CONTAINER="true"` from the else block to eliminate the
unused variable warning flagged by Shellcheck SC2034. The privileged flag
embedded in `SECURITY_CONTEXT_YAML` provides all the necessary security context
configuration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 1331676f-b268-4635-89d4-8f70bb0c131c
⛔ Files ignored due to path filters (1)
ci-operator/jobs/openshift/tls-scanner/openshift-tls-scanner-main-periodics.yamlis excluded by!ci-operator/jobs/**
📒 Files selected for processing (14)
ci-operator/config/openshift/tls-scanner/openshift-tls-scanner-main.yamlci-operator/step-registry/hypershift/modern-tls/OWNERSci-operator/step-registry/hypershift/modern-tls/hypershift-modern-tls-commands.shci-operator/step-registry/hypershift/modern-tls/hypershift-modern-tls-ref.metadata.jsonci-operator/step-registry/hypershift/modern-tls/hypershift-modern-tls-ref.yamlci-operator/step-registry/tls/scanner/hypershift-aws/OWNERSci-operator/step-registry/tls/scanner/hypershift-aws/tls-scanner-hypershift-aws-workflow.metadata.jsonci-operator/step-registry/tls/scanner/hypershift-aws/tls-scanner-hypershift-aws-workflow.yamlci-operator/step-registry/tls/scanner/hypershift-run/OWNERSci-operator/step-registry/tls/scanner/hypershift-run/tls-scanner-hypershift-run-commands.shci-operator/step-registry/tls/scanner/hypershift-run/tls-scanner-hypershift-run-ref.metadata.jsonci-operator/step-registry/tls/scanner/hypershift-run/tls-scanner-hypershift-run-ref.yamlci-operator/step-registry/tls/scanner/run/tls-scanner-run-commands.shci-operator/step-registry/tls/scanner/run/tls-scanner-run-ref.yaml
✅ Files skipped from review due to trivial changes (7)
- ci-operator/step-registry/tls/scanner/hypershift-run/tls-scanner-hypershift-run-commands.sh
- ci-operator/step-registry/tls/scanner/hypershift-aws/OWNERS
- ci-operator/step-registry/tls/scanner/hypershift-run/OWNERS
- ci-operator/step-registry/tls/scanner/hypershift-run/tls-scanner-hypershift-run-ref.metadata.json
- ci-operator/step-registry/tls/scanner/hypershift-aws/tls-scanner-hypershift-aws-workflow.metadata.json
- ci-operator/step-registry/hypershift/modern-tls/hypershift-modern-tls-ref.metadata.json
- ci-operator/step-registry/tls/scanner/run/tls-scanner-run-ref.yaml
🚧 Files skipped from review as they are similar to previous changes (6)
- ci-operator/step-registry/tls/scanner/hypershift-aws/tls-scanner-hypershift-aws-workflow.yaml
- ci-operator/step-registry/tls/scanner/hypershift-run/tls-scanner-hypershift-run-ref.yaml
- ci-operator/step-registry/hypershift/modern-tls/OWNERS
- ci-operator/step-registry/hypershift/modern-tls/hypershift-modern-tls-commands.sh
- ci-operator/config/openshift/tls-scanner/openshift-tls-scanner-main.yaml
- ci-operator/step-registry/hypershift/modern-tls/hypershift-modern-tls-ref.yaml
|
@richardsonnick: all tests passed! 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. |
testtesttest
Summary by CodeRabbit
This PR extends OpenShift CI infrastructure to continuously validate HyperShift “Modern TLS” (TLS 1.3) behavior in AWS by adding a new periodic HyperShift TLS scanner job and the HyperShift-specific TLS/Modern-TLS enablement and scanning steps.
What changed (practically)
periodic-hypershift-tls(runs every 72h) toci-operator/config/openshift/tls-scanner/openshift-tls-scanner-main.yaml, executing thetls-scanner-hypershift-awsworkflow for thehypershift-awscluster profile and reporting job state to#forum-case.hypershift-modern-tls):spec.configuration.apiServer.tlsSecurityProfile.type=Modern.kube-apiserverto reconcile/roll out (with a short generation-change wait) and verifies the guest TLS behavior by:apiserver/clusterforspec.tlsSecurityProfile.type=Modern, andtls-scanner-hypershift-aws):tls-scanner-runwith:TLS_SCANNER_CLUSTER_LABELto drive management vs guest targeting and scope artifacts.TLS_PROFILE_TYPEto run compliance checks without relying on APIServer CR mirroring (HyperShift quirk).run_tls_scan()and adjusts pod resource handling and artifact/JUnit output collection accordingly.tls-scanner-hypershift-run):TLS_PROFILE_TYPEto Modern for HyperShift compliance checks.TLS_SCANNER_RUN_HYPERSHIFT=true) and emits separate JUnit outputs per label (management vs guest).Ownership / reviewers
hypershift-modern-tlsis owned bygangwgr.richardsonnick,rhmdnd, andsmith-xyz(as configured in the relevant OWNERS/metadata files).