cli/server/add.go

@@ -23,29 +23,29 @@ var localFlag = &cli.BoolFlag{
 	Destination: &local,
 }
 
-func cleanUp(domainName string) {
+// cleanUp cleans up the partially created context/client details for a failed
-	if domainName != "default" {
+// "server add" attempt.
-		logrus.Infof("cleaning up context for %s", domainName)
+func cleanUp(name string) {
-		if err := client.DeleteContext(domainName); err != nil {
+	if name != "default" {
+		logrus.Debugf("serverAdd: cleanUp: cleaning up context for %s", name)
+		if err := client.DeleteContext(name); err != nil {
 			logrus.Fatal(err)
 		}
 	}
 
-	logrus.Infof("attempting to clean up server directory for %s", domainName)
+	serverDir := filepath.Join(config.SERVERS_DIR, name)
-
-	serverDir := filepath.Join(config.SERVERS_DIR, domainName)
 	files, err := config.GetAllFilesInDirectory(serverDir)
 	if err != nil {
-		logrus.Fatalf("unable to list files in %s: %s", serverDir, err)
+		logrus.Fatalf("serverAdd: cleanUp: unable to list files in %s: %s", serverDir, err)
 	}
 
 	if len(files) > 0 {
-		logrus.Warnf("aborting clean up of %s because it is not empty", serverDir)
+		logrus.Debugf("serverAdd: cleanUp: %s is not empty, aborting cleanup", serverDir)
 		return
 	}
 
 	if err := os.RemoveAll(serverDir); err != nil {
-		logrus.Fatal(err)
+		logrus.Fatalf("serverAdd: cleanUp: failed to remove %s: %s", serverDir, err)
 	}
 }
 
@@ -53,39 +53,42 @@ func cleanUp(domainName string) {
 // Docker manages SSH connection details. These are stored to disk in
 // ~/.docker. Abra can manage this completely for the user, so it's an
 // implementation detail.
-func newContext(c *cli.Context, domainName string) error {
+func newContext(name string) (bool, error) {
 	store := contextPkg.NewDefaultDockerContextStore()
 	contexts, err := store.Store.List()
 	if err != nil {
-		return err
+		return false, err
 	}
 
 	for _, context := range contexts {
-		if context.Name == domainName {
+		if context.Name == name {
-			logrus.Debugf("context for %s already exists", domainName)
+			logrus.Debugf("context for %s already exists", name)
-			return nil
+			return false, nil
 		}
 	}
 
-	logrus.Debugf("creating context with domain %s", domainName)
+	logrus.Debugf("creating context with domain %s", name)
 
-	if err := client.CreateContext(domainName); err != nil {
+	if err := client.CreateContext(name); err != nil {
-		return err
+		return false, nil
 	}
 
-	return nil
+	return true, nil
 }
 
 // createServerDir creates the ~/.abra/servers/... directory for a new server.
-func createServerDir(domainName string) error {
+func createServerDir(name string) (bool, error) {
-	if err := server.CreateServerDir(domainName); err != nil {
+	if err := server.CreateServerDir(name); err != nil {
 		if !os.IsExist(err) {
-			return err
+			return false, err
-		}
-		logrus.Debugf("server dir for %s already created", domainName)
 		}
 
-	return nil
+		logrus.Debugf("server dir for %s already created", name)
+
+		return false, nil
+	}
+
+	return true, nil
 }
 
 var serverAddCommand = cli.Command{
@@ -95,9 +98,9 @@ var serverAddCommand = cli.Command{
 	Description: `
 Add a new server to your configuration so that it can be managed by Abra.
 
-Abra uses the SSH command-line to discover connection details for your server.
+Abra relies on the standard SSH command-line and ~/.ssh/config for client
-It is advised to configure an entry per-host in your ~/.ssh/config for each
+connection details. You must configure an entry per-host in your ~/.ssh/config
-server. For example:
+for each server. For example:
 
   Host example.com example
     Hostname example.com
@@ -105,22 +108,20 @@ Host example.com example
     Port 12345
     IdentityFile ~/.ssh/example@somewhere
 
-Abra can then load SSH connection details from this configuratiion with:
+You can then add a server like so:
 
   abra server add example.com
 
-Or using the name "example" which is not resolvable on the public Internet:
-
-    abra server add -D example
-
 If "--local" is passed, then Abra assumes that the current local server is
 intended as the target server. This is useful when you want to have your entire
 Co-op Cloud config located on the server itself, and not on your local
 developer machine. The domain is then set to "default".
 
 You can also pass "--no-domain-checks/-D" flag to use any arbitrary name
-instead of a real domain. Host will be resolved with the "hostname" entry of
+instead of a real domain. The host will be resolved with the "Hostname" entry
-your SSH configuration. Checks for a valid online domain will be skipped.
+of your ~/.ssh/config. Checks for a valid online domain will be skipped:
+
+  abra server add -D example
 `,
 	Flags: []cli.Flag{
 		internal.DebugFlag,
@@ -129,59 +130,75 @@ your SSH configuration. Checks for a valid online domain will be skipped.
 		localFlag,
 	},
 	Before:    internal.SubCommandBefore,
-	ArgsUsage: "<domain>",
+	ArgsUsage: "<name>",
 	Action: func(c *cli.Context) error {
 		if len(c.Args()) > 0 && local || !internal.ValidateSubCmdFlags(c) {
-			err := errors.New("cannot use <domain> and --local together")
+			err := errors.New("cannot use <name> and --local together")
 			internal.ShowSubcommandHelpAndError(c, err)
 		}
 
-		var domainName string
+		var name string
 		if local {
-			domainName = "default"
+			name = "default"
 		} else {
-			domainName = internal.ValidateDomain(c)
+			name = internal.ValidateDomain(c)
 		}
 
+		// NOTE(d1): reasonable 5 second timeout for connections which can't
+		// succeed. The connection is attempted twice, so this results in 10
+		// seconds.
+		timeout := client.WithTimeout(5)
+
 		if local {
-			if err := createServerDir(domainName); err != nil {
+			created, err := createServerDir(name)
+			if err != nil {
 				logrus.Fatal(err)
 			}
 
-			logrus.Infof("attempting to create client for %s", domainName)
+			logrus.Debugf("attempting to create client for %s", name)
-			if _, err := client.New(domainName); err != nil {
+
-				cleanUp(domainName)
+			if _, err := client.New(name, timeout); err != nil {
+				cleanUp(name)
 				logrus.Fatal(err)
 			}
 
-			logrus.Info("local server added")
+			if created {
+				logrus.Info("local server successfully added")
+			} else {
+				logrus.Warn("local server already exists")
+			}
 
 			return nil
 		}
 
 		if !internal.NoDomainChecks {
-			if _, err := dns.EnsureIPv4(domainName); err != nil {
+			if _, err := dns.EnsureIPv4(name); err != nil {
 				logrus.Fatal(err)
 			}
 		}
 
-		if err := createServerDir(domainName); err != nil {
+		_, err := createServerDir(name)
+		if err != nil {
 			logrus.Fatal(err)
 		}
 
-		if err := newContext(c, domainName); err != nil {
+		created, err := newContext(name)
-			cleanUp(domainName)
+		if err != nil {
+			cleanUp(name)
 			logrus.Fatal(err)
 		}
 
-		logrus.Infof("attempting to create client for %s", domainName)
+		logrus.Debugf("attempting to create client for %s", name)
-		if _, err := client.New(domainName); err != nil {
+		if _, err := client.New(name, timeout); err != nil {
-			cleanUp(domainName)
+			cleanUp(name)
-			logrus.Debugf("failed to construct client for %s, saw %s", domainName, err.Error())
+			logrus.Fatal(sshPkg.Fatal(name, err))
-			logrus.Fatal(sshPkg.Fatal(domainName, err))
 		}
 
-		logrus.Infof("%s added", domainName)
+		if created {
+			logrus.Infof("%s successfully added", name)
+		} else {
+			logrus.Warnf("%s already exists", name)
+		}
 
 		return nil
 	},

cli/server/remove.go

@@ -41,7 +41,7 @@ rain.
 			logrus.Fatal(err)
 		}
 
-		logrus.Infof("server at %s has been lost in time, like tears in rain", serverName)
+		logrus.Infof("%s is now lost in time, like tears in rain", serverName)
 
 		return nil
 	},

pkg/client/client.go

@@ -16,11 +16,26 @@ import (
 	"github.com/sirupsen/logrus"
 )
 
+// Conf is a Docker client configuration.
+type Conf struct {
+	Timeout int
+}
+
+// Opt is a Docker client option.
+type Opt func(c *Conf)
+
+// WithTimeout specifies a timeout for a Docker client.
+func WithTimeout(timeout int) Opt {
+	return func(c *Conf) {
+		c.Timeout = timeout
+	}
+}
+
 // New initiates a new Docker client. New client connections are validated so
 // that we ensure connections via SSH to the daemon can succeed. It takes into
 // account that you may only want the local client and not communicate via SSH.
 // For this use-case, please pass "default" as the contextName.
-func New(serverName string) (*client.Client, error) {
+func New(serverName string, opts ...Opt) (*client.Client, error) {
 	var clientOpts []client.Opt
 
 	if serverName != "default" {
@@ -34,7 +49,12 @@ func New(serverName string) (*client.Client, error) {
 			return nil, err
 		}
 
-		helper, err := commandconnPkg.NewConnectionHelper(ctxEndpoint)
+		conf := &Conf{}
+		for _, opt := range opts {
+			opt(conf)
+		}
+
+		helper, err := commandconnPkg.NewConnectionHelper(ctxEndpoint, conf.Timeout)
 		if err != nil {
 			return nil, err
 		}
@@ -75,9 +95,9 @@ func New(serverName string) (*client.Client, error) {
 	if info.Swarm.LocalNodeState == "inactive" {
 		if serverName != "default" {
 			return cl, fmt.Errorf("swarm mode not enabled on %s?", serverName)
-		} else {
-			return cl, errors.New("swarm mode not enabled on local server?")
 		}
+
+		return cl, errors.New("swarm mode not enabled on local server?")
 	}
 
 	return cl, nil

pkg/server/server.go

@@ -17,11 +17,11 @@ func CreateServerDir(serverName string) error {
 			return err
 		}
 
-		logrus.Infof("%s already exists", serverPath)
+		logrus.Debugf("%s already exists", serverPath)
 		return nil
 	}
 
-	logrus.Infof("successfully created %s", serverPath)
+	logrus.Debugf("successfully created %s", serverPath)
 
 	return nil
 }

pkg/ssh/ssh.go

@@ -19,7 +19,7 @@ func Fatal(hostname string, err error) error {
 	} else if strings.Contains(out, "Permission denied") {
 		return fmt.Errorf("ssh auth: permission denied for %s", hostname)
 	} else if strings.Contains(out, "Network is unreachable") {
-		return fmt.Errorf("unable to connect to %s, network is unreachable?", hostname)
+		return fmt.Errorf("unable to connect to %s, please check your SSH config", hostname)
 	}
 
 	return err

pkg/upstream/commandconn/connection.go

@@ -2,6 +2,7 @@ package commandconn
 
 import (
 	"context"
+	"fmt"
 	"net"
 	"net/url"
 
@@ -14,14 +15,17 @@ import (
 )
 
 // GetConnectionHelper returns Docker-specific connection helper for the given URL.
-// GetConnectionHelper returns nil without error when no helper is registered for the scheme.
+func GetConnectionHelper(daemonURL string, timeout int) (*connhelper.ConnectionHelper, error) {
-//
+	if timeout != 0 {
-// ssh://<host> URL requires Docker 18.09 or later on the remote host.
+		return getConnectionHelper(daemonURL, []string{fmt.Sprintf("-o ConnectTimeout=%v", timeout)})
-func GetConnectionHelper(daemonURL string) (*connhelper.ConnectionHelper, error) {
-	return getConnectionHelper(daemonURL)
 	}
 
-func getConnectionHelper(daemonURL string) (*connhelper.ConnectionHelper, error) {
+	return getConnectionHelper(daemonURL, []string{})
+}
+
+// getConnectionHelper generates a connection helper from the underlying Docker
+// libraries.
+func getConnectionHelper(daemonURL string, sshFlags []string) (*connhelper.ConnectionHelper, error) {
 	url, err := url.Parse(daemonURL)
 	if err != nil {
 		return nil, err
@@ -35,7 +39,7 @@ func getConnectionHelper(daemonURL string) (*connhelper.ConnectionHelper, error)
 
 		return &connhelper.ConnectionHelper{
 			Dialer: func(ctx context.Context, network, addr string) (net.Conn, error) {
-				return New(ctx, "ssh", ctxConnDetails.Args("docker", "system", "dial-stdio")...)
+				return New(ctx, "ssh", append(sshFlags, ctxConnDetails.Args("docker", "system", "dial-stdio")...)...)
 			},
 			Host: "http://docker.example.com",
 		}, nil
@@ -46,8 +50,8 @@ func getConnectionHelper(daemonURL string) (*connhelper.ConnectionHelper, error)
 }
 
 // NewConnectionHelper creates new connection helper for a remote docker daemon.
-func NewConnectionHelper(daemonURL string) (*connhelper.ConnectionHelper, error) {
+func NewConnectionHelper(daemonURL string, timeout int) (*connhelper.ConnectionHelper, error) {
-	helper, err := GetConnectionHelper(daemonURL)
+	helper, err := GetConnectionHelper(daemonURL, timeout)
 	if err != nil {
 		return nil, err
 	}

tests/integration/server_add.bats

@@ -1,8 +1,25 @@
 #!/usr/bin/env bash
 
+setup_file(){
+  load "$PWD/tests/integration/helpers/common"
+  _common_setup
+
+  run docker swarm leave --force
+}
+
 setup(){
   load "$PWD/tests/integration/helpers/common"
   _common_setup
+
+  run docker swarm init
+  assert_success
+}
+
+teardown(){
+  run docker swarm leave --force
+  assert_success
+
+  _rm_server
 }
 
 @test "add new server" {
@@ -16,29 +33,24 @@ setup(){
   assert bash -c "docker context ls | grep -q $TEST_SERVER"
 }
 
-@test "error if using domain and --local together" {
+@test "error if using name and --local together" {
   run $ABRA server add "$TEST_SERVER" --local
   assert_failure
-  assert_output --partial 'cannot use <domain> and --local together'
+  assert_output --partial 'cannot use <name> and --local together'
 }
 
 @test "create local server" {
-  run docker swarm init
-  assert_success
-
   run $ABRA server add --local
   assert_success
   assert_exists "$ABRA_DIR/servers/default"
   assert bash -c "docker context ls | grep -q default"
-  assert_output --partial 'local server added'
+  assert_output --partial 'local server successfully added'
-
-  docker swarm leave --force
-  assert_success
-
-  _rm_default_server
 }
 
 @test "create local server fails when no docker swarm" {
+  run docker swarm leave --force
+  assert_success
+
   run $ABRA server add --local
   assert_failure
   assert_not_exists "$ABRA_DIR/servers/default"