diff --git a/error/error.go b/error/error.go index 67551e4ee..6896cd602 100644 --- a/error/error.go +++ b/error/error.go @@ -46,6 +46,7 @@ var errors = map[int]string{ EcodeIndexOrValueRequired: "Index or value is required", EcodeIndexValueMutex: "Index and value cannot both be specified", EcodeInvalidField: "Invalid field", + EcodeInvalidForm: "Invalid POST form", // raft related errors EcodeRaftInternal: "Raft Internal Error", @@ -84,6 +85,7 @@ const ( EcodeIndexOrValueRequired = 207 EcodeIndexValueMutex = 208 EcodeInvalidField = 209 + EcodeInvalidForm = 210 EcodeRaftInternal = 300 EcodeLeaderElect = 301 @@ -104,6 +106,10 @@ type Error struct { Index uint64 `json:"index"` } +func NewRequestError(errorCode int, cause string) *Error { + return NewError(errorCode, cause, 0) +} + func NewError(errorCode int, cause string, index uint64) *Error { return &Error{ ErrorCode: errorCode, diff --git a/etcdserver/etcdhttp/http.go b/etcdserver/etcdhttp/http.go index 10c9bb451..72f5993b7 100644 --- a/etcdserver/etcdhttp/http.go +++ b/etcdserver/etcdhttp/http.go @@ -180,13 +180,13 @@ func (h Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { func (h Handler) serveKeys(ctx context.Context, w http.ResponseWriter, r *http.Request) { rr, err := parseRequest(r, genId()) if err != nil { - http.Error(w, err.Error(), http.StatusBadRequest) + writeError(w, err) return } resp, err := h.Server.Do(ctx, rr) if err != nil { - writeInternalError(w, err) + writeError(w, err) return } @@ -200,7 +200,7 @@ func (h Handler) serveKeys(ctx context.Context, w http.ResponseWriter, r *http.R return } default: - writeInternalError(w, errors.New("received response with no Event/Watcher!")) + writeError(w, errors.New("received response with no Event/Watcher!")) return } @@ -257,11 +257,17 @@ func parseRequest(r *http.Request, id int64) (etcdserverpb.Request, error) { var err error if err = r.ParseForm(); err != nil { - return emptyReq, err + return emptyReq, etcdErr.NewRequestError( + etcdErr.EcodeInvalidForm, + err.Error(), + ) } if !strings.HasPrefix(r.URL.Path, keysPrefix) { - return emptyReq, errors.New("unexpected key prefix!") + return emptyReq, etcdErr.NewRequestError( + etcdErr.EcodeInvalidForm, + "incorrect key prefix", + ) } path := r.URL.Path[len(keysPrefix):] @@ -269,24 +275,39 @@ func parseRequest(r *http.Request, id int64) (etcdserverpb.Request, error) { var pIdx, wIdx, ttl uint64 if pIdx, err = parseUint64(q.Get("prevIndex")); err != nil { - return emptyReq, errors.New("invalid value for prevIndex") + return emptyReq, etcdErr.NewRequestError( + etcdErr.EcodeIndexNaN, + "invalid value for prevIndex", + ) } if wIdx, err = parseUint64(q.Get("waitIndex")); err != nil { - return emptyReq, errors.New("invalid value for waitIndex") + return emptyReq, etcdErr.NewRequestError( + etcdErr.EcodeIndexNaN, + "invalid value for waitIndex", + ) } if ttl, err = parseUint64(q.Get("ttl")); err != nil { - return emptyReq, errors.New("invalid value for ttl") + return emptyReq, etcdErr.NewRequestError( + etcdErr.EcodeTTLNaN, + "invalid value for ttl", + ) } var rec, sort, wait bool if rec, err = parseBool(q.Get("recursive")); err != nil { - return emptyReq, errors.New("invalid value for recursive") + return emptyReq, etcdErr.NewRequestError( + etcdErr.EcodeInvalidField, + "invalid value for recursive") } if sort, err = parseBool(q.Get("sorted")); err != nil { - return emptyReq, errors.New("invalid value for sorted") + return emptyReq, etcdErr.NewRequestError( + etcdErr.EcodeInvalidField, + "invalid value for sorted") } if wait, err = parseBool(q.Get("wait")); err != nil { - return emptyReq, errors.New("invalid value for wait") + return emptyReq, etcdErr.NewRequestError( + etcdErr.EcodeInvalidField, + "invalid value for wait") } rr := etcdserverpb.Request{ @@ -332,9 +353,10 @@ func parseUint64(s string) (uint64, error) { return strconv.ParseUint(s, 10, 64) } -// writeInternalError logs and writes the given Error to the ResponseWriter +// writeError logs and writes the given Error to the ResponseWriter // If Error is an etcdErr, it is rendered to the ResponseWriter -func writeInternalError(w http.ResponseWriter, err error) { +// Otherwise, it is assumed to be an InternalServerError +func writeError(w http.ResponseWriter, err error) { if err == nil { return } diff --git a/etcdserver/etcdhttp/http_test.go b/etcdserver/etcdhttp/http_test.go index 285d04ff3..b61c7d389 100644 --- a/etcdserver/etcdhttp/http_test.go +++ b/etcdserver/etcdhttp/http_test.go @@ -56,7 +56,8 @@ func TestParseUint64(t *testing.T) { func TestBadParseRequest(t *testing.T) { tests := []struct { - in *http.Request + in *http.Request + wcode int }{ { // parseForm failure @@ -64,36 +65,86 @@ func TestBadParseRequest(t *testing.T) { Body: nil, Method: "PUT", }, + etcdErr.EcodeInvalidForm, }, { // bad key prefix &http.Request{ URL: mustNewURL(t, "/badprefix/"), }, + etcdErr.EcodeInvalidForm, }, // bad values for prevIndex, waitIndex, ttl - {mustNewRequest(t, "?prevIndex=foo")}, - {mustNewRequest(t, "?prevIndex=1.5")}, - {mustNewRequest(t, "?prevIndex=-1")}, - {mustNewRequest(t, "?waitIndex=garbage")}, - {mustNewRequest(t, "?waitIndex=??")}, - {mustNewRequest(t, "?ttl=-1")}, + { + mustNewRequest(t, "?prevIndex=foo"), + etcdErr.EcodeIndexNaN, + }, + { + mustNewRequest(t, "?prevIndex=1.5"), + etcdErr.EcodeIndexNaN, + }, + { + mustNewRequest(t, "?prevIndex=-1"), + etcdErr.EcodeIndexNaN, + }, + { + mustNewRequest(t, "?waitIndex=garbage"), + etcdErr.EcodeIndexNaN, + }, + { + mustNewRequest(t, "?waitIndex=??"), + etcdErr.EcodeIndexNaN, + }, + { + mustNewRequest(t, "?ttl=-1"), + etcdErr.EcodeTTLNaN, + }, // bad values for recursive, sorted, wait - {mustNewRequest(t, "?recursive=hahaha")}, - {mustNewRequest(t, "?recursive=1234")}, - {mustNewRequest(t, "?recursive=?")}, - {mustNewRequest(t, "?sorted=hahaha")}, - {mustNewRequest(t, "?sorted=!!")}, - {mustNewRequest(t, "?wait=notreally")}, - {mustNewRequest(t, "?wait=what!")}, + { + mustNewRequest(t, "?recursive=hahaha"), + etcdErr.EcodeInvalidField, + }, + { + mustNewRequest(t, "?recursive=1234"), + etcdErr.EcodeInvalidField, + }, + { + mustNewRequest(t, "?recursive=?"), + etcdErr.EcodeInvalidField, + }, + { + mustNewRequest(t, "?sorted=hahaha"), + etcdErr.EcodeInvalidField, + }, + { + mustNewRequest(t, "?sorted=!!"), + etcdErr.EcodeInvalidField, + }, + { + mustNewRequest(t, "?wait=notreally"), + etcdErr.EcodeInvalidField, + }, + { + mustNewRequest(t, "?wait=what!"), + etcdErr.EcodeInvalidField, + }, } for i, tt := range tests { got, err := parseRequest(tt.in, 1234) if err == nil { - t.Errorf("case %d: unexpected nil error!", i) + t.Errorf("#%d: unexpected nil error!", i) + continue + } + ee, ok := err.(*etcdErr.Error) + if !ok { + t.Errorf("#%d: err is not etcd.Error!", i) + continue + } + if ee.ErrorCode != tt.wcode { + t.Errorf("#%d: code=%d, want %v", i, ee.ErrorCode, tt.wcode) } if !reflect.DeepEqual(got, etcdserverpb.Request{}) { - t.Errorf("case %d: unexpected non-empty Request: %#v", i, got) + t.Errorf("#%d: unexpected non-empty Request: %#v", i, got) } } } @@ -205,10 +256,10 @@ func (w *eventingWatcher) EventChan() chan *store.Event { func (w *eventingWatcher) Remove() {} -func TestWriteInternalError(t *testing.T) { +func TestWriteError(t *testing.T) { // nil error should not panic rw := httptest.NewRecorder() - writeInternalError(rw, nil) + writeError(rw, nil) h := rw.Header() if len(h) > 0 { t.Fatalf("unexpected non-empty headers: %#v", h) @@ -241,7 +292,7 @@ func TestWriteInternalError(t *testing.T) { for i, tt := range tests { rw := httptest.NewRecorder() - writeInternalError(rw, tt.err) + writeError(rw, tt.err) if code := rw.Code; code != tt.wcode { t.Errorf("#%d: code=%d, want %d", i, code, tt.wcode) }