fix: Adjust duration validation to include format for `1h10m` #1125

engedaam · 2024-03-21T17:31:14Z

Fixes aws/karpenter-provider-aws#5913

Description

We should be able to create a budget with a duration format of 1h10m

How was this change tested?

N/A

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

njtran · 2024-03-21T17:45:41Z

pkg/apis/v1beta1/nodepool.go

@@ -124,7 +124,7 @@ type Budget struct {
 	// This is required if Schedule is set.
 	// This regex has an optional 0s at the end since the duration.String() always adds
 	// a 0s at the end.
-	// +kubebuilder:validation:Pattern=`^([0-9]+(m|h)+(0s)?)$`
+	// +kubebuilder:validation:Pattern=`^(([0-9]+(h|m)(0s)?)|([0-9]+h+[0-9]+m+(0s)?))$`


I think there only needs to be one 0s at the end of the whole string, right before the $. It also seems like this means I could do 1h2h3m4m, which I don't think is what we want to do.

Suggested change

// +kubebuilder:validation:Pattern=`^(([0-9]+(h|m)(0s)?)|([0-9]+h+[0-9]+m+(0s)?))$`

// +kubebuilder:validation:Pattern=`^(([0-9]+(h|m)(0s)?)|([0-9]+h+[0-9]+m+(0s)?))$`

This should not allow for that kind of format. We are only allowing 1h/1m or 1h1m

jonathan-innis · 2024-03-21T21:07:47Z

pkg/apis/crds/karpenter.sh_nodepools.yaml

@@ -80,7 +80,7 @@ spec:
                              This is required if Schedule is set.
                              This regex has an optional 0s at the end since the duration.String() always adds
                              a 0s at the end.
-                            pattern: ^([0-9]+(m|h)+(0s)?)$
+                            pattern: ^((([0-9]+(h|m))|([0-9]+h[0-9]+m))(0s)?)$


Is there a simpler way to write this pattern? I could imagine that you could do something like ([1-9](h|m|s)+)|(0s)

The format you suggested would allow 1h2m1h yo happen

We also don't want to allow seconds, since this breaches the smallest denominator value of the schedule, which is minutes

I could understand disallowing a leading 0 on the front of the duration, but I don't think it's a big deal, since they should really be treated the exact same. Disallowing it means that users can't do something like 0h5m or 0h or 0m or 5h0m. These are all weird in that there's better ways to define it, but it's up to us on how we want to allow/disallow this.

Yep, makes sense. I just looked over it quick. Current regex makes sense 👍🏼

coveralls · 2024-03-21T21:08:30Z

Pull Request Test Coverage Report for Build 8382299321

Details

3 of 5 (60.0%) changed or added relevant lines in 1 file are covered.
2 unchanged lines in 1 file lost coverage.
Overall coverage decreased (-0.009%) to 78.993%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/apis/v1beta1/nodepool_validation.go	3	5	60.0%

Files with Coverage Reduction	New Missed Lines	%
pkg/test/expectations/expectations.go	2	95.59%

Totals
Change from base Build 8366597003:	-0.009%
Covered Lines:	8265
Relevant Lines:	10463

💛 - Coveralls

njtran · 2024-03-21T21:19:11Z

pkg/apis/crds/karpenter.sh_nodepools.yaml

@@ -80,7 +80,7 @@ spec:
                              This is required if Schedule is set.
                              This regex has an optional 0s at the end since the duration.String() always adds
                              a 0s at the end.
-                            pattern: ^([0-9]+(m|h)+(0s)?)$
+                            pattern: ^((([0-9]+(h|m))|([0-9]+h[0-9]+m))(0s)?)$


We also don't want to allow seconds, since this breaches the smallest denominator value of the schedule, which is minutes

njtran · 2024-03-21T21:20:33Z

pkg/apis/crds/karpenter.sh_nodepools.yaml

@@ -80,7 +80,7 @@ spec:
                              This is required if Schedule is set.
                              This regex has an optional 0s at the end since the duration.String() always adds
                              a 0s at the end.
-                            pattern: ^([0-9]+(m|h)+(0s)?)$
+                            pattern: ^((([0-9]+(h|m))|([0-9]+h[0-9]+m))(0s)?)$


I could understand disallowing a leading 0 on the front of the duration, but I don't think it's a big deal, since they should really be treated the exact same. Disallowing it means that users can't do something like 0h5m or 0h or 0m or 5h0m. These are all weird in that there's better ways to define it, but it's up to us on how we want to allow/disallow this.

pkg/apis/v1beta1/nodepool_validation_cel_test.go

pkg/apis/v1beta1/nodepool_validation_webhook_test.go

Bryce-Soghigian · 2024-03-22T08:12:34Z

pkg/apis/v1beta1/nodepool.go

@@ -124,7 +124,7 @@ type Budget struct {
 	// This is required if Schedule is set.
 	// This regex has an optional 0s at the end since the duration.String() always adds
 	// a 0s at the end.
-	// +kubebuilder:validation:Pattern=`^([0-9]+(m|h)+(0s)?)$`
+	// +kubebuilder:validation:Pattern=`^((([0-9]+(h|m))|([0-9]+h[0-9]+m))(0s)?)$`


Devils advocate: can't we just lower the unit we are using? 1h10m is just 70m. Maybe it gets more complex with 12d10m vs 730m.

I commonly see

consolidateAfter: 300s consolidationPolicy: WhenEmpty expireAfter: 720h

If we do move to a 1h10m pattern, we should probably do it everywhere to be consistent. here for example we follow a different pattern. It will be fairly confusing for users to see they can set 1h10m in one place but not everywhere

This PR would allow both to happen. We currently promise both format to customer and this is addressing a bug

It seems in your link @Bryce-Soghigian that the only difference in these two regexes is the order. Seems compound durations are enabled there as well.

njtran

/lgtm
/approve

njtran · 2024-03-22T19:27:25Z

pkg/apis/v1beta1/nodepool.go

@@ -124,7 +124,7 @@ type Budget struct {
 	// This is required if Schedule is set.
 	// This regex has an optional 0s at the end since the duration.String() always adds
 	// a 0s at the end.
-	// +kubebuilder:validation:Pattern=`^([0-9]+(m|h)+(0s)?)$`
+	// +kubebuilder:validation:Pattern=`^((([0-9]+(h|m))|([0-9]+h[0-9]+m))(0s)?)$`


It seems in your link @Bryce-Soghigian that the only difference in these two regexes is the order. Seems compound durations are enabled there as well.

k8s-ci-robot · 2024-03-22T19:27:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: engedaam, njtran

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~~ [njtran]

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

…etes-sigs#1125)

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 21, 2024

k8s-ci-robot requested review from jmdeal and tallaxes March 21, 2024 17:31

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

njtran reviewed Mar 21, 2024

View reviewed changes

engedaam force-pushed the fix-duration branch 5 times, most recently from bf28f09 to f247f26 Compare March 21, 2024 21:04

jonathan-innis reviewed Mar 21, 2024

View reviewed changes

njtran reviewed Mar 21, 2024

View reviewed changes

Fix duration validation

9fafe87

engedaam force-pushed the fix-duration branch from f247f26 to 9fafe87 Compare March 21, 2024 22:02

Bryce-Soghigian reviewed Mar 22, 2024

View reviewed changes

njtran approved these changes Mar 22, 2024

View reviewed changes

k8s-ci-robot assigned njtran Mar 22, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 22, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 22, 2024

k8s-ci-robot merged commit d1d94a3 into kubernetes-sigs:main Mar 22, 2024
12 checks passed

engedaam mentioned this pull request Mar 22, 2024

Disruption Duration validation regex is wrong aws/karpenter-provider-aws#5913

Closed

engedaam added a commit to engedaam/karpenter-core that referenced this pull request Mar 25, 2024

fix: Adjust duration validation to include format for 1h10m (kubern…

8a7c20b

…etes-sigs#1125)

engedaam added a commit to engedaam/karpenter-core that referenced this pull request Mar 25, 2024

fix: Adjust duration validation to include format for 1h10m (kubern…

88c6745

…etes-sigs#1125)

engedaam added a commit to engedaam/karpenter-core that referenced this pull request Mar 25, 2024

fix: Adjust duration validation to include format for 1h10m (kubern…

58f9517

…etes-sigs#1125)

engedaam added a commit to engedaam/karpenter-core that referenced this pull request Mar 25, 2024

fix: Adjust duration validation to include format for 1h10m (kubern…

3171eca

…etes-sigs#1125)

This was referenced Mar 25, 2024

cherrypick(v0.35): fix: Adjust duration validation to include format for 1h10m #1136

Merged

cherrypick(v0.34): fix: Adjust duration validation to include format for 1h10m #1137

Merged

engedaam deleted the fix-duration branch April 2, 2024 17:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Adjust duration validation to include format for `1h10m` #1125

fix: Adjust duration validation to include format for `1h10m` #1125

engedaam commented Mar 21, 2024 •

edited

njtran Mar 21, 2024

engedaam Mar 21, 2024

jonathan-innis Mar 21, 2024

engedaam Mar 21, 2024

njtran Mar 21, 2024

njtran Mar 21, 2024

jonathan-innis Mar 22, 2024

coveralls commented Mar 21, 2024 •

edited

njtran Mar 21, 2024

njtran Mar 21, 2024

Bryce-Soghigian Mar 22, 2024

engedaam Mar 22, 2024

njtran Mar 22, 2024

njtran left a comment

njtran Mar 22, 2024

k8s-ci-robot commented Mar 22, 2024

	// +kubebuilder:validation:Pattern=`^(([0-9]+(h\|m)(0s)?)\|([0-9]+h+[0-9]+m+(0s)?))$`
	// +kubebuilder:validation:Pattern=`^(([0-9]+(h\|m)(0s)?)\|([0-9]+h+[0-9]+m+(0s)?))$`

fix: Adjust duration validation to include format for 1h10m #1125

fix: Adjust duration validation to include format for 1h10m #1125

Conversation

engedaam commented Mar 21, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Mar 21, 2024 • edited

Pull Request Test Coverage Report for Build 8382299321

Details

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njtran left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Mar 22, 2024

fix: Adjust duration validation to include format for `1h10m` #1125

fix: Adjust duration validation to include format for `1h10m` #1125

engedaam commented Mar 21, 2024 •

edited

coveralls commented Mar 21, 2024 •

edited