bgpv1: Add support for setting BGP timer parameters in CiliumBGPNeighbor CRD

[rastislavs]

Extends the CiliumBGPNeighbor CRD with 3 new configuration options: ConnectRetryTime, HoldTime and KeepAliveTime. These can be used to fine-tune BGP peering, e.g. to achieve faster failover times. If not set, the default values for the affected BGP timers remain the same as before this change.

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
      holdTime: 120s
      keepAliveTime: 30s
      connectRetryTime: 60s

Example state dump of peers after configuring:

# cilium bgp peers -o json
[
  {
    "applied-hold-time-seconds": 120,
    "applied-keep-alive-time-seconds": 30,
    "configured-hold-time-seconds": 120,
    "configured-keep-alive-time-seconds": 30,
    "connect-retry-time-seconds": 60,
    "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": 8144057623
  }
]

Implementation notes:

• The default values for the timer params are the same as GoBGP uses internally, so there is no change in behaviour if they are not explicitly set.
• The timer params are defaulted in the CiliumBGPPeeringPolicy at the BGP Agent level, so that the same defaults can be consistently applied across all future BGP backends.
• The BGP Router interface has been extended with a new method UpdateNeighbor, which is called from the BGP Manager's neighborReconciler when a neighbor needs to be updated.

Add support for setting BGP timer parameters in CiliumBGPNeighbor CRD

[harsimran-pabla]

Minor comments, overall looks good

[YutaroHayakawa]

Made some comments. My main concern here is a validation of the timer values and how to feedback the wrong configurations to users.

I don't see any range checking on controller side and we currently let GoBGP to validate the value. That means we effectively leaking the underlying BGP's specification to CRD. Ideally, we should be able to control which value is valid and which is not.

Another problem of validating the value in GoBGP level is we don't have a good way to feedback the wrong configuration to users since the error happens during async reconciliation. Ideally, users should be able to notice the invalid value when they create/update the CRD. In that sense, it would be nice to validate configuration at k8s level (with OpenAPI constraint).

[nathanjsweet]

LGTM for K8s and API

[YutaroHayakawa]

Now overall looks great! At last, let me ask you to consolidate the last two commits into the first commit for readability. You made an API response name changes and some logic changes in those commits, but the final form is only meaningful, so it's more reader-friendly to only show the final form. It's fine to do force-push. It's common to do that in this project for the clean commit history.

[rastislavs]

@YutaroHayakawa

Now overall looks great! At last, let me ask you to consolidate the last two commits into the first commit for readability.

Consolidated into a single commit, as keeping the second one would require resolving too many conflicts and it was not that significant (just unit tests). Kept the gist of all commit messages in the final one.

[YutaroHayakawa]

Consolidation looks good! And sorry, one more. I noticed I didn't answer to your question here.

#25408 (comment)

What I mean by "unit" here is like a "Seconds".

[rastislavs]

@YutaroHayakawa

#25408 (comment)

What I mean by "unit" here is like a "Seconds".

Ah, so you meant including e.g. "Seconds" suffix for the timer intervals in the CRD API? I don't think it is necessary, as the datatype is metav1.Duaration (time.Duration) which means the units are defined in the value - e.g. 60s / 1m etc: connectRetryTime: 60s / connectRetryTime: 1m.

[YutaroHayakawa]

Ah, alright. That's a good point, but since we internally round them up, better to mention it in the document.

[YutaroHayakawa]

Now, LGTM! Thanks!

[rastislavs]

Ah, alright. That's a good point, but since we internally round them up, better to mention it in the document.

Fair point, added Rounded internally to the nearest whole second. to the docs.

[rastislavs]

/test

[rastislavs]

/test

[rastislavs]

/test-runtime

looks like a flake in bgpv1 Test_NeighborAddDel: #25637

(failed in https://jenkins.cilium.io/job/Cilium-PR-Runtime-kernel-net-next/120/)