feat: deprecated AMIs should be discoverable when specifying ami id o…

pkg/operator/operator.go

@@ -142,7 +142,7 @@ func NewOperator(ctx context.Context, operator *operator.Operator) (context.Cont
 	)
 	versionProvider := version.NewDefaultProvider(operator.KubernetesInterface, cache.New(awscache.DefaultTTL, awscache.DefaultCleanupInterval))
 	ssmProvider := ssmp.NewDefaultProvider(ssm.New(sess), cache.New(awscache.SSMGetParametersByPathTTL, awscache.DefaultCleanupInterval))
-	amiProvider := amifamily.NewDefaultProvider(versionProvider, ssmProvider, ec2api, cache.New(awscache.DefaultTTL, awscache.DefaultCleanupInterval))
+	amiProvider := amifamily.NewDefaultProvider(operator.Clock, versionProvider, ssmProvider, ec2api, cache.New(awscache.DefaultTTL, awscache.DefaultCleanupInterval))
 	amiResolver := amifamily.NewDefaultResolver()
 	launchTemplateProvider := launchtemplate.NewDefaultProvider(
 		ctx,

pkg/providers/amifamily/ami.go

@@ -18,14 +18,14 @@ import (
 	"context"
 	"fmt"
 	"sync"
-	"time"
 
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/service/ec2"
 	"github.com/aws/aws-sdk-go/service/ec2/ec2iface"
 	"github.com/mitchellh/hashstructure/v2"
 	"github.com/patrickmn/go-cache"
 	"github.com/samber/lo"
+	"k8s.io/utils/clock"
 	"sigs.k8s.io/controller-runtime/pkg/log"
 
 	v1 "github.com/aws/karpenter-provider-aws/pkg/apis/v1"
@@ -44,15 +44,18 @@ type Provider interface {
 
 type DefaultProvider struct {
 	sync.Mutex
+
+	clk             clock.Clock
 	cache           *cache.Cache
 	ec2api          ec2iface.EC2API
 	cm              *pretty.ChangeMonitor
 	versionProvider version.Provider
 	ssmProvider     ssm.Provider
 }
 
-func NewDefaultProvider(versionProvider version.Provider, ssmProvider ssm.Provider, ec2api ec2iface.EC2API, cache *cache.Cache) *DefaultProvider {
+func NewDefaultProvider(clk clock.Clock, versionProvider version.Provider, ssmProvider ssm.Provider, ec2api ec2iface.EC2API, cache *cache.Cache) *DefaultProvider {
 	return &DefaultProvider{
+		clk:             clk,
 		cache:           cache,
 		ec2api:          ec2api,
 		cm:              pretty.NewChangeMonitor(),
@@ -166,24 +169,25 @@ func (p *DefaultProvider) amis(ctx context.Context, queries []DescribeImageQuery
 				// Each image may have multiple associated sets of requirements. For example, an image may be compatible with Neuron instances
 				// and GPU instances. In that case, we'll have a set of requirements for each, and will create one "image" for each.
 				for _, reqs := range query.RequirementsForImageWithArchitecture(lo.FromPtr(image.ImageId), arch) {
-					// If we already have an image with the same set of requirements, but this image is newer, replace the previous image.
+					// Checks and store for AMIs
+					// Following checks are needed in order to always priortize non deprecated AMIs
+					// If we already have an image with the same set of requirements, but this image (candidate) is newer, replace the previous (existing) image.
+					// If we already have an image with the same set of requirements which is deprecated, but this image (candidate) is newer or non deprecated, replace the previous (existing) image
 					reqsHash := lo.Must(hashstructure.Hash(reqs.NodeSelectorRequirements(), hashstructure.FormatV2, &hashstructure.HashOptions{SlicesAsSets: true}))
-					if v, ok := images[reqsHash]; ok {
-						candidateCreationTime, _ := time.Parse(time.RFC3339, lo.FromPtr(image.CreationDate))
-						existingCreationTime, _ := time.Parse(time.RFC3339, v.CreationDate)
-						if existingCreationTime == candidateCreationTime && lo.FromPtr(image.Name) < v.Name {
-							continue
-						}
-						if candidateCreationTime.Unix() < existingCreationTime.Unix() {
-							continue
-						}
-					}
-					images[reqsHash] = AMI{
+					candidateDeprecated := parseTimeWithDefault(lo.FromPtr(image.DeprecationTime), maxTime).Unix() <= p.clk.Now().Unix()
+					ami := AMI{
 						Name:         lo.FromPtr(image.Name),
 						AmiID:        lo.FromPtr(image.ImageId),
 						CreationDate: lo.FromPtr(image.CreationDate),
+						Deprecated:   candidateDeprecated,
 						Requirements: reqs,
 					}
+					if v, ok := images[reqsHash]; ok {
+						if cmpResult := compareAMI(v, ami); cmpResult <= 0 {
+							continue
+						}
+					}
+					images[reqsHash] = ami
 				}
 			}
 			return true
@@ -211,3 +215,30 @@ func MapToInstanceTypes(instanceTypes []*cloudprovider.InstanceType, amis []v1.A
 	}
 	return amiIDs
 }
+
+// Compare two AMI's based on their deprecation status, creation time or name
+// If both AMIs are deprecated, compare creation time and return the one with the newer creation time
+// If both AMIs are non-deprecated, compare creation time and return the one with the newer creation time
+// If one AMI is deprecated, return the non deprecated one
+// The result will be
+// 0 if AMI i == AMI j, where creation date, deprecation status and name are all equal
+// -1 if AMI i < AMI j, if AMI i is non-deprecated or newer than AMI j
+// +1 if AMI i > AMI j, if AMI j is non-deprecated or newer than AMI i
+func compareAMI(i, j AMI) int {
+	iCreationDate := parseTimeWithDefault(i.CreationDate, minTime)
+	jCreationDate := parseTimeWithDefault(j.CreationDate, minTime)
+	// Prioritize non-deprecated AMIs over deprecated ones
+	if i.Deprecated != j.Deprecated {
+		return lo.Ternary(i.Deprecated, 1, -1)
+	}
+	// If both are either non-deprecated or deprecated, compare by creation date
+	if iCreationDate.Unix() != jCreationDate.Unix() {
+		return lo.Ternary(iCreationDate.Unix() > jCreationDate.Unix(), -1, 1)
+	}
+	// If they have the same creation date, use the name as a tie-breaker
+	if i.Name != j.Name {
+		return lo.Ternary(i.Name > j.Name, -1, 1)
+	}
+	// If all attributes are are equal, both AMIs are exactly identical
+	return 0
+}

pkg/providers/amifamily/suite_test.go

@@ -303,6 +303,182 @@ var _ = Describe("AMIProvider", func() {
 			}))
 		})
 	})
+	Context("AMI List requirements", func() {
+		BeforeEach(func() {
+			// Set time using the injectable/fake clock to now
+			awsEnv.Clock.SetTime(time.Now())
+			nodeClass.Spec.AMISelectorTerms = []v1.AMISelectorTerm{
+				{
+					Tags: map[string]string{"*": "*"},
+				},
+			}
+		})
+		It("should priortize the older non-deprecated ami without deprecation time", func() {
+			// Here we have two AMIs one which is deprecated and newer and one which is older and non-deprecated without a deprecation time
+			// List operation will priortize the non-deprecated AMI without the deprecation time
+			awsEnv.EC2API.DescribeImagesOutput.Set(&ec2.DescribeImagesOutput{
+				Images: []*ec2.Image{
+					{
+						Name:            aws.String(amd64AMI),
+						ImageId:         aws.String("ami-5678"),
+						CreationDate:    aws.String("2021-08-31T00:12:42.000Z"),
+						DeprecationTime: aws.String(awsEnv.Clock.Now().Add(-1 * time.Hour).Format(time.RFC3339)),
+						Architecture:    aws.String("x86_64"),
+						Tags: []*ec2.Tag{
+							{Key: aws.String("Name"), Value: aws.String(amd64AMI)},
+							{Key: aws.String("foo"), Value: aws.String("bar")},
+						},
+					},
+					{
+						Name:         aws.String(amd64AMI),
+						ImageId:      aws.String("ami-1234"),
+						CreationDate: aws.String("2020-08-31T00:08:42.000Z"),
+						Architecture: aws.String("x86_64"),
+						Tags: []*ec2.Tag{
+							{Key: aws.String("Name"), Value: aws.String(amd64AMI)},
+							{Key: aws.String("foo"), Value: aws.String("bar")},
+						},
+					},
+				},
+			})
+			amis, err := awsEnv.AMIProvider.List(ctx, nodeClass)
+			Expect(err).ToNot(HaveOccurred())
+			Expect(amis).To(HaveLen(1))
+			Expect(amis).To(ConsistOf(amifamily.AMI{
+				Name:         amd64AMI,
+				AmiID:        "ami-1234",
+				CreationDate: "2020-08-31T00:08:42.000Z",
+				Requirements: scheduling.NewRequirements(
+					scheduling.NewRequirement(corev1.LabelArchStable, corev1.NodeSelectorOpIn, karpv1.ArchitectureAmd64),
+				),
+			}))
+		})
+		It("should priortize the non-deprecated ami with deprecation time when both have same creation time", func() {
+			// Here we have two AMIs one which is deprecated and one which is non-deprecated both with the same creation time
+			// List operation will priortize the non-deprecated AMI
+			awsEnv.EC2API.DescribeImagesOutput.Set(&ec2.DescribeImagesOutput{
+				Images: []*ec2.Image{
+					{
+						Name:            aws.String(amd64AMI),
+						ImageId:         aws.String("ami-5678"),
+						CreationDate:    aws.String("2021-08-31T00:12:42.000Z"),
+						DeprecationTime: aws.String(awsEnv.Clock.Now().Add(-10 * time.Minute).Format(time.RFC3339)),
+						Architecture:    aws.String("x86_64"),
+						Tags: []*ec2.Tag{
+							{Key: aws.String("Name"), Value: aws.String(amd64AMI)},
+							{Key: aws.String("foo"), Value: aws.String("bar")},
+						},
+					},
+					{
+						Name:            aws.String(amd64AMI),
+						ImageId:         aws.String("ami-1234"),
+						CreationDate:    aws.String("2021-08-31T00:12:42.000Z"),
+						DeprecationTime: aws.String(awsEnv.Clock.Now().Add(10 * time.Minute).Format(time.RFC3339)),
+						Architecture:    aws.String("x86_64"),
+						Tags: []*ec2.Tag{
+							{Key: aws.String("Name"), Value: aws.String(amd64AMI)},
+							{Key: aws.String("foo"), Value: aws.String("bar")},
+						},
+					},
+				},
+			})
+			amis, err := awsEnv.AMIProvider.List(ctx, nodeClass)
+			Expect(err).ToNot(HaveOccurred())
+			Expect(amis).To(HaveLen(1))
+			Expect(amis).To(ConsistOf(amifamily.AMI{
+				Name:         amd64AMI,
+				AmiID:        "ami-1234",
+				CreationDate: "2021-08-31T00:12:42.000Z",
+				Deprecated:   false,
+				Requirements: scheduling.NewRequirements(
+					scheduling.NewRequirement(corev1.LabelArchStable, corev1.NodeSelectorOpIn, karpv1.ArchitectureAmd64),
+				),
+			}))
+		})
+		It("should priortize the non-deprecated ami with deprecation time when both have same creation time and different name", func() {
+			// Here we have two AMIs one which is deprecated and one which is non-deprecated both with the same creation time but with different names
+			// List operation will priortize the non-deprecated AMI
+			awsEnv.EC2API.DescribeImagesOutput.Set(&ec2.DescribeImagesOutput{
+				Images: []*ec2.Image{
+					{
+						Name:            aws.String("test-ami-2"),
+						ImageId:         aws.String("ami-5678"),
+						CreationDate:    aws.String("2021-08-31T00:12:42.000Z"),
+						DeprecationTime: aws.String(awsEnv.Clock.Now().Add(-10 * time.Minute).Format(time.RFC3339)),
+						Architecture:    aws.String("x86_64"),
+						Tags: []*ec2.Tag{
+							{Key: aws.String("Name"), Value: aws.String("test-ami-2")},
+							{Key: aws.String("foo"), Value: aws.String("bar")},
+						},
+					},
+					{
+						Name:            aws.String("test-ami-1"),
+						ImageId:         aws.String("ami-1234"),
+						CreationDate:    aws.String("2021-08-31T00:12:42.000Z"),
+						DeprecationTime: aws.String(awsEnv.Clock.Now().Add(10 * time.Minute).Format(time.RFC3339)),
+						Architecture:    aws.String("x86_64"),
+						Tags: []*ec2.Tag{
+							{Key: aws.String("Name"), Value: aws.String("test-ami-1")},
+							{Key: aws.String("foo"), Value: aws.String("bar")},
+						},
+					},
+				},
+			})
+			amis, err := awsEnv.AMIProvider.List(ctx, nodeClass)
+			Expect(err).ToNot(HaveOccurred())
+			Expect(amis).To(HaveLen(1))
+			Expect(amis).To(ConsistOf(amifamily.AMI{
+				Name:         "test-ami-1",
+				AmiID:        "ami-1234",
+				CreationDate: "2021-08-31T00:12:42.000Z",
+				Deprecated:   false,
+				Requirements: scheduling.NewRequirements(
+					scheduling.NewRequirement(corev1.LabelArchStable, corev1.NodeSelectorOpIn, karpv1.ArchitectureAmd64),
+				),
+			}))
+		})
+		It("should priortize the newer ami if both are deprecated", func() {
+			//Both amis are deprecated and have the same deprecation time
+			awsEnv.EC2API.DescribeImagesOutput.Set(&ec2.DescribeImagesOutput{
+				Images: []*ec2.Image{
+					{
+						Name:            aws.String(amd64AMI),
+						ImageId:         aws.String("ami-5678"),
+						CreationDate:    aws.String("2021-08-31T00:12:42.000Z"),
+						DeprecationTime: aws.String(awsEnv.Clock.Now().Add(-1 * time.Hour).Format(time.RFC3339)),
+						Architecture:    aws.String("x86_64"),
+						Tags: []*ec2.Tag{
+							{Key: aws.String("Name"), Value: aws.String(amd64AMI)},
+							{Key: aws.String("foo"), Value: aws.String("bar")},
+						},
+					},
+					{
+						Name:            aws.String(amd64AMI),
+						ImageId:         aws.String("ami-1234"),
+						CreationDate:    aws.String("2020-08-31T00:08:42.000Z"),
+						DeprecationTime: aws.String(awsEnv.Clock.Now().Add(-1 * time.Hour).Format(time.RFC3339)),
+						Architecture:    aws.String("x86_64"),
+						Tags: []*ec2.Tag{
+							{Key: aws.String("Name"), Value: aws.String(amd64AMI)},
+							{Key: aws.String("foo"), Value: aws.String("bar")},
+						},
+					},
+				},
+			})
+			amis, err := awsEnv.AMIProvider.List(ctx, nodeClass)
+			Expect(err).ToNot(HaveOccurred())
+			Expect(amis).To(HaveLen(1))
+			Expect(amis).To(ConsistOf(amifamily.AMI{
+				Name:         amd64AMI,
+				AmiID:        "ami-5678",
+				CreationDate: "2021-08-31T00:12:42.000Z",
+				Deprecated:   true,
+				Requirements: scheduling.NewRequirements(
+					scheduling.NewRequirement(corev1.LabelArchStable, corev1.NodeSelectorOpIn, karpv1.ArchitectureAmd64),
+				),
+			}))
+		})
+	})
 	Context("AMI Selectors", func() {
 		// When you tag public or shared resources, the tags you assign are available only to your AWS account; no other AWS account will have access to those tags
 		// https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/Using_Tags.html#tag-restrictions
@@ -553,6 +729,72 @@ var _ = Describe("AMIProvider", func() {
 				},
 			))
 		})
+		It("should sort deprecated amis with the same name and deprecation time consistently", func() {
+			amis := amifamily.AMIs{
+				{
+					Name:         "test-ami-1",
+					AmiID:        "test-ami-4-id",
+					CreationDate: "2021-08-31T00:10:42.000Z",
+					Deprecated:   true,
+					Requirements: scheduling.NewRequirements(),
+				},
+				{
+					Name:         "test-ami-1",
+					AmiID:        "test-ami-3-id",
+					CreationDate: "2021-08-31T00:10:42.000Z",
+					Deprecated:   true,
+					Requirements: scheduling.NewRequirements(),
+				},
+				{
+					Name:         "test-ami-1",
+					AmiID:        "test-ami-2-id",
+					CreationDate: "2021-08-31T00:10:42.000Z",
+					Deprecated:   true,
+					Requirements: scheduling.NewRequirements(),
+				},
+				{
+					Name:         "test-ami-1",
+					AmiID:        "test-ami-1-id",
+					CreationDate: "2021-08-31T00:10:42.000Z",
+					Deprecated:   true,
+					Requirements: scheduling.NewRequirements(),
+				},
+			}
+
+			amis.Sort()
+			Expect(amis).To(Equal(
+				amifamily.AMIs{
+					{
+						Name:         "test-ami-1",
+						AmiID:        "test-ami-1-id",
+						CreationDate: "2021-08-31T00:10:42.000Z",
+						Deprecated:   true,
+						Requirements: scheduling.NewRequirements(),
+					},
+					{
+						Name:         "test-ami-1",
+						AmiID:        "test-ami-2-id",
+						CreationDate: "2021-08-31T00:10:42.000Z",
+						Deprecated:   true,
+						Requirements: scheduling.NewRequirements(),
+					},
+					{
+						Name:         "test-ami-1",
+						AmiID:        "test-ami-3-id",
+						CreationDate: "2021-08-31T00:10:42.000Z",
+						Deprecated:   true,
+						Requirements: scheduling.NewRequirements(),
+					},
+					{
+						Name:         "test-ami-1",
+						AmiID:        "test-ami-4-id",
+						CreationDate: "2021-08-31T00:10:42.000Z",
+						Deprecated:   true,
+						Requirements: scheduling.NewRequirements(),
+					},
+				},
+			))
+		})
 	})
 })