Metallb: Add support for "node peers"

[johananl]

Introduction

We use MetalLB in our k8s distro and are generally very happy with it. We use the BGP mode exclusively. Our main use case is k8s clusters running on Packet, i.e. peering with Packet's ToR switches which run BGP.

The Packet network topology requires us to use per-node BGP peering configuration where each node forms a BGP session with a dedicated IP address on its ToR switch (similar to https://github.com/metallb/metallb/issues/52). To emphasize, each node has its own remote IP address with which it needs to peer, which is true even when multiple nodes are connected to the same ToR switch. I believe this is the case for all Packet users.

Here is a sample configuration which demonstrates the use case:

apiVersion: v1
kind: ConfigMap
metadata:
  namespace: metallb-system
  name: config
data:
  config: |
    peers:
    - node-selectors:
      - match-labels:
        kubernetes.io/hostname: worker-0
      peer-address: 10.64.43.10
      peer-asn: 65530
      my-asn: 65000
      hold-time: 3s
    - node-selectors:
      - match-labels:
          kubernetes.io/hostname: worker-1
      peer-address: 10.64.54.2
      peer-asn: 65530
      my-asn: 65000
      hold-time: 3s

    address-pools:
    - name: default
      protocol: bgp
      addresses:
      - 147.73.140.49/32

Problem

We deploy MetalLB automatically while bootstrapping a k8s cluster. Our users typically deploy clusters in an automated way (from version-controlled configuration files), which means we can't rely on manual steps that the user needs to do in order to get a functional cluster (in this context, "functional" means "a cluster with working load balancing"). This means we need both the initial deployment of MetalLB as well as any dynamic change due to node addition/removal/failure to be completely automatic.

Due to MetalLB's reliance on a user-defined ConfigMap, we are facing the following challenges:

• Deploying MetalLB requires populating the ConfigMap with peer IP addresses which aren't known at cluster creation time.
• When nodes are added to the cluster, the ConfigMap must be updated manually before BGP peers can be established for the newly-added nodes.
• When a node fails and a new node is spawned to take its place, the new node doesn't establish a BGP session because it doesn't have a matching peer configuration in the ConfigMap.

To emphasize, the problem isn't just having per-node BGP peers (addressed in https://github.com/metallb/metallb/issues/52) but also updating peers automatically in dynamic clusters.

Ruled Out Solutions

Before considering contributing a new feature to MetalLB (or forking it if the use case is too specialized), we've evaluated a couple of simpler approaches to address the problem. Unfortunately, none of them seem to fully address the problem in a maintainable and reliable way.

Smart Node Selectors

Due to the fact that each node needs to peer with a different address, we can't just specify a single peer configuration with a node selector which matches all the relevant nodes. When a node fails and gets replaced, the new node may have a different hostname from the failed node as well as a different remote peer IP, which means the peer configuration of the old node doesn't apply to the new node.

Automating ConfigMap Generation

While generating the ConfigMap programmatically is probably possible, even if we find a way to "squeeze" this logic between creating a cluster and deploying MetalLB, we would still have the problem of added/replaced nodes. The ConfigMap would have to be kept up to date with the current node statue at all times for BGP peers to work reliably in dynamic clusters.

Running a Separate Node Controller

Running a separate k8s controller which watches Node objects and updates the ConfigMap is most likely possible, however this isn't a very elegant solution in my opinion.

Addressing the Problem in MetalLB

I've seen the discussion in https://github.com/metallb/metallb/issues/196 about switching to CRDs. While I do like the idea, if I understand it correctly this isn't going to address the use case above since now the user has to create and edit CRs instead of a ConfigMap but the rest stays the same. In other words, something would still have to create or update custom resources when nodes are added or removed in order to maintain peers for all the nodes in a cluster.

Therefore, I would like to gather initial feedback about the following proposed design for a new MetalLB feature which could address the above use case while possibly making MetalLB more flexible, without replacing or interacting negatively with existing functionality.

The main thing I'm unsure about is whether this functionality is generic enough to justify modifying MetalLB in the way described below. While I strongly believe anyone who wants to run MetalLB on Packet is going to encounter the same problem, I don't know how many of the MetalLB users run clusters on Packet.

In any case, if this looks like a sane functionality to add to MetalLB, we (Kinvolk) are happy to help with the implementation.

Proposed Design

General

I propose adding the notion of "node peers" to MetalLB. A node peer is a BGP peer that's configured for a specific k8s node. The peer configuration is specified in the Node k8s object corresponding to the machine for which the peer should be established rather than in a centralized ConfigMap. Since a node peer is always associated with a specific node, node peers don't have—or need—configurable node selectors. Rather, a node peer is always implicitly associated with the Node object on which the peer configuration is applied.

Rationale

The main assumption—and the rationale for this design—is that node machines can figure out their own remote peer addresses after booting. On Packet this is as simple as checking the gateway address for the private NIC, which is also exposed by the Packet metadata service. If a node can then specify its own "discovered" configuration in a place where MetalLB can pick it up, we now practically have k8s nodes which can "self-configure" their BGP peers.

NOTE: I'm deliberately focusing on the remote peer IP here. While a basic BGP configuration also requires configuring ASNs, hold times etc., these other parameters are typically shared among all nodes in a cluster and therefore are less of a problem here.

Choosing Where to Store Configuration

A question which remains to be figured out is where to store the BGP configuration. The natural place is probably node annotations since they allow specifying arbitrary values on Node objects, however at the time of writing kubelet is unable to specify its own annotations when joining a cluster.

This makes me think that, while not ideal, we can specify node peer configuration using k8s labels. Node labels can be specified using the --node-labels kubelet flag. Many k8s distros support specifying node labels as part of the cluster bootstrap process, which makes it easy to create Node objects which contain configuration for their own node peers. This way, as long as a worker node is smart enough to figure out its own remote peer address and specify this value in the --node-labels kubelet argument, we have solved the problem.

If implemented using lables, node peer configuration could look similar to the following:

• metallb.universe.tf/my-asn=65000
• metallb.universe.tf/peer-asn=65530
• metallb.universe.tf/peer-address=10.1.1.1
• metallb.universe.tf/peer-port=179
• metallb.universe.tf/router-id=10.1.1.2
• metallb.universe.tf/hold-time=3s

NOTE: I've deliberately omitted the BGP password parameter from the example above. I am very uncomfortable specifying passwords in k8s labels, and while writing passwords to a ConfigMap practically exposes it as clear text as well, I feel like this topic should be open for discussion. https://github.com/metallb/metallb/issues/408 discusses storing passwords in k8s secrets, which makes a lot of sense to me.

Downsides

One downside of the proposed approach is that MetalLB configuration could come from multiple sources. The ConfigMap would no longer be a single source of truth for configuration. This creates the following concerns:

• Configuration would be harder to debug, i.e. it would become harder to understand why MetalLB is doing something. To mitigate this, MetalLB could expose an endpoint which would expose the "merged" configuration.
• We need to define the behavior when a user specifies conflicting BGP configuration using both node peers and the ConfigMap.

Implementation

AFAICT, implementing this feature would require making changes to the speaker codebase. The SetNode() method on the BGP controller seems to be the right place to trigger the node peer reconciliation logic since this method is triggered on changes to Node objects. The new logic would have to look for the specific labels on the Node object and try to construct a valid peer configuration from them.

In terms of node selectors, I think we could always create a single node selector with kubernetes.io/hostname=<name-of-changed-node>, thus ensuring the node peer is created only on the node which correlates to the changed Node object.

Invalid or partial node peer configurations could probably be logged and ignored without affecting the rest of the reconciliation logic.

We should figure out if it makes sense to append node peers to the same data structure as the one used for peers read from the ConfigMap (https://github.com/metallb/metallb/blob/b6678ebb6cb7b0ec591e88de9b34808d59212b1e/speaker/bgp_controller.go#L43). I foresee two problems with doing so:

1. The SetConfig() and SetNode() methods of the BGP controller are triggered on different events: the former is triggered on ConfigMap changes and the latter—on node changes. Using a single slice to store all peers will likely require us to have separate treatment for the two types of peers since the update and removal logic for node peers is going to be different from "regular" peers. For example, we don't want to remove a node peer when SetConfig() doesn't find it in the ConfigMap.
2. We will likely have to look up node peers based on node name since node peers are always associated with one node. This, IMO, hints strongly in the direction of using a map instead of a slice to avoid unnecessary iteration, which can become costly in large clusters, especially if we have to do reflect.DeepEqual() to figure out if the current peer is the one we are interested in.

In terms of interaction with the existing functionality (configuring peers via a ConfigMap), I think that as long as the user doesn't specify the same peer configuration using both a ConfigMap and node labels, things should just work. We probably want to actively check that we don't attempt to create the same BGP session twice as this will likely cause things to break, and decide what we do in this case (ignore the peer and log an error, decide that one config option always takes precedence etc.).

To address the observability problem with multiple configuration sources, we should look into adding an HTTP endpoint which allows querying the "final", in-memory config of MetalLB. This endpoint should probably be exposed by the speaker since only the speaker knows about node peers. We need to figure out if this makes sense since the speaker runs as a DaemonSet, which means that one would have to query multiple endpoints to get the whole picture. OTOH node peers are always per-node, so as long as it's the speaker which holds peering configuration I don't see how we could have one centralized endpoint for querying for configuration.

[sfudeus]

Just out of curiousity: Why do you need node-specific peer-ips on your ToR switches? Is this a precondition in packet?
In out on-premise setup, we peer with ToR switches as well, but we have a ToR label set on each node and based on that, the right peer is selected - but multiple hosts would peer with the same peerIP (would, because we currently limit our speaker count to 1 per ToR). We have nodes coming and going automatically as well, the only manual step we need is when a new rack/ToR switch is added to the cluster, then the central configmap needs to be updated (from our addon deployment pipeline).

[johananl]

On Packet, each server gets a /31 CIDR. The even address is a virtual interface on the ToR switch and the odd address is the server. The even address is also the BGP endpoint with which the server needs to peer.

~Server A cannot talk to a ToR interface which belongs to server B (most likely due to layer 2 isolation). Each server must establish a BGP session with its own individual peer address.~ I've just checked this and server A can ping the ToR interface of service B, most likely because the ToR switch routes to its local interface. ~I don't know about establishing a BGP session.~ It cannot establish a BGP session though since Packet likely filters TCP connections on port 179 and allow only the right server as a source. But anyway, as a Packet user you are not "supposed" to peer with a different host's BGP endpoint.

BTW, the above describes the "layer 3" networking mode on Packet. Packet also offers a "layer 2" mode which works differently and has some limitations which make it unsuitable for some workloads.

[johananl]

We are currently in contact with Packet regarding the possibility of having the Packet CCM enrich Node objects with annotations. Such annotations could include node-specific BGP information which would alleviate the need for nodes to configure their own labels and would allow us to use annotations rather than labels for configuring node peers.

In order to avoid having MetalLB look for Packet-specific annotations, we could let the user tell MetalLB "get your remote peer from annotation X" or something similar.

@danderson do you think this approach is preferred, or should we still stick to labels in order to allow non-Packet users set node labels via kubelet flags?

[danderson]

@johananl I think I'm okay with either option. Strictly speaking, I would prefer to use annotations and then upstream a k8s bug to support setting annotations in kubelet flags. But that's obviously a lot more work, and I'm not convinced it's worth it.

In other words: I trust you, and I'm happy with either option. Do whichever you think is better :)

[johananl]

I've looked into this a bit more and think we could actually support both options (labels and annotations) since the code for handling one or the other is practically identical. This approach would likely allow us to cover more use cases with a minimal effort.

So, in terms of UX it could look similar to the following:

apiVersion: v1
kind: ConfigMap
metadata:
  namespace: metallb-system
  name: config
data:
  config: |
    address-pools:
    - name: default
      protocol: bgp
      addresses:
      - 192.168.10.0/24
    peer-autodiscovery:
      from-annotations:
        # Tell MetalLB to look for an annotation called `example.com/hold-time`
        # on the node object and use its value to configure the hold time for the
        # node peer.
        hold-time: example.com/hold-time
        my-asn: example.com/my-asn
        peer-address: example.com/peer-address
        peer-asn: example.com/peer-asn
        peer-port: example.com/peer-port
        router-id: example.com/router-id
      from-labels:
        hold-time: example.com/hold-time
        ...
      # Tell MetalLB to configure autodiscovered peers with a BGP password
      # stored in a key called `bgpauth` at a key called `pass`.
      bgp-password:
        secret-name: bgpauth
        secret-key: pass

We could then decide, for example, that a node peer discovered via labels is preferred over annotations, and state so in the docs.

One caveat is BGP passwords: I don't think it's a good idea to write passwords to labels/annotations, so maybe we should somehow combine this feature with https://github.com/metallb/metallb/issues/408 (see sample UX above). The main problem is that using this approach users cannot specify per-peer BGP passwords, so maybe we could define a standard structure for the secret such as the following:

apiVersion: v1
kind: Secret
metadata:
  name: bgpauth
type: Opaque
data:
  node1: c3R1ZmYK
  node2: b3RoZXJzdHVmZgo=

MetalLB could then look the password up for each node using the node name as the key, but this assumes there is something (e.g. a CCM) which knows how to populate this secret, and this is possibly becoming too specialized already :-/

Of course, we could also decide not to support per-node BGP passwords for autodiscovered peers. Maybe this use case isn't very common.

I've already started implementing this. For now I'm simply ignoring BGP passwords for node peers as this is a relatively small detail in the context of this feature. I'll wait for feedback and keep thinking about this in parallel.

Thoughts are welcome.

[johananl]

A quick update on my end: the feature (called "peer autodiscovery") is largely implemented and I'm currently fixing bugs and increasing the test coverage while at it.

Following is a summary of what has been done:

• I've added a flexible discovery mechanism which allows users to configure peer autodiscovery from node labels or annotations. The discovery mechanism is platform-agnostic and allows the user to specify which labels and/or annotations to look up for figuring out the BGP configuration.
• The peer autodiscovery mechanism interacts nicely (pending some bug fixes) with the existing discovery mechanism: the user can use one or both of them, and in case of a "competition" (i.e. a static peer and a discovered peer have the same BGP config) the BGP controller prefers the statically-configured peer.
• I've added a stats HTTP endpoint which exposes the internal state of the BGP peers on the speaker. In addition to being useful for ops, this was a game changer in my efforts to fix all the edge cases when working on the feature. So, thanks a lot @danderson for suggesting we add this! :+1:

The WIP is at https://github.com/kinvolk/metallb/tree/johananl/node-peers-refactored. I'll update the docs and open a PR as soon as I fix the remaining edge cases I know about.

P.S: There is one caveat - the peer autodiscovery currently doesn't support BGP authentication. Since putting clear-text passwords in Node objects is in my opinion a no-go, I think we can only add support for this if we add support for storing BGP passwords in secrets (see https://github.com/metallb/metallb/issues/408).