From 51b34bd93281dfb0a6ebfc7fc149bdad547c9d4e Mon Sep 17 00:00:00 2001 From: Matthew Booth Date: Fri, 15 Mar 2024 13:15:50 +0000 Subject: [PATCH] Store []ResolvedPortSpec in ReferencedMachineResources The purpose of this change is fix an issue where we are storing unresolved references in ReferencedMachineResources. Specifically we are storing a PortOpts, which is a user-intent struct. PortOpts can contain unresolved references to both subnets and security groups, as well fields requiring additional processing which reference external objects: the port name, description, and tags. We create a new type, ResolvedPortSpec, which can contain only fully resolved data. This can be seen in the new signature of CreatePorts(), which no longer requires any source of data other than the []ResolvedPortSpec from ReferencedMachineResources, and is now greatly simplified. Fully resolving the port name also allows a simplification in port adoption. All of the complexity now moves to ConstructPorts(), which is updated to return []ResolvedPortSpec instead of []PortOpts. ConstructPorts() is updated to resolve security groups, port name, description, and all subnets referenced in FixedIPs. --- api/v1alpha5/zz_generated.conversion.go | 60 +- api/v1alpha6/conversion_test.go | 8 +- api/v1alpha6/zz_generated.conversion.go | 62 +- api/v1alpha7/types_conversion.go | 67 + api/v1alpha7/zz_generated.conversion.go | 64 +- api/v1beta1/openstackmachine_types.go | 4 +- api/v1beta1/types.go | 100 +- api/v1beta1/zz_generated.deepcopy.go | 187 ++- ...re.cluster.x-k8s.io_openstackclusters.yaml | 250 +--- ...er.x-k8s.io_openstackclustertemplates.yaml | 4 +- ...re.cluster.x-k8s.io_openstackmachines.yaml | 250 +--- ...er.x-k8s.io_openstackmachinetemplates.yaml | 4 +- controllers/openstackcluster_controller.go | 87 +- .../openstackcluster_controller_test.go | 79 +- controllers/openstackmachine_controller.go | 77 +- .../openstackmachine_controller_test.go | 321 +---- docs/book/src/api/v1beta1/api.md | 409 ++++-- .../services/compute/dependent_resources.go | 4 +- pkg/cloud/services/compute/instance_test.go | 9 +- pkg/cloud/services/compute/instance_types.go | 2 - .../services/compute/referenced_resources.go | 29 +- .../compute/referenced_resources_test.go | 297 +++-- pkg/cloud/services/networking/network_test.go | 6 +- pkg/cloud/services/networking/port.go | 392 +++--- pkg/cloud/services/networking/port_test.go | 1154 +++++++++-------- pkg/cloud/services/networking/trunk.go | 14 +- pkg/cloud/services/networking/trunk_test.go | 57 +- pkg/utils/names/names.go | 6 + test/e2e/suites/e2e/e2e_test.go | 5 +- 29 files changed, 1838 insertions(+), 2170 deletions(-) diff --git a/api/v1alpha5/zz_generated.conversion.go b/api/v1alpha5/zz_generated.conversion.go index 4d472de0be..3d85f20fc5 100644 --- a/api/v1alpha5/zz_generated.conversion.go +++ b/api/v1alpha5/zz_generated.conversion.go @@ -1352,10 +1352,8 @@ func autoConvert_v1alpha5_PortOpts_To_v1beta1_PortOpts(in *PortOpts, out *v1beta if err := optional.Convert_string_To_optional_String(&in.Description, &out.Description, s); err != nil { return err } - out.AdminStateUp = (*bool)(unsafe.Pointer(in.AdminStateUp)) - if err := optional.Convert_string_To_optional_String(&in.MACAddress, &out.MACAddress, s); err != nil { - return err - } + // WARNING: in.AdminStateUp requires manual conversion: does not exist in peer-type + // WARNING: in.MACAddress requires manual conversion: does not exist in peer-type if in.FixedIPs != nil { in, out := &in.FixedIPs, &out.FixedIPs *out = make([]v1beta1.FixedIP, len(*in)) @@ -1371,26 +1369,12 @@ func autoConvert_v1alpha5_PortOpts_To_v1beta1_PortOpts(in *PortOpts, out *v1beta // WARNING: in.ProjectID requires manual conversion: does not exist in peer-type // INFO: in.SecurityGroups opted out of conversion generation // INFO: in.SecurityGroupFilters opted out of conversion generation - if in.AllowedAddressPairs != nil { - in, out := &in.AllowedAddressPairs, &out.AllowedAddressPairs - *out = make([]v1beta1.AddressPair, len(*in)) - for i := range *in { - if err := Convert_v1alpha5_AddressPair_To_v1beta1_AddressPair(&(*in)[i], &(*out)[i], s); err != nil { - return err - } - } - } else { - out.AllowedAddressPairs = nil - } + // WARNING: in.AllowedAddressPairs requires manual conversion: does not exist in peer-type out.Trunk = (*bool)(unsafe.Pointer(in.Trunk)) - if err := optional.Convert_string_To_optional_String(&in.HostID, &out.HostID, s); err != nil { - return err - } - if err := optional.Convert_string_To_optional_String(&in.VNICType, &out.VNICType, s); err != nil { - return err - } - // WARNING: in.Profile requires manual conversion: inconvertible types (map[string]string vs *sigs.k8s.io/cluster-api-provider-openstack/api/v1beta1.BindingProfile) - out.DisablePortSecurity = (*bool)(unsafe.Pointer(in.DisablePortSecurity)) + // WARNING: in.HostID requires manual conversion: does not exist in peer-type + // WARNING: in.VNICType requires manual conversion: does not exist in peer-type + // WARNING: in.Profile requires manual conversion: does not exist in peer-type + // WARNING: in.DisablePortSecurity requires manual conversion: does not exist in peer-type out.Tags = *(*[]string)(unsafe.Pointer(&in.Tags)) return nil } @@ -1405,14 +1389,10 @@ func autoConvert_v1beta1_PortOpts_To_v1alpha5_PortOpts(in *v1beta1.PortOpts, out } else { out.Network = nil } - if err := optional.Convert_optional_String_To_string(&in.NameSuffix, &out.NameSuffix, s); err != nil { - return err - } if err := optional.Convert_optional_String_To_string(&in.Description, &out.Description, s); err != nil { return err } - out.AdminStateUp = (*bool)(unsafe.Pointer(in.AdminStateUp)) - if err := optional.Convert_optional_String_To_string(&in.MACAddress, &out.MACAddress, s); err != nil { + if err := optional.Convert_optional_String_To_string(&in.NameSuffix, &out.NameSuffix, s); err != nil { return err } if in.FixedIPs != nil { @@ -1437,29 +1417,9 @@ func autoConvert_v1beta1_PortOpts_To_v1alpha5_PortOpts(in *v1beta1.PortOpts, out } else { out.SecurityGroups = nil } - if in.AllowedAddressPairs != nil { - in, out := &in.AllowedAddressPairs, &out.AllowedAddressPairs - *out = make([]AddressPair, len(*in)) - for i := range *in { - if err := Convert_v1beta1_AddressPair_To_v1alpha5_AddressPair(&(*in)[i], &(*out)[i], s); err != nil { - return err - } - } - } else { - out.AllowedAddressPairs = nil - } - out.Trunk = (*bool)(unsafe.Pointer(in.Trunk)) - if err := optional.Convert_optional_String_To_string(&in.HostID, &out.HostID, s); err != nil { - return err - } - if err := optional.Convert_optional_String_To_string(&in.VNICType, &out.VNICType, s); err != nil { - return err - } - // WARNING: in.Profile requires manual conversion: inconvertible types (*sigs.k8s.io/cluster-api-provider-openstack/api/v1beta1.BindingProfile vs map[string]string) - out.DisablePortSecurity = (*bool)(unsafe.Pointer(in.DisablePortSecurity)) - // WARNING: in.PropagateUplinkStatus requires manual conversion: does not exist in peer-type out.Tags = *(*[]string)(unsafe.Pointer(&in.Tags)) - // WARNING: in.ValueSpecs requires manual conversion: does not exist in peer-type + out.Trunk = (*bool)(unsafe.Pointer(in.Trunk)) + // WARNING: in.ResolvedPortSpecFields requires manual conversion: does not exist in peer-type return nil } diff --git a/api/v1alpha6/conversion_test.go b/api/v1alpha6/conversion_test.go index 984d7ffb46..3e5484cafd 100644 --- a/api/v1alpha6/conversion_test.go +++ b/api/v1alpha6/conversion_test.go @@ -570,7 +570,9 @@ func TestPortOptsConvertTo(t *testing.T) { SecurityGroups: uuids, }}, hubPortOpts: []infrav1.PortOpts{{ - Profile: &convertedPortProfile, + ResolvedPortSpecFields: infrav1.ResolvedPortSpecFields{ + Profile: &convertedPortProfile, + }, SecurityGroups: securityGroupsUuids, }}, }, @@ -582,7 +584,9 @@ func TestPortOptsConvertTo(t *testing.T) { SecurityGroupFilters: securityGroupFilter, }}, hubPortOpts: []infrav1.PortOpts{{ - Profile: &convertedPortProfile, + ResolvedPortSpecFields: infrav1.ResolvedPortSpecFields{ + Profile: &convertedPortProfile, + }, SecurityGroups: securityGroupFilterMerged, }}, }, diff --git a/api/v1alpha6/zz_generated.conversion.go b/api/v1alpha6/zz_generated.conversion.go index 7530cc7dea..050c93317b 100644 --- a/api/v1alpha6/zz_generated.conversion.go +++ b/api/v1alpha6/zz_generated.conversion.go @@ -1387,10 +1387,8 @@ func autoConvert_v1alpha6_PortOpts_To_v1beta1_PortOpts(in *PortOpts, out *v1beta if err := optional.Convert_string_To_optional_String(&in.Description, &out.Description, s); err != nil { return err } - out.AdminStateUp = (*bool)(unsafe.Pointer(in.AdminStateUp)) - if err := optional.Convert_string_To_optional_String(&in.MACAddress, &out.MACAddress, s); err != nil { - return err - } + // WARNING: in.AdminStateUp requires manual conversion: does not exist in peer-type + // WARNING: in.MACAddress requires manual conversion: does not exist in peer-type if in.FixedIPs != nil { in, out := &in.FixedIPs, &out.FixedIPs *out = make([]v1beta1.FixedIP, len(*in)) @@ -1406,28 +1404,14 @@ func autoConvert_v1alpha6_PortOpts_To_v1beta1_PortOpts(in *PortOpts, out *v1beta // WARNING: in.ProjectID requires manual conversion: does not exist in peer-type // INFO: in.SecurityGroups opted out of conversion generation // INFO: in.SecurityGroupFilters opted out of conversion generation - if in.AllowedAddressPairs != nil { - in, out := &in.AllowedAddressPairs, &out.AllowedAddressPairs - *out = make([]v1beta1.AddressPair, len(*in)) - for i := range *in { - if err := Convert_v1alpha6_AddressPair_To_v1beta1_AddressPair(&(*in)[i], &(*out)[i], s); err != nil { - return err - } - } - } else { - out.AllowedAddressPairs = nil - } + // WARNING: in.AllowedAddressPairs requires manual conversion: does not exist in peer-type out.Trunk = (*bool)(unsafe.Pointer(in.Trunk)) - if err := optional.Convert_string_To_optional_String(&in.HostID, &out.HostID, s); err != nil { - return err - } - if err := optional.Convert_string_To_optional_String(&in.VNICType, &out.VNICType, s); err != nil { - return err - } - // WARNING: in.Profile requires manual conversion: inconvertible types (map[string]string vs *sigs.k8s.io/cluster-api-provider-openstack/api/v1beta1.BindingProfile) - out.DisablePortSecurity = (*bool)(unsafe.Pointer(in.DisablePortSecurity)) + // WARNING: in.HostID requires manual conversion: does not exist in peer-type + // WARNING: in.VNICType requires manual conversion: does not exist in peer-type + // WARNING: in.Profile requires manual conversion: does not exist in peer-type + // WARNING: in.DisablePortSecurity requires manual conversion: does not exist in peer-type out.Tags = *(*[]string)(unsafe.Pointer(&in.Tags)) - out.ValueSpecs = *(*[]v1beta1.ValueSpec)(unsafe.Pointer(&in.ValueSpecs)) + // WARNING: in.ValueSpecs requires manual conversion: does not exist in peer-type return nil } @@ -1441,14 +1425,10 @@ func autoConvert_v1beta1_PortOpts_To_v1alpha6_PortOpts(in *v1beta1.PortOpts, out } else { out.Network = nil } - if err := optional.Convert_optional_String_To_string(&in.NameSuffix, &out.NameSuffix, s); err != nil { - return err - } if err := optional.Convert_optional_String_To_string(&in.Description, &out.Description, s); err != nil { return err } - out.AdminStateUp = (*bool)(unsafe.Pointer(in.AdminStateUp)) - if err := optional.Convert_optional_String_To_string(&in.MACAddress, &out.MACAddress, s); err != nil { + if err := optional.Convert_optional_String_To_string(&in.NameSuffix, &out.NameSuffix, s); err != nil { return err } if in.FixedIPs != nil { @@ -1473,29 +1453,9 @@ func autoConvert_v1beta1_PortOpts_To_v1alpha6_PortOpts(in *v1beta1.PortOpts, out } else { out.SecurityGroups = nil } - if in.AllowedAddressPairs != nil { - in, out := &in.AllowedAddressPairs, &out.AllowedAddressPairs - *out = make([]AddressPair, len(*in)) - for i := range *in { - if err := Convert_v1beta1_AddressPair_To_v1alpha6_AddressPair(&(*in)[i], &(*out)[i], s); err != nil { - return err - } - } - } else { - out.AllowedAddressPairs = nil - } - out.Trunk = (*bool)(unsafe.Pointer(in.Trunk)) - if err := optional.Convert_optional_String_To_string(&in.HostID, &out.HostID, s); err != nil { - return err - } - if err := optional.Convert_optional_String_To_string(&in.VNICType, &out.VNICType, s); err != nil { - return err - } - // WARNING: in.Profile requires manual conversion: inconvertible types (*sigs.k8s.io/cluster-api-provider-openstack/api/v1beta1.BindingProfile vs map[string]string) - out.DisablePortSecurity = (*bool)(unsafe.Pointer(in.DisablePortSecurity)) - // WARNING: in.PropagateUplinkStatus requires manual conversion: does not exist in peer-type out.Tags = *(*[]string)(unsafe.Pointer(&in.Tags)) - out.ValueSpecs = *(*[]ValueSpec)(unsafe.Pointer(&in.ValueSpecs)) + out.Trunk = (*bool)(unsafe.Pointer(in.Trunk)) + // WARNING: in.ResolvedPortSpecFields requires manual conversion: does not exist in peer-type return nil } diff --git a/api/v1alpha7/types_conversion.go b/api/v1alpha7/types_conversion.go index 7b966d1320..fbc6fde3e2 100644 --- a/api/v1alpha7/types_conversion.go +++ b/api/v1alpha7/types_conversion.go @@ -17,6 +17,8 @@ limitations under the License. package v1alpha7 import ( + "errors" + apiconversion "k8s.io/apimachinery/pkg/conversion" "k8s.io/utils/pointer" @@ -253,6 +255,38 @@ func Convert_v1alpha7_PortOpts_To_v1beta1_PortOpts(in *PortOpts, out *infrav1.Po return err } + // Copy members of ResolvedPortSpecFields + var allowedAddressPairs []infrav1.AddressPair + if len(in.AllowedAddressPairs) > 0 { + allowedAddressPairs = make([]infrav1.AddressPair, len(in.AllowedAddressPairs)) + for i := range in.AllowedAddressPairs { + aap := &in.AllowedAddressPairs[i] + allowedAddressPairs[i] = infrav1.AddressPair{ + MACAddress: &aap.MACAddress, + IPAddress: aap.IPAddress, + } + } + } + var valueSpecs []infrav1.ValueSpec + if len(in.ValueSpecs) > 0 { + valueSpecs = make([]infrav1.ValueSpec, len(in.ValueSpecs)) + for i, vs := range in.ValueSpecs { + valueSpecs[i] = infrav1.ValueSpec(vs) + } + } + out.AdminStateUp = in.AdminStateUp + out.AllowedAddressPairs = allowedAddressPairs + out.DisablePortSecurity = in.DisablePortSecurity + out.PropagateUplinkStatus = in.PropagateUplinkStatus + out.ValueSpecs = valueSpecs + if err := errors.Join( + optional.Convert_string_To_optional_String(&in.MACAddress, &out.MACAddress, s), + optional.Convert_string_To_optional_String(&in.HostID, &out.HostID, s), + optional.Convert_string_To_optional_String(&in.VNICType, &out.VNICType, s), + ); err != nil { + return err + } + if len(in.SecurityGroupFilters) > 0 { out.SecurityGroups = make([]infrav1.SecurityGroupFilter, len(in.SecurityGroupFilters)) for i := range in.SecurityGroupFilters { @@ -277,6 +311,39 @@ func Convert_v1beta1_PortOpts_To_v1alpha7_PortOpts(in *infrav1.PortOpts, out *Po return err } + // Copy members of ResolvedPortSpecFields + var allowedAddressPairs []AddressPair + if len(in.AllowedAddressPairs) > 0 { + allowedAddressPairs = make([]AddressPair, len(in.AllowedAddressPairs)) + for i := range in.AllowedAddressPairs { + inAAP := &in.AllowedAddressPairs[i] + outAAP := &allowedAddressPairs[i] + if err := optional.Convert_optional_String_To_string(&inAAP.MACAddress, &outAAP.MACAddress, s); err != nil { + return err + } + outAAP.IPAddress = inAAP.IPAddress + } + } + var valueSpecs []ValueSpec + if len(in.ValueSpecs) > 0 { + valueSpecs = make([]ValueSpec, len(in.ValueSpecs)) + for i, vs := range in.ValueSpecs { + valueSpecs[i] = ValueSpec(vs) + } + } + out.AdminStateUp = in.AdminStateUp + out.AllowedAddressPairs = allowedAddressPairs + out.DisablePortSecurity = in.DisablePortSecurity + out.PropagateUplinkStatus = in.PropagateUplinkStatus + out.ValueSpecs = valueSpecs + if err := errors.Join( + optional.Convert_optional_String_To_string(&in.MACAddress, &out.MACAddress, s), + optional.Convert_optional_String_To_string(&in.HostID, &out.HostID, s), + optional.Convert_optional_String_To_string(&in.VNICType, &out.VNICType, s), + ); err != nil { + return err + } + if len(in.SecurityGroups) > 0 { out.SecurityGroupFilters = make([]SecurityGroupFilter, len(in.SecurityGroups)) for i := range in.SecurityGroups { diff --git a/api/v1alpha7/zz_generated.conversion.go b/api/v1alpha7/zz_generated.conversion.go index d22715b316..b09e82c922 100644 --- a/api/v1alpha7/zz_generated.conversion.go +++ b/api/v1alpha7/zz_generated.conversion.go @@ -1587,10 +1587,8 @@ func autoConvert_v1alpha7_PortOpts_To_v1beta1_PortOpts(in *PortOpts, out *v1beta if err := optional.Convert_string_To_optional_String(&in.Description, &out.Description, s); err != nil { return err } - out.AdminStateUp = (*bool)(unsafe.Pointer(in.AdminStateUp)) - if err := optional.Convert_string_To_optional_String(&in.MACAddress, &out.MACAddress, s); err != nil { - return err - } + // WARNING: in.AdminStateUp requires manual conversion: does not exist in peer-type + // WARNING: in.MACAddress requires manual conversion: does not exist in peer-type if in.FixedIPs != nil { in, out := &in.FixedIPs, &out.FixedIPs *out = make([]v1beta1.FixedIP, len(*in)) @@ -1603,29 +1601,15 @@ func autoConvert_v1alpha7_PortOpts_To_v1beta1_PortOpts(in *PortOpts, out *v1beta out.FixedIPs = nil } // WARNING: in.SecurityGroupFilters requires manual conversion: does not exist in peer-type - if in.AllowedAddressPairs != nil { - in, out := &in.AllowedAddressPairs, &out.AllowedAddressPairs - *out = make([]v1beta1.AddressPair, len(*in)) - for i := range *in { - if err := Convert_v1alpha7_AddressPair_To_v1beta1_AddressPair(&(*in)[i], &(*out)[i], s); err != nil { - return err - } - } - } else { - out.AllowedAddressPairs = nil - } + // WARNING: in.AllowedAddressPairs requires manual conversion: does not exist in peer-type out.Trunk = (*bool)(unsafe.Pointer(in.Trunk)) - if err := optional.Convert_string_To_optional_String(&in.HostID, &out.HostID, s); err != nil { - return err - } - if err := optional.Convert_string_To_optional_String(&in.VNICType, &out.VNICType, s); err != nil { - return err - } - // WARNING: in.Profile requires manual conversion: inconvertible types (sigs.k8s.io/cluster-api-provider-openstack/api/v1alpha7.BindingProfile vs *sigs.k8s.io/cluster-api-provider-openstack/api/v1beta1.BindingProfile) - out.DisablePortSecurity = (*bool)(unsafe.Pointer(in.DisablePortSecurity)) - out.PropagateUplinkStatus = (*bool)(unsafe.Pointer(in.PropagateUplinkStatus)) + // WARNING: in.HostID requires manual conversion: does not exist in peer-type + // WARNING: in.VNICType requires manual conversion: does not exist in peer-type + // WARNING: in.Profile requires manual conversion: does not exist in peer-type + // WARNING: in.DisablePortSecurity requires manual conversion: does not exist in peer-type + // WARNING: in.PropagateUplinkStatus requires manual conversion: does not exist in peer-type out.Tags = *(*[]string)(unsafe.Pointer(&in.Tags)) - out.ValueSpecs = *(*[]v1beta1.ValueSpec)(unsafe.Pointer(&in.ValueSpecs)) + // WARNING: in.ValueSpecs requires manual conversion: does not exist in peer-type return nil } @@ -1639,14 +1623,10 @@ func autoConvert_v1beta1_PortOpts_To_v1alpha7_PortOpts(in *v1beta1.PortOpts, out } else { out.Network = nil } - if err := optional.Convert_optional_String_To_string(&in.NameSuffix, &out.NameSuffix, s); err != nil { - return err - } if err := optional.Convert_optional_String_To_string(&in.Description, &out.Description, s); err != nil { return err } - out.AdminStateUp = (*bool)(unsafe.Pointer(in.AdminStateUp)) - if err := optional.Convert_optional_String_To_string(&in.MACAddress, &out.MACAddress, s); err != nil { + if err := optional.Convert_optional_String_To_string(&in.NameSuffix, &out.NameSuffix, s); err != nil { return err } if in.FixedIPs != nil { @@ -1661,29 +1641,9 @@ func autoConvert_v1beta1_PortOpts_To_v1alpha7_PortOpts(in *v1beta1.PortOpts, out out.FixedIPs = nil } // WARNING: in.SecurityGroups requires manual conversion: does not exist in peer-type - if in.AllowedAddressPairs != nil { - in, out := &in.AllowedAddressPairs, &out.AllowedAddressPairs - *out = make([]AddressPair, len(*in)) - for i := range *in { - if err := Convert_v1beta1_AddressPair_To_v1alpha7_AddressPair(&(*in)[i], &(*out)[i], s); err != nil { - return err - } - } - } else { - out.AllowedAddressPairs = nil - } - out.Trunk = (*bool)(unsafe.Pointer(in.Trunk)) - if err := optional.Convert_optional_String_To_string(&in.HostID, &out.HostID, s); err != nil { - return err - } - if err := optional.Convert_optional_String_To_string(&in.VNICType, &out.VNICType, s); err != nil { - return err - } - // WARNING: in.Profile requires manual conversion: inconvertible types (*sigs.k8s.io/cluster-api-provider-openstack/api/v1beta1.BindingProfile vs sigs.k8s.io/cluster-api-provider-openstack/api/v1alpha7.BindingProfile) - out.DisablePortSecurity = (*bool)(unsafe.Pointer(in.DisablePortSecurity)) - out.PropagateUplinkStatus = (*bool)(unsafe.Pointer(in.PropagateUplinkStatus)) out.Tags = *(*[]string)(unsafe.Pointer(&in.Tags)) - out.ValueSpecs = *(*[]ValueSpec)(unsafe.Pointer(&in.ValueSpecs)) + out.Trunk = (*bool)(unsafe.Pointer(in.Trunk)) + // WARNING: in.ResolvedPortSpecFields requires manual conversion: does not exist in peer-type return nil } diff --git a/api/v1beta1/openstackmachine_types.go b/api/v1beta1/openstackmachine_types.go index 52caccbaf1..b5a6b3bafc 100644 --- a/api/v1beta1/openstackmachine_types.go +++ b/api/v1beta1/openstackmachine_types.go @@ -60,7 +60,9 @@ type OpenStackMachineSpec struct { // Whether the server instance is created on a trunk port or not. Trunk bool `json:"trunk,omitempty"` - // Machine tags + // Tags which will be added to the machine and all dependent resources + // which support them. These are in addition to Tags defined on the + // cluster. // Requires Nova api 2.52 minimum! // +listType=set Tags []string `json:"tags,omitempty"` diff --git a/api/v1beta1/types.go b/api/v1beta1/types.go index 94625b20db..7bebfd68e9 100644 --- a/api/v1beta1/types.go +++ b/api/v1beta1/types.go @@ -171,21 +171,13 @@ type PortOpts struct { // +optional Network *NetworkFilter `json:"network,omitempty"` - // NameSuffix will be appended to the name of the port if specified. If unspecified, instead the 0-based index of the port in the list is used. - // +optional - NameSuffix optional.String `json:"nameSuffix,omitempty"` - // Description is a human-readable description for the port. // +optional Description optional.String `json:"description,omitempty"` - // AdminStateUp specifies whether the port should be created in the up (true) or down (false) state. The default is up. - // +optional - AdminStateUp *bool `json:"adminStateUp,omitempty"` - - // MACAddress specifies the MAC address of the port. If not specified, the MAC address will be generated. + // NameSuffix will be appended to the name of the port if specified. If unspecified, instead the 0-based index of the port in the list is used. // +optional - MACAddress optional.String `json:"macAddress,omitempty"` + NameSuffix optional.String `json:"nameSuffix,omitempty"` // FixedIPs is a list of pairs of subnet and/or IP address to assign to the port. If specified, these must be subnets of the port's network. // +optional @@ -197,13 +189,11 @@ type PortOpts struct { // +listType=atomic SecurityGroups []SecurityGroupFilter `json:"securityGroups,omitempty"` - // AllowedAddressPairs is a list of address pairs which Neutron will - // allow the port to send traffic from in addition to the port's - // addresses. If not specified, the MAC Address will be the MAC Address - // of the port. Depending on the configuration of Neutron, it may be - // supported to specify a CIDR instead of a specific IP address. + // Tags applied to the port (and corresponding trunk, if a trunk is configured.) + // These tags are applied in addition to the instance's tags, which will also be applied to the port. + // +listType=set // +optional - AllowedAddressPairs []AddressPair `json:"allowedAddressPairs,omitempty"` + Tags []string `json:"tags,omitempty"` // Trunk specifies whether trunking is enabled at the port level. If not // provided the value is inherited from the machine, or false for a @@ -211,6 +201,29 @@ type PortOpts struct { // +optional Trunk *bool `json:"trunk,omitempty"` + ResolvedPortSpecFields `json:",inline"` +} + +// ResolvePortSpecFields is a convenience struct containing all fields of a +// PortOpts which don't contain references which need to be resolved, and can +// therefore be shared with ResolvedPortSpec. +type ResolvedPortSpecFields struct { + // AdminStateUp specifies whether the port should be created in the up (true) or down (false) state. The default is up. + // +optional + AdminStateUp *bool `json:"adminStateUp,omitempty"` + + // MACAddress specifies the MAC address of the port. If not specified, the MAC address will be generated. + // +optional + MACAddress optional.String `json:"macAddress,omitempty"` + + // AllowedAddressPairs is a list of address pairs which Neutron will + // allow the port to send traffic from in addition to the port's + // addresses. If not specified, the MAC Address will be the MAC Address + // of the port. Depending on the configuration of Neutron, it may be + // supported to specify a CIDR instead of a specific IP address. + // +optional + AllowedAddressPairs []AddressPair `json:"allowedAddressPairs,omitempty"` + // HostID specifies the ID of the host where the port resides. // +optional HostID optional.String `json:"hostID,omitempty"` @@ -245,12 +258,6 @@ type PortOpts struct { // +optional PropagateUplinkStatus *bool `json:"propagateUplinkStatus,omitempty"` - // Tags applied to the port (and corresponding trunk, if a trunk is configured.) - // These tags are applied in addition to the instance's tags, which will also be applied to the port. - // +listType=set - // +optional - Tags []string `json:"tags,omitempty"` - // Value specs are extra parameters to include in the API request with OpenStack. // This is an extension point for the API, so what they do and if they are supported, // depends on the specific OpenStack implementation. @@ -260,6 +267,39 @@ type PortOpts struct { ValueSpecs []ValueSpec `json:"valueSpecs,omitempty"` } +// ResolvedPortSpec is a PortOpts with all contained references fully resolved. +type ResolvedPortSpec struct { + // Name is the name of the port. + Name string `json:"name"` + + // Description is a human-readable description for the port. + Description string `json:"description"` + + // NetworkID is the ID of the network the port will be created in. + NetworkID string `json:"networkID"` + + // Tags applied to the port (and corresponding trunk, if a trunk is configured.) + // +listType=set + // +optional + Tags []string `json:"tags,omitempty"` + + // Trunk specifies whether trunking is enabled at the port level. + // +optional + Trunk optional.Bool `json:"trunk,omitempty"` + + // FixedIPs is a list of pairs of subnet and/or IP address to assign to the port. If specified, these must be subnets of the port's network. + // +optional + // +listType=atomic + FixedIPs []ResolvedFixedIP `json:"fixedIPs,omitempty"` + + // SecurityGroups is a list of security group IDs to assign to the port. + // +optional + // +listType=atomic + SecurityGroups []string `json:"securityGroups,omitempty"` + + ResolvedPortSpecFields `json:",inline"` +} + type PortStatus struct { // ID is the unique identifier of the port. // +required @@ -290,6 +330,20 @@ type FixedIP struct { IPAddress optional.String `json:"ipAddress,omitempty"` } +// ResolvedFixedIP is a FixedIP with the Subnet resolved to an ID. +type ResolvedFixedIP struct { + // SubnetID is the id of a subnet to create the fixed IP of a port in. + // +optional + SubnetID optional.String `json:"subnet,omitempty"` + + // IPAddress is a specific IP address to assign to the port. If SubnetID + // is also specified, IPAddress must be a valid IP address in the + // subnet. If Subnet is not specified, IPAddress must be a valid IP + // address in any subnet of the port's network. + // +optional + IPAddress optional.String `json:"ipAddress,omitempty"` +} + type AddressPair struct { // IPAddress is the IP address of the allowed address pair. Depending on // the configuration of Neutron, it may be supported to specify a CIDR @@ -663,7 +717,7 @@ type ReferencedMachineResources struct { // Ports is the fully resolved list of ports to create for the machine. // +optional - Ports []PortOpts `json:"ports,omitempty"` + Ports []ResolvedPortSpec `json:"ports,omitempty"` } type DependentMachineResources struct { diff --git a/api/v1beta1/zz_generated.deepcopy.go b/api/v1beta1/zz_generated.deepcopy.go index 73e1fb8368..2e5e335dfb 100644 --- a/api/v1beta1/zz_generated.deepcopy.go +++ b/api/v1beta1/zz_generated.deepcopy.go @@ -1079,23 +1079,13 @@ func (in *PortOpts) DeepCopyInto(out *PortOpts) { *out = new(NetworkFilter) (*in).DeepCopyInto(*out) } - if in.NameSuffix != nil { - in, out := &in.NameSuffix, &out.NameSuffix - *out = new(string) - **out = **in - } if in.Description != nil { in, out := &in.Description, &out.Description *out = new(string) **out = **in } - if in.AdminStateUp != nil { - in, out := &in.AdminStateUp, &out.AdminStateUp - *out = new(bool) - **out = **in - } - if in.MACAddress != nil { - in, out := &in.MACAddress, &out.MACAddress + if in.NameSuffix != nil { + in, out := &in.NameSuffix, &out.NameSuffix *out = new(string) **out = **in } @@ -1113,53 +1103,17 @@ func (in *PortOpts) DeepCopyInto(out *PortOpts) { (*in)[i].DeepCopyInto(&(*out)[i]) } } - if in.AllowedAddressPairs != nil { - in, out := &in.AllowedAddressPairs, &out.AllowedAddressPairs - *out = make([]AddressPair, len(*in)) - for i := range *in { - (*in)[i].DeepCopyInto(&(*out)[i]) - } - } - if in.Trunk != nil { - in, out := &in.Trunk, &out.Trunk - *out = new(bool) - **out = **in - } - if in.HostID != nil { - in, out := &in.HostID, &out.HostID - *out = new(string) - **out = **in - } - if in.VNICType != nil { - in, out := &in.VNICType, &out.VNICType - *out = new(string) - **out = **in - } - if in.Profile != nil { - in, out := &in.Profile, &out.Profile - *out = new(BindingProfile) - (*in).DeepCopyInto(*out) - } - if in.DisablePortSecurity != nil { - in, out := &in.DisablePortSecurity, &out.DisablePortSecurity - *out = new(bool) - **out = **in - } - if in.PropagateUplinkStatus != nil { - in, out := &in.PropagateUplinkStatus, &out.PropagateUplinkStatus - *out = new(bool) - **out = **in - } if in.Tags != nil { in, out := &in.Tags, &out.Tags *out = make([]string, len(*in)) copy(*out, *in) } - if in.ValueSpecs != nil { - in, out := &in.ValueSpecs, &out.ValueSpecs - *out = make([]ValueSpec, len(*in)) - copy(*out, *in) + if in.Trunk != nil { + in, out := &in.Trunk, &out.Trunk + *out = new(bool) + **out = **in } + in.ResolvedPortSpecFields.DeepCopyInto(&out.ResolvedPortSpecFields) } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new PortOpts. @@ -1192,7 +1146,7 @@ func (in *ReferencedMachineResources) DeepCopyInto(out *ReferencedMachineResourc *out = *in if in.Ports != nil { in, out := &in.Ports, &out.Ports - *out = make([]PortOpts, len(*in)) + *out = make([]ResolvedPortSpec, len(*in)) for i := range *in { (*in)[i].DeepCopyInto(&(*out)[i]) } @@ -1209,6 +1163,131 @@ func (in *ReferencedMachineResources) DeepCopy() *ReferencedMachineResources { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *ResolvedFixedIP) DeepCopyInto(out *ResolvedFixedIP) { + *out = *in + if in.SubnetID != nil { + in, out := &in.SubnetID, &out.SubnetID + *out = new(string) + **out = **in + } + if in.IPAddress != nil { + in, out := &in.IPAddress, &out.IPAddress + *out = new(string) + **out = **in + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ResolvedFixedIP. +func (in *ResolvedFixedIP) DeepCopy() *ResolvedFixedIP { + if in == nil { + return nil + } + out := new(ResolvedFixedIP) + in.DeepCopyInto(out) + return out +} + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *ResolvedPortSpec) DeepCopyInto(out *ResolvedPortSpec) { + *out = *in + if in.Tags != nil { + in, out := &in.Tags, &out.Tags + *out = make([]string, len(*in)) + copy(*out, *in) + } + if in.Trunk != nil { + in, out := &in.Trunk, &out.Trunk + *out = new(bool) + **out = **in + } + if in.FixedIPs != nil { + in, out := &in.FixedIPs, &out.FixedIPs + *out = make([]ResolvedFixedIP, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } + if in.SecurityGroups != nil { + in, out := &in.SecurityGroups, &out.SecurityGroups + *out = make([]string, len(*in)) + copy(*out, *in) + } + in.ResolvedPortSpecFields.DeepCopyInto(&out.ResolvedPortSpecFields) +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ResolvedPortSpec. +func (in *ResolvedPortSpec) DeepCopy() *ResolvedPortSpec { + if in == nil { + return nil + } + out := new(ResolvedPortSpec) + in.DeepCopyInto(out) + return out +} + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *ResolvedPortSpecFields) DeepCopyInto(out *ResolvedPortSpecFields) { + *out = *in + if in.AdminStateUp != nil { + in, out := &in.AdminStateUp, &out.AdminStateUp + *out = new(bool) + **out = **in + } + if in.MACAddress != nil { + in, out := &in.MACAddress, &out.MACAddress + *out = new(string) + **out = **in + } + if in.AllowedAddressPairs != nil { + in, out := &in.AllowedAddressPairs, &out.AllowedAddressPairs + *out = make([]AddressPair, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } + if in.HostID != nil { + in, out := &in.HostID, &out.HostID + *out = new(string) + **out = **in + } + if in.VNICType != nil { + in, out := &in.VNICType, &out.VNICType + *out = new(string) + **out = **in + } + if in.Profile != nil { + in, out := &in.Profile, &out.Profile + *out = new(BindingProfile) + (*in).DeepCopyInto(*out) + } + if in.DisablePortSecurity != nil { + in, out := &in.DisablePortSecurity, &out.DisablePortSecurity + *out = new(bool) + **out = **in + } + if in.PropagateUplinkStatus != nil { + in, out := &in.PropagateUplinkStatus, &out.PropagateUplinkStatus + *out = new(bool) + **out = **in + } + if in.ValueSpecs != nil { + in, out := &in.ValueSpecs, &out.ValueSpecs + *out = make([]ValueSpec, len(*in)) + copy(*out, *in) + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ResolvedPortSpecFields. +func (in *ResolvedPortSpecFields) DeepCopy() *ResolvedPortSpecFields { + if in == nil { + return nil + } + out := new(ResolvedPortSpecFields) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *RootVolume) DeepCopyInto(out *RootVolume) { *out = *in diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclusters.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclusters.yaml index f2e491f4b3..ad4a0764f6 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclusters.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclusters.yaml @@ -5558,7 +5558,9 @@ spec: type: string tags: description: |- - Machine tags + Tags which will be added to the machine and all dependent resources + which support them. These are in addition to Tags defined on the + cluster. Requires Nova api 2.52 minimum! items: type: string @@ -6265,6 +6267,8 @@ spec: description: Ports is the fully resolved list of ports to create for the machine. items: + description: ResolvedPortSpec is a PortOpts with all contained + references fully resolved. properties: adminStateUp: description: AdminStateUp specifies whether the port @@ -6309,91 +6313,20 @@ spec: IP address to assign to the port. If specified, these must be subnets of the port's network. items: + description: ResolvedFixedIP is a FixedIP with the + Subnet resolved to an ID. properties: ipAddress: description: |- - IPAddress is a specific IP address to assign to the port. If Subnet + IPAddress is a specific IP address to assign to the port. If SubnetID is also specified, IPAddress must be a valid IP address in the subnet. If Subnet is not specified, IPAddress must be a valid IP address in any subnet of the port's network. type: string subnet: - description: |- - Subnet is an openstack subnet query that will return the id of a subnet to create - the fixed IP of a port in. This query must not return more than one subnet. - properties: - cidr: - type: string - description: - type: string - gatewayIP: - type: string - id: - type: string - ipVersion: - type: integer - ipv6AddressMode: - type: string - ipv6RAMode: - type: string - name: - type: string - notTags: - description: |- - NotTags is a list of tags to filter by. If specified, resources which - contain all of the given tags will be excluded from the result. - items: - description: |- - NeutronTag represents a tag on a Neutron resource. - It may not be empty and may not contain commas. - minLength: 1 - pattern: ^[^,]+$ - type: string - type: array - x-kubernetes-list-type: set - notTagsAny: - description: |- - NotTagsAny is a list of tags to filter by. If specified, resources - which contain any of the given tags will be excluded from the result. - items: - description: |- - NeutronTag represents a tag on a Neutron resource. - It may not be empty and may not contain commas. - minLength: 1 - pattern: ^[^,]+$ - type: string - type: array - x-kubernetes-list-type: set - projectID: - type: string - tags: - description: |- - Tags is a list of tags to filter by. If specified, the resource must - have all of the tags specified to be included in the result. - items: - description: |- - NeutronTag represents a tag on a Neutron resource. - It may not be empty and may not contain commas. - minLength: 1 - pattern: ^[^,]+$ - type: string - type: array - x-kubernetes-list-type: set - tagsAny: - description: |- - TagsAny is a list of tags to filter by. If specified, the resource - must have at least one of the tags specified to be included in the - result. - items: - description: |- - NeutronTag represents a tag on a Neutron resource. - It may not be empty and may not contain commas. - minLength: 1 - pattern: ^[^,]+$ - type: string - type: array - x-kubernetes-list-type: set - type: object + description: SubnetID is the id of a subnet to + create the fixed IP of a port in. + type: string type: object type: array x-kubernetes-list-type: atomic @@ -6406,78 +6339,13 @@ spec: the port. If not specified, the MAC address will be generated. type: string - nameSuffix: - description: NameSuffix will be appended to the name - of the port if specified. If unspecified, instead - the 0-based index of the port in the list is used. + name: + description: Name is the name of the port. + type: string + networkID: + description: NetworkID is the ID of the network the + port will be created in. type: string - network: - description: |- - Network is a query for an openstack network that the port will be created or discovered on. - This will fail if the query returns more than one network. - properties: - description: - type: string - id: - type: string - name: - type: string - notTags: - description: |- - NotTags is a list of tags to filter by. If specified, resources which - contain all of the given tags will be excluded from the result. - items: - description: |- - NeutronTag represents a tag on a Neutron resource. - It may not be empty and may not contain commas. - minLength: 1 - pattern: ^[^,]+$ - type: string - type: array - x-kubernetes-list-type: set - notTagsAny: - description: |- - NotTagsAny is a list of tags to filter by. If specified, resources - which contain any of the given tags will be excluded from the result. - items: - description: |- - NeutronTag represents a tag on a Neutron resource. - It may not be empty and may not contain commas. - minLength: 1 - pattern: ^[^,]+$ - type: string - type: array - x-kubernetes-list-type: set - projectID: - type: string - tags: - description: |- - Tags is a list of tags to filter by. If specified, the resource must - have all of the tags specified to be included in the result. - items: - description: |- - NeutronTag represents a tag on a Neutron resource. - It may not be empty and may not contain commas. - minLength: 1 - pattern: ^[^,]+$ - type: string - type: array - x-kubernetes-list-type: set - tagsAny: - description: |- - TagsAny is a list of tags to filter by. If specified, the resource - must have at least one of the tags specified to be included in the - result. - items: - description: |- - NeutronTag represents a tag on a Neutron resource. - It may not be empty and may not contain commas. - minLength: 1 - pattern: ^[^,]+$ - type: string - type: array - x-kubernetes-list-type: set - type: object profile: description: |- Profile is a set of key-value pairs that are used for binding @@ -6502,88 +6370,22 @@ spec: the propagate uplink status on the port. type: boolean securityGroups: - description: SecurityGroups is a list of the names, - uuids, filters or any combination these of the security - groups to assign to the instance. + description: SecurityGroups is a list of security group + IDs to assign to the port. items: - properties: - description: - type: string - id: - type: string - name: - type: string - notTags: - description: |- - NotTags is a list of tags to filter by. If specified, resources which - contain all of the given tags will be excluded from the result. - items: - description: |- - NeutronTag represents a tag on a Neutron resource. - It may not be empty and may not contain commas. - minLength: 1 - pattern: ^[^,]+$ - type: string - type: array - x-kubernetes-list-type: set - notTagsAny: - description: |- - NotTagsAny is a list of tags to filter by. If specified, resources - which contain any of the given tags will be excluded from the result. - items: - description: |- - NeutronTag represents a tag on a Neutron resource. - It may not be empty and may not contain commas. - minLength: 1 - pattern: ^[^,]+$ - type: string - type: array - x-kubernetes-list-type: set - projectID: - type: string - tags: - description: |- - Tags is a list of tags to filter by. If specified, the resource must - have all of the tags specified to be included in the result. - items: - description: |- - NeutronTag represents a tag on a Neutron resource. - It may not be empty and may not contain commas. - minLength: 1 - pattern: ^[^,]+$ - type: string - type: array - x-kubernetes-list-type: set - tagsAny: - description: |- - TagsAny is a list of tags to filter by. If specified, the resource - must have at least one of the tags specified to be included in the - result. - items: - description: |- - NeutronTag represents a tag on a Neutron resource. - It may not be empty and may not contain commas. - minLength: 1 - pattern: ^[^,]+$ - type: string - type: array - x-kubernetes-list-type: set - type: object + type: string type: array x-kubernetes-list-type: atomic tags: - description: |- - Tags applied to the port (and corresponding trunk, if a trunk is configured.) - These tags are applied in addition to the instance's tags, which will also be applied to the port. + description: Tags applied to the port (and corresponding + trunk, if a trunk is configured.) items: type: string type: array x-kubernetes-list-type: set trunk: - description: |- - Trunk specifies whether trunking is enabled at the port level. If not - provided the value is inherited from the machine, or false for a - bastion host. + description: Trunk specifies whether trunking is enabled + at the port level. type: boolean valueSpecs: description: |- @@ -6626,6 +6428,10 @@ spec: implementations. What type of vNIC is actually available depends on deployments. If not specified, the Neutron default value is used. type: string + required: + - description + - name + - networkID type: object type: array serverGroupID: diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclustertemplates.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclustertemplates.yaml index f577f0eb3b..c6b787d2db 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclustertemplates.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclustertemplates.yaml @@ -2992,7 +2992,9 @@ spec: type: string tags: description: |- - Machine tags + Tags which will be added to the machine and all dependent resources + which support them. These are in addition to Tags defined on the + cluster. Requires Nova api 2.52 minimum! items: type: string diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackmachines.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackmachines.yaml index 26e74f69e6..e4ba8b1a88 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackmachines.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackmachines.yaml @@ -2340,7 +2340,9 @@ spec: type: string tags: description: |- - Machine tags + Tags which will be added to the machine and all dependent resources + which support them. These are in addition to Tags defined on the + cluster. Requires Nova api 2.52 minimum! items: type: string @@ -2482,6 +2484,8 @@ spec: description: Ports is the fully resolved list of ports to create for the machine. items: + description: ResolvedPortSpec is a PortOpts with all contained + references fully resolved. properties: adminStateUp: description: AdminStateUp specifies whether the port should @@ -2526,91 +2530,20 @@ spec: IP address to assign to the port. If specified, these must be subnets of the port's network. items: + description: ResolvedFixedIP is a FixedIP with the Subnet + resolved to an ID. properties: ipAddress: description: |- - IPAddress is a specific IP address to assign to the port. If Subnet + IPAddress is a specific IP address to assign to the port. If SubnetID is also specified, IPAddress must be a valid IP address in the subnet. If Subnet is not specified, IPAddress must be a valid IP address in any subnet of the port's network. type: string subnet: - description: |- - Subnet is an openstack subnet query that will return the id of a subnet to create - the fixed IP of a port in. This query must not return more than one subnet. - properties: - cidr: - type: string - description: - type: string - gatewayIP: - type: string - id: - type: string - ipVersion: - type: integer - ipv6AddressMode: - type: string - ipv6RAMode: - type: string - name: - type: string - notTags: - description: |- - NotTags is a list of tags to filter by. If specified, resources which - contain all of the given tags will be excluded from the result. - items: - description: |- - NeutronTag represents a tag on a Neutron resource. - It may not be empty and may not contain commas. - minLength: 1 - pattern: ^[^,]+$ - type: string - type: array - x-kubernetes-list-type: set - notTagsAny: - description: |- - NotTagsAny is a list of tags to filter by. If specified, resources - which contain any of the given tags will be excluded from the result. - items: - description: |- - NeutronTag represents a tag on a Neutron resource. - It may not be empty and may not contain commas. - minLength: 1 - pattern: ^[^,]+$ - type: string - type: array - x-kubernetes-list-type: set - projectID: - type: string - tags: - description: |- - Tags is a list of tags to filter by. If specified, the resource must - have all of the tags specified to be included in the result. - items: - description: |- - NeutronTag represents a tag on a Neutron resource. - It may not be empty and may not contain commas. - minLength: 1 - pattern: ^[^,]+$ - type: string - type: array - x-kubernetes-list-type: set - tagsAny: - description: |- - TagsAny is a list of tags to filter by. If specified, the resource - must have at least one of the tags specified to be included in the - result. - items: - description: |- - NeutronTag represents a tag on a Neutron resource. - It may not be empty and may not contain commas. - minLength: 1 - pattern: ^[^,]+$ - type: string - type: array - x-kubernetes-list-type: set - type: object + description: SubnetID is the id of a subnet to create + the fixed IP of a port in. + type: string type: object type: array x-kubernetes-list-type: atomic @@ -2622,78 +2555,13 @@ spec: description: MACAddress specifies the MAC address of the port. If not specified, the MAC address will be generated. type: string - nameSuffix: - description: NameSuffix will be appended to the name of - the port if specified. If unspecified, instead the 0-based - index of the port in the list is used. + name: + description: Name is the name of the port. + type: string + networkID: + description: NetworkID is the ID of the network the port + will be created in. type: string - network: - description: |- - Network is a query for an openstack network that the port will be created or discovered on. - This will fail if the query returns more than one network. - properties: - description: - type: string - id: - type: string - name: - type: string - notTags: - description: |- - NotTags is a list of tags to filter by. If specified, resources which - contain all of the given tags will be excluded from the result. - items: - description: |- - NeutronTag represents a tag on a Neutron resource. - It may not be empty and may not contain commas. - minLength: 1 - pattern: ^[^,]+$ - type: string - type: array - x-kubernetes-list-type: set - notTagsAny: - description: |- - NotTagsAny is a list of tags to filter by. If specified, resources - which contain any of the given tags will be excluded from the result. - items: - description: |- - NeutronTag represents a tag on a Neutron resource. - It may not be empty and may not contain commas. - minLength: 1 - pattern: ^[^,]+$ - type: string - type: array - x-kubernetes-list-type: set - projectID: - type: string - tags: - description: |- - Tags is a list of tags to filter by. If specified, the resource must - have all of the tags specified to be included in the result. - items: - description: |- - NeutronTag represents a tag on a Neutron resource. - It may not be empty and may not contain commas. - minLength: 1 - pattern: ^[^,]+$ - type: string - type: array - x-kubernetes-list-type: set - tagsAny: - description: |- - TagsAny is a list of tags to filter by. If specified, the resource - must have at least one of the tags specified to be included in the - result. - items: - description: |- - NeutronTag represents a tag on a Neutron resource. - It may not be empty and may not contain commas. - minLength: 1 - pattern: ^[^,]+$ - type: string - type: array - x-kubernetes-list-type: set - type: object profile: description: |- Profile is a set of key-value pairs that are used for binding @@ -2718,88 +2586,22 @@ spec: propagate uplink status on the port. type: boolean securityGroups: - description: SecurityGroups is a list of the names, uuids, - filters or any combination these of the security groups - to assign to the instance. + description: SecurityGroups is a list of security group + IDs to assign to the port. items: - properties: - description: - type: string - id: - type: string - name: - type: string - notTags: - description: |- - NotTags is a list of tags to filter by. If specified, resources which - contain all of the given tags will be excluded from the result. - items: - description: |- - NeutronTag represents a tag on a Neutron resource. - It may not be empty and may not contain commas. - minLength: 1 - pattern: ^[^,]+$ - type: string - type: array - x-kubernetes-list-type: set - notTagsAny: - description: |- - NotTagsAny is a list of tags to filter by. If specified, resources - which contain any of the given tags will be excluded from the result. - items: - description: |- - NeutronTag represents a tag on a Neutron resource. - It may not be empty and may not contain commas. - minLength: 1 - pattern: ^[^,]+$ - type: string - type: array - x-kubernetes-list-type: set - projectID: - type: string - tags: - description: |- - Tags is a list of tags to filter by. If specified, the resource must - have all of the tags specified to be included in the result. - items: - description: |- - NeutronTag represents a tag on a Neutron resource. - It may not be empty and may not contain commas. - minLength: 1 - pattern: ^[^,]+$ - type: string - type: array - x-kubernetes-list-type: set - tagsAny: - description: |- - TagsAny is a list of tags to filter by. If specified, the resource - must have at least one of the tags specified to be included in the - result. - items: - description: |- - NeutronTag represents a tag on a Neutron resource. - It may not be empty and may not contain commas. - minLength: 1 - pattern: ^[^,]+$ - type: string - type: array - x-kubernetes-list-type: set - type: object + type: string type: array x-kubernetes-list-type: atomic tags: - description: |- - Tags applied to the port (and corresponding trunk, if a trunk is configured.) - These tags are applied in addition to the instance's tags, which will also be applied to the port. + description: Tags applied to the port (and corresponding + trunk, if a trunk is configured.) items: type: string type: array x-kubernetes-list-type: set trunk: - description: |- - Trunk specifies whether trunking is enabled at the port level. If not - provided the value is inherited from the machine, or false for a - bastion host. + description: Trunk specifies whether trunking is enabled + at the port level. type: boolean valueSpecs: description: |- @@ -2841,6 +2643,10 @@ spec: implementations. What type of vNIC is actually available depends on deployments. If not specified, the Neutron default value is used. type: string + required: + - description + - name + - networkID type: object type: array serverGroupID: diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackmachinetemplates.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackmachinetemplates.yaml index 0780b5e09f..6a90500b59 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackmachinetemplates.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackmachinetemplates.yaml @@ -2019,7 +2019,9 @@ spec: type: string tags: description: |- - Machine tags + Tags which will be added to the machine and all dependent resources + which support them. These are in addition to Tags defined on the + cluster. Requires Nova api 2.52 minimum! items: type: string diff --git a/controllers/openstackcluster_controller.go b/controllers/openstackcluster_controller.go index 6f0b9592b3..4b4138f862 100644 --- a/controllers/openstackcluster_controller.go +++ b/controllers/openstackcluster_controller.go @@ -52,6 +52,7 @@ import ( "sigs.k8s.io/cluster-api-provider-openstack/pkg/scope" utils "sigs.k8s.io/cluster-api-provider-openstack/pkg/utils/controllers" "sigs.k8s.io/cluster-api-provider-openstack/pkg/utils/filterconvert" + "sigs.k8s.io/cluster-api-provider-openstack/pkg/utils/names" ) const ( @@ -147,11 +148,13 @@ func (r *OpenStackClusterReconciler) reconcileDelete(ctx context.Context, scope return ctrl.Result{RequeueAfter: 5 * time.Second}, nil } + clusterName := names.ClusterName(cluster) + // A bastion may have been created if cluster initialisation previously reached populating the network status // We attempt to delete it even if no status was written, just in case if openStackCluster.Status.Network != nil { // Attempt to resolve bastion resources before delete. We don't need to worry about starting if the resources have changed on update. - if _, err := resolveBastionResources(scope, openStackCluster); err != nil { + if _, err := resolveBastionResources(scope, clusterName, openStackCluster); err != nil { return reconcile.Result{}, err } @@ -165,8 +168,6 @@ func (r *OpenStackClusterReconciler) reconcileDelete(ctx context.Context, scope return reconcile.Result{}, err } - clusterName := fmt.Sprintf("%s-%s", cluster.Namespace, cluster.Name) - if openStackCluster.Spec.APIServerLoadBalancer.IsEnabled() { loadBalancerService, err := loadbalancer.NewService(scope) if err != nil { @@ -217,12 +218,16 @@ func contains(arr []string, target string) bool { return false } -func resolveBastionResources(scope *scope.WithLogger, openStackCluster *infrav1.OpenStackCluster) (bool, error) { +func resolveBastionResources(scope *scope.WithLogger, clusterName string, openStackCluster *infrav1.OpenStackCluster) (bool, error) { + // Resolve and store referenced & dependent resources for the bastion if openStackCluster.Spec.Bastion != nil && openStackCluster.Spec.Bastion.Enabled { if openStackCluster.Status.Bastion == nil { openStackCluster.Status.Bastion = &infrav1.BastionStatus{} } - changed, err := compute.ResolveReferencedMachineResources(scope, openStackCluster, &openStackCluster.Spec.Bastion.Instance, &openStackCluster.Status.Bastion.ReferencedResources) + changed, err := compute.ResolveReferencedMachineResources(scope, + &openStackCluster.Spec.Bastion.Instance, &openStackCluster.Status.Bastion.ReferencedResources, + clusterName, bastionName(clusterName), + openStackCluster, getBastionSecurityGroupID(openStackCluster)) if err != nil { return false, err } @@ -232,7 +237,6 @@ func resolveBastionResources(scope *scope.WithLogger, openStackCluster *infrav1. } err = compute.AdoptDependentMachineResources(scope, - bastionName(openStackCluster.Name), &openStackCluster.Status.Bastion.ReferencedResources, &openStackCluster.Status.Bastion.DependentResources) if err != nil { @@ -380,7 +384,8 @@ func reconcileNormal(scope *scope.WithLogger, cluster *clusterv1.Cluster, openSt func reconcileBastion(scope *scope.WithLogger, cluster *clusterv1.Cluster, openStackCluster *infrav1.OpenStackCluster) (*ctrl.Result, error) { scope.Logger().V(4).Info("Reconciling Bastion") - changed, err := resolveBastionResources(scope, openStackCluster) + clusterName := names.ClusterName(cluster) + changed, err := resolveBastionResources(scope, clusterName, openStackCluster) if err != nil { return nil, err } @@ -417,7 +422,6 @@ func reconcileBastion(scope *scope.WithLogger, cluster *clusterv1.Cluster, openS if err != nil { return nil, err } - clusterName := fmt.Sprintf("%s-%s", cluster.Namespace, cluster.Name) bastionHash, err := compute.HashInstanceSpec(instanceSpec) if err != nil { @@ -433,7 +437,7 @@ func reconcileBastion(scope *scope.WithLogger, cluster *clusterv1.Cluster, openS return &reconcile.Result{}, nil } - err = getOrCreateBastionPorts(openStackCluster, networkingService, cluster.Name) + err = getOrCreateBastionPorts(openStackCluster, networkingService) if err != nil { handleUpdateOSCError(openStackCluster, fmt.Errorf("failed to get or create ports for bastion: %w", err)) return nil, fmt.Errorf("failed to get or create ports for bastion: %w", err) @@ -526,64 +530,47 @@ func bastionAddFloatingIP(openStackCluster *infrav1.OpenStackCluster, clusterNam } func bastionToInstanceSpec(openStackCluster *infrav1.OpenStackCluster, cluster *clusterv1.Cluster) (*compute.InstanceSpec, error) { - if openStackCluster.Spec.Bastion == nil { + bastionSpec := openStackCluster.Spec.Bastion + if bastionSpec == nil { return nil, fmt.Errorf("bastion spec is nil") } if openStackCluster.Status.Bastion == nil { return nil, fmt.Errorf("bastion status is nil") } - instanceSpec := &compute.InstanceSpec{ - Name: bastionName(cluster.Name), - Flavor: openStackCluster.Spec.Bastion.Instance.Flavor, - SSHKeyName: openStackCluster.Spec.Bastion.Instance.SSHKeyName, - ImageID: openStackCluster.Status.Bastion.ReferencedResources.ImageID, - FailureDomain: openStackCluster.Spec.Bastion.AvailabilityZone, - RootVolume: openStackCluster.Spec.Bastion.Instance.RootVolume, - } - - instanceSpec.SecurityGroups = openStackCluster.Spec.Bastion.Instance.SecurityGroups - if openStackCluster.Spec.ManagedSecurityGroups != nil { - if openStackCluster.Status.BastionSecurityGroup != nil { - instanceSpec.SecurityGroups = append(instanceSpec.SecurityGroups, infrav1.SecurityGroupFilter{ - ID: openStackCluster.Status.BastionSecurityGroup.ID, - }) - } - } - instanceSpec.SecurityGroups = getBastionSecurityGroups(openStackCluster) + referencedResources := &openStackCluster.Status.Bastion.ReferencedResources - instanceSpec.Ports = openStackCluster.Spec.Bastion.Instance.Ports - - return instanceSpec, nil + machineSpec := &bastionSpec.Instance + return &compute.InstanceSpec{ + Name: bastionName(cluster.Name), + Flavor: machineSpec.Flavor, + SSHKeyName: machineSpec.SSHKeyName, + ImageID: referencedResources.ImageID, + FailureDomain: bastionSpec.AvailabilityZone, + RootVolume: machineSpec.RootVolume, + ServerGroupID: referencedResources.ServerGroupID, + Tags: compute.InstanceTags(machineSpec, openStackCluster), + }, nil } func bastionName(clusterName string) string { return fmt.Sprintf("%s-bastion", clusterName) } -// getBastionSecurityGroups returns a combination of openStackCluster.Spec.Bastion.Instance.SecurityGroups -// and the security group managed by the OpenStackCluster. -func getBastionSecurityGroups(openStackCluster *infrav1.OpenStackCluster) []infrav1.SecurityGroupFilter { - instanceSpecSecurityGroups := openStackCluster.Spec.Bastion.Instance.SecurityGroups - +// getBastionSecurityGroupID returns the ID of the bastion security group if +// managed security groups is enabled. +func getBastionSecurityGroupID(openStackCluster *infrav1.OpenStackCluster) *string { if openStackCluster.Spec.ManagedSecurityGroups == nil { - return instanceSpecSecurityGroups + return nil } - var managedSecurityGroup string if openStackCluster.Status.BastionSecurityGroup != nil { - managedSecurityGroup = openStackCluster.Status.BastionSecurityGroup.ID + return &openStackCluster.Status.BastionSecurityGroup.ID } - - if managedSecurityGroup != "" { - instanceSpecSecurityGroups = append(instanceSpecSecurityGroups, infrav1.SecurityGroupFilter{ - ID: managedSecurityGroup, - }) - } - return instanceSpecSecurityGroups + return nil } -func getOrCreateBastionPorts(openStackCluster *infrav1.OpenStackCluster, networkingService *networking.Service, clusterName string) error { +func getOrCreateBastionPorts(openStackCluster *infrav1.OpenStackCluster, networkingService *networking.Service) error { desiredPorts := openStackCluster.Status.Bastion.ReferencedResources.Ports dependentResources := &openStackCluster.Status.Bastion.DependentResources @@ -591,9 +578,7 @@ func getOrCreateBastionPorts(openStackCluster *infrav1.OpenStackCluster, network return nil } - securityGroups := getBastionSecurityGroups(openStackCluster) - bastionTags := []string{} - err := networkingService.CreatePorts(openStackCluster, clusterName, bastionName(clusterName), securityGroups, bastionTags, desiredPorts, dependentResources) + err := networkingService.CreatePorts(openStackCluster, desiredPorts, dependentResources) if err != nil { return fmt.Errorf("failed to create ports for bastion %s: %w", bastionName(openStackCluster.Name), err) } @@ -611,7 +596,7 @@ func bastionHashHasChanged(computeHash string, clusterAnnotations map[string]str } func reconcileNetworkComponents(scope *scope.WithLogger, cluster *clusterv1.Cluster, openStackCluster *infrav1.OpenStackCluster) error { - clusterName := fmt.Sprintf("%s-%s", cluster.Namespace, cluster.Name) + clusterName := names.ClusterName(cluster) networkingService, err := networking.NewService(scope) if err != nil { diff --git a/controllers/openstackcluster_controller_test.go b/controllers/openstackcluster_controller_test.go index 3f3ec703d7..3de0b5c175 100644 --- a/controllers/openstackcluster_controller_test.go +++ b/controllers/openstackcluster_controller_test.go @@ -23,6 +23,7 @@ import ( "testing" "github.com/golang/mock/gomock" + "github.com/google/go-cmp/cmp" "github.com/gophercloud/gophercloud" "github.com/gophercloud/gophercloud/openstack/compute/v2/servers" "github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/external" @@ -234,11 +235,9 @@ var _ = Describe("OpenStackCluster controller", func() { Bastion: &infrav1.BastionStatus{ ReferencedResources: infrav1.ReferencedMachineResources{ ImageID: "imageID", - Ports: []infrav1.PortOpts{ + Ports: []infrav1.ResolvedPortSpec{ { - Network: &infrav1.NetworkFilter{ - ID: "network-id", - }, + NetworkID: "network-id", }, }, }, @@ -280,16 +279,14 @@ var _ = Describe("OpenStackCluster controller", func() { networkClientRecorder.ListFloatingIP(floatingips.ListOpts{PortID: "portID1"}).Return(make([]floatingips.FloatingIP, 1), nil) res, err := reconcileBastion(scope, capiCluster, testCluster) - Expect(testCluster.Status.Bastion).To(Equal(&infrav1.BastionStatus{ + expectedStatus := &infrav1.BastionStatus{ ID: "adopted-bastion-uuid", State: "ACTIVE", ReferencedResources: infrav1.ReferencedMachineResources{ ImageID: "imageID", - Ports: []infrav1.PortOpts{ + Ports: []infrav1.ResolvedPortSpec{ { - Network: &infrav1.NetworkFilter{ - ID: "network-id", - }, + NetworkID: "network-id", }, }, }, @@ -300,7 +297,8 @@ var _ = Describe("OpenStackCluster controller", func() { }, }, }, - })) + } + Expect(testCluster.Status.Bastion).To(Equal(expectedStatus), cmp.Diff(testCluster.Status.Bastion, expectedStatus)) Expect(err).To(BeNil()) Expect(res).To(BeNil()) }) @@ -326,11 +324,9 @@ var _ = Describe("OpenStackCluster controller", func() { ID: "adopted-fip-bastion-uuid", ReferencedResources: infrav1.ReferencedMachineResources{ ImageID: "imageID", - Ports: []infrav1.PortOpts{ + Ports: []infrav1.ResolvedPortSpec{ { - Network: &infrav1.NetworkFilter{ - ID: "network-id", - }, + NetworkID: "network-id", }, }, }, @@ -370,11 +366,9 @@ var _ = Describe("OpenStackCluster controller", func() { State: "ACTIVE", ReferencedResources: infrav1.ReferencedMachineResources{ ImageID: "imageID", - Ports: []infrav1.PortOpts{ + Ports: []infrav1.ResolvedPortSpec{ { - Network: &infrav1.NetworkFilter{ - ID: "network-id", - }, + NetworkID: "network-id", }, }, }, @@ -411,11 +405,9 @@ var _ = Describe("OpenStackCluster controller", func() { ID: "requeue-bastion-uuid", ReferencedResources: infrav1.ReferencedMachineResources{ ImageID: "imageID", - Ports: []infrav1.PortOpts{ + Ports: []infrav1.ResolvedPortSpec{ { - Network: &infrav1.NetworkFilter{ - ID: "network-id", - }, + NetworkID: "network-id", }, }, }, @@ -449,11 +441,9 @@ var _ = Describe("OpenStackCluster controller", func() { State: "BUILD", ReferencedResources: infrav1.ReferencedMachineResources{ ImageID: "imageID", - Ports: []infrav1.PortOpts{ + Ports: []infrav1.ResolvedPortSpec{ { - Network: &infrav1.NetworkFilter{ - ID: "network-id", - }, + NetworkID: "network-id", }, }, }, @@ -791,40 +781,3 @@ func Test_getAPIServerPort(t *testing.T) { }) } } - -func TestGetBastionSecurityGroups(t *testing.T) { - openStackCluster := &infrav1.OpenStackCluster{ - Spec: infrav1.OpenStackClusterSpec{ - Bastion: &infrav1.Bastion{ - Instance: infrav1.OpenStackMachineSpec{ - SecurityGroups: []infrav1.SecurityGroupFilter{ - { - ID: "sg-123", - }, - }, - }, - }, - ManagedSecurityGroups: &infrav1.ManagedSecurityGroups{}, - }, - Status: infrav1.OpenStackClusterStatus{ - BastionSecurityGroup: &infrav1.SecurityGroupStatus{ - ID: "sg-456", - }, - }, - } - - expectedSecurityGroups := []infrav1.SecurityGroupFilter{ - { - ID: "sg-123", - }, - { - ID: "sg-456", - }, - } - - securityGroups := getBastionSecurityGroups(openStackCluster) - - if !reflect.DeepEqual(securityGroups, expectedSecurityGroups) { - t.Errorf("Expected security groups %v, but got %v", expectedSecurityGroups, securityGroups) - } -} diff --git a/controllers/openstackmachine_controller.go b/controllers/openstackmachine_controller.go index 294959e410..3bce8afa22 100644 --- a/controllers/openstackmachine_controller.go +++ b/controllers/openstackmachine_controller.go @@ -161,11 +161,12 @@ func (r *OpenStackMachineReconciler) Reconcile(ctx context.Context, req ctrl.Req return r.reconcileNormal(ctx, scope, clusterName, infraCluster, machine, openStackMachine) } -func resolveMachineResources(scope *scope.WithLogger, openStackCluster *infrav1.OpenStackCluster, openStackMachine *infrav1.OpenStackMachine) (bool, error) { +func resolveMachineResources(scope *scope.WithLogger, clusterName string, openStackCluster *infrav1.OpenStackCluster, openStackMachine *infrav1.OpenStackMachine, machine *clusterv1.Machine) (bool, error) { // Resolve and store referenced resources changed, err := compute.ResolveReferencedMachineResources(scope, - openStackCluster, - &openStackMachine.Spec, &openStackMachine.Status.ReferencedResources) + &openStackMachine.Spec, &openStackMachine.Status.ReferencedResources, + clusterName, openStackMachine.Name, + openStackCluster, getManagedSecurityGroup(openStackCluster, machine)) if err != nil { return false, err } @@ -175,7 +176,7 @@ func resolveMachineResources(scope *scope.WithLogger, openStackCluster *infrav1. } // Adopt any existing dependent resources - return false, compute.AdoptDependentMachineResources(scope, openStackMachine.Name, &openStackMachine.Status.ReferencedResources, &openStackMachine.Status.DependentResources) + return false, compute.AdoptDependentMachineResources(scope, &openStackMachine.Status.ReferencedResources, &openStackMachine.Status.DependentResources) } func patchMachine(ctx context.Context, patchHelper *patch.Helper, openStackMachine *infrav1.OpenStackMachine, machine *clusterv1.Machine, options ...patch.Option) error { @@ -245,7 +246,7 @@ func (r *OpenStackMachineReconciler) reconcileDelete(scope *scope.WithLogger, cl // We may have resources to adopt if the cluster is ready if openStackCluster.Status.Ready && openStackCluster.Status.Network != nil { - if _, err := resolveMachineResources(scope, openStackCluster, openStackMachine); err != nil { + if _, err := resolveMachineResources(scope, clusterName, openStackCluster, openStackMachine, machine); err != nil { return ctrl.Result{}, err } } @@ -483,7 +484,7 @@ func (r *OpenStackMachineReconciler) reconcileNormal(ctx context.Context, scope return ctrl.Result{RequeueAfter: waitForClusterInfrastructureReadyDuration}, nil } - if changed, err := resolveMachineResources(scope, openStackCluster, openStackMachine); changed || err != nil { + if changed, err := resolveMachineResources(scope, clusterName, openStackCluster, openStackMachine, machine); changed || err != nil { return ctrl.Result{}, err } @@ -514,7 +515,7 @@ func (r *OpenStackMachineReconciler) reconcileNormal(ctx context.Context, scope return ctrl.Result{}, err } - err = getOrCreateMachinePorts(openStackCluster, machine, openStackMachine, networkingService, clusterName) + err = getOrCreateMachinePorts(openStackMachine, networkingService) if err != nil { return ctrl.Result{}, err } @@ -656,7 +657,7 @@ func (r *OpenStackMachineReconciler) reconcileAPIServerLoadBalancer(scope *scope return nil } -func getOrCreateMachinePorts(openStackCluster *infrav1.OpenStackCluster, machine *clusterv1.Machine, openStackMachine *infrav1.OpenStackMachine, networkingService *networking.Service, clusterName string) error { +func getOrCreateMachinePorts(openStackMachine *infrav1.OpenStackMachine, networkingService *networking.Service) error { desiredPorts := openStackMachine.Status.ReferencedResources.Ports dependentResources := &openStackMachine.Status.DependentResources @@ -664,9 +665,7 @@ func getOrCreateMachinePorts(openStackCluster *infrav1.OpenStackCluster, machine return nil } - instanceTags := getInstanceTags(openStackMachine, openStackCluster) - managedSecurityGroups := getManagedSecurityGroups(openStackCluster, machine, openStackMachine) - if err := networkingService.CreatePorts(openStackMachine, clusterName, openStackMachine.Name, managedSecurityGroups, instanceTags, desiredPorts, dependentResources); err != nil { + if err := networkingService.CreatePorts(openStackMachine, desiredPorts, dependentResources); err != nil { return fmt.Errorf("creating ports: %w", err) } @@ -735,69 +734,29 @@ func machineToInstanceSpec(openStackCluster *infrav1.OpenStackCluster, machine * instanceSpec.FailureDomain = *machine.Spec.FailureDomain } - instanceSpec.Tags = getInstanceTags(openStackMachine, openStackCluster) - instanceSpec.SecurityGroups = getManagedSecurityGroups(openStackCluster, machine, openStackMachine) - instanceSpec.Ports = openStackMachine.Spec.Ports + instanceSpec.Tags = compute.InstanceTags(&openStackMachine.Spec, openStackCluster) return &instanceSpec } -// getInstanceTags returns the tags that should be applied to the instance. -// The tags are a combination of the tags specified on the OpenStackMachine and -// the ones specified on the OpenStackCluster. -func getInstanceTags(openStackMachine *infrav1.OpenStackMachine, openStackCluster *infrav1.OpenStackCluster) []string { - machineTags := []string{} - - // Append machine specific tags - machineTags = append(machineTags, openStackMachine.Spec.Tags...) - - // Append cluster scope tags - machineTags = append(machineTags, openStackCluster.Spec.Tags...) - - // tags need to be unique or the "apply tags" call will fail. - deduplicate := func(tags []string) []string { - seen := make(map[string]struct{}, len(machineTags)) - unique := make([]string, 0, len(machineTags)) - for _, tag := range tags { - if _, ok := seen[tag]; !ok { - seen[tag] = struct{}{} - unique = append(unique, tag) - } - } - return unique - } - machineTags = deduplicate(machineTags) - - return machineTags -} - -// getManagedSecurityGroups returns a combination of OpenStackMachine.Spec.SecurityGroups -// and the security group managed by the OpenStackCluster whether it's a control plane or a worker machine. -func getManagedSecurityGroups(openStackCluster *infrav1.OpenStackCluster, machine *clusterv1.Machine, openStackMachine *infrav1.OpenStackMachine) []infrav1.SecurityGroupFilter { - machineSpecSecurityGroups := openStackMachine.Spec.SecurityGroups - +// getManagedSecurityGroup returns the ID of the security group managed by the +// OpenStackCluster whether it's a control plane or a worker machine. +func getManagedSecurityGroup(openStackCluster *infrav1.OpenStackCluster, machine *clusterv1.Machine) *string { if openStackCluster.Spec.ManagedSecurityGroups == nil { - return machineSpecSecurityGroups + return nil } - var managedSecurityGroup string if util.IsControlPlaneMachine(machine) { if openStackCluster.Status.ControlPlaneSecurityGroup != nil { - managedSecurityGroup = openStackCluster.Status.ControlPlaneSecurityGroup.ID + return &openStackCluster.Status.ControlPlaneSecurityGroup.ID } } else { if openStackCluster.Status.WorkerSecurityGroup != nil { - managedSecurityGroup = openStackCluster.Status.WorkerSecurityGroup.ID + return &openStackCluster.Status.WorkerSecurityGroup.ID } } - if managedSecurityGroup != "" { - machineSpecSecurityGroups = append(machineSpecSecurityGroups, infrav1.SecurityGroupFilter{ - ID: managedSecurityGroup, - }) - } - - return machineSpecSecurityGroups + return nil } func (r *OpenStackMachineReconciler) reconcileLoadBalancerMember(scope *scope.WithLogger, openStackCluster *infrav1.OpenStackCluster, openStackMachine *infrav1.OpenStackMachine, instanceNS *compute.InstanceNetworkStatus, clusterName string) error { diff --git a/controllers/openstackmachine_controller_test.go b/controllers/openstackmachine_controller_test.go index 648b1abc0d..1351f3e2f6 100644 --- a/controllers/openstackmachine_controller_test.go +++ b/controllers/openstackmachine_controller_test.go @@ -20,6 +20,7 @@ import ( "reflect" "testing" + "github.com/google/go-cmp/cmp" . "github.com/onsi/gomega" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/utils/pointer" @@ -112,11 +113,10 @@ func getDefaultInstanceSpec() *compute.InstanceSpec { Metadata: map[string]string{ "test-metadata": "test-value", }, - ConfigDrive: *pointer.Bool(true), - FailureDomain: *pointer.String(failureDomain), - ServerGroupID: serverGroupUUID, - SecurityGroups: []infrav1.SecurityGroupFilter{}, - Tags: []string{"test-tag"}, + ConfigDrive: *pointer.Bool(true), + FailureDomain: *pointer.String(failureDomain), + ServerGroupID: serverGroupUUID, + Tags: []string{"test-tag"}, } } @@ -137,102 +137,6 @@ func Test_machineToInstanceSpec(t *testing.T) { openStackMachine: getDefaultOpenStackMachine, wantInstanceSpec: getDefaultInstanceSpec, }, - { - name: "Control plane security group", - openStackCluster: func() *infrav1.OpenStackCluster { - c := getDefaultOpenStackCluster() - c.Spec.ManagedSecurityGroups = &infrav1.ManagedSecurityGroups{} - return c - }, - machine: func() *clusterv1.Machine { - m := getDefaultMachine() - m.Labels = map[string]string{ - clusterv1.MachineControlPlaneLabel: "true", - } - return m - }, - openStackMachine: getDefaultOpenStackMachine, - wantInstanceSpec: func() *compute.InstanceSpec { - i := getDefaultInstanceSpec() - i.SecurityGroups = []infrav1.SecurityGroupFilter{{ID: controlPlaneSecurityGroupUUID}} - return i - }, - }, - { - name: "Worker security group", - openStackCluster: func() *infrav1.OpenStackCluster { - c := getDefaultOpenStackCluster() - c.Spec.ManagedSecurityGroups = &infrav1.ManagedSecurityGroups{} - return c - }, - machine: getDefaultMachine, - openStackMachine: getDefaultOpenStackMachine, - wantInstanceSpec: func() *compute.InstanceSpec { - i := getDefaultInstanceSpec() - i.SecurityGroups = []infrav1.SecurityGroupFilter{{ID: workerSecurityGroupUUID}} - return i - }, - }, - { - name: "Control plane security group not applied to worker", - openStackCluster: func() *infrav1.OpenStackCluster { - c := getDefaultOpenStackCluster() - c.Spec.ManagedSecurityGroups = &infrav1.ManagedSecurityGroups{} - c.Status.WorkerSecurityGroup = nil - return c - }, - machine: getDefaultMachine, - openStackMachine: getDefaultOpenStackMachine, - wantInstanceSpec: func() *compute.InstanceSpec { - i := getDefaultInstanceSpec() - i.SecurityGroups = []infrav1.SecurityGroupFilter{} - return i - }, - }, - { - name: "Worker security group not applied to control plane", - openStackCluster: func() *infrav1.OpenStackCluster { - c := getDefaultOpenStackCluster() - c.Spec.ManagedSecurityGroups = &infrav1.ManagedSecurityGroups{} - c.Status.ControlPlaneSecurityGroup = nil - return c - }, - machine: func() *clusterv1.Machine { - m := getDefaultMachine() - m.Labels = map[string]string{ - clusterv1.MachineControlPlaneLabel: "true", - } - return m - }, - openStackMachine: getDefaultOpenStackMachine, - wantInstanceSpec: func() *compute.InstanceSpec { - i := getDefaultInstanceSpec() - i.SecurityGroups = []infrav1.SecurityGroupFilter{} - return i - }, - }, - { - name: "Extra security group", - openStackCluster: func() *infrav1.OpenStackCluster { - c := getDefaultOpenStackCluster() - c.Spec.ManagedSecurityGroups = &infrav1.ManagedSecurityGroups{} - return c - }, - machine: getDefaultMachine, - openStackMachine: func() *infrav1.OpenStackMachine { - m := getDefaultOpenStackMachine() - m.Spec.SecurityGroups = []infrav1.SecurityGroupFilter{{ID: extraSecurityGroupUUID}} - return m - }, - wantInstanceSpec: func() *compute.InstanceSpec { - i := getDefaultInstanceSpec() - i.SecurityGroups = []infrav1.SecurityGroupFilter{ - {ID: extraSecurityGroupUUID}, - {ID: workerSecurityGroupUUID}, - } - return i - }, - }, { name: "Tags", openStackCluster: func() *infrav1.OpenStackCluster { @@ -255,220 +159,11 @@ func Test_machineToInstanceSpec(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) got := machineToInstanceSpec(tt.openStackCluster(), tt.machine(), tt.openStackMachine(), "user-data") - Expect(got).To(Equal(tt.wantInstanceSpec())) - }) - } -} - -func Test_getInstanceTags(t *testing.T) { - tests := []struct { - name string - openStackMachine func() *infrav1.OpenStackMachine - openStackCluster func() *infrav1.OpenStackCluster - wantMachineTags []string - }{ - { - name: "No tags", - openStackMachine: func() *infrav1.OpenStackMachine { - return &infrav1.OpenStackMachine{ - Spec: infrav1.OpenStackMachineSpec{}, - } - }, - openStackCluster: func() *infrav1.OpenStackCluster { - return &infrav1.OpenStackCluster{ - Spec: infrav1.OpenStackClusterSpec{}, - } - }, - wantMachineTags: []string{}, - }, - { - name: "Machine tags only", - openStackMachine: func() *infrav1.OpenStackMachine { - return &infrav1.OpenStackMachine{ - Spec: infrav1.OpenStackMachineSpec{ - Tags: []string{"machine-tag1", "machine-tag2"}, - }, - } - }, - openStackCluster: func() *infrav1.OpenStackCluster { - return &infrav1.OpenStackCluster{ - Spec: infrav1.OpenStackClusterSpec{}, - } - }, - wantMachineTags: []string{"machine-tag1", "machine-tag2"}, - }, - { - name: "Cluster tags only", - openStackMachine: func() *infrav1.OpenStackMachine { - return &infrav1.OpenStackMachine{ - Spec: infrav1.OpenStackMachineSpec{}, - } - }, - openStackCluster: func() *infrav1.OpenStackCluster { - return &infrav1.OpenStackCluster{ - Spec: infrav1.OpenStackClusterSpec{ - Tags: []string{"cluster-tag1", "cluster-tag2"}, - }, - } - }, - wantMachineTags: []string{"cluster-tag1", "cluster-tag2"}, - }, - { - name: "Machine and cluster tags", - openStackMachine: func() *infrav1.OpenStackMachine { - return &infrav1.OpenStackMachine{ - Spec: infrav1.OpenStackMachineSpec{ - Tags: []string{"machine-tag1", "machine-tag2"}, - }, - } - }, - openStackCluster: func() *infrav1.OpenStackCluster { - return &infrav1.OpenStackCluster{ - Spec: infrav1.OpenStackClusterSpec{ - Tags: []string{"cluster-tag1", "cluster-tag2"}, - }, - } - }, - wantMachineTags: []string{"machine-tag1", "machine-tag2", "cluster-tag1", "cluster-tag2"}, - }, - { - name: "Duplicate tags", - openStackMachine: func() *infrav1.OpenStackMachine { - return &infrav1.OpenStackMachine{ - Spec: infrav1.OpenStackMachineSpec{ - Tags: []string{"tag1", "tag2", "tag1"}, - }, - } - }, - openStackCluster: func() *infrav1.OpenStackCluster { - return &infrav1.OpenStackCluster{ - Spec: infrav1.OpenStackClusterSpec{ - Tags: []string{"tag2", "tag3", "tag3"}, - }, - } - }, - wantMachineTags: []string{"tag1", "tag2", "tag3"}, - }, - } + wanted := tt.wantInstanceSpec() - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - gotMachineTags := getInstanceTags(tt.openStackMachine(), tt.openStackCluster()) - if !reflect.DeepEqual(gotMachineTags, tt.wantMachineTags) { - t.Errorf("getInstanceTags() = %v, want %v", gotMachineTags, tt.wantMachineTags) - } - }) - } -} - -func Test_getManagedSecurityGroups(t *testing.T) { - tests := []struct { - name string - openStackCluster func() *infrav1.OpenStackCluster - machine func() *clusterv1.Machine - openStackMachine func() *infrav1.OpenStackMachine - wantSecurityGroups []infrav1.SecurityGroupFilter - }{ - { - name: "Defaults", - openStackCluster: getDefaultOpenStackCluster, - machine: getDefaultMachine, - openStackMachine: getDefaultOpenStackMachine, - wantSecurityGroups: []infrav1.SecurityGroupFilter{}, - }, - { - name: "Control plane machine with control plane security group", - openStackCluster: func() *infrav1.OpenStackCluster { - c := getDefaultOpenStackCluster() - c.Spec.ManagedSecurityGroups = &infrav1.ManagedSecurityGroups{} - c.Status.ControlPlaneSecurityGroup = &infrav1.SecurityGroupStatus{ID: controlPlaneSecurityGroupUUID} - return c - }, - machine: func() *clusterv1.Machine { - m := getDefaultMachine() - m.Labels = map[string]string{ - clusterv1.MachineControlPlaneLabel: "true", - } - return m - }, - openStackMachine: getDefaultOpenStackMachine, - wantSecurityGroups: []infrav1.SecurityGroupFilter{ - {ID: controlPlaneSecurityGroupUUID}, - }, - }, - { - name: "Worker machine with worker security group", - openStackCluster: func() *infrav1.OpenStackCluster { - c := getDefaultOpenStackCluster() - c.Spec.ManagedSecurityGroups = &infrav1.ManagedSecurityGroups{} - c.Status.WorkerSecurityGroup = &infrav1.SecurityGroupStatus{ID: workerSecurityGroupUUID} - return c - }, - machine: getDefaultMachine, - openStackMachine: getDefaultOpenStackMachine, - wantSecurityGroups: []infrav1.SecurityGroupFilter{ - {ID: workerSecurityGroupUUID}, - }, - }, - { - name: "Control plane machine without control plane security group", - openStackCluster: func() *infrav1.OpenStackCluster { - c := getDefaultOpenStackCluster() - c.Spec.ManagedSecurityGroups = &infrav1.ManagedSecurityGroups{} - c.Status.ControlPlaneSecurityGroup = nil - return c - }, - machine: func() *clusterv1.Machine { - m := getDefaultMachine() - m.Labels = map[string]string{ - clusterv1.MachineControlPlaneLabel: "true", - } - return m - }, - openStackMachine: getDefaultOpenStackMachine, - wantSecurityGroups: []infrav1.SecurityGroupFilter{}, - }, - { - name: "Worker machine without worker security group", - openStackCluster: func() *infrav1.OpenStackCluster { - c := getDefaultOpenStackCluster() - c.Spec.ManagedSecurityGroups = &infrav1.ManagedSecurityGroups{} - c.Status.WorkerSecurityGroup = nil - return c - }, - machine: getDefaultMachine, - openStackMachine: getDefaultOpenStackMachine, - wantSecurityGroups: []infrav1.SecurityGroupFilter{}, - }, - { - name: "Machine with additional security groups", - openStackCluster: func() *infrav1.OpenStackCluster { - c := getDefaultOpenStackCluster() - c.Spec.ManagedSecurityGroups = &infrav1.ManagedSecurityGroups{} - c.Status.ControlPlaneSecurityGroup = &infrav1.SecurityGroupStatus{ID: controlPlaneSecurityGroupUUID} - c.Status.WorkerSecurityGroup = &infrav1.SecurityGroupStatus{ID: workerSecurityGroupUUID} - return c - }, - machine: getDefaultMachine, - openStackMachine: func() *infrav1.OpenStackMachine { - m := getDefaultOpenStackMachine() - m.Spec.SecurityGroups = []infrav1.SecurityGroupFilter{{ID: extraSecurityGroupUUID}} - return m - }, - wantSecurityGroups: []infrav1.SecurityGroupFilter{ - {ID: extraSecurityGroupUUID}, - {ID: workerSecurityGroupUUID}, - }, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - gotMachineSecurity := getManagedSecurityGroups(tt.openStackCluster(), tt.machine(), tt.openStackMachine()) - if !reflect.DeepEqual(gotMachineSecurity, tt.wantSecurityGroups) { - t.Errorf("getManagedSecurityGroups() = %v, want %v", gotMachineSecurity, tt.wantSecurityGroups) - } + g.Expect(got).To(Equal(wanted), cmp.Diff(got, wanted)) }) } } diff --git a/docs/book/src/api/v1beta1/api.md b/docs/book/src/api/v1beta1/api.md index ae9159d5fb..28835e99ad 100644 --- a/docs/book/src/api/v1beta1/api.md +++ b/docs/book/src/api/v1beta1/api.md @@ -654,7 +654,9 @@ bool -

Machine tags +

Tags which will be added to the machine and all dependent resources +which support them. These are in addition to Tags defined on the +cluster. Requires Nova api 2.52 minimum!

@@ -977,7 +979,7 @@ additional storage options.

(Appears on: -PortOpts) +ResolvedPortSpecFields)

@@ -1232,7 +1234,7 @@ DependentMachineResources

(Appears on: -PortOpts) +ResolvedPortSpecFields)

@@ -3119,7 +3121,9 @@ bool -

Machine tags +

Tags which will be added to the machine and all dependent resources +which support them. These are in addition to Tags defined on the +cluster. Requires Nova api 2.52 minimum!

@@ -3486,7 +3490,9 @@ bool -

Machine tags +

Tags which will be added to the machine and all dependent resources +which support them. These are in addition to Tags defined on the +cluster. Requires Nova api 2.52 minimum!

@@ -3625,8 +3631,7 @@ OpenStackMachineTemplateResource

(Appears on: -OpenStackMachineSpec, -ReferencedMachineResources) +OpenStackMachineSpec)

@@ -3655,18 +3660,6 @@ This will fail if the query returns more than one network.

-nameSuffix
- -string - - - -(Optional) -

NameSuffix will be appended to the name of the port if specified. If unspecified, instead the 0-based index of the port in the list is used.

- - - - description
string @@ -3679,26 +3672,14 @@ string -adminStateUp
- -bool - - - -(Optional) -

AdminStateUp specifies whether the port should be created in the up (true) or down (false) state. The default is up.

- - - - -macAddress
+nameSuffix
string (Optional) -

MACAddress specifies the MAC address of the port. If not specified, the MAC address will be generated.

+

NameSuffix will be appended to the name of the port if specified. If unspecified, instead the 0-based index of the port in the list is used.

@@ -3731,20 +3712,15 @@ string -allowedAddressPairs
+tags
- -[]AddressPair - +[]string (Optional) -

AllowedAddressPairs is a list of address pairs which Neutron will -allow the port to send traffic from in addition to the port’s -addresses. If not specified, the MAC Address will be the MAC Address -of the port. Depending on the configuration of Neutron, it may be -supported to specify a CIDR instead of a specific IP address.

+

Tags applied to the port (and corresponding trunk, if a trunk is configured.) +These tags are applied in addition to the instance’s tags, which will also be applied to the port.

@@ -3763,118 +3739,162 @@ bastion host.

-hostID
+ResolvedPortSpecFields
-string + +ResolvedPortSpecFields + -(Optional) -

HostID specifies the ID of the host where the port resides.

+

+(Members of ResolvedPortSpecFields are embedded into this type.) +

+ + +

PortStatus +

+

+(Appears on: +DependentMachineResources) +

+

+

+ + + + + + + + + +
FieldDescription
-vnicType
+id
string 		-(Optional) -

VNICType specifies the type of vNIC which this port should be -attached to. This is used to determine which mechanism driver(s) to -be used to bind the port. The valid values are normal, macvtap, -direct, baremetal, direct-physical, virtio-forwarder, smart-nic and -remote-managed, although these values will not be validated in this -API to ensure compatibility with future neutron changes or custom -implementations. What type of vNIC is actually available depends on -deployments. If not specified, the Neutron default value is used.

+

ID is the unique identifier of the port.
+

ReferencedMachineResources +

+

+(Appears on: +BastionStatus, +OpenStackMachineStatus) +

+

+

ReferencedMachineResources contains resolved references to resources required by the machine.

+

+ + + + + + + + + +
FieldDescription
-profile
+serverGroupID
- -BindingProfile - +string 		(Optional) -

Profile is a set of key-value pairs that are used for binding -details. We intentionally don’t expose this as a map[string]string -because we only want to enable the users to set the values of the -keys that are known to work in OpenStack Networking API. See -https://docs.openstack.org/api-ref/network/v2/index.html?expanded=create-port-detail#create-port -To set profiles, your tenant needs permissions rule:create_port, and -rule:create_port:binding:profile

+

ServerGroupID is the ID of the server group the machine should be added to and is calculated based on ServerGroupFilter.
-disablePortSecurity
+imageID
-bool +string 		(Optional) -

DisablePortSecurity enables or disables the port security when set. -When not set, it takes the value of the corresponding field at the network level.

+

ImageID is the ID of the image to use for the machine and is calculated based on ImageFilter.
-propagateUplinkStatus
+ports
-bool + +[]ResolvedPortSpec + 		(Optional) -

PropageteUplinkStatus enables or disables the propagate uplink status on the port.

+

Ports is the fully resolved list of ports to create for the machine.
+

ResolvedFixedIP +

+

+(Appears on: +ResolvedPortSpec) +

+

+

ResolvedFixedIP is a FixedIP with the Subnet resolved to an ID.

+

+ + + + + + + +
FieldDescription
-tags
+subnet
-[]string +string 		(Optional) -

Tags applied to the port (and corresponding trunk, if a trunk is configured.) -These tags are applied in addition to the instance’s tags, which will also be applied to the port.

+

SubnetID is the id of a subnet to create the fixed IP of a port in.
-valueSpecs
+ipAddress
- -[]ValueSpec - +string 		(Optional) -

Value specs are extra parameters to include in the API request with OpenStack. -This is an extension point for the API, so what they do and if they are supported, -depends on the specific OpenStack implementation.

+

IPAddress is a specific IP address to assign to the port. If SubnetID +is also specified, IPAddress must be a valid IP address in the +subnet. If Subnet is not specified, IPAddress must be a valid IP +address in any subnet of the port’s network.
-

PortStatus +

ResolvedPortSpec

(Appears on: -DependentMachineResources) +ReferencedMachineResources)

+

ResolvedPortSpec is a PortOpts with all contained references fully resolved.

@@ -3886,26 +3906,115 @@ depends on the specific OpenStack implementation.

+ + + + + + + + + + + + + + + + + + + + + + + + + + + +
-id
+name
string 		-

ID is the unique identifier of the port.

+

Name is the name of the port.

+
+description
+ +string + +		 +

Description is a human-readable description for the port.

+
+networkID
+ +string + +		 +

NetworkID is the ID of the network the port will be created in.

+
+tags
+ +[]string + +		 +(Optional) +

Tags applied to the port (and corresponding trunk, if a trunk is configured.)

+
+trunk
+ +bool + +		 +(Optional) +

Trunk specifies whether trunking is enabled at the port level.

+
+fixedIPs
+ + +[]ResolvedFixedIP + + +		 +(Optional) +

FixedIPs is a list of pairs of subnet and/or IP address to assign to the port. If specified, these must be subnets of the port’s network.

+
+securityGroups
+ +[]string + +		 +(Optional) +

SecurityGroups is a list of security group IDs to assign to the port.

+
+ResolvedPortSpecFields
+ + +ResolvedPortSpecFields + + +		 +

+(Members of ResolvedPortSpecFields are embedded into this type.) +
-

ReferencedMachineResources +

ResolvedPortSpecFields

(Appears on: -BastionStatus, -OpenStackMachineStatus) +PortOpts, +ResolvedPortSpec)

-

ReferencedMachineResources contains resolved references to resources required by the machine.

+

ResolvePortSpecFields is a convenience struct containing all fields of a +PortOpts which don’t contain references which need to be resolved, and can +therefore be shared with ResolvedPortSpec.

@@ -3917,40 +4026,136 @@ string + + + + + + + + + + + + + + + + + + + + + + + + @@ -4880,7 +5085,7 @@ outside of these ranges manually.

(Appears on: -PortOpts) +ResolvedPortSpecFields)

ValueSpec represents a single value_spec key-value pair.

diff --git a/pkg/cloud/services/compute/dependent_resources.go b/pkg/cloud/services/compute/dependent_resources.go index 1425c46fbf..0d45f07fb4 100644 --- a/pkg/cloud/services/compute/dependent_resources.go +++ b/pkg/cloud/services/compute/dependent_resources.go @@ -22,11 +22,11 @@ import ( "sigs.k8s.io/cluster-api-provider-openstack/pkg/scope" ) -func AdoptDependentMachineResources(scope *scope.WithLogger, baseName string, referencedResources *infrav1.ReferencedMachineResources, dependentResources *infrav1.DependentMachineResources) error { +func AdoptDependentMachineResources(scope *scope.WithLogger, referencedResources *infrav1.ReferencedMachineResources, dependentResources *infrav1.DependentMachineResources) error { networkingService, err := networking.NewService(scope) if err != nil { return err } - return networkingService.AdoptPorts(scope, baseName, referencedResources.Ports, dependentResources) + return networkingService.AdoptPorts(scope, referencedResources.Ports, dependentResources) } diff --git a/pkg/cloud/services/compute/instance_test.go b/pkg/cloud/services/compute/instance_test.go index e715171556..af9214e8a7 100644 --- a/pkg/cloud/services/compute/instance_test.go +++ b/pkg/cloud/services/compute/instance_test.go @@ -175,11 +175,10 @@ func getDefaultInstanceSpec() *InstanceSpec { Metadata: map[string]string{ "test-metadata": "test-value", }, - ConfigDrive: *pointer.Bool(true), - FailureDomain: *pointer.String(failureDomain), - ServerGroupID: serverGroupUUID, - Tags: []string{"test-tag"}, - SecurityGroups: []infrav1.SecurityGroupFilter{{ID: workerSecurityGroupUUID}}, + ConfigDrive: *pointer.Bool(true), + FailureDomain: *pointer.String(failureDomain), + ServerGroupID: serverGroupUUID, + Tags: []string{"test-tag"}, } } diff --git a/pkg/cloud/services/compute/instance_types.go b/pkg/cloud/services/compute/instance_types.go index 6c9c211936..76f2650a83 100644 --- a/pkg/cloud/services/compute/instance_types.go +++ b/pkg/cloud/services/compute/instance_types.go @@ -46,8 +46,6 @@ type InstanceSpec struct { ServerGroupID string Trunk bool Tags []string - SecurityGroups []infrav1.SecurityGroupFilter - Ports []infrav1.PortOpts } // InstanceIdentifier describes an instance which has not necessarily been fetched. diff --git a/pkg/cloud/services/compute/referenced_resources.go b/pkg/cloud/services/compute/referenced_resources.go index ae967b19cc..11004b699e 100644 --- a/pkg/cloud/services/compute/referenced_resources.go +++ b/pkg/cloud/services/compute/referenced_resources.go @@ -18,6 +18,7 @@ package compute import ( "fmt" + "slices" infrav1 "sigs.k8s.io/cluster-api-provider-openstack/api/v1beta1" "sigs.k8s.io/cluster-api-provider-openstack/pkg/cloud/services/networking" @@ -30,7 +31,7 @@ import ( // Note that we only set the fields in ReferencedMachineResources that are not set yet. This is ok because: // - OpenStackMachine is immutable, so we can't change the spec after the machine is created. // - the bastion is mutable, but we delete the bastion when the spec changes, so the bastion status will be empty. -func ResolveReferencedMachineResources(scope *scope.WithLogger, openStackCluster *infrav1.OpenStackCluster, spec *infrav1.OpenStackMachineSpec, resources *infrav1.ReferencedMachineResources) (changed bool, err error) { +func ResolveReferencedMachineResources(scope *scope.WithLogger, spec *infrav1.OpenStackMachineSpec, resources *infrav1.ReferencedMachineResources, clusterName, baseName string, openStackCluster *infrav1.OpenStackCluster, managedSecurityGroup *string) (changed bool, err error) { changed = false computeService, err := NewService(scope) @@ -72,13 +73,8 @@ func ResolveReferencedMachineResources(scope *scope.WithLogger, openStackCluster // Network resources are required in order to get ports options. if len(resources.Ports) == 0 { - // For now we put this here but realistically an OpenStack administrator could enable/disable trunk - // support at any time, so we should probably check this on every reconcile. - trunkSupported, err := networkingService.IsTrunkExtSupported() - if err != nil { - return changed, err - } - portsOpts, err := networkingService.ConstructPorts(openStackCluster, spec.Ports, spec.Trunk, trunkSupported) + defaultNetwork := openStackCluster.Status.Network + portsOpts, err := networkingService.ConstructPorts(spec, clusterName, baseName, defaultNetwork, managedSecurityGroup, InstanceTags(spec, openStackCluster)) if err != nil { return changed, err } @@ -88,3 +84,20 @@ func ResolveReferencedMachineResources(scope *scope.WithLogger, openStackCluster return changed, nil } + +// InstanceTags returns the tags that should be applied to an instance. +// The tags are a deduplicated combination of the tags specified in the +// OpenStackMachineSpec and the ones specified on the OpenStackCluster. +func InstanceTags(spec *infrav1.OpenStackMachineSpec, openStackCluster *infrav1.OpenStackCluster) []string { + machineTags := slices.Concat(spec.Tags, openStackCluster.Spec.Tags) + + seen := make(map[string]struct{}, len(machineTags)) + unique := make([]string, 0, len(machineTags)) + for _, tag := range machineTags { + if _, ok := seen[tag]; !ok { + seen[tag] = struct{}{} + unique = append(unique, tag) + } + } + return slices.Clip(unique) +} diff --git a/pkg/cloud/services/compute/referenced_resources_test.go b/pkg/cloud/services/compute/referenced_resources_test.go index 1dd50788b6..489f92cde2 100644 --- a/pkg/cloud/services/compute/referenced_resources_test.go +++ b/pkg/cloud/services/compute/referenced_resources_test.go @@ -17,6 +17,7 @@ limitations under the License. package compute import ( + "reflect" "testing" "github.com/go-logr/logr/testr" @@ -24,7 +25,6 @@ import ( "github.com/google/go-cmp/cmp" "github.com/gophercloud/gophercloud/openstack/compute/v2/extensions/servergroups" "github.com/gophercloud/gophercloud/openstack/imageservice/v2/images" - "github.com/gophercloud/gophercloud/openstack/networking/v2/extensions" . "github.com/onsi/gomega" "k8s.io/utils/pointer" @@ -34,154 +34,160 @@ import ( ) func Test_ResolveReferencedMachineResources(t *testing.T) { - constFalse := false - const serverGroupID1 = "ce96e584-7ebc-46d6-9e55-987d72e3806c" - imageID1 := "de96e584-7ebc-46d6-9e55-987d72e3806c" + const ( + serverGroupID1 = "ce96e584-7ebc-46d6-9e55-987d72e3806c" + imageID1 = "de96e584-7ebc-46d6-9e55-987d72e3806c" + networkID1 = "23ab8b71-89d4-425f-ac81-4eb83b35125a" + networkID2 = "cc8f75ce-6ce4-4b8a-836e-e5dac91cc9c8" + subnetID = "32dc0e7f-34b6-4544-a69b-248955618736" + ) - minimumReferences := &infrav1.ReferencedMachineResources{ - ImageID: imageID1, + defaultPorts := []infrav1.ResolvedPortSpec{ + { + Name: "test-instance-0", + Description: "Created by cluster-api-provider-openstack cluster test-cluster", + NetworkID: networkID1, + FixedIPs: []infrav1.ResolvedFixedIP{ + {SubnetID: pointer.String(subnetID)}, + }, + }, } tests := []struct { - testName string - serverGroupFilter *infrav1.ServerGroupFilter - imageFilter *infrav1.ImageFilter - portsOpts *[]infrav1.PortOpts - clusterStatus *infrav1.OpenStackClusterStatus - expectComputeMock func(m *mock.MockComputeClientMockRecorder) - expectImageMock func(m *mock.MockImageClientMockRecorder) - expectNetworkMock func(m *mock.MockNetworkClientMockRecorder) - want *infrav1.ReferencedMachineResources - wantErr bool + testName string + spec infrav1.OpenStackMachineSpec + managedSecurityGroup *string + expectComputeMock func(m *mock.MockComputeClientMockRecorder) + expectImageMock func(m *mock.MockImageClientMockRecorder) + expectNetworkMock func(m *mock.MockNetworkClientMockRecorder) + before *infrav1.ReferencedMachineResources + want *infrav1.ReferencedMachineResources + wantErr bool }{ { - testName: "Resources ID passed", - serverGroupFilter: &infrav1.ServerGroupFilter{ID: serverGroupID1}, - imageFilter: &infrav1.ImageFilter{ID: &imageID1}, - expectComputeMock: func(m *mock.MockComputeClientMockRecorder) {}, - expectImageMock: func(m *mock.MockImageClientMockRecorder) {}, - expectNetworkMock: func(m *mock.MockNetworkClientMockRecorder) {}, - want: &infrav1.ReferencedMachineResources{ImageID: imageID1, ServerGroupID: serverGroupID1}, - wantErr: false, + testName: "Resources ID passed", + spec: infrav1.OpenStackMachineSpec{ + ServerGroup: &infrav1.ServerGroupFilter{ID: serverGroupID1}, + Image: infrav1.ImageFilter{ID: pointer.String(imageID1)}, + }, + want: &infrav1.ReferencedMachineResources{ + ImageID: imageID1, + ServerGroupID: serverGroupID1, + Ports: defaultPorts, + }, }, { - testName: "Server group filter nil", - serverGroupFilter: nil, - expectComputeMock: func(m *mock.MockComputeClientMockRecorder) {}, - expectImageMock: func(m *mock.MockImageClientMockRecorder) {}, - expectNetworkMock: func(m *mock.MockNetworkClientMockRecorder) {}, - want: minimumReferences, - wantErr: false, + testName: "Only image ID passed: want image id and default ports", + spec: infrav1.OpenStackMachineSpec{ + Image: infrav1.ImageFilter{ID: pointer.String(imageID1)}, + }, + want: &infrav1.ReferencedMachineResources{ + ImageID: imageID1, + Ports: defaultPorts, + }, }, { - testName: "Server group ID empty", - serverGroupFilter: &infrav1.ServerGroupFilter{}, - expectComputeMock: func(m *mock.MockComputeClientMockRecorder) {}, - expectImageMock: func(m *mock.MockImageClientMockRecorder) {}, - expectNetworkMock: func(m *mock.MockNetworkClientMockRecorder) {}, - want: minimumReferences, - wantErr: false, + testName: "Server group empty", + spec: infrav1.OpenStackMachineSpec{ + Image: infrav1.ImageFilter{ID: pointer.String(imageID1)}, + ServerGroup: &infrav1.ServerGroupFilter{}, + }, + want: &infrav1.ReferencedMachineResources{ + ImageID: imageID1, + Ports: defaultPorts, + }, }, { - testName: "Server group by Name not found", - serverGroupFilter: &infrav1.ServerGroupFilter{Name: "test-server-group"}, + testName: "Server group by Name not found", + spec: infrav1.OpenStackMachineSpec{ + Image: infrav1.ImageFilter{ID: pointer.String(imageID1)}, + ServerGroup: &infrav1.ServerGroupFilter{Name: "test-server-group"}, + }, expectComputeMock: func(m *mock.MockComputeClientMockRecorder) { m.ListServerGroups().Return( []servergroups.ServerGroup{}, nil) }, - expectImageMock: func(m *mock.MockImageClientMockRecorder) {}, - expectNetworkMock: func(m *mock.MockNetworkClientMockRecorder) {}, - want: &infrav1.ReferencedMachineResources{}, - wantErr: true, + want: &infrav1.ReferencedMachineResources{}, + wantErr: true, }, { - testName: "Image by Name not found", - imageFilter: &infrav1.ImageFilter{Name: pointer.String("test-image")}, - expectComputeMock: func(m *mock.MockComputeClientMockRecorder) {}, + testName: "Image by Name not found", + spec: infrav1.OpenStackMachineSpec{ + Image: infrav1.ImageFilter{Name: pointer.String("test-image")}, + }, expectImageMock: func(m *mock.MockImageClientMockRecorder) { - m.ListImages(images.ListOpts{Name: "test-image"}).Return( - []images.Image{}, - nil) + m.ListImages(images.ListOpts{Name: "test-image"}).Return([]images.Image{}, nil) }, - expectNetworkMock: func(m *mock.MockNetworkClientMockRecorder) {}, - want: &infrav1.ReferencedMachineResources{}, - wantErr: true, + want: &infrav1.ReferencedMachineResources{}, + wantErr: true, }, { - testName: "PortsOpts set", - clusterStatus: &infrav1.OpenStackClusterStatus{ - Network: &infrav1.NetworkStatusWithSubnets{ - Subnets: []infrav1.Subnet{ - { - ID: "test-subnet-id", + testName: "Ports set", + spec: infrav1.OpenStackMachineSpec{ + Image: infrav1.ImageFilter{ID: pointer.String(imageID1)}, + Ports: []infrav1.PortOpts{ + { + Network: &infrav1.NetworkFilter{ + ID: networkID2, }, }, }, }, - portsOpts: &[]infrav1.PortOpts{ - { - Network: &infrav1.NetworkFilter{ - ID: "test-network-id", - }, - Trunk: &constFalse, - }, - }, - expectComputeMock: func(m *mock.MockComputeClientMockRecorder) {}, - expectImageMock: func(m *mock.MockImageClientMockRecorder) {}, - expectNetworkMock: func(m *mock.MockNetworkClientMockRecorder) { - m.ListExtensions().Return([]extensions.Extension{}, nil) - }, want: &infrav1.ReferencedMachineResources{ ImageID: imageID1, - Ports: []infrav1.PortOpts{ + Ports: []infrav1.ResolvedPortSpec{ { - Network: &infrav1.NetworkFilter{ - ID: "test-network-id", - }, - Trunk: &constFalse, + Name: "test-instance-0", + Description: "Created by cluster-api-provider-openstack cluster test-cluster", + NetworkID: networkID2, }, }, }, - wantErr: false, }, } - for _, tt := range tests { + for i, tt := range tests { t.Run(tt.testName, func(t *testing.T) { + tt := &tests[i] g := NewWithT(t) log := testr.New(t) mockCtrl := gomock.NewController(t) mockScopeFactory := scope.NewMockScopeFactory(mockCtrl, "") - tt.expectComputeMock(mockScopeFactory.ComputeClient.EXPECT()) - tt.expectImageMock(mockScopeFactory.ImageClient.EXPECT()) - tt.expectNetworkMock(mockScopeFactory.NetworkClient.EXPECT()) - - // Set defaults for required fields - imageFilter := &infrav1.ImageFilter{ID: pointer.String(imageID1)} - if tt.imageFilter != nil { - imageFilter = tt.imageFilter + if tt.expectComputeMock != nil { + tt.expectComputeMock(mockScopeFactory.ComputeClient.EXPECT()) } - portsOpts := &[]infrav1.PortOpts{} - if tt.portsOpts != nil { - portsOpts = tt.portsOpts + if tt.expectImageMock != nil { + tt.expectImageMock(mockScopeFactory.ImageClient.EXPECT()) } - - openStackCluster := &infrav1.OpenStackCluster{} - if tt.clusterStatus != nil { - openStackCluster.Status = *tt.clusterStatus + if tt.expectNetworkMock != nil { + tt.expectNetworkMock(mockScopeFactory.NetworkClient.EXPECT()) } - machineSpec := &infrav1.OpenStackMachineSpec{ - ServerGroup: tt.serverGroupFilter, - Image: *imageFilter, - Ports: *portsOpts, + openStackCluster := &infrav1.OpenStackCluster{ + Status: infrav1.OpenStackClusterStatus{ + Network: &infrav1.NetworkStatusWithSubnets{ + NetworkStatus: infrav1.NetworkStatus{ + ID: networkID1, + }, + Subnets: []infrav1.Subnet{ + { + ID: subnetID, + }, + }, + }, + }, } - resources := &infrav1.ReferencedMachineResources{} + resources := tt.before + if resources == nil { + resources = &infrav1.ReferencedMachineResources{} + } + clusterName := "test-cluster" + baseName := "test-instance" scope := scope.NewWithLogger(mockScopeFactory, log) - _, err := ResolveReferencedMachineResources(scope, openStackCluster, machineSpec, resources) + _, err := ResolveReferencedMachineResources(scope, &tt.spec, resources, clusterName, baseName, openStackCluster, tt.managedSecurityGroup) if tt.wantErr { g.Expect(err).Error() return @@ -191,3 +197,94 @@ func Test_ResolveReferencedMachineResources(t *testing.T) { }) } } + +func Test_getInstanceTags(t *testing.T) { + tests := []struct { + name string + spec func() *infrav1.OpenStackMachineSpec + openStackCluster func() *infrav1.OpenStackCluster + wantMachineTags []string + }{ + { + name: "No tags", + spec: func() *infrav1.OpenStackMachineSpec { + return &infrav1.OpenStackMachineSpec{} + }, + openStackCluster: func() *infrav1.OpenStackCluster { + return &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{}, + } + }, + wantMachineTags: []string{}, + }, + { + name: "Machine tags only", + spec: func() *infrav1.OpenStackMachineSpec { + return &infrav1.OpenStackMachineSpec{ + Tags: []string{"machine-tag1", "machine-tag2"}, + } + }, + openStackCluster: func() *infrav1.OpenStackCluster { + return &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{}, + } + }, + wantMachineTags: []string{"machine-tag1", "machine-tag2"}, + }, + { + name: "Cluster tags only", + spec: func() *infrav1.OpenStackMachineSpec { + return &infrav1.OpenStackMachineSpec{} + }, + openStackCluster: func() *infrav1.OpenStackCluster { + return &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ + Tags: []string{"cluster-tag1", "cluster-tag2"}, + }, + } + }, + wantMachineTags: []string{"cluster-tag1", "cluster-tag2"}, + }, + { + name: "Machine and cluster tags", + spec: func() *infrav1.OpenStackMachineSpec { + return &infrav1.OpenStackMachineSpec{ + Tags: []string{"machine-tag1", "machine-tag2"}, + } + }, + openStackCluster: func() *infrav1.OpenStackCluster { + return &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ + Tags: []string{"cluster-tag1", "cluster-tag2"}, + }, + } + }, + wantMachineTags: []string{"machine-tag1", "machine-tag2", "cluster-tag1", "cluster-tag2"}, + }, + { + name: "Duplicate tags", + spec: func() *infrav1.OpenStackMachineSpec { + return &infrav1.OpenStackMachineSpec{ + Tags: []string{"tag1", "tag2", "tag1"}, + } + }, + openStackCluster: func() *infrav1.OpenStackCluster { + return &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ + Tags: []string{"tag2", "tag3", "tag3"}, + }, + } + }, + wantMachineTags: []string{"tag1", "tag2", "tag3"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gotMachineTags := InstanceTags(tt.spec(), tt.openStackCluster()) + if !reflect.DeepEqual(gotMachineTags, tt.wantMachineTags) { + t.Errorf("getInstanceTags() = %v, want %v", gotMachineTags, tt.wantMachineTags) + } + }) + } +} diff --git a/pkg/cloud/services/networking/network_test.go b/pkg/cloud/services/networking/network_test.go index 52b0fdb0d4..4a3401ce55 100644 --- a/pkg/cloud/services/networking/network_test.go +++ b/pkg/cloud/services/networking/network_test.go @@ -34,11 +34,14 @@ import ( "sigs.k8s.io/cluster-api-provider-openstack/pkg/utils/names" ) +const ( + clusterName = "test-cluster" +) + func Test_ReconcileNetwork(t *testing.T) { mockCtrl := gomock.NewController(t) defer mockCtrl.Finish() - clusterName := "test-cluster" expectedNetworkName := getNetworkName(clusterName) fakeNetworkID := "d08803fc-2fa5-4179-b9f7-8c43d0af2fe6" @@ -435,7 +438,6 @@ func Test_ReconcileSubnet(t *testing.T) { mockCtrl := gomock.NewController(t) defer mockCtrl.Finish() - clusterName := "test-cluster" expectedSubnetName := getSubnetName(clusterName) expectedSubnetDesc := names.GetDescription(clusterName) fakeSubnetID := "d08803fc-2fa5-4179-b9d7-8c43d0af2fe6" diff --git a/pkg/cloud/services/networking/port.go b/pkg/cloud/services/networking/port.go index b3fd46b3c2..4b6a894d56 100644 --- a/pkg/cloud/services/networking/port.go +++ b/pkg/cloud/services/networking/port.go @@ -20,6 +20,7 @@ import ( "context" "errors" "fmt" + "slices" "strings" "time" @@ -109,123 +110,94 @@ func (s *Service) GetPortForExternalNetwork(instanceID string, externalNetworkID return nil, nil } -func (s *Service) CreatePort(eventObject runtime.Object, clusterName string, portName string, portOpts *infrav1.PortOpts, defaultSecurityGroups []string, baseTags []string) (*ports.Port, error) { - var err error - networkID := portOpts.Network.ID - - var description string - if portOpts.Description != nil { - description = *portOpts.Description - } else { - description = names.GetDescription(clusterName) - } - - var securityGroups []string - addressPairs := []ports.AddressPair{} - if portOpts.DisablePortSecurity == nil || !*portOpts.DisablePortSecurity { - for _, ap := range portOpts.AllowedAddressPairs { +func (s *Service) CreatePort(eventObject runtime.Object, portSpec *infrav1.ResolvedPortSpec) (*ports.Port, error) { + var addressPairs []ports.AddressPair + if !pointer.BoolDeref(portSpec.DisablePortSecurity, false) { + for _, ap := range portSpec.AllowedAddressPairs { addressPairs = append(addressPairs, ports.AddressPair{ IPAddress: ap.IPAddress, MACAddress: pointer.StringDeref(ap.MACAddress, ""), }) } - if portOpts.SecurityGroups != nil { - securityGroups, err = s.GetSecurityGroups(portOpts.SecurityGroups) - if err != nil { - return nil, fmt.Errorf("error getting security groups: %v", err) - } - } - // inherit port security groups from the instance if not explicitly specified - if len(securityGroups) == 0 { - securityGroups = defaultSecurityGroups - } } - var fixedIPs interface{} - if len(portOpts.FixedIPs) > 0 { - fips := make([]ports.IP, 0, len(portOpts.FixedIPs)+1) - for _, fixedIP := range portOpts.FixedIPs { - subnetID, err := s.getSubnetIDForFixedIP(fixedIP.Subnet, networkID) - if err != nil { - return nil, err - } - fips = append(fips, ports.IP{ - SubnetID: subnetID, + var fixedIPs []ports.IP + if len(portSpec.FixedIPs) > 0 { + fixedIPs = make([]ports.IP, len(portSpec.FixedIPs)) + for i, fixedIP := range portSpec.FixedIPs { + fixedIPs[i] = ports.IP{ + SubnetID: pointer.StringDeref(fixedIP.SubnetID, ""), IPAddress: pointer.StringDeref(fixedIP.IPAddress, ""), - }) + } } - fixedIPs = fips } var valueSpecs *map[string]string - if len(portOpts.ValueSpecs) > 0 { - vs := make(map[string]string, len(portOpts.ValueSpecs)) - for _, valueSpec := range portOpts.ValueSpecs { + if len(portSpec.ValueSpecs) > 0 { + vs := make(map[string]string, len(portSpec.ValueSpecs)) + for _, valueSpec := range portSpec.ValueSpecs { vs[valueSpec.Key] = valueSpec.Value } valueSpecs = &vs } - var createOpts ports.CreateOptsBuilder - - // Gophercloud expects a *[]string. We translate a nil slice to a nil pointer. - var securityGroupsPtr *[]string - if securityGroups != nil { - securityGroupsPtr = &securityGroups - } - - createOpts = ports.CreateOpts{ - Name: portName, - NetworkID: networkID, - Description: description, - AdminStateUp: portOpts.AdminStateUp, - MACAddress: pointer.StringDeref(portOpts.MACAddress, ""), - SecurityGroups: securityGroupsPtr, + var builder ports.CreateOptsBuilder + createOpts := ports.CreateOpts{ + Name: portSpec.Name, + NetworkID: portSpec.NetworkID, + Description: portSpec.Description, + AdminStateUp: portSpec.AdminStateUp, + MACAddress: pointer.StringDeref(portSpec.MACAddress, ""), AllowedAddressPairs: addressPairs, - FixedIPs: fixedIPs, ValueSpecs: valueSpecs, - PropagateUplinkStatus: portOpts.PropagateUplinkStatus, + PropagateUplinkStatus: portSpec.PropagateUplinkStatus, + } + if fixedIPs != nil { + createOpts.FixedIPs = fixedIPs + } + if portSpec.SecurityGroups != nil { + createOpts.SecurityGroups = &portSpec.SecurityGroups } + builder = createOpts - if portOpts.DisablePortSecurity != nil { - portSecurity := !*portOpts.DisablePortSecurity - createOpts = portsecurity.PortCreateOptsExt{ - CreateOptsBuilder: createOpts, + if portSpec.DisablePortSecurity != nil { + portSecurity := !*portSpec.DisablePortSecurity + portSecurityOpts := portsecurity.PortCreateOptsExt{ + CreateOptsBuilder: builder, PortSecurityEnabled: &portSecurity, } + builder = portSecurityOpts } - createOpts = portsbinding.CreateOptsExt{ - CreateOptsBuilder: createOpts, - HostID: pointer.StringDeref(portOpts.HostID, ""), - VNICType: pointer.StringDeref(portOpts.VNICType, ""), - Profile: getPortProfile(portOpts.Profile), + portsBindingOpts := portsbinding.CreateOptsExt{ + CreateOptsBuilder: builder, + HostID: pointer.StringDeref(portSpec.HostID, ""), + VNICType: pointer.StringDeref(portSpec.VNICType, ""), + Profile: getPortProfile(portSpec.Profile), } + builder = portsBindingOpts - port, err := s.client.CreatePort(createOpts) + port, err := s.client.CreatePort(builder) if err != nil { - record.Warnf(eventObject, "FailedCreatePort", "Failed to create port %s: %v", portName, err) + record.Warnf(eventObject, "FailedCreatePort", "Failed to create port %s: %v", port.Name, err) return nil, err } - var tags []string - tags = append(tags, baseTags...) - tags = append(tags, portOpts.Tags...) - if len(tags) > 0 { - if err = s.replaceAllAttributesTags(eventObject, portResource, port.ID, tags); err != nil { - record.Warnf(eventObject, "FailedReplaceTags", "Failed to replace port tags %s: %v", portName, err) + if len(portSpec.Tags) > 0 { + if err = s.replaceAllAttributesTags(eventObject, portResource, port.ID, portSpec.Tags); err != nil { + record.Warnf(eventObject, "FailedReplaceTags", "Failed to replace port tags %s: %v", portSpec.Name, err) return nil, err } } record.Eventf(eventObject, "SuccessfulCreatePort", "Created port %s with id %s", port.Name, port.ID) - if portOpts.Trunk != nil && *portOpts.Trunk { - trunk, err := s.getOrCreateTrunk(eventObject, clusterName, port.Name, port.ID) + if pointer.BoolDeref(portSpec.Trunk, false) { + trunk, err := s.getOrCreateTrunkForPort(eventObject, port) if err != nil { - record.Warnf(eventObject, "FailedCreateTrunk", "Failed to create trunk for port %s: %v", portName, err) + record.Warnf(eventObject, "FailedCreateTrunk", "Failed to create trunk for port %s: %v", port.Name, err) return nil, err } - if err = s.replaceAllAttributesTags(eventObject, trunkResource, trunk.ID, tags); err != nil { - record.Warnf(eventObject, "FailedReplaceTags", "Failed to replace trunk tags %s: %v", portName, err) + if err = s.replaceAllAttributesTags(eventObject, trunkResource, trunk.ID, portSpec.Tags); err != nil { + record.Warnf(eventObject, "FailedReplaceTags", "Failed to replace trunk tags %s: %v", port.Name, err) return nil, err } } @@ -233,32 +205,6 @@ func (s *Service) CreatePort(eventObject runtime.Object, clusterName string, por return port, nil } -func (s *Service) getSubnetIDForFixedIP(subnet *infrav1.SubnetFilter, networkID string) (string, error) { - if subnet == nil { - return "", nil - } - // Do not query for subnets if UUID is already provided - if subnet.ID != "" { - return subnet.ID, nil - } - - opts := filterconvert.SubnetFilterToListOpts(subnet) - opts.NetworkID = networkID - subnets, err := s.client.ListSubnet(opts) - if err != nil { - return "", err - } - - switch len(subnets) { - case 0: - return "", fmt.Errorf("subnet query %v, returns no subnets", *subnet) - case 1: - return subnets[0].ID, nil - default: - return "", fmt.Errorf("subnet query %v, returns too many subnets: %v", *subnet, subnets) - } -} - func getPortProfile(p *infrav1.BindingProfile) map[string]interface{} { if p == nil { return nil @@ -357,30 +303,24 @@ func (s *Service) DeleteClusterPorts(openStackCluster *infrav1.OpenStackCluster) return nil } -// GetPortName appends a suffix to an instance name in order to try and get a unique name per port. -func GetPortName(instanceName string, opts *infrav1.PortOpts, netIndex int) string { - if opts != nil && opts.NameSuffix != nil { - return fmt.Sprintf("%s-%s", instanceName, *opts.NameSuffix) +// getPortName appends a suffix to an instance name in order to try and get a unique name per port. +func getPortName(baseName string, portSpec *infrav1.PortOpts, netIndex int) string { + if portSpec != nil && portSpec.NameSuffix != nil { + return fmt.Sprintf("%s-%s", baseName, *portSpec.NameSuffix) } - return fmt.Sprintf("%s-%d", instanceName, netIndex) + return fmt.Sprintf("%s-%d", baseName, netIndex) } -func (s *Service) CreatePorts(eventObject runtime.Object, clusterName, baseName string, securityGroups []infrav1.SecurityGroupFilter, baseTags []string, desiredPorts []infrav1.PortOpts, dependentResources *infrav1.DependentMachineResources) error { - defaultSecurityGroups, err := s.GetSecurityGroups(securityGroups) - if err != nil { - return fmt.Errorf("error getting security groups: %v", err) - } - +func (s *Service) CreatePorts(eventObject runtime.Object, desiredPorts []infrav1.ResolvedPortSpec, dependentResources *infrav1.DependentMachineResources) error { for i := range desiredPorts { // Skip creation of ports which already exist if i < len(dependentResources.Ports) { continue } - portOpts := &desiredPorts[i] - portName := GetPortName(baseName, portOpts, i) + portSpec := &desiredPorts[i] // Events are recorded in CreatePort - port, err := s.CreatePort(eventObject, clusterName, portName, portOpts, defaultSecurityGroups, baseTags) + port, err := s.CreatePort(eventObject, portSpec) if err != nil { return err } @@ -393,112 +333,159 @@ func (s *Service) CreatePorts(eventObject runtime.Object, clusterName, baseName return nil } -// ConstructPorts builds an array of ports from the instance spec. +// ConstructPorts builds an array of ports from the machine spec. // If no ports are in the spec, returns a single port for a network connection to the default cluster network. -func (s *Service) ConstructPorts(openStackCluster *infrav1.OpenStackCluster, ports []infrav1.PortOpts, trunkEnabled bool, trunkSupported bool) ([]infrav1.PortOpts, error) { - // If no network is specified, return nil - if openStackCluster.Status.Network == nil { - return nil, nil +func (s *Service) ConstructPorts(spec *infrav1.OpenStackMachineSpec, clusterName, baseName string, defaultNetwork *infrav1.NetworkStatusWithSubnets, managedSecurityGroup *string, baseTags []string) ([]infrav1.ResolvedPortSpec, error) { + ports := spec.Ports + + defaultSecurityGroupIDs, err := s.GetSecurityGroups(spec.SecurityGroups) + if err != nil { + return nil, fmt.Errorf("error getting security groups: %v", err) + } + if managedSecurityGroup != nil { + defaultSecurityGroupIDs = append(defaultSecurityGroupIDs, *managedSecurityGroup) } // Ensure user-specified ports have all required fields - ports, err := s.normalizePorts(ports, openStackCluster, trunkEnabled) + resolvedPorts, err := s.normalizePorts(ports, clusterName, baseName, spec.Trunk, defaultSecurityGroupIDs, defaultNetwork, baseTags) if err != nil { return nil, err } // no networks or ports found in the spec, so create a port on the cluster network - if len(ports) == 0 { - port := infrav1.PortOpts{ - Network: &infrav1.NetworkFilter{ - ID: openStackCluster.Status.Network.ID, - }, - Trunk: &trunkEnabled, + if len(resolvedPorts) == 0 { + resolvedPorts = make([]infrav1.ResolvedPortSpec, 1) + resolvedPort := &resolvedPorts[0] + resolvedPort.Name = getPortName(baseName, nil, 0) + resolvedPort.Description = names.GetDescription(clusterName) + if len(baseTags) > 0 { + resolvedPort.Tags = baseTags } - for _, subnet := range openStackCluster.Status.Network.Subnets { - port.FixedIPs = append(port.FixedIPs, infrav1.FixedIP{ - Subnet: &infrav1.SubnetFilter{ - ID: subnet.ID, - }, - }) + if spec.Trunk { + resolvedPort.Trunk = &spec.Trunk } - ports = []infrav1.PortOpts{port} + resolvedPort.SecurityGroups = defaultSecurityGroupIDs + resolvedPort.NetworkID, resolvedPort.FixedIPs, _ = defaultNetworkTarget(defaultNetwork) } // trunk support is required if any port has trunk enabled portUsesTrunk := func() bool { - for _, port := range ports { - if port.Trunk != nil && *port.Trunk { + for _, port := range resolvedPorts { + if pointer.BoolDeref(port.Trunk, false) { return true } } return false } if portUsesTrunk() { + trunkSupported, err := s.IsTrunkExtSupported() + if err != nil { + return nil, err + } + if !trunkSupported { return nil, fmt.Errorf("there is no trunk support. please ensure that the trunk extension is enabled in your OpenStack deployment") } } - return ports, nil + return resolvedPorts, nil } // normalizePorts ensures that a user-specified PortOpts has all required fields set. Specifically it: // - sets the Trunk field to the instance spec default if not specified // - sets the Network ID field if not specified. -func (s *Service) normalizePorts(ports []infrav1.PortOpts, openStackCluster *infrav1.OpenStackCluster, trunkEnabled bool) ([]infrav1.PortOpts, error) { - normalizedPorts := make([]infrav1.PortOpts, 0, len(ports)) +func (s *Service) normalizePorts(ports []infrav1.PortOpts, clusterName, baseName string, trunkEnabled bool, defaultSecurityGroupIDs []string, defaultNetwork *infrav1.NetworkStatusWithSubnets, baseTags []string) ([]infrav1.ResolvedPortSpec, error) { + normalizedPorts := make([]infrav1.ResolvedPortSpec, len(ports)) for i := range ports { - // Deep copy the port to avoid mutating the original - port := ports[i].DeepCopy() + port := &ports[i] + normalizedPort := &normalizedPorts[i] + + // Copy fields which don't need to be resolved + normalizedPort.ResolvedPortSpecFields = port.ResolvedPortSpecFields + + // Generate a standardised name + normalizedPort.Name = getPortName(baseName, port, i) + + // Generate a description if none is provided + if port.Description != nil { + normalizedPort.Description = *port.Description + } else { + normalizedPort.Description = names.GetDescription(clusterName) + } + + // Tags are inherited base tags plus any port-specific tags + normalizedPort.Tags = slices.Concat(baseTags, port.Tags) // No Trunk field specified for the port, inherit the machine default if port.Trunk == nil { - port.Trunk = &trunkEnabled + if trunkEnabled { + normalizedPort.Trunk = &trunkEnabled + } + } else { + normalizedPort.Trunk = port.Trunk } - if err := s.normalizePortTarget(port, openStackCluster, i); err != nil { + // Resolve network ID and fixed IPs + var err error + normalizedPort.NetworkID, normalizedPort.FixedIPs, err = s.normalizePortTarget(port, defaultNetwork, i) + if err != nil { return nil, err } - normalizedPorts = append(normalizedPorts, *port) + // Resolve security groups + if len(port.SecurityGroups) == 0 { + normalizedPort.SecurityGroups = defaultSecurityGroupIDs + } else { + normalizedPort.SecurityGroups, err = s.GetSecurityGroups(port.SecurityGroups) + if err != nil { + return nil, fmt.Errorf("error getting security groups: %v", err) + } + } } return normalizedPorts, nil } -// normalizePortTarget ensures that the port has a network ID. -func (s *Service) normalizePortTarget(port *infrav1.PortOpts, openStackCluster *infrav1.OpenStackCluster, portIdx int) error { - // Treat no Network and empty Network the same - noNetwork := port.Network.IsEmpty() +func defaultNetworkTarget(network *infrav1.NetworkStatusWithSubnets) (string, []infrav1.ResolvedFixedIP, error) { + networkID := network.ID + fixedIPs := make([]infrav1.ResolvedFixedIP, len(network.Subnets)) + for i := range network.Subnets { + subnet := &network.Subnets[i] + fixedIPs[i].SubnetID = &subnet.ID + } + return networkID, fixedIPs, nil +} +// normalizePortTarget ensures that the port has a network ID. +func (s *Service) normalizePortTarget(port *infrav1.PortOpts, defaultNetwork *infrav1.NetworkStatusWithSubnets, portIdx int) (string, []infrav1.ResolvedFixedIP, error) { // No network or subnets defined: use cluster defaults - if noNetwork && len(port.FixedIPs) == 0 { - port.Network = &infrav1.NetworkFilter{ - ID: openStackCluster.Status.Network.ID, - } - for _, subnet := range openStackCluster.Status.Network.Subnets { - port.FixedIPs = append(port.FixedIPs, infrav1.FixedIP{ - Subnet: &infrav1.SubnetFilter{ - ID: subnet.ID, - }, - }) - } + if port.Network == nil && len(port.FixedIPs) == 0 { + return defaultNetworkTarget(defaultNetwork) + } - return nil + var networkID string + var resolvedFixedIPs []infrav1.ResolvedFixedIP + if len(port.FixedIPs) > 0 { + resolvedFixedIPs = make([]infrav1.ResolvedFixedIP, len(port.FixedIPs)) } + switch { + case port.Network != nil && port.Network.ID != "": + networkID = port.Network.ID + // No network, but fixed IPs are defined(we handled the no fixed // IPs case above): try to infer network from a subnet - if noNetwork { + case len(port.FixedIPs) > 0: s.scope.Logger().V(4).Info("No network defined for port, attempting to infer from subnet", "port", portIdx) // Look for a unique subnet defined in FixedIPs. If we find one // we can use it to infer the network ID. We don't need to worry // here about the case where different FixedIPs have different - // networks because that will cause an error later when we try - // to create the port. - networkID, err := func() (string, error) { + // networks because that will cause an error later. + var err error + networkID, err = func() (string, error) { for i, fixedIP := range port.FixedIPs { + resolvedFixedIP := &resolvedFixedIPs[i] + if fixedIP.Subnet == nil { continue } @@ -514,8 +501,8 @@ func (s *Service) normalizePortTarget(port *infrav1.PortOpts, openStackCluster * return "", err } - // Cache the subnet ID in the FixedIP - fixedIP.Subnet.ID = subnet.ID + // Cache the known subnet ID in the FixedIP so we don't fetch it again later + resolvedFixedIP.SubnetID = &subnet.ID return subnet.NetworkID, nil } @@ -523,38 +510,40 @@ func (s *Service) normalizePortTarget(port *infrav1.PortOpts, openStackCluster * return "", fmt.Errorf("port %d has no network and unable to infer from fixed IPs", portIdx) }() if err != nil { - return err + return "", nil, err } - port.Network = &infrav1.NetworkFilter{ - ID: networkID, + // Network is defined by filter + default: + networkListOpts := filterconvert.NetworkFilterToListOpts(port.Network) + netIDs, err := s.GetNetworkIDsByFilter(networkListOpts) + if err != nil { + return "", nil, err } - return nil - } - - // Nothing to do if network ID is already set - if port.Network.ID != "" { - return nil - } - - // Network is defined by Filter - networkListOpts := filterconvert.NetworkFilterToListOpts(port.Network) - netIDs, err := s.GetNetworkIDsByFilter(networkListOpts) - if err != nil { - return err + // TODO: These are spec errors: they should set the machine to failed + if len(netIDs) > 1 { + return "", nil, fmt.Errorf("network filter for port %d returns more than one result", portIdx) + } else if len(netIDs) == 0 { + return "", nil, fmt.Errorf("network filter for port %d returns no networks", portIdx) + } + networkID = netIDs[0] } - // TODO: These are spec errors: they should set the machine to failed - if len(netIDs) > 1 { - return fmt.Errorf("network filter for port %d returns more than one result", portIdx) - } else if len(netIDs) == 0 { - return fmt.Errorf("network filter for port %d returns no networks", portIdx) + // Network ID is now known. Resolve all FixedIPs + for i, fixedIP := range port.FixedIPs { + resolvedFixedIP := &resolvedFixedIPs[i] + resolvedFixedIP.IPAddress = fixedIP.IPAddress + if fixedIP.Subnet != nil && resolvedFixedIP.SubnetID == nil { + subnet, err := s.GetNetworkSubnetByFilter(networkID, fixedIP.Subnet) + if err != nil { + return "", nil, err + } + resolvedFixedIP.SubnetID = &subnet.ID + } } - port.Network.ID = netIDs[0] - - return nil + return networkID, resolvedFixedIPs, nil } // IsTrunkExtSupported verifies trunk setup on the OpenStack deployment. @@ -571,7 +560,7 @@ func (s *Service) IsTrunkExtSupported() (trunknSupported bool, err error) { // AdoptPorts looks for ports in desiredPorts which were previously created, and adds them to dependentResources.Ports. // A port matches if it has the same name and network ID as the desired port. -func (s *Service) AdoptPorts(scope *scope.WithLogger, baseName string, desiredPorts []infrav1.PortOpts, dependentResources *infrav1.DependentMachineResources) error { +func (s *Service) AdoptPorts(scope *scope.WithLogger, desiredPorts []infrav1.ResolvedPortSpec, dependentResources *infrav1.DependentMachineResources) error { // We can skip adoption if the ports are already in the status if len(desiredPorts) == len(dependentResources.Ports) { return nil @@ -582,21 +571,20 @@ func (s *Service) AdoptPorts(scope *scope.WithLogger, baseName string, desiredPo // We create ports in order and adopt them in order in PortsStatus. // This means that if port N doesn't exist we know that ports >N don't exist. // We can therefore stop searching for ports once we find one that doesn't exist. - for i, port := range desiredPorts { + for i := range desiredPorts { // check if the port is in status first and if it is, skip it if i < len(dependentResources.Ports) { scope.Logger().V(5).Info("Port already in status, skipping it", "port index", i) continue } - portOpts := &desiredPorts[i] - portName := GetPortName(baseName, portOpts, i) + portSpec := &desiredPorts[i] ports, err := s.client.ListPort(ports.ListOpts{ - Name: portName, - NetworkID: port.Network.ID, + Name: portSpec.Name, + NetworkID: portSpec.NetworkID, }) if err != nil { - return fmt.Errorf("searching for existing port %s in network %s: %v", portName, port.Network.ID, err) + return fmt.Errorf("searching for existing port %s in network %s: %v", portSpec.Name, portSpec.NetworkID, err) } // if the port is not found, we stop the adoption of ports since the rest of the ports will not be found either // and will be created after the adoption @@ -605,7 +593,7 @@ func (s *Service) AdoptPorts(scope *scope.WithLogger, baseName string, desiredPo return nil } if len(ports) > 1 { - return fmt.Errorf("found multiple ports with name %s", portName) + return fmt.Errorf("found multiple ports with name %s", portSpec.Name) } // The desired port was found, so we add it to the status diff --git a/pkg/cloud/services/networking/port_test.go b/pkg/cloud/services/networking/port_test.go index 340d78ff91..ecf567fe74 100644 --- a/pkg/cloud/services/networking/port_test.go +++ b/pkg/cloud/services/networking/port_test.go @@ -22,14 +22,17 @@ import ( "github.com/go-logr/logr/testr" "github.com/golang/mock/gomock" "github.com/google/go-cmp/cmp" + "github.com/gophercloud/gophercloud/openstack/networking/v2/extensions" "github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/attributestags" "github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/portsbinding" "github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/portsecurity" + "github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/security/groups" "github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/trunks" "github.com/gophercloud/gophercloud/openstack/networking/v2/networks" "github.com/gophercloud/gophercloud/openstack/networking/v2/ports" "github.com/gophercloud/gophercloud/openstack/networking/v2/subnets" . "github.com/onsi/gomega" + "github.com/onsi/gomega/types" "k8s.io/utils/pointer" infrav1 "sigs.k8s.io/cluster-api-provider-openstack/api/v1beta1" @@ -38,370 +41,282 @@ import ( ) func Test_CreatePort(t *testing.T) { - // Arbitrary GUIDs used in the tests - netID := "7fd24ceb-788a-441f-ad0a-d8e2f5d31a1d" - subnetID1 := "d9c88a6d-0b8c-48ff-8f0e-8d85a078c194" - subnetID2 := "d9c2346d-05gc-48er-9ut4-ig83ayt8c7h4" - portID1 := "50214c48-c09e-4a54-914f-97b40fd22802" - hostID := "825c1b11-3dca-4bfe-a2d8-a3cc1964c8d5" - trunkID := "eb7541fa-5e2a-4cca-b2c3-dfa409b917ce" - portSecurityGroupID := "f51d1206-fc5a-4f7a-a5c0-2e03e44e4dc0" - - // Other arbitrary variables passed in to the tests - instanceSecurityGroups := []string{"instance-secgroup"} - securityGroupUUIDs := []string{portSecurityGroupID} - portSecurityGroupFilters := []infrav1.SecurityGroupFilter{{ID: portSecurityGroupID, Name: "port-secgroup"}} - valueSpecs := map[string]string{"key": "value"} + // Arbitrary values used in the tests + const ( + netID = "7fd24ceb-788a-441f-ad0a-d8e2f5d31a1d" + subnetID1 = "d9c88a6d-0b8c-48ff-8f0e-8d85a078c194" + subnetID2 = "d9c2346d-05gc-48er-9ut4-ig83ayt8c7h4" + portID = "50214c48-c09e-4a54-914f-97b40fd22802" + hostID = "825c1b11-3dca-4bfe-a2d8-a3cc1964c8d5" + trunkID = "eb7541fa-5e2a-4cca-b2c3-dfa409b917ce" + portSecurityGroupID = "f51d1206-fc5a-4f7a-a5c0-2e03e44e4dc0" + ipAddress1 = "192.0.2.1" + ipAddress2 = "198.51.100.1" + macAddress = "de:ad:be:ef:fe:ed" + ) tests := []struct { - name string - portName string - port infrav1.PortOpts - instanceSecurityGroups []string - tags []string - expect func(m *mock.MockNetworkClientMockRecorder) + name string + port infrav1.ResolvedPortSpec + expect func(m *mock.MockNetworkClientMockRecorder, g Gomega) // Note the 'wanted' port isn't so important, since it will be whatever we tell ListPort or CreatePort to return. // Mostly in this test suite, we're checking that CreatePort is called with the expected port opts. want *ports.Port wantErr bool }{ { - "creates port with defaults (description and secgroups) if not specified in portOpts", - "foo-port-1", - infrav1.PortOpts{ - Network: &infrav1.NetworkFilter{ - ID: netID, - }, - }, - instanceSecurityGroups, - []string{}, - func(m *mock.MockNetworkClientMockRecorder) { - m. - CreatePort(portsbinding.CreateOptsExt{ - CreateOptsBuilder: ports.CreateOpts{ - Name: "foo-port-1", - Description: "Created by cluster-api-provider-openstack cluster test-cluster", - SecurityGroups: &instanceSecurityGroups, - NetworkID: netID, - AllowedAddressPairs: []ports.AddressPair{}, - }, - }).Return(&ports.Port{ID: portID1}, nil) - }, - &ports.Port{ID: portID1}, - false, - }, - { - "creates port with specified portOpts if no matching port exists", - "foo-port-bar", - infrav1.PortOpts{ - Network: &infrav1.NetworkFilter{ - ID: netID, - }, - NameSuffix: pointer.String("bar"), - Description: pointer.String("this is a test port"), - MACAddress: pointer.String("fe:fe:fe:fe:fe:fe"), - AdminStateUp: pointer.Bool(true), - FixedIPs: []infrav1.FixedIP{ + name: "creates port correctly with all options specified except tags, trunk and disablePortSecurity", + port: infrav1.ResolvedPortSpec{ + Name: "foo-port-1", + Description: "Created by cluster-api-provider-openstack cluster test-cluster", + NetworkID: netID, + FixedIPs: []infrav1.ResolvedFixedIP{ + { + SubnetID: pointer.String(subnetID1), + IPAddress: pointer.String(ipAddress1), + }, { - Subnet: &infrav1.SubnetFilter{ - Name: "subnetFoo", + IPAddress: pointer.String(ipAddress2), + }, + { + SubnetID: pointer.String(subnetID2), + }, + }, + SecurityGroups: []string{portSecurityGroupID}, + ResolvedPortSpecFields: infrav1.ResolvedPortSpecFields{ + AdminStateUp: pointer.Bool(true), + MACAddress: pointer.String(macAddress), + AllowedAddressPairs: []infrav1.AddressPair{ + { + IPAddress: ipAddress1, + MACAddress: pointer.String(macAddress), + }, + { + IPAddress: ipAddress2, }, - IPAddress: pointer.String("192.168.0.50"), - }, { - IPAddress: pointer.String("192.168.1.50"), - }, - }, - SecurityGroups: portSecurityGroupFilters, - AllowedAddressPairs: []infrav1.AddressPair{{ - IPAddress: "10.10.10.10", - MACAddress: pointer.String("f1:f1:f1:f1:f1:f1"), - }}, - HostID: pointer.String(hostID), - VNICType: pointer.String("direct"), - Profile: &infrav1.BindingProfile{ - OVSHWOffload: pointer.Bool(true), - TrustedVF: pointer.Bool(true), - }, - DisablePortSecurity: pointer.Bool(false), - Tags: []string{"my-port-tag"}, - }, - nil, - nil, - func(m *mock.MockNetworkClientMockRecorder) { - portCreateOpts := ports.CreateOpts{ + }, + HostID: pointer.String(hostID), + VNICType: pointer.String("normal"), + Profile: &infrav1.BindingProfile{ + OVSHWOffload: pointer.Bool(true), + TrustedVF: pointer.Bool(true), + }, + PropagateUplinkStatus: pointer.Bool(true), + ValueSpecs: []infrav1.ValueSpec{ + { + Name: "test-valuespec", + Key: "test-key", + Value: "test-value", + }, + }, + }, + }, + expect: func(m *mock.MockNetworkClientMockRecorder, g Gomega) { + var expectedCreateOpts ports.CreateOptsBuilder + expectedCreateOpts = ports.CreateOpts{ + Name: "foo-port-1", + Description: "Created by cluster-api-provider-openstack cluster test-cluster", NetworkID: netID, - Name: "foo-port-bar", - Description: "this is a test port", AdminStateUp: pointer.Bool(true), - MACAddress: "fe:fe:fe:fe:fe:fe", + MACAddress: macAddress, FixedIPs: []ports.IP{ { SubnetID: subnetID1, - IPAddress: "192.168.0.50", - }, { - IPAddress: "192.168.1.50", + IPAddress: ipAddress1, + }, + { + IPAddress: ipAddress2, + }, + { + SubnetID: subnetID2, }, }, - SecurityGroups: &securityGroupUUIDs, - AllowedAddressPairs: []ports.AddressPair{{ - IPAddress: "10.10.10.10", - MACAddress: "f1:f1:f1:f1:f1:f1", - }}, - } - portsecurityCreateOptsExt := portsecurity.PortCreateOptsExt{ - CreateOptsBuilder: portCreateOpts, - PortSecurityEnabled: pointer.Bool(true), + SecurityGroups: &[]string{portSecurityGroupID}, + AllowedAddressPairs: []ports.AddressPair{ + { + IPAddress: ipAddress1, + MACAddress: macAddress, + }, + { + IPAddress: ipAddress2, + }, + }, + PropagateUplinkStatus: pointer.Bool(true), + ValueSpecs: &map[string]string{ + "test-key": "test-value", + }, } - portbindingCreateOptsExt := portsbinding.CreateOptsExt{ - // Note for the test matching, the order in which the builders are composed - // must be the same as in the function we are testing. - CreateOptsBuilder: portsecurityCreateOptsExt, + expectedCreateOpts = portsbinding.CreateOptsExt{ + CreateOptsBuilder: expectedCreateOpts, HostID: hostID, - VNICType: "direct", + VNICType: "normal", Profile: map[string]interface{}{ "capabilities": []string{"switchdev"}, "trusted": true, }, } - m. - CreatePort(portbindingCreateOptsExt). - Return(&ports.Port{ - ID: portID1, - }, nil) - m.ReplaceAllAttributesTags("ports", portID1, attributestags.ReplaceAllOpts{Tags: []string{"my-port-tag"}}).Return([]string{"my-port-tag"}, nil) - m. - ListSubnet(subnets.ListOpts{ - Name: "subnetFoo", - NetworkID: netID, - }).Return([]subnets.Subnet{ - { - ID: subnetID1, - Name: "subnetFoo", - NetworkID: netID, - }, - }, nil) - }, - &ports.Port{ - ID: portID1, + + // The following allows us to use gomega to + // compare the argument instead of gomock. + // Gomock's output in the case of a mismatch is + // not usable for this struct. + m.CreatePort(gomock.Any()).DoAndReturn(func(builder ports.CreateOptsBuilder) (*ports.Port, error) { + gotCreateOpts := builder.(portsbinding.CreateOptsExt) + g.Expect(gotCreateOpts).To(Equal(expectedCreateOpts), cmp.Diff(gotCreateOpts, expectedCreateOpts)) + return &ports.Port{ID: portID}, nil + }) }, - false, + want: &ports.Port{ID: portID}, }, { - "fails to create port with specified portOpts if subnet query returns more than one subnet", - "foo-port-bar", - infrav1.PortOpts{ - Network: &infrav1.NetworkFilter{ - ID: netID, - }, - NameSuffix: pointer.String("foo-port-bar"), - Description: pointer.String("this is a test port"), - FixedIPs: []infrav1.FixedIP{{ - Subnet: &infrav1.SubnetFilter{ - FilterByNeutronTags: infrav1.FilterByNeutronTags{ - Tags: []infrav1.NeutronTag{"Foo"}, - }, - }, - IPAddress: pointer.String("192.168.0.50"), - }}, - }, - nil, - nil, - func(m *mock.MockNetworkClientMockRecorder) { - m. - ListSubnet(subnets.ListOpts{ - Tags: "Foo", - NetworkID: netID, - }).Return([]subnets.Subnet{ - { - ID: subnetID1, - NetworkID: netID, - Name: "subnetFoo", - }, - { - ID: subnetID2, - NetworkID: netID, - Name: "subnetBar", - }, - }, nil) + name: "creates minimum port correctly", + port: infrav1.ResolvedPortSpec{ + Name: "test-port", + NetworkID: netID, + }, + expect: func(m *mock.MockNetworkClientMockRecorder, g Gomega) { + var expectedCreateOpts ports.CreateOptsBuilder + expectedCreateOpts = ports.CreateOpts{ + NetworkID: netID, + Name: "test-port", + } + expectedCreateOpts = portsbinding.CreateOptsExt{ + CreateOptsBuilder: expectedCreateOpts, + } + m.CreatePort(gomock.Any()).DoAndReturn(func(builder ports.CreateOptsBuilder) (*ports.Port, error) { + gotCreateOpts := builder.(portsbinding.CreateOptsExt) + g.Expect(gotCreateOpts).To(Equal(expectedCreateOpts), cmp.Diff(gotCreateOpts, expectedCreateOpts)) + return &ports.Port{ID: portID}, nil + }) }, - nil, - true, + want: &ports.Port{ID: portID}, }, { - "overrides default (instance) security groups if port security groups are specified", - "foo-port-1", - infrav1.PortOpts{ - Network: &infrav1.NetworkFilter{ - ID: netID, - }, - SecurityGroups: portSecurityGroupFilters, - }, - instanceSecurityGroups, - []string{}, - func(m *mock.MockNetworkClientMockRecorder) { - m. - CreatePort(portsbinding.CreateOptsExt{ - CreateOptsBuilder: ports.CreateOpts{ - Name: "foo-port-1", - Description: "Created by cluster-api-provider-openstack cluster test-cluster", - SecurityGroups: &securityGroupUUIDs, - NetworkID: netID, - AllowedAddressPairs: []ports.AddressPair{}, + name: "disable port security also ignores allowed address pairs", + port: infrav1.ResolvedPortSpec{ + Name: "test-port", + NetworkID: netID, + ResolvedPortSpecFields: infrav1.ResolvedPortSpecFields{ + DisablePortSecurity: pointer.Bool(true), + AllowedAddressPairs: []infrav1.AddressPair{ + { + IPAddress: ipAddress1, + MACAddress: pointer.String(macAddress), }, }, - ).Return(&ports.Port{ID: portID1}, nil) + }, }, - &ports.Port{ID: portID1}, - false, - }, - { - "creates port with instance tags when port tags aren't specified", - "foo-port-1", - infrav1.PortOpts{ - Network: &infrav1.NetworkFilter{ - ID: netID, - }, - }, - nil, - []string{"my-instance-tag"}, - func(m *mock.MockNetworkClientMockRecorder) { - m.CreatePort(portsbinding.CreateOptsExt{ - CreateOptsBuilder: ports.CreateOpts{ - Name: "foo-port-1", - Description: "Created by cluster-api-provider-openstack cluster test-cluster", - NetworkID: netID, - AllowedAddressPairs: []ports.AddressPair{}, - }, - }).Return(&ports.Port{ID: portID1}, nil) - m.ReplaceAllAttributesTags("ports", portID1, attributestags.ReplaceAllOpts{Tags: []string{"my-instance-tag"}}).Return([]string{"my-instance-tag"}, nil) - }, - &ports.Port{ID: portID1}, - false, - }, - { - "creates port with port specific tags appending to instance tags", - "foo-port-1", - infrav1.PortOpts{ - Network: &infrav1.NetworkFilter{ - ID: netID, - }, - Tags: []string{"my-port-tag"}, - }, - nil, - []string{"my-instance-tag"}, - func(m *mock.MockNetworkClientMockRecorder) { - m.CreatePort(portsbinding.CreateOptsExt{ - CreateOptsBuilder: ports.CreateOpts{ - Name: "foo-port-1", - Description: "Created by cluster-api-provider-openstack cluster test-cluster", - NetworkID: netID, - AllowedAddressPairs: []ports.AddressPair{}, - }, - }).Return(&ports.Port{ID: portID1}, nil) - m. - ReplaceAllAttributesTags("ports", portID1, attributestags.ReplaceAllOpts{Tags: []string{"my-instance-tag", "my-port-tag"}}). - Return([]string{"my-instance-tag", "my-port-tag"}, nil) - }, - &ports.Port{ID: portID1}, - false, + expect: func(m *mock.MockNetworkClientMockRecorder, g Gomega) { + var expectedCreateOpts ports.CreateOptsBuilder + expectedCreateOpts = ports.CreateOpts{ + NetworkID: netID, + Name: "test-port", + } + expectedCreateOpts = portsecurity.PortCreateOptsExt{ + CreateOptsBuilder: expectedCreateOpts, + PortSecurityEnabled: pointer.Bool(false), + } + expectedCreateOpts = portsbinding.CreateOptsExt{ + CreateOptsBuilder: expectedCreateOpts, + } + m.CreatePort(gomock.Any()).DoAndReturn(func(builder ports.CreateOptsBuilder) (*ports.Port, error) { + gotCreateOpts := builder.(portsbinding.CreateOptsExt) + g.Expect(gotCreateOpts).To(Equal(expectedCreateOpts), cmp.Diff(gotCreateOpts, expectedCreateOpts)) + return &ports.Port{ID: portID}, nil + }) + }, + want: &ports.Port{ID: portID}, }, { - "creates port and trunk (with tags) if they aren't found", - "foo-port-1", - infrav1.PortOpts{ - Network: &infrav1.NetworkFilter{ - ID: netID, - }, - Trunk: pointer.Bool(true), - }, - nil, - []string{"my-tag"}, - func(m *mock.MockNetworkClientMockRecorder) { - m. - CreatePort(portsbinding.CreateOptsExt{ - CreateOptsBuilder: ports.CreateOpts{ - Name: "foo-port-1", - Description: "Created by cluster-api-provider-openstack cluster test-cluster", - NetworkID: netID, - AllowedAddressPairs: []ports.AddressPair{}, + name: "disable port security explicitly false includes allowed address pairs", + port: infrav1.ResolvedPortSpec{ + Name: "test-port", + NetworkID: netID, + ResolvedPortSpecFields: infrav1.ResolvedPortSpecFields{ + DisablePortSecurity: pointer.Bool(false), + AllowedAddressPairs: []infrav1.AddressPair{ + { + IPAddress: ipAddress1, + MACAddress: pointer.String(macAddress), }, - }).Return(&ports.Port{Name: "foo-port-1", ID: portID1}, nil) - m. - ListTrunk(trunks.ListOpts{ - Name: "foo-port-1", - PortID: portID1, - }).Return([]trunks.Trunk{}, nil) - m. - CreateTrunk(trunks.CreateOpts{ - Name: "foo-port-1", - PortID: portID1, - Description: "Created by cluster-api-provider-openstack cluster test-cluster", - }).Return(&trunks.Trunk{ID: trunkID}, nil) - - m.ReplaceAllAttributesTags("ports", portID1, attributestags.ReplaceAllOpts{Tags: []string{"my-tag"}}).Return([]string{"my-tag"}, nil) - m.ReplaceAllAttributesTags("trunks", trunkID, attributestags.ReplaceAllOpts{Tags: []string{"my-tag"}}).Return([]string{"my-tag"}, nil) - }, - &ports.Port{Name: "foo-port-1", ID: portID1}, - false, - }, - { - "creates port with value_specs", - "foo-port-1", - infrav1.PortOpts{ - Network: &infrav1.NetworkFilter{ - ID: netID, + }, }, - ValueSpecs: []infrav1.ValueSpec{ - { - Name: "Not important", - Key: "key", - Value: "value", - }, - }, - }, - nil, - nil, - func(m *mock.MockNetworkClientMockRecorder) { - m. - CreatePort(portsbinding.CreateOptsExt{ - CreateOptsBuilder: ports.CreateOpts{ - Name: "foo-port-1", - Description: "Created by cluster-api-provider-openstack cluster test-cluster", - NetworkID: netID, - AllowedAddressPairs: []ports.AddressPair{}, - ValueSpecs: &valueSpecs, + }, + expect: func(m *mock.MockNetworkClientMockRecorder, g types.Gomega) { + var expectedCreateOpts ports.CreateOptsBuilder + expectedCreateOpts = ports.CreateOpts{ + NetworkID: netID, + Name: "test-port", + AllowedAddressPairs: []ports.AddressPair{ + { + IPAddress: ipAddress1, + MACAddress: macAddress, }, - }).Return(&ports.Port{ID: portID1}, nil) + }, + } + expectedCreateOpts = portsecurity.PortCreateOptsExt{ + CreateOptsBuilder: expectedCreateOpts, + PortSecurityEnabled: pointer.Bool(true), + } + expectedCreateOpts = portsbinding.CreateOptsExt{ + CreateOptsBuilder: expectedCreateOpts, + } + m.CreatePort(gomock.Any()).DoAndReturn(func(builder ports.CreateOptsBuilder) (*ports.Port, error) { + gotCreateOpts := builder.(portsbinding.CreateOptsExt) + g.Expect(gotCreateOpts).To(Equal(expectedCreateOpts), cmp.Diff(gotCreateOpts, expectedCreateOpts)) + return &ports.Port{ID: portID}, nil + }) }, - &ports.Port{ID: portID1}, - false, + want: &ports.Port{ID: portID}, }, { - "creates port with propagate uplink status", - "foo-port-1", - infrav1.PortOpts{ - Network: &infrav1.NetworkFilter{ - ID: netID, - }, - PropagateUplinkStatus: pointer.Bool(true), - }, - instanceSecurityGroups, - []string{}, - func(m *mock.MockNetworkClientMockRecorder) { - m. - CreatePort(portsbinding.CreateOptsExt{ - CreateOptsBuilder: ports.CreateOpts{ - Name: "foo-port-1", - Description: "Created by cluster-api-provider-openstack cluster test-cluster", - SecurityGroups: &instanceSecurityGroups, - NetworkID: netID, - AllowedAddressPairs: []ports.AddressPair{}, - PropagateUplinkStatus: pointer.Bool(true), - }, - }).Return(&ports.Port{ID: portID1, PropagateUplinkStatus: true}, nil) + name: "tags and trunk", + port: infrav1.ResolvedPortSpec{ + Name: "test-port", + NetworkID: netID, + Tags: []string{"tag1", "tag2"}, + Trunk: pointer.Bool(true), }, - &ports.Port{ID: portID1, PropagateUplinkStatus: true}, - false, + expect: func(m *mock.MockNetworkClientMockRecorder, g types.Gomega) { + var expectedCreateOpts ports.CreateOptsBuilder + expectedCreateOpts = ports.CreateOpts{ + NetworkID: netID, + Name: "test-port", + } + expectedCreateOpts = portsbinding.CreateOptsExt{ + CreateOptsBuilder: expectedCreateOpts, + } + + // Create the port + m.CreatePort(gomock.Any()).DoAndReturn(func(builder ports.CreateOptsBuilder) (*ports.Port, error) { + gotCreateOpts := builder.(portsbinding.CreateOptsExt) + g.Expect(gotCreateOpts).To(Equal(expectedCreateOpts), cmp.Diff(gotCreateOpts, expectedCreateOpts)) + return &ports.Port{ID: portID, Name: "test-port"}, nil + }) + + // Tag the port + m.ReplaceAllAttributesTags("ports", portID, attributestags.ReplaceAllOpts{ + Tags: []string{"tag1", "tag2"}, + }) + + // Look for existing trunk + m.ListTrunk(trunks.ListOpts{ + PortID: portID, + Name: "test-port", + }).Return([]trunks.Trunk{}, nil) + + // Create the trunk + m.CreateTrunk(trunks.CreateOpts{ + PortID: portID, + Name: "test-port", + }).Return(&trunks.Trunk{ID: trunkID}, nil) + + // Tag the trunk + m.ReplaceAllAttributesTags("trunks", trunkID, attributestags.ReplaceAllOpts{ + Tags: []string{"tag1", "tag2"}, + }) + }, + want: &ports.Port{ID: portID, Name: "test-port"}, }, } @@ -414,17 +329,13 @@ func Test_CreatePort(t *testing.T) { g := NewWithT(t) mockClient := mock.NewMockNetworkClient(mockCtrl) - tt.expect(mockClient.EXPECT()) + tt.expect(mockClient.EXPECT(), g) s := Service{ client: mockClient, } got, err := s.CreatePort( eventObject, - "test-cluster", - tt.portName, &tt.port, - tt.instanceSecurityGroups, - tt.tags, ) if tt.wantErr { g.Expect(err).To(HaveOccurred()) @@ -436,163 +347,159 @@ func Test_CreatePort(t *testing.T) { } } -func TestService_normalizePorts(t *testing.T) { +func TestService_ConstructPorts(t *testing.T) { const ( defaultNetworkID = "3c66f3ca-2d26-4d9d-ae3b-568f54129773" defaultSubnetID = "d8dbba89-8c39-4192-a571-e702fca35bac" - networkID = "afa54944-1443-4132-9ef5-ce37eb4d6ab6" - subnetID = "d786e715-c299-4a97-911d-640c10fc0392" + networkID = "afa54944-1443-4132-9ef5-ce37eb4d6ab6" + subnetID1 = "d786e715-c299-4a97-911d-640c10fc0392" + subnetID2 = "41ad8201-5b2f-4e0e-b29d-3d82fad6ef10" + securityGroupID1 = "044f6d31-3938-4f09-ad45-47b661e2ba1c" + securityGroupID2 = "427b77ee-40b7-4f1b-b025-72ad1a42ee51" + + defaultDescription = "Created by cluster-api-provider-openstack cluster test-cluster" ) - openStackCluster := &infrav1.OpenStackCluster{ - Status: infrav1.OpenStackClusterStatus{ - Network: &infrav1.NetworkStatusWithSubnets{ - NetworkStatus: infrav1.NetworkStatus{ - ID: defaultNetworkID, - }, - Subnets: []infrav1.Subnet{ - {ID: defaultSubnetID}, - }, - }, - }, + expectListExtensions := func(m *mock.MockNetworkClientMockRecorder) { + trunkExtension := extensions.Extension{} + trunkExtension.Alias = "trunk" + m.ListExtensions().Return([]extensions.Extension{trunkExtension}, nil) } tests := []struct { - name string - ports []infrav1.PortOpts - instanceTrunk bool - expectNetwork func(m *mock.MockNetworkClientMockRecorder) - want []infrav1.PortOpts - wantErr bool + name string + spec infrav1.OpenStackMachineSpec + managedSecurityGroup *string + expectNetwork func(m *mock.MockNetworkClientMockRecorder) + want []infrav1.ResolvedPortSpec + wantErr bool }{ { - name: "No ports: no ports", - ports: []infrav1.PortOpts{}, - want: []infrav1.PortOpts{}, - }, - { - name: "Nil network, no fixed IPs: cluster defaults", - ports: []infrav1.PortOpts{ - { - Network: nil, - FixedIPs: nil, - }, - }, - want: []infrav1.PortOpts{ + name: "No ports creates port on default network", + spec: infrav1.OpenStackMachineSpec{}, + want: []infrav1.ResolvedPortSpec{ { - Network: &infrav1.NetworkFilter{ - ID: defaultNetworkID, - }, - FixedIPs: []infrav1.FixedIP{ - { - Subnet: &infrav1.SubnetFilter{ - ID: defaultSubnetID, - }, - }, + Name: "test-instance-0", + Description: defaultDescription, + Tags: []string{"test-tag"}, + NetworkID: defaultNetworkID, + FixedIPs: []infrav1.ResolvedFixedIP{ + {SubnetID: pointer.String(defaultSubnetID)}, }, - Trunk: pointer.Bool(false), }, }, }, { - name: "Empty network, no fixed IPs: cluster defaults", - ports: []infrav1.PortOpts{ - { - Network: &infrav1.NetworkFilter{}, - FixedIPs: nil, + name: "Nil network, no fixed IPs: cluster defaults", + spec: infrav1.OpenStackMachineSpec{ + Ports: []infrav1.PortOpts{ + { + NameSuffix: pointer.String("custom"), + Network: nil, + FixedIPs: nil, + }, }, }, - want: []infrav1.PortOpts{ + want: []infrav1.ResolvedPortSpec{ { - Network: &infrav1.NetworkFilter{ - ID: defaultNetworkID, - }, - FixedIPs: []infrav1.FixedIP{ + Name: "test-instance-custom", + Description: defaultDescription, + NetworkID: defaultNetworkID, + FixedIPs: []infrav1.ResolvedFixedIP{ { - Subnet: &infrav1.SubnetFilter{ - ID: defaultSubnetID, - }, + SubnetID: pointer.String(defaultSubnetID), }, }, - Trunk: pointer.Bool(false), + Tags: []string{"test-tag"}, }, }, }, { name: "Port inherits trunk from instance", - ports: []infrav1.PortOpts{ - { - Network: &infrav1.NetworkFilter{}, - FixedIPs: nil, + spec: infrav1.OpenStackMachineSpec{ + Ports: []infrav1.PortOpts{ + { + NameSuffix: pointer.String("custom"), + Network: nil, + FixedIPs: nil, + }, }, + Trunk: true, + }, + expectNetwork: func(m *mock.MockNetworkClientMockRecorder) { + expectListExtensions(m) }, - instanceTrunk: true, - want: []infrav1.PortOpts{ + want: []infrav1.ResolvedPortSpec{ { - Network: &infrav1.NetworkFilter{ - ID: defaultNetworkID, - }, - FixedIPs: []infrav1.FixedIP{ - { - Subnet: &infrav1.SubnetFilter{ - ID: defaultSubnetID, - }, - }, + Name: "test-instance-custom", + Description: defaultDescription, + NetworkID: defaultNetworkID, + FixedIPs: []infrav1.ResolvedFixedIP{ + {SubnetID: pointer.String(defaultSubnetID)}, }, + Tags: []string{"test-tag"}, Trunk: pointer.Bool(true), }, }, + wantErr: false, }, { name: "Port overrides trunk from instance", - ports: []infrav1.PortOpts{ - { - Network: &infrav1.NetworkFilter{}, - FixedIPs: nil, - Trunk: pointer.Bool(true), + spec: infrav1.OpenStackMachineSpec{ + Ports: []infrav1.PortOpts{ + { + Trunk: pointer.Bool(true), + }, }, + Trunk: false, }, - want: []infrav1.PortOpts{ + expectNetwork: func(m *mock.MockNetworkClientMockRecorder) { + expectListExtensions(m) + }, + want: []infrav1.ResolvedPortSpec{ { - Network: &infrav1.NetworkFilter{ - ID: defaultNetworkID, - }, - FixedIPs: []infrav1.FixedIP{ - { - Subnet: &infrav1.SubnetFilter{ - ID: defaultSubnetID, - }, - }, + Name: "test-instance-0", + Description: defaultDescription, + NetworkID: defaultNetworkID, + FixedIPs: []infrav1.ResolvedFixedIP{ + {SubnetID: pointer.String(defaultSubnetID)}, }, + Tags: []string{"test-tag"}, Trunk: pointer.Bool(true), }, }, }, { - name: "Network defined by ID: unchanged", - ports: []infrav1.PortOpts{ - { - Network: &infrav1.NetworkFilter{ - ID: networkID, + name: "Network defined by ID: no lookup", + spec: infrav1.OpenStackMachineSpec{ + Ports: []infrav1.PortOpts{ + { + Network: &infrav1.NetworkFilter{ + ID: networkID, + }, }, }, }, - want: []infrav1.PortOpts{ + want: []infrav1.ResolvedPortSpec{ { - Network: &infrav1.NetworkFilter{ - ID: networkID, - }, - Trunk: pointer.Bool(false), + NetworkID: networkID, + + // Defaults + Name: "test-instance-0", + Description: defaultDescription, + Tags: []string{"test-tag"}, }, }, }, { name: "Network defined by filter: add ID from network lookup", - ports: []infrav1.PortOpts{ - { - Network: &infrav1.NetworkFilter{ - Name: "test-network", + spec: infrav1.OpenStackMachineSpec{ + Ports: []infrav1.PortOpts{ + { + Network: &infrav1.NetworkFilter{ + Name: "test-network", + }, }, }, }, @@ -601,56 +508,59 @@ func TestService_normalizePorts(t *testing.T) { {ID: networkID}, }, nil) }, - want: []infrav1.PortOpts{ + want: []infrav1.ResolvedPortSpec{ { - Network: &infrav1.NetworkFilter{ - ID: networkID, - Name: "test-network", - }, - Trunk: pointer.Bool(false), + NetworkID: networkID, + + // Defaults + Name: "test-instance-0", + Description: defaultDescription, + Tags: []string{"test-tag"}, }, }, }, { name: "No network, fixed IP has subnet by ID: add ID from subnet", - ports: []infrav1.PortOpts{ - { - FixedIPs: []infrav1.FixedIP{ - { - Subnet: &infrav1.SubnetFilter{ - ID: subnetID, + spec: infrav1.OpenStackMachineSpec{ + Ports: []infrav1.PortOpts{ + { + FixedIPs: []infrav1.FixedIP{ + { + Subnet: &infrav1.SubnetFilter{ + ID: subnetID1, + }, }, }, }, }, }, expectNetwork: func(m *mock.MockNetworkClientMockRecorder) { - m.GetSubnet(subnetID).Return(&subnets.Subnet{ID: subnetID, NetworkID: networkID}, nil) + m.GetSubnet(subnetID1).Return(&subnets.Subnet{ID: subnetID1, NetworkID: networkID}, nil) }, - want: []infrav1.PortOpts{ + want: []infrav1.ResolvedPortSpec{ { - Network: &infrav1.NetworkFilter{ - ID: networkID, + NetworkID: networkID, + FixedIPs: []infrav1.ResolvedFixedIP{ + {SubnetID: pointer.String(subnetID1)}, }, - FixedIPs: []infrav1.FixedIP{ - { - Subnet: &infrav1.SubnetFilter{ - ID: subnetID, - }, - }, - }, - Trunk: pointer.Bool(false), + + // Defaults + Name: "test-instance-0", + Description: defaultDescription, + Tags: []string{"test-tag"}, }, }, }, { name: "No network, fixed IP has subnet by filter: add ID from subnet", - ports: []infrav1.PortOpts{ - { - FixedIPs: []infrav1.FixedIP{ - { - Subnet: &infrav1.SubnetFilter{ - Name: "test-subnet", + spec: infrav1.OpenStackMachineSpec{ + Ports: []infrav1.PortOpts{ + { + FixedIPs: []infrav1.FixedIP{ + { + Subnet: &infrav1.SubnetFilter{ + Name: "test-subnet", + }, }, }, }, @@ -658,34 +568,35 @@ func TestService_normalizePorts(t *testing.T) { }, expectNetwork: func(m *mock.MockNetworkClientMockRecorder) { m.ListSubnet(subnets.ListOpts{Name: "test-subnet"}).Return([]subnets.Subnet{ - {ID: subnetID, NetworkID: networkID}, + {ID: subnetID1, NetworkID: networkID}, }, nil) }, - want: []infrav1.PortOpts{ + want: []infrav1.ResolvedPortSpec{ { - Network: &infrav1.NetworkFilter{ - ID: networkID, - }, - FixedIPs: []infrav1.FixedIP{ + NetworkID: networkID, + FixedIPs: []infrav1.ResolvedFixedIP{ { - Subnet: &infrav1.SubnetFilter{ - ID: subnetID, - Name: "test-subnet", - }, + SubnetID: pointer.String(subnetID1), }, }, - Trunk: pointer.Bool(false), + + // Defaults + Name: "test-instance-0", + Description: defaultDescription, + Tags: []string{"test-tag"}, }, }, }, { name: "No network, fixed IP subnet returns no matches: error", - ports: []infrav1.PortOpts{ - { - FixedIPs: []infrav1.FixedIP{ - { - Subnet: &infrav1.SubnetFilter{ - Name: "test-subnet", + spec: infrav1.OpenStackMachineSpec{ + Ports: []infrav1.PortOpts{ + { + FixedIPs: []infrav1.FixedIP{ + { + Subnet: &infrav1.SubnetFilter{ + Name: "test-subnet", + }, }, }, }, @@ -698,12 +609,14 @@ func TestService_normalizePorts(t *testing.T) { }, { name: "No network, only fixed IP subnet returns multiple matches: error", - ports: []infrav1.PortOpts{ - { - FixedIPs: []infrav1.FixedIP{ - { - Subnet: &infrav1.SubnetFilter{ - Name: "test-subnet", + spec: infrav1.OpenStackMachineSpec{ + Ports: []infrav1.PortOpts{ + { + FixedIPs: []infrav1.FixedIP{ + { + Subnet: &infrav1.SubnetFilter{ + Name: "test-subnet", + }, }, }, }, @@ -711,7 +624,7 @@ func TestService_normalizePorts(t *testing.T) { }, expectNetwork: func(m *mock.MockNetworkClientMockRecorder) { m.ListSubnet(subnets.ListOpts{Name: "test-subnet"}).Return([]subnets.Subnet{ - {ID: subnetID, NetworkID: networkID}, + {ID: subnetID1, NetworkID: networkID}, {ID: "8008494c-301e-4e5c-951b-a8ab568447fd", NetworkID: "5d48bfda-db28-42ee-8374-50e13d1fe5ea"}, }, nil) }, @@ -719,17 +632,19 @@ func TestService_normalizePorts(t *testing.T) { }, { name: "No network, first fixed IP subnet returns multiple matches: used ID from second fixed IP", - ports: []infrav1.PortOpts{ - { - FixedIPs: []infrav1.FixedIP{ - { - Subnet: &infrav1.SubnetFilter{ - Name: "test-subnet1", + spec: infrav1.OpenStackMachineSpec{ + Ports: []infrav1.PortOpts{ + { + FixedIPs: []infrav1.FixedIP{ + { + Subnet: &infrav1.SubnetFilter{ + Name: "test-subnet1", + }, }, - }, - { - Subnet: &infrav1.SubnetFilter{ - Name: "test-subnet2", + { + Subnet: &infrav1.SubnetFilter{ + Name: "test-subnet2", + }, }, }, }, @@ -737,43 +652,143 @@ func TestService_normalizePorts(t *testing.T) { }, expectNetwork: func(m *mock.MockNetworkClientMockRecorder) { m.ListSubnet(subnets.ListOpts{Name: "test-subnet1"}).Return([]subnets.Subnet{ - {ID: subnetID, NetworkID: networkID}, + {ID: subnetID1, NetworkID: networkID}, {ID: "8008494c-301e-4e5c-951b-a8ab568447fd", NetworkID: "5d48bfda-db28-42ee-8374-50e13d1fe5ea"}, }, nil) m.ListSubnet(subnets.ListOpts{Name: "test-subnet2"}).Return([]subnets.Subnet{ - {ID: subnetID, NetworkID: networkID}, + {ID: subnetID2, NetworkID: networkID}, + }, nil) + // Fetch the first subnet again, this time with network ID from the second subnet + m.ListSubnet(subnets.ListOpts{NetworkID: networkID, Name: "test-subnet1"}).Return([]subnets.Subnet{ + {ID: subnetID1, NetworkID: networkID}, }, nil) }, - want: []infrav1.PortOpts{ + want: []infrav1.ResolvedPortSpec{ { - Network: &infrav1.NetworkFilter{ - ID: networkID, - }, - FixedIPs: []infrav1.FixedIP{ + NetworkID: networkID, + FixedIPs: []infrav1.ResolvedFixedIP{ { - Subnet: &infrav1.SubnetFilter{ - Name: "test-subnet1", - }, + SubnetID: pointer.String(subnetID1), }, { - Subnet: &infrav1.SubnetFilter{ - ID: subnetID, - Name: "test-subnet2", - }, + SubnetID: pointer.String(subnetID2), }, }, - Trunk: pointer.Bool(false), + + // Defaults + Name: "test-instance-0", + Description: defaultDescription, + Tags: []string{"test-tag"}, + }, + }, + }, + { + name: "machine spec security groups added to defaults", + spec: infrav1.OpenStackMachineSpec{ + SecurityGroups: []infrav1.SecurityGroupFilter{ + {Name: "test-security-group"}, + }, + }, + expectNetwork: func(m *mock.MockNetworkClientMockRecorder) { + m.ListSecGroup(groups.ListOpts{Name: "test-security-group"}).Return([]groups.SecGroup{ + {ID: securityGroupID1}, + }, nil) + }, + want: []infrav1.ResolvedPortSpec{ + { + Name: "test-instance-0", + NetworkID: defaultNetworkID, + FixedIPs: []infrav1.ResolvedFixedIP{ + {SubnetID: pointer.String(defaultSubnetID)}, + }, + Description: defaultDescription, + Tags: []string{"test-tag"}, + SecurityGroups: []string{securityGroupID1}, + }, + }, + }, + { + name: "port security groups override machine spec security groups", + spec: infrav1.OpenStackMachineSpec{ + SecurityGroups: []infrav1.SecurityGroupFilter{ + {Name: "machine-security-group"}, + }, + Ports: []infrav1.PortOpts{ + {SecurityGroups: []infrav1.SecurityGroupFilter{{Name: "port-security-group"}}}, + }, + }, + expectNetwork: func(m *mock.MockNetworkClientMockRecorder) { + m.ListSecGroup(groups.ListOpts{Name: "machine-security-group"}).Return([]groups.SecGroup{ + {ID: securityGroupID1}, + }, nil) + m.ListSecGroup(groups.ListOpts{Name: "port-security-group"}).Return([]groups.SecGroup{ + {ID: securityGroupID2}, + }, nil) + }, + want: []infrav1.ResolvedPortSpec{ + { + Name: "test-instance-0", + NetworkID: defaultNetworkID, + FixedIPs: []infrav1.ResolvedFixedIP{ + {SubnetID: pointer.String(defaultSubnetID)}, + }, + Description: defaultDescription, + Tags: []string{"test-tag"}, + SecurityGroups: []string{securityGroupID2}, + }, + }, + }, + { + name: "managed security group added to port", + spec: infrav1.OpenStackMachineSpec{}, + managedSecurityGroup: pointer.String(securityGroupID1), + want: []infrav1.ResolvedPortSpec{ + { + Name: "test-instance-0", + NetworkID: defaultNetworkID, + FixedIPs: []infrav1.ResolvedFixedIP{ + {SubnetID: pointer.String(defaultSubnetID)}, + }, + Description: defaultDescription, + Tags: []string{"test-tag"}, + SecurityGroups: []string{securityGroupID1}, + }, + }, + }, + { + name: "managed security group and machine security groups added to port", + spec: infrav1.OpenStackMachineSpec{ + SecurityGroups: []infrav1.SecurityGroupFilter{{Name: "machine-security-group"}}, + }, + managedSecurityGroup: pointer.String(securityGroupID1), + expectNetwork: func(m *mock.MockNetworkClientMockRecorder) { + m.ListSecGroup(groups.ListOpts{Name: "machine-security-group"}).Return([]groups.SecGroup{ + {ID: securityGroupID2}, + }, nil) + }, + want: []infrav1.ResolvedPortSpec{ + { + Name: "test-instance-0", + NetworkID: defaultNetworkID, + FixedIPs: []infrav1.ResolvedFixedIP{ + {SubnetID: pointer.String(defaultSubnetID)}, + }, + Description: defaultDescription, + Tags: []string{"test-tag"}, + SecurityGroups: []string{securityGroupID2, securityGroupID1}, }, }, }, } - for _, tt := range tests { + for i := range tests { + tt := &tests[i] t.Run(tt.name, func(t *testing.T) { + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + g := NewWithT(t) log := testr.New(t) - mockCtrl := gomock.NewController(t) - defer mockCtrl.Finish() mockClient := mock.NewMockNetworkClient(mockCtrl) if tt.expectNetwork != nil { tt.expectNetwork(mockClient.EXPECT()) @@ -784,7 +799,19 @@ func TestService_normalizePorts(t *testing.T) { scope: scope.NewWithLogger(mockScopeFactory, log), } - got, err := s.normalizePorts(tt.ports, openStackCluster, tt.instanceTrunk) + defaultNetwork := &infrav1.NetworkStatusWithSubnets{ + NetworkStatus: infrav1.NetworkStatus{ + ID: defaultNetworkID, + }, + Subnets: []infrav1.Subnet{ + {ID: defaultSubnetID}, + }, + } + + clusterName := "test-cluster" + baseName := "test-instance" + baseTags := []string{"test-tag"} + got, err := s.ConstructPorts(&tt.spec, clusterName, baseName, defaultNetwork, tt.managedSecurityGroup, baseTags) if tt.wantErr { g.Expect(err).To(HaveOccurred()) return @@ -797,40 +824,52 @@ func TestService_normalizePorts(t *testing.T) { } func Test_getPortName(t *testing.T) { - type args struct { + tests := []struct { + name string instanceName string - opts *infrav1.PortOpts + spec *infrav1.PortOpts netIndex int - } - tests := []struct { - name string - args args - want string + want string }{ { - name: "with nil PortOpts", - args: args{"test-1-instance", nil, 2}, - want: "test-1-instance-2", + name: "with nil PortOpts", + instanceName: "test-1-instance", + netIndex: 2, + want: "test-1-instance-2", }, { - name: "with PortOpts name suffix", - args: args{"test-1-instance", &infrav1.PortOpts{NameSuffix: pointer.String("foo")}, 4}, - want: "test-1-instance-foo", + name: "with PortOpts name suffix", + instanceName: "test-1-instance", + spec: &infrav1.PortOpts{ + NameSuffix: pointer.String("foo"), + }, + netIndex: 4, + want: "test-1-instance-foo", }, { - name: "without PortOpts name suffix", - args: args{"test-1-instance", &infrav1.PortOpts{}, 4}, - want: "test-1-instance-4", + name: "without PortOpts name suffix", + instanceName: "test-1-instance", + spec: &infrav1.PortOpts{}, + netIndex: 4, + want: "test-1-instance-4", }, { - name: "with PortOpts name suffix", - args: args{"test-1-instance", &infrav1.PortOpts{NameSuffix: pointer.String("foo2"), Network: &infrav1.NetworkFilter{ID: "bar"}, DisablePortSecurity: pointer.Bool(true)}, 4}, - want: "test-1-instance-foo2", + name: "with PortOpts name suffix", + instanceName: "test-1-instance", + spec: &infrav1.PortOpts{ + NameSuffix: pointer.String("foo2"), + Network: &infrav1.NetworkFilter{ID: "bar"}, + ResolvedPortSpecFields: infrav1.ResolvedPortSpecFields{ + DisablePortSecurity: pointer.Bool(true), + }, + }, + netIndex: 4, + want: "test-1-instance-foo2", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if got := GetPortName(tt.args.instanceName, tt.args.opts, tt.args.netIndex); got != tt.want { + if got := getPortName(tt.instanceName, tt.spec, tt.netIndex); got != tt.want { t.Errorf("getPortName() = %v, want %v", got, tt.want) } }) @@ -848,7 +887,7 @@ func Test_AdoptPorts(t *testing.T) { tests := []struct { testName string - desiredPorts []infrav1.PortOpts + desiredPorts []infrav1.ResolvedPortSpec dependentResources infrav1.DependentMachineResources expect func(*mock.MockNetworkClientMockRecorder) want infrav1.DependentMachineResources @@ -859,8 +898,8 @@ func Test_AdoptPorts(t *testing.T) { }, { testName: "desired port already in status: no-op", - desiredPorts: []infrav1.PortOpts{ - {Network: &infrav1.NetworkFilter{ID: networkID1}}, + desiredPorts: []infrav1.ResolvedPortSpec{ + {NetworkID: networkID1}, }, dependentResources: infrav1.DependentMachineResources{ Ports: []infrav1.PortStatus{ @@ -879,8 +918,8 @@ func Test_AdoptPorts(t *testing.T) { }, { testName: "desired port not in status, exists: adopt", - desiredPorts: []infrav1.PortOpts{ - {Network: &infrav1.NetworkFilter{ID: networkID1}}, + desiredPorts: []infrav1.ResolvedPortSpec{ + {Name: "test-machine-0", NetworkID: networkID1}, }, expect: func(m *mock.MockNetworkClientMockRecorder) { m.ListPort(ports.ListOpts{Name: "test-machine-0", NetworkID: networkID1}). @@ -896,8 +935,8 @@ func Test_AdoptPorts(t *testing.T) { }, { testName: "desired port not in status, does not exist: ignore", - desiredPorts: []infrav1.PortOpts{ - {Network: &infrav1.NetworkFilter{ID: networkID1}}, + desiredPorts: []infrav1.ResolvedPortSpec{ + {Name: "test-machine-0", NetworkID: networkID1}, }, expect: func(m *mock.MockNetworkClientMockRecorder) { m.ListPort(ports.ListOpts{Name: "test-machine-0", NetworkID: networkID1}). @@ -907,9 +946,9 @@ func Test_AdoptPorts(t *testing.T) { }, { testName: "2 desired ports, first in status, second exists: adopt second", - desiredPorts: []infrav1.PortOpts{ - {Network: &infrav1.NetworkFilter{ID: networkID1}}, - {Network: &infrav1.NetworkFilter{ID: networkID2}}, + desiredPorts: []infrav1.ResolvedPortSpec{ + {Name: "test-machine-0", NetworkID: networkID1}, + {Name: "test-machine-1", NetworkID: networkID2}, }, dependentResources: infrav1.DependentMachineResources{ Ports: []infrav1.PortStatus{ @@ -931,10 +970,10 @@ func Test_AdoptPorts(t *testing.T) { }, { testName: "3 desired ports, first in status, second does not exist: ignore, do no look for third", - desiredPorts: []infrav1.PortOpts{ - {Network: &infrav1.NetworkFilter{ID: networkID1}}, - {Network: &infrav1.NetworkFilter{ID: networkID2}}, - {Network: &infrav1.NetworkFilter{ID: networkID3}}, + desiredPorts: []infrav1.ResolvedPortSpec{ + {Name: "test-machine-0", NetworkID: networkID1}, + {Name: "test-machine-1", NetworkID: networkID2}, + {Name: "test-machine-2", NetworkID: networkID3}, }, dependentResources: infrav1.DependentMachineResources{ Ports: []infrav1.PortStatus{ @@ -953,6 +992,30 @@ func Test_AdoptPorts(t *testing.T) { }, }, }, + { + testName: "3 desired ports with arbitrary names, first in status, second does not exist: ignore, do no look for third", + desiredPorts: []infrav1.ResolvedPortSpec{ + {Name: "test-machine-foo", NetworkID: networkID1}, + {Name: "test-machine-bar", NetworkID: networkID2}, + {Name: "test-machine-baz", NetworkID: networkID3}, + }, + dependentResources: infrav1.DependentMachineResources{ + Ports: []infrav1.PortStatus{ + { + ID: portID1, + }, + }, + }, + expect: func(m *mock.MockNetworkClientMockRecorder) { + m.ListPort(ports.ListOpts{Name: "test-machine-bar", NetworkID: networkID2}). + Return(nil, nil) + }, + want: infrav1.DependentMachineResources{ + Ports: []infrav1.PortStatus{ + {ID: portID1}, + }, + }, + }, } for i := range tests { tt := &tests[i] @@ -972,7 +1035,6 @@ func Test_AdoptPorts(t *testing.T) { } err := s.AdoptPorts(scope.NewWithLogger(mockScopeFactory, log), - "test-machine", tt.desiredPorts, &tt.dependentResources) if tt.wantErr { g.Expect(err).Error() diff --git a/pkg/cloud/services/networking/trunk.go b/pkg/cloud/services/networking/trunk.go index 6743478ffd..12ef3f4462 100644 --- a/pkg/cloud/services/networking/trunk.go +++ b/pkg/cloud/services/networking/trunk.go @@ -22,12 +22,12 @@ import ( "time" "github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/trunks" + "github.com/gophercloud/gophercloud/openstack/networking/v2/ports" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/wait" "sigs.k8s.io/cluster-api-provider-openstack/pkg/record" capoerrors "sigs.k8s.io/cluster-api-provider-openstack/pkg/utils/errors" - "sigs.k8s.io/cluster-api-provider-openstack/pkg/utils/names" ) const ( @@ -49,10 +49,10 @@ func (s *Service) GetTrunkSupport() (bool, error) { return false, nil } -func (s *Service) getOrCreateTrunk(eventObject runtime.Object, clusterName, trunkName, portID string) (*trunks.Trunk, error) { +func (s *Service) getOrCreateTrunkForPort(eventObject runtime.Object, port *ports.Port) (*trunks.Trunk, error) { trunkList, err := s.client.ListTrunk(trunks.ListOpts{ - Name: trunkName, - PortID: portID, + Name: port.Name, + PortID: port.ID, }) if err != nil { return nil, fmt.Errorf("searching for existing trunk for server: %v", err) @@ -63,9 +63,9 @@ func (s *Service) getOrCreateTrunk(eventObject runtime.Object, clusterName, trun } trunkCreateOpts := trunks.CreateOpts{ - Name: trunkName, - PortID: portID, - Description: names.GetDescription(clusterName), + Name: port.Name, + PortID: port.ID, + Description: port.Description, } trunk, err := s.client.CreateTrunk(trunkCreateOpts) diff --git a/pkg/cloud/services/networking/trunk_test.go b/pkg/cloud/services/networking/trunk_test.go index a75519d8a8..079f5e45f9 100644 --- a/pkg/cloud/services/networking/trunk_test.go +++ b/pkg/cloud/services/networking/trunk_test.go @@ -21,6 +21,7 @@ import ( "github.com/golang/mock/gomock" "github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/trunks" + "github.com/gophercloud/gophercloud/openstack/networking/v2/ports" . "github.com/onsi/gomega" infrav1 "sigs.k8s.io/cluster-api-provider-openstack/api/v1beta1" @@ -31,54 +32,63 @@ func Test_GetOrCreateTrunk(t *testing.T) { mockCtrl := gomock.NewController(t) defer mockCtrl.Finish() + const portID = "021e5dbe-a27b-4824-839e-239d5a027c7f" + tests := []struct { - name string - trunkName string - portID string - expect func(m *mock.MockNetworkClientMockRecorder) + name string + port *ports.Port + expect func(m *mock.MockNetworkClientMockRecorder) // Note the 'wanted' port isn't so important, since it will be whatever we tell ListPort or CreatePort to return. // Mostly in this test suite, we're checking that ListPort/CreatePort is called with the expected port opts. want *trunks.Trunk wantErr bool }{ { - "return trunk if found", - "trunk-1", - "port-1", - func(m *mock.MockNetworkClientMockRecorder) { + name: "return trunk if found", + port: &ports.Port{ + ID: portID, + Name: "trunk-1", + }, + expect: func(m *mock.MockNetworkClientMockRecorder) { m. ListTrunk(trunks.ListOpts{ Name: "trunk-1", - PortID: "port-1", + PortID: portID, }).Return([]trunks.Trunk{{ Name: "trunk-1", - ID: "port-1", + ID: portID, }}, nil) }, - &trunks.Trunk{Name: "trunk-1", ID: "port-1"}, - false, + want: &trunks.Trunk{ + Name: "trunk-1", + ID: portID, + }, + wantErr: false, }, { - "creates trunk if not found", - "trunk-1", - "port-1", - func(m *mock.MockNetworkClientMockRecorder) { + name: "creates trunk if not found", + port: &ports.Port{ + ID: portID, + Name: "trunk-1", + Description: "Created by cluster-api-provider-openstack cluster test-cluster", + }, + expect: func(m *mock.MockNetworkClientMockRecorder) { // No ports found m. ListTrunk(trunks.ListOpts{ Name: "trunk-1", - PortID: "port-1", + PortID: portID, }).Return([]trunks.Trunk{}, nil) m. CreateTrunk(trunks.CreateOpts{ Name: "trunk-1", - PortID: "port-1", + PortID: portID, Description: "Created by cluster-api-provider-openstack cluster test-cluster", }, ).Return(&trunks.Trunk{Name: "trunk-1", ID: "port-1"}, nil) }, - &trunks.Trunk{Name: "trunk-1", ID: "port-1"}, - false, + want: &trunks.Trunk{Name: "trunk-1", ID: "port-1"}, + wantErr: false, }, } @@ -91,12 +101,7 @@ func Test_GetOrCreateTrunk(t *testing.T) { s := Service{ client: mockClient, } - got, err := s.getOrCreateTrunk( - eventObject, - "test-cluster", - tt.trunkName, - tt.portID, - ) + got, err := s.getOrCreateTrunkForPort(eventObject, tt.port) if tt.wantErr { g.Expect(err).To(HaveOccurred()) } else { diff --git a/pkg/utils/names/names.go b/pkg/utils/names/names.go index 95c1bd455c..ee1238f48f 100644 --- a/pkg/utils/names/names.go +++ b/pkg/utils/names/names.go @@ -19,6 +19,8 @@ package names import ( "fmt" "strings" + + clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" ) const ( @@ -36,3 +38,7 @@ func GetFloatingAddressClaimName(openStackMachineName string) string { func GetOpenStackMachineNameFromClaimName(claimName string) string { return strings.TrimSuffix(claimName, fmt.Sprintf("-%s", FloatingAddressIPClaimNameSuffix)) } + +func ClusterName(cluster *clusterv1.Cluster) string { + return fmt.Sprintf("%s-%s", cluster.Namespace, cluster.Name) +} diff --git a/test/e2e/suites/e2e/e2e_test.go b/test/e2e/suites/e2e/e2e_test.go index e57c5bdd99..caf55b15fa 100644 --- a/test/e2e/suites/e2e/e2e_test.go +++ b/test/e2e/suites/e2e/e2e_test.go @@ -123,11 +123,10 @@ var _ = Describe("e2e tests [PR-Blocking]", func() { openStackCluster, err := shared.ClusterForSpec(ctx, e2eCtx, namespace) Expect(err).NotTo(HaveOccurred()) - // Tag: clusterName is declared on OpenStackCluster and gets propagated to all machines - // except the bastion host + // Tag: clusterName is declared on OpenStackCluster and gets propagated to all machines, including the bastion. allServers, err := shared.DumpOpenStackServers(e2eCtx, servers.ListOpts{Tags: clusterName}) Expect(err).NotTo(HaveOccurred()) - Expect(allServers).To(HaveLen(2)) + Expect(allServers).To(HaveLen(3)) // When listing servers with multiple tags, nova api requires a single, comma-separated string // with all the tags
-serverGroupID
+adminStateUp
+ +bool + +		 +(Optional) +

AdminStateUp specifies whether the port should be created in the up (true) or down (false) state. The default is up.

+
+macAddress
string 		(Optional) -

ServerGroupID is the ID of the server group the machine should be added to and is calculated based on ServerGroupFilter.

+

MACAddress specifies the MAC address of the port. If not specified, the MAC address will be generated.
-imageID
+allowedAddressPairs
+ + +[]AddressPair + + +		 +(Optional) +

AllowedAddressPairs is a list of address pairs which Neutron will +allow the port to send traffic from in addition to the port’s +addresses. If not specified, the MAC Address will be the MAC Address +of the port. Depending on the configuration of Neutron, it may be +supported to specify a CIDR instead of a specific IP address.

+
+hostID
string 		(Optional) -

ImageID is the ID of the image to use for the machine and is calculated based on ImageFilter.

+

HostID specifies the ID of the host where the port resides.
-ports
+vnicType
- -[]PortOpts +string + +		 +(Optional) +

VNICType specifies the type of vNIC which this port should be +attached to. This is used to determine which mechanism driver(s) to +be used to bind the port. The valid values are normal, macvtap, +direct, baremetal, direct-physical, virtio-forwarder, smart-nic and +remote-managed, although these values will not be validated in this +API to ensure compatibility with future neutron changes or custom +implementations. What type of vNIC is actually available depends on +deployments. If not specified, the Neutron default value is used.

+
+profile
+ + +BindingProfile 		(Optional) -

Ports is the fully resolved list of ports to create for the machine.

+

Profile is a set of key-value pairs that are used for binding +details. We intentionally don’t expose this as a map[string]string +because we only want to enable the users to set the values of the +keys that are known to work in OpenStack Networking API. See +https://docs.openstack.org/api-ref/network/v2/index.html?expanded=create-port-detail#create-port +To set profiles, your tenant needs permissions rule:create_port, and +rule:create_port:binding:profile

+
+disablePortSecurity
+ +bool + +		 +(Optional) +

DisablePortSecurity enables or disables the port security when set. +When not set, it takes the value of the corresponding field at the network level.

+
+propagateUplinkStatus
+ +bool + +		 +(Optional) +

PropageteUplinkStatus enables or disables the propagate uplink status on the port.

+
+valueSpecs
+ + +[]ValueSpec + + +		 +(Optional) +

Value specs are extra parameters to include in the API request with OpenStack. +This is an extension point for the API, so what they do and if they are supported, +depends on the specific OpenStack implementation.