daemon: Perform early (partial) local node info initialization #24866
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,139 @@ | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
| // Copyright Authors of Cilium | ||
|
|
||
| package cmd | ||
|
|
||
| import ( | ||
| "context" | ||
| "fmt" | ||
| "net" | ||
|
|
||
| k8sLabels "k8s.io/apimachinery/pkg/labels" | ||
|
|
||
| agentK8s "github.com/cilium/cilium/daemon/k8s" | ||
| "github.com/cilium/cilium/pkg/hive/cell" | ||
| "github.com/cilium/cilium/pkg/k8s" | ||
| "github.com/cilium/cilium/pkg/k8s/client" | ||
| "github.com/cilium/cilium/pkg/k8s/resource" | ||
| slim_corev1 "github.com/cilium/cilium/pkg/k8s/slim/k8s/api/core/v1" | ||
| "github.com/cilium/cilium/pkg/logging/logfields" | ||
| "github.com/cilium/cilium/pkg/node" | ||
| "github.com/cilium/cilium/pkg/node/addressing" | ||
| "github.com/cilium/cilium/pkg/option" | ||
| "github.com/cilium/cilium/pkg/source" | ||
| ) | ||
|
|
||
| type localNodeInitializerParams struct { | ||
| cell.In | ||
|
|
||
| Config *option.DaemonConfig | ||
| Clientset client.Clientset | ||
| LocalNode agentK8s.LocalNodeResource | ||
| } | ||
|
|
||
| // localNodeInitializer performs the bootstrapping of the LocalNodeStore, | ||
| // which contains information about the local Cilium node populated from | ||
| // configuration and Kubernetes. | ||
| type localNodeInitializer struct { | ||
| localNodeInitializerParams | ||
| } | ||
|
|
||
| func (ini *localNodeInitializer) InitLocalNode(ctx context.Context, n *node.LocalNode) error { | ||
| n.Source = source.Local | ||
|
|
||
| if err := ini.initFromConfig(ctx, n); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if err := ini.initFromK8s(ctx, n); err != nil { | ||
|
There was a problem hiding this comment. It seems like this aspect is pulling a k8s dependency in much earlier, or node initialization much later in the initialization process. From the commit message that seems somewhat intentional. I guess this raises the same question I had earlier, how do we reason about the initialization ordering? Related, I see that newDaemon has the steps for node discovery and `k8s.waitForNodeInformation(), how do those aspects line up with this initializer? There was a problem hiding this comment. The There was a problem hiding this comment. Is it fair to say that "By moving more of this logic behind an Observable Cell, this commit makes progress towards a code structure where the full node object may be initialized before exposing any updates for the node to the other subsystems. This provides stronger guardrails against race conditions where users of the Node objects may inadvertently access the structure before it has been initialized"? I'm think the core of my question is "why is it safe to move this initialization earlier, will it cause other logic to be reordered?". If I understand, the counter-argument is "well it's not safe today, other modules could attempt to read local node state before it's initialized". So by moving towards a better structure, maybe at some point we can move all of the users of the local node over to relying on the Then my concern about reordering other logic should be a non-issue, the new There was a problem hiding this comment. I've now made this a bit more safer by leaving the |
||
| return err | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func newLocalNodeInitializer(p localNodeInitializerParams) node.LocalNodeInitializer { | ||
| return &localNodeInitializer{p} | ||
| } | ||
|
|
||
| func (ini *localNodeInitializer) initFromConfig(ctx context.Context, n *node.LocalNode) error { | ||
| // If there is one device specified, use it to derive better default | ||
| // allocation prefixes | ||
| node.SetDefaultPrefix(ini.Config, ini.Config.DirectRoutingDevice, n) | ||
joamaki marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| // Initialize node IP addresses from configuration. | ||
| if ini.Config.IPv6NodeAddr != "auto" { | ||
| if ip := net.ParseIP(ini.Config.IPv6NodeAddr); ip == nil { | ||
| return fmt.Errorf("invalid IPv6 node address: %q", ini.Config.IPv6NodeAddr) | ||
| } else { | ||
| if !ip.IsGlobalUnicast() { | ||
| return fmt.Errorf("Invalid IPv6 node address: %q not a global unicast address", ip) | ||
| } | ||
| n.SetNodeInternalIP(ip) | ||
joestringer marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
| } | ||
| if ini.Config.IPv4NodeAddr != "auto" { | ||
| if ip := net.ParseIP(ini.Config.IPv4NodeAddr); ip == nil { | ||
| return fmt.Errorf("Invalid IPv4 node address: %q", ini.Config.IPv4NodeAddr) | ||
| } else { | ||
| n.SetNodeInternalIP(ip) | ||
| } | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func (ini *localNodeInitializer) getK8sLocalNode(ctx context.Context) (*slim_corev1.Node, error) { | ||
| ctx, cancel := context.WithCancel(ctx) | ||
| defer cancel() | ||
| for ev := range ini.LocalNode.Events(ctx) { | ||
| ev.Done(nil) | ||
| if ev.Kind == resource.Upsert { | ||
| return ev.Object, nil | ||
| } | ||
| } | ||
| return nil, ctx.Err() | ||
| } | ||
|
|
||
| func (ini *localNodeInitializer) initFromK8s(ctx context.Context, node *node.LocalNode) error { | ||
| if ini.LocalNode == nil { | ||
| return nil | ||
| } | ||
|
|
||
| k8sNode, err := ini.getK8sLocalNode(ctx) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| parsedNode := k8s.ParseNode(k8sNode, source.Kubernetes) | ||
|
|
||
| // Initialize the fields in local node where the source of truth is in Kubernetes. | ||
| // Later stages will deal with updating rest of the fields depending on configuration. | ||
| // | ||
| // The fields left uninitialized/unrestored here: | ||
| // - Cilium internal IPs (restored from cilium_host or allocated by IPAM) | ||
| // - Health IPs (allocated by IPAM) | ||
| // - Ingress IPs (restored from ipcachemap or allocated) | ||
| // - Wireguard key (set by wireguard agent) | ||
| // - IPsec key (set by IPsec) | ||
| // - alloc CIDRs (depends on IPAM mode; restored from Node or CiliumNode) | ||
| // - ClusterID (set by NodeDiscovery) | ||
| // - NodeIdentity (always unset) | ||
| node.Name = parsedNode.Name | ||
| node.Labels = parsedNode.Labels | ||
| node.Annotations = parsedNode.Annotations | ||
| node.Cluster = parsedNode.Cluster | ||
| for _, addr := range parsedNode.IPAddresses { | ||
| if addr.Type == addressing.NodeInternalIP { | ||
| node.SetNodeInternalIP(addr.IP) | ||
| } else if addr.Type == addressing.NodeExternalIP { | ||
| node.SetNodeExternalIP(addr.IP) | ||
| } | ||
| } | ||
|
|
||
| if ini.Config.NodeEncryptionOptOutLabels.Matches(k8sLabels.Set(node.Labels)) { | ||
| log.WithField(logfields.Selector, ini.Config.NodeEncryptionOptOutLabels). | ||
| Infof("Opting out from node-to-node encryption on this node as per '%s' label selector", | ||
| option.NodeEncryptionOptOutLabels) | ||
| node.OptOutNodeEncryption = true | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why doesn't the Local Node initialize itself?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want that logic to be separate of the LocalNodeStore itself to keep things non-cyclic. Having the initializer in
daemon/cmdmakes most sense as that's where we're in the best position to depend on different sub-systems to initialize this.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgive my ignorance here, is that a general long term principle or is this more to do with the need for intermediate solutions given the current structure with most logic being started in
newDaemon()? Is this for testing purposes or there's some other principle here?Do you think it makes sense to add wording something like the following to the hive guidelines?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there's any general principles in play here. Sorry if I gave that impression. The initialization of this node local information is complicated as the different sources overlap and I don't think it makes sense to go overly modular here (though I did first think about making initializers group values). It's much easier to follow if we keep the initialization linear and located to one file and resolve "merge conflicts" there. It does mean we have this central initializer, but I don't really see other good options.
To make this more concrete: we're essentially merging
Node,CiliumNode,cilium_host IP,IPsec key,WireGuard key,IngressIP(and maybe few others) into a singletypes.Nodestruct. Many of these overlap and so we can't allow these "initializations" to proceed in a random or even dependency-driven order.