CMP-4354: Reviving profile test jobs on rhcos10#80728
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 change adds a new FIPS SSH key preparation step, updates existing FIPS AWS pre-steps to use it, and adds new RHEL-10 ComplianceAsCode jobs for AWS and baremetal IPI testing. ChangesFIPS SSH key setup and RHEL-10 jobs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
🚥 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)
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
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
ci-operator/config/ComplianceAsCode/content/ComplianceAsCode-content-master__4.22.yaml (3)
849-849: ⚡ Quick winConsider removing
set -xfor consistency and security.The
set -xdirective enables command tracing, which logs all executed commands with their arguments to CI logs. Per coding guidelines for step registry scripts, debug tracing should only be enabled when actively debugging, and the default should beset -euo pipefailwithout-x. While this test script doesn't handle secrets directly, enabling trace mode is inconsistent with the other 10 jobs in this PR and can inadvertently leak sensitive information.♻️ Proposed fix to remove debug tracing
- set -x export PROFILE=bsi🤖 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/ComplianceAsCode/content/ComplianceAsCode-content-master__4.22.yaml` at line 849, Remove the `set -x` directive from the script block that enables command tracing, as it logs all executed commands with arguments to CI logs which can inadvertently leak sensitive information. Replace it with `set -euo pipefail` to enable proper error handling and exit on undefined variables, which aligns with the security guidelines and provides consistency with the other 10 jobs in this PR.Source: Coding guidelines
924-953: ⚡ Quick winConsider removing
set -xfor consistency and security.The
set -xdirective enables command tracing, which logs all executed commands with their arguments to CI logs. Per coding guidelines for step registry scripts, debug tracing should only be enabled when actively debugging, and the default should beset -euo pipefailwithout-x. While this test script doesn't handle secrets directly, enabling trace mode is inconsistent with the other 10 jobs in this PR and can inadvertently leak sensitive information.♻️ Proposed fix to remove debug tracing
- set -x export PROFILE=nerc-cip🤖 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/ComplianceAsCode/content/ComplianceAsCode-content-master__4.22.yaml` around lines 924 - 953, Remove the `set -x` directive from the commands section in the test step of the e2e-aws-rhcos4-nerc-cip-rhcos10 job. The `set -x` enables debug tracing which logs all executed commands and can inadvertently leak sensitive information in CI logs. Replace it with `set -euo pipefail` which is the standard for step registry scripts to ensure consistency across all jobs in this PR and to follow security best practices.Source: Coding guidelines
879-879: ⚡ Quick winConsider removing
set -xfor consistency and security.The
set -xdirective enables command tracing, which logs all executed commands with their arguments to CI logs. Per coding guidelines for step registry scripts, debug tracing should only be enabled when actively debugging, and the default should beset -euo pipefailwithout-x. While this test script doesn't handle secrets directly, enabling trace mode is inconsistent with the other 10 jobs in this PR and can inadvertently leak sensitive information.♻️ Proposed fix to remove debug tracing
- set -x export PROFILE=e8🤖 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/ComplianceAsCode/content/ComplianceAsCode-content-master__4.22.yaml` at line 879, Remove the `set -x` directive from the script as it enables command tracing that logs all executed commands to CI logs, creating inconsistency with the other 10 jobs in the PR and potentially leaking sensitive information. Replace it with `set -euo pipefail` to follow the standard coding guidelines for step registry scripts where debug tracing should only be enabled when actively debugging, not by default.Source: Coding guidelines
🤖 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/config/ComplianceAsCode/content/ComplianceAsCode-content-master__4.22.yaml`:
- Line 849: Remove the `set -x` directive from the script block that enables
command tracing, as it logs all executed commands with arguments to CI logs
which can inadvertently leak sensitive information. Replace it with `set -euo
pipefail` to enable proper error handling and exit on undefined variables, which
aligns with the security guidelines and provides consistency with the other 10
jobs in this PR.
- Around line 924-953: Remove the `set -x` directive from the commands section
in the test step of the e2e-aws-rhcos4-nerc-cip-rhcos10 job. The `set -x`
enables debug tracing which logs all executed commands and can inadvertently
leak sensitive information in CI logs. Replace it with `set -euo pipefail` which
is the standard for step registry scripts to ensure consistency across all jobs
in this PR and to follow security best practices.
- Line 879: Remove the `set -x` directive from the script as it enables command
tracing that logs all executed commands to CI logs, creating inconsistency with
the other 10 jobs in the PR and potentially leaking sensitive information.
Replace it with `set -euo pipefail` to follow the standard coding guidelines for
step registry scripts where debug tracing should only be enabled when actively
debugging, not by default.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 0ffb646c-6a32-4f6e-88d8-108f5a6904eb
⛔ Files ignored due to path filters (1)
ci-operator/jobs/ComplianceAsCode/content/ComplianceAsCode-content-master-presubmits.yamlis excluded by!ci-operator/jobs/**
📒 Files selected for processing (1)
ci-operator/config/ComplianceAsCode/content/ComplianceAsCode-content-master__4.22.yaml
|
/pj-rehearse pull-ci-ComplianceAsCode-content-master-4.22-e2e-aws-ocp4-bsi-node-rhcos10 |
|
@Anna-Koudelkova: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse pull-ci-ComplianceAsCode-content-master-4.22-e2e-aws-ocp4-moderate-node-rhcos10 |
|
@Anna-Koudelkova: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse pull-ci-ComplianceAsCode-content-master-4.22-e2e-aws-rhcos4-high-rhcos10 |
|
@Anna-Koudelkova: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse pull-ci-ComplianceAsCode-content-master-4.22-e2e-aws-ocp4-moderate-node-rhcos10 |
|
@Anna-Koudelkova: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse pull-ci-ComplianceAsCode-content-master-4.22-e2e-aws-rhcos4-high-rhcos10 |
|
@Anna-Koudelkova: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
Anna-Koudelkova
changed the title
|
@Anna-Koudelkova: This pull request references CMP-4354 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-reference
|
/pj-rehearse pull-ci-ComplianceAsCode-content-master-4.22-e2e-aws-ocp4-moderate-node-rhcos10 |
|
@yuumasato: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
@yuumasato: requesting more than one rehearsal in one comment is not supported. If you would like to rehearse multiple specific jobs, please separate the job names by a space in a single command. |
|
/pj-rehearse pull-ci-ComplianceAsCode-content-master-4.22-e2e-aws-ocp4-pci-dss-node-rhcos10 |
|
@Anna-Koudelkova: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse pull-ci-ComplianceAsCode-content-master-4.22-e2e-aws-ocp4-cis-node-rhcos10 |
|
@Anna-Koudelkova: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
Anna-Koudelkova
force-pushed
the
rhcos10_profile_tests
branch
from
fc672f8 to
a223fde
Compare
|
/pj-rehearse pull-ci-ComplianceAsCode-content-master-4.22-e2e-metal-ds-ipi-ovn-rhcos4-stig-rhcos10 |
|
@Anna-Koudelkova: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse abort |
|
@Anna-Koudelkova: 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
🤖 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/config/ComplianceAsCode/content/ComplianceAsCode-content-master__4.22.yaml`:
- Line 902: Remove all instances of the `set -x` debug tracing command from the
test steps at lines 902, 932, and 991 in the YAML configuration. The `set -x`
command enables command tracing which increases log verbosity and exposes
sensitive runtime values in CI logs. Simply delete each `set -x` line while
preserving the remaining shell commands in those test steps.
🪄 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: a5ff9b46-1767-4ad6-8ea5-6d829e56db3c
⛔ Files ignored due to path filters (1)
ci-operator/jobs/ComplianceAsCode/content/ComplianceAsCode-content-master-presubmits.yamlis excluded by!ci-operator/jobs/**
📒 Files selected for processing (1)
ci-operator/config/ComplianceAsCode/content/ComplianceAsCode-content-master__4.22.yaml
| - as: test | ||
| cli: latest | ||
| commands: | | ||
| set -x |
There was a problem hiding this comment.
Remove debug tracing (set -x) from these test steps.
This increases log verbosity and can expose sensitive runtime values in CI logs.
As per coding guidelines, “Be cautious with set -x … variable expansions in traced commands will expose their values in logs.”
Suggested patch
@@
- set -x
export PROFILE=bsi
@@
- set -x
export PROFILE=e8
@@
- set -x
export PROFILE=nerc-cipAlso applies to: 932-932, 991-991
🤖 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/ComplianceAsCode/content/ComplianceAsCode-content-master__4.22.yaml`
at line 902, Remove all instances of the `set -x` debug tracing command from the
test steps at lines 902, 932, and 991 in the YAML configuration. The `set -x`
command enables command tracing which increases log verbosity and exposes
sensitive runtime values in CI logs. Simply delete each `set -x` line while
preserving the remaining shell commands in those test steps.
Source: Coding guidelines
| capabilities: | ||
| - intranet | ||
| steps: | ||
| cluster_profile: equinix-ocp-metal-qe |
There was a problem hiding this comment.
Is this cluster profile expected or a copy pasta mistake?
There was a problem hiding this comment.
It is a copy and paste, but we are using it in downstream testing as well, so it was deliberate
Anna-Koudelkova
force-pushed
the
rhcos10_profile_tests
branch
from
9164266 to
72effa3
Compare
|
/pj-rehearse pull-ci-ComplianceAsCode-content-master-4.22-e2e-aws-ocp4-moderate-node-rhcos10 |
|
@Anna-Koudelkova: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse pull-ci-ComplianceAsCode-content-master-4.22-e2e-aws-ocp4-moderate-node-rhcos10 |
|
@Anna-Koudelkova: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
Anna-Koudelkova
force-pushed
the
rhcos10_profile_tests
branch
from
72effa3 to
57b2af2
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Anna-Koudelkova 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 |
openshift-ci
Bot
removed
the
approved
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/ipi/conf/fips-sshkey/ipi-conf-fips-sshkey-commands.sh`:
- Around line 32-45: The key generation loop in ipi-conf-fips-sshkey-commands.sh
only blocks ed25519, so overridden values like dsa can still slip through and
produce weak non-FIPS keys. Update the SSH_KEY_TYPE_LIST handling in the key
generation logic to whitelist only the supported FIPS key types (for example
ecdsa and rsa) and reject anything else with a clear error before calling
ssh-keygen. While doing so, update the ssh-keygen invocation in the same loop to
build an argument array instead of concatenating an optional string, using the
key_type-specific options in a safe, explicit way.
🪄 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: 9aff19c4-a9ac-4945-b3f6-05e18fd3c29f
⛔ Files ignored due to path filters (1)
ci-operator/jobs/ComplianceAsCode/content/ComplianceAsCode-content-master-presubmits.yamlis excluded by!ci-operator/jobs/**
📒 Files selected for processing (8)
ci-operator/config/ComplianceAsCode/content/ComplianceAsCode-content-master__4.22.yamlci-operator/step-registry/ipi/aws/pre/fips/OWNERSci-operator/step-registry/ipi/aws/pre/fips/ipi-aws-pre-fips-chain.metadata.jsonci-operator/step-registry/ipi/aws/pre/fips/ipi-aws-pre-fips-chain.yamlci-operator/step-registry/ipi/conf/fips-sshkey/OWNERSci-operator/step-registry/ipi/conf/fips-sshkey/ipi-conf-fips-sshkey-commands.shci-operator/step-registry/ipi/conf/fips-sshkey/ipi-conf-fips-sshkey-ref.metadata.jsonci-operator/step-registry/ipi/conf/fips-sshkey/ipi-conf-fips-sshkey-ref.yaml
✅ Files skipped from review due to trivial changes (4)
- ci-operator/step-registry/ipi/aws/pre/fips/OWNERS
- ci-operator/step-registry/ipi/conf/fips-sshkey/OWNERS
- ci-operator/step-registry/ipi/conf/fips-sshkey/ipi-conf-fips-sshkey-ref.metadata.json
- ci-operator/step-registry/ipi/aws/pre/fips/ipi-aws-pre-fips-chain.metadata.json
🚧 Files skipped from review as they are similar to previous changes (1)
- ci-operator/config/ComplianceAsCode/content/ComplianceAsCode-content-master__4.22.yaml
openshift-ci
Bot
added
the
do-not-merge/invalid-owners-file
|
/pj-rehearse pull-ci-ComplianceAsCode-content-master-4.22-e2e-aws-ocp4-moderate-node-rhcos10 |
|
@Anna-Koudelkova: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
Anna-Koudelkova
force-pushed
the
rhcos10_profile_tests
branch
3 times, most recently
from
85476e2 to
1caf0e4
Compare
openshift-ci
Bot
removed
the
do-not-merge/invalid-owners-file
Anna-Koudelkova
force-pushed
the
rhcos10_profile_tests
branch
from
1caf0e4 to
226fb7c
Compare
|
/pj-rehearse pull-ci-ComplianceAsCode-content-master-4.22-e2e-aws-ocp4-moderate-node-rhcos10 |
|
@Anna-Koudelkova: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
Anna-Koudelkova
force-pushed
the
rhcos10_profile_tests
branch
from
226fb7c to
e3d5b6d
Compare
|
/pj-rehearse pull-ci-ComplianceAsCode-content-master-4.22-e2e-aws-ocp4-moderate-node-rhcos10 |
|
@Anna-Koudelkova: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
Anna-Koudelkova
force-pushed
the
rhcos10_profile_tests
branch
from
e3d5b6d to
21967de
Compare
|
[REHEARSALNOTIFIER]
A total of 44 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 pull-ci-ComplianceAsCode-content-master-4.22-e2e-gcp-ocp4-moderate-node-rhcos10 |
|
@Anna-Koudelkova: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
@Anna-Koudelkova: 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
Node and platform tests using RHCOS 10 has some issues, so we have decided to revert back to using the profile tests to map the behavior of rules on RHCOS 10 cluster
Summary by CodeRabbit
This PR updates
ci-operator/config/ComplianceAsCode/content/ComplianceAsCode-content-master__4.22.yamlto improve ComplianceAsCode testing coverage for RHEL-10/RHCOS10 by adding profile-based end-to-end remediation jobs (to validate rule behavior on RHCOS10), alongside existing platform/node compliance jobs.It:
base_imagesblock for the 4.22cli,dev-scripts, andinstallerimages.ipi-aws-pre-fips) and updates existing RHCOS4 FIPS AWS jobs (e2e-aws-rhcos4-moderate,e2e-aws-rhcos4-high,e2e-aws-rhcos4-stig) to usepre: chain: ipi-aws-pre-fipsinstead of the prioripi-aws-pre+fips-checkflow.OS_IMAGE_STREAM: rhel-10with*-rhcos10naming. These jobs runTestProfileRemediationswithPROFILE/PRODUCTset per profile variant (including moderate/high/BSI/PCI-DSS/CIS/STIG pluse8andnerc-cip), typically withFEATURE_SET: TechPreviewNoUpgrade,OPENSHIFT_INSTALL_RELEASE_IMAGE_OVERRIDE: release:nightly-latest, and (for FIPS variants)pre: chain: ipi-aws-pre-fipsandFIPS_ENABLED: "true".e2e-metal-ds-ipi-ovn-rhcos4-stig-rhcos10, enabling OVN + FIPS, settingDEVSCRIPTS_CONFIG, and running the metal rehearsed workflow with the appropriatepresequence (includingfips-check).e2e-aws-openshift-platform-compliance-rhcos10ande2e-aws-openshift-node-compliance-rhcos10), which still runTestPlatformCompliance/TestNodeCompliance.Additionally, it adds/updates step-registry support for the FIPS pre chain and FIPS-compatible SSH keys (new step-registry YAML/metadata, updated OWNERS approvers, and a new
ipi-conf-fips-sshkey-commands.shscript).