From 06f4caa866a3ffef2c40a50093963dd8eae4a18e Mon Sep 17 00:00:00 2001
From: Donal McBreen <donal@37signals.com>
Date: Tue, 10 Sep 2024 10:24:14 +0100
Subject: [PATCH] Make the secrets commands inline aware

Rather than redirecting the global $stdout, which is not never clever in
a threaded program, we'll make the secrets commands aware they are
being inlined, so they return the value instead of printing it.

Additionally we no longer need to interrupt the parent process on error
as we've inlined the command - exit 1 is enough.
---
 lib/kamal/cli/secrets.rb                      | 35 +++++++++++--------
 lib/kamal/secrets.rb                          | 10 +-----
 .../dotenv/inline_command_substitution.rb     | 11 ++----
 ...dotenv_inline_command_substitution_test.rb |  2 +-
 4 files changed, 26 insertions(+), 32 deletions(-)

diff --git a/lib/kamal/cli/secrets.rb b/lib/kamal/cli/secrets.rb
index 572476f9..cab53ca8 100644
--- a/lib/kamal/cli/secrets.rb
+++ b/lib/kamal/cli/secrets.rb
@@ -3,26 +3,26 @@ class Kamal::Cli::Secrets < Kamal::Cli::Base
   option :adapter, type: :string, aliases: "-a", required: true, desc: "Which vault adapter to use"
   option :account, type: :string, required: true, desc: "The account identifier or username"
   option :from, type: :string, required: false, desc: "A vault or folder to fetch the secrets from"
+  option :inline, type: :boolean, required: false, hidden: true
   def fetch(*secrets)
-    results = adapter(options[:adapter]).fetch(secrets, **options.slice(:account, :from).symbolize_keys)
-    puts JSON.dump(results).shellescape
-  rescue => e
-    handle_error(e)
+    handle_output(inline: options[:inline]) do
+      results = adapter(options[:adapter]).fetch(secrets, **options.slice(:account, :from).symbolize_keys)
+      JSON.dump(results).shellescape
+    end
   end
 
   desc "extract", "Extract a single secret from the results of a fetch call"
+  option :inline, type: :boolean, required: false, hidden: true
   def extract(name, secrets)
-    parsed_secrets = JSON.parse(secrets)
+    handle_output(inline: options[:inline]) do
+      parsed_secrets = JSON.parse(secrets)
 
-    if (value = parsed_secrets[name]).nil?
-      value = parsed_secrets.find { |k, v| k.end_with?("/#{name}") }&.last
+      value = parsed_secrets[name] || parsed_secrets.find { |k, v| k.end_with?("/#{name}") }&.last
+
+      raise "Could not find secret #{name}" if value.nil?
+
+      value
     end
-
-    raise "Could not find secret #{name}" if value.nil?
-
-    puts value
-  rescue => e
-    handle_error(e)
   end
 
   private
@@ -30,11 +30,18 @@ class Kamal::Cli::Secrets < Kamal::Cli::Base
       Kamal::Secrets::Adapters.lookup(adapter)
     end
 
+    def handle_output(inline: nil)
+      yield.tap do |output|
+        puts output unless inline
+      end
+    rescue => e
+      handle_error(e)
+    end
+
     def handle_error(e)
       $stderr.puts "  \e[31mERROR (#{e.class}): #{e.message}\e[0m"
       $stderr.puts e.backtrace if ENV["VERBOSE"]
 
-      Process.kill("INT", Process.ppid) if ENV["KAMAL_SECRETS_INT_PARENT"]
       exit 1
     end
 end
diff --git a/lib/kamal/secrets.rb b/lib/kamal/secrets.rb
index dc135331..34d30aae 100644
--- a/lib/kamal/secrets.rb
+++ b/lib/kamal/secrets.rb
@@ -30,17 +30,9 @@ class Kamal::Secrets
 
     def parse_secrets
       if secrets_file
-        interrupting_parent_on_error { ::Dotenv.parse(secrets_file) }
+        ::Dotenv.parse(secrets_file)
       else
         {}
       end
     end
-
-    def interrupting_parent_on_error
-      # Make any `kamal secrets` calls in dotenv interpolation interrupt this process if there are errors
-      ENV["KAMAL_SECRETS_INT_PARENT"] = "1"
-      yield
-    ensure
-      ENV.delete("KAMAL_SECRETS_INT_PARENT")
-    end
 end
diff --git a/lib/kamal/secrets/dotenv/inline_command_substitution.rb b/lib/kamal/secrets/dotenv/inline_command_substitution.rb
index e8e12d5c..c9ef9879 100644
--- a/lib/kamal/secrets/dotenv/inline_command_substitution.rb
+++ b/lib/kamal/secrets/dotenv/inline_command_substitution.rb
@@ -16,7 +16,7 @@ class Kamal::Secrets::Dotenv::InlineCommandSubstitution
         else
           if command =~ /\A\s*kamal\s*secrets\s+/
             # Inline the command
-            capture_stdout { Kamal::Cli::Main.start(command.shellsplit[1..]) }.chomp
+            inline_secrets_command(command)
           else
             # Execute the command and return the value
             `#{command}`.chomp
@@ -25,13 +25,8 @@ class Kamal::Secrets::Dotenv::InlineCommandSubstitution
       end
     end
 
-    def capture_stdout
-      old_stdout = $stdout
-      $stdout = StringIO.new
-      yield
-      $stdout.string
-    ensure
-      $stdout = old_stdout
+    def inline_secrets_command(command)
+      Kamal::Cli::Main.start(command.shellsplit[1..] + [ "--inline" ]).chomp
     end
   end
 end
diff --git a/test/secrets/dotenv_inline_command_substitution_test.rb b/test/secrets/dotenv_inline_command_substitution_test.rb
index 041ada29..3ae10ca9 100644
--- a/test/secrets/dotenv_inline_command_substitution_test.rb
+++ b/test/secrets/dotenv_inline_command_substitution_test.rb
@@ -2,7 +2,7 @@ require "test_helper"
 
 class SecretsInlineCommandSubstitution < SecretAdapterTestCase
   test "inlines kamal secrets commands" do
-    Kamal::Cli::Main.expects(:start).with { |command| puts "results"; command == [ "secrets", "fetch", "..." ] }
+    Kamal::Cli::Main.expects(:start).with { |command| command == [ "secrets", "fetch", "...", "--inline" ] }.returns("results")
     substituted = Kamal::Secrets::Dotenv::InlineCommandSubstitution.call("FOO=$(kamal secrets fetch ...)", nil, overwrite: false)
     assert_equal "FOO=results", substituted
   end