support labeling node with not-supported for CNI managing events (#164)

* support labeling node with not-supported for CNI managing events

* change the label workflow to false, true/not-supported

controllers/core/event_controller.go

@@ -165,7 +165,9 @@ func (r *EventReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl
 				return ctrl.Result{}, nil
 			} else {
 				if r.isEventToManageNode(event) && r.isValidEventForSGP(event) {
-					labelled, err := r.K8sAPI.AddLabelToManageNode(node, config.HasTrunkAttachedLabel, config.BooleanTrue)
+					// make the label value to false that indicates the node is ok to be proceeded by providers
+					// provider will decide if the node can be attached with trunk interface
+					labelled, err := r.K8sAPI.AddLabelToManageNode(node, config.HasTrunkAttachedLabel, config.BooleanFalse)
 					if err != nil {
 						// by returning an error, we request that our controller will get Reconcile() called again
 						// we use the GetNode() as a safeguard to avoid repeating reconciling on deleted nodes
@@ -174,7 +176,7 @@ func (r *EventReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl
 					}
 					if labelled {
 						eventControllerSGPEventsCount.WithLabelValues(SGPEventsCountMetrics).Inc()
-						r.Log.Info("Label node with trunk label as true", "Node", node.Name, "Label", config.HasTrunkAttachedLabel)
+						r.Log.V(1).Info("Label node with trunk label", "Node", node.Name, "Label", config.HasTrunkAttachedLabel)
 					}
 				}
 				if r.isValidEventForCustomNetworking(event) {
@@ -189,7 +191,7 @@ func (r *EventReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl
 						}
 						if labelled {
 							eventControllerCNEventsCount.WithLabelValues(CNEventsCountMetrics).Inc()
-							r.Log.Info("Label node with eniconfig label with configured name", "Node", node.Name, "Label", config.CustomNetworkingLabel)
+							r.Log.V(1).Info("Label node with eniconfig label with configured name", "Node", node.Name, "Label", config.CustomNetworkingLabel)
 						}
 					}
 				}

controllers/core/event_controller_test.go

@@ -185,9 +185,9 @@ func TestEventReconciler_Reconcile_SGPEvent(t *testing.T) {
 			// if the event is older, these func are not expected to be called.
 			mock.MockK8sAPI.EXPECT().GetNode(mockEventNodeName).Return(eventNode, nil).AnyTimes()
 			if e.successfullyLabelNode {
-				mock.MockK8sAPI.EXPECT().AddLabelToManageNode(eventNode, config.HasTrunkAttachedLabel, "true").Return(true, nil)
+				mock.MockK8sAPI.EXPECT().AddLabelToManageNode(eventNode, config.HasTrunkAttachedLabel, config.BooleanFalse).Return(true, nil)
 			} else {
-				mock.MockK8sAPI.EXPECT().AddLabelToManageNode(eventNode, config.HasTrunkAttachedLabel, "true").Return(false, errors.New("sgp-test"))
+				mock.MockK8sAPI.EXPECT().AddLabelToManageNode(eventNode, config.HasTrunkAttachedLabel, config.BooleanFalse).Return(false, errors.New("sgp-test"))
 			}
 		}
 		res, err := mock.Reconciler.Reconcile(context.TODO(), sgpEventReconcileRequest)

pkg/config/type.go

@@ -37,8 +37,10 @@ const (
 	HasTrunkAttachedLabel = "vpc.amazonaws.com/has-trunk-attached"
 	// CustomNetworkingLabel is the label with the name of ENIConfig to be used by the node for custom networking
 	CustomNetworkingLabel = "vpc.amazonaws.com/eniConfig"
-	BooleanTrue           = "true"
-	BooleanFalse          = "false"
+	// Trunk attaching status value
+	BooleanTrue         = "true"
+	BooleanFalse        = "false"
+	NotSupportedEc2Type = "not-supported"
 	// NodeLabelOS is the Kubernetes Operating System label
 	NodeLabelOS = "kubernetes.io/os"
 	// NodeLabelOS is the Kubernetes Operating System label used before k8s version 1.16

pkg/provider/branch/provider.go

@@ -462,13 +462,35 @@ func (b *branchENIProvider) GetPool(_ string) (pool.Pool, bool) {
 // IsInstanceSupported returns true for linux node as pod eni is only supported for linux worker node
 func (b *branchENIProvider) IsInstanceSupported(instance ec2.EC2Instance) bool {
 	limits, found := vpc.Limits[instance.Type()]
-	if !found {
-		return false
-	}
-	if instance.Os() == config.OSLinux && limits.IsTrunkingCompatible {
-		return true
+	supported := found && instance.Os() == config.OSLinux && limits.IsTrunkingCompatible
+
+	// 1, since VPC CNI will not send events for nodes that are labelled as not-supported
+	// 2, since VPC CNI will not keep sending events if nodes have been labelled as true and trunk has been attached
+	// here calling GetNode API will not cause large call volume
+	if node, err := b.apiWrapper.K8sAPI.GetNode(instance.Name()); err != nil {
+		b.log.Error(err, "couldn't get the node when checking if the node is supported for trunk interface", "NodeName", instance.Name())
+	} else {
+		if node.Labels != nil {
+			if _, ok := node.Labels[config.HasTrunkAttachedLabel]; ok {
+				labelValue := config.BooleanTrue
+				if !supported {
+					labelValue = config.NotSupportedEc2Type
+				}
+				updated, err := b.apiWrapper.K8sAPI.AddLabelToManageNode(node, config.HasTrunkAttachedLabel, labelValue)
+				if err != nil {
+					b.log.Error(err, "failed to update node label", "NodeName", instance.Name(), "LabelKey", config.HasTrunkAttachedLabel, "ExpectedLabelValue", labelValue)
+				} else if !updated {
+					b.log.V(1).Info("unable to update node with the existing label key/value pair", "NodeName", instance.Name(), "LabelKey", config.HasTrunkAttachedLabel, "ExpectedLabelValue", labelValue)
+				}
+			}
+		} else {
+			// at this point, the label map shouldn't be nil
+			// TODO: https://github.com/aws/amazon-vpc-resource-controller-k8s/issues/163
+			b.log.Error(err, "unexpected nil map for node labels", "NodeName", instance.Name())
+		}
 	}
-	return false
+
+	return supported
 }
 
 func (b *branchENIProvider) Introspect() interface{} {

pkg/provider/branch/provider_test.go

@@ -20,12 +20,12 @@ import (
 	"reflect"
 	"testing"
 
-	"github.com/aws/amazon-vpc-resource-controller-k8s/mocks/amazon-vcp-resource-controller-k8s/pkg/aws/ec2"
-	"github.com/aws/amazon-vpc-resource-controller-k8s/mocks/amazon-vcp-resource-controller-k8s/pkg/k8s"
-	"github.com/aws/amazon-vpc-resource-controller-k8s/mocks/amazon-vcp-resource-controller-k8s/pkg/k8s/pod"
-	"github.com/aws/amazon-vpc-resource-controller-k8s/mocks/amazon-vcp-resource-controller-k8s/pkg/provider/branch/trunk"
-	"github.com/aws/amazon-vpc-resource-controller-k8s/mocks/amazon-vcp-resource-controller-k8s/pkg/utils"
-	"github.com/aws/amazon-vpc-resource-controller-k8s/mocks/amazon-vcp-resource-controller-k8s/pkg/worker"
+	mock_ec2 "github.com/aws/amazon-vpc-resource-controller-k8s/mocks/amazon-vcp-resource-controller-k8s/pkg/aws/ec2"
+	mock_k8s "github.com/aws/amazon-vpc-resource-controller-k8s/mocks/amazon-vcp-resource-controller-k8s/pkg/k8s"
+	mock_pod "github.com/aws/amazon-vpc-resource-controller-k8s/mocks/amazon-vcp-resource-controller-k8s/pkg/k8s/pod"
+	mock_trunk "github.com/aws/amazon-vpc-resource-controller-k8s/mocks/amazon-vcp-resource-controller-k8s/pkg/provider/branch/trunk"
+	mock_utils "github.com/aws/amazon-vpc-resource-controller-k8s/mocks/amazon-vcp-resource-controller-k8s/pkg/utils"
+	mock_worker "github.com/aws/amazon-vpc-resource-controller-k8s/mocks/amazon-vcp-resource-controller-k8s/pkg/worker"
 	"github.com/aws/amazon-vpc-resource-controller-k8s/pkg/api"
 	"github.com/aws/amazon-vpc-resource-controller-k8s/pkg/aws/vpc"
 	"github.com/aws/amazon-vpc-resource-controller-k8s/pkg/config"
@@ -293,6 +293,31 @@ func TestBranchENIProvider_GetResourceCapacity_NotSupported(t *testing.T) {
 	assert.NoError(t, err)
 }
 
+func TestBranchENIProvider_NotSupported_LabelNode(t *testing.T) {
+	ctrl := gomock.NewController(t)
+	defer ctrl.Finish()
+
+	provider, mockK8sWrapper := getProviderAndMockK8sWrapper(ctrl)
+	mockInstance := mock_ec2.NewMockEC2Instance(ctrl)
+
+	supportedInstanceType := "t3.medium"
+	node := &v1.Node{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:   NodeName,
+			Labels: map[string]string{config.HasTrunkAttachedLabel: config.BooleanFalse},
+		},
+	}
+
+	mockInstance.EXPECT().Name().Return(NodeName)
+	mockInstance.EXPECT().Os().Return("linux")
+	mockInstance.EXPECT().Type().Return(supportedInstanceType)
+	mockK8sWrapper.EXPECT().GetNode(NodeName).Return(node, nil)
+	mockK8sWrapper.EXPECT().AddLabelToManageNode(node, config.HasTrunkAttachedLabel, config.NotSupportedEc2Type).Return(true, nil)
+
+	supported := provider.IsInstanceSupported(mockInstance)
+	assert.False(t, supported)
+}
+
 // TestBranchENIProvider_CreateAndAnnotateResources tests that create is invoked equal to the number of resources to
 // be created
 func TestBranchENIProvider_CreateAndAnnotateResources(t *testing.T) {