Knob to disable/enable leaked eni cleanup #1641

jayanthvn · 2021-09-28T07:06:50Z

What type of PR is this?
Enhancement

Which issue does this PR fix:
N/A

What does this PR do / Why do we need it:
This adds 2 additional tags for ENIs allocated by aws-node -

    1. kubernetes.io/cluster/<cluster-name>: owned
    2. eks:eni:owner: amazon-vpc-cni

Also added a knob to disable leaked ENI collection.

If an issue # is not available please add repro steps and logs from IPAMD/CNI showing the issue:
n/a

Testing done on this change:

Yes

UTs

PASS
coverage: 100.0% of statements
ok  	github.com/aws/amazon-vpc-cni-k8s/pkg/utils/retry	0.003s	coverage: 100.0% of statements
?   	github.com/aws/amazon-vpc-cni-k8s/pkg/utils/ttime	[no test files]
?   	github.com/aws/amazon-vpc-cni-k8s/pkg/utils/ttime/mocks	[no test files]
?   	github.com/aws/amazon-vpc-cni-k8s/pkg/version	[no test files]

Automation added to e2e:

Will add as a follow up PR

Will this break upgrades or downgrades. Has updating a running cluster been tested?:
No

Does this change require updates to the CNI daemonset config files to work?:

No

Does this PR introduce any user-facing change?:

2 additional tags will be created on ENIs allocated by aws-node

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

achevuru · 2021-09-28T07:17:50Z

pkg/awsutils/awsutils.go

@@ -411,7 +416,7 @@ func New(useCustomNetworking, disableENIProvisioning, v4Enabled, v6Enabled bool)
 	}

 	// Clean up leaked ENIs in the background
-	if !disableENIProvisioning {
+	if !disableENIProvisioning && !disableLeakedENICollection {


Thinking about it..Why don't we disable this for v6 completely since v6 support in PD mode will not be creating any additional ENIs? We only have/need Primary ENI in v6 PD mode..

Yeah we can. Only catch is if they moved from v4 to v6 cluster and there are leaked ENIs. Maybe should be fine?

So, v6 cluster will have to be a brand new cluster and more importantly the proposed IPv6 IAM policy for an IPv6 cluster will not have permissions to delete an ENI, so it is not going to be of any use.

Say in future if we support custom networking with v6 then we might still need this logic.

Sure, we might but we can probably remove the v6 check at that point along with v6 specific checks we placed around custom nw code. Problem I see with this right now is the ipamd logs might be filled with 403s(Unauthorized) every hr when the aws-node pod attempts to delete a leaked ENI.

Yeah makes sense to disable it for now until we have actual support for custom networking

achevuru · 2022-05-20T19:51:07Z

pkg/awsutils/awsutils.go

@@ -54,6 +54,11 @@ const (
 	eniClusterTagKey        = "cluster.k8s.amazonaws.com/name"
 	additionalEniTagsEnvVar = "ADDITIONAL_ENI_TAGS"
 	reservedTagKeyPrefix    = "k8s.amazonaws.com"
+	clusterNameTagKeyFormat = "kubernetes.io/cluster/%s"


We already add a cluster name tag -

amazon-vpc-cni-k8s/pkg/awsutils/awsutils.go

Line 837 in 88c1223

tags[eniClusterTagKey] = cache.clusterName

. We use this for our Cluster specific IAM policy right now. MAO will also populate this env variable by default going forward. Do we still need another tag for cluster name?

github-actions · 2022-07-20T00:16:04Z