SyncAuthorizedEntries RPC implementation #4648
Conversation
Implements the SyncAuthorizedEntries RPC, which allows agent's to only sync down changes instead of the entire set of entries. The server-side implementation is always on and available. Agent's only use the RPC if the "use_sync_authorized_entries" feature flag is enabled in the experimental configuration. Signed-off-by: Andrew Harding <azdagron@gmail.com>
azdagron
requested review from
evan2645,
amartinezfayo,
MarcosDY and
rturner3
as code owners
Signed-off-by: Andrew Harding <azdagron@gmail.com>
| telemetry.Selectors: serverEntry.Selectors, | ||
| telemetry.Error: err.Error(), | ||
| }).Warn("Received malformed entry from SPIRE server") | ||
| continue |
There was a problem hiding this comment.
mmm, if it failed to parse (that is unexpected) entry will be removed from cache, like when entry was removed,
is this something you expected?
There was a problem hiding this comment.
Yes, I imagine if the agent does not fully understand the entry, then it's dangerous to continue serving the entry. It might have changed in significant, security sensitive ways.
| resp.Entries = append(resp.Entries, entriesToSearch[i]) | ||
| } | ||
| entriesToSearch = entriesToSearch[i:] | ||
| if len(entriesToSearch) == 0 { |
There was a problem hiding this comment.
what happens if there are some IDs left, to be returned, but we get out of entries? may we log that?
and in case entry not found, nothing is either, and slice is "recalculated" with [0:] that is doing nothing, but may we avoid that?
There was a problem hiding this comment.
IMO, the re-slicing to [0:] in this corner case seems ok since it is a valid operation and I think any code added to try and prevent it happening hurts readability....
I'm not sure about logging either. This endpoint is intended for agents and a well-behaving agent won't make a request with IDs that it didn't observe in the initial response. The code is robust against that happening but in practice it would be strange for that to happen.... how strongly do you feel about the logging? I am happy to add it if you are very worried.
| More: false, | ||
| }, | ||
| }, | ||
| // Request nothing |
There was a problem hiding this comment.
can you add a test case where you have 3 items, but in the middle one is not found?
that will reasult in a single page of 2
and what happens if the 3rd is not found? (I suspect it will be page with 2 + another page with nil.
There was a problem hiding this comment.
Sure. Added test cases.
| if totalEntries < 0 || totalEntries > maxEntries { | ||
| t.Skip() | ||
| } | ||
| if staleEntries < 0 || staleEntries > totalEntries { |
There was a problem hiding this comment.
based in boundaries, it is not possible to happen, but just in case, can you add logs or error? to prevent to have invalid tests? (same for both skip)
There was a problem hiding this comment.
Fuzzing will very much produce combinations that hit these conditions.
Signed-off-by: Andrew Harding <azdagron@gmail.com>
Signed-off-by: Andrew Harding <azdagron@gmail.com>
Signed-off-by: Andrew Harding <azdagron@gmail.com>
Signed-off-by: Andrew Harding <azdagron@gmail.com>
Signed-off-by: Andrew Harding <azdagron@gmail.com>
|
I think I've addressed all your comments, @MarcosDY. I also converted the feature flag to an experimental flag. Please let me know if I've missed something! |
* SyncAuthorizedEntries RPC implementation Implements the SyncAuthorizedEntries RPC, which allows agent's to only sync down changes instead of the entire set of entries. The server-side implementation is always on and available. Agent's only use the RPC if the "use_sync_authorized_entries" feature flag is enabled in the experimental configuration. Signed-off-by: Andrew Harding <azdagron@gmail.com>
Implements the SyncAuthorizedEntries RPC, which allows agent's to only sync down changes instead of the entire set of entries.
Note that the RPC only switches into revision diff based syncing of entries when the number of authorized entries is more than the entry page size (currently 500). This means that many deployments won't notice a change.
The server-side implementation is always on and available. Agent's only use the RPC if the "use_sync_authorized_entries" feature flag is enabled in the experimental configuration.
Fixes: #3496