pkg/actuators/machine/utils: Client-side state filtering in getInstanceByID and getInstances

pkg/actuators/machine/actuator_test.go

@@ -169,7 +169,7 @@ func TestMachineEvents(t *testing.T) {
 
 			mockAWSClient.EXPECT().RunInstances(gomock.Any()).Return(stubReservation("ami-a9acbbd6", "i-02fcb933c5da7085c"), tc.runInstancesErr).AnyTimes()
 			if tc.describeInstancesOutput == nil {
-				mockAWSClient.EXPECT().DescribeInstances(gomock.Any()).Return(stubDescribeInstancesOutput("ami-a9acbbd6", "i-02fcb933c5da7085c"), tc.describeInstancesErr).AnyTimes()
+				mockAWSClient.EXPECT().DescribeInstances(gomock.Any()).Return(stubDescribeInstancesOutput("ami-a9acbbd6", "i-02fcb933c5da7085c", ec2.InstanceStateNameRunning), tc.describeInstancesErr).AnyTimes()
 			} else {
 				mockAWSClient.EXPECT().DescribeInstances(gomock.Any()).Return(tc.describeInstancesOutput, tc.describeInstancesErr).AnyTimes()
 			}
@@ -546,7 +546,7 @@ func TestActuator(t *testing.T) {
 			mockAWSClient.EXPECT().RunInstances(gomock.Any()).Return(stubReservation("ami-a9acbbd6", "i-02fcb933c5da7085c"), tc.runInstancesErr).AnyTimes()
 
 			if tc.describeInstancesOutput == nil {
-				mockAWSClient.EXPECT().DescribeInstances(gomock.Any()).Return(stubDescribeInstancesOutput("ami-a9acbbd6", "i-02fcb933c5da7085c"), tc.describeInstancesErr).AnyTimes()
+				mockAWSClient.EXPECT().DescribeInstances(gomock.Any()).Return(stubDescribeInstancesOutput("ami-a9acbbd6", "i-02fcb933c5da7085c", ec2.InstanceStateNameRunning), tc.describeInstancesErr).AnyTimes()
 			} else {
 				mockAWSClient.EXPECT().DescribeInstances(gomock.Any()).Return(tc.describeInstancesOutput, tc.describeInstancesErr).AnyTimes()
 			}
@@ -656,7 +656,7 @@ func TestAvailabilityZone(t *testing.T) {
 							ImageId:    aws.String("ami-a9acbbd6"),
 							InstanceId: aws.String("i-02fcb933c5da7085c"),
 							State: &ec2.InstanceState{
-								Name: aws.String("Running"),
+								Name: aws.String(ec2.InstanceStateNameRunning),
 							},
 							LaunchTime: aws.Time(time.Now()),
 							Placement: &ec2.Placement{
@@ -675,7 +675,7 @@ func TestAvailabilityZone(t *testing.T) {
 									ImageId:    aws.String("ami-a9acbbd6"),
 									InstanceId: aws.String("i-02fcb933c5da7085c"),
 									State: &ec2.InstanceState{
-										Name: aws.String("Running"),
+										Name: aws.String(ec2.InstanceStateNameRunning),
 										Code: aws.Int64(16),
 									},
 									LaunchTime: aws.Time(time.Now()),
@@ -804,7 +804,7 @@ func TestCreate(t *testing.T) {
 	mockAWSClient.EXPECT().DescribeSecurityGroups(gomock.Any()).Return(nil, fmt.Errorf("describeSecurityGroups error")).AnyTimes()
 	mockAWSClient.EXPECT().DescribeAvailabilityZones(gomock.Any()).Return(nil, fmt.Errorf("describeAvailabilityZones error")).AnyTimes()
 	mockAWSClient.EXPECT().DescribeImages(gomock.Any()).Return(nil, fmt.Errorf("describeImages error")).AnyTimes()
-	mockAWSClient.EXPECT().DescribeInstances(gomock.Any()).Return(stubDescribeInstancesOutput("ami-a9acbbd6", "i-02fcb933c5da7085c"), nil).AnyTimes()
+	mockAWSClient.EXPECT().DescribeInstances(gomock.Any()).Return(stubDescribeInstancesOutput("ami-a9acbbd6", "i-02fcb933c5da7085c", ec2.InstanceStateNameRunning), nil).AnyTimes()
 	mockAWSClient.EXPECT().TerminateInstances(gomock.Any()).Return(&ec2.TerminateInstancesOutput{}, nil).AnyTimes()
 	mockAWSClient.EXPECT().RunInstances(gomock.Any()).Return(stubReservation("ami-a9acbbd6", "i-02fcb933c5da7085c"), nil).AnyTimes()
 	mockAWSClient.EXPECT().RegisterInstancesWithLoadBalancer(gomock.Any()).Return(nil, nil).AnyTimes()
@@ -1175,7 +1175,7 @@ func TestGetMachineInstances(t *testing.T) {
 				}
 
 				mockAWSClient.EXPECT().DescribeInstances(request).Return(
-					stubDescribeInstancesOutput(imageID, instanceID),
+					stubDescribeInstancesOutput(imageID, instanceID, ec2.InstanceStateNameRunning),
 					nil,
 				).Times(1)
 
@@ -1184,7 +1184,7 @@ func TestGetMachineInstances(t *testing.T) {
 			exists: true,
 		},
 		{
-			testcase: "has-status-search-by-id",
+			testcase: "has-status-search-by-id-running",
 			providerStatus: providerconfigv1.AWSMachineProviderStatus{
 				InstanceID: aws.String(instanceID),
 			},
@@ -1196,7 +1196,7 @@ func TestGetMachineInstances(t *testing.T) {
 				}
 
 				mockAWSClient.EXPECT().DescribeInstances(request).Return(
-					stubDescribeInstancesOutput(imageID, instanceID),
+					stubDescribeInstancesOutput(imageID, instanceID, ec2.InstanceStateNameRunning),
 					nil,
 				).Times(1)
 
@@ -1205,23 +1205,21 @@ func TestGetMachineInstances(t *testing.T) {
 			exists: true,
 		},
 		{
-			testcase: "has-status-search-by-id and machine is terminated",
+			testcase: "has-status-search-by-id-terminated",
 			providerStatus: providerconfigv1.AWSMachineProviderStatus{
 				InstanceID: aws.String(instanceID),
 			},
 			awsClientFunc: func(ctrl *gomock.Controller) awsclient.Client {
 				mockAWSClient := mockaws.NewMockClient(ctrl)
 
-				request := &ec2.DescribeInstancesInput{
+				first := mockAWSClient.EXPECT().DescribeInstances(&ec2.DescribeInstancesInput{
 					InstanceIds: aws.StringSlice([]string{instanceID}),
-				}
-
-				mockAWSClient.EXPECT().DescribeInstances(request).Return(
-					stubTerminatedInstanceDescribeInstancesOutput(imageID, instanceID),
+				}).Return(
+					stubDescribeInstancesOutput(imageID, instanceID, ec2.InstanceStateNameTerminated),
 					nil,
 				).Times(1)
 
-				request2 := &ec2.DescribeInstancesInput{
+				mockAWSClient.EXPECT().DescribeInstances(&ec2.DescribeInstancesInput{
 					Filters: []*ec2.Filter{
 						{
 							Name:   awsTagFilter("Name"),
@@ -1230,16 +1228,13 @@ func TestGetMachineInstances(t *testing.T) {
 
 						clusterFilter(clusterID),
 					},
-				}
-
-				mockAWSClient.EXPECT().DescribeInstances(request2).Return(
-					stubTerminatedInstanceDescribeInstancesOutput(imageID, instanceID),
+				}).Return(
+					stubDescribeInstancesOutput(imageID, instanceID, ec2.InstanceStateNameTerminated),
 					nil,
-				).Times(1)
+				).Times(1).After(first)
 
 				return mockAWSClient
 			},
-			exists: false,
 		},
 	}
 
@@ -1268,12 +1263,12 @@ func TestGetMachineInstances(t *testing.T) {
 				t.Errorf("Error creating Actuator: %v", err)
 			}
 
-			instance, err := actuator.getMachineInstances(nil, machineCopy)
+			instances, err := actuator.getMachineInstances(nil, machineCopy)
 			if err != nil {
 				t.Errorf("Unexpected error from getMachineInstances: %v", err)
 			}
-			if tc.exists != (instance != nil) {
-				t.Errorf("Expected instance exists: %t, got instance: %v", tc.exists, instance)
+			if tc.exists != (len(instances) > 0) {
+				t.Errorf("Expected instance exists: %t, got instances: %v", tc.exists, instances)
 			}
 		})
 	}

pkg/actuators/machine/awsclient_test.go

@@ -44,7 +44,7 @@ func TestAwsClient(t *testing.T) {
 								ImageId:    aws.String("ami-a9acbbd6"),
 								InstanceId: aws.String("i-02fcb933c5da7085c"),
 								State: &ec2.InstanceState{
-									Name: aws.String("Running"),
+									Name: aws.String(ec2.InstanceStateNameRunning),
 								},
 								LaunchTime:       aws.Time(time.Now()),
 								PublicDnsName:    aws.String(""),

pkg/actuators/machine/instances_test.go

@@ -239,7 +239,7 @@ func TestRunningInstance(t *testing.T) {
 						ImageId:    aws.String("ami-a9acbbd6"),
 						InstanceId: aws.String("i-02fcb933c5da7085c"),
 						State: &ec2.InstanceState{
-							Name: aws.String("Running"),
+							Name: aws.String(ec2.InstanceStateNameRunning),
 						},
 						LaunchTime: aws.Time(time.Now()),
 					},

pkg/actuators/machine/stubs.go

@@ -180,7 +180,7 @@ func stubInstance(imageID, instanceID string) *ec2.Instance {
 		ImageId:    aws.String(imageID),
 		InstanceId: aws.String(instanceID),
 		State: &ec2.InstanceState{
-			Name: aws.String("Running"),
+			Name: aws.String(ec2.InstanceStateNameRunning),
 			Code: aws.Int64(16),
 		},
 		LaunchTime:       aws.Time(time.Now()),
@@ -261,7 +261,7 @@ func stubReservation(imageID, instanceID string) *ec2.Reservation {
 				ImageId:    aws.String(imageID),
 				InstanceId: aws.String(instanceID),
 				State: &ec2.InstanceState{
-					Name: aws.String("Running"),
+					Name: aws.String(ec2.InstanceStateNameRunning),
 					Code: aws.Int64(16),
 				},
 				LaunchTime: aws.Time(time.Now()),
@@ -273,7 +273,7 @@ func stubReservation(imageID, instanceID string) *ec2.Reservation {
 	}
 }
 
-func stubDescribeInstancesOutput(imageID, instanceID string) *ec2.DescribeInstancesOutput {
+func stubDescribeInstancesOutput(imageID, instanceID string, state string) *ec2.DescribeInstancesOutput {
 	return &ec2.DescribeInstancesOutput{
 		Reservations: []*ec2.Reservation{
 			{
@@ -282,27 +282,7 @@ func stubDescribeInstancesOutput(imageID, instanceID string) *ec2.DescribeInstan
 						ImageId:    aws.String(imageID),
 						InstanceId: aws.String(instanceID),
 						State: &ec2.InstanceState{
-							Name: aws.String("Running"),
-							Code: aws.Int64(16),
-						},
-						LaunchTime: aws.Time(time.Now()),
-					},
-				},
-			},
-		},
-	}
-}
-
-func stubTerminatedInstanceDescribeInstancesOutput(imageID, instanceID string) *ec2.DescribeInstancesOutput {
-	return &ec2.DescribeInstancesOutput{
-		Reservations: []*ec2.Reservation{
-			{
-				Instances: []*ec2.Instance{
-					{
-						ImageId:    aws.String(imageID),
-						InstanceId: aws.String(instanceID),
-						State: &ec2.InstanceState{
-							Name: aws.String(ec2.InstanceStateNameTerminated),
+							Name: aws.String(state),
 							Code: aws.Int64(16),
 						},
 						LaunchTime: aws.Time(time.Now()),

pkg/actuators/machine/utils.go

@@ -18,6 +18,7 @@ package machine
 
 import (
 	"fmt"
+	"strings"
 
 	"github.com/golang/glog"
 
@@ -87,37 +88,42 @@ func getStoppedInstances(machine *machinev1.Machine, client awsclient.Client) ([
 
 // getExistingInstances returns all instances not terminated
 func getExistingInstances(machine *machinev1.Machine, client awsclient.Client) ([]*ec2.Instance, error) {
-	instances, err := getInstances(machine, client, nil)
-	if err != nil {
-		return nil, err
+	return getInstances(machine, client, existingInstanceStates())
+}
+
+func getExistingInstanceByID(id string, client awsclient.Client) (*ec2.Instance, error) {
+	return getInstanceByID(id, client, existingInstanceStates())
+}
+
+func instanceHasAllowedState(instance *ec2.Instance, instanceStateFilter []*string) error {
+	if instance.InstanceId == nil {
+		return fmt.Errorf("instance has nil ID")
 	}
 
-	var existingInstances []*ec2.Instance
-	for key := range instances {
-		if (instances[key].State) != nil {
-			if aws.StringValue(instances[key].State.Name) != ec2.InstanceStateNameTerminated {
-				existingInstances = append(existingInstances, instances[key])
-			}
-		}
+	if instance.State == nil {
+		return fmt.Errorf("instance %s has nil state", *instance.InstanceId)
 	}
-	return existingInstances, nil
-}
 
-func getExistingInstanceByID(id string, client awsclient.Client) (*ec2.Instance, error) {
-	instance, err := getInstanceByID(id, client)
-	if err != nil {
-		return nil, err
+	if len(instanceStateFilter) == 0 {
+		return nil
 	}
-	if instance.State != nil {
-		if aws.StringValue(instance.State.Name) == ec2.InstanceStateNameTerminated {
-			return nil, fmt.Errorf("failed to getExistingInstanceByID for instance-id %s, instance is terminated", id)
+
+	actualState := aws.StringValue(instance.State.Name)
+	for _, allowedState := range instanceStateFilter {
+		if aws.StringValue(allowedState) == actualState {
+			return nil
 		}
 	}
-	return instance, nil
+
+	allowedStates := make([]string, 0, len(instanceStateFilter))
+	for _, allowedState := range instanceStateFilter {
+		allowedStates = append(allowedStates, aws.StringValue(allowedState))
+	}
+	return fmt.Errorf("instance %s state %q is not in %s", *instance.InstanceId, actualState, strings.Join(allowedStates, ", "))
 }
 
 // getInstanceByID returns the instance with the given ID if it exists.
-func getInstanceByID(id string, client awsclient.Client) (*ec2.Instance, error) {
+func getInstanceByID(id string, client awsclient.Client, instanceStateFilter []*string) (*ec2.Instance, error) {
 	if id == "" {
 		return nil, fmt.Errorf("instance-id not specified")
 	}
@@ -141,7 +147,9 @@ func getInstanceByID(id string, client awsclient.Client) (*ec2.Instance, error)
 		return nil, fmt.Errorf("found %d instances for instance-id %s", len(reservation.Instances), id)
 	}
 
-	return reservation.Instances[0], nil
+	instance := reservation.Instances[0]
+
+	return instance, instanceHasAllowedState(instance, instanceStateFilter)
 }
 
 // getInstances returns all instances that have a tag matching our machine name,
@@ -159,12 +167,6 @@ func getInstances(machine *machinev1.Machine, client awsclient.Client, instanceS
 		},
 		clusterFilter(clusterID),
 	}
-	if instanceStateFilter != nil {
-		requestFilters = append(requestFilters, &ec2.Filter{
-			Name:   aws.String("instance-state-name"),
-			Values: instanceStateFilter,
-		})
-	}
 
 	// Query instances with our machine's name, and in running/pending state.
 	request := &ec2.DescribeInstancesInput{
@@ -177,9 +179,15 @@ func getInstances(machine *machinev1.Machine, client awsclient.Client, instanceS
 	}
 
 	instances := make([]*ec2.Instance, 0, len(result.Reservations))
+
 	for _, reservation := range result.Reservations {
 		for _, instance := range reservation.Instances {
-			instances = append(instances, instance)
+			err := instanceHasAllowedState(instance, instanceStateFilter)
+			if err != nil {
+				glog.Errorf("Excluding instance matching %s: %v", machine.Name, err)
+			} else {
+				instances = append(instances, instance)
+			}
 		}
 	}
 

pkg/client/fake/fake.go

@@ -81,7 +81,7 @@ func (c *awsClient) DescribeInstances(input *ec2.DescribeInstancesInput) (*ec2.D
 						ImageId:    aws.String("ami-a9acbbd6"),
 						InstanceId: aws.String("i-02fcb933c5da7085c"),
 						State: &ec2.InstanceState{
-							Name: aws.String("Running"),
+							Name: aws.String(ec2.InstanceStateNameRunning),
 							Code: aws.Int64(16),
 						},
 						LaunchTime: aws.Time(time.Now()),