diff --git a/CHANGELOG.md b/CHANGELOG.md index dbb67fa49..fbafca0fd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,8 +6,8 @@ Canonical reference for changes, improvements, and bugfixes for Boundary. ### New and Improved -* controller: Improved error handling in kms repo - ([PR](https://github.com/hashicorp/boundary/pull/848)) +* controller: Improved error handling in kms, oplog and perms repos + ([PR1](https://github.com/hashicorp/boundary/pull/848), [PR2](https://github.com/hashicorp/boundary/pull/854)) ### Bug Fixes diff --git a/internal/db/read_writer.go b/internal/db/read_writer.go index c268626cb..6bb5eb4ca 100644 --- a/internal/db/read_writer.go +++ b/internal/db/read_writer.go @@ -905,7 +905,7 @@ func (w *Db) DoTx(ctx context.Context, retries uint, backOff Backoff, Handler Tx if err := newTx.Rollback().Error; err != nil { return info, errors.Wrap(err, op) } - if errors.Is(err, oplog.ErrTicketAlreadyRedeemed) { + if errors.Match(errors.T(errors.TicketAlreadyRedeemed), err) { d := backOff.Duration(attempts) info.Retries++ info.Backoff = info.Backoff + d diff --git a/internal/db/read_writer_test.go b/internal/db/read_writer_test.go index 863a9cd33..8e685107a 100644 --- a/internal/db/read_writer_test.go +++ b/internal/db/read_writer_test.go @@ -1044,7 +1044,7 @@ func TestDb_DoTx(t *testing.T) { func(Reader, Writer) error { attempts += 1 if attempts < 9 { - return oplog.ErrTicketAlreadyRedeemed + return errors.E(errors.WithCode(errors.TicketAlreadyRedeemed)) } return nil }) @@ -1060,7 +1060,7 @@ func TestDb_DoTx(t *testing.T) { func(Reader, Writer) error { attempts += 1 if attempts < 2 { - return oplog.ErrTicketAlreadyRedeemed + return errors.E(errors.WithCode(errors.TicketAlreadyRedeemed)) } return nil }) @@ -1076,7 +1076,7 @@ func TestDb_DoTx(t *testing.T) { func(Reader, Writer) error { attempts += 1 if attempts < 3 { - return oplog.ErrTicketAlreadyRedeemed + return errors.E(errors.WithCode(errors.TicketAlreadyRedeemed)) } return nil }) @@ -1092,7 +1092,7 @@ func TestDb_DoTx(t *testing.T) { func(Reader, Writer) error { attempts += 1 if attempts < 4 { - return oplog.ErrTicketAlreadyRedeemed + return errors.E(errors.WithCode(errors.TicketAlreadyRedeemed)) } return nil }) @@ -1124,13 +1124,16 @@ func TestDb_DoTx(t *testing.T) { got, err := w.DoTx(context.Background(), 1, ExpBackoff{}, func(Reader, Writer) error { return stderrors.New("not a retry error") }) require.Error(err) assert.Equal(RetryInfo{}, got) - assert.NotEqual(oplog.ErrTicketAlreadyRedeemed, err) + assert.False(errors.Match(errors.T(errors.TicketAlreadyRedeemed), err)) }) t.Run("too-many-retries", func(t *testing.T) { assert, require := assert.New(t), require.New(t) w := &Db{underlying: db} attempts := 0 - got, err := w.DoTx(context.Background(), 2, ExpBackoff{}, func(Reader, Writer) error { attempts += 1; return oplog.ErrTicketAlreadyRedeemed }) + got, err := w.DoTx(context.Background(), 2, ExpBackoff{}, func(Reader, Writer) error { + attempts += 1 + return errors.E(errors.WithCode(errors.TicketAlreadyRedeemed)) + }) require.Error(err) assert.Equal(3, got.Retries) assert.Equal("db.DoTx: Too many retries: 3 of 3: db transaction issue: error #1103", err.Error()) diff --git a/internal/errors/code.go b/internal/errors/code.go index f482af9ef..2dfd23f72 100644 --- a/internal/errors/code.go +++ b/internal/errors/code.go @@ -21,12 +21,15 @@ const ( Unknown Code = 0 // Unknown will be equal to a zero value for Codes // General function errors are reserved Codes 100-999 - InvalidParameter Code = 100 // InvalidParameter represents an invalid parameter for an operation. - InvalidAddress Code = 101 // InvalidAddress represents an invalid host address for an operation - InvalidPublicId Code = 102 // InvalidPublicId represents an invalid public Id for an operation - InvalidFieldMask Code = 103 // InvalidFieldMask represents an invalid field mast for an operation - EmptyFieldMask Code = 104 // EmptyFieldMask represents an empty field mask for an operation - KeyNotFound Code = 105 // KeyNotFound represents that a key/version was not found in the KMS + InvalidParameter Code = 100 // InvalidParameter represents an invalid parameter for an operation. + InvalidAddress Code = 101 // InvalidAddress represents an invalid host address for an operation + InvalidPublicId Code = 102 // InvalidPublicId represents an invalid public Id for an operation + InvalidFieldMask Code = 103 // InvalidFieldMask represents an invalid field mast for an operation + EmptyFieldMask Code = 104 // EmptyFieldMask represents an empty field mask for an operation + KeyNotFound Code = 105 // KeyNotFound represents that a key/version was not found in the KMS + TicketAlreadyRedeemed Code = 106 // TicketAlreadyRedeemed represents that the ticket version has already been redeemed + TicketNotFound Code = 107 // TicketNotFound represents that the ticket was not found + Io Code = 108 // Io represents that an io error occurred in an underlying call (i.e binary.Write) // PasswordTooShort results from attempting to set a password which is to short. PasswordTooShort Code = 200 @@ -45,6 +48,8 @@ const ( Encrypt Code = 300 // Encrypt represents an error occurred during the underlying encryption process Decrypt Code = 301 // Decrypt represents an error occurred during the underlying decryption process + Encode Code = 302 // Encode represents an error occurred during the underlying encoding/marshaling process + Decode Code = 303 // Decode represents an error occurred during the underlying decoding/unmarshaling process // DB errors are reserved Codes from 1000-1999 CheckConstraint Code = 1000 // CheckConstraint represents a check constraint error diff --git a/internal/errors/code_test.go b/internal/errors/code_test.go index 9545b3cef..d2bf3144c 100644 --- a/internal/errors/code_test.go +++ b/internal/errors/code_test.go @@ -57,6 +57,21 @@ func TestCode_Both_String_Info(t *testing.T) { c: KeyNotFound, want: KeyNotFound, }, + { + name: "TicketAlreadyRedeemed", + c: TicketAlreadyRedeemed, + want: TicketAlreadyRedeemed, + }, + { + name: "TicketNotFound", + c: TicketNotFound, + want: TicketNotFound, + }, + { + name: "Io", + c: Io, + want: Io, + }, { name: "PasswordTooShort", c: PasswordTooShort, @@ -87,6 +102,16 @@ func TestCode_Both_String_Info(t *testing.T) { c: Decrypt, want: Decrypt, }, + { + name: "Encode", + c: Encode, + want: Encode, + }, + { + name: "Decode", + c: Decode, + want: Decode, + }, { name: "CheckConstraint", c: CheckConstraint, diff --git a/internal/errors/info.go b/internal/errors/info.go index 751604e74..91555d6df 100644 --- a/internal/errors/info.go +++ b/internal/errors/info.go @@ -40,6 +40,18 @@ var errorCodeInfo = map[Code]Info{ Message: "key/version not found", Kind: Integrity, }, + TicketAlreadyRedeemed: { + Message: "ticket already redeemed", + Kind: Integrity, + }, + TicketNotFound: { + Message: "ticket not found", + Kind: Integrity, + }, + Io: { + Message: "error during io operation", + Kind: Integrity, + }, PasswordTooShort: { Message: "too short", Kind: Password, @@ -64,6 +76,14 @@ var errorCodeInfo = map[Code]Info{ Message: "error occurred during decrypt", Kind: Encryption, }, + Encode: { + Message: "error occurred during encode", + Kind: Encoding, + }, + Decode: { + Message: "error occurred during decode", + Kind: Encoding, + }, CheckConstraint: { Message: "constraint check failed", Kind: Integrity, diff --git a/internal/errors/kind.go b/internal/errors/kind.go index 7fb1742f7..092518ab4 100644 --- a/internal/errors/kind.go +++ b/internal/errors/kind.go @@ -11,6 +11,7 @@ const ( Password Transaction Encryption + Encoding ) func (e Kind) String() string { @@ -22,5 +23,6 @@ func (e Kind) String() string { Password: "password violation", Transaction: "db transaction issue", Encryption: "encryption issue", + Encoding: "encoding issue", }[e] } diff --git a/internal/oplog/oplog.go b/internal/oplog/oplog.go index 4d54b97a4..60bd6d838 100644 --- a/internal/oplog/oplog.go +++ b/internal/oplog/oplog.go @@ -3,14 +3,13 @@ package oplog import ( "bytes" "context" - "errors" "fmt" "io" - wrapping "github.com/hashicorp/go-kms-wrapping" - structwrapping "github.com/hashicorp/go-kms-wrapping/structwrapping" - + "github.com/hashicorp/boundary/internal/errors" "github.com/hashicorp/boundary/internal/oplog/store" + wrapping "github.com/hashicorp/go-kms-wrapping" + "github.com/hashicorp/go-kms-wrapping/structwrapping" _ "github.com/lib/pq" "google.golang.org/protobuf/proto" ) @@ -39,6 +38,7 @@ type Metadata map[string][]string // NewEntry creates a new Entry func NewEntry(aggregateName string, metadata Metadata, cipherer wrapping.Wrapper, ticketer Ticketer) (*Entry, error) { + const op = "oplog.NewEntry" entry := Entry{ Entry: &store.Entry{ AggregateName: aggregateName, @@ -62,37 +62,39 @@ func NewEntry(aggregateName string, metadata Metadata, cipherer wrapping.Wrapper entry.Metadata = storeMD } if err := entry.validate(); err != nil { - return nil, fmt.Errorf("error creating entry: %w", err) + return nil, errors.Wrap(err, op) } return &entry, nil } func (e *Entry) validate() error { + const op = "oplog.(Entry).validate" if e.Cipherer == nil { - return errors.New("entry Cipherer is nil") + return errors.New(errors.InvalidParameter, op, "nil cipherer") } if e.Ticketer == nil { - return errors.New("entry Ticketer is nil") + return errors.New(errors.InvalidParameter, op, "nil ticketer") } if e.Entry == nil { - return errors.New("store.Entry is nil") + return errors.New(errors.InvalidParameter, op, "nil entry") } if e.Entry.Version == "" { - return errors.New("entry version is not set") + return errors.New(errors.InvalidParameter, op, "missing entry version") } if e.Entry.AggregateName == "" { - return errors.New("entry aggregate name is not set") + return errors.New(errors.InvalidParameter, op, "missing entry aggregate name") } return nil } // UnmarshalData the data attribute from []byte (treated as a FIFO QueueBuffer) to a []proto.Message func (e *Entry) UnmarshalData(types *TypeCatalog) ([]Message, error) { + const op = "oplog.(Entry).UnmarshalData" if types == nil { - return nil, errors.New("TypeCatalog is nil") + return nil, errors.New(errors.InvalidParameter, op, "nil type catalog") } if len(e.Data) == 0 { - return nil, errors.New("no Data to unmarshal") + return nil, errors.New(errors.InvalidParameter, op, "missing data") } msgs := []Message{} queue := Queue{ @@ -105,11 +107,11 @@ func (e *Entry) UnmarshalData(types *TypeCatalog) ([]Message, error) { break } if err != nil { - return nil, fmt.Errorf("error removing item from queue: %w", err) + return nil, errors.Wrap(err, op, errors.WithMsg("error removing item from queue")) } name, err := types.GetTypeName(m) if err != nil { - return nil, fmt.Errorf("error getting TypeName: %w", err) + return nil, errors.Wrap(err, op) } msgs = append(msgs, Message{Message: m, TypeName: name, OpType: typ, FieldMaskPaths: fieldMaskPaths, SetToNullPaths: nullPaths}) } @@ -119,86 +121,103 @@ func (e *Entry) UnmarshalData(types *TypeCatalog) ([]Message, error) { // WriteEntryWith the []proto.Message marshaled into the entry data as a FIFO QueueBuffer // if Cipherer != nil then the data is authentication encrypted func (e *Entry) WriteEntryWith(ctx context.Context, tx Writer, ticket *store.Ticket, msgs ...*Message) error { + const op = "oplog.(Entry).WriteEntryWith" if tx == nil { - return errors.New("bad writer") + return errors.New(errors.InvalidParameter, op, "nil writer") } if err := e.validate(); err != nil { - return fmt.Errorf("error vetting entry for writing: %w", err) + return errors.Wrap(err, op) } - if ticket == nil || ticket.Version == 0 { - return errors.New("bad ticket") + if ticket == nil { + return errors.New(errors.InvalidParameter, op, "nil ticket") + } + if ticket.Version == 0 { + return errors.New(errors.InvalidParameter, op, "missing ticket version") } queue := Queue{} for _, m := range msgs { if m == nil { - return errors.New("bad message") + return errors.New(errors.InvalidParameter, op, "nil message") } if err := queue.Add(m.Message, m.TypeName, m.OpType, WithFieldMaskPaths(m.FieldMaskPaths), WithSetToNullPaths(m.SetToNullPaths)); err != nil { - return fmt.Errorf("error adding message to queue: %w", err) + return errors.Wrap(err, op, errors.WithMsg("error adding message to queue")) } } - e.Data = append(e.Data, []byte(queue.Bytes())...) + e.Data = append(e.Data, queue.Bytes()...) if e.Cipherer != nil { if err := e.EncryptData(ctx); err != nil { - return fmt.Errorf("error encrypting entry: %w", err) + return errors.Wrap(err, op) } } if err := tx.Create(e); err != nil { - return fmt.Errorf("error writing data to storage: %w", err) + return errors.Wrap(err, op, errors.WithMsg("error writing data to storage")) } - return e.Ticketer.Redeem(ticket) + if err := e.Ticketer.Redeem(ticket); err != nil { + return errors.Wrap(err, op) + } + return nil } // Write the entry as is with whatever it has for e.Data marshaled into a FIFO QueueBuffer // Cipherer != nil then the data is authentication encrypted func (e *Entry) Write(ctx context.Context, tx Writer, ticket *store.Ticket) error { + const op = "oplog.(Entry).Write" if err := e.validate(); err != nil { - return fmt.Errorf("error vetting entry for writing: %w", err) + return errors.Wrap(err, op) } - if ticket == nil || ticket.Version == 0 { - return errors.New("bad ticket") + if ticket == nil { + return errors.New(errors.InvalidParameter, op, "nil ticket") + } + if ticket.Version == 0 { + return errors.New(errors.InvalidParameter, op, "missing ticket version") } if e.Cipherer != nil { if err := e.EncryptData(ctx); err != nil { - return fmt.Errorf("error encrypting entry: %w", err) + return errors.Wrap(err, op) } } if err := tx.Create(e); err != nil { - return fmt.Errorf("error writing data to storage: %w", err) + return errors.Wrap(err, op, errors.WithMsg("error writing data to storage")) } - return e.Ticketer.Redeem(ticket) + if err := e.Ticketer.Redeem(ticket); err != nil { + return errors.Wrap(err, op) + } + return nil } // EncryptData the entry's data using its Cipherer (wrapping.Wrapper) func (e *Entry) EncryptData(ctx context.Context) error { + const op = "oplog.(Entry).EncryptData" // structwrapping doesn't support embedding, so we'll pass in the store.Entry directly if err := structwrapping.WrapStruct(ctx, e.Cipherer, e.Entry, nil); err != nil { - return fmt.Errorf("error encrypting entry: %w", err) + return errors.Wrap(err, op, errors.WithCode(errors.Encrypt)) } return nil } // DecryptData will decrypt the entry's data using its Cipherer (wrapping.Wrapper) func (e *Entry) DecryptData(ctx context.Context) error { + const op = "oplog.(Entry).DecryptData" // structwrapping doesn't support embedding, so we'll pass in the store.Entry directly if err := structwrapping.UnwrapStruct(ctx, e.Cipherer, e.Entry, nil); err != nil { - return fmt.Errorf("error decrypting entry: %w", err) + return errors.Wrap(err, op, errors.WithCode(errors.Decrypt)) } return nil } // Replay provides the ability to replay an entry. you must initialize any new tables ending with the tableSuffix before // calling Replay, otherwise you'll get "a table doesn't exist" error. -func (e *Entry) Replay(ctx context.Context, tx Writer, types *TypeCatalog, tableSuffix string) error { +func (e *Entry) Replay(_ context.Context, tx Writer, types *TypeCatalog, tableSuffix string) error { + const op = "oplog.(Entry).Replay" msgs, err := e.UnmarshalData(types) if err != nil { - return fmt.Errorf("error on UnmarshalData: %w", err) + return errors.Wrap(err, op) } for _, m := range msgs { em, ok := m.Message.(ReplayableMessage) if !ok { - return fmt.Errorf("%T is not a ReplayableMessage", m.Message) + return errors.New(errors.InvalidParameter, op, fmt.Sprintf("%T is not a replayable message", m.Message)) } origTableName := em.TableName() defer em.SetTableName(origTableName) @@ -217,7 +236,7 @@ func (e *Entry) Replay(ctx context.Context, tx Writer, types *TypeCatalog, table replayTable := origTableName + tableSuffix if !tx.hasTable(replayTable) { if err := tx.createTableLike(origTableName, replayTable); err != nil { - return fmt.Errorf("replay: %w", err) + return errors.Wrap(err, op) } } @@ -225,18 +244,18 @@ func (e *Entry) Replay(ctx context.Context, tx Writer, types *TypeCatalog, table switch m.OpType { case OpType_OP_TYPE_CREATE: if err := tx.Create(m.Message); err != nil { - return fmt.Errorf("replay error: %w", err) + return errors.Wrap(err, op) } case OpType_OP_TYPE_UPDATE: if err := tx.Update(m.Message, m.FieldMaskPaths, m.SetToNullPaths); err != nil { - return fmt.Errorf("replay error: %w", err) + return errors.Wrap(err, op) } case OpType_OP_TYPE_DELETE: if err := tx.Delete(m.Message); err != nil { - return fmt.Errorf("replay error: %w", err) + return errors.Wrap(err, op) } default: - return fmt.Errorf("replay error: unhandled operation %T", m.OpType) + return errors.New(errors.InvalidParameter, op, fmt.Sprintf("invalid operation %T", m.OpType)) } } return nil diff --git a/internal/oplog/oplog_test.go b/internal/oplog/oplog_test.go index ac6bb6207..afd527042 100644 --- a/internal/oplog/oplog_test.go +++ b/internal/oplog/oplog_test.go @@ -182,7 +182,7 @@ func Test_NewEntry(t *testing.T) { ticketer, ) require.Error(err) - assert.Equal("error creating entry: entry aggregate name is not set", err.Error()) + assert.Equal("oplog.NewEntry: oplog.(Entry).validate: missing entry aggregate name: parameter violation: error #100", err.Error()) }) t.Run("bad cipherer", func(t *testing.T) { assert, require := assert.New(t), require.New(t) @@ -200,7 +200,7 @@ func Test_NewEntry(t *testing.T) { ticketer, ) require.Error(err) - assert.Equal("error creating entry: entry Cipherer is nil", err.Error()) + assert.Equal("oplog.NewEntry: oplog.(Entry).validate: nil cipherer: parameter violation: error #100", err.Error()) }) t.Run("bad ticket", func(t *testing.T) { assert, require := assert.New(t), require.New(t) @@ -216,7 +216,7 @@ func Test_NewEntry(t *testing.T) { nil, ) require.Error(err) - assert.Equal("error creating entry: entry Ticketer is nil", err.Error()) + assert.Equal("oplog.NewEntry: oplog.(Entry).validate: nil ticketer: parameter violation: error #100", err.Error()) }) } @@ -281,7 +281,7 @@ func Test_UnmarshalData(t *testing.T) { entry.Data = queue.Bytes() _, err = entry.UnmarshalData(types) require.Error(err) - assert.Equal("no Data to unmarshal", err.Error()) + assert.Equal("oplog.(Entry).UnmarshalData: missing data: parameter violation: error #100", err.Error()) }) t.Run("nil types", func(t *testing.T) { @@ -306,7 +306,7 @@ func Test_UnmarshalData(t *testing.T) { require.NoError(err) _, err = entry.UnmarshalData(nil) require.Error(err) - assert.Equal("TypeCatalog is nil", err.Error()) + assert.Equal("oplog.(Entry).UnmarshalData: nil type catalog: parameter violation: error #100", err.Error()) }) t.Run("missing type", func(t *testing.T) { @@ -334,7 +334,7 @@ func Test_UnmarshalData(t *testing.T) { require.NoError(err) _, err = entry.UnmarshalData(types) require.Error(err) - assert.Equal("error removing item from queue: error getting the TypeName for Remove: error typeName is not found for Get", err.Error()) + assert.Equal("oplog.(Entry).UnmarshalData: error removing item from queue: oplog.(Queue).Remove: error getting the TypeName: oplog.(TypeCatalog).Get: type name not found: integrity violation: error #105", err.Error()) }) } @@ -570,7 +570,7 @@ func Test_WriteEntryWith(t *testing.T) { &Message{Message: &u, TypeName: "user", OpType: OpType_OP_TYPE_CREATE}, &Message{Message: &u2, TypeName: "user", OpType: OpType_OP_TYPE_CREATE}) require.Error(err) - assert.Equal("bad writer", err.Error()) + assert.Equal("oplog.(Entry).WriteEntryWith: nil writer: parameter violation: error #100", err.Error()) }) t.Run("nil ticket", func(t *testing.T) { assert, require := assert.New(t), require.New(t) @@ -589,7 +589,7 @@ func Test_WriteEntryWith(t *testing.T) { &Message{Message: &u, TypeName: "user", OpType: OpType_OP_TYPE_CREATE}, &Message{Message: &u2, TypeName: "user", OpType: OpType_OP_TYPE_CREATE}) require.Error(err) - assert.Equal("bad ticket", err.Error()) + assert.Equal("oplog.(Entry).WriteEntryWith: nil ticket: parameter violation: error #100", err.Error()) }) t.Run("nil ticket", func(t *testing.T) { assert, require := assert.New(t), require.New(t) @@ -606,7 +606,7 @@ func Test_WriteEntryWith(t *testing.T) { require.NoError(err) err = newLogEntry.WriteEntryWith(context.Background(), &GormWriter{db}, ticket, nil) require.Error(err) - assert.Equal("bad message", err.Error()) + assert.Equal("oplog.(Entry).WriteEntryWith: nil message: parameter violation: error #100", err.Error()) }) } diff --git a/internal/oplog/queue.go b/internal/oplog/queue.go index 597e28c1f..ed1fc88c0 100644 --- a/internal/oplog/queue.go +++ b/internal/oplog/queue.go @@ -3,11 +3,12 @@ package oplog import ( "bytes" "encoding/binary" - "errors" "fmt" + "io" "sync" - common "github.com/hashicorp/boundary/internal/db/common" + "github.com/hashicorp/boundary/internal/db/common" + "github.com/hashicorp/boundary/internal/errors" "google.golang.org/genproto/protobuf/field_mask" "google.golang.org/protobuf/proto" ) @@ -29,6 +30,7 @@ type Queue struct { // update, etc). If OpType == OpType_OP_TYPE_UPDATE, the WithFieldMaskPaths() // and SetToNullPaths() options are supported. func (q *Queue) Add(m proto.Message, typeName string, t OpType, opt ...Option) error { + const op = "oplog.(Queue).Add" // we're not checking the Catalog for nil, since it's not used // when Adding messages to the queue opts := GetOpts(opt...) @@ -36,15 +38,15 @@ func (q *Queue) Add(m proto.Message, typeName string, t OpType, opt ...Option) e withNullPaths := opts[optionWithSetToNullPaths].([]string) if _, ok := m.(ReplayableMessage); !ok { - return fmt.Errorf("error %T is not a ReplayableMessage", m) + return errors.New(errors.InvalidParameter, op, fmt.Sprintf("%T is not a replayable message", m)) } value, err := proto.Marshal(m) if err != nil { - return fmt.Errorf("error marshaling add parameter: %w", err) + return errors.New(errors.Encode, op, "error marshaling add parameter", errors.WithWrap(err)) } if t == OpType_OP_TYPE_UPDATE { if len(withFieldMasks) == 0 && len(withNullPaths) == 0 { - return fmt.Errorf("queue add: missing field masks or null paths for update") + return errors.New(errors.InvalidParameter, op, "missing field masks or null paths for update") } fMasks := withFieldMasks if fMasks == nil { @@ -56,10 +58,10 @@ func (q *Queue) Add(m proto.Message, typeName string, t OpType, opt ...Option) e } i, _, _, err := common.Intersection(fMasks, nullPaths) if err != nil { - return fmt.Errorf("queue add: %w", err) + return errors.Wrap(err, op) } if len(i) != 0 { - return fmt.Errorf("queue add: field masks and null paths intersect with: %s", i) + return errors.New(errors.InvalidParameter, op, fmt.Sprintf("field masks and null paths intersect with: %s", i)) } } @@ -72,20 +74,20 @@ func (q *Queue) Add(m proto.Message, typeName string, t OpType, opt ...Option) e } data, err := proto.Marshal(msg) if err != nil { - return fmt.Errorf("error marhaling the msg for Add: %w", err) + return errors.New(errors.Encode, op, "error marshaling message", errors.WithWrap(err)) } q.mx.Lock() defer q.mx.Unlock() err = binary.Write(q, binary.LittleEndian, uint32(len(data))) if err != nil { - return err + return errors.New(errors.Io, op, "binary write error", errors.WithWrap(err)) } n, err := q.Write(data) if err != nil { - return fmt.Errorf("error writing to queue buffer: %w", err) + return errors.New(errors.Io, op, "error writing to queue buffer", errors.WithWrap(err)) } if n != len(data) { - return fmt.Errorf("error writing to queue buffer (incorrect number of bytes %d of %d)", n, len(data)) + return errors.New(errors.Io, op, fmt.Sprintf("error writing to queue buffer (incorrect number of bytes %d of %d)", n, len(data))) } return nil } @@ -94,32 +96,36 @@ func (q *Queue) Add(m proto.Message, typeName string, t OpType, opt ...Option) e // for the msg and if it's OpType_OP_TYPE_UPDATE, the it will also return the // fieldMask and setToNullPaths for the update operation. func (q *Queue) Remove() (proto.Message, OpType, []string, []string, error) { + const op = "oplog.(Queue).Remove" if q.Catalog == nil { - return nil, OpType_OP_TYPE_UNSPECIFIED, nil, nil, errors.New("remove Catalog is nil") + return nil, OpType_OP_TYPE_UNSPECIFIED, nil, nil, errors.New(errors.InvalidParameter, op, "nil catalog") } q.mx.Lock() defer q.mx.Unlock() var n uint32 err := binary.Read(q, binary.LittleEndian, &n) + if err == io.EOF { + return nil, 0, nil, nil, err // intentionally not wrapping error, return io.EOF so client can handle it correctly + } if err != nil { - return nil, 0, nil, nil, err // intentionally not wrapping error so client can test for sentinel EOF error + return nil, 0, nil, nil, errors.New(errors.Io, op, "binary read error", errors.WithWrap(err)) } data := q.Next(int(n)) msg := new(AnyOperation) err = proto.Unmarshal(data, msg) if err != nil { - return nil, 0, nil, nil, fmt.Errorf("error marshaling the msg for Remove: %w", err) + return nil, 0, nil, nil, errors.New(errors.Decode, op, "error unmarshaling message", errors.WithWrap(err)) } if msg.Value == nil { return nil, 0, nil, nil, nil } any, err := q.Catalog.Get(msg.TypeName) if err != nil { - return nil, 0, nil, nil, fmt.Errorf("error getting the TypeName for Remove: %w", err) + return nil, 0, nil, nil, errors.Wrap(err, op, errors.WithMsg("error getting the TypeName")) } pm := any.(proto.Message) if err = proto.Unmarshal(msg.Value, pm); err != nil { - return nil, 0, nil, nil, fmt.Errorf("error unmarshaling the value for Remove: %w", err) + return nil, 0, nil, nil, errors.New(errors.Decode, op, "error unmarshaling value", errors.WithWrap(err)) } var masks, nullPaths []string if msg.OperationType == OpType_OP_TYPE_UPDATE { @@ -130,7 +136,7 @@ func (q *Queue) Remove() (proto.Message, OpType, []string, []string, error) { nullPaths = msg.NullMask.GetPaths() } if len(masks) == 0 && len(nullPaths) == 0 { - return nil, 0, nil, nil, errors.New("error unmarshaling the value for Remove: field mask or null paths is required") + return nil, 0, nil, nil, errors.New(errors.InvalidParameter, op, "field mask or null paths is required") } } diff --git a/internal/oplog/queue_test.go b/internal/oplog/queue_test.go index 0bc4de1cc..093a9e296 100644 --- a/internal/oplog/queue_test.go +++ b/internal/oplog/queue_test.go @@ -104,7 +104,7 @@ func Test_Queue(t *testing.T) { queue := Queue{} _, _, _, _, err = queue.Remove() require.Error(err) - assert.Equal("remove Catalog is nil", err.Error()) + assert.Equal("oplog.(Queue).Remove: nil catalog: parameter violation: error #100", err.Error()) }) t.Run("not replayable", func(t *testing.T) { assert, require := assert.New(t), require.New(t) @@ -115,13 +115,13 @@ func Test_Queue(t *testing.T) { } err = queue.Add(u, "user", OpType_OP_TYPE_CREATE) require.Error(err) - assert.Equal("error *oplog_test.TestNonReplayableUser is not a ReplayableMessage", err.Error()) + assert.Equal("oplog.(Queue).Add: *oplog_test.TestNonReplayableUser is not a replayable message: parameter violation: error #100", err.Error()) }) t.Run("nil message", func(t *testing.T) { assert, require := assert.New(t), require.New(t) err = queue.Add(nil, "user", OpType_OP_TYPE_CREATE) require.Error(err) - assert.Equal("error is not a ReplayableMessage", err.Error()) + assert.Equal("oplog.(Queue).Add: is not a replayable message: parameter violation: error #100", err.Error()) }) t.Run("missing both field mask and null paths for update", func(t *testing.T) { require.Error(t, queue.Add(userUpdate, "user", OpType_OP_TYPE_UPDATE)) diff --git a/internal/oplog/ticketer.go b/internal/oplog/ticketer.go index cbf4d38e4..75d8942fa 100644 --- a/internal/oplog/ticketer.go +++ b/internal/oplog/ticketer.go @@ -1,19 +1,11 @@ package oplog import ( - "errors" - "fmt" - + "github.com/hashicorp/boundary/internal/errors" "github.com/hashicorp/boundary/internal/oplog/store" "github.com/jinzhu/gorm" ) -var ( - ErrTicketNotFound = errors.New("ticket not found") - ErrTicketAlreadyRedeemed = errors.New("ticket already redeemed") - ErrTicketRedeeming = errors.New("error trying to redeem ticket") -) - const DefaultAggregateName = "global" // Ticketer provides an interface to storage for Tickets, so you can easily substitute your own ticketer @@ -39,8 +31,9 @@ type GormTicketer struct { // NewGormTicketer creates a new ticketer that uses gorm for storage func NewGormTicketer(tx *gorm.DB, opt ...Option) (*GormTicketer, error) { + const op = "oplog.NewGormTicketer" if tx == nil { - return nil, errors.New("tx is nil") + return nil, errors.New(errors.InvalidParameter, op, "nil tx") } opts := GetOpts(opt...) enableAggregateNames := opts[optionWithAggregateNames].(bool) @@ -50,8 +43,9 @@ func NewGormTicketer(tx *gorm.DB, opt ...Option) (*GormTicketer, error) { // GetTicket returns a ticket for the specified name. You MUST GetTicket in the same transaction // that you're using to write to the database tables. Names allow us to shard tickets around domain root names func (ticketer *GormTicketer) GetTicket(aggregateName string) (*store.Ticket, error) { + const op = "oplog.(GormTicketer).GetTicket" if aggregateName == "" { - return nil, errors.New("bad ticket name") + return nil, errors.New(errors.InvalidParameter, op, "missing ticket name") } name := DefaultAggregateName if ticketer.withAggregateNames { @@ -60,24 +54,25 @@ func (ticketer *GormTicketer) GetTicket(aggregateName string) (*store.Ticket, er ticket := store.Ticket{} if err := ticketer.tx.First(&ticket, store.Ticket{Name: name}).Error; err != nil { if gorm.IsRecordNotFoundError(err) { - return nil, ErrTicketNotFound + return nil, errors.New(errors.TicketNotFound, op, "ticket not found") } - return nil, fmt.Errorf("error retreiving ticket from storage: %w", err) + return nil, errors.Wrap(err, op, errors.WithMsg("error retrieving ticket from storage")) } return &ticket, nil } // Redeem will attempt to redeem the ticket. If the ticket version has already been used, then an error is returned func (ticketer *GormTicketer) Redeem(t *store.Ticket) error { + const op = "oplog.(GormTicketer).Redeem" if t == nil { - return errors.New("ticket is nil") + return errors.New(errors.InvalidParameter, op, "nil ticket") } tx := ticketer.tx.Model(t).Where("version = ?", t.Version).Update("version", t.Version+1) if tx.Error != nil { - return ErrTicketRedeeming + return errors.Wrap(tx.Error, op, errors.WithMsg("error trying to redeem ticket")) } if tx.RowsAffected != 1 { - return ErrTicketAlreadyRedeemed + return errors.New(errors.TicketAlreadyRedeemed, op, "ticket already redeemed") } return nil } diff --git a/internal/oplog/ticketer_gorm_test.go b/internal/oplog/ticketer_gorm_test.go index 50654b222..9852645dd 100644 --- a/internal/oplog/ticketer_gorm_test.go +++ b/internal/oplog/ticketer_gorm_test.go @@ -22,7 +22,7 @@ func Test_NewGormTicketer(t *testing.T) { assert, require := assert.New(t), require.New(t) _, err := NewGormTicketer(nil, WithAggregateNames(true)) require.Error(err) - assert.Equal("tx is nil", err.Error()) + assert.Equal("oplog.NewGormTicketer: nil tx: parameter violation: error #100", err.Error()) }) } @@ -46,7 +46,7 @@ func Test_GetTicket(t *testing.T) { assert, require := assert.New(t), require.New(t) ticket, err := ticketer.GetTicket("") require.Error(err) - assert.Equal("bad ticket name", err.Error()) + assert.Equal("oplog.(GormTicketer).GetTicket: missing ticket name: parameter violation: error #100", err.Error()) assert.Nil(ticket) }) } @@ -80,7 +80,7 @@ func Test_Redeem(t *testing.T) { require.NoError(err) err = ticketer.Redeem(nil) require.Error(err) - assert.Equal("ticket is nil", err.Error()) + assert.Equal("oplog.(GormTicketer).Redeem: nil ticket: parameter violation: error #100", err.Error()) }) t.Run("detect two redemptions in separate concurrent transactions", func(t *testing.T) { @@ -104,6 +104,6 @@ func Test_Redeem(t *testing.T) { tx.Commit() err = secondTicketer.Redeem(secondTicket) - assert.Equal("ticket already redeemed", err.Error()) + assert.Equal("oplog.(GormTicketer).Redeem: ticket already redeemed: integrity violation: error #106", err.Error()) }) } diff --git a/internal/oplog/type_catalog.go b/internal/oplog/type_catalog.go index f3f3ff4e7..74d0aabda 100644 --- a/internal/oplog/type_catalog.go +++ b/internal/oplog/type_catalog.go @@ -1,9 +1,10 @@ package oplog import ( - "errors" - fmt "fmt" + "fmt" "reflect" + + "github.com/hashicorp/boundary/internal/errors" ) // TypeCatalog is an abstraction for dealing with oplog data and their underlying types @@ -21,13 +22,14 @@ type Type struct { // NewTypeCatalog creates a catalog with the types you pass in func NewTypeCatalog(withTypes ...Type) (*TypeCatalog, error) { + const op = "oplog.NewTypeCatalog" reg := TypeCatalog{} for _, t := range withTypes { if t == (Type{}) { - return nil, errors.New("error type is {} (in NewTypeCatalog)") + return nil, errors.New(errors.InvalidParameter, op, "error type is {}") } if err := reg.Set(t.Interface, t.Name); err != nil { - return nil, fmt.Errorf("error setting the type: %w (in NewTypeCatalog)", err) + return nil, errors.Wrap(err, op, errors.WithMsg("error setting the type")) } } return ®, nil @@ -35,11 +37,12 @@ func NewTypeCatalog(withTypes ...Type) (*TypeCatalog, error) { // GetTypeName returns the interface's name from the catalog func (t *TypeCatalog) GetTypeName(i interface{}) (string, error) { + const op = "oplog.(TypeCatalog).GetTypeName" if i == nil { - return "", errors.New("error interface parameter is nil for GetTypeName") + return "", errors.New(errors.InvalidParameter, op, "nil interface") } if reflect.ValueOf(i).Kind() != reflect.Ptr { - return "", errors.New("error interface parameter must to be a pointer for GetTypeName") + return "", errors.New(errors.InvalidParameter, op, "interface must to be a pointer") } interfaceType := reflect.TypeOf(i) for name, t := range *t { @@ -47,19 +50,20 @@ func (t *TypeCatalog) GetTypeName(i interface{}) (string, error) { return name, nil } } - return "", fmt.Errorf("error unknown name for interface: %T", i) + return "", errors.New(errors.InvalidParameter, op, fmt.Sprintf("unknown name for interface: %T", i)) } // Set creates an entry in the catalog for the interface func (t TypeCatalog) Set(i interface{}, typeName string) error { + const op = "oplog.(TypeCatalog).Set" if i == nil { - return errors.New("error interface parameter is nil for Set") + return errors.New(errors.InvalidParameter, op, "nil interface") } if reflect.ValueOf(i).Kind() != reflect.Ptr { - return errors.New("error interface parameter must to be a pointer for Set") + return errors.New(errors.InvalidParameter, op, "interface must to be a pointer") } if typeName == "" { - return errors.New("typeName is an empty string for Set") + return errors.New(errors.InvalidParameter, op, "missing type name") } t[typeName] = reflect.TypeOf(i) return nil @@ -67,11 +71,12 @@ func (t TypeCatalog) Set(i interface{}, typeName string) error { // Get retrieves the interface via a name func (t TypeCatalog) Get(typeName string) (interface{}, error) { + const op = "oplog.(TypeCatalog).Get" if typeName == "" { - return nil, errors.New("error typeName is empty string for Get") + return nil, errors.New(errors.InvalidParameter, op, "missing type name") } if typ, ok := t[typeName]; ok { return reflect.New(typ.Elem()).Elem().Addr().Interface(), nil } - return nil, errors.New("error typeName is not found for Get") + return nil, errors.New(errors.KeyNotFound, op, "type name not found") } diff --git a/internal/oplog/type_catalog_test.go b/internal/oplog/type_catalog_test.go index ee7218c3e..a406f1076 100644 --- a/internal/oplog/type_catalog_test.go +++ b/internal/oplog/type_catalog_test.go @@ -61,7 +61,7 @@ func Test_NewTypeCatalog(t *testing.T) { ) assert.Nil(types) assert.Error(err) - assert.Equal("error setting the type: typeName is an empty string for Set (in NewTypeCatalog)", err.Error()) + assert.Equal("oplog.NewTypeCatalog: error setting the type: oplog.(TypeCatalog).Set: missing type name: parameter violation: error #100", err.Error()) }) t.Run("missing Type.Interface", func(t *testing.T) { assert := assert.New(t) @@ -71,7 +71,7 @@ func Test_NewTypeCatalog(t *testing.T) { ) assert.Nil(types) assert.Error(err) - assert.Equal("error type is {} (in NewTypeCatalog)", err.Error()) + assert.Equal("oplog.NewTypeCatalog: error type is {}: parameter violation: error #100", err.Error()) }) t.Run("empty Type", func(t *testing.T) { assert := assert.New(t) @@ -81,7 +81,7 @@ func Test_NewTypeCatalog(t *testing.T) { ) assert.Nil(types) assert.Error(err) - assert.Equal("error type is {} (in NewTypeCatalog)", err.Error()) + assert.Equal("oplog.NewTypeCatalog: error type is {}: parameter violation: error #100", err.Error()) }) } @@ -113,7 +113,7 @@ func Test_GetTypeName(t *testing.T) { n, err := types.GetTypeName(new(oplog_test.TestCar)) require.Error(err) assert.Equal(n, "") - assert.Equal("error unknown name for interface: *oplog_test.TestCar", err.Error()) + assert.Equal("oplog.(TypeCatalog).GetTypeName: unknown name for interface: *oplog_test.TestCar: parameter violation: error #100", err.Error()) }) t.Run("nil interface", func(t *testing.T) { assert, require := assert.New(t), require.New(t) @@ -126,7 +126,7 @@ func Test_GetTypeName(t *testing.T) { n, err := types.GetTypeName(nil) require.Error(err) assert.Equal(n, "") - assert.Equal("error interface parameter is nil for GetTypeName", err.Error()) + assert.Equal("oplog.(TypeCatalog).GetTypeName: nil interface: parameter violation: error #100", err.Error()) }) } @@ -158,7 +158,7 @@ func Test_Get(t *testing.T) { n, err := types.Get("car") require.Error(err) assert.Equal(n, nil) - assert.Equal("error typeName is not found for Get", err.Error()) + assert.Equal("oplog.(TypeCatalog).Get: type name not found: integrity violation: error #105", err.Error()) }) t.Run("bad typeName", func(t *testing.T) { assert, require := assert.New(t), require.New(t) @@ -171,7 +171,7 @@ func Test_Get(t *testing.T) { n, err := types.Get("") require.Error(err) assert.Equal(n, nil) - assert.Equal("error typeName is empty string for Get", err.Error()) + assert.Equal("oplog.(TypeCatalog).Get: missing type name: parameter violation: error #100", err.Error()) }) } @@ -197,7 +197,7 @@ func Test_Set(t *testing.T) { u := new(oplog_test.TestUser) err = types.Set(u, "") require.Error(err) - assert.Equal("typeName is an empty string for Set", err.Error()) + assert.Equal("oplog.(TypeCatalog).Set: missing type name: parameter violation: error #100", err.Error()) assert.Equal(types, &TypeCatalog{}) }) t.Run("bad interface", func(t *testing.T) { @@ -207,7 +207,7 @@ func Test_Set(t *testing.T) { require.NoError(err) err = types.Set(nil, "") require.Error(err) - assert.Equal("error interface parameter is nil for Set", err.Error()) + assert.Equal("oplog.(TypeCatalog).Set: nil interface: parameter violation: error #100", err.Error()) assert.Equal(types, &TypeCatalog{}) }) } diff --git a/internal/oplog/writer.go b/internal/oplog/writer.go index ebd89ab96..d9724ed97 100644 --- a/internal/oplog/writer.go +++ b/internal/oplog/writer.go @@ -1,11 +1,10 @@ package oplog import ( - "errors" "fmt" "github.com/hashicorp/boundary/internal/db/common" - + "github.com/hashicorp/boundary/internal/errors" "github.com/jinzhu/gorm" ) @@ -41,14 +40,15 @@ type GormWriter struct { // Create an object in storage func (w *GormWriter) Create(i interface{}) error { + const op = "oplog.(GormWriter).Create" if w.Tx == nil { - return errors.New("create Tx is nil") + return errors.New(errors.InvalidParameter, op, "nil tx") } if i == nil { - return errors.New("create interface is nil") + return errors.New(errors.InvalidParameter, op, "nil interface") } if err := w.Tx.Create(i).Error; err != nil { - return fmt.Errorf("error creating: %w", err) + return errors.Wrap(err, op) } return nil } @@ -57,37 +57,39 @@ func (w *GormWriter) Create(i interface{}) error { // Paths from field_mask.proto. fieldMaskPaths and setNullPaths cannot // intersect and both cannot be zero len. func (w *GormWriter) Update(i interface{}, fieldMaskPaths, setToNullPaths []string) error { + const op = "oplog.(GormWriter).Update" if w.Tx == nil { - return errors.New("update Tx is nil") + return errors.New(errors.InvalidParameter, op, "nil tx") } if i == nil { - return errors.New("update interface is nil") + return errors.New(errors.InvalidParameter, op, "nil interface") } if len(fieldMaskPaths) == 0 && len(setToNullPaths) == 0 { - return errors.New("update both fieldMaskPaths and setToNullPaths are missing") + return errors.New(errors.InvalidParameter, op, "missing field mask paths and set to null paths") } // common.UpdateFields will also check to ensure that fieldMaskPaths and // setToNullPaths do not intersect. updateFields, err := common.UpdateFields(i, fieldMaskPaths, setToNullPaths) if err != nil { - return fmt.Errorf("error updating: unable to build update fields %w", err) + return errors.Wrap(err, op, errors.WithMsg("unable to build update fields")) } if err := w.Tx.Model(i).Updates(updateFields).Error; err != nil { - return fmt.Errorf("error updating: %w", err) + return errors.Wrap(err, op) } return nil } // Deleting an object in storage func (w *GormWriter) Delete(i interface{}) error { + const op = "oplog.(GormWriter).Delete" if w.Tx == nil { - return errors.New("delete Tx is nil") + return errors.New(errors.InvalidParameter, op, "nil tx") } if i == nil { - return errors.New("delete interface is nil") + return errors.New(errors.InvalidParameter, op, "nil interface") } if err := w.Tx.Delete(i).Error; err != nil { - return fmt.Errorf("error deleting: %w", err) + return errors.Wrap(err, op) } return nil } @@ -103,11 +105,12 @@ func (w *GormWriter) hasTable(tableName string) bool { // CreateTableLike will create a newTableName like the model's table // the new table should have all things like the existing model's table (defaults, constraints, indexes, etc) func (w *GormWriter) createTableLike(existingTableName string, newTableName string) error { + const op = "oplog.(GormWriter).createTableLike" if existingTableName == "" { - return errors.New("error existingTableName is empty string") + return errors.New(errors.InvalidParameter, op, "missing existing table name") } if newTableName == "" { - return errors.New("error newTableName is empty string") + return errors.New(errors.InvalidParameter, op, "missing new table name") } existingTableName = w.Tx.Dialect().Quote(existingTableName) newTableName = w.Tx.Dialect().Quote(newTableName) @@ -125,15 +128,24 @@ func (w *GormWriter) createTableLike(existingTableName string, newTableName stri existingTableName, ) default: - return errors.New("error unsupported RDBMS") + return errors.New(errors.InvalidParameter, op, "unsupported RDBMS") } - return w.Tx.Exec(sql).Error + err := w.Tx.Exec(sql).Error + if err != nil { + return errors.Wrap(err, op) + } + return nil } // DropTableIfExists will drop the table if it exists func (w *GormWriter) dropTableIfExists(tableName string) error { + const op = "oplog.(GormWriter).dropTableIfExists" if tableName == "" { - return errors.New("cannot drop table whose name is an empty string") + return errors.New(errors.InvalidParameter, op, "missing table name") } - return w.Tx.DropTableIfExists(tableName).Error + err := w.Tx.DropTableIfExists(tableName).Error + if err != nil { + return errors.Wrap(err, op) + } + return nil } diff --git a/internal/oplog/writer_test.go b/internal/oplog/writer_test.go index 772190ad7..72c482243 100644 --- a/internal/oplog/writer_test.go +++ b/internal/oplog/writer_test.go @@ -34,7 +34,7 @@ func Test_GormWriterCreate(t *testing.T) { err := w.Create(&oplog_test.TestUser{}) require.Error(err) - assert.Equal("create Tx is nil", err.Error()) + assert.Equal("oplog.(GormWriter).Create: nil tx: parameter violation: error #100", err.Error()) }) t.Run("nil model", func(t *testing.T) { assert, require := assert.New(t), require.New(t) @@ -43,7 +43,7 @@ func Test_GormWriterCreate(t *testing.T) { w := GormWriter{tx} err := w.Create(nil) require.Error(err) - assert.Equal("create interface is nil", err.Error()) + assert.Equal("oplog.(GormWriter).Create: nil interface: parameter violation: error #100", err.Error()) }) } @@ -71,7 +71,7 @@ func Test_GormWriterDelete(t *testing.T) { w := GormWriter{nil} err := w.Delete(&oplog_test.TestUser{}) require.Error(err) - assert.Equal("delete Tx is nil", err.Error()) + assert.Equal("oplog.(GormWriter).Delete: nil tx: parameter violation: error #100", err.Error()) }) t.Run("nil model", func(t *testing.T) { assert, require := assert.New(t), require.New(t) @@ -80,7 +80,7 @@ func Test_GormWriterDelete(t *testing.T) { w := GormWriter{tx} err := w.Delete(nil) require.Error(err) - assert.Equal("delete interface is nil", err.Error()) + assert.Equal("oplog.(GormWriter).Delete: nil interface: parameter violation: error #100", err.Error()) }) } @@ -147,7 +147,7 @@ func Test_GormWriterCreateTable(t *testing.T) { err := w.createTableLike("", newTableName) require.Error(err) assert.Error(err, err.Error(), nil) - assert.Equal("error existingTableName is empty string", err.Error()) + assert.Equal("oplog.(GormWriter).createTableLike: missing existing table name: parameter violation: error #100", err.Error()) }) t.Run("blank name", func(t *testing.T) { assert, require := assert.New(t), require.New(t) @@ -156,7 +156,7 @@ func Test_GormWriterCreateTable(t *testing.T) { err := w.createTableLike(u.TableName(), "") require.Error(err) assert.Error(err, err.Error(), nil) - assert.Equal("error newTableName is empty string", err.Error()) + assert.Equal("oplog.(GormWriter).createTableLike: missing new table name: parameter violation: error #100", err.Error()) }) } @@ -181,7 +181,7 @@ func Test_GormWriterDropTableIfExists(t *testing.T) { w := GormWriter{Tx: db} err := w.dropTableIfExists("") require.Error(err) - assert.Equal("cannot drop table whose name is an empty string", err.Error()) + assert.Equal("oplog.(GormWriter).dropTableIfExists: missing table name: parameter violation: error #100", err.Error()) }) } diff --git a/internal/perms/grants.go b/internal/perms/grants.go index a5487c9ea..6b7809a0a 100644 --- a/internal/perms/grants.go +++ b/internal/perms/grants.go @@ -2,11 +2,11 @@ package perms import ( "encoding/json" - "errors" "fmt" "sort" "strings" + "github.com/hashicorp/boundary/internal/errors" "github.com/hashicorp/boundary/internal/types/action" "github.com/hashicorp/boundary/internal/types/resource" "github.com/hashicorp/boundary/internal/types/scope" @@ -114,6 +114,7 @@ func (g Grant) CanonicalString() string { // MarshalJSON provides a custom marshaller for grants func (g Grant) MarshalJSON() ([]byte, error) { + const op = "perms.(Grant).MarshalJSON" res := make(map[string]interface{}, 4) if g.id != "" { res["id"] = g.id @@ -129,38 +130,43 @@ func (g Grant) MarshalJSON() ([]byte, error) { sort.Strings(actions) res["actions"] = actions } - return json.Marshal(res) + b, err := json.Marshal(res) + if err != nil { + return nil, errors.Wrap(err, op, errors.WithCode(errors.Encode)) + } + return b, nil } // This is purposefully unexported since the values being set here are not being // checked for validity. This should only be called by the main parsing function // when JSON is detected. func (g *Grant) unmarshalJSON(data []byte) error { + const op = "perms.(Grant).unmarshalJSON" raw := make(map[string]interface{}, 4) if err := json.Unmarshal(data, &raw); err != nil { - return err + return errors.Wrap(err, op, errors.WithCode(errors.Decode)) } if rawId, ok := raw["id"]; ok { id, ok := rawId.(string) if !ok { - return fmt.Errorf("unable to interpret %q as string", "id") + return errors.New(errors.InvalidParameter, op, fmt.Sprintf("unable to interpret %q as string", "id")) } g.id = id } if rawType, ok := raw["type"]; ok { typ, ok := rawType.(string) if !ok { - return fmt.Errorf("unable to interpret %q as string", "type") + return errors.New(errors.InvalidParameter, op, fmt.Sprintf("unable to interpret %q as string", "type")) } g.typ = resource.Map[typ] if g.typ == resource.Unknown { - return fmt.Errorf("unknown type specifier %q", typ) + return errors.New(errors.InvalidParameter, op, fmt.Sprintf("unknown type specifier %q", typ)) } } if rawActions, ok := raw["actions"]; ok { interfaceActions, ok := rawActions.([]interface{}) if !ok { - return fmt.Errorf("unable to interpret %q as array", "actions") + return errors.New(errors.InvalidParameter, op, fmt.Sprintf("unable to interpret %q as array", "actions")) } if len(interfaceActions) > 0 { g.actionsBeingParsed = make([]string, 0, len(interfaceActions)) @@ -168,9 +174,9 @@ func (g *Grant) unmarshalJSON(data []byte) error { actionStr, ok := v.(string) switch { case !ok: - return fmt.Errorf("unable to interpret %v in actions array as string", v) + return errors.New(errors.InvalidParameter, op, fmt.Sprintf("unable to interpret %v in actions array as string", v)) case actionStr == "": - return errors.New("empty action found") + return errors.New(errors.InvalidParameter, op, "empty action found") default: g.actionsBeingParsed = append(g.actionsBeingParsed, strings.ToLower(actionStr)) } @@ -181,6 +187,7 @@ func (g *Grant) unmarshalJSON(data []byte) error { } func (g *Grant) unmarshalText(grantString string) error { + const op = "perms.(Grant).unmarshalText" segments := strings.Split(grantString, ";") for _, segment := range segments { kv := strings.Split(segment, "=") @@ -188,11 +195,11 @@ func (g *Grant) unmarshalText(grantString string) error { // Ensure we don't accept "foo=bar=baz", "=foo", or "foo=" switch { case len(kv) != 2: - return fmt.Errorf("segment %q not formatted correctly, wrong number of equal signs", segment) + return errors.New(errors.InvalidParameter, op, fmt.Sprintf("segment %q not formatted correctly, wrong number of equal signs", segment)) case len(kv[0]) == 0: - return fmt.Errorf("segment %q not formatted correctly, missing key", segment) + return errors.New(errors.InvalidParameter, op, fmt.Sprintf("segment %q not formatted correctly, missing key", segment)) case len(kv[1]) == 0: - return fmt.Errorf("segment %q not formatted correctly, missing value", segment) + return errors.New(errors.InvalidParameter, op, fmt.Sprintf("segment %q not formatted correctly, missing value", segment)) } switch kv[0] { @@ -203,7 +210,7 @@ func (g *Grant) unmarshalText(grantString string) error { typeString := strings.ToLower(kv[1]) g.typ = resource.Map[typeString] if g.typ == resource.Unknown { - return fmt.Errorf("unknown type specifier %q", typeString) + return errors.New(errors.InvalidParameter, op, fmt.Sprintf("unknown type specifier %q", typeString)) } case "actions": @@ -212,7 +219,7 @@ func (g *Grant) unmarshalText(grantString string) error { g.actionsBeingParsed = make([]string, 0, len(actions)) for _, action := range actions { if action == "" { - return errors.New("empty action found") + return errors.New(errors.InvalidParameter, op, "empty action found") } g.actionsBeingParsed = append(g.actionsBeingParsed, strings.ToLower(action)) } @@ -231,18 +238,17 @@ func (g *Grant) unmarshalText(grantString string) error { // The scope must be the org and project where this grant originated, not the // request. func Parse(scopeId, grantString string, opt ...Option) (Grant, error) { + const op = "perms.Parse" if len(grantString) == 0 { - return Grant{}, errors.New("grant string is empty") + return Grant{}, errors.New(errors.InvalidParameter, op, "missing grant string") } - if scopeId == "" { - return Grant{}, errors.New("no scope ID provided") + return Grant{}, errors.New(errors.InvalidParameter, op, "missing scope id") } grant := Grant{ scope: Scope{Id: scopeId}, } - switch { case scopeId == "global": grant.scope.Type = scope.Global @@ -251,18 +257,18 @@ func Parse(scopeId, grantString string, opt ...Option) (Grant, error) { case strings.HasPrefix(scopeId, scope.Project.Prefix()): grant.scope.Type = scope.Project default: - return Grant{}, errors.New("invalid scope type") + return Grant{}, errors.New(errors.InvalidParameter, op, "invalid scope type") } switch { case grantString[0] == '{': if err := grant.unmarshalJSON([]byte(grantString)); err != nil { - return Grant{}, fmt.Errorf("unable to parse JSON grant string: %w", err) + return Grant{}, errors.Wrap(err, op, errors.WithMsg("unable to parse JSON grant string")) } default: if err := grant.unmarshalText(grantString); err != nil { - return Grant{}, fmt.Errorf("unable to parse grant string: %w", err) + return Grant{}, errors.Wrap(err, op, errors.WithMsg("unable to parse grant string")) } } @@ -283,16 +289,16 @@ func Parse(scopeId, grantString string, opt ...Option) (Grant, error) { grant.id = opts.withAccountId } default: - return Grant{}, fmt.Errorf("unknown template %q in grant %q value", grant.id, "id") + return Grant{}, errors.New(errors.InvalidParameter, op, fmt.Sprintf("unknown template %q in grant %q value", grant.id, "id")) } } if err := grant.validateType(); err != nil { - return Grant{}, err + return Grant{}, errors.Wrap(err, op) } if err := grant.parseAndValidateActions(); err != nil { - return Grant{}, err + return Grant{}, errors.Wrap(err, op) } if !opts.withSkipFinalValidation { @@ -315,7 +321,7 @@ func Parse(scopeId, grantString string, opt ...Option) (Grant, error) { } } if !allowed { - return Grant{}, errors.New("parsed grant string would not result in any action being authorized") + return Grant{}, errors.New(errors.InvalidParameter, op, "parsed grant string would not result in any action being authorized") } } @@ -323,6 +329,7 @@ func Parse(scopeId, grantString string, opt ...Option) (Grant, error) { } func (g Grant) validateType() error { + const op = "perms.(Grant).validateType" switch g.typ { case resource.Unknown, resource.All, @@ -339,30 +346,31 @@ func (g Grant) validateType() error { resource.Session: return nil } - return fmt.Errorf("unknown type specifier %q", g.typ) + return errors.New(errors.InvalidParameter, op, fmt.Sprintf("unknown type specifier %q", g.typ)) } func (g *Grant) parseAndValidateActions() error { + const op = "perms.(Grant).parseAndValidateActions" if len(g.actionsBeingParsed) == 0 { - return errors.New("no actions specified") + return errors.New(errors.InvalidParameter, op, "missing actions") } for _, a := range g.actionsBeingParsed { if a == "" { - return errors.New("empty action found") + return errors.New(errors.InvalidParameter, op, "empty action found") } if g.actions == nil { g.actions = make(map[action.Type]bool, len(g.actionsBeingParsed)) } if am := action.Map[a]; am == action.Unknown { - return fmt.Errorf("unknown action %q", a) + return errors.New(errors.InvalidParameter, op, fmt.Sprintf("unknown action %q", a)) } else { g.actions[am] = true } } if len(g.actions) > 1 && g.actions[action.All] { - return fmt.Errorf("%q cannot be specified with other actions", action.All.String()) + return errors.New(errors.InvalidParameter, op, fmt.Sprintf("%q cannot be specified with other actions", action.All.String())) } g.actionsBeingParsed = nil diff --git a/internal/perms/grants_test.go b/internal/perms/grants_test.go index 7d1f2d8c4..c441d484d 100644 --- a/internal/perms/grants_test.go +++ b/internal/perms/grants_test.go @@ -1,7 +1,6 @@ package perms import ( - "strings" "testing" "github.com/hashicorp/boundary/internal/types/action" @@ -24,21 +23,21 @@ func Test_ActionParsingValidation(t *testing.T) { tests := []input{ { name: "no actions", - errResult: "no actions specified", + errResult: "perms.(Grant).parseAndValidateActions: missing actions: parameter violation: error #100", }, { name: "empty action", input: Grant{ actionsBeingParsed: []string{"create", "", "read"}, }, - errResult: "empty action found", + errResult: "perms.(Grant).parseAndValidateActions: empty action found: parameter violation: error #100", }, { name: "unknown action", input: Grant{ actionsBeingParsed: []string{"create", "foobar", "read"}, }, - errResult: `unknown action "foobar"`, + errResult: `perms.(Grant).parseAndValidateActions: unknown action "foobar": parameter violation: error #100`, }, { name: "all", @@ -56,7 +55,7 @@ func Test_ActionParsingValidation(t *testing.T) { input: Grant{ actionsBeingParsed: []string{"list", "create", "update", "*", "read", "delete", "authenticate", "authorize-session"}, }, - errResult: `"*" cannot be specified with other actions`, + errResult: `perms.(Grant).parseAndValidateActions: "*" cannot be specified with other actions: parameter violation: error #100`, }, { name: "all valid", @@ -211,14 +210,14 @@ func Test_Unmarshaling(t *testing.T) { { name: "bad json", jsonInput: `w329uf`, - jsonErr: "invalid character 'w' looking for beginning of value", + jsonErr: "perms.(Grant).unmarshalJSON: error occurred during decode, encoding issue: error #303: invalid character 'w' looking for beginning of value", }, { name: "bad segment", jsonInput: `{"id":true}`, - jsonErr: `unable to interpret "id" as string`, + jsonErr: `perms.(Grant).unmarshalJSON: unable to interpret "id" as string: parameter violation: error #100`, textInput: `id=`, - textErr: `segment "id=" not formatted correctly, missing value`, + textErr: `perms.(Grant).unmarshalText: segment "id=" not formatted correctly, missing value: parameter violation: error #100`, }, { name: "good id", @@ -231,9 +230,9 @@ func Test_Unmarshaling(t *testing.T) { { name: "bad id", jsonInput: `{"id":true}`, - jsonErr: `unable to interpret "id" as string`, + jsonErr: `perms.(Grant).unmarshalJSON: unable to interpret "id" as string: parameter violation: error #100`, textInput: `=id`, - textErr: `segment "=id" not formatted correctly, missing key`, + textErr: `perms.(Grant).unmarshalText: segment "=id" not formatted correctly, missing key: parameter violation: error #100`, }, { name: "good type", @@ -246,9 +245,9 @@ func Test_Unmarshaling(t *testing.T) { { name: "bad type", jsonInput: `{"type":true}`, - jsonErr: `unable to interpret "type" as string`, + jsonErr: `perms.(Grant).unmarshalJSON: unable to interpret "type" as string: parameter violation: error #100`, textInput: `type=host-catalog=id`, - textErr: `segment "type=host-catalog=id" not formatted correctly, wrong number of equal signs`, + textErr: `perms.(Grant).unmarshalText: segment "type=host-catalog=id" not formatted correctly, wrong number of equal signs: parameter violation: error #100`, }, { name: "good actions", @@ -261,21 +260,21 @@ func Test_Unmarshaling(t *testing.T) { { name: "bad actions", jsonInput: `{"actions":true}`, - jsonErr: `unable to interpret "actions" as array`, + jsonErr: `perms.(Grant).unmarshalJSON: unable to interpret "actions" as array: parameter violation: error #100`, textInput: `type=host-catalog=id`, - textErr: `segment "type=host-catalog=id" not formatted correctly, wrong number of equal signs`, + textErr: `perms.(Grant).unmarshalText: segment "type=host-catalog=id" not formatted correctly, wrong number of equal signs: parameter violation: error #100`, }, { name: "empty actions", jsonInput: `{"actions":[""]}`, - jsonErr: `empty action found`, + jsonErr: `perms.(Grant).unmarshalJSON: empty action found: parameter violation: error #100`, textInput: `actions=,`, - textErr: `empty action found`, + textErr: `perms.(Grant).unmarshalText: empty action found: parameter violation: error #100`, }, { name: "bad json action", jsonInput: `{"actions":[1, true]}`, - jsonErr: `unable to interpret 1 in actions array as string`, + jsonErr: `perms.(Grant).unmarshalJSON: unable to interpret 1 in actions array as string: parameter violation: error #100`, }, } @@ -325,32 +324,32 @@ func Test_Parse(t *testing.T) { tests := []input{ { name: "empty", - err: `grant string is empty`, + err: `perms.Parse: missing grant string: parameter violation: error #100`, }, { name: "bad json", input: "{2:193}", - err: `unable to parse JSON grant string:`, + err: `perms.Parse: unable to parse JSON grant string: perms.(Grant).unmarshalJSON: error occurred during decode, encoding issue: error #303: invalid character '2' looking for beginning of object key string`, }, { name: "bad text", input: "id=foo=bar", - err: `unable to parse grant string:`, + err: `perms.Parse: unable to parse grant string: perms.(Grant).unmarshalText: segment "id=foo=bar" not formatted correctly, wrong number of equal signs: parameter violation: error #100`, }, { name: "bad type", input: "id=foobar;type=barfoo;actions=create,read", - err: `unknown type specifier "barfoo"`, + err: `perms.Parse: unable to parse grant string: perms.(Grant).unmarshalText: unknown type specifier "barfoo": parameter violation: error #100`, }, { name: "bad actions", input: "id=foobar;type=host-catalog;actions=createread", - err: `unknown action "createread"`, + err: `perms.Parse: perms.(Grant).parseAndValidateActions: unknown action "createread": parameter violation: error #100`, }, { name: "empty id and type", input: "actions=create", - err: `parsed grant string would not result in any action being authorized`, + err: `perms.Parse: parsed grant string would not result in any action being authorized: parameter violation: error #100`, }, { name: "good json type", @@ -462,7 +461,7 @@ func Test_Parse(t *testing.T) { name: "bad user id template", input: `id={{superman}};actions=create,read`, userId: "u_abcd1234", - err: `unknown template "{{superman}}" in grant "id" value`, + err: `perms.Parse: unknown template "{{superman}}" in grant "id" value: parameter violation: error #100`, }, { name: "good user id template", @@ -484,7 +483,7 @@ func Test_Parse(t *testing.T) { name: "bad account id template", input: `id={{superman}};actions=create,read`, accountId: "apw_1234567890", - err: `unknown template "{{superman}}" in grant "id" value`, + err: `perms.Parse: unknown template "{{superman}}" in grant "id" value: parameter violation: error #100`, }, { name: "good account id template", @@ -506,11 +505,11 @@ func Test_Parse(t *testing.T) { _, err := Parse("", "") require.Error(t, err) - assert.Equal(t, "grant string is empty", err.Error()) + assert.Equal(t, "perms.Parse: missing grant string: parameter violation: error #100", err.Error()) _, err = Parse("", "{}") require.Error(t, err) - assert.Equal(t, "no scope ID provided", err.Error()) + assert.Equal(t, "perms.Parse: missing scope id: parameter violation: error #100", err.Error()) for _, test := range tests { t.Run(test.name, func(t *testing.T) { @@ -523,7 +522,7 @@ func Test_Parse(t *testing.T) { grant, err := Parse(scope, test.input, WithUserId(test.userId), WithAccountId(test.accountId)) if test.err != "" { require.Error(err) - assert.True(strings.Contains(err.Error(), test.err), err.Error()) + assert.Equal(test.err, err.Error()) } else { require.NoError(err) assert.Equal(test.expected, grant)