embed: make v2 endpoint optional #7170

vimalk78 · 2017-01-16T17:06:37Z

added a config flag enable-v2 with default value true
if this flag is set to false, v2 handler is not created

codecov-io · 2017-01-16T18:10:40Z

Current coverage is 64.13% (diff: 47.82%)

No coverage report found for master at b2d5c91.

Powered by Codecov. Last update b2d5c91...79c4918

vimalk78 · 2017-01-16T18:17:07Z

embed/etcd.go

+		} else {
+			plog.Infof("etcd v2 endpoints not enabled.")
+			return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+				http.Error(w, "etcd v2 endpoints not enabled", http.StatusNotFound)


should the http error be changed ?

heyitsanthony · 2017-01-17T18:34:28Z

embed/config.go

@@ -102,6 +102,7 @@ type Config struct {
 	InitialCluster      string `json:"initial-cluster"`
 	InitialClusterToken string `json:"initial-cluster-token"`
 	StrictReconfigCheck bool   `json:"strict-reconfig-check"`
+	EnableV2Endpoints   bool   `json:"enable-v2-endpoints"`


probably just EnableV2?

heyitsanthony · 2017-01-17T18:38:44Z

embed/etcd.go

-		Handler: v2http.NewClientHandler(e.Server, e.Server.Cfg.ReqTimeout()),
-		Info:    e.cfg.CorsInfo,
-	})
+	v2h := func() http.Handler {


Please nil this out if v2 is disabled, instead of having a handler that returns an error, and build a v3-only handler in grpcHandlerFunc in serve.go if it's nil like I suggested? The reasoning is that if the v2 endpoint is disabled, having a v2 handler only to respond with an error only adds extra overhead to request processing.

This still needs to be nil'd out if v2 is disabled, otherwise there's still additional overhead in the grpc handler:

var v2h http.Handler if e.Config().EnableV2 { v2h = http.Handler(&cors.CORSHandler{ Handler: v2http.NewClientHandler(e.Server, e.Server.Cfg.ReqTimeout()), Info: e.cfg.CorsInfo, }) }

v2h is nil if e.Config().EnableV2 == false , the function returns nil and sets v2h to nil.

but i agree it unnecessarily complicated, will change to the way you suggested.

Oh I see, yeah. Definitely confusing.

heyitsanthony · 2017-01-17T18:40:40Z

etcdmain/config.go

@@ -155,6 +155,7 @@ func newConfig() *config {
 		plog.Panicf("unexpected error setting up clusterStateFlag: %v", err)
 	}
 	fs.BoolVar(&cfg.StrictReconfigCheck, "strict-reconfig-check", cfg.StrictReconfigCheck, "Reject reconfiguration requests that would cause quorum loss.")
+	fs.BoolVar(&cfg.EnableV2Endpoints, "enable-v2-endpoints", true, "etcd will start the v2 endpoints for the clients.")


s/enable-v2-endpoints/enable-v2/
s/etcd will start the v2 endpoints for the clients/Accept etcd V2 client requests.
?

heyitsanthony · 2017-01-17T18:40:55Z

etcdmain/help.go

@@ -88,6 +88,8 @@ clustering flags:
 		reject reconfiguration requests that would cause quorum loss.
 	--auto-compaction-retention '0'
 		auto compaction retention in hour. 0 means disable auto compaction.
+	--enable-v2-endpoints


--enable-v2?

heyitsanthony · 2017-01-17T18:41:06Z

etcdmain/help.go

@@ -88,6 +88,8 @@ clustering flags:
 		reject reconfiguration requests that would cause quorum loss.
 	--auto-compaction-retention '0'
 		auto compaction retention in hour. 0 means disable auto compaction.
+	--enable-v2-endpoints
+		etcd will start the v2 endpoints for the clients.


Accept etcd V2 client requests.?

vimalk78 · 2017-01-19T15:40:38Z

@heyitsanthony , please have a look.

xiang90 · 2017-01-19T16:37:52Z

@gyuho @heyitsanthony

We probably need a release note for this. 1yr after 3.2 release, we will probably change the default flag of enable-v2 to false. (and we might use a simulated v2). later on, we will completely remove v2.

heyitsanthony

two last changes and this should be ready

heyitsanthony · 2017-01-19T23:03:35Z

embed/etcd.go

-		Handler: v2http.NewClientHandler(e.Server, e.Server.Cfg.ReqTimeout()),
-		Info:    e.cfg.CorsInfo,
-	})
+	v2h := func() http.Handler {


This still needs to be nil'd out if v2 is disabled, otherwise there's still additional overhead in the grpc handler:

var v2h http.Handler if e.Config().EnableV2 { v2h = http.Handler(&cors.CORSHandler{ Handler: v2http.NewClientHandler(e.Server, e.Server.Cfg.ReqTimeout()), Info: e.cfg.CorsInfo, }) }

heyitsanthony · 2017-01-19T23:07:51Z

embed/serve.go

@@ -122,13 +122,19 @@ func (sctx *serveCtx) serve(s *etcdserver.EtcdServer, tlscfg *tls.Config, handle
 // grpcHandlerFunc returns an http.Handler that delegates to grpcServer on incoming gRPC
 // connections or otherHandler otherwise. Copied from cockroachdb.
 func grpcHandlerFunc(grpcServer *grpc.Server, otherHandler http.Handler) http.Handler {
-	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		if r.ProtoMajor == 2 && strings.Contains(r.Header.Get("Content-Type"), "application/grpc") {
+	if otherHandler != nil {


probably just have:

if otherHandler == nil { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { grpcServer.ServeHTTP(w, r) }) } return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.ProtoMajor == 2 && strings.Contains(r.Header.Get("Content-Type"), "application/grpc") { ...

to avoid the extra indent level and keep the diff smaller

vimalk78 · 2017-01-20T00:02:53Z

any suggestions for integration/e2e test cases based on this flag? i am still in the process of understanding the test framework, will send the test cases in another commit/PR.

since this PR touches the method grpcHandlerFunc which is used in sctx.secure case, i wanted to test this path. But i couldn't find how to configure for client URLs. the hack/tls-setup generates for peer URLs and the Procfile also uses the same.
is there any such cert/key files and Procfile for client urls?

i lazily went through below urls, to look for a ready made thing like for peer urls :)
https://coreos.com/etcd/docs/latest/op-guide/security.html
https://coreos.com/os/docs/latest/generate-self-signed-certificates.html

heyitsanthony · 2017-01-20T00:24:50Z

@vimalk78 check the e2e tests that have "TLS" in them. There's stuff like configClientTLS, configPeerTLS, etc, for that.

heyitsanthony · 2017-01-20T18:46:10Z

lgtm

xiang90 · 2017-01-20T19:14:18Z

LGTM

vimalk78 commented Jan 16, 2017

View reviewed changes

heyitsanthony suggested changes Jan 17, 2017

View reviewed changes

vimalk78 force-pushed the make-v2-endpoint-optional-#7100 branch 2 times, most recently from 79c4918 to 179de1b Compare January 19, 2017 11:49

heyitsanthony suggested changes Jan 19, 2017

View reviewed changes

embed/etcd.go: make v2 endpoint optional. fixes etcd-io#7100

f80914f

vimalk78 force-pushed the make-v2-endpoint-optional-#7100 branch from 179de1b to f80914f Compare January 20, 2017 07:12

heyitsanthony added the kind/release-note label Jan 20, 2017

heyitsanthony approved these changes Jan 20, 2017

View reviewed changes

xiang90 merged commit a630735 into etcd-io:master Jan 20, 2017

gyuho changed the title ~~embed/etcd.go: make v2 endpoint optional. fixes #7100~~ embed: make v2 endpoint optional May 2, 2017

gyuho removed the kind/need-release-note-v3.3 label Nov 22, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

embed: make v2 endpoint optional #7170

embed: make v2 endpoint optional #7170

vimalk78 commented Jan 16, 2017 •

edited

Loading

codecov-io commented Jan 16, 2017 •

edited

Loading

vimalk78 Jan 16, 2017

heyitsanthony Jan 17, 2017

heyitsanthony Jan 17, 2017

heyitsanthony Jan 19, 2017

vimalk78 Jan 19, 2017

heyitsanthony Jan 19, 2017

heyitsanthony Jan 17, 2017

heyitsanthony Jan 17, 2017

heyitsanthony Jan 17, 2017

vimalk78 commented Jan 19, 2017

xiang90 commented Jan 19, 2017

heyitsanthony left a comment

heyitsanthony Jan 19, 2017

heyitsanthony Jan 19, 2017

vimalk78 Jan 19, 2017

vimalk78 commented Jan 20, 2017

heyitsanthony commented Jan 20, 2017

heyitsanthony commented Jan 20, 2017

xiang90 commented Jan 20, 2017

embed: make v2 endpoint optional #7170

embed: make v2 endpoint optional #7170

Conversation

vimalk78 commented Jan 16, 2017 • edited Loading

codecov-io commented Jan 16, 2017 • edited Loading

Current coverage is 64.13% (diff: 47.82%)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vimalk78 commented Jan 19, 2017

xiang90 commented Jan 19, 2017

heyitsanthony left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vimalk78 commented Jan 20, 2017

heyitsanthony commented Jan 20, 2017

heyitsanthony commented Jan 20, 2017

xiang90 commented Jan 20, 2017

vimalk78 commented Jan 16, 2017 •

edited

Loading

codecov-io commented Jan 16, 2017 •

edited

Loading