API Refactor: Make API error reporting consistent #1734

marcosy · 2020-07-21T21:47:48Z

Pull Request check list

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

Affected functionality

Error reporting and logging for the new API

Description of change

This PR aims to increase the consistency in the error reporting and logging for the new API.

The following consistency criteria were adopted based on offline conversations and the current API style (feel free to propose changes!):

Errors must be returned and logged on the server side.
Inner errors must be logged as fields instead of being part of the message.
Errors with InvalidArgument code must prefix the logged message with an "Invalid argument:" string.
Errors with NotFound code, must ignore the inner error and just log/return the message.

It was hard to find a pattern for the OK status code. Sometimes it is desired to log a message, sometimes not, sometimes the log level is different.

It would be great to hear ideas/proposals about it 🙂

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

amartinezfayo

Thanks for this @marcosy! It's looking great.

amartinezfayo · 2020-07-21T22:07:37Z

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

-		return nil, status.Error(codes.InvalidArgument, "only the trust domain of the server can be appended")
-	}
-
-	dsResp, err := s.ds.FetchBundle(ctx, &datastore.FetchBundleRequest{


Looks like this was not really used, good catch!

the intention in that code was to validate that bundle exist before trying to append a new certificate, can you add a new test to validate that ds prevents that scenary?

Oh I see. The AppendBundle datastore RPC checks for the existence of the bundle and creates it if it does not exists.
The current implementation (the one that makes the Fetch call) fails if the bundle does not exist.

I'm not sure which is the desired behaviour for this RPC. If we want it to fail when the bundle does not exist we should update the AppendBundle datastore RPC to handle this and avoid possible races.

I don't think this preemptive fetch is necessary. The datastore will do the right thing here.

amartinezfayo · 2020-07-21T22:21:36Z

pkg/server/api/status.go

+}
+
+// MakeStatus logs and returns a status composed of: msg, err and code.
+// Errors are treated differentially according to its gRPC code.


Suggested change

// Errors are treated differentially according to its gRPC code.

// Errors are treated differently according to its gRPC code.

amartinezfayo · 2020-07-21T22:21:58Z

pkg/server/api/status.go

+	return CreateStatus(code, status.Convert(e).Message())
+}
+
+// MakeErr logs and returns an error comoposed of: msg, err and code.


Suggested change

// MakeErr logs and returns an error comoposed of: msg, err and code.

// MakeErr logs and returns an error composed of: msg, err and code.

amartinezfayo · 2020-07-21T22:22:14Z

pkg/server/api/status.go

+}
+
+// MakeErr logs and returns an error comoposed of: msg, err and code.
+// Errors are treated differentially according to its gRPC code.


Suggested change

// Errors are treated differentially according to its gRPC code.

// Errors are treated differently according to its gRPC code.

amartinezfayo · 2020-07-21T22:40:41Z

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

@@ -250,21 +230,18 @@ func (s *Service) RenewAgent(ctx context.Context, req *agent.RenewAgentRequest)
 	log := rpccontext.Logger(ctx)

 	if err := rpccontext.RateLimit(ctx, 1); err != nil {
-		log.WithError(err).Error("Rejecting request due to renew agent rate limiting")
-		return nil, err
+		return nil, api.MakeErr(log, codes.Unavailable, "rejecting request due to renew agent rate limiting", err)


Maybe codes.ResourceExhausted is a better fit for rate limiting errors?

do you verify what error is returned by rate limit? i can recall it returned a grpc status with the correct code

I was wondering the same... I am not sure which one to choose. I found precedence for both of them:

The HTTP to gRPC mapping table suggests mapping code 429 Too Many Requests to UNAVAILABLE.

The Google cloud API design guide suggests using RESOURCE_EXAUSTED

I did not see your previous comment @MarcosDY. Now that you mention it, it seems to me that the right thing to do is to extract and use the error code that comes from the rate limiter. Which could be: Internal, ResourceExausted or InvalidArgument.

or we can just use error from limiter as it was

amartinezfayo · 2020-07-21T22:53:52Z

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

 					Data: logrus.Fields{
 						telemetry.TrustDomainID: serverTrustDomain.String(),
 						logrus.ErrorKey:         "unable to parse JWT authority: missing key ID",
 					},
 				},
 			},
 		},
-		{
-			name: "datasource fails",


We still call the datastore to append the bundle, so I think that we should keep the datastore failure case. It needs to be updated with the "failed to append bundle" error instead of "failed to fetch server bundle".

Right! good catch!

MarcosDY · 2020-07-22T12:57:17Z

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

 	}

 	_, err = s.ds.DeleteAttestedNode(ctx, &datastore.DeleteAttestedNodeRequest{
 		SpiffeId: id.String(),
 	})
 	switch status.Code(err) {
 	case codes.OK:
-		log.Info("Agent deleted")
-		return &empty.Empty{}, nil
+		return &empty.Empty{}, api.MakeErr(log, codes.OK, "agent deleted", nil)


it does not feel right to use MakeErr for creating an OK message, we really need to have a message for OK? it looks to me like a Succes is enough in all case

MarcosDY · 2020-07-22T13:12:30Z

pkg/server/api/status.go

+		// Add the prefix 'Invalid argument' for InvalidArgument errors
+		if err != nil {
+			log = log.WithError(err)
+			errMsg = fmt.Sprintf(msg+": %v", err)


it is a little hard to read, why not make: fmt.Sprintf("%s: %v", msg, err)?

MarcosDY · 2020-07-22T13:15:57Z

pkg/server/api/status.go

+			errMsg = fmt.Sprintf(msg+": %v", err)
+		}
+
+		log.Error("Invalid argument: " + msg)


log.Errorf("Invalid argument: %s", msg)

MarcosDY · 2020-07-22T13:27:46Z

pkg/server/api/status.go

+// MakeErr logs and returns an error comoposed of: msg, err and code.
+// Errors are treated differentially according to its gRPC code.
+func MakeErr(log logrus.FieldLogger, code codes.Code, msg string, err error) error {
+	errMsg := msg


errMsg is not really used, it is replaced after initialized

errMsg is used in the default and InvalidArgument cases. In those cases the returned error contains the description of the inner error (if not nil).

MarcosDY · 2020-07-22T13:28:33Z

pkg/server/api/status.go

+	case codes.NotFound:
+		// Do not log nor return the inner error for NotFound errors
+		log.Error(capitalize(msg))
+		return status.Error(code, errMsg)


no changes on errMsg you can use msg here

Right, both can be used. I'd rather use errMsg here for consistency:
msg -> for all the logged messages
errMsg -> for the returned error.

MarcosDY · 2020-07-22T13:28:58Z

pkg/server/api/status.go

+	default:
+		if err != nil {
+			log = log.WithError(err)
+			errMsg = fmt.Sprintf(msg+": %v", err)


same comments that for InvalidArgument

MarcosDY · 2020-07-22T13:46:52Z

pkg/server/api/status.go

+func MakeErr(log logrus.FieldLogger, code codes.Code, msg string, err error) error {
+	errMsg := msg
+	switch code {
+	case codes.OK:


it feels like OK can have its own methods, it does not feel right to call MakeErr on case of success

Sounds good! I'll change it

azdagron · 2020-07-24T18:00:40Z

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

-		return nil, status.Error(codes.InvalidArgument, "only the trust domain of the server can be appended")
-	}
-
-	dsResp, err := s.ds.FetchBundle(ctx, &datastore.FetchBundleRequest{


I don't think this preemptive fetch is necessary. The datastore will do the right thing here.

azdagron · 2020-07-24T18:02:31Z

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

 	}

 	resp, err := s.ds.AppendBundle(ctx, &datastore.AppendBundleRequest{
 		Bundle: &common.Bundle{
 			TrustDomainId:  td.IDString(),
 			JwtSigningKeys: dsBundle.JwtSigningKeys,
 			RootCas:        dsBundle.RootCas,
+			RefreshHint:    dsBundle.RefreshHint,


Append only operates on the JWT signing key and Root CAs.

I'm wondering if taking a bundle as input is even appropriate considering we'll ignore the sequence number and refresh hint...

Good catch! I thought it was missing. Right, perhaps the AppendBundleRequest could take only the Root CAs and the JWT keys.

azdagron

Thanks, @marcosy !

This commit improves the consistency for the returned and logged errors in the new API. In addition, the AppendBundle RPC is slightly modified to not require a full bundle but only the Root CAs and JWT keys instead. Signed-off-by: Marcos Yedro <marcosyedro@gmail.com>

marcosy requested review from amartinezfayo, APTy, azdagron, evan2645 and mcpherrinm as code owners July 21, 2020 21:47

amartinezfayo reviewed Jul 21, 2020

View reviewed changes

MarcosDY reviewed Jul 22, 2020

View reviewed changes

azdagron reviewed Jul 24, 2020

View reviewed changes

azdagron previously approved these changes Jul 29, 2020

View reviewed changes

marcosy dismissed azdagron’s stale review via 84923e6 July 29, 2020 14:52

marcosy force-pushed the api-refactor-make-error-reporting-consistent branch from 8e05078 to 84923e6 Compare July 29, 2020 14:52

azdagron approved these changes Jul 29, 2020

View reviewed changes

amartinezfayo approved these changes Jul 29, 2020

View reviewed changes

azdagron merged commit 1c9858f into spiffe:master Jul 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API Refactor: Make API error reporting consistent #1734

API Refactor: Make API error reporting consistent #1734

marcosy commented Jul 21, 2020

amartinezfayo left a comment

amartinezfayo Jul 21, 2020

MarcosDY Jul 22, 2020

marcosy Jul 22, 2020

azdagron Jul 24, 2020

amartinezfayo Jul 21, 2020

amartinezfayo Jul 21, 2020

amartinezfayo Jul 21, 2020

amartinezfayo Jul 21, 2020

MarcosDY Jul 22, 2020

marcosy Jul 22, 2020

marcosy Jul 22, 2020

MarcosDY Jul 22, 2020

amartinezfayo Jul 21, 2020

marcosy Jul 22, 2020

MarcosDY Jul 22, 2020

MarcosDY Jul 22, 2020

MarcosDY Jul 22, 2020

MarcosDY Jul 22, 2020

marcosy Jul 22, 2020

MarcosDY Jul 22, 2020

marcosy Jul 22, 2020

MarcosDY Jul 22, 2020

MarcosDY Jul 22, 2020

marcosy Jul 22, 2020

azdagron Jul 24, 2020

azdagron Jul 24, 2020

marcosy Jul 24, 2020

azdagron left a comment

	// Errors are treated differentially according to its gRPC code.
	// Errors are treated differently according to its gRPC code.

	// MakeErr logs and returns an error comoposed of: msg, err and code.
	// MakeErr logs and returns an error composed of: msg, err and code.

API Refactor: Make API error reporting consistent #1734

API Refactor: Make API error reporting consistent #1734

Conversation

marcosy commented Jul 21, 2020

amartinezfayo left a 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

azdagron left a comment

Choose a reason for hiding this comment