OCPBUGS-77760: verify FIPS mode after installation completes - #10348
Conversation
|
@zaneb: This pull request references AGENT-1455 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 story to target the "4.22.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. |
|
/test e2e-aws-ovn-fips |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andfasano The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retitle OCPBUGS-77760: verify FIPS mode after installation completes |
|
@zaneb: This pull request references Jira Issue OCPBUGS-77760, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
/jira refresh |
|
@zaneb: This pull request references Jira Issue OCPBUGS-77760, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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. |
1d1b6aa to
154e406
Compare
|
OK, now we see the check running as expected: |
WalkthroughRefactors APIs to pass asset.Store instances (not directory strings), adds FIPS verification during install completion, and replaces platform-branching timeout logic with a configurable WaitOptions struct derived from the install-config. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/openshift-install/waitfor.go`:
- Around line 31-34: The code uses an undefined variable installConfig and the
wrong field path; update the aws-specific branch to use the function parameter
ic (type *types.InstallConfig) and access the AWS platform via ic.Platform.AWS,
then check ic.Platform.AWS != nil and its UserProvisionedDNS value; set
options.UserProvisionedDNSEnabled = true when ic.Platform.AWS != nil &&
ic.Platform.AWS.UserProvisionedDNS == dns.UserProvisionedDNSEnabled so the
checks reference ic, Platform, and AWS correctly.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dde4a6f1-a449-40aa-bf40-09964e5e58c0
📒 Files selected for processing (7)
cmd/openshift-install/agent/waitfor.gocmd/openshift-install/command/waitfor.gocmd/openshift-install/create.gocmd/openshift-install/waitfor.gopkg/agent/cluster.gopkg/agent/rest.gopkg/nodejoiner/monitoraddnodes.go
| case aws.Name: | ||
| if installConfig.(*installconfig.InstallConfig).Config.AWS != nil && | ||
| installConfig.(*installconfig.InstallConfig).Config.AWS.UserProvisionedDNS == dns.UserProvisionedDNSEnabled { | ||
| options.UserProvisionedDNSEnabled = true |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Helper body:"
sed -n '22,39p' cmd/openshift-install/waitfor.go
echo
echo "Exact installConfig token references in this file:"
rg -nP '(?<![[:alnum:]_])installConfig(?![[:alnum:]_])' cmd/openshift-install/waitfor.go || true
echo
echo "Declarations/assignments for installConfig in this file:"
rg -nP '\b(var\s+installConfig\b|installConfig\s*:=|installConfig\s*=)' cmd/openshift-install/waitfor.go || trueRepository: openshift/installer
Length of output: 1031
🏁 Script executed:
find . -name "*.go" -type f -exec grep -l "type InstallConfig struct" {} \; | head -5Repository: openshift/installer
Length of output: 149
🏁 Script executed:
# Check the types.InstallConfig definition
rg -A 20 "type InstallConfig struct" --type go | head -50Repository: openshift/installer
Length of output: 3445
🏁 Script executed:
# Verify that ic.AWS is accessible
rg -B 5 -A 5 "\.AWS\s*\*" --type go | grep -A 5 -B 5 "InstallConfig" | head -30Repository: openshift/installer
Length of output: 45
🏁 Script executed:
cat -n pkg/types/installconfig.go | head -100Repository: openshift/installer
Length of output: 4327
🏁 Script executed:
# Search for AWS field in types.InstallConfig
rg -A 100 "^type InstallConfig struct" pkg/types/installconfig.go | grep -E "(AWS|^})" | head -5Repository: openshift/installer
Length of output: 45
🏁 Script executed:
# Read more of the types.InstallConfig struct definition
sed -n '93,250p' pkg/types/installconfig.goRepository: openshift/installer
Length of output: 6801
🏁 Script executed:
# Check the Platform type definition to understand how to access AWS config
rg -A 30 "^type Platform struct" pkg/types/Repository: openshift/installer
Length of output: 28102
🏁 Script executed:
# Also check if there's an embedded AWS type or Config field
rg "type Platform struct" pkg/types/ -A 50 | head -80Repository: openshift/installer
Length of output: 6189
🏁 Script executed:
# Check aws.Platform struct to verify UserProvisionedDNS field exists
rg -A 50 "type Platform struct" pkg/types/aws/platform.go | grep -E "(UserProvisionedDNS|type Platform)" | head -20Repository: openshift/installer
Length of output: 85
🏁 Script executed:
# Search for UserProvisionedDNS field in aws.Platform
rg "UserProvisionedDNS" pkg/types/aws/platform.go -B 3 -A 1Repository: openshift/installer
Length of output: 474
🏁 Script executed:
# Also check dns package for UserProvisionedDNSEnabled constant
rg "UserProvisionedDNSEnabled" --type goRepository: openshift/installer
Length of output: 5290
Fix the undefined installConfig reference and correct the field access path.
Lines 32-33 use an undefined identifier installConfig causing a compilation error, plus the suggested field access path is incorrect. The function parameter ic is *types.InstallConfig, which requires accessing the nested Platform field. The correct expression is ic.Platform.AWS, not ic.AWS (the Platform field is not embedded).
Suggested fix
func getWaitOptionsFromInstallConfig(ic *types.InstallConfig) command.WaitOptions {
options := command.WaitOptions{
VerifyFIPS: false, // IPI/UPI installer doesn't verify FIPS
}
if ic != nil {
switch ic.Platform.Name() {
case baremetal.Name:
options.ExtendTimeoutForBaremetal = true
case aws.Name:
- if installConfig.(*installconfig.InstallConfig).Config.AWS != nil &&
- installConfig.(*installconfig.InstallConfig).Config.AWS.UserProvisionedDNS == dns.UserProvisionedDNSEnabled {
+ if ic.Platform.AWS != nil && ic.Platform.AWS.UserProvisionedDNS == dns.UserProvisionedDNSEnabled {
options.UserProvisionedDNSEnabled = true
}
}
}
return options
}📝 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.
| case aws.Name: | |
| if installConfig.(*installconfig.InstallConfig).Config.AWS != nil && | |
| installConfig.(*installconfig.InstallConfig).Config.AWS.UserProvisionedDNS == dns.UserProvisionedDNSEnabled { | |
| options.UserProvisionedDNSEnabled = true | |
| case aws.Name: | |
| if ic.Platform.AWS != nil && ic.Platform.AWS.UserProvisionedDNS == dns.UserProvisionedDNSEnabled { | |
| options.UserProvisionedDNSEnabled = true |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/openshift-install/waitfor.go` around lines 31 - 34, The code uses an
undefined variable installConfig and the wrong field path; update the
aws-specific branch to use the function parameter ic (type *types.InstallConfig)
and access the AWS platform via ic.Platform.AWS, then check ic.Platform.AWS !=
nil and its UserProvisionedDNS value; set options.UserProvisionedDNSEnabled =
true when ic.Platform.AWS != nil && ic.Platform.AWS.UserProvisionedDNS ==
dns.UserProvisionedDNSEnabled so the checks reference ic, Platform, and AWS
correctly.
Move install-config loading from inside WaitForInstallComplete to each call site, and introduce a WaitOptions struct to pass configuration instead of the entire asset store. Note that ABI uses an OptionalInstallConfig rather than an InstallConfig asset, so it has never used an extended timeout for baremetal. Assisted-by: Claude Code
When install-config specifies fips: true, the agent-based installer now verifies that FIPS mode was actually enabled on the cluster before declaring installation successful. The verification queries both worker and master MachineConfigPools to retrieve their rendered MachineConfigs (the combined configs that machine-config-operator actually applies to nodes), and verifies that FIPS is enabled in each. This verification only runs for the agent wait-for install-complete command. Other installations are unchanged. Assisted-by: Claude Code
NewCluster() and FindRendezvousIPAndSSHKeyFromAssetStore() now accept an asset.Store parameter directly instead of a directory path string. This allows callers to reuse an existing Store instance and makes the API more explicit about its dependencies. Also fixed typo in function name: FindRendezvouIPAndSSHKeyFromAssetStore -> FindRendezvousIPAndSSHKeyFromAssetStore. Assisted-by: Claude Code
There was a problem hiding this comment.
♻️ Duplicate comments (1)
cmd/openshift-install/waitfor.go (1)
31-35:⚠️ Potential issue | 🔴 CriticalIncorrect field access path causes compilation error.
The code references
ic.AWSbuttypes.InstallConfighas the AWS platform config nested underPlatform.AWS. This will not compile.🐛 Proposed fix
case aws.Name: - if ic.AWS != nil && - ic.AWS.UserProvisionedDNS == dns.UserProvisionedDNSEnabled { + if ic.Platform.AWS != nil && + ic.Platform.AWS.UserProvisionedDNS == dns.UserProvisionedDNSEnabled { options.UserProvisionedDNSEnabled = true }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/openshift-install/waitfor.go` around lines 31 - 35, The case handling for aws.Name is using the wrong field path (ic.AWS) and won't compile; update the check to read the AWS platform config from ic.Platform.AWS with proper nil guards (e.g., ensure ic.Platform != nil and ic.Platform.AWS != nil) and then set options.UserProvisionedDNSEnabled = true when ic.Platform.AWS.UserProvisionedDNS == dns.UserProvisionedDNSEnabled so the code compiles and uses the correct InstallConfig nesting.
🧹 Nitpick comments (1)
cmd/openshift-install/agent/waitfor.go (1)
134-136: Consider adding timeout extension options for agent workflows.The
WaitOptionsstruct supportsExtendTimeoutForBaremetalandUserProvisionedDNSEnabledfor extending timeouts, but they're not set here. If agent-based baremetal/vsphere installations need extended timeouts like IPI/UPI, consider populating these from the install-config.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/openshift-install/agent/waitfor.go` around lines 134 - 136, The WaitOptions assignment currently only sets VerifyFIPS and omits timeout-extension flags; update the options variable (the WaitOptions struct created alongside fipsEnabled) to also set ExtendTimeoutForBaremetal and UserProvisionedDNSEnabled by reading the corresponding booleans from the install-config (e.g., the agent/baremetal and dns provisioning fields you already parse elsewhere in this flow), so: set options.ExtendTimeoutForBaremetal = <value from install-config baremetal/agent flag> and options.UserProvisionedDNSEnabled = <value from install-config DNS type check> before passing options into the wait call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cmd/openshift-install/waitfor.go`:
- Around line 31-35: The case handling for aws.Name is using the wrong field
path (ic.AWS) and won't compile; update the check to read the AWS platform
config from ic.Platform.AWS with proper nil guards (e.g., ensure ic.Platform !=
nil and ic.Platform.AWS != nil) and then set options.UserProvisionedDNSEnabled =
true when ic.Platform.AWS.UserProvisionedDNS == dns.UserProvisionedDNSEnabled so
the code compiles and uses the correct InstallConfig nesting.
---
Nitpick comments:
In `@cmd/openshift-install/agent/waitfor.go`:
- Around line 134-136: The WaitOptions assignment currently only sets VerifyFIPS
and omits timeout-extension flags; update the options variable (the WaitOptions
struct created alongside fipsEnabled) to also set ExtendTimeoutForBaremetal and
UserProvisionedDNSEnabled by reading the corresponding booleans from the
install-config (e.g., the agent/baremetal and dns provisioning fields you
already parse elsewhere in this flow), so: set options.ExtendTimeoutForBaremetal
= <value from install-config baremetal/agent flag> and
options.UserProvisionedDNSEnabled = <value from install-config DNS type check>
before passing options into the wait call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aa44974c-4a15-47da-8b30-596258eac234
📒 Files selected for processing (7)
cmd/openshift-install/agent/waitfor.gocmd/openshift-install/command/waitfor.gocmd/openshift-install/create.gocmd/openshift-install/waitfor.gopkg/agent/cluster.gopkg/agent/rest.gopkg/nodejoiner/monitoraddnodes.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/nodejoiner/monitoraddnodes.go
- pkg/agent/rest.go
|
@zaneb: The following test 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. |
|
/retest |
|
Logs show that we are actually checking FIPS status. |
|
@zaneb: This PR has been marked as verified by 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. |
|
/lgtm |
|
@zaneb: Jira Issue Verification Checks: Jira Issue OCPBUGS-77760 Jira Issue OCPBUGS-77760 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓 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. |
When install-config specifies fips: true, verify
that FIPS mode was actually enabled on the cluster before declaring an ABI
installation successful.
The verification queries both worker and master MachineConfigPools to
retrieve their rendered MachineConfigs (the combined configs that
machine-config-operator actually applies to nodes), and verifies that
FIPS is enabled in each.
This verification only runs for the agent wait-for install-complete
command. Regular installations are unchanged.