Skip to content

OCPBUGS-62892: openstack: Handle empty MAPO network subnets #381

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
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

OCPBUGS-62892: openstack: Handle empty MAPO network subnets #381

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
06a1f5a
openstack: Trivial variable rename
stephenfin Oct 13, 2025
6004d19
OCPBUGS-62892: openstack: Handle empty MAPO network subnets
stephenfin Oct 9, 2025
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
69 changes: 38 additions & 31 deletions pkg/conversion/capi2mapi/openstack.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -354,33 +354,36 @@ func convertCAPOPortOptsToMAPONetwork(fldPath *field.Path, capoPort openstackv1.

mapoNetwork := mapiv1alpha1.NetworkParam{}

// We have already asserted that .Network is non-nil in the caller

switch {
case capoPort.Network.ID != nil:
mapoNetwork.UUID = *capoPort.Network.ID
case capoPort.Network.Filter != nil:
mapoNetwork.Filter = mapiv1alpha1.Filter{
// DeprecatedAdminStateUp is deprecated and ignored by MAPO so we don't set it
// DeprecatedLimit is deprecated and ignored by MAPO so we don't set it
// DeprecatedMarker is deprecated and ignored by MAPO so we don't set it
// DeprecatedShared is deprecated and ignored by MAPO so we don't set it
// DeprecatedSortKey is deprecated and ignored by MAPO so we don't set it
// DeprecatedSortDir is deprecated and ignored by MAPO so we don't set it
// DeprecatedStatus is deprecated and ignored by MAPO so we don't set it
Description: capoPort.Network.Filter.Description,
// ID is deprecated and covered by UUID on the parent NetworkParam so we don't set it
Name: capoPort.Network.Filter.Name,
NotTags: joinTags(capoPort.Network.Filter.NotTags),
NotTagsAny: joinTags(capoPort.Network.Filter.NotTagsAny),
ProjectID: capoPort.Network.Filter.ProjectID,
Tags: joinTags(capoPort.Network.Filter.Tags),
TagsAny: joinTags(capoPort.Network.Filter.TagsAny),
// TenantID is deprecated and covered by ProjectID so we don't set it
if capoPort.Network != nil {
switch {
case capoPort.Network != nil && capoPort.Network.ID != nil:
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: the capoPort.Network != nil is redundant now we're guarding the whole switch.

mapoNetwork.UUID = *capoPort.Network.ID
case capoPort.Network != nil && capoPort.Network.Filter != nil:
mapoNetwork.Filter = mapiv1alpha1.Filter{
// DeprecatedAdminStateUp is deprecated and ignored by MAPO so we don't set it
// DeprecatedLimit is deprecated and ignored by MAPO so we don't set it
// DeprecatedMarker is deprecated and ignored by MAPO so we don't set it
// DeprecatedShared is deprecated and ignored by MAPO so we don't set it
// DeprecatedSortKey is deprecated and ignored by MAPO so we don't set it
// DeprecatedSortDir is deprecated and ignored by MAPO so we don't set it
// DeprecatedStatus is deprecated and ignored by MAPO so we don't set it
Description: capoPort.Network.Filter.Description,
// ID is deprecated and covered by UUID on the parent NetworkParam so we don't set it
Name: capoPort.Network.Filter.Name,
NotTags: joinTags(capoPort.Network.Filter.NotTags),
NotTagsAny: joinTags(capoPort.Network.Filter.NotTagsAny),
ProjectID: capoPort.Network.Filter.ProjectID,
Tags: joinTags(capoPort.Network.Filter.Tags),
TagsAny: joinTags(capoPort.Network.Filter.TagsAny),
// TenantID is deprecated and covered by ProjectID so we don't set it
}
default:
// TODO(OSASINFRA-3779): Add VAP to prevent usage of the below field.
errors = append(errors, field.Invalid(fldPath.Child("network"), capoPort.Network, "A network must be referenced by a UUID or filter"))
}
default:
} else if len(capoPort.FixedIPs) == 0 {
// TODO(OSASINFRA-3779): Add VAP to prevent usage of the below field.
errors = append(errors, field.Invalid(fldPath.Child("network"), capoPort.Network, "A network must be referenced by a UUID or filter"))
errors = append(errors, field.Invalid(fldPath.Child("network"), capoPort.Network, "A network must be referenced by a UUID or filter, else a subnet must be referenced"))
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I think it would be less confusing to say 'Either network or fixedIPs must be specified on a port'. The bit about UUID and filter doesn't have any context here.

}

if capoPort.DisablePortSecurity != nil {
Expand All @@ -397,8 +400,13 @@ func convertCAPOPortOptsToMAPONetwork(fldPath *field.Path, capoPort openstackv1.
}

mapoSubnet := mapiv1alpha1.SubnetParam{
UUID: *capoFixedIP.Subnet.ID,
Filter: mapiv1alpha1.SubnetFilter{
UUID: *capoFixedIP.Subnet.ID,
Copy link
Contributor

Choose a reason for hiding this comment

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

I appreciate this isn't new, but what stops us panicing here if the subnet is specified by filter instead of ID? Do we have test coverage of this?

PortTags: capoPort.Tags,
// PortSecurity is deprecated and ignored by MAPO so we don't set it here
}

if capoFixedIP.Subnet.Filter != nil {
mapoSubnet.Filter = mapiv1alpha1.SubnetFilter{
CIDR: capoFixedIP.Subnet.Filter.CIDR,
Description: capoFixedIP.Subnet.Filter.Description,
GatewayIP: capoFixedIP.Subnet.Filter.GatewayIP,
Expand All @@ -412,10 +420,9 @@ func convertCAPOPortOptsToMAPONetwork(fldPath *field.Path, capoPort openstackv1.
ProjectID: capoFixedIP.Subnet.Filter.ProjectID,
Tags: joinTags(capoFixedIP.Subnet.Filter.Tags),
TagsAny: joinTags(capoFixedIP.Subnet.Filter.TagsAny),
},
PortTags: capoPort.Tags,
// PortSecurity is deprecated and ignored by MAPO so we don't set it here
}
}

mapoSubnets[i] = mapoSubnet
}

Expand Down Expand Up @@ -593,7 +600,7 @@ func convertCAPOPortOptsToMAPO(fldPath *field.Path, capoPorts []openstackv1.Port
warnings := []string{}

for i, capoPort := range capoPorts {
if capoPort.Network != nil && capoPort.Network.Filter != nil {
if (capoPort.Network != nil && capoPort.Network.Filter != nil) || (capoPort.Network == nil && len(capoPort.FixedIPs) > 0 && capoPort.FixedIPs[0].Subnet != nil) {
mapoNetwork, warns, errs := convertCAPOPortOptsToMAPONetwork(fldPath.Index(i), capoPort)
mapoNetworks = append(mapoNetworks, mapoNetwork)
errors = append(errors, errs...)
Expand Down
101 changes: 53 additions & 48 deletions pkg/conversion/mapi2capi/openstack.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -371,7 +371,7 @@ func convertMAPOImageToCAPO(mapoImage string) openstackv1.ImageParam {
}

//nolint:funlen
func convertMAPONetworksToCAPO(fldPath *field.Path, mapoNetworks []mapiv1alpha1.NetworkParam) ([]openstackv1.PortOpts, []string, field.ErrorList) { //nolint:gocognit,cyclop
func convertMAPONetworksToCAPO(fldPath *field.Path, mapoNetworks []mapiv1alpha1.NetworkParam) ([]openstackv1.PortOpts, []string, field.ErrorList) { //nolint:gocognit,cyclop,gocyclo
Copy link
Contributor

Choose a reason for hiding this comment

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

I have to agree with the linter here: this function is getting hard to read!

errors := field.ErrorList{}
warnings := []string{}

Expand All @@ -385,43 +385,46 @@ func convertMAPONetworksToCAPO(fldPath *field.Path, mapoNetworks []mapiv1alpha1.
warnings = append(warnings, field.Invalid(fldPath.Index(i).Child("fixedIP"), mapoNetwork.FixedIp, "fixedIp is ignored by MAPO, ignoring").Error())
}

network := openstackv1.NetworkParam{}
capoNetwork := openstackv1.NetworkParam{}

networkID := mapoNetwork.UUID
if networkID == "" {
networkID = mapoNetwork.Filter.ID
}

if networkID != "" {
network.ID = &networkID
capoNetwork.ID = &networkID
}

// convert .Filter
projectID := mapoNetwork.Filter.ProjectID
if projectID == "" {
projectID = mapoNetwork.Filter.TenantID
}
if (mapoNetwork.Filter != mapiv1alpha1.Filter{}) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's an error for a network to specify both ID and filter. Filter is redundant if we specified ID, so we could add an additional guard here that networkID is not set.

projectID := mapoNetwork.Filter.ProjectID
if projectID == "" {
projectID = mapoNetwork.Filter.TenantID
}

network.Filter = &openstackv1.NetworkFilter{
Name: mapoNetwork.Filter.Name,
Description: mapoNetwork.Filter.Description,
ProjectID: projectID,
FilterByNeutronTags: openstackv1.FilterByNeutronTags{
NotTags: splitTags(mapoNetwork.Filter.NotTags),
NotTagsAny: splitTags(mapoNetwork.Filter.NotTagsAny),
Tags: splitTags(mapoNetwork.Filter.Tags),
TagsAny: splitTags(mapoNetwork.Filter.TagsAny),
},
capoNetwork.Filter = &openstackv1.NetworkFilter{
Name: mapoNetwork.Filter.Name,
Description: mapoNetwork.Filter.Description,
ProjectID: projectID,
FilterByNeutronTags: openstackv1.FilterByNeutronTags{
NotTags: splitTags(mapoNetwork.Filter.NotTags),
NotTagsAny: splitTags(mapoNetwork.Filter.NotTagsAny),
Tags: splitTags(mapoNetwork.Filter.Tags),
TagsAny: splitTags(mapoNetwork.Filter.TagsAny),
},
}
}

tags := mapoNetwork.PortTags

// convert .Subnets
if networkID == "" && (mapoNetwork.Filter == mapiv1alpha1.Filter{}) { //nolint:nestif
if (capoNetwork == openstackv1.NetworkParam{}) { //nolint:nestif
Copy link
Contributor

Choose a reason for hiding this comment

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

Code structure observation. We have:

capoNetwork := switch {
  case mapoNetwork.UUID != "":
    addCAPOSubnets(capoNetwork{ID = &mapoNetwork.UUID}, ...)

  case mapoNetwork.Filter != nil:
    addCAPOSubnets(capoNetworkFromFilter(), ...)

  default:
    capoNetworkFromSubnets() (this if statement, which is the default/else of the above)
}

addCAPOSubnets() is the else branch starting line 483 below.

// Case: network is undefined and only has subnets
// Create a port for each subnet
for j, mapoSubnet := range mapoNetwork.Subnets {
portTags := append(tags, mapoSubnet.PortTags...) //nolint:gocritic
capoPort := openstackv1.PortOpts{Tags: portTags}

subnetID := mapoSubnet.UUID
if subnetID == "" {
Expand All @@ -438,33 +441,35 @@ func convertMAPONetworksToCAPO(fldPath *field.Path, mapoNetworks []mapiv1alpha1.
warnings = append(warnings, field.Invalid(fldPath.Index(i).Child("subnets").Index(j).Child("filter", "networkId"), mapoSubnet.Filter.NetworkID, "networkId is ignored by MAPO, ignoring").Error())
}

capoPort := openstackv1.PortOpts{
Network: &network,
FixedIPs: []openstackv1.FixedIP{
{
Subnet: &openstackv1.SubnetParam{
ID: &subnetID,
Filter: &openstackv1.SubnetFilter{
CIDR: mapoSubnet.Filter.CIDR,
Description: mapoSubnet.Filter.Description,
GatewayIP: mapoSubnet.Filter.GatewayIP,
IPVersion: mapoSubnet.Filter.IPVersion,
IPv6AddressMode: mapoSubnet.Filter.IPv6AddressMode,
IPv6RAMode: mapoSubnet.Filter.IPv6RAMode,
Name: mapoSubnet.Filter.Name,
// We ignore NetworkID since it's silently ignored by MAPO itself
ProjectID: projectID,
FilterByNeutronTags: openstackv1.FilterByNeutronTags{
NotTags: splitTags(mapoSubnet.Filter.NotTags),
NotTagsAny: splitTags(mapoSubnet.Filter.NotTagsAny),
Tags: splitTags(mapoSubnet.Filter.Tags),
TagsAny: splitTags(mapoSubnet.Filter.TagsAny),
},
},
},
capoSubnet := openstackv1.SubnetParam{}
if subnetID != "" {
capoSubnet.ID = &subnetID
}

if (mapoSubnet.Filter != mapiv1alpha1.SubnetFilter{}) {
capoSubnet.Filter = &openstackv1.SubnetFilter{
CIDR: mapoSubnet.Filter.CIDR,
Description: mapoSubnet.Filter.Description,
GatewayIP: mapoSubnet.Filter.GatewayIP,
IPVersion: mapoSubnet.Filter.IPVersion,
IPv6AddressMode: mapoSubnet.Filter.IPv6AddressMode,
IPv6RAMode: mapoSubnet.Filter.IPv6RAMode,
Name: mapoSubnet.Filter.Name,
// We ignore NetworkID since it's silently ignored by MAPO itself
ProjectID: projectID,
FilterByNeutronTags: openstackv1.FilterByNeutronTags{
NotTags: splitTags(mapoSubnet.Filter.NotTags),
NotTagsAny: splitTags(mapoSubnet.Filter.NotTagsAny),
Tags: splitTags(mapoSubnet.Filter.Tags),
TagsAny: splitTags(mapoSubnet.Filter.TagsAny),
},
},
Tags: portTags,
}
}

capoFixedIP := openstackv1.FixedIP{}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: This doesn't seem to be doing anything outside the if block below. You could move it in there.

if (capoSubnet != openstackv1.SubnetParam{}) {
capoFixedIP.Subnet = &capoSubnet
capoPort.FixedIPs = []openstackv1.FixedIP{capoFixedIP}
}

if mapoSubnet.PortSecurity != nil && !*mapoSubnet.PortSecurity {
Expand All @@ -478,7 +483,7 @@ func convertMAPONetworksToCAPO(fldPath *field.Path, mapoNetworks []mapiv1alpha1.
} else {
// Case: network and subnet are defined
// Create a single port with an interface for each subnet
fixedIPs := make([]openstackv1.FixedIP, len(mapoNetwork.Subnets))
capoFixedIPs := make([]openstackv1.FixedIP, len(mapoNetwork.Subnets))

for j, mapoSubnet := range mapoNetwork.Subnets {
subnetID := mapoSubnet.UUID
Expand All @@ -496,7 +501,7 @@ func convertMAPONetworksToCAPO(fldPath *field.Path, mapoNetworks []mapiv1alpha1.
warnings = append(warnings, field.Invalid(fldPath.Index(j).Child("subnets").Index(j).Child("filter", "networkId"), mapoSubnet.Filter.NetworkID, "networkId is ignored by MAPO, ignoring").Error())
}

fixedIPs[j] = openstackv1.FixedIP{
capoFixedIPs[j] = openstackv1.FixedIP{
Subnet: &openstackv1.SubnetParam{
ID: &subnetID,
Filter: &openstackv1.SubnetFilter{
Expand All @@ -523,8 +528,8 @@ func convertMAPONetworksToCAPO(fldPath *field.Path, mapoNetworks []mapiv1alpha1.
}

capoPort := openstackv1.PortOpts{
FixedIPs: fixedIPs,
Network: &network,
FixedIPs: capoFixedIPs,
Network: &capoNetwork,
Tags: tags,
}

Expand Down
7 changes: 6 additions & 1 deletion pkg/conversion/mapi2capi/openstack_fuzz_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,8 +126,13 @@ func openstackProviderSpecFuzzerFuncs(codecs runtimeserializer.CodecFactory) []a
func(network *mapiv1alpha1.NetworkParam, c randfill.Continue) {
switch c.Int31n(2) {
case 0:
network.UUID = uuid.NewString()
network.UUID = ""
network.Filter = mapiv1alpha1.Filter{}
network.Subnets = []mapiv1alpha1.SubnetParam{
{
UUID: uuid.NewString(),
},
}
case 1:
network.UUID = ""
c.Fill(&network.Filter)
Expand Down