Skip to content

OSASINFRA-4400: make openstack allowed address pairs configurable #230

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
danchild wants to merge 2 commits into
base: main
Choose a base branch
from
+109 −13
Open

OSASINFRA-4400: make openstack allowed address pairs configurable #230

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ce24807
Add platform-os-max-allowed-address-pairs arg
danchild Jul 9, 2026
d1a1b28
Add unit test coverage for the new platform-os-max-allowed-address-pa…
danchild Jul 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions cmd/cloud-network-config-controller/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,7 @@ func init() {
flag.StringVar(&platformCfg.AzureEnvironment, "platform-azure-environment", "AzurePublicCloud", "The Azure environment name, used to select API endpoints")
flag.StringVar(&platformCfg.AWSCAOverride, "platform-aws-ca-override", "", "Path to a separate CA bundle to use when connecting to the AWS API")
flag.StringVar(&kubeConfig, "kubeconfig", "", "Path to a kubeconfig. Only required if out-of-cluster.")
flag.IntVar(&platformCfg.OSMaxAllowedAddressPairs, "platform-os-max-allowed-address-pairs", 0, "Maximum number of allowed address pairs per OpenStack port. Overrides the Neutron default of 10.")
flag.Parse()

// Verify required arguments
Expand Down
3 changes: 2 additions & 1 deletion pkg/cloudprovider/cloudprovider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,8 @@ type CloudProviderConfig struct {
Region string // region, only used by AWS
AWSCAOverride string

AzureEnvironment string // The azure "environment", which is a set of API endpoints
AzureEnvironment string // The azure "environment", which is a set of API endpoints
OSMaxAllowedAddressPairs int
}

type CloudProvider struct {
Expand Down
19 changes: 16 additions & 3 deletions pkg/cloudprovider/openstack.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ const (
// to the default value of 10, always; and we might want to document that OSP environments must set
// max_allowed_address_pairs >= 10. For more details, see:
// https://github.com/openstack/neutron/blob/800f863ccc502b334cb2dd79ec54066440e43e27/neutron/conf/extensions/allowedaddresspairs.py#L21
openstackMaxCapacity = 10
defaultOpenstackMaxCapacity = 10
)

// OpenStack implements the API wrapper for talking
Expand Down Expand Up @@ -549,8 +549,9 @@ func (o *OpenStack) GetNodeEgressIPConfiguration(node *corev1.Node, cpicIPs sets
// The interface is keyed by a neutron UUID.
// If multiple IPv4 repectively multiple IPv6 subnets are attached to the same port, throw an error.
// The IP capacity is per port. The definition of this field does unfortunately not play very well with the way how
// neutron operates as there is no such thing as a per port quota or limit. Therefore we set a ceiling of
// `openstackMaxCapacity`. The number of unique IP addresses in allowed_address_pair and fixed_ips is subtracted from
// neutron operates as there is no such thing as a per port quota or limit. However, a quota can be set to mirror
// the max_allowed_address_pairs configuration in neutron.conf, and when unset, it will default to
// `defaultOpenstackMaxCapacity`. The number of unique IP addresses in allowed_address_pair and fixed_ips is subtracted from
// that ceiling.
func (o *OpenStack) getNeutronPortNodeEgressIPConfiguration(p neutronports.Port, cpicIPs sets.Set[string]) (*NodeEgressIPConfiguration, error) {
var ipv4, ipv6 string
Expand Down Expand Up @@ -597,7 +598,19 @@ func (o *OpenStack) getNeutronPortNodeEgressIPConfiguration(p neutronports.Port,
}
}

var openstackMaxCapacity int

if o.cfg.OSMaxAllowedAddressPairs > 0 {
openstackMaxCapacity = o.cfg.OSMaxAllowedAddressPairs
} else {
openstackMaxCapacity = defaultOpenstackMaxCapacity
}

c := openstackMaxCapacity + cloudPrivateIPsCount - len(p.AllowedAddressPairs)
if c < 0 {
return nil, fmt.Errorf("max allowed address pairs (%d) is too small for port %s which already has (%d) allowed address pairs and (%d) managed by CNCC",
openstackMaxCapacity, p.ID, len(p.AllowedAddressPairs), cloudPrivateIPsCount)
}
Comment on lines +611 to +613

@coderabbitai coderabbitai Bot Jul 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Remove parentheses around the format specifiers.

The formatting (%d) around the integer counts produces unusual output (e.g., "which already has (5) allowed address pairs"). Removing the parentheses improves readability. Additionally, this avoids breaking test assertions that check for unparenthesized substrings, as observed in the accompanying tests.

💡 Proposed fix 
-		return nil, fmt.Errorf("max allowed address pairs (%d) is too small for port %s which already has (%d) allowed address pairs and (%d) managed by CNCC",
+		return nil, fmt.Errorf("max allowed address pairs (%d) is too small for port %s which already has %d allowed address pairs and %d managed by CNCC",
 			openstackMaxCapacity, p.ID, len(p.AllowedAddressPairs), cloudPrivateIPsCount)
📝 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.

Suggested change
return nil, fmt.Errorf("max allowed address pairs (%d) is too small for port %s which already has (%d) allowed address pairs and (%d) managed by CNCC",
openstackMaxCapacity, p.ID, len(p.AllowedAddressPairs), cloudPrivateIPsCount)
}
return nil, fmt.Errorf("max allowed address pairs (%d) is too small for port %s which already has %d allowed address pairs and %d managed by CNCC",
openstackMaxCapacity, p.ID, len(p.AllowedAddressPairs), cloudPrivateIPsCount)
}
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/cloudprovider/openstack.go` around lines 611 - 613, Update the error
message format string in the address-pair capacity check to remove parentheses
around all integer format specifiers, while preserving the existing values,
wording, and argument order.

return &NodeEgressIPConfiguration{
Interface: p.ID,
IFAddr: ifAddr{
Expand Down
99 changes: 90 additions & 9 deletions pkg/cloudprovider/openstack_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -685,7 +685,7 @@ func TestOpenStackPlugin(t *testing.T) {
IPv6: "2000::/64",
},
Capacity: capacity{
IP: ptr.To(openstackMaxCapacity),
IP: ptr.To(defaultOpenstackMaxCapacity),
},
},
}
Expand Down Expand Up @@ -812,7 +812,7 @@ func TestGetNodeEgressIPConfiguration(t *testing.T) {
IPv6: "2000::/64",
},
Capacity: capacity{
IP: ptr.To(openstackMaxCapacity),
IP: ptr.To(defaultOpenstackMaxCapacity),
},
},
},
Expand Down Expand Up @@ -848,7 +848,7 @@ func TestGetNodeEgressIPConfiguration(t *testing.T) {
IPv6: "2001::/64",
},
Capacity: capacity{
IP: ptr.To(openstackMaxCapacity),
IP: ptr.To(defaultOpenstackMaxCapacity),
},
},
},
Expand Down Expand Up @@ -892,7 +892,7 @@ func TestGetNodeEgressIPConfiguration(t *testing.T) {
IPv6: "2000::/64",
},
Capacity: capacity{
IP: ptr.To(openstackMaxCapacity),
IP: ptr.To(defaultOpenstackMaxCapacity),
},
},
},
Expand Down Expand Up @@ -939,7 +939,7 @@ func TestGetNodeEgressIPConfiguration(t *testing.T) {
IPv6: "2000::/64",
},
Capacity: capacity{
IP: ptr.To(openstackMaxCapacity),
IP: ptr.To(defaultOpenstackMaxCapacity),
},
},
},
Expand All @@ -951,7 +951,7 @@ func TestGetNodeEgressIPConfiguration(t *testing.T) {
IPv6: "2001::/64",
},
Capacity: capacity{
IP: ptr.To(openstackMaxCapacity),
IP: ptr.To(defaultOpenstackMaxCapacity),
},
},
},
Expand Down Expand Up @@ -1036,7 +1036,7 @@ func TestGetNeutronPortNodeEgressIPConfiguration(t *testing.T) {
IPv6: "2000::/64",
},
Capacity: capacity{
IP: ptr.To(openstackMaxCapacity - 5), // 5 allowed_address_pairs configured on the port.
IP: ptr.To(defaultOpenstackMaxCapacity - 5), // 5 allowed_address_pairs configured on the port.
},
},
},
Expand All @@ -1049,7 +1049,7 @@ func TestGetNeutronPortNodeEgressIPConfiguration(t *testing.T) {
IPv6: "2000::/64",
},
Capacity: capacity{
IP: ptr.To(openstackMaxCapacity - 2), // excluding 2 allowed_address_pairs configured on the port.
IP: ptr.To(defaultOpenstackMaxCapacity - 2), // excluding 2 allowed_address_pairs configured on the port.
},
},
// Configure IPs with 3 ips are within neutron subnet, 1 ip outside neutron subnet.
Expand All @@ -1066,7 +1066,7 @@ func TestGetNeutronPortNodeEgressIPConfiguration(t *testing.T) {
}

for i, tc := range tcs {
cpicIPs := sets.New[string](tc.cloudPrivateIPConfigs...)
cpicIPs := sets.New(tc.cloudPrivateIPConfigs...)
nodeEgressIPConfig, err := o.getNeutronPortNodeEgressIPConfiguration(tc.port, cpicIPs)
if err != nil {
if !strings.Contains(err.Error(), tc.errString) {
Expand All @@ -1081,6 +1081,87 @@ func TestGetNeutronPortNodeEgressIPConfiguration(t *testing.T) {
}
}

func TestGetNeutronPortNodeEgressIPConfigurationMaxAllowedAddressPairs(t *testing.T) {
th.SetupHTTP()
defer th.TeardownHTTP()
HandleSubnetList(t)
HandlePortListAndCreation(t)

tcs := []struct {
name string
maxAllowedAddrPairs int
port neutronports.Port
nodeEgressIPConfig NodeEgressIPConfiguration
cloudPrivateIPConfigs []string
errString string
}{
{
name: "custom positive value overrides default",
maxAllowedAddrPairs: 20,
port: portMap["9ab428d4-58f8-42d7-9672-90c3f5641f83"],
nodeEgressIPConfig: NodeEgressIPConfiguration{
Interface: "9ab428d4-58f8-42d7-9672-90c3f5641f83",
IFAddr: ifAddr{
IPv4: "192.0.2.0/24",
IPv6: "2000::/64",
},
Capacity: capacity{
IP: ptr.To(20 - 5), // 5 allowed_address_pairs on the port
},
},
},
{
name: "max too small for existing address pairs",
maxAllowedAddrPairs: 3,
port: portMap["9ab428d4-58f8-42d7-9672-90c3f5641f83"],
errString: "max allowed address pairs (3) is too small for port 9ab428d4-58f8-42d7-9672-90c3f5641f83 which already has (5) allowed address pairs and (0) managed by CNCC",
},
{
name: "unset flag uses default capacity",
maxAllowedAddrPairs: 0,
port: portMap["9ab428d4-58f8-42d7-9672-90c3f5641f83"],
nodeEgressIPConfig: NodeEgressIPConfiguration{
Interface: "9ab428d4-58f8-42d7-9672-90c3f5641f83",
IFAddr: ifAddr{
IPv4: "192.0.2.0/24",
IPv6: "2000::/64",
},
Capacity: capacity{
IP: ptr.To(defaultOpenstackMaxCapacity - 5),
},
},
},
}

for _, tc := range tcs {
t.Run(tc.name, func(t *testing.T) {
o := OpenStack{
CloudProvider: CloudProvider{
cfg: CloudProviderConfig{
OSMaxAllowedAddressPairs: tc.maxAllowedAddrPairs,
},
},
novaClient: testclient.ServiceClient(),
neutronClient: testclient.ServiceClient(),
}
cpicIPs := sets.New(tc.cloudPrivateIPConfigs...)
nodeEgressIPConfig, err := o.getNeutronPortNodeEgressIPConfiguration(tc.port, cpicIPs)
if err != nil {
if tc.errString == "" || !strings.Contains(err.Error(), tc.errString) {
t.Fatalf("Received unexpected error, err: %q, expected to contain: %q", err, tc.errString)
}
return
}
if tc.errString != "" {
t.Fatalf("Expected error containing %q but got none", tc.errString)
}
if !reflect.DeepEqual(*nodeEgressIPConfig, tc.nodeEgressIPConfig) {
t.Fatalf("Received unexpected nodeEgressIPConfig.\nExpected: %v\nGot: %v", tc.nodeEgressIPConfig, nodeEgressIPConfig)
}
})
}
}

// TestAllowUnAllowIPAddressOnNeutronPort tests both allowIPAddressOnNeutronPort and
// unAllowIPAddressOnNeutronPort.
func TestAllowUnAllowIPAddressOnNeutronPort(t *testing.T) {
Expand Down