From 1b3987eb379200aea3e5f2bf79d8498ca5bf39a9 Mon Sep 17 00:00:00 2001 From: Kumar Atish Date: Fri, 9 Aug 2024 19:09:12 +0530 Subject: [PATCH] Uniform BGP router ID selection for IPv4 and IPv6 Fixes #6550 Signed-off-by: Kumar Atish --- docs/bgp-policy.md | 18 ++-- pkg/agent/controller/bgp/controller.go | 22 ++--- pkg/agent/controller/bgp/controller_test.go | 98 +++++++++++++++------ 3 files changed, 94 insertions(+), 44 deletions(-) diff --git a/docs/bgp-policy.md b/docs/bgp-policy.md index ef40882cc9e..bc2678a2077 100644 --- 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 index ed303852f1a..51c667acd84 100644 --- 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 index fba9c586697..13b7041ba8e 100644 --- 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,6 +1210,48 @@ 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, @@ -1217,12 +1259,12 @@ func TestNodeUpdate(t *testing.T) { 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)