Skip to content

Commit

Permalink
Fix UTs and update SGs for EKS create ENIs
Browse files Browse the repository at this point in the history
  • Loading branch information
@jayanthvn
jayanthvn committed Aug 5, 2020
1 parent 071eff2 commit 54c8d13
Show file tree
Hide file tree
Showing 5 changed files with 109 additions and 16 deletions.
55 changes: 55 additions & 0 deletions pkg/awsutils/awsutils.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,9 @@ type APIs interface {

// GetPrimaryENImac returns the mac address of the primary ENI
GetPrimaryENImac() string

//Setunmanaged ENI
SetUnmanagedENIs(eniID []string) error
}

// EC2InstanceMetadataCache caches instance metadata
Expand All @@ -170,6 +173,8 @@ type EC2InstanceMetadataCache struct {
primaryENImac string
availabilityZone string
region string
accountID string
unmanagedENIs StringSet

ec2Metadata ec2metadata.EC2Metadata
ec2SVC ec2wrapper.EC2
Expand Down Expand Up @@ -387,15 +392,58 @@ func (cache *EC2InstanceMetadataCache) refreshSGIDs(mac string) error {
newSGs := StringSet{}
newSGs.Set(sgIDs)
addedSGs := newSGs.Difference(&cache.securityGroups)
addedSGsCount := 0
deletedSGs := cache.securityGroups.Difference(&newSGs)
deletedSGsCount := 0

for _, sg := range addedSGs.SortedList() {
log.Infof("Found %s, added to ipamd cache", sg)
addedSGsCount++
}
for _, sg := range deletedSGs.SortedList() {
log.Infof("Removed %s from ipamd cache", sg)
deletedSGsCount++
}
cache.securityGroups.Set(sgIDs)

if addedSGsCount != 0 || deletedSGsCount != 0 {
var sgIDsPtrs []*string
sgIDsPtrs = aws.StringSlice(sgIDs)

allENIs, err := cache.GetAttachedENIs()
if err != nil {
return errors.Wrap(err, "DescribeAllENIs: failed to get local ENI metadata")
}

var eniIDs []string
for _, eni := range allENIs {
log.Debugf("Appending eni %s", eni.ENIID)
eniIDs = append(eniIDs, string(eni.ENIID))
}

newENIs := StringSet{}
newENIs.Set(eniIDs)

filteredenis := newENIs.Difference(&cache.unmanagedENIs)

//This will update managed ENIs created by EKS.
for _, eniID := range filteredenis.SortedList() {
log.Debugf("GOT ENI %s", eniID)

// Also change the ENI's attribute so that the ENI will be deleted when the instance is deleted.
attributeInput := &ec2.ModifyNetworkInterfaceAttributeInput{
Groups: sgIDsPtrs,
NetworkInterfaceId: aws.String(eniID),
}
start := time.Now()
_, err = cache.ec2SVC.ModifyNetworkInterfaceAttributeWithContext(context.Background(), attributeInput, userAgent)
awsAPILatency.WithLabelValues("ModifyNetworkInterfaceAttribute", fmt.Sprint(err != nil)).Observe(msSince(start))
if err != nil {
awsAPIErrInc("ModifyNetworkInterfaceAttribute", err)
return errors.Wrap(err, "refreshSGIDs: unable to update the ENI's SG")
}
}
}
return nil
}

Expand Down Expand Up @@ -1332,3 +1380,10 @@ func (cache *EC2InstanceMetadataCache) GetPrimaryENI() string {
func (cache *EC2InstanceMetadataCache) GetPrimaryENImac() string {
return cache.primaryENImac
}

func (cache *EC2InstanceMetadataCache) SetUnmanagedENIs(eniID []string) error {
if len(eniID) != 0 {
cache.unmanagedENIs.Set(eniID)
}
return nil
}
32 changes: 26 additions & 6 deletions pkg/awsutils/awsutils_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ func setup(t *testing.T) (*gomock.Controller,
func TestInitWithEC2metadata(t *testing.T) {
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Millisecond)
defer cancel()
ctrl, mockMetadata, _ := setup(t)
ctrl, mockMetadata, mockEC2 := setup(t)
defer ctrl.Finish()

metadataVPCIPv4CIDRs := "192.168.0.0/16 100.66.0.0/1"
Expand All @@ -77,15 +77,21 @@ func TestInitWithEC2metadata(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, nil)
mockMetadata.EXPECT().GetMetadata(metadataMACPath).Return(primaryMAC, nil)
mockMetadata.EXPECT().GetMetadata(metadataMACPath+primaryMAC+metadataDeviceNum).Return(eni1Device, nil)
mockMetadata.EXPECT().GetMetadata(metadataMACPath+primaryMAC+metadataInterface).Return(primaryMAC, nil)
mockMetadata.EXPECT().GetMetadata(metadataMAC).Return(primaryMAC, nil).AnyTimes()
mockMetadata.EXPECT().GetMetadata(metadataMACPath).Return(primaryMAC, nil).AnyTimes()
mockMetadata.EXPECT().GetMetadata(metadataMACPath+primaryMAC+metadataDeviceNum).Return(eni1Device, nil).AnyTimes()
mockMetadata.EXPECT().GetMetadata(metadataMACPath+primaryMAC+metadataOwnerID).Return("1234", nil)
mockMetadata.EXPECT().GetMetadata(metadataMACPath+primaryMAC+metadataInterface).Return(primaryMAC, nil).AnyTimes()
mockMetadata.EXPECT().GetMetadata(metadataMACPath+primaryMAC+metadataSGs).Return(sgs, nil).AnyTimes()
mockMetadata.EXPECT().GetMetadata(metadataMACPath+primaryMAC+metadataSubnetID).Return(subnetID, nil).AnyTimes()
mockMetadata.EXPECT().GetMetadata(metadataMACPath+primaryMAC+metadataSubnetCIDR).Return(subnetCIDR, nil)
mockMetadata.EXPECT().GetMetadata(metadataMACPath+primaryMAC+metadataVPCcidr).Return(vpcCIDR, nil).AnyTimes()
mockMetadata.EXPECT().GetMetadata(metadataMACPath+primaryMAC+metadataVPCcidrs).Return(metadataVPCIPv4CIDRs, nil).AnyTimes()
mockMetadata.EXPECT().GetMetadata(metadataMACPath+primaryMAC+metadataIPv4s).Return("", nil)

ins := &EC2InstanceMetadataCache{ec2Metadata: mockMetadata}
mockEC2.EXPECT().ModifyNetworkInterfaceAttributeWithContext(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, nil)

ins := &EC2InstanceMetadataCache{ec2Metadata: mockMetadata, ec2SVC: mockEC2}
err := ins.initWithEC2Metadata(ctx)
assert.NoError(t, err)
assert.Equal(t, az, ins.availabilityZone)
Expand Down Expand Up @@ -409,12 +415,25 @@ func TestTagEni(t *testing.T) {
mockMetadata.EXPECT().GetMetadata(metadataInstanceID).Return(instanceID, nil)
mockMetadata.EXPECT().GetMetadata(metadataInstanceType).Return(instanceType, nil)
mockMetadata.EXPECT().GetMetadata(metadataMAC).Return(primaryMAC, nil)
<<<<<<< HEAD
mockMetadata.EXPECT().GetMetadata(metadataMACPath).Return(primaryMAC, nil)
mockMetadata.EXPECT().GetMetadata(metadataMACPath+primaryMAC+metadataDeviceNum).Return(eni1Device, nil)
mockMetadata.EXPECT().GetMetadata(metadataMACPath+primaryMAC+metadataInterface).Return(primaryMAC, nil)
mockMetadata.EXPECT().GetMetadata(metadataMACPath+primaryMAC+metadataSGs).Return(sgs, nil).AnyTimes()
mockMetadata.EXPECT().GetMetadata(metadataMACPath+primaryMAC+metadataSubnetID).Return(subnetID, nil)
=======
mockMetadata.EXPECT().GetMetadata(metadataMACPath).Return(primaryMAC, nil).AnyTimes()
mockMetadata.EXPECT().GetMetadata(metadataMACPath+primaryMAC+metadataDeviceNum).Return(eni1Device, nil).AnyTimes()
mockMetadata.EXPECT().GetMetadata(metadataMACPath+primaryMAC+metadataOwnerID).Return("1234", nil)
mockMetadata.EXPECT().GetMetadata(metadataMACPath+primaryMAC+metadataInterface).Return(primaryMAC, nil).AnyTimes()
mockMetadata.EXPECT().GetMetadata(metadataMACPath+primaryMAC+metadataSGs).Return(sgs, nil).AnyTimes()
mockMetadata.EXPECT().GetMetadata(metadataMACPath+primaryMAC+metadataSubnetID).Return(subnetID, nil)
mockMetadata.EXPECT().GetMetadata(metadataMACPath+primaryMAC+metadataSubnetCIDR).Return(subnetCIDR, nil).AnyTimes()
mockMetadata.EXPECT().GetMetadata(metadataMACPath+primaryMAC+metadataVPCcidr).Return(vpcCIDR, nil)
>>>>>>> Fix UTs and update SGs for EKS create ENIs
mockMetadata.EXPECT().GetMetadata(metadataMACPath+primaryMAC+metadataVPCcidrs).Return(vpcCIDR, nil).AnyTimes()
mockMetadata.EXPECT().GetMetadata(metadataMACPath+primaryMAC+metadataIPv4s).Return("", nil)
mockEC2.EXPECT().ModifyNetworkInterfaceAttributeWithContext(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, nil)

ins := &EC2InstanceMetadataCache{ec2Metadata: mockMetadata, ec2SVC: mockEC2}
err := ins.initWithEC2Metadata(ctx)
Expand All @@ -424,6 +443,7 @@ func TestTagEni(t *testing.T) {
mockEC2.EXPECT().CreateTagsWithContext(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, errors.New("tagging failed"))
mockEC2.EXPECT().CreateTagsWithContext(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, errors.New("tagging failed"))
mockEC2.EXPECT().CreateTagsWithContext(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, nil)

ins.tagENI(eniID, time.Millisecond)
assert.NoError(t, err)
}
Expand Down
17 changes: 15 additions & 2 deletions pkg/awsutils/mocks/awsutils_mocks.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 4 additions & 1 deletion pkg/ipamd/ipamd.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -225,16 +225,19 @@ func (c *IPAMContext) setUnmanagedENIs(tagMap map[string]awsutils.TagMap) {
if len(tagMap) == 0 {
return
}
var unmanagedENIlist []string
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)
unmanagedENIlist = append(unmanagedENIlist, eniID)
}
}
}
c.awsClient.SetUnmanagedENIs(unmanagedENIlist)
}

// ReconcileCooldownCache keep track of recently freed IPs to avoid reading stale EC2 metadata
Expand Down Expand Up @@ -1089,7 +1092,7 @@ func (c *IPAMContext) filterUnmanagedENIs(enis []awsutils.ENIMetadata) []awsutil
for _, eni := range enis {
// 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)
log.Infof("Skipping ENI %s: tagged with %s", eni.ENIID, eniNoManageTagKey)
numFiltered++
continue
}
Expand Down
16 changes: 9 additions & 7 deletions pkg/ipamd/ipamd_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -553,18 +553,20 @@ func TestIPAMContext_filterUnmanagedENIs(t *testing.T) {
mockAWSUtils.EXPECT().GetPrimaryENI().Times(2).Return(eni1.ENIID)

tests := []struct {
name string
tagMap map[string]awsutils.TagMap
enis []awsutils.ENIMetadata
want []awsutils.ENIMetadata
name string
tagMap map[string]awsutils.TagMap
enis []awsutils.ENIMetadata
want []awsutils.ENIMetadata
unmangedenis []string
}{
{"No tags at all", nil, allENIs, allENIs},
{"Primary ENI unmanaged", eni1TagMap, allENIs, allENIs},
{"Secondary ENI unmanaged", eni2TagMap, allENIs, primaryENIonly},
{"No tags at all", nil, allENIs, allENIs, nil},
{"Primary ENI unmanaged", eni1TagMap, allENIs, allENIs, nil},
{"Secondary ENI unmanaged", eni2TagMap, allENIs, primaryENIonly, []string{eni2.ENIID}},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
c := &IPAMContext{awsClient: mockAWSUtils}
mockAWSUtils.EXPECT().SetUnmanagedENIs(tt.unmangedenis).Return(nil).AnyTimes()
c.setUnmanagedENIs(tt.tagMap)
if got := c.filterUnmanagedENIs(tt.enis); !reflect.DeepEqual(got, tt.want) {
t.Errorf("filterUnmanagedENIs() = %v, want %v", got, tt.want)
Expand Down

0 comments on commit 54c8d13

Please sign in to comment.