🐛 add marketType field validation #5374

athiruma · 2025-03-06T10:49:31Z

What type of PR is this?

If only MarketType=Spot is specified but spotMarketType is not provided, then we must explicitly add spotMarketType to the API configuration.
Webhook validation for the marketType field value

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Checklist:

Release note:

None

k8s-ci-robot · 2025-03-06T10:49:41Z

Hi @athiruma. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

alexander-demicev · 2025-03-06T11:39:35Z

/ok-to-test

athiruma · 2025-03-12T04:06:07Z

@JoelSpeed ptal 👋

JoelSpeed · 2025-03-12T09:07:46Z

Changes look ok, have we considered if we should use CEL validations rather than webhooks for this kind of validation moving forward?

athiruma · 2025-03-12T12:35:54Z

Changes look ok, have we considered if we should use CEL validations rather than webhooks for this kind of validation moving forward?

Im not aware of CEL, may I'll look into it.

JoelSpeed · 2025-03-12T14:16:40Z

You can add a marker, something along the lines of

// +kubebuilder:validation:XValidation:rule="!has(self.capacityReservationID) || !has(self.marketType) || self.marketType != 'Spot'",message="capacityReservationID may not be set when marketType is Spot"

damdo · 2025-03-15T13:51:14Z

@athiruma are you able to switch to the CEL validation for this?

JoelSpeed · 2025-03-17T08:54:14Z

api/v1beta2/awsmachine_types.go

 	//  If this value is selected, CapacityReservationID must be specified to identify the target reservation.
 	// If marketType is not specified and spotMarketOptions is provided, the marketType defaults to "Spot".
 	// +optional
+	// +kubebuilder:validation:XValidation:rule="!has(self.capacityReservationID) || !has(self.marketType) || self.marketType != 'Spot'",message="capacityReservationID may not be set when marketType is Spot"


This must be added to the parent struct, and not this field, as "self" in the rule will apply to the field if you apply it here, or the parent struct if you apply it to the parent struct

athiruma · 2025-03-21T03:26:14Z

/retest-required

damdo · 2025-03-22T11:58:09Z

/retest

damdo · 2025-03-23T11:12:40Z

@athiruma we don't do vendoring here in upstream CAPA, could you drop the vendor folders please?

athiruma · 2025-03-25T17:41:29Z

/retest-required

damdo · 2025-03-26T21:14:08Z

You could try to rebase to see if apidiff gets fixed

damdo · 2025-03-29T13:44:00Z

@athiruma looks like units need updating after the error message changes.

=== RUN   TestGetInstanceMarketOptionsRequest/with_spotMarketOptions_and_capacityRerservationID_specified
    instances_test.go:5878: 
        Expected
            <string>: "unable..."
        to equal       |
            <string>: "can't ..."

athiruma · 2025-04-03T06:52:05Z

@damdo ptal!

damdo

Looks good, last little change to make it more consistent, then I am happy with it!
Thanks a lot

api/v1beta2/awsmachine_types.go

damdo · 2025-04-03T09:41:14Z

/label tide/merge-method-squash

damdo

/lgtm

/assign @nrb @richardcase @AndiDog @dlipovetsky @Ankitasw

for approval

richardcase · 2025-04-04T09:28:17Z

api/v1beta2/awsmachine_types.go


 // 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"


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?

Yes, it will raise an error.

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

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?

Sounds like we are all good then.

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

richardcase · 2025-04-05T09:55:02Z

/approve

k8s-ci-robot · 2025-04-05T09:55:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: richardcase

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [richardcase]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

* Add spotMarketRequest on marketType spot * Add CEL validation * update vendor files * added validation * Updated the comments

damdo · 2025-04-09T10:18:28Z

/cherry-pick release-2.8

k8s-infra-cherrypick-robot · 2025-04-09T10:19:25Z

@damdo: Failed to get PR patch from GitHub. This PR will need to be manually cherrypicked.

Error message

status code 422 not one of [200], body: {"message":"The patch is taking too long to generate.","documentation_url":"https://docs.github.com/rest/pulls/pulls#get-a-pull-request","status":"422"}

In response to this:

/cherry-pick release-2.8

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

* Add spotMarketRequest on marketType spot * Add CEL validation * update vendor files * added validation * Updated the comments

damdo · 2025-06-05T08:56:29Z

/cherry-pick release-2.8

k8s-infra-cherrypick-robot · 2025-06-05T08:57:14Z

@damdo: Failed to get PR patch from GitHub. This PR will need to be manually cherrypicked.

Error message

status code 422 not one of [200], body: {"message":"The patch is taking too long to generate.","documentation_url":"https://docs.github.com/rest/pulls/pulls#get-a-pull-request","status":"422"}

In response to this:

/cherry-pick release-2.8

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

* Add spotMarketRequest on marketType spot * Add CEL validation * update vendor files * added validation * Updated the comments

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 6, 2025

k8s-ci-robot requested review from faiq and fiunchinho March 6, 2025 10:49

k8s-ci-robot added needs-priority needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 6, 2025

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 6, 2025

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 6, 2025

athiruma force-pushed the fix/market-type branch from 5c51cdd to 8e8d4be Compare March 7, 2025 05:35

athiruma force-pushed the fix/market-type branch from 8e8d4be to 0e747a4 Compare March 16, 2025 18:16

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 16, 2025

athiruma force-pushed the fix/market-type branch from 966755e to 95dfbed Compare March 17, 2025 04:42

JoelSpeed reviewed Mar 17, 2025

View reviewed changes

athiruma force-pushed the fix/market-type branch from 317b8f1 to 3f4c874 Compare March 23, 2025 14:34

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 23, 2025

athiruma force-pushed the fix/market-type branch from 022d46f to 8a46c2b Compare March 29, 2025 14:00

damdo reviewed Apr 3, 2025

View reviewed changes

api/v1beta2/awsmachine_types.go Outdated Show resolved Hide resolved

Updated the comments

22650ee

athiruma force-pushed the fix/market-type branch from 8a46c2b to 22650ee Compare April 3, 2025 08:03

k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Apr 3, 2025

damdo reviewed Apr 3, 2025

View reviewed changes

k8s-ci-robot assigned AndiDog, Ankitasw, dlipovetsky, nrb, richardcase and damdo Apr 3, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 3, 2025

richardcase reviewed Apr 4, 2025

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 5, 2025

k8s-ci-robot merged commit e5b01de into kubernetes-sigs:main Apr 5, 2025
18 checks passed

damdo mentioned this pull request Jun 5, 2025

OCPBUGS-61044,OCPBUGS-60943,OCPBUGS-61040: Merge https://github.com/kubernetes-sigs/cluster-api-provider-aws:v2.9.0 (dcfaef0) into main openshift/cluster-api-provider-aws#553

Merged

🐛 add marketType field validation #5374

🐛 add marketType field validation #5374

Uh oh!

Conversation

athiruma commented Mar 6, 2025

Uh oh!

k8s-ci-robot commented Mar 6, 2025

Uh oh!

alexander-demicev commented Mar 6, 2025

Uh oh!

athiruma commented Mar 12, 2025

Uh oh!

JoelSpeed commented Mar 12, 2025

Uh oh!

athiruma commented Mar 12, 2025

Uh oh!

JoelSpeed commented Mar 12, 2025

Uh oh!

damdo commented Mar 15, 2025

Uh oh!

JoelSpeed Mar 17, 2025

Choose a reason for hiding this comment

Uh oh!

athiruma commented Mar 21, 2025

Uh oh!

damdo commented Mar 22, 2025

Uh oh!

damdo commented Mar 23, 2025

Uh oh!

athiruma commented Mar 25, 2025

Uh oh!

damdo commented Mar 26, 2025

Uh oh!

damdo commented Mar 29, 2025

Uh oh!

athiruma commented Apr 3, 2025

Uh oh!

damdo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

damdo commented Apr 3, 2025

Uh oh!

damdo left a comment

Choose a reason for hiding this comment

Uh oh!

richardcase Apr 4, 2025

Choose a reason for hiding this comment

Uh oh!

athiruma Apr 4, 2025

Choose a reason for hiding this comment

Uh oh!

JoelSpeed Apr 4, 2025

Choose a reason for hiding this comment

Uh oh!

damdo Apr 4, 2025

Choose a reason for hiding this comment

Uh oh!

richardcase Apr 4, 2025

Choose a reason for hiding this comment

Uh oh!

damdo Apr 4, 2025

Choose a reason for hiding this comment

Uh oh!

richardcase commented Apr 5, 2025

Uh oh!

k8s-ci-robot commented Apr 5, 2025

Uh oh!

Uh oh!

damdo commented Apr 9, 2025

Uh oh!

k8s-infra-cherrypick-robot commented Apr 9, 2025

Uh oh!

damdo commented Jun 5, 2025

Uh oh!

k8s-infra-cherrypick-robot commented Jun 5, 2025

Uh oh!

Reviewers

Assignees

Labels