Fix proxy host overrides in destination configs

Author: cookesan

lib/kamal/configuration.rb

@@ -31,7 +31,50 @@ def load_raw_config(config_file:, destination: nil)
 
     private
       def load_config_files(*files)
-        files.inject({}) { |config, file| config.deep_merge! load_config_file(file) }
+        files.inject({}) { |config, file| merge_config_file(config, load_config_file(file)) }
+      end
+
+      def merge_config_file(config, overrides)
+        remove_replaced_proxy_host_keys(config, overrides)
+        config.deep_merge! overrides
+      end
+
+      def remove_replaced_proxy_host_keys(config, overrides)
+        remove_replaced_host_key value_for(config, :proxy), value_for(overrides, :proxy)
+        remove_replaced_role_proxy_host_keys value_for(config, :servers), value_for(overrides, :servers)
+      end
+
+      def remove_replaced_role_proxy_host_keys(config, overrides)
+        return unless config.is_a?(Hash) && overrides.is_a?(Hash)
+
+        overrides.each do |role, role_overrides|
+          role_config = value_for(config, role)
+          remove_replaced_host_key value_for(role_config, "proxy"), value_for(role_overrides, "proxy")
+        end
+      end
+
+      def remove_replaced_host_key(config, overrides)
+        return unless config.is_a?(Hash) && overrides.is_a?(Hash)
+
+        delete_key(config, "host") if has_key?(overrides, "hosts")
+        delete_key(config, "hosts") if has_key?(overrides, "host")
+      end
+
+      def has_key?(config, key)
+        config.key?(key) || config.key?(key.to_sym)
+      end
+
+      def delete_key(config, key)
+        config.delete(key)
+        config.delete(key.to_sym)
+      end
+
+      def value_for(config, key)
+        return unless config.is_a?(Hash)
+
+        alternate_key = key.is_a?(Symbol) ? key.to_s : key.to_sym
+        return config[key] if config.key?(key)
+        config[alternate_key] if config.key?(alternate_key)
       end
 
       def load_config_file(file)

lib/kamal/configuration/proxy.rb

@@ -110,10 +110,19 @@ def stop_command_args(**options)
   end
 
   def merge(other)
-    self.class.new config: config, proxy_config: other.proxy_config.deep_merge(proxy_config), role_name: role_name, secrets: secrets
+    self.class.new config: config, proxy_config: merge_proxy_config(other.proxy_config, proxy_config), role_name: role_name, secrets: secrets
   end
 
   private
+    def merge_proxy_config(inherited_config, overrides)
+      inherited_config = inherited_config.dup
+
+      inherited_config.delete("host") if overrides.key?("hosts")
+      inherited_config.delete("hosts") if overrides.key?("host")
+
+      inherited_config.deep_merge(overrides)
+    end
+
     def tls_path(directory, filename)
       File.join([ directory, role_name, filename ].compact) if custom_ssl_certificate?
     end

test/configuration/role_test.rb

@@ -68,6 +68,20 @@ class ConfigurationRoleTest < ActiveSupport::TestCase
     assert_equal [ "--label", "service=\"app\"", "--label", "role=\"beta\"", "--label", "destination" ], config.role(:beta).label_args
   end
 
+  test "role proxy hosts replace root proxy host" do
+    @deploy_with_roles[:proxy] = { "host" => "app.example.com" }
+    @deploy_with_roles[:servers]["web"] = { "hosts" => [ "1.1.1.1" ], "proxy" => { "hosts" => [ "web.example.com", "files.example.com" ] } }
+
+    assert_equal [ "web.example.com", "files.example.com" ], config_with_roles.role(:web).proxy.hosts
+  end
+
+  test "role proxy host replaces root proxy hosts" do
+    @deploy_with_roles[:proxy] = { "hosts" => [ "app.example.com", "files.example.com" ] }
+    @deploy_with_roles[:servers]["workers"]["proxy"] = { "host" => "jobs.example.com" }
+
+    assert_equal [ "jobs.example.com" ], config_with_roles.role(:workers).proxy.hosts
+  end
+
   test "env overwritten by role" do
     assert_equal "redis://a/b", config_with_roles.role(:workers).env("1.1.1.3").clear["REDIS_URL"]
 

test/configuration_test.rb

@@ -267,6 +267,122 @@ class ConfigurationTest < ActiveSupport::TestCase
     assert_equal "1.1.1.3", config.all_hosts.first
   end
 
+  test "destination proxy hosts replace proxy host" do
+    with_config_files(
+      "deploy.yml" => <<~YAML,
+        service: app
+        image: dhh/app
+        registry:
+          username: dhh
+          password: secret
+        builder:
+          arch: amd64
+        servers:
+          - 1.1.1.1
+        proxy:
+          host: myapp.dev
+      YAML
+      "deploy.staging.yml" => <<~YAML
+        proxy:
+          hosts:
+            - myapp.dev
+            - files.myapp.dev
+      YAML
+    ) do |config_file|
+      config = Kamal::Configuration.create_from config_file: config_file, destination: "staging"
+      assert_equal [ "myapp.dev", "files.myapp.dev" ], config.proxy.hosts
+    end
+  end
+
+  test "destination proxy host replaces proxy hosts" do
+    with_config_files(
+      "deploy.yml" => <<~YAML,
+        service: app
+        image: dhh/app
+        registry:
+          username: dhh
+          password: secret
+        builder:
+          arch: amd64
+        servers:
+          - 1.1.1.1
+        proxy:
+          hosts:
+            - myapp.dev
+            - files.myapp.dev
+      YAML
+      "deploy.staging.yml" => <<~YAML
+        proxy:
+          host: myapp.dev
+      YAML
+    ) do |config_file|
+      config = Kamal::Configuration.create_from config_file: config_file, destination: "staging"
+      assert_equal [ "myapp.dev" ], config.proxy.hosts
+    end
+  end
+
+  test "destination role proxy hosts replace role proxy host" do
+    with_config_files(
+      "deploy.yml" => <<~YAML,
+        service: app
+        image: dhh/app
+        registry:
+          username: dhh
+          password: secret
+        builder:
+          arch: amd64
+        servers:
+          web:
+            hosts:
+              - 1.1.1.1
+            proxy:
+              host: web.myapp.dev
+      YAML
+      "deploy.staging.yml" => <<~YAML
+        servers:
+          web:
+            proxy:
+              hosts:
+                - web.myapp.dev
+                - files.myapp.dev
+      YAML
+    ) do |config_file|
+      config = Kamal::Configuration.create_from config_file: config_file, destination: "staging"
+      assert_equal [ "web.myapp.dev", "files.myapp.dev" ], config.role(:web).proxy.hosts
+    end
+  end
+
+  test "destination role proxy host replaces role proxy hosts" do
+    with_config_files(
+      "deploy.yml" => <<~YAML,
+        service: app
+        image: dhh/app
+        registry:
+          username: dhh
+          password: secret
+        builder:
+          arch: amd64
+        servers:
+          web:
+            hosts:
+              - 1.1.1.1
+            proxy:
+              hosts:
+                - web.myapp.dev
+                - files.myapp.dev
+      YAML
+      "deploy.staging.yml" => <<~YAML
+        servers:
+          web:
+            proxy:
+              host: web.myapp.dev
+      YAML
+    ) do |config_file|
+      config = Kamal::Configuration.create_from config_file: config_file, destination: "staging"
+      assert_equal [ "web.myapp.dev" ], config.role(:web).proxy.hosts
+    end
+  end
+
   test "destination yml config file missing" do
     dest_config_file = Pathname.new(File.expand_path("fixtures/deploy_for_dest.yml", __dir__))
 
@@ -463,4 +579,15 @@ class ConfigurationTest < ActiveSupport::TestCase
     end
     assert_match /Invalid hooks_output 'invalid' for hook 'pre-deploy'/, error.message
   end
+
+  private
+    def with_config_files(files)
+      Dir.mktmpdir do |dir|
+        files.each do |name, contents|
+          File.write(File.join(dir, name), contents)
+        end
+
+        yield Pathname.new(File.join(dir, "deploy.yml"))
+      end
+    end
 end