From 054cdcd127673344aad1bca9388124113dfe52bc Mon Sep 17 00:00:00 2001 From: Gyuho Lee Date: Wed, 9 May 2018 11:14:45 -0700 Subject: [PATCH 01/15] tests/Dockerfile: add "mvdan.cc/unparam" Signed-off-by: Gyuho Lee --- tests/Dockerfile | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/Dockerfile b/tests/Dockerfile index 8693f32377f..e13d9d4a28e 100644 --- a/tests/Dockerfile +++ b/tests/Dockerfile @@ -34,6 +34,7 @@ RUN ln -s /lib64/libhunspell-1.6.so /lib64/libhunspell.so RUN go get -v -u -tags spell github.com/chzchzchz/goword \ && go get -v -u github.com/coreos/license-bill-of-materials \ && go get -v -u github.com/mdempsky/unconvert \ + && go get -v -u mvdan.cc/unparam \ && go get -v -u honnef.co/go/tools/cmd/gosimple \ && go get -v -u honnef.co/go/tools/cmd/unused \ && go get -v -u honnef.co/go/tools/cmd/staticcheck \ From 775e9d5ba638ed50dd47f5cb9d631a2b90e094a2 Mon Sep 17 00:00:00 2001 From: Gyuho Lee Date: Wed, 9 May 2018 11:25:15 -0700 Subject: [PATCH 02/15] test: add "unused_pass" Signed-off-by: Gyuho Lee --- test | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/test b/test index 224d1e88c52..1388d1582f1 100755 --- a/test +++ b/test @@ -479,6 +479,18 @@ function unused_pass { fi } +function unparam_pass { + if which unparam >/dev/null; then + unparamResult=$(unparam "${STATIC_ANALYSIS_PATHS[@]}" 2>&1 || true) + if [ -n "${unparamResult}" ]; then + echo -e "unparam checking failed:\\n${unparamResult}" + exit 255 + fi + else + echo "Skipping unparam..." + fi +} + function staticcheck_pass { if which staticcheck >/dev/null; then staticcheckResult=$(staticcheck "${STATIC_ANALYSIS_PATHS[@]}" 2>&1 || true) @@ -601,6 +613,7 @@ function fmt_pass { govet_shadow \ gosimple \ unused \ + unparam \ staticcheck \ unconvert \ ineffassign \ From 167c7114677c3f1d72cd80bc2fde2ce70ee66396 Mon Sep 17 00:00:00 2001 From: Gyuho Lee Date: Wed, 9 May 2018 13:17:55 -0700 Subject: [PATCH 03/15] etcdctl/ctlv3: fix fmt test warnings Signed-off-by: Gyuho Lee --- etcdctl/ctlv3/command/del_command.go | 4 ++-- etcdctl/ctlv3/command/get_command.go | 4 ++-- etcdctl/ctlv3/command/put_command.go | 4 ++-- etcdctl/ctlv3/command/txn_command.go | 10 ++++------ etcdctl/ctlv3/command/watch_command.go | 8 ++++---- 5 files changed, 14 insertions(+), 16 deletions(-) diff --git a/etcdctl/ctlv3/command/del_command.go b/etcdctl/ctlv3/command/del_command.go index bb0663024aa..00c93bf5b14 100644 --- a/etcdctl/ctlv3/command/del_command.go +++ b/etcdctl/ctlv3/command/del_command.go @@ -43,7 +43,7 @@ func NewDelCommand() *cobra.Command { // delCommandFunc executes the "del" command. func delCommandFunc(cmd *cobra.Command, args []string) { - key, opts := getDelOp(cmd, args) + key, opts := getDelOp(args) ctx, cancel := commandCtx(cmd) resp, err := mustClientFromCmd(cmd).Delete(ctx, key, opts...) cancel() @@ -53,7 +53,7 @@ func delCommandFunc(cmd *cobra.Command, args []string) { display.Del(*resp) } -func getDelOp(cmd *cobra.Command, args []string) (string, []clientv3.OpOption) { +func getDelOp(args []string) (string, []clientv3.OpOption) { if len(args) == 0 || len(args) > 2 { ExitWithError(ExitBadArgs, fmt.Errorf("del command needs one argument as key and an optional argument as range_end.")) } diff --git a/etcdctl/ctlv3/command/get_command.go b/etcdctl/ctlv3/command/get_command.go index 5f9c74f3c55..85848b57930 100644 --- a/etcdctl/ctlv3/command/get_command.go +++ b/etcdctl/ctlv3/command/get_command.go @@ -56,7 +56,7 @@ func NewGetCommand() *cobra.Command { // getCommandFunc executes the "get" command. func getCommandFunc(cmd *cobra.Command, args []string) { - key, opts := getGetOp(cmd, args) + key, opts := getGetOp(args) ctx, cancel := commandCtx(cmd) resp, err := mustClientFromCmd(cmd).Get(ctx, key, opts...) cancel() @@ -74,7 +74,7 @@ func getCommandFunc(cmd *cobra.Command, args []string) { display.Get(*resp) } -func getGetOp(cmd *cobra.Command, args []string) (string, []clientv3.OpOption) { +func getGetOp(args []string) (string, []clientv3.OpOption) { if len(args) == 0 { ExitWithError(ExitBadArgs, fmt.Errorf("range command needs arguments.")) } diff --git a/etcdctl/ctlv3/command/put_command.go b/etcdctl/ctlv3/command/put_command.go index a72f0c4dcf5..21db69bdaaa 100644 --- a/etcdctl/ctlv3/command/put_command.go +++ b/etcdctl/ctlv3/command/put_command.go @@ -65,7 +65,7 @@ will store the content of the file to . // putCommandFunc executes the "put" command. func putCommandFunc(cmd *cobra.Command, args []string) { - key, value, opts := getPutOp(cmd, args) + key, value, opts := getPutOp(args) ctx, cancel := commandCtx(cmd) resp, err := mustClientFromCmd(cmd).Put(ctx, key, value, opts...) @@ -76,7 +76,7 @@ func putCommandFunc(cmd *cobra.Command, args []string) { display.Put(*resp) } -func getPutOp(cmd *cobra.Command, args []string) (string, string, []clientv3.OpOption) { +func getPutOp(args []string) (string, string, []clientv3.OpOption) { if len(args) == 0 { ExitWithError(ExitBadArgs, fmt.Errorf("put command needs 1 argument and input from stdin or 2 arguments.")) } diff --git a/etcdctl/ctlv3/command/txn_command.go b/etcdctl/ctlv3/command/txn_command.go index eec1e0916c3..e18a7e9bd69 100644 --- a/etcdctl/ctlv3/command/txn_command.go +++ b/etcdctl/ctlv3/command/txn_command.go @@ -28,9 +28,7 @@ import ( "github.com/spf13/cobra" ) -var ( - txnInteractive bool -) +var txnInteractive bool // NewTxnCommand returns the cobra command for "txn". func NewTxnCommand() *cobra.Command { @@ -129,17 +127,17 @@ func parseRequestUnion(line string) (*clientv3.Op, error) { put := NewPutCommand() put.Run = func(cmd *cobra.Command, args []string) { - key, value, opts := getPutOp(cmd, args) + key, value, opts := getPutOp(args) opc <- clientv3.OpPut(key, value, opts...) } get := NewGetCommand() get.Run = func(cmd *cobra.Command, args []string) { - key, opts := getGetOp(cmd, args) + key, opts := getGetOp(args) opc <- clientv3.OpGet(key, opts...) } del := NewDelCommand() del.Run = func(cmd *cobra.Command, args []string) { - key, opts := getDelOp(cmd, args) + key, opts := getDelOp(args) opc <- clientv3.OpDelete(key, opts...) } cmds := &cobra.Command{SilenceErrors: true} diff --git a/etcdctl/ctlv3/command/watch_command.go b/etcdctl/ctlv3/command/watch_command.go index 1a2cc4213f5..15cc835becb 100644 --- a/etcdctl/ctlv3/command/watch_command.go +++ b/etcdctl/ctlv3/command/watch_command.go @@ -258,9 +258,9 @@ func parseWatchArgs(osArgs, commandArgs []string, envKey, envRange string, inter } flagset := NewWatchCommand().Flags() - if err := flagset.Parse(watchArgs); err != nil { + if perr := flagset.Parse(watchArgs); perr != nil { watchPrefix, watchRev, watchPrevKey = false, 0, false - return nil, nil, err + return nil, nil, perr } pArgs := flagset.Args() @@ -298,8 +298,8 @@ func parseWatchArgs(osArgs, commandArgs []string, envKey, envRange string, inter if interactive { flagset := NewWatchCommand().Flags() - if err := flagset.Parse(argsWithSep); err != nil { - return nil, nil, err + if perr := flagset.Parse(argsWithSep); perr != nil { + return nil, nil, perr } watchArgs = flagset.Args() From f091641e463bfa41fa5fe8361873827e7c1f9842 Mon Sep 17 00:00:00 2001 From: Gyuho Lee Date: Wed, 9 May 2018 13:18:13 -0700 Subject: [PATCH 04/15] etcdmain: remove shadowed error variable Signed-off-by: Gyuho Lee --- etcdmain/gateway.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/etcdmain/gateway.go b/etcdmain/gateway.go index b260ddc7d18..e107476ff8f 100644 --- a/etcdmain/gateway.go +++ b/etcdmain/gateway.go @@ -106,8 +106,8 @@ func startGateway(cmd *cobra.Command, args []string) { srvs.Endpoints = stripSchema(srvs.Endpoints) if len(srvs.SRVs) == 0 { for _, ep := range srvs.Endpoints { - h, p, err := net.SplitHostPort(ep) - if err != nil { + h, p, serr := net.SplitHostPort(ep) + if serr != nil { fmt.Printf("error parsing endpoint %q", ep) os.Exit(1) } From d7c4c226fe69acc31c735cf11410b8724ba892b4 Mon Sep 17 00:00:00 2001 From: Gyuho Lee Date: Wed, 9 May 2018 13:18:28 -0700 Subject: [PATCH 05/15] functional/tester: remove shadowed error variable Signed-off-by: Gyuho Lee --- functional/tester/cluster.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/functional/tester/cluster.go b/functional/tester/cluster.go index e0d19f769cb..f198be943d0 100644 --- a/functional/tester/cluster.go +++ b/functional/tester/cluster.go @@ -493,9 +493,9 @@ func (clus *Cluster) sendOpWithResp(idx int, op rpcpb.Operation) (*rpcpb.Respons m, secure := clus.Members[idx], false for _, cu := range m.Etcd.AdvertiseClientURLs { - u, err := url.Parse(cu) - if err != nil { - return nil, err + u, perr := url.Parse(cu) + if perr != nil { + return nil, perr } if u.Scheme == "https" { // TODO: handle unix secure = true From 8235982f6eaf41a7b827a579550d5d86c7f66c95 Mon Sep 17 00:00:00 2001 From: Gyuho Lee Date: Wed, 9 May 2018 13:41:41 -0700 Subject: [PATCH 06/15] etcdserver/api/v2http: remove unused parameters Signed-off-by: Gyuho Lee --- etcdserver/api/v2http/capability.go | 6 ++-- etcdserver/api/v2http/client.go | 2 +- etcdserver/api/v2http/client_auth.go | 10 +++---- etcdserver/api/v2http/client_auth_test.go | 36 +++++++++++------------ etcdserver/api/v2http/metrics.go | 3 +- 5 files changed, 28 insertions(+), 29 deletions(-) diff --git a/etcdserver/api/v2http/capability.go b/etcdserver/api/v2http/capability.go index fa0bcca5e84..ab484bc6b6a 100644 --- a/etcdserver/api/v2http/capability.go +++ b/etcdserver/api/v2http/capability.go @@ -22,10 +22,10 @@ import ( "github.com/coreos/etcd/etcdserver/api/v2http/httptypes" ) -func capabilityHandler(c api.Capability, fn func(http.ResponseWriter, *http.Request)) http.HandlerFunc { +func authCapabilityHandler(fn func(http.ResponseWriter, *http.Request)) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { - if !api.IsCapabilityEnabled(c) { - notCapable(w, r, c) + if !api.IsCapabilityEnabled(api.AuthCapability) { + notCapable(w, r, api.AuthCapability) return } fn(w, r) diff --git a/etcdserver/api/v2http/client.go b/etcdserver/api/v2http/client.go index e947686609f..153c6415f31 100644 --- a/etcdserver/api/v2http/client.go +++ b/etcdserver/api/v2http/client.go @@ -155,7 +155,7 @@ func (h *keysHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { plog.Errorf("error writing event (%v)", err) } } - reportRequestCompleted(rr, resp, startTime) + reportRequestCompleted(rr, startTime) case resp.Watcher != nil: ctx, cancel := context.WithTimeout(context.Background(), defaultWatchTimeout) defer cancel() diff --git a/etcdserver/api/v2http/client_auth.go b/etcdserver/api/v2http/client_auth.go index 5316d5bf4d5..edf4c9ebbc9 100644 --- a/etcdserver/api/v2http/client_auth.go +++ b/etcdserver/api/v2http/client_auth.go @@ -228,11 +228,11 @@ func writeNoAuth(lg *zap.Logger, w http.ResponseWriter, r *http.Request) { } func handleAuth(mux *http.ServeMux, sh *authHandler) { - mux.HandleFunc(authPrefix+"/roles", capabilityHandler(api.AuthCapability, sh.baseRoles)) - mux.HandleFunc(authPrefix+"/roles/", capabilityHandler(api.AuthCapability, sh.handleRoles)) - mux.HandleFunc(authPrefix+"/users", capabilityHandler(api.AuthCapability, sh.baseUsers)) - mux.HandleFunc(authPrefix+"/users/", capabilityHandler(api.AuthCapability, sh.handleUsers)) - mux.HandleFunc(authPrefix+"/enable", capabilityHandler(api.AuthCapability, sh.enableDisable)) + mux.HandleFunc(authPrefix+"/roles", authCapabilityHandler(sh.baseRoles)) + mux.HandleFunc(authPrefix+"/roles/", authCapabilityHandler(sh.handleRoles)) + mux.HandleFunc(authPrefix+"/users", authCapabilityHandler(sh.baseUsers)) + mux.HandleFunc(authPrefix+"/users/", authCapabilityHandler(sh.handleUsers)) + mux.HandleFunc(authPrefix+"/enable", authCapabilityHandler(sh.enableDisable)) } func (sh *authHandler) baseRoles(w http.ResponseWriter, r *http.Request) { diff --git a/etcdserver/api/v2http/client_auth_test.go b/etcdserver/api/v2http/client_auth_test.go index 93fc5a40965..695155702c2 100644 --- a/etcdserver/api/v2http/client_auth_test.go +++ b/etcdserver/api/v2http/client_auth_test.go @@ -438,8 +438,8 @@ func TestGetUserGrantedWithNonexistingRole(t *testing.T) { } } -func mustAuthRequest(method, username, password string) *http.Request { - req, err := http.NewRequest(method, "path", strings.NewReader("")) +func mustAuthRequest(username, password string) *http.Request { + req, err := http.NewRequest(http.MethodGet, "path", strings.NewReader("")) if err != nil { panic("Cannot make auth request: " + err.Error()) } @@ -447,8 +447,8 @@ func mustAuthRequest(method, username, password string) *http.Request { return req } -func unauthedRequest(method string) *http.Request { - req, err := http.NewRequest(method, "path", strings.NewReader("")) +func unauthedRequest() *http.Request { + req, err := http.NewRequest(http.MethodGet, "path", strings.NewReader("")) if err != nil { panic("Cannot make request: " + err.Error()) } @@ -484,7 +484,7 @@ func TestPrefixAccess(t *testing.T) { }{ { key: "/foo", - req: mustAuthRequest("GET", "root", "good"), + req: mustAuthRequest("root", "good"), store: &mockAuthStore{ users: map[string]*v2auth.User{ "root": { @@ -506,7 +506,7 @@ func TestPrefixAccess(t *testing.T) { }, { key: "/foo", - req: mustAuthRequest("GET", "user", "good"), + req: mustAuthRequest("user", "good"), store: &mockAuthStore{ users: map[string]*v2auth.User{ "user": { @@ -534,7 +534,7 @@ func TestPrefixAccess(t *testing.T) { }, { key: "/foo", - req: mustAuthRequest("GET", "user", "good"), + req: mustAuthRequest("user", "good"), store: &mockAuthStore{ users: map[string]*v2auth.User{ "user": { @@ -562,7 +562,7 @@ func TestPrefixAccess(t *testing.T) { }, { key: "/foo", - req: mustAuthRequest("GET", "user", "bad"), + req: mustAuthRequest("user", "bad"), store: &mockAuthStore{ users: map[string]*v2auth.User{ "user": { @@ -590,7 +590,7 @@ func TestPrefixAccess(t *testing.T) { }, { key: "/foo", - req: mustAuthRequest("GET", "user", "good"), + req: mustAuthRequest("user", "good"), store: &mockAuthStore{ users: map[string]*v2auth.User{}, err: errors.New("Not the user"), @@ -659,7 +659,7 @@ func TestPrefixAccess(t *testing.T) { // check access for multiple roles { key: "/foo", - req: mustAuthRequest("GET", "user", "good"), + req: mustAuthRequest("user", "good"), store: &mockAuthStore{ users: map[string]*v2auth.User{ "user": { @@ -815,20 +815,20 @@ func TestUserFromClientCertificate(t *testing.T) { }{ { // non tls request - req: unauthedRequest("GET"), + req: unauthedRequest(), userExists: false, store: witherror, }, { // cert with cn of existing user - req: tlsAuthedRequest(unauthedRequest("GET"), "user"), + req: tlsAuthedRequest(unauthedRequest(), "user"), userExists: true, username: "user", store: noerror, }, { // cert with cn of non-existing user - req: tlsAuthedRequest(unauthedRequest("GET"), "otheruser"), + req: tlsAuthedRequest(unauthedRequest(), "otheruser"), userExists: false, store: witherror, }, @@ -871,30 +871,30 @@ func TestUserFromBasicAuth(t *testing.T) { { // valid user, valid pass username: "user", - req: mustAuthRequest("GET", "user", "password"), + req: mustAuthRequest("user", "password"), userExists: true, }, { // valid user, bad pass username: "user", - req: mustAuthRequest("GET", "user", "badpass"), + req: mustAuthRequest("user", "badpass"), userExists: false, }, { // valid user, no pass username: "user", - req: mustAuthRequest("GET", "user", ""), + req: mustAuthRequest("user", ""), userExists: false, }, { // missing user username: "missing", - req: mustAuthRequest("GET", "missing", "badpass"), + req: mustAuthRequest("missing", "badpass"), userExists: false, }, { // no basic auth - req: unauthedRequest("GET"), + req: unauthedRequest(), userExists: false, }, } diff --git a/etcdserver/api/v2http/metrics.go b/etcdserver/api/v2http/metrics.go index 2e89984445a..f8fa2b30ba2 100644 --- a/etcdserver/api/v2http/metrics.go +++ b/etcdserver/api/v2http/metrics.go @@ -20,7 +20,6 @@ import ( "net/http" - "github.com/coreos/etcd/etcdserver" "github.com/coreos/etcd/etcdserver/api/v2http/httptypes" "github.com/coreos/etcd/etcdserver/etcdserverpb" "github.com/coreos/etcd/etcdserver/v2error" @@ -64,7 +63,7 @@ func reportRequestReceived(request etcdserverpb.Request) { incomingEvents.WithLabelValues(methodFromRequest(request)).Inc() } -func reportRequestCompleted(request etcdserverpb.Request, response etcdserver.Response, startTime time.Time) { +func reportRequestCompleted(request etcdserverpb.Request, startTime time.Time) { method := methodFromRequest(request) successfulEventsHandlingTime.WithLabelValues(method).Observe(time.Since(startTime).Seconds()) } From 7c5cf7013fcb1ecd8f8e60c0ce56920438fd4364 Mon Sep 17 00:00:00 2001 From: Gyuho Lee Date: Wed, 9 May 2018 13:42:03 -0700 Subject: [PATCH 07/15] etcdserver/v2auth: remove unused parameters Signed-off-by: Gyuho Lee --- etcdserver/v2auth/auth.go | 12 ++++++------ etcdserver/v2auth/auth_requests.go | 16 ++++++++-------- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/etcdserver/v2auth/auth.go b/etcdserver/v2auth/auth.go index 743bc9907dd..685d8b6967d 100644 --- a/etcdserver/v2auth/auth.go +++ b/etcdserver/v2auth/auth.go @@ -171,7 +171,7 @@ func (_ passwordStore) HashPassword(password string) (string, error) { } func (s *store) AllUsers() ([]string, error) { - resp, err := s.requestResource("/users/", false, false) + resp, err := s.requestResource("/users/", false) if err != nil { if e, ok := err.(*v2error.Error); ok { if e.ErrorCode == v2error.EcodeKeyNotFound { @@ -245,7 +245,7 @@ func (s *store) DeleteUser(name string) error { if s.AuthEnabled() && name == "root" { return authErr(http.StatusForbidden, "Cannot delete root user while auth is enabled.") } - _, err := s.deleteResource("/users/" + name) + err := s.deleteResource("/users/" + name) if err != nil { if e, ok := err.(*v2error.Error); ok { if e.ErrorCode == v2error.EcodeKeyNotFound { @@ -293,7 +293,7 @@ func (s *store) UpdateUser(user User) (User, error) { func (s *store) AllRoles() ([]string, error) { nodes := []string{RootRoleName} - resp, err := s.requestResource("/roles/", false, false) + resp, err := s.requestResource("/roles/", false) if err != nil { if e, ok := err.(*v2error.Error); ok { if e.ErrorCode == v2error.EcodeKeyNotFound { @@ -338,7 +338,7 @@ func (s *store) DeleteRole(name string) error { if name == RootRoleName { return authErr(http.StatusForbidden, "Cannot modify role %s: is root role.", name) } - _, err := s.deleteResource("/roles/" + name) + err := s.deleteResource("/roles/" + name) if err != nil { if e, ok := err.(*v2error.Error); ok { if e.ErrorCode == v2error.EcodeKeyNotFound { @@ -696,7 +696,7 @@ func attachRootRole(u User) User { } func (s *store) getUser(name string, quorum bool) (User, error) { - resp, err := s.requestResource("/users/"+name, false, quorum) + resp, err := s.requestResource("/users/"+name, quorum) if err != nil { if e, ok := err.(*v2error.Error); ok { if e.ErrorCode == v2error.EcodeKeyNotFound { @@ -721,7 +721,7 @@ func (s *store) getRole(name string, quorum bool) (Role, error) { if name == RootRoleName { return rootRole, nil } - resp, err := s.requestResource("/roles/"+name, false, quorum) + resp, err := s.requestResource("/roles/"+name, quorum) if err != nil { if e, ok := err.(*v2error.Error); ok { if e.ErrorCode == v2error.EcodeKeyNotFound { diff --git a/etcdserver/v2auth/auth_requests.go b/etcdserver/v2auth/auth_requests.go index 8acb26ea73b..2127bdb2f6b 100644 --- a/etcdserver/v2auth/auth_requests.go +++ b/etcdserver/v2auth/auth_requests.go @@ -94,7 +94,7 @@ func (s *store) detectAuth() bool { if s.server == nil { return false } - value, err := s.requestResource("/enabled", false, false) + value, err := s.requestResource("/enabled", false) if err != nil { if e, ok := err.(*v2error.Error); ok { if e.ErrorCode == v2error.EcodeKeyNotFound { @@ -128,7 +128,7 @@ func (s *store) detectAuth() bool { return u } -func (s *store) requestResource(res string, dir, quorum bool) (etcdserver.Response, error) { +func (s *store) requestResource(res string, quorum bool) (etcdserver.Response, error) { ctx, cancel := context.WithTimeout(context.Background(), s.timeout) defer cancel() p := path.Join(StorePermsPrefix, res) @@ -139,7 +139,7 @@ func (s *store) requestResource(res string, dir, quorum bool) (etcdserver.Respon rr := etcdserverpb.Request{ Method: method, Path: p, - Dir: dir, + Dir: false, // TODO: always false? } return s.server.Do(ctx, rr) } @@ -171,19 +171,19 @@ func (s *store) setResource(res string, value interface{}, prevexist bool) (etcd return s.server.Do(ctx, rr) } -func (s *store) deleteResource(res string) (etcdserver.Response, error) { +func (s *store) deleteResource(res string) error { err := s.ensureAuthDirectories() if err != nil { - return etcdserver.Response{}, err + return err } ctx, cancel := context.WithTimeout(context.Background(), s.timeout) defer cancel() pex := true p := path.Join(StorePermsPrefix, res) - rr := etcdserverpb.Request{ + _, err = s.server.Do(ctx, etcdserverpb.Request{ Method: "DELETE", Path: p, PrevExist: &pex, - } - return s.server.Do(ctx, rr) + }) + return err } From 26e46702a26c98fcc6e57682706f836be92c1e68 Mon Sep 17 00:00:00 2001 From: Gyuho Lee Date: Wed, 9 May 2018 13:42:24 -0700 Subject: [PATCH 08/15] etcdserver/v2store: remove unused testing.T parameter Signed-off-by: Gyuho Lee --- etcdserver/v2store/store_v2_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/etcdserver/v2store/store_v2_test.go b/etcdserver/v2store/store_v2_test.go index 474b4d2ea7e..39e0745ffc3 100644 --- a/etcdserver/v2store/store_v2_test.go +++ b/etcdserver/v2store/store_v2_test.go @@ -30,6 +30,9 @@ type v2TestStore struct { func (s *v2TestStore) Close() {} func newTestStore(t *testing.T, ns ...string) StoreCloser { + if len(ns) == 0 { + t.Logf("new v2 store with no namespace") + } return &v2TestStore{v2store.New(ns...)} } From ba7cc04bace1ea3efc81ce0d5dc61215374b1a1c Mon Sep 17 00:00:00 2001 From: Gyuho Lee Date: Wed, 9 May 2018 13:42:37 -0700 Subject: [PATCH 09/15] etcdserver/api/v2v3: fix "getDir" with "sorted" Signed-off-by: Gyuho Lee --- etcdserver/api/v2v3/store.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/etcdserver/api/v2v3/store.go b/etcdserver/api/v2v3/store.go index b3da8091ec2..9efd1dc376e 100644 --- a/etcdserver/api/v2v3/store.go +++ b/etcdserver/api/v2v3/store.go @@ -18,6 +18,7 @@ import ( "context" "fmt" "path" + "sort" "strings" "time" @@ -94,6 +95,9 @@ func (s *v2v3Store) Get(nodePath string, recursive, sorted bool) (*v2store.Event func (s *v2v3Store) getDir(nodePath string, recursive, sorted bool, rev int64) ([]*v2store.NodeExtern, error) { rootNodes, err := s.getDirDepth(nodePath, 1, rev) if err != nil || !recursive { + if sorted { + sort.Sort(v2store.NodeExterns(rootNodes)) + } return rootNodes, err } nextNodes := rootNodes @@ -110,6 +114,10 @@ func (s *v2v3Store) getDir(nodePath string, recursive, sorted bool, rev int64) ( return nil, err } } + + if sorted { + sort.Sort(v2store.NodeExterns(rootNodes)) + } return rootNodes, nil } From 42a1d4c3b64f468d38da880b7793a2549c6b4a12 Mon Sep 17 00:00:00 2001 From: Gyuho Lee Date: Wed, 9 May 2018 13:52:54 -0700 Subject: [PATCH 10/15] functional: remove unused parameters Signed-off-by: Gyuho Lee --- functional/agent/handler.go | 24 ++++++++++++------------ functional/tester/stresser_runner.go | 1 + 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/functional/agent/handler.go b/functional/agent/handler.go index 76f2eda3e90..525c21006a5 100644 --- a/functional/agent/handler.go +++ b/functional/agent/handler.go @@ -71,13 +71,13 @@ func (srv *Server) handleTesterRequest(req *rpcpb.Request) (resp *rpcpb.Response return srv.handle_SIGQUIT_ETCD_AND_REMOVE_DATA_AND_STOP_AGENT() case rpcpb.Operation_BLACKHOLE_PEER_PORT_TX_RX: - return srv.handle_BLACKHOLE_PEER_PORT_TX_RX() + return srv.handle_BLACKHOLE_PEER_PORT_TX_RX(), nil case rpcpb.Operation_UNBLACKHOLE_PEER_PORT_TX_RX: - return srv.handle_UNBLACKHOLE_PEER_PORT_TX_RX() + return srv.handle_UNBLACKHOLE_PEER_PORT_TX_RX(), nil case rpcpb.Operation_DELAY_PEER_PORT_TX_RX: - return srv.handle_DELAY_PEER_PORT_TX_RX() + return srv.handle_DELAY_PEER_PORT_TX_RX(), nil case rpcpb.Operation_UNDELAY_PEER_PORT_TX_RX: - return srv.handle_UNDELAY_PEER_PORT_TX_RX() + return srv.handle_UNDELAY_PEER_PORT_TX_RX(), nil default: msg := fmt.Sprintf("operation not found (%v)", req.Operation) @@ -719,7 +719,7 @@ func (srv *Server) handle_SIGQUIT_ETCD_AND_REMOVE_DATA_AND_STOP_AGENT() (*rpcpb. }, nil } -func (srv *Server) handle_BLACKHOLE_PEER_PORT_TX_RX() (*rpcpb.Response, error) { +func (srv *Server) handle_BLACKHOLE_PEER_PORT_TX_RX() *rpcpb.Response { for port, px := range srv.advertisePeerPortToProxy { srv.lg.Info("blackholing", zap.Int("peer-port", port)) px.BlackholeTx() @@ -729,10 +729,10 @@ func (srv *Server) handle_BLACKHOLE_PEER_PORT_TX_RX() (*rpcpb.Response, error) { return &rpcpb.Response{ Success: true, Status: "blackholed peer port tx/rx", - }, nil + } } -func (srv *Server) handle_UNBLACKHOLE_PEER_PORT_TX_RX() (*rpcpb.Response, error) { +func (srv *Server) handle_UNBLACKHOLE_PEER_PORT_TX_RX() *rpcpb.Response { for port, px := range srv.advertisePeerPortToProxy { srv.lg.Info("unblackholing", zap.Int("peer-port", port)) px.UnblackholeTx() @@ -742,10 +742,10 @@ func (srv *Server) handle_UNBLACKHOLE_PEER_PORT_TX_RX() (*rpcpb.Response, error) return &rpcpb.Response{ Success: true, Status: "unblackholed peer port tx/rx", - }, nil + } } -func (srv *Server) handle_DELAY_PEER_PORT_TX_RX() (*rpcpb.Response, error) { +func (srv *Server) handle_DELAY_PEER_PORT_TX_RX() *rpcpb.Response { lat := time.Duration(srv.Tester.UpdatedDelayLatencyMs) * time.Millisecond rv := time.Duration(srv.Tester.DelayLatencyMsRv) * time.Millisecond @@ -767,10 +767,10 @@ func (srv *Server) handle_DELAY_PEER_PORT_TX_RX() (*rpcpb.Response, error) { return &rpcpb.Response{ Success: true, Status: "delayed peer port tx/rx", - }, nil + } } -func (srv *Server) handle_UNDELAY_PEER_PORT_TX_RX() (*rpcpb.Response, error) { +func (srv *Server) handle_UNDELAY_PEER_PORT_TX_RX() *rpcpb.Response { for port, px := range srv.advertisePeerPortToProxy { srv.lg.Info("undelaying", zap.Int("peer-port", port)) px.UndelayTx() @@ -780,5 +780,5 @@ func (srv *Server) handle_UNDELAY_PEER_PORT_TX_RX() (*rpcpb.Response, error) { return &rpcpb.Response{ Success: true, Status: "undelayed peer port tx/rx", - }, nil + } } diff --git a/functional/tester/stresser_runner.go b/functional/tester/stresser_runner.go index 8fb20980160..efcdfb6a7fb 100644 --- a/functional/tester/stresser_runner.go +++ b/functional/tester/stresser_runner.go @@ -54,6 +54,7 @@ func newRunnerStresser( return &runnerStresser{ stype: stype, etcdClientEndpoint: ep, + lg: lg, cmdStr: cmdStr, args: args, rl: rl, From b7443ad8496b2302d97221b3d27b06b81616b626 Mon Sep 17 00:00:00 2001 From: Gyuho Lee Date: Wed, 9 May 2018 13:55:32 -0700 Subject: [PATCH 11/15] integration: remove unused parameters Signed-off-by: Gyuho Lee --- integration/bridge.go | 6 +++--- integration/cluster.go | 6 +++--- integration/network_partition_test.go | 6 +++--- integration/v3_leadership_test.go | 2 +- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/integration/bridge.go b/integration/bridge.go index 9792ba89f94..a61c1b4cc16 100644 --- a/integration/bridge.go +++ b/integration/bridge.go @@ -155,12 +155,12 @@ func (b *bridge) serveConn(bc *bridgeConn) { var wg sync.WaitGroup wg.Add(2) go func() { - b.ioCopy(bc, bc.out, bc.in) + b.ioCopy(bc.out, bc.in) bc.close() wg.Done() }() go func() { - b.ioCopy(bc, bc.in, bc.out) + b.ioCopy(bc.in, bc.out) bc.close() wg.Done() }() @@ -200,7 +200,7 @@ func (b *bridge) Unblackhole() { } // ref. https://github.com/golang/go/blob/master/src/io/io.go copyBuffer -func (b *bridge) ioCopy(bc *bridgeConn, dst io.Writer, src io.Reader) (err error) { +func (b *bridge) ioCopy(dst io.Writer, src io.Reader) (err error) { buf := make([]byte, 32*1024) for { select { diff --git a/integration/cluster.go b/integration/cluster.go index 6e03a4ae294..ccd232f6fa5 100644 --- a/integration/cluster.go +++ b/integration/cluster.go @@ -450,10 +450,10 @@ func (c *cluster) waitLeader(t *testing.T, membs []*member) int { return -1 } -func (c *cluster) WaitNoLeader(t *testing.T) { c.waitNoLeader(t, c.Members) } +func (c *cluster) WaitNoLeader() { c.waitNoLeader(c.Members) } // waitNoLeader waits until given members lose leader. -func (c *cluster) waitNoLeader(t *testing.T, membs []*member) { +func (c *cluster) waitNoLeader(membs []*member) { noLeader := false for !noLeader { noLeader = true @@ -992,7 +992,7 @@ func (m *member) Stop(t *testing.T) { } // checkLeaderTransition waits for leader transition, returning the new leader ID. -func checkLeaderTransition(t *testing.T, m *member, oldLead uint64) uint64 { +func checkLeaderTransition(m *member, oldLead uint64) uint64 { interval := time.Duration(m.s.Cfg.TickMs) * time.Millisecond for m.s.Lead() == 0 || (m.s.Lead() == oldLead) { time.Sleep(interval) diff --git a/integration/network_partition_test.go b/integration/network_partition_test.go index cfbdda02a1b..db542943d40 100644 --- a/integration/network_partition_test.go +++ b/integration/network_partition_test.go @@ -41,7 +41,7 @@ func TestNetworkPartition5MembersLeaderInMinority(t *testing.T) { injectPartition(t, minorityMembers, majorityMembers) // minority leader must be lost - clus.waitNoLeader(t, minorityMembers) + clus.waitNoLeader(minorityMembers) // wait extra election timeout time.Sleep(2 * majorityMembers[0].ElectionTimeout()) @@ -89,7 +89,7 @@ func testNetworkPartition5MembersLeaderInMajority(t *testing.T) error { injectPartition(t, majorityMembers, minorityMembers) // minority leader must be lost - clus.waitNoLeader(t, minorityMembers) + clus.waitNoLeader(minorityMembers) // wait extra election timeout time.Sleep(2 * majorityMembers[0].ElectionTimeout()) @@ -128,7 +128,7 @@ func TestNetworkPartition4Members(t *testing.T) { injectPartition(t, leaderPartition, followerPartition) // no group has quorum, so leader must be lost in all members - clus.WaitNoLeader(t) + clus.WaitNoLeader() // recover network partition (bi-directional) recoverPartition(t, leaderPartition, followerPartition) diff --git a/integration/v3_leadership_test.go b/integration/v3_leadership_test.go index 7f41f3bfe5b..93c7f3ca6c2 100644 --- a/integration/v3_leadership_test.go +++ b/integration/v3_leadership_test.go @@ -41,7 +41,7 @@ func testMoveLeader(t *testing.T, auto bool) { for i := range clus.Members { if oldLeadIdx != i { go func(m *member) { - idc <- checkLeaderTransition(t, m, oldLeadID) + idc <- checkLeaderTransition(m, oldLeadID) }(clus.Members[i]) } } From 1a83c6ad80fcc07c2d9aaec220478f1feec4fe75 Mon Sep 17 00:00:00 2001 From: Gyuho Lee Date: Wed, 9 May 2018 13:57:33 -0700 Subject: [PATCH 12/15] mvcc: remove unused parameters Signed-off-by: Gyuho Lee --- mvcc/kv_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/mvcc/kv_test.go b/mvcc/kv_test.go index ae233b1981a..97d2163116e 100644 --- a/mvcc/kv_test.go +++ b/mvcc/kv_test.go @@ -639,13 +639,13 @@ func TestKVRestore(t *testing.T) { kvss = append(kvss, r.KVs) } - keysBefore := readGaugeInt(&keysGauge) + keysBefore := readGaugeInt(keysGauge) s.Close() // ns should recover the the previous state from backend. ns := NewStore(zap.NewExample(), b, &lease.FakeLessor{}, nil) - if keysRestore := readGaugeInt(&keysGauge); keysBefore != keysRestore { + if keysRestore := readGaugeInt(keysGauge); keysBefore != keysRestore { t.Errorf("#%d: got %d key count, expected %d", i, keysRestore, keysBefore) } @@ -664,9 +664,9 @@ func TestKVRestore(t *testing.T) { } } -func readGaugeInt(g *prometheus.Gauge) int { +func readGaugeInt(g prometheus.Gauge) int { ch := make(chan prometheus.Metric, 1) - keysGauge.Collect(ch) + g.Collect(ch) m := <-ch mm := &dto.Metric{} m.Write(mm) From c862712c73bbd02f853641819a2aeffa330238f7 Mon Sep 17 00:00:00 2001 From: Gyuho Lee Date: Wed, 9 May 2018 15:06:11 -0700 Subject: [PATCH 13/15] pkg/transport: remove unused parameter from "wrapTLS" Signed-off-by: Gyuho Lee --- pkg/transport/listener.go | 4 ++-- pkg/transport/timeout_listener.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/transport/listener.go b/pkg/transport/listener.go index 98755c5b748..466ec4571e5 100644 --- a/pkg/transport/listener.go +++ b/pkg/transport/listener.go @@ -41,7 +41,7 @@ func NewListener(addr, scheme string, tlsinfo *TLSInfo) (l net.Listener, err err if l, err = newListener(addr, scheme); err != nil { return nil, err } - return wrapTLS(addr, scheme, tlsinfo, l) + return wrapTLS(scheme, tlsinfo, l) } func newListener(addr string, scheme string) (net.Listener, error) { @@ -52,7 +52,7 @@ func newListener(addr string, scheme string) (net.Listener, error) { return net.Listen("tcp", addr) } -func wrapTLS(addr, scheme string, tlsinfo *TLSInfo, l net.Listener) (net.Listener, error) { +func wrapTLS(scheme string, tlsinfo *TLSInfo, l net.Listener) (net.Listener, error) { if scheme != "https" && scheme != "unixs" { return l, nil } diff --git a/pkg/transport/timeout_listener.go b/pkg/transport/timeout_listener.go index b35e04955bb..273e99fe038 100644 --- a/pkg/transport/timeout_listener.go +++ b/pkg/transport/timeout_listener.go @@ -32,7 +32,7 @@ func NewTimeoutListener(addr string, scheme string, tlsinfo *TLSInfo, rdtimeoutd rdtimeoutd: rdtimeoutd, wtimeoutd: wtimeoutd, } - if ln, err = wrapTLS(addr, scheme, tlsinfo, ln); err != nil { + if ln, err = wrapTLS(scheme, tlsinfo, ln); err != nil { return nil, err } return ln, nil From df87dba21804de4fa1a214379720371570196169 Mon Sep 17 00:00:00 2001 From: Gyuho Lee Date: Wed, 9 May 2018 15:14:23 -0700 Subject: [PATCH 14/15] tests/e2e: use different parameters Signed-off-by: Gyuho Lee --- tests/e2e/ctl_v2_test.go | 8 ++++---- tests/e2e/ctl_v3_auth_test.go | 2 +- tests/e2e/ctl_v3_kv_test.go | 3 ++- tests/e2e/ctl_v3_lock_test.go | 2 +- tests/e2e/ctl_v3_snapshot_test.go | 6 +++--- 5 files changed, 11 insertions(+), 10 deletions(-) diff --git a/tests/e2e/ctl_v2_test.go b/tests/e2e/ctl_v2_test.go index 60d9ce74cea..8af4425ee77 100644 --- a/tests/e2e/ctl_v2_test.go +++ b/tests/e2e/ctl_v2_test.go @@ -251,7 +251,7 @@ func testCtlV2Backup(t *testing.T, snapCount int, v3 bool) { t.Fatal(err) } - if err := etcdctlSet(epc1, "foo1", "bar"); err != nil { + if err := etcdctlSet(epc1, "foo1", "bar1"); err != nil { t.Fatal(err) } @@ -276,7 +276,7 @@ func testCtlV2Backup(t *testing.T, snapCount int, v3 bool) { epc2 := setupEtcdctlTest(t, &cfg2, false) // check if backup went through correctly - if err := etcdctlGet(epc2, "foo1", "bar", false); err != nil { + if err := etcdctlGet(epc2, "foo1", "bar1", false); err != nil { t.Fatal(err) } @@ -292,10 +292,10 @@ func testCtlV2Backup(t *testing.T, snapCount int, v3 bool) { } // check if it can serve client requests - if err := etcdctlSet(epc2, "foo2", "bar"); err != nil { + if err := etcdctlSet(epc2, "foo2", "bar2"); err != nil { t.Fatal(err) } - if err := etcdctlGet(epc2, "foo2", "bar", false); err != nil { + if err := etcdctlGet(epc2, "foo2", "bar2", false); err != nil { t.Fatal(err) } diff --git a/tests/e2e/ctl_v3_auth_test.go b/tests/e2e/ctl_v3_auth_test.go index db38d5fc615..f0c313e5f59 100644 --- a/tests/e2e/ctl_v3_auth_test.go +++ b/tests/e2e/ctl_v3_auth_test.go @@ -968,7 +968,7 @@ func authTestSnapshot(cx ctlCtx) { cx.user, cx.pass = "root", "root" authSetupTestUser(cx) - fpath := "test.snapshot" + fpath := "test-auth.snapshot" defer os.RemoveAll(fpath) // ordinary user cannot save a snapshot diff --git a/tests/e2e/ctl_v3_kv_test.go b/tests/e2e/ctl_v3_kv_test.go index 2e14bea6416..0533b067f1b 100644 --- a/tests/e2e/ctl_v3_kv_test.go +++ b/tests/e2e/ctl_v3_kv_test.go @@ -18,9 +18,10 @@ import ( "fmt" "strings" "testing" + "time" ) -func TestCtlV3Put(t *testing.T) { testCtl(t, putTest) } +func TestCtlV3Put(t *testing.T) { testCtl(t, putTest, withDialTimeout(7*time.Second)) } func TestCtlV3PutNoTLS(t *testing.T) { testCtl(t, putTest, withCfg(configNoTLS)) } func TestCtlV3PutClientTLS(t *testing.T) { testCtl(t, putTest, withCfg(configClientTLS)) } func TestCtlV3PutClientAutoTLS(t *testing.T) { testCtl(t, putTest, withCfg(configClientAutoTLS)) } diff --git a/tests/e2e/ctl_v3_lock_test.go b/tests/e2e/ctl_v3_lock_test.go index 6bcbe4de2c5..3e133297641 100644 --- a/tests/e2e/ctl_v3_lock_test.go +++ b/tests/e2e/ctl_v3_lock_test.go @@ -84,7 +84,7 @@ func testLock(cx ctlCtx) { if err = holder.Signal(os.Interrupt); err != nil { cx.t.Fatal(err) } - if err = closeWithTimeout(holder, time.Second); err != nil { + if err = closeWithTimeout(holder, 200*time.Millisecond+time.Second); err != nil { cx.t.Fatal(err) } diff --git a/tests/e2e/ctl_v3_snapshot_test.go b/tests/e2e/ctl_v3_snapshot_test.go index 632b05776d8..0df7a5f9db4 100644 --- a/tests/e2e/ctl_v3_snapshot_test.go +++ b/tests/e2e/ctl_v3_snapshot_test.go @@ -43,7 +43,7 @@ func snapshotTest(cx ctlCtx) { cx.t.Fatalf("snapshot: ctlV3Put error (%v)", err) } - fpath := "test.snapshot" + fpath := "test1.snapshot" defer os.RemoveAll(fpath) if err = ctlV3SnapshotSave(cx, fpath); err != nil { @@ -65,7 +65,7 @@ func snapshotTest(cx ctlCtx) { func TestCtlV3SnapshotCorrupt(t *testing.T) { testCtl(t, snapshotCorruptTest) } func snapshotCorruptTest(cx ctlCtx) { - fpath := "test.snapshot" + fpath := "test2.snapshot" defer os.RemoveAll(fpath) if err := ctlV3SnapshotSave(cx, fpath); err != nil { @@ -98,7 +98,7 @@ func snapshotCorruptTest(cx ctlCtx) { func TestCtlV3SnapshotStatusBeforeRestore(t *testing.T) { testCtl(t, snapshotStatusBeforeRestoreTest) } func snapshotStatusBeforeRestoreTest(cx ctlCtx) { - fpath := "test.snapshot" + fpath := "test3.snapshot" defer os.RemoveAll(fpath) if err := ctlV3SnapshotSave(cx, fpath); err != nil { From e7adfb0ebf6ef42593556c44a969a096db1897ce Mon Sep 17 00:00:00 2001 From: Gyuho Lee Date: Wed, 9 May 2018 15:18:57 -0700 Subject: [PATCH 15/15] raft: use different parameters for tests Signed-off-by: Gyuho Lee --- raft/raft_test.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/raft/raft_test.go b/raft/raft_test.go index 6540d0f7227..7fe05e4e90b 100644 --- a/raft/raft_test.go +++ b/raft/raft_test.go @@ -1182,7 +1182,7 @@ func TestCommit(t *testing.T) { storage.Append(tt.logs) storage.hardState = pb.HardState{Term: tt.smTerm} - sm := newTestRaft(1, []uint64{1}, 5, 1, storage) + sm := newTestRaft(1, []uint64{1}, 10, 2, storage) for j := 0; j < len(tt.matches); j++ { sm.setProgress(uint64(j)+1, tt.matches[j], tt.matches[j]+1, false) } @@ -2733,7 +2733,7 @@ func TestRestoreWithLearner(t *testing.T) { } storage := NewMemoryStorage() - sm := newTestLearnerRaft(3, []uint64{1, 2}, []uint64{3}, 10, 1, storage) + sm := newTestLearnerRaft(3, []uint64{1, 2}, []uint64{3}, 8, 2, storage) if ok := sm.restore(s); !ok { t.Error("restore fail, want succeed") } @@ -4070,16 +4070,16 @@ func (nw *network) drop(from, to uint64, perc float64) { } func (nw *network) cut(one, other uint64) { - nw.drop(one, other, 1) - nw.drop(other, one, 1) + nw.drop(one, other, 2.0) // always drop + nw.drop(other, one, 2.0) // always drop } func (nw *network) isolate(id uint64) { for i := 0; i < len(nw.peers); i++ { nid := uint64(i) + 1 if nid != id { - nw.drop(id, nid, 1.0) - nw.drop(nid, id, 1.0) + nw.drop(id, nid, 1.0) // always drop + nw.drop(nid, id, 1.0) // always drop } } }