Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bgpv1: Add support for eBGP-multihop in BGP control plane #25708

Merged
merged 2 commits into from Jun 7, 2023

Conversation

rastislavs
Copy link
Contributor

Extends the CiliumBGPNeighbor configuration in the CiliumBGPPeeringPolicy CRD with a new configuration option: EBGPMultihopTTL. This can be used to enable multihop feature for eBGP peers, with explicit TTL hop limit count.

Example Configuration:

apiVersion: "cilium.io/v2alpha1"
kind: CiliumBGPPeeringPolicy
metadata:
  name: tor
spec:
  virtualRouters:
  - localASN: 65001
    exportPodCIDR: true
    neighbors:
    - peerAddress: "172.0.0.1/32"
      peerASN: 65000
      eBGPMultihopTTL: 10   # >=1 means enabled, 0 means disabled, default=0.

Example state dump of peers after configuring:

# cilium bgp peers -o json
[
  {
    "applied-hold-time-seconds": 90,
    "applied-keep-alive-time-seconds": 30,
    "configured-hold-time-seconds": 90,
    "configured-keep-alive-time-seconds": 30,
    "connect-retry-time-seconds": 60,
    "ebgp-multihop-ttl": 10,
    "families": [
      {
        "advertised": 1,
        "afi": "ipv4",
        "received": 1,
        "safi": "unicast"
      },
      {
        "afi": "ipv6",
        "safi": "unicast"
      }
    ],
    "local-asn": 65001,
    "peer-address": "172.0.0.1",
    "peer-asn": 65000,
    "session-state": "established",
    "uptime-nanoseconds": 53443700101
  }
]

Example dump of a keepalive packet:

Screenshot from 2023-05-25 15-50-17

Fixes: #21753

Add support for eBGP-multihop configuration for CiliumBGPNeighbor in CiliumBGPPeeringPolicy CRD

@maintainer-s-little-helper 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 26, 2023
@rastislavs rastislavs added area/bgp release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels May 26, 2023
@maintainer-s-little-helper 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 May 26, 2023
go.mod Outdated Show resolved Hide resolved
@dylandreimerink
Copy link
Member

We should also add this feature to the docs: https://docs.cilium.io/en/stable/network/bgp-control-plane/ which are at
Documentation/network/bgp-control-plane.rst

Copy link
Member

@YutaroHayakawa YutaroHayakawa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@rastislavs rastislavs marked this pull request as ready for review June 5, 2023 06:54
@rastislavs rastislavs requested review from a team as code owners June 5, 2023 06:54
Copy link
Member

@rolinh rolinh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Vendor changes lgtm.

@rastislavs
Copy link
Contributor Author

Unfortunately, after rebasing with the recent bgpv1 unit tests changes, a unit test started triggering a timing issue that I had to fix in upstream: osrg/gobgp#2663

It seems that until the next GoBGP release we would need to stay on the version v3.15.1-0.20230605074248-03982e597eacz (one commit after the v3.15.0). Is this any problem in general in regards with Cilium dependencies @rolinh ? I see that there are multiple other dependencies like that in go.mod.

@rolinh
Copy link
Member

rolinh commented Jun 5, 2023

It seems that until the next GoBGP release we would need to stay on the version v3.15.1-0.20230605074248-03982e597eacz (one commit after the v3.15.0). Is this any problem in general in regards with Cilium dependencies @rolinh ?

No, it isn't a problem 🙂

@rastislavs
Copy link
Contributor Author

/test

Extends the CiliumBGPNeighbor CRD with a new option , which controls
the multi-hop feature for eBGP peers. If non-zero, the given value
is used in BGP packets sent to the neighbor.

Signed-off-by: Rastislav Szabo <rastislav.szabo@isovalent.com>
Updates GoBGP to the newest version to incorporate upstream fixes
for eBGP multihop and graceful restart. It can be updated to the
next patch/minor release version one it is out.

Signed-off-by: Rastislav Szabo <rastislav.szabo@isovalent.com>
@rastislavs
Copy link
Contributor Author

Cilium L4LB XDP (ci-l4lb) test failing: hitting complexity issue (#25892). Rebasing to pull the fix.

@rastislavs
Copy link
Contributor Author

/test

Copy link
Member

@nathanjsweet nathanjsweet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM for API.

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jun 6, 2023
@dylandreimerink dylandreimerink merged commit e5979b5 into cilium:main Jun 7, 2023
61 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/bgp ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CFP: Cilium BGP Control Plane supports EbgpMultihop
9 participants