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.

[catherinetcai]

@aauren Rebased my changes against master so that there's no more merge conflicts between recent changes to network_routes_controller.go and go.mod/go.sum.

[aauren]

Looking good! I think that we're almost there.

[aauren]

It looks like the rebase here, favored your older library versions instead of the ones that were updated by dependabot. We should fix this up to favor the upgraded library versions (as well as Go 1.25).

[aauren]

2 things here... In general, I try never to split lines on chained functions in the kube-router code base unless there is no other option. In this case, I think it would be better to either wrap the parameter node.Annotations or if that doesn't get it below the char limit, then I would split the string across multiple lines.

I'm a bit iffy on this. Technically tier 2 logging should probably only be used for debugging, but technically this can also leak BGP passwords in logs. I could see going both ways on this, but usually its better to lean more secure even if that gives people less debug info.

[aauren]

nitpic, but it should be Early exit

[aauren]

nitpic: don't break the line in the middle of a function chain, instead break on the parameters or split the string if you need to.

[aauren]

It looks like this can be configured in golines via: https://github.com/segmentio/golines?tab=readme-ov-file#chained-method-splitting

[catherinetcai]

Ah, thank you. I'll go update this! And yeah, it's my neovim config (sorry)