diff --git a/pkg/actuators/machine/actuator_test.go b/pkg/actuators/machine/actuator_test.go index 0e65699b5e..0c75396504 100644 --- a/pkg/actuators/machine/actuator_test.go +++ b/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) } }) } diff --git a/pkg/actuators/machine/awsclient_test.go b/pkg/actuators/machine/awsclient_test.go index 64e6c08f08..6d393b37c4 100644 --- a/pkg/actuators/machine/awsclient_test.go +++ b/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(""), diff --git a/pkg/actuators/machine/instances_test.go b/pkg/actuators/machine/instances_test.go index e659706c1e..f784cf0332 100644 --- a/pkg/actuators/machine/instances_test.go +++ b/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()), }, diff --git a/pkg/actuators/machine/stubs.go b/pkg/actuators/machine/stubs.go index 4f0c673470..f4e28396ca 100644 --- a/pkg/actuators/machine/stubs.go +++ b/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()), diff --git a/pkg/actuators/machine/utils.go b/pkg/actuators/machine/utils.go index dbbbd40728..ec1a8b11d0 100644 --- a/pkg/actuators/machine/utils.go +++ b/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, - }) - } request := &ec2.DescribeInstancesInput{ Filters: requestFilters, @@ -176,9 +178,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) + } } } diff --git a/pkg/client/fake/fake.go b/pkg/client/fake/fake.go index bb9c6ce5ab..d4377fd9c6 100644 --- a/pkg/client/fake/fake.go +++ b/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()),