feat(securityGroups): Improve robustness of security group reconcilers

outscale · Sep 9, 2024 · d79ec97 · d79ec97
1 parent 20beff9
commit d79ec97
Show file tree

Hide file tree

Showing 13 changed files with 154 additions and 296 deletions.
diff --git a/api/v1beta1/osccluster_validation.go b/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 {

diff --git a/api/v1beta1/types.go b/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 {
@@ -278,6 +275,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"`

diff --git a/cloud/scope/cluster.go b/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

diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_oscclusters.yaml b/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:
@@ -364,6 +361,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

diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_oscclustertemplates.yaml b/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:
@@ -436,6 +428,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

diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_oscmachines.yaml b/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

diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_oscmachinetemplates.yaml b/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:

diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml
@@ -27,29 +27,8 @@ rules:
   - cluster.x-k8s.io
   resources:
   - clusters
-  verbs:
-  - get
-  - list
-  - watch
-- apiGroups:
-  - cluster.x-k8s.io
-  resources:
   - clusters/status
-  verbs:
-  - get
-  - list
-  - watch
-- apiGroups:
-  - cluster.x-k8s.io
-  resources:
   - machines
-  verbs:
-  - get
-  - list
-  - watch
-- apiGroups:
-  - cluster.x-k8s.io
-  resources:
   - machines/status
   verbs:
   - get
@@ -59,32 +38,8 @@ rules:
   - infrastructure.cluster.x-k8s.io
   resources:
   - oscclusters
-  verbs:
-  - create
-  - delete
-  - get
-  - list
-  - patch
-  - update
-  - watch
-- apiGroups:
-  - infrastructure.cluster.x-k8s.io
-  resources:
-  - oscclusters/finalizers
-  verbs:
-  - update
-- apiGroups:
-  - infrastructure.cluster.x-k8s.io
-  resources:
-  - oscclusters/status
-  verbs:
-  - get
-  - patch
-  - update
-- apiGroups:
-  - infrastructure.cluster.x-k8s.io
-  resources:
   - oscmachines
+  - oscmachinetemplates
   verbs:
   - create
   - delete
@@ -96,32 +51,15 @@ rules:
 - apiGroups:
   - infrastructure.cluster.x-k8s.io
   resources:
+  - oscclusters/finalizers
   - oscmachines/finalizers
   verbs:
   - update
 - apiGroups:
   - infrastructure.cluster.x-k8s.io
   resources:
+  - oscclusters/status
   - oscmachines/status
-  verbs:
-  - get
-  - patch
-  - update
-- apiGroups:
-  - infrastructure.cluster.x-k8s.io
-  resources:
-  - oscmachinetemplates
-  verbs:
-  - create
-  - delete
-  - get
-  - list
-  - patch
-  - update
-  - watch
-- apiGroups:
-  - infrastructure.cluster.x-k8s.io
-  resources:
   - oscmachinetemplates/status
   verbs:
   - get

diff --git a/controllers/osccluster_controller.go b/controllers/osccluster_controller.go
@@ -372,10 +372,16 @@ func (r *OscClusterReconciler) reconcile(ctx context.Context, clusterScope *scop
 	securityGroupSvc := r.getSecurityGroupSvc(ctx, *clusterScope)
 	reconcileSecurityGroups, err := reconcileSecurityGroup(ctx, clusterScope, securityGroupSvc, tagSvc)
 	if err != nil {
-		clusterScope.Error(err, "failed to reconcile securityGroup")
+		clusterScope.Error(err, "failed to reconcile securityGroups")
 		conditions.MarkFalse(osccluster, infrastructurev1beta1.SecurityGroupReadyCondition, infrastructurev1beta1.SecurityGroupReconciliationFailedReason, clusterv1.ConditionSeverityWarning, err.Error())
 		return reconcileSecurityGroups, err
 	}
+	reconcileSecurityGroupRules, err := reconcileSecurityGroupRule(ctx, clusterScope, securityGroupSvc, tagSvc)
+	if err != nil {
+		clusterScope.Error(err, "failed to reconcile reconcileSecurityGroupRules")
+		conditions.MarkFalse(osccluster, infrastructurev1beta1.SecurityGroupReadyCondition, infrastructurev1beta1.SecurityGroupReconciliationFailedReason, clusterv1.ConditionSeverityWarning, err.Error())
+		return reconcileSecurityGroupRules, err
+	}
 	conditions.MarkTrue(osccluster, infrastructurev1beta1.SecurityGroupReadyCondition)
 
 	routeTableSvc := r.getRouteTableSvc(ctx, *clusterScope)