diff --git a/app/models/shipit/deploy_spec/file_system.rb b/app/models/shipit/deploy_spec/file_system.rb index fc55f1f49..06b3c4c81 100644 --- a/app/models/shipit/deploy_spec/file_system.rb +++ b/app/models/shipit/deploy_spec/file_system.rb @@ -151,9 +151,18 @@ def app_name end SHIPIT_CONFIG_INHERIT_FROM_KEY = "inherit_from" - def build_config(path, config_obj) + MAX_INHERIT_FROM_DEPTH = 10 + + def build_config(path, config_obj, depth = 0) return config_obj if config_obj.blank? || !config_obj.key?(SHIPIT_CONFIG_INHERIT_FROM_KEY) + # An inherit_from cycle (a.yml <-> b.yml) would otherwise recurse until + # SystemStackError. Surface a clear configuration error instead. + if depth >= MAX_INHERIT_FROM_DEPTH + raise Error, "inherit_from chain exceeds #{MAX_INHERIT_FROM_DEPTH} levels " \ + "(cycle?) while resolving #{path}" + end + inherits_from_path = path.dirname.join(config_obj.delete(SHIPIT_CONFIG_INHERIT_FROM_KEY)) if inherits_from_path.exist? inherits_config_obj = read_config(inherits_from_path) @@ -161,7 +170,7 @@ def build_config(path, config_obj) path = inherits_from_path end - build_config(path, config_obj) + build_config(path, config_obj, depth + 1) end def read_config(path) diff --git a/app/models/shipit/deploy_spec/git_object_file_system.rb b/app/models/shipit/deploy_spec/git_object_file_system.rb index de82b665f..cc2cf171c 100644 --- a/app/models/shipit/deploy_spec/git_object_file_system.rb +++ b/app/models/shipit/deploy_spec/git_object_file_system.rb @@ -79,7 +79,7 @@ def file(path, root: false) # file. The old checkout-based path is uncapped (a cycle loops forever) # and follows escaping paths onto the worker filesystem; both are hard # fallbacks here. - def build_config(path, config_obj) + def build_config(path, config_obj, depth = 0) if config_obj.present? && config_obj.key?(SHIPIT_CONFIG_INHERIT_FROM_KEY) @inherit_reads += 1 raise FallbackRequired.new(:inherit_depth, @inherit_chain.join(' -> ')) if @inherit_reads > MAX_INHERIT_READS diff --git a/test/models/deploy_spec_test.rb b/test/models/deploy_spec_test.rb index e492331ec..26d8019ff 100644 --- a/test/models/deploy_spec_test.rb +++ b/test/models/deploy_spec_test.rb @@ -11,6 +11,31 @@ class DeploySpecTest < ActiveSupport::TestCase @spec.stubs(:load_config).returns({}) end + test 'an inherit_from cycle raises a clear error instead of recursing forever' do + Dir.mktmpdir do |dir| + File.write(File.join(dir, 'shipit.production.yml'), "inherit_from: a.yml\n") + File.write(File.join(dir, 'a.yml'), "inherit_from: b.yml\n") + File.write(File.join(dir, 'b.yml'), "inherit_from: a.yml\n") + + spec = DeploySpec::FileSystem.new(dir, @stack) + error = assert_raises(DeploySpec::Error) { spec.cacheable } + assert_match(/inherit_from chain exceeds/, error.message) + end + end + + test 'an inherit_from chain under the depth cap resolves normally' do + Dir.mktmpdir do |dir| + File.write(File.join(dir, 'shipit.production.yml'), "inherit_from: a.yml\ndeploy:\n override:\n - echo child\n") + File.write(File.join(dir, 'a.yml'), "inherit_from: b.yml\n") + File.write(File.join(dir, 'b.yml'), "machine:\n environment:\n FOO: bar\n") + + spec = DeploySpec::FileSystem.new(dir, @stack) + config = spec.cacheable.config + assert_equal ['echo child'], config.dig('deploy', 'override') + assert_equal 'bar', config.dig('machine', 'environment', 'FOO') + end + end + test '#supports_fetch_deployed_revision? returns false by default' do refute @spec.supports_fetch_deployed_revision? end