API Refactor: GetFederatedBundle RPC #1576
Conversation
Signed-off-by: Marcos Yedro <marcosyedro@gmail.com>
pkg/server/api/bundle/v1/service.go
Outdated
| log.Warnf("Using a full SPIFFE ID as trust domain argument, path section will be ignored: %q", reqTrustDomainID.String()) | ||
| } | ||
|
|
||
| if callerID.MemberOf(reqTrustDomainID.TrustDomain()) { |
There was a problem hiding this comment.
We should be using the server's trust domain for this check (i.e. provide one in the configuration) instead of the caller.
pkg/server/api/bundle/v1/service.go
Outdated
| return nil, status.Error(codes.Internal, "cannot get SPIFFE ID from caller") | ||
| } | ||
|
|
||
| reqTrustDomainID, err := spiffeid.FromString(req.TrustDomain) |
There was a problem hiding this comment.
req.TrustDomain is just the trust domain name, e.g., "example.org". We should use spiffeid.TrustDomainFromString() and then fetch from the datastore via td.IDString()
nit: also reqTrustDomain is pretty long :) I think under this context something shorter would be just as clear?
| reqTrustDomainID, err := spiffeid.FromString(req.TrustDomain) | |
| td, err := spiffeid.FromString(req.TrustDomain) |
pkg/server/api/bundle/v1/service.go
Outdated
| } | ||
|
|
||
| dsResp, err := s.c.ds.FetchBundle(ctx, &datastore.FetchBundleRequest{ | ||
| TrustDomainId: req.TrustDomain, |
There was a problem hiding this comment.
| TrustDomainId: req.TrustDomain, | |
| TrustDomainId: td.IDString(), |
pkg/server/api/bundle/v1/service.go
Outdated
| return nil, status.Errorf(codes.InvalidArgument, "trust domain argument is not a valid SPIFFE ID: %q", req.TrustDomain) | ||
| } | ||
|
|
||
| if reqTrustDomainID.Empty() { |
There was a problem hiding this comment.
The FromString method will have already returned an error if the trust domain is empty. If you want to explicitly check for an empty req.TrustDomain field though, you should do that before the FromString call.
pkg/server/api/bundle/v1/service.go
Outdated
| return nil, status.Errorf(codes.InvalidArgument, "trust domain argument is empty") | ||
| } | ||
|
|
||
| if reqTrustDomainID.Path() != "" { |
There was a problem hiding this comment.
How would you feel about removing this? I don't see why the service can't be a little flexible and allow for both trust domain names and trust domain ids here. If you use spiffeid.TrustDomainFromString above as suggested then it will transparently handle both.
pkg/server/api/bundle/v1/service.go
Outdated
| return nil, status.Errorf(codes.NotFound, "bundle for %q not found", req.TrustDomain) | ||
| } | ||
|
|
||
| resp := &types.Bundle{} |
There was a problem hiding this comment.
How would you feel about moving this into a helper function that takes the common.Bundle and an output mask and returns a types.Bundle with the mask applied? We're going to need that everywhere.
There was a problem hiding this comment.
sounds good, I'll move it
pkg/server/api/bundle/v1/service.go
Outdated
| //TODO: Where do we get the sequence number from? | ||
| // There is not a `dsResp.Bundle.SequenceNumber` field |
There was a problem hiding this comment.
SPIRE does not yet support sequence numbers.
| //TODO: Where do we get the sequence number from? | |
| // There is not a `dsResp.Bundle.SequenceNumber` field | |
| //TODO: filter sequence numbers when SPIRE supports them |
pkg/server/api/bundle/v1/service.go
Outdated
|
|
||
| resp := &types.Bundle{} | ||
| if req.OutputMask.TrustDomainId { | ||
| resp.TrustDomainId.TrustDomain = dsResp.Bundle.TrustDomainId |
There was a problem hiding this comment.
Hmm, this points out some inconsistency in the API.types.Bundle should probably use the trust domain name instead of trust domain id.....
I'll open a conversation on the issue.
Signed-off-by: Marcos Yedro <marcosyedro@gmail.com>
Signed-off-by: Marcos Yedro <marcosyedro@gmail.com>
Signed-off-by: Marcos Yedro <marcosyedro@gmail.com>
Signed-off-by: Marcos Yedro <marcosyedro@gmail.com>
| return nil, status.Errorf(codes.Unimplemented, "method BatchDeleteFederatedBundle not implemented") | ||
| } | ||
|
|
||
| func applyMask(b *common.Bundle, mask *types.BundleMask) (*types.Bundle, error) { |
There was a problem hiding this comment.
I missed this in my first pass. Output mask is optional. If unset, it means "return everything".
pkg/server/api/bundle/v1/service.go
Outdated
| } | ||
|
|
||
| func (s *Service) GetFederatedBundle(ctx context.Context, req *bundle.GetFederatedBundleRequest) (*types.Bundle, error) { | ||
| log := rpccontext.Logger(ctx).WithField("RPC", "GetFedertedBundle") |
There was a problem hiding this comment.
This is handled by the logging middleware aleady (it adds the service and method fields)
There was a problem hiding this comment.
Great, I'll remove it!
Signed-off-by: Marcos Yedro <marcosyedro@gmail.com>
marcosy
changed the title
| "google.golang.org/grpc/status" | ||
| ) | ||
|
|
||
| var defaultMask = &types.BundleMask{ |
There was a problem hiding this comment.
I think we're going to need/want some common way of applying masks. I think we could wire up a little code generation to the makefile to generate mask application code for the various types. Either way I think this is ok for now.
Signed-off-by: Marcos Yedro marcosyedro@gmail.com
Pull Request check list
Affected functionality
API refactor
Description of change
Adds
GetFederatedBundle()RPC.This is still work in progress (need to add unit tests). Pushing as draft PR in case someone wants to provide early feedback and for coordination with people working on other bundle related RPCs.Which issue this PR fixes
Fixes: #1552