API Refactor: Implement Entry.GetAuthorizedEntries RPC #1647
Conversation
Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>
amartinezfayo
requested review from
APTy,
azdagron,
evan2645 and
mcpherrinm
as code owners
pkg/server/api/api_test.go
Outdated
| ) | ||
|
|
||
| type entryFetcher struct { |
There was a problem hiding this comment.
can you call it fakeEntry fetcher and move together with methods?
There was a problem hiding this comment.
maybe put it at the end of file (after all tests
There was a problem hiding this comment.
I'll move this to a fakeentryfetcher package
pkg/server/api/api_test.go
Outdated
| ctx := context.Background() | ||
|
|
||
| entry1 := types.Entry{ | ||
| Id: "entry-1", |
There was a problem hiding this comment.
nit: maybe for this tests we can provide entries with less information, we are not doing any parsing that requires this level of information
| ) | ||
|
|
||
| type entryFetcher struct { |
There was a problem hiding this comment.
can you move it to end of test file? and looks like it is the same we use on fetcher tests, may we expose it?
There was a problem hiding this comment.
I'll move this to a fakeentryfetcher package
| expectEntries: []*types.Entry{&entry1, &entry2}, | ||
| }, | ||
| { | ||
| name: "success with output mask", |
There was a problem hiding this comment.
can add a test case with mask all false?
There was a problem hiding this comment.
and another without results?
There was a problem hiding this comment.
another test where caller id is not provided?
| @@ -623,11 +623,14 @@ func TestServiceBatchNewX509SVID(t *testing.T) { | |||
| name: "no caller id", | |||
| reqs: []string{workloadEntry.Id}, | |||
| code: codes.Internal, | |||
| err: "caller ID missing from request context", | |||
| err: "failed to fetch registration entries: no caller ID on context", | |||
There was a problem hiding this comment.
maybe: failed to fetch registration entries: missing caller ID?
pkg/server/api/api.go
Outdated
|
|
||
| // FetchAuthEntries fetches authorized entries using caller ID from context | ||
| func FetchAuthEntries(ctx context.Context, log logrus.FieldLogger, ef AuthorizedEntryFetcher) (map[string]*types.Entry, error) { | ||
| entries, err := ef.FetchAuthorizedEntries(ctx) |
There was a problem hiding this comment.
we need to put callerID validation in some place, is the idea to move it into ef.FetchAuthorizedEntries(ctx)? if that is the case can you add comments to that interface so, when we we implement it we didnt miss that validation?
annd I think we must validate how our code works when that validation fails.
There was a problem hiding this comment.
The current implementation gets the caller ID in the fetchEntries function and sends the caller ID as an argument to the FetchAuthorizedEntries function along with the context, which seems to be redundant, since the FetchAuthorizedEntries function can get the caller ID from the context and should return the authorized entries based on that caller ID (current code calls rpccontext.CallerID(ctx) twice).
I was thinking that it should be documented that FetchAuthorizedEntries should do the proper caller ID validation. I'll add comments to have this documented.
There was a problem hiding this comment.
In my opinion, use of rpccontext should stay at the API layer. I'd rather the dependencies receive their arguments explicitly.
Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>
Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>
pkg/server/api/api.go
Outdated
|
|
||
| // FetchAuthEntries fetches authorized entries using caller ID from context | ||
| func FetchAuthEntries(ctx context.Context, log logrus.FieldLogger, ef AuthorizedEntryFetcher) (map[string]*types.Entry, error) { | ||
| entries, err := ef.FetchAuthorizedEntries(ctx) |
There was a problem hiding this comment.
In my opinion, use of rpccontext should stay at the API layer. I'd rather the dependencies receive their arguments explicitly.
pkg/server/api/api_test.go
Outdated
| @@ -51,6 +59,87 @@ func TestIDFromProto(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| func TestFetchAuthEntries(t *testing.T) { | |||
| ef := &fakeentryfetcher.EntryFetcher{} | |||
There was a problem hiding this comment.
Maybe this can just be initialized down inside the loop? Then it's clear that we're starting from a clean slate instead of having to reset state every time (e.g. the err)
There was a problem hiding this comment.
I'm removing the FetchAuthEntries public function from the api package.
pkg/server/api/api.go
Outdated
| // The provided implementation of that interface must ensure that | ||
| // the authorized entries are fetched from the caller that | ||
| // is obtained from the context. | ||
| func FetchAuthEntries(ctx context.Context, log logrus.FieldLogger, ef AuthorizedEntryFetcher) (map[string]*types.Entry, error) { |
There was a problem hiding this comment.
I understand wanting to DRY this up, but I think we should wait. Both places that use this (so far) have different needs. One wants to build a map, the other doesn't. And outside of building the map, there isn't much to share here (since the AuthorizedEntryFetcher does all the work).
There was a problem hiding this comment.
I was debating myself whether this should be here or in the specific packages. It's clear for me now that two different implementations are needed.
| @@ -26,6 +30,33 @@ func ProtoFromID(id spiffeid.ID) *types.SPIFFEID { | |||
| } | |||
| } | |||
|
|
|||
| // AuthorizedEntryFetcher is the interface to fetch authorized entries | |||
| type AuthorizedEntryFetcher interface { | |||
There was a problem hiding this comment.
Thanks for moving this here :)
Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>
pkg/server/api/entry/v1/service.go
Outdated
| var entriesWithMask []*types.Entry | ||
| for _, entry := range entries { |
There was a problem hiding this comment.
nit: can we reuse the entries slice?
for i, entry := range entries {
applyMask(entry, req.OutputMask)
entries[i] = entry
}
pkg/server/api/svid/v1/service.go
Outdated
| return nil, status.Error(codes.Internal, "failed to fetch registration entries") | ||
| } | ||
|
|
||
| entriesMap := make(map[string]*types.Entry) |
There was a problem hiding this comment.
super nit:Since the length is known, it may be worth pre-allocating map capacity to reduce allocation pressure? (the RPC that uses this is going to be 3rd on the most frequently list)
| entriesMap := make(map[string]*types.Entry) | |
| entriesMap := make(map[string]*types.Entry, len(entries)) |
Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>
Implementation of Entry.GetAuthorizedEntries RPC
Fixes: #1603