From 51b2069af15c764a2d20a56b76d7fe8688b80519 Mon Sep 17 00:00:00 2001 From: James Bishopp Date: Mon, 13 Jul 2015 07:56:11 -0700 Subject: [PATCH 1/2] Added fixer for ssh_key_path - ssh_key_path is removed in all cases. - The new fixer is not checking the builder type so it will execute for all builders. - If ssh_key_path exists without ssh_private_key_file then it will assign the value of ssh_path_path to ssh_private_key_file - If ssh_private_key_file and ssh_key_path exist, then ssh_key_path is simply removed I wanted to emit a warning messsage to the user but it didn't seem like the ui.* interface was exposed to fixers. --- fix/fixer.go | 2 + fix/fixer_sshkeypath.go | 50 ++++++++++++++++++++++ fix/fixer_sshkeypath_test.go | 83 ++++++++++++++++++++++++++++++++++++ 3 files changed, 135 insertions(+) create mode 100644 fix/fixer_sshkeypath.go create mode 100644 fix/fixer_sshkeypath_test.go diff --git a/fix/fixer.go b/fix/fixer.go index 5f7dd7a12..78f58f546 100644 --- a/fix/fixer.go +++ b/fix/fixer.go @@ -28,6 +28,7 @@ func init() { "vmware-rename": new(FixerVMwareRename), "parallels-headless": new(FixerParallelsHeadless), "parallels-deprecations": new(FixerParallelsDeprecations), + "sshkeypath": new(FixerSSHKeyPath), } FixerOrder = []string{ @@ -39,5 +40,6 @@ func init() { "vmware-rename", "parallels-headless", "parallels-deprecations", + "sshkeypath", } } diff --git a/fix/fixer_sshkeypath.go b/fix/fixer_sshkeypath.go new file mode 100644 index 000000000..b57cf762b --- /dev/null +++ b/fix/fixer_sshkeypath.go @@ -0,0 +1,50 @@ +package fix + +import ( + "github.com/mitchellh/mapstructure" +) + +// FixerSSHKeyPath changes the "ssh_key_path" of a template +// to "ssh_private_key_file". +type FixerSSHKeyPath struct{} + +func (FixerSSHKeyPath) Fix(input map[string]interface{}) (map[string]interface{}, error) { + // The type we'll decode into; we only care about builders + type template struct { + Builders []map[string]interface{} + } + + // Decode the input into our structure, if we can + var tpl template + if err := mapstructure.Decode(input, &tpl); err != nil { + return nil, err + } + + for _, builder := range tpl.Builders { + sshKeyPathRaw, ok := builder["ssh_key_path"] + if !ok { + continue + } + + sshKeyPath, ok := sshKeyPathRaw.(string) + if !ok { + continue + } + + // only assign to ssh_private_key_file if it doesn't + // already exist; otherwise we'll just ignore ssh_key_path + _, sshPrivateIncluded := builder["ssh_private_key_file"] + if !sshPrivateIncluded { + builder["ssh_private_key_file"] = sshKeyPath + } + + delete(builder, "ssh_key_path") + } + + input["builders"] = tpl.Builders + return input, nil +} + +func (FixerSSHKeyPath) Synopsis() string { + return `Updates builders using "ssh_key_path" to use "ssh_private_key_file"` +} diff --git a/fix/fixer_sshkeypath_test.go b/fix/fixer_sshkeypath_test.go new file mode 100644 index 000000000..865f8c1de --- /dev/null +++ b/fix/fixer_sshkeypath_test.go @@ -0,0 +1,83 @@ +package fix + +import ( + "reflect" + "testing" +) + +func TestFixerSSHKeyPath_Impl(t *testing.T) { + var _ Fixer = new(FixerSSHKeyPath) +} + +func TestFixerSSHKeyPath_Fix(t *testing.T) { + cases := []struct { + Input map[string]interface{} + Expected map[string]interface{} + }{ + // No key_path field + { + Input: map[string]interface{}{ + "type": "virtualbox", + }, + + Expected: map[string]interface{}{ + "type": "virtualbox", + }, + }, + + // private_key_file without key_path + { + Input: map[string]interface{}{ + "ssh_private_key_file": "id_rsa", + }, + + Expected: map[string]interface{}{ + "ssh_private_key_file": "id_rsa", + }, + }, + + // key_path without private_key_file + { + Input: map[string]interface{}{ + "ssh_key_path": "id_rsa", + }, + + Expected: map[string]interface{}{ + "ssh_private_key_file": "id_rsa", + }, + }, + + // key_path and private_key_file + { + Input: map[string]interface{}{ + "ssh_key_path": "key_id_rsa", + "ssh_private_key_file": "private_id_rsa", + }, + + Expected: map[string]interface{}{ + "ssh_private_key_file": "private_id_rsa", + }, + }, + } + + for _, tc := range cases { + var f FixerSSHKeyPath + + input := map[string]interface{}{ + "builders": []map[string]interface{}{tc.Input}, + } + + expected := map[string]interface{}{ + "builders": []map[string]interface{}{tc.Expected}, + } + + output, err := f.Fix(input) + if err != nil { + t.Fatalf("err: %s", err) + } + + if !reflect.DeepEqual(output, expected) { + t.Fatalf("unexpected: %#v\nexpected: %#v\n", output, expected) + } + } +} From bd8fb014c4acb8caaacc8cdef694aca0c6ba3f98 Mon Sep 17 00:00:00 2001 From: James Bishopp Date: Mon, 13 Jul 2015 08:35:30 -0700 Subject: [PATCH 2/2] Removed ssh_key_path - removed backwards compatibility code - ensured key usage came from SSHPrivateKey configuration - changed tests to use private_key --- builder/parallels/common/ssh.go | 2 +- builder/parallels/common/ssh_config.go | 4 ---- builder/parallels/common/ssh_config_test.go | 10 +++++----- builder/qemu/builder.go | 4 ---- builder/qemu/builder_test.go | 10 +++++----- builder/virtualbox/common/ssh.go | 2 +- builder/virtualbox/common/ssh_config.go | 4 ---- builder/virtualbox/common/ssh_config_test.go | 10 +++++----- builder/vmware/common/ssh_config.go | 4 ---- builder/vmware/common/ssh_config_test.go | 10 +++++----- 10 files changed, 22 insertions(+), 38 deletions(-) diff --git a/builder/parallels/common/ssh.go b/builder/parallels/common/ssh.go index 9e0b2b907..35b43311e 100644 --- a/builder/parallels/common/ssh.go +++ b/builder/parallels/common/ssh.go @@ -32,7 +32,7 @@ func SSHConfigFunc(config SSHConfig) func(multistep.StateBag) (*ssh.ClientConfig packerssh.PasswordKeyboardInteractive(config.Comm.SSHPassword)), } - if config.SSHKeyPath != "" { + if config.Comm.SSHPrivateKey != "" { signer, err := commonssh.FileSigner(config.Comm.SSHPrivateKey) if err != nil { return nil, err diff --git a/builder/parallels/common/ssh_config.go b/builder/parallels/common/ssh_config.go index bea164b06..e7d9357ff 100644 --- a/builder/parallels/common/ssh_config.go +++ b/builder/parallels/common/ssh_config.go @@ -12,15 +12,11 @@ type SSHConfig struct { // These are deprecated, but we keep them around for BC // TODO(@mitchellh): remove - SSHKeyPath string `mapstructure:"ssh_key_path"` SSHWaitTimeout time.Duration `mapstructure:"ssh_wait_timeout"` } func (c *SSHConfig) Prepare(ctx *interpolate.Context) []error { // TODO: backwards compatibility, write fixer instead - if c.SSHKeyPath != "" { - c.Comm.SSHPrivateKey = c.SSHKeyPath - } if c.SSHWaitTimeout != 0 { c.Comm.SSHTimeout = c.SSHWaitTimeout } diff --git a/builder/parallels/common/ssh_config_test.go b/builder/parallels/common/ssh_config_test.go index 01dd1ff62..887b59f8f 100644 --- a/builder/parallels/common/ssh_config_test.go +++ b/builder/parallels/common/ssh_config_test.go @@ -28,19 +28,19 @@ func TestSSHConfigPrepare(t *testing.T) { } } -func TestSSHConfigPrepare_SSHKeyPath(t *testing.T) { +func TestSSHConfigPrepare_SSHPrivateKey(t *testing.T) { var c *SSHConfig var errs []error c = testSSHConfig() - c.SSHKeyPath = "" + c.Comm.SSHPrivateKey = "" errs = c.Prepare(testConfigTemplate(t)) if len(errs) > 0 { t.Fatalf("should not have error: %#v", errs) } c = testSSHConfig() - c.SSHKeyPath = "/i/dont/exist" + c.Comm.SSHPrivateKey = "/i/dont/exist" errs = c.Prepare(testConfigTemplate(t)) if len(errs) == 0 { t.Fatal("should have error") @@ -59,7 +59,7 @@ func TestSSHConfigPrepare_SSHKeyPath(t *testing.T) { } c = testSSHConfig() - c.SSHKeyPath = tf.Name() + c.Comm.SSHPrivateKey = tf.Name() errs = c.Prepare(testConfigTemplate(t)) if len(errs) == 0 { t.Fatal("should have error") @@ -70,7 +70,7 @@ func TestSSHConfigPrepare_SSHKeyPath(t *testing.T) { tf.Truncate(0) tf.Write([]byte(testPem)) c = testSSHConfig() - c.SSHKeyPath = tf.Name() + c.Comm.SSHPrivateKey = tf.Name() errs = c.Prepare(testConfigTemplate(t)) if len(errs) > 0 { t.Fatalf("should not have error: %#v", errs) diff --git a/builder/qemu/builder.go b/builder/qemu/builder.go index 66c78e45e..59490049c 100644 --- a/builder/qemu/builder.go +++ b/builder/qemu/builder.go @@ -110,7 +110,6 @@ type Config struct { // These are deprecated, but we keep them around for BC // TODO(@mitchellh): remove - SSHKeyPath string `mapstructure:"ssh_key_path"` SSHWaitTimeout time.Duration `mapstructure:"ssh_wait_timeout"` // TODO(mitchellh): deprecate @@ -212,9 +211,6 @@ func (b *Builder) Prepare(raws ...interface{}) ([]string, error) { } // TODO: backwards compatibility, write fixer instead - if b.config.SSHKeyPath != "" { - b.config.Comm.SSHPrivateKey = b.config.SSHKeyPath - } if b.config.SSHWaitTimeout != 0 { b.config.Comm.SSHTimeout = b.config.SSHWaitTimeout } diff --git a/builder/qemu/builder_test.go b/builder/qemu/builder_test.go index a7ce88e6d..cebcf6fbb 100644 --- a/builder/qemu/builder_test.go +++ b/builder/qemu/builder_test.go @@ -357,11 +357,11 @@ func TestBuilderPrepare_SSHHostPort(t *testing.T) { } } -func TestBuilderPrepare_sshKeyPath(t *testing.T) { +func TestBuilderPrepare_SSHPrivateKey(t *testing.T) { var b Builder config := testConfig() - config["ssh_key_path"] = "" + config["ssh_private_key_file"] = "" b = Builder{} warns, err := b.Prepare(config) if len(warns) > 0 { @@ -371,7 +371,7 @@ func TestBuilderPrepare_sshKeyPath(t *testing.T) { t.Fatalf("should not have error: %s", err) } - config["ssh_key_path"] = "/i/dont/exist" + config["ssh_private_key_file"] = "/i/dont/exist" b = Builder{} warns, err = b.Prepare(config) if len(warns) > 0 { @@ -393,7 +393,7 @@ func TestBuilderPrepare_sshKeyPath(t *testing.T) { t.Fatalf("err: %s", err) } - config["ssh_key_path"] = tf.Name() + config["ssh_private_key_file"] = tf.Name() b = Builder{} warns, err = b.Prepare(config) if len(warns) > 0 { @@ -407,7 +407,7 @@ func TestBuilderPrepare_sshKeyPath(t *testing.T) { tf.Seek(0, 0) tf.Truncate(0) tf.Write([]byte(testPem)) - config["ssh_key_path"] = tf.Name() + config["ssh_private_key_file"] = tf.Name() b = Builder{} warns, err = b.Prepare(config) if len(warns) > 0 { diff --git a/builder/virtualbox/common/ssh.go b/builder/virtualbox/common/ssh.go index b04a14ac8..c9080df1d 100644 --- a/builder/virtualbox/common/ssh.go +++ b/builder/virtualbox/common/ssh.go @@ -24,7 +24,7 @@ func SSHConfigFunc(config SSHConfig) func(multistep.StateBag) (*gossh.ClientConf ssh.PasswordKeyboardInteractive(config.Comm.SSHPassword)), } - if config.SSHKeyPath != "" { + if config.Comm.SSHPrivateKey != "" { signer, err := commonssh.FileSigner(config.Comm.SSHPrivateKey) if err != nil { return nil, err diff --git a/builder/virtualbox/common/ssh_config.go b/builder/virtualbox/common/ssh_config.go index 8997c58f7..5095f2b03 100644 --- a/builder/virtualbox/common/ssh_config.go +++ b/builder/virtualbox/common/ssh_config.go @@ -17,7 +17,6 @@ type SSHConfig struct { // These are deprecated, but we keep them around for BC // TODO(@mitchellh): remove - SSHKeyPath string `mapstructure:"ssh_key_path"` SSHWaitTimeout time.Duration `mapstructure:"ssh_wait_timeout"` } @@ -31,9 +30,6 @@ func (c *SSHConfig) Prepare(ctx *interpolate.Context) []error { } // TODO: backwards compatibility, write fixer instead - if c.SSHKeyPath != "" { - c.Comm.SSHPrivateKey = c.SSHKeyPath - } if c.SSHWaitTimeout != 0 { c.Comm.SSHTimeout = c.SSHWaitTimeout } diff --git a/builder/virtualbox/common/ssh_config_test.go b/builder/virtualbox/common/ssh_config_test.go index e5b918fe9..72d92ac11 100644 --- a/builder/virtualbox/common/ssh_config_test.go +++ b/builder/virtualbox/common/ssh_config_test.go @@ -59,19 +59,19 @@ func TestSSHConfigPrepare_SSHHostPort(t *testing.T) { } } -func TestSSHConfigPrepare_SSHKeyPath(t *testing.T) { +func TestSSHConfigPrepare_SSHPrivateKey(t *testing.T) { var c *SSHConfig var errs []error c = testSSHConfig() - c.SSHKeyPath = "" + c.Comm.SSHPrivateKey = "" errs = c.Prepare(testConfigTemplate(t)) if len(errs) > 0 { t.Fatalf("should not have error: %#v", errs) } c = testSSHConfig() - c.SSHKeyPath = "/i/dont/exist" + c.Comm.SSHPrivateKey = "/i/dont/exist" errs = c.Prepare(testConfigTemplate(t)) if len(errs) == 0 { t.Fatal("should have error") @@ -90,7 +90,7 @@ func TestSSHConfigPrepare_SSHKeyPath(t *testing.T) { } c = testSSHConfig() - c.SSHKeyPath = tf.Name() + c.Comm.SSHPrivateKey = tf.Name() errs = c.Prepare(testConfigTemplate(t)) if len(errs) == 0 { t.Fatal("should have error") @@ -101,7 +101,7 @@ func TestSSHConfigPrepare_SSHKeyPath(t *testing.T) { tf.Truncate(0) tf.Write([]byte(testPem)) c = testSSHConfig() - c.SSHKeyPath = tf.Name() + c.Comm.SSHPrivateKey = tf.Name() errs = c.Prepare(testConfigTemplate(t)) if len(errs) > 0 { t.Fatalf("should not have error: %#v", errs) diff --git a/builder/vmware/common/ssh_config.go b/builder/vmware/common/ssh_config.go index 86754f6e4..a0bbf942e 100644 --- a/builder/vmware/common/ssh_config.go +++ b/builder/vmware/common/ssh_config.go @@ -12,16 +12,12 @@ type SSHConfig struct { // These are deprecated, but we keep them around for BC // TODO(@mitchellh): remove - SSHKeyPath string `mapstructure:"ssh_key_path"` SSHSkipRequestPty bool `mapstructure:"ssh_skip_request_pty"` SSHWaitTimeout time.Duration `mapstructure:"ssh_wait_timeout"` } func (c *SSHConfig) Prepare(ctx *interpolate.Context) []error { // TODO: backwards compatibility, write fixer instead - if c.SSHKeyPath != "" { - c.Comm.SSHPrivateKey = c.SSHKeyPath - } if c.SSHWaitTimeout != 0 { c.Comm.SSHTimeout = c.SSHWaitTimeout } diff --git a/builder/vmware/common/ssh_config_test.go b/builder/vmware/common/ssh_config_test.go index 1fd84c02f..64889195f 100644 --- a/builder/vmware/common/ssh_config_test.go +++ b/builder/vmware/common/ssh_config_test.go @@ -28,19 +28,19 @@ func TestSSHConfigPrepare(t *testing.T) { } } -func TestSSHConfigPrepare_SSHKeyPath(t *testing.T) { +func TestSSHConfigPrepare_SSHPrivateKey(t *testing.T) { var c *SSHConfig var errs []error c = testSSHConfig() - c.SSHKeyPath = "" + c.Comm.SSHPrivateKey = "" errs = c.Prepare(testConfigTemplate(t)) if len(errs) > 0 { t.Fatalf("should not have error: %#v", errs) } c = testSSHConfig() - c.SSHKeyPath = "/i/dont/exist" + c.Comm.SSHPrivateKey = "/i/dont/exist" errs = c.Prepare(testConfigTemplate(t)) if len(errs) == 0 { t.Fatal("should have error") @@ -59,7 +59,7 @@ func TestSSHConfigPrepare_SSHKeyPath(t *testing.T) { } c = testSSHConfig() - c.SSHKeyPath = tf.Name() + c.Comm.SSHPrivateKey = tf.Name() errs = c.Prepare(testConfigTemplate(t)) if len(errs) == 0 { t.Fatal("should have error") @@ -70,7 +70,7 @@ func TestSSHConfigPrepare_SSHKeyPath(t *testing.T) { tf.Truncate(0) tf.Write([]byte(testPem)) c = testSSHConfig() - c.SSHKeyPath = tf.Name() + c.Comm.SSHPrivateKey = tf.Name() errs = c.Prepare(testConfigTemplate(t)) if len(errs) > 0 { t.Fatalf("should not have error: %#v", errs)