diff --git a/PROJECT b/PROJECT index b9b09ae0..22571eb8 100644 --- a/PROJECT +++ b/PROJECT @@ -19,6 +19,7 @@ resources: version: v1beta1 - api: crdVersion: v1 + controller: true domain: k8s.aws group: vpcresources kind: CNINode diff --git a/apis/vpcresources/v1alpha1/cninode_types.go b/apis/vpcresources/v1alpha1/cninode_types.go index 8555f14a..7954673a 100644 --- a/apis/vpcresources/v1alpha1/cninode_types.go +++ b/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. diff --git a/apis/vpcresources/v1alpha1/zz_generated.deepcopy.go b/apis/vpcresources/v1alpha1/zz_generated.deepcopy.go index 3fbda7f0..30d7a9ed 100644 --- a/apis/vpcresources/v1alpha1/zz_generated.deepcopy.go +++ b/apis/vpcresources/v1alpha1/zz_generated.deepcopy.go @@ -89,6 +89,13 @@ func (in *CNINodeSpec) DeepCopyInto(out *CNINodeSpec) { *out = make([]Feature, len(*in)) copy(*out, *in) } + if in.Tags != nil { + in, out := &in.Tags, &out.Tags + *out = make(map[string]string, len(*in)) + for key, val := range *in { + (*out)[key] = val + } + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new CNINodeSpec. diff --git a/config/crd/bases/vpcresources.k8s.aws_cninodes.yaml b/config/crd/bases/vpcresources.k8s.aws_cninodes.yaml index 393a50ab..0b4566da 100644 --- a/config/crd/bases/vpcresources.k8s.aws_cninodes.yaml +++ b/config/crd/bases/vpcresources.k8s.aws_cninodes.yaml @@ -56,6 +56,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. diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index b292d57b..5a87d016 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -61,6 +61,8 @@ rules: - create - get - list + - patch + - update - watch - apiGroups: - vpcresources.k8s.aws diff --git a/controllers/core/node_controller.go b/controllers/core/node_controller.go index 8a1f8b0e..326e53cb 100644 --- a/controllers/core/node_controller.go +++ b/controllers/core/node_controller.go @@ -20,6 +20,7 @@ import ( "github.com/aws/amazon-vpc-resource-controller-k8s/apis/vpcresources/v1alpha1" "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/condition" + "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/config" rcHealthz "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/healthz" "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/k8s" "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/node/manager" @@ -36,15 +37,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/healthz" ) -// 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 -// one routines to help high rate churn and larger nodes groups restarting -// when the controller has to be restarted for various reasons. -const ( - MaxNodeConcurrentReconciles = 10 -) - // NodeReconciler reconciles a Node object type NodeReconciler struct { client.Client @@ -117,7 +109,7 @@ func (r *NodeReconciler) SetupWithManager(mgr ctrl.Manager, healthzHandler *rcHe return ctrl.NewControllerManagedBy(mgr). For(&corev1.Node{}). - WithOptions(controller.Options{MaxConcurrentReconciles: MaxNodeConcurrentReconciles}). + WithOptions(controller.Options{MaxConcurrentReconciles: config.MaxNodeConcurrentReconciles}). Owns(&v1alpha1.CNINode{}). Complete(r) } diff --git a/controllers/crds/cninode_controller.go b/controllers/crds/cninode_controller.go new file mode 100644 index 00000000..a67d08d2 --- /dev/null +++ b/controllers/crds/cninode_controller.go @@ -0,0 +1,223 @@ +// Copyright Amazon.com Inc. or its affiliates. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"). You may +// not use this file except in compliance with the License. A copy of the +// License is located at +// +// http://aws.amazon.com/apache2.0/ +// +// or in the "license" file accompanying this file. This file is distributed +// on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either +// express or implied. See the License for the specific language governing +// permissions and limitations under the License. + +package crds + +import ( + "context" + "fmt" + "time" + + "github.com/aws/amazon-vpc-resource-controller-k8s/apis/vpcresources/v1alpha1" + ec2API "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/aws/ec2/api" + "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/aws/ec2/api/cleanup" + "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/config" + "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/k8s" + "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/utils" + "github.com/go-logr/logr" + "github.com/prometheus/client_golang/prometheus" + v1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/wait" + "k8s.io/client-go/util/retry" + 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/metrics" +) + +var ( + prometheusRegistered = false + recreateCNINodeCallCount = prometheus.NewCounter( + prometheus.CounterOpts{ + Name: "recreate_cniNode_call_count", + Help: "The number of requests made by controller to recreate CNINode when node exists", + }, + ) + recreateCNINodeErrCount = prometheus.NewCounter( + prometheus.CounterOpts{ + Name: "recreate_cniNode_err_count", + Help: "The number of requests that failed when controller tried to recreate the CNINode", + }, + ) +) + +func prometheusRegister() { + prometheusRegistered = true + + metrics.Registry.MustRegister( + recreateCNINodeCallCount, + recreateCNINodeErrCount) + + prometheusRegistered = true +} + +// CNINodeReconciler reconciles a CNINode object +type CNINodeReconciler struct { + client.Client + Scheme *runtime.Scheme + Context context.Context + Log logr.Logger + EC2Wrapper ec2API.EC2Wrapper + K8sAPI k8s.K8sWrapper + ClusterName string + VPCID string + FinalizerManager k8s.FinalizerManager +} + +//+kubebuilder:rbac:groups=vpcresources.k8s.aws,resources=cninodes,verbs=get;list;watch;create;update;patch; + +// Reconcile handles CNINode create/update/delete events +// Reconciler will add the finalizer and cluster name tag if it does not exist and finalize on CNINode on deletion to clean up leaked resource on node +func (r *CNINodeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { + cniNode := &v1alpha1.CNINode{} + if err := r.Client.Get(ctx, req.NamespacedName, cniNode); err != nil { + if errors.IsNotFound(err) { + r.Log.Info("CNINode is deleted", "CNINode", req.NamespacedName) + } + // Ignore not found error + return ctrl.Result{}, client.IgnoreNotFound(err) + } + + if cniNode.GetDeletionTimestamp().IsZero() { + // Add cluster name tag if it does not exist + clusterNameTagKey := fmt.Sprintf(config.ClusterNameTagKeyFormat, r.ClusterName) + val, ok := cniNode.Spec.Tags[clusterNameTagKey] + if !ok || val != config.ClusterNameTagValue { + cniNodeCopy := cniNode.DeepCopy() + if len(cniNodeCopy.Spec.Tags) != 0 { + cniNodeCopy.Spec.Tags[clusterNameTagKey] = config.ClusterNameTagValue + } else { + cniNodeCopy.Spec.Tags = map[string]string{ + clusterNameTagKey: config.ClusterNameTagValue, + } + } + return ctrl.Result{}, r.Client.Patch(ctx, cniNodeCopy, client.MergeFromWithOptions(cniNode, client.MergeFromWithOptimisticLock{})) + } + if err := r.FinalizerManager.AddFinalizers(ctx, cniNode, config.NodeTerminationFinalizer); err != nil { + r.Log.Error(err, "failed to add finalizer on CNINode, will retry", "cniNode", cniNode.Name, "finalizer", config.NodeTerminationFinalizer) + return ctrl.Result{}, err + } + r.Log.Info("added finalizer on cninode", "finalizer", config.NodeTerminationFinalizer, "cniNode", cniNode.Name) + return ctrl.Result{}, nil + + } else { // CNINode is marked for deletion + // check if node object exists + node := &v1.Node{} + if err := r.Client.Get(ctx, req.NamespacedName, node); err != nil { + if errors.IsNotFound(err) { + // node is also deleted, proceed with running the cleanup routine and remove the finalizer + r.Log.Info("running the finalizer routine on cniNode", "cniNode", cniNode.Name) + cleaner := &cleanup.NodeTerminationCleaner{ + NodeName: cniNode.Name, + } + cleaner.ENICleaner = &cleanup.ENICleaner{ + EC2Wrapper: r.EC2Wrapper, + Manager: cleaner, + VPCID: r.VPCID, + Log: ctrl.Log.WithName("eniCleaner").WithName("node"), + } + // Return err if failed to delete leaked ENIs on node so it can be retried + if err := cleaner.DeleteLeakedResources(); err != nil { + r.Log.Error(err, "failed to cleanup resources during node termination, request will be requeued") + return ctrl.Result{}, err + } + if err = r.FinalizerManager.RemoveFinalizers(ctx, cniNode, config.NodeTerminationFinalizer); err != nil { + r.Log.Error(err, "failed to remove finalizer on CNINode, will retry", "cniNode", cniNode.Name, "finalizer", config.NodeTerminationFinalizer) + return ctrl.Result{}, err + } + r.Log.Info("removed finalizer on cniNode", "finalizer", config.NodeTerminationFinalizer, "cniNode", cniNode.Name) + return ctrl.Result{}, nil + } else { + r.Log.Error(err, "failed to get the node object in CNINode reconciliation, will retry") + // Requeue request so it can be retried + return ctrl.Result{}, err + } + } else { + // node exists, do not run the cleanup routine(periodic cleanup routine will anyway delete leaked ENIs), remove the finalizer + // to proceed with object deletion, and recreate similar object + + // Create a copy without deletion timestamp for creation + newCNINode := &v1alpha1.CNINode{ + ObjectMeta: metav1.ObjectMeta{ + Name: cniNode.Name, + Namespace: "", + OwnerReferences: cniNode.OwnerReferences, + // TODO: should we include finalizers at object creation or let controller patch it on Create/Update event? + Finalizers: cniNode.Finalizers, + }, + Spec: cniNode.Spec, + } + + if err := r.FinalizerManager.RemoveFinalizers(ctx, cniNode, config.NodeTerminationFinalizer); err != nil { + r.Log.Error(err, "failed to remove finalizer on CNINode on node deletion, will retry") + return ctrl.Result{}, err + } + // wait till CNINode is deleted before recreation as the new object will be created with same name to avoid "object already exists" error + if err := r.waitTillCNINodeDeleted(k8s.NamespacedName(newCNINode)); err != nil { + // raise event if CNINode could not be deleted after removing the finalizer + r.K8sAPI.BroadcastEvent(cniNode, utils.CNINodeDeleteFailed, "CNINode deletion failed and object could not be recreated by the vpc-resource-controller, will retry", + v1.EventTypeWarning) + // requeue here to check if CNINode deletion is successful and retry CNINode deletion if node exists + return ctrl.Result{}, err + } + + r.Log.Info("creating CNINode after it has been deleted as node still exists", "cniNode", newCNINode.Name) + recreateCNINodeCallCount.Inc() + if err := r.createCNINodeFromObj(ctx, newCNINode); err != nil { + recreateCNINodeErrCount.Inc() + // raise event on node publish warning that CNINode is deleted and could not be recreated by controller + utils.SendNodeEventWithNodeName(r.K8sAPI, node.Name, utils.CNINodeCreateFailed, + fmt.Sprint("CNINode was deleted and failed to be recreated by the vpc-resource-controller"), v1.EventTypeWarning, r.Log) + // return nil as deleted and we cannot recreate the object now + return ctrl.Result{}, nil + } + r.Log.Info("successfully recreated CNINode", "cniNode", newCNINode.Name) + } + } + return ctrl.Result{}, nil +} + +// SetupWithManager sets up the controller with the Manager. +func (r *CNINodeReconciler) SetupWithManager(mgr ctrl.Manager) error { + if !prometheusRegistered { + prometheusRegister() + } + return ctrl.NewControllerManagedBy(mgr). + For(&v1alpha1.CNINode{}). + WithOptions(controller.Options{MaxConcurrentReconciles: config.MaxNodeConcurrentReconciles}). + Complete(r) +} + +// waitTillCNINodeDeleted waits for CNINode to be deleted with timeout and returns error +func (r *CNINodeReconciler) waitTillCNINodeDeleted(nameSpacedCNINode types.NamespacedName) error { + oldCNINode := &v1alpha1.CNINode{} + + return wait.PollUntilContextTimeout(context.TODO(), 500*time.Millisecond, time.Second*3, true, func(ctx context.Context) (bool, error) { + if err := r.Client.Get(ctx, nameSpacedCNINode, oldCNINode); err != nil && errors.IsNotFound(err) { + return true, nil + } + return false, nil + }) +} + +// createCNINodeFromObj will create CNINode with backoff and returns error if CNINode is not recreated +func (r *CNINodeReconciler) createCNINodeFromObj(ctx context.Context, newCNINode client.Object) error { + return retry.OnError(retry.DefaultBackoff, func(error) bool { return true }, + func() error { + return r.Client.Create(ctx, newCNINode) + }) +} diff --git a/controllers/crds/suite_test.go b/controllers/crds/suite_test.go new file mode 100644 index 00000000..d0d49241 --- /dev/null +++ b/controllers/crds/suite_test.go @@ -0,0 +1,79 @@ +// Copyright Amazon.com Inc. or its affiliates. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"). You may +// not use this file except in compliance with the License. A copy of the +// License is located at +// +// http://aws.amazon.com/apache2.0/ +// +// or in the "license" file accompanying this file. This file is distributed +// on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either +// express or implied. See the License for the specific language governing +// permissions and limitations under the License. + +package crds + +import ( + "path/filepath" + "testing" + + "github.com/aws/amazon-vpc-resource-controller-k8s/apis/vpcresources/v1alpha1" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + "k8s.io/client-go/kubernetes/scheme" + "k8s.io/client-go/rest" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/envtest" + logf "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/log/zap" + //+kubebuilder:scaffold:imports +) + +// These tests use Ginkgo (BDD-style Go testing framework). Refer to +// http://onsi.github.io/ginkgo/ to learn more about Ginkgo. + +var cfg *rest.Config +var k8sClient client.Client +var testEnv *envtest.Environment + +func TestAPIs(t *testing.T) { + RegisterFailHandler(Fail) + + RunSpecs(t, "Controller Suite") +} + +var _ = BeforeSuite(func() { + done := make(chan interface{}) + go func() { + logf.SetLogger(zap.New(zap.WriteTo(GinkgoWriter), zap.UseDevMode(true))) + + By("bootstrapping test environment") + testEnv = &envtest.Environment{ + CRDDirectoryPaths: []string{filepath.Join("..", "..", "config", "crd", "bases")}, + ErrorIfCRDPathMissing: false, + } + + var err error + // cfg is defined in this file globally. + cfg, err = testEnv.Start() + Expect(err).NotTo(HaveOccurred()) + Expect(cfg).NotTo(BeNil()) + + err = v1alpha1.AddToScheme(scheme.Scheme) + Expect(err).NotTo(HaveOccurred()) + //+kubebuilder:scaffold:scheme + + k8sClient, err = client.New(cfg, client.Options{Scheme: scheme.Scheme}) + Expect(err).NotTo(HaveOccurred()) + Expect(k8sClient).NotTo(BeNil()) + }() + Eventually(done, 60).Should(BeClosed()) + +}) + +var _ = AfterSuite(func() { + By("tearing down the test environment") + err := testEnv.Stop() + Expect(err).NotTo(HaveOccurred()) +}) diff --git a/main.go b/main.go index 5b867f29..7426bef8 100644 --- a/main.go +++ b/main.go @@ -26,8 +26,10 @@ import ( vpcresourcesv1beta1 "github.com/aws/amazon-vpc-resource-controller-k8s/apis/vpcresources/v1beta1" "github.com/aws/amazon-vpc-resource-controller-k8s/controllers/apps" corecontroller "github.com/aws/amazon-vpc-resource-controller-k8s/controllers/core" + crdcontroller "github.com/aws/amazon-vpc-resource-controller-k8s/controllers/crds" "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/api" ec2API "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/aws/ec2/api" + eniCleaner "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/aws/ec2/api/cleanup" "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/condition" "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/config" rcHealthz "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/healthz" @@ -107,6 +109,7 @@ func main() { var healthCheckTimeout int var enableWindowsPrefixDelegation bool var region string + var vpcID string flag.StringVar(&metricsAddr, "metrics-bind-address", ":8080", "The address the metric endpoint binds to.") @@ -141,6 +144,7 @@ func main() { flag.BoolVar(&enableWindowsPrefixDelegation, "enable-windows-prefix-delegation", false, "Enable the feature flag for Windows prefix delegation") flag.StringVar(®ion, "aws-region", "", "The aws region of the k8s cluster") + flag.StringVar(&vpcID, "vpc-id", "", "The VPC ID where EKS cluster is deployed") flag.Parse() @@ -183,6 +187,11 @@ func main() { os.Exit(1) } + if vpcID == "" { + setupLog.Error(fmt.Errorf("vpc-id is a required parameter"), "unable to start the controller") + os.Exit(1) + } + // Profiler disabled by default, to enable set the enableProfiling argument if enableProfiling { // To use the profiler - https://golang.org/pkg/net/http/pprof/ @@ -311,7 +320,7 @@ func main() { nodeManagerWorkers := asyncWorkers.NewDefaultWorkerPool("node async workers", 10, 1, ctrl.Log.WithName("node async workers"), ctx) nodeManager, err := manager.NewNodeManager(ctrl.Log.WithName("node manager"), resourceManager, - apiWrapper, nodeManagerWorkers, controllerConditions, version.GitVersion, healthzHandler) + apiWrapper, nodeManagerWorkers, controllerConditions, clusterName, version.GitVersion, healthzHandler) if err != nil { ctrl.Log.Error(err, "failed to init node manager") @@ -332,11 +341,17 @@ func main() { os.Exit(1) } - if err := (&ec2API.ENICleaner{ - EC2Wrapper: ec2Wrapper, + cleaner := &eniCleaner.ClusterENICleaner{ ClusterName: clusterName, - Log: ctrl.Log.WithName("eni cleaner"), - }).SetupWithManager(ctx, mgr, healthzHandler); err != nil { + } + cleaner.ENICleaner = &eniCleaner.ENICleaner{ + EC2Wrapper: ec2Wrapper, + Manager: cleaner, + VPCID: vpcID, + Log: ctrl.Log.WithName("eniCleaner").WithName("cluster"), + } + + if err := cleaner.SetupWithManager(ctx, mgr, healthzHandler); err != nil { setupLog.Error(err, "unable to start eni cleaner") os.Exit(1) } @@ -386,6 +401,21 @@ func main() { os.Exit(1) } + finalizerManager := k8s.NewDefaultFinalizerManager(mgr.GetClient(), ctrl.Log.WithName("finalizer manager")) + if err = (&crdcontroller.CNINodeReconciler{ + Client: mgr.GetClient(), + Scheme: mgr.GetScheme(), + Context: ctx, + Log: ctrl.Log.WithName("controllers").WithName("CNINode"), + EC2Wrapper: ec2Wrapper, + K8sAPI: k8sApi, + ClusterName: clusterName, + VPCID: vpcID, + FinalizerManager: finalizerManager, + }).SetupWithManager(mgr); err != nil { + setupLog.Error(err, "unable to create controller", "controller", "CNINode") + os.Exit(1) + } // +kubebuilder:scaffold:builder setupLog.Info("setting up webhook server") webhookServer := mgr.GetWebhookServer() 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 fe0b7d5e..6bd94eb3 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 @@ -210,18 +210,18 @@ func (mr *MockEC2APIHelperMockRecorder) DisassociateTrunkInterface(arg0 interfac } // GetBranchNetworkInterface mocks base method. -func (m *MockEC2APIHelper) GetBranchNetworkInterface(arg0 *string) ([]*ec2.NetworkInterface, error) { +func (m *MockEC2APIHelper) GetBranchNetworkInterface(arg0, arg1 *string) ([]*ec2.NetworkInterface, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GetBranchNetworkInterface", arg0) + ret := m.ctrl.Call(m, "GetBranchNetworkInterface", arg0, arg1) ret0, _ := ret[0].([]*ec2.NetworkInterface) ret1, _ := ret[1].(error) return ret0, ret1 } // GetBranchNetworkInterface indicates an expected call of GetBranchNetworkInterface. -func (mr *MockEC2APIHelperMockRecorder) GetBranchNetworkInterface(arg0 interface{}) *gomock.Call { +func (mr *MockEC2APIHelperMockRecorder) GetBranchNetworkInterface(arg0, arg1 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetBranchNetworkInterface", reflect.TypeOf((*MockEC2APIHelper)(nil).GetBranchNetworkInterface), arg0) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetBranchNetworkInterface", reflect.TypeOf((*MockEC2APIHelper)(nil).GetBranchNetworkInterface), arg0, arg1) } // GetInstanceDetails mocks base method. 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 4553389e..53515c5d 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 @@ -182,6 +182,21 @@ func (mr *MockEC2WrapperMockRecorder) DescribeNetworkInterfaces(arg0 interface{} return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DescribeNetworkInterfaces", reflect.TypeOf((*MockEC2Wrapper)(nil).DescribeNetworkInterfaces), arg0) } +// DescribeNetworkInterfacesPages mocks base method. +func (m *MockEC2Wrapper) DescribeNetworkInterfacesPages(arg0 *ec2.DescribeNetworkInterfacesInput) ([]*ec2.NetworkInterface, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "DescribeNetworkInterfacesPages", arg0) + ret0, _ := ret[0].([]*ec2.NetworkInterface) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// DescribeNetworkInterfacesPages indicates an expected call of DescribeNetworkInterfacesPages. +func (mr *MockEC2WrapperMockRecorder) DescribeNetworkInterfacesPages(arg0 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DescribeNetworkInterfacesPages", reflect.TypeOf((*MockEC2Wrapper)(nil).DescribeNetworkInterfacesPages), arg0) +} + // DescribeSubnets mocks base method. func (m *MockEC2Wrapper) DescribeSubnets(arg0 *ec2.DescribeSubnetsInput) (*ec2.DescribeSubnetsOutput, error) { m.ctrl.T.Helper() diff --git a/mocks/amazon-vcp-resource-controller-k8s/pkg/k8s/mock_k8swrapper.go b/mocks/amazon-vcp-resource-controller-k8s/pkg/k8s/mock_k8swrapper.go index 9b376ede..93e151d5 100644 --- a/mocks/amazon-vcp-resource-controller-k8s/pkg/k8s/mock_k8swrapper.go +++ b/mocks/amazon-vcp-resource-controller-k8s/pkg/k8s/mock_k8swrapper.go @@ -96,17 +96,17 @@ func (mr *MockK8sWrapperMockRecorder) BroadcastEvent(arg0, arg1, arg2, arg3 inte } // CreateCNINode mocks base method. -func (m *MockK8sWrapper) CreateCNINode(arg0 *v10.Node) error { +func (m *MockK8sWrapper) CreateCNINode(arg0 *v10.Node, arg1 string) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "CreateCNINode", arg0) + ret := m.ctrl.Call(m, "CreateCNINode", arg0, arg1) ret0, _ := ret[0].(error) return ret0 } // CreateCNINode indicates an expected call of CreateCNINode. -func (mr *MockK8sWrapperMockRecorder) CreateCNINode(arg0 interface{}) *gomock.Call { +func (mr *MockK8sWrapperMockRecorder) CreateCNINode(arg0, arg1 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CreateCNINode", reflect.TypeOf((*MockK8sWrapper)(nil).CreateCNINode), arg0) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CreateCNINode", reflect.TypeOf((*MockK8sWrapper)(nil).CreateCNINode), arg0, arg1) } // GetCNINode mocks base method. diff --git a/mocks/amazon-vcp-resource-controller-k8s/pkg/provider/branch/trunk/mock_trunk.go b/mocks/amazon-vcp-resource-controller-k8s/pkg/provider/branch/trunk/mock_trunk.go index ac4b1c73..8120b4f9 100644 --- a/mocks/amazon-vcp-resource-controller-k8s/pkg/provider/branch/trunk/mock_trunk.go +++ b/mocks/amazon-vcp-resource-controller-k8s/pkg/provider/branch/trunk/mock_trunk.go @@ -64,18 +64,6 @@ func (mr *MockTrunkENIMockRecorder) CreateAndAssociateBranchENIs(arg0, arg1, arg return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CreateAndAssociateBranchENIs", reflect.TypeOf((*MockTrunkENI)(nil).CreateAndAssociateBranchENIs), arg0, arg1, arg2) } -// DeleteAllBranchENIs mocks base method. -func (m *MockTrunkENI) DeleteAllBranchENIs() { - m.ctrl.T.Helper() - m.ctrl.Call(m, "DeleteAllBranchENIs") -} - -// DeleteAllBranchENIs indicates an expected call of DeleteAllBranchENIs. -func (mr *MockTrunkENIMockRecorder) DeleteAllBranchENIs() *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteAllBranchENIs", reflect.TypeOf((*MockTrunkENI)(nil).DeleteAllBranchENIs)) -} - // DeleteCooledDownENIs mocks base method. func (m *MockTrunkENI) DeleteCooledDownENIs() { m.ctrl.T.Helper() @@ -88,6 +76,18 @@ func (mr *MockTrunkENIMockRecorder) DeleteCooledDownENIs() *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteCooledDownENIs", reflect.TypeOf((*MockTrunkENI)(nil).DeleteCooledDownENIs)) } +// DisassociateAllBranchENIs mocks base method. +func (m *MockTrunkENI) DisassociateAllBranchENIs() { + m.ctrl.T.Helper() + m.ctrl.Call(m, "DisassociateAllBranchENIs") +} + +// DisassociateAllBranchENIs indicates an expected call of DisassociateAllBranchENIs. +func (mr *MockTrunkENIMockRecorder) DisassociateAllBranchENIs() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DisassociateAllBranchENIs", reflect.TypeOf((*MockTrunkENI)(nil).DisassociateAllBranchENIs)) +} + // InitTrunk mocks base method. func (m *MockTrunkENI) InitTrunk(arg0 ec2.EC2Instance, arg1 []v1.Pod) error { m.ctrl.T.Helper() diff --git a/pkg/aws/ec2/api/cleanup/eni_cleanup.go b/pkg/aws/ec2/api/cleanup/eni_cleanup.go new file mode 100644 index 00000000..5f8def7b --- /dev/null +++ b/pkg/aws/ec2/api/cleanup/eni_cleanup.go @@ -0,0 +1,213 @@ +// Copyright Amazon.com Inc. or its affiliates. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"). You may +// not use this file except in compliance with the License. A copy of the +// License is located at +// +// http://aws.amazon.com/apache2.0/ +// +// or in the "license" file accompanying this file. This file is distributed +// on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either +// express or implied. See the License for the specific language governing +// permissions and limitations under the License. + +package cleanup + +import ( + "context" + "fmt" + "time" + + "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/aws/ec2/api" + "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/config" + rcHealthz "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/healthz" + "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/utils" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/ec2" + "github.com/go-logr/logr" + kerrors "k8s.io/apimachinery/pkg/util/errors" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/healthz" +) + +// NetworkInterfaceManager interface allows to define the ENI filters and checks if ENI should be deleted for different callers like in the periodic cleanup routine or +// during node termination +type NetworkInterfaceManager interface { + GetENITagFilters() []*ec2.Filter + ShouldDeleteENI(eniID *string) bool + UpdateAvailableENIsIfNeeded(eniMap *map[string]struct{}) + UpdateCleanupMetrics(vpcrcAvailableCount int, vpccniAvailableCount int, leakedENICount int) +} + +type ENICleaner struct { + EC2Wrapper api.EC2Wrapper + Manager NetworkInterfaceManager + VPCID string + Log logr.Logger +} + +// common filters for describing network interfaces +var CommonNetworkInterfaceFilters = []*ec2.Filter{ + { + Name: aws.String("status"), + Values: []*string{aws.String(ec2.NetworkInterfaceStatusAvailable)}, + }, + { + Name: aws.String("tag:" + config.NetworkInterfaceOwnerTagKey), + Values: aws.StringSlice([]string{config.NetworkInterfaceOwnerTagValue, + config.NetworkInterfaceOwnerVPCCNITagValue}), + }, +} + +// ClusterENICleaner periodically deletes leaked network interfaces(provisioned by the controller or VPC-CNI) in the cluster +type ClusterENICleaner struct { + ClusterName string + shutdown bool + ctx context.Context + availableENIs map[string]struct{} + *ENICleaner +} + +func (e *ClusterENICleaner) SetupWithManager(ctx context.Context, mgr ctrl.Manager, healthzHandler *rcHealthz.HealthzHandler) error { + e.ctx = ctx + e.availableENIs = make(map[string]struct{}) + healthzHandler.AddControllersHealthCheckers( + map[string]healthz.Checker{ + "health-interface-cleaner": rcHealthz.SimplePing("interface cleanup", e.Log), + }, + ) + + return mgr.Add(e) +} + +// StartENICleaner starts the ENI Cleaner routine that cleans up dangling ENIs created by the controller +func (e *ClusterENICleaner) Start(ctx context.Context) error { + e.Log.Info("starting eni clean up routine") + + // Start routine to listen for shut down signal, on receiving the signal it set shutdown to true + go func() { + <-ctx.Done() + e.shutdown = true + }() + // Perform ENI cleanup after fixed time intervals till shut down variable is set to true on receiving the shutdown + // signal + for !e.shutdown { + e.DeleteLeakedResources() + time.Sleep(config.ENICleanUpInterval) + } + + return nil +} + +// DeleteLeakedResources describes all the network interfaces in available status that are created by the controller or VPC-CNI +// This is called by periodically by ClusterENICleaner which deletes available ENIs cluster-wide, and by the NodeTermination cleaner on node termination +// The available ENIs are deleted if ShouldDeleteENI is true, defined in the respective cleaners +// The function also updates metrics for the periodic cleanup routine and the node termination cleanup +func (e *ENICleaner) DeleteLeakedResources() error { + var errors []error + availableENIs := make(map[string]struct{}) + vpcrcAvailableCount := 0 + vpccniAvailableCount := 0 + leakedENICount := 0 + + filters := CommonNetworkInterfaceFilters + // Append the VPC-ID deep filter for the paginated call + filters = append(filters, []*ec2.Filter{ + { + Name: aws.String("vpc-id"), + Values: []*string{aws.String(e.VPCID)}, + }, + }...) + // get cleaner specific filters + filters = append(filters, e.Manager.GetENITagFilters()...) + describeNetworkInterfaceIp := &ec2.DescribeNetworkInterfacesInput{ + Filters: filters, + } + networkInterfaces, err := e.EC2Wrapper.DescribeNetworkInterfacesPages(describeNetworkInterfaceIp) + if err != nil { + e.Log.Error(err, "failed to describe network interfaces, cleanup will be retried in next cycle") + return err + } + + for _, networkInterface := range networkInterfaces { + if e.Manager.ShouldDeleteENI(networkInterface.NetworkInterfaceId) { + tagMap := utils.GetTagKeyValueMap(networkInterface.TagSet) + if val, ok := tagMap[config.NetworkInterfaceOwnerTagKey]; ok { + // Increment promethues metrics for number of leaked ENIs cleaned up + switch val { + case config.NetworkInterfaceOwnerTagValue: + vpcrcAvailableCount += 1 + case config.NetworkInterfaceOwnerVPCCNITagValue: + vpccniAvailableCount += 1 + default: + // We should not hit this case as we only filter for relevant tag values, log error and continue if unexpected ENIs found + e.Log.Error(fmt.Errorf("found available ENI not created by VPC-CNI/VPC-RC"), "eniID", *networkInterface.NetworkInterfaceId) + continue + } + } + _, err := e.EC2Wrapper.DeleteNetworkInterface(&ec2.DeleteNetworkInterfaceInput{ + NetworkInterfaceId: networkInterface.NetworkInterfaceId, + }) + if err != nil { + // append err and continue, we will retry deletion in the next period/reconcile + leakedENICount += 1 + errors = append(errors, fmt.Errorf("failed to delete leaked network interface %v:%v", *networkInterface.NetworkInterfaceId, err)) + e.Log.Error(err, "failed to delete the leaked network interface", + "id", *networkInterface.NetworkInterfaceId) + continue + } + e.Log.Info("deleted leaked ENI successfully", "eni id", networkInterface.NetworkInterfaceId) + } else { + // Seeing the ENI for the first time, add it to the new list of available network interfaces + availableENIs[*networkInterface.NetworkInterfaceId] = struct{}{} + e.Log.V(1).Info("adding eni to to the map of available ENIs, will be removed if present in "+ + "next run too", "id", *networkInterface.NetworkInterfaceId) + } + + } + + e.Manager.UpdateCleanupMetrics(vpcrcAvailableCount, vpccniAvailableCount, leakedENICount) + e.Manager.UpdateAvailableENIsIfNeeded(&availableENIs) + return kerrors.NewAggregate(errors) +} + +func (e *ClusterENICleaner) GetENITagFilters() []*ec2.Filter { + clusterNameTagKey := fmt.Sprintf(config.ClusterNameTagKeyFormat, e.ClusterName) + return []*ec2.Filter{ + { + Name: aws.String("tag:" + clusterNameTagKey), + Values: []*string{aws.String(config.ClusterNameTagValue)}, + }, + } +} + +// ShouldDeleteENI returns true if the ENI should be deleted. +func (e *ClusterENICleaner) ShouldDeleteENI(eniID *string) bool { + if _, exists := e.availableENIs[*eniID]; exists { + return true + } + return false +} + +// Set the available ENIs to the list of ENIs seen in the current cycle +// This adds ENIs that should not be deleted in the current cleanup cycle to the internal cache so it can be deleted in next cycle +// This prevents the clean up routine to remove ENIs that are created by another routines and are yet not attached to +// an instance or associated with a trunk interface in the periodic cleanup routine + +// Example +// 1st cycle, Describe Available NetworkInterface Result - Interface 1, Interface 2, Interface 3 +// 2nd cycle, Describe Available NetworkInterface Result - Interface 2, Interface 3 +// In the second cycle we can conclude that Interface 2 and 3 are leaked because they have been sitting for the time +// interval between cycle 1 and 2 and hence can be safely deleted. And we can also conclude that Interface 1 was +// created but not attached at the the time when 1st cycle ran and hence it should not be deleted. +func (e *ClusterENICleaner) UpdateAvailableENIsIfNeeded(eniMap *map[string]struct{}) { + e.availableENIs = *eniMap +} + +// Update cluster cleanup metrics for the current cleanup cycle +func (e *ClusterENICleaner) UpdateCleanupMetrics(vpcrcAvailableCount int, vpccniAvailableCount int, leakedENICount int) { + api.VpcRcAvailableClusterENICnt.Set(float64(vpcrcAvailableCount)) + api.VpcCniAvailableClusterENICnt.Set(float64(vpccniAvailableCount)) + api.LeakedENIClusterCleanupCnt.Set(float64(leakedENICount)) +} diff --git a/pkg/aws/ec2/api/cleanup/eni_cleanup_test.go b/pkg/aws/ec2/api/cleanup/eni_cleanup_test.go new file mode 100644 index 00000000..a19c7d90 --- /dev/null +++ b/pkg/aws/ec2/api/cleanup/eni_cleanup_test.go @@ -0,0 +1,241 @@ +// Copyright Amazon.com Inc. or its affiliates. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"). You may +// not use this file except in compliance with the License. A copy of the +// License is located at +// +// http://aws.amazon.com/apache2.0/ +// +// or in the "license" file accompanying this file. This file is distributed +// on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either +// express or implied. See the License for the specific language governing +// permissions and limitations under the License. + +package cleanup + +import ( + "context" + "fmt" + "reflect" + "testing" + + mock_api "github.com/aws/amazon-vpc-resource-controller-k8s/mocks/amazon-vcp-resource-controller-k8s/pkg/aws/ec2/api" + "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/config" + "sigs.k8s.io/controller-runtime/pkg/log/zap" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/ec2" + "github.com/golang/mock/gomock" + "github.com/stretchr/testify/assert" +) + +var ( + mockClusterName = "cluster-name" + mockNodeName = "node-name" + mockClusterNameTagKey = fmt.Sprintf(config.ClusterNameTagKeyFormat, mockClusterName) + + mockNetworkInterfaceId1 = "eni-000000000000000" + mockNetworkInterfaceId2 = "eni-000000000000001" + mockNetworkInterfaceId3 = "eni-000000000000002" + + mockVPCID = "vpc-0000000000000000" + + mockClusterTagInput = &ec2.DescribeNetworkInterfacesInput{ + Filters: []*ec2.Filter{ + { + Name: aws.String("status"), + Values: []*string{aws.String(ec2.NetworkInterfaceStatusAvailable)}, + }, + { + Name: aws.String("tag:" + config.NetworkInterfaceOwnerTagKey), + Values: aws.StringSlice([]string{config.NetworkInterfaceOwnerTagValue, + config.NetworkInterfaceOwnerVPCCNITagValue}), + }, + { + Name: aws.String("vpc-id"), + Values: []*string{aws.String(mockVPCID)}, + }, + { + Name: aws.String("tag:" + mockClusterNameTagKey), + Values: []*string{aws.String(config.ClusterNameTagValue)}, + }, + }, + } + + mockNodenameTagInput = &ec2.DescribeNetworkInterfacesInput{ + Filters: []*ec2.Filter{ + { + Name: aws.String("status"), + Values: []*string{aws.String(ec2.NetworkInterfaceStatusAvailable)}, + }, + { + Name: aws.String("tag:" + config.NetworkInterfaceOwnerTagKey), + Values: aws.StringSlice([]string{config.NetworkInterfaceOwnerTagValue, + config.NetworkInterfaceOwnerVPCCNITagValue}), + }, + { + Name: aws.String("vpc-id"), + Values: []*string{aws.String(mockVPCID)}, + }, + { + Name: aws.String("tag:" + config.NetworkInterfaceNodenameKey), + Values: []*string{aws.String(mockNodeName)}, + }, + }, + } + + mockDescribeInterfaceOpWith1And2 = []*ec2.NetworkInterface{ + {NetworkInterfaceId: &mockNetworkInterfaceId1}, + {NetworkInterfaceId: &mockNetworkInterfaceId2}, + } + mockDescribeInterfaceOpWith1And3 = []*ec2.NetworkInterface{ + {NetworkInterfaceId: &mockNetworkInterfaceId1}, + {NetworkInterfaceId: &mockNetworkInterfaceId3}, + } +) + +func getMockClusterENICleaner(ctrl *gomock.Controller) (*ClusterENICleaner, *mock_api.MockEC2Wrapper) { + mockEC2Wrapper := mock_api.NewMockEC2Wrapper(ctrl) + mockclusterENICleaner := ClusterENICleaner{ + availableENIs: map[string]struct{}{}, + ctx: context.Background(), + ClusterName: mockClusterName, + } + mockclusterENICleaner.ENICleaner = &ENICleaner{ + EC2Wrapper: mockEC2Wrapper, + Manager: &mockclusterENICleaner, + Log: zap.New(zap.UseDevMode(true)).WithName("cluster eni cleaner test"), + VPCID: mockVPCID, + } + return &mockclusterENICleaner, mockEC2Wrapper +} + +func TestENICleaner_DeleteLeakedResources(t *testing.T) { + type fields struct { + mockEC2Wrapper *mock_api.MockEC2Wrapper + clusterENICleaner *ClusterENICleaner + } + testENICleaner_DeleteLeakedResources := []struct { + name string + getENICleaner func() (*ENICleaner, *ClusterENICleaner) + prepare func(f *fields) + assertFirstCall func(f *fields) + assertSecondCall func(f *fields) + }{ + { + name: "ClusterENICleaner, verifies leaked ENIs are deleted in the periodic cleanup routine and availableENI is updated", + getENICleaner: func() (*ENICleaner, *ClusterENICleaner) { + mockClusterENICleaner := &ClusterENICleaner{ + ClusterName: mockClusterName, + ctx: context.Background(), + availableENIs: map[string]struct{}{}, + } + mockClusterENICleaner.ENICleaner = &ENICleaner{ + Manager: mockClusterENICleaner, + VPCID: mockVPCID, + Log: zap.New(zap.UseDevMode(true)).WithName("cluster eni cleaner test"), + } + return mockClusterENICleaner.ENICleaner, mockClusterENICleaner + }, + prepare: func(f *fields) { + gomock.InOrder( + // Return network interface 1 and 2 in first cycle + f.mockEC2Wrapper.EXPECT().DescribeNetworkInterfacesPages(mockClusterTagInput). + Return(mockDescribeInterfaceOpWith1And2, nil), + // Return network interface 1 and 3 in the second cycle + f.mockEC2Wrapper.EXPECT().DescribeNetworkInterfacesPages(mockClusterTagInput). + Return(mockDescribeInterfaceOpWith1And3, nil), + // Expect to delete the network interface 1 + f.mockEC2Wrapper.EXPECT().DeleteNetworkInterface( + &ec2.DeleteNetworkInterfaceInput{NetworkInterfaceId: &mockNetworkInterfaceId1}).Return(nil, nil), + ) + + }, + assertFirstCall: func(f *fields) { + // After first call, network interface 1 and 2 should be added to the map of available ENIs + assert.True(t, reflect.DeepEqual( + map[string]struct{}{mockNetworkInterfaceId1: {}, mockNetworkInterfaceId2: {}}, f.clusterENICleaner.availableENIs)) + + }, + assertSecondCall: func(f *fields) { + // After second call, network interface 1 should be deleted and network interface 3 added to list + assert.True(t, reflect.DeepEqual( + map[string]struct{}{mockNetworkInterfaceId3: {}}, f.clusterENICleaner.availableENIs)) + }, + }, + { + name: "NodeTerminationENICleaner, verifies ENIs are deleted on node termination", + getENICleaner: func() (*ENICleaner, *ClusterENICleaner) { + mocknodeCleaner := &NodeTerminationCleaner{ + NodeName: mockNodeName, + } + mocknodeCleaner.ENICleaner = &ENICleaner{ + Manager: mocknodeCleaner, + VPCID: mockVPCID, + Log: zap.New(zap.UseDevMode(true)).WithName("cluster eni cleaner test"), + } + return mocknodeCleaner.ENICleaner, nil + }, + prepare: func(f *fields) { + gomock.InOrder( + + // Return network interface 1 and 2 in first cycle, expect to call delete on both + f.mockEC2Wrapper.EXPECT().DescribeNetworkInterfacesPages(mockNodenameTagInput). + Return(mockDescribeInterfaceOpWith1And2, nil), + f.mockEC2Wrapper.EXPECT().DeleteNetworkInterface( + &ec2.DeleteNetworkInterfaceInput{NetworkInterfaceId: &mockNetworkInterfaceId1}).Return(nil, nil), + f.mockEC2Wrapper.EXPECT().DeleteNetworkInterface( + &ec2.DeleteNetworkInterfaceInput{NetworkInterfaceId: &mockNetworkInterfaceId2}).Return(nil, nil), + // Return network interface 1 and 3 in the second cycle, again expect to call delete on both + f.mockEC2Wrapper.EXPECT().DescribeNetworkInterfacesPages(mockNodenameTagInput). + Return(mockDescribeInterfaceOpWith1And3, nil), + f.mockEC2Wrapper.EXPECT().DeleteNetworkInterface( + &ec2.DeleteNetworkInterfaceInput{NetworkInterfaceId: &mockNetworkInterfaceId1}).Return(nil, nil), + f.mockEC2Wrapper.EXPECT().DeleteNetworkInterface( + &ec2.DeleteNetworkInterfaceInput{NetworkInterfaceId: &mockNetworkInterfaceId3}).Return(nil, nil), + ) + }, + }, + } + + for _, tt := range testENICleaner_DeleteLeakedResources { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + mockEC2Wrapper := mock_api.NewMockEC2Wrapper(ctrl) + var mockENICleaner *ENICleaner + var mockClusterENICleaner *ClusterENICleaner + if tt.getENICleaner != nil { + mockENICleaner, mockClusterENICleaner = tt.getENICleaner() + } + mockENICleaner.EC2Wrapper = mockEC2Wrapper + f := fields{ + mockEC2Wrapper: mockEC2Wrapper, + clusterENICleaner: mockClusterENICleaner, + } + if tt.prepare != nil { + tt.prepare(&f) + } + + err := mockENICleaner.DeleteLeakedResources() + assert.NoError(t, err) + if tt.assertFirstCall != nil { + tt.assertFirstCall(&f) + } + + err = mockENICleaner.DeleteLeakedResources() + assert.NoError(t, err) + if tt.assertSecondCall != nil { + tt.assertSecondCall(&f) + } + } +} + +// TestENICleaner_StartENICleaner_Shutdown tests that ENICleaner would not start if shutdown is set to true. +func TestENICleaner_StartENICleaner_Shutdown(t *testing.T) { + ctrl := gomock.NewController(t) + eniCleaner, _ := getMockClusterENICleaner(ctrl) + + eniCleaner.shutdown = true + + eniCleaner.Start(context.TODO()) +} diff --git a/pkg/aws/ec2/api/cleanup/node_cleanup.go b/pkg/aws/ec2/api/cleanup/node_cleanup.go new file mode 100644 index 00000000..12c7c0ac --- /dev/null +++ b/pkg/aws/ec2/api/cleanup/node_cleanup.go @@ -0,0 +1,50 @@ +// Copyright Amazon.com Inc. or its affiliates. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"). You may +// not use this file except in compliance with the License. A copy of the +// License is located at +// +// http://aws.amazon.com/apache2.0/ +// +// or in the "license" file accompanying this file. This file is distributed +// on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either +// express or implied. See the License for the specific language governing +// permissions and limitations under the License. + +package cleanup + +import ( + "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/config" + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/ec2" +) + +// NodeTerminationCleanerto handle resource cleanup at node termination +type NodeTerminationCleaner struct { + NodeName string + *ENICleaner +} + +func (n *NodeTerminationCleaner) GetENITagFilters() []*ec2.Filter { + return []*ec2.Filter{ + { + Name: aws.String("tag:" + config.NetworkInterfaceNodenameKey), + Values: []*string{aws.String(n.NodeName)}, + }, + } +} + +// Return true. As the node is terminating all available ENIs need to be deleted +func (n *NodeTerminationCleaner) ShouldDeleteENI(eniID *string) bool { + return true +} + +func (n *NodeTerminationCleaner) UpdateAvailableENIsIfNeeded(eniMap *map[string]struct{}) { + // Nothing to do for the node termination cleaner + return +} + +// Updating node termination metrics does not make much sense as it will be updated on each node deletion and does not give us much info +func (n *NodeTerminationCleaner) UpdateCleanupMetrics(vpcrcAvailableCount int, vpccniAvailableCount int, leakedENICount int) { + return +} diff --git a/pkg/aws/ec2/api/cleanup/resource_cleaner.go b/pkg/aws/ec2/api/cleanup/resource_cleaner.go new file mode 100644 index 00000000..d5c7e6d1 --- /dev/null +++ b/pkg/aws/ec2/api/cleanup/resource_cleaner.go @@ -0,0 +1,19 @@ +// Copyright Amazon.com Inc. or its affiliates. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"). You may +// not use this file except in compliance with the License. A copy of the +// License is located at +// +// http://aws.amazon.com/apache2.0/ +// +// or in the "license" file accompanying this file. This file is distributed +// on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either +// express or implied. See the License for the specific language governing +// permissions and limitations under the License. + +package cleanup + +// ResourceCleaner interface should be implemented by components that need to delete leaked AWS resources +type ResourceCleaner interface { + DeleteLeakedResources() error +} diff --git a/pkg/aws/ec2/api/eni_cleanup.go b/pkg/aws/ec2/api/eni_cleanup.go deleted file mode 100644 index 6f3db155..00000000 --- a/pkg/aws/ec2/api/eni_cleanup.go +++ /dev/null @@ -1,178 +0,0 @@ -// Copyright Amazon.com Inc. or its affiliates. All Rights Reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"). You may -// not use this file except in compliance with the License. A copy of the -// License is located at -// -// http://aws.amazon.com/apache2.0/ -// -// or in the "license" file accompanying this file. This file is distributed -// on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either -// express or implied. See the License for the specific language governing -// permissions and limitations under the License. - -package api - -import ( - "context" - "fmt" - "time" - - "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/config" - rcHealthz "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/healthz" - "github.com/prometheus/client_golang/prometheus" - "golang.org/x/exp/slices" - - "github.com/aws/aws-sdk-go/aws" - "github.com/aws/aws-sdk-go/service/ec2" - "github.com/go-logr/logr" - ctrl "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/healthz" -) - -type ENICleaner struct { - EC2Wrapper EC2Wrapper - ClusterName string - Log logr.Logger - - availableENIs map[string]struct{} - shutdown bool - clusterNameTagKey string - ctx context.Context -} - -var ( - vpcCniLeakedENICleanupCnt = prometheus.NewCounter( - prometheus.CounterOpts{ - Name: "vpc_cni_created_leaked_eni_cleanup_count", - Help: "The number of leaked ENIs created by VPC-CNI that is cleaned up by the controller", - }, - ) - vpcrcLeakedENICleanupCnt = prometheus.NewCounter( - prometheus.CounterOpts{ - Name: "vpc_rc_created_leaked_eni_cleanup_count", - Help: "The number of leaked ENIs created by VPC-RC that is cleaned up by the controller", - }, - ) -) - -func (e *ENICleaner) SetupWithManager(ctx context.Context, mgr ctrl.Manager, healthzHandler *rcHealthz.HealthzHandler) error { - e.clusterNameTagKey = fmt.Sprintf(config.ClusterNameTagKeyFormat, e.ClusterName) - e.availableENIs = make(map[string]struct{}) - e.ctx = ctx - - healthzHandler.AddControllersHealthCheckers( - map[string]healthz.Checker{ - "health-interface-cleaner": rcHealthz.SimplePing("interface cleanup", e.Log), - }, - ) - - return mgr.Add(e) -} - -// StartENICleaner starts the ENI Cleaner routine that cleans up dangling ENIs created by the controller -func (e *ENICleaner) Start(ctx context.Context) error { - e.Log.Info("starting eni clean up routine") - // Start routine to listen for shut down signal, on receiving the signal it set shutdown to true - go func() { - <-ctx.Done() - e.shutdown = true - }() - // Perform ENI cleanup after fixed time intervals till shut down variable is set to true on receiving the shutdown - // signal - for !e.shutdown { - e.cleanUpAvailableENIs() - time.Sleep(config.ENICleanUpInterval) - } - - return nil -} - -// cleanUpAvailableENIs describes all the network interfaces in available status that are created by the controller, -// on seeing the a network interface for the first time, it is added to the map of available network interfaces, on -// seeing the network interface for the second time the network interface is deleted. This ensures that we are deleting -// the network interfaces that have been in available for upto the time interval between running the clean up routine. -// This prevents the clean up routine to remove ENIs that are created by another routines and are yet not attached to -// an instance or associated with a trunk interface -// Example, -// 1st cycle, Describe Available NetworkInterface Result - Interface 1, Interface 2, Interface 3 -// 2nd cycle, Describe Available NetworkInterface Result - Interface 2, Interface 3 -// In the second cycle we can conclude that Interface 2 and 3 are leaked because they have been sitting for the time -// interval between cycle 1 and 2 and hence can be safely deleted. And we can also conclude that Interface 1 was -// created but not attached at the the time when 1st cycle ran and hence it should not be deleted. -func (e *ENICleaner) cleanUpAvailableENIs() { - describeNetworkInterfaceIp := &ec2.DescribeNetworkInterfacesInput{ - Filters: []*ec2.Filter{ - { - Name: aws.String("status"), - Values: []*string{aws.String(ec2.NetworkInterfaceStatusAvailable)}, - }, - { - Name: aws.String("tag:" + e.clusterNameTagKey), - Values: []*string{aws.String(config.ClusterNameTagValue)}, - }, - { - Name: aws.String("tag:" + config.NetworkInterfaceOwnerTagKey), - Values: aws.StringSlice([]string{config.NetworkInterfaceOwnerTagValue, - config.NetworkInterfaceOwnerVPCCNITagValue}), - }, - }, - } - - availableENIs := make(map[string]struct{}) - - for { - describeNetworkInterfaceOp, err := e.EC2Wrapper.DescribeNetworkInterfaces(describeNetworkInterfaceIp) - if err != nil { - e.Log.Error(err, "failed to describe network interfaces, will retry") - return - } - - for _, networkInterface := range describeNetworkInterfaceOp.NetworkInterfaces { - if _, exists := e.availableENIs[*networkInterface.NetworkInterfaceId]; exists { - // Increment promethues metrics for number of leaked ENIs cleaned up - if tagIdx := slices.IndexFunc(networkInterface.TagSet, func(tag *ec2.Tag) bool { - return *tag.Key == config.NetworkInterfaceOwnerTagKey - }); tagIdx != -1 { - switch *networkInterface.TagSet[tagIdx].Value { - case config.NetworkInterfaceOwnerTagValue: - vpcrcLeakedENICleanupCnt.Inc() - case config.NetworkInterfaceOwnerVPCCNITagValue: - vpcCniLeakedENICleanupCnt.Inc() - default: - // We will not hit this case as we only filter for above two tag values, adding it for any future use cases - e.Log.Info("found available ENI not created by VPC-CNI/VPC-RC") - } - } - - // The ENI in available state has been sitting for at least the eni clean up interval and it should - // be removed - _, err := e.EC2Wrapper.DeleteNetworkInterface(&ec2.DeleteNetworkInterfaceInput{ - NetworkInterfaceId: networkInterface.NetworkInterfaceId, - }) - if err != nil { - // Log and continue, if the ENI is still present it will be cleaned up in next 2 cycles - e.Log.Error(err, "failed to delete the dangling network interface", - "id", *networkInterface.NetworkInterfaceId) - continue - } - e.Log.Info("deleted dangling ENI successfully", - "eni id", networkInterface.NetworkInterfaceId) - } else { - // Seeing the ENI for the first time, add it to the new list of available network interfaces - availableENIs[*networkInterface.NetworkInterfaceId] = struct{}{} - e.Log.V(1).Info("adding eni to to the map of available ENIs, will be removed if present in "+ - "next run too", "id", *networkInterface.NetworkInterfaceId) - } - } - - if describeNetworkInterfaceOp.NextToken == nil { - break - } - - describeNetworkInterfaceIp.NextToken = describeNetworkInterfaceOp.NextToken - } - - // Set the available ENIs to the list of ENIs seen in the current cycle - e.availableENIs = availableENIs -} diff --git a/pkg/aws/ec2/api/eni_cleanup_test.go b/pkg/aws/ec2/api/eni_cleanup_test.go deleted file mode 100644 index 199f6368..00000000 --- a/pkg/aws/ec2/api/eni_cleanup_test.go +++ /dev/null @@ -1,117 +0,0 @@ -// Copyright Amazon.com Inc. or its affiliates. All Rights Reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"). You may -// not use this file except in compliance with the License. A copy of the -// License is located at -// -// http://aws.amazon.com/apache2.0/ -// -// or in the "license" file accompanying this file. This file is distributed -// on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either -// express or implied. See the License for the specific language governing -// permissions and limitations under the License. - -package api - -import ( - "context" - "fmt" - "reflect" - "testing" - - mock_api "github.com/aws/amazon-vpc-resource-controller-k8s/mocks/amazon-vcp-resource-controller-k8s/pkg/aws/ec2/api" - "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/config" - - "github.com/aws/aws-sdk-go/aws" - "github.com/aws/aws-sdk-go/service/ec2" - "github.com/golang/mock/gomock" - "github.com/stretchr/testify/assert" - "sigs.k8s.io/controller-runtime/pkg/log/zap" -) - -var ( - mockClusterName = "cluster-name" - mockClusterNameTagKey = fmt.Sprintf(config.ClusterNameTagKeyFormat, mockClusterName) - - mockNetworkInterfaceId1 = "eni-000000000000000" - mockNetworkInterfaceId2 = "eni-000000000000001" - mockNetworkInterfaceId3 = "eni-000000000000002" - - mockDescribeNetworkInterfaceIp = &ec2.DescribeNetworkInterfacesInput{ - Filters: []*ec2.Filter{ - { - Name: aws.String("status"), - Values: []*string{aws.String(ec2.NetworkInterfaceStatusAvailable)}, - }, - { - Name: aws.String("tag:" + mockClusterNameTagKey), - Values: []*string{aws.String(config.ClusterNameTagValue)}, - }, - { - Name: aws.String("tag:" + config.NetworkInterfaceOwnerTagKey), - Values: aws.StringSlice([]string{config.NetworkInterfaceOwnerTagValue, - config.NetworkInterfaceOwnerVPCCNITagValue}), - }, - }, - } - mockDescribeInterfaceOpWith1And2 = &ec2.DescribeNetworkInterfacesOutput{ - NetworkInterfaces: []*ec2.NetworkInterface{ - {NetworkInterfaceId: &mockNetworkInterfaceId1}, - {NetworkInterfaceId: &mockNetworkInterfaceId2}, - }, - } - mockDescribeInterfaceOpWith1And3 = &ec2.DescribeNetworkInterfacesOutput{ - NetworkInterfaces: []*ec2.NetworkInterface{ - {NetworkInterfaceId: &mockNetworkInterfaceId1}, - {NetworkInterfaceId: &mockNetworkInterfaceId3}, - }, - } -) - -func getMockENICleaner(ctrl *gomock.Controller) (*ENICleaner, *mock_api.MockEC2Wrapper) { - mockEC2Wrapper := mock_api.NewMockEC2Wrapper(ctrl) - return &ENICleaner{ - EC2Wrapper: mockEC2Wrapper, - availableENIs: map[string]struct{}{}, - Log: zap.New(zap.UseDevMode(true)), - clusterNameTagKey: mockClusterNameTagKey, - ctx: context.Background(), - }, mockEC2Wrapper -} - -func TestENICleaner_cleanUpAvailableENIs(t *testing.T) { - ctrl := gomock.NewController(t) - eniCleaner, mockWrapper := getMockENICleaner(ctrl) - - gomock.InOrder( - // Return network interface 1 and 2 in first cycle - mockWrapper.EXPECT().DescribeNetworkInterfaces(mockDescribeNetworkInterfaceIp). - Return(mockDescribeInterfaceOpWith1And2, nil), - // Return network interface 1 and 3 in the second cycle - mockWrapper.EXPECT().DescribeNetworkInterfaces(mockDescribeNetworkInterfaceIp). - Return(mockDescribeInterfaceOpWith1And3, nil), - // Expect to delete the network interface 1 - mockWrapper.EXPECT().DeleteNetworkInterface( - &ec2.DeleteNetworkInterfaceInput{NetworkInterfaceId: &mockNetworkInterfaceId1}).Return(nil, nil), - ) - - // Run 1st cycle, network interface 1 and 2 should be added to the map of available ENIs - eniCleaner.cleanUpAvailableENIs() - assert.True(t, reflect.DeepEqual( - map[string]struct{}{mockNetworkInterfaceId1: {}, mockNetworkInterfaceId2: {}}, eniCleaner.availableENIs)) - - // Run the second cycle, this time network interface 1 should be deleted and network interface 3 added to list - eniCleaner.cleanUpAvailableENIs() - assert.True(t, reflect.DeepEqual( - map[string]struct{}{mockNetworkInterfaceId3: {}}, eniCleaner.availableENIs)) -} - -// TestENICleaner_StartENICleaner_Shutdown tests that ENICleaner would not start if shutdown is set to true. -func TestENICleaner_StartENICleaner_Shutdown(t *testing.T) { - ctrl := gomock.NewController(t) - eniCleaner, _ := getMockENICleaner(ctrl) - - eniCleaner.shutdown = true - - eniCleaner.Start(context.TODO()) -} diff --git a/pkg/aws/ec2/api/helper.go b/pkg/aws/ec2/api/helper.go index 2b634271..48ac7e91 100644 --- a/pkg/aws/ec2/api/helper.go +++ b/pkg/aws/ec2/api/helper.go @@ -79,7 +79,7 @@ type EC2APIHelper interface { ipResourceCount *config.IPResourceCount, interfaceType *string) (*ec2.NetworkInterface, error) DeleteNetworkInterface(interfaceId *string) error GetSubnet(subnetId *string) (*ec2.Subnet, error) - GetBranchNetworkInterface(trunkID *string) ([]*ec2.NetworkInterface, error) + GetBranchNetworkInterface(trunkID *string, subnetID *string) ([]*ec2.NetworkInterface, error) GetInstanceNetworkInterface(instanceId *string) ([]*ec2.InstanceNetworkInterface, error) DescribeNetworkInterfaces(nwInterfaceIds []*string) ([]*ec2.NetworkInterface, error) DescribeTrunkInterfaceAssociation(trunkInterfaceId *string) ([]*ec2.TrunkInterfaceAssociation, error) @@ -563,43 +563,20 @@ func (h *ec2APIHelper) UnassignIPv4Resources(eniID string, resourceType config.R return err } -func (h *ec2APIHelper) GetBranchNetworkInterface(trunkID *string) ([]*ec2.NetworkInterface, error) { - filters := []*ec2.Filter{{ - Name: aws.String("tag:" + config.TrunkENIIDTag), - Values: []*string{trunkID}, - }} - - describeNetworkInterfacesInput := &ec2.DescribeNetworkInterfacesInput{Filters: filters} - var nwInterfaces []*ec2.NetworkInterface - for { - describeNetworkInterfaceOutput, err := h.ec2Wrapper.DescribeNetworkInterfaces(describeNetworkInterfacesInput) - if err != nil { - return nil, err - } - - if describeNetworkInterfaceOutput == nil || describeNetworkInterfaceOutput.NetworkInterfaces == nil || - len(describeNetworkInterfaceOutput.NetworkInterfaces) == 0 { - // No more interface associated with the trunk, return the result - break - } - - // One or more interface associated with the trunk, return the result - for _, nwInterface := range describeNetworkInterfaceOutput.NetworkInterfaces { - // Only attach the required details to avoid consuming extra memory - nwInterfaces = append(nwInterfaces, &ec2.NetworkInterface{ - NetworkInterfaceId: nwInterface.NetworkInterfaceId, - TagSet: nwInterface.TagSet, - }) - } - - if describeNetworkInterfaceOutput.NextToken == nil { - break - } - - describeNetworkInterfacesInput.NextToken = describeNetworkInterfaceOutput.NextToken +func (h *ec2APIHelper) GetBranchNetworkInterface(trunkID *string, subnetID *string) ([]*ec2.NetworkInterface, error) { + filters := []*ec2.Filter{ + { + Name: aws.String("tag:" + config.TrunkENIIDTag), + Values: []*string{trunkID}, + }, + { + Name: aws.String("subnet-id"), + Values: []*string{subnetID}, + }, } - return nwInterfaces, nil + describeNetworkInterfacesInput := &ec2.DescribeNetworkInterfacesInput{Filters: filters} + return h.ec2Wrapper.DescribeNetworkInterfacesPages(describeNetworkInterfacesInput) } // DetachAndDeleteNetworkInterface detaches the network interface first and then deletes it diff --git a/pkg/aws/ec2/api/helper_test.go b/pkg/aws/ec2/api/helper_test.go index 971e8211..38cb16bc 100644 --- a/pkg/aws/ec2/api/helper_test.go +++ b/pkg/aws/ec2/api/helper_test.go @@ -179,27 +179,20 @@ var ( tokenID = "token" - describeTrunkInterfaceInput1 = &ec2.DescribeNetworkInterfacesInput{ - Filters: []*ec2.Filter{{ - Name: aws.String("tag:" + config.TrunkENIIDTag), - Values: []*string{&trunkInterfaceId}, - }}, - } - describeTrunkInterfaceInput2 = &ec2.DescribeNetworkInterfacesInput{ - Filters: []*ec2.Filter{{ - Name: aws.String("tag:" + config.TrunkENIIDTag), - Values: []*string{&trunkInterfaceId}, - }}, - NextToken: &tokenID, + describeTrunkInterfaceInput = &ec2.DescribeNetworkInterfacesInput{ + Filters: []*ec2.Filter{ + { + Name: aws.String("tag:" + config.TrunkENIIDTag), + Values: []*string{&trunkInterfaceId}, + }, + { + Name: aws.String("subnet-id"), + Values: aws.StringSlice([]string{subnetId}), + }, + }, } - describeTrunkInterfaceOutput1 = &ec2.DescribeNetworkInterfacesOutput{ - NetworkInterfaces: []*ec2.NetworkInterface{&networkInterface1}, - NextToken: &tokenID, - } - describeTrunkInterfaceOutput2 = &ec2.DescribeNetworkInterfacesOutput{ - NetworkInterfaces: []*ec2.NetworkInterface{&networkInterface2}, - } + describeTrunkInterfaceOutput = []*ec2.NetworkInterface{&networkInterface1, &networkInterface2} describeTrunkInterfaceAssociationsInput = &ec2.DescribeTrunkInterfaceAssociationsInput{ Filters: []*ec2.Filter{{ @@ -1178,16 +1171,15 @@ func TestEC2APIHelper_AssignIPv4ResourcesAndWaitTillReady_TypeIPv4Prefix_Describ } // TestEc2APIHelper_GetBranchNetworkInterface_PaginatedResults returns the branch interface when paginated results is returned -func TestEc2APIHelper_GetBranchNetworkInterface_PaginatedResults(t *testing.T) { +func TestEc2APIHelper_GetBranchNetworkInterface(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() ec2ApiHelper, mockWrapper := getMockWrapper(ctrl) - mockWrapper.EXPECT().DescribeNetworkInterfaces(describeTrunkInterfaceInput1).Return(describeTrunkInterfaceOutput1, nil) - mockWrapper.EXPECT().DescribeNetworkInterfaces(describeTrunkInterfaceInput2).Return(describeTrunkInterfaceOutput2, nil) + mockWrapper.EXPECT().DescribeNetworkInterfacesPages(describeTrunkInterfaceInput).Return(describeTrunkInterfaceOutput, nil) - branchInterfaces, err := ec2ApiHelper.GetBranchNetworkInterface(&trunkInterfaceId) + branchInterfaces, err := ec2ApiHelper.GetBranchNetworkInterface(&trunkInterfaceId, &subnetId) assert.NoError(t, err) assert.ElementsMatch(t, []*ec2.NetworkInterface{&networkInterface1, &networkInterface2}, branchInterfaces) } diff --git a/pkg/aws/ec2/api/wrapper.go b/pkg/aws/ec2/api/wrapper.go index 4beaff87..97a20f3b 100644 --- a/pkg/aws/ec2/api/wrapper.go +++ b/pkg/aws/ec2/api/wrapper.go @@ -21,6 +21,7 @@ import ( "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/config" "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/utils" + "k8s.io/apimachinery/pkg/util/wait" "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/aws/credentials" @@ -52,6 +53,7 @@ type EC2Wrapper interface { AssignPrivateIPAddresses(input *ec2.AssignPrivateIpAddressesInput) (*ec2.AssignPrivateIpAddressesOutput, error) UnassignPrivateIPAddresses(input *ec2.UnassignPrivateIpAddressesInput) (*ec2.UnassignPrivateIpAddressesOutput, error) DescribeNetworkInterfaces(input *ec2.DescribeNetworkInterfacesInput) (*ec2.DescribeNetworkInterfacesOutput, error) + DescribeNetworkInterfacesPages(input *ec2.DescribeNetworkInterfacesInput) ([]*ec2.NetworkInterface, error) CreateTags(input *ec2.CreateTagsInput) (*ec2.CreateTagsOutput, error) DescribeSubnets(input *ec2.DescribeSubnetsInput) (*ec2.DescribeSubnetsOutput, error) AssociateTrunkInterface(input *ec2.AssociateTrunkInterfaceInput) (*ec2.AssociateTrunkInterfaceOutput, error) @@ -322,6 +324,40 @@ var ( }, ) + VpcCniAvailableClusterENICnt = prometheus.NewGauge( + prometheus.GaugeOpts{ + Name: "vpc_cni_created_available_eni_count", + Help: "The number of available ENIs created by VPC-CNI that will tried to be deleted by the controller", + }, + ) + + VpcRcAvailableClusterENICnt = prometheus.NewGauge( + prometheus.GaugeOpts{ + Name: "vpc_rc_created_available_eni_count", + Help: "The number of available ENIs created by VPC-RC that will tried to be deleted by the controller", + }, + ) + + LeakedENIClusterCleanupCnt = prometheus.NewGauge( + prometheus.GaugeOpts{ + Name: "leaked_eni_count", + Help: "The number of available ENIs that failed to be deleted by the controller", + }, + ) + + ec2DescribeNetworkInterfacesPagesAPICallCnt = prometheus.NewCounter( + prometheus.CounterOpts{ + Name: "ec2_describe_network_interfaces_pages_api_call_count", + Help: "The number of calls made to describe network interfaces (paginated)", + }, + ) + ec2DescribeNetworkInterfacesPagesAPIErrCnt = prometheus.NewCounter( + prometheus.CounterOpts{ + Name: "ec2_describe_network_interfaces_pages_api_err_count", + Help: "The number of errors encountered while making call to describe network interfaces (paginated)", + }, + ) + prometheusRegistered = false ) @@ -360,10 +396,13 @@ func prometheusRegister() { ec2modifyNetworkInterfaceAttributeAPICallCnt, ec2modifyNetworkInterfaceAttributeAPIErrCnt, ec2APICallLatencies, - vpcCniLeakedENICleanupCnt, - vpcrcLeakedENICleanupCnt, ec2DisassociateTrunkInterfaceCallCnt, ec2DisassociateTrunkInterfaceErrCnt, + VpcRcAvailableClusterENICnt, + VpcCniAvailableClusterENICnt, + LeakedENIClusterCleanupCnt, + ec2DescribeNetworkInterfacesPagesAPICallCnt, + ec2DescribeNetworkInterfacesPagesAPIErrCnt, ) prometheusRegistered = true @@ -656,6 +695,38 @@ func (e *ec2Wrapper) DescribeNetworkInterfaces(input *ec2.DescribeNetworkInterfa return describeNetworkInterfacesOutput, err } +// DescribeNetworkInterfacesPages returns network interfaces that match the filters specified in the input with MaxResult set to 1000(max value) +// This API is used during periodic ENI cleanup routine and trunk initialization to list all network interfaces that match the given filters (vpc-id or subnet-id, and tag) +// Only required fields, network interface ID and tag set, is populated to avoid consuming extra memory +func (e *ec2Wrapper) DescribeNetworkInterfacesPages(input *ec2.DescribeNetworkInterfacesInput) ([]*ec2.NetworkInterface, error) { + var networkInterfaces []*ec2.NetworkInterface + input.MaxResults = aws.Int64(config.DescribeNetworkInterfacesMaxResults) + + start := time.Now() + if err := e.userServiceClient.DescribeNetworkInterfacesPages(input, func(output *ec2.DescribeNetworkInterfacesOutput, _ bool) bool { + ec2APICallCnt.Inc() + ec2DescribeNetworkInterfacesPagesAPICallCnt.Inc() + //Currently only network interface ID and the tag set is require, only add required details to avoid consuming extra memory + for _, nwInterface := range output.NetworkInterfaces { + networkInterfaces = append(networkInterfaces, &ec2.NetworkInterface{ + NetworkInterfaceId: nwInterface.NetworkInterfaceId, + TagSet: nwInterface.TagSet, + }) + } + // Add jitter to avoid EC2 API throttling in the account + time.Sleep(wait.Jitter(500*time.Millisecond, 0.5)) + return true + + }); err != nil { + ec2APIErrCnt.Inc() + ec2DescribeNetworkInterfacesPagesAPIErrCnt.Inc() + return nil, err + } + ec2APICallLatencies.WithLabelValues("describe_network_interfaces_pages").Observe(timeSinceMs(start)) + + return networkInterfaces, nil +} + func (e *ec2Wrapper) AssignPrivateIPAddresses(input *ec2.AssignPrivateIpAddressesInput) (*ec2.AssignPrivateIpAddressesOutput, error) { start := time.Now() assignPrivateIPAddressesOutput, err := e.userServiceClient.AssignPrivateIpAddresses(input) diff --git a/pkg/config/type.go b/pkg/config/type.go index d7673640..a0c271cb 100644 --- a/pkg/config/type.go +++ b/pkg/config/type.go @@ -51,6 +51,8 @@ const ( OSWindows = "windows" // OSLinux is the the linux Operating System OSLinux = "linux" + // Node termination finalizer on CNINode CRD + NodeTerminationFinalizer = "networking.k8s.aws/resource-cleanup" ) // EC2 Tags @@ -65,6 +67,7 @@ const ( NetworkInterfaceOwnerTagKey = "eks:eni:owner" NetworkInterfaceOwnerTagValue = "eks-vpc-resource-controller" NetworkInterfaceOwnerVPCCNITagValue = "amazon-vpc-cni" + NetworkInterfaceNodenameKey = "node.k8s.amazonaws.com/nodename" ) const ( @@ -86,6 +89,17 @@ const ( VpcCNIDaemonSetName = "aws-node" OldVPCControllerDeploymentName = "vpc-resource-controller" BranchENICooldownPeriodKey = "branch-eni-cooldown" + // DescribeNetworkInterfacesMaxResults defines the max number of requests to return for DescribeNetworkInterfaces API call + DescribeNetworkInterfacesMaxResults = int64(1000) +) + +// 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 +// one routines to help high rate churn and larger nodes groups restarting +// when the controller has to be restarted for various reasons. +const ( + MaxNodeConcurrentReconciles = 10 ) type ResourceType string diff --git a/pkg/k8s/finalizer.go b/pkg/k8s/finalizer.go new file mode 100644 index 00000000..06f6a059 --- /dev/null +++ b/pkg/k8s/finalizer.go @@ -0,0 +1,82 @@ +// Copyright Amazon.com Inc. or its affiliates. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"). You may +// not use this file except in compliance with the License. A copy of the +// License is located at +// +// http://aws.amazon.com/apache2.0/ +// +// or in the "license" file accompanying this file. This file is distributed +// on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either +// express or implied. See the License for the specific language governing +// permissions and limitations under the License. + +package k8s + +import ( + "context" + + "github.com/go-logr/logr" + "k8s.io/client-go/util/retry" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" +) + +type FinalizerManager interface { + AddFinalizers(ctx context.Context, object client.Object, finalizers ...string) error + RemoveFinalizers(ctx context.Context, object client.Object, finalizers ...string) error +} + +func NewDefaultFinalizerManager(k8sClient client.Client, log logr.Logger) FinalizerManager { + return &defaultFinalizerManager{ + k8sClient: k8sClient, + log: log, + } +} + +type defaultFinalizerManager struct { + k8sClient client.Client + log logr.Logger +} + +func (m *defaultFinalizerManager) AddFinalizers(ctx context.Context, obj client.Object, finalizers ...string) error { + return retry.RetryOnConflict(retry.DefaultBackoff, func() error { + if err := m.k8sClient.Get(ctx, NamespacedName(obj), obj); err != nil { + return err + } + + oldObj := obj.DeepCopyObject().(client.Object) + needsUpdate := false + for _, finalizer := range finalizers { + if !controllerutil.ContainsFinalizer(obj, finalizer) { + controllerutil.AddFinalizer(obj, finalizer) + needsUpdate = true + } + } + if !needsUpdate { + return nil + } + return m.k8sClient.Patch(ctx, obj, client.MergeFromWithOptions(oldObj, client.MergeFromWithOptimisticLock{})) + }) +} + +func (m *defaultFinalizerManager) RemoveFinalizers(ctx context.Context, obj client.Object, finalizers ...string) error { + return retry.RetryOnConflict(retry.DefaultBackoff, func() error { + if err := m.k8sClient.Get(ctx, NamespacedName(obj), obj); err != nil { + return err + } + + oldObj := obj.DeepCopyObject().(client.Object) + needsUpdate := false + for _, finalizer := range finalizers { + if controllerutil.ContainsFinalizer(obj, finalizer) { + controllerutil.RemoveFinalizer(obj, finalizer) + needsUpdate = true + } + } + if !needsUpdate { + return nil + } + return m.k8sClient.Patch(ctx, obj, client.MergeFromWithOptions(oldObj, client.MergeFromWithOptimisticLock{})) + }) +} diff --git a/pkg/k8s/utils.go b/pkg/k8s/utils.go new file mode 100644 index 00000000..bdf47cc7 --- /dev/null +++ b/pkg/k8s/utils.go @@ -0,0 +1,23 @@ +// Copyright Amazon.com Inc. or its affiliates. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"). You may +// not use this file except in compliance with the License. A copy of the +// License is located at +// +// http://aws.amazon.com/apache2.0/ +// +// or in the "license" file accompanying this file. This file is distributed +// on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either +// express or implied. See the License for the specific language governing +// permissions and limitations under the License. + +package k8s + +import ( + "sigs.k8s.io/controller-runtime/pkg/client" +) + +// NamespacedName returns the namespaced name for k8s objects +func NamespacedName(obj client.Object) client.ObjectKey { + return client.ObjectKeyFromObject(obj) +} diff --git a/pkg/k8s/wrapper.go b/pkg/k8s/wrapper.go index 681274f0..90e8355f 100644 --- a/pkg/k8s/wrapper.go +++ b/pkg/k8s/wrapper.go @@ -15,6 +15,7 @@ package k8s import ( "context" + "fmt" "strconv" "github.com/aws/amazon-vpc-cni-k8s/pkg/apis/crd/v1alpha1" @@ -80,7 +81,7 @@ type K8sWrapper interface { AddLabelToManageNode(node *v1.Node, labelKey string, labelValue string) (bool, error) ListEvents(ops []client.ListOption) (*eventsv1.EventList, error) GetCNINode(namespacedName types.NamespacedName) (*rcv1alpha1.CNINode, error) - CreateCNINode(node *v1.Node) error + CreateCNINode(node *v1.Node, clusterName string) error } // k8sWrapper is the wrapper object with the client @@ -233,7 +234,7 @@ func (k *k8sWrapper) GetCNINode(namespacedName types.NamespacedName) (*rcv1alpha return cninode, nil } -func (k *k8sWrapper) CreateCNINode(node *v1.Node) error { +func (k *k8sWrapper) CreateCNINode(node *v1.Node, clusterName string) error { cniNode := &rcv1alpha1.CNINode{ ObjectMeta: metav1.ObjectMeta{ Name: node.Name, @@ -248,6 +249,12 @@ func (k *k8sWrapper) CreateCNINode(node *v1.Node) error { Controller: lo.ToPtr(true), }, }, + Finalizers: []string{config.NodeTerminationFinalizer}, // finalizer to clean up leaked ENIs at node termination + }, + Spec: rcv1alpha1.CNINodeSpec{ + Tags: map[string]string{ + fmt.Sprintf(config.ClusterNameTagKeyFormat, clusterName): config.ClusterNameTagValue, + }, }, } diff --git a/pkg/k8s/wrapper_test.go b/pkg/k8s/wrapper_test.go index 572a26e9..20ed2b81 100644 --- a/pkg/k8s/wrapper_test.go +++ b/pkg/k8s/wrapper_test.go @@ -37,6 +37,7 @@ import ( var ( nodeName = "node-name" + mockClusterName = "cluster-name" mockResourceName = config.ResourceNamePodENI existingResource = "extended-resource" @@ -196,12 +197,12 @@ func TestK8sWrapper_CreateCNINodeWithExistedObject_NoError(t *testing.T) { ctrl := gomock.NewController(t) wrapper, _, _ := getMockK8sWrapperWithClient(ctrl, []runtime.Object{mockCNINode}) - err := wrapper.CreateCNINode(mockNode) + err := wrapper.CreateCNINode(mockNode, mockClusterName) assert.NoError(t, err) cniNode, err := wrapper.GetCNINode(types.NamespacedName{Name: mockNode.Name}) assert.NoError(t, err) assert.Equal(t, mockNode.Name, cniNode.Name) - err = wrapper.CreateCNINode(mockNode) + err = wrapper.CreateCNINode(mockNode, mockClusterName) assert.NoError(t, err) } @@ -209,7 +210,7 @@ func TestK8sWrapper_CreateCNINode_NoError(t *testing.T) { ctrl := gomock.NewController(t) wrapper, _, _ := getMockK8sWrapperWithClient(ctrl, []runtime.Object{mockCNINode}) - err := wrapper.CreateCNINode(mockNode) + err := wrapper.CreateCNINode(mockNode, mockClusterName) assert.NoError(t, err) cniNode, err := wrapper.GetCNINode(types.NamespacedName{Name: mockNode.Name}) assert.NoError(t, err) diff --git a/pkg/node/manager/manager.go b/pkg/node/manager/manager.go index c9b6f9c2..5a0089f4 100644 --- a/pkg/node/manager/manager.go +++ b/pkg/node/manager/manager.go @@ -57,6 +57,7 @@ type manager struct { worker asyncWorker.Worker conditions condition.Conditions controllerVersion string + clusterName string } // Manager to perform operation on list of managed/un-managed node @@ -98,7 +99,7 @@ type AsyncOperationJob struct { // NewNodeManager returns a new node manager func NewNodeManager(logger logr.Logger, resourceManager resource.ResourceManager, - wrapper api.Wrapper, worker asyncWorker.Worker, conditions condition.Conditions, controllerVersion string, healthzHandler *rcHealthz.HealthzHandler) (Manager, error) { + wrapper api.Wrapper, worker asyncWorker.Worker, conditions condition.Conditions, clusterName string, controllerVersion string, healthzHandler *rcHealthz.HealthzHandler) (Manager, error) { manager := &manager{ resourceManager: resourceManager, @@ -108,6 +109,7 @@ func NewNodeManager(logger logr.Logger, resourceManager resource.ResourceManager worker: worker, conditions: conditions, controllerVersion: controllerVersion, + clusterName: clusterName, } // add health check on subpath for node manager @@ -224,7 +226,7 @@ func (m *manager) CreateCNINodeIfNotExisting(node *v1.Node) error { ); err != nil { if apierrors.IsNotFound(err) { m.Log.Info("Will create a new CNINode", "CNINodeName", node.Name) - return m.wrapper.K8sAPI.CreateCNINode(node) + return m.wrapper.K8sAPI.CreateCNINode(node, m.clusterName) } return err } else { diff --git a/pkg/node/manager/manager_test.go b/pkg/node/manager/manager_test.go index c8450927..ecb3f48b 100644 --- a/pkg/node/manager/manager_test.go +++ b/pkg/node/manager/manager_test.go @@ -50,6 +50,7 @@ var ( subnetID = "subnet-id" nodeName = "ip-192-168-55-73.us-west-2.compute.internal" securityGroupId = "sg-1" + mockClusterName = "cluster-name" eniConfig = &v1alpha1.ENIConfig{ ObjectMeta: metav1.ObjectMeta{ @@ -147,6 +148,7 @@ func NewMock(ctrl *gomock.Controller, existingDataStore map[string]node.Node) Mo worker: mockAsyncWorker, resourceManager: mockResourceManager, conditions: mockConditions, + clusterName: mockClusterName, }, MockK8sAPI: mockK8sWrapper, MockEC2API: mockEC2APIHelper, @@ -165,7 +167,7 @@ func Test_GetNewManager(t *testing.T) { mock := NewMock(ctrl, map[string]node.Node{}) mock.MockWorker.EXPECT().StartWorkerPool(gomock.Any()).Return(nil) - manager, err := NewNodeManager(zap.New(), nil, api.Wrapper{}, mock.MockWorker, mock.MockConditions, "v1.3.1", healthzHandler) + manager, err := NewNodeManager(zap.New(), nil, api.Wrapper{}, mock.MockWorker, mock.MockConditions, mockClusterName, "v1.3.1", healthzHandler) assert.NotNil(t, manager) assert.NoError(t, err) @@ -179,7 +181,7 @@ func Test_GetNewManager_Error(t *testing.T) { mock := NewMock(ctrl, map[string]node.Node{}) mock.MockWorker.EXPECT().StartWorkerPool(gomock.Any()).Return(mockError) - manager, err := NewNodeManager(zap.New(), nil, api.Wrapper{}, mock.MockWorker, mock.MockConditions, "v1.3.1", healthzHandler) + manager, err := NewNodeManager(zap.New(), nil, api.Wrapper{}, mock.MockWorker, mock.MockConditions, mockClusterName, "v1.3.1", healthzHandler) assert.NotNil(t, manager) assert.Error(t, err, mockError) @@ -201,7 +203,7 @@ func Test_AddNode_CNINode_Existing(t *testing.T) { mock.MockK8sAPI.EXPECT().GetNode(nodeName).Return(v1Node, nil).Times(1) mock.MockWorker.EXPECT().SubmitJob(gomock.All(NewAsyncOperationMatcher(expectedJob))) - mock.MockK8sAPI.EXPECT().CreateCNINode(v1Node).Return(nil).Times(0) + mock.MockK8sAPI.EXPECT().CreateCNINode(v1Node, mockClusterName).Return(nil).Times(0) mock.MockK8sAPI.EXPECT().GetCNINode(types.NamespacedName{Name: v1Node.Name}).Return(&rcV1alpha1.CNINode{}, nil).Times(2) err := mock.Manager.AddNode(nodeName) @@ -224,7 +226,7 @@ func Test_AddNode_CNINode_Not_Existing(t *testing.T) { mock.MockK8sAPI.EXPECT().GetNode(nodeName).Return(v1Node, nil).Times(1) mock.MockWorker.EXPECT().SubmitJob(gomock.All(NewAsyncOperationMatcher(expectedJob))) - mock.MockK8sAPI.EXPECT().CreateCNINode(v1Node).Return(nil).Times(1) + mock.MockK8sAPI.EXPECT().CreateCNINode(v1Node, mock.Manager.clusterName).Return(nil).Times(1) mock.MockK8sAPI.EXPECT().GetCNINode(types.NamespacedName{Name: v1Node.Name}).Return( &rcV1alpha1.CNINode{}, apierrors.NewNotFound(schema.GroupResource{Group: "vpcresources.k8s.aws", Resource: "1"}, "test")). Times(2) @@ -245,7 +247,7 @@ func Test_AddNode_UnManaged(t *testing.T) { nodeWithoutLabel.Labels = map[string]string{} mock.MockK8sAPI.EXPECT().GetNode(nodeName).Return(nodeWithoutLabel, nil).Times(1) - mock.MockK8sAPI.EXPECT().CreateCNINode(nodeWithoutLabel).Return(nil).Times(1) + mock.MockK8sAPI.EXPECT().CreateCNINode(nodeWithoutLabel, mock.Manager.clusterName).Return(nil).Times(1) mock.MockK8sAPI.EXPECT().GetCNINode(types.NamespacedName{Name: nodeWithoutLabel.Name}).Return( &rcV1alpha1.CNINode{}, apierrors.NewNotFound(schema.GroupResource{Group: "vpcresources.k8s.aws", Resource: "1"}, "test")). Times(1) // unmanaged node won't check custom networking subnets and call GetCNINode only once @@ -287,7 +289,7 @@ func Test_AddNode_CustomNetworking_CNINode(t *testing.T) { mock.MockK8sAPI.EXPECT().GetNode(nodeName).Return(nodeWithENIConfig, nil) mock.MockK8sAPI.EXPECT().GetENIConfig(eniConfigName).Return(eniConfig, nil).Times(1) mock.MockWorker.EXPECT().SubmitJob(gomock.All(NewAsyncOperationMatcher(job))) - mock.MockK8sAPI.EXPECT().CreateCNINode(nodeWithENIConfig).Return(nil).Times(1) + mock.MockK8sAPI.EXPECT().CreateCNINode(nodeWithENIConfig, mock.Manager.clusterName).Return(nil).Times(1) mock.MockK8sAPI.EXPECT().GetCNINode(types.NamespacedName{Name: nodeWithENIConfig.Name}).Return(&rcV1alpha1.CNINode{ Spec: rcV1alpha1.CNINodeSpec{ Features: []rcV1alpha1.Feature{{Name: rcV1alpha1.CustomNetworking, Value: eniConfigName}}, @@ -327,7 +329,7 @@ func Test_AddNode_CustomNetworking_CNINode_No_EniConfigName(t *testing.T) { mock.MockK8sAPI.EXPECT().BroadcastEvent(nodeWithENIConfig, utils.EniConfigNameNotFoundReason, msg, v1.EventTypeWarning).Times(1) mock.MockK8sAPI.EXPECT().GetENIConfig(eniConfigName).Return(eniConfig, nil).Times(0) mock.MockWorker.EXPECT().SubmitJob(gomock.All(NewAsyncOperationMatcher(job))).Times(0) - mock.MockK8sAPI.EXPECT().CreateCNINode(nodeWithENIConfig).Return(nil).Times(1) + mock.MockK8sAPI.EXPECT().CreateCNINode(nodeWithENIConfig, mock.Manager.clusterName).Return(nil).Times(1) mock.MockK8sAPI.EXPECT().GetCNINode(types.NamespacedName{Name: nodeWithENIConfig.Name}).Return(&rcV1alpha1.CNINode{ Spec: rcV1alpha1.CNINodeSpec{ Features: []rcV1alpha1.Feature{{Name: rcV1alpha1.CustomNetworking}}, @@ -364,7 +366,7 @@ func Test_AddNode_CustomNetworking_NodeLabel(t *testing.T) { mock.MockK8sAPI.EXPECT().GetNode(nodeName).Return(nodeWithENIConfig, nil) mock.MockK8sAPI.EXPECT().GetENIConfig(eniConfigName).Return(eniConfig, nil).Times(1) mock.MockWorker.EXPECT().SubmitJob(gomock.All(NewAsyncOperationMatcher(job))) - mock.MockK8sAPI.EXPECT().CreateCNINode(nodeWithENIConfig).Return(nil).Times(1) + mock.MockK8sAPI.EXPECT().CreateCNINode(nodeWithENIConfig, mock.Manager.clusterName).Return(nil).Times(1) mock.MockK8sAPI.EXPECT().GetCNINode(types.NamespacedName{Name: nodeWithENIConfig.Name}).Return(&rcV1alpha1.CNINode{ Spec: rcV1alpha1.CNINodeSpec{ Features: []rcV1alpha1.Feature{{Name: rcV1alpha1.CustomNetworking}}, @@ -397,7 +399,7 @@ func Test_AddNode_CustomNetworking_Incorrect_ENIConfig(t *testing.T) { 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))) - mock.MockK8sAPI.EXPECT().CreateCNINode(nodeWithENIConfig).Return(nil).Times(1) + mock.MockK8sAPI.EXPECT().CreateCNINode(nodeWithENIConfig, mockClusterName).Return(nil).Times(1) mock.MockK8sAPI.EXPECT().GetCNINode(types.NamespacedName{Name: nodeWithENIConfig.Name}).Return(&rcV1alpha1.CNINode{ Spec: rcV1alpha1.CNINodeSpec{ Features: []rcV1alpha1.Feature{{Name: rcV1alpha1.CustomNetworking}}, @@ -421,7 +423,7 @@ func Test_AddNode_CustomNetworking_NoENIConfig(t *testing.T) { nodeWithENIConfig.Labels[config.CustomNetworkingLabel] = eniConfigName mock.MockK8sAPI.EXPECT().GetNode(nodeName).Return(nodeWithENIConfig, nil) - mock.MockK8sAPI.EXPECT().CreateCNINode(nodeWithENIConfig).Return(nil).Times(1) + mock.MockK8sAPI.EXPECT().CreateCNINode(nodeWithENIConfig, mock.Manager.clusterName).Return(nil).Times(1) mock.MockK8sAPI.EXPECT().GetENIConfig(eniConfigName).Return(nil, mockError) mock.MockK8sAPI.EXPECT().GetCNINode(types.NamespacedName{Name: nodeWithENIConfig.Name}).Return(&rcV1alpha1.CNINode{}, apierrors.NewNotFound(schema.GroupResource{Group: "vpcresources.k8s.aws", Resource: "1"}, "test")) diff --git a/pkg/provider/branch/provider.go b/pkg/provider/branch/provider.go index 0b525a33..2cc50756 100644 --- a/pkg/provider/branch/provider.go +++ b/pkg/provider/branch/provider.go @@ -76,7 +76,7 @@ var ( // NodeDeleteRequeueRequestDelay represents the time after which the resources belonging to a node will be cleaned // up after receiving the actual node delete event. - NodeDeleteRequeueRequestDelay = time.Minute * 5 + NodeDeleteRequeueRequestDelay = time.Minute * 1 prometheusRegistered = false ) @@ -175,8 +175,8 @@ func (b *branchENIProvider) InitResource(instance ec2.EC2Instance) error { } branchProviderOperationLatency.WithLabelValues(operationInitTrunk, "1").Observe(timeSinceMs(start)) - // Add the Trunk ENI to cache - if err := b.addTrunkToCache(nodeName, trunkENI); err != nil { + // Add the Trunk ENI to cache if it does not already exist + if err := b.addTrunkToCache(nodeName, trunkENI); err != nil && err != ErrTrunkExistInCache { branchProviderOperationsErrCount.WithLabelValues("add_trunk_to_cache").Inc() return err } @@ -239,7 +239,7 @@ func (b *branchENIProvider) DeleteNode(nodeName string) (ctrl.Result, error) { return ctrl.Result{}, fmt.Errorf("failed to find node %s", nodeName) } - trunkENI.DeleteAllBranchENIs() + trunkENI.DisassociateAllBranchENIs() b.removeTrunkFromCache(nodeName) b.log.Info("de-initialized resource provider successfully", "node name", nodeName) diff --git a/pkg/provider/branch/trunk/trunk.go b/pkg/provider/branch/trunk/trunk.go index 940bd3bb..f8bc8baa 100644 --- a/pkg/provider/branch/trunk/trunk.go +++ b/pkg/provider/branch/trunk/trunk.go @@ -33,6 +33,8 @@ import ( "github.com/go-logr/logr" "github.com/prometheus/client_golang/prometheus" v1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/util/wait" + "k8s.io/client-go/util/retry" "sigs.k8s.io/controller-runtime/pkg/metrics" ) @@ -93,8 +95,8 @@ type TrunkENI interface { Reconcile(pods []v1.Pod) bool // PushENIsToFrontOfDeleteQueue pushes the eni network interfaces to the front of the delete queue PushENIsToFrontOfDeleteQueue(*v1.Pod, []*ENIDetails) - // DeleteAllBranchENIs deletes all the branch ENI associated with the trunk and also clears the cool down queue - DeleteAllBranchENIs() + // DisassociateAllBranchENIs removes association of all the branch ENI with the trunk + DisassociateAllBranchENIs() // Introspect returns the state of the Trunk ENI Introspect() IntrospectResponse } @@ -117,6 +119,8 @@ type trunkENI struct { uidToBranchENIMap map[string][]*ENIDetails // deleteQueue is the queue of ENIs that are being cooled down before being deleted deleteQueue []*ENIDetails + // nodeName tag is the tag added to trunk and branch ENIs created on the node + nodeNameTag []*awsEC2.Tag } // PodENI is a json convertible structure that stores the Branch ENI details that can be @@ -169,6 +173,12 @@ func NewTrunkENI(logger logr.Logger, instance ec2.EC2Instance, helper api.EC2API ec2ApiHelper: helper, instance: instance, uidToBranchENIMap: make(map[string][]*ENIDetails), + nodeNameTag: []*awsEC2.Tag{ + { + Key: aws.String(config.NetworkInterfaceNodenameKey), + Value: aws.String(instance.Name()), + }, + }, } } @@ -221,7 +231,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.CurrentInstanceSecurityGroups(), nil, &freeIndex, &TrunkEniDescription, &InterfaceTypeTrunk, nil) + t.instance.CurrentInstanceSecurityGroups(), t.nodeNameTag, &freeIndex, &TrunkEniDescription, &InterfaceTypeTrunk, nil) if err != nil { trunkENIOperationsErrCount.WithLabelValues("create_trunk_eni").Inc() return err @@ -234,7 +244,7 @@ func (t *trunkENI) InitTrunk(instance ec2.EC2Instance, podList []v1.Pod) error { } // Get the list of branch ENIs - branchInterfaces, err := t.ec2ApiHelper.GetBranchNetworkInterface(&t.trunkENIId) + branchInterfaces, err := t.ec2ApiHelper.GetBranchNetworkInterface(&t.trunkENIId, aws.String(t.instance.SubnetID())) if err != nil { return err } @@ -374,6 +384,8 @@ func (t *trunkENI) CreateAndAssociateBranchENIs(pod *v1.Pod, securityGroups []st Value: &t.trunkENIId, }, } + // append the nodeName tag to add to branch ENIs + tags = append(tags, t.nodeNameTag...) // Create Branch ENI nwInterface, err = t.ec2ApiHelper.CreateNetworkInterface(&BranchEniDescription, aws.String(t.instance.SubnetID()), securityGroups, tags, nil, nil) @@ -425,29 +437,37 @@ func (t *trunkENI) CreateAndAssociateBranchENIs(pod *v1.Pod, securityGroups []st return newENIs, nil } -// DeleteAllBranchENIs deletes all the branch ENIs associated with the trunk and all the ENIs present in the cool down -// queue, this is the last API call to the the Trunk ENI before it is removed from cache -func (t *trunkENI) DeleteAllBranchENIs() { - // Delete all the branch used by the pod on this trunk ENI +// DisassociateAllBranchENIs removes association between all the branch ENIs with the trunk. The branch ENIs will be deleted by the node termination finalizer +// This is the last API call to the the Trunk ENI before it is removed from cache +func (t *trunkENI) DisassociateAllBranchENIs() { + // Disassociate all the branch used by the pod on this trunk ENI, it will be cleaned up by the finalizer routine // Since after this call, the trunk will be removed from cache. No need to clean up its branch map for _, podENIs := range t.uidToBranchENIMap { for _, eni := range podENIs { - err := t.deleteENI(eni) - if err != nil { - // Just log, if the ENI still exists it can be removed by the dangling ENI cleaner routine - t.log.Error(err, "failed to delete eni", "eni id", eni.ID) + err := retry.OnError( + wait.Backoff{ + Duration: time.Millisecond * 100, + Factor: 3.0, + Jitter: 0.1, + Steps: 7, + Cap: time.Second * 10, + }, + func(err error) bool { + if strings.Contains(err.Error(), ec2Errors.NotFoundAssociationID) { + // association is already deleted, do not retry + return false + } + return true + }, func() error { + return t.ec2ApiHelper.DisassociateTrunkInterface(&eni.AssociationID) + }, + ) + if err != nil && !strings.Contains(err.Error(), ec2Errors.NotFoundAssociationID) { + // Just log, if the ENI disassociation fails, it will be force deleted + t.log.Error(err, "failed to disassociate eni", "eni id", eni.ID) } } } - - // Delete all the branch ENI present in the cool down queue - for _, eni := range t.deleteQueue { - err := t.deleteENI(eni) - if err != nil { - // Just log, if the ENI still exists it can be removed by the dangling ENI cleaner routine - t.log.Error(err, "failed to delete eni", "eni id", eni.ID) - } - } } // DeleteBranchNetworkInterface deletes the branch network interface and returns an error in case of failure to delete diff --git a/pkg/provider/branch/trunk/trunk_test.go b/pkg/provider/branch/trunk/trunk_test.go index 438711a3..4b79f641 100644 --- a/pkg/provider/branch/trunk/trunk_test.go +++ b/pkg/provider/branch/trunk/trunk_test.go @@ -29,6 +29,7 @@ import ( "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/provider/branch/cooldown" "github.com/aws/aws-sdk-go/aws" + awsEC2 "github.com/aws/aws-sdk-go/service/ec2" awsEc2 "github.com/aws/aws-sdk-go/service/ec2" "github.com/golang/mock/gomock" "github.com/stretchr/testify/assert" @@ -158,6 +159,12 @@ var ( Status: aws.String(awsEc2.AttachmentStatusAttached), }, } + nodeNametag = []*awsEC2.Tag{ + { + Key: aws.String(config.NetworkInterfaceNodenameKey), + Value: aws.String(FakeInstance.Name()), + }, + } trunkIDTag = &awsEc2.Tag{ Key: aws.String(config.TrunkENIIDTag), @@ -255,11 +262,17 @@ func getMockTrunk() trunkENI { log: log, usedVlanIds: make([]bool, MaxAllocatableVlanIds), uidToBranchENIMap: map[string][]*ENIDetails{}, + nodeNameTag: []*awsEC2.Tag{ + { + Key: aws.String(config.NetworkInterfaceNodenameKey), + Value: aws.String(FakeInstance.Name()), + }, + }, } } func TestNewTrunkENI(t *testing.T) { - trunkENI := NewTrunkENI(zap.New(), nil, nil) + trunkENI := NewTrunkENI(zap.New(), FakeInstance, nil) assert.NotNil(t, trunkENI) } @@ -700,7 +713,7 @@ func TestTrunkENI_InitTrunk(t *testing.T) { f.mockEC2APIHelper.EXPECT().GetInstanceNetworkInterface(&InstanceId).Return([]*awsEc2.InstanceNetworkInterface{}, nil) f.mockInstance.EXPECT().GetHighestUnusedDeviceIndex().Return(freeIndex, nil) f.mockInstance.EXPECT().SubnetID().Return(SubnetId) - f.mockEC2APIHelper.EXPECT().CreateAndAttachNetworkInterface(&InstanceId, &SubnetId, SecurityGroups, nil, + f.mockEC2APIHelper.EXPECT().CreateAndAttachNetworkInterface(&InstanceId, &SubnetId, SecurityGroups, f.trunkENI.nodeNameTag, &freeIndex, &TrunkEniDescription, &InterfaceTypeTrunk, nil).Return(trunkInterface, nil) }, // Pass nil to set the instance to fields.mockInstance in the function later @@ -749,7 +762,8 @@ func TestTrunkENI_InitTrunk(t *testing.T) { f.mockInstance.EXPECT().InstanceID().Return(InstanceId) f.mockEC2APIHelper.EXPECT().GetInstanceNetworkInterface(&InstanceId).Return(instanceNwInterfaces, nil) f.mockEC2APIHelper.EXPECT().WaitForNetworkInterfaceStatusChange(&trunkId, awsEc2.AttachmentStatusAttached).Return(nil) - f.mockEC2APIHelper.EXPECT().GetBranchNetworkInterface(&trunkId).Return(branchInterfaces, nil) + f.mockInstance.EXPECT().SubnetID().Return(SubnetId) + f.mockEC2APIHelper.EXPECT().GetBranchNetworkInterface(&trunkId, &SubnetId).Return(branchInterfaces, nil) }, args: args{instance: FakeInstance, podList: []v1.Pod{*MockPod1, *MockPod2}}, wantErr: false, @@ -777,7 +791,8 @@ func TestTrunkENI_InitTrunk(t *testing.T) { f.mockInstance.EXPECT().InstanceID().Return(InstanceId) f.mockEC2APIHelper.EXPECT().GetInstanceNetworkInterface(&InstanceId).Return(instanceNwInterfaces, nil) f.mockEC2APIHelper.EXPECT().WaitForNetworkInterfaceStatusChange(&trunkId, awsEc2.AttachmentStatusAttached).Return(nil) - f.mockEC2APIHelper.EXPECT().GetBranchNetworkInterface(&trunkId).Return(branchInterfaces, nil) + f.mockInstance.EXPECT().SubnetID().Return(SubnetId) + f.mockEC2APIHelper.EXPECT().GetBranchNetworkInterface(&trunkId, &SubnetId).Return(branchInterfaces, nil) }, args: args{instance: FakeInstance, podList: []v1.Pod{*MockPod2}}, wantErr: false, @@ -829,23 +844,19 @@ func TestTrunkENI_InitTrunk(t *testing.T) { } } -// TestTrunkENI_DeleteAllBranchENIs tests all branch ENI associated with the trunk are deleted -func TestTrunkENI_DeleteAllBranchENIs(t *testing.T) { +// TestTrunkENI_DisassociateAllBranchENIs tests all branch ENI are disassociated with the trunk +func TestTrunkENI_DisassociateAllBranchENIs(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() trunkENI, mockEC2APIHelper, _ := getMockHelperInstanceAndTrunkObject(ctrl) trunkENI.uidToBranchENIMap[PodUID] = branchENIs1 trunkENI.uidToBranchENIMap[PodUID2] = branchENIs2 - 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(&MockAssociationID1).Return(nil) mockEC2APIHelper.EXPECT().DisassociateTrunkInterface(&MockAssociationID2).Return(nil) - mockEC2APIHelper.EXPECT().DeleteNetworkInterface(&Branch2Id).Return(nil) - trunkENI.DeleteAllBranchENIs() + trunkENI.DisassociateAllBranchENIs() } // TestTrunkENI_CreateAndAssociateBranchENIs test branch is created and associated with the trunk and valid eni details @@ -863,9 +874,9 @@ func TestTrunkENI_CreateAndAssociateBranchENIs(t *testing.T) { mockInstance.EXPECT().SubnetV6CidrBlock().Return(SubnetV6CidrBlock).Times(2) mockEC2APIHelper.EXPECT().CreateNetworkInterface(&BranchEniDescription, &SubnetId, SecurityGroups, - vlan1Tag, nil, nil).Return(BranchInterface1, nil) + append(vlan1Tag, trunkENI.nodeNameTag...), nil, nil).Return(BranchInterface1, nil) mockEC2APIHelper.EXPECT().AssociateBranchToTrunk(&trunkId, &Branch1Id, VlanId1).Return(mockAssociationOutput1, nil) - mockEC2APIHelper.EXPECT().CreateNetworkInterface(&BranchEniDescription, &SubnetId, SecurityGroups, vlan2Tag, + mockEC2APIHelper.EXPECT().CreateNetworkInterface(&BranchEniDescription, &SubnetId, SecurityGroups, append(vlan2Tag, trunkENI.nodeNameTag...), nil, nil).Return(BranchInterface2, nil) mockEC2APIHelper.EXPECT().AssociateBranchToTrunk(&trunkId, &Branch2Id, VlanId2).Return(mockAssociationOutput2, nil) @@ -897,10 +908,10 @@ func TestTrunkENI_CreateAndAssociateBranchENIs_InstanceSecurityGroup(t *testing. mockInstance.EXPECT().CurrentInstanceSecurityGroups().Return(InstanceSecurityGroup) mockEC2APIHelper.EXPECT().CreateNetworkInterface(&BranchEniDescription, &SubnetId, InstanceSecurityGroup, - vlan1Tag, nil, nil).Return(BranchInterface1, nil) + append(vlan1Tag, trunkENI.nodeNameTag...), nil, nil).Return(BranchInterface1, nil) mockEC2APIHelper.EXPECT().AssociateBranchToTrunk(&trunkId, &Branch1Id, VlanId1).Return(mockAssociationOutput1, nil) mockEC2APIHelper.EXPECT().CreateNetworkInterface(&BranchEniDescription, &SubnetId, InstanceSecurityGroup, - vlan2Tag, nil, nil).Return(BranchInterface2, nil) + append(vlan2Tag, trunkENI.nodeNameTag...), nil, nil).Return(BranchInterface2, nil) mockEC2APIHelper.EXPECT().AssociateBranchToTrunk(&trunkId, &Branch2Id, VlanId2).Return(mockAssociationOutput2, nil) eniDetails, err := trunkENI.CreateAndAssociateBranchENIs(MockPod2, []string{}, 2) @@ -931,10 +942,10 @@ func TestTrunkENI_CreateAndAssociateBranchENIs_ErrorAssociate(t *testing.T) { gomock.InOrder( mockEC2APIHelper.EXPECT().CreateNetworkInterface(&BranchEniDescription, &SubnetId, SecurityGroups, - vlan1Tag, nil, nil).Return(BranchInterface1, nil), + append(vlan1Tag, trunkENI.nodeNameTag...), nil, nil).Return(BranchInterface1, nil), mockEC2APIHelper.EXPECT().AssociateBranchToTrunk(&trunkId, &Branch1Id, VlanId1).Return(mockAssociationOutput1, nil), mockEC2APIHelper.EXPECT().CreateNetworkInterface(&BranchEniDescription, &SubnetId, SecurityGroups, - vlan2Tag, nil, nil).Return(BranchInterface2, nil), + append(vlan2Tag, trunkENI.nodeNameTag...), nil, nil).Return(BranchInterface2, nil), mockEC2APIHelper.EXPECT().AssociateBranchToTrunk(&trunkId, &Branch2Id, VlanId2).Return(nil, MockError), ) @@ -958,10 +969,10 @@ func TestTrunkENI_CreateAndAssociateBranchENIs_ErrorCreate(t *testing.T) { mockInstance.EXPECT().SubnetV6CidrBlock().Return(SubnetV6CidrBlock).Times(1) gomock.InOrder( - mockEC2APIHelper.EXPECT().CreateNetworkInterface(&BranchEniDescription, &SubnetId, SecurityGroups, vlan1Tag, + mockEC2APIHelper.EXPECT().CreateNetworkInterface(&BranchEniDescription, &SubnetId, SecurityGroups, append(vlan1Tag, trunkENI.nodeNameTag...), nil, nil).Return(BranchInterface1, nil), mockEC2APIHelper.EXPECT().AssociateBranchToTrunk(&trunkId, &Branch1Id, VlanId1).Return(mockAssociationOutput1, nil), - mockEC2APIHelper.EXPECT().CreateNetworkInterface(&BranchEniDescription, &SubnetId, SecurityGroups, vlan2Tag, + mockEC2APIHelper.EXPECT().CreateNetworkInterface(&BranchEniDescription, &SubnetId, SecurityGroups, append(vlan2Tag, trunkENI.nodeNameTag...), nil, nil).Return(nil, MockError), ) diff --git a/pkg/utils/events.go b/pkg/utils/events.go index 54efd250..bc389e5e 100644 --- a/pkg/utils/events.go +++ b/pkg/utils/events.go @@ -23,12 +23,13 @@ import ( const ( UnsupportedInstanceTypeReason = "Unsupported" InsufficientCidrBlocksReason = "InsufficientCidrBlocks" - CNINodeCreatedReason = "CNINodeCreation" NodeTrunkInitiatedReason = "NodeTrunkInitiated" NodeTrunkFailedInitializationReason = "NodeTrunkFailedInit" EniConfigNameNotFoundReason = "EniConfigNameNotFound" VersionNotice = "ControllerVersionNotice" BranchENICoolDownUpdateReason = "BranchENICoolDownPeriodUpdated" + CNINodeDeleteFailed = "CNINodeDeletionFailed" + CNINodeCreateFailed = "CNINodeCreationFailed" ) func SendNodeEventWithNodeName(client k8s.K8sWrapper, nodeName, reason, msg, eventType string, logger logr.Logger) { diff --git a/pkg/utils/set.go b/pkg/utils/set.go index ab7a037e..326ffa6d 100644 --- a/pkg/utils/set.go +++ b/pkg/utils/set.go @@ -13,6 +13,10 @@ package utils +import ( + "github.com/aws/aws-sdk-go/service/ec2" +) + // Difference returns a-b, elements present in a and not in b func Difference[T comparable](a, b []T) (diff []T) { m := make(map[T]struct{}) @@ -35,3 +39,11 @@ func GetKeyValSlice(m map[string]string) (key []string, val []string) { } return } + +func GetTagKeyValueMap(tagSet []*ec2.Tag) map[string]string { + m := make(map[string]string) + for _, tag := range tagSet { + m[*tag.Key] = *tag.Value + } + return m +}