🐛 add marketType field validation (#5374)

* Add spotMarketRequest on marketType spot

* Add CEL validation

* update vendor files

* added validation

* Updated the comments

Author: athiruma

api/v1beta2/awsmachine_types.go

@@ -64,6 +64,8 @@ const (
 )
 
 // AWSMachineSpec defines the desired state of an Amazon EC2 instance.
+// +kubebuilder:validation:XValidation:rule="!has(self.capacityReservationId) || !has(self.marketType) || self.marketType != 'Spot'",message="capacityReservationId may not be set when marketType is Spot"
+// +kubebuilder:validation:XValidation:rule="!has(self.capacityReservationId) || !has(self.spotMarketOptions)",message="capacityReservationId cannot be set when spotMarketOptions is specified"
 type AWSMachineSpec struct {
 	// ProviderID is the unique identifier as specified by the cloud provider.
 	ProviderID *string `json:"providerID,omitempty"`

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

@@ -1104,6 +1104,13 @@ spec:
             required:
             - instanceType
             type: object
+            x-kubernetes-validations:
+            - message: capacityReservationId may not be set when marketType is Spot
+              rule: '!has(self.capacityReservationId) || !has(self.marketType) ||
+                self.marketType != ''Spot'''
+            - message: capacityReservationId cannot be set when spotMarketOptions
+                is specified
+              rule: '!has(self.capacityReservationId) || !has(self.spotMarketOptions)'
           status:
             description: AWSMachineStatus defines the observed state of AWSMachine.
             properties:

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

@@ -1045,6 +1045,14 @@ spec:
                     required:
                     - instanceType
                     type: object
+                    x-kubernetes-validations:
+                    - message: capacityReservationId may not be set when marketType
+                        is Spot
+                      rule: '!has(self.capacityReservationId) || !has(self.marketType)
+                        || self.marketType != ''Spot'''
+                    - message: capacityReservationId cannot be set when spotMarketOptions
+                        is specified
+                      rule: '!has(self.capacityReservationId) || !has(self.spotMarketOptions)'
                 required:
                 - spec
                 type: object

exp/api/v1beta2/awsmachinepool_webhook.go

@@ -17,6 +17,7 @@ limitations under the License.
 package v1beta2
 
 import (
+	"fmt"
 	"time"
 
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
@@ -201,6 +202,18 @@ func (r *AWSMachinePool) validateInstanceMarketType() field.ErrorList {
 	if r.Spec.AWSLaunchTemplate.MarketType == v1beta2.MarketTypeCapacityBlock && r.Spec.AWSLaunchTemplate.CapacityReservationID == nil {
 		allErrs = append(allErrs, field.Forbidden(field.NewPath("spec.awsLaunchTemplate.capacityReservationID"), "is required when CapacityBlock is provided"))
 	}
+	switch r.Spec.AWSLaunchTemplate.MarketType {
+	case "", v1beta2.MarketTypeOnDemand, v1beta2.MarketTypeSpot, v1beta2.MarketTypeCapacityBlock:
+	default:
+		allErrs = append(allErrs, field.Invalid(field.NewPath("spec.awsLaunchTemplate.MarketType"), r.Spec.AWSLaunchTemplate.MarketType, fmt.Sprintf("Valid values are: %s, %s, %s and omitted", v1beta2.MarketTypeOnDemand, v1beta2.MarketTypeSpot, v1beta2.MarketTypeCapacityBlock)))
+	}
+	if r.Spec.AWSLaunchTemplate.MarketType == v1beta2.MarketTypeSpot && r.Spec.AWSLaunchTemplate.CapacityReservationID != nil {
+		allErrs = append(allErrs, field.Forbidden(field.NewPath("spec.awsLaunchTemplate.marketType"), "cannot be set to 'Spot' when CapacityReservationID is specified"))
+	}
+
+	if r.Spec.AWSLaunchTemplate.CapacityReservationID != nil && r.Spec.AWSLaunchTemplate.SpotMarketOptions != nil {
+		allErrs = append(allErrs, field.Forbidden(field.NewPath("spec.awsLaunchTemplate.spotMarketOptions"), "cannot be set to when CapacityReservationID is specified"))
+	}
 
 	return allErrs
 }

exp/api/v1beta2/awsmachinepool_webhook_test.go

@@ -174,6 +174,53 @@ func TestAWSMachinePoolValidateCreate(t *testing.T) {
 			},
 			wantErr: true,
 		},
+		{
+			name: "with invalid MarketType provided",
+			pool: &AWSMachinePool{
+				Spec: AWSMachinePoolSpec{
+					AWSLaunchTemplate: AWSLaunchTemplate{
+						MarketType: "invalid",
+					},
+				},
+			},
+			wantErr: true,
+		},
+		{
+			name: "with MarketType empty value provided",
+			pool: &AWSMachinePool{
+				Spec: AWSMachinePoolSpec{
+					AWSLaunchTemplate: AWSLaunchTemplate{
+						MarketType: "",
+					},
+				},
+			},
+			wantErr: false,
+		},
+		{
+			name: "with MarketType Spot and CapacityReservationID value provided",
+			pool: &AWSMachinePool{
+				Spec: AWSMachinePoolSpec{
+					AWSLaunchTemplate: AWSLaunchTemplate{
+						MarketType:            infrav1.MarketTypeSpot,
+						CapacityReservationID: aws.String("cr-123"),
+					},
+				},
+			},
+			wantErr: true,
+		},
+		{
+			name: "with CapacityReservationID and SpotMarketOptions value provided",
+			pool: &AWSMachinePool{
+				Spec: AWSMachinePoolSpec{
+					AWSLaunchTemplate: AWSLaunchTemplate{
+						SpotMarketOptions:     &infrav1.SpotMarketOptions{},
+						CapacityReservationID: aws.String("cr-123"),
+					},
+				},
+			},
+			wantErr: true,
+		},
+
 		{
 			name: "invalid, MarketType set to MarketTypeCapacityBlock and spotMarketOptions are specified",
 			pool: &AWSMachinePool{

pkg/cloud/services/ec2/instances.go

@@ -1154,6 +1154,10 @@ func getInstanceMarketOptionsRequest(i *infrav1.Instance) (*ec2.InstanceMarketOp
 		return nil, errors.New("can't create spot capacity-blocks, remove spot market request")
 	}
 
+	if (i.MarketType == infrav1.MarketTypeSpot || i.SpotMarketOptions != nil) && i.CapacityReservationID != nil {
+		return nil, errors.New("unable to generate marketOptions for spot instance, capacityReservationID is incompatible with marketType spot and spotMarketOptions")
+	}
+
 	// Infer MarketType if not explicitly set
 	if i.SpotMarketOptions != nil && i.MarketType == "" {
 		i.MarketType = infrav1.MarketTypeSpot
@@ -1163,6 +1167,10 @@ func getInstanceMarketOptionsRequest(i *infrav1.Instance) (*ec2.InstanceMarketOp
 		i.MarketType = infrav1.MarketTypeOnDemand
 	}
 
+	if i.MarketType == infrav1.MarketTypeSpot && i.SpotMarketOptions == nil {
+		i.SpotMarketOptions = &infrav1.SpotMarketOptions{}
+	}
+
 	switch i.MarketType {
 	case infrav1.MarketTypeCapacityBlock:
 		if i.CapacityReservationID == nil {

pkg/cloud/services/ec2/instances_test.go

@@ -5770,6 +5770,35 @@ func TestGetInstanceMarketOptionsRequest(t *testing.T) {
 			},
 			expectedError: nil,
 		},
+		{
+			name: "with marketType Spot specified",
+			instance: &infrav1.Instance{
+				MarketType: infrav1.MarketTypeSpot,
+			},
+			expectedRequest: &ec2.InstanceMarketOptionsRequest{
+				MarketType: aws.String(ec2.MarketTypeSpot),
+				SpotOptions: &ec2.SpotMarketOptions{
+					InstanceInterruptionBehavior: aws.String(ec2.InstanceInterruptionBehaviorTerminate),
+					SpotInstanceType:             aws.String(ec2.SpotInstanceTypeOneTime),
+				},
+			},
+		},
+		{
+			name: "with marketType Spot and capacityRerservationID specified",
+			instance: &infrav1.Instance{
+				MarketType:            infrav1.MarketTypeSpot,
+				CapacityReservationID: mockCapacityReservationID,
+			},
+			expectedError: errors.Errorf("unable to generate marketOptions for spot instance, capacityReservationID is incompatible with marketType spot and spotMarketOptions"),
+		},
+		{
+			name: "with spotMarketOptions and capacityRerservationID specified",
+			instance: &infrav1.Instance{
+				SpotMarketOptions:     &infrav1.SpotMarketOptions{},
+				CapacityReservationID: mockCapacityReservationID,
+			},
+			expectedError: errors.Errorf("unable to generate marketOptions for spot instance, capacityReservationID is incompatible with marketType spot and spotMarketOptions"),
+		},
 		{
 			name: "with an empty MaxPrice specified",
 			instance: &infrav1.Instance{