Associate primary network interface SG with the trunk ENI when SG is …

…not specified in ENIConfig (#221)

* Associate primary network interface SG with the trunk ENI when SG is not specified in ENIConfig

pkg/aws/ec2/instance.go

@@ -43,20 +43,20 @@ type ec2Instance struct {
 	currentSubnetID string
 	// currentSubnetCIDRBlock can either point to the Subnet CIDR block for instance subnet or subnet from ENIConfig
 	currentSubnetCIDRBlock string
-	// currentInstanceSecurityGroup can either point to the instance security group or the security group in ENIConfig
-	currentInstanceSecurityGroup []string
+	// currentInstanceSecurityGroups can either point to the primary network interface security groups or the security groups in ENIConfig
+	currentInstanceSecurityGroups []string
 	// subnetMask is the mask of the subnet CIDR block
 	subnetMask string
 	// deviceIndexes is the list of indexes used by the EC2 Instance
 	deviceIndexes []bool
-	// instanceSecurityGroups is the security group used by the primary network interface
-	instanceSecurityGroups []string
+	// primaryENIGroups is the security group used by the primary network interface
+	primaryENISecurityGroups []string
 	// primaryENIID is the ID of the primary network interface of the instance
 	primaryENIID string
 	// newCustomNetworkingSubnetID is the SubnetID from the ENIConfig
 	newCustomNetworkingSubnetID string
-	// newCustomNetworkingSecurityGroup is the security group from the ENIConfig
-	newCustomNetworkingSecurityGroup []string
+	// newCustomNetworkingSecurityGroups is the security groups from the ENIConfig
+	newCustomNetworkingSecurityGroups []string
 }
 
 // EC2Instance exposes the immutable details of an ec2 instance and common operations on an EC2 Instance
@@ -72,7 +72,7 @@ type EC2Instance interface {
 	SubnetMask() string
 	SubnetCidrBlock() string
 	PrimaryNetworkInterfaceID() string
-	InstanceSecurityGroup() []string
+	CurrentInstanceSecurityGroups() []string
 	SetNewCustomNetworkingSpec(subnetID string, securityGroup []string)
 	UpdateCurrentSubnetAndCidrBlock(helper api.EC2APIHelper) error
 }
@@ -140,11 +140,11 @@ func (i *ec2Instance) LoadDetails(ec2APIHelper api.EC2APIHelper) error {
 		i.deviceIndexes[*index] = true
 
 		// Load the Security group of the primary network interface
-		if i.instanceSecurityGroups == nil && (nwInterface.PrivateIpAddress != nil && instance.PrivateIpAddress != nil && *nwInterface.PrivateIpAddress == *instance.PrivateIpAddress) {
+		if i.primaryENISecurityGroups == nil && (nwInterface.PrivateIpAddress != nil && instance.PrivateIpAddress != nil && *nwInterface.PrivateIpAddress == *instance.PrivateIpAddress) {
 			i.primaryENIID = *nwInterface.NetworkInterfaceId
 			// TODO: Group can change, should be refreshed each time we want to use this
 			for _, group := range nwInterface.Groups {
-				i.instanceSecurityGroups = append(i.instanceSecurityGroups, *group.GroupId)
+				i.primaryENISecurityGroups = append(i.primaryENISecurityGroups, *group.GroupId)
 			}
 		}
 	}
@@ -192,12 +192,13 @@ func (i *ec2Instance) PrimaryNetworkInterfaceID() string {
 	return i.primaryENIID
 }
 
-// InstanceSecurityGroup returns the instance security group of the primary network interface
-func (i *ec2Instance) InstanceSecurityGroup() []string {
+// CurrentInstanceSecurityGroups returns the current instance security groups
+// (primary network interface SG or SG specified in the ENIConfig)
+func (i *ec2Instance) CurrentInstanceSecurityGroups() []string {
 	i.lock.RLock()
 	defer i.lock.RUnlock()
 
-	return i.currentInstanceSecurityGroup
+	return i.currentInstanceSecurityGroups
 }
 
 // GetHighestUnusedDeviceIndex assigns a free device index from the end of the list since IPAMD assigns indexes from
@@ -236,7 +237,7 @@ func (i *ec2Instance) SetNewCustomNetworkingSpec(subnet string, securityGroups [
 	defer i.lock.Unlock()
 
 	i.newCustomNetworkingSubnetID = subnet
-	i.newCustomNetworkingSecurityGroup = securityGroups
+	i.newCustomNetworkingSecurityGroups = securityGroups
 }
 
 // UpdateCurrentSubnetAndCidrBlock updates the subnet details under a write lock
@@ -253,7 +254,12 @@ func (i *ec2Instance) updateCurrentSubnetAndCidrBlock(ec2APIHelper api.EC2APIHel
 	// Custom networking is being used on node, point the current subnet ID, CIDR block and
 	// instance security group to the one's present in the Custom networking spec
 	if i.newCustomNetworkingSubnetID != "" {
-		i.currentInstanceSecurityGroup = i.newCustomNetworkingSecurityGroup
+		if i.newCustomNetworkingSecurityGroups != nil && len(i.newCustomNetworkingSecurityGroups) > 0 {
+			i.currentInstanceSecurityGroups = i.newCustomNetworkingSecurityGroups
+		} else {
+			// when security groups are not specified in ENIConfig, use the primary network interface SG as per custom networking documentation
+			i.currentInstanceSecurityGroups = i.primaryENISecurityGroups
+		}
 		// Only get the subnet CIDR block again if the subnet ID has changed
 		if i.newCustomNetworkingSubnetID != i.currentSubnetID {
 			customSubnet, err := ec2APIHelper.GetSubnet(&i.newCustomNetworkingSubnetID)
@@ -267,11 +273,11 @@ func (i *ec2Instance) updateCurrentSubnetAndCidrBlock(ec2APIHelper api.EC2APIHel
 			i.currentSubnetCIDRBlock = *customSubnet.CidrBlock
 		}
 	} else {
-		// Custom networking in not being used, point to the instance security group and
+		// Custom networking in not being used, point to the primary network interface security group and
 		// subnet details
 		i.currentSubnetID = i.instanceSubnetID
 		i.currentSubnetCIDRBlock = i.instanceSubnetCidrBlock
-		i.currentInstanceSecurityGroup = i.instanceSecurityGroups
+		i.currentInstanceSecurityGroups = i.primaryENISecurityGroups
 	}
 
 	return nil

pkg/aws/ec2/instance_test.go

@@ -119,7 +119,7 @@ func TestEc2Instance_LoadDetails(t *testing.T) {
 	assert.Equal(t, subnetCidrBlock, ec2Instance.SubnetCidrBlock())
 	assert.Equal(t, instanceType, ec2Instance.Type())
 	assert.Equal(t, []bool{true, false, true}, ec2Instance.deviceIndexes)
-	assert.Equal(t, []string{securityGroup1, securityGroup2}, ec2Instance.InstanceSecurityGroup())
+	assert.Equal(t, []string{securityGroup1, securityGroup2}, ec2Instance.CurrentInstanceSecurityGroups())
 	assert.Equal(t, primaryInterfaceID, ec2Instance.PrimaryNetworkInterfaceID())
 }
 
@@ -198,7 +198,7 @@ func TestEc2Instance_LoadDetails_SubnetPreLoaded(t *testing.T) {
 
 	// Set the custom networking subnet ID and CIDR block
 	ec2Instance.newCustomNetworkingSubnetID = customNWSubnetID
-	ec2Instance.newCustomNetworkingSecurityGroup = customNWSecurityGroups
+	ec2Instance.newCustomNetworkingSecurityGroups = customNWSecurityGroups
 
 	customSubnet := &ec2.Subnet{CidrBlock: &customNWSubnetCidr}
 
@@ -209,7 +209,7 @@ func TestEc2Instance_LoadDetails_SubnetPreLoaded(t *testing.T) {
 	err := ec2Instance.LoadDetails(mockEC2ApiHelper)
 	assert.NoError(t, err)
 	assert.Equal(t, customNWSubnetID, ec2Instance.currentSubnetID)
-	assert.Equal(t, customNWSecurityGroups, ec2Instance.currentInstanceSecurityGroup)
+	assert.Equal(t, customNWSecurityGroups, ec2Instance.currentInstanceSecurityGroups)
 	assert.Equal(t, customNWSubnetCidr, ec2Instance.currentSubnetCIDRBlock)
 }
 
@@ -332,3 +332,34 @@ func TestEc2Instance_E2E(t *testing.T) {
 	ec2Instance.FreeDeviceIndex(deviceIndex0)
 	assert.False(t, ec2Instance.deviceIndexes[deviceIndex0])
 }
+
+// Tests instance details when custom networking is incorrectly configured- missing security groups
+func TestEc2Instance_LoadDetails_InvalidCustomNetworkingConfiguration(t *testing.T) {
+	ctrl := gomock.NewController(t)
+	defer ctrl.Finish()
+
+	ec2Instance, mockEC2ApiHelper := getMockInstance(ctrl)
+
+	// Set the instance subnet ID and CIDR block
+	ec2Instance.instanceSubnetID = subnetID
+	ec2Instance.instanceSubnetCidrBlock = subnetCidrBlock
+
+	// Set the custom networking subnet ID and CIDR block
+	customNWSubnetID := "custom-networking"
+	ec2Instance.newCustomNetworkingSubnetID = customNWSubnetID
+	ec2Instance.newCustomNetworkingSecurityGroups = []string{}
+
+	customNWSubnetCidr := "192.2.0.0/24"
+	customSubnet := &ec2.Subnet{CidrBlock: &customNWSubnetCidr}
+
+	mockEC2ApiHelper.EXPECT().GetInstanceDetails(&instanceID).Return(nwInterfaces, nil)
+	mockEC2ApiHelper.EXPECT().GetSubnet(&subnetID).Return(subnet, nil)
+	mockEC2ApiHelper.EXPECT().GetSubnet(&customNWSubnetID).Return(customSubnet, nil)
+
+	err := ec2Instance.LoadDetails(mockEC2ApiHelper)
+	assert.NoError(t, err)
+	assert.Equal(t, customNWSubnetID, ec2Instance.currentSubnetID)
+	// Expect the primary network interface security groups when ENIConfig SG is missing
+	assert.Equal(t, []string{securityGroup1, securityGroup2}, ec2Instance.currentInstanceSecurityGroups)
+	assert.Equal(t, customNWSubnetCidr, ec2Instance.currentSubnetCIDRBlock)
+}

pkg/node/manager/manager_test.go

@@ -38,15 +38,24 @@ import (
 )
 
 var (
-	instanceID    = "i-01234567890abcdef"
-	providerId    = "aws:///us-west-2c/" + instanceID
-	eniConfigName = "eni-config-name"
-	subnetID      = "subnet-id"
-	nodeName      = "ip-192-168-55-73.us-west-2.compute.internal"
+	instanceID      = "i-01234567890abcdef"
+	providerId      = "aws:///us-west-2c/" + instanceID
+	eniConfigName   = "eni-config-name"
+	subnetID        = "subnet-id"
+	nodeName        = "ip-192-168-55-73.us-west-2.compute.internal"
+	securityGroupId = "sg-1"
 
 	eniConfig = &v1alpha1.ENIConfig{
 		Spec: v1alpha1.ENIConfigSpec{
-			Subnet: subnetID,
+			SecurityGroups: []string{securityGroupId},
+			Subnet:         subnetID,
+		},
+	}
+
+	eniConfig_empty_sg = &v1alpha1.ENIConfig{
+		Spec: v1alpha1.ENIConfigSpec{
+			SecurityGroups: []string{},
+			Subnet:         subnetID,
 		},
 	}
 
@@ -246,6 +255,34 @@ func Test_AddNode_CustomNetworking(t *testing.T) {
 	assert.True(t, AreNodesEqual(mock.Manager.dataStore[nodeName], managedNode))
 }
 
+// Test adding node when custom networking is enabled but incorrect ENIConfig is defined; it should succeed
+// TODO: combine with other Test_AddNode_CustomNetworking tests
+func Test_AddNode_CustomNetworking_Incorrect_ENIConfig(t *testing.T) {
+	ctrl := gomock.NewController(t)
+	defer ctrl.Finish()
+
+	mock := NewMock(ctrl, map[string]node.Node{})
+
+	job := AsyncOperationJob{
+		op:       Init,
+		nodeName: nodeName,
+		node:     managedNode,
+	}
+
+	nodeWithENIConfig := v1Node.DeepCopy()
+	nodeWithENIConfig.Labels[config.CustomNetworkingLabel] = eniConfigName
+
+	mock.MockK8sAPI.EXPECT().GetNode(nodeName).Return(nodeWithENIConfig, nil)
+	mock.MockK8sAPI.EXPECT().GetENIConfig(eniConfigName).Return(eniConfig_empty_sg, nil)
+	mock.MockWorker.EXPECT().SubmitJob(gomock.All(NewAsyncOperationMatcher(job)))
+
+	err := mock.Manager.AddNode(nodeName)
+	assert.NoError(t, err)
+	assert.Contains(t, mock.Manager.dataStore, nodeName)
+	assert.True(t, AreNodesEqual(mock.Manager.dataStore[nodeName], managedNode))
+
+}
+
 func Test_AddNode_CustomNetworking_NoENIConfig(t *testing.T) {
 	ctrl := gomock.NewController(t)
 	defer ctrl.Finish()

pkg/provider/branch/trunk/trunk.go

@@ -188,7 +188,7 @@ func (t *trunkENI) InitTrunk(instance ec2.EC2Instance, podList []v1.Pod) error {
 		}
 
 		trunk, err := t.ec2ApiHelper.CreateAndAttachNetworkInterface(&instanceID, aws.String(t.instance.SubnetID()),
-			t.instance.InstanceSecurityGroup(), nil, &freeIndex, &TrunkEniDescription, &InterfaceTypeTrunk, 0)
+			t.instance.CurrentInstanceSecurityGroups(), nil, &freeIndex, &TrunkEniDescription, &InterfaceTypeTrunk, 0)
 		if err != nil {
 			trunkENIOperationsErrCount.WithLabelValues("create_trunk_eni").Inc()
 			log.Error(err, "failed to create trunk interface")
@@ -311,7 +311,7 @@ func (t *trunkENI) CreateAndAssociateBranchENIs(pod *v1.Pod, securityGroups []st
 
 	// If the security group is empty use the instance security group
 	if securityGroups == nil || len(securityGroups) == 0 {
-		securityGroups = t.instance.InstanceSecurityGroup()
+		securityGroups = t.instance.CurrentInstanceSecurityGroups()
 	}
 
 	var newENIs []*ENIDetails

pkg/provider/branch/trunk/trunk_test.go

@@ -597,7 +597,7 @@ func TestTrunkENI_InitTrunk_TrunkNotExists(t *testing.T) {
 	freeIndex := int64(2)
 
 	mockInstance.EXPECT().InstanceID().Return(InstanceId)
-	mockInstance.EXPECT().InstanceSecurityGroup().Return(SecurityGroups)
+	mockInstance.EXPECT().CurrentInstanceSecurityGroups().Return(SecurityGroups)
 	mockEC2APIHelper.EXPECT().GetInstanceNetworkInterface(&InstanceId).Return([]*awsEc2.InstanceNetworkInterface{}, nil)
 	mockInstance.EXPECT().GetHighestUnusedDeviceIndex().Return(freeIndex, nil)
 	mockInstance.EXPECT().SubnetID().Return(SubnetId)
@@ -774,7 +774,7 @@ func TestTrunkENI_CreateAndAssociateBranchENIs_InstanceSecurityGroup(t *testing.
 	mockInstance.EXPECT().Type().Return(InstanceType)
 	mockInstance.EXPECT().SubnetID().Return(SubnetId).Times(2)
 	mockInstance.EXPECT().SubnetCidrBlock().Return(SubnetCidrBlock).Times(2)
-	mockInstance.EXPECT().InstanceSecurityGroup().Return(InstanceSecurityGroup)
+	mockInstance.EXPECT().CurrentInstanceSecurityGroups().Return(InstanceSecurityGroup)
 
 	mockEC2APIHelper.EXPECT().CreateNetworkInterface(&BranchEniDescription, &SubnetId, InstanceSecurityGroup,
 		vlan1Tag, 0, nil).Return(BranchInterface1, nil)

pkg/provider/ip/eni/eni.go

@@ -163,7 +163,7 @@ func (e *eniManager) CreateIPV4Address(required int, ec2APIHelper api.EC2APIHelp
 			want = ipLimit
 		}
 		nwInterface, err := ec2APIHelper.CreateAndAttachNetworkInterface(aws.String(e.instance.InstanceID()),
-			aws.String(e.instance.SubnetID()), e.instance.InstanceSecurityGroup(), nil, aws.Int64(deviceIndex),
+			aws.String(e.instance.SubnetID()), e.instance.CurrentInstanceSecurityGroups(), nil, aws.Int64(deviceIndex),
 			&ENIDescription, nil, want)
 		if err != nil {
 			// TODO: Check if any clean up is required here for linux nodes only?

pkg/provider/ip/eni/eni_test.go

@@ -247,7 +247,7 @@ func TestEniManager_CreateIPV4Address_FromNewENI(t *testing.T) {
 	mockInstance.EXPECT().InstanceID().Return(instanceID).Times(2)
 	mockInstance.EXPECT().SubnetID().Return(subnetID).Times(2)
 	mockInstance.EXPECT().SubnetMask().Return(subnetMask).Times(4)
-	mockInstance.EXPECT().InstanceSecurityGroup().Return(instanceSG).Times(2)
+	mockInstance.EXPECT().CurrentInstanceSecurityGroups().Return(instanceSG).Times(2)
 
 	gomock.InOrder(
 		mockEc2APIHelper.EXPECT().CreateAndAttachNetworkInterface(&instanceID, &subnetID, instanceSG, nil, aws.Int64(3),
@@ -283,7 +283,7 @@ func TestEniManager_CreateIPV4Address_InBetweenENIFail(t *testing.T) {
 	mockInstance.EXPECT().GetHighestUnusedDeviceIndex().Return(int64(3), nil).Times(2)
 	mockInstance.EXPECT().InstanceID().Return(instanceID).Times(2)
 	mockInstance.EXPECT().SubnetID().Return(subnetID).Times(2)
-	mockInstance.EXPECT().InstanceSecurityGroup().Return(instanceSG).Times(2)
+	mockInstance.EXPECT().CurrentInstanceSecurityGroups().Return(instanceSG).Times(2)
 
 	gomock.InOrder(
 		mockEc2APIHelper.EXPECT().CreateAndAttachNetworkInterface(&instanceID, &subnetID, instanceSG, nil, aws.Int64(3),