From b66e49223d7cc40c3391356d586a4fb3935981d8 Mon Sep 17 00:00:00 2001 From: Megan Marsh Date: Wed, 1 Apr 2020 16:07:14 -0700 Subject: [PATCH] extract the building of command args into a testable helper function --- provisioner/ansible/provisioner.go | 50 ++++++---- provisioner/ansible/provisioner_test.go | 119 ++++++++++++++++++++++++ 2 files changed, 149 insertions(+), 20 deletions(-) diff --git a/provisioner/ansible/provisioner.go b/provisioner/ansible/provisioner.go index 923a22fd3..17432a9b9 100644 --- a/provisioner/ansible/provisioner.go +++ b/provisioner/ansible/provisioner.go @@ -547,37 +547,31 @@ func (p *Provisioner) executeGalaxy(ui packer.Ui, comm packer.Communicator) erro return nil } -func (p *Provisioner) executeAnsible(ui packer.Ui, comm packer.Communicator, privKeyFile string) error { - playbook, _ := filepath.Abs(p.config.PlaybookFile) - inventory := p.config.InventoryFile +func (p *Provisioner) createCmdArgs(httpAddr, inventory, playbook, privKeyFile string) (args []string, envVars []string) { + args = []string{} - var envvars []string - - // Fetch external dependencies - if len(p.config.GalaxyFile) > 0 { - if err := p.executeGalaxy(ui, comm); err != nil { - return fmt.Errorf("Error executing Ansible Galaxy: %s", err) - } + if p.config.PackerBuildName != "" { + // HCL configs don't currently have the PakcerBuildName. Don't + // cause weirdness with a half-set variable + args = append(args, "-e", fmt.Sprintf("packer_build_name=%s", p.config.PackerBuildName)) } - args := []string{"-e", fmt.Sprintf("packer_build_name=%s", p.config.PackerBuildName), "-e", fmt.Sprintf("packer_builder_type=%s", p.config.PackerBuilderType)} + args = append(args, "-e", fmt.Sprintf("packer_builder_type=%s", p.config.PackerBuilderType)) if len(privKeyFile) > 0 { - // Changed this from using --private-key to supplying -e ansible_ssh_private_key_file as the latter - // is treated as a highest priority variable, and thus prevents overriding by dynamic variables - // as seen in #5852 - // args = append(args, "--private-key", privKeyFile) + // "-e ansible_ssh_private_key_file" is preferable to "--private-key" + // because it is a higher priority variable and therefore won't get + // overridden by dynamic variables. See #5852 for more details. args = append(args, "-e", fmt.Sprintf("ansible_ssh_private_key_file=%s", privKeyFile)) } // expose packer_http_addr extra variable - httpAddr := common.GetHTTPAddr() if httpAddr != "" { - args = append(args, "-e", fmt.Sprintf(" packer_http_addr=%s", httpAddr)) + args = append(args, "-e", fmt.Sprintf("packer_http_addr=%s", httpAddr)) } // Add password to ansible call. if p.config.UseProxy.False() && p.generatedData["ConnType"] == "winrm" { - args = append(args, "-e", fmt.Sprintf(" ansible_password=%s", p.generatedData["Password"])) + args = append(args, "-e", fmt.Sprintf("ansible_password=%s", p.generatedData["Password"])) } if p.generatedData["ConnType"] == "ssh" { @@ -589,11 +583,27 @@ func (p *Provisioner) executeAnsible(ui packer.Ui, comm packer.Communicator, pri args = append(args, p.config.ExtraArguments...) if len(p.config.AnsibleEnvVars) > 0 { - envvars = append(envvars, p.config.AnsibleEnvVars...) + envVars = append(envVars, p.config.AnsibleEnvVars...) } + return args, envVars +} + +func (p *Provisioner) executeAnsible(ui packer.Ui, comm packer.Communicator, privKeyFile string) error { + playbook, _ := filepath.Abs(p.config.PlaybookFile) + inventory := p.config.InventoryFile + httpAddr := common.GetHTTPAddr() + + // Fetch external dependencies + if len(p.config.GalaxyFile) > 0 { + if err := p.executeGalaxy(ui, comm); err != nil { + return fmt.Errorf("Error executing Ansible Galaxy: %s", err) + } + } + + args, envvars := p.createCmdArgs(httpAddr, inventory, playbook, privKeyFile) + cmd := exec.Command(p.config.Command, args...) - log.Printf("Megan cmd is %#v", cmd) cmd.Env = os.Environ() if len(envvars) > 0 { diff --git a/provisioner/ansible/provisioner_test.go b/provisioner/ansible/provisioner_test.go index a964a76dc..0f7e3bb7b 100644 --- a/provisioner/ansible/provisioner_test.go +++ b/provisioner/ansible/provisioner_test.go @@ -16,6 +16,7 @@ import ( confighelper "github.com/hashicorp/packer/helper/config" "github.com/hashicorp/packer/packer" + "github.com/stretchr/testify/assert" ) // Be sure to remove the Ansible stub file in each test with: @@ -482,6 +483,124 @@ func basicGenData(input map[string]interface{}) map[string]interface{} { return gd } +func TestCreateCmdArgs(t *testing.T) { + type testcase struct { + PackerBuildName string + PackerBuilderType string + UseProxy confighelper.Trilean + generatedData map[string]interface{} + ExtraArguments []string + AnsibleEnvVars []string + callArgs []string // httpAddr inventory playbook privKeyFile + ExpectedArgs []string + ExpectedEnvVars []string + } + TestCases := []testcase{ + { + // SSH with private key and an extra argument. + PackerBuildName: "packerparty", + generatedData: basicGenData(nil), + ExtraArguments: []string{"-e", "hello-world"}, + AnsibleEnvVars: []string{"ENV_1=pancakes", "ENV_2=bananas"}, + callArgs: []string{"", "/var/inventory", "test-playbook.yml", "/path/to/privkey.pem"}, + ExpectedArgs: []string{"-e", "packer_build_name=packerparty", "-e", "packer_builder_type=fakebuilder", "-e", "ansible_ssh_private_key_file=/path/to/privkey.pem", "--ssh-extra-args", "-o IdentitiesOnly=yes", "-i", "/var/inventory", "test-playbook.yml", "-e", "hello-world"}, + ExpectedEnvVars: []string{"ENV_1=pancakes", "ENV_2=bananas"}, + }, + { + PackerBuildName: "packerparty", + UseProxy: confighelper.TriTrue, + generatedData: basicGenData(nil), + ExtraArguments: []string{"-e", "hello-world"}, + callArgs: []string{"", "/var/inventory", "test-playbook.yml", "/path/to/privkey.pem"}, + ExpectedArgs: []string{"-e", "packer_build_name=packerparty", "-e", "packer_builder_type=fakebuilder", "-e", "ansible_ssh_private_key_file=/path/to/privkey.pem", "--ssh-extra-args", "-o IdentitiesOnly=yes", "-i", "/var/inventory", "test-playbook.yml", "-e", "hello-world"}, + ExpectedEnvVars: []string{}, + }, + { + // Winrm, but no_proxy is unset so we don't do anything with ansible_password. + PackerBuildName: "packerparty", + generatedData: basicGenData(map[string]interface{}{ + "ConnType": "winrm", + }), + ExtraArguments: []string{"-e", "hello-world"}, + AnsibleEnvVars: []string{"ENV_1=pancakes", "ENV_2=bananas"}, + callArgs: []string{"", "/var/inventory", "test-playbook.yml", ""}, + ExpectedArgs: []string{"-e", "packer_build_name=packerparty", "-e", "packer_builder_type=fakebuilder", "-i", "/var/inventory", "test-playbook.yml", "-e", "hello-world"}, + ExpectedEnvVars: []string{"ENV_1=pancakes", "ENV_2=bananas"}, + }, + { + // HTTPAddr should be set. No env vars. + PackerBuildName: "packerparty", + ExtraArguments: []string{"-e", "hello-world"}, + generatedData: basicGenData(nil), + callArgs: []string{"123.45.67.89", "/var/inventory", "test-playbook.yml", ""}, + ExpectedArgs: []string{"-e", "packer_build_name=packerparty", "-e", "packer_builder_type=fakebuilder", "-e", "packer_http_addr=123.45.67.89", "--ssh-extra-args", "-o IdentitiesOnly=yes", "-i", "/var/inventory", "test-playbook.yml", "-e", "hello-world"}, + ExpectedEnvVars: []string{}, + }, + { + // Add ansible_password for proxyless winrm connection. + UseProxy: confighelper.TriFalse, + generatedData: basicGenData(map[string]interface{}{ + "ConnType": "winrm", + "Password": "ilovebananapancakes", + }), + AnsibleEnvVars: []string{"ENV_1=pancakes", "ENV_2=bananas"}, + callArgs: []string{"123.45.67.89", "/var/inventory", "test-playbook.yml", ""}, + ExpectedArgs: []string{"-e", "packer_builder_type=fakebuilder", "-e", "packer_http_addr=123.45.67.89", "-e", "ansible_password=ilovebananapancakes", "-i", "/var/inventory", "test-playbook.yml"}, + ExpectedEnvVars: []string{"ENV_1=pancakes", "ENV_2=bananas"}, + }, + { + // Neither special ssh stuff, nor special windows stuff. This is docker! + PackerBuildName: "packerparty", + generatedData: basicGenData(map[string]interface{}{ + "ConnType": "docker", + }), + ExtraArguments: []string{"-e", "hello-world"}, + AnsibleEnvVars: []string{"ENV_1=pancakes", "ENV_2=bananas"}, + callArgs: []string{"", "/var/inventory", "test-playbook.yml", ""}, + ExpectedArgs: []string{"-e", "packer_build_name=packerparty", "-e", "packer_builder_type=fakebuilder", "-i", "/var/inventory", "test-playbook.yml", "-e", "hello-world"}, + ExpectedEnvVars: []string{"ENV_1=pancakes", "ENV_2=bananas"}, + }, + { + // Windows, no proxy, with extra vars. + UseProxy: confighelper.TriFalse, + generatedData: basicGenData(map[string]interface{}{ + "ConnType": "winrm", + "Password": "ilovebananapancakes", + }), + ExtraArguments: []string{"-e", "hello-world"}, + AnsibleEnvVars: []string{"ENV_1=pancakes", "ENV_2=bananas"}, + callArgs: []string{"123.45.67.89", "/var/inventory", "test-playbook.yml", ""}, + ExpectedArgs: []string{"-e", "packer_builder_type=fakebuilder", "-e", "packer_http_addr=123.45.67.89", "-e", "ansible_password=ilovebananapancakes", "-i", "/var/inventory", "test-playbook.yml", "-e", "hello-world"}, + ExpectedEnvVars: []string{"ENV_1=pancakes", "ENV_2=bananas"}, + }, + { + // No builder name. This shouldn't cause an error, it just shouldn't be set. HCL, yo. + generatedData: basicGenData(nil), + callArgs: []string{"", "/var/inventory", "test-playbook.yml", ""}, + ExpectedArgs: []string{"-e", "packer_builder_type=fakebuilder", "--ssh-extra-args", "-o IdentitiesOnly=yes", "-i", "/var/inventory", "test-playbook.yml"}, + ExpectedEnvVars: []string{}, + }, + } + + for _, tc := range TestCases { + var p Provisioner + p.Prepare(testConfig(t)) + defer os.Remove(p.config.Command) + p.config.UseProxy = tc.UseProxy + p.config.PackerBuilderType = "fakebuilder" + p.config.PackerBuildName = tc.PackerBuildName + p.generatedData = tc.generatedData + p.config.ExtraArguments = tc.ExtraArguments + p.config.AnsibleEnvVars = tc.AnsibleEnvVars + + args, envVars := p.createCmdArgs(tc.callArgs[0], tc.callArgs[1], tc.callArgs[2], tc.callArgs[3]) + + assert.ElementsMatch(t, args, tc.ExpectedArgs, + "Args didn't match expected:\n\n expected: \n%s\n; recieved: \n%s\n", tc.ExpectedArgs, args) + assert.ElementsMatch(t, envVars, tc.ExpectedEnvVars, "EnvVars didn't match expected:\n\n expected: \n%s\n; recieved: \n%s\n", tc.ExpectedEnvVars, envVars) + } +} + func TestUseProxy(t *testing.T) { type testcase struct { UseProxy confighelper.Trilean