Merging Eni cleanup branch to master (#535)

* Centralized leaked ENI cleanup- CNINode CRD changes

* Centralized leaked ENI cleanup- refactor periodic cleanup & add node termination cleaner (#505)

* Centralized leaked ENI cleanup- refactor periodic cleanup & add node termination cleaner

* defer updating metrics

* WIP commit no.3

* instance id as tag, not requeuing on node cleanup failure

* not requeing on node termination cleaner failure

* minor log change

* adding node termination failure metric

* minor test change

* using paginated calls to describe network interface while eni cleanup

* refactor based on comments

---------

Co-authored-by: Yash Thakkar <[email protected]>

---------

Co-authored-by: sushrk <[email protected]>
Co-authored-by: Sushmitha Ravikumar <[email protected]>

Author: 2 people

PROJECT

@@ -19,6 +19,7 @@ resources:
   version: v1beta1
 - api:
     crdVersion: v1
+  controller: true
   domain: k8s.aws
   group: vpcresources
   kind: CNINode

apis/vpcresources/v1alpha1/cninode_types.go

@@ -35,6 +35,8 @@ type Feature struct {
 // CNINodeSpec defines the desired state of CNINode
 type CNINodeSpec struct {
 	Features []Feature `json:"features,omitempty"`
+	// Additional tag key/value added to all network interfaces provisioned by the vpc-resource-controller and VPC-CNI
+	Tags map[string]string `json:"tags,omitempty"`
 }
 
 // CNINodeStatus defines the managed VPC resources.

apis/vpcresources/v1alpha1/zz_generated.deepcopy.go

@@ -61,6 +61,12 @@ spec:
                       type: string
                   type: object
                 type: array
+              tags:
+                additionalProperties:
+                  type: string
+                description: Additional tag key/value added to all network interfaces
+                  provisioned by the vpc-resource-controller and VPC-CNI
+                type: object
             type: object
           status:
             description: CNINodeStatus defines the managed VPC resources.

config/crd/bases/vpcresources.k8s.aws_cninodes.yaml

@@ -60,6 +60,8 @@ rules:
   - create
   - get
   - list
+  - patch
+  - update
   - watch
 - apiGroups:
   - vpcresources.k8s.aws

config/rbac/role.yaml

@@ -27,7 +27,6 @@ import (
 
 	"github.com/go-logr/logr"
 	corev1 "k8s.io/api/core/v1"
-	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime"
 	ctrl "sigs.k8s.io/controller-runtime"
@@ -86,7 +85,7 @@ func (r *ConfigMapReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 				r.K8sAPI,
 				utils.BranchENICoolDownUpdateReason,
 				fmt.Sprintf("Branch ENI cool down period has been updated to %s", cooldown.GetCoolDown().GetCoolDownPeriod()),
-				v1.EventTypeNormal,
+				corev1.EventTypeNormal,
 				r.Log,
 			)
 		}

controllers/core/configmap_controller.go

@@ -22,7 +22,6 @@ import (
 	"github.com/golang/mock/gomock"
 	"github.com/stretchr/testify/assert"
 	corev1 "k8s.io/api/core/v1"
-	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
@@ -225,8 +224,8 @@ func Test_Reconcile_UpdateNode_Error(t *testing.T) {
 
 }
 
-func createCoolDownMockCM(cooldownTime string) *v1.ConfigMap {
-	return &v1.ConfigMap{
+func createCoolDownMockCM(cooldownTime string) *corev1.ConfigMap {
+	return &corev1.ConfigMap{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      config.VpcCniConfigMapName,
 			Namespace: config.KubeSystemNamespace,

controllers/core/configmap_controller_test.go

@@ -15,7 +15,6 @@ package controllers
 
 import (
 	"context"
-	"fmt"
 	"net/http"
 	"time"
 
@@ -25,8 +24,6 @@ import (
 	"github.com/aws/amazon-vpc-resource-controller-k8s/pkg/k8s"
 	"github.com/aws/amazon-vpc-resource-controller-k8s/pkg/node/manager"
 	"github.com/google/uuid"
-	"github.com/prometheus/client_golang/prometheus"
-	"sigs.k8s.io/controller-runtime/pkg/metrics"
 
 	"github.com/go-logr/logr"
 	corev1 "k8s.io/api/core/v1"
@@ -36,21 +33,9 @@ import (
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller"
-	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	"sigs.k8s.io/controller-runtime/pkg/healthz"
 )
 
-var (
-	leakedCNINodeResourceCount = prometheus.NewCounter(
-		prometheus.CounterOpts{
-			Name: "orphaned_cninode_objects",
-			Help: "The number of CNINode objects that do not have a parent Node object (likely indicating a leak from a deleted node)",
-		},
-	)
-
-	prometheusRegistered = false
-)
-
 // MaxNodeConcurrentReconciles is the number of go routines that can invoke
 // Reconcile in parallel. Since Node Reconciler, performs local operation
 // on cache only a single go routine should be sufficient. Using more than
@@ -93,23 +78,6 @@ func (r *NodeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.
 
 	if nodeErr := r.Client.Get(ctx, req.NamespacedName, node); nodeErr != nil {
 		if errors.IsNotFound(nodeErr) {
-			// clean up CNINode finalizer
-			cniNode := &v1alpha1.CNINode{}
-			if cninodeErr := r.Client.Get(ctx, req.NamespacedName, cniNode); cninodeErr == nil {
-				if yes := controllerutil.ContainsFinalizer(cniNode, NodeTerminationFinalizer); yes {
-					updated := cniNode.DeepCopy()
-					if yes = controllerutil.RemoveFinalizer(updated, NodeTerminationFinalizer); yes {
-						if err := r.Client.Patch(ctx, updated, client.MergeFrom(cniNode)); err != nil {
-							return ctrl.Result{}, err
-						}
-						r.Log.Info("removed leaked CNINode resource's finalizer", "cninode", cniNode.Name)
-					}
-					leakedCNINodeResourceCount.Inc()
-				}
-			} else if !errors.IsNotFound(cninodeErr) {
-				return ctrl.Result{}, fmt.Errorf("failed getting CNINode %s from cached client, %w", cniNode.Name, cninodeErr)
-			}
-
 			// clean up local cached nodes
 			_, found := r.Manager.GetNode(req.Name)
 			if found {
@@ -148,8 +116,6 @@ func (r *NodeReconciler) SetupWithManager(mgr ctrl.Manager, maxConcurrentReconci
 		map[string]healthz.Checker{"health-node-controller": r.Check()},
 	)
 
-	prometheusRegister()
-
 	return ctrl.NewControllerManagedBy(mgr).
 		For(&corev1.Node{}).
 		WithOptions(controller.Options{MaxConcurrentReconciles: maxConcurrentReconciles}).
@@ -193,11 +159,3 @@ func (r *NodeReconciler) Check() healthz.Checker {
 		return err
 	}
 }
-
-func prometheusRegister() {
-	if !prometheusRegistered {
-		metrics.Registry.MustRegister(leakedCNINodeResourceCount)
-
-		prometheusRegistered = true
-	}
-}

controllers/core/node_controller.go

@@ -32,7 +32,6 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	fakeClient "sigs.k8s.io/controller-runtime/pkg/client/fake"
-	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	"sigs.k8s.io/controller-runtime/pkg/log/zap"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 )
@@ -143,43 +142,6 @@ func TestNodeReconciler_Reconcile_DeleteNonExistentNode(t *testing.T) {
 	assert.Equal(t, res, reconcile.Result{})
 }
 
-func TestNodeReconciler_Reconcile_DeleteNonExistentNodesCNINode(t *testing.T) {
-	ctrl := gomock.NewController(t)
-	defer ctrl.Finish()
-
-	mock := NewNodeMock(ctrl)
-	cniNode := &v1alpha1.CNINode{
-		ObjectMeta: v1.ObjectMeta{
-			Name:       mockNodeName,
-			Finalizers: []string{NodeTerminationFinalizer},
-		},
-	}
-	mock.Reconciler.Client = fakeClient.NewClientBuilder().WithScheme(mock.Reconciler.Scheme).WithObjects(cniNode).Build()
-
-	mock.Conditions.EXPECT().GetPodDataStoreSyncStatus().Return(true)
-	mock.Manager.EXPECT().GetNode(mockNodeName).Return(mock.MockNode, false)
-
-	original := &v1alpha1.CNINode{}
-	err := mock.Reconciler.Client.Get(context.TODO(), types.NamespacedName{Name: cniNode.Name}, original)
-	assert.NoError(t, err)
-	assert.True(t, controllerutil.ContainsFinalizer(original, NodeTerminationFinalizer), "the CNINode has finalizer")
-
-	res, err := mock.Reconciler.Reconcile(context.TODO(), reconcileRequest)
-	assert.NoError(t, err)
-	assert.Equal(t, res, reconcile.Result{})
-
-	node := &corev1.Node{}
-	updated := &v1alpha1.CNINode{}
-	err = mock.Reconciler.Client.Get(context.TODO(), types.NamespacedName{Name: cniNode.Name}, node)
-	assert.Error(t, err, "the node shouldn't existing")
-	assert.True(t, errors.IsNotFound(err))
-
-	err = mock.Reconciler.Client.Get(context.TODO(), types.NamespacedName{Name: cniNode.Name}, updated)
-	assert.NoError(t, err)
-	assert.True(t, updated.Name == mockNodeName, "the CNINode should existing and waiting for finalizer removal")
-	assert.False(t, controllerutil.ContainsFinalizer(updated, NodeTerminationFinalizer), "CNINode finalizer should be removed when the node is gone")
-}
-
 func TestNodeReconciler_Reconcile_DeleteNonExistentUnmanagedNode(t *testing.T) {
 	ctrl := gomock.NewController(t)
 	defer ctrl.Finish()

controllers/core/node_controller_test.go

@@ -153,7 +153,7 @@ func (r *PodReconciler) Reconcile(request custom.Request) (ctrl.Result, error) {
 		} else {
 			result, err = resourceHandler.HandleCreate(int(totalCount), pod)
 		}
-		if err != nil || result.Requeue == true {
+		if err != nil || result.Requeue {
 			return result, err
 		}
 		logger.V(1).Info("handled resource without error",