Add API Error Handler (#74)

* Add error handler that converts from rpc.Status errors to Watchtower API errors.

* Better testing for ErrorHandler.

* Rename testcase and remove empty default switch case.

* Change the error status back to int64.

* Converting everything to int32s.

jsonpb wraps int64s as a string which we dont like.  We'll figure out how to use values larger than int64s when it comes up.

* Remove special casing for TraceId which isn't needed anymore.

* Removing wrappers from error details since we never need to know when they are set or unset from the end user.

* Using the helper error functions inside the project service.

* Correct usage of hclog, replace panic with Error log.

* Adding periods to all API returned errors, correct spelling, fix missed invalid argument error not using helper function.

* Change our logged errors to Error instead of Warn.

* Add TODO for defining and using our own defined API error codes.

* Add TODO to remove internal error messages.

internal/servers/controller/handler.go

@@ -15,6 +15,7 @@ import (
 	"github.com/hashicorp/watchtower/api"
 	"github.com/hashicorp/watchtower/globals"
 	"github.com/hashicorp/watchtower/internal/gen/controller/api/services"
+	"github.com/hashicorp/watchtower/internal/servers/controller/handlers"
 	"github.com/hashicorp/watchtower/internal/servers/controller/handlers/host_catalogs"
 	"github.com/hashicorp/watchtower/internal/servers/controller/handlers/host_sets"
 	"github.com/hashicorp/watchtower/internal/servers/controller/handlers/hosts"
@@ -57,7 +58,7 @@ func handleGrpcGateway(c *Controller) http.Handler {
 	// Register*ServiceHandlerServer methods ignore the passed in ctx.  Using the baseContext now just in case this changes
 	// in the future, at which point we'll want to be using the baseContext.
 	ctx := c.baseContext
-	mux := runtime.NewServeMux()
+	mux := runtime.NewServeMux(runtime.WithProtoErrorHandler(handlers.ErrorHandler(c.logger)))
 	services.RegisterHostCatalogServiceHandlerServer(ctx, mux, &host_catalogs.Service{})
 	services.RegisterHostSetServiceHandlerServer(ctx, mux, &host_sets.Service{})
 	services.RegisterHostServiceHandlerServer(ctx, mux, &hosts.Service{})

internal/servers/controller/handlers/errors.go

@@ -0,0 +1,88 @@
+package handlers
+
+import (
+	"context"
+	"fmt"
+	"io"
+	"net/http"
+
+	"github.com/grpc-ecosystem/grpc-gateway/runtime"
+	"github.com/hashicorp/go-hclog"
+	pb "github.com/hashicorp/watchtower/internal/gen/controller/api"
+	"google.golang.org/genproto/googleapis/rpc/errdetails"
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
+)
+
+func NotFoundErrorf(msg string, a ...interface{}) error {
+	return status.Errorf(codes.NotFound, msg, a...)
+}
+
+func InvalidArgumentErrorf(msg string, fields []string) error {
+	st := status.New(codes.InvalidArgument, msg)
+	br := &errdetails.BadRequest{}
+	for _, f := range fields {
+		br.FieldViolations = append(br.FieldViolations, &errdetails.BadRequest_FieldViolation{Field: f})
+	}
+	st, err := st.WithDetails(br)
+	if err != nil {
+		hclog.Default().Error("failure building status with details", "details", br, "error", err)
+		return status.Error(codes.Internal, "Failed to build InvalidArgument error.")
+	}
+	return st.Err()
+}
+
+func statusErrorToApiError(s *status.Status) *pb.Error {
+	apiErr := &pb.Error{}
+	apiErr.Status = int32(runtime.HTTPStatusFromCode(s.Code()))
+	apiErr.Message = s.Message()
+	// TODO(ICU-193): Decouple from the status codes and instead use codes defined specifically for our API.
+	apiErr.Code = s.Code().String()
+
+	for _, ed := range s.Details() {
+		switch ed.(type) {
+		case *errdetails.BadRequest:
+			br := ed.(*errdetails.BadRequest)
+			for _, fv := range br.GetFieldViolations() {
+				if apiErr.Details == nil {
+					apiErr.Details = &pb.ErrorDetails{}
+				}
+				apiErr.Details.RequestFields = append(apiErr.Details.RequestFields, fv.GetField())
+			}
+		}
+	}
+	return apiErr
+}
+
+// TODO(ICU-194): Remove all information from internal errors.
+func ErrorHandler(logger hclog.Logger) runtime.ProtoErrorHandlerFunc {
+	const errorFallback = `{"error": "failed to marshal error message"}`
+	return func(ctx context.Context, _ *runtime.ServeMux, marshaler runtime.Marshaler, w http.ResponseWriter, r *http.Request, inErr error) {
+		if inErr == runtime.ErrUnknownURI {
+			// grpc gateway uses this error when the path was not matched, but the error uses codes.Unimplemented which doesn't match the intention.
+			// Overwrite the error to match our expected behavior.
+			inErr = status.Error(codes.NotFound, http.StatusText(http.StatusNotFound))
+		}
+		s, ok := status.FromError(inErr)
+		if !ok {
+			s = status.New(codes.Unknown, inErr.Error())
+		}
+		apiErr := statusErrorToApiError(s)
+		buf, merr := marshaler.Marshal(apiErr)
+		if merr != nil {
+			logger.Error("failed to marshal error response", "response", fmt.Sprintf("%#v", apiErr), "error", merr)
+			w.WriteHeader(http.StatusInternalServerError)
+			if _, err := io.WriteString(w, errorFallback); err != nil {
+				logger.Error("failed to write response", "error", err)
+			}
+			return
+		}
+
+		w.Header().Set("Content-Type", marshaler.ContentType())
+		w.WriteHeader(int(apiErr.GetStatus()))
+		if _, err := w.Write(buf); err != nil {
+			logger.Error("failed to send response chunk", "error", err)
+			return
+		}
+	}
+}

internal/servers/controller/handlers/errors_test.go

@@ -0,0 +1,101 @@
+package handlers
+
+import (
+	"context"
+	"io/ioutil"
+	"net/http"
+	"net/http/httptest"
+	"testing"
+
+	"github.com/golang/protobuf/proto"
+	"github.com/grpc-ecosystem/grpc-gateway/runtime"
+	"github.com/hashicorp/go-hclog"
+	pb "github.com/hashicorp/watchtower/internal/gen/controller/api"
+	"github.com/stretchr/testify/assert"
+	sdpb "google.golang.org/genproto/googleapis/rpc/errdetails"
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
+)
+
+func TestApiErrorHandler(t *testing.T) {
+	ctx := context.Background()
+	req, err := http.NewRequest("GET", "madeup/for/the/test", nil)
+	if err != nil {
+		t.Fatalf("Couldn't create test request")
+	}
+	mux := runtime.NewServeMux()
+	_, outMarsh := runtime.MarshalerForRequest(mux, req)
+
+	tested := ErrorHandler(hclog.L())
+
+	testCases := []struct {
+		name          string
+		err           error
+		statusDetails []proto.Message
+		expected      *pb.Error
+	}{
+		{
+			name: "Not Found",
+			err:  status.Error(codes.NotFound, "test"),
+			expected: &pb.Error{
+				Status:  404,
+				Code:    codes.NotFound.String(),
+				Message: "test",
+			},
+		},
+		{
+			name: "GrpcGateway Routing Error",
+			err:  runtime.ErrUnknownURI,
+			expected: &pb.Error{
+				Status:  404,
+				Code:    codes.NotFound.String(),
+				Message: http.StatusText(http.StatusNotFound),
+			},
+		},
+		{
+			name: "Invalid Fields",
+			err:  status.Error(codes.InvalidArgument, "test"),
+			statusDetails: []proto.Message{
+				&sdpb.BadRequest{
+					FieldViolations: []*sdpb.BadRequest_FieldViolation{
+						{Field: "first"},
+						{Field: "second"},
+					},
+				},
+			},
+			expected: &pb.Error{
+				Status:  400,
+				Code:    codes.InvalidArgument.String(),
+				Message: "test",
+				Details: &pb.ErrorDetails{
+					RequestFields: []string{"first", "second"},
+				},
+			},
+		},
+	}
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			assert := assert.New(t)
+
+			if tc.statusDetails != nil {
+				s, ok := status.FromError(tc.err)
+				assert.True(ok)
+				s, err := s.WithDetails(tc.statusDetails...)
+				assert.NoError(err)
+				tc.err = s.Err()
+			}
+
+			w := httptest.NewRecorder()
+			tested(ctx, mux, outMarsh, w, req, tc.err)
+			resp := w.Result()
+			assert.EqualValues(tc.expected.Status, resp.StatusCode)
+
+			got, err := ioutil.ReadAll(resp.Body)
+			assert.NoError(err)
+			want, err := outMarsh.Marshal(tc.expected)
+			t.Logf("Got marshaled error: %q", want)
+			assert.NoError(err)
+			assert.JSONEq(string(want), string(got))
+		})
+	}
+}

internal/servers/controller/handlers/projects/project_service.go

@@ -11,6 +11,7 @@ import (
 	pb "github.com/hashicorp/watchtower/internal/gen/controller/api/resources/scopes"
 	pbs "github.com/hashicorp/watchtower/internal/gen/controller/api/services"
 	"github.com/hashicorp/watchtower/internal/iam"
+	"github.com/hashicorp/watchtower/internal/servers/controller/handlers"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
 )
@@ -97,7 +98,7 @@ func (s Service) getFromRepo(ctx context.Context, id string) (*pb.Project, error
 		return nil, err
 	}
 	if p == nil {
-		return nil, status.Errorf(codes.NotFound, "Could not find Project with id %q", id)
+		return nil, handlers.NotFoundErrorf("Project %q doesn't exist.", id)
 	}
 	return toProto(p), nil
 }
@@ -112,20 +113,20 @@ func (s Service) createInRepo(ctx context.Context, orgID string, item *pb.Projec
 	}
 	p, err := iam.NewProject(orgID, opts...)
 	if err != nil {
-		return nil, err
+		return nil, status.Errorf(codes.Internal, "Unable to build project for creation: %v.", err)
 	}
 	out, err := s.repo.CreateScope(ctx, p)
 	if err != nil {
-		return nil, err
+		return nil, status.Errorf(codes.Internal, "Unable to create project: %v.", err)
 	}
 	if out == nil {
-		return nil, status.Error(codes.Internal, "Unable to create scope but no error returned from repository.")
+		return nil, status.Error(codes.Internal, "Unable to create project but no error returned from repository.")
 	}
 	return toProto(out), nil
 }
 
-func (s Service) updateInRepo(ctx context.Context, orgID, projID string, mask []string, item *pb.Project) (*pb.Project, error) {
-	opts := []iam.Option{iam.WithPublicId(projID)}
+func (s Service) updateInRepo(ctx context.Context, orgID, projId string, mask []string, item *pb.Project) (*pb.Project, error) {
+	opts := []iam.Option{iam.WithPublicId(projId)}
 	if desc := item.GetDescription(); desc != nil {
 		opts = append(opts, iam.WithDescription(desc.GetValue()))
 	}
@@ -134,21 +135,21 @@ func (s Service) updateInRepo(ctx context.Context, orgID, projID string, mask []
 	}
 	p, err := iam.NewProject(orgID, opts...)
 	if err != nil {
-		return nil, err
+		return nil, status.Errorf(codes.Internal, "Unable to build project for update: %v.", err)
 	}
 	dbMask, err := toDbUpdateMask(mask)
 	if err != nil {
 		return nil, err
 	}
 	if len(dbMask) == 0 {
-		return nil, status.Errorf(codes.InvalidArgument, "No valid fields included in the update mask.")
+		return nil, handlers.InvalidArgumentErrorf("No valid fields included in the update mask.", []string{"update_mask"})
 	}
-	out, _, err := s.repo.UpdateScope(ctx, p, dbMask)
+	out, rowsUpdated, err := s.repo.UpdateScope(ctx, p, dbMask)
 	if err != nil {
-		return nil, err
+		return nil, status.Errorf(codes.Internal, "Unable to update project: %v.", err)
 	}
-	if out == nil {
-		return nil, status.Error(codes.NotFound, "Project doesn't exist.")
+	if rowsUpdated == 0 {
+		return nil, handlers.NotFoundErrorf("Project %q doesn't exist.", projId)
 	}
 	return toProto(out), nil
 }
@@ -156,7 +157,7 @@ func (s Service) updateInRepo(ctx context.Context, orgID, projID string, mask []
 func (s Service) deleteFromRepo(ctx context.Context, projId string) (bool, error) {
 	rows, err := s.repo.DeleteScope(ctx, projId)
 	if err != nil {
-		return false, status.Errorf(codes.Internal, "Unable to delete project: %v", err)
+		return false, status.Errorf(codes.Internal, "Unable to delete project: %v.", err)
 	}
 	return rows > 0, nil
 }
@@ -175,7 +176,7 @@ func toDbUpdateMask(paths []string) ([]string, error) {
 		}
 	}
 	if len(invalid) > 0 {
-		return nil, status.Errorf(codes.InvalidArgument, "Invalid fields passed in update_update mask: %v", invalid)
+		return nil, handlers.InvalidArgumentErrorf(fmt.Sprintf("Invalid fields passed in update_update mask: %v.", invalid), []string{"update_mask"})
 	}
 	return dbPaths, nil
 }
@@ -203,11 +204,15 @@ func validateGetProjectRequest(req *pbs.GetProjectRequest) error {
 	if err := validateAncestors(req); err != nil {
 		return err
 	}
-	if err := validateID(req.GetOrgId(), "o_"); err != nil {
-		return err
+	badFormat := []string{}
+	if !validID(req.GetId(), "p_") {
+		badFormat = append(badFormat, "id")
 	}
-	if err := validateID(req.GetId(), "p_"); err != nil {
-		return err
+	if !validID(req.GetOrgId(), "o_") {
+		badFormat = append(badFormat, "id")
+	}
+	if len(badFormat) > 0 {
+		return handlers.InvalidArgumentErrorf("Improperly formatted identifier.", badFormat)
 	}
 	return nil
 }
@@ -216,18 +221,25 @@ func validateCreateProjectRequest(req *pbs.CreateProjectRequest) error {
 	if err := validateAncestors(req); err != nil {
 		return err
 	}
-	if err := validateID(req.GetOrgId(), "o_"); err != nil {
-		return err
+	if !validID(req.GetOrgId(), "o_") {
+		handlers.InvalidArgumentErrorf("Improperly formatted identifier.", []string{"org_id"})
 	}
 	item := req.GetItem()
 	if item == nil {
-		return status.Errorf(codes.InvalidArgument, "A project's fields must be set to something .")
+		return handlers.InvalidArgumentErrorf("A project's fields must be set to something.", []string{"item"})
 	}
+	immutableFieldsSet := []string{}
 	if item.GetId() != "" {
-		return status.Errorf(codes.InvalidArgument, "Cannot set ID when creating a new project.")
+		immutableFieldsSet = append(immutableFieldsSet, "id")
 	}
-	if item.GetCreatedTime() != nil || item.GetUpdatedTime() != nil {
-		return status.Errorf(codes.InvalidArgument, "Cannot set Created or Updated time when creating a new project.")
+	if item.GetCreatedTime() != nil {
+		immutableFieldsSet = append(immutableFieldsSet, "created_time")
+	}
+	if item.GetUpdatedTime() != nil {
+		immutableFieldsSet = append(immutableFieldsSet, "updated_time")
+	}
+	if len(immutableFieldsSet) > 0 {
+		return handlers.InvalidArgumentErrorf("Cannot specify read only fields at creation time.", immutableFieldsSet)
 	}
 	return nil
 }
@@ -236,30 +248,42 @@ func validateUpdateProjectRequest(req *pbs.UpdateProjectRequest) error {
 	if err := validateAncestors(req); err != nil {
 		return err
 	}
-	if err := validateID(req.GetId(), "p_"); err != nil {
-		return err
+	badFormat := []string{}
+	if !validID(req.GetId(), "p_") {
+		badFormat = append(badFormat, "id")
 	}
-	if err := validateID(req.GetOrgId(), "o_"); err != nil {
-		return err
+	if !validID(req.GetOrgId(), "o_") {
+		badFormat = append(badFormat, "id")
 	}
+	if len(badFormat) > 0 {
+		handlers.InvalidArgumentErrorf("Improperly formatted identifier.", badFormat)
+	}
+
 	if req.GetUpdateMask() == nil {
-		return status.Errorf(codes.InvalidArgument, "UpdateMask not provided but is required to update a project.")
+		return handlers.InvalidArgumentErrorf("UpdateMask not provided but is required to update a project.", []string{"update_mask"})
 	}
+
 	item := req.GetItem()
 	if item == nil {
 		// It is legitimate for no item to be specified in an update request as it indicates all fields provided in
 		// the mask will be marked as unset.
 		return nil
 	}
-
-	if err := validateID(item.GetId(), "p_"); item.GetId() != "" && err != nil {
-		return err
-	}
 	if item.GetId() != "" && item.GetId() != req.GetId() {
-		return status.Errorf(codes.InvalidArgument, "Id in provided item and url must match. Item Id was %q, url id was %q", item.GetId(), req.GetId())
+		return handlers.InvalidArgumentErrorf("Id in provided item and url do not match.", []string{"id"})
 	}
-	if item.GetCreatedTime() != nil || item.GetUpdatedTime() != nil {
-		return status.Errorf(codes.InvalidArgument, "Cannot set Created or Updated time when updating a project.")
+	immutableFieldsSet := []string{}
+	if item.GetId() != "" {
+		immutableFieldsSet = append(immutableFieldsSet, "id")
+	}
+	if item.GetCreatedTime() != nil {
+		immutableFieldsSet = append(immutableFieldsSet, "created_time")
+	}
+	if item.GetUpdatedTime() != nil {
+		immutableFieldsSet = append(immutableFieldsSet, "updated_time")
+	}
+	if len(immutableFieldsSet) > 0 {
+		return handlers.InvalidArgumentErrorf("Cannot specify read only fields at update time.", immutableFieldsSet)
 	}
 
 	return nil
@@ -269,24 +293,28 @@ func validateDeleteProjectRequest(req *pbs.DeleteProjectRequest) error {
 	if err := validateAncestors(req); err != nil {
 		return err
 	}
-	if err := validateID(req.GetId(), "p_"); err != nil {
-		return err
+	badFields := []string{}
+	if !validID(req.GetId(), "p_") {
+		badFields = append(badFields, "id")
 	}
-	if err := validateID(req.GetOrgId(), "o_"); err != nil {
-		return err
+	if !validID(req.GetOrgId(), "o_") {
+		badFields = append(badFields, "org_id")
+	}
+	if len(badFields) > 0 {
+		return handlers.InvalidArgumentErrorf("Improperly formatted id.", badFields)
 	}
 	return nil
 }
 
-func validateID(id, prefix string) error {
+func validID(id, prefix string) bool {
 	if !strings.HasPrefix(id, prefix) {
-		return status.Errorf(codes.InvalidArgument, "ID start with a %q prefix, provided %q", prefix, id)
+		return false
 	}
 	id = strings.TrimPrefix(id, prefix)
 	if reInvalidID.Match([]byte(id)) {
-		return status.Errorf(codes.InvalidArgument, "Improperly formatted ID: %q", id)
+		return false
 	}
-	return nil
+	return true
 }
 
 type ancestorProvider interface {
@@ -296,7 +324,7 @@ type ancestorProvider interface {
 // validateAncestors verifies that the ancestors of this call are properly set and provided.
 func validateAncestors(r ancestorProvider) error {
 	if r.GetOrgId() == "" {
-		return status.Errorf(codes.InvalidArgument, "org_id must be provided.")
+		return handlers.InvalidArgumentErrorf("Missing organization id.", []string{"org_id"})
 	}
 	return nil
 }

internal/servers/controller/handlers/projects/service_test.go

@@ -470,6 +470,8 @@ func TestUpdate(t *testing.T) {
 			},
 			errCode: codes.OK,
 		},
+		// TODO: Updating a non existant project should result in a NotFound exception but currently results in
+		// the repo returning an internal error.
 		{
 			name: "Update a Non Existing Project",
 			req: &pbs.UpdateProjectRequest{
@@ -482,7 +484,7 @@ func TestUpdate(t *testing.T) {
 					Description: &wrappers.StringValue{Value: "desc"},
 				},
 			},
-			errCode: codes.Unknown,
+			errCode: codes.Internal,
 		},
 		{
 			name: "Cant change Id",