Consolidate Node Peer Annotations

[catherinetcai]

Adds support for peer configuration that's in YAML format according to: #1393

I consolidated the peer configurations from both the consolidated annotation and single annotations into populating a single config struct.

Tested this by spinning up a cluster using https://github.com/aauren/kube-router-automation and adding the following annotations onto the aws-controller and aws-worker nodes and then running kubectl exec into the pods to validate that the BGP configurations were picked up.

apiVersion: v1
kind: Node
metadata:
  name: aws-controller
  annotations:
    kube-router.io/peer.ips: "10.95.0.254"
    kube-router.io/peer.asns: "4200000001"
    kube-router.io/peer.localips: "192.168.1.0"

apiVersion: v1
kind: Node
metadata:
  name: aws-worker
  annotations:
    kube-router.io/peers: |
      - remoteip: 10.0.0.1
        remoteasn: 64640
        password: cGFzc3dvcmQ=
        localip: 192.168.0.1

I'm going to keep this MR in the draft state until I'm able to do a more thorough testing cycle with kubetest2.

@aauren

[aauren]

@catherinetcai Thanks for starting work on this!

Most of my comments are nits, there are no major problems that I can see with this work. I like the fact that you pulled out all of the BGP parsing stuff into functions on a struct. I think that's already going a long ways towards making the code more readable.

The other main thing I was looking for was keeping backwards compatibility, but it looks like you've done that pretty well.

The configuration structure itself looks pretty good, although I'm still mulling that over a bit to see if there would be some way to make some of the items less duplicated. For instance, people are likely to have similar Password, RemoteASN, and RemoteIP for many of the peers. Maybe it would be possible to define a global default that people could specify once, and then allow individual entries to override that default when necessary?

Or maybe we could introduce a concept of peer groups? https://github.com/osrg/gobgp/blob/master/docs/sources/configuration.md?plain=1#L175-L187 which would follow more of the gobgp standard?

FRR has this standard as well: https://github.com/aauren/kube-router-automation/blob/main/ansible/playbooks/roles/bgp_router/templates/frr.conf.j2#L36-L42

[catherinetcai]

@catherinetcai Thanks for starting work on this!

Most of my comments are nits, there are no major problems that I can see with this work. I like the fact that you pulled out all of the BGP parsing stuff into functions on a struct. I think that's already going a long ways towards making the code more readable.

The other main thing I was looking for was keeping backwards compatibility, but it looks like you've done that pretty well.

The configuration structure itself looks pretty good, although I'm still mulling that over a bit to see if there would be some way to make some of the items less duplicated. For instance, people are likely to have similar Password, RemoteASN, and RemoteIP for many of the peers. Maybe it would be possible to define a global default that people could specify once, and then allow individual entries to override that default when necessary?

Or maybe we could introduce a concept of peer groups? https://github.com/osrg/gobgp/blob/master/docs/sources/configuration.md?plain=1#L175-L187 which would follow more of the gobgp standard?

FRR has this standard as well: https://github.com/aauren/kube-router-automation/blob/main/ansible/playbooks/roles/bgp_router/templates/frr.conf.j2#L36-L42

I really like the idea of following more of the GoBGP standard. Do you have any thoughts for how those should be passed in?

[aauren]

@catherinetcai Thanks for starting work on this!
Most of my comments are nits, there are no major problems that I can see with this work. I like the fact that you pulled out all of the BGP parsing stuff into functions on a struct. I think that's already going a long ways towards making the code more readable.
The other main thing I was looking for was keeping backwards compatibility, but it looks like you've done that pretty well.
The configuration structure itself looks pretty good, although I'm still mulling that over a bit to see if there would be some way to make some of the items less duplicated. For instance, people are likely to have similar Password, RemoteASN, and RemoteIP for many of the peers. Maybe it would be possible to define a global default that people could specify once, and then allow individual entries to override that default when necessary?
Or maybe we could introduce a concept of peer groups? https://github.com/osrg/gobgp/blob/master/docs/sources/configuration.md?plain=1#L175-L187 which would follow more of the gobgp standard?
FRR has this standard as well: https://github.com/aauren/kube-router-automation/blob/main/ansible/playbooks/roles/bgp_router/templates/frr.conf.j2#L36-L42

I really like the idea of following more of the GoBGP standard. Do you have any thoughts for how those should be passed in?

Heh... I think that it felt more apparent to me when I hadn't thought it through all the way. I was thinking that you could just add a peerGroup to the annotation yaml and then reference it. But this is per-node not cluster wide. I suppose that we could do something like add a config file or allow users to add a group via a parameter to kube-router, but those all feel a bit like a hack.

I guess for now we leave it as it is. But maybe in the future, we add a CRD or something? This is similar to what Cilium does: https://docs.cilium.io/en/stable/network/bgp-control-plane/bgp-control-plane-v2 Although theirs is a bit different because they have nodeSelectors in their BGP config so that users can control node applications that way. Which I suppose is more k8s idiomatic.

[aauren]

If we return an error here, then we wouldn't actually use the default option right? I believe that returning an error here would mean that the BGP server would fail to start in startBGPServer() right?

Its possible that I misunderstood the code path here, but if I'm right, then we may want to change the helpers above to have an else statement so that we always have a default. Then remove the default blurb from this message as at this point we'll already have the default?

Alternatively we could turn these into a warn or an info log rather than an error.

[catherinetcai]

I looked back at the original implementation and while I think this validation causes there to be an earlier exit, the end result seems to ultimately be the same.

Going to just write out my interpretation of the original logic since it's very likely I'm just reading this incorrectly:

• We grab the list of peer ports here
• We then pass that list of peer ports (along with everything else) to newGlobalPeers. This is where I lifted the validation logic from. This does the same length check and returns an error if the length of the ports don't match the length of IPs.
• If an error is returned from newGlobalPeers, we end up calling StopBgp

I wonder based off of this comment if the original implementation had a bug and was supposed to actually look like this:

// Default to default BGP port if port annotation is not found
var peerPorts = make([]uint32, len(peerIPs))

for i := range peerPorts {
  peerPorts[i] = options.DefaultBgpPort
}

Or - the validation was supposed to warn (like you pointed out) and not actually return an error, since later in newGlobalPeers, we do construct the Peer structs with a default port.

---

Anyway - I think this might be a moot point since I don't think this was desirable behavior to begin with, so I'll update this MR to follow what you were suggesting 😄

[catherinetcai]

I looked back at the original implementation and while I think this validation causes there to be an earlier exit, the end result seems to ultimately be the same.

Going to just write out my interpretation of the original logic since it's very likely I'm just reading this incorrectly:

* We grab the list of peer ports [here](https://github.com/cloudnativelabs/kube-router/blob/00b46196001975939be6c1672f42a1f8e7990cc9/pkg/controllers/routing/network_routes_controller.go#L1134-L1148)

* We then pass that list of peer ports (along with everything else) to [newGlobalPeers](https://github.com/cloudnativelabs/kube-router/blob/00b46196001975939be6c1672f42a1f8e7990cc9/pkg/controllers/routing/bgp_peers.go#L303-L307). This is where I lifted the validation logic from. This does the same length check and returns an error if the length of the ports don't match the length of IPs.

* If an error is returned from newGlobalPeers, we end up [calling StopBgp](https://github.com/cloudnativelabs/kube-router/blob/00b46196001975939be6c1672f42a1f8e7990cc9/pkg/controllers/routing/network_routes_controller.go#L1199-L1206)

I wonder based off of this comment if the original implementation had a bug and was supposed to actually look like this:

// Default to default BGP port if port annotation is not found
var peerPorts = make([]uint32, len(peerIPs))

for i := range peerPorts {
  peerPorts[i] = options.DefaultBgpPort
}

Or - the validation was supposed to warn (like you pointed out) and not actually return an error, since later in newGlobalPeers, we do construct the Peer structs with a default port.

Anyway - I think this might be a moot point since I don't think this was desirable behavior to begin with, so I'll update this MR to follow what you were suggesting 😄

Ah never mind. You can ignore me. I'm just apparently arguing with myself now. The thing I was missing again was that the original implementation meant you had peer ports in the form of 179,,179 to make it pass the length check. Sorry - I'm finally picking up what you're putting down now.