Auto resolver bug (#219)

* Auto resolver updated The auto AMI resolver will now pick the latest AMI version to use. Previously it just picked the first image and used that. Issue #218 * Static Resolver updated to use latest AMIs The static resolver has been updated to use the latest (v24) images for the EKS nodes. This applies to both normal and GPU instance types.
eksctl-io · Sep 19, 2018 · 0281776 · 0281776
1 parent cd0c580
commit 0281776
Show file tree

Hide file tree

Showing 4 changed files with 110 additions and 21 deletions.
diff --git a/pkg/ami/api.go b/pkg/ami/api.go
@@ -1,6 +1,9 @@
 package ami
 
 import (
+	"sort"
+	"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"
@@ -42,7 +45,9 @@ func IsAvailable(api ec2iface.EC2API, id string) (bool, error) {
 	return *output.Images[0].State == "available", nil
 }
 
-// FindImage will get the AMI to use for the EKS nodes by querying AWS EC2 API
+// FindImage will get the AMI to use for the EKS nodes by querying AWS EC2 API.
+// It will only look for images with a status of available and it will pick the
+// image with the newest creation date.
 func FindImage(api ec2iface.EC2API, namePattern string) (string, error) {
 	input := &ec2.DescribeImagesInput{
 		Filters: []*ec2.Filter{
@@ -62,21 +67,32 @@ func FindImage(api ec2iface.EC2API, namePattern string) (string, error) {
 				Name:   aws.String("is-public"),
 				Values: []*string{aws.String("true")},
 			},
+			&ec2.Filter{
+				Name:   aws.String("state"),
+				Values: []*string{aws.String("available")},
+			},
 		},
 	}
 
 	output, err := api.DescribeImages(input)
 	if err != nil {
-		return "", errors.Wrapf(err, "cannot find image")
+		return "", errors.Wrapf(err, "error querying AWS for images")
 	}
 
 	if len(output.Images) < 1 {
 		return "", nil
 	}
 
-	if *output.Images[0].State == "available" {
+	if len(output.Images) == 1 {
 		return *output.Images[0].ImageId, nil
 	}
 
-	return "", nil
+	// Sort images so newest is first
+	sort.Slice(output.Images, func(i, j int) bool {
+		creationLeft, _ := time.Parse(time.RFC3339, *output.Images[i].CreationDate)
+		creationRight, _ := time.Parse(time.RFC3339, *output.Images[j].CreationDate)
+		return creationLeft.After(creationRight)
+	})
+
+	return *output.Images[0].ImageId, nil
 }
diff --git a/pkg/ami/auto_resolver_test.go b/pkg/ami/auto_resolver_test.go
@@ -12,9 +12,16 @@ import (
 	"github.com/weaveworks/eksctl/pkg/testutils"
 )
 
+type returnAmi struct {
+	imageId     string
+	state       string
+	createdDate string
+}
+
 var _ = Describe("AMI Auto Resolution", func() {
 
 	Describe("When resolving an AMI to use", func() {
+
 		var (
 			p            *testutils.MockProvider
 			err          error
@@ -41,7 +48,7 @@ var _ = Describe("AMI Auto Resolution", func() {
 						imageState = "available"
 
 						_, p = createProviders()
-						addMockDescribeImages(p, "amazon-eks-node-*", expectedAmi, imageState)
+						addMockDescribeImages(p, "amazon-eks-node-*", expectedAmi, imageState, "2018-08-20T23:25:53.000Z")
 
 						resolver := NewAutoResolver(p.MockEC2())
 						resolvedAmi, err = resolver.Resolve(region, instanceType)
@@ -60,12 +67,12 @@ var _ = Describe("AMI Auto Resolution", func() {
 					})
 				})
 
-				Context("and ami is pending", func() {
+				Context("and ami is NOT available", func() {
 					BeforeEach(func() {
 						imageState = "pending"
 
 						_, p = createProviders()
-						addMockDescribeImages(p, "amazon-eks-node-*", expectedAmi, imageState)
+						addMockDescribeImagesMultiple(p, "amazon-eks-node-*", []returnAmi{})
 
 						resolver := NewAutoResolver(p.MockEC2())
 						resolvedAmi, err = resolver.Resolve(region, instanceType)
@@ -83,6 +90,44 @@ var _ = Describe("AMI Auto Resolution", func() {
 						Expect(resolvedAmi).To(BeEquivalentTo(""))
 					})
 				})
+
+				Context("and there are 2 ami's available", func() {
+					BeforeEach(func() {
+						imageState = "available"
+						expectedAmi = "ami-5678"
+
+						_, p = createProviders()
+						images := []returnAmi{
+							returnAmi{
+								createdDate: "2018-08-20T23:25:53.000Z",
+								imageId:     "ami-1234",
+								state:       "available",
+							},
+							returnAmi{
+								createdDate: "2018-09-12T22:21:11.000Z",
+								imageId:     expectedAmi,
+								state:       "available",
+							},
+						}
+
+						addMockDescribeImagesMultiple(p, "amazon-eks-node-*", images)
+
+						resolver := NewAutoResolver(p.MockEC2())
+						resolvedAmi, err = resolver.Resolve(region, instanceType)
+					})
+
+					It("should not error", func() {
+						Expect(err).NotTo(HaveOccurred())
+					})
+
+					It("should have called AWS EC2 DescribeImages", func() {
+						Expect(p.MockEC2().AssertNumberOfCalls(GinkgoT(), "DescribeImages", 1)).To(BeTrue())
+					})
+
+					It("should have returned an ami id", func() {
+						Expect(resolvedAmi).To(BeEquivalentTo(expectedAmi))
+					})
+				})
 			})
 
 			Context("and gpu instance type", func() {
@@ -95,7 +140,7 @@ var _ = Describe("AMI Auto Resolution", func() {
 						imageState = "available"
 
 						_, p = createProviders()
-						addMockDescribeImages(p, "amazon-eks-gpu-node-*", expectedAmi, imageState)
+						addMockDescribeImages(p, "amazon-eks-gpu-node-*", expectedAmi, imageState, "2018-08-20T23:25:53.000Z")
 
 						resolver := NewAutoResolver(p.MockEC2())
 						resolvedAmi, err = resolver.Resolve(region, instanceType)
@@ -131,7 +176,7 @@ func createProviders() (*eks.ClusterProvider, *testutils.MockProvider) {
 	return c, p
 }
 
-func addMockDescribeImages(p *testutils.MockProvider, expectedNamePattern string, amiId string, amiState string) {
+func addMockDescribeImages(p *testutils.MockProvider, expectedNamePattern string, amiId string, amiState string, createdDate string) {
 	p.MockEC2().On("DescribeImages",
 		mock.MatchedBy(func(input *ec2.DescribeImagesInput) bool {
 			for _, filter := range input.Filters {
@@ -154,3 +199,31 @@ func addMockDescribeImages(p *testutils.MockProvider, expectedNamePattern string
 		},
 	}, nil)
 }
+
+func addMockDescribeImagesMultiple(p *testutils.MockProvider, expectedNamePattern string, returnAmis []returnAmi) {
+	images := make([]*ec2.Image, len(returnAmis))
+	for index, ami := range returnAmis {
+		images[index] = &ec2.Image{
+			ImageId:      aws.String(ami.imageId),
+			State:        aws.String(ami.state),
+			CreationDate: aws.String(ami.createdDate),
+		}
+	}
+
+	p.MockEC2().On("DescribeImages",
+		mock.MatchedBy(func(input *ec2.DescribeImagesInput) bool {
+			for _, filter := range input.Filters {
+				if *filter.Name == "name" {
+					if len(filter.Values) > 0 {
+						if *filter.Values[0] == expectedNamePattern {
+							return true
+						}
+					}
+				}
+			}
+			return false
+		}),
+	).Return(&ec2.DescribeImagesOutput{
+		Images: images,
+	}, nil)
+}
diff --git a/pkg/ami/static_resolver.go b/pkg/ami/static_resolver.go
@@ -16,14 +16,14 @@ import (
 var StaticImages = map[string]map[int]map[string]string{
 	ImageFamilyAmazonLinux2: {
 		ImageClassGeneral: {
-			api.EKS_REGION_US_WEST_2: "ami-08cab282f9979fc7a",
-			api.EKS_REGION_US_EAST_1: "ami-0b2ae3c6bda8b5c06",
-			api.EKS_REGION_EU_WEST_1: "ami-066110c1a7466949e",
+			api.EKS_REGION_US_WEST_2: "ami-0a54c984b9f908c81",
+			api.EKS_REGION_US_EAST_1: "ami-0440e4f6b9713faf6",
+			api.EKS_REGION_EU_WEST_1: "ami-0c7a4976cb6fafd3a",
 		},
 		ImageClassGPU: {
-			api.EKS_REGION_US_WEST_2: "ami-0d20f2404b9a1c4d1",
-			api.EKS_REGION_US_EAST_1: "ami-09fe6fc9106bda972",
-			api.EKS_REGION_EU_WEST_1: "ami-09e0c6b3d3cf906f1",
+			api.EKS_REGION_US_WEST_2: "ami-0731694d53ef9604b",
+			api.EKS_REGION_US_EAST_1: "ami-058bfb8c236caae89",
+			api.EKS_REGION_EU_WEST_1: "ami-0706dc8a5eed2eed9",
 		},
 	},
 }

diff --git a/pkg/ami/static_resolver_test.go b/pkg/ami/static_resolver_test.go
@@ -33,19 +33,19 @@ var _ = Describe("AMI Static Resolution", func() {
 		Entry("with non-gpu instance and us-west-2", ResolveCase{
 			Region:       "us-west-2",
 			InstanceType: "t2.medium",
-			ExpectedAMI:  "ami-08cab282f9979fc7a",
+			ExpectedAMI:  "ami-0a54c984b9f908c81",
 			ExpectError:  false,
 		}),
 		Entry("with non-gpu instance and us-east-1", ResolveCase{
 			Region:       "us-east-1",
 			InstanceType: "t2.medium",
-			ExpectedAMI:  "ami-0b2ae3c6bda8b5c06",
+			ExpectedAMI:  "ami-0440e4f6b9713faf6",
 			ExpectError:  false,
 		}),
 		Entry("with non-gpu instance and eu-west-1", ResolveCase{
 			Region:       "eu-west-1",
 			InstanceType: "t2.medium",
-			ExpectedAMI:  "ami-066110c1a7466949e",
+			ExpectedAMI:  "ami-0c7a4976cb6fafd3a",
 			ExpectError:  false,
 		}),
 		Entry("with non-gpu instance and non-eks enabled region", ResolveCase{
@@ -57,19 +57,19 @@ var _ = Describe("AMI Static Resolution", func() {
 		Entry("with gpu (p2) instance and us-west-2", ResolveCase{
 			Region:       "us-west-2",
 			InstanceType: "p2.xlarge",
-			ExpectedAMI:  "ami-0d20f2404b9a1c4d1",
+			ExpectedAMI:  "ami-0731694d53ef9604b",
 			ExpectError:  false,
 		}),
 		Entry("with gpu (p3) instance and us-east-1", ResolveCase{
 			Region:       "us-east-1",
 			InstanceType: "p3.2xlarge",
-			ExpectedAMI:  "ami-09fe6fc9106bda972",
+			ExpectedAMI:  "ami-058bfb8c236caae89",
 			ExpectError:  false,
 		}),
 		Entry("with gpu (p2) instance and eu-west-1", ResolveCase{
 			Region:       "eu-west-1",
 			InstanceType: "p2.xlarge",
-			ExpectedAMI:  "ami-09e0c6b3d3cf906f1",
+			ExpectedAMI:  "ami-0706dc8a5eed2eed9",
 			ExpectError:  false,
 		}),
 		Entry("with gpu (p3) instance and non-eks enabled region", ResolveCase{