Add BGP confederation support in BGPPolicy #6927

hongliangl · 2025-01-15T11:11:35Z

Done: add some text about ASN to documentation.

Depends on #6905

docs/bgp-policy.md

antoninbas · 2025-01-28T19:17:38Z

docs/bgp-policy.md

+### Confederation
+
+The `Confederation` field specifies that the BGP process operates within a confederation identified by ASN `65000`,
+with other member ASNs `64513` and `64514`.


This is the only field for which the description refers to specific values used in the example, so that's inconsistent. I would recommend just having a generic description here, and adding a link to the more comprehensive confederation example which comes later in the document.

docs/bgp-policy.md

pkg/agent/controller/bgp/controller.go

antoninbas · 2025-01-28T19:30:11Z

pkg/agent/controller/bgp/controller.go

+	return !(a == nil && b == nil) &&
+		(a == nil || b == nil || a.identifier != b.identifier || !a.peers.Equal(b.peers))


this seems incorrect to me. If a and b are both nil, the function will return false because of the negation (!(a == nil && b == nil). Am I missing something, and if not, should this be caught by unit tests? Could you double check?

I would have expected something like this:

return (a == nil && b == nil) || (a != nil && b != nil && a.identifier == b.identifier && a.peers.Equal(b.peers))

The code you provided improves readability within the function, but its logic represents confederationConfigNotChanged, not the current one confederationConfigChanged, if I understand correctly.

Currently, the code implements the opposite logic in confederationConfigChanged, which is less readable. The logic for confederationConfigChanged is:

!(a == nil && b == nil) && (a == nil || b == nil || a.identifier != b.identifier || !a.peers.Equal(b.peers)).

If we negate it, the result simplifies to:

!!(a == nil && b == nil) && (a == nil || b == nil || a.identifier != b.identifier || !a.peers.Equal(b.peers))

(a == nil && b == nil) || !(a == nil || b == nil || a.identifier != b.identifier || !a.peers.Equal(b.peers))

(a == nil && b == nil) || (a != nil && b != nil && a.identifier == b.identifier && a.peers.Equal(b.peers))

It is the same as the example you provided.

Signed-off-by: Hongliang Liu <[email protected]>

antoninbas · 2025-02-07T21:08:27Z

pkg/agent/controller/bgp/controller.go

+	}
+}
+
+func confederationConfigNotChanged(a, b *confederationConfig) bool {


maybe rename to confederationConfigEqual now that you have inverted the logic?

antoninbas · 2025-02-07T21:14:32Z

pkg/agent/controller/bgp/controller_test.go

@@ -1890,7 +2106,8 @@ func generateBGPPolicyState(bgpPolicyName string,
 	localASN int32,
 	routerID string,
 	bgpRoutes []bgp.Route,
-	peerConfigs []bgp.PeerConfig) *bgpPolicyState {
+	peerConfigs []bgp.PeerConfig,
+	patchFns ...func(*bgpPolicyState)) *bgpPolicyState {


it's weird to introduce this patching mechanism but only use it for configuring the confederation.

I think you should either:

use a *confederationConfig parameter (nil is used if there is no confederation), or

use "patch" functions for everything

An example of solution 2:

generateBGPPolicyState("foo", x, y, z, withRoutes(route1, route2), withPeers(peer1, peer2, peer3)) generateBGPPolicyState("bar", x, y, z, withRoutes(route1, route2), withPeers(peer1), withConfederation(c))

antoninbas · 2025-02-07T21:16:55Z

test/e2e/bgppolicy_test.go

+		defer data.crdClient.CrdV1alpha1().BGPPolicies().Delete(context.TODO(), bgpPolicyName, metav1.DeleteOptions{})
+		require.NoError(t, err)


these lines should be swapped?

antoninbas · 2025-02-07T21:17:25Z

test/e2e/bgppolicy_test.go

-			NodeSelector: metav1.LabelSelector{
-				MatchLabels: map[string]string{},
+	t.Run("One BGPPolicy applied to all Nodes", func(t *testing.T) {
+		const bgpPolicyName = "test-policy-alfa"


I think you mean "alpha" :P

antoninbas · 2025-02-07T21:18:52Z

test/e2e/bgppolicy_test.go

-	updatedBGPPolicy.Spec.Advertisements.Pod = nil
-	_, err = data.crdClient.CrdV1alpha1().BGPPolicies().Update(context.TODO(), updatedBGPPolicy, metav1.UpdateOptions{})
-	require.NoError(t, err)
+	t.Run("Multiple BGPPolies applied to different Nodes within a confederation", func(t *testing.T) {


typo: s/BGPPolies/BGPPolicies

antoninbas · 2025-02-07T21:20:04Z

test/e2e/bgppolicy_test.go

+		t.Log("Verify the connectivity of the installed routes on remote FRR route")
+		ipsToConnect := []string{podIP, clusterIP}
+		for _, ip := range ipsToConnect {
+			cmd := fmt.Sprintf("/usr/bin/wget -O - http://%s:8080/hostname -T 5", ip)
+			rc, stdout, _, err := exec.RunDockerExecCommand(externalInfo.externalFRRCID, cmd, "/", nil, "")
+			require.NoError(t, err)
+			require.Equal(t, 0, rc)
+			require.Equal(t, podName, stdout)
+		}


I think this is repeated multiple times, could you define a helper function - ideally inside the test function

antoninbas · 2025-02-07T21:20:17Z

test/e2e/bgppolicy_test.go

+		defer func() {
+			data.updateServiceInternalTrafficPolicy("agnhost-svc", false)
+		}()


defer data.updateServiceInternalTrafficPolicy("agnhost-svc", false)

antoninbas · 2025-02-07T21:21:11Z

test/e2e/bgppolicy_test.go

+			defer data.crdClient.CrdV1alpha1().BGPPolicies().Delete(context.TODO(), bgpPolicyName, metav1.DeleteOptions{})
+			require.NoError(t, err)


antoninbas · 2025-02-07T21:23:37Z

test/e2e/bgppolicy_test.go

+		const confederationIdentifier = int32(60000)
+		const updatedConfederationIdentifier = int32(60001)


these constants don't really need to be "typed"

antoninbas · 2025-02-07T21:24:14Z

test/e2e/bgppolicy_test.go

+		t.Log("Create a test BGPPolicy selecting all Nodes as well as advertising ClusterIPs and Pod CIDRs")
+		const confederationIdentifier = int32(60000)
+		const updatedConfederationIdentifier = int32(60001)
+		asnStart := int32(61000)


based on usage, I am not sure why you use a variable here while you use constants for confederation identifiers.

luolanzone · 2025-02-07T09:00:14Z

docs/bgp-policy.md

+- `identifier`: Specifies the ASN of the confederation, serving as its identifier.
+- `memberASNs`: Specifies the ASNs of other members that are part of the confederation.
+
+See example [BGP Confederation].


Maybe update the [Confederation] to [Advertise Pod IPs through BGP Confederation] to avoid the internal link with number like '(#confederation-1)' in TOC.

luolanzone · 2025-02-07T09:06:56Z

docs/bgp-policy.md

+apiVersion: crd.antrea.io/v1alpha1
+kind: BGPPolicy
+metadata:
+  name: example-bgp-policy


Suggested change

name: example-bgp-policy

name: example-bgp-policy-with-confederation

luolanzone · 2025-02-08T03:13:48Z

test/e2e/bgppolicy_test.go

+		notExpectedRoutes := []FRRRoute{{Prefix: clusterIP + "/32", Nexthops: getAllNodeIPs()}}
+		checkFRRRouterBGPRoutes(t, expectedRoutes, notExpectedRoutes)
+
+		t.Log("verify the connectivity of the installed routes on remote FRR route")


Suggested change

t.Log("verify the connectivity of the installed routes on remote FRR route")

t.Log("verify the connectivity of the installed routes on the remote FRR router")

luolanzone · 2025-02-08T03:15:22Z

test/e2e/bgppolicy_test.go

+			require.NoError(t, err)
+		}
+
+		t.Log("Get routes installed on remote FRR router and verify them")


Suggested change

t.Log("Get routes installed on remote FRR router and verify them")

t.Log("Get routes installed on the remote FRR router and verify them")

luolanzone · 2025-02-08T03:16:00Z

test/e2e/bgppolicy_test.go

-	expectedRoutes = []FRRRoute{{Prefix: clusterIP + "/32", Nexthops: []string{nodeIPv4(0)}}}
-	notExpectedRoutes := []FRRRoute{{Prefix: clusterIP + "/32", Nexthops: getAllNodeIPs()}}
-	checkFRRRouterBGPRoutes(t, expectedRoutes, notExpectedRoutes)
+		t.Log("Get the routes installed on remote FRR router and verify them")


luolanzone · 2025-02-08T03:18:23Z

test/e2e/bgppolicy_test.go

-	}
+
+		for _, bgpPolicy := range bgpPolicies {
+			t.Log("Update the test BGPPolicy with a new confederation identifier and removing Pod CIDR advertisement")


Suggested change

t.Log("Update the test BGPPolicy with a new confederation identifier and removing Pod CIDR advertisement")

t.Log("Update the test BGPPolicy %s with a new confederation identifier and removing Pod CIDR advertisement", bgpPolicy.Name)

hongliangl added action/release-note Indicates a PR that should be included in release notes. area/transit/bgp Issues or PRs related to BGP support. labels Jan 15, 2025

hongliangl added this to the Antrea v2.3 release milestone Jan 15, 2025

hongliangl force-pushed the 20241205-bgp-confederation-controller branch from 5e8cee4 to c1233b6 Compare January 21, 2025 13:14

hongliangl marked this pull request as ready for review January 21, 2025 13:14

hongliangl requested review from antoninbas, tnqn and luolanzone January 21, 2025 13:14

hongliangl force-pushed the 20241205-bgp-confederation-controller branch from c1233b6 to 8bd9ee8 Compare January 24, 2025 04:16

antoninbas reviewed Jan 28, 2025

View reviewed changes

Add BGP confederation support in BGPPolicy

8c3d330

Signed-off-by: Hongliang Liu <[email protected]>

hongliangl force-pushed the 20241205-bgp-confederation-controller branch from 8bd9ee8 to 8c3d330 Compare February 6, 2025 09:48

hongliangl requested a review from antoninbas February 6, 2025 09:50

antoninbas reviewed Feb 7, 2025

View reviewed changes

luolanzone reviewed Feb 8, 2025

View reviewed changes

luolanzone modified the milestones: Antrea v2.3 release, Antrea v2.4 release Feb 8, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add BGP confederation support in BGPPolicy #6927

Add BGP confederation support in BGPPolicy #6927

hongliangl commented Jan 15, 2025 •

edited

Loading

antoninbas Jan 28, 2025

hongliangl Feb 6, 2025

antoninbas Jan 28, 2025

hongliangl Feb 6, 2025

antoninbas Feb 7, 2025

antoninbas Feb 7, 2025

antoninbas Feb 7, 2025

antoninbas Feb 7, 2025

antoninbas Feb 7, 2025

antoninbas Feb 7, 2025

antoninbas Feb 7, 2025

antoninbas Feb 7, 2025

antoninbas Feb 7, 2025

antoninbas Feb 7, 2025

luolanzone Feb 7, 2025

luolanzone Feb 7, 2025

luolanzone Feb 8, 2025

luolanzone Feb 8, 2025

luolanzone Feb 8, 2025

luolanzone Feb 8, 2025

		return !(a == nil && b == nil) &&
		(a == nil \|\| b == nil \|\| a.identifier != b.identifier \|\| !a.peers.Equal(b.peers))

		defer data.crdClient.CrdV1alpha1().BGPPolicies().Delete(context.TODO(), bgpPolicyName, metav1.DeleteOptions{})
		require.NoError(t, err)

		const confederationIdentifier = int32(60000)
		const updatedConfederationIdentifier = int32(60001)

	name: example-bgp-policy
	name: example-bgp-policy-with-confederation

	t.Log("verify the connectivity of the installed routes on remote FRR route")
	t.Log("verify the connectivity of the installed routes on the remote FRR router")

	t.Log("Get routes installed on remote FRR router and verify them")
	t.Log("Get routes installed on the remote FRR router and verify them")

	t.Log("Update the test BGPPolicy with a new confederation identifier and removing Pod CIDR advertisement")
	t.Log("Update the test BGPPolicy %s with a new confederation identifier and removing Pod CIDR advertisement", bgpPolicy.Name)

Add BGP confederation support in BGPPolicy #6927

Are you sure you want to change the base?

Add BGP confederation support in BGPPolicy #6927

Conversation

hongliangl commented Jan 15, 2025 • 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

hongliangl commented Jan 15, 2025 •

edited

Loading