From 847a3ea0a20e61b722da51f6e0b6b9d66cf03330 Mon Sep 17 00:00:00 2001 From: Aditya Manthramurthy Date: Thu, 29 Aug 2019 13:53:27 -0700 Subject: [PATCH] Add unit tests and refactor to improve coverage (#7617) --- cmd/admin-handlers.go | 73 +++++++++++------- cmd/admin-handlers_test.go | 149 +++++++++++++++++++++++++++++++++++++ 2 files changed, 195 insertions(+), 27 deletions(-) diff --git a/cmd/admin-handlers.go b/cmd/admin-handlers.go index a1eefc571..2decb52cb 100644 --- a/cmd/admin-handlers.go +++ b/cmd/admin-handlers.go @@ -21,6 +21,7 @@ import ( "context" "encoding/base64" "encoding/json" + "errors" "fmt" "io" "io/ioutil" @@ -476,7 +477,6 @@ func newLockEntry(l lockRequesterInfo, resource, server string) *madmin.LockEntr } func topLockEntries(peerLocks []*PeerLocks) madmin.LockEntries { - const listCount int = 10 entryMap := make(map[string]*madmin.LockEntry) for _, peerLock := range peerLocks { if peerLock == nil { @@ -497,6 +497,7 @@ func topLockEntries(peerLocks []*PeerLocks) madmin.LockEntries { lockEntries = append(lockEntries, *v) } sort.Sort(lockEntries) + const listCount int = 10 if len(lockEntries) > listCount { lockEntries = lockEntries[:listCount] } @@ -654,45 +655,60 @@ func (a adminAPIHandlers) DownloadProfilingHandler(w http.ResponseWriter, r *htt } } -// extractHealInitParams - Validates params for heal init API. -func extractHealInitParams(r *http.Request) (bucket, objPrefix string, - hs madmin.HealOpts, clientToken string, forceStart bool, forceStop bool, - err APIErrorCode) { +type healInitParams struct { + bucket, objPrefix string + hs madmin.HealOpts + clientToken string + forceStart, forceStop bool +} - vars := mux.Vars(r) - bucket = vars[string(mgmtBucket)] - objPrefix = vars[string(mgmtPrefix)] +// extractHealInitParams - Validates params for heal init API. +func extractHealInitParams(vars map[string]string, qParms url.Values, r io.Reader) (hip healInitParams, err APIErrorCode) { + hip.bucket = vars[string(mgmtBucket)] + hip.objPrefix = vars[string(mgmtPrefix)] - if bucket == "" { - if objPrefix != "" { + if hip.bucket == "" { + if hip.objPrefix != "" { // Bucket is required if object-prefix is given err = ErrHealMissingBucket return } - } else if isReservedOrInvalidBucket(bucket, false) { + } else if isReservedOrInvalidBucket(hip.bucket, false) { err = ErrInvalidBucketName return } // empty prefix is valid. - if !IsValidObjectPrefix(objPrefix) { + if !IsValidObjectPrefix(hip.objPrefix) { err = ErrInvalidObjectName return } - qParms := r.URL.Query() if len(qParms[string(mgmtClientToken)]) > 0 { - clientToken = qParms[string(mgmtClientToken)][0] + hip.clientToken = qParms[string(mgmtClientToken)][0] } if _, ok := qParms[string(mgmtForceStart)]; ok { - forceStart = true + hip.forceStart = true } if _, ok := qParms[string(mgmtForceStop)]; ok { - forceStop = true + hip.forceStop = true } + + // Invalid request conditions: + // + // Cannot have both forceStart and forceStop in the same + // request; If clientToken is provided, request can only be + // to continue receiving logs, so it cannot be start or + // stop; + if (hip.forceStart && hip.forceStop) || + (hip.clientToken != "" && (hip.forceStart || hip.forceStop)) { + err = ErrInvalidRequest + return + } + // ignore body if clientToken is provided - if clientToken == "" { - jerr := json.NewDecoder(r.Body).Decode(&hs) + if hip.clientToken == "" { + jerr := json.NewDecoder(r).Decode(&hip.hs) if jerr != nil { logger.LogIf(context.Background(), jerr) err = ErrRequestBodyParse @@ -731,7 +747,7 @@ func (a adminAPIHandlers) HealHandler(w http.ResponseWriter, r *http.Request) { return } - bucket, objPrefix, hs, clientToken, forceStart, forceStop, errCode := extractHealInitParams(r) + hip, errCode := extractHealInitParams(mux.Vars(r), r.URL.Query(), r.Body) if errCode != ErrNone { writeErrorResponseJSON(ctx, w, errorCodes.ToAPIErr(errCode), r.URL) return @@ -805,8 +821,8 @@ func (a adminAPIHandlers) HealHandler(w http.ResponseWriter, r *http.Request) { info := objectAPI.StorageInfo(ctx) numDisks := info.Backend.OfflineDisks + info.Backend.OnlineDisks - healPath := pathJoin(bucket, objPrefix) - if clientToken == "" && !forceStart && !forceStop { + healPath := pathJoin(hip.bucket, hip.objPrefix) + if hip.clientToken == "" && !hip.forceStart && !hip.forceStop { nh, exists := globalAllHealState.getHealSequence(healPath) if exists && !nh.hasEnded() && len(nh.currentStatus.Items) > 0 { b, err := json.Marshal(madmin.HealStartSuccess{ @@ -825,11 +841,11 @@ func (a adminAPIHandlers) HealHandler(w http.ResponseWriter, r *http.Request) { } } - if clientToken != "" && !forceStart && !forceStop { + if hip.clientToken != "" && !hip.forceStart && !hip.forceStop { // Since clientToken is given, fetch heal status from running // heal sequence. respBytes, errCode := globalAllHealState.PopHealStatusJSON( - healPath, clientToken) + healPath, hip.clientToken) if errCode != ErrNone { writeErrorResponseJSON(ctx, w, errorCodes.ToAPIErr(errCode), r.URL) } else { @@ -840,14 +856,14 @@ func (a adminAPIHandlers) HealHandler(w http.ResponseWriter, r *http.Request) { respCh := make(chan healResp) switch { - case forceStop: + case hip.forceStop: go func() { respBytes, apiErr := globalAllHealState.stopHealSequence(healPath) hr := healResp{respBytes: respBytes, apiErr: apiErr} respCh <- hr }() - case clientToken == "": - nh := newHealSequence(bucket, objPrefix, handlers.GetSourceIP(r), numDisks, hs, forceStart) + case hip.clientToken == "": + nh := newHealSequence(hip.bucket, hip.objPrefix, handlers.GetSourceIP(r), numDisks, hip.hs, hip.forceStart) go func() { respBytes, apiErr, errMsg := globalAllHealState.LaunchNewHealSequence(nh) hr := healResp{respBytes, apiErr, errMsg} @@ -1609,7 +1625,10 @@ func convertValueType(elem []byte, jsonType gjson.Type) (interface{}, error) { case gjson.False, gjson.True: return strconv.ParseBool(str) case gjson.JSON: - return gjson.Parse(str).Value(), nil + if gjson.Valid(str) { + return gjson.Parse(str).Value(), nil + } + return nil, errors.New("invalid json") case gjson.String: return str, nil case gjson.Number: diff --git a/cmd/admin-handlers_test.go b/cmd/admin-handlers_test.go index 2beb16597..531d9503a 100644 --- a/cmd/admin-handlers_test.go +++ b/cmd/admin-handlers_test.go @@ -25,13 +25,16 @@ import ( "net/http" "net/http/httptest" "net/url" + "reflect" "strings" "sync" "testing" + "time" "github.com/gorilla/mux" "github.com/minio/minio/pkg/auth" "github.com/minio/minio/pkg/madmin" + "github.com/tidwall/gjson" ) var ( @@ -650,3 +653,149 @@ func TestToAdminAPIErrCode(t *testing.T) { } } } + +func TestTopLockEntries(t *testing.T) { + t1 := UTCNow() + t2 := UTCNow().Add(10 * time.Second) + peerLocks := []*PeerLocks{ + { + Addr: "1", + Locks: map[string][]lockRequesterInfo{ + "1": { + {false, "node2", "ep2", "2", t2, t2, ""}, + {true, "node1", "ep1", "1", t1, t1, ""}, + }, + "2": { + {false, "node2", "ep2", "2", t2, t2, ""}, + {true, "node1", "ep1", "1", t1, t1, ""}, + }, + }, + }, + { + Addr: "2", + Locks: map[string][]lockRequesterInfo{ + "1": { + {false, "node2", "ep2", "2", t2, t2, ""}, + {true, "node1", "ep1", "1", t1, t1, ""}, + }, + "2": { + {false, "node2", "ep2", "2", t2, t2, ""}, + {true, "node1", "ep1", "1", t1, t1, ""}, + }, + }, + }, + } + les := topLockEntries(peerLocks) + if len(les) != 2 { + t.Fatalf("Did not get 2 results") + } + if les[0].Timestamp.After(les[1].Timestamp) { + t.Fatalf("Got wrong sorted value") + } +} + +func TestExtractHealInitParams(t *testing.T) { + mkParams := func(clientToken string, forceStart, forceStop bool) url.Values { + v := url.Values{} + if clientToken != "" { + v.Add(string(mgmtClientToken), clientToken) + } + if forceStart { + v.Add(string(mgmtForceStart), "") + } + if forceStop { + v.Add(string(mgmtForceStop), "") + } + return v + } + qParmsArr := []url.Values{ + // Invalid cases + mkParams("", true, true), + mkParams("111", true, true), + mkParams("111", true, false), + mkParams("111", false, true), + // Valid cases follow + mkParams("", true, false), + mkParams("", false, true), + mkParams("", false, false), + mkParams("111", false, false), + } + varsArr := []map[string]string{ + // Invalid cases + {string(mgmtPrefix): "objprefix"}, + // Valid cases + {}, + {string(mgmtBucket): "bucket"}, + {string(mgmtBucket): "bucket", string(mgmtPrefix): "objprefix"}, + } + + // Body is always valid - we do not test JSON decoding. + body := `{"recursive": false, "dryRun": true, "remove": false, "scanMode": 0}` + + // Test all combinations! + for pIdx, parms := range qParmsArr { + for vIdx, vars := range varsArr { + _, err := extractHealInitParams(vars, parms, bytes.NewBuffer([]byte(body))) + isErrCase := false + if pIdx < 4 || vIdx < 1 { + isErrCase = true + } + + if err != ErrNone && !isErrCase { + t.Errorf("Got unexpected error: %v %v %v", pIdx, vIdx, err) + } else if err == ErrNone && isErrCase { + t.Errorf("Got no error but expected one: %v %v", pIdx, vIdx) + } + } + } + +} + +func TestNormalizeJSONKey(t *testing.T) { + cases := []struct { + input string + correctResult string + }{ + {"notify.webhook.1", "notify.webhook.:1"}, + {"notify.webhook.ok", "notify.webhook.ok"}, + {"notify.webhook.1.2", "notify.webhook.:1.:2"}, + {"abc", "abc"}, + } + for i, tcase := range cases { + res := normalizeJSONKey(tcase.input) + if res != tcase.correctResult { + t.Errorf("Case %d: failed to match", i) + } + } +} + +func TestConvertValueType(t *testing.T) { + cases := []struct { + elem []byte + jt gjson.Type + correctResult interface{} + isError bool + }{ + {[]byte(""), gjson.Null, nil, false}, + {[]byte("t"), gjson.False, true, false}, + {[]byte("f"), gjson.False, false, false}, + {[]byte(`{"a": 1}`), gjson.JSON, map[string]interface{}{"a": float64(1)}, false}, + {[]byte(`abc`), gjson.String, "abc", false}, + {[]byte(`1`), gjson.Number, float64(1), false}, + {[]byte(`a1`), gjson.Number, nil, true}, + {[]byte(`{"A": `), gjson.JSON, nil, true}, + {[]byte(`2`), gjson.False, nil, true}, + } + for i, tc := range cases { + res, err := convertValueType(tc.elem, tc.jt) + if err != nil { + if !tc.isError { + t.Errorf("Case %d: got an error when none was expected", i) + } + } else if err == nil && tc.isError { + t.Errorf("Case %d: got no error though we expected one", i) + } else if !reflect.DeepEqual(res, tc.correctResult) { + t.Errorf("Case %d: result mismatch - got %#v", i, res) + } + } +}