BREAKING CHANGE: make AMISelectorTerms required

cmd/controller/main.go

@@ -15,13 +15,20 @@ limitations under the License.
 package main
 
 import (
+	"context"
+	"fmt"
+	"strings"
+
 	"github.com/samber/lo"
+	"knative.dev/pkg/logging"
 
+	"github.com/aws/karpenter-provider-aws/pkg/apis/v1beta1"
 	"github.com/aws/karpenter-provider-aws/pkg/cloudprovider"
 	"github.com/aws/karpenter-provider-aws/pkg/controllers"
 	"github.com/aws/karpenter-provider-aws/pkg/operator"
 	"github.com/aws/karpenter-provider-aws/pkg/webhooks"
 
+	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/karpenter/pkg/cloudprovider/metrics"
 	corecontrollers "sigs.k8s.io/karpenter/pkg/controllers"
 	"sigs.k8s.io/karpenter/pkg/controllers/state"
@@ -43,6 +50,10 @@ func main() {
 	lo.Must0(op.AddHealthzCheck("cloud-provider", awsCloudProvider.LivenessProbe))
 	cloudProvider := metrics.Decorate(awsCloudProvider)
 
+	if err := validateEC2NodeClasses(ctx, op.GetAPIReader()); err != nil {
+		logging.FromContext(ctx).Fatalf("validating EC2NodeClasses, %s", err)
+	}
+
 	op.
 		WithControllers(ctx, corecontrollers.NewControllers(
 			op.Clock,
@@ -72,3 +83,18 @@ func main() {
 		WithWebhooks(ctx, webhooks.NewWebhooks()...).
 		Start(ctx)
 }
+
+// validateEC2NodeClasses ensures all EC2NodeClasses specify AMISelectorTerms (required as of v0.37.0)
+func validateEC2NodeClasses(ctx context.Context, reader client.Reader) error {
+	nodeClassList := v1beta1.EC2NodeClassList{}
+	err := reader.List(ctx, &nodeClassList)
+	if err != nil {
+		return fmt.Errorf("listing EC2NodeClasses on startup, %s", err.Error())
+	}
+	if invalidNodeClasses := lo.FilterMap(nodeClassList.Items, func(nc v1beta1.EC2NodeClass, _ int) (string, bool) {
+		return nc.Name, len(nc.Spec.AMISelectorTerms) == 0
+	}); len(invalidNodeClasses) != 0 {
+		return fmt.Errorf("detected EC2NodeClasses with un-set AMISelectorTerms (%s), refer to the 0.37.0+ upgrade guide: https://karpenter.sh/docs/upgrading/upgrade-guide/#upgrading-to-0370", strings.Join(invalidNodeClasses, ","))
+	}
+	return nil
+}

pkg/apis/crds/karpenter.k8s.aws_ec2nodeclasses.yaml

@@ -65,6 +65,25 @@ spec:
                     AMISelectorTerm defines selection logic for an ami used by Karpenter to launch nodes.
                     If multiple fields are used for selection, the requirements are ANDed.
                   properties:
+                    eksOptimized:
+                      description: EKSOptimized is an object which, when configured,
+                        allows Karpenter to automatically select the latest AMI from
+                        an optimized family.
+                      properties:
+                        family:
+                          description: Family is the AMI family to use when selecting
+                            EKS optimized AMIs.
+                          enum:
+                          - AL2
+                          - AL2023
+                          - Bottlerocket
+                          - Ubuntu
+                          - Windows2019
+                          - Windows2022
+                          type: string
+                      required:
+                      - family
+                      type: object
                     id:
                       description: ID is the ami id in EC2
                       pattern: ami-[0-9a-z]+
@@ -92,14 +111,24 @@ spec:
                         rule: self.all(k, k != '' && self[k] != '')
                   type: object
                 maxItems: 30
+                minItems: 1
                 type: array
                 x-kubernetes-validations:
-                - message: expected at least one, got none, ['tags', 'id', 'name']
-                  rule: self.all(x, has(x.tags) || has(x.id) || has(x.name))
+                - message: expected at least one, got none, ['eksOptimized', 'tags',
+                    'id', 'name']
+                  rule: self.all(x, has(x.eksOptimized) || has(x.tags) || has(x.id)
+                    || has(x.name))
                 - message: '''id'' is mutually exclusive, cannot be set with a combination
                     of other fields in amiSelectorTerms'
-                  rule: '!self.all(x, has(x.id) && (has(x.tags) || has(x.name) ||
-                    has(x.owner)))'
+                  rule: '!self.all(x, has(x.id) && (has(x.eksOptimized) || has(x.tags)
+                    || has(x.name) || has(x.owner)))'
+                - message: '''eksOptimized'' is mutually exclusive, cannot be set
+                    with a combination of other fields in amiSelectorTerms'
+                  rule: '!self.all(x, has(x.eksOptimized) && (has(x.id) || has(x.tags)
+                    || has(x.name) || has(x.owner)))'
+                - message: '''eksOptimized'' is mutually exclusive, cannot be set
+                    with other terms'
+                  rule: '!(self.exists(x, has(x.eksOptimized)) && self.size() != 1)'
               associatePublicIPAddress:
                 description: AssociatePublicIPAddress controls if public IP addresses
                   are assigned to instances that are launched with the nodeclass.
@@ -441,13 +470,11 @@ spec:
                 type: string
             required:
             - amiFamily
+            - amiSelectorTerms
             - securityGroupSelectorTerms
             - subnetSelectorTerms
             type: object
             x-kubernetes-validations:
-            - message: amiSelectorTerms is required when amiFamily == 'Custom'
-              rule: 'self.amiFamily == ''Custom'' ? self.amiSelectorTerms.size() !=
-                0 : true'
             - message: must specify exactly one of ['role', 'instanceProfile']
               rule: (has(self.role) && !has(self.instanceProfile)) || (!has(self.role)
                 && has(self.instanceProfile))
@@ -456,6 +483,11 @@ spec:
                 this.
               rule: (has(oldSelf.role) && has(self.role)) || (has(oldSelf.instanceProfile)
                 && has(self.instanceProfile))
+            - message: amiFamily must match amiSelectorTerms[].eksOptimized.family
+                or be 'Custom'
+              rule: 'self.amiFamily == ''Custom'' || (self.amiSelectorTerms.exists(x,
+                has(x.eksOptimized)) ? self.amiSelectorTerms.exists(x, has(x.eksOptimized)
+                && x.eksOptimized.family == self.amiFamily) : true)'
           status:
             description: EC2NodeClassStatus contains the resolved state of the EC2NodeClass
             properties:

pkg/apis/v1beta1/ec2nodeclass.go

@@ -46,11 +46,14 @@ type EC2NodeClassSpec struct {
 	// +optional
 	AssociatePublicIPAddress *bool `json:"associatePublicIPAddress,omitempty"`
 	// AMISelectorTerms is a list of or ami selector terms. The terms are ORed.
-	// +kubebuilder:validation:XValidation:message="expected at least one, got none, ['tags', 'id', 'name']",rule="self.all(x, has(x.tags) || has(x.id) || has(x.name))"
-	// +kubebuilder:validation:XValidation:message="'id' is mutually exclusive, cannot be set with a combination of other fields in amiSelectorTerms",rule="!self.all(x, has(x.id) && (has(x.tags) || has(x.name) || has(x.owner)))"
+	// +kubebuilder:validation:XValidation:message="expected at least one, got none, ['eksOptimized', 'tags', 'id', 'name']",rule="self.all(x, has(x.eksOptimized) || has(x.tags) || has(x.id) || has(x.name))"
+	// +kubebuilder:validation:XValidation:message="'id' is mutually exclusive, cannot be set with a combination of other fields in amiSelectorTerms",rule="!self.all(x, has(x.id) && (has(x.eksOptimized) || has(x.tags) || has(x.name) || has(x.owner)))"
+	// +kubebuilder:validation:XValidation:message="'eksOptimized' is mutually exclusive, cannot be set with a combination of other fields in amiSelectorTerms",rule="!self.all(x, has(x.eksOptimized) && (has(x.id) || has(x.tags) || has(x.name) || has(x.owner)))"
+	// +kubebuilder:validation:XValidation:message="'eksOptimized' is mutually exclusive, cannot be set with other terms",rule="!(self.exists(x, has(x.eksOptimized)) && self.size() != 1)"
+	// +kubebuilder:validation:MinItems:=1
 	// +kubebuilder:validation:MaxItems:=30
-	// +optional
-	AMISelectorTerms []AMISelectorTerm `json:"amiSelectorTerms,omitempty" hash:"ignore"`
+	// +required
+	AMISelectorTerms []AMISelectorTerm `json:"amiSelectorTerms" hash:"ignore"`
 	// AMIFamily is the AMI family that instances use.
 	// +kubebuilder:validation:Enum:={AL2,AL2023,Bottlerocket,Ubuntu,Custom,Windows2019,Windows2022}
 	// +required
@@ -155,6 +158,9 @@ type SecurityGroupSelectorTerm struct {
 // AMISelectorTerm defines selection logic for an ami used by Karpenter to launch nodes.
 // If multiple fields are used for selection, the requirements are ANDed.
 type AMISelectorTerm struct {
+	// EKSOptimized is an object which, when configured, allows Karpenter to automatically select the latest AMI from an optimized family.
+	// +optional
+	EKSOptimized *EKSOptimized `json:"eksOptimized,omitempty"`
 	// Tags is a map of key/value tags used to select subnets
 	// Specifying '*' for a value selects all values for a given tag key.
 	// +kubebuilder:validation:XValidation:message="empty tag keys or values aren't supported",rule="self.all(k, k != '' && self[k] != '')"
@@ -175,6 +181,12 @@ type AMISelectorTerm struct {
 	Owner string `json:"owner,omitempty"`
 }
 
+type EKSOptimized struct {
+	// Family is the AMI family to use when selecting EKS optimized AMIs.
+	// +kubebuilder:validation:Enum:={AL2,AL2023,Bottlerocket,Ubuntu,Windows2019,Windows2022}
+	Family string `json:"family"`
+}
+
 // MetadataOptions contains parameters for specifying the exposure of the
 // Instance Metadata Service to provisioned EC2 nodes.
 type MetadataOptions struct {
@@ -324,9 +336,9 @@ type EC2NodeClass struct {
 	metav1.TypeMeta   `json:",inline"`
 	metav1.ObjectMeta `json:"metadata,omitempty"`
 
-	// +kubebuilder:validation:XValidation:message="amiSelectorTerms is required when amiFamily == 'Custom'",rule="self.amiFamily == 'Custom' ? self.amiSelectorTerms.size() != 0 : true"
 	// +kubebuilder:validation:XValidation:message="must specify exactly one of ['role', 'instanceProfile']",rule="(has(self.role) && !has(self.instanceProfile)) || (!has(self.role) && has(self.instanceProfile))"
 	// +kubebuilder:validation:XValidation:message="changing from 'instanceProfile' to 'role' is not supported. You must delete and recreate this node class if you want to change this.",rule="(has(oldSelf.role) && has(self.role)) || (has(oldSelf.instanceProfile) && has(self.instanceProfile))"
+	// +kubebuilder:validation:XValidation:message="amiFamily must match amiSelectorTerms[].eksOptimized.family or be 'Custom'",rule="self.amiFamily == 'Custom' || (self.amiSelectorTerms.exists(x, has(x.eksOptimized)) ? self.amiSelectorTerms.exists(x, has(x.eksOptimized) && x.eksOptimized.family == self.amiFamily) : true)"
 	Spec   EC2NodeClassSpec   `json:"spec,omitempty"`
 	Status EC2NodeClassStatus `json:"status,omitempty"`
 }

pkg/apis/v1beta1/ec2nodeclass_validation.go

@@ -20,6 +20,7 @@ import (
 	"strings"
 
 	"github.com/aws/aws-sdk-go/service/ec2"
+	"github.com/samber/lo"
 	admissionregistrationv1 "k8s.io/api/admissionregistration/v1"
 	"k8s.io/apimachinery/pkg/api/resource"
 	"knative.dev/pkg/apis"
@@ -73,6 +74,11 @@ func (in *EC2NodeClassSpec) validate(_ context.Context) (errs *apis.FieldError)
 	if in.Role == "" && in.InstanceProfile == nil {
 		errs = errs.Also(apis.ErrMissingOneOf(rolePath, instanceProfilePath))
 	}
+	if term, ok := lo.Find(in.AMISelectorTerms, func(t AMISelectorTerm) bool {
+		return t.EKSOptimized != nil
+	}); ok && lo.FromPtr(in.AMIFamily) != term.EKSOptimized.Family && lo.FromPtr(in.AMIFamily) != AMIFamilyCustom {
+		errs = errs.Also(apis.ErrGeneric(`amiFamily must match amiSelectorTerms[].eksOptimized.family or be 'Custom'`))
+	}
 	return errs.Also(
 		in.validateSubnetSelectorTerms().ViaField(subnetSelectorTermsPath),
 		in.validateSecurityGroupSelectorTerms().ViaField(securityGroupSelectorTermsPath),
@@ -128,6 +134,14 @@ func (in *SecurityGroupSelectorTerm) validate() (errs *apis.FieldError) {
 }
 
 func (in *EC2NodeClassSpec) validateAMISelectorTerms() (errs *apis.FieldError) {
+	if len(in.AMISelectorTerms) == 0 {
+		errs = errs.Also(apis.ErrMissingOneOf())
+	}
+	if lo.ContainsBy(in.AMISelectorTerms, func(term AMISelectorTerm) bool {
+		return term.EKSOptimized != nil
+	}) && len(in.AMISelectorTerms) > 1 {
+		errs = errs.Also(apis.ErrGeneric(`"eksOptimized" is mutually exclusive, cannot be set with other terms`))
+	}
 	for _, term := range in.AMISelectorTerms {
 		errs = errs.Also(term.validate())
 	}
@@ -137,10 +151,12 @@ func (in *EC2NodeClassSpec) validateAMISelectorTerms() (errs *apis.FieldError) {
 //nolint:gocyclo
 func (in *AMISelectorTerm) validate() (errs *apis.FieldError) {
 	errs = errs.Also(validateTags(in.Tags).ViaField("tags"))
-	if len(in.Tags) == 0 && in.ID == "" && in.Name == "" {
-		errs = errs.Also(apis.ErrGeneric("expect at least one, got none", "tags", "id", "name"))
-	} else if in.ID != "" && (len(in.Tags) > 0 || in.Name != "" || in.Owner != "") {
+	if len(in.Tags) == 0 && in.ID == "" && in.Name == "" && in.EKSOptimized == nil {
+		errs = errs.Also(apis.ErrGeneric("expect at least one, got none", "tags", "id", "name", "eksOptimized"))
+	} else if in.ID != "" && (len(in.Tags) > 0 || in.Name != "" || in.Owner != "" || in.EKSOptimized != nil) {
 		errs = errs.Also(apis.ErrGeneric(`"id" is mutually exclusive, cannot be set with a combination of other fields in`))
+	} else if in.EKSOptimized != nil && (len(in.Tags) > 0 || in.Name != "" || in.Owner != "" || in.ID != "") {
+		errs = errs.Also(apis.ErrGeneric(`"eksOptimized" is mutually exclusive, cannot be set with a combination of other fields in`))
 	}
 	return errs
 }

pkg/apis/v1beta1/ec2nodeclass_validation_cel_test.go

@@ -16,6 +16,7 @@ package v1beta1_test
 
 import (
 	"github.com/aws/aws-sdk-go/aws"
+	"github.com/imdario/mergo"
 	"github.com/samber/lo"
 	"k8s.io/apimachinery/pkg/api/resource"
 	corev1beta1 "sigs.k8s.io/karpenter/pkg/apis/v1beta1"
@@ -318,6 +319,129 @@ var _ = Describe("CEL/Validation", func() {
 		})
 	})
 	Context("AMISelectorTerms", func() {
+		It("should fail when amiSelectorTerms is nil", func() {
+			nc.Spec.AMISelectorTerms = nil
+			Expect(env.Client.Create(ctx, nc)).ToNot(Succeed())
+		})
+		It("should fail when len(amiSelectorTerms) is zero", func() {
+			nc.Spec.AMISelectorTerms = []v1beta1.AMISelectorTerm{}
+			Expect(env.Client.Create(ctx, nc)).ToNot(Succeed())
+		})
+		DescribeTable(
+			"should succeed with a valid eksOptimized family",
+			func(family string) {
+				nc.Spec.AMIFamily = &family
+				nc.Spec.AMISelectorTerms = []v1beta1.AMISelectorTerm{{
+					EKSOptimized: &v1beta1.EKSOptimized{Family: family},
+				}}
+				Expect(env.Client.Create(ctx, nc)).To(Succeed())
+			},
+			Entry("AL2", v1beta1.AMIFamilyAL2),
+			Entry("AL2023", v1beta1.AMIFamilyAL2023),
+			Entry("Bottlerocket", v1beta1.AMIFamilyBottlerocket),
+			Entry("Windows2019", v1beta1.AMIFamilyWindows2019),
+			Entry("Windows2022", v1beta1.AMIFamilyWindows2022),
+			Entry("Ubuntu", v1beta1.AMIFamilyUbuntu),
+		)
+		It("should fail with an empty eksOptimized AMI Family", func() {
+			nc.Spec.AMISelectorTerms = []v1beta1.AMISelectorTerm{{
+				EKSOptimized: &v1beta1.EKSOptimized{},
+			}}
+			Expect(env.Client.Create(ctx, nc)).ToNot(Succeed())
+		})
+		It("should fail with an invalid eksOptimized AMI Family", func() {
+			nc.Spec.AMISelectorTerms = []v1beta1.AMISelectorTerm{{
+				EKSOptimized: &v1beta1.EKSOptimized{Family: "foo"},
+			}}
+			Expect(env.Client.Create(ctx, nc)).ToNot(Succeed())
+		})
+		It("should fail when eksOptimized family is specified with other fields", func() {
+			for _, modifier := range []*v1beta1.AMISelectorTerm{
+				{Tags: map[string]string{"*": "*"}},
+				{ID: "foo"},
+				{Name: "foo"},
+				{Owner: "foo"},
+			} {
+				nc.Spec.AMISelectorTerms = []v1beta1.AMISelectorTerm{{
+					EKSOptimized: &v1beta1.EKSOptimized{Family: v1beta1.AMIFamilyAL2},
+				}}
+				Expect(mergo.Merge(&nc.Spec.AMISelectorTerms[0], modifier)).To(Succeed())
+				Expect(env.Client.Create(ctx, nc)).ToNot(Succeed())
+			}
+		})
+		It("should fail when EKSOptimized is specified with other terms", func() {
+			nc.Spec.AMISelectorTerms = []v1beta1.AMISelectorTerm{
+				{EKSOptimized: &v1beta1.EKSOptimized{Family: v1beta1.AMIFamilyAL2}},
+				{ID: "foo"},
+			}
+			Expect(env.Client.Create(ctx, nc)).ToNot(Succeed())
+		})
+		It("should fail when the eksOptimized family doesn't match amiFamily", func() {
+			families := []string{
+				v1beta1.AMIFamilyAL2,
+				v1beta1.AMIFamilyAL2023,
+				v1beta1.AMIFamilyUbuntu,
+				v1beta1.AMIFamilyBottlerocket,
+				v1beta1.AMIFamilyWindows2019,
+				v1beta1.AMIFamilyWindows2022,
+			}
+			for i := range families {
+				for j := range families {
+					if i == j {
+						continue
+					}
+					nc = test.EC2NodeClass(v1beta1.EC2NodeClass{
+						Spec: v1beta1.EC2NodeClassSpec{
+							AMIFamily: &families[i],
+							AMISelectorTerms: []v1beta1.AMISelectorTerm{{
+								EKSOptimized: &v1beta1.EKSOptimized{Family: families[j]},
+							}},
+						},
+					})
+					Expect(env.Client.Create(ctx, nc)).ToNot(Succeed())
+				}
+			}
+		})
+		It("should succeed when the eksOptimized family matches amiFamily", func() {
+			for _, family := range []string{
+				v1beta1.AMIFamilyAL2,
+				v1beta1.AMIFamilyAL2023,
+				v1beta1.AMIFamilyUbuntu,
+				v1beta1.AMIFamilyBottlerocket,
+				v1beta1.AMIFamilyWindows2019,
+				v1beta1.AMIFamilyWindows2022,
+			} {
+				nc = test.EC2NodeClass(v1beta1.EC2NodeClass{
+					Spec: v1beta1.EC2NodeClassSpec{
+						AMIFamily: lo.ToPtr(family),
+						AMISelectorTerms: []v1beta1.AMISelectorTerm{{
+							EKSOptimized: &v1beta1.EKSOptimized{Family: family},
+						}},
+					},
+				})
+				Expect(env.Client.Create(ctx, nc)).To(Succeed())
+			}
+		})
+		It("should succed when the amiFamily is 'Custom' for any optimized ami", func() {
+			for _, family := range []string{
+				v1beta1.AMIFamilyAL2,
+				v1beta1.AMIFamilyAL2023,
+				v1beta1.AMIFamilyUbuntu,
+				v1beta1.AMIFamilyBottlerocket,
+				v1beta1.AMIFamilyWindows2019,
+				v1beta1.AMIFamilyWindows2022,
+			} {
+				nc = test.EC2NodeClass(v1beta1.EC2NodeClass{
+					Spec: v1beta1.EC2NodeClassSpec{
+						AMIFamily: lo.ToPtr(v1beta1.AMIFamilyCustom),
+						AMISelectorTerms: []v1beta1.AMISelectorTerm{{
+							EKSOptimized: &v1beta1.EKSOptimized{Family: family},
+						}},
+					},
+				})
+				Expect(env.Client.Create(ctx, nc)).To(Succeed())
+			}
+		})
 		It("should succeed with a valid ami selector on tags", func() {
 			nc.Spec.AMISelectorTerms = []v1beta1.AMISelectorTerm{
 				{
@@ -452,10 +576,6 @@ var _ = Describe("CEL/Validation", func() {
 			}
 			Expect(env.Client.Create(ctx, nc)).ToNot(Succeed())
 		})
-		It("should fail when AMIFamily is Custom and not AMISelectorTerms", func() {
-			nc.Spec.AMIFamily = &v1beta1.AMIFamilyCustom
-			Expect(env.Client.Create(ctx, nc)).ToNot(Succeed())
-		})
 	})
 	Context("MetadataOptions", func() {
 		It("should succeed for valid inputs", func() {