Merge pull request #28665 from hashicorp/alisdair/communicator-bastion-port-type

communicator/ssh: Fix crash with SSH bastion port

communicator/shared/shared.go

@@ -36,7 +36,7 @@ var ConnectionBlockSupersetSchema = &configschema.Block{
 			Optional: true,
 		},
 		"port": {
-			Type:     cty.String,
+			Type:     cty.Number,
 			Optional: true,
 		},
 		"timeout": {

communicator/ssh/communicator_test.go

@@ -331,7 +331,7 @@ func TestHostKey(t *testing.T) {
 		Password: "pass",
 		Host:     host,
 		HostKey:  pubKey,
-		Port:     port,
+		Port:     uint16(port),
 		Timeout:  "30s",
 	}
 
@@ -363,7 +363,7 @@ func TestHostKey(t *testing.T) {
 	port, _ = strconv.Atoi(p)
 
 	connInfo.HostKey = testClientPublicKey
-	connInfo.Port = port
+	connInfo.Port = uint16(port)
 
 	cfg, err = prepareSSHConfig(connInfo)
 	if err != nil {
@@ -406,7 +406,7 @@ func TestHostCert(t *testing.T) {
 		Password: "pass",
 		Host:     host,
 		HostKey:  testCAPublicKey,
-		Port:     port,
+		Port:     uint16(port),
 		Timeout:  "30s",
 	}
 
@@ -438,7 +438,7 @@ func TestHostCert(t *testing.T) {
 	port, _ = strconv.Atoi(p)
 
 	connInfo.HostKey = testClientPublicKey
-	connInfo.Port = port
+	connInfo.Port = uint16(port)
 
 	cfg, err = prepareSSHConfig(connInfo)
 	if err != nil {
@@ -528,7 +528,7 @@ func TestCertificateBasedAuth(t *testing.T) {
 		Host:        host,
 		PrivateKey:  CLIENT_PEM,
 		Certificate: CLIENT_CERT_SIGNED_BY_SERVER,
-		Port:        port,
+		Port:        uint16(port),
 		Timeout:     "30s",
 	}
 

communicator/ssh/provisioner.go

@@ -10,13 +10,13 @@ import (
 	"net"
 	"os"
 	"path/filepath"
-	"strconv"
 	"strings"
 	"time"
 
 	"github.com/hashicorp/terraform/communicator/shared"
 	sshagent "github.com/xanzy/ssh-agent"
 	"github.com/zclconf/go-cty/cty"
+	"github.com/zclconf/go-cty/cty/gocty"
 	"golang.org/x/crypto/ssh"
 	"golang.org/x/crypto/ssh/agent"
 	"golang.org/x/crypto/ssh/knownhosts"
@@ -55,7 +55,7 @@ type connectionInfo struct {
 	Certificate    string
 	Host           string
 	HostKey        string
-	Port           int
+	Port           uint16
 	Agent          bool
 	ScriptPath     string
 	TargetPlatform string
@@ -68,7 +68,7 @@ type connectionInfo struct {
 	BastionCertificate string
 	BastionHost        string
 	BastionHostKey     string
-	BastionPort        int
+	BastionPort        uint16
 
 	AgentIdentity string
 }
@@ -101,11 +101,9 @@ func decodeConnInfo(v cty.Value) (*connectionInfo, error) {
 		case "host_key":
 			connInfo.HostKey = v.AsString()
 		case "port":
-			p, err := strconv.Atoi(v.AsString())
+			if err := gocty.FromCtyValue(v, &connInfo.Port); err != nil {
-			if err != nil {
 				return nil, err
 			}
-			connInfo.Port = p
 		case "agent":
 			connInfo.Agent = v.True()
 		case "script_path":
@@ -127,11 +125,9 @@ func decodeConnInfo(v cty.Value) (*connectionInfo, error) {
 		case "bastion_host_key":
 			connInfo.BastionHostKey = v.AsString()
 		case "bastion_port":
-			p, err := strconv.Atoi(v.AsString())
+			if err := gocty.FromCtyValue(v, &connInfo.BastionPort); err != nil {
-			if err != nil {
 				return nil, err
 			}
-			connInfo.BastionPort = p
 		case "agent_identity":
 			connInfo.AgentIdentity = v.AsString()
 		}

communicator/ssh/provisioner_test.go

@@ -17,6 +17,7 @@ func TestProvisioner_connInfo(t *testing.T) {
 		"port":         cty.StringVal("22"),
 		"timeout":      cty.StringVal("30s"),
 		"bastion_host": cty.StringVal("127.0.1.1"),
+		"bastion_port": cty.NumberIntVal(20022),
 	})
 
 	conf, err := parseConnectionInfo(v)
@@ -54,7 +55,7 @@ func TestProvisioner_connInfo(t *testing.T) {
 	if conf.BastionHost != "127.0.1.1" {
 		t.Fatalf("bad: %v", conf)
 	}
-	if conf.BastionPort != 22 {
+	if conf.BastionPort != 20022 {
 		t.Fatalf("bad: %v", conf)
 	}
 	if conf.BastionUser != "root" {
@@ -135,3 +136,45 @@ func TestProvisioner_connInfoEmptyHostname(t *testing.T) {
 		t.Fatalf("bad: should not allow empty host")
 	}
 }
+
+func TestProvisioner_stringBastionPort(t *testing.T) {
+	v := cty.ObjectVal(map[string]cty.Value{
+		"type":         cty.StringVal("ssh"),
+		"user":         cty.StringVal("root"),
+		"password":     cty.StringVal("supersecret"),
+		"private_key":  cty.StringVal("someprivatekeycontents"),
+		"host":         cty.StringVal("example.com"),
+		"port":         cty.StringVal("22"),
+		"timeout":      cty.StringVal("30s"),
+		"bastion_host": cty.StringVal("example.com"),
+		"bastion_port": cty.StringVal("12345"),
+	})
+
+	conf, err := parseConnectionInfo(v)
+	if err != nil {
+		t.Fatalf("err: %v", err)
+	}
+
+	if conf.BastionPort != 12345 {
+		t.Fatalf("bad %v", conf)
+	}
+}
+
+func TestProvisioner_invalidPortNumber(t *testing.T) {
+	v := cty.ObjectVal(map[string]cty.Value{
+		"type":        cty.StringVal("ssh"),
+		"user":        cty.StringVal("root"),
+		"password":    cty.StringVal("supersecret"),
+		"private_key": cty.StringVal("someprivatekeycontents"),
+		"host":        cty.StringVal("example.com"),
+		"port":        cty.NumberIntVal(123456789),
+	})
+
+	_, err := parseConnectionInfo(v)
+	if err == nil {
+		t.Fatalf("bad: should not allow invalid port number")
+	}
+	if got, want := err.Error(), "value must be a whole number, between 0 and 65535 inclusive"; got != want {
+		t.Errorf("unexpected error\n got: %s\nwant: %s", got, want)
+	}
+}

communicator/winrm/communicator.go

@@ -33,7 +33,7 @@ func New(v cty.Value) (*Communicator, error) {
 
 	endpoint := &winrm.Endpoint{
 		Host:     connInfo.Host,
-		Port:     connInfo.Port,
+		Port:     int(connInfo.Port),
 		HTTPS:    connInfo.HTTPS,
 		Insecure: connInfo.Insecure,
 		Timeout:  connInfo.TimeoutVal,

communicator/winrm/provisioner.go

@@ -4,12 +4,12 @@ import (
 	"fmt"
 	"log"
 	"path/filepath"
-	"strconv"
 	"strings"
 	"time"
 
 	"github.com/hashicorp/terraform/communicator/shared"
 	"github.com/zclconf/go-cty/cty"
+	"github.com/zclconf/go-cty/cty/gocty"
 )
 
 const (
@@ -37,7 +37,7 @@ type connectionInfo struct {
 	User       string
 	Password   string
 	Host       string
-	Port       int
+	Port       uint16
 	HTTPS      bool
 	Insecure   bool
 	NTLM       bool   `mapstructure:"use_ntlm"`
@@ -69,11 +69,9 @@ func decodeConnInfo(v cty.Value) (*connectionInfo, error) {
 		case "host":
 			connInfo.Host = v.AsString()
 		case "port":
-			p, err := strconv.Atoi(v.AsString())
+			if err := gocty.FromCtyValue(v, &connInfo.Port); err != nil {
-			if err != nil {
 				return nil, err
 			}
-			connInfo.Port = p
 		case "https":
 			connInfo.HTTPS = v.True()
 		case "insecure":

terraform/node_resource_validate.go

@@ -177,7 +177,7 @@ var connectionBlockSupersetSchema = &configschema.Block{
 			Optional: true,
 		},
 		"port": {
-			Type:     cty.String,
+			Type:     cty.Number,
 			Optional: true,
 		},
 		"timeout": {

terraform/node_resource_validate_test.go

@@ -31,6 +31,7 @@ func TestNodeValidatableResource_ValidateProvisioner_valid(t *testing.T) {
 			Config: configs.SynthBody("", map[string]cty.Value{
 				"host": cty.StringVal("localhost"),
 				"type": cty.StringVal("ssh"),
+				"port": cty.NumberIntVal(10022),
 			}),
 		},
 	}
@@ -104,7 +105,7 @@ func TestNodeValidatableResource_ValidateProvisioner__warning(t *testing.T) {
 	}
 }
 
-func TestNodeValidatableResource_ValidateProvisioner__conntectionInvalid(t *testing.T) {
+func TestNodeValidatableResource_ValidateProvisioner__connectionInvalid(t *testing.T) {
 	ctx := &MockEvalContext{}
 	ctx.installSimpleEval()
 	mp := &MockProvisioner{}