Clean up and implement review comments

charts/karpenter/crds/karpenter.sh_provisioners.yaml

@@ -1,9 +1,10 @@
+
 ---
 apiVersion: apiextensions.k8s.io/v1
 kind: CustomResourceDefinition
 metadata:
   annotations:
-    controller-gen.kubebuilder.io/version: v0.8.0
+    controller-gen.kubebuilder.io/version: v0.7.0
   creationTimestamp: null
   name: provisioners.karpenter.sh
 spec:

pkg/apis/provisioning/v1alpha5/provisioner_validation.go

@@ -19,15 +19,16 @@ import (
 	"fmt"
 	"strings"
 
+	"github.com/aws/karpenter/pkg/utils/ptr"
 	v1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/apimachinery/pkg/util/validation"
 	"knative.dev/pkg/apis"
-
-	"github.com/aws/karpenter/pkg/utils/ptr"
 )
 
 var (
-	SupportedNodeSelectorOps = []string{string(v1.NodeSelectorOpIn), string(v1.NodeSelectorOpNotIn), string(v1.NodeSelectorOpExists), string(v1.NodeSelectorOpDoesNotExist)}
+	SupportedNodeSelectorOps sets.String = sets.NewString(string(v1.NodeSelectorOpIn), string(v1.NodeSelectorOpNotIn), string(v1.NodeSelectorOpExists), string(v1.NodeSelectorOpDoesNotExist))
+	SupportedProvisionerOps  sets.String = sets.NewString(string(v1.NodeSelectorOpIn), string(v1.NodeSelectorOpExists))
 )
 
 func (p *Provisioner) Validate(ctx context.Context) (errs *apis.FieldError) {
@@ -136,10 +137,14 @@ func (c *Constraints) validateTaints() (errs *apis.FieldError) {
 // When this function is called, the provisioner's requirments do not include the requirements from labels.
 // Provisioner requirements only support well known labels.
 func (c *Constraints) validateRequirements() (errs *apis.FieldError) {
-	// Ensure requirements are well known
-	for key := range c.Requirements.Keys() {
-		if !WellKnownLabels.Has(key) {
-			errs = errs.Also(apis.ErrInvalidKeyName(fmt.Sprintf("%s not in %v", key, WellKnownLabels.UnsortedList()), "key"))
+	for _, requirement := range c.Requirements.Requirements {
+		// Ensure requirements are well known
+		if !WellKnownLabels.Has(requirement.Key) {
+			errs = errs.Also(apis.ErrInvalidKeyName(fmt.Sprintf("%s not in %v", requirement.Key, WellKnownLabels.UnsortedList()), "key"))
+		}
+		// Ensure requirements operator is allowed
+		if !SupportedProvisionerOps.Has(string(requirement.Operator)) {
+			errs = errs.Also(apis.ErrInvalidKeyName(fmt.Sprintf("%s not in %v", requirement.Operator, SupportedProvisionerOps.UnsortedList()), "key"))
 		}
 	}
 	if err := c.Requirements.Validate(); err != nil {

pkg/apis/provisioning/v1alpha5/requirements.go

@@ -19,7 +19,6 @@ import (
 	"fmt"
 	"sort"
 
-	"github.com/aws/karpenter/pkg/utils/functional"
 	"github.com/aws/karpenter/pkg/utils/sets"
 	v1 "k8s.io/api/core/v1"
 	stringsets "k8s.io/apimachinery/pkg/util/sets"
@@ -74,11 +73,6 @@ var (
 		"beta.kubernetes.io/os":       v1.LabelOSStable,
 		v1.LabelInstanceType:          v1.LabelInstanceTypeStable,
 	}
-	// IgnoredLables are not considered in scheduling decisions
-	// and prevent validation errors when specified
-	IgnoredLabels = sets.NewString(
-		v1.LabelTopologyRegion,
-	)
 )
 
 // Requirements are an alias type that wrap []v1.NodeSelectorRequirement and
@@ -151,9 +145,9 @@ func (r Requirements) Add(requirements ...v1.NodeSelectorRequirement) Requiremen
 		r.Requirements = append(r.Requirements, requirement)
 		switch requirement.Operator {
 		case v1.NodeSelectorOpIn:
-			r.requirements[requirement.Key] = r.Values(requirement.Key).Intersection(sets.NewSet(requirement.Values...))
+			r.requirements[requirement.Key] = r.Get(requirement.Key).Intersection(sets.NewSet(requirement.Values...))
 		case v1.NodeSelectorOpNotIn:
-			r.requirements[requirement.Key] = r.Values(requirement.Key).Intersection(sets.NewComplementSet(requirement.Values...))
+			r.requirements[requirement.Key] = r.Get(requirement.Key).Intersection(sets.NewComplementSet(requirement.Values...))
 		}
 	}
 	return r
@@ -168,95 +162,99 @@ func (r Requirements) Keys() stringsets.String {
 	return keys
 }
 
-// Values returns the sets of values allowed by all included requirements
+// Get returns the sets of values allowed by all included requirements
 // following a denylist method. Values are allowed except specified
-func (r Requirements) Values(key string) sets.Set {
+func (r Requirements) Get(key string) sets.Set {
 	if _, ok := r.requirements[key]; !ok {
 		return sets.NewComplementSet()
 	}
 	return r.requirements[key]
 }
 
 func (r Requirements) Zones() stringsets.String {
-	return r.Values(v1.LabelTopologyZone).Values()
+	return r.Get(v1.LabelTopologyZone).Values()
 }
 
 func (r Requirements) InstanceTypes() stringsets.String {
-	return r.Values(v1.LabelInstanceTypeStable).Values()
+	return r.Get(v1.LabelInstanceTypeStable).Values()
 }
 
 func (r Requirements) Architectures() stringsets.String {
-	return r.Values(v1.LabelArchStable).Values()
+	return r.Get(v1.LabelArchStable).Values()
 }
 
 func (r Requirements) OperatingSystems() stringsets.String {
-	return r.Values(v1.LabelOSStable).Values()
+	return r.Get(v1.LabelOSStable).Values()
 }
 
 func (r Requirements) CapacityTypes() stringsets.String {
-	return r.Values(LabelCapacityType).Values()
+	return r.Get(LabelCapacityType).Values()
 }
 
 // Validate validates the feasibility of the requirements.
 func (r Requirements) Validate() (errs *apis.FieldError) {
 	for i, requirement := range r.Requirements {
 		for _, err := range validation.IsQualifiedName(requirement.Key) {
-			errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("%s, %s", requirement.Key, err), "key"))
+			errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("key %s, %s", requirement.Key, err), "key"))
 		}
 		for _, value := range requirement.Values {
 			for _, err := range validation.IsValidLabelValue(value) {
-				errs = errs.Also(apis.ErrInvalidArrayValue(fmt.Sprintf("%s, %s", value, err), "values", i))
+				errs = errs.Also(apis.ErrInvalidArrayValue(fmt.Sprintf("key %s, value %s, %s", requirement.Key, value, err), "values", i))
 			}
 		}
-		if !functional.ContainsString(SupportedNodeSelectorOps, string(requirement.Operator)) {
-			errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("%s not in %s", requirement.Operator, SupportedNodeSelectorOps), "operator"))
+		if !SupportedNodeSelectorOps.Has(string(requirement.Operator)) {
+			errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("%s not in %s for %s", requirement.Operator, SupportedNodeSelectorOps.UnsortedList(), requirement.Key), "operator"))
 		}
 		// Excludes cases when DoesNotExists appears together with In, NotIn, Exists
 		if requirement.Operator == v1.NodeSelectorOpDoesNotExist && (r.hasRequirement(withKeyAndOperator(requirement.Key, v1.NodeSelectorOpIn)) ||
 			r.hasRequirement(withKeyAndOperator(requirement.Key, v1.NodeSelectorOpNotIn)) ||
 			r.hasRequirement(withKeyAndOperator(requirement.Key, v1.NodeSelectorOpExists))) {
-			errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("operator %s and %s conflict", v1.NodeSelectorOpDoesNotExist, v1.NodeSelectorOpDoesNotExist), "operator"))
+			errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("operator %s and %s conflict for %s", v1.NodeSelectorOpDoesNotExist, v1.NodeSelectorOpDoesNotExist, requirement.Key), "operator"))
 		}
-	}
-	for key := range r.Keys() {
-		values := r.Values(key)
-		if values.Len() == 0 {
-			errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("no feasible value for requirement, %s", key), "values"))
+		if r.Get(requirement.Key).Len() == 0 {
+			errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("no feasible value for %s", requirement.Key), "values"))
 		}
 	}
 	return errs
 }
 
 // Compatible ensures the provided requirements can be met. It is
 // non-commutative (i.e., A.Compatible(B) != B.Compatible(A))
+//gocyclo:ignore
 func (r Requirements) Compatible(requirements Requirements) (errs *apis.FieldError) {
 	for i, key := range r.Keys().Union(requirements.Keys()).UnsortedList() {
 		// Key must be defined if required
-		if values := requirements.Values(key); values.Len() != 0 && !values.Complement && !r.hasRequirement(withKey(key)) {
-			errs = errs.Also(apis.ErrInvalidValue("is not defined", "key")).ViaFieldIndex("requirements", i)
+		if values := requirements.Get(key); values.Len() != 0 && !values.Complement && !r.hasRequirement(withKey(key)) {
+			errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("require values for key %s but is not defined", key), "key")).ViaFieldIndex("requirements", i)
 		}
 		// Values must overlap
-		if values := r.Values(key); values.Intersection(requirements.Values(key)).Len() == 0 {
-			errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("%s not in %s", values, requirements.Values(key)), "values")).ViaFieldIndex("requirements", i)
+		if values := r.Get(key); values.Intersection(requirements.Get(key)).Len() == 0 {
+			errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("%s not in %s, key %s", values, requirements.Get(key), key), "values")).ViaFieldIndex("requirements", i)
 		}
-
-		for r1, r2 := range map[*Requirements]*Requirements{
-			&r:            &requirements,
-			&requirements: &r,
-		} {
-			// Exists incompatible with DoesNotExist or undefined
-			if r1.hasRequirement(withKeyAndOperator(key, v1.NodeSelectorOpExists)) {
-				if r2.hasRequirement(withKeyAndOperator(key, v1.NodeSelectorOpDoesNotExist)) || !r2.hasRequirement(withKey(key)) {
-					errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("%s prohibits %s", v1.NodeSelectorOpExists, v1.NodeSelectorOpDoesNotExist), "operator")).ViaFieldIndex("requirements", i)
-				}
+		// Exists incompatible with DoesNotExist or undefined
+		if requirements.hasRequirement(withKeyAndOperator(key, v1.NodeSelectorOpExists)) {
+			if r.hasRequirement(withKeyAndOperator(key, v1.NodeSelectorOpDoesNotExist)) || !r.hasRequirement(withKey(key)) {
+				errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("%s prohibits %s, key %s", v1.NodeSelectorOpExists, v1.NodeSelectorOpDoesNotExist, key), "operator")).ViaFieldIndex("requirements", i)
 			}
-			// DoesNotExist requires DoesNotExist or undefined
-			if r1.hasRequirement(withKeyAndOperator(key, v1.NodeSelectorOpDoesNotExist)) {
-				if !(r2.hasRequirement(withKeyAndOperator(key, v1.NodeSelectorOpDoesNotExist)) || !r2.hasRequirement(withKey(key))) {
-					errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("%s requires %s", v1.NodeSelectorOpDoesNotExist, v1.NodeSelectorOpDoesNotExist), "operator")).ViaFieldIndex("requirements", i)
-				}
+		}
+		// DoesNotExist requires DoesNotExist or undefined
+		if requirements.hasRequirement(withKeyAndOperator(key, v1.NodeSelectorOpDoesNotExist)) {
+			if !(r.hasRequirement(withKeyAndOperator(key, v1.NodeSelectorOpDoesNotExist)) || !r.hasRequirement(withKey(key))) {
+				errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("%s requires %s, key %s", v1.NodeSelectorOpDoesNotExist, v1.NodeSelectorOpDoesNotExist, key), "operator")).ViaFieldIndex("requirements", i)
+			}
+		}
+		// Repeat for the other direction
+		// Exists incompatible with DoesNotExist or undefined
+		if r.hasRequirement(withKeyAndOperator(key, v1.NodeSelectorOpExists)) {
+			if requirements.hasRequirement(withKeyAndOperator(key, v1.NodeSelectorOpDoesNotExist)) || !requirements.hasRequirement(withKey(key)) {
+				errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("%s prohibits %s, key %s", v1.NodeSelectorOpExists, v1.NodeSelectorOpDoesNotExist, key), "operator")).ViaFieldIndex("requirements", i)
+			}
+		}
+		// DoesNotExist requires DoesNotExist or undefined
+		if r.hasRequirement(withKeyAndOperator(key, v1.NodeSelectorOpDoesNotExist)) {
+			if !(requirements.hasRequirement(withKeyAndOperator(key, v1.NodeSelectorOpDoesNotExist)) || !requirements.hasRequirement(withKey(key))) {
+				errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("%s requires %s, key %s", v1.NodeSelectorOpDoesNotExist, v1.NodeSelectorOpDoesNotExist, key), "operator")).ViaFieldIndex("requirements", i)
 			}
-
 		}
 	}
 	return errs

pkg/apis/provisioning/v1alpha5/suite_test.go

@@ -129,18 +129,16 @@ var _ = Describe("Validation", func() {
 			Expect(provisioner.Validate(ctx)).ToNot(Succeed())
 		})
 	})
-	Context("Requirements", func() {
+	Context("Validation", func() {
 		It("should allow supported ops", func() {
 			provisioner.Spec.Requirements = NewRequirements(
 				v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpIn, Values: []string{"test"}},
-				v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpNotIn, Values: []string{"bar"}},
 				v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpExists},
-				v1.NodeSelectorRequirement{Key: v1.LabelOSStable, Operator: v1.NodeSelectorOpDoesNotExist},
 			)
 			Expect(provisioner.Validate(ctx)).To(Succeed())
 		})
 		It("should fail for unsupported ops", func() {
-			for _, op := range []v1.NodeSelectorOperator{v1.NodeSelectorOpGt, v1.NodeSelectorOpLt} {
+			for _, op := range []v1.NodeSelectorOperator{v1.NodeSelectorOpNotIn, v1.NodeSelectorOpDoesNotExist, v1.NodeSelectorOpGt, v1.NodeSelectorOpLt} {
 				provisioner.Spec.Requirements = NewRequirements(
 					v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: op, Values: []string{"test"}},
 				)
@@ -196,6 +194,8 @@ var _ = Describe("Validation", func() {
 			)
 			Expect(provisioner.Spec.Requirements.Keys().UnsortedList()).To(Equal([]string{v1.LabelTopologyZone}))
 		})
+	})
+	Context("Compatibility", func() {
 		It("A should be compatible to B, <In, In> operator", func() {
 			A := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpIn, Values: []string{"test", "foo"}})
 			B := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpIn, Values: []string{"foo"}})
@@ -226,6 +226,11 @@ var _ = Describe("Validation", func() {
 			B := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpDoesNotExist})
 			Expect(A.Compatible(B)).ToNot(Succeed())
 		})
+		It("A should be compatible to B, <In, Empty> operator", func() {
+			A := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpIn, Values: []string{"foo"}})
+			B := NewRequirements()
+			Expect(A.Compatible(B)).To(Succeed())
+		})
 		It("A should be compatible to B, <NotIn, In> operator", func() {
 			A := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpNotIn, Values: []string{"foo"}})
 			B := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpIn, Values: []string{"test", "foo"}})
@@ -251,7 +256,11 @@ var _ = Describe("Validation", func() {
 			B := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpDoesNotExist})
 			Expect(A.Compatible(B)).ToNot(Succeed())
 		})
-
+		It("A should be compatible to B, <NotIn, Empty> operator", func() {
+			A := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpNotIn, Values: []string{"foo"}})
+			B := NewRequirements()
+			Expect(A.Compatible(B)).To(Succeed())
+		})
 		It("A should be compatible to B, <Exists, In> operator", func() {
 			A := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpExists})
 			B := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpIn, Values: []string{"foo"}})
@@ -272,6 +281,11 @@ var _ = Describe("Validation", func() {
 			B := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpDoesNotExist})
 			Expect(A.Compatible(B)).ToNot(Succeed())
 		})
+		It("A should fail to be compatible to B, <Exists, Empty> operator, conflicting", func() {
+			A := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpExists})
+			B := NewRequirements()
+			Expect(A.Compatible(B)).ToNot(Succeed())
+		})
 		It("A should fail to be compatible to B, <DoesNotExist, In> operator, conflicting", func() {
 			A := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpDoesNotExist})
 			B := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpIn, Values: []string{"foo"}})
@@ -292,8 +306,8 @@ var _ = Describe("Validation", func() {
 			B := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpDoesNotExist})
 			Expect(A.Compatible(B)).To(Succeed())
 		})
-		It("A should be compatible to B, <In, Empty> operator", func() {
-			A := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpIn, Values: []string{"foo"}})
+		It("A should be compatible to B, <DoesNotExist, Empty> operator", func() {
+			A := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpDoesNotExist, Values: []string{"foo"}})
 			B := NewRequirements()
 			Expect(A.Compatible(B)).To(Succeed())
 		})
@@ -307,30 +321,15 @@ var _ = Describe("Validation", func() {
 			B := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpNotIn, Values: []string{"foo"}})
 			Expect(A.Compatible(B)).To(Succeed())
 		})
-		It("A should be compatible to B, <NotIn, Empty> operator", func() {
-			A := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpNotIn, Values: []string{"foo"}})
-			B := NewRequirements()
-			Expect(A.Compatible(B)).To(Succeed())
-		})
 		It("A should fail to be compatible to B,, <Empty, Exists> operator, conflicting", func() {
 			A := NewRequirements()
 			B := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpExists})
 			Expect(A.Compatible(B)).ToNot(Succeed())
 		})
-		It("A should fail to be compatible to B, <Exists, Empty> operator, conflicting", func() {
-			A := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpExists})
-			B := NewRequirements()
-			Expect(A.Compatible(B)).ToNot(Succeed())
-		})
 		It("A should be compatible to B, <Empty, DoesNotExist> operator", func() {
 			A := NewRequirements()
 			B := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpDoesNotExist, Values: []string{"foo"}})
 			Expect(A.Compatible(B)).To(Succeed())
 		})
-		It("A should be compatible to B, <DoesNotExist, Empty> operator", func() {
-			A := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpDoesNotExist, Values: []string{"foo"}})
-			B := NewRequirements()
-			Expect(A.Compatible(B)).To(Succeed())
-		})
 	})
 })

pkg/controllers/provisioning/controller.go

@@ -103,7 +103,7 @@ func (c *Controller) Apply(ctx context.Context, provisioner *v1alpha5.Provisione
 		Add(requirements(instanceTypes)...).
 		Add(v1alpha5.NewLabelRequirements(provisioner.Spec.Labels).Requirements...)
 	if err := provisioner.Spec.Requirements.Validate(); err != nil {
-		return fmt.Errorf("provisioner requirements validation failed, %v", err)
+		return fmt.Errorf("validating requirements, %w", err)
 	}
 	// Update the provisioner if anything has changed
 	if c.hasChanged(ctx, provisioner) {
@@ -163,7 +163,6 @@ func requirements(instanceTypes []cloudprovider.InstanceType) []v1.NodeSelectorR
 		requirements = append(requirements, v1.NodeSelectorRequirement{Key: key, Operator: v1.NodeSelectorOpIn, Values: values.UnsortedList()})
 	}
 	return requirements
-
 }
 
 // Register the controller to the manager

pkg/controllers/provisioning/scheduling/topology.go

@@ -47,7 +47,7 @@ func (t *Topology) Inject(ctx context.Context, constraints *v1alpha5.Constraints
 		}
 		for _, pod := range topologyGroup.Pods {
 			domain := topologyGroup.NextDomain(constraints.Requirements.Add(v1alpha5.NewPodRequirements(pod).Requirements...).
-				Values(topologyGroup.Constraint.TopologyKey).
+				Get(topologyGroup.Constraint.TopologyKey).
 				Values())
 			pod.Spec.NodeSelector = functional.UnionStringMaps(pod.Spec.NodeSelector, map[string]string{topologyGroup.Constraint.TopologyKey: domain})
 		}