BGPv1: Adds Peer Port Support

A user can annotate a node with `local-port` to have BGPRouterManager listen on a port other than 179. However, the BgpServerInstance only creates a peering connection using port 179. This means either passive mode is required when one node specifies a non standard port or two Cilium nodes cannot establish a BGP neighbor relationship when both specify a non-standard port. This PR adds an optional `PeerPort` field to the CiliumBGPVirtualRouter API type, allowing a user to specify the BGP peer's TCP port. When unspecified, the BgpServerInstance continues to use port 179. Signed-off-by: Daneyon Hansen <daneyon.hansen@solo.io>
cilium · Jun 7, 2023 · e363698 · e363698
1 parent 63db66a
commit e363698
Show file tree

Hide file tree

Showing 15 changed files with 183 additions and 27 deletions.
diff --git a/api/v1/models/bgp_peer.go b/api/v1/models/bgp_peer.go
diff --git a/api/v1/openapi.yaml b/api/v1/openapi.yaml
@@ -3534,6 +3534,11 @@ definitions:
           Time To Live (TTL) value used in BGP packets sent to the eBGP neighbor.
           0 if eBGP multi-hop feature is disabled.
         type: integer
+      peer-port:
+        description: TCP port number of peer
+        type: integer
+        minimum: 1
+        maximum: 65535
       session-state:
         description: |
           BGP peer operational state as described here

diff --git a/api/v1/server/embedded_spec.go b/api/v1/server/embedded_spec.go
diff --git a/cilium/cmd/bgp_peer_get.go b/cilium/cmd/bgp_peer_get.go
@@ -50,7 +50,7 @@ func printSummary(peers []*models.BgpPeer) {
 	for _, peer := range peers {
 		fmt.Fprintf(w, "%d\t", peer.LocalAsn)
 		fmt.Fprintf(w, "%d\t", peer.PeerAsn)
-		fmt.Fprintf(w, "%s\t", peer.PeerAddress)
+		fmt.Fprintf(w, "%s:%d\t", peer.PeerAddress, peer.PeerPort)
 		fmt.Fprintf(w, "%s\t", peer.SessionState)
 
 		// Time is rounded to nearest second

diff --git a/pkg/bgpv1/agent/controller.go b/pkg/bgpv1/agent/controller.go
@@ -10,6 +10,7 @@ import (
 	"time"
 
 	"github.com/sirupsen/logrus"
+	"k8s.io/utils/pointer"
 
 	"github.com/cilium/workerpool"
 
@@ -435,6 +436,9 @@ func (c *Controller) applyPolicyDefaults(policy *v2alpha1api.CiliumBGPPeeringPol
 	for _, r := range p.Spec.VirtualRouters {
 		for j := range r.Neighbors {
 			n := &r.Neighbors[j]
+			// Set neighbor default port for peering if unspecified. This is done by kube-apiserver
+			// but added here for non-k8s environments, tests, etc.
+			setDefaultPeerPort(n)
 			if n.ConnectRetryTime.Duration == 0 {
 				n.ConnectRetryTime.Duration = types.DefaultBGPConnectRetryTime
 			}
@@ -478,3 +482,11 @@ func (c *Controller) validatePolicy(policy *v2alpha1api.CiliumBGPPeeringPolicy)
 	}
 	return nil
 }
+
+// setDefaultPeerPort sets the PeerPort of the provided neighbor
+// to defaultPeerPort (179) if unset.
+func setDefaultPeerPort(n *v2alpha1api.CiliumBGPNeighbor) {
+	if n.PeerPort == nil {
+		n.PeerPort = pointer.Int(types.DefaultPeerPort)
+	}
+}
diff --git a/pkg/bgpv1/agent/controller_test.go b/pkg/bgpv1/agent/controller_test.go
@@ -14,6 +14,7 @@ import (
 
 	"github.com/cilium/cilium/pkg/bgpv1/agent"
 	"github.com/cilium/cilium/pkg/bgpv1/mock"
+	"github.com/cilium/cilium/pkg/bgpv1/types"
 	v2alpha1api "github.com/cilium/cilium/pkg/k8s/apis/cilium.io/v2alpha1"
 	v1 "github.com/cilium/cilium/pkg/k8s/slim/k8s/apis/meta/v1"
 	nodeaddr "github.com/cilium/cilium/pkg/node"
@@ -141,7 +142,9 @@ func TestControllerSanity(t *testing.T) {
 				defaulted := false
 				for _, r := range p.Spec.VirtualRouters {
 					for _, n := range r.Neighbors {
-						if n.ConnectRetryTime.Duration != 0 &&
+						if n.PeerPort != nil &&
+							*n.PeerPort == types.DefaultPeerPort &&
+							n.ConnectRetryTime.Duration != 0 &&
 							n.HoldTime.Duration != 0 &&
 							n.KeepAliveTime.Duration != 0 &&
 							n.GracefulRestart.RestartTime.Duration != 0 {

diff --git a/pkg/bgpv1/gobgp/server.go b/pkg/bgpv1/gobgp/server.go
@@ -110,7 +110,7 @@ func (g *GoBGPServer) AddNeighbor(ctx context.Context, n types.NeighborRequest)
 		Peer: peer,
 	}
 	if err = g.server.AddPeer(ctx, peerReq); err != nil {
-		return fmt.Errorf("failed while adding peer %v %v: %w", n.Neighbor.PeerAddress, n.Neighbor.PeerASN, err)
+		return fmt.Errorf("failed while adding peer %v:%v with ASN %v: %w", n.Neighbor.PeerAddress, *n.Neighbor.PeerPort, n.Neighbor.PeerASN, err)
 	}
 	return nil
 }
@@ -128,12 +128,12 @@ func (g *GoBGPServer) UpdateNeighbor(ctx context.Context, n types.NeighborReques
 	}
 	updateRes, err := g.server.UpdatePeer(ctx, peerReq)
 	if err != nil {
-		return fmt.Errorf("failed while updating peer %v %v: %w", n.Neighbor.PeerAddress, n.Neighbor.PeerASN, err)
+		return fmt.Errorf("failed while updating peer %v:%v with ASN %v: %w", n.Neighbor.PeerAddress, *n.Neighbor.PeerPort, n.Neighbor.PeerASN, err)
 	}
 
 	// perform full / soft peer reset if necessary
 	if needsHardReset || updateRes.NeedsSoftResetIn {
-		g.logger.Infof("Resetting peer %s (ASN %d) due to a config change", peer.Conf.NeighborAddress, peer.Conf.PeerAsn)
+		g.logger.Infof("Resetting peer %s:%v (ASN %d) due to a config change", peer.Conf.NeighborAddress, *n.Neighbor.PeerPort, peer.Conf.PeerAsn)
 		resetReq := &gobgp.ResetPeerRequest{
 			Address:       peer.Conf.NeighborAddress,
 			Communication: "Peer configuration changed",
@@ -143,7 +143,7 @@ func (g *GoBGPServer) UpdateNeighbor(ctx context.Context, n types.NeighborReques
 			resetReq.Direction = gobgp.ResetPeerRequest_IN
 		}
 		if err = g.server.ResetPeer(ctx, resetReq); err != nil {
-			return fmt.Errorf("failed while resetting peer %v %v: %w", n.Neighbor.PeerAddress, n.Neighbor.PeerASN, err)
+			return fmt.Errorf("failed while resetting peer %v:%v in ASN %v: %w", n.Neighbor.PeerAddress, *n.Neighbor.PeerPort, n.Neighbor.PeerASN, err)
 		}
 	}
 
@@ -160,6 +160,11 @@ func (g *GoBGPServer) getPeerConfig(ctx context.Context, n *v2alpha1api.CiliumBG
 	}
 	peerAddr := prefix.Addr()
 
+	peerPort := uint32(types.DefaultPeerPort)
+	if n.PeerPort != nil {
+		peerPort = uint32(*n.PeerPort)
+	}
+
 	var existingPeer *gobgp.Peer
 	if isUpdate {
 		// If this is an update, try retrieving the existing Peer.
@@ -172,8 +177,13 @@ func (g *GoBGPServer) getPeerConfig(ctx context.Context, n *v2alpha1api.CiliumBG
 		}
 		// use only necessary parts of the existing peer struct
 		peer = &gobgp.Peer{
-			Conf:     existingPeer.Conf,
-			AfiSafis: existingPeer.AfiSafis,
+			Conf:      existingPeer.Conf,
+			Transport: existingPeer.Transport,
+			AfiSafis:  existingPeer.AfiSafis,
+		}
+		// Update the peer port if needed.
+		if existingPeer.Transport.RemotePort != peerPort {
+			peer.Transport.RemotePort = peerPort
 		}
 	} else {
 		// Create a new peer
@@ -182,6 +192,9 @@ func (g *GoBGPServer) getPeerConfig(ctx context.Context, n *v2alpha1api.CiliumBG
 				NeighborAddress: peerAddr.String(),
 				PeerAsn:         uint32(n.PeerASN),
 			},
+			Transport: &gobgp.Transport{
+				RemotePort: peerPort,
+			},
 			// tells the peer we are capable of unicast IPv4 and IPv6
 			// advertisements.
 			AfiSafis: []*gobgp.AfiSafi{
@@ -203,9 +216,9 @@ func (g *GoBGPServer) getPeerConfig(ctx context.Context, n *v2alpha1api.CiliumBG
 	// when calling AddPeer / UpdatePeer / ListPeer, we set it explicitly to a wildcard address
 	// based on peer's address family, to not cause unnecessary connection resets upon update.
 	if peerAddr.Is4() {
-		peer.Transport = &gobgp.Transport{LocalAddress: wildcardIPv4Addr}
+		peer.Transport.LocalAddress = wildcardIPv4Addr
 	} else {
-		peer.Transport = &gobgp.Transport{LocalAddress: wildcardIPv6Addr}
+		peer.Transport.LocalAddress = wildcardIPv6Addr
 	}
 
 	// Enable multi-hop for eBGP if non-zero TTL is provided

diff --git a/pkg/bgpv1/gobgp/state.go b/pkg/bgpv1/gobgp/state.go
@@ -51,6 +51,10 @@ func (g *GoBGPServer) GetPeerState(ctx context.Context) (types.GetPeerStateRespo
 
 		peerState := &models.BgpPeer{}
 
+		if peer.Transport != nil {
+			peerState.PeerPort = int64(peer.Transport.RemotePort)
+		}
+
 		if peer.Conf != nil {
 			peerState.LocalAsn = int64(peer.Conf.LocalAsn)
 			peerState.PeerAddress = peer.Conf.NeighborAddress

diff --git a/pkg/bgpv1/gobgp/state_test.go b/pkg/bgpv1/gobgp/state_test.go
@@ -9,6 +9,8 @@ import (
 	"testing"
 	"time"
 
+	"k8s.io/utils/pointer"
+
 	"github.com/cilium/cilium/api/v1/models"
 	"github.com/cilium/cilium/pkg/bgpv1/types"
 	v2alpha1api "github.com/cilium/cilium/pkg/k8s/apis/cilium.io/v2alpha1"
@@ -25,6 +27,7 @@ var (
 	neighbor64125 = &v2alpha1api.CiliumBGPNeighbor{
 		PeerASN:          64125,
 		PeerAddress:      "192.168.0.1/32",
+		PeerPort:         pointer.Int(types.DefaultPeerPort),
 		ConnectRetryTime: metav1.Duration{Duration: 99 * time.Second},
 		HoldTime:         metav1.Duration{Duration: 9 * time.Second},
 		KeepAliveTime:    metav1.Duration{Duration: 3 * time.Second},
@@ -34,6 +37,7 @@ var (
 	neighbor64125Update = &v2alpha1api.CiliumBGPNeighbor{
 		PeerASN:          64125,
 		PeerAddress:      "192.168.0.1/32",
+		PeerPort:         pointer.Int(types.DefaultPeerPort),
 		ConnectRetryTime: metav1.Duration{Duration: 101 * time.Second},
 		HoldTime:         metav1.Duration{Duration: 9 * time.Second},
 		KeepAliveTime:    metav1.Duration{Duration: 3 * time.Second},
@@ -43,6 +47,7 @@ var (
 	neighbor64125UpdateGR = &v2alpha1api.CiliumBGPNeighbor{
 		PeerASN:          64125,
 		PeerAddress:      "192.168.0.1/32",
+		PeerPort:         pointer.Int(types.DefaultPeerPort),
 		ConnectRetryTime: metav1.Duration{Duration: 101 * time.Second},
 		HoldTime:         metav1.Duration{Duration: 30 * time.Second},
 		KeepAliveTime:    metav1.Duration{Duration: 10 * time.Second},
@@ -56,6 +61,7 @@ var (
 	neighbor64125UpdateGRTimer = &v2alpha1api.CiliumBGPNeighbor{
 		PeerASN:          64125,
 		PeerAddress:      "192.168.0.1/32",
+		PeerPort:         pointer.Int(types.DefaultPeerPort),
 		ConnectRetryTime: metav1.Duration{Duration: 101 * time.Second},
 		HoldTime:         metav1.Duration{Duration: 30 * time.Second},
 		KeepAliveTime:    metav1.Duration{Duration: 10 * time.Second},
@@ -68,6 +74,7 @@ var (
 	neighbor64126 = &v2alpha1api.CiliumBGPNeighbor{
 		PeerASN:          64126,
 		PeerAddress:      "192.168.66.1/32",
+		PeerPort:         pointer.Int(types.DefaultPeerPort),
 		ConnectRetryTime: metav1.Duration{Duration: 99 * time.Second},
 		HoldTime:         metav1.Duration{Duration: 9 * time.Second},
 		KeepAliveTime:    metav1.Duration{Duration: 3 * time.Second},
@@ -77,6 +84,7 @@ var (
 	neighbor64126Update = &v2alpha1api.CiliumBGPNeighbor{
 		PeerASN:          64126,
 		PeerAddress:      "192.168.66.1/32",
+		PeerPort:         pointer.Int(types.DefaultPeerPort),
 		ConnectRetryTime: metav1.Duration{Duration: 99 * time.Second},
 		HoldTime:         metav1.Duration{Duration: 12 * time.Second},
 		KeepAliveTime:    metav1.Duration{Duration: 4 * time.Second},
@@ -103,6 +111,7 @@ var (
 	neighbor64128 = &v2alpha1api.CiliumBGPNeighbor{
 		PeerASN:          64128,
 		PeerAddress:      "192.168.77.1/32",
+		PeerPort:         pointer.Int(types.DefaultPeerPort),
 		ConnectRetryTime: metav1.Duration{Duration: 99 * time.Second},
 		HoldTime:         metav1.Duration{Duration: 9 * time.Second},
 		KeepAliveTime:    metav1.Duration{Duration: 3 * time.Second},
@@ -131,6 +140,21 @@ func TestGetPeerState(t *testing.T) {
 			localASN:  64124,
 			errStr:    "",
 		},
+		{
+			name: "test add neighbor with port",
+			neighbors: []*v2alpha1api.CiliumBGPNeighbor{
+				{
+					PeerASN:          64125,
+					PeerAddress:      "192.168.0.1/32",
+					PeerPort:         pointer.Int(175),
+					ConnectRetryTime: metav1.Duration{Duration: 99 * time.Second},
+					HoldTime:         metav1.Duration{Duration: 9 * time.Second},
+					KeepAliveTime:    metav1.Duration{Duration: 3 * time.Second},
+				},
+			},
+			localASN: 64124,
+			errStr:   "",
+		},
 		{
 			name: "test add + update neighbors",
 			neighbors: []*v2alpha1api.CiliumBGPNeighbor{