From 3171ecac9da6732d273afce6a98117a1033ea6c6 Mon Sep 17 00:00:00 2001
From: Amanuel Engeda <74629455+engedaam@users.noreply.github.com>
Date: Fri, 22 Mar 2024 12:30:48 -0700
Subject: [PATCH] fix: Adjust duration validation to include format for `1h10m`
 (#1125)

---
 pkg/apis/crds/karpenter.sh_nodepools.yaml            |  2 +-
 pkg/apis/v1beta1/nodepool.go                         |  2 +-
 pkg/apis/v1beta1/nodepool_validation_cel_test.go     | 10 +++++++++-
 pkg/apis/v1beta1/nodepool_validation_webhook_test.go | 10 +++++++++-
 4 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/pkg/apis/crds/karpenter.sh_nodepools.yaml b/pkg/apis/crds/karpenter.sh_nodepools.yaml
index b1abb94bc7..7d7cf0d60a 100644
--- a/pkg/apis/crds/karpenter.sh_nodepools.yaml
+++ b/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)?)$
                             type: string
                           nodes:
                             default: 10%
diff --git a/pkg/apis/v1beta1/nodepool.go b/pkg/apis/v1beta1/nodepool.go
index 796190439a..79109d586d 100644
--- a/pkg/apis/v1beta1/nodepool.go
+++ b/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)?)$`
 	// +kubebuilder:validation:Type="string"
 	// +optional
 	Duration *metav1.Duration `json:"duration,omitempty" hash:"ignore"`
diff --git a/pkg/apis/v1beta1/nodepool_validation_cel_test.go b/pkg/apis/v1beta1/nodepool_validation_cel_test.go
index 263146284a..79c5e7e14a 100644
--- a/pkg/apis/v1beta1/nodepool_validation_cel_test.go
+++ b/pkg/apis/v1beta1/nodepool_validation_cel_test.go
@@ -165,7 +165,7 @@ var _ = Describe("CEL/Validation", func() {
 		It("should fail when creating a budget with a duration but no cron", func() {
 			nodePool.Spec.Disruption.Budgets = []Budget{{
 				Nodes:    "10",
-				Duration: &metav1.Duration{Duration: lo.Must(time.ParseDuration("-20m"))},
+				Duration: &metav1.Duration{Duration: lo.Must(time.ParseDuration("20m"))},
 			}}
 			Expect(env.Client.Create(ctx, nodePool)).ToNot(Succeed())
 		})
@@ -177,6 +177,14 @@ var _ = Describe("CEL/Validation", func() {
 			}}
 			Expect(env.Client.Create(ctx, nodePool)).To(Succeed())
 		})
+		It("should succeed when creating a budget with hours and minutes in duration", func() {
+			nodePool.Spec.Disruption.Budgets = []Budget{{
+				Nodes:    "10",
+				Schedule: ptr.String("* * * * *"),
+				Duration: &metav1.Duration{Duration: lo.Must(time.ParseDuration("2h20m"))},
+			}}
+			Expect(env.Client.Create(ctx, nodePool)).To(Succeed())
+		})
 		It("should succeed when creating a budget with neither duration nor cron", func() {
 			nodePool.Spec.Disruption.Budgets = []Budget{{
 				Nodes: "10",
diff --git a/pkg/apis/v1beta1/nodepool_validation_webhook_test.go b/pkg/apis/v1beta1/nodepool_validation_webhook_test.go
index a4b024483e..b46e7cdcd0 100644
--- a/pkg/apis/v1beta1/nodepool_validation_webhook_test.go
+++ b/pkg/apis/v1beta1/nodepool_validation_webhook_test.go
@@ -105,7 +105,7 @@ var _ = Describe("Webhook/Validation", func() {
 		It("should fail to validate a budget with a duration but no cron", func() {
 			nodePool.Spec.Disruption.Budgets = []Budget{{
 				Nodes:    "10",
-				Duration: &metav1.Duration{Duration: lo.Must(time.ParseDuration("-20m"))},
+				Duration: &metav1.Duration{Duration: lo.Must(time.ParseDuration("20m"))},
 			}}
 			Expect(nodePool.Validate(ctx)).ToNot(Succeed())
 		})
@@ -131,6 +131,14 @@ var _ = Describe("Webhook/Validation", func() {
 			}}
 			Expect(nodePool.Validate(ctx)).To(Succeed())
 		})
+		It("should succeed when creating a budget with hours and minutes in duration", func() {
+			nodePool.Spec.Disruption.Budgets = []Budget{{
+				Nodes:    "10",
+				Schedule: ptr.String("* * * * *"),
+				Duration: &metav1.Duration{Duration: lo.Must(time.ParseDuration("2h20m"))},
+			}}
+			Expect(nodePool.Validate(ctx)).To(Succeed())
+		})
 		It("should fail when creating two budgets where one has an invalid crontab", func() {
 			nodePool.Spec.Disruption.Budgets = []Budget{
 				{