spire-server: improve entry lookup for NewJWTSvid and BatchX509SVID requests #5819
Conversation
sorindumitru
force-pushed
the
lookup-authorized-entries
branch
6 times, most recently
from
d8cc2ac to
5736cb7
Compare
sorindumitru
force-pushed
the
lookup-authorized-entries
branch
3 times, most recently
from
cdfce7e to
3b8b1da
Compare
sorindumitru
requested review from
evan2645,
amartinezfayo,
MarcosDY and
rturner3
as code owners
sorindumitru
force-pushed
the
lookup-authorized-entries
branch
from
3b8b1da to
24f8d31
Compare
There was a problem hiding this comment.
Thank you @sorindumitru for this improvement! I left some comments.
pkg/server/api/api.go
Outdated
| @@ -13,6 +13,9 @@ import ( | |||
|
|
|||
| // AuthorizedEntryFetcher is the interface to fetch authorized entries | |||
| type AuthorizedEntryFetcher interface { | |||
| // LookupAuthorizedEntries fetches the entries in entrIDs that the | |||
There was a problem hiding this comment.
| // LookupAuthorizedEntries fetches the entries in entrIDs that the | |
| // LookupAuthorizedEntries fetches the entries in entryIDs that the |
| // that are directly parented against the agent. Any entries that would be | ||
| // obtained via node aliasing will not be returned until the cache is | ||
| // updated with the node selectors for the agent. | ||
| agent, _ := c.agentsByID.Get(agentRecord{ID: agentID.String()}) |
There was a problem hiding this comment.
Should we check if agent is nil? Later we assume it's not nil by accessing its Selectors field.
There was a problem hiding this comment.
I mostly got this from the previous authorized entries lookup. Looks like the Get function doesn't return nil, but the zero value of the items stored. So it's safe to look at the selectors, it should be an empty map.
| if _, ok := requestedEntries[record.EntryID]; ok { | ||
| foundEntries[record.EntryID] = cloneEntry(record.EntryCloneOnly) | ||
| } | ||
| c.findDescendents(foundEntries, record.SPIFFEID, requestedEntries, parentSeen) |
There was a problem hiding this comment.
Have you considered using an iterative approach to avoid issues in deeply nested structures? I believe it's unlikely to encounter such a structure, but may worth considering.
There was a problem hiding this comment.
That was the initial approach here, I did this because it means we can avoid copying around some data (the descendants to iterate on) which is what getAuthorizedEntries does. I'll have a look to see if it was a meaningful difference
| @@ -164,6 +192,26 @@ func (c *Cache) appendDescendents(records []entryRecord, parentID string, parent | |||
| return records | |||
| } | |||
|
|
|||
| func (c *Cache) findDescendents(foundEntries map[string]*types.Entry, parentID string, requestedEntries map[string]struct{}, parentSeen stringSet) { | |||
There was a problem hiding this comment.
I think it should be findDescendants?
Also, I wonder if the name might be confusing since it's not immediately obvious that it mutates the foundEntries map by adding descendants. Would addDescendants be a more appropriate name?
sorindumitru
force-pushed
the
lookup-authorized-entries
branch
from
24f8d31 to
1a9422d
Compare
When we know the entry or entries we are looking up we can do a faster lookup since we only have to find and copy the entries specified in the lookup request. Signed-off-by: Sorin Dumitru <[email protected]>
Signed-off-by: Sorin Dumitru <[email protected]>
Signed-off-by: Sorin Dumitru <[email protected]>
sorindumitru
force-pushed
the
lookup-authorized-entries
branch
from
1a9422d to
c2bb137
Compare
When signing SVIDs we need to verify that the agent is authorized for the entry it has requested. This is currently done by fetching all authorized entries and then seeing if the entry the agent requests a SVID for is part of them. This can be quite wasteful, since for a lot of operations we only require a small subset of the entries.
Speed up this operation by adding a new cache API for looking up a set of entries.
Pull Request check list
Affected functionality
NewJWTSVID and BatchX509SVID server APIs.
Description of change
Instead of fetching all authorized entries for a new SVID signing requests, only fetch the ones required. In the case of signign JWT-SVIDs, it's only one so there's a big improvement. In the case of X509-SVID, this is usually done in a batch so improvements will depend on batch size and the number of entries the agent is authorized for.
Which issue this PR fixes
fixes #5801