OCPBUGS-69399: Allow ProvisioningCIDR for unmanaged network#553
Conversation
openshift-ci-robot
added
jira/valid-reference
|
@hroyrh: This pull request references Jira Issue OCPBUGS-69399, 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
jira/valid-bug
|
@hroyrh: This pull request references Jira Issue OCPBUGS-69399, 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. |
hroyrh
force-pushed
the
provisioning-cidr-unmanaged
branch
from
044ecb8 to
3b7c5ec
Compare
|
/test unit |
2 similar comments
|
/test unit |
|
/test unit |
| } | ||
|
|
||
| if provisioningNetworkMode != ProvisioningNetworkManaged { | ||
| if provisioningNetworkMode != ProvisioningNetworkManaged && provisioningNetworkMode != ProvisioningNetworkUnmanaged { |
There was a problem hiding this comment.
nit: if provisioningNetworkMode == ProvisioningNetworkDisabled
| return errs | ||
| } | ||
| // Verify Network CIDR | ||
| _, provisioningCIDR, err := net.ParseCIDR(cidr) |
There was a problem hiding this comment.
We aren't making the CIDR mandatory by reaching this code, are we?
There was a problem hiding this comment.
That shouldn't happen since we already return early for disabled provisioning network and we require CIDR for other cases, right ? I don't think we should allow empty CIDR for unmanaged network.
provisioning/baremetal_config.go
Outdated
| if config.ProvisioningNetwork == metal3iov1alpha1.ProvisioningNetworkManaged { | ||
| // Use provisioningCIDR for unmanaged provisioning network as well so that corresponding networking configuration | ||
| // happens at the time of deployment. | ||
| if config.ProvisioningNetwork == metal3iov1alpha1.ProvisioningNetworkManaged || config.ProvisioningNetwork == metal3iov1alpha1.ProvisioningNetworkUnmanaged { |
There was a problem hiding this comment.
nit: if provisioningNetworkMode != ProvisioningNetworkDisabled
In c00ff0a, provisioningIPCIDR is used only for managed network. Due to this, routes are not created for unmanaged network during deployment, which leads to broken networking. So, we are allowing provisioningCIDR for both managed and unmanaged networks. Signed-off-by: Himanshu Roy <hroy@redhat.com>
hroyrh
force-pushed
the
provisioning-cidr-unmanaged
branch
from
3b7c5ec to
b16f413
Compare
WalkthroughThe changes enable CIDR validation and formatting for unmanaged provisioning configurations. Previously, validation logic was skipped for non-Managed modes; now it runs for Unmanaged mode (while remaining skipped for Disabled). Provisioning IP is formatted with CIDR prefix for unmanaged networks, alongside corresponding test updates. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
|
@hroyrh: This pull request references Jira Issue OCPBUGS-69399, 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
♻️ Duplicate comments (1)
api/v1alpha1/provisioning_validation.go (1)
162-169:⚠️ Potential issue | 🟠 MajorDon't turn
provisioningNetworkCIDRinto a hard requirement for unmanaged mode.This branch now sends every unmanaged spec through
net.ParseCIDR(cidr), so an unmanaged config that only setsProvisioningIPstarts failing validation here. The newgetProvisioningIP()path also dropsPROVISIONING_IPin that case, so this is a backward-incompatible regression beyond “allow CIDR when present”. Please keep the CIDR-specific checks conditional for unmanaged mode and preserve the raw-IP fallback whenProvisioningNetworkCIDRis empty.🤖 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 162 - 169, The current validation forces parsing provisioningNetworkCIDR for unmanaged specs and rejects configs that only set ProvisioningIP; change the logic so CIDR-specific validation only runs when a CIDR is actually provided: in the block that currently calls net.ParseCIDR(cidr) (using provisioningNetworkMode, provisioningNetworkCIDR/provisioningCIDR and err variables), wrap or gate that parse/validation behind a check that cidr != "" (or the equivalent provisioningNetworkCIDR presence) and skip to the existing raw-IP fallback (getProvisioningIP / ProvisioningIP) when empty; ensure unmanaged-mode flows do not require a CIDR and preserve existing raw IP handling.
🧹 Nitpick comments (1)
provisioning/baremetal_config_test.go (1)
49-54: Please add the unmanaged-without-CIDR case here too.This only pins the new CIDR-present path. A companion case with
ProvisioningNetworkCIDR == ""would protect the existing unmanaged behavior while still covering the new route-creation flow.🤖 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 49 - 54, Add a companion test case for the unmanaged flow that leaves ProvisioningNetworkCIDR empty: copy the existing "Unmanaged ProvisioningIPCIDR" case (configName provisioningIP, spec from unmanagedProvisioning().build()) and add a new case named like "Unmanaged ProvisioningIP without CIDR" where you set the spec's ProvisioningNetworkCIDR to "" (e.g. via unmanagedProvisioning().withProvisioningNetworkCIDR("").build() or by clearing the field before build) and set expectedValue to the prior unmanaged behavior value (the same expectedValue used by the unmanaged tests before CIDR was introduced) so the test covers both CIDR-present and CIDR-empty code paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@api/v1alpha1/provisioning_validation.go`:
- Around line 162-169: The current validation forces parsing
provisioningNetworkCIDR for unmanaged specs and rejects configs that only set
ProvisioningIP; change the logic so CIDR-specific validation only runs when a
CIDR is actually provided: in the block that currently calls net.ParseCIDR(cidr)
(using provisioningNetworkMode, provisioningNetworkCIDR/provisioningCIDR and err
variables), wrap or gate that parse/validation behind a check that cidr != ""
(or the equivalent provisioningNetworkCIDR presence) and skip to the existing
raw-IP fallback (getProvisioningIP / ProvisioningIP) when empty; ensure
unmanaged-mode flows do not require a CIDR and preserve existing raw IP
handling.
---
Nitpick comments:
In `@provisioning/baremetal_config_test.go`:
- Around line 49-54: Add a companion test case for the unmanaged flow that
leaves ProvisioningNetworkCIDR empty: copy the existing "Unmanaged
ProvisioningIPCIDR" case (configName provisioningIP, spec from
unmanagedProvisioning().build()) and add a new case named like "Unmanaged
ProvisioningIP without CIDR" where you set the spec's ProvisioningNetworkCIDR to
"" (e.g. via unmanagedProvisioning().withProvisioningNetworkCIDR("").build() or
by clearing the field before build) and set expectedValue to the prior unmanaged
behavior value (the same expectedValue used by the unmanaged tests before CIDR
was introduced) so the test covers both CIDR-present and CIDR-empty code paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fab7d64c-3f95-4da8-837c-8d196522daef
📒 Files selected for processing (5)
api/v1alpha1/provisioning_validation.goapi/v1alpha1/provisioning_validation_test.goprovisioning/baremetal_config.goprovisioning/baremetal_config_test.goprovisioning/baremetal_pod_test.go
|
/retest-required |
1 similar comment
|
/retest-required |
|
@hroyrh: 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. |
|
/verified later @jadhaj |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: elfosardo, hroyrh 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 |
openshift-ci
bot
added
the
approved
|
/lgtm |
|
/verified later @jadhaj |
1 similar comment
|
/verified later @jadhaj |
|
@jadhaj: This PR has been marked to be verified later 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. |
openshift-ci-robot
added
the
verified
|
@hroyrh: Jira Issue OCPBUGS-69399: All pull requests linked via external trackers have merged: This pull request has the 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. |
|
/cherrypick release-4.21 |
|
@hroyrh: new pull request created: #576 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. |
8 participants
As provisioningCIDR was used only for managed network, it caused broken networking for unmanaged provisioning networking scenarios because all routes were not created. To fix the issue, this pr allows provisioningCIDR for both managed and unmanaged networks.
Summary by CodeRabbit
Bug Fixes
Tests