Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix index out of range in patricia tree #33

Merged
merged 1 commit into from
Jul 15, 2016
Merged

Fix index out of range in patricia tree #33

merged 1 commit into from
Jul 15, 2016

Conversation

soheilhy
Copy link
Owner

Bug #32 reported that there is an index out of range error. This
issue was introduced in 703b087.

@tamird
Copy link
Contributor

tamird commented Jul 15, 2016

No test?

@soheilhy
Copy link
Owner Author

👍 will add the tests. Just want to make sure the patch fixes the bug first.

@kalbasit
Copy link

works 👍

@kalbasit
Copy link

Although I am getting this:

2016/07/15 16:21:15 http: TLS handshake error from 10.2.64.1:41856: EOF

I'll have to dig more into it, currently on the way to the office, will check in 1/2 an hour.

Bug #32 reported that there is an index out of range error. This
issue was introduced in 703b087.

Fix #32 and add a test to detect this issue
@soheilhy
Copy link
Owner Author

Thanks for the confirmation!

@tamird, added the tests broken on master and fixed on this branch. ptal.

@soheilhy
Copy link
Owner Author

RE: EOF

 2016/07/15 16:21:15 http: TLS handshake error from 10.2.64.1:41856: EOF

I beleive this is because you're using an http server with a tls config. Instead, we need to create a tls.NewListener and then passing it to an HTTP server with no tls config. example_tls_test.go is a good example.

@kalbasit
Copy link

@soheilhy that wouldn't work the grpc-gateway :(

diff --git a/cmd/serve.go b/cmd/serve.go
index 381c7ce..9e61979 100644
--- a/cmd/serve.go
+++ b/cmd/serve.go
@@ -57,6 +57,7 @@ var (
        loggerService   *logger.Service
        serverClosed    chan bool
        readyToShutdown chan bool
+       tlsConfig       *tls.Config
 )

 // serveCmd represents the serve command
@@ -263,9 +264,10 @@ func serve(cmd *cobra.Command, args []string) {
                // create the HTTP server
                mainServer = &http.Server{
                        Handler: grpcHandlerFunc(gRPCServer, mux),
-                       TLSConfig: &tls.Config{
-                               Certificates: []tls.Certificate{keyPair},
-                       },
+               }
+               // create the TLS config
+               tlsConfig = &tls.Config{
+                       Certificates: []tls.Certificate{keyPair},
                }
        }

@@ -357,10 +359,8 @@ func serve(cmd *cobra.Command, args []string) {
                                }
                                // wait for the server to die
                                <-serverClosed
-                               // create a new HTTP server
-                               mainServer.TLSConfig = &tls.Config{
-                                       Certificates: []tls.Certificate{keyPair},
-                               }
+                               // update the certificate in tlsConfig
+                               tlsConfig.Certificates = []tls.Certificate{keyPair}
                                // create a new connection
                                if srvListener, err = net.Listen("tcp", listenAddr); err != nil {
                                        log.Fatalf("error opening a socket on %s: %s", listenAddr, err)
@@ -379,12 +379,12 @@ func startServer(mainServer, healthCheckServer *http.Server, srvListener net.Lis
        log.Printf("starting the HTTP/HTTPS/gRPC server bound to %q", listenAddr)
        // create a new connection multiplexer.
        m := cmux.New(srvListener)
-       // we first match on HTTP 1.1 methods.
-       httpl := m.Match(cmux.HTTP1Fast())
+       // we first match on the ReadinessProbe header
+       httpl := m.Match(cmux.HTTP1HeaderField("ReadinessProbe", "32b62d7a2c4227fbf239a9ebe7bce8b9a32cf069"))
        // if not matched, we assume that its TLS.
        tlsl := m.Match(cmux.Any())
        // start the mainServer
-       go mainServer.Serve(tls.NewListener(tlsl, mainServer.TLSConfig))
+       go mainServer.Serve(tls.NewListener(tlsl, tlsConfig))
        // start the healthCheckServer
        go healthCheckServer.Serve(httpl)
        // boot the connection multiplexer, this should return once the srvListener

when I run it:

2016/07/15 10:34:27 starting the HTTP/HTTPS/gRPC server bound to ":8091"
2016/07/15 10:34:27 transport: http2Client.notifyError got notified that the client transport was broken unexpected EOF.
2016/07/15 10:34:27 transport: http2Client.notifyError got notified that the client transport was broken unexpected EOF.
2016/07/15 10:34:29 grpc: addrConn.resetTransport failed to create client transport: connection error: desc = "transport: write tcp 127.0.0.1:52766->127.0.0.1:8091: write: broken pipe"; Reconnecting to {"127.0.0.1:8091" <nil>}

@soheilhy
Copy link
Owner Author

@kalbasit hmm... I don't have a good answer on why TLS doesn't work for you. I tried a few examples and seems to be working fine. Since this isn't related to this pull request, can you please create another issue with a minimal example to reproduce the bug?

Thanks.

@kalbasit
Copy link

@soheilhy agree, I'll investigate more and if I'll make an minimal example if needed.

@tamird
Copy link
Contributor

tamird commented Jul 15, 2016

lgtm

@soheilhy
Copy link
Owner Author

Thanks guys!

@soheilhy soheilhy merged commit b269515 into master Jul 15, 2016
tamird pushed a commit to cockroachdb/cmux that referenced this pull request Jul 15, 2016
Fix index out of range in patricia tree
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants