OCPBUGS-78151: Add provisioningNetworkGateway field to install-config#10387
OCPBUGS-78151: Add provisioningNetworkGateway field to install-config#10387jadhaj wants to merge 1 commit intoopenshift:mainfrom
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:
WalkthroughThis pull request adds support for configuring a provisioning network gateway IP address in baremetal deployments. The feature enables users to specify a custom gateway that will be supplied to hosts via DHCP, threading the value through configuration, type definitions, validation, and bootstrap components. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/asset/agent/manifests/agentclusterinstall.go (1)
295-302:⚠️ Potential issue | 🟠 MajorDon't gate
provisioningNetworkGatewayon per-host BMC overrides.Line 301 is inside the host-override branch, so the new field is only serialized into
install-config-overrideswhen at least oneAgentHostoverrides BMC data. A cluster that sets onlyprovisioningNetworkGatewaywill silently lose it in the generatedAgentClusterInstall.Suggested fix
+ if installConfig.Config.Platform.BareMetal.ProvisioningNetworkGateway != "" { + baremetalPlatform.ProvisioningNetworkGateway = installConfig.Config.Platform.BareMetal.ProvisioningNetworkGateway + bmIcOverridden = true + } + for _, host := range agentHosts.Hosts { // Override if BMC values are not the same as default if host.BMC.Username != "" || host.BMC.Password != "" || host.BMC.Address != "" { bmhost := baremetal.Host{ Name: host.Hostname, @@ - baremetalPlatform.ProvisioningNetworkGateway = installConfig.Config.Platform.BareMetal.ProvisioningNetworkGateway } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/asset/agent/manifests/agentclusterinstall.go` around lines 295 - 302, The provisioningNetworkGateway field is currently only assigned inside the AgentHost BMC-override branch so a cluster that only sets installConfig.Config.Platform.BareMetal.ProvisioningNetworkGateway gets dropped from the generated AgentClusterInstall/install-config-overrides; move or duplicate the assignment so baremetalPlatform.ProvisioningNetworkGateway is set unconditionally (outside the per-host BMC override branch) using installConfig.Config.Platform.BareMetal.ProvisioningNetworkGateway, ensuring AgentHost logic remains unchanged and the AgentClusterInstall includes the gateway even when no AgentHost overrides are present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/types/baremetal/validation/platform.go`:
- Around line 608-621: The gateway validation is nested inside the
clusterProvisioningIP failure branch so provisioningNetworkGateway checks only
run when clusterProvisioningIP is invalid; move the provisioningNetworkGateway
validation out so it always runs independently. Specifically, in the function
performing these checks (the block using p.ProvisioningNetworkCIDR.Contains and
fldPath.Child), ensure the block that checks p.ProvisioningNetworkGateway != ""
and then validates Contains(...) and equality against p.ClusterProvisioningIP is
not inside the if that appends the clusterProvisioningIP error — extract or
reposition the provisioningNetworkGateway if-block to follow the
clusterProvisioningIP check at the same scope so both validations execute
regardless of the other’s result.
---
Outside diff comments:
In `@pkg/asset/agent/manifests/agentclusterinstall.go`:
- Around line 295-302: The provisioningNetworkGateway field is currently only
assigned inside the AgentHost BMC-override branch so a cluster that only sets
installConfig.Config.Platform.BareMetal.ProvisioningNetworkGateway gets dropped
from the generated AgentClusterInstall/install-config-overrides; move or
duplicate the assignment so baremetalPlatform.ProvisioningNetworkGateway is set
unconditionally (outside the per-host BMC override branch) using
installConfig.Config.Platform.BareMetal.ProvisioningNetworkGateway, ensuring
AgentHost logic remains unchanged and the AgentClusterInstall includes the
gateway even when no AgentHost overrides are present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 797c6121-3d92-4413-b1d0-a65305263c3d
📒 Files selected for processing (7)
data/data/bootstrap/baremetal/files/etc/containers/systemd/ironic-dnsmasq.containerdata/data/bootstrap/baremetal/files/etc/ironic.env.templatedata/data/manifests/openshift/baremetal-provisioning-config.yaml.templatepkg/asset/agent/manifests/agentclusterinstall.gopkg/asset/ignition/bootstrap/baremetal/template.gopkg/types/baremetal/platform.gopkg/types/baremetal/validation/platform.go
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@data/data/install.openshift.io_installconfigs.yaml`:
- Around line 6110-6117: Update the provisioningNetworkGateway field description
to state it is only honored when the installer is managing provisioning DHCP
(i.e., installer-managed provisioning network path); specifically modify the
provisioningNetworkGateway description text so it clarifies that the provided
gateway is used only for installer-managed provisioning DHCP and is ignored in
unmanaged/disabled provisioning modes, leaving the format and type unchanged.
In `@pkg/types/baremetal/validation/platform.go`:
- Around line 613-622: The provisioningNetworkGateway value
(p.ProvisioningNetworkGateway) must be validated for correct IP format in
ValidatePlatform before calling net.ParseIP/Contains; add the same IP format
check used for bootstrapProvisioningIP/clusterProvisioningIP: attempt
net.ParseIP(p.ProvisioningNetworkGateway), and if nil append a field.Invalid on
fldPath.Child("provisioningNetworkGateway") with an "invalid IP address" message
and skip the subsequent CIDR containment and equality checks; only perform the
Contains check and the comparison to p.ClusterProvisioningIP when the parsed IP
is non-nil.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 996ad102-350b-4791-a845-17ee395290f6
📒 Files selected for processing (8)
data/data/bootstrap/baremetal/files/etc/containers/systemd/ironic-dnsmasq.containerdata/data/bootstrap/baremetal/files/etc/ironic.env.templatedata/data/install.openshift.io_installconfigs.yamldata/data/manifests/openshift/baremetal-provisioning-config.yaml.templatepkg/asset/agent/manifests/agentclusterinstall.gopkg/asset/ignition/bootstrap/baremetal/template.gopkg/types/baremetal/platform.gopkg/types/baremetal/validation/platform.go
🚧 Files skipped from review as they are similar to previous changes (2)
- data/data/bootstrap/baremetal/files/etc/ironic.env.template
- pkg/types/baremetal/platform.go
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/types/baremetal/validation/platform.go (1)
615-628:⚠️ Potential issue | 🔴 CriticalGateway validation is still incorrectly nested inside the
clusterProvisioningIPfailure block.The
provisioningNetworkGatewaychecks (lines 619-627) are nested inside theif !p.ProvisioningNetworkCIDR.Contains(...)block forclusterProvisioningIP(line 615). This means gateway validation only executes whenclusterProvisioningIPis outside the provisioning CIDR—an invalid gateway will be silently accepted wheneverclusterProvisioningIPis valid.The closing brace on line 628 closes the
clusterProvisioningIPcheck, not a separate block.Proposed fix: unnest gateway validation
// Ensure clusterProvisioningIP is in the provisioningNetworkCIDR if !p.ProvisioningNetworkCIDR.Contains(net.ParseIP(p.ClusterProvisioningIP)) { allErrs = append(allErrs, field.Invalid(fldPath.Child("clusterProvisioningIP"), p.ClusterProvisioningIP, fmt.Sprintf("%q is not in the provisioning network", p.ClusterProvisioningIP))) + } - // Ensure provisioningNetworkGateway is in the provisioningNetworkCIDR - if p.ProvisioningNetworkGateway != "" { - if !p.ProvisioningNetworkCIDR.Contains(net.ParseIP(p.ProvisioningNetworkGateway)) { - allErrs = append(allErrs, field.Invalid(fldPath.Child("provisioningNetworkGateway"), p.ProvisioningNetworkGateway, fmt.Sprintf("%q is not in the provisioning network", p.ProvisioningNetworkGateway))) - } - // Ensure gateway is not the same as clusterProvisioningIP - if p.ProvisioningNetworkGateway == p.ClusterProvisioningIP { - allErrs = append(allErrs, field.Invalid(fldPath.Child("provisioningNetworkGateway"), p.ProvisioningNetworkGateway, "cannot be the same as clusterProvisioningIP")) - } + // Ensure provisioningNetworkGateway is in the provisioningNetworkCIDR + if p.ProvisioningNetworkGateway != "" { + if !p.ProvisioningNetworkCIDR.Contains(net.ParseIP(p.ProvisioningNetworkGateway)) { + allErrs = append(allErrs, field.Invalid(fldPath.Child("provisioningNetworkGateway"), p.ProvisioningNetworkGateway, fmt.Sprintf("%q is not in the provisioning network", p.ProvisioningNetworkGateway))) + } + // Ensure gateway is not the same as clusterProvisioningIP + if p.ProvisioningNetworkGateway == p.ClusterProvisioningIP { + allErrs = append(allErrs, field.Invalid(fldPath.Child("provisioningNetworkGateway"), p.ProvisioningNetworkGateway, "cannot be the same as clusterProvisioningIP")) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/types/baremetal/validation/platform.go` around lines 615 - 628, The gateway validation for p.ProvisioningNetworkGateway is incorrectly nested inside the failure branch for p.ClusterProvisioningIP; move the block that checks p.ProvisioningNetworkGateway (the net.ParseIP + p.ProvisioningNetworkCIDR.Contains check and the equality check against p.ClusterProvisioningIP that append field.Invalid using fldPath.Child("provisioningNetworkGateway")) out of the surrounding if !p.ProvisioningNetworkCIDR.Contains(net.ParseIP(p.ClusterProvisioningIP)) block so gateway validation always runs independently, preserving the same error messages and use of net.ParseIP and field.Invalid.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/types/baremetal/validation/platform.go`:
- Around line 615-628: The gateway validation for p.ProvisioningNetworkGateway
is incorrectly nested inside the failure branch for p.ClusterProvisioningIP;
move the block that checks p.ProvisioningNetworkGateway (the net.ParseIP +
p.ProvisioningNetworkCIDR.Contains check and the equality check against
p.ClusterProvisioningIP that append field.Invalid using
fldPath.Child("provisioningNetworkGateway")) out of the surrounding if
!p.ProvisioningNetworkCIDR.Contains(net.ParseIP(p.ClusterProvisioningIP)) block
so gateway validation always runs independently, preserving the same error
messages and use of net.ParseIP and field.Invalid.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 80ce833b-69e9-4fba-8afc-bb302ab9a21c
📒 Files selected for processing (8)
data/data/bootstrap/baremetal/files/etc/containers/systemd/ironic-dnsmasq.containerdata/data/bootstrap/baremetal/files/etc/ironic.env.templatedata/data/install.openshift.io_installconfigs.yamldata/data/manifests/openshift/baremetal-provisioning-config.yaml.templatepkg/asset/agent/manifests/agentclusterinstall.gopkg/asset/ignition/bootstrap/baremetal/template.gopkg/types/baremetal/platform.gopkg/types/baremetal/validation/platform.go
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/asset/agent/manifests/agentclusterinstall.go
- data/data/install.openshift.io_installconfigs.yaml
- pkg/types/baremetal/platform.go
- data/data/manifests/openshift/baremetal-provisioning-config.yaml.template
jadhaj
changed the title
openshift-ci-robot
added
jira/severity-moderate
|
@jadhaj: This pull request references Jira Issue OCPBUGS-78151, 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 |
openshift-ci-robot
added
the
jira/valid-bug
|
@jadhaj: This pull request references Jira Issue OCPBUGS-78151, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: 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
removed
the
jira/invalid-bug
|
@openshift-ci-robot: GitHub didn't allow me to request PR reviews from the following users: jadhaj. Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs. 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 kubernetes-sigs/prow repository. |
|
@jadhaj: This pull request references Jira Issue OCPBUGS-78151, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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: GitHub didn't allow me to request PR reviews from the following users: jadhaj. Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs. 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 kubernetes-sigs/prow repository. |
|
@jadhaj: 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
Changes to installer:
- Added ProvisioningNetworkGateway field to baremetal Platform struct
- Added validation to ensure gateway is within provisioning network CIDR
- Added validation to ensure gateway is not in DHCP range
- Added validation to ensure gateway is not the same as cluster provisioning IP
- Added ProvisioningNetworkGateway field to template data struct
- Added assignment from config to template data
- Added provisioningNetworkGateway to the generated Provisioning CR
- Added GATEWAY_IP variable for dnsmasq
- Added GATEWAY_IP environment variable to dnsmasq container
Summary by CodeRabbit
Release Notes