OCPBUGS-78151: Add provisioningNetworkGateway field to CBO#575
OCPBUGS-78151: Add provisioningNetworkGateway field to CBO#575jadhaj wants to merge 5 commits intoopenshift:mainfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jadhaj 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 |
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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:
WalkthroughAdds an optional ProvisioningNetworkGateway field to the Provisioning API and CRD, validates the gateway (must be inside CIDR, outside DHCP range, not network/broadcast, not equal to provisioning IP), surfaces it in deployment config, and exposes it to the metal3-dnsmasq container with tests updated. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
api/v1alpha1/provisioning_validation.go (1)
82-88: Add targeted tests for the new gateway validation paths.
api/v1alpha1/provisioning_validation_test.go:406-445still only exercises empty-gateway configs, so these branches can regress silently. Please add cases for gateway outside CIDR, equal toProvisioningIP, insideProvisioningDHCPRange, andProvisioningNetworkomitted so the default-Managed behavior stays covered.Also applies to: 152-152, 186-201, 242-248
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v1alpha1/provisioning_validation.go` around lines 82 - 88, Add focused unit tests in provisioning_validation_test.go that exercise the new gateway validation branches for validateProvisioningNetworkSettings and the surrounding validation logic: create test cases where ProvisioningNetworkGateway is (1) outside ProvisioningNetworkCIDR, (2) equal to ProvisioningIP, (3) falls inside ProvisioningDHCPRange, and (4) omitted so getProvisioningNetworkMode() defaults to Managed; for each case instantiate a prov object (using fields ProvisioningIP, ProvisioningNetworkCIDR, ProvisioningDHCPRange, ProvisioningNetworkGateway and method getProvisioningNetworkMode()) and assert the expected validation error or success. Add similar cases to the other described test ranges (around the earlier blocks at the mentioned offsets) to ensure coverage for both Managed and Unmanaged control paths.provisioning/baremetal_pod_test.go (1)
250-250: Add a positive-path assertion forGATEWAY_IPvalue propagation.Line 250 validates the empty baseline, but this PR’s core behavior is non-empty gateway propagation. Add one
ManagedSpeccase withProvisioningNetworkGatewayset and assertGATEWAY_IP=<gateway>.Example test addition
+{ + name: "ManagedSpec with provisioning gateway", + config: managedProvisioning().ProvisioningNetworkGateway("172.30.20.1").build(), + expectedContainers: []corev1.Container{ + withEnv(containers["metal3-httpd"], sshkey), + withEnv(containers["metal3-ironic"], sshkey), + containers["metal3-ramdisk-logs"], + containers["metal3-static-ip-manager"], + withEnv(containers["metal3-dnsmasq"], envWithValue("GATEWAY_IP", "172.30.20.1")), + }, + sshkey: "sshkey", +},As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@provisioning/baremetal_pod_test.go` at line 250, Add a positive-path test case that verifies non-empty gateway propagation: in the test table inside provisioning/baremetal_pod_test.go add a ManagedSpec entry with ProvisioningNetworkGateway set (e.g., "192.0.2.1") and assert that the resulting env var list contains an entry with Name "GATEWAY_IP" and Value equal to that gateway string; update the test that currently only checks {Name: "GATEWAY_IP", Value: ""} to include this new case and its expected value so the provisioning logic that reads ManagedSpec.ProvisioningNetworkGateway is validated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@provisioning/baremetal_config.go`:
- Around line 128-134: getGatewayIP currently only returns the gateway when
spec.provisioningNetwork is explicitly set to ProvisioningNetworkManaged; change
the condition in getGatewayIP to treat an empty string as the implicit Managed
default (e.g. if config.ProvisioningNetwork ==
metal3iov1alpha1.ProvisioningNetworkManaged || config.ProvisioningNetwork == ""
then return &config.ProvisioningNetworkGateway) and make the same change to the
other helper that checks spec.provisioningNetwork (the similar check at lines
152-153) so CRs relying on the default Managed mode export GATEWAY_IP.
In `@provisioning/baremetal_pod.go`:
- Line 458: The test for UnmanagedSpec must be updated to expect that
buildEnvVar(gatewayIP, config) yields an EnvVar with only Name set (zero-value
Value) rather than relying on the base container's Value:""; update the
UnmanagedSpec test assertions to explicitly check that an EnvVar named
"GATEWAY_IP" is present and that its Value is the empty string/zero-value (or
nil-equivalent) as returned by buildEnvVar, or alternatively change the code
path in buildEnvVar/gatewayIP handling to omit GATEWAY_IP for unmanaged flows
and adjust the tests accordingly; reference buildEnvVar and the UnmanagedSpec
test case to locate where to change the expectation.
---
Nitpick comments:
In `@api/v1alpha1/provisioning_validation.go`:
- Around line 82-88: Add focused unit tests in provisioning_validation_test.go
that exercise the new gateway validation branches for
validateProvisioningNetworkSettings and the surrounding validation logic: create
test cases where ProvisioningNetworkGateway is (1) outside
ProvisioningNetworkCIDR, (2) equal to ProvisioningIP, (3) falls inside
ProvisioningDHCPRange, and (4) omitted so getProvisioningNetworkMode() defaults
to Managed; for each case instantiate a prov object (using fields
ProvisioningIP, ProvisioningNetworkCIDR, ProvisioningDHCPRange,
ProvisioningNetworkGateway and method getProvisioningNetworkMode()) and assert
the expected validation error or success. Add similar cases to the other
described test ranges (around the earlier blocks at the mentioned offsets) to
ensure coverage for both Managed and Unmanaged control paths.
In `@provisioning/baremetal_pod_test.go`:
- Line 250: Add a positive-path test case that verifies non-empty gateway
propagation: in the test table inside provisioning/baremetal_pod_test.go add a
ManagedSpec entry with ProvisioningNetworkGateway set (e.g., "192.0.2.1") and
assert that the resulting env var list contains an entry with Name "GATEWAY_IP"
and Value equal to that gateway string; update the test that currently only
checks {Name: "GATEWAY_IP", Value: ""} to include this new case and its expected
value so the provisioning logic that reads
ManagedSpec.ProvisioningNetworkGateway is validated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 935de458-c818-4195-9dc6-23a9f6c85b03
📒 Files selected for processing (10)
README.mdapi/v1alpha1/provisioning_types.goapi/v1alpha1/provisioning_validation.goconfig/crd/bases/metal3.io_provisionings.yamlmanifests/0000_31_cluster-baremetal-operator_02_metal3provisioning.crd.yamlprovisioning/baremetal_config.goprovisioning/baremetal_pod.goprovisioning/baremetal_pod_test.goprovisioning/image_cache.goprovisioning/image_customization.go
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
provisioning/baremetal_config.go (1)
128-133:⚠️ Potential issue | 🟠 MajorHonor the implicit Managed default when exporting
GATEWAY_IP.
ValidateBaremetalProvisioningConfig()treats an emptyspec.provisioningNetworkas Managed unlessprovisioningDHCPExternalflips it to Unmanaged. This helper only accepts an explicitManaged, so defaulted CRs validate successfully but never getGATEWAY_IPin the pod env.Possible fix
func getGatewayIP(config *metal3iov1alpha1.ProvisioningSpec) *string { - if config.ProvisioningNetwork == metal3iov1alpha1.ProvisioningNetworkManaged && - config.ProvisioningNetworkGateway != "" { + managed := config.ProvisioningNetwork == metal3iov1alpha1.ProvisioningNetworkManaged || + (config.ProvisioningNetwork == "" && !config.ProvisioningDHCPExternal) + if managed && config.ProvisioningNetworkGateway != "" { return &config.ProvisioningNetworkGateway } return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@provisioning/baremetal_config.go` around lines 128 - 133, getGatewayIP currently only returns the gateway when spec.ProvisioningNetwork == ProvisioningNetworkManaged, but ValidateBaremetalProvisioningConfig treats an empty ProvisioningNetwork as Managed (unless ProvisioningDHCPExternal flips it), so defaulted CRs miss GATEWAY_IP; update getGatewayIP to treat an empty ProvisioningNetwork as Managed when ProvisioningDHCPExternal is false/unspecified (i.e., treat "" the same as ProvisioningNetworkManaged unless ProvisioningDHCPExternal indicates Unmanaged), and still return &config.ProvisioningNetworkGateway when ProvisioningNetworkGateway != "" so exported env includes GATEWAY_IP for defaulted CRs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/v1alpha1/provisioning_validation.go`:
- Around line 186-201: The gateway validation must reject the subnet network and
broadcast addresses for IPv4 so they aren't used as provisioningNetworkGateway;
after parsing gateway (variable gateway) and confirming provisioningCIDR
contains it, detect IPv4 (gateway.To4() != nil), get the prefix length from
provisioningCIDR.Mask.Size(), and if the prefix length is <= 30 (i.e. there are
usable host addresses) compute the network address (provisioningCIDR.IP masked)
and the broadcast address (network | ^mask) and append errors if gateway equals
either the network or the broadcast address; keep the existing check that
gateway != provisioningIP (provisioningIP) intact.
---
Duplicate comments:
In `@provisioning/baremetal_config.go`:
- Around line 128-133: getGatewayIP currently only returns the gateway when
spec.ProvisioningNetwork == ProvisioningNetworkManaged, but
ValidateBaremetalProvisioningConfig treats an empty ProvisioningNetwork as
Managed (unless ProvisioningDHCPExternal flips it), so defaulted CRs miss
GATEWAY_IP; update getGatewayIP to treat an empty ProvisioningNetwork as Managed
when ProvisioningDHCPExternal is false/unspecified (i.e., treat "" the same as
ProvisioningNetworkManaged unless ProvisioningDHCPExternal indicates Unmanaged),
and still return &config.ProvisioningNetworkGateway when
ProvisioningNetworkGateway != "" so exported env includes GATEWAY_IP for
defaulted CRs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 758c0796-a847-4b26-bf1b-910af2f894db
📒 Files selected for processing (8)
README.mdapi/v1alpha1/provisioning_types.goapi/v1alpha1/provisioning_validation.goconfig/crd/bases/metal3.io_provisionings.yamlmanifests/0000_31_cluster-baremetal-operator_02_metal3provisioning.crd.yamlprovisioning/baremetal_config.goprovisioning/baremetal_pod.goprovisioning/baremetal_pod_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- api/v1alpha1/provisioning_types.go
- config/crd/bases/metal3.io_provisionings.yaml
- provisioning/baremetal_pod_test.go
- provisioning/baremetal_pod.go
There was a problem hiding this comment.
🧹 Nitpick comments (2)
api/v1alpha1/provisioning_validation.go (1)
217-236: ParsegatewayIPonce and reuse it in both validation blocks.Line 219 and Line 279 parse the same value independently. Reusing a single parsed
gatewayvariable keeps validation behavior consistent and reduces maintenance risk.♻️ Proposed refactor
func validateProvisioningNetworkSettings(ip string, cidr string, dhcpRange string, gatewayIP string, provisioningNetworkMode ProvisioningNetwork) []error { @@ - // Validate gateway IP if provided + var gateway net.IP + // Validate gateway IP if provided if gatewayIP != "" { - gateway := net.ParseIP(gatewayIP) + gateway = net.ParseIP(gatewayIP) if gateway == nil { errs = append(errs, fmt.Errorf("could not parse provisioningNetworkGateway %q", gatewayIP)) return errs @@ if start != nil && end != nil { @@ // Ensure gateway IP is not in the DHCP range if provided - if gatewayIP != "" { - gateway := net.ParseIP(gatewayIP) - if gateway != nil && bytes.Compare(gateway, start) >= 0 && bytes.Compare(gateway, end) <= 0 { - errs = append(errs, fmt.Errorf("invalid provisioningNetworkGateway %q, value must be outside of the provisioningDHCPRange %q", gatewayIP, dhcpRange)) - } + if gateway != nil && bytes.Compare(gateway, start) >= 0 && bytes.Compare(gateway, end) <= 0 { + errs = append(errs, fmt.Errorf("invalid provisioningNetworkGateway %q, value must be outside of the provisioningDHCPRange %q", gatewayIP, dhcpRange)) } }As per coding guidelines, "-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
Also applies to: 277-283
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v1alpha1/provisioning_validation.go` around lines 217 - 236, The gateway IP (gatewayIP) is parsed twice; parse it once into a single variable (e.g., gateway := net.ParseIP(gatewayIP)) before the gateway validation blocks and reuse that parsed "gateway" in both validation sections (the block that checks Contains, Equal against provisioningIP, and isNetworkOrBroadcastAddress and the later block around lines 277-283) to ensure consistent behavior; keep the same nil-check (append error and return errs) after parsing and remove the duplicate net.ParseIP call in the later validation so all checks reference the same parsed gateway value.provisioning/baremetal_config_test.go (1)
218-221: Add directgetMetal3DeploymentConfig(gatewayIP, ...)table coverage.The new builder setter is useful, but this file still lacks an explicit table case for the
gatewayIPconfig path (managed/default-managed vs unmanaged/disabled), which would protect this new behavior from regressions.As per coding guidelines, "-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@provisioning/baremetal_config_test.go` around lines 218 - 221, Add a table-driven test case exercising getMetal3DeploymentConfig for the gatewayIP path: add rows covering managed/default-managed and unmanaged/disabled scenarios and use the provisioningBuilder.ProvisioningNetworkGateway(...) setter to set the input gateway value for each row; assert the produced config (from getMetal3DeploymentConfig(...)) contains or omits the gatewayIP as expected and include expected output checks in the table entry to prevent regressions of the new ProvisioningNetworkGateway behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@api/v1alpha1/provisioning_validation.go`:
- Around line 217-236: The gateway IP (gatewayIP) is parsed twice; parse it once
into a single variable (e.g., gateway := net.ParseIP(gatewayIP)) before the
gateway validation blocks and reuse that parsed "gateway" in both validation
sections (the block that checks Contains, Equal against provisioningIP, and
isNetworkOrBroadcastAddress and the later block around lines 277-283) to ensure
consistent behavior; keep the same nil-check (append error and return errs)
after parsing and remove the duplicate net.ParseIP call in the later validation
so all checks reference the same parsed gateway value.
In `@provisioning/baremetal_config_test.go`:
- Around line 218-221: Add a table-driven test case exercising
getMetal3DeploymentConfig for the gatewayIP path: add rows covering
managed/default-managed and unmanaged/disabled scenarios and use the
provisioningBuilder.ProvisioningNetworkGateway(...) setter to set the input
gateway value for each row; assert the produced config (from
getMetal3DeploymentConfig(...)) contains or omits the gatewayIP as expected and
include expected output checks in the table entry to prevent regressions of the
new ProvisioningNetworkGateway behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5bd28d92-51e2-4bd3-88b6-dae4cb9c7995
📒 Files selected for processing (9)
README.mdapi/v1alpha1/provisioning_types.goapi/v1alpha1/provisioning_validation.goconfig/crd/bases/metal3.io_provisionings.yamlmanifests/0000_31_cluster-baremetal-operator_02_metal3provisioning.crd.yamlprovisioning/baremetal_config.goprovisioning/baremetal_config_test.goprovisioning/baremetal_pod.goprovisioning/baremetal_pod_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- provisioning/baremetal_pod.go
- provisioning/baremetal_pod_test.go
- api/v1alpha1/provisioning_types.go
|
@jadhaj: An error was encountered searching for bug OCPBUGS-78151 on the Jira server at https://issues.redhat.com. No known errors were detected, please see the full error message for details. Full error message.
request failed. Please analyze the request body for more details. Status code: 403:
Please contact an administrator to resolve this issue, then request a bug refresh with 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 |
|
@jadhaj: Jira Issue OCPBUGS-78151 is in a security level that is not in the allowed security levels for this repo.
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. |
|
/refresh |
|
/jira refresh |
|
@jadhaj: Jira Issue OCPBUGS-78151 is in a security level that is not in the allowed security levels for this repo.
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 |
|
@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: 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. |
|
@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. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
api/v1alpha1/provisioning_validation_test.go (1)
196-210: /31 and /32 cases don’t isolate the intended gateway behavior.These two cases keep the default
ProvisioningIP(172.30.20.3), which is outside the configured/31and/32CIDRs. The tests can therefore pass without actually proving the “usable host address” edge-case behavior for gateway values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v1alpha1/provisioning_validation_test.go` around lines 196 - 210, The two test cases ("ValidManagedGatewayOn31Network" and "ValidManagedGatewayOn32Network") still use the default ProvisioningIP (172.30.20.3) which lies outside the /31 and /32 CIDRs, so they don't exercise the gateway-in-CIDR edge case; update each spec built via managedProvisioning() to set a ProvisioningIP inside the configured CIDR using ProvisioningIP(...) (e.g., for the /31 case use an IP like 172.30.20.1 that falls within 172.30.20.0/31 and for the /32 case use the same host IP 172.30.20.1 for 172.30.20.1/32) so the test actually validates the “usable host address” behavior for ProvisioningNetworkGateway/ProvisioningIP.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/v1alpha1/provisioning_validation.go`:
- Around line 102-131: The isNetworkOrBroadcastAddress function incorrectly
treats IPv6 addresses as network addresses because the IPv6 early-return (bits
== 128) happens after the network-address check; update the function so the IPv6
short-circuit runs before any network/broadcast comparisons (i.e., check if bits
== 128 and return false prior to computing networkAddr and comparing
ip.Equal(cidr.IP.Mask(cidr.Mask))). This preserves the IPv4-focused behavior
while ensuring an IPv6 gateway equal to cidr.IP is not rejected.
---
Nitpick comments:
In `@api/v1alpha1/provisioning_validation_test.go`:
- Around line 196-210: The two test cases ("ValidManagedGatewayOn31Network" and
"ValidManagedGatewayOn32Network") still use the default ProvisioningIP
(172.30.20.3) which lies outside the /31 and /32 CIDRs, so they don't exercise
the gateway-in-CIDR edge case; update each spec built via managedProvisioning()
to set a ProvisioningIP inside the configured CIDR using ProvisioningIP(...)
(e.g., for the /31 case use an IP like 172.30.20.1 that falls within
172.30.20.0/31 and for the /32 case use the same host IP 172.30.20.1 for
172.30.20.1/32) so the test actually validates the “usable host address”
behavior for ProvisioningNetworkGateway/ProvisioningIP.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ee156305-3d3c-4372-a505-330d88b9cebe
📒 Files selected for processing (10)
README.mdapi/v1alpha1/provisioning_types.goapi/v1alpha1/provisioning_validation.goapi/v1alpha1/provisioning_validation_test.goconfig/crd/bases/metal3.io_provisionings.yamlmanifests/0000_31_cluster-baremetal-operator_02_metal3provisioning.crd.yamlprovisioning/baremetal_config.goprovisioning/baremetal_config_test.goprovisioning/baremetal_pod.goprovisioning/baremetal_pod_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- provisioning/baremetal_config_test.go
- manifests/0000_31_cluster-baremetal-operator_02_metal3provisioning.crd.yaml
- provisioning/baremetal_config.go
- provisioning/baremetal_pod.go
|
/retest-required |
|
LGTM aside from the nits |
|
/verified by @jadhaj |
|
@jadhaj: 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. |
|
/test unit |
Update the CRD manifest to match the generated output from controller-gen.
|
@jadhaj: 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. |
|
@dtantsur review comments addressed (hopefully) and all test passed |
|
/verified by @jadhaj |
|
@jadhaj: 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. |
No description provided.