add additionalSecurityGroups field to ROSAmachinePool

kubernetes-sigs · Mar 5, 2024 · 4df5b91 · 4df5b91
1 parent 2d004cc
commit 4df5b91
Show file tree

Hide file tree

Showing 5 changed files with 67 additions and 15 deletions.
diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_rosamachinepools.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_rosamachinepools.yaml
@@ -47,6 +47,12 @@ spec:
           spec:
             description: RosaMachinePoolSpec defines the desired state of RosaMachinePool.
             properties:
+              additionalSecurityGroups:
+                description: AdditionalSecurityGroups is an optional set of security
+                  groups to associate with all node instances of the machine pool.
+                items:
+                  type: string
+                type: array
               autoRepair:
                 default: false
                 description: AutoRepair specifies whether health checks should be

diff --git a/exp/api/v1beta2/rosamachinepool_types.go b/exp/api/v1beta2/rosamachinepool_types.go
@@ -79,6 +79,13 @@ type RosaMachinePoolSpec struct {
 	// +optional
 	TuningConfigs []string `json:"tuningConfigs,omitempty"`
 
+	// AdditionalSecurityGroups is an optional set of security groups to associate
+	// with all node instances of the machine pool.
+	//
+	// +immutable
+	// +optional
+	AdditionalSecurityGroups []string `json:"additionalSecurityGroups,omitempty"`
+
 	// ProviderIDList contain a ProviderID for each machine instance that's currently managed by this machine pool.
 	// +optional
 	ProviderIDList []string `json:"providerIDList,omitempty"`

diff --git a/exp/api/v1beta2/rosamachinepool_webhook.go b/exp/api/v1beta2/rosamachinepool_webhook.go
@@ -2,6 +2,8 @@ package v1beta2
 
 import (
 	"github.com/blang/semver"
+	"github.com/google/go-cmp/cmp"
+	"github.com/pkg/errors"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	runtime "k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/util/validation/field"
@@ -44,12 +46,20 @@ func (r *ROSAMachinePool) ValidateCreate() (warnings admission.Warnings, err err
 
 // ValidateUpdate implements admission.Validator.
 func (r *ROSAMachinePool) ValidateUpdate(old runtime.Object) (warnings admission.Warnings, err error) {
-	var allErrs field.ErrorList
+	oldPool, ok := old.(*ROSAMachinePool)
+	if !ok {
+		return nil, apierrors.NewInvalid(GroupVersion.WithKind("ROSAMachinePool").GroupKind(), r.Name, field.ErrorList{
+			field.InternalError(nil, errors.New("failed to convert old ROSAMachinePool to object")),
+		})
+	}
 
+	var allErrs field.ErrorList
 	if err := r.validateVersion(); err != nil {
 		allErrs = append(allErrs, err)
 	}
 
+	allErrs = append(allErrs, validateImmutable(oldPool.Spec.AdditionalSecurityGroups, r.Spec.AdditionalSecurityGroups, "additionalSecurityGroups")...)
+
 	if len(allErrs) == 0 {
 		return nil, nil
 	}
@@ -78,6 +88,19 @@ func (r *ROSAMachinePool) validateVersion() *field.Error {
 	return nil
 }
 
+func validateImmutable(old, updated interface{}, name string) field.ErrorList {
+	var allErrs field.ErrorList
+
+	if !cmp.Equal(old, updated) {
+		allErrs = append(
+			allErrs,
+			field.Invalid(field.NewPath("spec", name), updated, "field is immutable"),
+		)
+	}
+
+	return allErrs
+}
+
 // Default implements admission.Defaulter.
 func (r *ROSAMachinePool) Default() {
 }
diff --git a/exp/api/v1beta2/zz_generated.deepcopy.go b/exp/api/v1beta2/zz_generated.deepcopy.go
diff --git a/exp/controllers/rosamachinepool_controller.go b/exp/controllers/rosamachinepool_controller.go
@@ -352,16 +352,18 @@ func (r *ROSAMachinePoolReconciler) updateNodePool(machinePoolScope *scope.RosaM
 
 	currentSpec := nodePoolToRosaMachinePoolSpec(nodePool)
 	currentSpec.ProviderIDList = desiredSpec.ProviderIDList // providerIDList is set by the controller and shouldn't be compared here.
-	currentSpec.Version = desiredSpec.Version               // Version changed are reconciled separately and shouldn't be compared here.
+	currentSpec.Version = desiredSpec.Version               // Version changes are reconciled separately and shouldn't be compared here.
 
 	if cmp.Equal(desiredSpec, currentSpec) {
 		// no changes detected.
 		return nodePool, nil
 	}
 
-	npBuilder := nodePoolBuilder(*desiredSpec, machinePoolScope.MachinePool.Spec)
-	npBuilder.Version(nil) // eunsure version is unset.
+	// zero-out fields that shouldn't be part of the update call.
+	desiredSpec.Version = ""
+	desiredSpec.AdditionalSecurityGroups = nil
 
+	npBuilder := nodePoolBuilder(*desiredSpec, machinePoolScope.MachinePool.Spec)
 	nodePoolSpec, err := npBuilder.Build()
 	if err != nil {
 		return nil, fmt.Errorf("failed to build nodePool spec: %w", err)
@@ -401,8 +403,11 @@ func validateMachinePoolSpec(machinePoolScope *scope.RosaMachinePoolScope) (*str
 func nodePoolBuilder(rosaMachinePoolSpec expinfrav1.RosaMachinePoolSpec, machinePoolSpec expclusterv1.MachinePoolSpec) *cmv1.NodePoolBuilder {
 	npBuilder := cmv1.NewNodePool().ID(rosaMachinePoolSpec.NodePoolName).
 		Labels(rosaMachinePoolSpec.Labels).
-		AutoRepair(rosaMachinePoolSpec.AutoRepair).
-		TuningConfigs(rosaMachinePoolSpec.TuningConfigs...)
+		AutoRepair(rosaMachinePoolSpec.AutoRepair)
+
+	if rosaMachinePoolSpec.TuningConfigs != nil {
+		npBuilder = npBuilder.TuningConfigs(rosaMachinePoolSpec.TuningConfigs...)
+	}
 
 	if len(rosaMachinePoolSpec.Taints) > 0 {
 		taintBuilders := []*cmv1.TaintBuilder{}
@@ -430,7 +435,12 @@ func nodePoolBuilder(rosaMachinePoolSpec expinfrav1.RosaMachinePoolSpec, machine
 		npBuilder.Subnet(rosaMachinePoolSpec.Subnet)
 	}
 
-	npBuilder.AWSNodePool(cmv1.NewAWSNodePool().InstanceType(rosaMachinePoolSpec.InstanceType))
+	awsNodePool := cmv1.NewAWSNodePool().InstanceType(rosaMachinePoolSpec.InstanceType)
+	if rosaMachinePoolSpec.AdditionalSecurityGroups != nil {
+		awsNodePool = awsNodePool.AdditionalSecurityGroupIds(rosaMachinePoolSpec.AdditionalSecurityGroups...)
+	}
+	npBuilder.AWSNodePool(awsNodePool)
+
 	if rosaMachinePoolSpec.Version != "" {
 		npBuilder.Version(cmv1.NewVersion().ID(ocm.CreateVersionID(rosaMachinePoolSpec.Version, ocm.DefaultChannelGroup)))
 	}
@@ -440,14 +450,15 @@ func nodePoolBuilder(rosaMachinePoolSpec expinfrav1.RosaMachinePoolSpec, machine
 
 func nodePoolToRosaMachinePoolSpec(nodePool *cmv1.NodePool) expinfrav1.RosaMachinePoolSpec {
 	spec := expinfrav1.RosaMachinePoolSpec{
-		NodePoolName:     nodePool.ID(),
-		Version:          rosa.RawVersionID(nodePool.Version()),
-		AvailabilityZone: nodePool.AvailabilityZone(),
-		Subnet:           nodePool.Subnet(),
-		Labels:           nodePool.Labels(),
-		AutoRepair:       nodePool.AutoRepair(),
-		InstanceType:     nodePool.AWSNodePool().InstanceType(),
-		TuningConfigs:    nodePool.TuningConfigs(),
+		NodePoolName:             nodePool.ID(),
+		Version:                  rosa.RawVersionID(nodePool.Version()),
+		AvailabilityZone:         nodePool.AvailabilityZone(),
+		Subnet:                   nodePool.Subnet(),
+		Labels:                   nodePool.Labels(),
+		AutoRepair:               nodePool.AutoRepair(),
+		InstanceType:             nodePool.AWSNodePool().InstanceType(),
+		TuningConfigs:            nodePool.TuningConfigs(),
+		AdditionalSecurityGroups: nodePool.AWSNodePool().AdditionalSecurityGroupIds(),
 	}
 
 	if nodePool.Autoscaling() != nil {