Add FetchJWTSVIDs and SubscribeToJWTBundles #2789
Conversation
Signed-off-by: Yuhan Li <[email protected]>
Signed-off-by: Yuhan Li <[email protected]>
loveyana
requested review from
evan2645,
amartinezfayo,
azdagron and
rturner3
as code owners
| } | ||
| var spiffeIDs []spiffeid.ID | ||
|
|
||
| log = log.WithField(telemetry.Registered, true) |
There was a problem hiding this comment.
Looks like registered is set to false if spiffeIDs is empty, but errors from matching identities (278l) will have Registered = true.
May we set Registered = true, only if spiffeIDs != 0?
There was a problem hiding this comment.
I temporarily removed the Registered ID, and I looked at the description in telemetry. It is more like whether the caller is authenticated
| func (s *Service) SubscribeToJWTBundles(req *delegatedidentityv1.SubscribeToJWTBundlesRequest, stream delegatedidentityv1.DelegatedIdentity_SubscribeToJWTBundlesServer) error { | ||
| ctx := stream.Context() | ||
| log := rpccontext.Logger(ctx) | ||
| cachedSelectors, err := s.isCallerAuthorized(ctx, log, nil) |
There was a problem hiding this comment.
move cachedSelectors together to error verification
There was a problem hiding this comment.
I'm not sure if your comment is
var cachedSelectors []*common.Selector
if cachedSelectors, err: = s.is CallerAuthorized (ctx, log, nil); err! = nil {return err}
I looked at the spire code and did not recommend such an implementation. Are you sure we need this?
There was a problem hiding this comment.
sorry for the confusion... it is basically put error verification together with line where error is returned,
for example:
ctx := stream.Context()
log := rpccontext.Logger(ctx)
cachedSelectors, err := s.isCallerAuthorized(ctx, log, nil)
if err != nil {
return err
}
it is generally written this way to simplify reading
There was a problem hiding this comment.
😯 I do pay little attention to this matter, it has been completed now.
| managerErr error | ||
| expectTokenIDs []spiffeid.ID | ||
| }{ | ||
| { |
There was a problem hiding this comment.
can you add a test case for invalid selectors?
| @@ -441,6 +646,16 @@ func (m *FakeManager) SubscribeToCacheChanges(selectors cache.Selectors) cache.S | |||
| return newFakeSubscriber(m, m.updates) | |||
| } | |||
|
|
|||
| func (m *FakeManager) FetchJWTSVID(ctx context.Context, spiffeID spiffeid.ID, audience []string) (*client.JWTSVID, error) { | |||
| svid := m.ca.CreateJWTSVID(spiffeID, audience) | |||
| if m.err != nil { | |||
There was a problem hiding this comment.
Can you move error validation to the first line? (you are not validating generated SVID, if error is not nil)
There was a problem hiding this comment.
Thanks for this contribution @loveyana!
This is looking great. Could you please update the dependency of spire-api-sdk to point to the latest commit of next branch? That should clean the PR of build errors.
Signed-off-by: Yuhan Li <[email protected]>
Signed-off-by: Yuhan Li <[email protected]>
Signed-off-by: Yuhan Li <[email protected]>
…/spire into add-delegated-subscribe-jwt-api Signed-off-by: Yuhan Li <[email protected]>
Signed-off-by: Yuhan Li [email protected]
Pull Request check list
Affected functionality
Delegated Identity API
Description of change
Add FetchJWTSVIDs and SubscribeToJWTBundles in Delegated Identity API.
Which issue this PR fixes
#2788