API Refactor: BatchCreateFederatedBundle RPC #1606

marcosy · 2020-06-01T20:52:18Z

Pull Request check list

Commit conforms to CONTRIBUTING.md?
Proper tests/regressions included?
Documentation updated?

Affected functionality

API Refactor

Description of change

Implements BatchCreateFederatedBundle() RPC

Which issue this PR fixes

Fixes: #1553

amartinezfayo · 2020-06-03T00:42:17Z

pkg/server/api/bundle/v1/service.go

+	for _, b := range req.Bundle {
+		td, err := spiffeid.TrustDomainFromString(b.TrustDomain)
+		if err != nil {
+			log.Errorf("Trust domain argument is not a valid SPIFFE ID: %q", b.TrustDomain)


The trust domain string could be either the host part (e.g. example.org) or the SPIFFE ID URI (e.g. spiffe://example.org). We actually refer to "the name of the trust domain the bundle belongs to": https://github.com/spiffe/spire/blob/master/proto/spire-next/types/bundle.proto#L7
So I would just say in the error that the trust domain argument is not valid.

and in this king of logs it is better to add a field. with trust domain, instead of be part of error message.

for consistency in logs we are adding Invalid request: $ERROR_MESSAGE

amartinezfayo · 2020-06-03T00:46:45Z

pkg/server/api/bundle/v1/service.go

+		if err != nil {
+			log.Errorf("Trust domain argument is not a valid SPIFFE ID: %q", b.TrustDomain)
+			results = append(results, &bundle.BatchCreateFederatedBundleResponse_Result{
+				Status: CreateStatus(codes.InvalidArgument, "trust domain argument is not a valid SPIFFE ID: %q", b.TrustDomain),


The same comment about the trust domain string applies here.

MarcosDY · 2020-06-03T11:39:54Z

pkg/server/api/bundle/v1/service.go

+	for _, b := range req.Bundle {
+		td, err := spiffeid.TrustDomainFromString(b.TrustDomain)
+		if err != nil {
+			log.Errorf("Trust domain argument is not a valid SPIFFE ID: %q", b.TrustDomain)


and in this king of logs it is better to add a field. with trust domain, instead of be part of error message.

MarcosDY · 2020-06-03T11:42:17Z

pkg/server/api/bundle/v1/service.go

+	for _, b := range req.Bundle {
+		td, err := spiffeid.TrustDomainFromString(b.TrustDomain)
+		if err != nil {
+			log.Errorf("Trust domain argument is not a valid SPIFFE ID: %q", b.TrustDomain)


for consistency in logs we are adding Invalid request: $ERROR_MESSAGE

MarcosDY · 2020-06-03T11:44:00Z

pkg/server/api/bundle/v1/service.go

+			results = append(results, &bundle.BatchCreateFederatedBundleResponse_Result{
+				Status: CreateStatus(codes.InvalidArgument, "trust domain argument is not a valid SPIFFE ID: %q", b.TrustDomain),
+			})
+			continue


NIT: instead to have too many continue, you can create a method to createFederatedBundles annd return errors,
then in Batch method you can basically parse that error into status and create result, that will make it cleaner

MarcosDY · 2020-06-03T11:51:35Z

pkg/server/api/bundle/v1/service.go

+		}
+
+		if s.td.Compare(td) == 0 {
+			log.Errorf("%q is this server own trust domain", td.String())


from here that we have a td, we may add it as a field to all logs

Suggested change

log.Errorf("%q is this server own trust domain", td.String())

log.Error("Creating a federated bundle for the server's own trust domain is not allowed")

MarcosDY · 2020-06-03T12:06:30Z

pkg/server/api/bundle/v1/service.go

+		}
+		applyBundleMask(protoBundle, req.OutputMask)
+
+		log.Infof("Bundle created successfully for trust domain: %q", td.String())


not sure we want an info log here, it can spam if we add several bundles

I think this is probably ok? It shouldn't be too noisy considering how often this is expected to happen.

MarcosDY · 2020-06-03T12:09:12Z

pkg/server/api/bundle/v1/service.go


-	return out, nil
+// CreateStatus creates a proto Status
+func CreateStatus(code codes.Code, format string, a ...interface{}) *types.Status {


I dont think here is a good place for this method you have status methods on api: https://github.com/spiffe/spire/blob/master/pkg/server/api/status.go#L12

Right! I forgot to remove it after it got merged

MarcosDY · 2020-06-03T12:22:54Z

pkg/server/api/bundle/v1/service_test.go

+		expectedLogMsgs []spiretest.LogEntry
+		dsError         error
+	}{
+		{


maybe you can add a test case where you create more than one bundle and someone fails and another not, and verify that we always keep order, and expected logs+results

another test case to add is a mask with all false, what happens in that case? and when it is nil?

another test case when bundles is empty?

maybe you can add a test case where you create more than one bundle and someone fails and another not, and verify that we always keep order, and expected logs+results

This scenario is covered by "Create fails if bundle already exists"

another test case to add is a mask with all false, what happens in that case? and when it is nil?

Will do

MarcosDY · 2020-06-03T12:27:04Z

pkg/server/api/bundle/v1/service.go

-	return nil, status.Errorf(codes.Unimplemented, "method BatchCreateFederatedBundle not implemented")
+	log := rpccontext.Logger(ctx)
+
+	results := []*bundle.BatchCreateFederatedBundleResponse_Result{}


what happens when req is emtpy? must we do nothing? or return an error?

Hmm, good question. I think I'd lean towards returning an empty response instead of failing.

Also, I'd probably recommend either initializing this slice to nil, or preallocating the slice to the number of incoming bundles.

MarcosDY · 2020-06-03T12:29:00Z

pkg/server/api/bundle/v1/service_test.go

+			require.NotNil(t, resp)
+			spiretest.AssertLogs(t, test.logHook.AllEntries(), tt.expectedLogMsgs)
+
+			require.Equal(t, len(tt.expectedResults), len(resp.Results))


instead of this you can use spiretest.RequireProtoEqual

and you have have an expected where you play with mask, to be sure response has exactly what we want

MarcosDY · 2020-06-03T12:32:21Z

pkg/server/api/bundle/v1/service.go

+			})
+			continue
+		}
+		resp, err := s.ds.CreateBundle(ctx, &datastore.CreateBundleRequest{


I think before we call create bundle we must validate that rootca and public keys are valid

azdagron · 2020-06-03T18:28:45Z

pkg/server/api/bundle.go

+	rootCas := []*common.Certificate{}
+	for _, certificate := range b.X509Authorities {
+		rootCas = append(rootCas, &common.Certificate{
+			DerBytes: certificate.Asn1,


We should be validating that the incoming ASN.1 certificate data is well formed. See the helpers added in the AppendBundle PR.

Right, I'm thinking about moving the helpers added in AppendBundle PR to this file. And then, modify AppendBundle to use ProtoToBundle instead of the individual helpers.

Sounds good?

Yes, although the api package is where we've been sticking the *ToProto and ProtoTo* methods.

azdagron · 2020-06-03T18:29:02Z

pkg/server/api/bundle.go

+
+	jwtSigningKeys := []*common.PublicKey{}
+	for _, key := range b.JwtAuthorities {
+		jwtSigningKeys = append(jwtSigningKeys, &common.PublicKey{


We should be validating that the incoming key id and pkix bytes. See the helpers added in the AppendBundle PR.

azdagron · 2020-06-03T18:33:16Z

pkg/server/api/bundle/v1/service.go

-	return nil, status.Errorf(codes.Unimplemented, "method BatchCreateFederatedBundle not implemented")
+	log := rpccontext.Logger(ctx)
+
+	results := []*bundle.BatchCreateFederatedBundleResponse_Result{}


Hmm, good question. I think I'd lean towards returning an empty response instead of failing.

Also, I'd probably recommend either initializing this slice to nil, or preallocating the slice to the number of incoming bundles.

azdagron · 2020-06-03T18:36:18Z

pkg/server/api/bundle/v1/service.go

+		}
+
+		if s.td.Compare(td) == 0 {
+			log.Errorf("%q is this server own trust domain", td.String())


Suggested change

log.Errorf("%q is this server own trust domain", td.String())

log.Error("Creating a federated bundle for the server's own trust domain is not allowed")

azdagron · 2020-06-03T18:37:12Z

pkg/server/api/bundle/v1/service.go

+		if s.td.Compare(td) == 0 {
+			log.Errorf("%q is this server own trust domain", td.String())
+			results = append(results, &bundle.BatchCreateFederatedBundleResponse_Result{
+				Status: CreateStatus(codes.InvalidArgument, "%q is this server own trust domain", td.String()),


Suggested change

Status: CreateStatus(codes.InvalidArgument, "%q is this server own trust domain", td.String()),

Status: CreateStatus(codes.InvalidArgument, "creating a federated bundle for the server's own trust domain (%s) s not allowed", td.String()),

azdagron · 2020-06-03T18:39:02Z

pkg/server/api/bundle/v1/service.go

+		if status.Code(err) == codes.AlreadyExists {
+			log.Error("Bundle already exists")
+			results = append(results, &bundle.BatchCreateFederatedBundleResponse_Result{
+				Status: CreateStatus(codes.AlreadyExists, "bundle already exists"),
+			})
+			continue
+		}
+		if err != nil {
+			log.WithError(err).Error("Unable to create bundle")
+			results = append(results, &bundle.BatchCreateFederatedBundleResponse_Result{
+				Status: CreateStatus(codes.Internal, "unable to create bundle: %v", err),
+			})
+			continue
+		}


nit: switches are a nice way to group these to related conditions

azdagron · 2020-06-03T18:40:11Z

pkg/server/api/bundle/v1/service.go

+		}
+		applyBundleMask(protoBundle, req.OutputMask)
+
+		log.Infof("Bundle created successfully for trust domain: %q", td.String())


I think this is probably ok? It shouldn't be too noisy considering how often this is expected to happen.

azdagron · 2020-06-03T18:41:42Z

pkg/server/api/bundle/v1/service_test.go

@@ -451,3 +572,37 @@ func setupServiceTest(t *testing.T) *serviceTest {

 	return test
 }
+
+func getValidBundle(td spiffeid.TrustDomain) *types.Bundle {


nit: get implies you are obtaining something that already exists

Suggested change

func getValidBundle(td spiffeid.TrustDomain) *types.Bundle {

func makeBundle(td spiffeid.TrustDomain) *types.Bundle {

azdagron · 2020-06-03T18:42:01Z

pkg/server/api/bundle/v1/service_test.go

+	}
+}
+
+func getValidCommonBundle(t *testing.T, td spiffeid.TrustDomain) *common.Bundle {


nit: get implies you are obtaining something that already exists

Suggested change

func getValidCommonBundle(t *testing.T, td spiffeid.TrustDomain) *common.Bundle {

func makeCommonBundle(t *testing.T, td spiffeid.TrustDomain) *common.Bundle {

marcosy · 2020-06-04T22:56:53Z

Thank you all for the feedback! I think I fixed all the comments. Please let me know if I missed any of them 🙂

azdagron · 2020-06-05T21:29:15Z

pkg/server/api/bundle.go

+	}
+
+	td, err := spiffeid.TrustDomainFromString(b.TrustDomain)
+	if err != nil && b.TrustDomain != "" {


TrustDomain should be a required field on the bundle. I'm curious what case you hit where there wasn't one set that wasn't an error condition?

In the AppendBundle RPC the InputMask is applied before calling ProtoToBundle(...). So, it could be a case where TrustDomain flag is not set in the input mask and it is passed as empty to ProtoToBundle.

maybe we can apply mask after parsing? but here I got a question...
In actual scenary we does not validate fields that are not going to be updated, that make sense because if you dont want to update something you will just nil it on proto when sending it.

So maybe actuall appr calling mask before parsing make sense, but is that waht we want? (I'm scarred about issues because we get too permisive on our parsing)

azdagron · 2020-06-05T21:34:30Z

pkg/server/api/bundle.go

+		}
+
+		if key.KeyId == "" {
+			return nil, errors.New("missing KeyId")


Suggested change

return nil, errors.New("missing KeyId")

return nil, errors.New("missing key ID")

azdagron

Looking good! Thanks, @marcosy !

Signed-off-by: Marcos Yedro <marcosyedro@gmail.com>

marcosy requested review from amartinezfayo, APTy, azdagron, evan2645 and mcpherrinm as code owners June 1, 2020 20:52

amartinezfayo reviewed Jun 3, 2020

View reviewed changes

MarcosDY reviewed Jun 3, 2020

View reviewed changes

azdagron reviewed Jun 3, 2020

View reviewed changes

azdagron reviewed Jun 8, 2020

View reviewed changes

azdagron previously approved these changes Jun 10, 2020

View reviewed changes

Implement BatchCreateFederatedBundle RPC

69aa401

Signed-off-by: Marcos Yedro <marcosyedro@gmail.com>

marcosy dismissed azdagron’s stale review via 69aa401 June 10, 2020 20:04

marcosy force-pushed the 1553-BatchCreateFederatedBundleRPC branch from 7628207 to 69aa401 Compare June 10, 2020 20:04

azdagron approved these changes Jun 10, 2020

View reviewed changes

azdagron merged commit 867368b into spiffe:master Jun 10, 2020

	log.Errorf("%q is this server own trust domain", td.String())
	log.Error("Creating a federated bundle for the server's own trust domain is not allowed")

	Status: CreateStatus(codes.InvalidArgument, "%q is this server own trust domain", td.String()),
	Status: CreateStatus(codes.InvalidArgument, "creating a federated bundle for the server's own trust domain (%s) s not allowed", td.String()),

	func getValidBundle(td spiffeid.TrustDomain) *types.Bundle {
	func makeBundle(td spiffeid.TrustDomain) *types.Bundle {

	func getValidCommonBundle(t testing.T, td spiffeid.TrustDomain) common.Bundle {
	func makeCommonBundle(t testing.T, td spiffeid.TrustDomain) common.Bundle {

	return nil, errors.New("missing KeyId")
	return nil, errors.New("missing key ID")

API Refactor: BatchCreateFederatedBundle RPC #1606

API Refactor: BatchCreateFederatedBundle RPC #1606

Conversation

marcosy commented Jun 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcosy commented Jun 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcosDY Jun 9, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azdagron left a comment

Choose a reason for hiding this comment

MarcosDY Jun 9, 2020 •

edited