Merge pull request #485 from saurav-agarwalla/automated-cherry-pick-o…

…f-#484-release-1.20

Automated cherry pick of #484: Handle wrapped error for InvalidInstanceID.NotFound in

pkg/providers/v1/aws.go

@@ -44,6 +44,7 @@ import (
 	"github.com/aws/aws-sdk-go/aws/session"
 	"github.com/aws/aws-sdk-go/service/autoscaling"
 	"github.com/aws/aws-sdk-go/service/ec2"
+	"github.com/aws/aws-sdk-go/service/ec2/ec2iface"
 	"github.com/aws/aws-sdk-go/service/elb"
 	"github.com/aws/aws-sdk-go/service/elbv2"
 	"github.com/aws/aws-sdk-go/service/kms"
@@ -724,7 +725,7 @@ func (cfg *CloudConfig) getResolver() endpoints.ResolverFunc {
 
 // awsSdkEC2 is an implementation of the EC2 interface, backed by aws-sdk-go
 type awsSdkEC2 struct {
-	ec2 *ec2.EC2
+	ec2 ec2iface.EC2API
 }
 
 // Interface to make the CloudConfig immutable for awsSDKProvider
@@ -951,6 +952,13 @@ func (s *awsSdkEC2) DescribeInstances(request *ec2.DescribeInstancesInput) ([]*e
 	results := []*ec2.Instance{}
 	var nextToken *string
 	requestTime := time.Now()
+
+	if request.MaxResults == nil && len(request.InstanceIds) == 0 {
+		// MaxResults must be set in order for pagination to work
+		// MaxResults cannot be set with InstanceIds
+		request.MaxResults = aws.Int64(1000)
+	}
+
 	for {
 		response, err := s.ec2.DescribeInstances(request)
 		if err != nil {
@@ -1948,6 +1956,10 @@ func isAWSErrorInstanceNotFound(err error) bool {
 		if awsError.Code() == ec2.UnsuccessfulInstanceCreditSpecificationErrorCodeInvalidInstanceIdNotFound {
 			return true
 		}
+	} else if strings.Contains(err.Error(), ec2.UnsuccessfulInstanceCreditSpecificationErrorCodeInvalidInstanceIdNotFound) {
+		// In places like https://github.com/kubernetes/cloud-provider-aws/blob/1c6194aad0122ab44504de64187e3d1a7415b198/pkg/providers/v1/aws.go#L1007,
+		// the error has been transformed into something else so check the error string to see if it contains the error code we're looking for.
+		return true
 	}
 
 	return false

pkg/providers/v1/aws_test.go

@@ -32,6 +32,7 @@ import (
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/aws/awserr"
 	"github.com/aws/aws-sdk-go/service/ec2"
+	"github.com/aws/aws-sdk-go/service/ec2/ec2iface"
 	"github.com/aws/aws-sdk-go/service/elb"
 	"github.com/aws/aws-sdk-go/service/elbv2"
 	"github.com/stretchr/testify/assert"
@@ -3112,6 +3113,17 @@ func TestInstanceExistsByProviderIDWithNodeNameForFargate(t *testing.T) {
 	assert.True(t, instanceExist)
 }
 
+func TestInstanceExistsByProviderIDForInstanceNotFound(t *testing.T) {
+	mockedEC2API := newMockedEC2API()
+	c := &Cloud{ec2: &awsSdkEC2{ec2: mockedEC2API}}
+
+	mockedEC2API.On("DescribeInstances", mock.Anything).Return(&ec2.DescribeInstancesOutput{}, awserr.New("InvalidInstanceID.NotFound", "Instance not found", nil))
+
+	instanceExists, err := c.InstanceExistsByProviderID(context.TODO(), "aws:///us-west-2c/1abc-2def/i-not-found")
+	assert.Nil(t, err)
+	assert.False(t, instanceExists)
+}
+
 func TestInstanceNotExistsByProviderIDForFargate(t *testing.T) {
 	awsServices := newMockedFakeAWSServices(TestClusterID)
 	c, _ := newAWSCloud(CloudConfig{}, awsServices)
@@ -3156,3 +3168,105 @@ func TestGetZoneByProviderIDForFargate(t *testing.T) {
 	assert.Nil(t, err)
 	assert.Equal(t, "us-west-2c", zoneDetails.FailureDomain)
 }
+
+type MockedEC2API struct {
+	ec2iface.EC2API
+	mock.Mock
+}
+
+func (m *MockedEC2API) DescribeInstances(input *ec2.DescribeInstancesInput) (*ec2.DescribeInstancesOutput, error) {
+	args := m.Called(input)
+	return args.Get(0).(*ec2.DescribeInstancesOutput), args.Error(1)
+}
+
+func newMockedEC2API() *MockedEC2API {
+	return &MockedEC2API{}
+}
+
+func TestDescribeInstances(t *testing.T) {
+	tests := []struct {
+		name    string
+		input   *ec2.DescribeInstancesInput
+		expect  func(ec2iface.EC2API)
+		isError bool
+	}{
+		{
+			"MaxResults set on empty DescribeInstancesInput and NextToken respected.",
+			&ec2.DescribeInstancesInput{},
+			func(mockedEc2 ec2iface.EC2API) {
+				m := mockedEc2.(*MockedEC2API)
+				m.On("DescribeInstances",
+					&ec2.DescribeInstancesInput{
+						MaxResults: aws.Int64(1000),
+					},
+				).Return(
+					&ec2.DescribeInstancesOutput{
+						NextToken: aws.String("asdf"),
+					},
+					nil,
+				)
+				m.On("DescribeInstances",
+					&ec2.DescribeInstancesInput{
+						MaxResults: aws.Int64(1000),
+						NextToken:  aws.String("asdf"),
+					},
+				).Return(
+					&ec2.DescribeInstancesOutput{},
+					nil,
+				)
+			},
+			false,
+		},
+		{
+			"MaxResults only set if empty DescribeInstancesInput",
+			&ec2.DescribeInstancesInput{
+				MaxResults: aws.Int64(3),
+			},
+			func(mockedEc2 ec2iface.EC2API) {
+				m := mockedEc2.(*MockedEC2API)
+				m.On("DescribeInstances",
+					&ec2.DescribeInstancesInput{
+						MaxResults: aws.Int64(3),
+					},
+				).Return(
+					&ec2.DescribeInstancesOutput{},
+					nil,
+				)
+			},
+			false,
+		},
+		{
+			"MaxResults not set if instance IDs are provided",
+			&ec2.DescribeInstancesInput{
+				InstanceIds: []*string{aws.String("i-1234")},
+			},
+			func(mockedEc2 ec2iface.EC2API) {
+				m := mockedEc2.(*MockedEC2API)
+				m.On("DescribeInstances",
+					&ec2.DescribeInstancesInput{
+						InstanceIds: []*string{aws.String("i-1234")},
+					},
+				).Return(
+					&ec2.DescribeInstancesOutput{},
+					nil,
+				)
+			},
+			false,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			mockedEC2API := newMockedEC2API()
+			test.expect(mockedEC2API)
+			fakeEC2 := awsSdkEC2{
+				ec2: mockedEC2API,
+			}
+			_, err := fakeEC2.DescribeInstances(test.input)
+			if !test.isError {
+				assert.NoError(t, err)
+			}
+			mockedEC2API.AssertExpectations(t)
+		})
+	}
+}