Skip to content
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

fix: update CEL validation for expireAfter and consolidateAfter fields to block invalid values like 1hr #2055

Merged
merged 1 commit into from
Mar 7, 2025
Merged

fix: update CEL validation for expireAfter and consolidateAfter fields to block invalid values like 1hr #2055

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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: 1 addition & 1 deletion kwok/charts/crds/karpenter.sh_nodeclaims.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions kwok/charts/crds/karpenter.sh_nodepools.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion pkg/apis/crds/karpenter.sh_nodeclaims.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions pkg/apis/crds/karpenter.sh_nodepools.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion pkg/apis/v1/nodeclaim.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
62 changes: 53 additions & 9 deletions pkg/apis/v1/nodeclaim_validation_cel_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ package v1_test
import (
"strconv"
"strings"
"time"

"github.com/Pallinder/go-randomdata"
"github.com/awslabs/operatorpkg/object"
Expand All @@ -28,6 +27,8 @@ import (
"github.com/samber/lo"
v1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/util/sets"

. "sigs.k8s.io/karpenter/pkg/apis/v1"
Expand Down Expand Up @@ -60,6 +61,7 @@ var _ = Describe("Validation", func() {
},
},
}
nodeClaim.SetGroupVersionKind(object.GVK(nodeClaim)) // This is needed so that the GVK is set on the unstructured object
})

Context("Taints", func() {
Expand Down Expand Up @@ -244,13 +246,55 @@ 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}
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())
})
DescribeTable("should succeed on a valid terminationGracePeriod", func(value string) {
u := lo.Must(runtime.DefaultUnstructuredConverter.ToUnstructured(nodeClaim))
lo.Must0(unstructured.SetNestedField(u, value, "spec", "terminationGracePeriod"))
obj := &unstructured.Unstructured{}
lo.Must0(runtime.DefaultUnstructuredConverter.FromUnstructured(u, obj))

Expect(env.Client.Create(ctx, obj)).To(Succeed())
},
Entry("single unit", "30s"),
Entry("multiple units", "1h30m5s"),
)
DescribeTable("should fail on an invalid terminationGracePeriod", func(value string) {
u := lo.Must(runtime.DefaultUnstructuredConverter.ToUnstructured(nodeClaim))
lo.Must0(unstructured.SetNestedField(u, value, "spec", "terminationGracePeriod"))
obj := &unstructured.Unstructured{}
lo.Must0(runtime.DefaultUnstructuredConverter.FromUnstructured(u, obj))

Expect(env.Client.Create(ctx, obj)).To(Not(Succeed()))
},
Entry("negative", "-1s"),
Entry("invalid unit", "1hr"),
Entry("never", "Never"),
Entry("partial match", "FooNever"),
)
})
Context("ExpireAfter", func() {
DescribeTable("should succeed on a valid expireAfter", func(value string) {
u := lo.Must(runtime.DefaultUnstructuredConverter.ToUnstructured(nodeClaim))
lo.Must0(unstructured.SetNestedField(u, value, "spec", "expireAfter"))
obj := &unstructured.Unstructured{}
lo.Must0(runtime.DefaultUnstructuredConverter.FromUnstructured(u, obj))

Expect(env.Client.Create(ctx, obj)).To(Succeed())
},
Entry("single unit", "30s"),
Entry("multiple units", "1h30m5s"),
Entry("never", "Never"),
)
DescribeTable("should fail on an invalid expireAfter", func(value string) {
u := lo.Must(runtime.DefaultUnstructuredConverter.ToUnstructured(nodeClaim))
lo.Must0(unstructured.SetNestedField(u, value, "spec", "expireAfter"))
obj := &unstructured.Unstructured{}
lo.Must0(runtime.DefaultUnstructuredConverter.FromUnstructured(u, obj))

Expect(env.Client.Create(ctx, obj)).To(Not(Succeed()))
},
Entry("negative", "-1s"),
Entry("invalid unit", "1hr"),
Entry("partial match", "FooNever"),
)
})
})
4 changes: 2 additions & 2 deletions pkg/apis/v1/nodepool.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
102 changes: 77 additions & 25 deletions pkg/apis/v1/nodepool_validation_cel_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,14 @@ import (
"time"

"github.com/Pallinder/go-randomdata"
"github.com/awslabs/operatorpkg/object"
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
"github.com/samber/lo"
v1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/util/sets"

. "sigs.k8s.io/karpenter/pkg/apis/v1"
Expand Down Expand Up @@ -62,33 +65,66 @@ var _ = Describe("CEL/Validation", func() {
},
},
}
nodePool.SetGroupVersionKind(object.GVK(nodePool)) // This is needed so that the GVK is set on the unstructured object
})
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")
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())
})
DescribeTable("should succeed on a valid expireAfter", func(value string) {
u := lo.Must(runtime.DefaultUnstructuredConverter.ToUnstructured(nodePool))
lo.Must0(unstructured.SetNestedField(u, value, "spec", "template", "spec", "expireAfter"))
obj := &unstructured.Unstructured{}
obj.SetGroupVersionKind(nodePool.GroupVersionKind())
lo.Must0(runtime.DefaultUnstructuredConverter.FromUnstructured(u, obj))

Expect(env.Client.Create(ctx, obj)).To(Succeed())
},
Entry("single unit", "30s"),
Entry("multiple units", "1h30m5s"),
Entry("never", "Never"),
)
DescribeTable("should fail on an invalid expireAfter", func(value string) {
u := lo.Must(runtime.DefaultUnstructuredConverter.ToUnstructured(nodePool))
lo.Must0(unstructured.SetNestedField(u, value, "spec", "template", "spec", "expireAfter"))
obj := &unstructured.Unstructured{}
lo.Must0(runtime.DefaultUnstructuredConverter.FromUnstructured(u, obj))

Expect(env.Client.Create(ctx, obj)).To(Not(Succeed()))
},
Entry("negative", "-1s"),
Entry("invalid unit", "1hr"),
Entry("partial match", "FooNever"),
)
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")
nodePool.Spec.Disruption.ConsolidationPolicy = ConsolidationPolicyWhenEmpty
Expect(env.Client.Create(ctx, nodePool)).To(Succeed())
})
DescribeTable("should succeed on a valid consolidateAfter", func(value string) {
u := lo.Must(runtime.DefaultUnstructuredConverter.ToUnstructured(nodePool))
lo.Must0(unstructured.SetNestedField(u, value, "spec", "disruption", "consolidateAfter"))
obj := &unstructured.Unstructured{}
lo.Must0(runtime.DefaultUnstructuredConverter.FromUnstructured(u, obj))

Expect(env.Client.Create(ctx, obj)).To(Succeed())
},
Entry("single unit", "30s"),
Entry("multiple units", "1h30m5s"),
Entry("never", "Never"),
)
DescribeTable("should fail on an invalid consolidateAfter", func(value string) {
u := lo.Must(runtime.DefaultUnstructuredConverter.ToUnstructured(nodePool))
lo.Must0(unstructured.SetNestedField(u, value, "spec", "disruption", "consolidateAfter"))
obj := &unstructured.Unstructured{}
lo.Must0(runtime.DefaultUnstructuredConverter.FromUnstructured(u, obj))

Expect(env.Client.Create(ctx, obj)).To(Not(Succeed()))
},
Entry("negative", "-1s"),
Entry("invalid unit", "1hr"),
Entry("partial match", "FooNever"),
)
It("should succeed when setting consolidateAfter with consolidationPolicy=WhenEmpty", func() {
nodePool.Spec.Disruption.ConsolidateAfter = MustParseNillableDuration("30s")
nodePool.Spec.Disruption.ConsolidationPolicy = ConsolidationPolicyWhenEmpty
Expand Down Expand Up @@ -611,14 +647,30 @@ var _ = Describe("CEL/Validation", func() {
})
})
Context("TerminationGracePeriod", func() {
It("should succeed on a positive terminationGracePeriod duration", func() {
nodePool.Spec.Template.Spec.TerminationGracePeriod = &metav1.Duration{Duration: time.Second * 300}
Expect(env.Client.Create(ctx, nodePool)).To(Succeed())
})
It("should fail on a negative terminationGracePeriod duration", func() {
nodePool.Spec.Template.Spec.TerminationGracePeriod = &metav1.Duration{Duration: time.Second * -30}
Expect(env.Client.Create(ctx, nodePool)).ToNot(Succeed())
})
DescribeTable("should succeed on a valid terminationGracePeriod", func(value string) {
u := lo.Must(runtime.DefaultUnstructuredConverter.ToUnstructured(nodePool))
lo.Must0(unstructured.SetNestedField(u, value, "spec", "template", "spec", "terminationGracePeriod"))
obj := &unstructured.Unstructured{}
lo.Must0(runtime.DefaultUnstructuredConverter.FromUnstructured(u, obj))

Expect(env.Client.Create(ctx, obj)).To(Succeed())
},
Entry("single unit", "30s"),
Entry("multiple units", "1h30m5s"),
)
DescribeTable("should fail on an invalid terminationGracePeriod", func(value string) {
u := lo.Must(runtime.DefaultUnstructuredConverter.ToUnstructured(nodePool))
lo.Must0(unstructured.SetNestedField(u, value, "spec", "template", "spec", "terminationGracePeriod"))
obj := &unstructured.Unstructured{}
lo.Must0(runtime.DefaultUnstructuredConverter.FromUnstructured(u, obj))

Expect(env.Client.Create(ctx, obj)).To(Not(Succeed()))
},
Entry("negative", "-1s"),
Entry("invalid unit", "1hr"),
Entry("never", "Never"),
Entry("partial match", "FooNever"),
)
})
Context("NodeClassRef", func() {
It("should fail to mutate group", func() {
Expand Down