ipcache: Add ability to override identity via UpsertMetadata #21667

This fixes a nil pointer access in AllocateIdentity, where a new identity with the identical label set of a previously deleted identity which would try to bump the reference count of the deleted identity. Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>

Previously, the mock identity allocator would allocate a new identity for each global labelset. This is not how the real allocator behaves. This commit therefore changes the behavior of the mock allocator to return the already existing identity and bumping its reference count. Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>

As described in the comment added by this commit message, this logic is making assumptions about the way that identities for remote nodes are allocated, which hold true today but may not hold true in the future. If we do end up providing a way to associate additional remote-node labels with IPs corresponding to remote nodes, hopefully the future contributor will be able to reason about the impact and prevent weird bugs / interactions using this new comment. Signed-off-by: Joe Stringer <joe@cilium.io>

An upcoming commit will introduce a way to resolve an identity in the prefixInfo to a previously-allocated identity, rather than the identity that this logic may allocate. In order to simplify upcoming work to resolve the identity from a prefixInfo, this commit refactors the callers into a single function with all of the corresponding logic, resolveIdentity(). The prior comments about conflicts between ToCIDR policy and the identity resolution logic here for the metadata is still valid for now. This is because ToCIDR policy does not yet use the new IPCache metadata APIs that call into these functions, so there is no way that this logic could resolve that conflict. That said, the new core structure of the IPCache.metadata is designed to handle that conflict, and that will not require any subsequent changes to this function. So I just dropped the comments / references to ciliumGH-17962. This commit now also documents the datapath assumption being tied to the remote-node logic here, so that if we do end up providing a way to associate additional remote-node labels with IPs corresponding to remote nodes, hopefully the future contributor will be able to reason about the impact and prevent weird bugs / interactions with that new logic. This assumption already existed, it is not introduced by this commit. There may be a minor change in the behaviour now, in that if a caller somehow hands the IP of the host or a remote node into AllocateCIDRs() then previously the function would erroneously allocate a new CIDR identity for those IPs. The function will now consistently associate the reserved identity for those IPs. Other than that, this commit is not expected to have any side effects. Signed-off-by: Joe Stringer <joe@cilium.io> Signed-off-by: Chris Tarazi <chris@isovalent.com>

This commit adds a new flag to each IPCache entry, tracking if it was upserted using the new asynchronous metadata infrastructure or the legacy Upsert/Delete API. This is needed to properly track if an entry can safely be deleted once no metadata is associated with it anymore. Previously, the code relied on the identity reference count on this, but that does not work for reserved identities, or non-CIDR identities which may be shared among multiple prefixes. The latter was not a concern so far, but will be added in a future commit. For the former (reserved identities), the existing code relied on mixed API use for each prefix, requiring that other code eventually invoked a manual Delete call on each entry. Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>

This renames the `id` variable to `oldID`, to better distinguish it from the `newID` variable and indicate that `oldID` is the one about to be replaced or deleted. This commit contains no functional changes. Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>

See cilium#21667 (comment) for an in-depth discussion about this change. For local identities, this condition never evaluated to false, since identities are different for each prefix and thus no other prefix could have added a map entry for the previously allocated identity. A subsequent commit will cause InjectLabels to also allocate global identities. In that case, the condition is actively harmful, since it could cause an identity leak. Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>

The new asynchronous IPCache API tries to determine the identity of a prefix based on the provided labels. However, in some cases, the sources providing the IPCache metadata (e.g. CiliumNode, CiliumEndpoint) already know the numeric identity of the prefix. In that case, we want to give that predefined global identity preference over any local identity we might derive from the labels. All other metadata is still tracked. If the override is ever removed from the prefix, the metadata will be used to determine the new prefix identity. Prefixes added via UpsertIdentity otherwise are treated similarly to ones upserted via UpsertMetadata: The policy maps are still updated for any newly added identity, and the identity is removed from policy maps if RemoveIdentity caused the identity to be deallocated. Co-authored-by: Joe Stringer <joe@cilium.io> Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ipcache: Add ability to override identity via UpsertMetadata #21667

ipcache: Add ability to override identity via UpsertMetadata #21667

Commits on Jan 24, 2023