feat(securityGroups): Improve robustness of security group reconcilers

api/v1beta1/osccluster_validation.go

@@ -52,7 +52,7 @@ func ValidateOscClusterSpec(spec OscClusterSpec) field.ErrorList {
 // ValidateCidr check that the cidr string is a valide CIDR
 func ValidateCidr(cidr string) (string, error) {
 	if !strings.Contains(cidr, "/") {
-		return cidr, errors.New("Invalid Not A CIDR")
+		return cidr, errors.New("invalid Not A CIDR")
 	}
 	_, _, err := net.ParseCIDR(cidr)
 	if err != nil {

api/v1beta1/types.go

@@ -75,9 +75,6 @@ type OscNetwork struct {
 	// The subregion name
 	// + optional
 	SubregionName string `json:"subregionName,omitempty"`
-	// Add SecurityGroup Rule after the cluster is created
-	// + optional
-	ExtraSecurityGroupRule bool `json:"extraSecurityGroupRule,omitempty"`
 }
 
 type OscLoadBalancer struct {
@@ -219,6 +216,9 @@ type OscSecurityGroup struct {
 	// The description of the security group
 	// +optional
 	Description string `json:"description,omitempty"`
+	// Should the default allow all outbound rule be deleted
+	// +optional
+	DeleteDefaultOutboundRule bool `json:"deleteDefaultOutboundRule,omitempty"`
 	// The Security Group Rules configuration
 	// +optional
 	SecurityGroupRules []OscSecurityGroupRule `json:"securityGroupRules,omitempty"`
@@ -278,6 +278,9 @@ type OscSecurityGroupRule struct {
 	// The ip range of the security group rule
 	// +optional
 	IpRange string `json:"ipRange,omitempty"`
+	// The name of the security group to use as target
+	// +optional
+	TargetSecurityGroupName string `json:"targetSecurityGroupName,omitempty"`
 	// The beginning of the port range
 	// +optional
 	FromPortRange int32 `json:"fromPortRange,omitempty"`

cloud/scope/cluster.go

@@ -180,16 +180,6 @@ func (s *ClusterScope) GetNet() *infrastructurev1beta1.OscNet {
 	return &s.OscCluster.Spec.Network.Net
 }
 
-// GetExtraSecurityGroupRule return the extraSecurityGroupRule
-func (s *ClusterScope) GetExtraSecurityGroupRule() bool {
-	return s.OscCluster.Spec.Network.ExtraSecurityGroupRule
-}
-
-// SetExtraSecurityGroupRule set the extraSecurityGroupRule
-func (s *ClusterScope) SetExtraSecurityGroupRule(extraSecurityGroupRule bool) {
-	s.OscCluster.Spec.Network.ExtraSecurityGroupRule = extraSecurityGroupRule
-}
-
 // GetPublicIpNameAfterBastion return publicIpNameAfterBastion
 func (s *ClusterScope) GetPublicIpNameAfterBastion() bool {
 	return s.OscCluster.Spec.Network.Bastion.PublicIpNameAfterBastion

cloud/services/security/securitygroup.go

@@ -18,7 +18,6 @@ package security
 
 import (
 	"fmt"
-	"net/http"
 
 	"errors"
 
@@ -36,7 +35,7 @@ type OscSecurityGroupInterface interface {
 	CreateSecurityGroup(netId string, clusterName string, securityGroupName string, securityGroupDescription string, securityGroupTag string) (*osc.SecurityGroup, error)
 	CreateSecurityGroupRule(securityGroupId string, flow string, ipProtocol string, ipRange string, securityGroupMemberId string, fromPortRange int32, toPortRange int32) (*osc.SecurityGroup, error)
 	DeleteSecurityGroupRule(securityGroupId string, flow string, ipProtocol string, ipRange string, securityGroupMemberId string, fromPortRange int32, toPortRange int32) error
-	DeleteSecurityGroup(securityGroupId string) (error, *http.Response)
+	DeleteSecurityGroup(securityGroupId string) error
 	GetSecurityGroup(securityGroupId string) (*osc.SecurityGroup, error)
 	GetSecurityGroupFromSecurityGroupRule(securityGroupId string, Flow string, IpProtocols string, IpRanges string, securityGroupMemberId string, FromPortRanges int32, ToPortRanges int32) (*osc.SecurityGroup, error)
 	GetSecurityGroupIdsFromNetIds(netId string) ([]string, error)
@@ -218,7 +217,7 @@ func (s *Service) DeleteSecurityGroupRule(securityGroupId string, flow string, i
 	oscApiClient := s.scope.GetApi()
 	oscAuthClient := s.scope.GetAuth()
 
-	deleteSecurityGroupCallBack := func() (bool, error) {
+	deleteSecurityGroupRuleCallBack := func() (bool, error) {
 		var httpRes *_nethttp.Response
 		var err error
 
@@ -239,26 +238,45 @@ func (s *Service) DeleteSecurityGroupRule(securityGroupId string, flow string, i
 		return true, err
 	}
 	backoff := reconciler.EnvBackoff()
-	waitErr := wait.ExponentialBackoff(backoff, deleteSecurityGroupCallBack)
+	waitErr := wait.ExponentialBackoff(backoff, deleteSecurityGroupRuleCallBack)
 	if waitErr != nil {
 		return waitErr
 	}
 	return nil
 }
 
 // DeleteSecurityGroup delete the securitygroup associated with the net
-func (s *Service) DeleteSecurityGroup(securityGroupId string) (error, *http.Response) {
+func (s *Service) DeleteSecurityGroup(securityGroupId string) error {
 	deleteSecurityGroupRequest := osc.DeleteSecurityGroupRequest{SecurityGroupId: &securityGroupId}
 	oscApiClient := s.scope.GetApi()
 	oscAuthClient := s.scope.GetAuth()
-	_, httpRes, err := oscApiClient.SecurityGroupApi.DeleteSecurityGroup(oscAuthClient).DeleteSecurityGroupRequest(deleteSecurityGroupRequest).Execute()
-	if err != nil {
-		if httpRes != nil {
-			fmt.Printf("Error with http result %s", httpRes.Status)
-			return err, httpRes
+
+	deleteSecurityGroupCallBack := func() (bool, error) {
+		var httpRes *_nethttp.Response
+		var err error
+
+		_, httpRes, err = oscApiClient.SecurityGroupApi.DeleteSecurityGroup(oscAuthClient).DeleteSecurityGroupRequest(deleteSecurityGroupRequest).Execute()
+		if err != nil {
+			if httpRes != nil {
+				return false, fmt.Errorf("error %w httpRes %s", err, httpRes.Status)
+			}
+			requestStr := fmt.Sprintf("%v", deleteSecurityGroupRequest)
+			if reconciler.KeepRetryWithError(
+				requestStr,
+				httpRes.StatusCode,
+				reconciler.ThrottlingErrors) {
+				return false, nil
+			}
+			return false, err
 		}
+		return true, err
+	}
+	backoff := reconciler.EnvBackoff()
+	waitErr := wait.ExponentialBackoff(backoff, deleteSecurityGroupCallBack)
+	if waitErr != nil {
+		return waitErr
 	}
-	return nil, httpRes
+	return nil
 }
 
 // GetSecurityGroup retrieve security group object from the security group id
@@ -314,9 +332,7 @@ func (s *Service) GetSecurityGroupFromSecurityGroupRule(securityGroupId string,
 		fromPortRanges = -1
 		toPortRanges = -1
 	}
-	if fromPortRanges == 53 && toPortRanges == 53 {
-		ipProtocols = "udp"
-	}
+
 	switch {
 	case flow == "Inbound":
 		readSecurityGroupRuleRequest = osc.ReadSecurityGroupsRequest{

config/crd/bases/infrastructure.cluster.x-k8s.io_oscclusters.yaml

@@ -3,7 +3,7 @@ apiVersion: apiextensions.k8s.io/v1
 kind: CustomResourceDefinition
 metadata:
   annotations:
-    controller-gen.kubebuilder.io/version: v0.15.0
+    controller-gen.kubebuilder.io/version: v0.16.2
   name: oscclusters.infrastructure.cluster.x-k8s.io
 spec:
   group: infrastructure.cluster.x-k8s.io
@@ -122,9 +122,6 @@ spec:
                     items:
                       type: string
                     type: array
-                  extraSecurityGroupRule:
-                    description: Add SecurityGroup Rule after the cluster is created
-                    type: boolean
                   image:
                     description: The image configuration
                     properties:
@@ -329,6 +326,10 @@ spec:
                   securityGroups:
                     items:
                       properties:
+                        deleteDefaultOutboundRule:
+                          description: Should the default allow all outbound rule
+                            be deleted
+                          type: boolean
                         description:
                           description: The description of the security group
                           type: string
@@ -364,6 +365,10 @@ spec:
                               resourceId:
                                 description: The security group rule id
                                 type: string
+                              targetSecurityGroupName:
+                                description: The name of the security group to use
+                                  as target
+                                type: string
                               toPortRange:
                                 description: The end of the port range
                                 format: int32

config/crd/bases/infrastructure.cluster.x-k8s.io_oscclustertemplates.yaml

@@ -3,7 +3,7 @@ apiVersion: apiextensions.k8s.io/v1
 kind: CustomResourceDefinition
 metadata:
   annotations:
-    controller-gen.kubebuilder.io/version: v0.15.0
+    controller-gen.kubebuilder.io/version: v0.16.2
   name: oscclustertemplates.infrastructure.cluster.x-k8s.io
 spec:
   group: infrastructure.cluster.x-k8s.io
@@ -49,26 +49,22 @@ spec:
                       ObjectMeta is metadata that all persisted resources must have, which includes all objects
                       users must create. This is a copy of customizable fields from metav1.ObjectMeta.
 
-
                       ObjectMeta is embedded in `Machine.Spec`, `MachineDeployment.Template` and `MachineSet.Template`,
                       which are not top-level Kubernetes objects. Given that metav1.ObjectMeta has lots of special cases
                       and read-only fields which end up in the generated CRD validation, having it as a subset simplifies
                       the API and some issues that can impact user experience.
 
-
                       During the [upgrade to controller-tools@v2](https://github.com/kubernetes-sigs/cluster-api/pull/1054)
                       for v1alpha2, we noticed a failure would occur running Cluster API test suite against the new CRDs,
                       specifically `spec.metadata.creationTimestamp in body must be of type string: "null"`.
                       The investigation showed that `controller-tools@v2` behaves differently than its previous version
                       when handling types from [metav1](k8s.io/apimachinery/pkg/apis/meta/v1) package.
 
-
                       In more details, we found that embedded (non-top level) types that embedded `metav1.ObjectMeta`
                       had validation properties, including for `creationTimestamp` (metav1.Time).
                       The `metav1.Time` type specifies a custom json marshaller that, when IsZero() is true, returns `null`
                       which breaks validation because the field isn't marked as nullable.
 
-
                       In future versions, controller-tools@v2 might allow overriding the type and validation for embedded
                       types. When that happens, this hack should be revisited.
                     properties:
@@ -178,10 +174,6 @@ spec:
                             items:
                               type: string
                             type: array
-                          extraSecurityGroupRule:
-                            description: Add SecurityGroup Rule after the cluster
-                              is created
-                            type: boolean
                           image:
                             description: The image configuration
                             properties:
@@ -399,6 +391,10 @@ spec:
                           securityGroups:
                             items:
                               properties:
+                                deleteDefaultOutboundRule:
+                                  description: Should the default allow all outbound
+                                    rule be deleted
+                                  type: boolean
                                 description:
                                   description: The description of the security group
                                   type: string
@@ -436,6 +432,10 @@ spec:
                                       resourceId:
                                         description: The security group rule id
                                         type: string
+                                      targetSecurityGroupName:
+                                        description: The name of the security group
+                                          to use as target
+                                        type: string
                                       toPortRange:
                                         description: The end of the port range
                                         format: int32

config/crd/bases/infrastructure.cluster.x-k8s.io_oscmachines.yaml

@@ -3,7 +3,7 @@ apiVersion: apiextensions.k8s.io/v1
 kind: CustomResourceDefinition
 metadata:
   annotations:
-    controller-gen.kubebuilder.io/version: v0.15.0
+    controller-gen.kubebuilder.io/version: v0.16.2
   name: oscmachines.infrastructure.cluster.x-k8s.io
 spec:
   group: infrastructure.cluster.x-k8s.io

config/crd/bases/infrastructure.cluster.x-k8s.io_oscmachinetemplates.yaml

@@ -3,7 +3,7 @@ apiVersion: apiextensions.k8s.io/v1
 kind: CustomResourceDefinition
 metadata:
   annotations:
-    controller-gen.kubebuilder.io/version: v0.15.0
+    controller-gen.kubebuilder.io/version: v0.16.2
   name: oscmachinetemplates.infrastructure.cluster.x-k8s.io
 spec:
   group: infrastructure.cluster.x-k8s.io
@@ -50,26 +50,22 @@ spec:
                       ObjectMeta is metadata that all persisted resources must have, which includes all objects
                       users must create. This is a copy of customizable fields from metav1.ObjectMeta.
 
-
                       ObjectMeta is embedded in `Machine.Spec`, `MachineDeployment.Template` and `MachineSet.Template`,
                       which are not top-level Kubernetes objects. Given that metav1.ObjectMeta has lots of special cases
                       and read-only fields which end up in the generated CRD validation, having it as a subset simplifies
                       the API and some issues that can impact user experience.
 
-
                       During the [upgrade to controller-tools@v2](https://github.com/kubernetes-sigs/cluster-api/pull/1054)
                       for v1alpha2, we noticed a failure would occur running Cluster API test suite against the new CRDs,
                       specifically `spec.metadata.creationTimestamp in body must be of type string: "null"`.
                       The investigation showed that `controller-tools@v2` behaves differently than its previous version
                       when handling types from [metav1](k8s.io/apimachinery/pkg/apis/meta/v1) package.
 
-
                       In more details, we found that embedded (non-top level) types that embedded `metav1.ObjectMeta`
                       had validation properties, including for `creationTimestamp` (metav1.Time).
                       The `metav1.Time` type specifies a custom json marshaller that, when IsZero() is true, returns `null`
                       which breaks validation because the field isn't marked as nullable.
 
-
                       In future versions, controller-tools@v2 might allow overriding the type and validation for embedded
                       types. When that happens, this hack should be revisited.
                     properties: