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

)

HTTP 5XX errors were being returned incorrectly from
BoltDB errors that were actually bad requests, e.g.,
names that were too long for buckets, users, and
organizations. Map BoltDB errors to correct Influx
errors and return 4XX errors where appropriate. Also
add op codes to more errors

(cherry picked from commit a3fd489)

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)
 }