Rework ipcache to handle metadata from multiple sources via a dedicated worker goroutine #21142

joestringer · 2022-08-30T23:47:37Z

This meta issue tracks a variety of enhancements being made to pkg/ipcache in order to make it more resilient and able to consistently merge information gathered about IPs from various sources (kvstore, k8s resources, local node, etc.).

Broadly speaking, these changes define a new "desired state" structure plus a resolution loop in order to realize that desired state. The desired state is synchronously updated by callers to define information such as a list of labels that one resource wants to associate with an IP, or an encryption key to associate with it. Then the upsert/remove call (typically from a k8s watcher) would trigger a separate goroutine to then realize the state internally. The realization of the state will combine all of the information from different sources together, potentially allocate identities to associate with the IP, trigger policy calculation for any new identities, and ensure that the datapath is consistently updated with any new policy state as well as any new bpf ipcache map state. For more details on the high level API, see #19765.

Tasks:

Remove static package-level variables from pkg/ipcache
- Refactor IPCache to remove static package-level globals #19073
Create new asynchronous API for handling insertion of labels / metadata /auxiliary data into the IPCache
- Restructure IPCache to handle metadata merging #19765
Convert the existing callers of IPCache to the new API
- CIDR policies
  - Move policy package over to asynchronous IPCache API #20116
  - ~~(optional) Optimization: Use sentinel type to represent CIDR labels to be expanded~~
    - Don't expand CIDR labels, match smartly in Labels instead #30897
- NodeManager
  - Inject encryption state into the new API
  - ipcache: Add ability to override identity via UpsertMetadata #21667
  - ~~Move NodeManager over to asynchronous IPCache API #20117~~
  - node: Use new asynchronous IPCache API for Manager (v2) #23208
    - Ensure ingress IPs are excluded from CIDR identity resolution similar to health IPs
    - Ensure the node manager uses a unique resource ID depending on the source of information (k8s or kvstore; k8s node or CiliumNode)
- KVStore (pkg/ipcache/kvstore.go)
  Note: KVStore path likely does not need to allocate identities.
  - pkg/ipcache/kvstore.go
    - Convert Pods, CEP, and KVStore IPcache handling over to async API #31409
  - pkg/node/store/store.go
  - Define resource IDs for kvstore paths
- Local NodeDiscovery
  - Convert Daemon insert host IPs -> UpsertMetadata
    - Convert daemon ipcache usages to new ipcache async API #25749
  - Auxiliary data, other label based information
  - Pod NamedPorts
    - Convert Pods, CEP, and KVStore IPcache handling over to async API #31409
  - Introduce HasChanged in InjectLabels()
- CIDR Identity restore on startup
  - daemon/cmd/daemon.go
    - Convert daemon identity restore logic over to newer IPCache APIs #27255
    - ipcache: move CIDR restoration to asynchronous APIs #28673
- FQDN Policies (FQDN: transition to asynchronous ipcache APIs #28930)
  - Introduce a synchronization mechanism to ensure ipcache is plumbed (even if async) before releasing the corresponding DNS response
- Overlapping PodCIDRs
  - Add support for encoding cluster-specific metadata (cf. clustermesh: Make IPCache CPlane aware of the ClusterID #22935)
- ToServices policy translation
  - k8sServiceHandler() policy/k8s: Refactor and move ToServices translation to policy package #31062
- Migrate CiliumCIDRGroup watcher to ipcache async metadata APIs #30992
- Any others?
  - Evaluate remaining callsites
Remove old synchronous ipcache.Upsert() APIs
- Support natively injecting security identities into the ipcache API (possibly related to NodeManager changes) ipcache: Add ability to override identity via UpsertMetadata #21667
- Refactor the AllocateCIDRsForIPs to use the new ipcache API
  - Mitigate issues where a CIDRSet labels refer to the same IP prefix as the IP/Identity backing an FQDN
- Re-evaluate the use of pkg.Source for resolving ipcache upsert conflicts
  - Re-evaluate KubeAPIServer source at the least
- Merge the main IPCache and IPCache.metadata structures together to reduce duplication? (TBD)
Expand testing of these features
- Add e2e coverage of apiserver policy functionality #17829

Related: #17962
Related: #18301

The text was updated successfully, but these errors were encountered:

This new function is a convenience and safe wrapper for creating a copy of a Labels object from another. This is especially useful going forward with the IPcache rework (cilium#21142), where many call sites are being converted from using raw identities over to labels. Being able to create copies of labels without accidentally copying the reference to the backing slice is crucial to avoid very subtle and hard to find bugs. For example: ``` // Node 1 comes online. node1Labels := labels.LabelRemoteNote // Detect that node1 also has the kube-apiserver deployed on it, so add // its label. node1Labels.MergeLabels(labels.LabelKubeAPIServer) ... // Node 2 comes onlne. node2Labels := labels.LabelRemoteNote // Now node2Labels contains the kube-apiserver label as well because // node1Labels was set to the reference of labels.LabelRemoteNote. ``` Signed-off-by: Chris Tarazi <chris@isovalent.com>

This new function is a convenience and safe wrapper for creating a copy of a Labels object from another. This is especially useful going forward with the IPcache rework (#21142), where many call sites are being converted from using raw identities over to labels. Being able to create copies of labels without accidentally copying the reference to the backing slice is crucial to avoid very subtle and hard to find bugs. For example: ``` // Node 1 comes online. node1Labels := labels.LabelRemoteNote // Detect that node1 also has the kube-apiserver deployed on it, so add // its label. node1Labels.MergeLabels(labels.LabelKubeAPIServer) ... // Node 2 comes onlne. node2Labels := labels.LabelRemoteNote // Now node2Labels contains the kube-apiserver label as well because // node1Labels was set to the reference of labels.LabelRemoteNote. ``` Signed-off-by: Chris Tarazi <chris@isovalent.com>

Similar to the health reserved identity, the ingress identity shouldn't have CIDR labels associated with it, so exclude it from the identity resolution logic. The ingress IPs comes from the CiliumNode object. Without this commit, ingress IPs will have a CIDR labels and therefore a CIDR identity, instead of having the reserved ingress identity. Related: #21142 Signed-off-by: Chris Tarazi <chris@isovalent.com>

Switch over from using direct ipcache insertion with raw identities to a label-based model, see GH-21142. Signed-off-by: Chris Tarazi <chris@isovalent.com>

Move to the new async ipcache API, see GH-21142. Signed-off-by: Chris Tarazi <chris@isovalent.com>

Switch over from using direct ipcache insertion with raw identities to a label-based model, see GH-21142. Signed-off-by: Chris Tarazi <chris@isovalent.com>

Move to the new async ipcache API, see GH-21142. Signed-off-by: Chris Tarazi <chris@isovalent.com>

Switch over from using direct ipcache insertion with raw identities to a label-based model, see GH-21142. Signed-off-by: Chris Tarazi <chris@isovalent.com>

Move to the new async ipcache API, see GH-21142. Signed-off-by: Chris Tarazi <chris@isovalent.com>

Switch over from using direct ipcache insertion with raw identities to a label-based model, see ciliumGH-21142. Signed-off-by: Chris Tarazi <chris@isovalent.com>

Move to the new async ipcache API, see ciliumGH-21142. Signed-off-by: Chris Tarazi <chris@isovalent.com>

The following commits have introduced a new API for interacting with the ipcache, which is now preferred for all future interactions: - Commit e637814 ("ipcache: Introduce new asynchronous API") - Commit b6cef92 ("ipcache: Add new {Upsert,Remove}Prefixes() APIs") - Commit f9173b7 ("ipcache: Add ability to override identity via UpsertIdentity") Mark the older APIs as deprecated in order to help to slowly phase out future usage of those APIs. For more details, see GitHub issue #21142 ("Rework ipcache to handle metadata from multiple sources via a dedicated worker goroutine"). Note that there are still various paths using the older APIs to insert ipcache entries. If an entry is inserted using the older API, then the corresponding delete event should likewise use the same older API to delete the entry. Future users should both insert and delete using the newer API. Do not mix and match the upserts and deletes from deprecated APIs and non-deprecated APIs for the same event type / resource update into ipcache. Suggested-by: Aditi Ghag <aditi@cilium.io> Signed-off-by: Joe Stringer <joe@cilium.io>

joestringer assigned gandro, joestringer and christarazi Aug 30, 2022

joestringer changed the title ~~Rework ipcache to handle metadata from multiple sources via a dedicated worker thread~~ Rework ipcache to handle metadata from multiple sources via a dedicated worker goroutine Aug 31, 2022

joestringer added this to Needs triage in Agent Project Tracking via automation Aug 31, 2022

joestringer mentioned this issue Oct 31, 2022

ipcache: Fix IPcache leak of remote-node IP addresses #21932

Merged

christarazi mentioned this issue Jan 20, 2023

labels, ipcache: Introduce convenience NewFrom() #23218

Merged

christarazi moved this from Needs triage to High priority features in Agent Project Tracking Jan 27, 2023

christarazi mentioned this issue Jan 31, 2023

node: Use new asynchronous IPCache API for Manager (v2) #23208

Merged

joestringer added the kind/meta Meta-task for co-ordination. label Mar 16, 2023

christarazi mentioned this issue Apr 3, 2023

Three fixes for deadlocks #24672

Merged

christarazi mentioned this issue May 23, 2023

[WIP] policy: allocate identity for CIDR selectors on first use #25581

Closed

3 tasks

christarazi mentioned this issue May 29, 2023

Convert daemon ipcache usages to new ipcache async API #25749

Merged

christarazi added a commit that referenced this issue May 30, 2023

daemon: Convert host IP sync to async ipcache API

655e2f3

Switch over from using direct ipcache insertion with raw identities to a label-based model, see GH-21142. Signed-off-by: Chris Tarazi <chris@isovalent.com>

christarazi added a commit that referenced this issue May 30, 2023

dameon: Convert ingress restoration code to async ipcache API

efddbe6

Move to the new async ipcache API, see GH-21142. Signed-off-by: Chris Tarazi <chris@isovalent.com>

christarazi added a commit that referenced this issue Jun 7, 2023

daemon: Convert host IP sync to async ipcache API

ec2c1f9

Switch over from using direct ipcache insertion with raw identities to a label-based model, see GH-21142. Signed-off-by: Chris Tarazi <chris@isovalent.com>

christarazi added a commit that referenced this issue Jun 7, 2023

dameon: Convert ingress restoration code to async ipcache API

f868917

Move to the new async ipcache API, see GH-21142. Signed-off-by: Chris Tarazi <chris@isovalent.com>

christarazi mentioned this issue Jun 7, 2023

policy: allocate identity for CIDR selectors on first use (v2) #25979

Closed

3 tasks

dylandreimerink pushed a commit that referenced this issue Jun 8, 2023

daemon: Convert host IP sync to async ipcache API

9ad27a8

Switch over from using direct ipcache insertion with raw identities to a label-based model, see GH-21142. Signed-off-by: Chris Tarazi <chris@isovalent.com>

dylandreimerink pushed a commit that referenced this issue Jun 8, 2023

dameon: Convert ingress restoration code to async ipcache API

745cd32

Move to the new async ipcache API, see GH-21142. Signed-off-by: Chris Tarazi <chris@isovalent.com>

romanspb80 pushed a commit to romanspb80/cilium that referenced this issue Jun 22, 2023

dameon: Convert ingress restoration code to async ipcache API

cbcb2c9

Move to the new async ipcache API, see ciliumGH-21142. Signed-off-by: Chris Tarazi <chris@isovalent.com>

joestringer mentioned this issue Aug 8, 2023

ipcache: Fix refcounting with mix of APIs #27327

Merged

joestringer mentioned this issue Aug 17, 2023

ipcache: Deprecate old API #27576

Merged

christarazi mentioned this issue Oct 5, 2023

ipcache: fix flapping labels in SelectorCache when reserved:host identity has multiple IPs #28332

Merged

squeed self-assigned this Nov 14, 2023

gandro mentioned this issue Mar 11, 2024

ipcache: Remove synchronous CIDR identity allocation #31311

Merged

christarazi mentioned this issue Mar 15, 2024

Convert Pods, CEP, and KVStore IPcache handling over to async API #31409

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework ipcache to handle metadata from multiple sources via a dedicated worker goroutine #21142

Rework ipcache to handle metadata from multiple sources via a dedicated worker goroutine #21142

joestringer commented Aug 30, 2022 •

edited by christarazi

Rework ipcache to handle metadata from multiple sources via a dedicated worker goroutine #21142

Rework ipcache to handle metadata from multiple sources via a dedicated worker goroutine #21142

Comments

joestringer commented Aug 30, 2022 • edited by christarazi

joestringer commented Aug 30, 2022 •

edited by christarazi