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: do not fail on short connections #517

Merged
merged 3 commits into from
Jan 10, 2020
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,10 @@

### To be Released

* Add `--bind` arg to `db-tunnel` command that let us bind a custom host (and not only 127.0.0.1) [#517](https://github.com/Scalingo/cli/pull/517)

* Bugfix: Encrypted key with new OpenSSH format header for private keys was broken [#513](https://github.com/Scalingo/cli/pull/513)
* Bugfix: db-tunnel: better handling of short lived connections [#517](https://github.com/Scalingo/cli/pull/517)

### 1.16.4

Expand Down
2 changes: 2 additions & 0 deletions cmd/db_tunnel.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ var (
Flags: []cli.Flag{appFlag,
cli.IntFlag{Name: "port, p", Usage: "Local port to bind (default 10000)"},
cli.StringFlag{Name: "identity, i", Usage: "SSH Private Key"},
cli.StringFlag{Name: "bind, b", Usage: "IP to bind (default 127.0.0.1)"},
cli.BoolTFlag{Name: "reconnect", Usage: "true by default, automatically reconnect to the tunnel when disconnected"},
},
Description: `Create an SSH-encrypted connection to access your Scalingo database locally.
Expand Down Expand Up @@ -71,6 +72,7 @@ var (
DBEnvVar: c.Args()[0],
Identity: sshIdentity,
Port: c.Int("port"),
Bind: c.String("bind"),
Reconnect: c.BoolT("reconnect"),
}
err := db.Tunnel(opts)
Expand Down
59 changes: 38 additions & 21 deletions db/tunnel.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,10 @@ import (
"time"

"github.com/Scalingo/cli/config"
"github.com/Scalingo/go-scalingo/debug"
"github.com/Scalingo/cli/io"
netssh "github.com/Scalingo/cli/net/ssh"
"github.com/Scalingo/go-scalingo"
"github.com/Scalingo/go-scalingo/debug"
"golang.org/x/crypto/ssh"
"gopkg.in/errgo.v1"
)
Expand All @@ -25,12 +25,14 @@ var (
errTimeout = errors.New("timeout")
connIDGenerator = make(chan int)
defaultPort = 10000
defaultBind = "127.0.0.1"
)

type TunnelOpts struct {
App string
DBEnvVar string
Identity string
Bind string
Port int
Reconnect bool
}
Expand Down Expand Up @@ -63,7 +65,8 @@ func Tunnel(opts TunnelOpts) error {
}
fmt.Fprintf(os.Stderr, "Building tunnel to %s\n", dbUrl.Host)

client, key, err := netssh.Connect(netssh.ConnectOpts{
// Just test the connection
client, _, err := netssh.Connect(netssh.ConnectOpts{
Host: sshhost,
Identity: opts.Identity,
})
Expand All @@ -73,16 +76,19 @@ func Tunnel(opts TunnelOpts) error {
}
return errgo.Notef(err, "fail to connect to SSH server")
}
waitingConnectionM := &sync.Mutex{}
client.Close()

if opts.Port == 0 {
opts.Port = defaultPort
}
if opts.Bind == "" {
opts.Bind = defaultBind
}

var tcpAddr *net.TCPAddr
var sock *net.TCPListener
for {
tcpAddr, err = net.ResolveTCPAddr("tcp", fmt.Sprintf("localhost:%d", opts.Port))
tcpAddr, err = net.ResolveTCPAddr("tcp", fmt.Sprintf("%s:%d", opts.Bind, opts.Port))
if err != nil {
return errgo.Mask(err)
}
Expand Down Expand Up @@ -117,34 +123,37 @@ func Tunnel(opts TunnelOpts) error {
return errgo.Mask(err)
}
debug.Println("New local connection")
// Checking not in reconnection process
waitingConnectionM.Lock()
waitingConnectionM.Unlock()

go func() {
for {
err := handleConnToTunnel(client, dbUrl, connToTunnel, errs)
var client *ssh.Client
retryConnect := true
for retryConnect {
// Do not reuse key since the connection to the SSH agent might be broken
client, _, err = netssh.Connect(netssh.ConnectOpts{
Host: sshhost,
Identity: opts.Identity,
})
if err != nil {
fmt.Println("Fail to reconnect, waiting 10 seconds...")
time.Sleep(10 * time.Second)
} else {
retryConnect = false
}
}

err = handleConnToTunnel(client, dbUrl, connToTunnel, errs)
if err != nil {
debug.Println("Error happened in tunnel", err)
if !opts.Reconnect {
errs <- err
return
}
}
if err == errTimeout {
waitingConnectionM.Lock()
fmt.Println("Connection broken, reconnecting...")
for err != nil {
client, err = netssh.ConnectToSSHServerWithKey(sshhost, key)
if err != nil {
fmt.Println("Fail to reconnect, waiting 10 seconds...")
time.Sleep(10 * time.Second)
}
}
fmt.Println("Reconnected!")
waitingConnectionM.Unlock()
// If err is nil, the connection has been closed normally, close the routine
if err == nil {
return
}
break
}
}()
}
Expand Down Expand Up @@ -198,6 +207,14 @@ func handleConnToTunnel(sshClient *ssh.Client, dbUrl *url.URL, sock net.Conn, er
fmt.Printf("End of connection [%d]\n", connID)
// Connection timeout
if err != nil && strings.Contains(err.Error(), "use of closed network") {

// If the connection has been closed by the CLIENT, we must stop here and return a nil error
// If the connection has been closed by the SERVER, we must return a errTimeout and retry the connection
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it legitimate that the server closes the connection sometimes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not on this connection, if the server closed the conneciton, it will break the conneciton at line 191. Here it's the ssh conneciton that broke.

clientClosedConnectionTest := fmt.Sprintf("%s->%s: use of closed network connection", sock.LocalAddr(), sock.RemoteAddr()) // Golang error checking 101 <3
if strings.Contains(err.Error(), clientClosedConnectionTest) {
return nil
}

return errTimeout
johnsudaar marked this conversation as resolved.
Show resolved Hide resolved
}
return nil
Expand Down