Uniform BGP router ID selection for IPv4 and IPv6

Fixes #6550 Signed-off-by: Kumar Atish <[email protected]>
antrea-io · Aug 21, 2024 · 1b3987e · 1b3987e
1 parent 555ee9a
commit 1b3987e
Show file tree

Hide file tree

Showing 3 changed files with 94 additions and 44 deletions.
diff --git a/docs/bgp-policy.md b/docs/bgp-policy.md
@@ -117,14 +117,20 @@ The `bgpPeers` field lists the BGP peers to which the advertisements are sent.
 
 ## BGP router ID
 
-The BGP router identifier (ID) is a 4-byte field that is usually represented as an IPv4 address.
+The BGP router identifier (ID) is a 4-byte field that is usually represented as an IPv4 address. Antrea uses the following
+steps to choose the BGP router ID:
 
-For an IPv4-only or dual-stack Kubernetes cluster, the Node's IPv4 address (assigned to the transport interface) is used.
+1. If the `node.antrea.io/bgp-router-id` annotation is present on the Node and its value is a valid IPv4 address string,
+   we will use the provided value.
+2. Otherwise, for an IPv4-only or dual-stack Kubernetes cluster, the Node's IPv4 address (assigned to the transport
+   interface) is used.
+3. Otherwise, for IPv6-only clusters, a 32-bit integer will be generated by hashing the Node's name, then converted to the
+   string representation of an IPv4 address.
 
-For IPv6-only clusters, if the `node.antrea.io/bgp-router-id` annotation is present on the Node and its value is a valid
-IPv4 address string, we will use the provided value. Otherwise, a 32-bit integer will be generated by hashing the Node
-name, then converted to the string representation of an IPv4 address, and the `node.antrea.io/bgp-router-id` annotation
-is added / updated as necessary to reflect the selected BGP router ID.
+After this selection process, the `node.antrea.io/bgp-router-id` annotation is added or updated as necessary to reflect
+the selected BGP router ID.
+
+The router ID is generated once and will not be updated if the Node configuration changes (e.g., if the Node's IPv4 address is updated).
 
 ## BGP Authentication
 

diff --git a/pkg/agent/controller/bgp/controller.go b/pkg/agent/controller/bgp/controller.go
@@ -481,15 +481,13 @@ func (c *Controller) getRouterID() (string, error) {
 	//   startup and is the same for every local interface and BGP peer.
 	//
 	// In goBGP, only an IPv4 address can be used as the BGP Identifier (BGP router ID).
-	// For IPv4-only or dual-stack Kubernetes clusters, the Node's IPv4 address is used as the BGP router ID, ensuring
-	// uniqueness.
-	// For IPv6-only Kubernetes clusters without a Node IPv4 address, the router ID could be specified in the Node
-	// annotation `node.antrea.io/bgp-router-id`. If the annotation is not present, an IPv4 address will be generated by
-	// hashing the Node name and updated to the Node annotation `node.antrea.io/bgp-router-id`.
-
-	if c.enabledIPv4 {
-		return c.nodeIPv4Addr, nil
-	}
+	// The router ID could be specified in the Node annotation `node.antrea.io/bgp-router-id`.
+	// For IPv4-only or dual-stack Kubernetes clusters, if the annotation is not present,
+	// the Node's IPv4 address is used as the BGP router ID, ensuring uniqueness, and updated
+	// to the Node annotation `node.antrea.io/bgp-router-id`.
+	// For IPv6-only Kubernetes clusters without a Node IPv4 address, if the annotation is
+	// not present, an IPv4 address will be generated by hashing the Node name and updated
+	// to the Node annotation `node.antrea.io/bgp-router-id`.
 
 	nodeObj, err := c.nodeLister.Get(c.nodeName)
 	if err != nil {
@@ -500,7 +498,11 @@ func (c *Controller) getRouterID() (string, error) {
 	var routerID string
 	routerID, exists = nodeObj.GetAnnotations()[types.NodeBGPRouterIDAnnotationKey]
 	if !exists {
-		routerID = hashNodeNameToIP(c.nodeName)
+		if c.enabledIPv4 {
+			routerID = c.nodeIPv4Addr
+		} else {
+			routerID = hashNodeNameToIP(c.nodeName)
+		}
 		patch, _ := json.Marshal(map[string]interface{}{
 			"metadata": map[string]interface{}{
 				"annotations": map[string]string{

diff --git a/pkg/agent/controller/bgp/controller_test.go b/pkg/agent/controller/bgp/controller_test.go
@@ -265,7 +265,7 @@ func TestBGPPolicyAdd(t *testing.T) {
 			},
 			expectedState: generateBGPPolicyState(179,
 				65000,
-				nodeIPv4Addr.IP.String(),
+				nodeAnnotations1[types.NodeBGPRouterIDAnnotationKey],
 				[]string{ipStrToPrefix(clusterIPv4)},
 				[]bgp.PeerConfig{ipv4Peer1Config},
 			),
@@ -296,7 +296,7 @@ func TestBGPPolicyAdd(t *testing.T) {
 			},
 			expectedState: generateBGPPolicyState(179,
 				65000,
-				"192.168.77.100",
+				nodeAnnotations1[types.NodeBGPRouterIDAnnotationKey],
 				[]string{ipStrToPrefix(externalIPv6)},
 				[]bgp.PeerConfig{ipv6Peer1Config},
 			),
@@ -330,7 +330,7 @@ func TestBGPPolicyAdd(t *testing.T) {
 			},
 			expectedState: generateBGPPolicyState(179,
 				65000,
-				nodeIPv4Addr.IP.String(),
+				nodeAnnotations1[types.NodeBGPRouterIDAnnotationKey],
 				[]string{ipStrToPrefix(loadBalancerIPv4), ipStrToPrefix(loadBalancerIPv6)},
 				[]bgp.PeerConfig{ipv4Peer1Config, ipv6Peer1Config},
 			),
@@ -363,7 +363,7 @@ func TestBGPPolicyAdd(t *testing.T) {
 			},
 			expectedState: generateBGPPolicyState(179,
 				65000,
-				nodeIPv4Addr.IP.String(),
+				nodeAnnotations1[types.NodeBGPRouterIDAnnotationKey],
 				[]string{ipStrToPrefix(ipv4EgressIP1)},
 				[]bgp.PeerConfig{ipv4Peer1Config},
 			),
@@ -390,7 +390,7 @@ func TestBGPPolicyAdd(t *testing.T) {
 			objects: []runtime.Object{node},
 			expectedState: generateBGPPolicyState(179,
 				65000,
-				"192.168.77.100",
+				nodeAnnotations1[types.NodeBGPRouterIDAnnotationKey],
 				[]string{podIPv6CIDR.String()},
 				[]bgp.PeerConfig{ipv6Peer1Config},
 			),
@@ -424,7 +424,7 @@ func TestBGPPolicyAdd(t *testing.T) {
 			},
 			expectedState: generateBGPPolicyState(1179,
 				65001,
-				nodeIPv4Addr.IP.String(),
+				nodeAnnotations1[types.NodeBGPRouterIDAnnotationKey],
 				nil,
 				[]bgp.PeerConfig{ipv4Peer1Config, ipv6Peer1Config},
 			),
@@ -462,13 +462,13 @@ func TestBGPPolicyAdd(t *testing.T) {
 			objects: []runtime.Object{ipv4ClusterIP1, ipv4ClusterIP1Eps, node},
 			existingState: generateBGPPolicyState(179,
 				65000,
-				nodeIPv4Addr.IP.String(),
+				nodeAnnotations1[types.NodeBGPRouterIDAnnotationKey],
 				[]string{ipStrToPrefix(clusterIPv4)},
 				[]bgp.PeerConfig{ipv4Peer1Config},
 			),
 			expectedState: generateBGPPolicyState(179,
 				65000,
-				nodeIPv4Addr.IP.String(),
+				nodeAnnotations1[types.NodeBGPRouterIDAnnotationKey],
 				[]string{ipStrToPrefix(clusterIPv4)},
 				[]bgp.PeerConfig{ipv4Peer1Config},
 			),
@@ -525,7 +525,7 @@ func TestBGPPolicyUpdate(t *testing.T) {
 		})
 	effectivePolicyState := generateBGPPolicyState(179,
 		65000,
-		nodeIPv4Addr.IP.String(),
+		nodeAnnotations1[types.NodeBGPRouterIDAnnotationKey],
 		[]string{ipStrToPrefix(clusterIPv4),
 			ipStrToPrefix(clusterIPv6),
 			ipStrToPrefix(loadBalancerIPv4),
@@ -629,7 +629,7 @@ func TestBGPPolicyUpdate(t *testing.T) {
 			},
 			expectedState: generateBGPPolicyState(1179,
 				65000,
-				nodeIPv4Addr.IP.String(),
+				nodeAnnotations1[types.NodeBGPRouterIDAnnotationKey],
 				[]string{ipStrToPrefix(clusterIPv4),
 					ipStrToPrefix(clusterIPv6),
 					ipStrToPrefix(loadBalancerIPv4),
@@ -686,7 +686,7 @@ func TestBGPPolicyUpdate(t *testing.T) {
 				}),
 			expectedState: generateBGPPolicyState(179,
 				65000,
-				nodeIPv4Addr.IP.String(),
+				nodeAnnotations1[types.NodeBGPRouterIDAnnotationKey],
 				[]string{ipStrToPrefix(externalIPv4),
 					ipStrToPrefix(externalIPv6),
 					ipStrToPrefix(ipv4EgressIP1),
@@ -731,7 +731,7 @@ func TestBGPPolicyUpdate(t *testing.T) {
 				}),
 			expectedState: generateBGPPolicyState(179,
 				65001,
-				nodeIPv4Addr.IP.String(),
+				nodeAnnotations1[types.NodeBGPRouterIDAnnotationKey],
 				[]string{ipStrToPrefix(externalIPv4),
 					ipStrToPrefix(externalIPv6),
 					ipStrToPrefix(ipv4EgressIP1),
@@ -775,7 +775,7 @@ func TestBGPPolicyUpdate(t *testing.T) {
 				}),
 			expectedState: generateBGPPolicyState(1179,
 				65000,
-				nodeIPv4Addr.IP.String(),
+				nodeAnnotations1[types.NodeBGPRouterIDAnnotationKey],
 				[]string{ipStrToPrefix(clusterIPv4),
 					ipStrToPrefix(clusterIPv6),
 					ipStrToPrefix(loadBalancerIPv4),
@@ -822,7 +822,7 @@ func TestBGPPolicyUpdate(t *testing.T) {
 					ipv6Peer3}),
 			expectedState: generateBGPPolicyState(179,
 				65000,
-				nodeIPv4Addr.IP.String(),
+				nodeAnnotations1[types.NodeBGPRouterIDAnnotationKey],
 				[]string{ipStrToPrefix(clusterIPv4),
 					ipStrToPrefix(clusterIPv6),
 					ipStrToPrefix(loadBalancerIPv4),
@@ -945,7 +945,7 @@ func TestBGPPolicyDelete(t *testing.T) {
 		})
 	policy1State := generateBGPPolicyState(179,
 		65000,
-		nodeIPv4Addr.IP.String(),
+		nodeAnnotations1[types.NodeBGPRouterIDAnnotationKey],
 		[]string{
 			ipStrToPrefix(loadBalancerIPv4),
 			ipStrToPrefix(loadBalancerIPv6),
@@ -971,7 +971,7 @@ func TestBGPPolicyDelete(t *testing.T) {
 		})
 	policy2State := generateBGPPolicyState(179,
 		65000,
-		nodeIPv4Addr.IP.String(),
+		nodeAnnotations1[types.NodeBGPRouterIDAnnotationKey],
 		[]string{
 			ipStrToPrefix(externalIPv4),
 			ipStrToPrefix(externalIPv6),
@@ -996,7 +996,7 @@ func TestBGPPolicyDelete(t *testing.T) {
 		})
 	policy3State := generateBGPPolicyState(1179,
 		65000,
-		nodeIPv4Addr.IP.String(),
+		nodeAnnotations1[types.NodeBGPRouterIDAnnotationKey],
 		[]string{
 			ipStrToPrefix(externalIPv4),
 			ipStrToPrefix(externalIPv6),
@@ -1119,7 +1119,7 @@ func TestNodeUpdate(t *testing.T) {
 		[]v1alpha1.BGPPeer{ipv4Peer1, ipv6Peer1})
 	policy1State := generateBGPPolicyState(179,
 		65000,
-		nodeIPv4Addr.IP.String(),
+		nodeAnnotations1[types.NodeBGPRouterIDAnnotationKey],
 		[]string{podIPv4CIDR.String(), podIPv6CIDR.String()},
 		[]bgp.PeerConfig{ipv4Peer1Config, ipv6Peer1Config})
 	policy2 := generateBGPPolicy(bgpPolicyName2,
@@ -1135,7 +1135,7 @@ func TestNodeUpdate(t *testing.T) {
 		[]v1alpha1.BGPPeer{ipv4Peer1, ipv6Peer1})
 	policy2State := generateBGPPolicyState(1179,
 		65000,
-		nodeIPv4Addr.IP.String(),
+		nodeAnnotations1[types.NodeBGPRouterIDAnnotationKey],
 		[]string{podIPv4CIDR.String(), podIPv6CIDR.String()},
 		[]bgp.PeerConfig{ipv4Peer1Config, ipv6Peer1Config})
 	policy3 := generateBGPPolicy(bgpPolicyName3,
@@ -1210,19 +1210,61 @@ func TestNodeUpdate(t *testing.T) {
 			},
 			existingState: deepCopyBGPPolicyState(policy1State),
 		},
+		{
+			name:          "Update annotations, effective BGPPolicy router ID is updated",
+			ipv4Enabled:   true,
+			ipv6Enabled:   true,
+			node:          generateNode(localNodeName, nodeLabels1, nodeAnnotations1),
+			updatedNode:   generateNode(localNodeName, nodeLabels1, nodeAnnotations2),
+			existingState: deepCopyBGPPolicyState(policy1State),
+			expectedState: generateBGPPolicyState(179,
+				65000,
+				nodeAnnotations2[types.NodeBGPRouterIDAnnotationKey],
+				[]string{podIPv4CIDR.String(), podIPv6CIDR.String()},
+				[]bgp.PeerConfig{ipv4Peer1Config, ipv6Peer1Config}),
+			expectedCalls: func(mockBGPServer *bgptest.MockInterfaceMockRecorder) {
+				mockBGPServer.Start(gomock.Any())
+				mockBGPServer.Stop(gomock.Any())
+				mockBGPServer.AddPeer(gomock.Any(), ipv4Peer1Config)
+				mockBGPServer.AddPeer(gomock.Any(), ipv6Peer1Config)
+				mockBGPServer.AdvertiseRoutes(gomock.Any(), []bgp.Route{{Prefix: podIPv4CIDR.String()}})
+				mockBGPServer.AdvertiseRoutes(gomock.Any(), []bgp.Route{{Prefix: podIPv6CIDR.String()}})
+			},
+		},
+		{
+			name:          "Remove annotations, router ID is updated using Node's IPv4 address",
+			ipv4Enabled:   true,
+			ipv6Enabled:   true,
+			node:          generateNode(localNodeName, nodeLabels1, nodeAnnotations1),
+			updatedNode:   generateNode(localNodeName, nodeLabels1, nil),
+			existingState: deepCopyBGPPolicyState(policy1State),
+			expectedState: generateBGPPolicyState(179,
+				65000,
+				nodeIPv4Addr.IP.String(),
+				[]string{podIPv4CIDR.String(), podIPv6CIDR.String()},
+				[]bgp.PeerConfig{ipv4Peer1Config, ipv6Peer1Config}),
+			expectedCalls: func(mockBGPServer *bgptest.MockInterfaceMockRecorder) {
+				mockBGPServer.Start(gomock.Any())
+				mockBGPServer.Stop(gomock.Any())
+				mockBGPServer.AddPeer(gomock.Any(), ipv4Peer1Config)
+				mockBGPServer.AddPeer(gomock.Any(), ipv6Peer1Config)
+				mockBGPServer.AdvertiseRoutes(gomock.Any(), []bgp.Route{{Prefix: podIPv4CIDR.String()}})
+				mockBGPServer.AdvertiseRoutes(gomock.Any(), []bgp.Route{{Prefix: podIPv6CIDR.String()}})
+			},
+		},
 		{
 			name:        "IPv6 only, update annotations, effective BGPPolicy router ID is updated",
 			ipv6Enabled: true,
 			node:        generateNode(localNodeName, nodeLabels1, nodeAnnotations1),
 			updatedNode: generateNode(localNodeName, nodeLabels1, nodeAnnotations2),
 			existingState: generateBGPPolicyState(179,
 				65000,
-				"192.168.77.100",
+				nodeAnnotations1[types.NodeBGPRouterIDAnnotationKey],
 				[]string{podIPv6CIDR.String()},
 				[]bgp.PeerConfig{ipv6Peer1Config}),
 			expectedState: generateBGPPolicyState(179,
 				65000,
-				"10.10.0.100",
+				nodeAnnotations2[types.NodeBGPRouterIDAnnotationKey],
 				[]string{podIPv6CIDR.String()},
 				[]bgp.PeerConfig{ipv6Peer1Config}),
 			expectedCalls: func(mockBGPServer *bgptest.MockInterfaceMockRecorder) {
@@ -1239,7 +1281,7 @@ func TestNodeUpdate(t *testing.T) {
 			updatedNode: generateNode(localNodeName, nodeLabels1, nil),
 			existingState: generateBGPPolicyState(179,
 				65000,
-				"192.168.77.100",
+				nodeAnnotations1[types.NodeBGPRouterIDAnnotationKey],
 				[]string{podIPv6CIDR.String()},
 				[]bgp.PeerConfig{ipv6Peer1Config}),
 			expectedState: generateBGPPolicyState(179,
@@ -1687,7 +1729,7 @@ func TestSyncBGPPolicyFailures(t *testing.T) {
 
 	checkBGPPolicyState(t, generateBGPPolicyState(179,
 		65000,
-		nodeIPv4Addr.IP.String(),
+		nodeAnnotations1[types.NodeBGPRouterIDAnnotationKey],
 		[]string{ipStrToPrefix(loadBalancerIPv4)},
 		[]bgp.PeerConfig{ipv4Peer2Config}),
 		c.bgpPolicyState)
@@ -1703,7 +1745,7 @@ func TestSyncBGPPolicyFailures(t *testing.T) {
 	require.EqualError(t, c.syncBGPPolicy(ctx), "failed to stop current BGP server: failed reason")
 	checkBGPPolicyState(t, generateBGPPolicyState(179,
 		65000,
-		nodeIPv4Addr.IP.String(),
+		nodeAnnotations1[types.NodeBGPRouterIDAnnotationKey],
 		[]string{ipStrToPrefix(loadBalancerIPv4)},
 		[]bgp.PeerConfig{ipv4Peer2Config}),
 		c.bgpPolicyState)
@@ -1720,7 +1762,7 @@ func TestSyncBGPPolicyFailures(t *testing.T) {
 	require.EqualError(t, c.syncBGPPolicy(ctx), "failed to add BGP peer")
 	checkBGPPolicyState(t, generateBGPPolicyState(1179,
 		65000,
-		nodeIPv4Addr.IP.String(),
+		nodeAnnotations1[types.NodeBGPRouterIDAnnotationKey],
 		[]string{},
 		[]bgp.PeerConfig{}),
 		c.bgpPolicyState)
@@ -1734,7 +1776,7 @@ func TestSyncBGPPolicyFailures(t *testing.T) {
 	checkBGPPolicyState(t, generateBGPPolicyState(
 		1179,
 		65000,
-		nodeIPv4Addr.IP.String(),
+		nodeAnnotations1[types.NodeBGPRouterIDAnnotationKey],
 		[]string{},
 		[]bgp.PeerConfig{ipv4Peer1Config}),
 		c.bgpPolicyState)
@@ -1752,7 +1794,7 @@ func TestSyncBGPPolicyFailures(t *testing.T) {
 	doneDummyEvent(t, c)
 	checkBGPPolicyState(t, generateBGPPolicyState(1179,
 		65000,
-		nodeIPv4Addr.IP.String(),
+		nodeAnnotations1[types.NodeBGPRouterIDAnnotationKey],
 		[]string{ipStrToPrefix(externalIPv4)},
 		[]bgp.PeerConfig{ipv4Peer2Config}),
 		c.bgpPolicyState)
@@ -1770,7 +1812,7 @@ func TestSyncBGPPolicyFailures(t *testing.T) {
 	doneDummyEvent(t, c)
 	checkBGPPolicyState(t, generateBGPPolicyState(1179,
 		65000,
-		nodeIPv4Addr.IP.String(),
+		nodeAnnotations1[types.NodeBGPRouterIDAnnotationKey],
 		[]string{ipStrToPrefix(loadBalancerIPv4)},
 		[]bgp.PeerConfig{updatedIPv4Peer2Config}),
 		c.bgpPolicyState)