From bc571be8e2cade5979b49167fa944978b6d3f8b3 Mon Sep 17 00:00:00 2001 From: Claes Mogren Date: Fri, 17 Apr 2020 09:12:55 -0700 Subject: [PATCH] Reduce number of calls to EC2 API (#909) --- pkg/awsutils/awsutils.go | 200 +++++++++++++++++++-------- pkg/awsutils/awsutils_test.go | 89 ++++-------- pkg/awsutils/mocks/awsutils_mocks.go | 39 ++++-- pkg/ipamd/ipamd.go | 140 +++++++++++++------ pkg/ipamd/ipamd_test.go | 184 ++++++++++++++---------- 5 files changed, 396 insertions(+), 256 deletions(-) diff --git a/pkg/awsutils/awsutils.go b/pkg/awsutils/awsutils.go index f60c3bff8a..b70a682fd2 100644 --- a/pkg/awsutils/awsutils.go +++ b/pkg/awsutils/awsutils.go @@ -114,8 +114,11 @@ type APIs interface { // GetAttachedENIs retrieves eni information from instance metadata service GetAttachedENIs() (eniList []ENIMetadata, err error) - // DescribeENI returns the IPv4 addresses of ENI interface, tags, and the ENI attachment ID - DescribeENI(eniID string) (addrList []*ec2.NetworkInterfacePrivateIpAddress, tags map[string]string, attachemdID *string, err error) + // GetIPv4sFromEC2 returns the IPv4 addresses for a given ENI + GetIPv4sFromEC2(eniID string) (addrList []*ec2.NetworkInterfacePrivateIpAddress, err error) + + // DescribeAllENIs calls EC2 and returns the ENIMetadata and a tag map for each ENI + DescribeAllENIs() ([]ENIMetadata, map[string]TagMap, error) // AllocIPAddress allocates an IP address for an ENI AllocIPAddress(eniID string) error @@ -165,9 +168,6 @@ type EC2InstanceMetadataCache struct { region string accountID string - // dynamic - currentENIs int - ec2Metadata ec2metadata.EC2Metadata ec2SVC ec2wrapper.EC2 } @@ -188,9 +188,6 @@ type ENIMetadata struct { // The ip addresses allocated for the network interface IPv4Addresses []*ec2.NetworkInterfacePrivateIpAddress - - // Tags are the tags associated with this ENI in AWS - Tags map[string]string } func (eni ENIMetadata) PrimaryIPv4Address() string { @@ -202,6 +199,8 @@ func (eni ENIMetadata) PrimaryIPv4Address() string { return "" } +type TagMap map[string]string + // msSince returns milliseconds since start. func msSince(start time.Time) float64 { return float64(time.Since(start) / time.Millisecond) @@ -370,7 +369,6 @@ func (cache *EC2InstanceMetadataCache) setPrimaryENI() error { } eniMACs := strings.Fields(metadataENImacs) log.Debugf("Discovered %d interfaces.", len(eniMACs)) - cache.currentENIs = len(eniMACs) // retrieve the attached ENIs for _, eniMAC := range eniMACs { @@ -411,7 +409,7 @@ func (cache *EC2InstanceMetadataCache) setPrimaryENI() error { if cache.primaryENImac == result[0] { //primary interface cache.primaryENI = eni - log.Debugf("Found ENI %s is a primary ENI", eni) + log.Debugf("%s is the primary ENI of this instance", eni) return nil } } @@ -429,7 +427,6 @@ func (cache *EC2InstanceMetadataCache) GetAttachedENIs() (eniList []ENIMetadata, } macsStrs := strings.Fields(macs) log.Debugf("Total number of interfaces found: %d ", len(macsStrs)) - cache.currentENIs = len(macsStrs) var enis []ENIMetadata // retrieve the attached ENIs @@ -446,7 +443,7 @@ func (cache *EC2InstanceMetadataCache) GetAttachedENIs() (eniList []ENIMetadata, func (cache *EC2InstanceMetadataCache) getENIMetadata(macStr string) (ENIMetadata, error) { eniMACList := strings.Split(macStr, "/") eniMAC := eniMACList[0] - log.Debugf("Found ENI mac address : %s", eniMAC) + log.Debugf("Found ENI MAC address: %s", eniMAC) eni, deviceNum, err := cache.getENIDeviceNumber(eniMAC) if err != nil { @@ -458,42 +455,17 @@ func (cache *EC2InstanceMetadataCache) getENIMetadata(macStr string) (ENIMetadat if err != nil { return ENIMetadata{}, errors.Wrapf(err, "get ENI metadata: failed to retrieve IPs and CIDR for ENI: %s", eniMAC) } - privateIPv4s, tags, _, err := cache.DescribeENI(eni) - if err != nil { - return ENIMetadata{}, errors.Wrapf(err, "get ENI metadata: failed to describe ENI: %s, %v", eniMAC, err) - } - // getIPsAndCIDR() queries IMDS for IPv4 addresses attached to the ENI. - // DescribeENI() calls the DescribeNetworkInterfaces AWS API call, which - // technically should be the source of truth and contain the freshest - // information. Let's just do a quick scan here and output some diagnostic - // messages if we find stale info in the IMDS result. - imdsIPv4Set := sets.NewString(imdsIPv4s...) - privateIPv4Set := sets.String{} - for _, privateIPv4 := range privateIPv4s { - privateIPv4Set.Insert(aws.StringValue(privateIPv4.PrivateIpAddress)) - } - missingIMDS := privateIPv4Set.Difference(imdsIPv4Set).List() - missingDNI := imdsIPv4Set.Difference(privateIPv4Set).List() - if len(missingIMDS) > 0 { - strMissing := strings.Join(missingIMDS, ",") - log.Debugf("getENIMetadata: DescribeNetworkInterfaces(%s) yielded private IPv4 addresses %s that were not yet found in IMDS.", eni, strMissing) - } - if len(missingDNI) > 0 { - strMissing := strings.Join(missingDNI, ",") - log.Debugf("getENIMetadata: IMDS query yielded stale IPv4 addresses %s that were not found in DescribeNetworkInterfaces(%s).", strMissing, eni) - } return ENIMetadata{ ENIID: eni, MAC: eniMAC, DeviceNumber: deviceNum, SubnetIPv4CIDR: cidr, - IPv4Addresses: privateIPv4s, - Tags: tags, + IPv4Addresses: imdsIPv4s, }, nil } // getIPsAndCIDR return list of IPs, CIDR, error -func (cache *EC2InstanceMetadataCache) getIPsAndCIDR(eniMAC string) ([]string, string, error) { +func (cache *EC2InstanceMetadataCache) getIPsAndCIDR(eniMAC string) ([]*ec2.NetworkInterfacePrivateIpAddress, string, error) { start := time.Now() cidr, err := cache.ec2Metadata.GetMetadata(metadataMACPath + eniMAC + metadataSubnetCIDR) awsAPILatency.WithLabelValues("GetMetadata", fmt.Sprint(err != nil)).Observe(msSince(start)) @@ -506,7 +478,7 @@ func (cache *EC2InstanceMetadataCache) getIPsAndCIDR(eniMAC string) ([]string, s log.Debugf("Found CIDR %s for ENI %s", cidr, eniMAC) start = time.Now() - ipv4s, err := cache.ec2Metadata.GetMetadata(metadataMACPath + eniMAC + metadataIPv4s) + ipv4sAsString, err := cache.ec2Metadata.GetMetadata(metadataMACPath + eniMAC + metadataIPv4s) awsAPILatency.WithLabelValues("GetMetadata", fmt.Sprint(err != nil)).Observe(msSince(start)) if err != nil { awsAPIErrInc("GetMetadata", err) @@ -514,9 +486,20 @@ func (cache *EC2InstanceMetadataCache) getIPsAndCIDR(eniMAC string) ([]string, s return nil, "", errors.Wrapf(err, "failed to retrieve ENI %s local-ipv4s", eniMAC) } - ipv4Strs := strings.Fields(ipv4s) + ipv4Strs := strings.Fields(ipv4sAsString) log.Debugf("Found IP addresses %v on ENI %s", ipv4Strs, eniMAC) - return ipv4Strs, cidr, nil + ipv4s := make([]*ec2.NetworkInterfacePrivateIpAddress, 0, len(ipv4Strs)) + // network/interfaces/macs/mac/public-ipv4s The public IP address or Elastic IP addresses associated with the interface. + // There may be multiple IPv4 addresses on an instance. https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/instancedata-data-categories.html + isFirst := true + for _, ipv4 := range ipv4Strs { + // TODO: Verify that the first IP is always the primary + primary := isFirst + ip := ipv4 + ipv4s = append(ipv4s, &ec2.NetworkInterfacePrivateIpAddress{PrivateIpAddress: &ip, Primary: &primary}) + isFirst = false + } + return ipv4s, cidr, nil } // getENIDeviceNumber returns ENI ID, device number, error @@ -554,6 +537,7 @@ func (cache *EC2InstanceMetadataCache) getENIDeviceNumber(eniMAC string) (string return eni, int(deviceNum + 1), nil } +// awsGetFreeDeviceNumber calls EC2 API DescribeInstances to get the next free device index func (cache *EC2InstanceMetadataCache) awsGetFreeDeviceNumber() (int, error) { input := &ec2.DescribeInstancesInput{ InstanceIds: []*string{aws.String(cache.instanceID)}, @@ -637,7 +621,7 @@ func (cache *EC2InstanceMetadataCache) AllocENI(useCustomCfg bool, sg []*string, return eniID, nil } -// return attachment id, error +// attachENI calls EC2 API to attach the ENI and returns the attachment id func (cache *EC2InstanceMetadataCache) attachENI(eniID string) (string, error) { // attach to instance freeDevice, err := cache.awsGetFreeDeviceNumber() @@ -807,14 +791,13 @@ func (cache *EC2InstanceMetadataCache) freeENI(eniName string, sleepDelayAfterDe log.Infof("Trying to free ENI: %s", eniName) // Find out attachment - _, _, attachID, err := cache.DescribeENI(eniName) + attachID, err := cache.getENIAttachmentID(eniName) if err != nil { if err == ErrENINotFound { log.Infof("ENI %s not found. It seems to be already freed", eniName) return nil } - - awsUtilsErrInc("FreeENIDescribeENIFailed", err) + awsUtilsErrInc("getENIAttachmentIDFailed", err) log.Errorf("Failed to retrieve ENI %s attachment id: %v", eniName, err) return errors.Wrap(err, "FreeENI: failed to retrieve ENI's attachment id") } @@ -855,6 +838,28 @@ func (cache *EC2InstanceMetadataCache) freeENI(eniName string, sleepDelayAfterDe return nil } +// getENIAttachmentID calls EC2 to fetch the attachmentID of a given ENI +func (cache *EC2InstanceMetadataCache) getENIAttachmentID(eniID string) (*string, error) { + eniIds := make([]*string, 0) + eniIds = append(eniIds, aws.String(eniID)) + input := &ec2.DescribeNetworkInterfacesInput{NetworkInterfaceIds: eniIds} + + start := time.Now() + result, err := cache.ec2SVC.DescribeNetworkInterfaces(input) + awsAPILatency.WithLabelValues("DescribeNetworkInterfaces", fmt.Sprint(err != nil)).Observe(msSince(start)) + if err != nil { + if aerr, ok := err.(awserr.Error); ok { + if aerr.Code() == "InvalidNetworkInterfaceID.NotFound" { + return nil, ErrENINotFound + } + } + awsAPIErrInc("DescribeNetworkInterfaces", err) + log.Errorf("Failed to get ENI %s information from EC2 control plane %v", eniID, err) + return nil, errors.Wrap(err, "failed to describe network interface") + } + return result.NetworkInterfaces[0].Attachment.AttachmentId, nil +} + func (cache *EC2InstanceMetadataCache) deleteENI(eniName string, maxBackoffDelay time.Duration) error { log.Debugf("Trying to delete ENI: %s", eniName) deleteInput := &ec2.DeleteNetworkInterfaceInput{ @@ -882,9 +887,8 @@ func (cache *EC2InstanceMetadataCache) deleteENI(eniName string, maxBackoffDelay return err } -// DescribeENI returns the IPv4 addresses, tags, and attachment id of the given ENI -// return: private IP address, tags, attachment id, error -func (cache *EC2InstanceMetadataCache) DescribeENI(eniID string) ([]*ec2.NetworkInterfacePrivateIpAddress, map[string]string, *string, error) { +// GetIPv4sFromEC2 calls EC2 and returns a list of all addresses on the ENI +func (cache *EC2InstanceMetadataCache) GetIPv4sFromEC2(eniID string) (addrList []*ec2.NetworkInterfacePrivateIpAddress, err error) { eniIds := make([]*string, 0) eniIds = append(eniIds, aws.String(eniID)) input := &ec2.DescribeNetworkInterfacesInput{NetworkInterfaceIds: eniIds} @@ -895,23 +899,101 @@ func (cache *EC2InstanceMetadataCache) DescribeENI(eniID string) ([]*ec2.Network if err != nil { if aerr, ok := err.(awserr.Error); ok { if aerr.Code() == "InvalidNetworkInterfaceID.NotFound" { - return nil, nil, nil, ErrENINotFound + return nil, ErrENINotFound } } awsAPIErrInc("DescribeNetworkInterfaces", err) log.Errorf("Failed to get ENI %s information from EC2 control plane %v", eniID, err) - return nil, nil, nil, errors.Wrap(err, "failed to describe network interface") + return nil, errors.Wrap(err, "failed to describe network interface") } - tags := make(map[string]string, len(result.NetworkInterfaces[0].TagSet)) - for _, tag := range result.NetworkInterfaces[0].TagSet { - if tag.Key == nil || tag.Value == nil { - log.Errorf("nil tag on ENI: %v", eniID) - continue + return result.NetworkInterfaces[0].PrivateIpAddresses, nil +} + +// DescribeAllENIs calls EC2 to refrech the ENIMetadata and tags for all attached ENIs +func (cache *EC2InstanceMetadataCache) DescribeAllENIs() ([]ENIMetadata, map[string]TagMap, error) { + // Fetch all local ENI info from metadata + allENIs, err := cache.GetAttachedENIs() + if err != nil { + return nil, nil, errors.Wrap(err, "DescribeAllENIs: failed to get local ENI metadata") + } + + eniMap := make(map[string]ENIMetadata, len(allENIs)) + var eniIDs []string + for _, eni := range allENIs { + eniIDs = append(eniIDs, eni.ENIID) + eniMap[eni.ENIID] = eni + } + input := &ec2.DescribeNetworkInterfacesInput{NetworkInterfaceIds: aws.StringSlice(eniIDs)} + + start := time.Now() + ec2Response, err := cache.ec2SVC.DescribeNetworkInterfaces(input) + awsAPILatency.WithLabelValues("DescribeNetworkInterfaces", fmt.Sprint(err != nil)).Observe(msSince(start)) + if err != nil { + if aerr, ok := err.(awserr.Error); ok { + if aerr.Code() == "InvalidNetworkInterfaceID.NotFound" { + return nil, nil, ErrENINotFound + } + } + awsAPIErrInc("DescribeNetworkInterfaces", err) + log.Errorf("Failed to call ec2:DescribeNetworkInterfaces for %v: %v", eniIDs, err) + return nil, nil, errors.Wrap(err, "failed to describe network interfaces") + } + // Collect ENI response into ENI metadata and tags. + tagMap := make(map[string]TagMap, len(ec2Response.NetworkInterfaces)) + for _, ec2res := range ec2Response.NetworkInterfaces { + eniID := aws.StringValue(ec2res.NetworkInterfaceId) + eniMetadata := eniMap[eniID] + // Check IPv4 addresses + logOutOfSyncState(eniID, eniMetadata.IPv4Addresses, ec2res.PrivateIpAddresses) + tags := make(map[string]string, len(ec2res.TagSet)) + for _, tag := range ec2res.TagSet { + if tag.Key == nil || tag.Value == nil { + log.Errorf("nil tag on ENI: %v", eniMetadata.ENIID) + continue + } + tags[*tag.Key] = *tag.Value + } + if len(tags) > 0 { + tagMap[eniMetadata.ENIID] = tags } - tags[*tag.Key] = *tag.Value } + return allENIs, tagMap, nil +} - return result.NetworkInterfaces[0].PrivateIpAddresses, tags, result.NetworkInterfaces[0].Attachment.AttachmentId, nil +// logOutOfSyncState compares the IP and metadata returned by IMDS and the EC2 API DescribeNetworkInterfaces calls +func logOutOfSyncState(eniID string, imdsIPv4s, ec2IPv4s []*ec2.NetworkInterfacePrivateIpAddress) { + // Comparing the IMDS IPv4 addresses attached to the ENI with the DescribeNetworkInterfaces AWS API call, which + // technically should be the source of truth and contain the freshest information. Let's just do a quick scan here + // and output some diagnostic messages if we find stale info in the IMDS result. + imdsIPv4Set := sets.String{} + imdsPrimaryIP := "" + for _, imdsIPv4 := range imdsIPv4s { + imdsIPv4Set.Insert(aws.StringValue(imdsIPv4.PrivateIpAddress)) + if aws.BoolValue(imdsIPv4.Primary) { + imdsPrimaryIP = aws.StringValue(imdsIPv4.PrivateIpAddress) + } + } + ec2IPv4Set := sets.String{} + ec2IPv4PrimaryIP := "" + for _, privateIPv4 := range ec2IPv4s { + ec2IPv4Set.Insert(aws.StringValue(privateIPv4.PrivateIpAddress)) + if aws.BoolValue(privateIPv4.Primary) { + ec2IPv4PrimaryIP = aws.StringValue(privateIPv4.PrivateIpAddress) + } + } + missingIMDS := ec2IPv4Set.Difference(imdsIPv4Set).List() + missingDNI := imdsIPv4Set.Difference(ec2IPv4Set).List() + if len(missingIMDS) > 0 { + strMissing := strings.Join(missingIMDS, ",") + log.Infof("logOutOfSyncState: DescribeNetworkInterfaces(%s) yielded private IPv4 addresses %s that were not yet found in IMDS.", eniID, strMissing) + } + if len(missingDNI) > 0 { + strMissing := strings.Join(missingDNI, ",") + log.Infof("logOutOfSyncState: IMDS query yielded stale IPv4 addresses %s that were not found in DescribeNetworkInterfaces(%s).", strMissing, eniID) + } + if imdsPrimaryIP != ec2IPv4PrimaryIP { + log.Infof("logOutOfSyncState: Primary IPs do not mach for %s. IMDS: %s, EC2: %s", eniID, imdsPrimaryIP, ec2IPv4PrimaryIP) + } } // AllocIPAddress allocates an IP address for an ENI @@ -1001,11 +1083,11 @@ func (cache *EC2InstanceMetadataCache) AllocIPAddresses(eniID string, numIPs int _, err = cache.ec2SVC.AssignPrivateIpAddresses(input) awsAPILatency.WithLabelValues("AssignPrivateIpAddresses", fmt.Sprint(err != nil)).Observe(msSince(start)) if err != nil { + log.Errorf("Failed to allocate a private IP addresses on ENI %v: %v",eniID, err) awsAPIErrInc("AssignPrivateIpAddresses", err) if containsPrivateIPAddressLimitExceededError(err) { return nil } - log.Errorf("Failed to allocate a private IP address %v", err) return errors.Wrap(err, "allocate IP address: failed to allocate a private IP address") } return nil diff --git a/pkg/awsutils/awsutils_test.go b/pkg/awsutils/awsutils_test.go index 84d7954a70..cbf0760495 100644 --- a/pkg/awsutils/awsutils_test.go +++ b/pkg/awsutils/awsutils_test.go @@ -50,7 +50,7 @@ const ( eniAttachID = "eni-attach-beb21856" eni1Device = "0" eni1PrivateIP = "10.0.0.1" - eni2Device = "2" + eni2Device = "1" eni2PrivateIP = "10.0.0.2" eni2AttachID = "eni-attach-fafdfafd" eni2ID = "eni-12341234" @@ -76,7 +76,7 @@ func TestInitWithEC2metadata(t *testing.T) { mockMetadata.EXPECT().GetMetadata(metadataInstanceType).Return(instanceType, nil) mockMetadata.EXPECT().GetMetadata(metadataMAC).Return(primaryMAC, nil) mockMetadata.EXPECT().GetMetadata(metadataMACPath).Return(primaryMAC, nil) - mockMetadata.EXPECT().GetMetadata(metadataMACPath+primaryMAC+metadataDeviceNum).Return("1", nil) + mockMetadata.EXPECT().GetMetadata(metadataMACPath+primaryMAC+metadataDeviceNum).Return(eni1Device, nil) mockMetadata.EXPECT().GetMetadata(metadataMACPath+primaryMAC+metadataOwnerID).Return("1234", nil) mockMetadata.EXPECT().GetMetadata(metadataMACPath+primaryMAC+metadataInterface).Return(primaryMAC, nil) mockMetadata.EXPECT().GetMetadata(metadataMACPath+primaryMAC+metadataSGs).Return(sgs, nil) @@ -106,7 +106,7 @@ func TestInitWithEC2metadataVPCcidrErr(t *testing.T) { mockMetadata.EXPECT().GetMetadata(metadataInstanceType).Return(instanceType, nil) mockMetadata.EXPECT().GetMetadata(metadataMAC).Return(primaryMAC, nil) mockMetadata.EXPECT().GetMetadata(metadataMACPath).Return(primaryMAC, nil) - mockMetadata.EXPECT().GetMetadata(metadataMACPath+primaryMAC+metadataDeviceNum).Return("1", nil) + mockMetadata.EXPECT().GetMetadata(metadataMACPath+primaryMAC+metadataDeviceNum).Return(eni1Device, nil) mockMetadata.EXPECT().GetMetadata(metadataMACPath+primaryMAC+metadataOwnerID).Return("1234", nil) mockMetadata.EXPECT().GetMetadata(metadataMACPath+primaryMAC+metadataInterface).Return(primaryMAC, nil) mockMetadata.EXPECT().GetMetadata(metadataMACPath+primaryMAC+metadataSGs).Return(sgs, nil) @@ -128,7 +128,7 @@ func TestInitWithEC2metadataSubnetErr(t *testing.T) { mockMetadata.EXPECT().GetMetadata(metadataInstanceType).Return(instanceType, nil) mockMetadata.EXPECT().GetMetadata(metadataMAC).Return(primaryMAC, nil) mockMetadata.EXPECT().GetMetadata(metadataMACPath).Return(primaryMAC, nil) - mockMetadata.EXPECT().GetMetadata(metadataMACPath+primaryMAC+metadataDeviceNum).Return("1", nil) + mockMetadata.EXPECT().GetMetadata(metadataMACPath+primaryMAC+metadataDeviceNum).Return(eni1Device, nil) mockMetadata.EXPECT().GetMetadata(metadataMACPath+primaryMAC+metadataOwnerID).Return("1234", nil) mockMetadata.EXPECT().GetMetadata(metadataMACPath+primaryMAC+metadataInterface).Return(primaryMAC, nil) mockMetadata.EXPECT().GetMetadata(metadataMACPath+primaryMAC+metadataSGs).Return(sgs, nil) @@ -149,7 +149,7 @@ func TestInitWithEC2metadataSGErr(t *testing.T) { mockMetadata.EXPECT().GetMetadata(metadataInstanceType).Return(instanceType, nil) mockMetadata.EXPECT().GetMetadata(metadataMAC).Return(primaryMAC, nil) mockMetadata.EXPECT().GetMetadata(metadataMACPath).Return(primaryMAC, nil) - mockMetadata.EXPECT().GetMetadata(metadataMACPath+primaryMAC+metadataDeviceNum).Return("1", nil) + mockMetadata.EXPECT().GetMetadata(metadataMACPath+primaryMAC+metadataDeviceNum).Return(eni1Device, nil) mockMetadata.EXPECT().GetMetadata(metadataMACPath+primaryMAC+metadataOwnerID).Return("1234", nil) mockMetadata.EXPECT().GetMetadata(metadataMACPath+primaryMAC+metadataInterface).Return(primaryMAC, nil) mockMetadata.EXPECT().GetMetadata(metadataMACPath+primaryMAC+metadataSGs).Return(sgs, errors.New("Error on SG")) @@ -168,7 +168,7 @@ func TestInitWithEC2metadataENIErrs(t *testing.T) { mockMetadata.EXPECT().GetMetadata(metadataInstanceID).Return(instanceID, nil) mockMetadata.EXPECT().GetMetadata(metadataInstanceType).Return(instanceType, nil) mockMetadata.EXPECT().GetMetadata(metadataMAC).Return(primaryMAC, nil) - mockMetadata.EXPECT().GetMetadata(metadataMACPath).Return("", errors.New("Err on ENIs")) + mockMetadata.EXPECT().GetMetadata(metadataMACPath).Return("", errors.New("err on ENIs")) ins := &EC2InstanceMetadataCache{ec2Metadata: mockMetadata} err := ins.initWithEC2Metadata() @@ -183,7 +183,7 @@ func TestInitWithEC2metadataMACErr(t *testing.T) { mockMetadata.EXPECT().GetMetadata(metadataLocalIP).Return(localIP, nil) mockMetadata.EXPECT().GetMetadata(metadataInstanceID).Return(instanceID, nil) mockMetadata.EXPECT().GetMetadata(metadataInstanceType).Return(instanceType, nil) - mockMetadata.EXPECT().GetMetadata(metadataMAC).Return(primaryMAC, errors.New("Error on MAC")) + mockMetadata.EXPECT().GetMetadata(metadataMAC).Return(primaryMAC, errors.New("error on MAC")) ins := &EC2InstanceMetadataCache{ec2Metadata: mockMetadata} err := ins.initWithEC2Metadata() @@ -195,7 +195,7 @@ func TestInitWithEC2metadataLocalIPErr(t *testing.T) { defer ctrl.Finish() mockMetadata.EXPECT().GetMetadata(metadataAZ).Return(az, nil) - mockMetadata.EXPECT().GetMetadata(metadataLocalIP).Return(localIP, errors.New("Error on localIP")) + mockMetadata.EXPECT().GetMetadata(metadataLocalIP).Return(localIP, errors.New("error on localIP")) ins := &EC2InstanceMetadataCache{ec2Metadata: mockMetadata} err := ins.initWithEC2Metadata() @@ -208,7 +208,7 @@ func TestInitWithEC2metadataInstanceErr(t *testing.T) { mockMetadata.EXPECT().GetMetadata(metadataAZ).Return(az, nil) mockMetadata.EXPECT().GetMetadata(metadataLocalIP).Return(localIP, nil) - mockMetadata.EXPECT().GetMetadata(metadataInstanceID).Return(instanceID, errors.New("Error on instanceID")) + mockMetadata.EXPECT().GetMetadata(metadataInstanceID).Return(instanceID, errors.New("error on instanceID")) ins := &EC2InstanceMetadataCache{ec2Metadata: mockMetadata} err := ins.initWithEC2Metadata() @@ -219,7 +219,7 @@ func TestInitWithEC2metadataAZErr(t *testing.T) { ctrl, mockMetadata, _ := setup(t) defer ctrl.Finish() - mockMetadata.EXPECT().GetMetadata(metadataAZ).Return(az, errors.New("Error on metadata AZ")) + mockMetadata.EXPECT().GetMetadata(metadataAZ).Return(az, errors.New("error on metadata AZ")) ins := &EC2InstanceMetadataCache{ec2Metadata: mockMetadata} err := ins.initWithEC2Metadata() @@ -246,48 +246,11 @@ func TestSetPrimaryENs(t *testing.T) { } func TestGetAttachedENIs(t *testing.T) { - ctrl, mockMetadata, mockEC2 := setup(t) + ctrl, mockMetadata, _ := setup(t) defer ctrl.Finish() mockMetadata.EXPECT().GetMetadata(metadataMACPath).Return(primaryMAC+" "+eni2MAC, nil) - mockEC2.EXPECT().DescribeNetworkInterfaces(gomock.Any()). - DoAndReturn(func(input *ec2.DescribeNetworkInterfacesInput) (*ec2.DescribeNetworkInterfacesOutput, error) { - output := []*ec2.NetworkInterface{} - for _, in := range input.NetworkInterfaceIds { - ip := "" - attachID := "" - switch *in { - case eniID: - ip, attachID = eni1PrivateIP, eniAttachID - case eni2ID: - ip, attachID = eni2PrivateIP, eni2AttachID - default: - panic("no such id " + *in) - } - - output = append(output, &ec2.NetworkInterface{ - PrivateIpAddresses: []*ec2.NetworkInterfacePrivateIpAddress{ - { - PrivateIpAddress: &ip, - }, - }, - Attachment: &ec2.NetworkInterfaceAttachment{ - AttachmentId: &attachID, - }, - TagSet: []*ec2.Tag{ - { - Key: aws.String("foo"), - Value: aws.String("foo-value"), - }, - }, - }) - } - return &ec2.DescribeNetworkInterfacesOutput{ - NetworkInterfaces: output, - }, nil - }).Times(2) - gomock.InOrder( mockMetadata.EXPECT().GetMetadata(metadataMACPath+primaryMAC+metadataDeviceNum).Return(eni1Device, nil), mockMetadata.EXPECT().GetMetadata(metadataMACPath+primaryMAC+metadataInterface).Return(eniID, nil), @@ -299,7 +262,7 @@ func TestGetAttachedENIs(t *testing.T) { mockMetadata.EXPECT().GetMetadata(metadataMACPath+eni2MAC+metadataIPv4s).Return("", nil), ) - ins := &EC2InstanceMetadataCache{ec2Metadata: mockMetadata, ec2SVC: mockEC2} + ins := &EC2InstanceMetadataCache{ec2Metadata: mockMetadata} ens, err := ins.GetAttachedENIs() assert.NoError(t, err) assert.Equal(t, len(ens), 2) @@ -342,15 +305,18 @@ func TestAWSGetFreeDeviceNumberNoDevice(t *testing.T) { assert.Error(t, err) } -func TestDescribeENI(t *testing.T) { - ctrl, _, mockEC2 := setup(t) +func TestDescribeAllENIs(t *testing.T) { + ctrl, mockMetadata, mockEC2 := setup(t) defer ctrl.Finish() - attachmentID := eniAttachID - attachment := &ec2.NetworkInterfaceAttachment{AttachmentId: &attachmentID} + mockMetadata.EXPECT().GetMetadata(metadataMACPath).Times(2).Return(primaryMAC, nil) + mockMetadata.EXPECT().GetMetadata(metadataMACPath+primaryMAC+metadataDeviceNum).Times(2).Return(eni1Device, nil) + mockMetadata.EXPECT().GetMetadata(metadataMACPath+primaryMAC+metadataInterface).Times(2).Return(eniID, nil) + mockMetadata.EXPECT().GetMetadata(metadataMACPath+primaryMAC+metadataSubnetCIDR).Times(2).Return(subnetCIDR, nil) + mockMetadata.EXPECT().GetMetadata(metadataMACPath+primaryMAC+metadataIPv4s).Times(2).Return("", nil) + result := &ec2.DescribeNetworkInterfacesOutput{ NetworkInterfaces: []*ec2.NetworkInterface{{ - Attachment: attachment, TagSet: []*ec2.Tag{ {Key: aws.String("foo"), Value: aws.String("foo-value")}, }, @@ -359,22 +325,19 @@ func TestDescribeENI(t *testing.T) { testCases := []struct { name string - expID *string - exptags map[string]string + exptags map[string]TagMap awsErr error expErr error }{ - {"success DescribeENI", &attachmentID, map[string]string{"foo": "foo-value"}, nil, nil}, - {"not found error", nil, nil, awserr.New("InvalidNetworkInterfaceID.NotFound", "", nil), ErrENINotFound}, + {"success DescribeENI", map[string]TagMap{"": {"foo": "foo-value"}}, nil, nil}, + {"not found error", nil, awserr.New("InvalidNetworkInterfaceID.NotFound", "", nil), ErrENINotFound}, } for _, tc := range testCases { mockEC2.EXPECT().DescribeNetworkInterfaces(gomock.Any()).Return(result, tc.awsErr) - - ins := &EC2InstanceMetadataCache{ec2SVC: mockEC2} - _, tags, id, err := ins.DescribeENI("test-eni") + ins := &EC2InstanceMetadataCache{ec2Metadata: mockMetadata, ec2SVC: mockEC2} + _, tags, err := ins.DescribeAllENIs() assert.Equal(t, tc.expErr, err, tc.name) - assert.Equal(t, tc.expID, id, tc.name) assert.Equal(t, tc.exptags, tags, tc.name) } } @@ -388,7 +351,7 @@ func TestTagEni(t *testing.T) { mockMetadata.EXPECT().GetMetadata(metadataInstanceType).Return(instanceType, nil) mockMetadata.EXPECT().GetMetadata(metadataMAC).Return(primaryMAC, nil) mockMetadata.EXPECT().GetMetadata(metadataMACPath).Return(primaryMAC, nil) - mockMetadata.EXPECT().GetMetadata(metadataMACPath+primaryMAC+metadataDeviceNum).Return("1", nil) + mockMetadata.EXPECT().GetMetadata(metadataMACPath+primaryMAC+metadataDeviceNum).Return(eni1Device, nil) mockMetadata.EXPECT().GetMetadata(metadataMACPath+primaryMAC+metadataOwnerID).Return("1234", nil) mockMetadata.EXPECT().GetMetadata(metadataMACPath+primaryMAC+metadataInterface).Return(primaryMAC, nil) mockMetadata.EXPECT().GetMetadata(metadataMACPath+primaryMAC+metadataSGs).Return(sgs, nil) diff --git a/pkg/awsutils/mocks/awsutils_mocks.go b/pkg/awsutils/mocks/awsutils_mocks.go index 2038d23fde..aed4fa8c96 100644 --- a/pkg/awsutils/mocks/awsutils_mocks.go +++ b/pkg/awsutils/mocks/awsutils_mocks.go @@ -19,11 +19,10 @@ package mock_awsutils import ( - reflect "reflect" - awsutils "github.com/aws/amazon-vpc-cni-k8s/pkg/awsutils" ec2 "github.com/aws/aws-sdk-go/service/ec2" gomock "github.com/golang/mock/gomock" + reflect "reflect" ) // MockAPIs is a mock of APIs interface @@ -106,21 +105,20 @@ func (mr *MockAPIsMockRecorder) DeallocIPAddresses(arg0, arg1 interface{}) *gomo return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeallocIPAddresses", reflect.TypeOf((*MockAPIs)(nil).DeallocIPAddresses), arg0, arg1) } -// DescribeENI mocks base method -func (m *MockAPIs) DescribeENI(arg0 string) ([]*ec2.NetworkInterfacePrivateIpAddress, map[string]string, *string, error) { +// DescribeAllENIs mocks base method +func (m *MockAPIs) DescribeAllENIs() ([]awsutils.ENIMetadata, map[string]awsutils.TagMap, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "DescribeENI", arg0) - ret0, _ := ret[0].([]*ec2.NetworkInterfacePrivateIpAddress) - ret1, _ := ret[1].(map[string]string) - ret2, _ := ret[2].(*string) - ret3, _ := ret[3].(error) - return ret0, ret1, ret2, ret3 + ret := m.ctrl.Call(m, "DescribeAllENIs") + ret0, _ := ret[0].([]awsutils.ENIMetadata) + ret1, _ := ret[1].(map[string]awsutils.TagMap) + ret2, _ := ret[2].(error) + return ret0, ret1, ret2 } -// DescribeENI indicates an expected call of DescribeENI -func (mr *MockAPIsMockRecorder) DescribeENI(arg0 interface{}) *gomock.Call { +// DescribeAllENIs indicates an expected call of DescribeAllENIs +func (mr *MockAPIsMockRecorder) DescribeAllENIs() *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DescribeENI", reflect.TypeOf((*MockAPIs)(nil).DescribeENI), arg0) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DescribeAllENIs", reflect.TypeOf((*MockAPIs)(nil).DescribeAllENIs)) } // FreeENI mocks base method @@ -182,6 +180,21 @@ func (mr *MockAPIsMockRecorder) GetENIipLimit() *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetENIipLimit", reflect.TypeOf((*MockAPIs)(nil).GetENIipLimit)) } +// GetIPv4sFromEC2 mocks base method +func (m *MockAPIs) GetIPv4sFromEC2(arg0 string) ([]*ec2.NetworkInterfacePrivateIpAddress, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetIPv4sFromEC2", arg0) + ret0, _ := ret[0].([]*ec2.NetworkInterfacePrivateIpAddress) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetIPv4sFromEC2 indicates an expected call of GetIPv4sFromEC2 +func (mr *MockAPIsMockRecorder) GetIPv4sFromEC2(arg0 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetIPv4sFromEC2", reflect.TypeOf((*MockAPIs)(nil).GetIPv4sFromEC2), arg0) +} + // GetLocalIPv4 mocks base method func (m *MockAPIs) GetLocalIPv4() string { m.ctrl.T.Helper() diff --git a/pkg/ipamd/ipamd.go b/pkg/ipamd/ipamd.go index 007601c3a4..a6b3b5cb23 100644 --- a/pkg/ipamd/ipamd.go +++ b/pkg/ipamd/ipamd.go @@ -174,11 +174,12 @@ type IPAMContext struct { networkClient networkutils.NetworkAPIs maxIPsPerENI int maxENI int + unmanagedENIs UnmanagedENISet // a set of ENIs tagged with "node.k8s.amazonaws.com/no_manage" unmanagedENI int warmENITarget int warmIPTarget int minimumIPTarget int - primaryIP map[string]string + primaryIP map[string]string // primaryIP is a map from ENI ID to primary IP of that ENI lastNodeIPPoolAction time.Time lastDecreaseIPPool time.Time // reconcileCooldownCache keeps timestamps of the last time an IP address was unassigned from an ENI, @@ -187,16 +188,60 @@ type IPAMContext struct { terminating int32 // Flag to warn that the pod is about to shut down. } -//ReconcileCooldownCache keep track of recently freed IPs to avoid reading stale EC2 metadata +// UnmanagedENISet keeps a set of ENI IDs for ENIs tagged with "node.k8s.amazonaws.com/no_manage" +type UnmanagedENISet struct { + sync.RWMutex + data map[string]bool +} + +func (u *UnmanagedENISet) isUnmanaged(eniID string) bool { + val, ok := u.data[eniID] + return ok && val == true +} + +func (u *UnmanagedENISet) reset() { + u.Lock() + defer u.Unlock() + u.data = make(map[string]bool) +} + +func (u *UnmanagedENISet) add(eniID string) { + u.Lock() + defer u.Unlock() + if len(u.data) == 0 { + u.data = make(map[string]bool) + } + u.data[eniID] = true +} + +// setUnmanagedENIs will rebuild the set of ENI IDs for ENIs tagged as "no_manage" +func (c *IPAMContext) setUnmanagedENIs(tagMap map[string]awsutils.TagMap) { + c.unmanagedENIs.reset() + if len(tagMap) == 0 { + return + } + for eniID, tags := range tagMap { + if tags[eniNoManageTagKey] == "true" { + if eniID == c.awsClient.GetPrimaryENI() { + log.Debugf("Ignoring no_manage tag on primary ENI %s", eniID) + } else { + log.Debugf("Marking ENI %s tagged with %s as being unmanaged", eniID, eniNoManageTagKey) + c.unmanagedENIs.add(eniID) + } + } + } +} + +// ReconcileCooldownCache keep track of recently freed IPs to avoid reading stale EC2 metadata type ReconcileCooldownCache struct { + sync.RWMutex cache map[string]time.Time - lock sync.RWMutex } // Add sets a timestamp for the list of IPs added that says how long they are not to be put back in the data store. func (r *ReconcileCooldownCache) Add(ips []string) { - r.lock.Lock() - defer r.lock.Unlock() + r.Lock() + defer r.Unlock() expiry := time.Now().Add(ipReconcileCooldown) for _, ip := range ips { r.cache[ip] = expiry @@ -205,16 +250,16 @@ func (r *ReconcileCooldownCache) Add(ips []string) { // Remove removes an IP from the cooldown cache. func (r *ReconcileCooldownCache) Remove(ip string) { - r.lock.Lock() - defer r.lock.Unlock() + r.Lock() + defer r.Unlock() log.Debugf("Removing %s from cooldown cache.", ip) delete(r.cache, ip) } // RecentlyFreed checks if this IP was recently freed. func (r *ReconcileCooldownCache) RecentlyFreed(ip string) (found, recentlyFreed bool) { - r.lock.Lock() - defer r.lock.Unlock() + r.Lock() + defer r.Unlock() now := time.Now() if expiry, ok := r.cache[ip]; ok { log.Debugf("Checking if IP %s has been recently freed. Cooldown expires at: %s. (Cooldown: %v)", ip, expiry, now.Sub(expiry) < 0) @@ -274,23 +319,24 @@ func (c *IPAMContext) nodeInit() error { log.Debugf("Start node init") - allENIs, err := c.awsClient.GetAttachedENIs() + eniMetadata, tagMap, err := c.awsClient.DescribeAllENIs() if err != nil { return errors.New("ipamd init: failed to retrieve attached ENIs info") + } else { + log.Debugf("DescribeAllENIs success: ENIs: %d, tagged: %d", len(eniMetadata), len(tagMap)) } - enis, numUnmanaged := filterUnmanagedENIs(allENIs) + c.setUnmanagedENIs(tagMap) + enis := c.filterUnmanagedENIs(eniMetadata) nodeMaxENI, err := c.getMaxENI() if err != nil { log.Error("Failed to get ENI limit") return err } c.maxENI = nodeMaxENI - c.unmanagedENI = numUnmanaged c.maxIPsPerENI, err = c.awsClient.GetENIipLimit() if err != nil { return err } - c.updateIPStats(numUnmanaged) var pbVPCcidrs []string vpcCIDRs := c.awsClient.GetVPCIPv4CIDRs() @@ -723,8 +769,8 @@ func (c *IPAMContext) tryAssignIPs() (increasedPool bool, err error) { return false, errors.Wrap(err, fmt.Sprintf("failed to allocate one IP addresses on ENI %s, err: %v", eni.ID, err)) } } - - ec2Addrs, _, err := c.getENIaddresses(eni.ID) + // This call to EC2 is needed to verify which IPs got attached to this ENI. + ec2Addrs, err := c.awsClient.GetIPv4sFromEC2(eni.ID) if err != nil { ipamdErrInc("increaseIPPoolGetENIaddressesFailed") return true, errors.Wrap(err, "failed to get ENI IP addresses during IP allocation") @@ -748,7 +794,6 @@ func (c *IPAMContext) setupENI(eni string, eniMetadata awsutils.ENIMetadata) err // For secondary ENIs, set up the network if eni != c.awsClient.GetPrimaryENI() { - eniPrimaryIP := eniMetadata.PrimaryIPv4Address() err = c.networkClient.SetupENINetwork(eniPrimaryIP, eniMetadata.MAC, eniMetadata.DeviceNumber, eniMetadata.SubnetIPv4CIDR) if err != nil { @@ -780,22 +825,6 @@ func (c *IPAMContext) addENIaddressesToDataStore(ec2Addrs []*ec2.NetworkInterfac return primaryIP } -// returns all addresses on ENI, the primary address on ENI, error -func (c *IPAMContext) getENIaddresses(eni string) ([]*ec2.NetworkInterfacePrivateIpAddress, string, error) { - ec2Addrs, _, _, err := c.awsClient.DescribeENI(eni) - if err != nil { - return nil, "", errors.Wrapf(err, "failed to find ENI addresses for ENI %s", eni) - } - - for _, ec2Addr := range ec2Addrs { - if aws.BoolValue(ec2Addr.Primary) { - eniPrimaryIP := aws.StringValue(ec2Addr.PrivateIpAddress) - return ec2Addrs, eniPrimaryIP, nil - } - } - return nil, "", errors.Errorf("failed to find the ENI's primary address for ENI %s", eni) -} - func (c *IPAMContext) waitENIAttached(eni string) (awsutils.ENIMetadata, error) { // Wait until the ENI shows up in the instance metadata service retry := 0 @@ -936,10 +965,27 @@ func (c *IPAMContext) nodeIPPoolReconcile(interval time.Duration) { ipamdErrInc("reconcileFailedGetENIs") return } - attachedENIs, numUnmanaged := filterUnmanagedENIs(allENIs) - c.updateIPStats(numUnmanaged) - c.unmanagedENI = numUnmanaged - curENIs := c.dataStore.GetENIInfos() + attachedENIs := c.filterUnmanagedENIs(allENIs) + currentENIIPPools := c.dataStore.GetENIInfos().ENIIPPools + + // Check if a new ENI was added, if so we need to update the tags + needToUpdateTags := false + for _, attachedENI := range attachedENIs { + if _, ok := currentENIIPPools[attachedENI.ENIID]; !ok { + needToUpdateTags = true + break + } + } + if needToUpdateTags { + log.Debugf("A new ENI added but not by ipamd, updating tags") + allENIs, tagMap, err := c.awsClient.DescribeAllENIs() + if err != nil { + log.Warnf("Failed to call EC2 to describe ENIs, aborting reconcile: %v", err) + return + } + c.setUnmanagedENIs(tagMap) + attachedENIs = c.filterUnmanagedENIs(allENIs) + } // Mark phase for _, attachedENI := range attachedENIs { @@ -949,8 +995,8 @@ func (c *IPAMContext) nodeIPPoolReconcile(interval time.Duration) { log.Debugf("Reconcile existing ENI %s IP pool", attachedENI.ENIID) // Reconcile IP pool c.eniIPPoolReconcile(eniIPPool, attachedENI, attachedENI.ENIID) - // Mark action, remove this ENI from curENIs list - delete(curENIs.ENIIPPools, attachedENI.ENIID) + // Mark action, remove this ENI from currentENIIPPools map + delete(currentENIIPPools, attachedENI.ENIID) continue } @@ -967,7 +1013,7 @@ func (c *IPAMContext) nodeIPPoolReconcile(interval time.Duration) { } // Sweep phase: since the marked ENI have been removed, the remaining ones needs to be sweeped - for eni := range curENIs.ENIIPPools { + for eni := range currentENIIPPools { log.Infof("Reconcile and delete detached ENI %s", eni) // Force the delete, since aws local metadata has told us that this ENI is no longer // attached, so any IPs assigned from this ENI will no longer work. @@ -1001,8 +1047,8 @@ func (c *IPAMContext) eniIPPoolReconcile(ipPool map[string]*datastore.AddressInf continue } else { log.Debugf("This IP was recently freed, but is out of cooldown. We need to verify with EC2 control plane.") - // Call EC2 to verify - ec2Addresses, _, err := c.getENIaddresses(eni) + // Call EC2 to verify IPs on this ENI + ec2Addresses, err := c.awsClient.GetIPv4sFromEC2(eni) if err != nil { log.Error("Failed to fetch ENI IP addresses!") continue @@ -1102,18 +1148,22 @@ func getMinimumIPTarget() int { return noMinimumIPTarget } -func filterUnmanagedENIs(enis []awsutils.ENIMetadata) ([]awsutils.ENIMetadata, int) { +// filterUnmanagedENIs filters out ENIs marked with the "node.k8s.amazonaws.com/no_manage" tag +func (c *IPAMContext) filterUnmanagedENIs(enis []awsutils.ENIMetadata) []awsutils.ENIMetadata { numFiltered := 0 ret := make([]awsutils.ENIMetadata, 0, len(enis)) for _, eni := range enis { - if eni.Tags[eniNoManageTagKey] == "true" { - log.Debugf("skipping ENI %s: tagged with %s", eni.ENIID, eniNoManageTagKey) + // If we have unmanaged ENIs, filter them out + if c.unmanagedENIs.isUnmanaged(eni.ENIID) { + log.Debugf("Skipping ENI %s: tagged with %s", eni.ENIID, eniNoManageTagKey) numFiltered++ continue } ret = append(ret, eni) } - return ret, numFiltered + c.unmanagedENI = numFiltered + c.updateIPStats(numFiltered) + return ret } // ipTargetState determines the number of IPs `short` or `over` our WARM_IP_TARGET, diff --git a/pkg/ipamd/ipamd_test.go b/pkg/ipamd/ipamd_test.go index 4df550faf4..49f672999c 100644 --- a/pkg/ipamd/ipamd_test.go +++ b/pkg/ipamd/ipamd_test.go @@ -14,16 +14,6 @@ package ipamd import ( - "net" - "os" - "testing" - - "github.com/aws/aws-sdk-go/aws" - "github.com/aws/aws-sdk-go/service/ec2" - "github.com/golang/mock/gomock" - "github.com/stretchr/testify/assert" - "github.com/vishvananda/netlink" - "github.com/aws/amazon-vpc-cni-k8s/pkg/apis/crd/v1alpha1" "github.com/aws/amazon-vpc-cni-k8s/pkg/awsutils" mock_awsutils "github.com/aws/amazon-vpc-cni-k8s/pkg/awsutils/mocks" @@ -34,24 +24,32 @@ import ( "github.com/aws/amazon-vpc-cni-k8s/pkg/k8sapi" mock_k8sapi "github.com/aws/amazon-vpc-cni-k8s/pkg/k8sapi/mocks" mock_networkutils "github.com/aws/amazon-vpc-cni-k8s/pkg/networkutils/mocks" + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/ec2" + "github.com/golang/mock/gomock" + "github.com/stretchr/testify/assert" + "github.com/vishvananda/netlink" + "net" + "os" + "reflect" + "testing" ) const ( - primaryENIid = "eni-00000000" - secENIid = "eni-00000001" - testAttachmentID = "eni-00000000-attach" - primaryMAC = "12:ef:2a:98:e5:5a" - secMAC = "12:ef:2a:98:e5:5b" - primaryDevice = 0 - secDevice = 2 - primarySubnet = "10.10.10.0/24" - secSubnet = "10.10.20.0/24" - ipaddr01 = "10.10.10.11" - ipaddr02 = "10.10.10.12" - ipaddr03 = "10.10.10.13" - ipaddr11 = "10.10.20.11" - ipaddr12 = "10.10.20.12" - vpcCIDR = "10.10.0.0/16" + primaryENIid = "eni-00000000" + secENIid = "eni-00000001" + primaryMAC = "12:ef:2a:98:e5:5a" + secMAC = "12:ef:2a:98:e5:5b" + primaryDevice = 0 + secDevice = 2 + primarySubnet = "10.10.10.0/24" + secSubnet = "10.10.20.0/24" + ipaddr01 = "10.10.10.11" + ipaddr02 = "10.10.10.12" + ipaddr03 = "10.10.10.13" + ipaddr11 = "10.10.20.11" + ipaddr12 = "10.10.20.12" + vpcCIDR = "10.10.0.0/16" ) func setup(t *testing.T) (*gomock.Controller, @@ -72,12 +70,8 @@ func setup(t *testing.T) (*gomock.Controller, func TestNodeInit(t *testing.T) { ctrl, mockAWS, mockK8S, mockCRI, mockNetwork, _ := setup(t) defer ctrl.Finish() - primary := true - notPrimary := false - testAddr1 := ipaddr01 - testAddr2 := ipaddr02 - testAddr11 := ipaddr11 - testAddr12 := ipaddr12 + + mockContext := &IPAMContext{ awsClient: mockAWS, @@ -91,57 +85,24 @@ func TestNodeInit(t *testing.T) { criClient: mockCRI, networkClient: mockNetwork} - eni1 := awsutils.ENIMetadata{ - ENIID: primaryENIid, - MAC: primaryMAC, - DeviceNumber: primaryDevice, - SubnetIPv4CIDR: primarySubnet, - IPv4Addresses: []*ec2.NetworkInterfacePrivateIpAddress{ - { - PrivateIpAddress: &testAddr1, Primary: &primary, - }, - { - PrivateIpAddress: &testAddr2, Primary: ¬Primary, - }, - }, - } + eni1, eni2 := getDummyENIMetdata() - eni2 := awsutils.ENIMetadata{ - ENIID: secENIid, - MAC: secMAC, - DeviceNumber: secDevice, - SubnetIPv4CIDR: secSubnet, - IPv4Addresses: []*ec2.NetworkInterfacePrivateIpAddress{ - { - PrivateIpAddress: &testAddr11, Primary: ¬Primary, - }, - { - PrivateIpAddress: &testAddr12, Primary: ¬Primary, - }, - }, - } var cidrs []*string mockAWS.EXPECT().GetENILimit().Return(4, nil) mockAWS.EXPECT().GetENIipLimit().Return(14, nil) - mockAWS.EXPECT().GetAttachedENIs().Return([]awsutils.ENIMetadata{eni1, eni2}, nil) + mockAWS.EXPECT().GetIPv4sFromEC2(eni1.ENIID).Return(eni1.IPv4Addresses, nil) mockAWS.EXPECT().GetVPCIPv4CIDR().Return(vpcCIDR) - _, vpcCIDR, _ := net.ParseCIDR(vpcCIDR) + _, parsedVPCCIDR, _ := net.ParseCIDR(vpcCIDR) primaryIP := net.ParseIP(ipaddr01) mockAWS.EXPECT().GetVPCIPv4CIDRs().Return(cidrs) mockAWS.EXPECT().GetPrimaryENImac().Return("") - mockNetwork.EXPECT().SetupHostNetwork(vpcCIDR, cidrs, "", &primaryIP).Return(nil) + mockNetwork.EXPECT().SetupHostNetwork(parsedVPCCIDR, cidrs, "", &primaryIP).Return(nil) mockAWS.EXPECT().GetPrimaryENI().AnyTimes().Return(primaryENIid) - //primaryENIid - attachmentID := testAttachmentID - eniResp := []*ec2.NetworkInterfacePrivateIpAddress{ - { - PrivateIpAddress: &testAddr1, Primary: &primary}, - { - PrivateIpAddress: &testAddr2, Primary: ¬Primary}} - mockAWS.EXPECT().DescribeENI(primaryENIid).Return(eniResp, map[string]string{}, &attachmentID, nil) + eniMetadataSlice := []awsutils.ENIMetadata{eni1, eni2} + mockAWS.EXPECT().DescribeAllENIs().Return(eniMetadataSlice, map[string]awsutils.TagMap{}, nil) mockNetwork.EXPECT().SetupENINetwork(gomock.Any(), secMAC, secDevice, secSubnet) mockAWS.EXPECT().GetLocalIPv4().Return(ipaddr01) @@ -166,6 +127,45 @@ func TestNodeInit(t *testing.T) { assert.NoError(t, err) } +func getDummyENIMetdata() (awsutils.ENIMetadata, awsutils.ENIMetadata) { + primary := true + notPrimary := false + testAddr1 := ipaddr01 + testAddr2 := ipaddr02 + testAddr11 := ipaddr11 + testAddr12 := ipaddr12 + eni1 := awsutils.ENIMetadata{ + ENIID: primaryENIid, + MAC: primaryMAC, + DeviceNumber: primaryDevice, + SubnetIPv4CIDR: primarySubnet, + IPv4Addresses: []*ec2.NetworkInterfacePrivateIpAddress{ + { + PrivateIpAddress: &testAddr1, Primary: &primary, + }, + { + PrivateIpAddress: &testAddr2, Primary: ¬Primary, + }, + }, + } + + eni2 := awsutils.ENIMetadata{ + ENIID: secENIid, + MAC: secMAC, + DeviceNumber: secDevice, + SubnetIPv4CIDR: secSubnet, + IPv4Addresses: []*ec2.NetworkInterfacePrivateIpAddress{ + { + PrivateIpAddress: &testAddr11, Primary: ¬Primary, + }, + { + PrivateIpAddress: &testAddr12, Primary: ¬Primary, + }, + }, + } + return eni1, eni2 +} + func TestIncreaseIPPoolDefault(t *testing.T) { _ = os.Unsetenv(envCustomNetworkCfg) testIncreaseIPPool(t, false) @@ -355,7 +355,7 @@ func TestNodeIPPoolReconcile(t *testing.T) { testAddr1 := ipaddr01 testAddr2 := ipaddr02 - mockAWS.EXPECT().GetAttachedENIs().Return([]awsutils.ENIMetadata{ + eniMetadata := []awsutils.ENIMetadata{ { ENIID: primaryENIid, MAC: primaryMAC, @@ -370,11 +370,10 @@ func TestNodeIPPoolReconcile(t *testing.T) { }, }, }, - }, nil) - - mockAWS.EXPECT().GetPrimaryENI().Return(primaryENIid) - - mockAWS.EXPECT().GetPrimaryENI().Return(primaryENIid) + } + mockAWS.EXPECT().GetAttachedENIs().Return(eniMetadata, nil) + mockAWS.EXPECT().GetPrimaryENI().Times(2).Return(primaryENIid) + mockAWS.EXPECT().DescribeAllENIs().Return(eniMetadata, map[string]awsutils.TagMap{}, nil) mockContext.nodeIPPoolReconcile(0) @@ -564,3 +563,36 @@ func datastoreWith3Pods() *datastore.DataStore { _, _, _ = datastoreWith3Pods.AssignPodIPv4Address(&podInfo3) return datastoreWith3Pods } + +func TestIPAMContext_filterUnmanagedENIs(t *testing.T) { + ctrl := gomock.NewController(t) + + eni1, eni2 := getDummyENIMetdata() + allENIs := []awsutils.ENIMetadata{eni1, eni2} + primaryENIonly := []awsutils.ENIMetadata{eni1} + eni1TagMap := map[string]awsutils.TagMap{eni1.ENIID: {"hi": "tag", eniNoManageTagKey: "true"}} + eni2TagMap := map[string]awsutils.TagMap{eni2.ENIID: {"hi": "tag", eniNoManageTagKey: "true"}} + + mockAWSUtils := mock_awsutils.NewMockAPIs(ctrl) + mockAWSUtils.EXPECT().GetPrimaryENI().Times(2).Return(eni1.ENIID) + + tests := []struct { + name string + tagMap map[string]awsutils.TagMap + enis []awsutils.ENIMetadata + want []awsutils.ENIMetadata + }{ + {"No tags at all", nil, allENIs, allENIs}, + {"Primary ENI unmanaged", eni1TagMap, allENIs, allENIs}, + {"Secondary ENI unmanaged", eni2TagMap, allENIs, primaryENIonly}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + c := &IPAMContext{awsClient: mockAWSUtils} + c.setUnmanagedENIs(tt.tagMap) + if got := c.filterUnmanagedENIs(tt.enis); !reflect.DeepEqual(got, tt.want) { + t.Errorf("filterUnmanagedENIs() = %v, want %v", got, tt.want) + } + }) + } +}