Skip to content

Commit c169e98

Browse files
davidby-influxdavidby-influx
authored
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)
1 parent 66ebe36 commit c169e98

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

48 files changed

+561
-427
lines changed

‎authorization/error.go

Lines changed: 12 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package authorization
22

33
import (
4+
errors2 "errors"
45
"fmt"
56

67
"github.com/influxdata/influxdb/v2/kit/platform/errors"
@@ -49,18 +50,18 @@ func ErrInvalidAuthIDError(err error) *errors.Error {
4950
}
5051
}
5152

52-
// ErrInternalServiceError is used when the error comes from an internal system.
53-
func ErrInternalServiceError(err error) *errors.Error {
54-
return &errors.Error{
55-
Code: errors.EInternal,
56-
Err: err,
57-
}
58-
}
59-
6053
// UnexpectedAuthIndexError is used when the error comes from an internal system.
6154
func UnexpectedAuthIndexError(err error) *errors.Error {
62-
return &errors.Error{
63-
Code: errors.EInternal,
64-
Msg: fmt.Sprintf("unexpected error retrieving auth index; Err: %v", err),
55+
var e *errors.Error
56+
if !errors2.As(err, &e) {
57+
e = &errors.Error{
58+
Msg: fmt.Sprintf("unexpected error retrieving auth index; Err: %v", err),
59+
Code: errors.EInternal,
60+
Err: err,
61+
}
62+
}
63+
if e.Code == "" {
64+
e.Code = errors.EInternal
6565
}
66+
return e
6667
}

‎authorization/storage_authorization.go

Lines changed: 36 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,10 @@ func decodeAuthorization(b []byte, a *influxdb.Authorization) error {
5252

5353
// CreateAuthorization takes an Authorization object and saves it in storage using its token
5454
// using its token property as an index
55-
func (s *Store) CreateAuthorization(ctx context.Context, tx kv.Tx, a *influxdb.Authorization) error {
55+
func (s *Store) CreateAuthorization(ctx context.Context, tx kv.Tx, a *influxdb.Authorization) (retErr error) {
56+
defer func() {
57+
retErr = errors.ErrInternalServiceError(retErr, errors.WithErrorOp(influxdb.OpCreateAuthorization))
58+
}()
5659
// if the provided ID is invalid, or already maps to an existing Auth, then generate a new one
5760
if !a.ID.Valid() {
5861
id, err := s.generateSafeID(ctx, tx, authBucket)
@@ -91,10 +94,7 @@ func (s *Store) CreateAuthorization(ctx context.Context, tx kv.Tx, a *influxdb.A
9194
}
9295

9396
if err := idx.Put(authIndexKey(a.Token), encodedID); err != nil {
94-
return &errors.Error{
95-
Code: errors.EInternal,
96-
Err: err,
97-
}
97+
return err
9898
}
9999

100100
b, err := tx.Bucket(authBucket)
@@ -103,24 +103,25 @@ func (s *Store) CreateAuthorization(ctx context.Context, tx kv.Tx, a *influxdb.A
103103
}
104104

105105
if err := b.Put(encodedID, v); err != nil {
106-
return &errors.Error{
107-
Err: err,
108-
}
106+
return err
109107
}
110108

111109
return nil
112110
}
113111

114112
// GetAuthorization gets an authorization by its ID from the auth bucket in kv
115-
func (s *Store) GetAuthorizationByID(ctx context.Context, tx kv.Tx, id platform.ID) (*influxdb.Authorization, error) {
113+
func (s *Store) GetAuthorizationByID(ctx context.Context, tx kv.Tx, id platform.ID) (auth *influxdb.Authorization, retErr error) {
114+
defer func() {
115+
retErr = errors.ErrInternalServiceError(retErr, errors.WithErrorOp(influxdb.OpFindAuthorizationByID))
116+
}()
116117
encodedID, err := id.Encode()
117118
if err != nil {
118119
return nil, ErrInvalidAuthID
119120
}
120121

121122
b, err := tx.Bucket(authBucket)
122123
if err != nil {
123-
return nil, ErrInternalServiceError(err)
124+
return nil, err
124125
}
125126

126127
v, err := b.Get(encodedID)
@@ -129,21 +130,21 @@ func (s *Store) GetAuthorizationByID(ctx context.Context, tx kv.Tx, id platform.
129130
}
130131

131132
if err != nil {
132-
return nil, ErrInternalServiceError(err)
133+
return nil, err
133134
}
134135

135136
a := &influxdb.Authorization{}
136137
if err := decodeAuthorization(v, a); err != nil {
137-
return nil, &errors.Error{
138-
Code: errors.EInvalid,
139-
Err: err,
140-
}
138+
return nil, err
141139
}
142140

143141
return a, nil
144142
}
145143

146-
func (s *Store) GetAuthorizationByToken(ctx context.Context, tx kv.Tx, token string) (*influxdb.Authorization, error) {
144+
func (s *Store) GetAuthorizationByToken(ctx context.Context, tx kv.Tx, token string) (auth *influxdb.Authorization, retErr error) {
145+
defer func() {
146+
retErr = errors.ErrInternalServiceError(retErr, errors.WithErrorOp(influxdb.OpFindAuthorizationByToken))
147+
}()
147148
idx, err := authIndexBucket(tx)
148149
if err != nil {
149150
return nil, err
@@ -171,7 +172,10 @@ func (s *Store) GetAuthorizationByToken(ctx context.Context, tx kv.Tx, token str
171172

172173
// ListAuthorizations returns all the authorizations matching a set of FindOptions. This function is used for
173174
// FindAuthorizationByID, FindAuthorizationByToken, and FindAuthorizations in the AuthorizationService implementation
174-
func (s *Store) ListAuthorizations(ctx context.Context, tx kv.Tx, f influxdb.AuthorizationFilter) ([]*influxdb.Authorization, error) {
175+
func (s *Store) ListAuthorizations(ctx context.Context, tx kv.Tx, f influxdb.AuthorizationFilter) (auths []*influxdb.Authorization, retErr error) {
176+
defer func() {
177+
retErr = errors.ErrInternalServiceError(retErr, errors.WithErrorOp(influxdb.OpFindAuthorizations))
178+
}()
175179
var as []*influxdb.Authorization
176180
pred := authorizationsPredicateFn(f)
177181
filterFn := filterAuthorizationsFn(f)
@@ -223,21 +227,18 @@ func (s *Store) forEachAuthorization(ctx context.Context, tx kv.Tx, pred kv.Curs
223227
}
224228

225229
// UpdateAuthorization updates the status and description only of an authorization
226-
func (s *Store) UpdateAuthorization(ctx context.Context, tx kv.Tx, id platform.ID, a *influxdb.Authorization) (*influxdb.Authorization, error) {
230+
func (s *Store) UpdateAuthorization(ctx context.Context, tx kv.Tx, id platform.ID, a *influxdb.Authorization) (auth *influxdb.Authorization, retErr error) {
231+
defer func() {
232+
retErr = errors.ErrInternalServiceError(retErr, errors.WithErrorOp(influxdb.OpUpdateAuthorization))
233+
}()
227234
v, err := encodeAuthorization(a)
228235
if err != nil {
229-
return nil, &errors.Error{
230-
Code: errors.EInvalid,
231-
Err: err,
232-
}
236+
return nil, errors.ErrInternalServiceError(err, errors.WithErrorCode(errors.EInvalid))
233237
}
234238

235239
encodedID, err := a.ID.Encode()
236240
if err != nil {
237-
return nil, &errors.Error{
238-
Code: errors.ENotFound,
239-
Err: err,
240-
}
241+
return nil, errors.ErrInternalServiceError(err, errors.WithErrorCode(errors.ENotFound))
241242
}
242243

243244
idx, err := authIndexBucket(tx)
@@ -246,10 +247,7 @@ func (s *Store) UpdateAuthorization(ctx context.Context, tx kv.Tx, id platform.I
246247
}
247248

248249
if err := idx.Put(authIndexKey(a.Token), encodedID); err != nil {
249-
return nil, &errors.Error{
250-
Code: errors.EInternal,
251-
Err: err,
252-
}
250+
return nil, err
253251
}
254252

255253
b, err := tx.Bucket(authBucket)
@@ -258,17 +256,18 @@ func (s *Store) UpdateAuthorization(ctx context.Context, tx kv.Tx, id platform.I
258256
}
259257

260258
if err := b.Put(encodedID, v); err != nil {
261-
return nil, &errors.Error{
262-
Err: err,
263-
}
259+
return nil, err
264260
}
265261

266262
return a, nil
267263

268264
}
269265

270266
// DeleteAuthorization removes an authorization from storage
271-
func (s *Store) DeleteAuthorization(ctx context.Context, tx kv.Tx, id platform.ID) error {
267+
func (s *Store) DeleteAuthorization(ctx context.Context, tx kv.Tx, id platform.ID) (retErr error) {
268+
defer func() {
269+
retErr = errors.ErrInternalServiceError(retErr, errors.WithErrorOp(influxdb.OpDeleteAuthorization))
270+
}()
272271
a, err := s.GetAuthorizationByID(ctx, tx, id)
273272
if err != nil {
274273
return err
@@ -290,11 +289,11 @@ func (s *Store) DeleteAuthorization(ctx context.Context, tx kv.Tx, id platform.I
290289
}
291290

292291
if err := idx.Delete([]byte(a.Token)); err != nil {
293-
return ErrInternalServiceError(err)
292+
return err
294293
}
295294

296295
if err := b.Delete(encodedID); err != nil {
297-
return ErrInternalServiceError(err)
296+
return err
298297
}
299298

300299
return nil
@@ -342,7 +341,7 @@ func uniqueID(ctx context.Context, tx kv.Tx, id platform.ID) error {
342341

343342
b, err := tx.Bucket(authBucket)
344343
if err != nil {
345-
return ErrInternalServiceError(err)
344+
return errors.ErrInternalServiceError(err)
346345
}
347346

348347
_, err = b.Get(encodedID)

‎bolt/kv.go

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import (
1111
"sync"
1212
"time"
1313

14+
errors2 "github.com/influxdata/influxdb/v2/kit/platform/errors"
1415
"github.com/influxdata/influxdb/v2/kit/tracing"
1516
"github.com/influxdata/influxdb/v2/kv"
1617
"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 {
161162
span, ctx := tracing.StartSpanFromContext(ctx)
162163
defer span.Finish()
163164

164-
return s.DB().View(func(tx *bolt.Tx) error {
165+
err := s.DB().View(func(tx *bolt.Tx) error {
165166
return fn(&Tx{
166167
tx: tx,
167168
ctx: ctx,
168169
})
169170
})
171+
return errors2.BoltToInfluxError(err)
170172
}
171173

172174
// Update opens up an update transaction against the store.
173175
func (s *KVStore) Update(ctx context.Context, fn func(tx kv.Tx) error) error {
174176
span, ctx := tracing.StartSpanFromContext(ctx)
175177
defer span.Finish()
176178

177-
return s.DB().Update(func(tx *bolt.Tx) error {
179+
err := s.DB().Update(func(tx *bolt.Tx) error {
178180
return fn(&Tx{
179181
tx: tx,
180182
ctx: ctx,
181183
})
182184
})
185+
return errors2.BoltToInfluxError(err)
183186
}
184187

185188
// CreateBucket creates a bucket in the underlying boltdb store if it
186189
// does not already exist
187190
func (s *KVStore) CreateBucket(ctx context.Context, name []byte) error {
188191
return s.DB().Update(func(tx *bolt.Tx) error {
189192
_, err := tx.CreateBucketIfNotExists(name)
190-
return err
193+
return errors2.BoltToInfluxError(err)
191194
})
192195
}
193196

@@ -196,7 +199,7 @@ func (s *KVStore) CreateBucket(ctx context.Context, name []byte) error {
196199
func (s *KVStore) DeleteBucket(ctx context.Context, name []byte) error {
197200
return s.DB().Update(func(tx *bolt.Tx) error {
198201
if err := tx.DeleteBucket(name); err != nil && !errors.Is(err, bolt.ErrBucketNotFound) {
199-
return err
202+
return errors2.BoltToInfluxError(err)
200203
}
201204

202205
return nil
@@ -210,7 +213,7 @@ func (s *KVStore) Backup(ctx context.Context, w io.Writer) error {
210213

211214
return s.DB().View(func(tx *bolt.Tx) error {
212215
_, err := tx.WriteTo(w)
213-
return err
216+
return errors2.BoltToInfluxError(err)
214217
})
215218
}
216219

@@ -348,7 +351,7 @@ func (b *Bucket) Put(key []byte, value []byte) error {
348351
if err == bolt.ErrTxNotWritable {
349352
return kv.ErrTxNotWritable
350353
}
351-
return err
354+
return errors2.BoltToInfluxError(err)
352355
}
353356

354357
// Delete removes the provided key.
@@ -357,7 +360,7 @@ func (b *Bucket) Delete(key []byte) error {
357360
if err == bolt.ErrTxNotWritable {
358361
return kv.ErrTxNotWritable
359362
}
360-
return err
363+
return errors2.BoltToInfluxError(err)
361364
}
362365

363366
// ForwardCursor retrieves a cursor for iterating through the entries

‎bucket.go

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,10 @@ package influxdb
22

33
import (
44
"context"
5-
"fmt"
65
"strings"
76
"time"
87

98
"github.com/influxdata/influxdb/v2/kit/platform"
10-
"github.com/influxdata/influxdb/v2/kit/platform/errors"
119
)
1210

1311
const (
@@ -161,12 +159,3 @@ func (f BucketFilter) String() string {
161159
}
162160
return "[" + strings.Join(parts, ", ") + "]"
163161
}
164-
165-
func ErrInternalBucketServiceError(op string, err error) *errors.Error {
166-
return &errors.Error{
167-
Code: errors.EInternal,
168-
Msg: fmt.Sprintf("unexpected error in buckets; Err: %v", err),
169-
Op: op,
170-
Err: err,
171-
}
172-
}

‎checks/service_external_test.go

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1030,7 +1030,7 @@ func FindChecks(
10301030

10311031
for _, tt := range tests {
10321032
t.Run(tt.name, func(t *testing.T) {
1033-
s, _, opPrefix, done := init(tt.fields, t)
1033+
s, _, _, done := init(tt.fields, t)
10341034
defer done()
10351035
ctx := context.Background()
10361036

@@ -1049,7 +1049,7 @@ func FindChecks(
10491049
}
10501050

10511051
checks, _, err := s.FindChecks(ctx, filter, tt.args.findOptions)
1052-
diffPlatformErrors(tt.name, err, tt.wants.err, opPrefix, t)
1052+
diffPlatformErrors(tt.name, err, tt.wants.err, t)
10531053

10541054
if diff := cmp.Diff(checks, tt.wants.checks, checkCmpOptions...); diff != "" {
10551055
t.Errorf("checks are different -got/+want\ndiff %s", diff)
@@ -1536,14 +1536,14 @@ data = from(bucket: "telegraf") |> range(start: -1m)`,
15361536

15371537
for _, tt := range tests {
15381538
t.Run(tt.name, func(t *testing.T) {
1539-
s, _, opPrefix, done := init(tt.fields, t)
1539+
s, _, _, done := init(tt.fields, t)
15401540
defer done()
15411541
ctx := context.Background()
15421542

15431543
checkCreate := influxdb.CheckCreate{Check: tt.args.check, Status: influxdb.Active}
15441544

15451545
check, err := s.UpdateCheck(ctx, tt.args.id, checkCreate)
1546-
diffPlatformErrors(tt.name, err, tt.wants.err, opPrefix, t)
1546+
diffPlatformErrors(tt.name, err, tt.wants.err, t)
15471547

15481548
if diff := cmp.Diff(check, tt.wants.check, checkCmpOptions...); diff != "" {
15491549
t.Errorf("check is different -got/+want\ndiff %s", diff)
@@ -1731,7 +1731,7 @@ func MustIDBase16(s string) platform.ID {
17311731
return *id
17321732
}
17331733

1734-
func diffPlatformErrors(name string, actual, expected error, opPrefix string, t *testing.T) {
1734+
func diffPlatformErrors(name string, actual, expected error, t *testing.T) {
17351735
t.Helper()
17361736
ErrorsEqual(t, actual, expected)
17371737
}

0 commit comments

Comments
 (0)