Skip to content

Commit

Permalink
ipam: Fix invalid PodCIDR in CiliumNode in ENI/Azure/MultiPool mode
Browse files Browse the repository at this point in the history
[ upstream commit 1568558 ]

This commit fixes an issue where Cilium-Agent would announce its
internal auto-generated IPv4/IPv6AllocRange by writing it into the local
CiliumNode resource.

Cilium-Agents IPv4/IPv6AllocRange is Cilium's internal representation of
the local node's PodCIDR. The concept of a single allocation CIDR
however is outdated, since in many IPAM modes (ENI, Azure, AlibabaCloud,
MultiPool) there either is no PodCIDR to begin with (and pod IPs are
allocated using a different scheme), or there are multiple PodCIDRs (in
the case of MultiPool).

Before this commit, cilium-agent used to copy its internal PodCIDR into
the CiliumNode resource, but this behavior is only correct in Kubernetes
IPAM mode. In Kubernetes IPAM mode, the IPv4/IPv6AllocRange is taken
from the Kubernetes Node resource, and thus it makes sense to announce
it to other cilium-agent instances via the CiliumNode resource. This
field then is for example used by `auto-direct-node-routes` to install
routes for direct routing mode.

However, in all other IPAM modes that we have, copying the internal
PodCIDR into the CiliumNode resource does not make sense:

 - In ClusterPool mode, the CiliumNode PodCIDR field is populated by
   cilium-operator. Therefore, we should not try to overwrite it
   ourselves.  This behavior is unchanged by this commit.
 - In all other current IPAM modes, the CiliumNode PodCIDR field is not
   used. Instead we use fields specific to those IPAM modes (c.f.
   CiliumNode.Spec.IPAM). In those modes, the previous code therefore wrote
   a auto-generatead, but otherwise valid IPv4/IPv6AllocRange into the
   field. That auto-generated PodCIDR is not used for pod IP allocation in
   those modes, so it does not make sense to announce it either.

The announced auto-generated PodCIDR is causing issues in IPAM MultiPool
mode. With this commit, we will no longer install invalid routes for the
auto-generated PodCIDR. This was mostly harmless (since the
auto-generated PodCIDR rarely overlapped with the real ones), but still
a potential cause for confusion and bugs.

In addition, ENI-mode users have reported that they are confused by the
this "fake" PodCIDR, which is understandable, since that PodCIDR is
meaningless in ENI mode (#9409).

Long term, we want to remove the concept of the IPv4/IPv6AllocCIDR
completely (and remove the auto-generation code), but this is a first
incremental step towards that goal.

Fixes: #9409

Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
Signed-off-by: Paul Chaignon <paul.chaignon@gmail.com>
  • Loading branch information
gandro authored and dylandreimerink committed Jan 10, 2024
1 parent b975300 commit be672e7
Showing 1 changed file with 10 additions and 11 deletions.
21 changes: 10 additions & 11 deletions pkg/nodediscovery/nodediscovery.go
Original file line number Diff line number Diff line change
Expand Up @@ -512,17 +512,16 @@ func (n *NodeDiscovery) mutateNodeResource(nodeResource *ciliumv2.CiliumNode) er
}
}

switch option.Config.IPAM {
case ipamOption.IPAMClusterPool, ipamOption.IPAMClusterPoolV2:
// We want to keep the podCIDRs untouched in these IPAM modes because
// the operator will verify if it can assign such podCIDRs.
// If the user was running in non-IPAM Operator mode and then switched
// to IPAM Operator, then it is possible that the previous cluster CIDR
// from the old IPAM mode differs from the current cluster CIDR set in
// the operator.
// There is a chance that the operator won't be able to allocate these
// podCIDRs, resulting in an error in the CiliumNode status.
default:
if option.Config.IPAM == ipamOption.IPAMKubernetes {
// We only want to copy the PodCIDR from the Kubernetes Node resource to
// the CiliumNode resource in IPAM Kubernetes mode. In other PodCIDR
// based IPAM modes (such as ClusterPool or MultiPool), the operator
// will set the PodCIDRs of the CiliumNode and those might be different
// from the ones assigned by Kubernetes.
// For non-podCIDR based IPAM modes (e.g. ENI, Azure, AlibabaCloud), there
// is no such thing as a podCIDR to begin with. In those cases, the
// IPv4/IPv6AllocRange is auto-generated and otherwise unused, so it does not
// make sense to copy it into the CiliumNode it either.
nodeResource.Spec.IPAM.PodCIDRs = []string{}
if cidr := node.GetIPv4AllocRange(); cidr != nil {
nodeResource.Spec.IPAM.PodCIDRs = append(nodeResource.Spec.IPAM.PodCIDRs, cidr.String())
Expand Down

0 comments on commit be672e7

Please sign in to comment.