OCPBUGS-67159: openstack: Reserve addresses for load balancer#10375
OCPBUGS-67159: openstack: Reserve addresses for load balancer#10375stephenfin wants to merge 1 commit intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
jira/severity-important
|
@stephenfin: This pull request references Jira Issue OCPBUGS-67159, 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. |
WalkthroughModified OpenStack cluster manifest generation to compute subnet allocation pools using the go-cidr library. When no control plane port is specified, the code derives subnet CIDR, allocation start, and allocation end from the install configuration and populates Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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)
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.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions 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
🤖 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/asset/manifests/openstack/cluster.go`:
- Around line 90-100: The allocation pool end currently uses the CIDR's last
address from cidr.AddressRange (allocationEnd), which for IPv4 is the broadcast
address and is rejected by Neutron; update the logic where
openStackCluster.Spec.ManagedSubnets is built so that after calling
cidr.AddressRange(&networkCIDR.IPNet) you detect IPv4 (networkCIDR.IP.To4() !=
nil) and decrement allocationEnd by one IP (subtract one from the byte
representation) to exclude the broadcast address before assigning it to
capo.AllocationPool.End; keep allocationStart as returned and only adjust
allocationEnd for IPv4 to match the baremetal pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1de8d424-77a5-425e-951e-15b25b5bf6bf
📒 Files selected for processing (1)
pkg/asset/manifests/openstack/cluster.go
mandre
left a comment
mandre
left a comment
There was a problem hiding this comment.
This restores the behavior we had with the terraform-based installer:
installer/data/data/openstack/masters/private-network.tf
Lines 40 to 43 in 8b8a02b
/lgtm
|
/lgtm cancel CodeRabbit caught a potential bug: we got the allocation pool too big, we should drop the last 2 addresses from the end. |
stephenfin
force-pushed
the
OCPBUGS-67159
branch
from
b0164ea to
b90f601
Compare
When using Octavia, we use the .5 and .7 addresses on the first machine network for API and Ingress VIP, respectively [1]. However, we were not reserving these address when creating the network. This can result in an IP collision if another service happens to allocate one of these two IPs before the load balancer is created. This can happen if DHCP agents are running on the controller/network nodes, as each agent creates its own DHCP port on the subnet. The solution is to use allocation pools - which are designed exactly for this purpose - and configure one on the subnet we create (via CAPO). We reserve everything < .10 in case we need more addresses down the line. We were previously doing this with Terraform but lost this during the migration to CAPO. [1] `SetPlatformDefaults` in `pkg/types/openstack/defaults/platform.go` [2] https://github.com/openshift/installer/blob/release-4.20/data/data/openstack/masters/private-network.tf#L37-L43 Signed-off-by: Stephen Finucane <stephenfin@redhat.com>
stephenfin
force-pushed
the
OCPBUGS-67159
branch
from
b90f601 to
1a220e5
Compare
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@stephenfin: This pull request references Jira Issue OCPBUGS-67159, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira (imatza@redhat.com), skipping review request. 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
bot
added
lgtm
|
@stephenfin: This pull request references Jira Issue OCPBUGS-67159, which is valid. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira (imatza@redhat.com), skipping review request. 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. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/asset/manifests/openstack/cluster.go (1)
85-101: Please add a regression test for the allocation-pool bounds.This path now depends on a couple of boundary decisions (
.10start and last-usable end). A focused unit test here would make future off-by-one regressions much harder.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/asset/manifests/openstack/cluster.go` around lines 85 - 101, Add a focused unit test that asserts the allocation-pool bounds produced in openStackCluster.Spec.ManagedSubnets match the intended boundary decisions (start at host .10 and end at the last-usable IP). In a new test in the same package, create a deterministic installConfig CIDR, call the function that builds the OpenStack cluster object (the code path using capiutils.CIDRFromInstallConfig and producing capo.SubnetSpec with AllocationPool), then assert that the AllocationPool.Start equals the value returned by cidr.Host(&networkCIDR.IPNet, 10).String() and AllocationPool.End equals cidr.Dec(broadcastIP).String() where broadcastIP comes from cidr.AddressRange(&networkCIDR.IPNet); fail the test if those values differ to prevent off-by-one regressions.
🤖 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/asset/manifests/openstack/cluster.go`:
- Around line 90-91: The code unconditionally decrements allocationEnd via
cidr.Dec(broadcastIP), which wrongly removes one usable address for IPv6; change
it so you only decrement for IPv4 subnets by detecting IPv4 (e.g., test
networkCIDR.IPNet.IP.To4() != nil) before calling cidr.Dec; keep the use of
cidr.AddressRange to obtain broadcastIP but only set allocationEnd =
cidr.Dec(broadcastIP) when the IP is IPv4, otherwise use the broadcastIP (or its
equivalent) directly for IPv6 in the allocationEnd variable.
---
Nitpick comments:
In `@pkg/asset/manifests/openstack/cluster.go`:
- Around line 85-101: Add a focused unit test that asserts the allocation-pool
bounds produced in openStackCluster.Spec.ManagedSubnets match the intended
boundary decisions (start at host .10 and end at the last-usable IP). In a new
test in the same package, create a deterministic installConfig CIDR, call the
function that builds the OpenStack cluster object (the code path using
capiutils.CIDRFromInstallConfig and producing capo.SubnetSpec with
AllocationPool), then assert that the AllocationPool.Start equals the value
returned by cidr.Host(&networkCIDR.IPNet, 10).String() and AllocationPool.End
equals cidr.Dec(broadcastIP).String() where broadcastIP comes from
cidr.AddressRange(&networkCIDR.IPNet); fail the test if those values differ to
prevent off-by-one regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 696abdfd-c952-4593-ad8a-df0f19fb11ae
📒 Files selected for processing (1)
pkg/asset/manifests/openstack/cluster.go
|
/test e2e-openstack-proxy |
|
/verified later by @stephenfin |
|
@stephenfin: Only users can be targets for 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gryf, mandre 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
|
/verified later stephenfin |
|
@stephenfin: Only users can be targets for 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. |
|
@stephenfin: 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. |
|
/test e2e-openstack-proxy |
4 participants
When using Octavia, we use the
.5and.7addresses on the first machine network for API and Ingress VIP, respectively. However, we were not reserving these address when creating the network. This can result in an IP collision if another service happens to allocate one of these two IPs before the load balancer is created. This can happen if DHCP agents are running on the controller/network nodes, as each agent creates its own DHCP port on the subnet.The solution is to use allocation pools - which are designed exactly for this purpose - and configure one on the subnet we create (via CAPO). We reserve everything < .10 in case we need more addresses down the line.
Summary by CodeRabbit