BGPv1: Introduce generic bgp manager layer

[harsimran-pabla]

BGP subsystem refactor

TLDR

Introducing new manager layer in BGP subsystem, which decouples itself from gobgp structures. Also, using UUID in identifying paths instead of gobgp path pointers. This change is mostly refactoring code and not introducing any functionality.

Motivation/Problem Statement

The BGP subsystem was designed to be modular and support multiple BGP backends. In current architecture, BGP subsystem comprises of three parts controller, reconcilers, and the route manager. Controller works on Kubernetes specific events, whereas reconcilers and route manager use gobgp specific structures.

There is heavy usage of gobgp library in reconcilers and route manager. This raises few concerns

• If we want to introduce new backend, we will have lot of refactoring to do to achieve it.
• We are about to add many new features in BGP, which will make this decoupling difficult going forward.

flowchart 
    subgraph agent
        agent:node_spec("node event")
        agent:policy_spec("policy event")
        agent:signal("control loop")
    end
    agent:node_spec---agent:signal
    agent:policy_spec---agent:signal

    subgraph gobgp
        gobgp:router_manager("router manager")
        gobgp:reconcilers("reconcilers")
        gobgp:servers("server configs")
        gobgp:gobgp("gobgp client")
    end
    agent:signal---gobgp:router_manager
    gobgp:router_manager-->gobgp:servers
    gobgp:router_manager-->gobgp:reconcilers
    gobgp:reconcilers-->gobgp:gobgp

Goal

Introduce cilium specific BGP types and use those in reconcilers.

flowchart 
    subgraph agent
        agent:node_spec("node event")
        agent:policy_spec("policy event")
        agent:signal("control loop")
    end
    agent:node_spec---agent:signal
    agent:policy_spec---agent:signal

    subgraph manager
        manager:router_manager("router manager")
        manager:reconcilers("reconcilers")
        manager:servers("server configs")
    end
    agent:signal---manager:router_manager
    manager:router_manager-->manager:servers
    manager:router_manager-->manager:reconcilers

    subgraph types
        types:bgp("cilium bgp")
        types:router("bgp interface")
    end

    subgraph gobgp
    gobgp:gobgp("gobgp client")
    end

    manager:reconcilers-->gobgp:gobgp

Change

This change introduces new manager layer and BGP types which are cilium specific.

• Reconcilers and route manager is moved into generic manager layer.
  • Configuration is declarative up to the layer
  • Running configuration is stored in route manager, as changes are detected by controller, new declarative config is pushed into route manager.
  • Reconcilers calculate the diff and calls underlying routing process/library.
  • Routing process is called using synchronous API calls, and running state is updated in route manager.
• Note: this is current behaviour as well, this change is formalizing this contract between reconcilers and underlying routing layer.
• Generic BGP types are introduced
  • Cilium specific BGP data types
  • Router Spec which underlying routing shim layer implements ( like AddNeighbor, RemoveNeighbor etc )

Impact

There is no functional impact of this change.

[ldelossa]

@harsimran-pabla I have no major objections to these changes.

I like the idea of creating a generic layer, and the initial set of changes are palpable. I think we get these changes upstream, and then back down stream, we can continue in this direction.

[harsimran-pabla]

/test

[ldelossa]

general nit, acronyms should be fully capitalized.

s/Asn/ASN
s/Bgp/BGP

and any others.

[harsimran-pabla]

@ldelossa Some of the parameters are in gobgp API, which are defining ASN as Asn. :(

[ldelossa]

Aha! totally missed that. Okay, disregard :).

[harsimran-pabla]

/test

[harsimran-pabla]

/test

[dylandreimerink]

LGTM!

[harsimran-pabla]

/test-runtime

[harsimran-pabla]

/test-1.26-net-next

[YutaroHayakawa]

If this is GoBGP-specific, how about making it clear by name? Like GoBGPPathUuid?

[YutaroHayakawa]

And UUID is an acronym, so can be capitalized.

[YutaroHayakawa]

Better to document why this is signed 32bit (port number is unsigned 16bit in principle) and -1 has a special meaning.

[YutaroHayakawa]

I could find some gobgp leftover under the manager package. Some of them are necessary (e.g. NewGoBGPServerWithConfig), but others are not, I think (e.g. "component" field of the log).

$ rg gobgp pkg/bgpv1/manager
pkg/bgpv1/manager/instance.go
9:	"github.com/cilium/cilium/pkg/bgpv1/gobgp"
49:	s, err := gobgp.NewGoBGPServerWithConfig(ctx, log, global)

pkg/bgpv1/manager/manager.go
31:	// 'component=gobgp.{Struct}.{Method}' or 'component=gobgp.{Function}' to
35:	// `subsys=bgp-control-plane`, `component=gobgp.BgpServerInstance` and
52:// This BGPRouterMananger utilizes the gobgp project to implement a BGP routing
183:// registerBGPServer encapsulates the logic for instantiating a gobgp
368:// GetPeers gets peering state from previously initialized gobgp instances.

pkg/bgpv1/manager/reconcile.go
80:// permanent configurations for gobgp BgpServers (ones that cannot be changed after creation)
91:				"component": "gobgp.preflightReconciler",
226:				"component": "gobgp.neighborReconciler",
373:		component: "gobgp.exportPodCIDRReconciler",

Also, we should update the README.md as well.

[YutaroHayakawa]

Made some comments. Most of them are very easy to fix I believe. Otherwise, looks very nice to me 👍

[harsimran-pabla]

/test

[YutaroHayakawa]

Nit: gobgp.GoBGPPathUUID => gobgp.PathUUID?

[harsimran-pabla]

oh this is result of goland magic refactor :)

[YutaroHayakawa]

Now LGTM! @dylandreimerink I'm sorry, I accidentally re-requested your review. Please approve again if necessary 🙇

[harsimran-pabla]

/test