Skip to content

Commit

Permalink
feat(securityGroups): Improve robustness of security group reconcilers
Browse files Browse the repository at this point in the history
  • Loading branch information
Guido van der Hart committed Sep 10, 2024
1 parent 20beff9 commit 7eb0552
Show file tree
Hide file tree
Showing 14 changed files with 182 additions and 303 deletions.
2 changes: 1 addition & 1 deletion api/v1beta1/osccluster_validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
9 changes: 6 additions & 3 deletions api/v1beta1/types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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"`
Expand Down Expand Up @@ -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"`
Expand Down
10 changes: 0 additions & 10 deletions cloud/scope/cluster.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
13 changes: 9 additions & 4 deletions config/crd/bases/infrastructure.cluster.x-k8s.io_oscclusters.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
18 changes: 9 additions & 9 deletions config/crd/bases/infrastructure.cluster.x-k8s.io_oscclustertemplates.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion config/crd/bases/infrastructure.cluster.x-k8s.io_oscmachines.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 1 addition & 5 deletions config/crd/bases/infrastructure.cluster.x-k8s.io_oscmachinetemplates.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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:
Expand Down
68 changes: 3 additions & 65 deletions config/rbac/role.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand Down
8 changes: 7 additions & 1 deletion controllers/osccluster_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
14 changes: 7 additions & 7 deletions controllers/osccluster_loadbalancer_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -196,23 +196,23 @@ func reconcileLoadBalancer(ctx context.Context, clusterScope *scope.ClusterScope
clusterScope.V(2).Info("Create the desired loadBalancer", "loadBalancerName", loadBalancerName)
_, err := loadBalancerSvc.CreateLoadBalancer(loadBalancerSpec, subnetId, securityGroupId)
if err != nil {
return reconcile.Result{}, fmt.Errorf("%w Can not create loadBalancer for Osccluster %s/%s", err, clusterScope.GetNamespace(), clusterScope.GetName())
return reconcile.Result{}, fmt.Errorf("%w cannot create loadBalancer for Osccluster %s/%s", err, clusterScope.GetNamespace(), clusterScope.GetName())
}
clusterScope.V(2).Info("Delete default outbound rule for loadBalancer", "loadBalancerName", loadBalancerName)
err = securityGroupSvc.DeleteSecurityGroupRule(securityGroupId, "Outbound", "-1", "0.0.0.0/0", "", 0, 0)
if err != nil {
return reconcile.Result{}, fmt.Errorf("%w can not empty Outbound sg rules for loadBalancer for Osccluster %s/%s", err, clusterScope.GetNamespace(), clusterScope.GetName())
return reconcile.Result{}, fmt.Errorf("%w cannot empty Outbound sg rules for loadBalancer for Osccluster %s/%s", err, clusterScope.GetNamespace(), clusterScope.GetName())
}
clusterScope.V(2).Info("Configure the desired loadBalancer", "loadBalancerName", loadBalancerName)
loadbalancer, err = loadBalancerSvc.ConfigureHealthCheck(loadBalancerSpec)
clusterScope.V(4).Info("Get loadbalancer", "loadbalancer", loadbalancer)
if err != nil {
return reconcile.Result{}, fmt.Errorf("%w Can not configure healthcheck for Osccluster %s/%s", err, clusterScope.GetNamespace(), clusterScope.GetName())
return reconcile.Result{}, fmt.Errorf("%w cannot configure healthcheck for Osccluster %s/%s", err, clusterScope.GetNamespace(), clusterScope.GetName())
}
err = loadBalancerSvc.CreateLoadBalancerTag(loadBalancerSpec, nameTag)
clusterScope.V(2).Info("Create the desired loadBalancer tag name", "loadBalancerName", loadBalancerName)
if err != nil {
return reconcile.Result{}, fmt.Errorf("%w Can not tag loadBalancer for OscCluster %s/%s", err, clusterScope.GetNamespace(), clusterScope.GetName())
return reconcile.Result{}, fmt.Errorf("%w cannot tag loadBalancer for OscCluster %s/%s", err, clusterScope.GetNamespace(), clusterScope.GetName())
}
}
controlPlaneEndpoint := *loadbalancer.DnsName
Expand Down Expand Up @@ -256,7 +256,7 @@ func reconcileDeleteLoadBalancer(ctx context.Context, clusterScope *scope.Cluste
return reconcile.Result{}, err
}
if loadBalancerTag != nil && *loadBalancerTag.Key == nameTag.Key && *loadBalancerTag.Value != nameTag.Value {
clusterScope.V(4).Info("Can not delete LoadBalancer that already exists by other cluster", "loadBalancer", loadBalancerName)
clusterScope.V(4).Info("cannot delete LoadBalancer that already exists by other cluster", "loadBalancer", loadBalancerName)
return reconcile.Result{}, nil
}

Expand All @@ -270,12 +270,12 @@ func reconcileDeleteLoadBalancer(ctx context.Context, clusterScope *scope.Cluste
}
err = loadBalancerSvc.DeleteLoadBalancerTag(loadBalancerSpec, loadBalancerTagKey)
if err != nil {
return reconcile.Result{}, fmt.Errorf("%w Can not delete loadBalancer Tag for OscCluster %s/%s", err, clusterScope.GetNamespace(), clusterScope.GetName())
return reconcile.Result{}, fmt.Errorf("%w cannot delete loadBalancer Tag for OscCluster %s/%s", err, clusterScope.GetNamespace(), clusterScope.GetName())
}

err = loadBalancerSvc.DeleteLoadBalancer(loadBalancerSpec)
if err != nil {
return reconcile.Result{}, fmt.Errorf("%w Can not delete loadBalancer for Osccluster %s/%s", err, clusterScope.GetNamespace(), clusterScope.GetName())
return reconcile.Result{}, fmt.Errorf("%w cannot delete loadBalancer for Osccluster %s/%s", err, clusterScope.GetNamespace(), clusterScope.GetName())
}
return reconcile.Result{}, nil
}
Loading

0 comments on commit 7eb0552

Please sign in to comment.