diff --git a/lib/kitchen/docker/container/linux.rb b/lib/kitchen/docker/container/linux.rb index 07cfe655..b541ab7b 100644 --- a/lib/kitchen/docker/container/linux.rb +++ b/lib/kitchen/docker/container/linux.rb @@ -84,7 +84,10 @@ def execute(command) debug("Executing temp script #{remote_path}/#{filename} on container") remote_path = replace_env_variables(@config, remote_path) - container_exec(@config, "/bin/bash #{remote_path}/#{filename}") + # Escaped because the exec command line is assembled as one string: + # a temp_dir with a space in it was split before docker saw it, and + # bash was handed the first half of the directory as the script. + container_exec(@config, "/bin/bash #{Shellwords.escape("#{remote_path}/#{filename}")}") rescue => e raise "Failed to execute command on Linux container. #{e}" ensure diff --git a/lib/kitchen/docker/container/windows.rb b/lib/kitchen/docker/container/windows.rb index d6f83f9d..e9e64ace 100644 --- a/lib/kitchen/docker/container/windows.rb +++ b/lib/kitchen/docker/container/windows.rb @@ -69,7 +69,11 @@ def execute(command) # Replace any environment variables used in the path and execute script file debug("Executing temp script #{remote_path}\\#{filename} on container") remote_path = replace_env_variables(@config, remote_path) - cmd = build_powershell_command("-File #{remote_path}\\#{filename}") + # Quoted because PowerShell's -File takes exactly one argument. An + # unquoted path with a space in it -- which $env:TEMP has whenever the + # user's name does -- left PowerShell looking for a script named + # after the first word of the directory. + cmd = build_powershell_command(%{-File "#{remote_path}\\#{filename}"}) container_exec(@config, cmd) rescue => e diff --git a/lib/kitchen/docker/helpers/container_helper.rb b/lib/kitchen/docker/helpers/container_helper.rb index 18961bd5..a7896aff 100644 --- a/lib/kitchen/docker/helpers/container_helper.rb +++ b/lib/kitchen/docker/helpers/container_helper.rb @@ -155,6 +155,17 @@ def container_exec(state, command) # Creates a directory inside the container, on Linux or Windows. # + # The path is escaped for the shell, because it comes from the + # transport's +temp_dir+ and a `docker exec` command line is assembled + # as one string. A directory with a space in it was torn in two before + # docker ever saw it, and `mkdir -p` obligingly created both halves -- + # neither of them the directory that was asked for. Every upload that + # followed then went to a path that did not exist. + # + # The PowerShell branch already quotes the path itself, and its + # argument is reassembled by PowerShell rather than split by a shell, + # so it is left as it is. + # # @param state [Hash] instance state naming the container # @param path [String] the directory to create; environment variable # references are expanded first @@ -162,7 +173,7 @@ def container_exec(state, command) # @raise [RuntimeError] if the directory cannot be created def create_dir_on_container(state, path) path = replace_env_variables(state, path) - cmd = "mkdir -p #{path}" + cmd = "mkdir -p #{Shellwords.escape(path)}" if state[:platform].include?("windows") psh = "-Command if(-not (Test-Path '#{path}')) { New-Item -Path '#{path}' -Force }" diff --git a/spec/container_helper_spec.rb b/spec/container_helper_spec.rb index b9ea3469..6e3d9c1f 100644 --- a/spec/container_helper_spec.rb +++ b/spec/container_helper_spec.rb @@ -333,6 +333,89 @@ def helper_inspecting(output) end end + describe "#create_dir_on_container" do + def dir_maker(state) + helper.tap do |h| + @commands = [] + allow(h).to receive(:replace_env_variables) { |_s, path| path } + allow(h).to receive(:docker_command) { |cmd, _opts = {}| @commands << cmd; "" } + end + end + + it "creates the directory inside the container" do + dir_maker(nil).create_dir_on_container({ container_id: "abc", platform: "ubuntu-24.04" }, "/tmp") + expect(argv(@commands.first)).to include_consecutive("mkdir", "-p", "/tmp") + end + + it "expands an environment variable reference before creating it" do + h = helper + allow(h).to receive(:container_env_variables).and_return("TEMP" => "/var/tmp") + allow(h).to receive(:docker_command) { |cmd, _opts = {}| @cmd = cmd; "" } + h.create_dir_on_container({ container_id: "abc", platform: "ubuntu-24.04" }, "$TEMP/kitchen") + expect(argv(@cmd)).to include_consecutive("mkdir", "-p", "/var/tmp/kitchen") + end + + # A temp_dir with a space in it reached `mkdir -p` unquoted, so the shell + # tore it in two and mkdir made two directories -- neither of them the one + # asked for. Every later upload then landed somewhere that did not exist. + it "keeps a path containing a space as one argument" do + dir_maker(nil).create_dir_on_container({ container_id: "abc", platform: "ubuntu-24.04" }, + "/var/tmp/kitchen docker") + expect(argv(@commands.first)).to include_consecutive("mkdir", "-p", "/var/tmp/kitchen docker") + end + + it "uses PowerShell on a Windows container" do + dir_maker(nil).create_dir_on_container({ container_id: "abc", platform: "windows-2022" }, 'C:\\Temp') + expect(@commands.first).to include("powershell").and include("New-Item") + end + + it "says which directory it could not create" do + h = helper + allow(h).to receive(:replace_env_variables) { |_s, path| path } + allow(h).to receive(:docker_command).and_raise("boom") + expect { h.create_dir_on_container({ container_id: "abc", platform: "ubuntu-24.04" }, "/tmp/kitchen") } + .to raise_error(RuntimeError, %r{Failed to create directory /tmp/kitchen}) + end + end + + describe "#container_exec" do + it "runs the command through docker exec" do + h = helper + allow(h).to receive(:docker_command) { |cmd, _opts = {}| @cmd = cmd; "output" } + expect(h.container_exec({ container_id: "abc" }, "echo hi")).to eq "output" + expect(argv(@cmd)).to include_consecutive("exec", "abc", "echo", "hi") + end + + it "names the container operation when the command fails" do + h = helper + allow(h).to receive(:docker_command).and_raise("boom") + expect { h.container_exec({ container_id: "abc" }, "echo hi") } + .to raise_error(RuntimeError, /Failed to execute command on Docker container/) + end + end + + describe "#run_container" do + it "returns the id docker printed for the container it started" do + h = helper(instance_name: "kitchen-test") + allow(h).to receive(:docker_command).and_return(DockerOutput::RUN_CLEAN) + expect(h.run_container({ image_id: "sha256:abc" }, 22)).to eq DockerOutput::RUN_CONTAINER_ID + end + + it "publishes the port it was given" do + h = helper + allow(h).to receive(:docker_command) { |cmd, _opts = {}| @cmd = cmd; DockerOutput::RUN_CLEAN } + h.run_container({ image_id: "sha256:abc" }, 22) + expect(argv(@cmd)).to include_consecutive("-p", "22") + end + + it "publishes no port when there is none, as for a Windows container" do + h = helper + allow(h).to receive(:docker_command) { |cmd, _opts = {}| @cmd = cmd; DockerOutput::RUN_CLEAN } + h.run_container(image_id: "sha256:abc") + expect(argv(@cmd)).not_to include("-p") + end + end + describe "#copy_file_to_container" do let(:state) { { container_id: "abc", platform: "ubuntu-24.04" } } diff --git a/spec/linux_container_spec.rb b/spec/linux_container_spec.rb index a03ea5e1..6ccf81d1 100644 --- a/spec/linux_container_spec.rb +++ b/spec/linux_container_spec.rb @@ -211,6 +211,33 @@ def container_in(dir, &upload) c.execute("echo hi") expect(uploaded).to eq "echo hi" end + + # The staged script is run by path, and that path is built from temp_dir, + # which the user sets. + def ran_in(temp_dir) + c = container(temp_dir: temp_dir) + allow(c).to receive(:create_dir_on_container) + allow(c).to receive(:replace_env_variables) { |_cfg, path| path } + allow(c).to receive(:upload) + ran = nil + allow(c).to receive(:container_exec) { |_cfg, cmd| ran = cmd } + c.execute("echo hi") + ran + end + + it "runs the staged script with bash" do + ran = ran_in("/tmp") + expect(argv(ran).first).to eq "/bin/bash" + expect(argv(ran).last).to match(%r{\A/tmp/docker-[0-9a-f-]+\.sh\z}) + end + + # Interpolated unquoted, a temp_dir with a space in it reached `docker + # exec` as two arguments, and bash was handed the first half of the + # directory as the script to run. + it "keeps a temp_dir containing a space as one argument" do + expect(argv(ran_in("/var/tmp/kitchen docker")).last) + .to match(%r{\A/var/tmp/kitchen docker/docker-[0-9a-f-]+\.sh\z}) + end end describe "#generate_keys" do diff --git a/spec/windows_container_spec.rb b/spec/windows_container_spec.rb index 04c7e993..04b01a62 100644 --- a/spec/windows_container_spec.rb +++ b/spec/windows_container_spec.rb @@ -64,6 +64,60 @@ def container(config = {}) end end + describe "#execute" do + def staged(config = {}) + c = container({ temp_dir: 'C:\\Temp' }.merge(config)) + allow(c).to receive(:create_dir_on_container) + allow(c).to receive(:replace_env_variables) { |_cfg, path| path } + allow(c).to receive(:upload) + allow(c).to receive(:container_exec) { |_cfg, cmd| @ran = cmd } + c + end + + around do |example| + Dir.mktmpdir { |dir| Dir.chdir(dir) { example.run } } + end + + it "runs the staged script with powershell" do + staged.execute("Write-Host hi") + expect(@ran).to start_with "powershell -ExecutionPolicy Bypass -NoLogo " + expect(@ran).to match(/-File .*docker-[0-9a-f-]+\.ps1/) + end + + it "stages the script under the configured temp_dir" do + staged.execute("Write-Host hi") + expect(@ran).to include 'C:\\Temp\\docker-' + end + + it "accepts a temp_dir written with forward slashes" do + # Kitchen configuration is YAML that is often shared with Linux suites, + # so the separator is normalised rather than rejected. + staged(temp_dir: "C:/Temp").execute("Write-Host hi") + expect(@ran).to include 'C:\\Temp\\docker-' + end + + # A Windows temp_dir routinely contains a space -- $env:TEMP under a user + # whose name has one does. PowerShell's -File takes exactly one argument, + # so an unquoted path left it looking for a script named after the first + # word. + it "quotes a script path containing a space" do + staged(temp_dir: 'C:\\Users\\Foo Bar\\Temp').execute("Write-Host hi") + expect(@ran).to include('-File "C:\\Users\\Foo Bar\\Temp\\docker-').and end_with('.ps1"') + end + + it "removes the staged script once the command has run" do + staged.execute("Write-Host hi") + expect(Dir.glob(".kitchen/temp/docker-*.ps1")).to be_empty + end + + it "removes the staged script when the command fails" do + c = staged + allow(c).to receive(:container_exec).and_raise("command exploded") + expect { c.execute("Write-Host hi") }.to raise_error(/Failed to execute command/) + expect(Dir.glob(".kitchen/temp/docker-*.ps1")).to be_empty + end + end + describe "the contract it shares with the Linux container" do # The two classes share almost no code but must agree on the state they # populate, because the driver and transport read the same keys whichever