From 5dfc31c02a48b3a535db51e0b7c0bb6ba5f65212 Mon Sep 17 00:00:00 2001 From: Jay Pipes Date: Fri, 17 Apr 2020 08:45:09 -0400 Subject: [PATCH] ensure non-nil Attachement in getENIAttachmentID Practice good code safety in the `EC2MetadataCache.getENIAttachmentID()` method by not assuming that either the `DescribeNetworkInterfacesOutput` struct's `NetworkInterfaces` field is not empty and that the first `NetworkInterface` struct that collection has a non-nil `Attachment` field. Fixes Issue #914 however note that with #909, the source code changed dramatically and this patch will need to be written differently for v1.5.x branches. --- pkg/awsutils/awsutils.go | 36 +++++++++++++++++--- pkg/awsutils/awsutils_test.go | 62 +++++++++++++++++++++++++++++++++++ 2 files changed, 93 insertions(+), 5 deletions(-) diff --git a/pkg/awsutils/awsutils.go b/pkg/awsutils/awsutils.go index b70a682fd2..43487253d1 100644 --- a/pkg/awsutils/awsutils.go +++ b/pkg/awsutils/awsutils.go @@ -73,8 +73,13 @@ const ( eniCleanupStartupDelayMax = 300 ) -// ErrENINotFound is an error when ENI is not found. -var ErrENINotFound = errors.New("ENI is not found") +var ( + // ErrENINotFound is an error when ENI is not found. + ErrENINotFound = errors.New("ENI is not found") + // ErrNoNetworkInterfaces occurs when + // DesribeNetworkInterfaces(eniID) returns no network interfaces + ErrNoNetworkInterfaces = errors.New("No network interfaces found for ENI") +) var log = logger.Get() @@ -857,7 +862,20 @@ func (cache *EC2InstanceMetadataCache) getENIAttachmentID(eniID string) (*string 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 + // Shouldn't happen, but let's be safe + if len(result.NetworkInterfaces) == 0 { + return nil, ErrNoNetworkInterfaces + } + firstNI := result.NetworkInterfaces[0] + + // We cannot assume that the NetworkInterface.Attachment field is a non-nil + // pointer to a NetworkInterfaceAttachment struct. + // Ref: https://github.com/aws/amazon-vpc-cni-k8s/issues/914 + var attachID *string + if firstNI.Attachment != nil { + attachID = firstNI.Attachment.AttachmentId + } + return attachID, nil } func (cache *EC2InstanceMetadataCache) deleteENI(eniName string, maxBackoffDelay time.Duration) error { @@ -906,7 +924,14 @@ func (cache *EC2InstanceMetadataCache) GetIPv4sFromEC2(eniID string) (addrList [ 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].PrivateIpAddresses, nil + + // Shouldn't happen, but let's be safe + if len(result.NetworkInterfaces) == 0 { + return nil, ErrNoNetworkInterfaces + } + firstNI := result.NetworkInterfaces[0] + + return firstNI.PrivateIpAddresses, nil } // DescribeAllENIs calls EC2 to refrech the ENIMetadata and tags for all attached ENIs @@ -938,6 +963,7 @@ func (cache *EC2InstanceMetadataCache) DescribeAllENIs() ([]ENIMetadata, map[str 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 { @@ -1083,7 +1109,7 @@ 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) + log.Errorf("Failed to allocate a private IP addresses on ENI %v: %v", eniID, err) awsAPIErrInc("AssignPrivateIpAddresses", err) if containsPrivateIPAddressLimitExceededError(err) { return nil diff --git a/pkg/awsutils/awsutils_test.go b/pkg/awsutils/awsutils_test.go index 8c7177a226..153a66e472 100644 --- a/pkg/awsutils/awsutils_test.go +++ b/pkg/awsutils/awsutils_test.go @@ -305,6 +305,68 @@ func TestAWSGetFreeDeviceNumberNoDevice(t *testing.T) { assert.Error(t, err) } +func TestGetENIAttachmentID(t *testing.T) { + ctrl, _, mockEC2 := setup(t) + defer ctrl.Finish() + + attachmentID := aws.String("foo-attach") + testCases := []struct { + name string + output *ec2.DescribeNetworkInterfacesOutput + awsErr error + expID *string + expErr error + }{ + { + "success with attachment", + &ec2.DescribeNetworkInterfacesOutput{ + NetworkInterfaces: []*ec2.NetworkInterface{{ + Attachment: &ec2.NetworkInterfaceAttachment{ + AttachmentId: attachmentID, + }, + }}, + }, + nil, + attachmentID, + nil, + }, + { + "success no Attachment", + &ec2.DescribeNetworkInterfacesOutput{ + NetworkInterfaces: []*ec2.NetworkInterface{{}}, + }, + nil, + nil, + nil, + }, + { + "error empty net ifaces", + &ec2.DescribeNetworkInterfacesOutput{ + NetworkInterfaces: []*ec2.NetworkInterface{}, + }, + nil, + nil, + ErrNoNetworkInterfaces, + }, + { + "not found error", + nil, + awserr.New("InvalidNetworkInterfaceID.NotFound", "", nil), + nil, + ErrENINotFound, + }, + } + + for _, tc := range testCases { + mockEC2.EXPECT().DescribeNetworkInterfaces(gomock.Any()).Return(tc.output, tc.awsErr) + + ins := &EC2InstanceMetadataCache{ec2SVC: mockEC2} + id, err := ins.getENIAttachmentID("test-eni") + assert.Equal(t, tc.expErr, err) + assert.Equal(t, tc.expID, id) + } +} + func TestDescribeAllENIs(t *testing.T) { ctrl, mockMetadata, mockEC2 := setup(t) defer ctrl.Finish()