fix: update CEL validation for expireAfter and consolidateAfter field…

…s to block invalid values like 1hr
kubernetes-sigs · Mar 6, 2025 · e0e2ece · e0e2ece
1 parent af2b998
commit e0e2ece
Show file tree

Hide file tree

Showing 8 changed files with 64 additions and 30 deletions.
diff --git a/kwok/charts/crds/karpenter.sh_nodeclaims.yaml b/kwok/charts/crds/karpenter.sh_nodeclaims.yaml
@@ -83,7 +83,7 @@ spec:
                     before terminating a node, measured from when the node is created. This
                     is useful to implement features like eventually consistent node upgrade,
                     memory leak protection, and disruption testing.
-                  pattern: ^(([0-9]+(s|m|h))+)|(Never)$
+                  pattern: ^(([0-9]+(s|m|h))+|Never)$
                   type: string
                 nodeClassRef:
                   description: NodeClassRef is a reference to an object that defines provider specific configuration

diff --git a/kwok/charts/crds/karpenter.sh_nodepools.yaml b/kwok/charts/crds/karpenter.sh_nodepools.yaml
@@ -144,7 +144,7 @@ spec:
                         ConsolidateAfter is the duration the controller will wait
                         before attempting to terminate nodes that are underutilized.
                         Refer to ConsolidationPolicy for how underutilization is considered.
-                      pattern: ^(([0-9]+(s|m|h))+)|(Never)$
+                      pattern: ^(([0-9]+(s|m|h))+|Never)$
                       type: string
                     consolidationPolicy:
                       default: WhenEmptyOrUnderutilized
@@ -220,7 +220,7 @@ spec:
                             before terminating a node, measured from when the node is created. This
                             is useful to implement features like eventually consistent node upgrade,
                             memory leak protection, and disruption testing.
-                          pattern: ^(([0-9]+(s|m|h))+)|(Never)$
+                          pattern: ^(([0-9]+(s|m|h))+|Never)$
                           type: string
                         nodeClassRef:
                           description: NodeClassRef is a reference to an object that defines provider specific configuration

diff --git a/pkg/apis/crds/karpenter.sh_nodeclaims.yaml b/pkg/apis/crds/karpenter.sh_nodeclaims.yaml
@@ -83,7 +83,7 @@ spec:
                     before terminating a node, measured from when the node is created. This
                     is useful to implement features like eventually consistent node upgrade,
                     memory leak protection, and disruption testing.
-                  pattern: ^(([0-9]+(s|m|h))+)|(Never)$
+                  pattern: ^(([0-9]+(s|m|h))+|Never)$
                   type: string
                 nodeClassRef:
                   description: NodeClassRef is a reference to an object that defines provider specific configuration

diff --git a/pkg/apis/crds/karpenter.sh_nodepools.yaml b/pkg/apis/crds/karpenter.sh_nodepools.yaml
@@ -144,7 +144,7 @@ spec:
                         ConsolidateAfter is the duration the controller will wait
                         before attempting to terminate nodes that are underutilized.
                         Refer to ConsolidationPolicy for how underutilization is considered.
-                      pattern: ^(([0-9]+(s|m|h))+)|(Never)$
+                      pattern: ^(([0-9]+(s|m|h))+|Never)$
                       type: string
                     consolidationPolicy:
                       default: WhenEmptyOrUnderutilized
@@ -220,7 +220,7 @@ spec:
                             before terminating a node, measured from when the node is created. This
                             is useful to implement features like eventually consistent node upgrade,
                             memory leak protection, and disruption testing.
-                          pattern: ^(([0-9]+(s|m|h))+)|(Never)$
+                          pattern: ^(([0-9]+(s|m|h))+|Never)$
                           type: string
                         nodeClassRef:
                           description: NodeClassRef is a reference to an object that defines provider specific configuration

diff --git a/pkg/apis/v1/nodeclaim.go b/pkg/apis/v1/nodeclaim.go
@@ -69,7 +69,7 @@ type NodeClaimSpec struct {
 	// is useful to implement features like eventually consistent node upgrade,
 	// memory leak protection, and disruption testing.
 	// +kubebuilder:default:="720h"
-	// +kubebuilder:validation:Pattern=`^(([0-9]+(s|m|h))+)|(Never)$`
+	// +kubebuilder:validation:Pattern=`^(([0-9]+(s|m|h))+|Never)$`
 	// +kubebuilder:validation:Type="string"
 	// +kubebuilder:validation:Schemaless
 	// +optional

diff --git a/pkg/apis/v1/nodeclaim_validation_cel_test.go b/pkg/apis/v1/nodeclaim_validation_cel_test.go
@@ -244,13 +244,34 @@ var _ = Describe("Validation", func() {
 		})
 	})
 	Context("TerminationGracePeriod", func() {
-		It("should succeed on a positive terminationGracePeriod duration", func() {
-			nodeClaim.Spec.TerminationGracePeriod = &metav1.Duration{Duration: time.Second * 300}
+		DescribeTable("should succeed on a valid terminationGracePeriod", func(value string) {
+			nodeClaim.Spec.TerminationGracePeriod = &metav1.Duration{Duration: lo.Must(time.ParseDuration(value))}
 			Expect(env.Client.Create(ctx, nodeClaim)).To(Succeed())
-		})
-		It("should fail on a negative terminationGracePeriod duration", func() {
-			nodeClaim.Spec.TerminationGracePeriod = &metav1.Duration{Duration: time.Second * -30}
-			Expect(env.Client.Create(ctx, nodeClaim)).ToNot(Succeed())
-		})
+		},
+			Entry("single unit", "30s"),
+			Entry("multiple units", "1h30m5s"),
+		)
+		DescribeTable("should fail on an invalid terminationGracePeriod", func(value string) {
+			nodeClaim.Spec.TerminationGracePeriod = &metav1.Duration{Duration: lo.Must(time.ParseDuration(value))}
+			Expect(env.Client.Create(ctx, nodeClaim)).To(Not(Succeed()))
+		},
+			Entry("negative", "-1s"),
+		)
+	})
+	Context("ExpireAfter", func() {
+		DescribeTable("should succeed on a valid expireAfter", func(value string) {
+			nodeClaim.Spec.ExpireAfter = MustParseNillableDuration(value)
+			Expect(env.Client.Create(ctx, nodeClaim)).To(Succeed())
+		},
+			Entry("single unit", "30s"),
+			Entry("multiple units", "1h30m5s"),
+			Entry("never", "Never"),
+		)
+		DescribeTable("should fail on an invalid expireAfter", func(value string) {
+			nodeClaim.Spec.ExpireAfter = MustParseNillableDuration(value)
+			Expect(env.Client.Create(ctx, nodeClaim)).To(Not(Succeed()))
+		},
+			Entry("negative", "-1s"),
+		)
 	})
 })
diff --git a/pkg/apis/v1/nodepool.go b/pkg/apis/v1/nodepool.go
@@ -61,7 +61,7 @@ type Disruption struct {
 	// ConsolidateAfter is the duration the controller will wait
 	// before attempting to terminate nodes that are underutilized.
 	// Refer to ConsolidationPolicy for how underutilization is considered.
-	// +kubebuilder:validation:Pattern=`^(([0-9]+(s|m|h))+)|(Never)$`
+	// +kubebuilder:validation:Pattern=`^(([0-9]+(s|m|h))+|Never)$`
 	// +kubebuilder:validation:Type="string"
 	// +kubebuilder:validation:Schemaless
 	// +required
@@ -206,7 +206,7 @@ type NodeClaimTemplateSpec struct {
 	// is useful to implement features like eventually consistent node upgrade,
 	// memory leak protection, and disruption testing.
 	// +kubebuilder:default:="720h"
-	// +kubebuilder:validation:Pattern=`^(([0-9]+(s|m|h))+)|(Never)$`
+	// +kubebuilder:validation:Pattern=`^(([0-9]+(s|m|h))+|Never)$`
 	// +kubebuilder:validation:Type="string"
 	// +kubebuilder:validation:Schemaless
 	// +optional

diff --git a/pkg/apis/v1/nodepool_validation_cel_test.go b/pkg/apis/v1/nodepool_validation_cel_test.go
@@ -64,31 +64,44 @@ var _ = Describe("CEL/Validation", func() {
 		}
 	})
 	Context("Disruption", func() {
-		It("should fail on negative expireAfter", func() {
-			nodePool.Spec.Template.Spec.ExpireAfter = MustParseNillableDuration("-1s")
-			Expect(env.Client.Create(ctx, nodePool)).ToNot(Succeed())
-		})
 		It("should succeed on a disabled expireAfter", func() {
 			nodePool.Spec.Template.Spec.ExpireAfter = MustParseNillableDuration("Never")
 			Expect(env.Client.Create(ctx, nodePool)).To(Succeed())
 		})
-		It("should succeed on a valid expireAfter", func() {
-			nodePool.Spec.Template.Spec.ExpireAfter = MustParseNillableDuration("30s")
+		DescribeTable("should succeed on a valid expireAfter", func(value string) {
+			nodePool.Spec.Template.Spec.ExpireAfter = MustParseNillableDuration(value)
 			Expect(env.Client.Create(ctx, nodePool)).To(Succeed())
-		})
-		It("should fail on negative consolidateAfter", func() {
-			nodePool.Spec.Disruption.ConsolidateAfter = MustParseNillableDuration("-1s")
-			Expect(env.Client.Create(ctx, nodePool)).ToNot(Succeed())
-		})
+		},
+			Entry("single unit", "30s"),
+			Entry("multiple units", "1h30m5s"),
+			Entry("never", "Never"),
+		)
+		DescribeTable("should fail on an invalid expireAfter", func(value string) {
+			nodePool.Spec.Template.Spec.ExpireAfter = MustParseNillableDuration(value)
+			Expect(env.Client.Create(ctx, nodePool)).To(Not(Succeed()))
+		},
+			Entry("negative", "-1s"),
+		)
 		It("should succeed on a disabled consolidateAfter", func() {
 			nodePool.Spec.Disruption.ConsolidateAfter = MustParseNillableDuration("Never")
 			Expect(env.Client.Create(ctx, nodePool)).To(Succeed())
 		})
-		It("should succeed on a valid consolidateAfter", func() {
-			nodePool.Spec.Disruption.ConsolidateAfter = MustParseNillableDuration("30s")
+		DescribeTable("should succeed on a valid consolidateAfter", func(value string) {
+			nodePool.Spec.Disruption.ConsolidateAfter = MustParseNillableDuration(value)
 			nodePool.Spec.Disruption.ConsolidationPolicy = ConsolidationPolicyWhenEmpty
 			Expect(env.Client.Create(ctx, nodePool)).To(Succeed())
-		})
+		},
+			Entry("single unit", "30s"),
+			Entry("multiple units", "1h30m5s"),
+			Entry("never", "Never"),
+		)
+		DescribeTable("should fail on an invalid expireAfter", func(value string) {
+			nodePool.Spec.Disruption.ConsolidateAfter = MustParseNillableDuration(value)
+			nodePool.Spec.Disruption.ConsolidationPolicy = ConsolidationPolicyWhenEmpty
+			Expect(env.Client.Create(ctx, nodePool)).To(Not(Succeed()))
+		},
+			Entry("negative", "-1s"),
+		)
 		It("should succeed when setting consolidateAfter with consolidationPolicy=WhenEmpty", func() {
 			nodePool.Spec.Disruption.ConsolidateAfter = MustParseNillableDuration("30s")
 			nodePool.Spec.Disruption.ConsolidationPolicy = ConsolidationPolicyWhenEmpty