From c80a1141e306596202f694b101bfb1aab1864de9 Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Fri, 4 Jun 2021 12:10:17 +0000 Subject: [PATCH] Add latest changes from gitlab-org/gitlab@master --- .../components/file_nav/branch_switcher.vue | 118 +++++++++++------- .../components/redesigned_app.vue | 48 +++++-- .../components/section_layout.vue | 23 ++++ .../security_configuration/index.js | 3 +- app/services/ci/register_job_service.rb | 18 ++- ...0602122213_add_upcoming_reconciliations.rb | 23 ++++ db/schema_migrations/20210602122213 | 1 + db/structure.sql | 28 +++++ lib/gitlab/checks/matching_merge_request.rb | 64 +++++++++- lib/gitlab/redis/cache.rb | 8 +- lib/gitlab/redis/queues.rb | 8 +- lib/gitlab/redis/shared_state.rb | 8 +- lib/gitlab/redis/wrapper.rb | 6 - .../file-nav/branch_switcher_spec.js | 47 +++++-- .../components/redesigned_app_spec.js | 60 ++++++--- .../components/section_layout_spec.js | 49 ++++++++ .../checks/matching_merge_request_spec.rb | 92 ++++++++++++++ spec/lib/gitlab/redis/cache_spec.rb | 9 +- spec/lib/gitlab/redis/queues_spec.rb | 9 +- spec/lib/gitlab/redis/shared_state_spec.rb | 9 +- spec/lib/gitlab/redis/wrapper_spec.rb | 6 - spec/services/ci/register_job_service_spec.rb | 28 +++++ spec/support/redis/redis_shared_examples.rb | 11 +- workhorse/config.toml.example | 1 + workhorse/config_test.go | 9 ++ workhorse/internal/config/config.go | 3 +- workhorse/internal/redis/keywatcher.go | 18 +++ workhorse/internal/redis/keywatcher_test.go | 55 ++++++++ workhorse/main.go | 22 +++- 29 files changed, 667 insertions(+), 117 deletions(-) create mode 100644 app/assets/javascripts/security_configuration/components/section_layout.vue create mode 100644 db/migrate/20210602122213_add_upcoming_reconciliations.rb create mode 100644 db/schema_migrations/20210602122213 create mode 100644 spec/frontend/security_configuration/components/section_layout_spec.js diff --git a/app/assets/javascripts/pipeline_editor/components/file_nav/branch_switcher.vue b/app/assets/javascripts/pipeline_editor/components/file_nav/branch_switcher.vue index abc981493c7..137a2fc4b32 100644 --- a/app/assets/javascripts/pipeline_editor/components/file_nav/branch_switcher.vue +++ b/app/assets/javascripts/pipeline_editor/components/file_nav/branch_switcher.vue @@ -7,6 +7,8 @@ import { GlLoadingIcon, GlSearchBoxByType, } from '@gitlab/ui'; +import { produce } from 'immer'; +import { fetchPolicies } from '~/lib/graphql'; import { historyPushState } from '~/lib/utils/common_utils'; import { setUrlParams } from '~/lib/utils/url_utility'; import { s__ } from '~/locale'; @@ -43,12 +45,12 @@ export default { }, data() { return { - branches: [], - page: { - limit: this.paginationLimit, - offset: 0, - searchTerm: '', - }, + availableBranches: [], + filteredBranches: [], + isSearchingBranches: false, + pageLimit: this.paginationLimit, + pageCounter: 0, + searchTerm: '', }; }, apollo: { @@ -56,28 +58,20 @@ export default { query: getAvailableBranches, variables() { return { - limit: this.page.limit, - offset: this.page.offset, + limit: this.paginationLimit, + offset: 0, projectFullPath: this.projectFullPath, - searchPattern: this.searchPattern, + searchPattern: '*', }; }, update(data) { return data.project?.repository?.branchNames || []; }, - result({ data }) { - const newBranches = data.project?.repository?.branchNames || []; - - // check that we're not re-concatenating existing fetch results - if (!this.branches.includes(newBranches[0])) { - this.branches = this.branches.concat(newBranches); - } + result() { + this.pageCounter += 1; }, error() { - this.$emit('showError', { - type: DEFAULT_FAILURE, - reasons: [this.$options.i18n.fetchError], - }); + this.showFetchError(); }, }, currentBranch: { @@ -85,36 +79,57 @@ export default { }, }, computed: { + branches() { + return this.searchTerm.length > 0 ? this.filteredBranches : this.availableBranches; + }, isBranchesLoading() { - return this.$apollo.queries.availableBranches.loading; + return this.$apollo.queries.availableBranches.loading || this.isSearchingBranches; }, showBranchSwitcher() { - return this.branches.length > 0 || this.page.searchTerm.length > 0; - }, - searchPattern() { - if (this.page.searchTerm === '') { - return '*'; - } - - return `*${this.page.searchTerm}*`; + return this.branches.length > 0 || this.searchTerm.length > 0; }, }, methods: { + availableBranchesQueryVars() { + if (this.searchTerm.length > 0) { + return { + limit: this.totalBranches, + offset: 0, + projectFullPath: this.projectFullPath, + searchPattern: `*${this.searchTerm}*`, + }; + } + + return { + limit: this.paginationLimit, + offset: this.pageCounter * this.paginationLimit, + projectFullPath: this.projectFullPath, + searchPattern: '*', + }; + }, // if there is no searchPattern, paginate by {paginationLimit} branches fetchNextBranches() { if ( this.isBranchesLoading || - this.page.searchTerm.length > 0 || + this.searchTerm.length > 0 || this.branches.length === this.totalBranches ) { return; } - this.page = { - ...this.page, - limit: this.paginationLimit, - offset: this.page.offset + this.paginationLimit, - }; + this.$apollo.queries.availableBranches + .fetchMore({ + variables: this.availableBranchesQueryVars(), + updateQuery(previousResult, { fetchMoreResult }) { + const previousBranches = previousResult.project.repository.branchNames; + const newBranches = fetchMoreResult.project.repository.branchNames; + + return produce(fetchMoreResult, (draftData) => { + draftData.project.repository.branchNames = previousBranches.concat(newBranches); + }); + }, + }) + .catch(this.showFetchError); }, async selectBranch(newBranch) { if (newBranch === this.currentBranch) { @@ -131,13 +146,32 @@ export default { this.$emit('refetchContent'); }, - setSearchTerm(newSearchTerm) { - this.branches = []; - this.page = { - limit: newSearchTerm.trim() === '' ? this.paginationLimit : this.totalBranches, - offset: 0, - searchTerm: newSearchTerm.trim(), - }; + async setSearchTerm(newSearchTerm) { + this.pageCounter = 0; + this.searchTerm = newSearchTerm.trim(); + + if (this.searchTerm === '') { + this.pageLimit = this.paginationLimit; + return; + } + + this.isSearchingBranches = true; + const fetchResults = await this.$apollo + .query({ + query: getAvailableBranches, + fetchPolicy: fetchPolicies.NETWORK_ONLY, + variables: this.availableBranchesQueryVars(), + }) + .catch(this.showFetchError); + + this.isSearchingBranches = false; + this.filteredBranches = fetchResults?.data?.project?.repository?.branchNames || []; + }, + showFetchError() { + this.$emit('showError', { + type: DEFAULT_FAILURE, + reasons: [this.$options.i18n.fetchError], + }); }, }, }; diff --git a/app/assets/javascripts/security_configuration/components/redesigned_app.vue b/app/assets/javascripts/security_configuration/components/redesigned_app.vue index 9f3f7f2f703..1d51fe14901 100644 --- a/app/assets/javascripts/security_configuration/components/redesigned_app.vue +++ b/app/assets/javascripts/security_configuration/components/redesigned_app.vue @@ -2,6 +2,7 @@ import { GlTab, GlTabs, GlSprintf, GlLink } from '@gitlab/ui'; import { __, s__ } from '~/locale'; import FeatureCard from './feature_card.vue'; +import SectionLayout from './section_layout.vue'; export const i18n = { compliance: s__('SecurityConfiguration|Compliance'), @@ -23,12 +24,18 @@ export default { GlTabs, GlSprintf, FeatureCard, + SectionLayout, }, props: { augmentedSecurityFeatures: { type: Array, required: true, }, + augmentedComplianceFeatures: { + type: Array, + required: true, + }, + latestPipelinePath: { type: String, required: false, @@ -46,12 +53,11 @@ export default { -
-
-

{{ $options.i18n.securityTesting }}

+ +

-
-
+ + + + + + + + + + diff --git a/app/assets/javascripts/security_configuration/components/section_layout.vue b/app/assets/javascripts/security_configuration/components/section_layout.vue new file mode 100644 index 00000000000..1e1f83a6d99 --- /dev/null +++ b/app/assets/javascripts/security_configuration/components/section_layout.vue @@ -0,0 +1,23 @@ + + + diff --git a/app/assets/javascripts/security_configuration/index.js b/app/assets/javascripts/security_configuration/index.js index a74bc5163d0..6c4a01ea76b 100644 --- a/app/assets/javascripts/security_configuration/index.js +++ b/app/assets/javascripts/security_configuration/index.js @@ -20,7 +20,7 @@ export const initStaticSecurityConfiguration = (el) => { const { projectPath, upgradePath, features, latestPipelinePath } = el.dataset; if (gon.features.securityConfigurationRedesign) { - const { augmentedSecurityFeatures } = augmentFeatures( + const { augmentedSecurityFeatures, augmentedComplianceFeatures } = augmentFeatures( securityFeatures, complianceFeatures, features ? JSON.parse(features) : [], @@ -36,6 +36,7 @@ export const initStaticSecurityConfiguration = (el) => { render(createElement) { return createElement(RedesignedSecurityConfigurationApp, { props: { + augmentedComplianceFeatures, augmentedSecurityFeatures, latestPipelinePath, }, diff --git a/app/services/ci/register_job_service.rb b/app/services/ci/register_job_service.rb index 04956c48136..c97ab1ceaa8 100644 --- a/app/services/ci/register_job_service.rb +++ b/app/services/ci/register_job_service.rb @@ -22,11 +22,27 @@ module Ci end def execute(params = {}) + db_all_caught_up = ::Gitlab::Database::LoadBalancing::Sticking.all_caught_up?(:runner, runner.id) + @metrics.increment_queue_operation(:queue_attempt) - @metrics.observe_queue_time(:process, @runner.runner_type) do + result = @metrics.observe_queue_time(:process, @runner.runner_type) do process_queue(params) end + + # Since we execute this query against replica it might lead to false-positive + # We might receive the positive response: "hi, we don't have any more builds for you". + # This might not be true. If our DB replica is not up-to date with when runner event was generated + # we might still have some CI builds to be picked. Instead we should say to runner: + # "Hi, we don't have any more builds now, but not everything is right anyway, so try again". + # Runner will retry, but again, against replica, and again will check if replication lag did catch-up. + if !db_all_caught_up && !result.build + metrics.increment_queue_operation(:queue_replication_lag) + + ::Ci::RegisterJobService::Result.new(nil, false) # rubocop:disable Cop/AvoidReturnFromBlocks + else + result + end end private diff --git a/db/migrate/20210602122213_add_upcoming_reconciliations.rb b/db/migrate/20210602122213_add_upcoming_reconciliations.rb new file mode 100644 index 00000000000..90d0013b357 --- /dev/null +++ b/db/migrate/20210602122213_add_upcoming_reconciliations.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +class AddUpcomingReconciliations < ActiveRecord::Migration[6.1] + include Gitlab::Database::MigrationHelpers + + def up + with_lock_retries do + create_table :upcoming_reconciliations do |t| + t.references :namespace, index: { unique: true }, null: true, foreign_key: { on_delete: :cascade } + t.date :next_reconciliation_date, null: false + t.date :display_alert_from, null: false + + t.timestamps_with_timezone + end + end + end + + def down + with_lock_retries do + drop_table :upcoming_reconciliations + end + end +end diff --git a/db/schema_migrations/20210602122213 b/db/schema_migrations/20210602122213 new file mode 100644 index 00000000000..651f9789b36 --- /dev/null +++ b/db/schema_migrations/20210602122213 @@ -0,0 +1 @@ +66e50071130c2bd64be2f52d5c5f348a91883b2e9a9f4241175d1d2ad2a74434 \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 4db14820096..d0afdcf5cd8 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -18424,6 +18424,24 @@ CREATE SEQUENCE u2f_registrations_id_seq ALTER SEQUENCE u2f_registrations_id_seq OWNED BY u2f_registrations.id; +CREATE TABLE upcoming_reconciliations ( + id bigint NOT NULL, + namespace_id bigint, + next_reconciliation_date date NOT NULL, + display_alert_from date NOT NULL, + created_at timestamp with time zone NOT NULL, + updated_at timestamp with time zone NOT NULL +); + +CREATE SEQUENCE upcoming_reconciliations_id_seq + START WITH 1 + INCREMENT BY 1 + NO MINVALUE + NO MAXVALUE + CACHE 1; + +ALTER SEQUENCE upcoming_reconciliations_id_seq OWNED BY upcoming_reconciliations.id; + CREATE TABLE uploads ( id integer NOT NULL, size bigint NOT NULL, @@ -20291,6 +20309,8 @@ ALTER TABLE ONLY trending_projects ALTER COLUMN id SET DEFAULT nextval('trending ALTER TABLE ONLY u2f_registrations ALTER COLUMN id SET DEFAULT nextval('u2f_registrations_id_seq'::regclass); +ALTER TABLE ONLY upcoming_reconciliations ALTER COLUMN id SET DEFAULT nextval('upcoming_reconciliations_id_seq'::regclass); + ALTER TABLE ONLY uploads ALTER COLUMN id SET DEFAULT nextval('uploads_id_seq'::regclass); ALTER TABLE ONLY user_agent_details ALTER COLUMN id SET DEFAULT nextval('user_agent_details_id_seq'::regclass); @@ -21939,6 +21959,9 @@ ALTER TABLE ONLY trending_projects ALTER TABLE ONLY u2f_registrations ADD CONSTRAINT u2f_registrations_pkey PRIMARY KEY (id); +ALTER TABLE ONLY upcoming_reconciliations + ADD CONSTRAINT upcoming_reconciliations_pkey PRIMARY KEY (id); + ALTER TABLE ONLY uploads ADD CONSTRAINT uploads_pkey PRIMARY KEY (id); @@ -24687,6 +24710,8 @@ CREATE INDEX index_unit_test_failures_failed_at ON ci_unit_test_failures USING b CREATE UNIQUE INDEX index_unit_test_failures_unique_columns ON ci_unit_test_failures USING btree (unit_test_id, failed_at DESC, build_id); +CREATE UNIQUE INDEX index_upcoming_reconciliations_on_namespace_id ON upcoming_reconciliations USING btree (namespace_id); + CREATE INDEX index_uploads_on_checksum ON uploads USING btree (checksum); CREATE INDEX index_uploads_on_model_id_and_model_type ON uploads USING btree (model_id, model_type); @@ -26538,6 +26563,9 @@ ALTER TABLE ONLY user_custom_attributes ALTER TABLE ONLY ci_pending_builds ADD CONSTRAINT fk_rails_480669c3b3 FOREIGN KEY (project_id) REFERENCES projects(id) ON DELETE CASCADE; +ALTER TABLE ONLY upcoming_reconciliations + ADD CONSTRAINT fk_rails_497b4938ac FOREIGN KEY (namespace_id) REFERENCES namespaces(id) ON DELETE CASCADE; + ALTER TABLE ONLY ci_pipeline_artifacts ADD CONSTRAINT fk_rails_4a70390ca6 FOREIGN KEY (project_id) REFERENCES projects(id) ON DELETE CASCADE; diff --git a/lib/gitlab/checks/matching_merge_request.rb b/lib/gitlab/checks/matching_merge_request.rb index 2635ad04770..3c4625577d4 100644 --- a/lib/gitlab/checks/matching_merge_request.rb +++ b/lib/gitlab/checks/matching_merge_request.rb @@ -3,22 +3,78 @@ module Gitlab module Checks class MatchingMergeRequest + TOTAL_METRIC = :gitlab_merge_request_match_total + STALE_METRIC = :gitlab_merge_request_match_stale_secondary + def initialize(newrev, branch_name, project) @newrev = newrev @branch_name = branch_name @project = project end - # rubocop: disable CodeReuse/ActiveRecord def match? + if ::Gitlab::Database::LoadBalancing.enable? + # When a user merges a merge request, the following sequence happens: + # + # 1. Sidekiq: MergeService runs and updates the merge request in a locked state. + # 2. Gitaly: The UserMergeBranch RPC runs. + # 3. Gitaly (gitaly-ruby): This RPC calls the pre-receive hook. + # 4. Rails: This hook makes an API request to /api/v4/internal/allowed. + # 5. Rails: This API check does a SQL query for locked merge + # requests with a matching SHA. + # + # Since steps 1 and 5 will happen on different database + # sessions, replication lag could erroneously cause step 5 to + # report no matching merge requests. To avoid this, we check + # the write location to ensure the replica can make this query. + track_session_metrics do + if ::Feature.enabled?(:load_balancing_atomic_replica, @project, default_enabled: :yaml) + ::Gitlab::Database::LoadBalancing::Sticking.select_valid_host(:project, @project.id) + else + ::Gitlab::Database::LoadBalancing::Sticking.unstick_or_continue_sticking(:project, @project.id) + end + end + end + + # rubocop: disable CodeReuse/ActiveRecord @project.merge_requests .with_state(:locked) .where(in_progress_merge_commit_sha: @newrev, target_branch: @branch_name) .exists? + # rubocop: enable CodeReuse/ActiveRecord + end + + private + + def track_session_metrics + before = ::Gitlab::Database::LoadBalancing::Session.current.use_primary? + + yield + + after = ::Gitlab::Database::LoadBalancing::Session.current.use_primary? + + increment_attempt_count + + if !before && after + increment_stale_secondary_count + end + end + + def increment_attempt_count + total_counter.increment + end + + def increment_stale_secondary_count + stale_counter.increment + end + + def total_counter + @total_counter ||= ::Gitlab::Metrics.counter(TOTAL_METRIC, 'Total number of merge request match attempts') + end + + def stale_counter + @stale_counter ||= ::Gitlab::Metrics.counter(STALE_METRIC, 'Total number of merge request match attempts with lagging secondary') end - # rubocop: enable CodeReuse/ActiveRecord end end end - -Gitlab::Checks::MatchingMergeRequest.prepend_mod_with('Gitlab::Checks::MatchingMergeRequest') diff --git a/lib/gitlab/redis/cache.rb b/lib/gitlab/redis/cache.rb index cf4dbe48643..e923cee0292 100644 --- a/lib/gitlab/redis/cache.rb +++ b/lib/gitlab/redis/cache.rb @@ -5,10 +5,10 @@ module Gitlab class Cache < ::Gitlab::Redis::Wrapper CACHE_NAMESPACE = 'cache:gitlab' - class << self - def default_url - 'redis://localhost:6380' - end + private + + def raw_config_hash + super || { url: 'redis://localhost:6380' } end end end diff --git a/lib/gitlab/redis/queues.rb b/lib/gitlab/redis/queues.rb index a0777510cd5..61cb79f8357 100644 --- a/lib/gitlab/redis/queues.rb +++ b/lib/gitlab/redis/queues.rb @@ -9,10 +9,10 @@ module Gitlab SIDEKIQ_NAMESPACE = 'resque:gitlab' MAILROOM_NAMESPACE = 'mail_room:gitlab' - class << self - def default_url - 'redis://localhost:6381' - end + private + + def raw_config_hash + super || { url: 'redis://localhost:6381' } end end end diff --git a/lib/gitlab/redis/shared_state.rb b/lib/gitlab/redis/shared_state.rb index 8bd831741f3..db51d7d9258 100644 --- a/lib/gitlab/redis/shared_state.rb +++ b/lib/gitlab/redis/shared_state.rb @@ -8,10 +8,10 @@ module Gitlab USER_SESSIONS_LOOKUP_NAMESPACE = 'session:lookup:user:gitlab' IP_SESSIONS_LOOKUP_NAMESPACE = 'session:lookup:ip:gitlab2' - class << self - def default_url - 'redis://localhost:6382' - end + private + + def raw_config_hash + super || { url: 'redis://localhost:6382' } end end end diff --git a/lib/gitlab/redis/wrapper.rb b/lib/gitlab/redis/wrapper.rb index ea0802ffbdc..32447d39c02 100644 --- a/lib/gitlab/redis/wrapper.rb +++ b/lib/gitlab/redis/wrapper.rb @@ -51,10 +51,6 @@ module Gitlab end end - def default_url - raise NotImplementedError - end - def config_file_path(filename) path = File.join(rails_root, 'config', filename) return path if File.file?(path) @@ -137,8 +133,6 @@ module Gitlab if config_data config_data.is_a?(String) ? { url: config_data } : config_data.deep_symbolize_keys - else - { url: self.class.default_url } end end diff --git a/spec/frontend/pipeline_editor/components/file-nav/branch_switcher_spec.js b/spec/frontend/pipeline_editor/components/file-nav/branch_switcher_spec.js index d6763a7de41..435e82d6077 100644 --- a/spec/frontend/pipeline_editor/components/file-nav/branch_switcher_spec.js +++ b/spec/frontend/pipeline_editor/components/file-nav/branch_switcher_spec.js @@ -58,7 +58,7 @@ describe('Pipeline editor branch switcher', () => { }, data() { return { - branches: ['main'], + availableBranches: ['main'], currentBranch: mockDefaultBranch, }; }, @@ -99,6 +99,16 @@ describe('Pipeline editor branch switcher', () => { wrapper.destroy(); }); + const testErrorHandling = () => { + expect(wrapper.emitted('showError')).toBeDefined(); + expect(wrapper.emitted('showError')[0]).toEqual([ + { + reasons: [wrapper.vm.$options.i18n.fetchError], + type: DEFAULT_FAILURE, + }, + ]); + }; + describe('when querying for the first time', () => { beforeEach(() => { createComponentWithApollo(); @@ -152,13 +162,7 @@ describe('Pipeline editor branch switcher', () => { }); it('shows an error message', () => { - expect(wrapper.emitted('showError')).toBeDefined(); - expect(wrapper.emitted('showError')[0]).toEqual([ - { - reasons: [wrapper.vm.$options.i18n.fetchError], - type: DEFAULT_FAILURE, - }, - ]); + testErrorHandling(); }); }); @@ -215,11 +219,26 @@ describe('Pipeline editor branch switcher', () => { mockAvailableBranchQuery.mockResolvedValue(mockProjectBranches); createComponentWithApollo(mount); await waitForPromises(); + }); - mockAvailableBranchQuery.mockResolvedValue(mockSearchBranches); + afterEach(() => { + mockAvailableBranchQuery.mockClear(); + }); + + it('shows error message on fetch error', async () => { + mockAvailableBranchQuery.mockResolvedValue(new Error()); + + findSearchBox().vm.$emit('input', 'te'); + await waitForPromises(); + + testErrorHandling(); }); describe('with a search term', () => { + beforeEach(async () => { + mockAvailableBranchQuery.mockResolvedValue(mockSearchBranches); + }); + it('calls query with correct variables', async () => { findSearchBox().vm.$emit('input', 'te'); await waitForPromises(); @@ -253,6 +272,7 @@ describe('Pipeline editor branch switcher', () => { describe('without a search term', () => { beforeEach(async () => { + mockAvailableBranchQuery.mockResolvedValue(mockSearchBranches); findSearchBox().vm.$emit('input', 'te'); await waitForPromises(); @@ -326,6 +346,15 @@ describe('Pipeline editor branch switcher', () => { searchPattern: '*', }); }); + + it('shows error message on fetch error', async () => { + mockAvailableBranchQuery.mockResolvedValue(new Error()); + + findInfiniteScroll().vm.$emit('bottomReached'); + await waitForPromises(); + + testErrorHandling(); + }); }); describe('when search term exists', () => { diff --git a/spec/frontend/security_configuration/components/redesigned_app_spec.js b/spec/frontend/security_configuration/components/redesigned_app_spec.js index 1b11f372d8d..a1da7e8584c 100644 --- a/spec/frontend/security_configuration/components/redesigned_app_spec.js +++ b/spec/frontend/security_configuration/components/redesigned_app_spec.js @@ -1,4 +1,4 @@ -import { GlTab, GlTabs } from '@gitlab/ui'; +import { GlTab } from '@gitlab/ui'; import { mount } from '@vue/test-utils'; import { extendedWrapper } from 'helpers/vue_test_utils_helper'; import { @@ -7,14 +7,20 @@ import { SAST_DESCRIPTION, SAST_HELP_PATH, SAST_CONFIG_HELP_PATH, + LICENSE_COMPLIANCE_NAME, + LICENSE_COMPLIANCE_DESCRIPTION, + LICENSE_COMPLIANCE_HELP_PATH, } from '~/security_configuration/components/constants'; import FeatureCard from '~/security_configuration/components/feature_card.vue'; import RedesignedSecurityConfigurationApp, { i18n, } from '~/security_configuration/components/redesigned_app.vue'; -import { REPORT_TYPE_SAST } from '~/vue_shared/security_reports/constants'; +import { + REPORT_TYPE_LICENSE_COMPLIANCE, + REPORT_TYPE_SAST, +} from '~/vue_shared/security_reports/constants'; -describe('NewApp component', () => { +describe('redesigned App component', () => { let wrapper; const createComponent = (propsData) => { @@ -26,9 +32,8 @@ describe('NewApp component', () => { }; const findMainHeading = () => wrapper.find('h1'); - const findSubHeading = () => wrapper.find('h2'); const findTab = () => wrapper.findComponent(GlTab); - const findTabs = () => wrapper.findAllComponents(GlTabs); + const findTabs = () => wrapper.findAllComponents(GlTab); const findByTestId = (id) => wrapper.findByTestId(id); const findFeatureCards = () => wrapper.findAllComponents(FeatureCard); @@ -44,6 +49,16 @@ describe('NewApp component', () => { }, ]; + const complianceFeaturesMock = [ + { + name: LICENSE_COMPLIANCE_NAME, + description: LICENSE_COMPLIANCE_DESCRIPTION, + helpPath: LICENSE_COMPLIANCE_HELP_PATH, + type: REPORT_TYPE_LICENSE_COMPLIANCE, + configurationHelpPath: LICENSE_COMPLIANCE_HELP_PATH, + }, + ]; + afterEach(() => { wrapper.destroy(); }); @@ -52,6 +67,7 @@ describe('NewApp component', () => { beforeEach(() => { createComponent({ augmentedSecurityFeatures: securityFeaturesMock, + augmentedComplianceFeatures: complianceFeaturesMock, }); }); @@ -66,23 +82,22 @@ describe('NewApp component', () => { }); it('renders right amount of tabs with correct title ', () => { - expect(findTabs().length).toEqual(1); + expect(findTabs()).toHaveLength(2); }); it('renders security-testing tab', () => { - expect(findByTestId('security-testing-tab')).toExist(); + expect(findByTestId('security-testing-tab').exists()).toBe(true); }); - it('renders sub-heading with correct text', () => { - const subHeading = findSubHeading(); - expect(subHeading).toExist(); - expect(subHeading.text()).toContain(i18n.securityTesting); + it('renders compliance-testing tab', () => { + expect(findByTestId('compliance-testing-tab').exists()).toBe(true); }); it('renders right amount of feature cards for given props with correct props', () => { const cards = findFeatureCards(); - expect(cards.length).toEqual(1); + expect(cards).toHaveLength(2); expect(cards.at(0).props()).toEqual({ feature: securityFeaturesMock[0] }); + expect(cards.at(1).props()).toEqual({ feature: complianceFeaturesMock[0] }); }); it('should not show latest pipeline link when latestPipelinePath is not defined', () => { @@ -94,16 +109,29 @@ describe('NewApp component', () => { beforeEach(() => { createComponent({ augmentedSecurityFeatures: securityFeaturesMock, + augmentedComplianceFeatures: complianceFeaturesMock, latestPipelinePath: 'test/path', }); }); - it('should show latest pipeline info with correct link when latestPipelinePath is defined', () => { - expect(findByTestId('latest-pipeline-info').exists()).toBe(true); - expect(findByTestId('latest-pipeline-info').text()).toMatchInterpolatedText( + it('should show latest pipeline info on the security tab with correct link when latestPipelinePath is defined', () => { + const latestPipelineInfoSecurity = findByTestId('latest-pipeline-info-security'); + + expect(latestPipelineInfoSecurity.exists()).toBe(true); + expect(latestPipelineInfoSecurity.text()).toMatchInterpolatedText( i18n.securityTestingDescription, ); - expect(findByTestId('latest-pipeline-info').find('a').attributes('href')).toBe('test/path'); + expect(latestPipelineInfoSecurity.find('a').attributes('href')).toBe('test/path'); + }); + + it('should show latest pipeline info on the compliance tab with correct link when latestPipelinePath is defined', () => { + const latestPipelineInfoCompliance = findByTestId('latest-pipeline-info-compliance'); + + expect(latestPipelineInfoCompliance.exists()).toBe(true); + expect(latestPipelineInfoCompliance.text()).toMatchInterpolatedText( + i18n.securityTestingDescription, + ); + expect(latestPipelineInfoCompliance.find('a').attributes('href')).toBe('test/path'); }); }); }); diff --git a/spec/frontend/security_configuration/components/section_layout_spec.js b/spec/frontend/security_configuration/components/section_layout_spec.js new file mode 100644 index 00000000000..75da380bbb8 --- /dev/null +++ b/spec/frontend/security_configuration/components/section_layout_spec.js @@ -0,0 +1,49 @@ +import { mount } from '@vue/test-utils'; +import { extendedWrapper } from 'helpers/vue_test_utils_helper'; +import SectionLayout from '~/security_configuration/components/section_layout.vue'; + +describe('Section Layout component', () => { + let wrapper; + + const createComponent = (propsData) => { + wrapper = extendedWrapper( + mount(SectionLayout, { + propsData, + scopedSlots: { + description: 'foo', + features: 'bar', + }, + }), + ); + }; + + const findHeading = () => wrapper.find('h2'); + + afterEach(() => { + wrapper.destroy(); + }); + + describe('basic structure', () => { + beforeEach(() => { + createComponent({ heading: 'testheading' }); + }); + + const slots = { + description: 'foo', + features: 'bar', + }; + + it('should render heading when passed in as props', () => { + expect(findHeading().exists()).toBe(true); + expect(findHeading().text()).toBe('testheading'); + }); + + Object.keys(slots).forEach((slot) => { + it('renders the slots', () => { + const slotContent = slots[slot]; + createComponent({ heading: '' }); + expect(wrapper.text()).toContain(slotContent); + }); + }); + }); +}); diff --git a/spec/lib/gitlab/checks/matching_merge_request_spec.rb b/spec/lib/gitlab/checks/matching_merge_request_spec.rb index ca7ee784ee3..74087d13e2c 100644 --- a/spec/lib/gitlab/checks/matching_merge_request_spec.rb +++ b/spec/lib/gitlab/checks/matching_merge_request_spec.rb @@ -18,6 +18,9 @@ RSpec.describe Gitlab::Checks::MatchingMergeRequest do subject { described_class.new(newrev, target_branch, project) } + let(:total_counter) { subject.send(:total_counter) } + let(:stale_counter) { subject.send(:stale_counter) } + it 'matches a merge request' do expect(subject.match?).to be true end @@ -27,5 +30,94 @@ RSpec.describe Gitlab::Checks::MatchingMergeRequest do expect(matcher.match?).to be false end + + context 'with load balancing disabled', :request_store, :redis do + before do + expect(::Gitlab::Database::LoadBalancing).to receive(:enable?).at_least(:once).and_return(false) + expect(::Gitlab::Database::LoadBalancing::Sticking).not_to receive(:unstick_or_continue_sticking) + expect(::Gitlab::Database::LoadBalancing::Sticking).not_to receive(:select_valid_replicas) + end + + it 'does not attempt to stick to primary' do + expect(subject.match?).to be true + end + + it 'increments no counters' do + expect { subject.match? } + .to change { total_counter.get }.by(0) + .and change { stale_counter.get }.by(0) + end + end + + context 'with load balancing enabled', :request_store, :redis do + let(:session) { ::Gitlab::Database::LoadBalancing::Session.current } + let(:all_caught_up) { true } + + before do + expect(::Gitlab::Database::LoadBalancing).to receive(:enable?).at_least(:once).and_return(true) + allow(::Gitlab::Database::LoadBalancing::Sticking).to receive(:all_caught_up?).and_return(all_caught_up) + end + + shared_examples 'secondary that has caught up to a primary' do + it 'continues to use the secondary' do + expect(session.use_primary?).to be false + expect(subject.match?).to be true + end + + it 'only increments total counter' do + expect { subject.match? } + .to change { total_counter.get }.by(1) + .and change { stale_counter.get }.by(0) + end + end + + shared_examples 'secondary that is lagging primary' do + it 'sticks to the primary' do + expect(subject.match?).to be true + expect(session.use_primary?).to be true + end + + it 'increments both total and stale counters' do + expect { subject.match? } + .to change { total_counter.get }.by(1) + .and change { stale_counter.get }.by(1) + end + end + + context 'with load_balancing_atomic_replica feature flag enabled' do + before do + stub_feature_flags(load_balancing_atomic_replica: true) + + expect(::Gitlab::Database::LoadBalancing::Sticking).to receive(:select_valid_host).with(:project, project.id).and_call_original + allow(::Gitlab::Database::LoadBalancing::Sticking).to receive(:select_caught_up_replicas).with(:project, project.id).and_return(all_caught_up) + end + + it_behaves_like 'secondary that has caught up to a primary' + + context 'on secondary behind primary' do + let(:all_caught_up) { false } + + it_behaves_like 'secondary that is lagging primary' + end + end + + context 'with load_balancing_atomic_replica feature flag disabled' do + before do + stub_feature_flags(load_balancing_atomic_replica: false) + + expect(::Gitlab::Database::LoadBalancing::Sticking).not_to receive(:select_valid_host) + expect(::Gitlab::Database::LoadBalancing::Sticking).to receive(:unstick_or_continue_sticking).and_call_original + allow(::Gitlab::Database::LoadBalancing::Sticking).to receive(:all_caught_up?).and_return(all_caught_up) + end + + it_behaves_like 'secondary that has caught up to a primary' + + context 'on secondary behind primary' do + let(:all_caught_up) { false } + + it_behaves_like 'secondary that is lagging primary' + end + end + end end end diff --git a/spec/lib/gitlab/redis/cache_spec.rb b/spec/lib/gitlab/redis/cache_spec.rb index bc33bbe115a..31141ac1139 100644 --- a/spec/lib/gitlab/redis/cache_spec.rb +++ b/spec/lib/gitlab/redis/cache_spec.rb @@ -5,7 +5,14 @@ require 'spec_helper' RSpec.describe Gitlab::Redis::Cache do let(:instance_specific_config_file) { "config/redis.cache.yml" } let(:environment_config_file_name) { "GITLAB_REDIS_CACHE_CONFIG_FILE" } - let(:class_redis_url) { 'redis://localhost:6380' } include_examples "redis_shared_examples" + + describe '#raw_config_hash' do + it 'has a legacy default URL' do + expect(subject).to receive(:fetch_config) { false } + + expect(subject.send(:raw_config_hash)).to eq(url: 'redis://localhost:6380' ) + end + end end diff --git a/spec/lib/gitlab/redis/queues_spec.rb b/spec/lib/gitlab/redis/queues_spec.rb index 85fca9320cf..2e396cde3bf 100644 --- a/spec/lib/gitlab/redis/queues_spec.rb +++ b/spec/lib/gitlab/redis/queues_spec.rb @@ -5,7 +5,14 @@ require 'spec_helper' RSpec.describe Gitlab::Redis::Queues do let(:instance_specific_config_file) { "config/redis.queues.yml" } let(:environment_config_file_name) { "GITLAB_REDIS_QUEUES_CONFIG_FILE" } - let(:class_redis_url) { 'redis://localhost:6381' } include_examples "redis_shared_examples" + + describe '#raw_config_hash' do + it 'has a legacy default URL' do + expect(subject).to receive(:fetch_config) { false } + + expect(subject.send(:raw_config_hash)).to eq(url: 'redis://localhost:6381' ) + end + end end diff --git a/spec/lib/gitlab/redis/shared_state_spec.rb b/spec/lib/gitlab/redis/shared_state_spec.rb index 2543c4d9678..d240abfbf5b 100644 --- a/spec/lib/gitlab/redis/shared_state_spec.rb +++ b/spec/lib/gitlab/redis/shared_state_spec.rb @@ -5,7 +5,14 @@ require 'spec_helper' RSpec.describe Gitlab::Redis::SharedState do let(:instance_specific_config_file) { "config/redis.shared_state.yml" } let(:environment_config_file_name) { "GITLAB_REDIS_SHARED_STATE_CONFIG_FILE" } - let(:class_redis_url) { 'redis://localhost:6382' } include_examples "redis_shared_examples" + + describe '#raw_config_hash' do + it 'has a legacy default URL' do + expect(subject).to receive(:fetch_config) { false } + + expect(subject.send(:raw_config_hash)).to eq(url: 'redis://localhost:6382' ) + end + end end diff --git a/spec/lib/gitlab/redis/wrapper_spec.rb b/spec/lib/gitlab/redis/wrapper_spec.rb index 39156d06849..dd1f0d8b414 100644 --- a/spec/lib/gitlab/redis/wrapper_spec.rb +++ b/spec/lib/gitlab/redis/wrapper_spec.rb @@ -8,10 +8,4 @@ RSpec.describe Gitlab::Redis::Wrapper do expect { described_class.instrumentation_class }.to raise_error(NameError) end end - - describe '.default_url' do - it 'is not implemented' do - expect { described_class.default_url }.to raise_error(NotImplementedError) - end - end end diff --git a/spec/services/ci/register_job_service_spec.rb b/spec/services/ci/register_job_service_spec.rb index 067aed37468..99b176a692c 100644 --- a/spec/services/ci/register_job_service_spec.rb +++ b/spec/services/ci/register_job_service_spec.rb @@ -14,6 +14,34 @@ module Ci let!(:pending_job) { create(:ci_build, pipeline: pipeline) } describe '#execute' do + context 'checks database loadbalancing stickiness' do + subject { described_class.new(shared_runner).execute } + + before do + project.update!(shared_runners_enabled: false) + end + + it 'result is valid if replica did caught-up' do + allow(Gitlab::Database::LoadBalancing).to receive(:enable?) + .and_return(true) + + expect(Gitlab::Database::LoadBalancing::Sticking).to receive(:all_caught_up?) + .with(:runner, shared_runner.id) { true } + + expect(subject).to be_valid + end + + it 'result is invalid if replica did not caught-up' do + allow(Gitlab::Database::LoadBalancing).to receive(:enable?) + .and_return(true) + + expect(Gitlab::Database::LoadBalancing::Sticking).to receive(:all_caught_up?) + .with(:runner, shared_runner.id) { false } + + expect(subject).not_to be_valid + end + end + shared_examples 'handles runner assignment' do context 'runner follow tag list' do it "picks build with the same tag" do diff --git a/spec/support/redis/redis_shared_examples.rb b/spec/support/redis/redis_shared_examples.rb index 494a895e929..6e91b53ee5a 100644 --- a/spec/support/redis/redis_shared_examples.rb +++ b/spec/support/redis/redis_shared_examples.rb @@ -92,6 +92,7 @@ RSpec.shared_examples "redis_shared_examples" do subject { described_class.new(rails_env).params } let(:rails_env) { 'development' } + let(:config_file_name) { config_old_format_socket } it 'withstands mutation' do params1 = described_class.params @@ -153,6 +154,8 @@ RSpec.shared_examples "redis_shared_examples" do end describe '.url' do + let(:config_file_name) { config_old_format_socket } + it 'withstands mutation' do url1 = described_class.url url2 = described_class.url @@ -201,6 +204,8 @@ RSpec.shared_examples "redis_shared_examples" do end describe '.with' do + let(:config_file_name) { config_old_format_socket } + before do clear_pool end @@ -288,12 +293,6 @@ RSpec.shared_examples "redis_shared_examples" do end describe '#raw_config_hash' do - it 'returns default redis url when no config file is present' do - expect(subject).to receive(:fetch_config) { false } - - expect(subject.send(:raw_config_hash)).to eq(url: class_redis_url ) - end - it 'returns old-style single url config in a hash' do expect(subject).to receive(:fetch_config) { test_redis_url } expect(subject.send(:raw_config_hash)).to eq(url: test_redis_url) diff --git a/workhorse/config.toml.example b/workhorse/config.toml.example index 78828d3875a..cb29508a90b 100644 --- a/workhorse/config.toml.example +++ b/workhorse/config.toml.example @@ -1,4 +1,5 @@ # alt_document_root = '/home/git/public/assets' +# shutdown_timeout = "60s" [redis] URL = "unix:/home/git/gitlab/redis/redis.socket" diff --git a/workhorse/config_test.go b/workhorse/config_test.go index f9d12bd5e2b..b1b04cb9a65 100644 --- a/workhorse/config_test.go +++ b/workhorse/config_test.go @@ -16,12 +16,20 @@ import ( "gitlab.com/gitlab-org/gitlab-workhorse/internal/upstream" ) +func TestDefaultConfig(t *testing.T) { + _, cfg, err := buildConfig("test", []string{"-config", "/dev/null"}) + require.NoError(t, err, "build config") + + require.Equal(t, 0*time.Second, cfg.ShutdownTimeout.Duration) +} + func TestConfigFile(t *testing.T) { f, err := ioutil.TempFile("", "workhorse-config-test") require.NoError(t, err) defer os.Remove(f.Name()) data := ` +shutdown_timeout = "60s" [redis] password = "redis password" [object_storage] @@ -43,6 +51,7 @@ max_scaler_procs = 123 require.Equal(t, "redis password", cfg.Redis.Password) require.Equal(t, "test provider", cfg.ObjectStorageCredentials.Provider) require.Equal(t, uint32(123), cfg.ImageResizerConfig.MaxScalerProcs, "image resizer max_scaler_procs") + require.Equal(t, 60*time.Second, cfg.ShutdownTimeout.Duration) } func TestConfigErrorHelp(t *testing.T) { diff --git a/workhorse/internal/config/config.go b/workhorse/internal/config/config.go index d019a7710bf..9f214385f81 100644 --- a/workhorse/internal/config/config.go +++ b/workhorse/internal/config/config.go @@ -28,7 +28,7 @@ type TomlDuration struct { time.Duration } -func (d *TomlDuration) UnmarshalTest(text []byte) error { +func (d *TomlDuration) UnmarshalText(text []byte) error { temp, err := time.ParseDuration(string(text)) d.Duration = temp return err @@ -103,6 +103,7 @@ type Config struct { PropagateCorrelationID bool `toml:"-"` ImageResizerConfig ImageResizerConfig `toml:"image_resizer"` AltDocumentRoot string `toml:"alt_document_root"` + ShutdownTimeout TomlDuration `toml:"shutdown_timeout"` } var DefaultImageResizerConfig = ImageResizerConfig{ diff --git a/workhorse/internal/redis/keywatcher.go b/workhorse/internal/redis/keywatcher.go index 8f3e61b5e9f..10d80d13d22 100644 --- a/workhorse/internal/redis/keywatcher.go +++ b/workhorse/internal/redis/keywatcher.go @@ -17,6 +17,7 @@ import ( var ( keyWatcher = make(map[string][]chan string) keyWatcherMutex sync.Mutex + shutdown = make(chan struct{}) redisReconnectTimeout = backoff.Backoff{ //These are the defaults Min: 100 * time.Millisecond, @@ -112,6 +113,20 @@ func Process() { } } +func Shutdown() { + log.Info("keywatcher: shutting down") + + keyWatcherMutex.Lock() + defer keyWatcherMutex.Unlock() + + select { + case <-shutdown: + // already closed + default: + close(shutdown) + } +} + func notifyChanWatchers(key, value string) { keyWatcherMutex.Lock() defer keyWatcherMutex.Unlock() @@ -182,6 +197,9 @@ func WatchKey(key, value string, timeout time.Duration) (WatchKeyStatus, error) } select { + case <-shutdown: + log.WithFields(log.Fields{"key": key}).Info("stopping watch due to shutdown") + return WatchKeyStatusNoChange, nil case currentValue := <-kw.Chan: if currentValue == "" { return WatchKeyStatusNoChange, fmt.Errorf("keywatcher: redis GET failed") diff --git a/workhorse/internal/redis/keywatcher_test.go b/workhorse/internal/redis/keywatcher_test.go index f1ee77e2194..99892bc64b8 100644 --- a/workhorse/internal/redis/keywatcher_test.go +++ b/workhorse/internal/redis/keywatcher_test.go @@ -160,3 +160,58 @@ func TestWatchKeyMassivelyParallel(t *testing.T) { processMessages(runTimes, "somethingelse") wg.Wait() } + +func TestShutdown(t *testing.T) { + conn, td := setupMockPool() + defer td() + defer func() { shutdown = make(chan struct{}) }() + + conn.Command("GET", runnerKey).Expect("something") + + wg := &sync.WaitGroup{} + wg.Add(2) + + go func() { + val, err := WatchKey(runnerKey, "something", 10*time.Second) + + require.NoError(t, err, "Expected no error") + require.Equal(t, WatchKeyStatusNoChange, val, "Expected value not to change") + wg.Done() + }() + + go func() { + for countWatchers(runnerKey) == 0 { + time.Sleep(time.Millisecond) + } + + require.Equal(t, 1, countWatchers(runnerKey)) + + Shutdown() + wg.Done() + }() + + wg.Wait() + + for countWatchers(runnerKey) == 1 { + time.Sleep(time.Millisecond) + } + + require.Equal(t, 0, countWatchers(runnerKey)) + + // Adding a key after the shutdown should result in an immediate response + var val WatchKeyStatus + var err error + done := make(chan struct{}) + go func() { + val, err = WatchKey(runnerKey, "something", 10*time.Second) + close(done) + }() + + select { + case <-done: + require.NoError(t, err, "Expected no error") + require.Equal(t, WatchKeyStatusNoChange, val, "Expected value not to change") + case <-time.After(100 * time.Millisecond): + t.Fatal("timeout waiting for WatchKey") + } +} diff --git a/workhorse/main.go b/workhorse/main.go index cafe07057d3..de282b2c670 100644 --- a/workhorse/main.go +++ b/workhorse/main.go @@ -1,6 +1,7 @@ package main import ( + "context" "flag" "fmt" "io/ioutil" @@ -8,6 +9,7 @@ import ( "net/http" _ "net/http/pprof" "os" + "os/signal" "syscall" "time" @@ -144,6 +146,7 @@ func buildConfig(arg0 string, args []string) (*bootConfig, *config.Config, error cfg.ObjectStorageCredentials = cfgFromFile.ObjectStorageCredentials cfg.ImageResizerConfig = cfgFromFile.ImageResizerConfig cfg.AltDocumentRoot = cfgFromFile.AltDocumentRoot + cfg.ShutdownTimeout = cfgFromFile.ShutdownTimeout return boot, cfg, nil } @@ -225,7 +228,22 @@ func run(boot bootConfig, cfg config.Config) error { up := wrapRaven(upstream.NewUpstream(cfg, accessLogger)) - go func() { finalErrors <- http.Serve(listener, up) }() + done := make(chan os.Signal, 1) + signal.Notify(done, syscall.SIGINT, syscall.SIGTERM) - return <-finalErrors + server := http.Server{Handler: up} + go func() { finalErrors <- server.Serve(listener) }() + + select { + case err := <-finalErrors: + return err + case sig := <-done: + log.WithFields(log.Fields{"shutdown_timeout_s": cfg.ShutdownTimeout.Duration.Seconds(), "signal": sig.String()}).Infof("shutdown initiated") + + ctx, cancel := context.WithTimeout(context.Background(), cfg.ShutdownTimeout.Duration) // lint:allow context.Background + defer cancel() + + redis.Shutdown() + return server.Shutdown(ctx) + } }