Merge branch 'feature/gb/improve-include-config-errors-reporting' into 'master'

Improve validation errors for external CI/CD configuration

Closes #51369

See merge request gitlab-org/gitlab-ce!22394

18 files changed, 384 insertions, 204 deletions

diff --git a/changelogs/unreleased/feature-gb-improve-include-config-errors-reporting.yml b/changelogs/unreleased/feature-gb-improve-include-config-errors-reporting.yml
new file mode 100644
index 00000000000..67eb6b78096
--- /dev/null
+++ b/changelogs/unreleased/feature-gb-improve-include-config-errors-reporting.yml
@@ -0,0 +1,5 @@
+---
+title: Improve validation errors for external CI/CD configuration
+merge_request: 22394
+author:
+type: added
diff --git a/lib/gitlab/ci/config.rb b/lib/gitlab/ci/config.rb
index fe98d25af29..fedaf18ef30 100644
--- a/lib/gitlab/ci/config.rb
+++ b/lib/gitlab/ci/config.rb
@@ -13,10 +13,10 @@ module Gitlab
 
         @global = Entry::Global.new(@config)
         @global.compose!
-      rescue Loader::FormatError, Extendable::ExtensionError => e
+      rescue Loader::FormatError,
+             Extendable::ExtensionError,
+             External::Processor::IncludeError => e
         raise Config::ConfigError, e.message
-      rescue ::Gitlab::Ci::External::Processor::FileError => e
-        raise ::Gitlab::Ci::YamlProcessor::ValidationError, e.message
       end
 
       def valid?
@@ -81,7 +81,7 @@ module Gitlab
 
       def process_external_files(config, project, opts)
         sha = opts.fetch(:sha) { project.repository.root_ref_sha }
-        ::Gitlab::Ci::External::Processor.new(config, project, sha).perform
+        Config::External::Processor.new(config, project, sha).perform
       end
     end
   end
diff --git a/lib/gitlab/ci/config/external/file/base.rb b/lib/gitlab/ci/config/external/file/base.rb
new file mode 100644
index 00000000000..15ca47ef60e
--- /dev/null
+++ b/lib/gitlab/ci/config/external/file/base.rb
@@ -0,0 +1,75 @@
+# frozen_string_literal: true
+
+module Gitlab
+  module Ci
+    class Config
+      module External
+        module File
+          class Base
+            include Gitlab::Utils::StrongMemoize
+
+            attr_reader :location, :opts, :errors
+
+            YAML_WHITELIST_EXTENSION = /.+\.(yml|yaml)$/i.freeze
+
+            def initialize(location, opts = {})
+              @location = location
+              @opts = opts
+              @errors = []
+
+              validate!
+            end
+
+            def invalid_extension?
+              !::File.basename(location).match(YAML_WHITELIST_EXTENSION)
+            end
+
+            def valid?
+              errors.none?
+            end
+
+            def error_message
+              errors.first
+            end
+
+            def content
+              raise NotImplementedError, 'subclass must implement fetching raw content'
+            end
+
+            def to_hash
+              @hash ||= Ci::Config::Loader.new(content).load!
+            rescue Ci::Config::Loader::FormatError
+              nil
+            end
+
+            protected
+
+            def validate!
+              validate_location!
+              validate_content! if errors.none?
+              validate_hash! if errors.none?
+            end
+
+            def validate_location!
+              if invalid_extension?
+                errors.push("Included file `#{location}` does not have YAML extension!")
+              end
+            end
+
+            def validate_content!
+              if content.blank?
+                errors.push("Included file `#{location}` is empty or does not exist!")
+              end
+            end
+
+            def validate_hash!
+              if to_hash.blank?
+                errors.push("Included file `#{location}` does not have valid YAML syntax!")
+              end
+            end
+          end
+        end
+      end
+    end
+  end
+end
diff --git a/lib/gitlab/ci/config/external/file/local.rb b/lib/gitlab/ci/config/external/file/local.rb
new file mode 100644
index 00000000000..2a256aff65c
--- /dev/null
+++ b/lib/gitlab/ci/config/external/file/local.rb
@@ -0,0 +1,42 @@
+# frozen_string_literal: true
+
+module Gitlab
+  module Ci
+    class Config
+      module External
+        module File
+          class Local < Base
+            include Gitlab::Utils::StrongMemoize
+
+            attr_reader :project, :sha
+
+            def initialize(location, opts = {})
+              @project = opts.fetch(:project)
+              @sha = opts.fetch(:sha)
+
+              super
+            end
+
+            def content
+              strong_memoize(:content) { fetch_local_content }
+            end
+
+            private
+
+            def validate_content!
+              if content.nil?
+                errors.push("Local file `#{location}` does not exist!")
+              elsif content.blank?
+                errors.push("Local file `#{location}` is empty!")
+              end
+            end
+
+            def fetch_local_content
+              project.repository.blob_data_at(sha, location)
+            end
+          end
+        end
+      end
+    end
+  end
+end
diff --git a/lib/gitlab/ci/config/external/file/remote.rb b/lib/gitlab/ci/config/external/file/remote.rb
new file mode 100644
index 00000000000..86fa5ad8800
--- /dev/null
+++ b/lib/gitlab/ci/config/external/file/remote.rb
@@ -0,0 +1,49 @@
+# frozen_string_literal: true
+
+module Gitlab
+  module Ci
+    class Config
+      module External
+        module File
+          class Remote < Base
+            include Gitlab::Utils::StrongMemoize
+
+            def content
+              strong_memoize(:content) { fetch_remote_content }
+            end
+
+            private
+
+            def validate_location!
+              super
+
+              unless ::Gitlab::UrlSanitizer.valid?(location)
+                errors.push("Remote file `#{location}` does not have a valid address!")
+              end
+            end
+
+            def fetch_remote_content
+              begin
+                response = Gitlab::HTTP.get(location)
+              rescue SocketError
+                errors.push("Remote file `#{location}` could not be fetched because of a socket error!")
+              rescue Timeout::Error
+                errors.push("Remote file `#{location}` could not be fetched because of a timeout error!")
+              rescue Gitlab::HTTP::Error
+                errors.push("Remote file `#{location}` could not be fetched because of HTTP error!")
+              rescue Gitlab::HTTP::BlockedUrlError => e
+                errors.push("Remote file could not be fetched because #{e}!")
+              end
+
+              if response&.code.to_i >= 400
+                errors.push("Remote file `#{location}` could not be fetched because of HTTP code `#{response.code}` error!")
+              end
+
+              response.to_s if errors.none?
+            end
+          end
+        end
+      end
+    end
+  end
+end
diff --git a/lib/gitlab/ci/config/external/mapper.rb b/lib/gitlab/ci/config/external/mapper.rb
new file mode 100644
index 00000000000..def3563e505
--- /dev/null
+++ b/lib/gitlab/ci/config/external/mapper.rb
@@ -0,0 +1,33 @@
+# frozen_string_literal: true
+
+module Gitlab
+  module Ci
+    class Config
+      module External
+        class Mapper
+          def initialize(values, project, sha)
+            @locations = Array(values.fetch(:include, []))
+            @project = project
+            @sha = sha
+          end
+
+          def process
+            locations.map { |location| build_external_file(location) }
+          end
+
+          private
+
+          attr_reader :locations, :project, :sha
+
+          def build_external_file(location)
+            if ::Gitlab::UrlSanitizer.valid?(location)
+              External::File::Remote.new(location)
+            else
+              External::File::Local.new(location, project: project, sha: sha)
+            end
+          end
+        end
+      end
+    end
+  end
+end
diff --git a/lib/gitlab/ci/config/external/processor.rb b/lib/gitlab/ci/config/external/processor.rb
new file mode 100644
index 00000000000..eae0bdeb644
--- /dev/null
+++ b/lib/gitlab/ci/config/external/processor.rb
@@ -0,0 +1,50 @@
+# frozen_string_literal: true
+
+module Gitlab
+  module Ci
+    class Config
+      module External
+        class Processor
+          IncludeError = Class.new(StandardError)
+
+          def initialize(values, project, sha)
+            @values = values
+            @external_files = External::Mapper.new(values, project, sha).process
+            @content = {}
+          end
+
+          def perform
+            return @values if @external_files.empty?
+
+            validate_external_files!
+            merge_external_files!
+            append_inline_content!
+            remove_include_keyword!
+          end
+
+          private
+
+          def validate_external_files!
+            @external_files.each do |file|
+              raise IncludeError, file.error_message unless file.valid?
+            end
+          end
+
+          def merge_external_files!
+            @external_files.each do |file|
+              @content.deep_merge!(file.to_hash)
+            end
+          end
+
+          def append_inline_content!
+            @content.deep_merge!(@values)
+          end
+
+          def remove_include_keyword!
+            @content.tap { @content.delete(:include) }
+          end
+        end
+      end
+    end
+  end
+end
diff --git a/lib/gitlab/ci/external/file/base.rb b/lib/gitlab/ci/external/file/base.rb
deleted file mode 100644
index f4da07b0b02..00000000000
--- a/lib/gitlab/ci/external/file/base.rb
+++ /dev/null
@@ -1,29 +0,0 @@
-# frozen_string_literal: true
-
-module Gitlab
-  module Ci
-    module External
-      module File
-        class Base
-          YAML_WHITELIST_EXTENSION = /(yml|yaml)$/i.freeze
-
-          def initialize(location, opts = {})
-            @location = location
-          end
-
-          def valid?
-            location.match(YAML_WHITELIST_EXTENSION) && content
-          end
-
-          def content
-            raise NotImplementedError, 'content must be implemented and return a string or nil'
-          end
-
-          def error_message
-            raise NotImplementedError, 'error_message must be implemented and return a string'
-          end
-        end
-      end
-    end
-  end
-end
diff --git a/lib/gitlab/ci/external/file/local.rb b/lib/gitlab/ci/external/file/local.rb
deleted file mode 100644
index 1aa7f687507..00000000000
--- a/lib/gitlab/ci/external/file/local.rb
+++ /dev/null
@@ -1,34 +0,0 @@
-# frozen_string_literal: true
-
-module Gitlab
-  module Ci
-    module External
-      module File
-        class Local < Base
-          attr_reader :location, :project, :sha
-
-          def initialize(location, opts = {})
-            super
-
-            @project = opts.fetch(:project)
-            @sha = opts.fetch(:sha)
-          end
-
-          def content
-            @content ||= fetch_local_content
-          end
-
-          def error_message
-            "Local file '#{location}' is not valid."
-          end
-
-          private
-
-          def fetch_local_content
-            project.repository.blob_data_at(sha, location)
-          end
-        end
-      end
-    end
-  end
-end
diff --git a/lib/gitlab/ci/external/file/remote.rb b/lib/gitlab/ci/external/file/remote.rb
deleted file mode 100644
index 59bb3e8999e..00000000000
--- a/lib/gitlab/ci/external/file/remote.rb
+++ /dev/null
@@ -1,30 +0,0 @@
-# frozen_string_literal: true
-
-module Gitlab
-  module Ci
-    module External
-      module File
-        class Remote < Base
-          include Gitlab::Utils::StrongMemoize
-          attr_reader :location
-
-          def content
-            return @content if defined?(@content)
-
-            @content = strong_memoize(:content) do
-              begin
-                Gitlab::HTTP.get(location)
-              rescue Gitlab::HTTP::Error, Timeout::Error, SocketError, Gitlab::HTTP::BlockedUrlError
-                nil
-              end
-            end
-          end
-
-          def error_message
-            "Remote file '#{location}' is not valid."
-          end
-        end
-      end
-    end
-  end
-end
diff --git a/lib/gitlab/ci/external/mapper.rb b/lib/gitlab/ci/external/mapper.rb
deleted file mode 100644
index 58bd6a19acf..00000000000
--- a/lib/gitlab/ci/external/mapper.rb
+++ /dev/null
@@ -1,32 +0,0 @@
-# frozen_string_literal: true
-
-module Gitlab
-  module Ci
-    module External
-      class Mapper
-        def initialize(values, project, sha)
-          @locations = Array(values.fetch(:include, []))
-          @project = project
-          @sha = sha
-        end
-
-        def process
-          locations.map { |location| build_external_file(location) }
-        end
-
-        private
-
-        attr_reader :locations, :project, :sha
-
-        def build_external_file(location)
-          if ::Gitlab::UrlSanitizer.valid?(location)
-            Gitlab::Ci::External::File::Remote.new(location)
-          else
-            options = { project: project, sha: sha }
-            Gitlab::Ci::External::File::Local.new(location, options)
-          end
-        end
-      end
-    end
-  end
-end
diff --git a/lib/gitlab/ci/external/processor.rb b/lib/gitlab/ci/external/processor.rb
deleted file mode 100644
index 76cf3ce89f9..00000000000
--- a/lib/gitlab/ci/external/processor.rb
+++ /dev/null
@@ -1,52 +0,0 @@
-# frozen_string_literal: true
-
-module Gitlab
-  module Ci
-    module External
-      class Processor
-        FileError = Class.new(StandardError)
-
-        def initialize(values, project, sha)
-          @values = values
-          @external_files = Gitlab::Ci::External::Mapper.new(values, project, sha).process
-          @content = {}
-        end
-
-        def perform
-          return values if external_files.empty?
-
-          external_files.each do |external_file|
-            validate_external_file(external_file)
-            @content.deep_merge!(content_of(external_file))
-          end
-
-          append_inline_content
-          remove_include_keyword
-        end
-
-        private
-
-        attr_reader :values, :external_files, :content
-
-        def validate_external_file(external_file)
-          unless external_file.valid?
-            raise FileError, external_file.error_message
-          end
-        end
-
-        def content_of(external_file)
-          Gitlab::Ci::Config::Loader.new(external_file.content).load!
-        end
-
-        def append_inline_content
-          @content.deep_merge!(@values)
-        end
-
-        def remove_include_keyword
-          content.delete(:include)
-          content
-        end
-      end
-    end
-  end
-end
diff --git a/spec/lib/gitlab/ci/config/external/file/base_spec.rb b/spec/lib/gitlab/ci/config/external/file/base_spec.rb
new file mode 100644
index 00000000000..2e92d5204d6
--- /dev/null
+++ b/spec/lib/gitlab/ci/config/external/file/base_spec.rb
@@ -0,0 +1,52 @@
+# frozen_string_literal: true
+
+require 'fast_spec_helper'
+
+describe Gitlab::Ci::Config::External::File::Base do
+  subject { described_class.new(location) }
+
+  before do
+    allow_any_instance_of(described_class)
+      .to receive(:content).and_return('key: value')
+  end
+
+  describe '#valid?' do
+    context 'when location is not a YAML file' do
+      let(:location) { 'some/file.txt' }
+
+      it { is_expected.not_to be_valid }
+    end
+
+    context 'when location has not a valid naming scheme' do
+      let(:location) { 'some/file/.yml' }
+
+      it { is_expected.not_to be_valid }
+    end
+
+    context 'when location is a valid .yml extension' do
+      let(:location) { 'some/file/config.yml' }
+
+      it { is_expected.to be_valid }
+    end
+
+    context 'when location is a valid .yaml extension' do
+      let(:location) { 'some/file/config.yaml' }
+
+      it { is_expected.to be_valid }
+    end
+
+    context 'when there are YAML syntax errors' do
+      let(:location) { 'some/file/config.yml' }
+
+      before do
+        allow_any_instance_of(described_class)
+          .to receive(:content).and_return('invalid_syntax')
+      end
+
+      it 'is not a valid file' do
+        expect(subject).not_to be_valid
+        expect(subject.error_message).to match /does not have valid YAML syntax/
+      end
+    end
+  end
+end
diff --git a/spec/lib/gitlab/ci/external/file/local_spec.rb b/spec/lib/gitlab/ci/config/external/file/local_spec.rb
index 73bb4ccf468..2708d8d5b6b 100644
--- a/spec/lib/gitlab/ci/external/file/local_spec.rb
+++ b/spec/lib/gitlab/ci/config/external/file/local_spec.rb
@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-describe Gitlab::Ci::External::File::Local do
+describe Gitlab::Ci::Config::External::File::Local do
   let(:project) { create(:project, :repository) }
   let(:local_file) { described_class.new(location, { project: project, sha: '12345' }) }
 
@@ -72,7 +72,7 @@ describe Gitlab::Ci::External::File::Local do
     let(:location) { '/lib/gitlab/ci/templates/non-existent-file.yml' }
 
     it 'should return an error message' do
-      expect(local_file.error_message).to eq("Local file '#{location}' is not valid.")
+      expect(local_file.error_message).to eq("Local file `#{location}` does not exist!")
     end
   end
 end
diff --git a/spec/lib/gitlab/ci/external/file/remote_spec.rb b/spec/lib/gitlab/ci/config/external/file/remote_spec.rb
index b1819c8960b..7c1a1c38736 100644
--- a/spec/lib/gitlab/ci/external/file/remote_spec.rb
+++ b/spec/lib/gitlab/ci/config/external/file/remote_spec.rb
@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-describe Gitlab::Ci::External::File::Remote do
+describe Gitlab::Ci::Config::External::File::Remote do
   let(:remote_file) { described_class.new(location) }
   let(:location) { 'https://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml' }
   let(:remote_file_content) do
@@ -105,10 +105,53 @@ describe Gitlab::Ci::External::File::Remote do
   end
 
   describe "#error_message" do
-    let(:location) { 'not-valid://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml' }
+    subject { remote_file.error_message }
 
-    it 'should return an error message' do
-      expect(remote_file.error_message).to eq("Remote file '#{location}' is not valid.")
+    context 'when remote file location is not valid' do
+      let(:location) { 'not-valid://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml' }
+
+      it 'returns an error message describing invalid address' do
+        expect(subject).to match /does not have a valid address!/
+      end
+    end
+
+    context 'when timeout error has been raised' do
+      before do
+        WebMock.stub_request(:get, location).to_timeout
+      end
+
+      it 'should returns error message about a timeout' do
+        expect(subject).to match /could not be fetched because of a timeout error!/
+      end
+    end
+
+    context 'when HTTP error has been raised' do
+      before do
+        WebMock.stub_request(:get, location).to_raise(Gitlab::HTTP::Error)
+      end
+
+      it 'should returns error message about a HTTP error' do
+        expect(subject).to match /could not be fetched because of HTTP error!/
+      end
+    end
+
+    context 'when response has 404 status' do
+      before do
+        WebMock.stub_request(:get, location).to_return(body: remote_file_content, status: 404)
+      end
+
+      it 'should returns error message about a timeout' do
+        expect(subject).to match /could not be fetched because of HTTP code `404` error!/
+      end
+    end
+
+    context 'when the URL is blocked' do
+      let(:location) { 'http://127.0.0.1/some/path/to/config.yaml' }
+
+      it 'should include details about blocked URL' do
+        expect(subject).to eq "Remote file could not be fetched because URL '#{location}' " \
+                              'is blocked: Requests to localhost are not allowed!'
+      end
     end
   end
 end
diff --git a/spec/lib/gitlab/ci/external/mapper_spec.rb b/spec/lib/gitlab/ci/config/external/mapper_spec.rb
index d925d6af73d..5b236fe99f1 100644
--- a/spec/lib/gitlab/ci/external/mapper_spec.rb
+++ b/spec/lib/gitlab/ci/config/external/mapper_spec.rb
@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-describe Gitlab::Ci::External::Mapper do
+describe Gitlab::Ci::Config::External::Mapper do
   let(:project) { create(:project, :repository) }
   let(:file_content) do
     <<~HEREDOC
@@ -27,7 +27,8 @@ describe Gitlab::Ci::External::Mapper do
         end
 
         it 'returns File instances' do
-          expect(subject.first).to be_an_instance_of(Gitlab::Ci::External::File::Local)
+          expect(subject.first)
+            .to be_an_instance_of(Gitlab::Ci::Config::External::File::Local)
         end
       end
 
@@ -49,7 +50,8 @@ describe Gitlab::Ci::External::Mapper do
         end
 
         it 'returns File instances' do
-          expect(subject.first).to be_an_instance_of(Gitlab::Ci::External::File::Remote)
+          expect(subject.first)
+            .to be_an_instance_of(Gitlab::Ci::Config::External::File::Remote)
         end
       end
     end
diff --git a/spec/lib/gitlab/ci/external/processor_spec.rb b/spec/lib/gitlab/ci/config/external/processor_spec.rb
index 3c7394f53d2..1a05f716247 100644
--- a/spec/lib/gitlab/ci/external/processor_spec.rb
+++ b/spec/lib/gitlab/ci/config/external/processor_spec.rb
@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-describe Gitlab::Ci::External::Processor do
+describe Gitlab::Ci::Config::External::Processor do
   let(:project) { create(:project, :repository) }
   let(:processor) { described_class.new(values, project, '12345') }
 
@@ -20,8 +20,8 @@ describe Gitlab::Ci::External::Processor do
 
       it 'should raise an error' do
         expect { processor.perform }.to raise_error(
-          described_class::FileError,
-          "Local file '/lib/gitlab/ci/templates/non-existent-file.yml' is not valid."
+          described_class::IncludeError,
+          "Local file `/lib/gitlab/ci/templates/non-existent-file.yml` does not exist!"
         )
       end
     end
@@ -36,8 +36,8 @@ describe Gitlab::Ci::External::Processor do
 
       it 'should raise an error' do
         expect { processor.perform }.to raise_error(
-          described_class::FileError,
-          "Remote file '#{remote_file}' is not valid."
+          described_class::IncludeError,
+          "Remote file `#{remote_file}` could not be fetched because of a socket error!"
         )
       end
     end
@@ -92,7 +92,8 @@ describe Gitlab::Ci::External::Processor do
       end
 
       before do
-        allow_any_instance_of(Gitlab::Ci::External::File::Local).to receive(:fetch_local_content).and_return(local_file_content)
+        allow_any_instance_of(Gitlab::Ci::Config::External::File::Local)
+          .to receive(:fetch_local_content).and_return(local_file_content)
       end
 
       it 'should append the file to the values' do
@@ -131,7 +132,10 @@ describe Gitlab::Ci::External::Processor do
 
       before do
         local_file_content = File.read(Rails.root.join('spec/fixtures/gitlab/ci/external_files/.gitlab-ci-template-1.yml'))
-        allow_any_instance_of(Gitlab::Ci::External::File::Local).to receive(:fetch_local_content).and_return(local_file_content)
+
+        allow_any_instance_of(Gitlab::Ci::Config::External::File::Local)
+          .to receive(:fetch_local_content).and_return(local_file_content)
+
         WebMock.stub_request(:get, remote_file).to_return(body: remote_file_content)
       end
 
@@ -150,11 +154,15 @@ describe Gitlab::Ci::External::Processor do
       let(:local_file_content) { 'invalid content file ////' }
 
       before do
-        allow_any_instance_of(Gitlab::Ci::External::File::Local).to receive(:fetch_local_content).and_return(local_file_content)
+        allow_any_instance_of(Gitlab::Ci::Config::External::File::Local)
+          .to receive(:fetch_local_content).and_return(local_file_content)
       end
 
       it 'should raise an error' do
-        expect { processor.perform }.to raise_error(Gitlab::Ci::Config::Loader::FormatError)
+        expect { processor.perform }.to raise_error(
+          described_class::IncludeError,
+          "Included file `/lib/gitlab/ci/templates/template.yml` does not have valid YAML syntax!"
+        )
       end
     end
 
diff --git a/spec/lib/gitlab/ci/config_spec.rb b/spec/lib/gitlab/ci/config_spec.rb
index b43aca8a354..975e11e8cc1 100644
--- a/spec/lib/gitlab/ci/config_spec.rb
+++ b/spec/lib/gitlab/ci/config_spec.rb
@@ -1,6 +1,4 @@
-require 'fast_spec_helper'
-
-require_dependency 'active_model'
+require 'spec_helper'
 
 describe Gitlab::Ci::Config do
   let(:config) do
@@ -202,8 +200,8 @@ describe Gitlab::Ci::Config do
 
       it 'raises error YamlProcessor validationError' do
         expect { config }.to raise_error(
-          ::Gitlab::Ci::YamlProcessor::ValidationError,
-          "Local file 'invalid' is not valid."
+          described_class::ConfigError,
+          "Included file `invalid` does not have YAML extension!"
         )
       end
     end