Merge pull request #1108 from chuckha/time-error

🐛 handle an unhandled error

pkg/cloud/services/ec2/ami.go

@@ -86,7 +86,11 @@ func (s *Service) defaultAMILookup(ownerID, baseOS, baseOSVersion, kubernetesVer
 	if len(out.Images) == 0 {
 		return "", errors.Errorf("found no AMIs with the name: %q", amiName(baseOS, baseOSVersion, kubernetesVersion))
 	}
-	latestImage := getLatestImage(out.Images)
+	latestImage, err := getLatestImage(out.Images)
+	if err != nil {
+		s.scope.Error(err, "failed getting latest image from AMI list")
+		return "", err
+	}
 	s.scope.V(2).Info("Found and using an existing AMI", "ami-id", aws.StringValue(latestImage.ImageId))
 	return aws.StringValue(latestImage.ImageId), nil
 }
@@ -100,16 +104,10 @@ func (i images) Len() int {
 
 // Less reports whether the element with
 // index i should sort before the element with index j.
-// TODO(chuckha) Ignoring errors until this causes a problem
+// At this point all CreationDates have been checked for errors so ignoring the error is ok.
 func (i images) Less(k, j int) bool {
-	firstTime, err := time.Parse(createDateTimestampFormat, aws.StringValue(i[k].CreationDate))
-	if err != nil {
-		return false
-	}
-	secondTime, err := time.Parse(createDateTimestampFormat, aws.StringValue(i[j].CreationDate))
-	if err != nil {
-		return false
-	}
+	firstTime, _ := time.Parse(createDateTimestampFormat, aws.StringValue(i[k].CreationDate))
+	secondTime, _ := time.Parse(createDateTimestampFormat, aws.StringValue(i[j].CreationDate))
 	return firstTime.Before(secondTime)
 }
 
@@ -119,10 +117,15 @@ func (i images) Swap(k, j int) {
 }
 
 // getLatestImage assumes imgs is not empty. Responsibility of the caller to check.
-func getLatestImage(imgs []*ec2.Image) *ec2.Image {
+func getLatestImage(imgs []*ec2.Image) (*ec2.Image, error) {
+	for _, img := range imgs {
+		if _, err := time.Parse(createDateTimestampFormat, aws.StringValue(img.CreationDate)); err != nil {
+			return nil, err
+		}
+	}
 	// old to new (newest one is last)
 	sort.Sort(images(imgs))
-	return imgs[len(imgs)-1]
+	return imgs[len(imgs)-1], nil
 }
 
 func (s *Service) defaultBastionAMILookup(region string) string {

pkg/cloud/services/ec2/ami_test.go

@@ -89,3 +89,61 @@ func TestAMIs(t *testing.T) {
 		})
 	}
 }
+
+func TestAMIsWithInvalidCreationDate(t *testing.T) {
+	mockCtrl := gomock.NewController(t)
+	defer mockCtrl.Finish()
+
+	testCases := []struct {
+		name   string
+		expect func(m *mock_ec2iface.MockEC2APIMockRecorder)
+	}{
+		{
+			name: "simple test",
+			expect: func(m *mock_ec2iface.MockEC2APIMockRecorder) {
+				m.DescribeImages(gomock.AssignableToTypeOf(&ec2.DescribeImagesInput{})).
+					Return(&ec2.DescribeImagesOutput{
+						Images: []*ec2.Image{
+							{
+								ImageId:      aws.String("ancient"),
+								CreationDate: aws.String("2011-02-08T17:02:31.000Z"),
+							},
+							{
+								ImageId:      aws.String("pretty new"),
+								CreationDate: aws.String("invalid creation date"),
+							},
+							{
+								ImageId:      aws.String("pretty old"),
+								CreationDate: aws.String("2014-02-08T17:02:31.000Z"),
+							},
+						},
+					}, nil)
+			},
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			ec2Mock := mock_ec2iface.NewMockEC2API(mockCtrl)
+
+			scope, err := scope.NewClusterScope(scope.ClusterScopeParams{
+				Cluster:    &clusterv1.Cluster{},
+				AWSCluster: &infrav1.AWSCluster{},
+				AWSClients: scope.AWSClients{
+					EC2: ec2Mock,
+				},
+			})
+			if err != nil {
+				t.Fatalf("did not expect err: %v", err)
+			}
+
+			tc.expect(ec2Mock.EXPECT())
+
+			s := NewService(scope)
+			_, err = s.defaultAMILookup("", "base os", "baseos version", "1.11.1")
+			if err == nil {
+				t.Fatalf("expected an error but did not get one")
+			}
+		})
+	}
+}