fix: corrrectly return 4XX errors instead of 5XX errors (#24519)

authorization/error.go

@@ -1,6 +1,7 @@
 package authorization
 
 import (
+	errors2 "errors"
 	"fmt"
 
 	"github.com/influxdata/influxdb/v2/kit/platform/errors"
@@ -49,18 +50,18 @@ func ErrInvalidAuthIDError(err error) *errors.Error {
 	}
 }
 
-// ErrInternalServiceError is used when the error comes from an internal system.
-func ErrInternalServiceError(err error) *errors.Error {
-	return &errors.Error{
-		Code: errors.EInternal,
-		Err:  err,
-	}
-}
-
 // UnexpectedAuthIndexError is used when the error comes from an internal system.
 func UnexpectedAuthIndexError(err error) *errors.Error {
-	return &errors.Error{
-		Code: errors.EInternal,
-		Msg:  fmt.Sprintf("unexpected error retrieving auth index; Err: %v", err),
+	var e *errors.Error
+	if !errors2.As(err, &e) {
+		e = &errors.Error{
+			Msg:  fmt.Sprintf("unexpected error retrieving auth index; Err: %v", err),
+			Code: errors.EInternal,
+			Err:  err,
+		}
+	}
+	if e.Code == "" {
+		e.Code = errors.EInternal
 	}
+	return e
 }

authorization/storage_authorization.go

@@ -52,7 +52,10 @@ func decodeAuthorization(b []byte, a *influxdb.Authorization) error {
 
 // CreateAuthorization takes an Authorization object and saves it in storage using its token
 // using its token property as an index
-func (s *Store) CreateAuthorization(ctx context.Context, tx kv.Tx, a *influxdb.Authorization) error {
+func (s *Store) CreateAuthorization(ctx context.Context, tx kv.Tx, a *influxdb.Authorization) (retErr error) {
+	defer func() {
+		retErr = errors.ErrInternalServiceError(retErr, errors.WithErrorOp(influxdb.OpCreateAuthorization))
+	}()
 	// if the provided ID is invalid, or already maps to an existing Auth, then generate a new one
 	if !a.ID.Valid() {
 		id, err := s.generateSafeID(ctx, tx, authBucket)
@@ -91,10 +94,7 @@ func (s *Store) CreateAuthorization(ctx context.Context, tx kv.Tx, a *influxdb.A
 	}
 
 	if err := idx.Put(authIndexKey(a.Token), encodedID); err != nil {
-		return &errors.Error{
-			Code: errors.EInternal,
-			Err:  err,
-		}
+		return err
 	}
 
 	b, err := tx.Bucket(authBucket)
@@ -103,24 +103,25 @@ func (s *Store) CreateAuthorization(ctx context.Context, tx kv.Tx, a *influxdb.A
 	}
 
 	if err := b.Put(encodedID, v); err != nil {
-		return &errors.Error{
-			Err: err,
-		}
+		return err
 	}
 
 	return nil
 }
 
 // GetAuthorization gets an authorization by its ID from the auth bucket in kv
-func (s *Store) GetAuthorizationByID(ctx context.Context, tx kv.Tx, id platform.ID) (*influxdb.Authorization, error) {
+func (s *Store) GetAuthorizationByID(ctx context.Context, tx kv.Tx, id platform.ID) (auth *influxdb.Authorization, retErr error) {
+	defer func() {
+		retErr = errors.ErrInternalServiceError(retErr, errors.WithErrorOp(influxdb.OpFindAuthorizationByID))
+	}()
 	encodedID, err := id.Encode()
 	if err != nil {
 		return nil, ErrInvalidAuthID
 	}
 
 	b, err := tx.Bucket(authBucket)
 	if err != nil {
-		return nil, ErrInternalServiceError(err)
+		return nil, err
 	}
 
 	v, err := b.Get(encodedID)
@@ -129,21 +130,21 @@ func (s *Store) GetAuthorizationByID(ctx context.Context, tx kv.Tx, id platform.
 	}
 
 	if err != nil {
-		return nil, ErrInternalServiceError(err)
+		return nil, err
 	}
 
 	a := &influxdb.Authorization{}
 	if err := decodeAuthorization(v, a); err != nil {
-		return nil, &errors.Error{
-			Code: errors.EInvalid,
-			Err:  err,
-		}
+		return nil, err
 	}
 
 	return a, nil
 }
 
-func (s *Store) GetAuthorizationByToken(ctx context.Context, tx kv.Tx, token string) (*influxdb.Authorization, error) {
+func (s *Store) GetAuthorizationByToken(ctx context.Context, tx kv.Tx, token string) (auth *influxdb.Authorization, retErr error) {
+	defer func() {
+		retErr = errors.ErrInternalServiceError(retErr, errors.WithErrorOp(influxdb.OpFindAuthorizationByToken))
+	}()
 	idx, err := authIndexBucket(tx)
 	if err != nil {
 		return nil, err
@@ -171,7 +172,10 @@ func (s *Store) GetAuthorizationByToken(ctx context.Context, tx kv.Tx, token str
 
 // ListAuthorizations returns all the authorizations matching a set of FindOptions. This function is used for
 // FindAuthorizationByID, FindAuthorizationByToken, and FindAuthorizations in the AuthorizationService implementation
-func (s *Store) ListAuthorizations(ctx context.Context, tx kv.Tx, f influxdb.AuthorizationFilter) ([]*influxdb.Authorization, error) {
+func (s *Store) ListAuthorizations(ctx context.Context, tx kv.Tx, f influxdb.AuthorizationFilter) (auths []*influxdb.Authorization, retErr error) {
+	defer func() {
+		retErr = errors.ErrInternalServiceError(retErr, errors.WithErrorOp(influxdb.OpFindAuthorizations))
+	}()
 	var as []*influxdb.Authorization
 	pred := authorizationsPredicateFn(f)
 	filterFn := filterAuthorizationsFn(f)
@@ -223,21 +227,18 @@ func (s *Store) forEachAuthorization(ctx context.Context, tx kv.Tx, pred kv.Curs
 }
 
 // UpdateAuthorization updates the status and description only of an authorization
-func (s *Store) UpdateAuthorization(ctx context.Context, tx kv.Tx, id platform.ID, a *influxdb.Authorization) (*influxdb.Authorization, error) {
+func (s *Store) UpdateAuthorization(ctx context.Context, tx kv.Tx, id platform.ID, a *influxdb.Authorization) (auth *influxdb.Authorization, retErr error) {
+	defer func() {
+		retErr = errors.ErrInternalServiceError(retErr, errors.WithErrorOp(influxdb.OpUpdateAuthorization))
+	}()
 	v, err := encodeAuthorization(a)
 	if err != nil {
-		return nil, &errors.Error{
-			Code: errors.EInvalid,
-			Err:  err,
-		}
+		return nil, errors.ErrInternalServiceError(err, errors.WithErrorCode(errors.EInvalid))
 	}
 
 	encodedID, err := a.ID.Encode()
 	if err != nil {
-		return nil, &errors.Error{
-			Code: errors.ENotFound,
-			Err:  err,
-		}
+		return nil, errors.ErrInternalServiceError(err, errors.WithErrorCode(errors.ENotFound))
 	}
 
 	idx, err := authIndexBucket(tx)
@@ -246,10 +247,7 @@ func (s *Store) UpdateAuthorization(ctx context.Context, tx kv.Tx, id platform.I
 	}
 
 	if err := idx.Put(authIndexKey(a.Token), encodedID); err != nil {
-		return nil, &errors.Error{
-			Code: errors.EInternal,
-			Err:  err,
-		}
+		return nil, err
 	}
 
 	b, err := tx.Bucket(authBucket)
@@ -258,17 +256,18 @@ func (s *Store) UpdateAuthorization(ctx context.Context, tx kv.Tx, id platform.I
 	}
 
 	if err := b.Put(encodedID, v); err != nil {
-		return nil, &errors.Error{
-			Err: err,
-		}
+		return nil, err
 	}
 
 	return a, nil
 
 }
 
 // DeleteAuthorization removes an authorization from storage
-func (s *Store) DeleteAuthorization(ctx context.Context, tx kv.Tx, id platform.ID) error {
+func (s *Store) DeleteAuthorization(ctx context.Context, tx kv.Tx, id platform.ID) (retErr error) {
+	defer func() {
+		retErr = errors.ErrInternalServiceError(retErr, errors.WithErrorOp(influxdb.OpDeleteAuthorization))
+	}()
 	a, err := s.GetAuthorizationByID(ctx, tx, id)
 	if err != nil {
 		return err
@@ -290,11 +289,11 @@ func (s *Store) DeleteAuthorization(ctx context.Context, tx kv.Tx, id platform.I
 	}
 
 	if err := idx.Delete([]byte(a.Token)); err != nil {
-		return ErrInternalServiceError(err)
+		return err
 	}
 
 	if err := b.Delete(encodedID); err != nil {
-		return ErrInternalServiceError(err)
+		return err
 	}
 
 	return nil
@@ -342,7 +341,7 @@ func uniqueID(ctx context.Context, tx kv.Tx, id platform.ID) error {
 
 	b, err := tx.Bucket(authBucket)
 	if err != nil {
-		return ErrInternalServiceError(err)
+		return errors.ErrInternalServiceError(err)
 	}
 
 	_, err = b.Get(encodedID)

bolt/kv.go

@@ -11,6 +11,7 @@ import (
 	"sync"
 	"time"
 
+	errors2 "github.com/influxdata/influxdb/v2/kit/platform/errors"
 	"github.com/influxdata/influxdb/v2/kit/tracing"
 	"github.com/influxdata/influxdb/v2/kv"
 	"github.com/influxdata/influxdb/v2/kv/migration"
@@ -161,33 +162,35 @@ func (s *KVStore) View(ctx context.Context, fn func(tx kv.Tx) error) error {
 	span, ctx := tracing.StartSpanFromContext(ctx)
 	defer span.Finish()
 
-	return s.DB().View(func(tx *bolt.Tx) error {
+	err := s.DB().View(func(tx *bolt.Tx) error {
 		return fn(&Tx{
 			tx:  tx,
 			ctx: ctx,
 		})
 	})
+	return errors2.BoltToInfluxError(err)
 }
 
 // Update opens up an update transaction against the store.
 func (s *KVStore) Update(ctx context.Context, fn func(tx kv.Tx) error) error {
 	span, ctx := tracing.StartSpanFromContext(ctx)
 	defer span.Finish()
 
-	return s.DB().Update(func(tx *bolt.Tx) error {
+	err := s.DB().Update(func(tx *bolt.Tx) error {
 		return fn(&Tx{
 			tx:  tx,
 			ctx: ctx,
 		})
 	})
+	return errors2.BoltToInfluxError(err)
 }
 
 // CreateBucket creates a bucket in the underlying boltdb store if it
 // does not already exist
 func (s *KVStore) CreateBucket(ctx context.Context, name []byte) error {
 	return s.DB().Update(func(tx *bolt.Tx) error {
 		_, err := tx.CreateBucketIfNotExists(name)
-		return err
+		return errors2.BoltToInfluxError(err)
 	})
 }
 
@@ -196,7 +199,7 @@ func (s *KVStore) CreateBucket(ctx context.Context, name []byte) error {
 func (s *KVStore) DeleteBucket(ctx context.Context, name []byte) error {
 	return s.DB().Update(func(tx *bolt.Tx) error {
 		if err := tx.DeleteBucket(name); err != nil && !errors.Is(err, bolt.ErrBucketNotFound) {
-			return err
+			return errors2.BoltToInfluxError(err)
 		}
 
 		return nil
@@ -210,7 +213,7 @@ func (s *KVStore) Backup(ctx context.Context, w io.Writer) error {
 
 	return s.DB().View(func(tx *bolt.Tx) error {
 		_, err := tx.WriteTo(w)
-		return err
+		return errors2.BoltToInfluxError(err)
 	})
 }
 
@@ -348,7 +351,7 @@ func (b *Bucket) Put(key []byte, value []byte) error {
 	if err == bolt.ErrTxNotWritable {
 		return kv.ErrTxNotWritable
 	}
-	return err
+	return errors2.BoltToInfluxError(err)
 }
 
 // Delete removes the provided key.
@@ -357,7 +360,7 @@ func (b *Bucket) Delete(key []byte) error {
 	if err == bolt.ErrTxNotWritable {
 		return kv.ErrTxNotWritable
 	}
-	return err
+	return errors2.BoltToInfluxError(err)
 }
 
 // ForwardCursor retrieves a cursor for iterating through the entries

bucket.go

@@ -2,12 +2,10 @@ package influxdb
 
 import (
 	"context"
-	"fmt"
 	"strings"
 	"time"
 
 	"github.com/influxdata/influxdb/v2/kit/platform"
-	"github.com/influxdata/influxdb/v2/kit/platform/errors"
 )
 
 const (
@@ -161,12 +159,3 @@ func (f BucketFilter) String() string {
 	}
 	return "[" + strings.Join(parts, ", ") + "]"
 }
-
-func ErrInternalBucketServiceError(op string, err error) *errors.Error {
-	return &errors.Error{
-		Code: errors.EInternal,
-		Msg:  fmt.Sprintf("unexpected error in buckets; Err: %v", err),
-		Op:   op,
-		Err:  err,
-	}
-}

checks/service_external_test.go

@@ -1030,7 +1030,7 @@ func FindChecks(
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			s, _, opPrefix, done := init(tt.fields, t)
+			s, _, _, done := init(tt.fields, t)
 			defer done()
 			ctx := context.Background()
 
@@ -1049,7 +1049,7 @@ func FindChecks(
 			}
 
 			checks, _, err := s.FindChecks(ctx, filter, tt.args.findOptions)
-			diffPlatformErrors(tt.name, err, tt.wants.err, opPrefix, t)
+			diffPlatformErrors(tt.name, err, tt.wants.err, t)
 
 			if diff := cmp.Diff(checks, tt.wants.checks, checkCmpOptions...); diff != "" {
 				t.Errorf("checks are different -got/+want\ndiff %s", diff)
@@ -1536,14 +1536,14 @@ data = from(bucket: "telegraf") |> range(start: -1m)`,
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			s, _, opPrefix, done := init(tt.fields, t)
+			s, _, _, done := init(tt.fields, t)
 			defer done()
 			ctx := context.Background()
 
 			checkCreate := influxdb.CheckCreate{Check: tt.args.check, Status: influxdb.Active}
 
 			check, err := s.UpdateCheck(ctx, tt.args.id, checkCreate)
-			diffPlatformErrors(tt.name, err, tt.wants.err, opPrefix, t)
+			diffPlatformErrors(tt.name, err, tt.wants.err, t)
 
 			if diff := cmp.Diff(check, tt.wants.check, checkCmpOptions...); diff != "" {
 				t.Errorf("check is different -got/+want\ndiff %s", diff)
@@ -1731,7 +1731,7 @@ func MustIDBase16(s string) platform.ID {
 	return *id
 }
 
-func diffPlatformErrors(name string, actual, expected error, opPrefix string, t *testing.T) {
+func diffPlatformErrors(name string, actual, expected error, t *testing.T) {
 	t.Helper()
 	ErrorsEqual(t, actual, expected)
 }