diff --git a/lib/dash/cli/build.rb b/lib/dash/cli/build.rb index 45c66a8e..1ba29b23 100644 --- a/lib/dash/cli/build.rb +++ b/lib/dash/cli/build.rb @@ -74,10 +74,8 @@ def push desc "pull", "Pull app image from registry onto servers" def pull - login_to_registry_remotely unless DASH.registry.local? - forward_local_registry_port(DASH.hosts, **DASH.config.ssh.options) do - if (first_hosts = mirror_hosts).any? + if (first_hosts = login_and_mirror_hosts).any? #  Pull on a single host per mirror first to seed them say "Pulling image on #{first_hosts.join(", ")} to seed the #{"mirror".pluralize(first_hosts.count)}...", :magenta pull_on_hosts(first_hosts) @@ -225,19 +223,28 @@ def connect_to_remote_host(remote_host) end end - def mirror_hosts - if DASH.app_hosts.many? - mirror_hosts = Concurrent::Hash.new - on(DASH.app_hosts) do |host| - first_mirror = capture_with_info(*DASH.builder.first_mirror).strip.presence - mirror_hosts[first_mirror] ||= host.to_s if first_mirror - rescue SSHKit::Command::Failed => e - raise unless e.message =~ /error calling index: reflect: slice index out of range/ - end - mirror_hosts.values - else - [] + # The registry login and the mirror probe share one round trip per host. The probe only + # earns its keep where there is more than one app host to seed, so on a single host the + # login goes on its own; a local registry needs no login and the fold is the probe alone. + # + # A host with no mirror configured fails the `docker info` half with docker's own index + # error, which is what "no mirror" looks like. Anything else still raises - a rejected + # login short-circuits the `&&` and comes back with docker's `unauthorized`/`denied`, + # which this rescue does not match. + def login_and_mirror_hosts + unless DASH.app_hosts.many? + login_to_registry_remotely unless DASH.registry.local? + return [] + end + + mirror_hosts = Concurrent::Hash.new + on(DASH.app_hosts) do |host| + first_mirror = capture_with_info(*DASH.registry.login_then(DASH.builder.first_mirror)).strip.presence + mirror_hosts[first_mirror] ||= host.to_s if first_mirror + rescue SSHKit::Command::Failed => e + raise unless e.message =~ /error calling index: reflect: slice index out of range/ end + mirror_hosts.values end # Audit, clean and pull share one round trip. validate_image keeps its own: folding it diff --git a/lib/dash/commands/registry.rb b/lib/dash/commands/registry.rb index 3c3f743a..4b43ae83 100644 --- a/lib/dash/commands/registry.rb +++ b/lib/dash/commands/registry.rb @@ -13,6 +13,20 @@ def login(registry_config: nil) "-p", sensitive(Dash::Utils.escape_shell_value(registry_config.password)) end + # The login and whatever has to happen after it on the same host, in one round trip. + # `docker login` prints "Login Succeeded" to stdout, so its output is redirected away: + # a caller that captures this gets the folded command's answer and nothing else. A local + # registry needs no login at all, and the fold collapses to the commands alone. + # + # The credentials stay wrapped in sensitive(...) - composing keeps the array elements + # intact, so SSHKit redacts them here exactly as it does for a standalone login. + def login_then(*commands, registry_config: nil) + login = login(registry_config: registry_config) + login = [ *login, ">", "/dev/null" ] if login + + combine login, *commands + end + def logout(registry_config: nil) registry_config ||= config.registry diff --git a/test/cli/build_test.rb b/test/cli/build_test.rb index f4c60b21..a47b58d6 100644 --- a/test/cli/build_test.rb +++ b/test/cli/build_test.rb @@ -353,16 +353,17 @@ class CliBuildTest < CliTestCase end # The audit line, the stale-image removal and the pull are one round trip per host: the - # audit is still written first, and the removal still cannot fail the pull. + # audit is still written first, and the removal still cannot fail the pull. The registry + # login rides along in the mirror probe's round trip rather than paying for one of its own. test "pull" do run_command("pull").tap do |output| - assert_match /docker info --format '{{index .RegistryConfig.Mirrors 0}}'/, output + assert_match "docker login -u [REDACTED] -p [REDACTED] > /dev/null && docker info --format '{{index .RegistryConfig.Mirrors 0}}'", output assert_match %r{Pulled image with version 999" >> \.dash/app-audit\.log && \( docker image rm --force dhh/app:999 \|\| true \) && docker pull dhh/app:999}, output assert_match "docker inspect -f '{{ .Config.Labels.service }}' dhh/app:999 | grep -x app || (echo \"Image dhh/app:999 is missing the 'service' label\" && exit 1)", output end end - test "pull issues two commands per host" do + test "pull issues three round trips per host" do commands = recorded_commands { run_command("pull") } pulls = commands.select { |command| command.include?("docker pull dhh/app:999") } @@ -372,11 +373,50 @@ class CliBuildTest < CliTestCase # An exact total, not a rounded average: integer division would swallow one extra # command on a single host. assert_equal 2 * DASH.app_hosts.size, commands.count { |command| command.include?("dhh/app:999") } + + # The login is folded into the probe, so no host issues it on its own. + assert_equal DASH.app_hosts.size, commands.count { |command| command.include?("docker login") } + assert commands.grep(/docker login/).all? { |command| command.include?("docker info --format") }, commands.grep(/docker login/).inspect + + assert_equal 3 * DASH.app_hosts.size, commands.size, commands.inspect + end + + # Nothing to seed on one host, so the probe never runs and the login goes on its own. + test "pull on a single host logs in without probing for a mirror" do + commands = recorded_commands { run_command("pull", fixture: :with_two_roles_one_host) } + + assert_equal 1, DASH.app_hosts.size + assert commands.first.include?("docker login"), commands.inspect + assert_equal 1, commands.count { |command| command.include?("docker login") } + assert_equal 0, commands.count { |command| command.include?("docker info --format") } + assert_equal 3, commands.size, commands.inspect + end + + # A local registry needs no login, so the fold collapses to the probe alone. + test "pull with a local registry probes for a mirror without logging in" do + Dash::Cli::Build::PortForwarding.any_instance.stubs(:forward).yields + + commands = recorded_commands { run_command("pull", fixture: :with_local_registry) } + + assert_equal 0, commands.count { |command| command.include?("docker login") } + assert_equal DASH.app_hosts.size, commands.count { |command| command.include?("docker info --format") } + assert_equal 3 * DASH.app_hosts.size, commands.size, commands.inspect + end + + # The "no mirror configured" rescue matches docker's index error and nothing else, so a + # rejected login inside the same command still comes back out. + test "pull raises when the login folded into the mirror probe fails" do + SSHKit::Backend::Abstract.any_instance.stubs(:capture_with_info) + .with { |*args| args.include?(:login) } + .raises(SSHKit::Command::Failed.new("unauthorized: incorrect username or password")) + + error = assert_raises(SSHKit::Runner::ExecuteError) { run_command("pull") } + assert_match "unauthorized", error.message end test "pull with mirror" do SSHKit::Backend::Abstract.any_instance.expects(:capture_with_info) - .with(:docker, :info, "--format '{{index .RegistryConfig.Mirrors 0}}'") + .with { |*args| args.join(" ").end_with?("docker info --format '{{index .RegistryConfig.Mirrors 0}}'") } .returns("registry-mirror.example.com") .at_least_once @@ -390,7 +430,7 @@ class CliBuildTest < CliTestCase test "pull with mirrors" do SSHKit::Backend::Abstract.any_instance.expects(:capture_with_info) - .with(:docker, :info, "--format '{{index .RegistryConfig.Mirrors 0}}'") + .with { |*args| args.join(" ").end_with?("docker info --format '{{index .RegistryConfig.Mirrors 0}}'") } .returns("registry-mirror.example.com", "registry-mirror2.example.com") .at_least_once diff --git a/test/cli/main_test.rb b/test/cli/main_test.rb index c5a6f94f..de929f74 100644 --- a/test/cli/main_test.rb +++ b/test/cli/main_test.rb @@ -175,7 +175,7 @@ class CliMainTest < CliTestCase .returns("") SSHKit::Backend::Abstract.any_instance.expects(:capture_with_info) - .with(:docker, :info, "--format '{{index .RegistryConfig.Mirrors 0}}'") + .with { |*args| args.join(" ").end_with?("docker info --format '{{index .RegistryConfig.Mirrors 0}}'") } .returns("") .at_least_once @@ -250,7 +250,7 @@ class CliMainTest < CliTestCase .returns("") SSHKit::Backend::Abstract.any_instance.stubs(:capture_with_info) - .with(:docker, :info, "--format '{{index .RegistryConfig.Mirrors 0}}'") + .with { |*args| args.join(" ").end_with?("docker info --format '{{index .RegistryConfig.Mirrors 0}}'") } .returns("") assert_raises(Dash::Cli::LockError) do @@ -287,7 +287,7 @@ class CliMainTest < CliTestCase .returns("") SSHKit::Backend::Abstract.any_instance.stubs(:capture_with_info) - .with(:docker, :info, "--format '{{index .RegistryConfig.Mirrors 0}}'") + .with { |*args| args.join(" ").end_with?("docker info --format '{{index .RegistryConfig.Mirrors 0}}'") } .returns("") error = assert_raises(Dash::Cli::LockError) do @@ -344,7 +344,7 @@ class CliMainTest < CliTestCase .returns("") SSHKit::Backend::Abstract.any_instance.expects(:capture_with_info) - .with(:docker, :info, "--format '{{index .RegistryConfig.Mirrors 0}}'") + .with { |*args| args.join(" ").end_with?("docker info --format '{{index .RegistryConfig.Mirrors 0}}'") } .returns("") .at_least_once diff --git a/test/commands/registry_test.rb b/test/commands/registry_test.rb index d454b46f..9ccf34c7 100755 --- a/test/commands/registry_test.rb +++ b/test/commands/registry_test.rb @@ -73,6 +73,26 @@ class CommandsRegistryTest < ActiveSupport::TestCase end end + # The login's own stdout is discarded so a capture folded in behind it comes back with + # the following command's answer alone. + test "registry login then command" do + assert_equal \ + "docker login hub.docker.com -u \"dhh\" -p \"secret\" > /dev/null && docker info --format '{{index .RegistryConfig.Mirrors 0}}'", + registry.login_then([ :docker, :info, "--format '{{index .RegistryConfig.Mirrors 0}}'" ]).join(" ") + end + + test "registry login then command keeps the credentials sensitive" do + command = registry.login_then([ :docker, :info ]) + + assert_equal [ "\"dhh\"", "\"secret\"" ], command.grep(Dash::Utils::Sensitive).map(&:unredacted) + end + + test "registry login then command with a local registry issues the command alone" do + @config[:registry] = { "server" => "localhost:5000" } + + assert_equal "docker info", registry.login_then([ :docker, :info ]).join(" ") + end + test "registry logout" do assert_equal \ "docker logout hub.docker.com",