BGPv1: Adds Peer Port Support #25809

danehans · 2023-05-31T23:22:07Z

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.

Fixes: #24737
Fixes: #25683

Adds `peerPort` field to CiliumBGPPeeringPolicy for specifying the port of a BGP neighbor. If unspecified, port 179 is used.

danehans · 2023-06-01T16:02:42Z

Successfully tested on kind using port 42424 for both peers:

level=info msg="Cilium BGP Control Plane Controller woken for reconciliation" component=Controller.Run subsys=bgp-control-plane
level=debug msg="Successfully listed CiliumBGPPeeringPolicies" component=Controller.Reconcile count=2 subsys=bgp-control-plane
level=debug msg="Comparing BGP policy node selector with node's labels" component=PolicySelection nodeLabels="beta.kubernetes.io/arch=arm64,beta.kubernetes.io/os=linux,kubernetes.io/arch=arm64,kubernetes.io/hostname=kind-control-plane,kubernetes.io/os=linux,node-role.kubernetes.io/control-plane=,node.kubernetes.io/exclude-from-external-load-balancers=" policyNodeSelector="kubernetes.io/hostname=kind-worker" subsys=bgp-control-plane
level=debug msg="Comparing BGP policy node selector with node's labels" component=PolicySelection nodeLabels="beta.kubernetes.io/arch=arm64,beta.kubernetes.io/os=linux,kubernetes.io/arch=arm64,kubernetes.io/hostname=kind-control-plane,kubernetes.io/os=linux,node-role.kubernetes.io/control-plane=,node.kubernetes.io/exclude-from-external-load-balancers=" policyNodeSelector="kubernetes.io/hostname=kind-control-plane" subsys=bgp-control-plane
level=debug msg="Asking configured BGPRouterManager to configure peering" component=Controller.Reconcile subsys=bgp-control-plane
level=debug msg="Reconciling new CiliumBGPPeeringPolicy" component=manager.ConfigurePeers diff="Registering: [65340] Withdrawing: [] Reconciling: []" subsys=bgp-control-plane
level=info msg="Registering BGP servers for policy with local ASN 65340" component=manager.registerBGPServer subsys=bgp-control-plane
level=debug msg="Preflight for virtual router with ASN 65340 not necessary, first instantiation of this BgpServer." component=manager.preflightReconciler subsys=bgp-control-plane
level=debug msg="Begin reconciling peers for virtual router with local ASN 65340" component=manager.neighborReconciler subsys=bgp-control-plane
level=info msg="Reconciling peers for virtual router with local ASN 65340" component=manager.neighborReconciler subsys=bgp-control-plane
level=info msg="Adding peer 172.18.0.3/32 42424 65341 to local ASN 65340" component=manager.neighborReconciler subsys=bgp-control-plane
level=info msg="Add a peer configuration" Key=172.18.0.3 Topic=Peer asn=65340 component=gobgp.BgpServerInstance subsys=bgp-control-plane
level=debug msg="IdleHoldTimer expired" Duration=0 Key=172.18.0.3 Topic=Peer asn=65340 component=gobgp.BgpServerInstance subsys=bgp-control-plane
level=debug msg="state changed" Key=172.18.0.3 Topic=Peer asn=65340 component=gobgp.BgpServerInstance new=BGP_FSM_ACTIVE old=BGP_FSM_IDLE reason=idle-hold-timer-expired subsys=bgp-control-plane
level=info msg="Done reconciling peers for virtual router with local ASN 65340" component=manager.neighborReconciler subsys=bgp-control-plane
level=debug msg="Begin reconciling pod CIDR advertisements for virtual router with local ASN 65340" component=manager.exportPodCIDRReconciler subsys=bgp-control-plane
level=debug msg="pod CIDR advertisements disabled for virtual router with local ASN 65340" component=manager.exportPodCIDRReconciler subsys=bgp-control-plane
level=info msg="Successfully registered GoBGP servers for policy with local ASN 65340" component=manager.registerBGPServer subsys=bgp-control-plane
level=debug msg="Successfully completed reconciliation" component=Controller.Run subsys=bgp-control-plane
level=info msg="type:STATE peer:{conf:{local_asn:65340 neighbor_address:\"172.18.0.3\" peer_asn:65341} state:{local_asn:65340 neighbor_address:\"172.18.0.3\" peer_asn:65341 session_state:IDLE router_id:\"<nil>\"} transport:{local_address:\"<nil>\"}}"
level=info msg="type:STATE peer:{conf:{local_asn:65340 neighbor_address:\"172.18.0.3\" peer_asn:65341} state:{local_asn:65340 neighbor_address:\"172.18.0.3\" peer_asn:65341 session_state:ACTIVE router_id:\"<nil>\"} transport:{local_address:\"<nil>\"}}"
...
level=debug msg="Accepted a new passive connection" Key=172.18.0.3 Topic=Peer asn=65340 component=gobgp.BgpServerInstance subsys=bgp-control-plane
level=debug msg="stop connect loop" Key=172.18.0.3 Topic=Peer asn=65340 component=gobgp.BgpServerInstance subsys=bgp-control-plane
level=debug msg="state changed" Key=172.18.0.3 Topic=Peer asn=65340 component=gobgp.BgpServerInstance new=BGP_FSM_OPENSENT old=BGP_FSM_ACTIVE reason=new-connection subsys=bgp-control-plane
level=info msg="type:STATE peer:{conf:{local_asn:65340 neighbor_address:\"172.18.0.3\" peer_asn:65341} state:{local_asn:65340 neighbor_address:\"172.18.0.3\" peer_asn:65341 session_state:OPENSENT router_id:\"<nil>\"} transport:{local_address:\"172.18.0.2\" local_port:42424 remote_port:57129}}"
level=debug msg="state changed" Key=172.18.0.3 Topic=Peer asn=65340 component=gobgp.BgpServerInstance new=BGP_FSM_OPENCONFIRM old=BGP_FSM_OPENSENT reason=open-msg-received subsys=bgp-control-plane
level=info msg="type:STATE peer:{conf:{local_asn:65340 neighbor_address:\"172.18.0.3\" peer_asn:65341} state:{local_asn:65340 neighbor_address:\"172.18.0.3\" peer_asn:65341 session_state:OPENCONFIRM router_id:\"172.18.0.3\"} transport:{local_address:\"172.18.0.2\" local_port:42424 remote_port:57129}}"
level=info msg="Peer Up" Key=172.18.0.3 State=BGP_FSM_OPENCONFIRM Topic=Peer asn=65340 component=gobgp.BgpServerInstance subsys=bgp-control-plane
level=debug msg="state changed" Key=172.18.0.3 Topic=Peer asn=65340 component=gobgp.BgpServerInstance new=BGP_FSM_ESTABLISHED old=BGP_FSM_OPENCONFIRM reason=open-msg-negotiated subsys=bgp-control-plane
level=info msg="type:STATE peer:{conf:{local_asn:65340 neighbor_address:\"172.18.0.3\" peer_asn:65341} state:{local_asn:65340 neighbor_address:\"172.18.0.3\" peer_asn:65341 session_state:ESTABLISHED router_id:\"172.18.0.3\"} transport:{local_address:\"172.18.0.2\" local_port:42424 remote_port:57129}}"

danehans · 2023-06-01T18:17:25Z

@YutaroHayakawa PTAL when you have a moment.

harsimran-pabla

Thanks for this change.

Can you also please update component tests to use correct peer-port. Tests are in pkg/bgpv1/test directory.

cilium/cmd/bgp_peer_get.go

pkg/k8s/apis/cilium.io/v2alpha1/bgpp_types.go

pkg/bgpv1/manager/reconcile.go

pkg/bgpv1/gobgp/server.go

YutaroHayakawa

Thanks for working on this! I made some comments.

pkg/k8s/apis/cilium.io/v2alpha1/bgpp_types.go

pkg/bgpv1/manager/reconcile.go

pkg/bgpv1/manager/reconcile_test.go

pkg/k8s/apis/cilium.io/v2alpha1/bgpp_types.go

YutaroHayakawa

Sorry, I mistakenly approved.

danehans · 2023-06-02T17:30:32Z

Note: I created #25869 for updating user docs.

danehans · 2023-06-02T18:45:10Z

Thanks @rastislavs, @harsimran-pabla, and @YutaroHayakawa for the review. Commit a21d720 resolves your feedback and allows PeerPort of a neighbor to be updated. PTAL when you have a moment.

pkg/bgpv1/agent/controller.go

danehans · 2023-06-05T20:58:06Z

Commit 78a4ff4 rebases to resolve merge conflicts.

danehans · 2023-06-06T16:51:41Z

Commit db847a1 resolved merge conflicts.

nathanjsweet

Approved for API.

YutaroHayakawa

Now looks good to me 👍 Thanks for your contribution!

YutaroHayakawa · 2023-06-07T01:48:15Z

/test

YutaroHayakawa · 2023-06-07T06:39:28Z

Ugh, we still need rebase

pkg/bgpv1/agent/controller.go

pkg/bgpv1/types/bgp.go

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>

rastislavs · 2023-06-07T20:18:18Z

/test

rastislavs · 2023-06-08T11:14:16Z

Hit flakes in ConformanceEKS (ci-eks): #26008 and #26014

Conformance ginkgo (ci-ginkgo) is not required.

Marking as ready to merge.

danehans requested review from a team as code owners May 31, 2023 23:22

danehans requested review from youngnick, ldelossa and squeed May 31, 2023 23:22

maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label May 31, 2023

danehans requested a review from nathanjsweet May 31, 2023 23:22

github-actions bot added the kind/community-contribution This was a contribution made by a community member. label May 31, 2023

danehans force-pushed the issue_24737 branch 2 times, most recently from 1508c56 to cd9ec69 Compare June 1, 2023 03:21

danehans force-pushed the issue_24737 branch from cd9ec69 to eae37c9 Compare June 1, 2023 17:35

squeed approved these changes Jun 1, 2023

View reviewed changes

harsimran-pabla requested changes Jun 1, 2023

View reviewed changes

cilium/cmd/bgp_peer_get.go Outdated Show resolved Hide resolved

pkg/k8s/apis/cilium.io/v2alpha1/bgpp_types.go Outdated Show resolved Hide resolved

pkg/bgpv1/manager/reconcile.go Outdated Show resolved Hide resolved

pkg/bgpv1/gobgp/server.go Outdated Show resolved Hide resolved

YutaroHayakawa reviewed Jun 2, 2023

View reviewed changes

pkg/bgpv1/gobgp/server.go Outdated Show resolved Hide resolved

YutaroHayakawa approved these changes Jun 2, 2023

View reviewed changes

YutaroHayakawa requested changes Jun 2, 2023

View reviewed changes

rastislavs added area/bgp release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels Jun 2, 2023

maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jun 2, 2023

danehans mentioned this pull request Jun 2, 2023

BGP CP: Document PeerPort #25869

Closed

danehans force-pushed the issue_24737 branch 3 times, most recently from d559e66 to a21d720 Compare June 2, 2023 18:39

harsimran-pabla reviewed Jun 2, 2023

View reviewed changes

pkg/bgpv1/agent/controller.go Outdated Show resolved Hide resolved

YutaroHayakawa requested changes Jun 5, 2023

View reviewed changes

pkg/bgpv1/agent/controller.go Outdated Show resolved Hide resolved

pkg/bgpv1/agent/controller.go Outdated Show resolved Hide resolved

qmonnet added the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Jun 5, 2023

danehans requested a review from YutaroHayakawa June 5, 2023 15:23

danehans force-pushed the issue_24737 branch 2 times, most recently from e092c80 to 841ae0f Compare June 5, 2023 19:25

youngnick removed their request for review June 5, 2023 20:28

danehans force-pushed the issue_24737 branch 3 times, most recently from 52d6816 to 78a4ff4 Compare June 5, 2023 20:47

danehans force-pushed the issue_24737 branch from 78a4ff4 to db847a1 Compare June 6, 2023 16:01

nathanjsweet approved these changes Jun 6, 2023

View reviewed changes

YutaroHayakawa approved these changes Jun 7, 2023

View reviewed changes

YutaroHayakawa removed the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Jun 7, 2023

YutaroHayakawa added the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Jun 7, 2023

rastislavs reviewed Jun 7, 2023

View reviewed changes

pkg/bgpv1/agent/controller.go Outdated Show resolved Hide resolved

danehans force-pushed the issue_24737 branch from db847a1 to 11da300 Compare June 7, 2023 17:00

danehans commented Jun 7, 2023

View reviewed changes

pkg/bgpv1/types/bgp.go Show resolved Hide resolved

danehans force-pushed the issue_24737 branch from 11da300 to e363698 Compare June 7, 2023 18:01

rastislavs approved these changes Jun 7, 2023

View reviewed changes

rastislavs removed the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Jun 7, 2023

rastislavs added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jun 8, 2023

dylandreimerink merged commit 6b8adac into cilium:main Jun 8, 2023
63 of 64 checks passed

rastislavs mentioned this pull request Jun 8, 2023

bgpv1: Consolidate CRD API to follow K8s API Conventions #26040

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BGPv1: Adds Peer Port Support #25809

BGPv1: Adds Peer Port Support #25809

danehans commented May 31, 2023

danehans commented Jun 1, 2023

danehans commented Jun 1, 2023

harsimran-pabla left a comment

YutaroHayakawa left a comment

YutaroHayakawa left a comment

danehans commented Jun 2, 2023

danehans commented Jun 2, 2023

danehans commented Jun 5, 2023

danehans commented Jun 6, 2023

nathanjsweet left a comment

YutaroHayakawa left a comment

YutaroHayakawa commented Jun 7, 2023

YutaroHayakawa commented Jun 7, 2023

rastislavs commented Jun 7, 2023

rastislavs commented Jun 8, 2023

BGPv1: Adds Peer Port Support #25809

BGPv1: Adds Peer Port Support #25809

Conversation

danehans commented May 31, 2023

danehans commented Jun 1, 2023

danehans commented Jun 1, 2023

harsimran-pabla left a comment

Choose a reason for hiding this comment

YutaroHayakawa left a comment

Choose a reason for hiding this comment

YutaroHayakawa left a comment

Choose a reason for hiding this comment

danehans commented Jun 2, 2023

danehans commented Jun 2, 2023

danehans commented Jun 5, 2023

danehans commented Jun 6, 2023

nathanjsweet left a comment

Choose a reason for hiding this comment

YutaroHayakawa left a comment

Choose a reason for hiding this comment

YutaroHayakawa commented Jun 7, 2023

YutaroHayakawa commented Jun 7, 2023

rastislavs commented Jun 7, 2023

rastislavs commented Jun 8, 2023