rework vpc cni envvar logic to drop the forced keys

Signed-off-by: Luther Monson <[email protected]>

Author: luthermonson

docs/book/src/topics/eks/pod-networking.md

@@ -7,8 +7,26 @@ When creating a EKS cluster the Amazon VPC CNI will be used by default for Pod N
 ## Using the VPC CNI Addon
 You can use an explicit version of the Amazon VPC CNI by using the **vpc-cni** EKS addon. See the [addons](./addons.md) documentation for further details of how to use addons.
 
+## Using Custom VPC CNI Configuration
+If your use case demands [custom networking](https://docs.aws.amazon.com/eks/latest/userguide/cni-custom-network.html) VPC CNI configuration you might already be familiar with the [helm chart](https://github.com/aws/amazon-vpc-cni-k8s) which helps with the process. This gives you access to ENI Configs and you can set Environment Variables on the `aws-node` DaemonSet where the VPC CNI runs. CAPA is able to tune the same DaemonSet through Kubernetes.
 
-## Increase node pod limit
+The following example shows how to turn on custom network config and set a [label definition](https://github.com/aws/amazon-vpc-cni-k8s#eni_config_label_def).
+
+```yaml
+kind: AWSManagedControlPlane
+apiVersion: controlplane.cluster.x-k8s.io/v1beta1
+metadata:
+  name: "capi-managed-test-control-plane"
+spec:
+  vpcCni:
+    env:
+    - name: AWS_VPC_K8S_CNI_CUSTOM_NETWORK_CFG
+      value: "true" 
+    - name: ENABLE_PREFIX_DELEGATION
+      value: "true"
+```
+
+### Increase node pod limit
 You can increase the pod limit per-node as [per the upstream AWS documentation](https://aws.amazon.com/blogs/containers/amazon-vpc-cni-increases-pods-per-node-limits/). You'll need to enable the `vpc-cni` plugin addon on your EKS cluster as well as enable prefix assignment mode through the `ENABLE_PREFIX_DELEGATION` environment variable.
 
 ```yaml
@@ -19,6 +37,8 @@ metadata:
 spec:
   vpcCni:
     env:
+    - name: AWS_VPC_K8S_CNI_CUSTOM_NETWORK_CFG
+      value: "true" 
     - name: ENABLE_PREFIX_DELEGATION
       value: "true"
   addons:

pkg/cloud/services/awsnode/cni.go

@@ -72,7 +72,6 @@ func (s *Service) ReconcileCNI(ctx context.Context) error {
 		for i := range ds.Spec.Template.Spec.Containers {
 			container := &ds.Spec.Template.Spec.Containers[i]
 			if container.Name == "aws-node" {
-				container.Env = s.filterEnv(container.Env)
 				container.Env, needsUpdate = s.applyUserProvidedEnvironmentProperties(container.Env)
 			}
 		}
@@ -161,22 +160,7 @@ func (s *Service) ReconcileCNI(ctx context.Context) error {
 		}
 	}
 
-	s.scope.Info("updating containers", "cluster", klog.KRef(s.scope.Namespace(), s.scope.Name()))
-	for i := range ds.Spec.Template.Spec.Containers {
-		if ds.Spec.Template.Spec.Containers[i].Name == "aws-node" {
-			ds.Spec.Template.Spec.Containers[i].Env = append(s.filterEnv(ds.Spec.Template.Spec.Containers[i].Env),
-				corev1.EnvVar{
-					Name:  "AWS_VPC_K8S_CNI_CUSTOM_NETWORK_CFG",
-					Value: "true",
-				},
-				corev1.EnvVar{
-					Name:  "ENI_CONFIG_LABEL_DEF",
-					Value: "failure-domain.beta.kubernetes.io/zone",
-				},
-			)
-		}
-	}
-
+	s.scope.Info("updating containers", "cluster-name", s.scope.Name(), "cluster-namespace", s.scope.Namespace())
 	return remoteClient.Update(ctx, &ds, &client.UpdateOptions{})
 }
 
@@ -196,18 +180,6 @@ func (s *Service) getSecurityGroups() ([]string, error) {
 	return sgs, nil
 }
 
-func (s *Service) filterEnv(env []corev1.EnvVar) []corev1.EnvVar {
-	var i int
-	for _, e := range env {
-		if e.Name == "ENI_CONFIG_LABEL_DEF" || e.Name == "AWS_VPC_K8S_CNI_CUSTOM_NETWORK_CFG" {
-			continue
-		}
-		env[i] = e
-		i++
-	}
-	return env[:i]
-}
-
 // applyUserProvidedEnvironmentProperties takes a container environment and applies user provided values to it.
 func (s *Service) applyUserProvidedEnvironmentProperties(containerEnv []corev1.EnvVar) ([]corev1.EnvVar, bool) {
 	var (

pkg/cloud/services/awsnode/cni_test.go

@@ -55,16 +55,16 @@ func TestReconcileCniVpcCniValues(t *testing.T) {
 					Kind: "DaemonSet",
 				},
 				ObjectMeta: metav1.ObjectMeta{
-					Name:      "aws-node",
-					Namespace: "kube-system",
+					Name:      awsNodeName,
+					Namespace: awsNodeNamespace,
 				},
 				Spec: v1.DaemonSetSpec{
 					Template: corev1.PodTemplateSpec{
 						ObjectMeta: metav1.ObjectMeta{},
 						Spec: corev1.PodSpec{
 							Containers: []corev1.Container{
 								{
-									Name: "aws-node",
+									Name: awsNodeName,
 									Env:  []corev1.EnvVar{},
 								},
 							},
@@ -98,16 +98,16 @@ func TestReconcileCniVpcCniValues(t *testing.T) {
 					Kind: "DaemonSet",
 				},
 				ObjectMeta: metav1.ObjectMeta{
-					Name:      "aws-node",
-					Namespace: "kube-system",
+					Name:      awsNodeName,
+					Namespace: awsNodeNamespace,
 				},
 				Spec: v1.DaemonSetSpec{
 					Template: corev1.PodTemplateSpec{
 						ObjectMeta: metav1.ObjectMeta{},
 						Spec: corev1.PodSpec{
 							Containers: []corev1.Container{
 								{
-									Name: "aws-node",
+									Name: awsNodeName,
 									Env:  []corev1.EnvVar{},
 								},
 							},
@@ -141,16 +141,16 @@ func TestReconcileCniVpcCniValues(t *testing.T) {
 					Kind: "DaemonSet",
 				},
 				ObjectMeta: metav1.ObjectMeta{
-					Name:      "aws-node",
-					Namespace: "kube-system",
+					Name:      awsNodeName,
+					Namespace: awsNodeNamespace,
 				},
 				Spec: v1.DaemonSetSpec{
 					Template: corev1.PodTemplateSpec{
 						ObjectMeta: metav1.ObjectMeta{},
 						Spec: corev1.PodSpec{
 							Containers: []corev1.Container{
 								{
-									Name: "aws-node",
+									Name: awsNodeName,
 									Env: []corev1.EnvVar{
 										{
 											Name:  "NAME1",
@@ -184,7 +184,7 @@ func TestReconcileCniVpcCniValues(t *testing.T) {
 		},
 	}
 	for _, tc := range tests {
-		t.Run(tc.name, func(t *testing.T) {
+		t.Run(tc.name+" without secondary cidr", func(t *testing.T) {
 			mockCtrl := gomock.NewController(t)
 			defer mockCtrl.Finish()
 			g := NewWithT(t)
@@ -206,92 +206,37 @@ func TestReconcileCniVpcCniValues(t *testing.T) {
 			g.Expect(ds.Spec.Template.Spec.Containers[0].Env).To(ConsistOf(tc.consistsOf))
 		})
 	}
-}
 
-func TestReconcileCniVpcCniValuesWithSecondaryCidrBlock(t *testing.T) {
-	mockCtrl := gomock.NewController(t)
-	defer mockCtrl.Finish()
-	g := NewWithT(t)
-	daemonSet := &v1.DaemonSet{
-		TypeMeta: metav1.TypeMeta{
-			Kind: "DaemonSet",
-		},
-		ObjectMeta: metav1.ObjectMeta{
-			Name:      "aws-node",
-			Namespace: "kube-system",
-		},
-		Spec: v1.DaemonSetSpec{
-			Template: corev1.PodTemplateSpec{
-				ObjectMeta: metav1.ObjectMeta{},
-				Spec: corev1.PodSpec{
-					Containers: []corev1.Container{
-						{
-							Name: "aws-node",
-							Env: []corev1.EnvVar{
-								{
-									Name:  "NAME1",
-									Value: "OVERWRITE",
-								},
-								{
-									Name:  "NAME3",
-									Value: "VALUE3",
-								},
-							},
-						},
+	for _, tc := range tests {
+		t.Run(tc.name+" with secondary cidr", func(t *testing.T) {
+			mockCtrl := gomock.NewController(t)
+			defer mockCtrl.Finish()
+			g := NewWithT(t)
+			mockClient := &cachingClient{
+				getValue: tc.daemonSet,
+			}
+			m := &mockScope{
+				client:             mockClient,
+				cni:                tc.cniValues,
+				secondaryCidrBlock: aws.String("100.0.0.1/20"),
+				securityGroups: map[infrav1.SecurityGroupRole]infrav1.SecurityGroup{
+					"node": {
+						ID:   "sg-1234",
+						Name: "node",
 					},
 				},
-			},
-		},
-	}
-	values := ekscontrolplanev1.VpcCni{
-		Env: []corev1.EnvVar{
-			{
-				Name:  "NAME1",
-				Value: "VALUE1",
-			},
-		},
-	}
-	mockClient := &cachingClient{
-		getValue: daemonSet,
-	}
-	m := &mockScope{
-		client:             mockClient,
-		cni:                values,
-		secondaryCidrBlock: aws.String("100.0.0.1/20"),
-		securityGroups: map[infrav1.SecurityGroupRole]infrav1.SecurityGroup{
-			"node": {
-				ID:   "sg-1234",
-				Name: "node",
-			},
-		},
-		subnets: []infrav1.SubnetSpec{},
-	}
-	s := NewService(m)
+			}
+			s := NewService(m)
 
-	err := s.ReconcileCNI(context.Background())
-	g.Expect(err).NotTo(HaveOccurred())
-	g.Expect(mockClient.updateChain).NotTo(BeEmpty())
-	ds, ok := mockClient.updateChain[0].(*v1.DaemonSet)
-	g.Expect(ok).To(BeTrue())
-	g.Expect(ds.Spec.Template.Spec.Containers).NotTo(BeEmpty())
-	g.Expect(ds.Spec.Template.Spec.Containers[0].Env).To(ConsistOf([]corev1.EnvVar{
-		{
-			Name:  "NAME1",
-			Value: "VALUE1",
-		},
-		{
-			Name:  "NAME3",
-			Value: "VALUE3",
-		},
-		{
-			Name:  "AWS_VPC_K8S_CNI_CUSTOM_NETWORK_CFG",
-			Value: "true",
-		},
-		{
-			Name:  "ENI_CONFIG_LABEL_DEF",
-			Value: "failure-domain.beta.kubernetes.io/zone",
-		},
-	}))
+			err := s.ReconcileCNI(context.Background())
+			g.Expect(err).NotTo(HaveOccurred())
+			g.Expect(mockClient.updateChain).NotTo(BeEmpty())
+			ds, ok := mockClient.updateChain[0].(*v1.DaemonSet)
+			g.Expect(ok).To(BeTrue())
+			g.Expect(ds.Spec.Template.Spec.Containers).NotTo(BeEmpty())
+			g.Expect(ds.Spec.Template.Spec.Containers[0].Env).To(ConsistOf(tc.consistsOf))
+		})
+	}
 }
 
 type cachingClient struct {