cnf network: add ffrk8 metallb test cases #213

gkopels · 2024-09-21T17:24:34Z

Add 7 of the 9 test cases for epic metallb-frrk8

ajaggapa · 2024-09-23T15:23:01Z

tests/cnf/core/network/metallb/internal/frr/frr.go

@@ -91,6 +135,51 @@ func DefineBGPConfig(localBGPASN, remoteBGPASN int, neighborsIPAddresses []strin
 	return bgpConfig
 }

+// DefineBGPConfigWitgStaticRoutes returns string which represents BGP config file peering to all given IP addresses.
+func DefineBGPConfigWitgStaticRoutes(localBGPASN, remoteBGPASN int, neighborsIPAddresses []string,


ajaggapa · 2024-09-23T15:37:26Z

tests/cnf/core/network/metallb/tests/bfd-test.go

+	for _, pod := range podList {
+		if pod.Definition.Spec.NodeName == "worker-0" {
+			routesMap[pod.Definition.Spec.NodeName] = nextHopList[1]
+		} else {
+			routesMap[pod.Definition.Spec.NodeName] = nextHopList[0]
+		}
 	}



Does this loop need coverage for 3 worker cluster ?
I assume this covers when frr pods are only on worker-0 and worker-1.

I returned the func to its orginal form and created a new one for frrk8 test cases. There I only will use two worker nodes each with one hub pod. Because of security policies it is not possible on the same two communicate between two pods on the same IP address range. So the static routes need to be crossed between the two workers.

ajaggapa · 2024-09-23T15:39:29Z

tests/cnf/core/network/metallb/tests/common.go

-func createExternalNad() {
+func createHubConfigMap(name string) *configmap.Builder {
+	frrBFDConfig := frr.DefineBGPConfig(
+		64500, tsparams.LocalBGPASN, []string{"10.10.0.10"}, false, false)


Use constant for 64500 LocalBGPASN

evgenLevin · 2024-09-23T17:28:21Z

tests/cnf/core/network/metallb/internal/frr/frr.go

@@ -91,6 +135,51 @@ func DefineBGPConfig(localBGPASN, remoteBGPASN int, neighborsIPAddresses []strin
 	return bgpConfig
 }

+// DefineBGPConfigWitgStaticRoutes returns string which represents BGP config file peering to all given IP addresses.


wrong comment

Change name and description DefineBGPConfigWithStaticRouteAndNetwork

evgenLevin · 2024-09-23T17:30:54Z

tests/cnf/core/network/metallb/internal/frr/frr.go

+// DefineBGPConfigWitgStaticRoutes returns string which represents BGP config file peering to all given IP addresses.
+func DefineBGPConfigWitgStaticRoutes(localBGPASN, remoteBGPASN int, neighborsIPAddresses []string,
+	multiHop, bfd bool) string {
+	bgpConfig := tsparams.FRRBaseConfig + fmt.Sprintf("ip route %s/32 172.16.0.10\n", neighborsIPAddresses[1]) +


Where did you get 172.16.0.10 and 172.16.0.11. Make it as a func atribute or general parameter

added as func attribute

evgenLevin · 2024-09-23T17:41:00Z

tests/cnf/core/network/metallb/internal/frr/frr.go

+}
+
+// VerifyBGPReceivedRoutes verifies routes were received via BGP.
+func VerifyBGPReceivedRoutes(frrk8sPods []*pod.Builder) (string, error) {


Incorrect name, this function does more than just verify the routes.

Its only verifying the routes received on the frr nodes.

evgenLevin · 2024-09-23T17:41:22Z

tests/cnf/core/network/metallb/internal/frr/frr.go

+	return allRoutes.String(), nil
+}
+
+func parseBGPAdvertisedRoutes(jsonData string) (string, error) {


Please move all func to the enf of the file

evgenLevin · 2024-09-23T17:42:18Z

tests/cnf/core/network/metallb/internal/frr/frr.go

+		}
+
+		// Store the result for the current nodeIP
+		allRoutes.WriteString(fmt.Sprintf("Node IP: %s\n", nodeIP))


Why are you working with string not with maps?

changed to maps

nothing changed

evgenLevin · 2024-09-23T17:56:05Z

tests/cnf/core/network/metallb/tests/frrk8-tests.go

+
+				By("Checking that BGP session is established and up")
+				verifyMetalLbBGPSessionsAreUPOnFrrPod(frrPod, removePrefixFromIPList(ipv4NodeAddrList))
+				time.Sleep(30 * time.Second)


?
Use Eventually if needed

evgenLevin · 2024-09-23T18:02:24Z

tests/cnf/core/network/metallb/tests/frrk8-tests.go

+
+			for _, frrk8sPod := range frrk8sPods {
+				out, err := frr.SetStaticRoute(frrk8sPod, "del", "172.16.0.1", speakerRoutesMap)
+				fmt.Println("OUT", out)


evgenLevin · 2024-09-23T18:03:33Z

tests/cnf/core/network/metallb/tests/frrk8-tests.go

+
+func createNewMetalLbDaemonSetAndWaitUntilItsRunningWithAlwaysBlock(timeout time.Duration,
+	nodeLabel map[string]string, prefixes []string) error {
+	glog.V(90).Infof("Verifying if metalLb daemonset is running")


Use BY instead of glog

evgenLevin · 2024-09-23T18:05:01Z

tests/cnf/core/network/metallb/tests/frrk8-tests.go

+	frrPod := createFrrPod(
+		masterNodeList[0].Object.Name, masterConfigMap.Definition.Name, []string{}, staticIPAnnotation)
+
+	Eventually(func() error {


evgenLevin · 2024-09-23T18:05:06Z

tests/cnf/core/network/metallb/tests/frrk8-tests.go

+}
+
+func deployTestPods(addressPool []string) *pod.Builder {
+	var err error


evgenLevin · 2024-09-24T16:55:17Z

tests/cnf/core/network/metallb/internal/frr/frr.go

+		}
+
+		// Store the result for the current nodeIP
+		allRoutes.WriteString(fmt.Sprintf("Node IP: %s\n", nodeIP))


nothing changed

tests/cnf/core/network/metallb/tests/frrk8-tests.go

evgenLevin · 2024-09-24T17:01:45Z

Please rebase

Rebased.

oribon · 2024-09-25T07:35:42Z

tests/cnf/core/network/metallb/tests/frrk8-tests.go

+				}, 60*time.Second, 5*time.Second).Should(ContainSubstring(externalAdvertisedIPv4Routes[1]),
+					"Fail to find received routes")
+			})


iiuc this checks that the 2nd prefix is there, but does not validate that it is the only one (i.e that the 1st prefix is actually blocked), I think this should validate that the routes is exactly the 1st prefix (or at least that it does not contain the first prefix)?

added verifiying that the blocked route is not received

oribon · 2024-09-25T07:35:54Z

tests/cnf/core/network/metallb/internal/tsparams/consts.go

@@ -9,8 +9,13 @@ const (
 	LabelBGPTestCases = "bgp"
 	// LabelLayer2TestCases represents layer2 label that can be used for test cases selection.
 	LabelLayer2TestCases = "layer2"
+	// LabelFRRTestCases represents frrk8 label that can be used for test cases selection.
+	LabelFRRTestCases = "frrk8"


nit: frrk8 -> frrk8s

tests/cnf/core/network/metallb/tests/frrk8-tests.go

oribon · 2024-09-25T07:43:38Z

tests/cnf/core/network/metallb/tests/frrk8-tests.go

+
+				By("Create a frrconfiguration with prefix filter")
+				err := createFrrConfiguration("frrconfig-filtered", ipv4metalLbIPList[0],
+					64500, []string{externalAdvertisedIPv4Routes[0], externalAdvertisedIPv6Routes[0]}, false)


same, declare a filteredIPs var to make this more readable and reuse later.
also, the IPv6 prefix is not verified later, is that on purpose?

Added.
Currently we are having issues deploying IPv6 in our lab. We will add the IPv6 when it is possible to test.

oribon · 2024-09-25T07:45:12Z

tests/cnf/core/network/metallb/tests/frrk8-tests.go

+	if metalLbDs.IsReady(timeout) {
+		return nil
+	}
+


wdyt about also verifying that the frr-k8s daemonset is ready?

oribon · 2024-09-25T07:48:06Z

tests/cnf/core/network/metallb/tests/frrk8-tests.go

+				), "Fail to find all expected received routes")
+			})
+
+		It("Verify that a FRR speaker rejects a contrasting FRRConfiguration merge",


wdyt about keeping only the part about the conflicting configs? seems the metallb / peering part is redundant when all you want to check is that it handles conflicting configs properly

I agree. Removed the peering.

evgenLevin · 2024-09-26T16:15:48Z

tests/cnf/core/network/metallb/tests/frrk8-tests.go

+
+				By("Creating a new instance of MetalLB Speakers on workers blocking specific incoming prefixes")
+				err := createNewMetalLbDaemonSetAndWaitUntilItsRunningWithAlwaysBlock(tsparams.DefaultTimeout,
+					workerLabelMap, []string{externalAdvertisedIPv4Routes[0]})


use prefixToBlock

tests/cnf/core/network/metallb/tests/frrk8-tests.go

evgenLevin · 2024-09-26T16:25:35Z

tests/cnf/core/network/metallb/tests/frrk8-tests.go

+			})
+			Expect(err).ToNot(HaveOccurred(), "Fail to list speaker pods")
+
+			By("Collecting information before test")


evgenLevin · 2024-09-26T16:25:37Z

tests/cnf/core/network/metallb/tests/frrk8-tests.go

+
+			By("Collecting information before test")
+
+			Expect(err).ToNot(HaveOccurred(), "Failed to list speaker pods")


evgenLevin · 2024-09-26T16:28:51Z

tests/cnf/core/network/metallb/tests/frrk8-tests.go

+		}
+	}
+
+	glog.V(90).Infof("Create new metalLb speaker's daemonSet.")


change to By

evgenLevin · 2024-09-26T16:28:55Z

tests/cnf/core/network/metallb/tests/frrk8-tests.go

+		return err
+	}
+
+	glog.V(90).Infof("Waiting until the new metalLb daemonset is in Ready state.")


change to By

evgenLevin · 2024-09-26T16:29:48Z

tests/cnf/core/network/metallb/tests/frrk8-tests.go

+	_, err := frrConfig.Create()
+
+	if err != nil {
+		glog.V(90).Infof("failed to create a frrconfiugration")


evgenLevin · 2024-09-26T16:31:01Z

tests/cnf/core/network/metallb/tests/frrk8-tests.go

+
+})
+
+func createNewMetalLbDaemonSetAndWaitUntilItsRunningWithAlwaysBlock(timeout time.Duration,


Valdate all errors inside the local func via Expect()

evgenLevin · 2024-09-26T16:31:36Z

tests/cnf/core/network/metallb/tests/frrk8-tests.go

+	return masterConfigMap
+}
+
+func verifyExternalAdvertisedRoutes(frrPod *pod.Builder, ipv4NodeAddrList, externalExpectedRoutes []string) error {


Valdate all errors inside the local func via Expect()

evgenLevin · 2024-09-26T16:32:01Z

tests/cnf/core/network/metallb/tests/frrk8-tests.go

+	Expect(err).ToNot(HaveOccurred(), "Failed to clean test namespace")
+}
+
+func buildRoutesMapWithSpecificRoutes(podList []*pod.Builder, nextHopList []string) (map[string]string, error) {


Valdate all errors inside the local func via Expect()

tests/cnf/core/network/metallb/tests/frrk8-tests.go

evgenLevin · 2024-09-29T00:56:03Z

tests/cnf/core/network/metallb/tests/frrk8-tests.go

+		context.TODO(), 3*time.Second, timeout, true, func(ctx context.Context) (bool, error) {
+			metalLbDs, err = daemonset.Pull(APIClient, tsparams.MetalLbDsName, NetConfig.MlbOperatorNamespace)
+			if err != nil {
+				glog.V(90).Infof("Error pulling speakers in %s namespace %s, retry",


evgenLevin · 2024-09-29T00:56:07Z

tests/cnf/core/network/metallb/tests/frrk8-tests.go

+
+			metalLbDs, err = daemonset.Pull(APIClient, tsparams.FrrDsName, NetConfig.MlbOperatorNamespace)
+			if err != nil {
+				glog.V(90).Infof("Error pulling frrk8s in %s namespace %s, retry",


evgenLevin · 2024-09-29T00:57:55Z

tests/cnf/core/network/metallb/tests/frrk8-tests.go

+		WithPort(179, 0, 0)
+
+	_, err := frrConfig.Create()
+	if err != nil {


Valdate all errors inside the local func via Expect()

evgenLevin · 2024-09-29T01:03:32Z

tests/cnf/core/network/metallb/tests/frrk8-tests.go

+	routesMap := make(map[string]string)
+
+	for _, frrPod := range podList {
+		if frrPod.Definition.Spec.NodeName == "worker-0" {


Please don't hardcode the worker name. Please pull the workers and use the first wotker name

evgenLevin · 2024-09-30T16:12:07Z

tests/cnf/core/network/metallb/tests/frrk8-tests.go

+	return routesMap
+}
+
+func verifyFrrWebHookServerRunning() {


instead of this function. try to use something like:

By("Waiting until the new frr-k8s-webhook-server deployment is in Ready state.") frrk8sWebhookDeployment, err := deployment.Pull( APIClient, "frr-k8s-webhook-server", NetConfig.MlbOperatorNamespace) Expect(frrk8sWebhookDeployment.IsReady(30 * time.Second)).To(BeTrue(), "frr-k8s-webhook-server deployment is not ready")

evgenLevin

/lgtm

ajaggapa

lgtm

Co-authored-by: Gregory Kopels <[email protected]>

gkopels changed the title ~~bump-eco-infra-frrk8~~ cnf network: add ffrk8 metallb test cases Sep 22, 2024

gkopels force-pushed the add-metallb-frrk8-test-cases branch 6 times, most recently from 5d53f97 to 4424d8b Compare September 22, 2024 14:49

ajaggapa reviewed Sep 23, 2024

View reviewed changes

evgenLevin reviewed Sep 23, 2024

View reviewed changes

gkopels force-pushed the add-metallb-frrk8-test-cases branch 5 times, most recently from 54ed23f to 6c10dc8 Compare September 24, 2024 12:36

evgenLevin reviewed Sep 24, 2024

View reviewed changes

gkopels force-pushed the add-metallb-frrk8-test-cases branch 2 times, most recently from 7214d2c to 2dd2db7 Compare September 25, 2024 05:48

oribon reviewed Sep 25, 2024

View reviewed changes

gkopels force-pushed the add-metallb-frrk8-test-cases branch from 2dd2db7 to 9933d86 Compare September 26, 2024 12:16

evgenLevin reviewed Sep 26, 2024

View reviewed changes

gkopels force-pushed the add-metallb-frrk8-test-cases branch from 9933d86 to f629208 Compare September 28, 2024 18:08

evgenLevin reviewed Sep 29, 2024

View reviewed changes

gkopels force-pushed the add-metallb-frrk8-test-cases branch 4 times, most recently from 175e6ba to 42462ea Compare September 30, 2024 14:57

evgenLevin reviewed Sep 30, 2024

View reviewed changes

cnf network: add frrk8 metallb test cases

e6de986

gkopels force-pushed the add-metallb-frrk8-test-cases branch from 42462ea to e6de986 Compare September 30, 2024 16:33

evgenLevin approved these changes Sep 30, 2024

View reviewed changes

ajaggapa approved these changes Oct 1, 2024

View reviewed changes

ajaggapa merged commit 0a354a6 into openshift-kni:main Oct 1, 2024
1 check passed

josclark42 pushed a commit to josclark42/eco-gotests that referenced this pull request Oct 3, 2024

cnf network: add frrk8 metallb test cases (openshift-kni#213)

7ecf86a

Co-authored-by: Gregory Kopels <[email protected]>


		By("Collecting information before test")

		Expect(err).ToNot(HaveOccurred(), "Failed to list speaker pods")


		})

		func createNewMetalLbDaemonSetAndWaitUntilItsRunningWithAlwaysBlock(timeout time.Duration,

cnf network: add ffrk8 metallb test cases #213

cnf network: add ffrk8 metallb test cases #213

Conversation

gkopels commented Sep 21, 2024 • 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

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

evgenLevin commented Sep 24, 2024 • edited by gkopels 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evgenLevin left a comment

Choose a reason for hiding this comment

ajaggapa left a comment

Choose a reason for hiding this comment

gkopels commented Sep 21, 2024 •

edited

Loading

evgenLevin commented Sep 24, 2024 •

edited by gkopels

Loading