Skip to content

🐛 add marketType field validation #5374

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Apr 5, 2025
Merged

🐛 add marketType field validation #5374

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2b37a28
Add spotMarketRequest on marketType spot
Mar 6, 2025
c9baa03
Add CEL validation
Mar 16, 2025
3372511
update vendor files
Mar 17, 2025
6123a1a
added validation
Mar 17, 2025
22650ee
Updated the comments
Mar 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions api/v1beta2/awsmachine_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Member

@richardcase richardcase Apr 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll need to check the docs (but maybe @JoelSpeed knows), if we are tightening the validation rules is this considered a breaking behavioural change? Guess it depends on if it breaks currently.

@athiruma - whats the current behaviour if i set capacityReservationId and specify spot? Does it currently error in this situation?

Copy link
Contributor Author

@athiruma athiruma Apr 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it will raise an error.

Copy link

@JoelSpeed JoelSpeed Apr 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the capacity reservation ID field has already shipped within a release, then yes, this could cause an issue for users who have already started using the field

Given that EC2 will reject creating machines with this configuration already, we are bringing the validation earlier here realistically, and ratcheting should also help with invalid combinations if folks are updating the objects.

I'm inclined to say this is likely to be fairly safe depending on how long ago this shipped

Copy link
Member

@damdo damdo Apr 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has shipped in CAPA v2.8.1 a couple of weeks ago.
I think if we backport it to v2.8.z and then release it in v2.9.0 we should be fine right?

Copy link
Member

@richardcase richardcase Apr 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like we are all good then.

Copy link
Member

@damdo damdo Apr 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah considering this would already fail anyway at a later stage.
Happy to approve then @richardcase ?

type AWSMachineSpec struct {
// ProviderID is the unique identifier as specified by the cloud provider.
ProviderID *string `json:"providerID,omitempty"`
Expand Down
7 changes: 7 additions & 0 deletions config/crd/bases/infrastructure.cluster.x-k8s.io_awsmachines.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
8 changes: 8 additions & 0 deletions config/crd/bases/infrastructure.cluster.x-k8s.io_awsmachinetemplates.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
13 changes: 13 additions & 0 deletions exp/api/v1beta2/awsmachinepool_webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package v1beta2

import (
"fmt"
"time"

apierrors "k8s.io/apimachinery/pkg/api/errors"
Expand Down Expand Up @@ -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
}
Expand Down
47 changes: 47 additions & 0 deletions exp/api/v1beta2/awsmachinepool_webhook_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down
8 changes: 8 additions & 0 deletions pkg/cloud/services/ec2/instances.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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 {
Expand Down
29 changes: 29 additions & 0 deletions pkg/cloud/services/ec2/instances_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down
Loading