Ensure env secrets are merged correctly with roles

lib/mrsk/configuration.rb

@@ -7,7 +7,7 @@ require "mrsk/utils"
 
 class Mrsk::Configuration
   delegate :service, :image, :servers, :env, :labels, :registry, :builder, to: :config, allow_nil: true
-  delegate :argumentize, to: Mrsk::Utils
+  delegate :argumentize_env_with_secrets, to: Mrsk::Utils
 
   class << self
     def create_from(base_config_file, destination: nil, version: "missing")
@@ -81,11 +81,7 @@ class Mrsk::Configuration
 
   def env_args
     if config.env.present?
-      if config.env["secret"].present?
+      argumentize_env_with_secrets(config.env)
-        argumentize("-e", expand_env_secrets, redacted: true) + argumentize("-e", config.env["clear"])
-      else
-        argumentize "-e", config.env
-      end
     else
       []
     end
@@ -138,10 +134,6 @@ class Mrsk::Configuration
     def role_names
       config.servers.is_a?(Array) ? [ "web" ] : config.servers.keys.sort
     end
-
-    def expand_env_secrets
-      config.env["secret"].to_h { |key| [ key, ENV.fetch(key) ] }
-    end
 end
 
 require "mrsk/configuration/role"

lib/mrsk/configuration/role.rb

@@ -1,5 +1,5 @@
 class Mrsk::Configuration::Role
-  delegate :argumentize, to: Mrsk::Utils
+  delegate :argumentize, :argumentize_env_with_secrets, to: Mrsk::Utils
 
   attr_accessor :name
 
@@ -20,11 +20,15 @@ class Mrsk::Configuration::Role
   end
 
   def env
-    (config.env || {}).merge(specializations["env"] || {})
+    if config.env && config.env["secret"]
+      merged_env_with_secrets
+    else
+      merged_env
+    end
   end
 
   def env_args
-    argumentize "-e", env
+    argumentize_env_with_secrets env
   end
 
   def cmd
@@ -79,4 +83,20 @@ class Mrsk::Configuration::Role
         config.servers[name].except("hosts")
       end
     end
+
+    def specialized_env
+      specializations["env"] || {}
+    end
+
+    def merged_env
+      config.env&.merge(specialized_env) || {}
+    end
+
+    # Secrets are stored in an array, which won't merge by default, so have to do it by hand.
+    def merged_env_with_secrets
+      merged_env.tap do |new_env|
+        new_env["secret"] = Array(config.env["secret"]) + Array(specialized_env["secret"])
+        new_env["clear"]  = (Array(config.env["clear"] || config.env) + Array(specialized_env["clear"] || specialized_env)).uniq
+      end
+    end
 end

lib/mrsk/utils.rb

@@ -6,6 +6,16 @@ module Mrsk::Utils
     Array(attributes).flat_map { |k, v| [ argument, redacted ? redact("#{k}=#{v}") : "#{k}=#{v}" ] }
   end
 
+  # Return a list of shell arguments using the same named argument against the passed attributes,
+  # but redacts and expands secrets.
+  def argumentize_env_with_secrets(env)
+    if (secrets = env["secret"]).present?
+      argumentize("-e", secrets.to_h { |key| [ key, ENV.fetch(key) ] }, redacted: true) + argumentize("-e", env["clear"])
+    else
+      argumentize "-e", env
+    end
+  end
+
   # Copied from SSHKit::Backend::Abstract#redact to be available inside Commands classes
   def redact(arg) # Used in execute_command to hide redact() args a user passes in
     arg.to_s.extend(SSHKit::Redaction) # to_s due to our inability to extend Integer, etc

test/configuration_role_test.rb

@@ -74,4 +74,68 @@ class ConfigurationRoleTest < ActiveSupport::TestCase
     assert_equal "redis://a/b", @config_with_roles.role(:workers).env["REDIS_URL"]
     assert_equal ["-e", "REDIS_URL=redis://a/b", "-e", "WEB_CONCURRENCY=4"], @config_with_roles.role(:workers).env_args
   end
+
+  test "env secret overwritten by role" do
+    @deploy_with_roles[:env] = {
+      "clear" => {
+        "REDIS_URL" => "redis://a/b"
+      },
+      "secret" => [
+        "REDIS_PASSWORD"
+      ]
+    }
+
+    @deploy_with_roles[:servers]["workers"]["env"] = {
+      "clear" => {
+        "REDIS_URL" => "redis://a/b",
+        "WEB_CONCURRENCY" => 4
+      },
+      "secret" => [
+        "DB_PASSWORD"
+      ]
+    }
+
+    ENV["REDIS_PASSWORD"] = "secret456"
+    ENV["DB_PASSWORD"] = "secret123"
+    
+    assert_equal ["-e", "REDIS_PASSWORD=secret456", "-e", "DB_PASSWORD=secret123", "-e", "REDIS_URL=redis://a/b", "-e", "WEB_CONCURRENCY=4"], @config_with_roles.role(:workers).env_args
+  ensure
+    ENV["REDIS_PASSWORD"] = nil
+    ENV["DB_PASSWORD"] = nil
+  end
+
+  test "env secrets only in role" do
+    @deploy_with_roles[:servers]["workers"]["env"] = {
+      "clear" => {
+        "REDIS_URL" => "redis://a/b",
+        "WEB_CONCURRENCY" => 4
+      },
+      "secret" => [
+        "DB_PASSWORD"
+      ]
+    }
+
+    ENV["DB_PASSWORD"] = "secret123"
+    
+    assert_equal ["-e", "DB_PASSWORD=secret123", "-e", "REDIS_URL=redis://a/b", "-e", "WEB_CONCURRENCY=4"], @config_with_roles.role(:workers).env_args
+  ensure
+    ENV["DB_PASSWORD"] = nil
+  end
+
+  test "env secrets only at top level" do
+    @deploy_with_roles[:env] = {
+      "clear" => {
+        "REDIS_URL" => "redis://a/b"
+      },
+      "secret" => [
+        "REDIS_PASSWORD"
+      ]
+    }
+
+    ENV["REDIS_PASSWORD"] = "secret456"
+    
+    assert_equal ["-e", "REDIS_PASSWORD=secret456", "-e", "REDIS_URL=redis://a/b", "-e", "WEB_CONCURRENCY=4"], @config_with_roles.role(:workers).env_args
+  ensure
+    ENV["REDIS_PASSWORD"] = nil
+  end
 end