From 1504af93dd087b88ce2b6b9333df4d44e15b77fc Mon Sep 17 00:00:00 2001 From: Dan Moran Date: Wed, 23 Dec 2020 09:10:40 -0800 Subject: [PATCH] fix(dbrp): improve DBRP API validation errors --- dbrp/error.go | 51 +++++++++++++++-- dbrp/http_client_dbrp.go | 4 +- dbrp/http_server_dbrp.go | 58 ++++++++++++------- dbrp/http_server_dbrp_test.go | 104 +++++++++++++++++++--------------- dbrp/service.go | 4 +- 5 files changed, 147 insertions(+), 74 deletions(-) diff --git a/dbrp/error.go b/dbrp/error.go index dcc402161fb..cb20747a9df 100644 --- a/dbrp/error.go +++ b/dbrp/error.go @@ -1,16 +1,12 @@ package dbrp import ( + "fmt" + "github.com/influxdata/influxdb/v2" ) var ( - // ErrInvalidDBRPID is used when the ID of the DBRP cannot be encoded. - ErrInvalidDBRPID = &influxdb.Error{ - Code: influxdb.EInvalid, - Msg: "DBRP ID is invalid", - } - // ErrDBRPNotFound is used when the specified DBRP cannot be found. ErrDBRPNotFound = &influxdb.Error{ Code: influxdb.ENotFound, @@ -29,8 +25,51 @@ var ( Code: influxdb.EInternal, Msg: "unable to generate valid id", } + + ErrNoOrgProvided = &influxdb.Error{ + Code: influxdb.EInvalid, + Msg: "either 'org' or 'orgID' must be provided", + } ) +// ErrOrgNotFound returns a more informative error about a 404 on org name. +func ErrOrgNotFound(org string) error { + return &influxdb.Error{ + Code: influxdb.ENotFound, + Msg: fmt.Sprintf("invalid org %q", org), + Err: influxdb.ErrOrgNotFound, + } +} + +// ErrInvalidOrgID returns a more informative error about a failure +// to decode an organization ID. +func ErrInvalidOrgID(id string, err error) error { + return &influxdb.Error{ + Code: influxdb.EInvalid, + Msg: fmt.Sprintf("invalid org ID %q", id), + Err: err, + } +} + +// ErrInvalidBucketID returns a more informative error about a failure +// to decode a bucket ID. +func ErrInvalidBucketID(id string, err error) error { + return &influxdb.Error{ + Code: influxdb.EInvalid, + Msg: fmt.Sprintf("invalid bucket ID %q", id), + Err: err, + } +} + +// ErrInvalidDBRPID is used when the ID of the DBRP cannot be encoded. +func ErrInvalidDBRPID(id string, err error) error { + return &influxdb.Error{ + Code: influxdb.EInvalid, + Msg: fmt.Sprintf("invalid DBRP ID %q", id), + Err: err, + } +} + // ErrInvalidDBRP is used when a service was provided an invalid DBRP. func ErrInvalidDBRP(err error) *influxdb.Error { return &influxdb.Error{ diff --git a/dbrp/http_client_dbrp.go b/dbrp/http_client_dbrp.go index 884e8d8d3d7..56ce6ff8183 100644 --- a/dbrp/http_client_dbrp.go +++ b/dbrp/http_client_dbrp.go @@ -92,8 +92,8 @@ func (c *Client) Create(ctx context.Context, dbrp *influxdb.DBRPMappingV2) error Database: dbrp.Database, RetentionPolicy: dbrp.RetentionPolicy, Default: dbrp.Default, - OrganizationID: dbrp.OrganizationID, - BucketID: dbrp.BucketID, + OrganizationID: dbrp.OrganizationID.String(), + BucketID: dbrp.BucketID.String(), }, c.Prefix). DecodeJSON(&newDBRP). Do(ctx); err != nil { diff --git a/dbrp/http_server_dbrp.go b/dbrp/http_server_dbrp.go index f36dc090f22..a2c82912d9a 100644 --- a/dbrp/http_server_dbrp.go +++ b/dbrp/http_server_dbrp.go @@ -56,12 +56,21 @@ func NewHTTPHandler(log *zap.Logger, dbrpSvc influxdb.DBRPMappingServiceV2, orgS } type createDBRPRequest struct { - Database string `json:"database"` - RetentionPolicy string `json:"retention_policy"` - Default bool `json:"default"` - Org string `json:"org"` - OrganizationID influxdb.ID `json:"orgID"` - BucketID influxdb.ID `json:"bucketID"` + Database string `json:"database"` + RetentionPolicy string `json:"retention_policy"` + Default bool `json:"default"` + Org string `json:"org"` + // N.B. These are purposefully typed as string instead of + // influxdb.ID so we can provide more specific error messages. + // If they have the ID type, our JSON decoder will just return + // a generic "invalid ID" error without stating which ID is + // the problem. + // + // Ideally we'd fix the decoder so we could get more useful + // errors everywhere, but I'm worried about the impact of a + // system-wide change to our "invalid ID" error format. + OrganizationID string `json:"orgID"` + BucketID string `json:"bucketID"` } func (h *Handler) handlePostDBRP(w http.ResponseWriter, r *http.Request) { @@ -76,27 +85,38 @@ func (h *Handler) handlePostDBRP(w http.ResponseWriter, r *http.Request) { return } - if !req.OrganizationID.Valid() { + var orgID influxdb.ID + var bucketID influxdb.ID + + if req.OrganizationID == "" { if req.Org == "" { - h.api.Err(w, r, influxdb.ErrInvalidID) + h.api.Err(w, r, ErrNoOrgProvided) return } org, err := h.orgSvc.FindOrganization(r.Context(), influxdb.OrganizationFilter{ Name: &req.Org, }) if err != nil { - h.api.Err(w, r, influxdb.ErrOrgNotFound) + h.api.Err(w, r, ErrOrgNotFound(req.Org)) return } - req.OrganizationID = org.ID + orgID = org.ID + } else if err := orgID.DecodeFromString(req.OrganizationID); err != nil { + h.api.Err(w, r, ErrInvalidOrgID(req.OrganizationID, err)) + return + } + + if err := bucketID.DecodeFromString(req.BucketID); err != nil { + h.api.Err(w, r, ErrInvalidBucketID(req.BucketID, err)) + return } dbrp := &influxdb.DBRPMappingV2{ Database: req.Database, RetentionPolicy: req.RetentionPolicy, Default: req.Default, - OrganizationID: req.OrganizationID, - BucketID: req.BucketID, + OrganizationID: orgID, + BucketID: bucketID, } if err := h.dbrpSvc.Create(ctx, dbrp); err != nil { h.api.Err(w, r, err) @@ -294,12 +314,12 @@ func (h *Handler) getFilterFromHTTPRequest(r *http.Request) (f influxdb.DBRPMapp return f, nil } -func getIDFromHTTPRequest(r *http.Request, key string) (*influxdb.ID, error) { +func getIDFromHTTPRequest(r *http.Request, key string, onErr func(string, error) error) (*influxdb.ID, error) { var id influxdb.ID raw := r.URL.Query().Get(key) if raw != "" { if err := id.DecodeFromString(raw); err != nil { - return nil, influxdb.ErrInvalidID + return nil, onErr(raw, err) } } else { return nil, nil @@ -310,20 +330,20 @@ func getIDFromHTTPRequest(r *http.Request, key string) (*influxdb.ID, error) { // mustGetOrgIDFromHTTPRequest returns the org ID parameter from the request, falling // back to looking up the org ID by org name if the ID parameter is not present. func (h *Handler) mustGetOrgIDFromHTTPRequest(r *http.Request) (*influxdb.ID, error) { - orgID, err := getIDFromHTTPRequest(r, "orgID") + orgID, err := getIDFromHTTPRequest(r, "orgID", ErrInvalidOrgID) if err != nil { return nil, err } if orgID == nil { name := r.URL.Query().Get("org") if name == "" { - return nil, influxdb.ErrOrgNotFound + return nil, ErrNoOrgProvided } org, err := h.orgSvc.FindOrganization(r.Context(), influxdb.OrganizationFilter{ Name: &name, }) if err != nil { - return nil, influxdb.ErrOrgNotFound + return nil, ErrOrgNotFound(name) } orgID = &org.ID } @@ -331,9 +351,9 @@ func (h *Handler) mustGetOrgIDFromHTTPRequest(r *http.Request) (*influxdb.ID, er } func getDBRPIDFromHTTPRequest(r *http.Request) (*influxdb.ID, error) { - return getIDFromHTTPRequest(r, "id") + return getIDFromHTTPRequest(r, "id", ErrInvalidDBRPID) } func getBucketIDFromHTTPRequest(r *http.Request) (*influxdb.ID, error) { - return getIDFromHTTPRequest(r, "bucketID") + return getIDFromHTTPRequest(r, "bucketID", ErrInvalidBucketID) } diff --git a/dbrp/http_server_dbrp_test.go b/dbrp/http_server_dbrp_test.go index aaa24aa7cd4..45eda595fe3 100644 --- a/dbrp/http_server_dbrp_test.go +++ b/dbrp/http_server_dbrp_test.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "errors" + "github.com/stretchr/testify/assert" "io" "io/ioutil" "net/http" @@ -52,7 +53,7 @@ func initHttpService(t *testing.T) (influxdb.DBRPMappingServiceV2, *httptest.Ser func Test_handlePostDBRP(t *testing.T) { table := []struct { Name string - ExpectedErr *influxdb.Error + ExpectedErr error ExpectedDBRP *influxdb.DBRPMappingV2 Input io.Reader }{ @@ -82,6 +83,16 @@ func Test_handlePostDBRP(t *testing.T) { OrganizationID: influxdbtesting.MustIDBase16("059af7ed2a034000"), }, }, + { + Name: "Create with no orgID or org", + Input: strings.NewReader(`{ + "bucketID": "5555f7ed2a035555", + "database": "mydb", + "retention_policy": "autogen", + "default": false +}`), + ExpectedErr: dbrp.ErrNoOrgProvided, + }, { Name: "Create with invalid orgID", Input: strings.NewReader(`{ @@ -91,11 +102,7 @@ func Test_handlePostDBRP(t *testing.T) { "retention_policy": "autogen", "default": false }`), - ExpectedErr: &influxdb.Error{ - Code: influxdb.EInvalid, - Msg: "invalid json structure", - Err: influxdb.ErrInvalidID.Err, - }, + ExpectedErr: dbrp.ErrInvalidOrgID("invalid", influxdb.ErrInvalidIDLength), }, { Name: "Create with invalid org name", @@ -106,14 +113,25 @@ func Test_handlePostDBRP(t *testing.T) { "retention_policy": "autogen", "default": false }`), - ExpectedErr: influxdb.ErrOrgNotFound, + ExpectedErr: dbrp.ErrOrgNotFound("invalid"), + }, + { + Name: "Create with invalid bucket ID", + Input: strings.NewReader(`{ + "bucketID": "invalid", + "org": "org", + "database": "mydb", + "retention_policy": "autogen", + "default": false +}`), + ExpectedErr: dbrp.ErrInvalidBucketID("invalid", influxdb.ErrInvalidIDLength), }, } for _, tt := range table { t.Run(tt.Name, func(t *testing.T) { if tt.ExpectedErr != nil && tt.ExpectedDBRP != nil { - t.Error("one of those has to be set") + t.Fatal("one of `ExpectedErr` or `ExpectedDBRP` has to be set") } _, server, shutdown := initHttpService(t) defer shutdown() @@ -130,10 +148,12 @@ func Test_handlePostDBRP(t *testing.T) { if err != nil { t.Fatal(err) } - - if !strings.Contains(string(b), tt.ExpectedErr.Error()) { - t.Fatal(string(b)) + var actualErr influxdb.Error + if err := json.Unmarshal(b, &actualErr); err != nil { + t.Fatal(err) } + + assert.Equal(t, tt.ExpectedErr.Error(), actualErr.Error()) return } dbrp := &influxdb.DBRPMappingV2{} @@ -160,7 +180,7 @@ func Test_handleGetDBRPs(t *testing.T) { table := []struct { Name string QueryParams string - ExpectedErr *influxdb.Error + ExpectedErr error ExpectedDBRPs []influxdb.DBRPMappingV2 }{ { @@ -180,18 +200,12 @@ func Test_handleGetDBRPs(t *testing.T) { { Name: "invalid org", QueryParams: "orgID=invalid", - ExpectedErr: &influxdb.Error{ - Code: influxdb.EInvalid, - Msg: "invalid ID", - }, + ExpectedErr: dbrp.ErrInvalidOrgID("invalid", influxdb.ErrInvalidIDLength), }, { Name: "invalid bucket", QueryParams: "orgID=059af7ed2a034000&bucketID=invalid", - ExpectedErr: &influxdb.Error{ - Code: influxdb.EInvalid, - Msg: "invalid ID", - }, + ExpectedErr: dbrp.ErrInvalidBucketID("invalid", influxdb.ErrInvalidIDLength), }, { Name: "invalid default", @@ -204,7 +218,7 @@ func Test_handleGetDBRPs(t *testing.T) { { Name: "no org", QueryParams: "default=true&retention_police=lol", - ExpectedErr: influxdb.ErrOrgNotFound, + ExpectedErr: dbrp.ErrNoOrgProvided, }, { Name: "no match", @@ -276,10 +290,12 @@ func Test_handleGetDBRPs(t *testing.T) { if err != nil { t.Fatal(err) } - - if !strings.Contains(string(b), tt.ExpectedErr.Error()) { - t.Fatal(string(b)) + var actualErr influxdb.Error + if err := json.Unmarshal(b, &actualErr); err != nil { + t.Fatal(err) } + + assert.Equal(t, tt.ExpectedErr.Error(), actualErr.Error()) return } dbrps := struct { @@ -304,7 +320,7 @@ func Test_handleGetDBRPs(t *testing.T) { func Test_handlePatchDBRP(t *testing.T) { table := []struct { Name string - ExpectedErr *influxdb.Error + ExpectedErr error ExpectedDBRP *influxdb.DBRPMappingV2 URLSuffix string Input io.Reader @@ -347,10 +363,7 @@ func Test_handlePatchDBRP(t *testing.T) { Input: strings.NewReader(`{ "database": "updatedb" }`), - ExpectedErr: &influxdb.Error{ - Code: influxdb.EInvalid, - Msg: "invalid ID", - }, + ExpectedErr: dbrp.ErrInvalidOrgID("invalid", influxdb.ErrInvalidIDLength), }, { Name: "no org", @@ -358,7 +371,7 @@ func Test_handlePatchDBRP(t *testing.T) { Input: strings.NewReader(`{ "database": "updatedb" }`), - ExpectedErr: influxdb.ErrOrgNotFound, + ExpectedErr: dbrp.ErrNoOrgProvided, }, { Name: "not found", @@ -405,10 +418,12 @@ func Test_handlePatchDBRP(t *testing.T) { if err != nil { t.Fatal(err) } - - if !strings.Contains(string(b), tt.ExpectedErr.Error()) { - t.Fatal(string(b)) + var actualErr influxdb.Error + if err := json.Unmarshal(b, &actualErr); err != nil { + t.Fatal(err) } + + assert.Equal(t, tt.ExpectedErr.Error(), actualErr.Error()) return } dbrpResponse := struct { @@ -430,7 +445,7 @@ func Test_handleDeleteDBRP(t *testing.T) { table := []struct { Name string URLSuffix string - ExpectedErr *influxdb.Error + ExpectedErr error ExpectStillExists bool }{ { @@ -442,22 +457,19 @@ func Test_handleDeleteDBRP(t *testing.T) { URLSuffix: "/1111111111111111?org=org", }, { - Name: "invalid org", - URLSuffix: "/1111111111111111?orgID=invalid", - ExpectedErr: &influxdb.Error{ - Code: influxdb.EInvalid, - Msg: "invalid ID", - }, + Name: "invalid org", + URLSuffix: "/1111111111111111?orgID=invalid", + ExpectedErr: dbrp.ErrInvalidOrgID("invalid", influxdb.ErrInvalidIDLength), }, { Name: "invalid org name", URLSuffix: "/1111111111111111?org=invalid", - ExpectedErr: influxdb.ErrOrgNotFound, + ExpectedErr: dbrp.ErrOrgNotFound("invalid"), }, { Name: "no org", URLSuffix: "/1111111111111111", - ExpectedErr: influxdb.ErrOrgNotFound, + ExpectedErr: dbrp.ErrNoOrgProvided, }, { // Not found is not an error for Delete. @@ -499,10 +511,12 @@ func Test_handleDeleteDBRP(t *testing.T) { if err != nil { t.Fatal(err) } - - if !strings.Contains(string(b), tt.ExpectedErr.Error()) { - t.Fatal(string(b)) + var actualErr influxdb.Error + if err := json.Unmarshal(b, &actualErr); err != nil { + t.Fatal(err) } + + assert.Equal(t, tt.ExpectedErr.Error(), actualErr.Error()) return } diff --git a/dbrp/service.go b/dbrp/service.go index 5f1b7b9b7b7..4747423dc14 100644 --- a/dbrp/service.go +++ b/dbrp/service.go @@ -203,7 +203,7 @@ func (s *Service) isDBRPUnique(ctx context.Context, m influxdb.DBRPMappingV2) er func (s *Service) FindByID(ctx context.Context, orgID, id influxdb.ID) (*influxdb.DBRPMappingV2, error) { encodedID, err := id.Encode() if err != nil { - return nil, ErrInvalidDBRPID + return nil, ErrInvalidDBRPID(id.String(), err) } m := &influxdb.DBRPMappingV2{} @@ -363,7 +363,7 @@ func (s *Service) Create(ctx context.Context, dbrp *influxdb.DBRPMappingV2) erro encodedID, err := dbrp.ID.Encode() if err != nil { - return ErrInvalidDBRPID + return ErrInvalidDBRPID(dbrp.ID.String(), err) } // OrganizationID has been validated by Validate