From 1d80809a16fc97d7351824d2c921578133a93f65 Mon Sep 17 00:00:00 2001
From: Tom Clegg <tclegg@veritasgenetics.com>
Date: Thu, 17 Jan 2019 12:01:15 -0500
Subject: [PATCH] 14325: Configurable SSH target port for cloud VMs.

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tclegg@veritasgenetics.com>
---
 lib/dispatchcloud/dispatcher.go               |  1 +
 lib/dispatchcloud/ssh_executor/executor.go    | 26 ++++++++++++--
 .../ssh_executor/executor_test.go             | 36 +++++++++++++++++++
 sdk/go/arvados/config.go                      |  7 +++-
 4 files changed, 66 insertions(+), 4 deletions(-)

diff --git a/lib/dispatchcloud/dispatcher.go b/lib/dispatchcloud/dispatcher.go
index 7884633304..ae954a2db9 100644
--- a/lib/dispatchcloud/dispatcher.go
+++ b/lib/dispatchcloud/dispatcher.go
@@ -89,6 +89,7 @@ func (disp *dispatcher) Close() {
 // Make a worker.Executor for the given instance.
 func (disp *dispatcher) newExecutor(inst cloud.Instance) worker.Executor {
 	exr := ssh_executor.New(inst)
+	exr.SetTargetPort(disp.Cluster.CloudVMs.SSHPort)
 	exr.SetSigners(disp.sshKey)
 	return exr
 }
diff --git a/lib/dispatchcloud/ssh_executor/executor.go b/lib/dispatchcloud/ssh_executor/executor.go
index 4b2478e940..d0fb54c54c 100644
--- a/lib/dispatchcloud/ssh_executor/executor.go
+++ b/lib/dispatchcloud/ssh_executor/executor.go
@@ -36,9 +36,10 @@ func New(t cloud.ExecutorTarget) *Executor {
 //
 // An Executor must not be copied.
 type Executor struct {
-	target  cloud.ExecutorTarget
-	signers []ssh.Signer
-	mtx     sync.RWMutex // controls access to instance after creation
+	target     cloud.ExecutorTarget
+	targetPort string
+	signers    []ssh.Signer
+	mtx        sync.RWMutex // controls access to instance after creation
 
 	client      *ssh.Client
 	clientErr   error
@@ -67,6 +68,17 @@ func (exr *Executor) SetTarget(t cloud.ExecutorTarget) {
 	exr.target = t
 }
 
+// SetTargetPort sets the default port (name or number) to connect
+// to. This is used only when the address returned by the target's
+// Address() method does not specify a port. If the given port is
+// empty (or SetTargetPort is not called at all), the default port is
+// "ssh".
+func (exr *Executor) SetTargetPort(port string) {
+	exr.mtx.Lock()
+	defer exr.mtx.Unlock()
+	exr.targetPort = port
+}
+
 // Target returns the current target.
 func (exr *Executor) Target() cloud.ExecutorTarget {
 	exr.mtx.RLock()
@@ -167,6 +179,14 @@ func (exr *Executor) setupSSHClient() (*ssh.Client, error) {
 	if addr == "" {
 		return nil, errors.New("instance has no address")
 	}
+	if h, p, err := net.SplitHostPort(addr); err != nil || p == "" {
+		// Target address does not specify a port.  Use
+		// targetPort, or "ssh".
+		if p = exr.targetPort; p == "" {
+			p = "ssh"
+		}
+		addr = net.JoinHostPort(h, p)
+	}
 	var receivedKey ssh.PublicKey
 	client, err := ssh.Dial("tcp", addr, &ssh.ClientConfig{
 		User: "root",
diff --git a/lib/dispatchcloud/ssh_executor/executor_test.go b/lib/dispatchcloud/ssh_executor/executor_test.go
index 619e47383f..526840b137 100644
--- a/lib/dispatchcloud/ssh_executor/executor_test.go
+++ b/lib/dispatchcloud/ssh_executor/executor_test.go
@@ -8,6 +8,7 @@ import (
 	"bytes"
 	"io"
 	"io/ioutil"
+	"net"
 	"sync"
 	"testing"
 	"time"
@@ -32,6 +33,25 @@ func (*testTarget) VerifyHostKey(ssh.PublicKey, *ssh.Client) error {
 	return nil
 }
 
+// Address returns the wrapped SSHService's host, with the port
+// stripped. This ensures the executor won't work until
+// SetTargetPort() is called -- see (*testTarget)Port().
+func (tt *testTarget) Address() string {
+	h, _, err := net.SplitHostPort(tt.SSHService.Address())
+	if err != nil {
+		panic(err)
+	}
+	return h
+}
+
+func (tt *testTarget) Port() string {
+	_, p, err := net.SplitHostPort(tt.SSHService.Address())
+	if err != nil {
+		panic(err)
+	}
+	return p
+}
+
 type ExecutorSuite struct{}
 
 func (s *ExecutorSuite) TestExecute(c *check.C) {
@@ -77,6 +97,22 @@ func (s *ExecutorSuite) TestExecute(c *check.C) {
 		exr := New(srv)
 		exr.SetSigners(clientpriv)
 
+		// Use the default target port (ssh). Execute will
+		// return a connection error or an authentication
+		// error, depending on whether the test host is
+		// running an SSH server.
+		_, _, err = exr.Execute(nil, command, nil)
+		c.Check(err, check.ErrorMatches, `.*(unable to authenticate|connection refused).*`)
+
+		// Use a bogus target port. Execute will return a
+		// connection error.
+		exr.SetTargetPort("0")
+		_, _, err = exr.Execute(nil, command, nil)
+		c.Check(err, check.ErrorMatches, `.*connection refused.*`)
+
+		// Use the test server's listening port.
+		exr.SetTargetPort(srv.Port())
+
 		done := make(chan bool)
 		go func() {
 			stdout, stderr, err := exr.Execute(map[string]string{"TESTVAR": "test value"}, command, bytes.NewBufferString(stdinData))
diff --git a/sdk/go/arvados/config.go b/sdk/go/arvados/config.go
index bfa86abf6a..1154f922ba 100644
--- a/sdk/go/arvados/config.go
+++ b/sdk/go/arvados/config.go
@@ -121,7 +121,12 @@ type CloudVMs struct {
 	// and ready to run containers, e.g., "mount | grep
 	// /encrypted-tmp"
 	BootProbeCommand string
-	SyncInterval     Duration
+
+	// Listening port (name or number) of SSH servers on worker
+	// VMs
+	SSHPort string
+
+	SyncInterval Duration
 
 	// Maximum idle time before automatic shutdown
 	TimeoutIdle Duration
-- 
2.39.5