Commit b887d106 authored by Douwe Maan's avatar Douwe Maan

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
parents ec748a8f a91899be
---
title: Improve validation errors for external CI/CD configuration
merge_request: 22394
author:
type: added
...@@ -13,10 +13,10 @@ module Gitlab ...@@ -13,10 +13,10 @@ module Gitlab
@global = Entry::Global.new(@config) @global = Entry::Global.new(@config)
@global.compose! @global.compose!
rescue Loader::FormatError, Extendable::ExtensionError => e rescue Loader::FormatError,
Extendable::ExtensionError,
External::Processor::IncludeError => e
raise Config::ConfigError, e.message raise Config::ConfigError, e.message
rescue ::Gitlab::Ci::External::Processor::FileError => e
raise ::Gitlab::Ci::YamlProcessor::ValidationError, e.message
end end
def valid? def valid?
...@@ -81,7 +81,7 @@ module Gitlab ...@@ -81,7 +81,7 @@ module Gitlab
def process_external_files(config, project, opts) def process_external_files(config, project, opts)
sha = opts.fetch(:sha) { project.repository.root_ref_sha } 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 end
end end
......
# 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
# 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
# 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
# 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
# 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
# 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
# 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
# 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
# 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
# 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
# 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
...@@ -2,7 +2,7 @@ ...@@ -2,7 +2,7 @@
require 'spec_helper' require 'spec_helper'
describe Gitlab::Ci::External::File::Local do describe Gitlab::Ci::Config::External::File::Local do
let(:project) { create(:project, :repository) } let(:project) { create(:project, :repository) }
let(:local_file) { described_class.new(location, { project: project, sha: '12345' }) } let(:local_file) { described_class.new(location, { project: project, sha: '12345' }) }
...@@ -72,7 +72,7 @@ describe Gitlab::Ci::External::File::Local do ...@@ -72,7 +72,7 @@ describe Gitlab::Ci::External::File::Local do
let(:location) { '/lib/gitlab/ci/templates/non-existent-file.yml' } let(:location) { '/lib/gitlab/ci/templates/non-existent-file.yml' }
it 'should return an error message' do 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 end
end end
...@@ -2,7 +2,7 @@ ...@@ -2,7 +2,7 @@
require 'spec_helper' 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(:remote_file) { described_class.new(location) }
let(:location) { 'https://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml' } let(:location) { 'https://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml' }
let(:remote_file_content) do let(:remote_file_content) do
...@@ -105,10 +105,53 @@ describe Gitlab::Ci::External::File::Remote do ...@@ -105,10 +105,53 @@ describe Gitlab::Ci::External::File::Remote do
end end
describe "#error_message" do describe "#error_message" do
subject { remote_file.error_message }
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' } let(:location) { 'not-valid://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml' }
it 'should return an error message' do it 'returns an error message describing invalid address' do
expect(remote_file.error_message).to eq("Remote file '#{location}' is not valid.") 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 end
end end
...@@ -2,7 +2,7 @@ ...@@ -2,7 +2,7 @@
require 'spec_helper' require 'spec_helper'
describe Gitlab::Ci::External::Mapper do describe Gitlab::Ci::Config::External::Mapper do
let(:project) { create(:project, :repository) } let(:project) { create(:project, :repository) }
let(:file_content) do let(:file_content) do
<<~HEREDOC <<~HEREDOC
...@@ -27,7 +27,8 @@ describe Gitlab::Ci::External::Mapper do ...@@ -27,7 +27,8 @@ describe Gitlab::Ci::External::Mapper do
end end
it 'returns File instances' do 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
end end
...@@ -49,7 +50,8 @@ describe Gitlab::Ci::External::Mapper do ...@@ -49,7 +50,8 @@ describe Gitlab::Ci::External::Mapper do
end end
it 'returns File instances' do 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 end
end end
......
...@@ -2,7 +2,7 @@ ...@@ -2,7 +2,7 @@
require 'spec_helper' require 'spec_helper'
describe Gitlab::Ci::External::Processor do describe Gitlab::Ci::Config::External::Processor do
let(:project) { create(:project, :repository) } let(:project) { create(:project, :repository) }
let(:processor) { described_class.new(values, project, '12345') } let(:processor) { described_class.new(values, project, '12345') }
...@@ -20,8 +20,8 @@ describe Gitlab::Ci::External::Processor do ...@@ -20,8 +20,8 @@ describe Gitlab::Ci::External::Processor do
it 'should raise an error' do it 'should raise an error' do
expect { processor.perform }.to raise_error( expect { processor.perform }.to raise_error(
described_class::FileError, described_class::IncludeError,
"Local file '/lib/gitlab/ci/templates/non-existent-file.yml' is not valid." "Local file `/lib/gitlab/ci/templates/non-existent-file.yml` does not exist!"
) )
end end
end end
...@@ -36,8 +36,8 @@ describe Gitlab::Ci::External::Processor do ...@@ -36,8 +36,8 @@ describe Gitlab::Ci::External::Processor do
it 'should raise an error' do it 'should raise an error' do
expect { processor.perform }.to raise_error( expect { processor.perform }.to raise_error(
described_class::FileError, described_class::IncludeError,
"Remote file '#{remote_file}' is not valid." "Remote file `#{remote_file}` could not be fetched because of a socket error!"
) )
end end
end end
...@@ -92,7 +92,8 @@ describe Gitlab::Ci::External::Processor do ...@@ -92,7 +92,8 @@ describe Gitlab::Ci::External::Processor do
end end
before do 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 end
it 'should append the file to the values' do it 'should append the file to the values' do
...@@ -131,7 +132,10 @@ describe Gitlab::Ci::External::Processor do ...@@ -131,7 +132,10 @@ describe Gitlab::Ci::External::Processor do
before do before do
local_file_content = File.read(Rails.root.join('spec/fixtures/gitlab/ci/external_files/.gitlab-ci-template-1.yml')) 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) WebMock.stub_request(:get, remote_file).to_return(body: remote_file_content)
end end
...@@ -150,11 +154,15 @@ describe Gitlab::Ci::External::Processor do ...@@ -150,11 +154,15 @@ describe Gitlab::Ci::External::Processor do
let(:local_file_content) { 'invalid content file ////' } let(:local_file_content) { 'invalid content file ////' }
before do 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 end
it 'should raise an error' do 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
end end
......
require 'fast_spec_helper' require 'spec_helper'
require_dependency 'active_model'
describe Gitlab::Ci::Config do describe Gitlab::Ci::Config do
let(:config) do let(:config) do
...@@ -202,8 +200,8 @@ describe Gitlab::Ci::Config do ...@@ -202,8 +200,8 @@ describe Gitlab::Ci::Config do
it 'raises error YamlProcessor validationError' do it 'raises error YamlProcessor validationError' do
expect { config }.to raise_error( expect { config }.to raise_error(
::Gitlab::Ci::YamlProcessor::ValidationError, described_class::ConfigError,
"Local file 'invalid' is not valid." "Included file `invalid` does not have YAML extension!"
) )
end end
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment