Merge pull request #9831 from hashicorp/remedial_remote_export_config_tests

add tests to make sure prepare for export defaults properly

builder/vmware/common/driver.go

@@ -80,6 +80,9 @@ type Driver interface {
 
 	// Export the vm to ovf or ova format using ovftool
 	Export([]string) error
+
+	// OvfTool
+	VerifyOvfTool(bool, bool) error
 }
 
 // NewDriver returns a new driver implementation for this operating
@@ -628,3 +631,22 @@ func (d *VmwareDriver) Export(args []string) error {
 
 	return nil
 }
+
+func (d *VmwareDriver) VerifyOvfTool(SkipExport, _ bool) error {
+	if SkipExport {
+		return nil
+	}
+
+	log.Printf("Verifying that ovftool exists...")
+	// Validate that tool exists, but no need to validate credentials.
+	ovftool := GetOVFTool()
+	if ovftool != "" {
+		return nil
+	} else {
+		return fmt.Errorf("Couldn't find ovftool in path! Please either " +
+			"set `skip_export = true` and remove the `format` option " +
+			"from your template, or make sure ovftool is installed on " +
+			"your build system. ")
+	}
+
+}

builder/vmware/common/driver_config.go

@@ -3,14 +3,8 @@
 package common
 
 import (
-	"bytes"
-	"context"
 	"fmt"
-	"net/url"
 	"os"
-	"os/exec"
-	"strings"
-	"time"
 
 	"github.com/hashicorp/packer/template/interpolate"
 )
@@ -93,65 +87,5 @@ func (c *DriverConfig) Validate(SkipExport bool) error {
 			"that you set a value for remote_password")
 	}
 
-	if c.RemoteType == "" {
-		// Validate that tool exists, but no need to validate credentials.
-		ovftool := GetOVFTool()
-		if ovftool != "" {
-			return nil
-		} else {
-			return fmt.Errorf("Couldn't find ovftool in path! Please either " +
-				"set `skip_export = true` and remove the `format` option " +
-				"from your template, or make sure ovftool is installed on " +
-				"your build system. ")
-		}
-	}
-
-	if c.SkipValidateCredentials {
-		return nil
-	}
-
-	// check that password is valid by sending a dummy ovftool command
-	// now, so that we don't fail for a simple mistake after a long
-	// build
-	ovftool := GetOVFTool()
-
-	// Generate the uri of the host, with embedded credentials
-	ovftool_uri := fmt.Sprintf("vi://%s", c.RemoteHost)
-	u, err := url.Parse(ovftool_uri)
-	if err != nil {
-		return fmt.Errorf("Couldn't generate uri for ovftool: %s", err)
-	}
-	u.User = url.UserPassword(c.RemoteUser, c.RemotePassword)
-
-	ovfToolArgs := []string{"--noSSLVerify", "--verifyOnly", u.String()}
-
-	var out bytes.Buffer
-	cmdCtx, cancel := context.WithTimeout(context.Background(), 15*time.Second)
-	defer cancel()
-	cmd := exec.CommandContext(cmdCtx, ovftool, ovfToolArgs...)
-	cmd.Stdout = &out
-
-	// Need to manually close stdin or else the ofvtool call will hang
-	// forever in a situation where the user has provided an invalid
-	// password or username
-	stdin, _ := cmd.StdinPipe()
-	defer stdin.Close()
-
-	if err := cmd.Run(); err != nil {
-		outString := out.String()
-		// The command *should* fail with this error, if it
-		// authenticates properly.
-		if !strings.Contains(outString, "Found wrong kind of object") {
-			err := fmt.Errorf("ovftool validation error: %s; %s",
-				err, outString)
-			if strings.Contains(outString,
-				"Enter login information for source") {
-				err = fmt.Errorf("The username or password you " +
-					"provided to ovftool is invalid.")
-			}
-			return err
-		}
-	}
-
 	return nil
 }

builder/vmware/common/driver_esx5.go

@@ -11,7 +11,9 @@ import (
 	"io"
 	"log"
 	"net"
+	"net/url"
 	"os"
+	"os/exec"
 	"path"
 	"path/filepath"
 	"strconv"
@@ -263,6 +265,68 @@ func (d *ESX5Driver) Verify() error {
 	return nil
 }
 
+func (d *ESX5Driver) VerifyOvfTool(SkipExport, skipValidateCredentials bool) error {
+	err := d.base.VerifyOvfTool(SkipExport, skipValidateCredentials)
+	if err != nil {
+		return err
+	}
+
+	log.Printf("Verifying that ovftool credentials are valid...")
+	// check that password is valid by sending a dummy ovftool command
+	// now, so that we don't fail for a simple mistake after a long
+	// build
+	ovftool := GetOVFTool()
+
+	if skipValidateCredentials {
+		return nil
+	}
+
+	if d.Password == "" {
+		return fmt.Errorf("exporting the vm from esxi with ovftool requires " +
+			"that you set a value for remote_password")
+	}
+
+	// Generate the uri of the host, with embedded credentials
+	ovftool_uri := fmt.Sprintf("vi://%s", d.Host)
+	u, err := url.Parse(ovftool_uri)
+	if err != nil {
+		return fmt.Errorf("Couldn't generate uri for ovftool: %s", err)
+	}
+	u.User = url.UserPassword(d.Username, d.Password)
+
+	ovfToolArgs := []string{"--noSSLVerify", "--verifyOnly", u.String()}
+
+	var out bytes.Buffer
+	cmdCtx, cancel := context.WithTimeout(context.Background(), 15*time.Second)
+	defer cancel()
+	cmd := exec.CommandContext(cmdCtx, ovftool, ovfToolArgs...)
+	cmd.Stdout = &out
+
+	// Need to manually close stdin or else the ofvtool call will hang
+	// forever in a situation where the user has provided an invalid
+	// password or username
+	stdin, _ := cmd.StdinPipe()
+	defer stdin.Close()
+
+	if err := cmd.Run(); err != nil {
+		outString := out.String()
+		// The command *should* fail with this error, if it
+		// authenticates properly.
+		if !strings.Contains(outString, "Found wrong kind of object") {
+			err := fmt.Errorf("ovftool validation error: %s; %s",
+				err, outString)
+			if strings.Contains(outString,
+				"Enter login information for source") {
+				err = fmt.Errorf("The username or password you " +
+					"provided to ovftool is invalid.")
+			}
+			return err
+		}
+	}
+
+	return nil
+}
+
 func (d *ESX5Driver) HostIP(multistep.StateBag) (string, error) {
 	conn, err := net.Dial("tcp", fmt.Sprintf("%s:%d", d.Host, d.Port))
 	if err != nil {

builder/vmware/common/driver_mock.go

@@ -95,6 +95,8 @@ type DriverMock struct {
 
 	VerifyCalled bool
 	VerifyErr    error
+
+	VerifyOvftoolCalled bool
 }
 
 type NetworkMapperMock struct {
@@ -279,3 +281,7 @@ func (d *DriverMock) GetVmwareDriver() VmwareDriver {
 	}
 	return state
 }
+
+func (d *DriverMock) VerifyOvfTool(_ bool, _ bool) error {
+	return nil
+}

builder/vmware/iso/builder.go

@@ -35,6 +35,11 @@ func (b *Builder) Run(ctx context.Context, ui packer.Ui, hook packer.Hook) (pack
 	if err != nil {
 		return nil, fmt.Errorf("Failed creating VMware driver: %s", err)
 	}
+	// Before we get deep into the build, make sure ovftool is present and
+	// credentials are valid, if we're going to use ovftool.
+	if err := driver.VerifyOvfTool(b.config.SkipExport, b.config.SkipValidateCredentials); err != nil {
+		return nil, err
+	}
 
 	// Setup the state bag
 	state := new(multistep.BasicStateBag)

builder/vmware/iso/builder_test.go

@@ -198,6 +198,123 @@ func TestBuilderPrepare_RemoteType(t *testing.T) {
 	}
 }
 
+func TestBuilderPrepare_Export(t *testing.T) {
+	type testCase struct {
+		InputConfigVals         map[string]string
+		ExpectedSkipExportValue bool
+		ExpectedFormat          string
+		ExpectedErr             bool
+		Reason                  string
+	}
+	testCases := []testCase{
+		{
+			InputConfigVals: map[string]string{
+				"remote_type": "",
+				"format":      "",
+			},
+			ExpectedSkipExportValue: true,
+			ExpectedFormat:          "vmx",
+			ExpectedErr:             false,
+			Reason:                  "should have defaulted format to vmx.",
+		},
+		{
+			InputConfigVals: map[string]string{
+				"remote_type":     "esx5",
+				"format":          "",
+				"remote_host":     "fakehost.com",
+				"remote_password": "fakepassword",
+				"remote_username": "fakeuser",
+			},
+			ExpectedSkipExportValue: false,
+			ExpectedFormat:          "ovf",
+			ExpectedErr:             false,
+			Reason:                  "should have defaulted format to ovf with remote set to esx5.",
+		},
+		{
+			InputConfigVals: map[string]string{
+				"remote_type": "esx5",
+				"format":      "",
+			},
+			ExpectedSkipExportValue: false,
+			ExpectedFormat:          "ovf",
+			ExpectedErr:             true,
+			Reason:                  "should have errored because remote host isn't set for remote build.",
+		},
+		{
+			InputConfigVals: map[string]string{
+				"remote_type":     "invalid",
+				"format":          "",
+				"remote_host":     "fakehost.com",
+				"remote_password": "fakepassword",
+				"remote_username": "fakeuser",
+			},
+			ExpectedSkipExportValue: false,
+			ExpectedFormat:          "ovf",
+			ExpectedErr:             true,
+			Reason:                  "should error with invalid remote type",
+		},
+		{
+			InputConfigVals: map[string]string{
+				"remote_type": "",
+				"format":      "invalid",
+			},
+			ExpectedSkipExportValue: false,
+			ExpectedFormat:          "invalid",
+			ExpectedErr:             true,
+			Reason:                  "should error with invalid format",
+		},
+		{
+			InputConfigVals: map[string]string{
+				"remote_type": "",
+				"format":      "ova",
+			},
+			ExpectedSkipExportValue: false,
+			ExpectedFormat:          "ova",
+			ExpectedErr:             false,
+			Reason:                  "should set user-given ova format",
+		},
+		{
+			InputConfigVals: map[string]string{
+				"remote_type":     "esx5",
+				"format":          "ova",
+				"remote_host":     "fakehost.com",
+				"remote_password": "fakepassword",
+				"remote_username": "fakeuser",
+			},
+			ExpectedSkipExportValue: false,
+			ExpectedFormat:          "ova",
+			ExpectedErr:             false,
+			Reason:                  "should set user-given ova format",
+		},
+	}
+	for _, tc := range testCases {
+		config := testConfig()
+		for k, v := range tc.InputConfigVals {
+			config[k] = v
+		}
+		config["skip_validate_credentials"] = true
+		outCfg := &Config{}
+		warns, errs := (outCfg).Prepare(config)
+
+		if len(warns) > 0 {
+			t.Fatalf("bad: %#v", warns)
+		}
+
+		if (errs != nil) != tc.ExpectedErr {
+			t.Fatalf("received error: \n %s \n but 'expected err' was %t", errs, tc.ExpectedErr)
+		}
+
+		if outCfg.Format != tc.ExpectedFormat {
+			t.Fatalf("Expected: %s. Actual: %s. Reason: %s", tc.ExpectedFormat,
+				outCfg.Format, tc.Reason)
+		}
+		if outCfg.SkipExport != tc.ExpectedSkipExportValue {
+			t.Fatalf("For SkipExport expected %t but recieved %t",
+				tc.ExpectedSkipExportValue, outCfg.SkipExport)
+		}
+	}
+}
+
 func TestBuilderPrepare_RemoteExport(t *testing.T) {
 	var b Builder
 	config := testConfig()

builder/vmware/iso/config.go

@@ -177,14 +177,14 @@ func (c *Config) Prepare(raws ...interface{}) ([]string, error) {
 	}
 
 	if c.Format == "" {
-		if c.RemoteType != "esx5" {
+		if c.RemoteType == "" {
 			c.Format = "vmx"
 		} else {
 			c.Format = "ovf"
 		}
 	}
 
-	if c.RemoteType != "esx5" && c.Format == "vmx" {
+	if c.RemoteType == "" && c.Format == "vmx" {
 		// if we're building locally and want a vmx, there's nothing to export.
 		// Set skip export flag here to keep the export step from attempting
 		// an unneded export

builder/vmware/vmx/builder.go

@@ -40,6 +40,11 @@ func (b *Builder) Run(ctx context.Context, ui packer.Ui, hook packer.Hook) (pack
 	if err != nil {
 		return nil, fmt.Errorf("Failed creating VMware driver: %s", err)
 	}
+	// Before we get deep into the build, make sure ovftool is present and
+	// credentials are valid, if we're going to use ovftool.
+	if err := driver.VerifyOvfTool(b.config.SkipExport, b.config.SkipValidateCredentials); err != nil {
+		return nil, err
+	}
 
 	// Set up the state.
 	state := new(multistep.BasicStateBag)

builder/vmware/vmx/config.go

@@ -124,14 +124,14 @@ func (c *Config) Prepare(raws ...interface{}) ([]string, error) {
 	}
 
 	if c.Format == "" {
-		if c.RemoteType != "esx5" {
+		if c.RemoteType == "" {
 			c.Format = "vmx"
 		} else {
 			c.Format = "ovf"
 		}
 	}
 
-	if c.RemoteType != "esx5" && c.Format == "vmx" {
+	if c.RemoteType == "" && c.Format == "vmx" {
 		// if we're building locally and want a vmx, there's nothing to export.
 		// Set skip export flag here to keep the export step from attempting
 		// an unneded export
@@ -143,14 +143,6 @@ func (c *Config) Prepare(raws ...interface{}) ([]string, error) {
 		errs = packer.MultiErrorAppend(errs, err)
 	}
 
-	if c.Format == "" {
-		if c.RemoteType != "esx5" {
-			c.Format = "vmx"
-		} else {
-			c.Format = "ovf"
-		}
-	}
-
 	// Warnings
 	var warnings []string
 	if c.ShutdownCommand == "" {

builder/vmware/vmx/config_test.go

@@ -58,3 +58,120 @@ func TestNewConfig_sourcePath(t *testing.T) {
 	warns, errs = (&Config{}).Prepare(cfg)
 	testConfigOk(t, warns, errs)
 }
+
+func TestNewConfig_exportConfig(t *testing.T) {
+	type testCase struct {
+		InputConfigVals         map[string]string
+		ExpectedSkipExportValue bool
+		ExpectedFormat          string
+		ExpectedErr             bool
+		Reason                  string
+	}
+	testCases := []testCase{
+		{
+			InputConfigVals: map[string]string{
+				"remote_type": "",
+				"format":      "",
+			},
+			ExpectedSkipExportValue: true,
+			ExpectedFormat:          "vmx",
+			ExpectedErr:             false,
+			Reason:                  "should have defaulted format to vmx.",
+		},
+		{
+			InputConfigVals: map[string]string{
+				"remote_type":     "esx5",
+				"format":          "",
+				"remote_host":     "fakehost.com",
+				"remote_password": "fakepassword",
+				"remote_username": "fakeuser",
+			},
+			ExpectedSkipExportValue: false,
+			ExpectedFormat:          "ovf",
+			ExpectedErr:             false,
+			Reason:                  "should have defaulted format to ovf with remote set to esx5.",
+		},
+		{
+			InputConfigVals: map[string]string{
+				"remote_type": "esx5",
+				"format":      "",
+			},
+			ExpectedSkipExportValue: false,
+			ExpectedFormat:          "ovf",
+			ExpectedErr:             true,
+			Reason:                  "should have errored because remote host isn't set for remote build.",
+		},
+		{
+			InputConfigVals: map[string]string{
+				"remote_type":     "invalid",
+				"format":          "",
+				"remote_host":     "fakehost.com",
+				"remote_password": "fakepassword",
+				"remote_username": "fakeuser",
+			},
+			ExpectedSkipExportValue: false,
+			ExpectedFormat:          "ovf",
+			ExpectedErr:             true,
+			Reason:                  "should error with invalid remote type",
+		},
+		{
+			InputConfigVals: map[string]string{
+				"remote_type": "",
+				"format":      "invalid",
+			},
+			ExpectedSkipExportValue: false,
+			ExpectedFormat:          "invalid",
+			ExpectedErr:             true,
+			Reason:                  "should error with invalid format",
+		},
+		{
+			InputConfigVals: map[string]string{
+				"remote_type": "",
+				"format":      "ova",
+			},
+			ExpectedSkipExportValue: false,
+			ExpectedFormat:          "ova",
+			ExpectedErr:             false,
+			Reason:                  "should set user-given ova format",
+		},
+		{
+			InputConfigVals: map[string]string{
+				"remote_type":     "esx5",
+				"format":          "ova",
+				"remote_host":     "fakehost.com",
+				"remote_password": "fakepassword",
+				"remote_username": "fakeuser",
+			},
+			ExpectedSkipExportValue: false,
+			ExpectedFormat:          "ova",
+			ExpectedErr:             false,
+			Reason:                  "should set user-given ova format",
+		},
+	}
+	for _, tc := range testCases {
+		cfg := testConfig(t)
+		for k, v := range tc.InputConfigVals {
+			cfg[k] = v
+		}
+		cfg["skip_validate_credentials"] = true
+		outCfg := &Config{}
+		warns, errs := (outCfg).Prepare(cfg)
+
+		if len(warns) > 0 {
+			t.Fatalf("bad: %#v", warns)
+		}
+
+		if (errs != nil) != tc.ExpectedErr {
+			t.Fatalf("received error: \n %s \n but 'expected err' was %t", errs, tc.ExpectedErr)
+		}
+
+		if outCfg.Format != tc.ExpectedFormat {
+			t.Fatalf("Expected: %s. Actual: %s. Reason: %s", tc.ExpectedFormat,
+				outCfg.Format, tc.Reason)
+		}
+		if outCfg.SkipExport != tc.ExpectedSkipExportValue {
+			t.Fatalf("For SkipExport expected %t but recieved %t",
+				tc.ExpectedSkipExportValue, outCfg.SkipExport)
+		}
+	}
+}