[DRAFT]: NodeAddress table and refactored NodeAddressing #28712

joamaki · 2023-10-20T14:05:39Z

Add Table[NodeAddress] which derives from the low-level Table[*Device] to pick which IP addresses are considered host addresses and used for NodePort and BPF masquerading.

Refactor NodeAddressing to look up IP addresses from the Table[NodeAddress] and K8s related information from LocalNodeStore.

To add support for multiple NodePort IP addresses per network device, this PR adds the flag --nodeport-addresses that
allows user to specify which CIDRs the NodePort frontends can be picked from. This allows us to solve e.g. #24481.

joamaki · 2023-10-20T14:21:06Z

/test

joamaki · 2023-10-24T13:49:51Z

The last commit that I just pushed now implements --nodeport-addresses=10.0.0.0/8,2001::/8 option for whitelisting which addresses are consider node's IP addresses. If this option is not set, the default behavior of only using the primary IP address (one with lowest scope) per device is kept.

While the impact of this is pretty clear for LoadBalancerNodeAddresses(), I'm not entirely sure yet what it means for LocalAddresses() which are used for the host identity. Prior to this, LocalAddresses() was the same set as LoadBalancerNodeAddresses(), with the exception of not having a "zero" address and including the "cilium_host" IP address. So in principle this could make sense.

Second thing to think about is whether this filtering should happen in NodeAddressing or whether the addresses should already be filtered out in Table[*Device]... Or whether we should have a separate table for the "node IP addresses" to separate this notion from Table[*Device] which is really meant to just mirror the kernel's link&addr state. The latter might be a good design since the idea anyway is to remove NodeAddressing to instead remove an unnecessary abstraction and encourage direct queries into the state and to be able to wait for the state to change. E.g.

type NodeAddr struct {
  Addr netip.Addr
  Device *Device // The device from which the address is inherited
}
var NodeAddrTable Table[*NodeAddr] = NewTable("node-addresses",  AddrIndex) 

func (f *featureFoo) reconcileNodeAddresses(db *DB, nodeAddrTable Table[*NodeAddr]) {
  for {
    addrs, watch := nodeAddrTable.All(db.ReadTxn())
    // reconcile the new node addresses
    for addr, _, ok := addrs.Next(); ok; addr, _, ok = addrs.Next() {
      // ...
    }
    // wait for node addresses to change again    
    <-watch 
  }
}

joamaki · 2023-10-25T15:55:02Z

Ended up implementing the Table[NodeAddress]. This is essentially the same information as was stored in ipv*NodePortAddrs.

Redid LocalAddresses by extracting global addresses from all devices as the original code did with netlink.AddrList. It'd probably be a good idea to think whether it'd be possible to merge NodePortAddresses and LocalAddresses to simplify things overall, but hard to judge at this point what the impact would be, so this PR keeps things semantically the same.

I'll squash the changes down to couple commits once it's in a matured state. Right now there's lots of rewriting going on.

maintainer-s-little-helper · 2023-10-27T09:04:21Z

Commit 2320fdd does not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

maintainer-s-little-helper · 2023-10-30T07:27:59Z

Commit 2320fdd does not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

maintainer-s-little-helper · 2023-10-30T13:32:12Z

Commit 2320fdd does not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

maintainer-s-little-helper · 2023-10-30T15:43:15Z

Commit dcf18a8 does not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

maintainer-s-little-helper · 2023-10-30T16:04:14Z

Commit dcf18a8 does not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

Add a helper to construct only RWTable[T] and TableMeta to allow creating a table in a way that enforces that the table is populated by the producer before it can be read: var Cell = cell.Module("example", "Example module", // Privately provides RWTable[*Foo]: statedb.NewPrivateRWTableCell[*Foo]("foos", FooIDIndex), cell.Provide(New), ) func New(lc hive.Lifecycle, t statedb.RWTable[*Foo]) (FooController, Table[*Foo]) { fooCtrl := ... lc.Append(fooCtrl) // fooCtrl now starts before anything that reads Table[*Foo] return fooCtl, t } Signed-off-by: Jussi Maki <jussi@isovalent.com>

To enforce that the device and route tables are populated before being read from, provide the Table[*Device] and Table[*Route[ from the DevicesController constructor. This makes sure that anything that depends on e.g. 'Table[*Device]' will be constructed after DevicesController and thus the DevicesController start hook will execute before it. Signed-off-by: Jussi Maki <jussi@isovalent.com>

As Linux does not send comprehensive set of route delete messages on device deletion, flush the routes on the link delete message. Since the link and route messages are coming over separate sockets they may come out of order, so keep track of what link indexes have been deleted and ignore route updates for deleted links. Linux does reuse the index after the 32 bits roll around, so remove the "dead link index" on link creation. Signed-off-by: Jussi Maki <jussi@isovalent.com>

Since subscribing to links/addresses/routes and then listing them is racy we might end up seeing the same address in the initial listing and as an update. Avoid adding the address twice by checking its existence first. Side note: we need the initial listing to be able to populate the tables before readers access them in order to keep the existing non-reconciling semantics. The netlink library we're using does not have a mechanism to inform that the initial listing is done in the subscriptions (even though netlink DONE message is sent for this), so instead DevicesController does subscribe + list which necessitates dealing with issues like this. Signed-off-by: Jussi Maki <jussi@isovalent.com>

In order to write the NodeAddressing's LocalAddresses() in terms of Table[*Device], support the --local-max-addr-scope option to filter out addresses by scope. By default only addresses with scope lower than LINK are used. Also filter out loopback addresses as was done by the earlier code. Signed-off-by: Jussi Maki <jussi@isovalent.com>

As a step towards removing the global variables in pkg/node that back GetNodePortIPv4Addrs and friends, implement NodeAddressing as queries against the devices table. The big functional change this brings is that now all IP addresses of selected external network devices are used for NodePort frontends. Signed-off-by: Jussi Maki <jussi@isovalent.com>

The code is no longer specific to Linux as it is implemented in terms of queries against the LocalNodeStore and the devices table. Signed-off-by: Jussi Maki <jussi@isovalent.com>

Now that NodeAddressing is implemented on top of Table[*Device], the GetNodePort*Addrs*() functions are no longer needed. Adapt HeaderfileWriter to use NodeAddressing and add in DirectRouting() accessor to it to look up the direct routing device ifindex and IP address. Signed-off-by: Jussi Maki <jussi@isovalent.com>

Support "*cidr.CIDR" and "[]*cidr.CIDR" as a config flag by parsing it from a string slice. For example: type MyConfig struct { MyCidrs []*cidr.CIDR } func (MyConfig) Flags(flags *pflag.FlagSet) { flags.StringSlice("my-cidrs", nil, "My CIDRs") } Signed-off-by: Jussi Maki <jussi@isovalent.com>

If the user specifies e.g. "--nodeport-addresses=10.0.0.0/8" then only addresses within that CIDR will be considered node's IP addresses. When set Cilium will serve NodePort on more than one IP address per network device for external traffic. Signed-off-by: Jussi Maki <jussi@isovalent.com>

Signed-off-by: Jussi Maki <jussi@isovalent.com> Refresh of datapath-add-table-nodeaddress