initial commit for e2e test for #6229 #6695

hkiiita · 2024-09-26T22:08:11Z

This is an initial commit for e2e test regarding #6229

Signed-off-by: Hemant <[email protected]>

Dyanngg

Some initial comments, will continue with reviewing later

Dyanngg · 2024-09-26T22:27:33Z

test/e2e/fqdn_dns_cache_test.go

@@ -0,0 +1,459 @@
+package e2e
+
+import (


Please reorganize input into groups: 1. go native imports 2. external libraries 2. antrea packages. Refer to any other go file in Antrea on how those are organized

Dyanngg · 2024-09-26T22:29:06Z

test/e2e/fqdn_dns_cache_test.go

+15) curl the FQDN again with IP , simulating usage of cache -- and it must fail with no connectivity.
+*/
+
+func TestFQDNPolicyWithCachedDNS(t *testing.T) {


This function is way too long and is not very readable. Please create smaller functions (e.g. for step 1-4, call it configureAntreaForCachedDNSTests)

Dyanngg · 2024-09-26T22:29:49Z

test/e2e/fqdn_dns_cache_test.go

+		t.Fatalf("Error when setting up test: %v", err)
+	}
+
+	testFqdn := "nginx-test-pod.lfx.test"


Create constant for this

Dyanngg · 2024-09-26T22:30:35Z

test/e2e/fqdn_dns_cache_test.go

+	ipFamily := v1.IPv4Protocol
+
+	// Create the service .
+	//TODO: Should the names be put up as constants instead of direct strings here?


Yes, also udpPort = 53

Dyanngg · 2024-09-26T22:32:45Z

test/e2e/fqdn_dns_cache_test.go

+	//TODO: Check for IPv6 ?
+	ipFamily := v1.IPv4Protocol
+
+	// Create the service .


General comment: please check for grammar in comments, and also refrain from adding comments that are self-explanatory based on the code below, like create the service or print the IP.

Dyanngg · 2024-09-26T22:41:58Z

test/e2e/fqdn_dns_cache_test.go

+		t.Fatalf("failed to unmarshal Agent config from ConfigMap: %v", err)
+	}
+	require.NoError(t, err)
+


L73 to L108 should be a util function of its own. Also the second read as of now is completely redundant: it does not even verify that the dns server value is set to the desired one

Dyanngg · 2024-09-26T22:45:46Z

test/e2e/fqdn_dns_cache_test.go

+	t.Logf("dns server value set to %+v in antrea \n", agentConfChanged.DNSServerOverride)
+
+	// Set up nginx server
+	nginxConfig := `events {}


Why do we need this? Is there a place later in the tests where we verify the output of curl commands matches pod name?

Dyanngg · 2024-09-26T22:49:44Z

test/e2e/k8s_util.go

+							},
+							Env: []v1.EnvVar{
+								{
+									Name: "HOSTNAME",


Is there a reason why we have to create nginx deployments in this tests instead of the agnhost pods we use right now? I understand that with this configuration it can output pod names, but is it needed?

Dyanngg · 2024-09-26T22:50:39Z

test/e2e/k8s_util.go

@@ -18,6 +18,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	v12 "k8s.io/api/rbac/v1"


Suggested change

v12 "k8s.io/api/rbac/v1"

rbacv1 "k8s.io/api/rbac/v1"

Dyanngg · 2024-09-26T22:52:51Z

test/e2e/fqdn_dns_cache_test.go

+			},
+		},
+	}
+	// TODO: Delete role on teardown.


this is usually done with defer statements in the main test function

antoninbas · 2024-09-26T22:56:31Z

test/e2e/framework.go

 		return err
 	}
+	_, err = data.PodWaitFor(10*time.Second, podObj.Name, podObj.Namespace, func(pod2 *corev1.Pod) (bool, error) {


You have some linting errors. It is a good idea to always run make golangci locally before pushing, to check for errors.

antoninbas · 2024-09-26T22:57:13Z

test/e2e/framework.go

 		return err
 	}
+	_, err = data.PodWaitFor(10*time.Second, podObj.Name, podObj.Namespace, func(pod2 *corev1.Pod) (bool, error) {


It is unlikely that modifying this function is a very good idea, but if you want to propose a change to it, it should probably be in a separate PR

antoninbas · 2024-09-26T23:00:53Z

test/e2e/k8s_util.go

@@ -332,6 +334,68 @@ func decidePingProbeResult(stdout string, probeNum int) PodConnectivityMark {
 	}
 	return Error
 }
+func (k *KubernetesUtils) digDnSCustom(


This function can probably be greatly simplified by using the +short dig option:

C02F16ASMD6R:antrea abas$ dig @8.8.8.8 www.google.com ; <<>> DiG 9.10.6 <<>> @8.8.8.8 www.google.com ; (1 server found) ;; global options: +cmd ;; Got answer: ;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 64670 ;; flags: qr rd ra; QUERY: 1, ANSWER: 1, AUTHORITY: 0, ADDITIONAL: 1 ;; OPT PSEUDOSECTION: ; EDNS: version: 0, flags:; udp: 512 ;; QUESTION SECTION: ;www.google.com. IN A ;; ANSWER SECTION: www.google.com. 298 IN A 142.251.46.196 ;; Query time: 110 msec ;; SERVER: 8.8.8.8#53(8.8.8.8) ;; WHEN: Thu Sep 26 16:00:18 PDT 2024 ;; MSG SIZE rcvd: 59 C02F16ASMD6R:antrea abas$ dig @8.8.8.8 +short www.google.com 142.251.46.196

antoninbas · 2024-09-26T23:04:24Z

test/e2e/fqdn_dns_cache_test.go

+	dnsDeploymentLabels := map[string]string{
+		"app": "custom-dns",
+	}
+	dnsDeploymentObj, err := data.CreateCustomDnsDeployment("custom-dns-deployment", data.testNamespace, customDNSconfigMapObject.Name, sa.Name, dnsDeploymentLabels, 1)


let's create a single Pod here, we don't need to create a Deployment

antoninbas · 2024-09-26T23:05:38Z

test/e2e/fqdn_dns_cache_test.go

+	}
+	require.NoError(t, err)
+
+	clusterRoleSpec := &v12.ClusterRole{


all of this should not be necessary for out use case: we don't need to monitor K8s resources, and you should be able to just remove the kubernetes entry from your custom CoreDNS configuration?

…and to use short flag and added annotation update for dns pod to attempt to make configmap changes reflect early. Signed-off-by: Hemant <[email protected]>

antoninbas · 2024-09-27T22:12:29Z

test/e2e/k8s_util.go

+		digCmd,
+	}
+	fmt.Printf("Running: kubectl exec %s -c %s -n %s -- %s", pod.Name, pod.Spec.Containers[0].Name, pod.Namespace, strings.Join(cmd, " "))
+	stdout, stderr, err := k.RunCommandFromPod(pod.Namespace, pod.Name, pod.Spec.Containers[0].Name, cmd)


please refer to my earlier comment about running make golangci locally before every push, to catch linter errors

Sorry i missed on that, my bad. I ran make golangci and found some issues that i'll address.

antoninbas · 2024-09-27T22:13:40Z

test/e2e/k8s_util.go

+	if isValidIPv4(out) {
+		return out, nil
+	}


this is not a valuable check IMO. Why would you restrict yourself to IPv4. I can understand the call to net.ParseIP to validate that it is a valid IP address, and you may even want to have this function return a net.IP value instead of a string.

antoninbas · 2024-09-27T22:13:55Z

test/e2e/k8s_util.go

+func (data *TestData) CreateRole(clusterRole *v12.ClusterRole) (*v12.ClusterRole, error) {
+	role, err := data.clientset.RbacV1().ClusterRoles().Create(context.Background(), clusterRole, metav1.CreateOptions{})
+	if err != nil {
+		return nil, err
+	}
+	return role, nil
+}
+
+func (data *TestData) CreateRoleBinding(roleBinding *v12.ClusterRoleBinding) error {
+	_, err := data.clientset.RbacV1().ClusterRoleBindings().Create(context.Background(), roleBinding, metav1.CreateOptions{})
+	return err
+}


you can remove these

antoninbas · 2024-09-27T22:14:31Z

test/e2e/fqdn_dns_cache_test.go

+
+}
+
+func tearDown(t *testing.T, data *TestData, builder *utils.ClusterNetworkPolicySpecBuilder) {


not a useful function

antoninbas · 2024-09-27T22:17:06Z

test/e2e/fqdn_dns_cache_test.go

+
+}
+
+func buildFqdnPolicy(t *testing.T) *utils.ClusterNetworkPolicySpecBuilder {


could we use an ANP instead of an ACNP?
overall it makes cleanup easier, as the ANP can belong to the test Namespace, which means it is deleted when the test Namespace is automatically deleted at the end of the test case

antoninbas · 2024-09-27T22:20:27Z

test/e2e/fqdn_dns_cache_test.go

+	// idea copied from antctl_test.go:284
+	// getEndpointStatus will return "Success", "Failure", or the empty string when out is not a
+	// marshalled metav1.Status object.
+	getEndpointStatus := func(out []byte) string {
+		var status metav1.Status
+		if err := json.Unmarshal(out, &status); err != nil {
+			// Output is not JSON or does not encode a metav1.Status object.
+			return ""
+		}
+		return status.Status
+	}


I don't think this makes sense here. Are you sure you are accessing an endpoint that returns a serialized metav1.Status value?

If you are using agnhost netexec for your server Pod, it returns a timestamp: https://pkg.go.dev/k8s.io/kubernetes/test/images/agnhost/netexec

IMO, you don't care about the contents of the HTTP response here, you only care about the exit code of the curl command (err value returned by RunCommandFromPod)

antoninbas · 2024-09-27T22:23:18Z

test/e2e/k8s_util.go

@@ -333,6 +336,44 @@ func decidePingProbeResult(stdout string, probeNum int) PodConnectivityMark {
 	return Error
 }

+func (k *KubernetesUtils) digUsingShort(


Please move this to framework.go and rename to runDNSQuery

antoninbas · 2024-09-27T22:24:13Z

test/e2e/k8s_util.go

+	dnsServiceIP string) (string, error) {
+
+	// Get the Pod
+	pod, err := k.clientset.CoreV1().Pods(podNamespace).Get(context.TODO(), podName, metav1.GetOptions{})


It is not worth it to get the Pod this way just to get the container name. You should have the container name as a function parameter.

antoninbas · 2024-09-27T22:29:49Z

test/e2e/fqdn_dns_cache_test.go

+func getCustomDnsServerPodLabel() map[string]string {
+	return map[string]string{"app": "custom-dns"}
+}
+
+func createCustomDnsService(data *TestData) (*corev1.Service, error) {
+	return testData.CreateUDPService(customDnsServiceName, data.testNamespace, customDnsPort, customDnsTargetPort,
+		getCustomDnsServerPodLabel(), false, false, corev1.ServiceTypeClusterIP, getIPFamily())
+}
+
+// CreateUDPService creates a service with a UDP port and targetPort.
+func (data *TestData) CreateUDPService(serviceName, namespace string, port, targetPort int32, selector map[string]string,
+	affinity, nodeLocalExternal bool,
+	serviceType corev1.ServiceType, ipFamily *corev1.IPFamily) (*corev1.Service, error) {
+	annotation := make(map[string]string)
+	return data.CreateServiceWithAnnotations(serviceName, namespace, port, targetPort, corev1.ProtocolUDP, selector,
+		affinity, nodeLocalExternal, serviceType, ipFamily, annotation)
+}
+
+func getCustomDnsServiceIP(t *testing.T, data *TestData, customDnsService *corev1.Service) string {
+	// Usually the IP gets assigned quickly to a service , but maybe it's a safe idea to use Get() for getting the IP.
+	customCoreDnsServiceObject, err := data.clientset.CoreV1().Services(data.testNamespace).Get(context.Background(),
+		customDnsService.Name, metav1.GetOptions{})
+	require.NoError(t, err, "Error when getting custom DNS service object : %v", err)
+
+	if customCoreDnsServiceObject.Spec.ClusterIP == "" {
+		require.Fail(t, "ClusterIP is empty for the custom DNS service")
+	}
+
+	t.Logf("ClusterIP of the dns service: %s\n", customCoreDnsServiceObject.Spec.ClusterIP)
+	return customCoreDnsServiceObject.Spec.ClusterIP
+}
+
+func getIPFamily() *corev1.IPFamily {
+	ipFamily := corev1.IPv4Protocol
+	return &ipFamily
+}


IMO, all of these functions are not necessary. The code could be much less verbose.
For example, can you check if Spec.ClusterIP is already populated in the return value of CreateServiceWithAnnotations?

antoninbas · 2024-09-27T22:30:11Z

test/e2e/fqdn_dns_cache_test.go

+
+	dnsServiceIP := getCustomDnsServiceIP(t, data, customDnsService)
+
+	setDnsServerAddressInAntrea(t, data, dnsServiceIP)


when do you undo that change?

Signed-off-by: Hemant <[email protected]>

- Corrected issues in the policy build - Formatted imports for consistency - Fixed spelling errors Signed-off-by: Hemant <[email protected]>

initial commit for e2e test for antrea-io#6229

ab09340

Signed-off-by: Hemant <[email protected]>

Dyanngg reviewed Sep 26, 2024

View reviewed changes

Dyanngg requested review from antoninbas and tnqn September 26, 2024 22:54

antoninbas reviewed Sep 26, 2024

View reviewed changes

Refactored to remove nginx and include agnHost. Also Updated dig comm…

e63de57

…and to use short flag and added annotation update for dns pod to attempt to make configmap changes reflect early. Signed-off-by: Hemant <[email protected]>

hkiiita force-pushed the fqdn-e2e-test branch from 314f3f5 to e63de57 Compare September 27, 2024 19:50

antoninbas reviewed Sep 27, 2024

View reviewed changes

hkiiita added 2 commits October 1, 2024 02:26

refactored with changes provided in review

34a0a15

Signed-off-by: Hemant <[email protected]>

fix: update policy build in code to ensure tests run successfully

5412d1f

- Corrected issues in the policy build - Formatted imports for consistency - Fixed spelling errors Signed-off-by: Hemant <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

initial commit for e2e test for #6229 #6695

initial commit for e2e test for #6229 #6695

hkiiita commented Sep 26, 2024

Dyanngg left a comment

Dyanngg Sep 26, 2024

Dyanngg Sep 26, 2024

Dyanngg Sep 26, 2024

Dyanngg Sep 26, 2024

Dyanngg Sep 26, 2024

Dyanngg Sep 26, 2024

Dyanngg Sep 26, 2024

Dyanngg Sep 26, 2024

Dyanngg Sep 26, 2024

Dyanngg Sep 26, 2024

antoninbas Sep 26, 2024

antoninbas Sep 26, 2024

antoninbas Sep 26, 2024

antoninbas Sep 26, 2024

antoninbas Sep 26, 2024

antoninbas Sep 27, 2024

hkiiita Sep 30, 2024

antoninbas Sep 27, 2024

antoninbas Sep 27, 2024

antoninbas Sep 27, 2024

antoninbas Sep 27, 2024

antoninbas Sep 27, 2024

antoninbas Sep 27, 2024

antoninbas Sep 27, 2024

antoninbas Sep 27, 2024

antoninbas Sep 27, 2024


		}

		func tearDown(t testing.T, data TestData, builder *utils.ClusterNetworkPolicySpecBuilder) {


		}

		func buildFqdnPolicy(t testing.T) utils.ClusterNetworkPolicySpecBuilder {


		dnsServiceIP := getCustomDnsServiceIP(t, data, customDnsService)

		setDnsServerAddressInAntrea(t, data, dnsServiceIP)

initial commit for e2e test for #6229 #6695

Are you sure you want to change the base?

initial commit for e2e test for #6229 #6695

Conversation

hkiiita commented Sep 26, 2024

Dyanngg left a 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

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

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

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