identity: Fix user-space security identity lookup in cluster mesh #13205

gandro · 2020-09-17T14:33:30Z

When looking up a security identity (either by its numeric id or by
labels) in user-space (e.g. in Hubble or the API), we want to ensure to
also include identities owned by remote clusters in cluster mesh too.

Before this commit, GetIdentities function of the identity allocator
(e.g. used for cilium identity list) would return all global
identities (i.e. including the ones from remote clusters as well), while
LookupIdentity{,ByID} would only return identitiies found the main
kvstore, ignoring any attached remote kvstores. This commit

This fixes multiple missed annotations which can occur in cluster-mesh
setups:

Hubble failed to annotate identities from remote clusters (Hubble fails to resolve identities across a cluster mesh. #13076).
While the API would list remote identities in /v1/identities,
performing a lookup on identities from remote clusters via API would
fail with a "not found error". This 404 could be observed in
cilium identity get <remote-id> or in cilium bpf policy get.
DNS proxy logrecords would not have the destination endpoint labels
populated.
The CiliumEndpoint.Status.Policy CRD field would not contain
labels for identities for remote clusters (if CEP status updates
were enabled).

Fixes: #13076

Fix bug where Hubble and the Cilium CLI would fail to resolve security identities across a cluster mesh.

gandro · 2020-09-17T14:43:53Z

pkg/identity/cache/cache.go

@@ -201,7 +201,8 @@ func (w *identityWatcher) stop() {

 // LookupIdentity looks up the identity by its labels but does not create it.
 // This function will first search through the local cache and fall back to
-// querying the kvstore.
+// querying all connected kvstores (first the main kvstore, followed by any
+// watched remote kvstores in clustermesh)
 func (m *CachingIdentityAllocator) LookupIdentity(ctx context.Context, lbls labels.Labels) *identity.Identity {


Note to reviewers: While we surely want to fix the lookup in this function for the API to work correctly (any identity returned by /v1/identities should also be fetchable via labels), the only non-API call site (outside of unit tests) of this function is in the IPCache:

cilium/pkg/ipcache/cidr.go

Line 118 in 3a49c6c

if id := IdentityAllocator.LookupIdentity(context.TODO(), cidr.GetCIDRLabels(prefix)); id != nil {

IPCache is performing a lookup on CIDR labels. I don't think we want to send that lookup to remote kvstores (since CIDR identies are always local), so maybe we have to introduce an additional lookup function for the IPCache usecase? Unfortunatley I'm not too familiar with local identity management, so feedback welcome.

Yeah, CIDR identities are purely local to the agent (not even shared across the cluster), so we should follow up to change the lookup function to reflect this usage.

gandro · 2020-09-17T16:04:26Z

test-me-please

tgraf

LGTM except for what we discussed on Zoom

When looking up a security identity (either by its numeric id or by labels) in user-space (e.g. in Hubble or the API), we want to ensure to also include identities owned by remote clusters in cluster mesh too. Before this commit, `GetIdentities` function of the identity allocator (e.g. used for `cilium identity list`) would return all global identities (i.e. including the ones from remote clusters as well), while `LookupIdentity{,ByID}` would only return identities found the main kvstore, ignoring any indentities cached from remote kvstores. This fixes multiple missed annotations which can occur in cluster-mesh setups: - Hubble failed to annotate identities from remote clusters (#13076). - While the API would list remote identities in `/v1/identities`, performing a lookup on identities from remote clusters via API would fail with a "not found error". This 404 could be observed in `cilium identity get <remote-id>` or in `cilium bpf policy get`. - DNS proxy logrecords would not have the destination endpoint labels populated. - The `CiliumEndpoint.Status.Policy` CRD field would not contain labels for identities for remote clusters (if CEP status updates were enabled). Fixes: #13076 Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>

gandro · 2020-09-17T17:16:06Z

Pushed the discussed change to not query remote backends in order avoid accidental DDoS. We now only check the remote caches. Diff of change since approval

gandro · 2020-09-17T17:16:17Z

test-me-please

joestringer · 2020-09-17T17:34:40Z

pkg/identity/cache/cache.go

@@ -201,7 +201,8 @@ func (w *identityWatcher) stop() {

 // LookupIdentity looks up the identity by its labels but does not create it.
 // This function will first search through the local cache and fall back to
-// querying the kvstore.
+// querying all connected kvstores (first the main kvstore, followed by any
+// watched remote kvstores in clustermesh)
 func (m *CachingIdentityAllocator) LookupIdentity(ctx context.Context, lbls labels.Labels) *identity.Identity {


Yeah, CIDR identities are purely local to the agent (not even shared across the cluster), so we should follow up to change the lookup function to reflect this usage.

Reserved and CIDR identities are local to the agent and not stored in the kvstore. This commit changes the identity cache to avoid performing a kvstore lookup for CIDR entries (which is currently done when a CIDR identity is released). This is a follow-up to #13205 (comment) Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>

[ upstream commit f3424a3 ] Reserved and CIDR identities are local to the agent and not stored in the kvstore. This commit changes the identity cache to avoid performing a kvstore lookup for CIDR entries (which is currently done when a CIDR identity is released). This is a follow-up to #13205 (comment) Signed-off-by: Sebastian Wicki <sebastian@isovalent.com> Signed-off-by: Michal Rostecki <mrostecki@opensuse.org>

[ upstream commit f3424a3 ] Reserved and CIDR identities are local to the agent and not stored in the kvstore. This commit changes the identity cache to avoid performing a kvstore lookup for CIDR entries (which is currently done when a CIDR identity is released). This is a follow-up to #13205 (comment) Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>

gandro added release-note/bug This PR fixes an issue in a previous release of Cilium. area/clustermesh Relates to multi-cluster routing functionality in Cilium. labels Sep 17, 2020

gandro requested a review from a team as a code owner September 17, 2020 14:33

gandro requested a review from a team September 17, 2020 14:33

gandro added needs-backport/1.7 labels Sep 17, 2020

maintainer-s-little-helper bot added this to Needs backport from master in 1.8.4 Sep 17, 2020

maintainer-s-little-helper bot added this to Needs backport from master in 1.7.10 Sep 17, 2020

gandro commented Sep 17, 2020

View reviewed changes

tgraf self-requested a review September 17, 2020 14:53

tgraf approved these changes Sep 17, 2020

View reviewed changes

gandro force-pushed the pr/gandro/hubble-fix-identity-lookup-in-cluster-mesh branch from 4d2ed3b to b2ea910 Compare September 17, 2020 17:14

joestringer approved these changes Sep 17, 2020

View reviewed changes

maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Sep 17, 2020

joestringer merged commit 72f6685 into master Sep 17, 2020

joestringer deleted the pr/gandro/hubble-fix-identity-lookup-in-cluster-mesh branch September 17, 2020 19:29

joestringer mentioned this pull request Sep 17, 2020

v1.7 backports 2020-09-17 #13209

Merged

joestringer added backport-pending/1.7 and removed needs-backport/1.7 labels Sep 17, 2020

maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.7 in 1.7.10 Sep 17, 2020

kkourt mentioned this pull request Sep 18, 2020

v1.8 backport 2020-09-18 #13212

Merged

kkourt added backport-pending/1.8 and removed needs-backport/1.8 labels Sep 18, 2020

maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.8 in 1.8.4 Sep 18, 2020

aanm removed the backport-pending/1.7 label Sep 18, 2020

aanm added the backport-done/1.7 label Sep 18, 2020

maintainer-s-little-helper bot moved this from Backport pending to v1.7 to Backport done to v1.7 in 1.7.10 Sep 18, 2020

kkourt mentioned this pull request Sep 18, 2020

Apply #13205 (identity: Perform identity lookup in remote kvstore caches too) into 1.7.9 #13213

Closed

gandro mentioned this pull request Sep 21, 2020

identity: Avoid kvstore lookup for local identities #13228

Merged

kkourt added backport-done/1.8 and removed backport-pending/1.8 labels Sep 21, 2020

maintainer-s-little-helper bot moved this from Backport pending to v1.8 to Backport done to v1.8 in 1.8.4 Sep 21, 2020

This was referenced Sep 30, 2020

Prepare for release v1.7.10 #13358

Merged

Prepare for release v1.8.4 #13361

Merged

joestringer mentioned this pull request Oct 9, 2020

Segfault in master during lookupByID() #13479

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

identity: Fix user-space security identity lookup in cluster mesh #13205

identity: Fix user-space security identity lookup in cluster mesh #13205

gandro commented Sep 17, 2020

gandro Sep 17, 2020 •

edited

joestringer Sep 17, 2020

gandro commented Sep 17, 2020

tgraf left a comment

gandro commented Sep 17, 2020

gandro commented Sep 17, 2020

joestringer Sep 17, 2020

identity: Fix user-space security identity lookup in cluster mesh #13205

identity: Fix user-space security identity lookup in cluster mesh #13205

Conversation

gandro commented Sep 17, 2020

gandro Sep 17, 2020 • edited

Choose a reason for hiding this comment

joestringer Sep 17, 2020

Choose a reason for hiding this comment

gandro commented Sep 17, 2020

tgraf left a comment

Choose a reason for hiding this comment

gandro commented Sep 17, 2020

gandro commented Sep 17, 2020

joestringer Sep 17, 2020

Choose a reason for hiding this comment

gandro Sep 17, 2020 •

edited