diff --git a/api/v1alpha5/zz_generated.conversion.go b/api/v1alpha5/zz_generated.conversion.go index 44dbc571e2..628339325d 100644 --- a/api/v1alpha5/zz_generated.conversion.go +++ b/api/v1alpha5/zz_generated.conversion.go @@ -1351,10 +1351,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)) @@ -1370,26 +1368,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 } @@ -1404,14 +1388,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 { @@ -1436,29 +1416,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 e3d5251fae..097ed5e67e 100644 --- a/api/v1alpha6/zz_generated.conversion.go +++ b/api/v1alpha6/zz_generated.conversion.go @@ -1386,10 +1386,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)) @@ -1405,28 +1403,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 } @@ -1440,14 +1424,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 { @@ -1472,29 +1452,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 5f364226bb..a9acd45ad4 100644 --- a/api/v1alpha7/zz_generated.conversion.go +++ b/api/v1alpha7/zz_generated.conversion.go @@ -1586,10 +1586,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)) @@ -1602,29 +1600,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 } @@ -1638,14 +1622,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 { @@ -1660,29 +1640,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 9e36fa9dd8..6c986b220b 100644 --- a/api/v1beta1/openstackmachine_types.go +++ b/api/v1beta1/openstackmachine_types.go @@ -58,7 +58,10 @@ 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 4dccc85c7a..1cb345090e 100644 --- a/api/v1beta1/types.go +++ b/api/v1beta1/types.go @@ -164,21 +164,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 @@ -190,13 +182,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 @@ -204,6 +194,26 @@ type PortOpts struct { // +optional Trunk *bool `json:"trunk,omitempty"` + ResolvedPortSpecFields `json:",inline"` +} + +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"` @@ -238,12 +248,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. @@ -253,6 +257,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"` + + // 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 bool `json:"trunk,omitempty"` + + // NetworkID is the ID of the network the port will be created in. + NetworkID string `json:"networkID"` + + // 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 @@ -283,6 +320,19 @@ type FixedIP struct { IPAddress optional.String `json:"ipAddress,omitempty"` } +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 @@ -656,7 +706,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 abd378418e..c5a439de1e 100644 --- a/api/v1beta1/zz_generated.deepcopy.go +++ b/api/v1beta1/zz_generated.deepcopy.go @@ -1064,23 +1064,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 } @@ -1098,53 +1088,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. @@ -1177,7 +1131,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]) } @@ -1194,6 +1148,126 @@ 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.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 2e31f55442..3a7589be6a 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclusters.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclusters.yaml @@ -5528,7 +5528,11 @@ 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 @@ -6234,6 +6238,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 @@ -6281,88 +6287,15 @@ spec: 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 @@ -6375,78 +6308,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 @@ -6471,88 +6339,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: |- @@ -6595,6 +6397,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 5dc38e4098..1fe9e83855 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclustertemplates.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclustertemplates.yaml @@ -2960,7 +2960,11 @@ 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 b47a3fe347..e90f39ae23 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackmachines.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackmachines.yaml @@ -2312,7 +2312,11 @@ 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 @@ -2453,6 +2457,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 @@ -2500,88 +2506,15 @@ spec: 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 @@ -2593,78 +2526,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 @@ -2689,88 +2557,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: |- @@ -2812,6 +2614,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 975c907650..f6973af53b 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackmachinetemplates.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackmachinetemplates.yaml @@ -1989,7 +1989,11 @@ 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 e5b3f35854..4f0269fb48 100644 --- a/controllers/openstackcluster_controller.go +++ b/controllers/openstackcluster_controller.go @@ -54,6 +54,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 ( @@ -128,7 +129,11 @@ func (r *OpenStackClusterReconciler) Reconcile(ctx context.Context, req ctrl.Req if openStackCluster.Status.Bastion == nil { openStackCluster.Status.Bastion = &infrav1.BastionStatus{} } - changed, err := compute.ResolveReferencedMachineResources(scope, openStackCluster, &openStackCluster.Spec.Bastion.Instance, &openStackCluster.Status.Bastion.ReferencedResources) + clusterName := names.ClusterName(cluster) + changed, err := compute.ResolveReferencedMachineResources(scope, + &openStackCluster.Spec.Bastion.Instance, &openStackCluster.Status.Bastion.ReferencedResources, + clusterName, bastionName(clusterName), + openStackCluster, getBastionSecurityGroupID(openStackCluster)) if err != nil { return reconcile.Result{}, err } @@ -137,7 +142,7 @@ func (r *OpenStackClusterReconciler) Reconcile(ctx context.Context, req ctrl.Req return reconcile.Result{}, nil } - changed, err = compute.AdoptDependentMachineResources(scope, bastionName(cluster.Name), &openStackCluster.Status.Bastion.ReferencedResources, &openStackCluster.Status.Bastion.DependentResources) + changed, err = compute.AdoptDependentMachineResources(scope, &openStackCluster.Status.Bastion.ReferencedResources, &openStackCluster.Status.Bastion.DependentResources) if err != nil { return reconcile.Result{}, err } @@ -182,7 +187,7 @@ func (r *OpenStackClusterReconciler) reconcileDelete(ctx context.Context, scope return reconcile.Result{}, err } - clusterName := fmt.Sprintf("%s-%s", cluster.Namespace, cluster.Name) + clusterName := names.ClusterName(cluster) if openStackCluster.Spec.APIServerLoadBalancer.IsEnabled() { loadBalancerService, err := loadbalancer.NewService(scope) @@ -393,7 +398,7 @@ func reconcileBastion(scope *scope.WithLogger, cluster *clusterv1.Cluster, openS if err != nil { return reconcile.Result{}, err } - clusterName := fmt.Sprintf("%s-%s", cluster.Namespace, cluster.Name) + clusterName := names.ClusterName(cluster) bastionHash, err := compute.HashInstanceSpec(instanceSpec) if err != nil { return reconcile.Result{}, fmt.Errorf("failed computing bastion hash from instance spec: %w", err) @@ -404,7 +409,7 @@ func reconcileBastion(scope *scope.WithLogger, cluster *clusterv1.Cluster, openS } } - 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 ctrl.Result{}, fmt.Errorf("failed to get or create ports for bastion: %w", err) @@ -491,64 +496,47 @@ func reconcileBastion(scope *scope.WithLogger, cluster *clusterv1.Cluster, openS } 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) - - instanceSpec.Ports = openStackCluster.Spec.Bastion.Instance.Ports + referencedResources := &openStackCluster.Status.Bastion.ReferencedResources - 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 @@ -556,9 +544,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) } @@ -576,7 +562,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 5ab69b2d77..7f3b01b816 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(Equal(reconcile.Result{})) }) @@ -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 750900b267..1565aaf608 100644 --- a/controllers/openstackmachine_controller.go +++ b/controllers/openstackmachine_controller.go @@ -53,6 +53,7 @@ import ( "sigs.k8s.io/cluster-api-provider-openstack/pkg/cloud/services/loadbalancer" "sigs.k8s.io/cluster-api-provider-openstack/pkg/cloud/services/networking" "sigs.k8s.io/cluster-api-provider-openstack/pkg/scope" + "sigs.k8s.io/cluster-api-provider-openstack/pkg/utils/names" ) // OpenStackMachineReconciler reconciles a OpenStackMachine object. @@ -149,7 +150,10 @@ func (r *OpenStackMachineReconciler) Reconcile(ctx context.Context, req ctrl.Req scope := scope.NewWithLogger(clientScope, log) // Resolve and store referenced resources - changed, err := compute.ResolveReferencedMachineResources(scope, infraCluster, &openStackMachine.Spec, &openStackMachine.Status.ReferencedResources) + changed, err := compute.ResolveReferencedMachineResources(scope, + &openStackMachine.Spec, &openStackMachine.Status.ReferencedResources, + names.ClusterName(cluster), openStackMachine.Name, + infraCluster, getManagedSecurityGroup(infraCluster, machine)) if err != nil { return reconcile.Result{}, err } @@ -159,7 +163,7 @@ func (r *OpenStackMachineReconciler) Reconcile(ctx context.Context, req ctrl.Req } // Adopt any existing dependent resources - changed, err = compute.AdoptDependentMachineResources(scope, openStackMachine.Name, &openStackMachine.Status.ReferencedResources, &openStackMachine.Status.DependentResources) + changed, err = compute.AdoptDependentMachineResources(scope, &openStackMachine.Status.ReferencedResources, &openStackMachine.Status.DependentResources) if err != nil { return reconcile.Result{}, err } @@ -244,7 +248,7 @@ func (r *OpenStackMachineReconciler) SetupWithManager(ctx context.Context, mgr c func (r *OpenStackMachineReconciler) reconcileDelete(scope *scope.WithLogger, cluster *clusterv1.Cluster, openStackCluster *infrav1.OpenStackCluster, machine *clusterv1.Machine, openStackMachine *infrav1.OpenStackMachine) (ctrl.Result, error) { //nolint:unparam scope.Logger().Info("Reconciling Machine delete") - clusterName := fmt.Sprintf("%s-%s", cluster.ObjectMeta.Namespace, cluster.Name) + clusterName := names.ClusterName(cluster) computeService, err := compute.NewService(scope) if err != nil { @@ -367,7 +371,7 @@ func (r *OpenStackMachineReconciler) reconcileNormal(ctx context.Context, scope } scope.Logger().Info("Reconciling Machine") - clusterName := fmt.Sprintf("%s-%s", cluster.ObjectMeta.Namespace, cluster.Name) + clusterName := names.ClusterName(cluster) computeService, err := compute.NewService(scope) if err != nil { @@ -379,7 +383,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 } @@ -493,7 +497,7 @@ func (r *OpenStackMachineReconciler) reconcileNormal(ctx context.Context, scope return ctrl.Result{}, 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 @@ -501,9 +505,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) } @@ -572,69 +574,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 a03120ce5e..3a6df21017 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 ce660d5e0d..b2b2b50b9d 100644 --- a/docs/book/src/api/v1beta1/api.md +++ b/docs/book/src/api/v1beta1/api.md @@ -654,8 +654,10 @@ bool -

Machine tags -Requires Nova api 2.52 minimum!

+

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!

@@ -963,7 +965,7 @@ additional storage options.

(Appears on: -PortOpts) +ResolvedPortSpecFields)

@@ -1218,7 +1220,7 @@ DependentMachineResources

(Appears on: -PortOpts) +ResolvedPortSpecFields)

@@ -3101,8 +3103,10 @@ bool -

Machine tags -Requires Nova api 2.52 minimum!

+

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!

@@ -3454,8 +3458,10 @@ bool -

Machine tags -Requires Nova api 2.52 minimum!

+

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!

@@ -3579,8 +3585,7 @@ OpenStackMachineTemplateResource

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

@@ -3609,18 +3614,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 @@ -3633,26 +3626,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.

@@ -3685,20 +3666,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.

@@ -3717,118 +3693,161 @@ 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) +

+

+

+ + + + + + + +
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.

@@ -3840,26 +3859,112 @@ 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.

+
+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.

+
+networkID
+ +string + +
+

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

+
+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.

@@ -3871,40 +3976,136 @@ string + + + + + + + + + + + + + + + + + + + + + + + + @@ -4834,7 +5035,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 a9b1c49dc8..f6be8a3dfe 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) (bool, error) { +func AdoptDependentMachineResources(scope *scope.WithLogger, referencedResources *infrav1.ReferencedMachineResources, dependentResources *infrav1.DependentMachineResources) (bool, error) { networkingService, err := networking.NewService(scope) if err != nil { return false, 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 bc5b01def5..74dab8bfa5 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 8fad1bf36b..277730e5e2 100644 --- a/pkg/cloud/services/compute/referenced_resources.go +++ b/pkg/cloud/services/compute/referenced_resources.go @@ -17,6 +17,8 @@ limitations under the License. package compute import ( + "slices" + infrav1 "sigs.k8s.io/cluster-api-provider-openstack/api/v1beta1" "sigs.k8s.io/cluster-api-provider-openstack/pkg/cloud/services/networking" "sigs.k8s.io/cluster-api-provider-openstack/pkg/scope" @@ -28,7 +30,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) @@ -62,14 +64,9 @@ func ResolveReferencedMachineResources(scope *scope.WithLogger, openStackCluster } // Network resources are required in order to get ports options. - if len(resources.Ports) == 0 && openStackCluster.Status.Network != nil { - // 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) + if len(resources.Ports) == 0 { + defaultNetwork := openStackCluster.Status.Network + portsOpts, err := networkingService.ConstructPorts(spec, clusterName, baseName, defaultNetwork, managedSecurityGroup, InstanceTags(spec, openStackCluster)) if err != nil { return changed, err } @@ -79,3 +76,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 a62ce19e2f..377b0d5e61 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,8 +25,8 @@ 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" infrav1 "sigs.k8s.io/cluster-api-provider-openstack/api/v1beta1" "sigs.k8s.io/cluster-api-provider-openstack/pkg/clients/mock" @@ -33,154 +34,160 @@ import ( ) func Test_ResolveReferencedMachineResources(t *testing.T) { - constFalse := false - const serverGroupID1 = "ce96e584-7ebc-46d6-9e55-987d72e3806c" - const 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: 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: 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: 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: 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: "test-image"}, - expectComputeMock: func(m *mock.MockComputeClientMockRecorder) {}, + testName: "Image by Name not found", + spec: infrav1.OpenStackMachineSpec{ + Image: infrav1.ImageFilter{Name: "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: 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: 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 @@ -190,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 709be4b8f6..769a7174fa 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" @@ -58,123 +59,94 @@ func (s *Service) GetPortFromInstanceIP(instanceID string, ip string) ([]ports.P return s.client.ListPort(portOpts) } -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 portSpec.Trunk { + 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 } } @@ -182,32 +154,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 @@ -306,30 +252,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 } @@ -342,112 +282,155 @@ 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, - }, - }) - } - ports = []infrav1.PortOpts{port} + resolvedPort.Trunk = spec.Trunk + 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 port.Trunk { 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 + 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 } @@ -463,8 +446,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 } @@ -472,38 +455,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. @@ -520,7 +505,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) (changed bool, err error) { +func (s *Service) AdoptPorts(scope *scope.WithLogger, desiredPorts []infrav1.ResolvedPortSpec, dependentResources *infrav1.DependentMachineResources) (changed bool, err error) { changed = false // We can skip adoption if the ports are already in the status @@ -533,21 +518,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 changed, fmt.Errorf("searching for existing port %s in network %s: %v", portName, port.Network.ID, err) + return changed, 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 @@ -556,7 +540,7 @@ func (s *Service) AdoptPorts(scope *scope.WithLogger, baseName string, desiredPo return changed, nil } if len(ports) > 1 { - return changed, fmt.Errorf("found multiple ports with name %s", portName) + return changed, 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 c6bb99c95d..ba94e5168d 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,373 +41,282 @@ import ( ) func Test_CreatePort(t *testing.T) { - mockCtrl := gomock.NewController(t) - defer mockCtrl.Finish() - - // 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), + }, + { + IPAddress: pointer.String(ipAddress2), + }, { - Subnet: &infrav1.SubnetFilter{ - Name: "subnetFoo", + 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: 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{ + { + IPAddress: ipAddress2, + }, + }, + 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, - }, - false, + + // 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 + }) + }, + 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: true, + }, + 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"}, + }) }, - &ports.Port{ID: portID1, PropagateUplinkStatus: true}, - false, + want: &ports.Port{ID: portID, Name: "test-port"}, }, } @@ -412,19 +324,18 @@ func Test_CreatePort(t *testing.T) { 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) 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,166 +347,159 @@ func Test_CreatePort(t *testing.T) { } } -func TestService_normalizePorts(t *testing.T) { - mockCtrl := gomock.NewController(t) - defer mockCtrl.Finish() - +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{ + name: "No ports creates port on default network", + spec: infrav1.OpenStackMachineSpec{}, + want: []infrav1.ResolvedPortSpec{ { - Network: nil, - FixedIPs: nil, - }, - }, - want: []infrav1.PortOpts{ - { - Network: &infrav1.NetworkFilter{ - ID: defaultNetworkID, + Name: "test-instance-0", + Description: defaultDescription, + Tags: []string{"test-tag"}, + NetworkID: defaultNetworkID, + FixedIPs: []infrav1.ResolvedFixedIP{ + {SubnetID: pointer.String(defaultSubnetID)}, }, - FixedIPs: []infrav1.FixedIP{ - { - Subnet: &infrav1.SubnetFilter{ - ID: 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, }, - instanceTrunk: true, - want: []infrav1.PortOpts{ + expectNetwork: func(m *mock.MockNetworkClientMockRecorder) { + expectListExtensions(m) + }, + want: []infrav1.ResolvedPortSpec{ { - Network: &infrav1.NetworkFilter{ - ID: defaultNetworkID, + Name: "test-instance-custom", + Description: defaultDescription, + NetworkID: defaultNetworkID, + FixedIPs: []infrav1.ResolvedFixedIP{ + {SubnetID: pointer.String(defaultSubnetID)}, }, - FixedIPs: []infrav1.FixedIP{ - { - Subnet: &infrav1.SubnetFilter{ - ID: defaultSubnetID, - }, - }, - }, - Trunk: pointer.Bool(true), + Tags: []string{"test-tag"}, + Trunk: 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, + }, + expectNetwork: func(m *mock.MockNetworkClientMockRecorder) { + expectListExtensions(m) }, - want: []infrav1.PortOpts{ + 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)}, }, - Trunk: pointer.Bool(true), + Tags: []string{"test-tag"}, + Trunk: 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", + }, }, }, }, @@ -604,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", + }, }, }, }, @@ -661,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", + }, }, }, }, @@ -701,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", + }, }, }, }, @@ -714,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) }, @@ -722,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", + }, }, }, }, @@ -740,38 +652,140 @@ 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, tt := range tests { t.Run(tt.name, func(t *testing.T) { + tt := &tests[i] + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + g := NewWithT(t) log := testr.New(t) @@ -785,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 @@ -798,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) } }) 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 e97ccc41c5..12f9366afb 100644 --- a/pkg/utils/names/names.go +++ b/pkg/utils/names/names.go @@ -18,8 +18,14 @@ package names import ( "fmt" + + clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" ) func GetDescription(clusterName string) string { return fmt.Sprintf("Created by cluster-api-provider-openstack cluster %s", clusterName) } + +func ClusterName(cluster *clusterv1.Cluster) string { + return fmt.Sprintf("%s-%s", cluster.Namespace, cluster.Name) +}
-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.