perf(build): fold the registry login into the mirror probe's round trip

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

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
 

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
 

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
 

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",