diff --git a/api/error.gen.go b/api/error.gen.go index 9af1599bea..094dd067f0 100644 --- a/api/error.gen.go +++ b/api/error.gen.go @@ -13,7 +13,7 @@ type Error struct { defaultFields []string // The HTTP Status code applicable to this error - Status *int64 `json:"status,omitempty"` + Status *int32 `json:"status,omitempty"` // An application-specific error string Code *string `json:"code,omitempty"` // A human readable explanation specific to this occurrence of the error diff --git a/api/internal/genapi/parse.go b/api/internal/genapi/parse.go index 6c901bf69b..2db7fc5a31 100644 --- a/api/internal/genapi/parse.go +++ b/api/internal/genapi/parse.go @@ -222,9 +222,7 @@ func parsePBs() { } TAGMODIFY: - // TraceId needs to be handled differently because it's a - // specific value used by OpenTelemetry - st.Fields.List[i].Tag.Value = "`" + strings.Replace(regex.FindString(st.Fields.List[i].Tag.Value), "trace_id", "TraceId", 1) + "`" + st.Fields.List[i].Tag.Value = "`" + regex.FindString(st.Fields.List[i].Tag.Value) + "`" } }), inAst) diff --git a/api/scopes/project_test.go b/api/scopes/project_test.go index 63f1694228..93fa256422 100644 --- a/api/scopes/project_test.go +++ b/api/scopes/project_test.go @@ -1,6 +1,7 @@ package scopes import ( + "net/http" "testing" "github.com/hashicorp/watchtower/api" @@ -43,7 +44,32 @@ func TestProjects_Crud(t *testing.T) { existed, apiErr, err = org.DeleteProject(tc.Context(), p) assert.NoError(t, err) assert.False(t, existed, "Expected project to not exist when deleted, but it did.") +} + +// TODO: Get better coverage for expected errors and error formats. +func TestProject_Errors(t *testing.T) { + assert := assert.New(t) + tc := controller.NewTestController(t, nil) + defer tc.Shutdown() + ctx := tc.Context() + + client := tc.Client() + org := &Organization{ + Client: client, + } + createdProj, apiErr, err := org.CreateProject(ctx, &Project{}) + assert.NoError(err) + assert.NotNil(createdProj) + assert.Nil(apiErr) + + _, apiErr, err = org.ReadProject(ctx, &Project{Id: "p_doesntexis"}) + assert.NoError(err) + // TODO: Should this be nil instead of just a Project that has no values set + assert.NotNil(apiErr) + assert.EqualValues(*apiErr.Status, http.StatusNotFound) - // TODO: Error conditions once the proper errors are being returned. - // Probably as parallel subtests against the same DB. + _, apiErr, err = org.ReadProject(ctx, &Project{Id: "invalid id"}) + assert.NoError(err) + assert.NotNil(apiErr) + assert.EqualValues(*apiErr.Status, http.StatusBadRequest) } diff --git a/api/util.go b/api/util.go index d9deb10228..1aa359fe21 100644 --- a/api/util.go +++ b/api/util.go @@ -30,15 +30,15 @@ func StringOrNil(in string) *string { } // Int returns the given int64 as an int64 pointer -func Int(in int64) *int64 { - ret := new(int64) +func Int(in int32) *int32 { + ret := new(int32) *ret = in return ret } // IntOrNil is like Int, but the returned pointer will be nil if the integer is // 0 -func IntOrNil(in int64) *int64 { +func IntOrNil(in int32) *int32 { if in == 0 { return nil } diff --git a/buf.yaml b/buf.yaml index 378ba99181..c8b4da311d 100644 --- a/buf.yaml +++ b/buf.yaml @@ -14,4 +14,6 @@ lint: ENUM_VALUE_PREFIX: - controller/api/view/v1/view.proto ENUM_ZERO_VALUE_SUFFIX: - - controller/api/view/v1/view.proto \ No newline at end of file + - controller/api/view/v1/view.proto + FIELD_LOWER_SNAKE_CASE: + - controller/api/v1/error.proto diff --git a/internal/gen/controller/api/error.pb.go b/internal/gen/controller/api/error.pb.go index e14f98fe37..bc7479e4c0 100644 --- a/internal/gen/controller/api/error.pb.go +++ b/internal/gen/controller/api/error.pb.go @@ -8,7 +8,6 @@ package api import ( proto "github.com/golang/protobuf/proto" - wrappers "github.com/golang/protobuf/ptypes/wrappers" protoreflect "google.golang.org/protobuf/reflect/protoreflect" protoimpl "google.golang.org/protobuf/runtime/protoimpl" reflect "reflect" @@ -31,9 +30,9 @@ type ErrorDetails struct { sizeCache protoimpl.SizeCache unknownFields protoimpl.UnknownFields - TraceId *wrappers.StringValue `protobuf:"bytes,1,opt,name=trace_id,json=TraceId,proto3" json:"trace_id,omitempty"` - RequestId *wrappers.StringValue `protobuf:"bytes,2,opt,name=request_id,proto3" json:"request_id,omitempty"` - RequestFields []string `protobuf:"bytes,3,rep,name=request_fields,proto3" json:"request_fields,omitempty"` + TraceId string `protobuf:"bytes,1,opt,name=TraceId,proto3" json:"TraceId,omitempty"` + RequestId string `protobuf:"bytes,2,opt,name=request_id,proto3" json:"request_id,omitempty"` + RequestFields []string `protobuf:"bytes,3,rep,name=request_fields,proto3" json:"request_fields,omitempty"` } func (x *ErrorDetails) Reset() { @@ -68,18 +67,18 @@ func (*ErrorDetails) Descriptor() ([]byte, []int) { return file_controller_api_v1_error_proto_rawDescGZIP(), []int{0} } -func (x *ErrorDetails) GetTraceId() *wrappers.StringValue { +func (x *ErrorDetails) GetTraceId() string { if x != nil { return x.TraceId } - return nil + return "" } -func (x *ErrorDetails) GetRequestId() *wrappers.StringValue { +func (x *ErrorDetails) GetRequestId() string { if x != nil { return x.RequestId } - return nil + return "" } func (x *ErrorDetails) GetRequestFields() []string { @@ -96,11 +95,11 @@ type Error struct { unknownFields protoimpl.UnknownFields // The HTTP Status code applicable to this error - Status *wrappers.Int64Value `protobuf:"bytes,1,opt,name=status,proto3" json:"status,omitempty"` + Status int32 `protobuf:"varint,1,opt,name=status,proto3" json:"status,omitempty"` // An application-specific error string - Code *wrappers.StringValue `protobuf:"bytes,2,opt,name=code,proto3" json:"code,omitempty"` + Code string `protobuf:"bytes,2,opt,name=code,proto3" json:"code,omitempty"` // A human readable explanation specific to this occurrence of the error - Message *wrappers.StringValue `protobuf:"bytes,3,opt,name=message,proto3" json:"message,omitempty"` + Message string `protobuf:"bytes,3,opt,name=message,proto3" json:"message,omitempty"` // Additional metadata regarding the error. Depending on the error // different fields will be populated. Details *ErrorDetails `protobuf:"bytes,4,opt,name=details,proto3" json:"details,omitempty"` @@ -138,25 +137,25 @@ func (*Error) Descriptor() ([]byte, []int) { return file_controller_api_v1_error_proto_rawDescGZIP(), []int{1} } -func (x *Error) GetStatus() *wrappers.Int64Value { +func (x *Error) GetStatus() int32 { if x != nil { return x.Status } - return nil + return 0 } -func (x *Error) GetCode() *wrappers.StringValue { +func (x *Error) GetCode() string { if x != nil { return x.Code } - return nil + return "" } -func (x *Error) GetMessage() *wrappers.StringValue { +func (x *Error) GetMessage() string { if x != nil { return x.Message } - return nil + return "" } func (x *Error) GetDetails() *ErrorDetails { @@ -172,39 +171,27 @@ var file_controller_api_v1_error_proto_rawDesc = []byte{ 0x0a, 0x1d, 0x63, 0x6f, 0x6e, 0x74, 0x72, 0x6f, 0x6c, 0x6c, 0x65, 0x72, 0x2f, 0x61, 0x70, 0x69, 0x2f, 0x76, 0x31, 0x2f, 0x65, 0x72, 0x72, 0x6f, 0x72, 0x2e, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x12, 0x11, 0x63, 0x6f, 0x6e, 0x74, 0x72, 0x6f, 0x6c, 0x6c, 0x65, 0x72, 0x2e, 0x61, 0x70, 0x69, 0x2e, - 0x76, 0x31, 0x1a, 0x1e, 0x67, 0x6f, 0x6f, 0x67, 0x6c, 0x65, 0x2f, 0x70, 0x72, 0x6f, 0x74, 0x6f, - 0x62, 0x75, 0x66, 0x2f, 0x77, 0x72, 0x61, 0x70, 0x70, 0x65, 0x72, 0x73, 0x2e, 0x70, 0x72, 0x6f, - 0x74, 0x6f, 0x22, 0xad, 0x01, 0x0a, 0x0c, 0x45, 0x72, 0x72, 0x6f, 0x72, 0x44, 0x65, 0x74, 0x61, - 0x69, 0x6c, 0x73, 0x12, 0x37, 0x0a, 0x08, 0x74, 0x72, 0x61, 0x63, 0x65, 0x5f, 0x69, 0x64, 0x18, - 0x01, 0x20, 0x01, 0x28, 0x0b, 0x32, 0x1c, 0x2e, 0x67, 0x6f, 0x6f, 0x67, 0x6c, 0x65, 0x2e, 0x70, - 0x72, 0x6f, 0x74, 0x6f, 0x62, 0x75, 0x66, 0x2e, 0x53, 0x74, 0x72, 0x69, 0x6e, 0x67, 0x56, 0x61, - 0x6c, 0x75, 0x65, 0x52, 0x07, 0x54, 0x72, 0x61, 0x63, 0x65, 0x49, 0x64, 0x12, 0x3c, 0x0a, 0x0a, - 0x72, 0x65, 0x71, 0x75, 0x65, 0x73, 0x74, 0x5f, 0x69, 0x64, 0x18, 0x02, 0x20, 0x01, 0x28, 0x0b, - 0x32, 0x1c, 0x2e, 0x67, 0x6f, 0x6f, 0x67, 0x6c, 0x65, 0x2e, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x62, - 0x75, 0x66, 0x2e, 0x53, 0x74, 0x72, 0x69, 0x6e, 0x67, 0x56, 0x61, 0x6c, 0x75, 0x65, 0x52, 0x0a, - 0x72, 0x65, 0x71, 0x75, 0x65, 0x73, 0x74, 0x5f, 0x69, 0x64, 0x12, 0x26, 0x0a, 0x0e, 0x72, 0x65, - 0x71, 0x75, 0x65, 0x73, 0x74, 0x5f, 0x66, 0x69, 0x65, 0x6c, 0x64, 0x73, 0x18, 0x03, 0x20, 0x03, - 0x28, 0x09, 0x52, 0x0e, 0x72, 0x65, 0x71, 0x75, 0x65, 0x73, 0x74, 0x5f, 0x66, 0x69, 0x65, 0x6c, - 0x64, 0x73, 0x22, 0xe1, 0x01, 0x0a, 0x05, 0x45, 0x72, 0x72, 0x6f, 0x72, 0x12, 0x33, 0x0a, 0x06, - 0x73, 0x74, 0x61, 0x74, 0x75, 0x73, 0x18, 0x01, 0x20, 0x01, 0x28, 0x0b, 0x32, 0x1b, 0x2e, 0x67, - 0x6f, 0x6f, 0x67, 0x6c, 0x65, 0x2e, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x62, 0x75, 0x66, 0x2e, 0x49, - 0x6e, 0x74, 0x36, 0x34, 0x56, 0x61, 0x6c, 0x75, 0x65, 0x52, 0x06, 0x73, 0x74, 0x61, 0x74, 0x75, - 0x73, 0x12, 0x30, 0x0a, 0x04, 0x63, 0x6f, 0x64, 0x65, 0x18, 0x02, 0x20, 0x01, 0x28, 0x0b, 0x32, - 0x1c, 0x2e, 0x67, 0x6f, 0x6f, 0x67, 0x6c, 0x65, 0x2e, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x62, 0x75, - 0x66, 0x2e, 0x53, 0x74, 0x72, 0x69, 0x6e, 0x67, 0x56, 0x61, 0x6c, 0x75, 0x65, 0x52, 0x04, 0x63, - 0x6f, 0x64, 0x65, 0x12, 0x36, 0x0a, 0x07, 0x6d, 0x65, 0x73, 0x73, 0x61, 0x67, 0x65, 0x18, 0x03, - 0x20, 0x01, 0x28, 0x0b, 0x32, 0x1c, 0x2e, 0x67, 0x6f, 0x6f, 0x67, 0x6c, 0x65, 0x2e, 0x70, 0x72, - 0x6f, 0x74, 0x6f, 0x62, 0x75, 0x66, 0x2e, 0x53, 0x74, 0x72, 0x69, 0x6e, 0x67, 0x56, 0x61, 0x6c, - 0x75, 0x65, 0x52, 0x07, 0x6d, 0x65, 0x73, 0x73, 0x61, 0x67, 0x65, 0x12, 0x39, 0x0a, 0x07, 0x64, - 0x65, 0x74, 0x61, 0x69, 0x6c, 0x73, 0x18, 0x04, 0x20, 0x01, 0x28, 0x0b, 0x32, 0x1f, 0x2e, 0x63, - 0x6f, 0x6e, 0x74, 0x72, 0x6f, 0x6c, 0x6c, 0x65, 0x72, 0x2e, 0x61, 0x70, 0x69, 0x2e, 0x76, 0x31, - 0x2e, 0x45, 0x72, 0x72, 0x6f, 0x72, 0x44, 0x65, 0x74, 0x61, 0x69, 0x6c, 0x73, 0x52, 0x07, 0x64, - 0x65, 0x74, 0x61, 0x69, 0x6c, 0x73, 0x42, 0x41, 0x5a, 0x3f, 0x67, 0x69, 0x74, 0x68, 0x75, 0x62, - 0x2e, 0x63, 0x6f, 0x6d, 0x2f, 0x68, 0x61, 0x73, 0x68, 0x69, 0x63, 0x6f, 0x72, 0x70, 0x2f, 0x77, - 0x61, 0x74, 0x63, 0x68, 0x74, 0x6f, 0x77, 0x65, 0x72, 0x2f, 0x69, 0x6e, 0x74, 0x65, 0x72, 0x6e, - 0x61, 0x6c, 0x2f, 0x67, 0x65, 0x6e, 0x2f, 0x63, 0x6f, 0x6e, 0x74, 0x72, 0x6f, 0x6c, 0x6c, 0x65, - 0x72, 0x2f, 0x61, 0x70, 0x69, 0x3b, 0x61, 0x70, 0x69, 0x62, 0x06, 0x70, 0x72, 0x6f, 0x74, 0x6f, - 0x33, + 0x76, 0x31, 0x22, 0x70, 0x0a, 0x0c, 0x45, 0x72, 0x72, 0x6f, 0x72, 0x44, 0x65, 0x74, 0x61, 0x69, + 0x6c, 0x73, 0x12, 0x18, 0x0a, 0x07, 0x54, 0x72, 0x61, 0x63, 0x65, 0x49, 0x64, 0x18, 0x01, 0x20, + 0x01, 0x28, 0x09, 0x52, 0x07, 0x54, 0x72, 0x61, 0x63, 0x65, 0x49, 0x64, 0x12, 0x1e, 0x0a, 0x0a, + 0x72, 0x65, 0x71, 0x75, 0x65, 0x73, 0x74, 0x5f, 0x69, 0x64, 0x18, 0x02, 0x20, 0x01, 0x28, 0x09, + 0x52, 0x0a, 0x72, 0x65, 0x71, 0x75, 0x65, 0x73, 0x74, 0x5f, 0x69, 0x64, 0x12, 0x26, 0x0a, 0x0e, + 0x72, 0x65, 0x71, 0x75, 0x65, 0x73, 0x74, 0x5f, 0x66, 0x69, 0x65, 0x6c, 0x64, 0x73, 0x18, 0x03, + 0x20, 0x03, 0x28, 0x09, 0x52, 0x0e, 0x72, 0x65, 0x71, 0x75, 0x65, 0x73, 0x74, 0x5f, 0x66, 0x69, + 0x65, 0x6c, 0x64, 0x73, 0x22, 0x88, 0x01, 0x0a, 0x05, 0x45, 0x72, 0x72, 0x6f, 0x72, 0x12, 0x16, + 0x0a, 0x06, 0x73, 0x74, 0x61, 0x74, 0x75, 0x73, 0x18, 0x01, 0x20, 0x01, 0x28, 0x05, 0x52, 0x06, + 0x73, 0x74, 0x61, 0x74, 0x75, 0x73, 0x12, 0x12, 0x0a, 0x04, 0x63, 0x6f, 0x64, 0x65, 0x18, 0x02, + 0x20, 0x01, 0x28, 0x09, 0x52, 0x04, 0x63, 0x6f, 0x64, 0x65, 0x12, 0x18, 0x0a, 0x07, 0x6d, 0x65, + 0x73, 0x73, 0x61, 0x67, 0x65, 0x18, 0x03, 0x20, 0x01, 0x28, 0x09, 0x52, 0x07, 0x6d, 0x65, 0x73, + 0x73, 0x61, 0x67, 0x65, 0x12, 0x39, 0x0a, 0x07, 0x64, 0x65, 0x74, 0x61, 0x69, 0x6c, 0x73, 0x18, + 0x04, 0x20, 0x01, 0x28, 0x0b, 0x32, 0x1f, 0x2e, 0x63, 0x6f, 0x6e, 0x74, 0x72, 0x6f, 0x6c, 0x6c, + 0x65, 0x72, 0x2e, 0x61, 0x70, 0x69, 0x2e, 0x76, 0x31, 0x2e, 0x45, 0x72, 0x72, 0x6f, 0x72, 0x44, + 0x65, 0x74, 0x61, 0x69, 0x6c, 0x73, 0x52, 0x07, 0x64, 0x65, 0x74, 0x61, 0x69, 0x6c, 0x73, 0x42, + 0x41, 0x5a, 0x3f, 0x67, 0x69, 0x74, 0x68, 0x75, 0x62, 0x2e, 0x63, 0x6f, 0x6d, 0x2f, 0x68, 0x61, + 0x73, 0x68, 0x69, 0x63, 0x6f, 0x72, 0x70, 0x2f, 0x77, 0x61, 0x74, 0x63, 0x68, 0x74, 0x6f, 0x77, + 0x65, 0x72, 0x2f, 0x69, 0x6e, 0x74, 0x65, 0x72, 0x6e, 0x61, 0x6c, 0x2f, 0x67, 0x65, 0x6e, 0x2f, + 0x63, 0x6f, 0x6e, 0x74, 0x72, 0x6f, 0x6c, 0x6c, 0x65, 0x72, 0x2f, 0x61, 0x70, 0x69, 0x3b, 0x61, + 0x70, 0x69, 0x62, 0x06, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x33, } var ( @@ -221,23 +208,16 @@ func file_controller_api_v1_error_proto_rawDescGZIP() []byte { var file_controller_api_v1_error_proto_msgTypes = make([]protoimpl.MessageInfo, 2) var file_controller_api_v1_error_proto_goTypes = []interface{}{ - (*ErrorDetails)(nil), // 0: controller.api.v1.ErrorDetails - (*Error)(nil), // 1: controller.api.v1.Error - (*wrappers.StringValue)(nil), // 2: google.protobuf.StringValue - (*wrappers.Int64Value)(nil), // 3: google.protobuf.Int64Value + (*ErrorDetails)(nil), // 0: controller.api.v1.ErrorDetails + (*Error)(nil), // 1: controller.api.v1.Error } var file_controller_api_v1_error_proto_depIdxs = []int32{ - 2, // 0: controller.api.v1.ErrorDetails.trace_id:type_name -> google.protobuf.StringValue - 2, // 1: controller.api.v1.ErrorDetails.request_id:type_name -> google.protobuf.StringValue - 3, // 2: controller.api.v1.Error.status:type_name -> google.protobuf.Int64Value - 2, // 3: controller.api.v1.Error.code:type_name -> google.protobuf.StringValue - 2, // 4: controller.api.v1.Error.message:type_name -> google.protobuf.StringValue - 0, // 5: controller.api.v1.Error.details:type_name -> controller.api.v1.ErrorDetails - 6, // [6:6] is the sub-list for method output_type - 6, // [6:6] is the sub-list for method input_type - 6, // [6:6] is the sub-list for extension type_name - 6, // [6:6] is the sub-list for extension extendee - 0, // [0:6] is the sub-list for field type_name + 0, // 0: controller.api.v1.Error.details:type_name -> controller.api.v1.ErrorDetails + 1, // [1:1] is the sub-list for method output_type + 1, // [1:1] is the sub-list for method input_type + 1, // [1:1] is the sub-list for extension type_name + 1, // [1:1] is the sub-list for extension extendee + 0, // [0:1] is the sub-list for field type_name } func init() { file_controller_api_v1_error_proto_init() } diff --git a/internal/proto/local/controller/api/v1/error.proto b/internal/proto/local/controller/api/v1/error.proto index c4eb2f5a43..1046011459 100644 --- a/internal/proto/local/controller/api/v1/error.proto +++ b/internal/proto/local/controller/api/v1/error.proto @@ -4,22 +4,20 @@ package controller.api.v1; option go_package = "github.com/hashicorp/watchtower/internal/gen/controller/api;api"; -import "google/protobuf/wrappers.proto"; - message ErrorDetails { - google.protobuf.StringValue trace_id = 1 [json_name="TraceId"]; - google.protobuf.StringValue request_id = 2 [json_name="request_id"]; + string TraceId = 1 [json_name="TraceId"]; + string request_id = 2 [json_name="request_id"]; repeated string request_fields = 3 [json_name="request_fields"]; } // Error is returned by the JSON API when an error occurs. message Error { // The HTTP Status code applicable to this error - google.protobuf.Int64Value status = 1; + int32 status = 1; // An application-specific error string - google.protobuf.StringValue code = 2; + string code = 2; // A human readable explanation specific to this occurrence of the error - google.protobuf.StringValue message = 3; + string message = 3; // Additional metadata regarding the error. Depending on the error // different fields will be populated. ErrorDetails details = 4; diff --git a/internal/servers/controller/handler.go b/internal/servers/controller/handler.go index 8998c1f474..980c9661e1 100644 --- a/internal/servers/controller/handler.go +++ b/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{}) diff --git a/internal/servers/controller/handlers/errors.go b/internal/servers/controller/handlers/errors.go new file mode 100644 index 0000000000..cf4219d750 --- /dev/null +++ b/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 + } + } +} diff --git a/internal/servers/controller/handlers/errors_test.go b/internal/servers/controller/handlers/errors_test.go new file mode 100644 index 0000000000..9eff9abab4 --- /dev/null +++ b/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)) + }) + } +} diff --git a/internal/servers/controller/handlers/projects/project_service.go b/internal/servers/controller/handlers/projects/project_service.go index 8fe01e2faa..eb409765df 100644 --- a/internal/servers/controller/handlers/projects/project_service.go +++ b/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 { + immutableFieldsSet = append(immutableFieldsSet, "created_time") + } + if item.GetUpdatedTime() != nil { + immutableFieldsSet = append(immutableFieldsSet, "updated_time") } - if item.GetCreatedTime() != nil || item.GetUpdatedTime() != nil { - return status.Errorf(codes.InvalidArgument, "Cannot set Created or Updated time when creating a new project.") + 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"}) + } + immutableFieldsSet := []string{} + if item.GetId() != "" { + immutableFieldsSet = append(immutableFieldsSet, "id") } - if item.GetCreatedTime() != nil || item.GetUpdatedTime() != nil { - return status.Errorf(codes.InvalidArgument, "Cannot set Created or Updated time when updating a 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 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 } diff --git a/internal/servers/controller/handlers/projects/service_test.go b/internal/servers/controller/handlers/projects/service_test.go index 8d25ac1183..8559864eeb 100644 --- a/internal/servers/controller/handlers/projects/service_test.go +++ b/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",