From a1770732f05eedd531a64e7f258b220d0f8bd419 Mon Sep 17 00:00:00 2001 From: Sushmitha Ravikumar <58063229+sushrk@users.noreply.github.com> Date: Thu, 22 Feb 2024 11:30:07 -0800 Subject: [PATCH] Call DisassociateTrunkInterface before deleting branch ENI (#372) * Call DisassociateTrunkInterface before deleting branch ENI --- .../pkg/aws/ec2/api/mock_ec2_apihelper.go | 14 ++ .../pkg/aws/ec2/api/mock_ec2_wrapper.go | 14 ++ pkg/aws/ec2/api/helper.go | 8 + pkg/aws/ec2/api/wrapper.go | 37 ++- pkg/provider/branch/trunk/trunk.go | 20 +- pkg/provider/branch/trunk/trunk_test.go | 218 +++++++++++++----- 6 files changed, 250 insertions(+), 61 deletions(-) diff --git a/mocks/amazon-vcp-resource-controller-k8s/pkg/aws/ec2/api/mock_ec2_apihelper.go b/mocks/amazon-vcp-resource-controller-k8s/pkg/aws/ec2/api/mock_ec2_apihelper.go index 18f1c8f4..fe0b7d5e 100644 --- a/mocks/amazon-vcp-resource-controller-k8s/pkg/aws/ec2/api/mock_ec2_apihelper.go +++ b/mocks/amazon-vcp-resource-controller-k8s/pkg/aws/ec2/api/mock_ec2_apihelper.go @@ -195,6 +195,20 @@ func (mr *MockEC2APIHelperMockRecorder) DetachNetworkInterfaceFromInstance(arg0 return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DetachNetworkInterfaceFromInstance", reflect.TypeOf((*MockEC2APIHelper)(nil).DetachNetworkInterfaceFromInstance), arg0) } +// DisassociateTrunkInterface mocks base method. +func (m *MockEC2APIHelper) DisassociateTrunkInterface(arg0 *string) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "DisassociateTrunkInterface", arg0) + ret0, _ := ret[0].(error) + return ret0 +} + +// DisassociateTrunkInterface indicates an expected call of DisassociateTrunkInterface. +func (mr *MockEC2APIHelperMockRecorder) DisassociateTrunkInterface(arg0 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DisassociateTrunkInterface", reflect.TypeOf((*MockEC2APIHelper)(nil).DisassociateTrunkInterface), arg0) +} + // GetBranchNetworkInterface mocks base method. func (m *MockEC2APIHelper) GetBranchNetworkInterface(arg0 *string) ([]*ec2.NetworkInterface, error) { m.ctrl.T.Helper() diff --git a/mocks/amazon-vcp-resource-controller-k8s/pkg/aws/ec2/api/mock_ec2_wrapper.go b/mocks/amazon-vcp-resource-controller-k8s/pkg/aws/ec2/api/mock_ec2_wrapper.go index f40d94c6..4553389e 100644 --- a/mocks/amazon-vcp-resource-controller-k8s/pkg/aws/ec2/api/mock_ec2_wrapper.go +++ b/mocks/amazon-vcp-resource-controller-k8s/pkg/aws/ec2/api/mock_ec2_wrapper.go @@ -227,6 +227,20 @@ func (mr *MockEC2WrapperMockRecorder) DetachNetworkInterface(arg0 interface{}) * return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DetachNetworkInterface", reflect.TypeOf((*MockEC2Wrapper)(nil).DetachNetworkInterface), arg0) } +// DisassociateTrunkInterface mocks base method. +func (m *MockEC2Wrapper) DisassociateTrunkInterface(arg0 *ec2.DisassociateTrunkInterfaceInput) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "DisassociateTrunkInterface", arg0) + ret0, _ := ret[0].(error) + return ret0 +} + +// DisassociateTrunkInterface indicates an expected call of DisassociateTrunkInterface. +func (mr *MockEC2WrapperMockRecorder) DisassociateTrunkInterface(arg0 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DisassociateTrunkInterface", reflect.TypeOf((*MockEC2Wrapper)(nil).DisassociateTrunkInterface), arg0) +} + // ModifyNetworkInterfaceAttribute mocks base method. func (m *MockEC2Wrapper) ModifyNetworkInterfaceAttribute(arg0 *ec2.ModifyNetworkInterfaceAttributeInput) (*ec2.ModifyNetworkInterfaceAttributeOutput, error) { m.ctrl.T.Helper() diff --git a/pkg/aws/ec2/api/helper.go b/pkg/aws/ec2/api/helper.go index 3a6cb3ea..2b634271 100644 --- a/pkg/aws/ec2/api/helper.go +++ b/pkg/aws/ec2/api/helper.go @@ -93,6 +93,7 @@ type EC2APIHelper interface { GetInstanceDetails(instanceId *string) (*ec2.Instance, error) AssignIPv4ResourcesAndWaitTillReady(eniID string, resourceType config.ResourceType, count int) ([]string, error) UnassignIPv4Resources(eniID string, resourceType config.ResourceType, resources []string) error + DisassociateTrunkInterface(associationID *string) error } // CreateNetworkInterface creates a new network interface @@ -617,3 +618,10 @@ func (h *ec2APIHelper) DetachAndDeleteNetworkInterface(attachmentID *string, nwI } return nil } + +func (h *ec2APIHelper) DisassociateTrunkInterface(associationID *string) error { + input := &ec2.DisassociateTrunkInterfaceInput{ + AssociationId: associationID, + } + return h.ec2Wrapper.DisassociateTrunkInterface(input) +} diff --git a/pkg/aws/ec2/api/wrapper.go b/pkg/aws/ec2/api/wrapper.go index bcf4cc74..4beaff87 100644 --- a/pkg/aws/ec2/api/wrapper.go +++ b/pkg/aws/ec2/api/wrapper.go @@ -58,6 +58,7 @@ type EC2Wrapper interface { DescribeTrunkInterfaceAssociations(input *ec2.DescribeTrunkInterfaceAssociationsInput) (*ec2.DescribeTrunkInterfaceAssociationsOutput, error) ModifyNetworkInterfaceAttribute(input *ec2.ModifyNetworkInterfaceAttributeInput) (*ec2.ModifyNetworkInterfaceAttributeOutput, error) CreateNetworkInterfacePermission(input *ec2.CreateNetworkInterfacePermissionInput) (*ec2.CreateNetworkInterfacePermissionOutput, error) + DisassociateTrunkInterface(input *ec2.DisassociateTrunkInterfaceInput) error } var ( @@ -253,7 +254,7 @@ var ( ec2AssociateTrunkInterfaceAPIErrCnt = prometheus.NewCounter( prometheus.CounterOpts{ Name: "ec2_associate_trunk_interface_api_err_count", - Help: "The number of errors encountered while disassociating Trunk with Branch ENI", + Help: "The number of errors encountered while associating Trunk with Branch ENI", }, ) @@ -307,6 +308,20 @@ var ( }, ) + ec2DisassociateTrunkInterfaceCallCnt = prometheus.NewCounter( + prometheus.CounterOpts{ + Name: "ec2_disassociate_trunk_interface_api_req_count", + Help: "The number of calls made to EC2 to remove association between a branch and trunk network interface", + }, + ) + + ec2DisassociateTrunkInterfaceErrCnt = prometheus.NewCounter( + prometheus.CounterOpts{ + Name: "ec2_disassociate_trunk_interface_api_err_count", + Help: "The number of errors encountered while removing association between a branch and trunk network interface", + }, + ) + prometheusRegistered = false ) @@ -347,6 +362,8 @@ func prometheusRegister() { ec2APICallLatencies, vpcCniLeakedENICleanupCnt, vpcrcLeakedENICleanupCnt, + ec2DisassociateTrunkInterfaceCallCnt, + ec2DisassociateTrunkInterfaceErrCnt, ) prometheusRegistered = true @@ -375,7 +392,7 @@ func NewEC2Wrapper(roleARN, clusterName, region string, log logr.Logger) (EC2Wra // Role ARN is passed, assume the role ARN to make EC2 API Calls if roleARN != "" { - // Create the instance service client with low QPS, it will be only used fro associate branch to trunk calls + // Create the instance service client with low QPS, it will be only used for associate branch to trunk calls log.Info("Creating INSTANCE service client with configured QPS", "QPS", config.InstanceServiceClientQPS, "Burst", config.InstanceServiceClientBurst) instanceServiceClient, err := ec2Wrapper.getInstanceServiceClient(config.InstanceServiceClientQPS, config.InstanceServiceClientBurst, instanceSession) @@ -788,3 +805,19 @@ func (e *ec2Wrapper) CreateNetworkInterfacePermission(input *ec2.CreateNetworkIn return output, err } + +func (e *ec2Wrapper) DisassociateTrunkInterface(input *ec2.DisassociateTrunkInterfaceInput) error { + start := time.Now() + // Using the instance role + _, err := e.instanceServiceClient.DisassociateTrunkInterface(input) + ec2APICallLatencies.WithLabelValues("disassociate_branch_from_trunk").Observe(timeSinceMs(start)) + + ec2APICallCnt.Inc() + ec2DisassociateTrunkInterfaceCallCnt.Inc() + + if err != nil { + ec2APIErrCnt.Inc() + ec2DisassociateTrunkInterfaceErrCnt.Inc() + } + return err +} diff --git a/pkg/provider/branch/trunk/trunk.go b/pkg/provider/branch/trunk/trunk.go index 6a2eb5dc..940bd3bb 100644 --- a/pkg/provider/branch/trunk/trunk.go +++ b/pkg/provider/branch/trunk/trunk.go @@ -138,6 +138,8 @@ type ENIDetails struct { deletionTimeStamp time.Time // deleteRetryCount is the deleteRetryCount int + // ID of association between branch and trunk ENI + AssociationID string `json:"associationID"` } type IntrospectResponse struct { @@ -398,12 +400,14 @@ func (t *trunkENI) CreateAndAssociateBranchENIs(pod *v1.Pod, securityGroups []st newENIs = append(newENIs, newENI) // Associate Branch to trunk - _, err = t.ec2ApiHelper.AssociateBranchToTrunk(&t.trunkENIId, nwInterface.NetworkInterfaceId, vlanID) + var associationOutput *awsEC2.AssociateTrunkInterfaceOutput + associationOutput, err = t.ec2ApiHelper.AssociateBranchToTrunk(&t.trunkENIId, nwInterface.NetworkInterfaceId, vlanID) if err != nil { err = fmt.Errorf("associating branch to trunk, %w", err) trunkENIOperationsErrCount.WithLabelValues("associate_branch").Inc() break } + newENI.AssociationID = *associationOutput.InterfaceAssociation.AssociationId } if err != nil { @@ -499,7 +503,19 @@ func (t *trunkENI) DeleteCooledDownENIs() { // deleteENIs deletes the provided ENIs and frees up the Vlan assigned to then func (t *trunkENI) deleteENI(eniDetail *ENIDetails) (err error) { - // Delete Branch network interface first + // Disassociate branch ENI from trunk if association ID exists and delete branch network interface + if eniDetail.AssociationID != "" { + err = t.ec2ApiHelper.DisassociateTrunkInterface(&eniDetail.AssociationID) + if err != nil { + trunkENIOperationsErrCount.WithLabelValues("disassociate_trunk_error").Inc() + if !strings.Contains(err.Error(), ec2Errors.NotFoundAssociationID) { + t.log.Error(err, "failed to disassciate branch ENI from trunk, will try to delete the branch ENI") + // Not returning error here, fallback to force branch ENI deletion + } else { + t.log.Info("AssociationID not found when disassociating branch from trunk ENI, it is already disassociated so delete the branch ENI") + } + } + } err = t.ec2ApiHelper.DeleteNetworkInterface(&eniDetail.ID) if err != nil { branchENIOperationsFailureCount.WithLabelValues("delete_branch_error").Inc() diff --git a/pkg/provider/branch/trunk/trunk_test.go b/pkg/provider/branch/trunk/trunk_test.go index cb766cad..438711a3 100644 --- a/pkg/provider/branch/trunk/trunk_test.go +++ b/pkg/provider/branch/trunk/trunk_test.go @@ -62,8 +62,9 @@ var ( Name: MockPodName1, Namespace: MockPodNamespace1, Annotations: map[string]string{config.ResourceNamePodENI: "[{\"eniId\":\"eni-00000000000000000\",\"ifAddress\":\"FF:FF:FF:FF:FF:FF\",\"privateIp\":\"192.168.0.15\"," + - "\"ipv6Addr\":\"2600::\",\"vlanId\":1,\"subnetCidr\":\"192.168.0.0/16\",\"subnetV6Cidr\":\"2600::/64\"},{\"eniId\":\"eni-00000000000000001\",\"ifAddress\":\"" + - "FF:FF:FF:FF:FF:F9\",\"privateIp\":\"192.168.0.16\",\"ipv6Addr\":\"2600::1\",\"vlanId\":2,\"subnetCidr\":\"192.168.0.0/16\",\"subnetV6Cidr\":\"2600::/64\"}]"}}, + "\"ipv6Addr\":\"2600::\",\"vlanId\":1,\"subnetCidr\":\"192.168.0.0/16\",\"subnetV6Cidr\":\"2600::/64\",\"AssociationId\":\"trunk-assoc-0000000000000000\"},{\"eniId\":\"eni-00000000000000001\"" + + ",\"ifAddress\":\"FF:FF:FF:FF:FF:F9\",\"privateIp\":\"192.168.0.16\",\"ipv6Addr\":\"2600::1\",\"vlanId\":2,\"subnetCidr\":\"192.168.0.0/16\",\"subnetV6Cidr\":\"2600::/64\"," + + "\"AssociationId\":\"trunk-assoc-0000000000000001\"}]"}}, Spec: v1.PodSpec{NodeName: NodeName}, Status: v1.PodStatus{}, } @@ -93,20 +94,23 @@ var ( SecurityGroups = []string{SecurityGroup1, SecurityGroup2} // Branch Interface 1 - Branch1Id = "eni-00000000000000000" - MacAddr1 = "FF:FF:FF:FF:FF:FF" - BranchIp1 = "192.168.0.15" - BranchV6Ip1 = "2600::" - VlanId1 = 1 + Branch1Id = "eni-00000000000000000" + MacAddr1 = "FF:FF:FF:FF:FF:FF" + BranchIp1 = "192.168.0.15" + BranchV6Ip1 = "2600::" + VlanId1 = 1 + MockAssociationID1 = "trunk-assoc-0000000000000000" + MockAssociationID2 = "trunk-assoc-0000000000000001" EniDetails1 = &ENIDetails{ - ID: Branch1Id, - MACAdd: MacAddr1, - IPV4Addr: BranchIp1, - IPV6Addr: BranchV6Ip1, - VlanID: VlanId1, - SubnetCIDR: SubnetCidrBlock, - SubnetV6CIDR: SubnetV6CidrBlock, + ID: Branch1Id, + MACAdd: MacAddr1, + IPV4Addr: BranchIp1, + IPV6Addr: BranchV6Ip1, + VlanID: VlanId1, + SubnetCIDR: SubnetCidrBlock, + SubnetV6CIDR: SubnetV6CidrBlock, + AssociationID: MockAssociationID1, } branchENIs1 = []*ENIDetails{EniDetails1} @@ -126,13 +130,14 @@ var ( VlanId2 = 2 EniDetails2 = &ENIDetails{ - ID: Branch2Id, - MACAdd: MacAddr2, - IPV4Addr: BranchIp2, - IPV6Addr: BranchV6Ip2, - VlanID: VlanId2, - SubnetCIDR: SubnetCidrBlock, - SubnetV6CIDR: SubnetV6CidrBlock, + ID: Branch2Id, + MACAdd: MacAddr2, + IPV4Addr: BranchIp2, + IPV6Addr: BranchV6Ip2, + VlanID: VlanId2, + SubnetCIDR: SubnetCidrBlock, + SubnetV6CIDR: SubnetV6CidrBlock, + AssociationID: MockAssociationID2, } BranchInterface2 = &awsEc2.NetworkInterface{ @@ -200,6 +205,27 @@ var ( }, } + mockAssociationOutput1 = &awsEc2.AssociateTrunkInterfaceOutput{ + InterfaceAssociation: &awsEc2.TrunkInterfaceAssociation{ + AssociationId: &MockAssociationID1, + }, + } + mockAssociationOutput2 = &awsEc2.AssociateTrunkInterfaceOutput{ + InterfaceAssociation: &awsEc2.TrunkInterfaceAssociation{ + AssociationId: &MockAssociationID2, + }, + } + + ENIDetailsMissingAssociationID = &ENIDetails{ + ID: Branch2Id, + MACAdd: MacAddr2, + IPV4Addr: BranchIp2, + IPV6Addr: BranchV6Ip2, + VlanID: VlanId2, + SubnetCIDR: SubnetCidrBlock, + SubnetV6CIDR: SubnetV6CidrBlock, + } + MockError = fmt.Errorf("mock error") ) @@ -400,34 +426,106 @@ func TestTrunkENI_getBranchInterfaceMap_EmptyList(t *testing.T) { assert.Zero(t, len(branchENIsMap)) } -// TestTrunkENI_deleteENI tests the trunk is deleted and vlan ID freed in case of no errors +// TestTrunkENI_deleteENI tests deleting branch ENI func TestTrunkENI_deleteENI(t *testing.T) { - ctrl := gomock.NewController(t) - defer ctrl.Finish() - - trunkENI, ec2APIHelper, _ := getMockHelperInstanceAndTrunkObject(ctrl) - trunkENI.markVlanAssigned(VlanId1) - - ec2APIHelper.EXPECT().DeleteNetworkInterface(&Branch1Id).Return(nil) - - err := trunkENI.deleteENI(EniDetails1) - assert.NoError(t, err) - assert.False(t, trunkENI.usedVlanIds[VlanId1]) -} - -// TestTrunkENI_deleteENI_Fail tests if the ENI deletion fails then the vlan ID is not freed -func TestTrunkENI_deleteENI_Fail(t *testing.T) { - ctrl := gomock.NewController(t) - defer ctrl.Finish() + type args struct { + eniDetail *ENIDetails + VlanID int + } + type fields struct { + mockEC2APIHelper *mock_api.MockEC2APIHelper + trunkENI *trunkENI + } + testTrunkENI_deleteENI := []struct { + name string + prepare func(f *fields) + args args + wantErr bool + asserts func(f *fields) + }{ + { + name: "Vland_Freed, verifies VLANID is freed when branch ENI is deleted", + prepare: func(f *fields) { + f.mockEC2APIHelper.EXPECT().DisassociateTrunkInterface(&MockAssociationID1).Return(nil) + f.mockEC2APIHelper.EXPECT().DeleteNetworkInterface(&Branch1Id).Return(nil) + }, + args: args{ + eniDetail: EniDetails1, + VlanID: VlanId1, + }, + wantErr: false, + asserts: func(f *fields) { + assert.False(t, f.trunkENI.usedVlanIds[VlanId1]) + }, + }, + { + name: "Vland_NotFreed, verifies VLANID is not freed when branch ENI delete fails", + prepare: func(f *fields) { + f.mockEC2APIHelper.EXPECT().DisassociateTrunkInterface(&MockAssociationID1).Return(nil) + f.mockEC2APIHelper.EXPECT().DeleteNetworkInterface(&Branch1Id).Return(MockError) + }, + args: args{ + eniDetail: EniDetails1, + VlanID: VlanId1, + }, + wantErr: true, + asserts: func(f *fields) { + assert.True(t, f.trunkENI.usedVlanIds[VlanId1]) + }, + }, + { + name: "DisassociateTrunkInterface_Fails, verifies branch ENI is deleted when disassociation fails for backward compatibility", + prepare: func(f *fields) { + f.mockEC2APIHelper.EXPECT().DisassociateTrunkInterface(&MockAssociationID1).Return(MockError) + f.mockEC2APIHelper.EXPECT().DeleteNetworkInterface(&Branch1Id).Return(nil) + }, + args: args{ + eniDetail: EniDetails1, + VlanID: VlanId1, + }, + wantErr: false, + asserts: func(f *fields) { + assert.False(t, f.trunkENI.usedVlanIds[VlanId1]) + }, + }, + { + name: "MissingAssociationID, verifies DisassociateTrunkInterface is skipped when association ID is missing and branch ENI is deleted for backward compatibility", + prepare: func(f *fields) { + f.mockEC2APIHelper.EXPECT().DeleteNetworkInterface(&Branch2Id).Return(nil) + }, + args: args{ + eniDetail: ENIDetailsMissingAssociationID, + VlanID: VlanId2, + }, + wantErr: false, + asserts: func(f *fields) { + assert.False(t, f.trunkENI.usedVlanIds[VlanId2]) + }, + }, + } - trunkENI, ec2APIHelper, _ := getMockHelperInstanceAndTrunkObject(ctrl) - trunkENI.markVlanAssigned(VlanId1) + for _, tt := range testTrunkENI_deleteENI { + t.Run(tt.name, func(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() - ec2APIHelper.EXPECT().DeleteNetworkInterface(&Branch1Id).Return(MockError) + trunkENI, ec2APIHelper, _ := getMockHelperInstanceAndTrunkObject(ctrl) + trunkENI.markVlanAssigned(tt.args.VlanID) - err := trunkENI.deleteENI(EniDetails1) - assert.Error(t, MockError, err) - assert.True(t, trunkENI.usedVlanIds[VlanId1]) + f := fields{ + mockEC2APIHelper: ec2APIHelper, + trunkENI: trunkENI, + } + if tt.prepare != nil { + tt.prepare(&f) + } + err := f.trunkENI.deleteENI(tt.args.eniDetail) + assert.Equal(t, err != nil, tt.wantErr) + if tt.asserts != nil { + tt.asserts(&f) + } + }) + } } // TestTrunkENI_DeleteCooledDownENIs_NotCooledDown tests that ENIs that have not cooled down are not deleted @@ -463,7 +561,9 @@ func TestTrunkENI_DeleteCooledDownENIs_NoDeletionTimeStamp(t *testing.T) { trunkENI.deleteQueue = append(trunkENI.deleteQueue, EniDetails1, EniDetails2) + ec2APIHelper.EXPECT().DisassociateTrunkInterface(&MockAssociationID1).Return(nil) ec2APIHelper.EXPECT().DeleteNetworkInterface(&EniDetails1.ID).Return(nil) + ec2APIHelper.EXPECT().DisassociateTrunkInterface(&MockAssociationID2).Return(nil) ec2APIHelper.EXPECT().DeleteNetworkInterface(&EniDetails2.ID).Return(nil) mockK8sAPI := mock_k8s.NewMockK8sWrapper(ctrl) @@ -487,6 +587,7 @@ func TestTrunkENI_DeleteCooledDownENIs_CooledDownResource(t *testing.T) { trunkENI.deleteQueue = append(trunkENI.deleteQueue, EniDetails1, EniDetails2) + ec2APIHelper.EXPECT().DisassociateTrunkInterface(&MockAssociationID1).Return(nil) ec2APIHelper.EXPECT().DeleteNetworkInterface(&EniDetails1.ID).Return(nil) mockK8sAPI := mock_k8s.NewMockK8sWrapper(ctrl) @@ -512,16 +613,17 @@ func TestTrunkENI_DeleteCooledDownENIs_DeleteFailed(t *testing.T) { trunkENI.usedVlanIds[VlanId2] = true trunkENI.deleteQueue = append(trunkENI.deleteQueue, EniDetails1, EniDetails2) - gomock.InOrder( - coolDown.EXPECT().GetCoolDownPeriod().Return(time.Second*60).AnyTimes(), - ec2APIHelper.EXPECT().DeleteNetworkInterface(&EniDetails1.ID).Return(MockError).Times(MaxDeleteRetries), - ec2APIHelper.EXPECT().DeleteNetworkInterface(&EniDetails2.ID).Return(nil), - ) mockK8sAPI := mock_k8s.NewMockK8sWrapper(ctrl) mockK8sAPI.EXPECT().GetConfigMap(config.VpcCniConfigMapName, config.KubeSystemNamespace).Return(createCoolDownMockCM("60"), nil) cooldown.InitCoolDownPeriod(mockK8sAPI, zap.New(zap.UseDevMode(true)).WithName("cooldown")) + coolDown.EXPECT().GetCoolDownPeriod().Return(time.Second * 60).AnyTimes() + ec2APIHelper.EXPECT().DisassociateTrunkInterface(&MockAssociationID1).Return(nil).Times(MaxDeleteRetries) + ec2APIHelper.EXPECT().DeleteNetworkInterface(&EniDetails1.ID).Return(MockError).Times(MaxDeleteRetries) + ec2APIHelper.EXPECT().DisassociateTrunkInterface(&MockAssociationID2).Return(nil) + ec2APIHelper.EXPECT().DeleteNetworkInterface(&EniDetails2.ID).Return(nil) + trunkENI.DeleteCooledDownENIs() assert.Zero(t, len(trunkENI.deleteQueue)) } @@ -738,7 +840,9 @@ func TestTrunkENI_DeleteAllBranchENIs(t *testing.T) { trunkENI.deleteQueue = append(trunkENI.deleteQueue, branchENIs1[0]) // Since we added the same branch ENIs in the cool down queue and in the pod to eni map + mockEC2APIHelper.EXPECT().DisassociateTrunkInterface(&MockAssociationID1).Return(nil).Times(2) mockEC2APIHelper.EXPECT().DeleteNetworkInterface(&Branch1Id).Return(nil).Times(2) + mockEC2APIHelper.EXPECT().DisassociateTrunkInterface(&MockAssociationID2).Return(nil) mockEC2APIHelper.EXPECT().DeleteNetworkInterface(&Branch2Id).Return(nil) trunkENI.DeleteAllBranchENIs() @@ -760,10 +864,10 @@ func TestTrunkENI_CreateAndAssociateBranchENIs(t *testing.T) { mockEC2APIHelper.EXPECT().CreateNetworkInterface(&BranchEniDescription, &SubnetId, SecurityGroups, vlan1Tag, nil, nil).Return(BranchInterface1, nil) - mockEC2APIHelper.EXPECT().AssociateBranchToTrunk(&trunkId, &Branch1Id, VlanId1).Return(nil, nil) + mockEC2APIHelper.EXPECT().AssociateBranchToTrunk(&trunkId, &Branch1Id, VlanId1).Return(mockAssociationOutput1, nil) mockEC2APIHelper.EXPECT().CreateNetworkInterface(&BranchEniDescription, &SubnetId, SecurityGroups, vlan2Tag, nil, nil).Return(BranchInterface2, nil) - mockEC2APIHelper.EXPECT().AssociateBranchToTrunk(&trunkId, &Branch2Id, VlanId2).Return(nil, nil) + mockEC2APIHelper.EXPECT().AssociateBranchToTrunk(&trunkId, &Branch2Id, VlanId2).Return(mockAssociationOutput2, nil) eniDetails, err := trunkENI.CreateAndAssociateBranchENIs(MockPod2, SecurityGroups, 2) expectedENIDetails := []*ENIDetails{EniDetails1, EniDetails2} @@ -794,10 +898,10 @@ func TestTrunkENI_CreateAndAssociateBranchENIs_InstanceSecurityGroup(t *testing. mockEC2APIHelper.EXPECT().CreateNetworkInterface(&BranchEniDescription, &SubnetId, InstanceSecurityGroup, vlan1Tag, nil, nil).Return(BranchInterface1, nil) - mockEC2APIHelper.EXPECT().AssociateBranchToTrunk(&trunkId, &Branch1Id, VlanId1).Return(nil, nil) + mockEC2APIHelper.EXPECT().AssociateBranchToTrunk(&trunkId, &Branch1Id, VlanId1).Return(mockAssociationOutput1, nil) mockEC2APIHelper.EXPECT().CreateNetworkInterface(&BranchEniDescription, &SubnetId, InstanceSecurityGroup, vlan2Tag, nil, nil).Return(BranchInterface2, nil) - mockEC2APIHelper.EXPECT().AssociateBranchToTrunk(&trunkId, &Branch2Id, VlanId2).Return(nil, nil) + mockEC2APIHelper.EXPECT().AssociateBranchToTrunk(&trunkId, &Branch2Id, VlanId2).Return(mockAssociationOutput2, nil) eniDetails, err := trunkENI.CreateAndAssociateBranchENIs(MockPod2, []string{}, 2) expectedENIDetails := []*ENIDetails{EniDetails1, EniDetails2} @@ -828,7 +932,7 @@ func TestTrunkENI_CreateAndAssociateBranchENIs_ErrorAssociate(t *testing.T) { gomock.InOrder( mockEC2APIHelper.EXPECT().CreateNetworkInterface(&BranchEniDescription, &SubnetId, SecurityGroups, vlan1Tag, nil, nil).Return(BranchInterface1, nil), - mockEC2APIHelper.EXPECT().AssociateBranchToTrunk(&trunkId, &Branch1Id, VlanId1).Return(nil, nil), + mockEC2APIHelper.EXPECT().AssociateBranchToTrunk(&trunkId, &Branch1Id, VlanId1).Return(mockAssociationOutput1, nil), mockEC2APIHelper.EXPECT().CreateNetworkInterface(&BranchEniDescription, &SubnetId, SecurityGroups, vlan2Tag, nil, nil).Return(BranchInterface2, nil), mockEC2APIHelper.EXPECT().AssociateBranchToTrunk(&trunkId, &Branch2Id, VlanId2).Return(nil, MockError), @@ -836,7 +940,7 @@ func TestTrunkENI_CreateAndAssociateBranchENIs_ErrorAssociate(t *testing.T) { _, err := trunkENI.CreateAndAssociateBranchENIs(MockPod2, SecurityGroups, 2) assert.Error(t, MockError, err) - assert.Equal(t, []*ENIDetails{EniDetails1, EniDetails2}, trunkENI.deleteQueue) + assert.Equal(t, []*ENIDetails{EniDetails1, ENIDetailsMissingAssociationID}, trunkENI.deleteQueue) } // TestTrunkENI_CreateAndAssociateBranchENIs_ErrorCreate tests if error is returned on associate then the created interfaces @@ -856,7 +960,7 @@ func TestTrunkENI_CreateAndAssociateBranchENIs_ErrorCreate(t *testing.T) { gomock.InOrder( mockEC2APIHelper.EXPECT().CreateNetworkInterface(&BranchEniDescription, &SubnetId, SecurityGroups, vlan1Tag, nil, nil).Return(BranchInterface1, nil), - mockEC2APIHelper.EXPECT().AssociateBranchToTrunk(&trunkId, &Branch1Id, VlanId1).Return(nil, nil), + mockEC2APIHelper.EXPECT().AssociateBranchToTrunk(&trunkId, &Branch1Id, VlanId1).Return(mockAssociationOutput1, nil), mockEC2APIHelper.EXPECT().CreateNetworkInterface(&BranchEniDescription, &SubnetId, SecurityGroups, vlan2Tag, nil, nil).Return(nil, MockError), )