CNTRLPLANE-3574: Migrate FIPS testing on AWS nad AKS to upstream#80394
CNTRLPLANE-3574: Migrate FIPS testing on AWS nad AKS to upstream#80394mgencur wants to merge 5 commits into
Conversation
- Add e2e-aws-ovn-conformance-fips periodic jobs (4.16-5.0) - Add e2e-conformance-fips optional presubmit for main - Remove fips-guest-f28-destructive and fips-mgmt-f14 from QE configs - Keep fips-guest-openldap-f14 for IdP integration coverage Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
openshift-ci-robot
added
the
jira/valid-reference
|
@mgencur: This pull request references CNTRLPLANE-3574 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. |
WalkthroughAdds a hostedcluster FIPS verification step, metadata and OWNERS; adds FIPS-enabled conformance periodics (4.16–5.0) that run the FIPS check then hypershift-conformance; and updates private-nightly hypershift job variants and a workflow skip list. ChangesHyperShift FIPS Conformance Testing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/pj-rehearse periodic-ci-openshift-hypershift-release-5.0-periodics-e2e-aws-ovn-conformance-fips |
|
@mgencur: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
- New hypershift-hostedcluster-fips-check ref (symlinks fips-check script) - Override test phase in all FIPS conformance configs (4.16-5.0 + main) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
/pj-rehearse periodic-ci-openshift-hypershift-release-5.0-periodics-e2e-aws-ovn-conformance-fips |
|
@mgencur: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/hypershift/hostedcluster/fips-check/hypershift-hostedcluster-fips-check-commands.sh`:
- Around line 13-21: The script currently loops over the result of oc get nodes
and silently passes if that list is empty; capture the node list into a variable
(e.g., nodes="$(oc get nodes -o jsonpath='{.items[*].metadata.name}')" or
similar), check if that variable is empty/unset before entering the for loop,
and if so print an explicit error ("no nodes returned") and set failed=1 and
exit non-zero (or return non-zero) so the job fails; then iterate over the nodes
variable (not invoking oc again for the list) and keep the existing per-node
check using oc debug node/"${node}" and the fips variable. Ensure you reference
and update the existing variables node, nodes, fips, and failed in the script.
🪄 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: c515c388-df95-446b-85fb-2a2400027ef4
📒 Files selected for processing (14)
ci-operator/config/openshift/hypershift/openshift-hypershift-main.yamlci-operator/config/openshift/hypershift/openshift-hypershift-release-4.16__periodics.yamlci-operator/config/openshift/hypershift/openshift-hypershift-release-4.17__periodics.yamlci-operator/config/openshift/hypershift/openshift-hypershift-release-4.18__periodics.yamlci-operator/config/openshift/hypershift/openshift-hypershift-release-4.19__periodics.yamlci-operator/config/openshift/hypershift/openshift-hypershift-release-4.20__periodics.yamlci-operator/config/openshift/hypershift/openshift-hypershift-release-4.21__periodics.yamlci-operator/config/openshift/hypershift/openshift-hypershift-release-4.22__periodics.yamlci-operator/config/openshift/hypershift/openshift-hypershift-release-4.23__periodics.yamlci-operator/config/openshift/hypershift/openshift-hypershift-release-5.0__periodics.yamlci-operator/step-registry/hypershift/hostedcluster/fips-check/OWNERSci-operator/step-registry/hypershift/hostedcluster/fips-check/hypershift-hostedcluster-fips-check-commands.shci-operator/step-registry/hypershift/hostedcluster/fips-check/hypershift-hostedcluster-fips-check-ref.metadata.jsonci-operator/step-registry/hypershift/hostedcluster/fips-check/hypershift-hostedcluster-fips-check-ref.yaml
🚧 Files skipped from review as they are similar to previous changes (12)
- ci-operator/config/openshift/hypershift/openshift-hypershift-release-4.21__periodics.yaml
- ci-operator/config/openshift/hypershift/openshift-hypershift-release-4.20__periodics.yaml
- ci-operator/config/openshift/hypershift/openshift-hypershift-release-4.23__periodics.yaml
- ci-operator/config/openshift/hypershift/openshift-hypershift-main.yaml
- ci-operator/step-registry/hypershift/hostedcluster/fips-check/OWNERS
- ci-operator/config/openshift/hypershift/openshift-hypershift-release-5.0__periodics.yaml
- ci-operator/config/openshift/hypershift/openshift-hypershift-release-4.22__periodics.yaml
- ci-operator/config/openshift/hypershift/openshift-hypershift-release-4.16__periodics.yaml
- ci-operator/config/openshift/hypershift/openshift-hypershift-release-4.19__periodics.yaml
- ci-operator/step-registry/hypershift/hostedcluster/fips-check/hypershift-hostedcluster-fips-check-ref.metadata.json
- ci-operator/config/openshift/hypershift/openshift-hypershift-release-4.17__periodics.yaml
- ci-operator/config/openshift/hypershift/openshift-hypershift-release-4.18__periodics.yaml
| for node in $(oc get nodes -o jsonpath='{.items[*].metadata.name}'); do | ||
| fips=$(oc debug node/"${node}" -- cat /proc/sys/crypto/fips_enabled 2>/dev/null || echo "error") | ||
| if [[ "${fips}" == "1" ]]; then | ||
| echo " ${node}: FIPS enabled" | ||
| else | ||
| echo " ${node}: FIPS NOT enabled (got: ${fips})" | ||
| failed=1 | ||
| fi | ||
| done |
There was a problem hiding this comment.
Fail explicitly when no nodes are returned instead of passing by default.
If oc get nodes yields an empty set, this check currently reports success without validating any node. For a conformance gate, that is a false positive.
Suggested patch
failed=0
-for node in $(oc get nodes -o jsonpath='{.items[*].metadata.name}'); do
+nodes="$(oc get nodes -o jsonpath='{.items[*].metadata.name}')"
+if [[ -z "${nodes}" ]]; then
+ echo "FIPS check FAILED: no nodes returned from hosted cluster"
+ exit 1
+fi
+
+for node in ${nodes}; do
fips=$(oc debug node/"${node}" -- cat /proc/sys/crypto/fips_enabled 2>/dev/null || echo "error")
if [[ "${fips}" == "1" ]]; then
echo " ${node}: FIPS enabled"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for node in $(oc get nodes -o jsonpath='{.items[*].metadata.name}'); do | |
| fips=$(oc debug node/"${node}" -- cat /proc/sys/crypto/fips_enabled 2>/dev/null || echo "error") | |
| if [[ "${fips}" == "1" ]]; then | |
| echo " ${node}: FIPS enabled" | |
| else | |
| echo " ${node}: FIPS NOT enabled (got: ${fips})" | |
| failed=1 | |
| fi | |
| done | |
| nodes="$(oc get nodes -o jsonpath='{.items[*].metadata.name}')" | |
| if [[ -z "${nodes}" ]]; then | |
| echo "FIPS check FAILED: no nodes returned from hosted cluster" | |
| exit 1 | |
| fi | |
| for node in ${nodes}; do | |
| fips=$(oc debug node/"${node}" -- cat /proc/sys/crypto/fips_enabled 2>/dev/null || echo "error") | |
| if [[ "${fips}" == "1" ]]; then | |
| echo " ${node}: FIPS enabled" | |
| else | |
| echo " ${node}: FIPS NOT enabled (got: ${fips})" | |
| failed=1 | |
| fi | |
| done |
🤖 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/hypershift/hostedcluster/fips-check/hypershift-hostedcluster-fips-check-commands.sh`
around lines 13 - 21, The script currently loops over the result of oc get nodes
and silently passes if that list is empty; capture the node list into a variable
(e.g., nodes="$(oc get nodes -o jsonpath='{.items[*].metadata.name}')" or
similar), check if that variable is empty/unset before entering the for loop,
and if so print an explicit error ("no nodes returned") and set failed=1 and
exit non-zero (or return non-zero) so the job fails; then iterate over the nodes
variable (not invoking oc again for the list) and keep the existing per-node
check using oc debug node/"${node}" and the fips variable. Ensure you reference
and update the existing variables node, nodes, fips, and failed in the script.
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
…e workflow Add e2e-azure-aks-ovn-conformance-fips periodic tests using the hypershift-azure-aks-conformance workflow with --fips and FIPS node check. Weekly for 5.0/4.23, biweekly for 4.19-4.22. Add optional presubmit for main branch. Remove azure-aks-hypershift-byo-vnet-fips-guest and fips-mgmt tests from openshift-tests-private configs (4.19-4.22, 5.0) as they are replaced by the new upstream tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…vate Remove aws-ipi-ovn-hypershift-fips-guest-f28-destructive and aws-ipi-ovn-hypershift-fips-mgmt-f14 from the 5.0 amd64-nightly config, matching the removal already done for 4.16-4.22. These are replaced by the upstream e2e-aws-ovn-conformance-fips periodic tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mgencur 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 |
mgencur
changed the title
|
[REHEARSALNOTIFIER]
A total of 245 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 periodic-ci-openshift-hypershift-release-5.0-periodics-e2e-aws-ovn-conformance-fips periodic-ci-openshift-hypershift-release-5.0-periodics-e2e-azure-aks-ovn-conformance-fips |
|
@mgencur: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse periodic-ci-openshift-hypershift-release-4.22-periodics-e2e-aws-ovn-conformance-fips periodic-ci-openshift-hypershift-release-4.22-periodics-e2e-azure-aks-ovn-conformance-fips |
|
@mgencur: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
@mgencur: 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. |
2 participants
Partially covers https://redhat.atlassian.net/browse/CNTRLPLANE-3574 (only partially because we also need to remove old tests from openshift-tests-private)
Summary by CodeRabbit
This PR migrates HyperShift FIPS testing on AWS from QE-owned private job definitions into upstream OpenShift CI conformance workflows as part of CNTRLPLANE-3574.
What changed (practical impact)
Rationale / impact