diff options
Diffstat (limited to 'spec/lib')
17 files changed, 513 insertions, 149 deletions
diff --git a/spec/lib/gitlab/auth/ldap/config_spec.rb b/spec/lib/gitlab/auth/ldap/config_spec.rb index 4e7fbab396f..160fd78b2b9 100644 --- a/spec/lib/gitlab/auth/ldap/config_spec.rb +++ b/spec/lib/gitlab/auth/ldap/config_spec.rb @@ -122,7 +122,8 @@ AtlErSqafbECNDSwS5BX8yDpu5yRBJ4xegO/rNlmb8ICRYkuJapD1xXicFOsmfUK host: 'ldap.example.com', port: 386, hosts: nil, - encryption: nil + encryption: nil, + instrumentation_service: ActiveSupport::Notifications ) end diff --git a/spec/lib/gitlab/ci/build/context/build_spec.rb b/spec/lib/gitlab/ci/build/context/build_spec.rb index 7f862a3b80a..74739a67be0 100644 --- a/spec/lib/gitlab/ci/build/context/build_spec.rb +++ b/spec/lib/gitlab/ci/build/context/build_spec.rb @@ -2,11 +2,11 @@ require 'spec_helper' -RSpec.describe Gitlab::Ci::Build::Context::Build do +RSpec.describe Gitlab::Ci::Build::Context::Build, feature_category: :pipeline_authoring do let(:pipeline) { create(:ci_pipeline) } let(:seed_attributes) { { 'name' => 'some-job' } } - let(:context) { described_class.new(pipeline, seed_attributes) } + subject(:context) { described_class.new(pipeline, seed_attributes) } shared_examples 'variables collection' do it { is_expected.to include('CI_COMMIT_REF_NAME' => 'master') } @@ -22,6 +22,12 @@ RSpec.describe Gitlab::Ci::Build::Context::Build do it { is_expected.to include('CI_BUILD_REF_NAME' => 'master') } it { is_expected.to include('CI_PROJECT_PATH' => pipeline.project.full_path) } end + + context 'when environment:name is provided' do + let(:seed_attributes) { { 'name' => 'some-job', 'environment' => 'test' } } + + it { is_expected.to include('CI_ENVIRONMENT_NAME' => 'test') } + end end describe '#variables' do diff --git a/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb b/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb index e552b9e9c0c..5433f03fb09 100644 --- a/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Gitlab::Ci::Pipeline::Seed::Build do +RSpec.describe Gitlab::Ci::Pipeline::Seed::Build, feature_category: :pipeline_authoring do let_it_be_with_reload(:project) { create(:project, :repository) } let_it_be(:head_sha) { project.repository.head_commit.id } @@ -949,6 +949,40 @@ RSpec.describe Gitlab::Ci::Pipeline::Seed::Build do end end + context 'with a rule using CI_ENVIRONMENT_NAME variable' do + let(:rule_set) do + [{ if: '$CI_ENVIRONMENT_NAME == "test"' }] + end + + context 'when environment:name satisfies the rule' do + let(:attributes) { { name: 'rspec', rules: rule_set, environment: 'test', when: 'on_success' } } + + it { is_expected.to be_included } + + it 'correctly populates when:' do + expect(seed_build.attributes).to include(when: 'on_success') + end + end + + context 'when environment:name does not satisfy rule' do + let(:attributes) { { name: 'rspec', rules: rule_set, environment: 'dev', when: 'on_success' } } + + it { is_expected.not_to be_included } + + it 'correctly populates when:' do + expect(seed_build.attributes).to include(when: 'never') + end + end + + context 'when environment:name is not set' do + it { is_expected.not_to be_included } + + it 'correctly populates when:' do + expect(seed_build.attributes).to include(when: 'never') + end + end + end + context 'with no rules' do let(:rule_set) { [] } diff --git a/spec/lib/gitlab/ci/variables/builder_spec.rb b/spec/lib/gitlab/ci/variables/builder_spec.rb index 052d3024254..6b0faf5c5ca 100644 --- a/spec/lib/gitlab/ci/variables/builder_spec.rb +++ b/spec/lib/gitlab/ci/variables/builder_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Gitlab::Ci::Variables::Builder, :clean_gitlab_redis_cache do +RSpec.describe Gitlab::Ci::Variables::Builder, :clean_gitlab_redis_cache, feature_category: :pipeline_authoring do include Ci::TemplateHelpers let_it_be(:group) { create(:group) } let_it_be(:project) { create(:project, :repository, namespace: group) } @@ -13,7 +13,8 @@ RSpec.describe Gitlab::Ci::Variables::Builder, :clean_gitlab_redis_cache do name: 'rspec:test 1', pipeline: pipeline, user: user, - yaml_variables: [{ key: 'YAML_VARIABLE', value: 'value' }] + yaml_variables: [{ key: 'YAML_VARIABLE', value: 'value' }], + environment: 'test' ) end @@ -32,6 +33,8 @@ RSpec.describe Gitlab::Ci::Variables::Builder, :clean_gitlab_redis_cache do value: job.stage_name }, { key: 'CI_NODE_TOTAL', value: '1' }, + { key: 'CI_ENVIRONMENT_NAME', + value: 'test' }, { key: 'CI_BUILD_NAME', value: 'rspec:test 1' }, { key: 'CI_BUILD_STAGE', diff --git a/spec/lib/gitlab/github_import/bulk_importing_spec.rb b/spec/lib/gitlab/github_import/bulk_importing_spec.rb index e170496ff7b..af31cb6c873 100644 --- a/spec/lib/gitlab/github_import/bulk_importing_spec.rb +++ b/spec/lib/gitlab/github_import/bulk_importing_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Gitlab::GithubImport::BulkImporting do +RSpec.describe Gitlab::GithubImport::BulkImporting, feature_category: :importer do let(:project) { instance_double(Project, id: 1) } let(:importer) { MyImporter.new(project, double) } let(:importer_class) do @@ -12,22 +12,33 @@ RSpec.describe Gitlab::GithubImport::BulkImporting do def object_type :object_type end + + def model + Label + end end end + let(:label) { instance_double('Label', invalid?: false) } + before do stub_const 'MyImporter', importer_class end describe '#build_database_rows' do - it 'returns an Array containing the rows to insert' do + it 'returns an Array containing the rows to insert and validation errors if object invalid' do object = double(:object, title: 'Foo') expect(importer) - .to receive(:build) + .to receive(:build_attributes) .with(object) .and_return({ title: 'Foo' }) + expect(Label) + .to receive(:new) + .with({ title: 'Foo' }) + .and_return(label) + expect(importer) .to receive(:already_imported?) .with(object) @@ -53,14 +64,17 @@ RSpec.describe Gitlab::GithubImport::BulkImporting do enum = [[object, 1]].to_enum - expect(importer.build_database_rows(enum)).to eq([{ title: 'Foo' }]) + rows, errors = importer.build_database_rows(enum) + + expect(rows).to match_array([{ title: 'Foo' }]) + expect(errors).to be_empty end it 'does not import objects that have already been imported' do object = double(:object, title: 'Foo') expect(importer) - .not_to receive(:build) + .not_to receive(:build_attributes) expect(importer) .to receive(:already_imported?) @@ -87,14 +101,16 @@ RSpec.describe Gitlab::GithubImport::BulkImporting do enum = [[object, 1]].to_enum - expect(importer.build_database_rows(enum)).to be_empty + rows, errors = importer.build_database_rows(enum) + + expect(rows).to be_empty + expect(errors).to be_empty end end describe '#bulk_insert' do it 'bulk inserts rows into the database' do rows = [{ title: 'Foo' }] * 10 - model = double(:model, table_name: 'kittens') expect(Gitlab::Import::Logger) .to receive(:info) @@ -119,14 +135,43 @@ RSpec.describe Gitlab::GithubImport::BulkImporting do expect(ApplicationRecord) .to receive(:legacy_bulk_insert) .ordered - .with('kittens', rows.first(5)) + .with('labels', rows.first(5)) expect(ApplicationRecord) .to receive(:legacy_bulk_insert) .ordered - .with('kittens', rows.last(5)) + .with('labels', rows.last(5)) + + importer.bulk_insert(rows, batch_size: 5) + end + end + + describe '#bulk_insert_failures', :timecop do + let(:import_failures) { instance_double('ImportFailure::ActiveRecord_Associations_CollectionProxy') } + let(:label) { Label.new(title: 'invalid,title') } + let(:validation_errors) { ActiveModel::Errors.new(label) } + let(:formatted_errors) do + [{ + source: 'MyImporter', + exception_class: 'ActiveRecord::RecordInvalid', + exception_message: 'Title invalid', + correlation_id_value: 'cid', + retry_count: nil, + created_at: Time.zone.now + }] + end - importer.bulk_insert(model, rows, batch_size: 5) + it 'bulk inserts validation errors into import_failures' do + error = ActiveModel::Errors.new(label) + error.add(:base, 'Title invalid') + + freeze_time do + expect(project).to receive(:import_failures).and_return(import_failures) + expect(import_failures).to receive(:insert_all).with(formatted_errors) + expect(Labkit::Correlation::CorrelationId).to receive(:current_or_new_id).and_return('cid') + + importer.bulk_insert_failures([error]) + end end end end diff --git a/spec/lib/gitlab/github_import/importer/diff_note_importer_spec.rb b/spec/lib/gitlab/github_import/importer/diff_note_importer_spec.rb index 8eeb2332131..1bcb7fec327 100644 --- a/spec/lib/gitlab/github_import/importer/diff_note_importer_spec.rb +++ b/spec/lib/gitlab/github_import/importer/diff_note_importer_spec.rb @@ -218,6 +218,16 @@ RSpec.describe Gitlab::GithubImport::Importer::DiffNoteImporter, :aggregate_fail end end end + + context 'when diff note is invalid' do + it 'fails validation' do + stub_user_finder(user.id, true) + + expect(note_representation).to receive(:line_code).and_return(nil) + + expect { subject.execute }.to raise_error(ActiveRecord::RecordInvalid) + end + end end end diff --git a/spec/lib/gitlab/github_import/importer/label_links_importer_spec.rb b/spec/lib/gitlab/github_import/importer/label_links_importer_spec.rb index e68849755b2..e005d8eda84 100644 --- a/spec/lib/gitlab/github_import/importer/label_links_importer_spec.rb +++ b/spec/lib/gitlab/github_import/importer/label_links_importer_spec.rb @@ -36,26 +36,11 @@ RSpec.describe Gitlab::GithubImport::Importer::LabelLinksImporter do expect(importer) .to receive(:find_target_id) - .and_return(1) + .and_return(4) - freeze_time do - expect(ApplicationRecord) - .to receive(:legacy_bulk_insert) - .with( - LabelLink.table_name, - [ - { - label_id: 2, - target_id: 1, - target_type: Issue, - created_at: Time.zone.now, - updated_at: Time.zone.now - } - ] - ) + expect(LabelLink).to receive(:bulk_insert!) - importer.create_labels - end + importer.create_labels end it 'does not insert label links for non-existing labels' do @@ -64,9 +49,9 @@ RSpec.describe Gitlab::GithubImport::Importer::LabelLinksImporter do .with('bug') .and_return(nil) - expect(ApplicationRecord) - .to receive(:legacy_bulk_insert) - .with(LabelLink.table_name, []) + expect(LabelLink) + .to receive(:bulk_insert!) + .with([]) importer.create_labels end diff --git a/spec/lib/gitlab/github_import/importer/labels_importer_spec.rb b/spec/lib/gitlab/github_import/importer/labels_importer_spec.rb index 81d534c566f..ad9ef4afddd 100644 --- a/spec/lib/gitlab/github_import/importer/labels_importer_spec.rb +++ b/spec/lib/gitlab/github_import/importer/labels_importer_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Gitlab::GithubImport::Importer::LabelsImporter, :clean_gitlab_redis_cache do +RSpec.describe Gitlab::GithubImport::Importer::LabelsImporter, :clean_gitlab_redis_cache, feature_category: :importer do let(:project) { create(:project, import_source: 'foo/bar') } let(:client) { double(:client) } let(:importer) { described_class.new(project, client) } @@ -11,40 +11,58 @@ RSpec.describe Gitlab::GithubImport::Importer::LabelsImporter, :clean_gitlab_red it 'imports the labels in bulk' do label_hash = { title: 'bug', color: '#fffaaa' } - expect(importer) - .to receive(:build_labels) - .and_return([label_hash]) - - expect(importer) - .to receive(:bulk_insert) - .with(Label, [label_hash]) - - expect(importer) - .to receive(:build_labels_cache) + expect(importer).to receive(:build_labels).and_return([[label_hash], []]) + expect(importer).to receive(:bulk_insert).with([label_hash]) + expect(importer).to receive(:build_labels_cache) importer.execute end end describe '#build_labels' do - it 'returns an Array containnig label rows' do + it 'returns an Array containing label rows' do label = { name: 'bug', color: 'ffffff' } expect(importer).to receive(:each_label).and_return([label]) - rows = importer.build_labels + rows, errors = importer.build_labels expect(rows.length).to eq(1) expect(rows[0][:title]).to eq('bug') + expect(errors).to be_blank end - it 'does not create labels that already exist' do + it 'does not build labels that already exist' do create(:label, project: project, title: 'bug') label = { name: 'bug', color: 'ffffff' } expect(importer).to receive(:each_label).and_return([label]) - expect(importer.build_labels).to be_empty + + rows, errors = importer.build_labels + + expect(rows).to be_empty + expect(errors).to be_empty + end + + it 'does not build labels that are invalid' do + label = { id: 1, name: 'bug,bug', color: 'ffffff' } + + expect(importer).to receive(:each_label).and_return([label]) + expect(Gitlab::Import::Logger).to receive(:error) + .with( + import_type: :github, + project_id: project.id, + importer: described_class.name, + message: ['Title is invalid'], + github_identifier: 1 + ) + + rows, errors = importer.build_labels + + expect(rows).to be_empty + expect(errors.length).to eq(1) + expect(errors[0].full_messages).to match_array(['Title is invalid']) end end @@ -58,9 +76,9 @@ RSpec.describe Gitlab::GithubImport::Importer::LabelsImporter, :clean_gitlab_red end end - describe '#build' do + describe '#build_attributes' do let(:label_hash) do - importer.build({ name: 'bug', color: 'ffffff' }) + importer.build_attributes({ name: 'bug', color: 'ffffff' }) end it 'returns the attributes of the label as a Hash' do diff --git a/spec/lib/gitlab/github_import/importer/milestones_importer_spec.rb b/spec/lib/gitlab/github_import/importer/milestones_importer_spec.rb index 04d76bd1f06..8667729d79b 100644 --- a/spec/lib/gitlab/github_import/importer/milestones_importer_spec.rb +++ b/spec/lib/gitlab/github_import/importer/milestones_importer_spec.rb @@ -2,7 +2,8 @@ require 'spec_helper' -RSpec.describe Gitlab::GithubImport::Importer::MilestonesImporter, :clean_gitlab_redis_cache do +RSpec.describe Gitlab::GithubImport::Importer::MilestonesImporter, :clean_gitlab_redis_cache, + feature_category: :importer do let(:project) { create(:project, import_source: 'foo/bar') } let(:client) { double(:client) } let(:importer) { described_class.new(project, client) } @@ -38,41 +39,61 @@ RSpec.describe Gitlab::GithubImport::Importer::MilestonesImporter, :clean_gitlab it 'imports the milestones in bulk' do milestone_hash = { number: 1, title: '1.0' } - expect(importer) - .to receive(:build_milestones) - .and_return([milestone_hash]) - - expect(importer) - .to receive(:bulk_insert) - .with(Milestone, [milestone_hash]) - - expect(importer) - .to receive(:build_milestones_cache) + expect(importer).to receive(:build_milestones).and_return([[milestone_hash], []]) + expect(importer).to receive(:bulk_insert).with([milestone_hash]) + expect(importer).to receive(:build_milestones_cache) importer.execute end end describe '#build_milestones' do - it 'returns an Array containnig milestone rows' do + it 'returns an Array containing milestone rows' do expect(importer) .to receive(:each_milestone) .and_return([milestone]) - rows = importer.build_milestones + rows, errors = importer.build_milestones expect(rows.length).to eq(1) expect(rows[0][:title]).to eq('1.0') + expect(errors).to be_empty end - it 'does not create milestones that already exist' do + it 'does not build milestones that already exist' do create(:milestone, project: project, title: '1.0', iid: 1) expect(importer) .to receive(:each_milestone) .and_return([milestone]) - expect(importer.build_milestones).to be_empty + rows, errors = importer.build_milestones + + expect(rows).to be_empty + expect(errors).to be_empty + end + + it 'does not build milestones that are invalid' do + milestone = { id: 1, title: nil } + + expect(importer) + .to receive(:each_milestone) + .and_return([milestone]) + + expect(Gitlab::Import::Logger).to receive(:error) + .with( + import_type: :github, + project_id: project.id, + importer: described_class.name, + message: ["Title can't be blank"], + github_identifier: 1 + ) + + rows, errors = importer.build_milestones + + expect(rows).to be_empty + expect(errors.length).to eq(1) + expect(errors[0].full_messages).to match_array(["Title can't be blank"]) end end @@ -86,9 +107,9 @@ RSpec.describe Gitlab::GithubImport::Importer::MilestonesImporter, :clean_gitlab end end - describe '#build' do - let(:milestone_hash) { importer.build(milestone) } - let(:milestone_hash2) { importer.build(milestone2) } + describe '#build_attributes' do + let(:milestone_hash) { importer.build_attributes(milestone) } + let(:milestone_hash2) { importer.build_attributes(milestone2) } it 'returns the attributes of the milestone as a Hash' do expect(milestone_hash).to be_an_instance_of(Hash) diff --git a/spec/lib/gitlab/github_import/importer/note_importer_spec.rb b/spec/lib/gitlab/github_import/importer/note_importer_spec.rb index c60ecd85e92..5ac50578b6a 100644 --- a/spec/lib/gitlab/github_import/importer/note_importer_spec.rb +++ b/spec/lib/gitlab/github_import/importer/note_importer_spec.rb @@ -113,6 +113,19 @@ RSpec.describe Gitlab::GithubImport::Importer::NoteImporter do .to eq('There were an invalid char "" <= right here') end end + + context 'when note is invalid' do + it 'fails validation' do + expect(importer.user_finder) + .to receive(:author_id_for) + .with(github_note) + .and_return([user.id, true]) + + expect(github_note).to receive(:discussion_id).and_return('invalid') + + expect { importer.execute }.to raise_error(ActiveRecord::RecordInvalid) + end + end end context 'when the noteable does not exist' do diff --git a/spec/lib/gitlab/github_import/importer/pull_request_importer_spec.rb b/spec/lib/gitlab/github_import/importer/pull_request_importer_spec.rb index 08c4af3035d..dd73b6879e0 100644 --- a/spec/lib/gitlab/github_import/importer/pull_request_importer_spec.rb +++ b/spec/lib/gitlab/github_import/importer/pull_request_importer_spec.rb @@ -202,6 +202,20 @@ RSpec.describe Gitlab::GithubImport::Importer::PullRequestImporter, :clean_gitla importer.create_merge_request end end + + context 'when merge request is invalid' do + before do + allow(pull_request).to receive(:formatted_source_branch).and_return(nil) + allow(importer.user_finder) + .to receive(:author_id_for) + .with(pull_request) + .and_return([project.creator_id, false]) + end + + it 'fails validation' do + expect { importer.create_merge_request }.to raise_error(ActiveRecord::RecordInvalid) + end + end end describe '#set_merge_request_assignees' do diff --git a/spec/lib/gitlab/github_import/importer/releases_importer_spec.rb b/spec/lib/gitlab/github_import/importer/releases_importer_spec.rb index 84d639a09ef..ccbe5b5fc50 100644 --- a/spec/lib/gitlab/github_import/importer/releases_importer_spec.rb +++ b/spec/lib/gitlab/github_import/importer/releases_importer_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Gitlab::GithubImport::Importer::ReleasesImporter do +RSpec.describe Gitlab::GithubImport::Importer::ReleasesImporter, feature_category: :importer do let(:project) { create(:project) } let(:client) { double(:client) } let(:importer) { described_class.new(project, client) } @@ -48,8 +48,8 @@ RSpec.describe Gitlab::GithubImport::Importer::ReleasesImporter do released_at: released_at } - expect(importer).to receive(:build_releases).and_return([release_hash]) - expect(importer).to receive(:bulk_insert).with(Release, [release_hash]) + expect(importer).to receive(:build_releases).and_return([[release_hash], []]) + expect(importer).to receive(:bulk_insert).with([release_hash]) importer.execute end @@ -86,24 +86,29 @@ RSpec.describe Gitlab::GithubImport::Importer::ReleasesImporter do it 'returns an Array containing release rows' do expect(importer).to receive(:each_release).and_return([github_release]) - rows = importer.build_releases + rows, errors = importer.build_releases expect(rows.length).to eq(1) expect(rows[0][:tag]).to eq('1.0') + expect(errors).to be_empty end it 'does not create releases that already exist' do create(:release, project: project, tag: '1.0', description: '1.0') expect(importer).to receive(:each_release).and_return([github_release]) - expect(importer.build_releases).to be_empty + + rows, errors = importer.build_releases + + expect(rows).to be_empty + expect(errors).to be_empty end it 'uses a default release description if none is provided' do github_release[:body] = nil expect(importer).to receive(:each_release).and_return([github_release]) - release = importer.build_releases.first + release, _ = importer.build_releases.first expect(release[:description]).to eq('Release for tag 1.0') end @@ -115,20 +120,36 @@ RSpec.describe Gitlab::GithubImport::Importer::ReleasesImporter do } expect(importer).to receive(:each_release).and_return([null_tag_release]) - expect(importer.build_releases).to be_empty + + rows, errors = importer.build_releases + + expect(rows).to be_empty + expect(errors).to be_empty end it 'does not create duplicate release tags' do expect(importer).to receive(:each_release).and_return([github_release, github_release]) - releases = importer.build_releases + releases, _ = importer.build_releases expect(releases.length).to eq(1) expect(releases[0][:description]).to eq('This is my release') end + + it 'does not create invalid release' do + github_release[:body] = SecureRandom.alphanumeric(Gitlab::Database::MAX_TEXT_SIZE_LIMIT + 1) + + expect(importer).to receive(:each_release).and_return([github_release]) + + releases, errors = importer.build_releases + + expect(releases).to be_empty + expect(errors.length).to eq(1) + expect(errors[0].full_messages).to match_array(['Description is too long (maximum is 1000000 characters)']) + end end - describe '#build' do - let(:release_hash) { importer.build(github_release) } + describe '#build_attributes' do + let(:release_hash) { importer.build_attributes(github_release) } context 'the returned Hash' do before do diff --git a/spec/lib/gitlab/instrumentation/redis_base_spec.rb b/spec/lib/gitlab/instrumentation/redis_base_spec.rb index a84a26a8d92..9cf65a11bce 100644 --- a/spec/lib/gitlab/instrumentation/redis_base_spec.rb +++ b/spec/lib/gitlab/instrumentation/redis_base_spec.rb @@ -153,29 +153,39 @@ RSpec.describe Gitlab::Instrumentation::RedisBase, :request_store do end describe '.redis_cluster_validate!' do - context 'Rails environments' do - where(:env, :should_raise) do - 'production' | false - 'staging' | false - 'development' | true - 'test' | true - end + let(:args) { [[:mget, 'foo', 'bar']] } + + before do + instrumentation_class_a.enable_redis_cluster_validation + end - before do - instrumentation_class_a.enable_redis_cluster_validation + context 'Rails environments' do + where(:env, :allowed, :should_raise) do + 'production' | false | false + 'production' | true | false + 'staging' | false | false + 'staging' | true | false + 'development' | true | false + 'development' | false | true + 'test' | true | false + 'test' | false | true end with_them do it do stub_rails_env(env) - args = [[:mget, 'foo', 'bar']] + validation = -> { instrumentation_class_a.redis_cluster_validate!(args) } + under_test = if allowed + -> { Gitlab::Instrumentation::RedisClusterValidator.allow_cross_slot_commands(&validation) } + else + validation + end if should_raise - expect { instrumentation_class_a.redis_cluster_validate!(args) } - .to raise_error(::Gitlab::Instrumentation::RedisClusterValidator::CrossSlotError) + expect(&under_test).to raise_error(::Gitlab::Instrumentation::RedisClusterValidator::CrossSlotError) else - expect { instrumentation_class_a.redis_cluster_validate!(args) }.not_to raise_error + expect(&under_test).not_to raise_error end end end diff --git a/spec/lib/gitlab/instrumentation/redis_cluster_validator_spec.rb b/spec/lib/gitlab/instrumentation/redis_cluster_validator_spec.rb index c5462bd8545..892b8e69124 100644 --- a/spec/lib/gitlab/instrumentation/redis_cluster_validator_spec.rb +++ b/spec/lib/gitlab/instrumentation/redis_cluster_validator_spec.rb @@ -7,83 +7,102 @@ require 'rspec-parameterized' RSpec.describe Gitlab::Instrumentation::RedisClusterValidator do include RailsHelpers - describe '.validate!' do + describe '.validate' do using RSpec::Parameterized::TableSyntax - where(:command, :arguments, :should_raise) do - :rename | %w(foo bar) | true - :RENAME | %w(foo bar) | true - 'rename' | %w(foo bar) | true - 'RENAME' | %w(foo bar) | true - :rename | %w(iaa ahy) | false # 'iaa' and 'ahy' hash to the same slot - :rename | %w({foo}:1 {foo}:2) | false - :rename | %w(foo foo bar) | false # This is not a valid command but should not raise here - :mget | %w(foo bar) | true - :mget | %w(foo foo bar) | true - :mget | %w(foo foo) | false - :blpop | %w(foo bar 1) | true - :blpop | %w(foo foo 1) | false - :mset | %w(foo a bar a) | true - :mset | %w(foo a foo a) | false - :del | %w(foo bar) | true - :del | [%w(foo bar)] | true # Arguments can be a nested array - :del | %w(foo foo) | false - :hset | %w(foo bar) | false # Not a multi-key command - :mget | [] | false # This is invalid, but not because it's a cross-slot command + where(:command, :arguments, :keys, :is_valid) do + :rename | %w(foo bar) | 2 | false + :RENAME | %w(foo bar) | 2 | false + 'rename' | %w(foo bar) | 2 | false + 'RENAME' | %w(foo bar) | 2 | false + :rename | %w(iaa ahy) | 2 | true # 'iaa' and 'ahy' hash to the same slot + :rename | %w({foo}:1 {foo}:2) | 2 | true + :rename | %w(foo foo bar) | 2 | true # This is not a valid command but should not raise here + :mget | %w(foo bar) | 2 | false + :mget | %w(foo foo bar) | 3 | false + :mget | %w(foo foo) | 2 | true + :blpop | %w(foo bar 1) | 2 | false + :blpop | %w(foo foo 1) | 2 | true + :mset | %w(foo a bar a) | 2 | false + :mset | %w(foo a foo a) | 2 | true + :del | %w(foo bar) | 2 | false + :del | [%w(foo bar)] | 2 | false # Arguments can be a nested array + :del | %w(foo foo) | 2 | true + :hset | %w(foo bar) | 1 | nil # Single key write + :get | %w(foo) | 1 | nil # Single key read + :mget | [] | 0 | true # This is invalid, but not because it's a cross-slot command end with_them do it do args = [[command] + arguments] - - if should_raise - expect { described_class.validate!(args) } - .to raise_error(described_class::CrossSlotError) + if is_valid.nil? + expect(described_class.validate(args)).to eq(nil) else - expect { described_class.validate!(args) }.not_to raise_error + expect(described_class.validate(args)[:valid]).to eq(is_valid) + expect(described_class.validate(args)[:allowed]).to eq(false) + expect(described_class.validate(args)[:command_name]).to eq(command.to_s.upcase) + expect(described_class.validate(args)[:key_count]).to eq(keys) end end end - where(:arguments, :should_raise) do - [[:get, "foo"], [:get, "bar"]] | true - [[:get, "foo"], [:mget, "foo", "bar"]] | true # mix of single-key and multi-key cmds - [[:get, "{foo}:name"], [:get, "{foo}:profile"]] | false - [[:del, "foo"], [:del, "bar"]] | true - [] | false # pipeline or transaction opened and closed without ops + where(:arguments, :should_raise, :output) do + [ + [ + [[:get, "foo"], [:get, "bar"]], + true, + { valid: false, key_count: 2, command_name: 'PIPELINE/MULTI', allowed: false } + ], + [ + [[:get, "foo"], [:mget, "foo", "bar"]], + true, + { valid: false, key_count: 3, command_name: 'PIPELINE/MULTI', allowed: false } + ], + [ + [[:get, "{foo}:name"], [:get, "{foo}:profile"]], + false, + { valid: true, key_count: 2, command_name: 'PIPELINE/MULTI', allowed: false } + ], + [ + [[:del, "foo"], [:del, "bar"]], + true, + { valid: false, key_count: 2, command_name: 'PIPELINE/MULTI', allowed: false } + ], + [ + [], + false, + nil # pipeline or transaction opened and closed without ops + ] + ] end with_them do it do - if should_raise - expect { described_class.validate!(arguments) } - .to raise_error(described_class::CrossSlotError) - else - expect { described_class.validate!(arguments) }.not_to raise_error - end + expect(described_class.validate(arguments)).to eq(output) end end end describe '.allow_cross_slot_commands' do - it 'does not raise for invalid arguments' do - expect do + it 'skips validation for allowed commands' do + expect( described_class.allow_cross_slot_commands do - described_class.validate!([[:mget, 'foo', 'bar']]) + described_class.validate([[:mget, 'foo', 'bar']]) end - end.not_to raise_error + ).to eq({ valid: true, key_count: 2, command_name: 'MGET', allowed: true }) end it 'allows nested invocation' do - expect do + expect( described_class.allow_cross_slot_commands do described_class.allow_cross_slot_commands do - described_class.validate!([[:mget, 'foo', 'bar']]) + described_class.validate([[:mget, 'foo', 'bar']]) end - described_class.validate!([[:mget, 'foo', 'bar']]) + described_class.validate([[:mget, 'foo', 'bar']]) end - end.not_to raise_error + ).to eq({ valid: true, key_count: 2, command_name: 'MGET', allowed: true }) end end end diff --git a/spec/lib/gitlab/instrumentation/redis_interceptor_spec.rb b/spec/lib/gitlab/instrumentation/redis_interceptor_spec.rb index 0bcc85190c2..120430accf1 100644 --- a/spec/lib/gitlab/instrumentation/redis_interceptor_spec.rb +++ b/spec/lib/gitlab/instrumentation/redis_interceptor_spec.rb @@ -64,12 +64,6 @@ RSpec.describe Gitlab::Instrumentation::RedisInterceptor, :clean_gitlab_redis_sh end end - it 'skips count for non-cross-slot requests' do - expect(instrumentation_class).not_to receive(:increment_cross_slot_request_count).and_call_original - - Gitlab::Redis::SharedState.with { |redis| redis.call(:mget, '{foo}bar', '{foo}baz') } - end - it 'counts exceptions' do expect(instrumentation_class).to receive(:instance_count_exception) .with(instance_of(Redis::CommandError)).and_call_original @@ -82,16 +76,42 @@ RSpec.describe Gitlab::Instrumentation::RedisInterceptor, :clean_gitlab_redis_sh end.to raise_exception(Redis::CommandError) end - context 'in production env' do + context 'in production environment' do before do stub_rails_env('production') # to avoid raising CrossSlotError end - it 'counts cross-slot requests' do + it 'counts disallowed cross-slot requests' do expect(instrumentation_class).to receive(:increment_cross_slot_request_count).and_call_original Gitlab::Redis::SharedState.with { |redis| redis.call(:mget, 'foo', 'bar') } end + + it 'does not count allowed cross-slot requests' do + expect(instrumentation_class).not_to receive(:increment_cross_slot_request_count).and_call_original + + Gitlab::Instrumentation::RedisClusterValidator.allow_cross_slot_commands do + Gitlab::Redis::SharedState.with { |redis| redis.call(:mget, 'foo', 'bar') } + end + end + + it 'skips count for non-cross-slot requests' do + expect(instrumentation_class).not_to receive(:increment_cross_slot_request_count).and_call_original + + Gitlab::Redis::SharedState.with { |redis| redis.call(:mget, '{foo}bar', '{foo}baz') } + end + end + + context 'without active RequestStore' do + before do + ::RequestStore.end! + end + + it 'still runs cross-slot validation' do + expect do + Gitlab::Redis::SharedState.with { |redis| redis.mget('foo', 'bar') } + end.to raise_error(instance_of(Gitlab::Instrumentation::RedisClusterValidator::CrossSlotError)) + end end end diff --git a/spec/lib/gitlab/instrumentation_helper_spec.rb b/spec/lib/gitlab/instrumentation_helper_spec.rb index b2d6e1af1e7..b4738ed663c 100644 --- a/spec/lib/gitlab/instrumentation_helper_spec.rb +++ b/spec/lib/gitlab/instrumentation_helper_spec.rb @@ -71,6 +71,26 @@ RSpec.describe Gitlab::InstrumentationHelper do end end + context 'when LDAP requests are made' do + let(:provider) { 'ldapmain' } + let(:adapter) { Gitlab::Auth::Ldap::Adapter.new(provider) } + let(:conn) { instance_double(Net::LDAP::Connection, search: search) } + let(:search) { double(:search, result_code: 200) } # rubocop: disable RSpec/VerifiedDoubles + + it 'adds LDAP data' do + allow_next_instance_of(Net::LDAP) do |net_ldap| + allow(net_ldap).to receive(:use_connection).and_yield(conn) + end + + adapter.users('uid', 'foo') + subject + + # Query count should be 2, as it will call `open` then `search` + expect(payload[:net_ldap_count]).to eq(2) + expect(payload[:net_ldap_duration_s]).to be >= 0 + end + end + context 'when the request matched a Rack::Attack safelist' do it 'logs the safelist name' do Gitlab::Instrumentation::Throttle.safelist = 'foobar' diff --git a/spec/lib/gitlab/metrics/subscribers/ldap_spec.rb b/spec/lib/gitlab/metrics/subscribers/ldap_spec.rb new file mode 100644 index 00000000000..b81000be62a --- /dev/null +++ b/spec/lib/gitlab/metrics/subscribers/ldap_spec.rb @@ -0,0 +1,124 @@ +# frozen_string_literal: true + +require "spec_helper" + +RSpec.describe Gitlab::Metrics::Subscribers::Ldap, :request_store, feature_category: :logging do + let(:transaction) { Gitlab::Metrics::WebTransaction.new({}) } + let(:subscriber) { described_class.new } + + let(:attributes) do + [ + :altServer, :namingContexts, :supportedCapabilities, :supportedControl, + :supportedExtension, :supportedFeatures, :supportedLdapVersion, :supportedSASLMechanisms + ] + end + + let(:event_1) do + instance_double( + ActiveSupport::Notifications::Event, + name: "open.net_ldap", + payload: { + ignore_server_caps: true, + base: "", + scope: 0, + attributes: attributes, + result: nil + }, + time: Time.current, + duration: 0.321 + ) + end + + let(:event_2) do + instance_double( + ActiveSupport::Notifications::Event, + name: "search.net_ldap", + payload: { + ignore_server_caps: true, + base: "", + scope: 0, + attributes: attributes, + result: nil + }, + time: Time.current, + duration: 0.12 + ) + end + + let(:event_3) do + instance_double( + ActiveSupport::Notifications::Event, + name: "search.net_ldap", + payload: { + ignore_server_caps: true, + base: "", + scope: 0, + attributes: attributes, + result: nil + }, + time: Time.current, + duration: 5.3 + ) + end + + around do |example| + freeze_time { example.run } + end + + describe ".payload" do + context "when SafeRequestStore is empty" do + it "returns an empty array" do + expect(described_class.payload).to eql(net_ldap_count: 0, net_ldap_duration_s: 0.0) + end + end + + context "when LDAP recorded some values" do + before do + Gitlab::SafeRequestStore[:net_ldap_count] = 7 + Gitlab::SafeRequestStore[:net_ldap_duration_s] = 1.2 + end + + it "returns the populated payload" do + expect(described_class.payload).to eql(net_ldap_count: 7, net_ldap_duration_s: 1.2) + end + end + end + + describe "#observe_event" do + before do + allow(subscriber).to receive(:current_transaction).and_return(transaction) + end + + it "tracks LDAP request count" do + expect(transaction).to receive(:increment) + .with(:gitlab_net_ldap_total, 1, { name: "open" }) + expect(transaction).to receive(:increment) + .with(:gitlab_net_ldap_total, 1, { name: "search" }) + + subscriber.observe_event(event_1) + subscriber.observe_event(event_2) + end + + it "tracks LDAP request duration" do + expect(transaction).to receive(:observe) + .with(:gitlab_net_ldap_duration_seconds, 0.321, { name: "open" }) + expect(transaction).to receive(:observe) + .with(:gitlab_net_ldap_duration_seconds, 0.12, { name: "search" }) + expect(transaction).to receive(:observe) + .with(:gitlab_net_ldap_duration_seconds, 5.3, { name: "search" }) + + subscriber.observe_event(event_1) + subscriber.observe_event(event_2) + subscriber.observe_event(event_3) + end + + it "stores per-request counters" do + subscriber.observe_event(event_1) + subscriber.observe_event(event_2) + subscriber.observe_event(event_3) + + expect(Gitlab::SafeRequestStore[:net_ldap_count]).to eq(3) + expect(Gitlab::SafeRequestStore[:net_ldap_duration_s]).to eq(5.741) # 0.321 + 0.12 + 5.3 + end + end +end |