Adding Network Policy test #395

jaydeokar · 2023-12-04T20:55:30Z

Issue #, if available:
N/A
Description of changes:
go test -v --kubeconfig=$KUBECONFIG
This adds a basic test for aws-vpc-cni Network Policy. The test does the following -

Installs the latest vpc-cni using helm chart and enables network policy support on the cluster
Creates three servers
- Verifies all are reachable when no network policy is applied
- Verifies traffic is blocked when network policy is applied (which proves the bpf program was loaded)

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

e2e2/internal/framework_extensions/helm.go

cartermckinnon · 2023-12-05T00:19:56Z

e2e2/test/cases/netpol/main_test.go

+			}
+
+			return ctx, nil
+		},


Can you add waiter(s) here so that we don't try to run the test cases until your pre-reqs are met?

The deployment function here uses wait for the resource to be ready
https://github.com/aws/aws-k8s-tester/pull/395/files#diff-4082a58579cf8719172663a0888661788612744805944b7e28a64de150aec95dR226

e2e2/test/cases/netpol/main_test.go

cartermckinnon · 2023-12-05T00:24:15Z

e2e2/test/cases/netpol/main_test.go

+	for _, resource := range resources {
+		if _, err := fwext.ResourceExists(resource, releaseNamespace, config.KubeconfigFile()); err == nil {
+			log.Printf("Patching resource: %s labels and annotations..", resource)
+			fwext.Label(resource, "app.kubernetes.io/managed-by=Helm", releaseNamespace, config.KubeconfigFile())
+			fwext.Annotate(resource, fmt.Sprintf("meta.helm.sh/release-name=%s", releaseName), releaseNamespace, config.KubeconfigFile())
+			fwext.Annotate(resource, fmt.Sprintf("meta.helm.sh/release-namespace=%s", releaseNamespace), releaseNamespace, config.KubeconfigFile())
+		} else {
+			log.Printf("Resource %s does not exist, skipping patching labels/annotations..", resource)
+		}
+	}


What's the goal of this?

So vpc-cni is installed by default on a cluster and we need to import the resource into the helm chart (apply annotations/labels). More details here -
https://github.com/aws/eks-charts/tree/master/stable/aws-vpc-cni#adopting-the-existing-aws-node-resources-in-an-eks-cluster

I've updated this to delete the resources instead of patching for labels/annotations

cartermckinnon · 2023-12-05T00:26:01Z

e2e2/test/cases/netpol/main_test.go

+	wait.For(conditions.New(config.Client().Resources()).ResourceMatch(cniDS, func(object k8s.Object) bool {
+		d := object.(*appsv1.DaemonSet)
+		status := false
+		if d.Status.DesiredNumberScheduled == d.Status.NumberReady {


Can you use the existing utility in the framework for your daemonset checks?

https://github.com/kubernetes-sigs/e2e-framework/blob/c61338a3773c2b64550d0e46037bfe541fa54645/klient/wait/conditions/conditions.go#L314

Seems like we need a e2e-framework release to use this utility. I've added a TODO to update to use this when we have a new version of the framework

we could pull in the latest changes with a go v0.0.0-$COMMIT version, but you could also use the one I've put in the frameworkextensions package: https://github.com/aws/aws-k8s-tester/blob/main/e2e2/internal/framework_extensions/conditions.go#L32

e2e2/test/cases/netpol/main_test.go

cartermckinnon

My biggest concern is the use of the VPC CNI helm chart, which could change between executions of this test case. I assume you don't plan to use this test case to qualify the network policy agent itself (because it's using the prod helm chart), so it'd be more stable to at least pin a specific version of the chart.

cartermckinnon · 2023-12-12T23:31:20Z

Dockerfile.kubetest2

@@ -17,7 +17,7 @@ COPY kubetest2/ .
 RUN go install ./...
 WORKDIR $GOPATH/github.com/aws/aws-k8s-tester/e2e2
 COPY e2e2/ .
-RUN go test -c ./test/cases/nvidia -o $GOPATH/bin/e2e-nvidia
+RUN go test -c ./test/cases/nvidia -o $GOPATH/bin/e2e-nvidia && go test -c ./test/cases/netpol -o $GOPATH/bin/e2e-netpol


Can you move this to a separate RUN for readability? We're not too concerned about minimizing the number of layers in this image

(or add a line break)

cartermckinnon · 2023-12-12T23:32:52Z

Dockerfile.kubetest2

@@ -51,4 +51,5 @@ ENV PATH=$PATH:/info
 RUN cp kubernetes-version.txt /info/
 RUN mv kubernetes/*/bin/* /bin/
 RUN rm -rf /workdir
+RUN curl -fsSL -o get_helm.sh https://raw.githubusercontent.com/helm/helm/main/scripts/get-helm-3 && chmod 700 get_helm.sh && VERIFY_CHECKSUM=false ./get_helm.sh && rm -rf ./get_helm.sh


line breaks for readability

Suggested change

RUN curl -fsSL -o get_helm.sh https://raw.githubusercontent.com/helm/helm/main/scripts/get-helm-3 && chmod 700 get_helm.sh && VERIFY_CHECKSUM=false ./get_helm.sh && rm -rf ./get_helm.sh

RUN curl -fsSL -o get_helm.sh https://raw.githubusercontent.com/helm/helm/main/scripts/get-helm-3 && \

chmod 700 get_helm.sh && \

VERIFY_CHECKSUM=false ./get_helm.sh && \

rm -rf ./get_helm.sh

cartermckinnon · 2023-12-12T23:36:36Z

e2e2/test/cases/netpol/main_test.go

+	client, err := config.NewClient()
+
+	if err != nil {
+		log.Print("Failed to get client")


Instead of logging before returning errors throughout this function, you could use errors.Wrap if the added context is really valuable

cartermckinnon · 2023-12-12T23:38:20Z

e2e2/test/cases/netpol/main_test.go

+	// Delete Cluster role
+	client.Resources().Delete(ctx, &rbac.ClusterRole{ObjectMeta: metav1.ObjectMeta{Name: "aws-node", Namespace: releaseNamespace}})
+
+	// Delete Cluster-Role Binding
+	client.Resources().Delete(ctx, &rbac.ClusterRoleBinding{ObjectMeta: metav1.ObjectMeta{Name: "aws-node", Namespace: releaseNamespace}})
+
+	// Delete ServiceAccount
+	client.Resources().Delete(ctx, &v1.ServiceAccount{ObjectMeta: metav1.ObjectMeta{Name: "aws-node", Namespace: releaseNamespace}})
+
+	// Delete Daemonset
+	client.Resources().Delete(ctx, &appsv1.DaemonSet{ObjectMeta: metav1.ObjectMeta{Name: "aws-node", Namespace: releaseNamespace}})
+
+	// // Delete Configmap
+	client.Resources().Delete(ctx, &v1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "amazon-vpc-cni", Namespace: releaseNamespace}})


This seems fragile to me, we'll have to keep this in sync with the resources that the managed addon for VPC CNI provides

Yes, you are right. I don't think we have an easier way to import the resources into the helm chart at the moment.
I also considered using eks managed addons to avoid this, but that would mean adding a extra dependency which doesn't seem right here.

I am going to try few more things to see if that helps remove this.
That being said, we have not added new resources to this chart in a while..

Found a way to not keep this resource mapping by using helm template followed by applying the rendered manifest file. However we would need to use another go library for helm client as the inbuilt helm package (e2e-framework) only validates if the chart can be rendered.

jaydeokar · 2023-12-13T00:45:04Z

My biggest concern is the use of the VPC CNI helm chart, which could change between executions of this test case. I assume you don't plan to use this test case to qualify the network policy agent itself (because it's using the prod helm chart), so it'd be more stable to at least pin a specific version of the chart.

Yes, this tests is not to qualify Network Policy agent. The main goal of this is to verify ebpf programs gets loaded on the node whenever a new AMI is being released (mostly to catch issues when there is a kernel version update).. I explicitly did not pin this to a specific version of helm chart because we need to ensure only the latest versions of network policy agent is compatible

cartermckinnon · 2024-01-12T08:30:21Z

Thanks for all the work on this @jaydeokar !

jaydeokar force-pushed the netpol_test branch 2 times, most recently from 44dfb0c to 2aba942 Compare December 4, 2023 22:29

cartermckinnon reviewed Dec 5, 2023

View reviewed changes

e2e2/internal/framework_extensions/helm.go Outdated Show resolved Hide resolved

cartermckinnon reviewed Dec 5, 2023

View reviewed changes

e2e2/internal/framework_extensions/helm.go Outdated Show resolved Hide resolved

cartermckinnon reviewed Dec 5, 2023

View reviewed changes

e2e2/internal/framework_extensions/helm.go Outdated Show resolved Hide resolved

cartermckinnon reviewed Dec 5, 2023

View reviewed changes

e2e2/test/cases/netpol/main_test.go Outdated Show resolved Hide resolved

cartermckinnon reviewed Dec 5, 2023

View reviewed changes

e2e2/test/cases/netpol/main_test.go Outdated Show resolved Hide resolved

jaydeokar force-pushed the netpol_test branch from 2aba942 to 93eabe3 Compare December 5, 2023 21:42

jaydeokar marked this pull request as ready for review December 5, 2023 21:50

jaydeokar force-pushed the netpol_test branch from 93eabe3 to 6cdd07b Compare December 5, 2023 21:54

cartermckinnon requested changes Dec 12, 2023

View reviewed changes

jaydeokar force-pushed the netpol_test branch from 6cdd07b to 04978e2 Compare December 13, 2023 18:43

Adding Network Policy test

bd5dced

jaydeokar force-pushed the netpol_test branch 2 times, most recently from b441367 to 869e422 Compare January 4, 2024 19:38

Using eks addons to upgrade and downgrade vpc-cni

826cf6a

jaydeokar force-pushed the netpol_test branch from 869e422 to 826cf6a Compare January 4, 2024 20:45

jaydeokar requested a review from cartermckinnon January 8, 2024 18:26

cartermckinnon approved these changes Jan 12, 2024

View reviewed changes

cartermckinnon merged commit fc54f0d into aws:main Jan 12, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding Network Policy test #395

Adding Network Policy test #395

jaydeokar commented Dec 4, 2023 •

edited

Loading

cartermckinnon Dec 5, 2023

jaydeokar Dec 5, 2023

cartermckinnon Dec 5, 2023

jaydeokar Dec 5, 2023

cartermckinnon Dec 5, 2023

jaydeokar Dec 5, 2023

cartermckinnon Dec 12, 2023

cartermckinnon left a comment

cartermckinnon Dec 12, 2023 •

edited

Loading

cartermckinnon Dec 12, 2023

cartermckinnon Dec 12, 2023

cartermckinnon Dec 12, 2023

jaydeokar Dec 13, 2023

jaydeokar Dec 15, 2023

jaydeokar commented Dec 13, 2023

cartermckinnon commented Jan 12, 2024

-RUN curl -fsSL -o get_helm.sh https://raw.githubusercontent.com/helm/helm/main/scripts/get-helm-3 && chmod 700 get_helm.sh && VERIFY_CHECKSUM=false ./get_helm.sh && rm -rf ./get_helm.sh
+RUN curl -fsSL -o get_helm.sh https://raw.githubusercontent.com/helm/helm/main/scripts/get-helm-3 && \
+    chmod 700 get_helm.sh && \
+    VERIFY_CHECKSUM=false ./get_helm.sh && \
+    rm -rf ./get_helm.sh

Adding Network Policy test #395

Adding Network Policy test #395

Conversation

jaydeokar commented Dec 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cartermckinnon left a comment

Choose a reason for hiding this comment

cartermckinnon Dec 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaydeokar commented Dec 13, 2023

cartermckinnon commented Jan 12, 2024

jaydeokar commented Dec 4, 2023 •

edited

Loading

cartermckinnon Dec 12, 2023 •

edited

Loading