Merge pull request #102927 from kishorj/ec2-instance-cache-maxage

Set EC2 instance cache max age to 10 mins

staging/src/k8s.io/legacy-cloud-providers/aws/aws_loadbalancer.go

@@ -26,6 +26,7 @@ import (
 	"regexp"
 	"strconv"
 	"strings"
+	"time"
 
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/aws/awserr"
@@ -52,6 +53,9 @@ const (
 	lbAttrAccessLogsS3Enabled           = "access_logs.s3.enabled"
 	lbAttrAccessLogsS3Bucket            = "access_logs.s3.bucket"
 	lbAttrAccessLogsS3Prefix            = "access_logs.s3.prefix"
+
+	// defaultEC2InstanceCacheMaxAge is the max age for the EC2 instance cache
+	defaultEC2InstanceCacheMaxAge = 10 * time.Minute
 )
 
 var (
@@ -1606,7 +1610,7 @@ func (c *Cloud) findInstancesForELB(nodes []*v1.Node, annotations map[string]str
 	instanceIDs := mapToAWSInstanceIDsTolerant(targetNodes)
 
 	cacheCriteria := cacheCriteria{
-		// MaxAge not required, because we only care about security groups, which should not change
+		MaxAge:       defaultEC2InstanceCacheMaxAge,
 		HasInstances: instanceIDs, // Refresh if any of the instance ids are missing
 	}
 	snapshot, err := c.instanceCache.describeAllInstancesCached(cacheCriteria)

staging/src/k8s.io/legacy-cloud-providers/aws/aws_loadbalancer_test.go

@@ -19,15 +19,20 @@ limitations under the License.
 package aws
 
 import (
-	"k8s.io/apimachinery/pkg/types"
+	"fmt"
+	"reflect"
 	"testing"
+	"time"
+
+	v1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/types"
 
 	"github.com/aws/aws-sdk-go/aws"
+	"github.com/aws/aws-sdk-go/service/ec2"
 	"github.com/aws/aws-sdk-go/service/elb"
 	"github.com/aws/aws-sdk-go/service/elbv2"
 	"github.com/stretchr/testify/assert"
-
-	"k8s.io/api/core/v1"
 )
 
 func TestElbProtocolsAreEqual(t *testing.T) {
@@ -542,6 +547,86 @@ func TestFilterTargetNodes(t *testing.T) {
 	}
 }
 
+func makeNodeInstancePair(offset int) (*v1.Node, *ec2.Instance) {
+	instanceID := fmt.Sprintf("i-%x", int64(0x03bcc3496da09f78e)+int64(offset))
+	instance := &ec2.Instance{
+		InstanceId: aws.String(instanceID),
+		Placement: &ec2.Placement{
+			AvailabilityZone: aws.String("us-east-1b"),
+		},
+		PrivateDnsName:   aws.String(fmt.Sprintf("ip-192-168-32-%d.ec2.internal", 101+offset)),
+		PrivateIpAddress: aws.String(fmt.Sprintf("192.168.32.%d", 101+offset)),
+		PublicIpAddress:  aws.String(fmt.Sprintf("1.2.3.%d", 1+offset)),
+	}
+
+	var tag ec2.Tag
+	tag.Key = aws.String(fmt.Sprintf("%s%s", TagNameKubernetesClusterPrefix, TestClusterID))
+	tag.Value = aws.String("owned")
+	instance.Tags = []*ec2.Tag{&tag}
+
+	node := &v1.Node{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: fmt.Sprintf("ip-192-168-0-%d.ec2.internal", 101+offset),
+		},
+		Spec: v1.NodeSpec{
+			ProviderID: fmt.Sprintf("aws:///us-east-1b/%s", instanceID),
+		},
+	}
+	return node, instance
+}
+
+func TestCloud_findInstancesForELB(t *testing.T) {
+	defaultNode := &v1.Node{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "ip-172-20-0-100.ec2.internal",
+		},
+		Spec: v1.NodeSpec{
+			ProviderID: "aws:///us-east-1a/i-self",
+		},
+	}
+	newNode, newInstance := makeNodeInstancePair(1)
+	awsServices := NewFakeAWSServices(TestClusterID)
+	c, err := newAWSCloud(CloudConfig{}, awsServices)
+	if err != nil {
+		t.Errorf("Error building aws cloud: %v", err)
+		return
+	}
+
+	want := map[InstanceID]*ec2.Instance{
+		"i-self": awsServices.selfInstance,
+	}
+	got, err := c.findInstancesForELB([]*v1.Node{defaultNode}, nil)
+	assert.NoError(t, err)
+	assert.True(t, reflect.DeepEqual(want, got))
+
+	// Add a new EC2 instance
+	awsServices.instances = append(awsServices.instances, newInstance)
+	want = map[InstanceID]*ec2.Instance{
+		"i-self": awsServices.selfInstance,
+		InstanceID(aws.StringValue(newInstance.InstanceId)): newInstance,
+	}
+	got, err = c.findInstancesForELB([]*v1.Node{defaultNode, newNode}, nil)
+	assert.NoError(t, err)
+	assert.True(t, reflect.DeepEqual(want, got))
+
+	// Verify existing instance cache gets used
+	cacheExpiryOld := c.instanceCache.snapshot.timestamp
+	got, err = c.findInstancesForELB([]*v1.Node{defaultNode, newNode}, nil)
+	assert.NoError(t, err)
+	assert.True(t, reflect.DeepEqual(want, got))
+	cacheExpiryNew := c.instanceCache.snapshot.timestamp
+	assert.Equal(t, cacheExpiryOld, cacheExpiryNew)
+
+	// Force cache expiry and verify cache gets updated with new timestamp
+	cacheExpiryOld = c.instanceCache.snapshot.timestamp
+	c.instanceCache.snapshot.timestamp = c.instanceCache.snapshot.timestamp.Add(-(defaultEC2InstanceCacheMaxAge + 1*time.Second))
+	got, err = c.findInstancesForELB([]*v1.Node{defaultNode, newNode}, nil)
+	assert.NoError(t, err)
+	assert.True(t, reflect.DeepEqual(want, got))
+	cacheExpiryNew = c.instanceCache.snapshot.timestamp
+	assert.True(t, cacheExpiryNew.After(cacheExpiryOld))
+}
+
 func TestCloud_chunkTargetDescriptions(t *testing.T) {
 	type args struct {
 		targets   []*elbv2.TargetDescription