From c4e5c2a99503874e160154e091cf712788213566 Mon Sep 17 00:00:00 2001 From: Matthew Booth Date: Tue, 19 Mar 2024 10:04:11 +0000 Subject: [PATCH] Remove security group rules from status --- api/v1alpha5/conversion.go | 43 --- api/v1alpha6/types_conversion.go | 47 +-- api/v1alpha7/openstackcluster_conversion.go | 4 - api/v1alpha7/types_conversion.go | 80 +---- api/v1beta1/types.go | 53 --- api/v1beta1/zz_generated.deepcopy.go | 63 +--- ...re.cluster.x-k8s.io_openstackclusters.yaml | 156 --------- docs/book/src/api/v1beta1/api.md | 147 -------- go.mod | 2 +- .../services/networking/securitygroups.go | 226 ++++++------ .../networking/securitygroups_test.go | 329 +++++++++++------- 11 files changed, 311 insertions(+), 839 deletions(-) diff --git a/api/v1alpha5/conversion.go b/api/v1alpha5/conversion.go index ed0bbe4571..e01b164813 100644 --- a/api/v1alpha5/conversion.go +++ b/api/v1alpha5/conversion.go @@ -687,55 +687,12 @@ func Convert_v1alpha5_Bastion_To_v1beta1_Bastion(in *Bastion, out *infrav1.Basti func Convert_v1beta1_SecurityGroupStatus_To_v1alpha5_SecurityGroup(in *infrav1.SecurityGroupStatus, out *SecurityGroup, s conversion.Scope) error { //nolint:revive out.ID = in.ID out.Name = in.Name - out.Rules = make([]SecurityGroupRule, len(in.Rules)) - for i, rule := range in.Rules { - out.Rules[i] = SecurityGroupRule{ - ID: rule.ID, - Direction: rule.Direction, - } - if rule.Description != nil { - out.Rules[i].Description = *rule.Description - } - if rule.EtherType != nil { - out.Rules[i].EtherType = *rule.EtherType - } - if rule.PortRangeMin != nil { - out.Rules[i].PortRangeMin = *rule.PortRangeMin - } - if rule.PortRangeMax != nil { - out.Rules[i].PortRangeMax = *rule.PortRangeMax - } - if rule.Protocol != nil { - out.Rules[i].Protocol = *rule.Protocol - } - if rule.RemoteGroupID != nil { - out.Rules[i].RemoteGroupID = *rule.RemoteGroupID - } - if rule.RemoteIPPrefix != nil { - out.Rules[i].RemoteIPPrefix = *rule.RemoteIPPrefix - } - } return nil } func Convert_v1alpha5_SecurityGroup_To_v1beta1_SecurityGroupStatus(in *SecurityGroup, out *infrav1.SecurityGroupStatus, s conversion.Scope) error { //nolint:revive out.ID = in.ID out.Name = in.Name - out.Rules = make([]infrav1.SecurityGroupRuleStatus, len(in.Rules)) - for i, rule := range in.Rules { - out.Rules[i] = infrav1.SecurityGroupRuleStatus{ - ID: rule.ID, - Description: pointer.String(rule.Description), - Direction: rule.Direction, - EtherType: pointer.String(rule.EtherType), - PortRangeMin: pointer.Int(rule.PortRangeMin), - PortRangeMax: pointer.Int(rule.PortRangeMax), - Protocol: pointer.String(rule.Protocol), - RemoteGroupID: pointer.String(rule.RemoteGroupID), - RemoteIPPrefix: pointer.String(rule.RemoteIPPrefix), - } - } - return nil } diff --git a/api/v1alpha6/types_conversion.go b/api/v1alpha6/types_conversion.go index c43f8a23ac..6ec354ecee 100644 --- a/api/v1alpha6/types_conversion.go +++ b/api/v1alpha6/types_conversion.go @@ -364,63 +364,18 @@ func restorev1alpha6SecurityGroup(previous *SecurityGroup, dst *SecurityGroup) { return } - for i, rule := range previous.Rules { - dst.Rules[i].SecurityGroupID = rule.SecurityGroupID - } + dst.Rules = previous.Rules } func Convert_v1beta1_SecurityGroupStatus_To_v1alpha6_SecurityGroup(in *infrav1.SecurityGroupStatus, out *SecurityGroup, _ apiconversion.Scope) error { out.ID = in.ID out.Name = in.Name - out.Rules = make([]SecurityGroupRule, len(in.Rules)) - for i, rule := range in.Rules { - out.Rules[i] = SecurityGroupRule{ - ID: rule.ID, - Direction: rule.Direction, - } - if rule.Description != nil { - out.Rules[i].Description = *rule.Description - } - if rule.EtherType != nil { - out.Rules[i].EtherType = *rule.EtherType - } - if rule.PortRangeMin != nil { - out.Rules[i].PortRangeMin = *rule.PortRangeMin - } - if rule.PortRangeMax != nil { - out.Rules[i].PortRangeMax = *rule.PortRangeMax - } - if rule.Protocol != nil { - out.Rules[i].Protocol = *rule.Protocol - } - if rule.RemoteGroupID != nil { - out.Rules[i].RemoteGroupID = *rule.RemoteGroupID - } - if rule.RemoteIPPrefix != nil { - out.Rules[i].RemoteIPPrefix = *rule.RemoteIPPrefix - } - } return nil } func Convert_v1alpha6_SecurityGroup_To_v1beta1_SecurityGroupStatus(in *SecurityGroup, out *infrav1.SecurityGroupStatus, _ apiconversion.Scope) error { out.ID = in.ID out.Name = in.Name - out.Rules = make([]infrav1.SecurityGroupRuleStatus, len(in.Rules)) - for i, rule := range in.Rules { - out.Rules[i] = infrav1.SecurityGroupRuleStatus{ - ID: rule.ID, - Description: pointer.String(rule.Description), - Direction: rule.Direction, - EtherType: pointer.String(rule.EtherType), - PortRangeMin: pointer.Int(rule.PortRangeMin), - PortRangeMax: pointer.Int(rule.PortRangeMax), - Protocol: pointer.String(rule.Protocol), - RemoteGroupID: pointer.String(rule.RemoteGroupID), - RemoteIPPrefix: pointer.String(rule.RemoteIPPrefix), - } - } - return nil } diff --git a/api/v1alpha7/openstackcluster_conversion.go b/api/v1alpha7/openstackcluster_conversion.go index cd31d29f7f..6f1f88e923 100644 --- a/api/v1alpha7/openstackcluster_conversion.go +++ b/api/v1alpha7/openstackcluster_conversion.go @@ -346,10 +346,6 @@ func restorev1alpha7ClusterStatus(previous *OpenStackClusterStatus, dst *OpenSta } func restorev1beta1ClusterStatus(previous *infrav1.OpenStackClusterStatus, dst *infrav1.OpenStackClusterStatus) { - restorev1beta1SecurityGroupStatus(previous.ControlPlaneSecurityGroup, dst.ControlPlaneSecurityGroup) - restorev1beta1SecurityGroupStatus(previous.WorkerSecurityGroup, dst.WorkerSecurityGroup) - restorev1beta1SecurityGroupStatus(previous.BastionSecurityGroup, dst.BastionSecurityGroup) - // ReferencedResources have no equivalent in v1alpha7 if previous.Bastion != nil { dst.Bastion.ReferencedResources = previous.Bastion.ReferencedResources diff --git a/api/v1alpha7/types_conversion.go b/api/v1alpha7/types_conversion.go index 7b966d1320..3c3d0629f2 100644 --- a/api/v1alpha7/types_conversion.go +++ b/api/v1alpha7/types_conversion.go @@ -18,7 +18,6 @@ package v1alpha7 import ( apiconversion "k8s.io/apimachinery/pkg/conversion" - "k8s.io/utils/pointer" infrav1 "sigs.k8s.io/cluster-api-provider-openstack/api/v1beta1" "sigs.k8s.io/cluster-api-provider-openstack/pkg/utils/optional" @@ -40,9 +39,7 @@ func restorev1alpha7SecurityGroup(previous *SecurityGroup, dst *SecurityGroup) { return } - for i, rule := range previous.Rules { - dst.Rules[i].SecurityGroupID = rule.SecurityGroupID - } + dst.Rules = previous.Rules } func Convert_v1alpha7_SecurityGroupFilter_To_v1beta1_SecurityGroupFilter(in *SecurityGroupFilter, out *infrav1.SecurityGroupFilter, s apiconversion.Scope) error { @@ -142,39 +139,6 @@ func Convert_v1beta1_RouterFilter_To_v1alpha7_RouterFilter(in *infrav1.RouterFil return nil } -func restorev1beta1SecurityGroupStatus(previous *infrav1.SecurityGroupStatus, dst *infrav1.SecurityGroupStatus) { - if previous == nil || dst == nil { - return - } - - for i := range dst.Rules { - dstRule := &dst.Rules[i] - - // Conversion from scalar to *scalar is lossy for zero values. We need to restore only nil values. - if dstRule.Description != nil && *dstRule.Description == "" { - dstRule.Description = previous.Rules[i].Description - } - if dstRule.EtherType != nil && *dstRule.EtherType == "" { - dstRule.EtherType = previous.Rules[i].EtherType - } - if dstRule.PortRangeMin != nil && *dstRule.PortRangeMin == 0 { - dstRule.PortRangeMin = previous.Rules[i].PortRangeMin - } - if dstRule.PortRangeMax != nil && *dstRule.PortRangeMax == 0 { - dstRule.PortRangeMax = previous.Rules[i].PortRangeMax - } - if dstRule.Protocol != nil && *dstRule.Protocol == "" { - dstRule.Protocol = previous.Rules[i].Protocol - } - if dstRule.RemoteGroupID != nil && *dstRule.RemoteGroupID == "" { - dstRule.RemoteGroupID = previous.Rules[i].RemoteGroupID - } - if dstRule.RemoteIPPrefix != nil && *dstRule.RemoteIPPrefix == "" { - dstRule.RemoteIPPrefix = previous.Rules[i].RemoteIPPrefix - } - } -} - /* PortOpts */ func restorev1alpha7Port(previous *PortOpts, dst *PortOpts) { @@ -300,20 +264,6 @@ func Convert_v1beta1_PortOpts_To_v1alpha7_PortOpts(in *infrav1.PortOpts, out *Po func Convert_v1alpha7_SecurityGroup_To_v1beta1_SecurityGroupStatus(in *SecurityGroup, out *infrav1.SecurityGroupStatus, _ apiconversion.Scope) error { out.ID = in.ID out.Name = in.Name - out.Rules = make([]infrav1.SecurityGroupRuleStatus, len(in.Rules)) - for i, rule := range in.Rules { - out.Rules[i] = infrav1.SecurityGroupRuleStatus{ - ID: rule.ID, - Description: pointer.String(rule.Description), - Direction: rule.Direction, - EtherType: pointer.String(rule.EtherType), - PortRangeMin: pointer.Int(rule.PortRangeMin), - PortRangeMax: pointer.Int(rule.PortRangeMax), - Protocol: pointer.String(rule.Protocol), - RemoteGroupID: pointer.String(rule.RemoteGroupID), - RemoteIPPrefix: pointer.String(rule.RemoteIPPrefix), - } - } return nil } @@ -321,34 +271,6 @@ func Convert_v1alpha7_SecurityGroup_To_v1beta1_SecurityGroupStatus(in *SecurityG func Convert_v1beta1_SecurityGroupStatus_To_v1alpha7_SecurityGroup(in *infrav1.SecurityGroupStatus, out *SecurityGroup, _ apiconversion.Scope) error { out.ID = in.ID out.Name = in.Name - out.Rules = make([]SecurityGroupRule, len(in.Rules)) - for i, rule := range in.Rules { - out.Rules[i] = SecurityGroupRule{ - ID: rule.ID, - Direction: rule.Direction, - } - if rule.Description != nil { - out.Rules[i].Description = *rule.Description - } - if rule.EtherType != nil { - out.Rules[i].EtherType = *rule.EtherType - } - if rule.PortRangeMin != nil { - out.Rules[i].PortRangeMin = *rule.PortRangeMin - } - if rule.PortRangeMax != nil { - out.Rules[i].PortRangeMax = *rule.PortRangeMax - } - if rule.Protocol != nil { - out.Rules[i].Protocol = *rule.Protocol - } - if rule.RemoteGroupID != nil { - out.Rules[i].RemoteGroupID = *rule.RemoteGroupID - } - if rule.RemoteIPPrefix != nil { - out.Rules[i].RemoteIPPrefix = *rule.RemoteIPPrefix - } - } return nil } diff --git a/api/v1beta1/types.go b/api/v1beta1/types.go index 94625b20db..b7950d294e 100644 --- a/api/v1beta1/types.go +++ b/api/v1beta1/types.go @@ -448,10 +448,6 @@ type SecurityGroupStatus struct { // id of the security group // +kubebuilder:validation:Required ID string `json:"id"` - - // list of security group rules - // +optional - Rules []SecurityGroupRuleStatus `json:"rules,omitempty"` } // SecurityGroupRuleSpec represent the basic information of the associated OpenStack @@ -514,55 +510,6 @@ type SecurityGroupRuleSpec struct { RemoteManagedGroups []ManagedSecurityGroupName `json:"remoteManagedGroups,omitempty"` } -type SecurityGroupRuleStatus struct { - // id of the security group rule - // +kubebuilder:validation:Required - ID string `json:"id"` - - // description of the security group rule. - // +optional - Description *string `json:"description,omitempty"` - - // direction in which the security group rule is applied. The only values - // allowed are "ingress" or "egress". For a compute instance, an ingress - // security group rule is applied to incoming (ingress) traffic for that - // instance. An egress rule is applied to traffic leaving the instance. - // +kubebuilder:validation:Required - // +kubebuilder:validation:enum=ingress;egress - Direction string `json:"direction"` - - // etherType must be IPv4 or IPv6, and addresses represented in CIDR must match the - // ingress or egress rules. - // +kubebuilder:validation:enum=IPv4;IPv6 - // +optional - EtherType *string `json:"etherType,omitempty"` - - // portRangeMin is a number in the range that is matched by the security group - // rule. If the protocol is TCP or UDP, this value must be less than or equal - // to the value of the portRangeMax attribute. - // +optional - PortRangeMin *int `json:"portRangeMin,omitempty"` - - // portRangeMax is a number in the range that is matched by the security group - // rule. The portRangeMin attribute constrains the portRangeMax attribute. - // +optional - PortRangeMax *int `json:"portRangeMax,omitempty"` - - // protocol is the protocol that is matched by the security group rule. - // +optional - Protocol *string `json:"protocol,omitempty"` - - // remoteGroupID is the remote group ID to be associated with this security group rule. - // You can specify either remoteGroupID or remoteIPPrefix or remoteManagedGroups. - // +optional - RemoteGroupID *string `json:"remoteGroupID,omitempty"` - - // remoteIPPrefix is the remote IP prefix to be associated with this security group rule. - // You can specify either remoteGroupID or remoteIPPrefix or remoteManagedGroups. - // +optional - RemoteIPPrefix *string `json:"remoteIPPrefix,omitempty"` -} - // +kubebuilder:validation:Enum=bastion;controlplane;worker type ManagedSecurityGroupName string diff --git a/api/v1beta1/zz_generated.deepcopy.go b/api/v1beta1/zz_generated.deepcopy.go index 73e1fb8368..9635d65ab9 100644 --- a/api/v1beta1/zz_generated.deepcopy.go +++ b/api/v1beta1/zz_generated.deepcopy.go @@ -652,17 +652,17 @@ func (in *OpenStackClusterStatus) DeepCopyInto(out *OpenStackClusterStatus) { if in.ControlPlaneSecurityGroup != nil { in, out := &in.ControlPlaneSecurityGroup, &out.ControlPlaneSecurityGroup *out = new(SecurityGroupStatus) - (*in).DeepCopyInto(*out) + **out = **in } if in.WorkerSecurityGroup != nil { in, out := &in.WorkerSecurityGroup, &out.WorkerSecurityGroup *out = new(SecurityGroupStatus) - (*in).DeepCopyInto(*out) + **out = **in } if in.BastionSecurityGroup != nil { in, out := &in.BastionSecurityGroup, &out.BastionSecurityGroup *out = new(SecurityGroupStatus) - (*in).DeepCopyInto(*out) + **out = **in } if in.Bastion != nil { in, out := &in.Bastion, &out.Bastion @@ -1336,66 +1336,9 @@ func (in *SecurityGroupRuleSpec) DeepCopy() *SecurityGroupRuleSpec { return out } -// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *SecurityGroupRuleStatus) DeepCopyInto(out *SecurityGroupRuleStatus) { - *out = *in - if in.Description != nil { - in, out := &in.Description, &out.Description - *out = new(string) - **out = **in - } - if in.EtherType != nil { - in, out := &in.EtherType, &out.EtherType - *out = new(string) - **out = **in - } - if in.PortRangeMin != nil { - in, out := &in.PortRangeMin, &out.PortRangeMin - *out = new(int) - **out = **in - } - if in.PortRangeMax != nil { - in, out := &in.PortRangeMax, &out.PortRangeMax - *out = new(int) - **out = **in - } - if in.Protocol != nil { - in, out := &in.Protocol, &out.Protocol - *out = new(string) - **out = **in - } - if in.RemoteGroupID != nil { - in, out := &in.RemoteGroupID, &out.RemoteGroupID - *out = new(string) - **out = **in - } - if in.RemoteIPPrefix != nil { - in, out := &in.RemoteIPPrefix, &out.RemoteIPPrefix - *out = new(string) - **out = **in - } -} - -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new SecurityGroupRuleStatus. -func (in *SecurityGroupRuleStatus) DeepCopy() *SecurityGroupRuleStatus { - if in == nil { - return nil - } - out := new(SecurityGroupRuleStatus) - in.DeepCopyInto(out) - return out -} - // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *SecurityGroupStatus) DeepCopyInto(out *SecurityGroupStatus) { *out = *in - if in.Rules != nil { - in, out := &in.Rules, &out.Rules - *out = make([]SecurityGroupRuleStatus, len(*in)) - for i := range *in { - (*in)[i].DeepCopyInto(&(*out)[i]) - } - } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new SecurityGroupStatus. diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclusters.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclusters.yaml index f2e491f4b3..30ae084647 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclusters.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclusters.yaml @@ -6651,58 +6651,6 @@ spec: name: description: name of the security group type: string - rules: - description: list of security group rules - items: - properties: - description: - description: description of the security group rule. - type: string - direction: - description: |- - direction in which the security group rule is applied. The only values - allowed are "ingress" or "egress". For a compute instance, an ingress - security group rule is applied to incoming (ingress) traffic for that - instance. An egress rule is applied to traffic leaving the instance. - type: string - etherType: - description: |- - etherType must be IPv4 or IPv6, and addresses represented in CIDR must match the - ingress or egress rules. - type: string - id: - description: id of the security group rule - type: string - portRangeMax: - description: |- - portRangeMax is a number in the range that is matched by the security group - rule. The portRangeMin attribute constrains the portRangeMax attribute. - type: integer - portRangeMin: - description: |- - portRangeMin is a number in the range that is matched by the security group - rule. If the protocol is TCP or UDP, this value must be less than or equal - to the value of the portRangeMax attribute. - type: integer - protocol: - description: protocol is the protocol that is matched by - the security group rule. - type: string - remoteGroupID: - description: |- - remoteGroupID is the remote group ID to be associated with this security group rule. - You can specify either remoteGroupID or remoteIPPrefix or remoteManagedGroups. - type: string - remoteIPPrefix: - description: |- - remoteIPPrefix is the remote IP prefix to be associated with this security group rule. - You can specify either remoteGroupID or remoteIPPrefix or remoteManagedGroups. - type: string - required: - - direction - - id - type: object - type: array required: - id - name @@ -6719,58 +6667,6 @@ spec: name: description: name of the security group type: string - rules: - description: list of security group rules - items: - properties: - description: - description: description of the security group rule. - type: string - direction: - description: |- - direction in which the security group rule is applied. The only values - allowed are "ingress" or "egress". For a compute instance, an ingress - security group rule is applied to incoming (ingress) traffic for that - instance. An egress rule is applied to traffic leaving the instance. - type: string - etherType: - description: |- - etherType must be IPv4 or IPv6, and addresses represented in CIDR must match the - ingress or egress rules. - type: string - id: - description: id of the security group rule - type: string - portRangeMax: - description: |- - portRangeMax is a number in the range that is matched by the security group - rule. The portRangeMin attribute constrains the portRangeMax attribute. - type: integer - portRangeMin: - description: |- - portRangeMin is a number in the range that is matched by the security group - rule. If the protocol is TCP or UDP, this value must be less than or equal - to the value of the portRangeMax attribute. - type: integer - protocol: - description: protocol is the protocol that is matched by - the security group rule. - type: string - remoteGroupID: - description: |- - remoteGroupID is the remote group ID to be associated with this security group rule. - You can specify either remoteGroupID or remoteIPPrefix or remoteManagedGroups. - type: string - remoteIPPrefix: - description: |- - remoteIPPrefix is the remote IP prefix to be associated with this security group rule. - You can specify either remoteGroupID or remoteIPPrefix or remoteManagedGroups. - type: string - required: - - direction - - id - type: object - type: array required: - id - name @@ -6926,58 +6822,6 @@ spec: name: description: name of the security group type: string - rules: - description: list of security group rules - items: - properties: - description: - description: description of the security group rule. - type: string - direction: - description: |- - direction in which the security group rule is applied. The only values - allowed are "ingress" or "egress". For a compute instance, an ingress - security group rule is applied to incoming (ingress) traffic for that - instance. An egress rule is applied to traffic leaving the instance. - type: string - etherType: - description: |- - etherType must be IPv4 or IPv6, and addresses represented in CIDR must match the - ingress or egress rules. - type: string - id: - description: id of the security group rule - type: string - portRangeMax: - description: |- - portRangeMax is a number in the range that is matched by the security group - rule. The portRangeMin attribute constrains the portRangeMax attribute. - type: integer - portRangeMin: - description: |- - portRangeMin is a number in the range that is matched by the security group - rule. If the protocol is TCP or UDP, this value must be less than or equal - to the value of the portRangeMax attribute. - type: integer - protocol: - description: protocol is the protocol that is matched by - the security group rule. - type: string - remoteGroupID: - description: |- - remoteGroupID is the remote group ID to be associated with this security group rule. - You can specify either remoteGroupID or remoteIPPrefix or remoteManagedGroups. - type: string - remoteIPPrefix: - description: |- - remoteIPPrefix is the remote IP prefix to be associated with this security group rule. - You can specify either remoteGroupID or remoteIPPrefix or remoteManagedGroups. - type: string - required: - - direction - - id - type: object - type: array required: - id - name diff --git a/docs/book/src/api/v1beta1/api.md b/docs/book/src/api/v1beta1/api.md index ae9159d5fb..2b12258bef 100644 --- a/docs/book/src/api/v1beta1/api.md +++ b/docs/book/src/api/v1beta1/api.md @@ -4365,139 +4365,6 @@ You can specify either remoteGroupID or remoteIPPrefix or remoteManagedGroups. -

SecurityGroupRuleStatus -

-

-(Appears on: -SecurityGroupStatus) -

-

-

- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
FieldDescription
-id
- -string - -		 -

id of the security group rule

-
-description
- -string - -		 -(Optional) -

description of the security group rule.

-
-direction
- -string - -		 -

direction in which the security group rule is applied. The only values -allowed are “ingress” or “egress”. For a compute instance, an ingress -security group rule is applied to incoming (ingress) traffic for that -instance. An egress rule is applied to traffic leaving the instance.

-
-etherType
- -string - -		 -(Optional) -

etherType must be IPv4 or IPv6, and addresses represented in CIDR must match the -ingress or egress rules.

-
-portRangeMin
- -int - -		 -(Optional) -

portRangeMin is a number in the range that is matched by the security group -rule. If the protocol is TCP or UDP, this value must be less than or equal -to the value of the portRangeMax attribute.

-
-portRangeMax
- -int - -		 -(Optional) -

portRangeMax is a number in the range that is matched by the security group -rule. The portRangeMin attribute constrains the portRangeMax attribute.

-
-protocol
- -string - -		 -(Optional) -

protocol is the protocol that is matched by the security group rule.

-
-remoteGroupID
- -string - -		 -(Optional) -

remoteGroupID is the remote group ID to be associated with this security group rule. -You can specify either remoteGroupID or remoteIPPrefix or remoteManagedGroups.

-
-remoteIPPrefix
- -string - -		 -(Optional) -

remoteIPPrefix is the remote IP prefix to be associated with this security group rule. -You can specify either remoteGroupID or remoteIPPrefix or remoteManagedGroups.

-

SecurityGroupStatus

@@ -4538,20 +4405,6 @@ string

id of the security group

- - -rules
- - -[]SecurityGroupRuleStatus - - - - -(Optional) -

list of security group rules

- -

ServerGroupFilter diff --git a/go.mod b/go.mod index 50ebe0a6a8..c2bfc1a442 100644 --- a/go.mod +++ b/go.mod @@ -8,6 +8,7 @@ require ( github.com/golang/mock v1.6.0 github.com/google/go-cmp v0.6.0 github.com/google/gofuzz v1.2.0 + github.com/google/uuid v1.3.1 github.com/gophercloud/gophercloud v1.7.0 github.com/gophercloud/utils v0.0.0-20231010081019-80377eca5d56 github.com/hashicorp/go-version v1.4.0 @@ -80,7 +81,6 @@ require ( github.com/google/go-querystring v1.1.0 // indirect github.com/google/pprof v0.0.0-20210720184732-4bb14d4b1be1 // indirect github.com/google/safetext v0.0.0-20220905092116-b49f7bc46da2 // indirect - github.com/google/uuid v1.3.1 // indirect github.com/grpc-ecosystem/go-grpc-prometheus v1.2.0 // indirect github.com/grpc-ecosystem/grpc-gateway/v2 v2.16.0 // indirect github.com/hashicorp/go-uuid v1.0.3 // indirect diff --git a/pkg/cloud/services/networking/securitygroups.go b/pkg/cloud/services/networking/securitygroups.go index 0b67e5fd92..76cc8e349e 100644 --- a/pkg/cloud/services/networking/securitygroups.go +++ b/pkg/cloud/services/networking/securitygroups.go @@ -18,6 +18,7 @@ package networking import ( "fmt" + "slices" "github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/attributestags" "github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/security/groups" @@ -45,52 +46,74 @@ func (s *Service) ReconcileSecurityGroups(openStackCluster *infrav1.OpenStackClu return nil } + bastionEnabled := openStackCluster.Spec.Bastion != nil && openStackCluster.Spec.Bastion.Enabled + secControlPlaneGroupName := getSecControlPlaneGroupName(clusterName) secWorkerGroupName := getSecWorkerGroupName(clusterName) - secGroupNames := map[string]string{ + suffixToNameMap := map[string]string{ controlPlaneSuffix: secControlPlaneGroupName, workerSuffix: secWorkerGroupName, } - if openStackCluster.Spec.Bastion != nil && openStackCluster.Spec.Bastion.Enabled { + if bastionEnabled { secBastionGroupName := getSecBastionGroupName(clusterName) - secGroupNames[bastionSuffix] = secBastionGroupName + suffixToNameMap[bastionSuffix] = secBastionGroupName } // create security groups first, because desired rules use group ids. - for _, v := range secGroupNames { - if err := s.createSecurityGroupIfNotExists(openStackCluster, v); err != nil { + observedSecGroupBySuffix := make(map[string]*groups.SecGroup) + for suffix, secGroupName := range suffixToNameMap { + group, err := s.getOrCreateSecurityGroup(openStackCluster, secGroupName) + if err != nil { return err } + observedSecGroupBySuffix[suffix] = group + + normaliseTags := func(tags []string) []string { + tags = slices.Clone(tags) + slices.Sort(tags) + return slices.Compact(tags) + } + + if !slices.Equal(normaliseTags(openStackCluster.Spec.Tags), normaliseTags(group.Tags)) { + _, err = s.client.ReplaceAllAttributesTags("security-groups", group.ID, attributestags.ReplaceAllOpts{ + Tags: openStackCluster.Spec.Tags, + }) + if err != nil { + return err + } + s.scope.Logger().V(6).Info("Updated tags for security group", "name", group.Name, "id", group.ID) + } } + // create desired security groups - desiredSecGroups, err := s.generateDesiredSecGroups(openStackCluster, secGroupNames) + desiredSecGroupsBySuffix, err := s.generateDesiredSecGroups(openStackCluster, suffixToNameMap, observedSecGroupBySuffix) if err != nil { return err } - observedSecGroups := make(map[string]*infrav1.SecurityGroupStatus) - for k, desiredSecGroup := range desiredSecGroups { - var err error - observedSecGroups[k], err = s.getSecurityGroupByName(desiredSecGroup.Name) + for suffix := range desiredSecGroupsBySuffix { + desiredSecGroup := desiredSecGroupsBySuffix[suffix] + observedSecGroup, ok := observedSecGroupBySuffix[suffix] + if !ok { + // This should never happen + return fmt.Errorf("unable to reconcile security groups: security group %s not found", suffix) + } + err := s.reconcileGroupRules(&desiredSecGroup, observedSecGroup) if err != nil { return err } - - if observedSecGroups[k].ID != "" { - observedSecGroup, err := s.reconcileGroupRules(desiredSecGroup, *observedSecGroups[k]) - if err != nil { - return err - } - observedSecGroups[k] = &observedSecGroup - continue - } + continue } - openStackCluster.Status.ControlPlaneSecurityGroup = observedSecGroups[controlPlaneSuffix] - openStackCluster.Status.WorkerSecurityGroup = observedSecGroups[workerSuffix] - openStackCluster.Status.BastionSecurityGroup = observedSecGroups[bastionSuffix] + openStackCluster.Status.ControlPlaneSecurityGroup = convertOSSecGroupToConfigSecGroup(observedSecGroupBySuffix[controlPlaneSuffix]) + openStackCluster.Status.WorkerSecurityGroup = convertOSSecGroupToConfigSecGroup(observedSecGroupBySuffix[workerSuffix]) + if bastionEnabled { + openStackCluster.Status.BastionSecurityGroup = convertOSSecGroupToConfigSecGroup(observedSecGroupBySuffix[bastionSuffix]) + } else { + openStackCluster.Status.BastionSecurityGroup = nil + } return nil } @@ -111,24 +134,22 @@ type resolvedSecurityGroupRuleSpec struct { RemoteIPPrefix string `json:"remoteIPPrefix,omitempty"` } -func (r resolvedSecurityGroupRuleSpec) Matches(other infrav1.SecurityGroupRuleStatus) bool { - return r.Description == *other.Description && +func (r resolvedSecurityGroupRuleSpec) Matches(other rules.SecGroupRule) bool { + return r.Description == other.Description && r.Direction == other.Direction && - r.EtherType == *other.EtherType && - r.PortRangeMin == *other.PortRangeMin && - r.PortRangeMax == *other.PortRangeMax && - r.Protocol == *other.Protocol && - r.RemoteGroupID == *other.RemoteGroupID && - r.RemoteIPPrefix == *other.RemoteIPPrefix + r.EtherType == other.EtherType && + r.PortRangeMin == other.PortRangeMin && + r.PortRangeMax == other.PortRangeMax && + r.Protocol == other.Protocol && + r.RemoteGroupID == other.RemoteGroupID && + r.RemoteIPPrefix == other.RemoteIPPrefix } -func (s *Service) generateDesiredSecGroups(openStackCluster *infrav1.OpenStackCluster, secGroupNames map[string]string) (map[string]securityGroupSpec, error) { +func (s *Service) generateDesiredSecGroups(openStackCluster *infrav1.OpenStackCluster, suffixToNameMap map[string]string, observedSecGroupsBySuffix map[string]*groups.SecGroup) (map[string]securityGroupSpec, error) { if openStackCluster.Spec.ManagedSecurityGroups == nil { return nil, nil } - desiredSecGroups := make(map[string]securityGroupSpec) - var secControlPlaneGroupID string var secWorkerGroupID string var secBastionGroupID string @@ -139,12 +160,13 @@ func (s *Service) generateDesiredSecGroups(openStackCluster *infrav1.OpenStackCl // For now, we only reference the control plane and worker security groups. remoteManagedGroups := make(map[string]string) - for i, v := range secGroupNames { - secGroup, err := s.getSecurityGroupByName(v) - if err != nil { - return desiredSecGroups, err + for suffix := range suffixToNameMap { + secGroup, ok := observedSecGroupsBySuffix[suffix] + if !ok { + // This should never happen, as we should have created the security group earlier in this reconcile if it does not exist. + return nil, fmt.Errorf("unable to generate desired security group rules: security group for %s not found", suffix) } - switch i { + switch suffix { case controlPlaneSuffix: secControlPlaneGroupID = secGroup.ID remoteManagedGroups[controlPlaneSuffix] = secControlPlaneGroupID @@ -182,17 +204,19 @@ func (s *Service) generateDesiredSecGroups(openStackCluster *infrav1.OpenStackCl // Instead, we append the rules for allNodes to the control plane and worker security groups. allNodesRules, err := getAllNodesRules(remoteManagedGroups, openStackCluster.Spec.ManagedSecurityGroups.AllNodesSecurityGroupRules) if err != nil { - return desiredSecGroups, err + return nil, err } controlPlaneRules = append(controlPlaneRules, allNodesRules...) workerRules = append(workerRules, allNodesRules...) + desiredSecGroupsBySuffix := make(map[string]securityGroupSpec) + if openStackCluster.Spec.Bastion != nil && openStackCluster.Spec.Bastion.Enabled { controlPlaneRules = append(controlPlaneRules, getSGControlPlaneSSH(secBastionGroupID)...) workerRules = append(workerRules, getSGWorkerSSH(secBastionGroupID)...) - desiredSecGroups[bastionSuffix] = securityGroupSpec{ - Name: secGroupNames[bastionSuffix], + desiredSecGroupsBySuffix[bastionSuffix] = securityGroupSpec{ + Name: suffixToNameMap[bastionSuffix], Rules: append( []resolvedSecurityGroupRuleSpec{ { @@ -209,16 +233,16 @@ func (s *Service) generateDesiredSecGroups(openStackCluster *infrav1.OpenStackCl } } - desiredSecGroups[controlPlaneSuffix] = securityGroupSpec{ - Name: secGroupNames[controlPlaneSuffix], + desiredSecGroupsBySuffix[controlPlaneSuffix] = securityGroupSpec{ + Name: suffixToNameMap[controlPlaneSuffix], Rules: controlPlaneRules, } - desiredSecGroups[workerSuffix] = securityGroupSpec{ - Name: secGroupNames[workerSuffix], + desiredSecGroupsBySuffix[workerSuffix] = securityGroupSpec{ + Name: suffixToNameMap[workerSuffix], Rules: workerRules, } - return desiredSecGroups, nil + return desiredSecGroupsBySuffix, nil } // getAllNodesRules returns the rules for the allNodes security group that should be created. @@ -345,7 +369,7 @@ func (s *Service) deleteSecurityGroup(openStackCluster *infrav1.OpenStackCluster if err != nil { return err } - if group.ID == "" { + if group == nil { // nothing to do return nil } @@ -361,7 +385,7 @@ func (s *Service) deleteSecurityGroup(openStackCluster *infrav1.OpenStackCluster // reconcileGroupRules reconciles an already existing observed group by deleting rules not needed anymore and // creating rules that are missing. -func (s *Service) reconcileGroupRules(desired securityGroupSpec, observed infrav1.SecurityGroupStatus) (infrav1.SecurityGroupStatus, error) { +func (s *Service) reconcileGroupRules(desired *securityGroupSpec, observed *groups.SecGroup) error { var rulesToDelete []string // fills rulesToDelete by calculating observed - desired for _, observedRule := range observed.Rules { @@ -382,7 +406,6 @@ func (s *Service) reconcileGroupRules(desired securityGroupSpec, observed infrav } rulesToCreate := []resolvedSecurityGroupRuleSpec{} - reconciledRules := make([]infrav1.SecurityGroupRuleStatus, 0, len(desired.Rules)) // fills rulesToCreate by calculating desired - observed // also adds rules which are in observed and desired to reconcileGroupRules. for _, desiredRule := range desired.Rules { @@ -394,7 +417,6 @@ func (s *Service) reconcileGroupRules(desired securityGroupSpec, observed infrav for _, observedRule := range observed.Rules { if r.Matches(observedRule) { // add already existing rules to reconciledRules because we won't touch them anymore - reconciledRules = append(reconciledRules, observedRule) createRule = false break } @@ -409,7 +431,7 @@ func (s *Service) reconcileGroupRules(desired securityGroupSpec, observed infrav s.scope.Logger().V(6).Info("Deleting rule", "ID", rule, "name", observed.Name) err := s.client.DeleteSecGroupRule(rule) if err != nil { - return infrav1.SecurityGroupStatus{}, err + return err } } @@ -419,61 +441,44 @@ func (s *Service) reconcileGroupRules(desired securityGroupSpec, observed infrav if r.RemoteGroupID == remoteGroupIDSelf { r.RemoteGroupID = observed.ID } - newRule, err := s.createRule(observed.ID, r) + err := s.createRule(observed.ID, r) if err != nil { - return infrav1.SecurityGroupStatus{}, err + return err } - reconciledRules = append(reconciledRules, newRule) } - observed.Rules = reconciledRules - if len(reconciledRules) == 0 { - return infrav1.SecurityGroupStatus{}, nil - } - - return observed, nil + return nil } -func (s *Service) createSecurityGroupIfNotExists(openStackCluster *infrav1.OpenStackCluster, groupName string) error { +func (s *Service) getOrCreateSecurityGroup(openStackCluster *infrav1.OpenStackCluster, groupName string) (*groups.SecGroup, error) { secGroup, err := s.getSecurityGroupByName(groupName) if err != nil { - return err + return nil, err + } + if secGroup != nil { + s.scope.Logger().V(6).Info("Reusing existing SecurityGroup", "name", groupName, "id", secGroup.ID) + return secGroup, nil } - if secGroup == nil || secGroup.ID == "" { - s.scope.Logger().V(6).Info("Group doesn't exist, creating it", "name", groupName) - - createOpts := groups.CreateOpts{ - Name: groupName, - Description: "Cluster API managed group", - } - s.scope.Logger().V(6).Info("Creating group", "name", groupName) - - group, err := s.client.CreateSecGroup(createOpts) - if err != nil { - record.Warnf(openStackCluster, "FailedCreateSecurityGroup", "Failed to create security group %s: %v", groupName, err) - return err - } - if len(openStackCluster.Spec.Tags) > 0 { - _, err = s.client.ReplaceAllAttributesTags("security-groups", group.ID, attributestags.ReplaceAllOpts{ - Tags: openStackCluster.Spec.Tags, - }) - if err != nil { - return err - } - } + s.scope.Logger().V(6).Info("Group doesn't exist, creating it", "name", groupName) - record.Eventf(openStackCluster, "SuccessfulCreateSecurityGroup", "Created security group %s with id %s", groupName, group.ID) - return nil + createOpts := groups.CreateOpts{ + Name: groupName, + Description: "Cluster API managed group", } + s.scope.Logger().V(6).Info("Creating group", "name", groupName) - sInfo := fmt.Sprintf("Reuse Existing SecurityGroup %s with %s", groupName, secGroup.ID) - s.scope.Logger().V(6).Info(sInfo) + group, err := s.client.CreateSecGroup(createOpts) + if err != nil { + record.Warnf(openStackCluster, "FailedCreateSecurityGroup", "Failed to create security group %s: %v", groupName, err) + return nil, err + } - return nil + record.Eventf(openStackCluster, "SuccessfulCreateSecurityGroup", "Created security group %s with id %s", groupName, group.ID) + return group, nil } -func (s *Service) getSecurityGroupByName(name string) (*infrav1.SecurityGroupStatus, error) { +func (s *Service) getSecurityGroupByName(name string) (*groups.SecGroup, error) { opts := groups.ListOpts{ Name: name, } @@ -481,20 +486,20 @@ func (s *Service) getSecurityGroupByName(name string) (*infrav1.SecurityGroupSta s.scope.Logger().V(6).Info("Attempting to fetch security group with", "name", name) allGroups, err := s.client.ListSecGroup(opts) if err != nil { - return &infrav1.SecurityGroupStatus{}, err + return nil, err } switch len(allGroups) { case 0: - return &infrav1.SecurityGroupStatus{}, nil + return nil, nil case 1: - return convertOSSecGroupToConfigSecGroup(allGroups[0]), nil + return &allGroups[0], nil } - return &infrav1.SecurityGroupStatus{}, fmt.Errorf("more than one security group found named: %s", name) + return nil, fmt.Errorf("more than one security group found named: %s", name) } -func (s *Service) createRule(securityGroupID string, r resolvedSecurityGroupRuleSpec) (infrav1.SecurityGroupRuleStatus, error) { +func (s *Service) createRule(securityGroupID string, r resolvedSecurityGroupRuleSpec) error { dir := rules.RuleDirection(r.Direction) proto := rules.RuleProtocol(r.Protocol) etherType := rules.RuleEtherType(r.EtherType) @@ -511,11 +516,11 @@ func (s *Service) createRule(securityGroupID string, r resolvedSecurityGroupRule SecGroupID: securityGroupID, } s.scope.Logger().V(6).Info("Creating rule", "description", r.Description, "direction", dir, "portRangeMin", r.PortRangeMin, "portRangeMax", r.PortRangeMax, "proto", proto, "etherType", etherType, "remoteGroupID", r.RemoteGroupID, "remoteIPPrefix", r.RemoteIPPrefix, "securityGroupID", securityGroupID) - rule, err := s.client.CreateSecGroupRule(createOpts) + _, err := s.client.CreateSecGroupRule(createOpts) if err != nil { - return infrav1.SecurityGroupRuleStatus{}, err + return err } - return convertOSSecGroupRuleToConfigSecGroupRule(*rule), nil + return nil } func getSecControlPlaneGroupName(clusterName string) string { @@ -530,29 +535,10 @@ func getSecBastionGroupName(clusterName string) string { return fmt.Sprintf("%s-cluster-%s-secgroup-%s", secGroupPrefix, clusterName, bastionSuffix) } -func convertOSSecGroupToConfigSecGroup(osSecGroup groups.SecGroup) *infrav1.SecurityGroupStatus { - securityGroupRules := make([]infrav1.SecurityGroupRuleStatus, len(osSecGroup.Rules)) - for i, rule := range osSecGroup.Rules { - securityGroupRules[i] = convertOSSecGroupRuleToConfigSecGroupRule(rule) - } +func convertOSSecGroupToConfigSecGroup(osSecGroup *groups.SecGroup) *infrav1.SecurityGroupStatus { return &infrav1.SecurityGroupStatus{ - ID: osSecGroup.ID, - Name: osSecGroup.Name, - Rules: securityGroupRules, - } -} - -func convertOSSecGroupRuleToConfigSecGroupRule(osSecGroupRule rules.SecGroupRule) infrav1.SecurityGroupRuleStatus { - return infrav1.SecurityGroupRuleStatus{ - ID: osSecGroupRule.ID, - Direction: osSecGroupRule.Direction, - Description: &osSecGroupRule.Description, - EtherType: &osSecGroupRule.EtherType, - PortRangeMin: &osSecGroupRule.PortRangeMin, - PortRangeMax: &osSecGroupRule.PortRangeMax, - Protocol: &osSecGroupRule.Protocol, - RemoteGroupID: &osSecGroupRule.RemoteGroupID, - RemoteIPPrefix: &osSecGroupRule.RemoteIPPrefix, + ID: osSecGroup.ID, + Name: osSecGroup.Name, } } diff --git a/pkg/cloud/services/networking/securitygroups_test.go b/pkg/cloud/services/networking/securitygroups_test.go index b88a81cd43..04d4ff870d 100644 --- a/pkg/cloud/services/networking/securitygroups_test.go +++ b/pkg/cloud/services/networking/securitygroups_test.go @@ -19,8 +19,11 @@ import ( "reflect" "testing" + "github.com/go-logr/logr" "github.com/go-logr/logr/testr" "github.com/golang/mock/gomock" + "github.com/google/go-cmp/cmp" + "github.com/google/uuid" "github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/security/groups" "github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/security/rules" . "github.com/onsi/gomega" @@ -224,18 +227,25 @@ func TestGetAllNodesRules(t *testing.T) { } func TestGenerateDesiredSecGroups(t *testing.T) { - mockCtrl := gomock.NewController(t) - defer mockCtrl.Finish() - secGroupNames := map[string]string{ "controlplane": "k8s-cluster-mycluster-secgroup-controlplane", "worker": "k8s-cluster-mycluster-secgroup-worker", } + observedSecGroupsBySuffix := map[string]*groups.SecGroup{ + "controlplane": { + ID: "0", + Name: "k8s-cluster-mycluster-secgroup-controlplane", + }, + "worker": { + ID: "1", + Name: "k8s-cluster-mycluster-secgroup-worker", + }, + } + tests := []struct { name string openStackCluster *infrav1.OpenStackCluster - mockExpect func(m *mock.MockNetworkClientMockRecorder) // We could also test the exact rules that are returned, but that'll be a lot data to write out. // For now we just make sure that the number of rules is correct. expectedNumberSecurityGroupRules int @@ -244,7 +254,6 @@ func TestGenerateDesiredSecGroups(t *testing.T) { { name: "Valid openStackCluster with unmanaged securityGroups", openStackCluster: &infrav1.OpenStackCluster{}, - mockExpect: func(m *mock.MockNetworkClientMockRecorder) {}, expectedNumberSecurityGroupRules: 0, wantErr: false, }, @@ -255,20 +264,6 @@ func TestGenerateDesiredSecGroups(t *testing.T) { ManagedSecurityGroups: &infrav1.ManagedSecurityGroups{}, }, }, - mockExpect: func(m *mock.MockNetworkClientMockRecorder) { - m.ListSecGroup(groups.ListOpts{Name: "k8s-cluster-mycluster-secgroup-controlplane"}).Return([]groups.SecGroup{ - { - ID: "0", - Name: "k8s-cluster-mycluster-secgroup-controlplane", - }, - }, nil) - m.ListSecGroup(groups.ListOpts{Name: "k8s-cluster-mycluster-secgroup-worker"}).Return([]groups.SecGroup{ - { - ID: "1", - Name: "k8s-cluster-mycluster-secgroup-worker", - }, - }, nil) - }, expectedNumberSecurityGroupRules: 12, wantErr: false, }, @@ -288,20 +283,6 @@ func TestGenerateDesiredSecGroups(t *testing.T) { }, }, }, - mockExpect: func(m *mock.MockNetworkClientMockRecorder) { - m.ListSecGroup(groups.ListOpts{Name: "k8s-cluster-mycluster-secgroup-controlplane"}).Return([]groups.SecGroup{ - { - ID: "0", - Name: "k8s-cluster-mycluster-secgroup-controlplane", - }, - }, nil) - m.ListSecGroup(groups.ListOpts{Name: "k8s-cluster-mycluster-secgroup-worker"}).Return([]groups.SecGroup{ - { - ID: "1", - Name: "k8s-cluster-mycluster-secgroup-worker", - }, - }, nil) - }, expectedNumberSecurityGroupRules: 16, wantErr: false, }, @@ -321,26 +302,15 @@ func TestGenerateDesiredSecGroups(t *testing.T) { }, }, }, - mockExpect: func(m *mock.MockNetworkClientMockRecorder) { - m.ListSecGroup(groups.ListOpts{Name: "k8s-cluster-mycluster-secgroup-controlplane"}).Return([]groups.SecGroup{ - { - ID: "0", - Name: "k8s-cluster-mycluster-secgroup-controlplane", - }, - }, nil) - m.ListSecGroup(groups.ListOpts{Name: "k8s-cluster-mycluster-secgroup-worker"}).Return([]groups.SecGroup{ - { - ID: "1", - Name: "k8s-cluster-mycluster-secgroup-worker", - }, - }, nil) - }, wantErr: true, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + g := NewWithT(t) log := testr.New(t) mockScopeFactory := scope.NewMockScopeFactory(mockCtrl, "") @@ -349,9 +319,8 @@ func TestGenerateDesiredSecGroups(t *testing.T) { if err != nil { t.Fatalf("Failed to create service: %v", err) } - tt.mockExpect(mockScopeFactory.NetworkClient.EXPECT()) - gotSecurityGroups, err := s.generateDesiredSecGroups(tt.openStackCluster, secGroupNames) + gotSecurityGroups, err := s.generateDesiredSecGroups(tt.openStackCluster, secGroupNames, observedSecGroupsBySuffix) if tt.wantErr { g.Expect(err).To(HaveOccurred()) } else { @@ -367,27 +336,35 @@ func TestGenerateDesiredSecGroups(t *testing.T) { } func TestReconcileGroupRules(t *testing.T) { - mockCtrl := gomock.NewController(t) - defer mockCtrl.Finish() + const ( + sgID = "6260e813-af79-4592-8d1a-0f42dd26cc42" + sgRuleID = "52a532c4-2b44-4582-ba87-b64e62e19b1a" + sgLegacyRuleID = "a057dcc4-1535-469d-9d28-923cad9d4c56" + sgName = "k8s-cluster-mycluster-secgroup-controlplane" + ) tests := []struct { - name string - desiredSGSpecs securityGroupSpec - observedSGStatus infrav1.SecurityGroupStatus - mockExpect func(m *mock.MockNetworkClientMockRecorder) - wantSGStatus infrav1.SecurityGroupStatus + name string + desiredSGSpec securityGroupSpec + observedSG groups.SecGroup + mockExpect func(m *mock.MockNetworkClientMockRecorder) + wantSGStatus infrav1.SecurityGroupStatus }{ { - name: "Empty desiredSGSpecs and observedSGStatus", - desiredSGSpecs: securityGroupSpec{}, - observedSGStatus: infrav1.SecurityGroupStatus{}, - mockExpect: func(m *mock.MockNetworkClientMockRecorder) {}, - wantSGStatus: infrav1.SecurityGroupStatus{}, + name: "Empty desiredSGSpec and observedSG", + desiredSGSpec: securityGroupSpec{}, + observedSG: groups.SecGroup{ + ID: sgID, + Name: sgName, + Rules: []rules.SecGroupRule{}, + }, + mockExpect: func(m *mock.MockNetworkClientMockRecorder) {}, + wantSGStatus: infrav1.SecurityGroupStatus{}, }, { - name: "Same desiredSGSpecs and observedSGStatus produces no changes", - desiredSGSpecs: securityGroupSpec{ - Name: "k8s-cluster-mycluster-secgroup-controlplane", + name: "Same desiredSGSpec and observedSG produces no changes", + desiredSGSpec: securityGroupSpec{ + Name: sgName, Rules: []resolvedSecurityGroupRuleSpec{ { Description: "Allow SSH", @@ -401,46 +378,29 @@ func TestReconcileGroupRules(t *testing.T) { }, }, }, - observedSGStatus: infrav1.SecurityGroupStatus{ - ID: "idSG", - Name: "k8s-cluster-mycluster-secgroup-controlplane", - Rules: []infrav1.SecurityGroupRuleStatus{ + observedSG: groups.SecGroup{ + ID: sgID, + Name: sgName, + Rules: []rules.SecGroupRule{ { - Description: pointer.String("Allow SSH"), + Description: "Allow SSH", Direction: "ingress", - EtherType: pointer.String("IPv4"), + EtherType: "IPv4", ID: "idSGRule", - Protocol: pointer.String("tcp"), - PortRangeMin: pointer.Int(22), - PortRangeMax: pointer.Int(22), - RemoteGroupID: pointer.String("1"), - RemoteIPPrefix: pointer.String(""), + Protocol: "tcp", + PortRangeMin: 22, + PortRangeMax: 22, + RemoteGroupID: "1", + RemoteIPPrefix: "", }, }, }, mockExpect: func(m *mock.MockNetworkClientMockRecorder) {}, - wantSGStatus: infrav1.SecurityGroupStatus{ - ID: "idSG", - Name: "k8s-cluster-mycluster-secgroup-controlplane", - Rules: []infrav1.SecurityGroupRuleStatus{ - { - Description: pointer.String("Allow SSH"), - Direction: "ingress", - EtherType: pointer.String("IPv4"), - ID: "idSGRule", - Protocol: pointer.String("tcp"), - PortRangeMin: pointer.Int(22), - PortRangeMax: pointer.Int(22), - RemoteGroupID: pointer.String("1"), - RemoteIPPrefix: pointer.String(""), - }, - }, - }, }, { - name: "Different desiredSGSpecs and observedSGStatus produces changes", - desiredSGSpecs: securityGroupSpec{ - Name: "k8s-cluster-mycluster-secgroup-controlplane", + name: "Different desiredSGSpec and observedSG produces changes", + desiredSGSpec: securityGroupSpec{ + Name: sgName, Rules: []resolvedSecurityGroupRuleSpec{ { Description: "Allow SSH", @@ -454,27 +414,27 @@ func TestReconcileGroupRules(t *testing.T) { }, }, }, - observedSGStatus: infrav1.SecurityGroupStatus{ - ID: "idSG", - Name: "k8s-cluster-mycluster-secgroup-controlplane", - Rules: []infrav1.SecurityGroupRuleStatus{ + observedSG: groups.SecGroup{ + ID: sgID, + Name: sgName, + Rules: []rules.SecGroupRule{ { - Description: pointer.String("Allow SSH legacy"), + ID: sgLegacyRuleID, + Description: "Allow SSH legacy", Direction: "ingress", - EtherType: pointer.String("IPv4"), - ID: "idSGRuleLegacy", - Protocol: pointer.String("tcp"), - PortRangeMin: pointer.Int(222), - PortRangeMax: pointer.Int(222), - RemoteGroupID: pointer.String("2"), - RemoteIPPrefix: pointer.String(""), + EtherType: "IPv4", + Protocol: "tcp", + PortRangeMin: 222, + PortRangeMax: 222, + RemoteGroupID: "2", + RemoteIPPrefix: "", }, }, }, mockExpect: func(m *mock.MockNetworkClientMockRecorder) { - m.DeleteSecGroupRule("idSGRuleLegacy").Return(nil) + m.DeleteSecGroupRule(sgLegacyRuleID).Return(nil) m.CreateSecGroupRule(rules.CreateOpts{ - SecGroupID: "idSG", + SecGroupID: sgID, Description: "Allow SSH", Direction: "ingress", EtherType: "IPv4", @@ -483,7 +443,7 @@ func TestReconcileGroupRules(t *testing.T) { PortRangeMax: 22, RemoteGroupID: "1", }).Return(&rules.SecGroupRule{ - ID: "idSGRule", + ID: sgRuleID, Description: "Allow SSH", Direction: "ingress", EtherType: "IPv4", @@ -493,28 +453,15 @@ func TestReconcileGroupRules(t *testing.T) { RemoteGroupID: "1", }, nil) }, - wantSGStatus: infrav1.SecurityGroupStatus{ - ID: "idSG", - Name: "k8s-cluster-mycluster-secgroup-controlplane", - Rules: []infrav1.SecurityGroupRuleStatus{ - { - Description: pointer.String("Allow SSH"), - Direction: "ingress", - EtherType: pointer.String("IPv4"), - ID: "idSGRule", - Protocol: pointer.String("tcp"), - PortRangeMin: pointer.Int(22), - PortRangeMax: pointer.Int(22), - RemoteGroupID: pointer.String("1"), - RemoteIPPrefix: pointer.String(""), - }, - }, - }, }, } - for _, tt := range tests { + for i := range tests { + tt := &tests[i] t.Run(tt.name, func(t *testing.T) { + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + g := NewWithT(t) log := testr.New(t) mockScopeFactory := scope.NewMockScopeFactory(mockCtrl, "") @@ -525,9 +472,131 @@ func TestReconcileGroupRules(t *testing.T) { } tt.mockExpect(mockScopeFactory.NetworkClient.EXPECT()) - sgStatus, err := s.reconcileGroupRules(tt.desiredSGSpecs, tt.observedSGStatus) + err = s.reconcileGroupRules(&tt.desiredSGSpec, &tt.observedSG) g.Expect(err).To(BeNil()) - g.Expect(sgStatus).To(Equal(tt.wantSGStatus)) + }) + } +} + +func TestService_ReconcileSecurityGroups(t *testing.T) { + const ( + clusterName = "test-cluster" + + controlPlaneSGName = "k8s-cluster-test-cluster-secgroup-controlplane" + workerSGName = "k8s-cluster-test-cluster-secgroup-worker" + bastionSGName = "k8s-cluster-test-cluster-secgroup-bastion" + ) + + tests := []struct { + name string + openStackClusterSpec infrav1.OpenStackClusterSpec + expectedClusterStatus infrav1.OpenStackClusterStatus + expect func(log logr.Logger, m *mock.MockNetworkClientMockRecorder) + wantErr bool + }{ + { + name: "Do nothing if ManagedSecurityGroups is not enabled", + openStackClusterSpec: infrav1.OpenStackClusterSpec{}, + expectedClusterStatus: infrav1.OpenStackClusterStatus{}, + }, + { + name: "Default control plane and worker security groups", + openStackClusterSpec: infrav1.OpenStackClusterSpec{ + ManagedSecurityGroups: &infrav1.ManagedSecurityGroups{}, + }, + expect: func(log logr.Logger, m *mock.MockNetworkClientMockRecorder) { + m.ListSecGroup(groups.ListOpts{Name: controlPlaneSGName}). + Return([]groups.SecGroup{{ID: "0", Name: controlPlaneSGName}}, nil) + m.ListSecGroup(groups.ListOpts{Name: workerSGName}). + Return([]groups.SecGroup{{ID: "1", Name: workerSGName}}, nil) + + // We expect a total of 12 rules to be created. + // Nothing actually looks at the generated + // rules, but we give them unique IDs anyway + m.CreateSecGroupRule(gomock.Any()).DoAndReturn(func(opts rules.CreateOpts) (*rules.SecGroupRule, error) { + log.Info("Created rule", "securityGroup", opts.SecGroupID, "description", opts.Description) + return &rules.SecGroupRule{ID: uuid.NewString()}, nil + }).Times(12) + }, + expectedClusterStatus: infrav1.OpenStackClusterStatus{ + ControlPlaneSecurityGroup: &infrav1.SecurityGroupStatus{ + ID: "0", + Name: controlPlaneSGName, + }, + WorkerSecurityGroup: &infrav1.SecurityGroupStatus{ + ID: "1", + Name: workerSGName, + }, + }, + }, + { + name: "Default control plane, worker, and bastion security groups", + openStackClusterSpec: infrav1.OpenStackClusterSpec{ + Bastion: &infrav1.Bastion{ + Enabled: true, + }, + ManagedSecurityGroups: &infrav1.ManagedSecurityGroups{}, + }, + expect: func(log logr.Logger, m *mock.MockNetworkClientMockRecorder) { + m.ListSecGroup(groups.ListOpts{Name: controlPlaneSGName}). + Return([]groups.SecGroup{{ID: "0", Name: controlPlaneSGName}}, nil) + m.ListSecGroup(groups.ListOpts{Name: workerSGName}). + Return([]groups.SecGroup{{ID: "1", Name: workerSGName}}, nil) + m.ListSecGroup(groups.ListOpts{Name: bastionSGName}). + Return([]groups.SecGroup{{ID: "2", Name: bastionSGName}}, nil) + + // We expect a total of 12 rules to be created. + // Nothing actually looks at the generated + // rules, but we give them unique IDs anyway + m.CreateSecGroupRule(gomock.Any()).DoAndReturn(func(opts rules.CreateOpts) (*rules.SecGroupRule, error) { + log.Info("Created rule", "securityGroup", opts.SecGroupID, "description", opts.Description) + return &rules.SecGroupRule{ID: uuid.NewString()}, nil + }).Times(17) + }, + expectedClusterStatus: infrav1.OpenStackClusterStatus{ + ControlPlaneSecurityGroup: &infrav1.SecurityGroupStatus{ + ID: "0", + Name: controlPlaneSGName, + }, + WorkerSecurityGroup: &infrav1.SecurityGroupStatus{ + ID: "1", + Name: workerSGName, + }, + BastionSecurityGroup: &infrav1.SecurityGroupStatus{ + ID: "2", + Name: bastionSGName, + }, + }, + }, + } + for i := range tests { + tt := &tests[i] + t.Run(tt.name, func(t *testing.T) { + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + + g := NewWithT(t) + log := testr.New(t) + mockScopeFactory := scope.NewMockScopeFactory(mockCtrl, "") + scope := scope.NewWithLogger(mockScopeFactory, log) + + s := &Service{ + scope: scope, + client: mockScopeFactory.NetworkClient, + } + if tt.expect != nil { + tt.expect(log, mockScopeFactory.NetworkClient.EXPECT()) + } + openStackCluster := &infrav1.OpenStackCluster{ + Spec: tt.openStackClusterSpec, + } + err := s.ReconcileSecurityGroups(openStackCluster, clusterName) + if tt.wantErr { + g.Expect(err).ToNot(BeNil(), "ReconcileSecurityGroups") + } else { + g.Expect(err).To(BeNil(), "ReconcileSecurityGroups") + g.Expect(openStackCluster.Status).To(Equal(tt.expectedClusterStatus), cmp.Diff(openStackCluster.Status, tt.expectedClusterStatus)) + } }) } }