diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2021-03-22 12:09:02 +0000 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2021-03-22 12:09:02 +0000 |
commit | cea25900b73fcb7029845eeca5e5a9170349f1dd (patch) | |
tree | 4d7c7c000de534569c65d71cc55e638a6a2dfbde | |
parent | b014ff26b4416de3aa49d83483d8ce5d273c0708 (diff) | |
download | gitlab-ce-cea25900b73fcb7029845eeca5e5a9170349f1dd.tar.gz |
Add latest changes from gitlab-org/gitlab@master
-rw-r--r-- | GITALY_SERVER_VERSION | 2 | ||||
-rw-r--r-- | app/models/concerns/safe_url.rb | 4 | ||||
-rw-r--r-- | app/models/remote_mirror.rb | 2 | ||||
-rw-r--r-- | app/views/users/terms/index.html.haml | 4 | ||||
-rw-r--r-- | changelogs/unreleased/btn-confirm-users.yml | 5 | ||||
-rw-r--r-- | changelogs/unreleased/qmnguyen0711-expand-a-histogram-bucket.yml | 5 | ||||
-rw-r--r-- | doc/administration/pages/index.md | 5 | ||||
-rw-r--r-- | doc/development/licensing.md | 7 | ||||
-rw-r--r-- | lib/gitlab/metrics/subscribers/active_record.rb | 17 | ||||
-rw-r--r-- | spec/lib/gitlab/metrics/subscribers/active_record_spec.rb | 170 | ||||
-rw-r--r-- | spec/models/concerns/safe_url_spec.rb | 12 |
11 files changed, 163 insertions, 70 deletions
diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION index 0cbfc674111..b6707734a9a 100644 --- a/GITALY_SERVER_VERSION +++ b/GITALY_SERVER_VERSION @@ -1 +1 @@ -6314c40a2e0ae9d43eb4c01026d0aef57c10d91c +cc879cfb5db4ed342c4f0ea744dbbfdc9649b35f diff --git a/app/models/concerns/safe_url.rb b/app/models/concerns/safe_url.rb index febca7d241f..7dce05bddba 100644 --- a/app/models/concerns/safe_url.rb +++ b/app/models/concerns/safe_url.rb @@ -3,12 +3,12 @@ module SafeUrl extend ActiveSupport::Concern - def safe_url(usernames_whitelist: []) + def safe_url(allowed_usernames: []) return if url.nil? uri = URI.parse(url) uri.password = '*****' if uri.password - uri.user = '*****' if uri.user && !usernames_whitelist.include?(uri.user) + uri.user = '*****' if uri.user && allowed_usernames.exclude?(uri.user) uri.to_s rescue URI::Error end diff --git a/app/models/remote_mirror.rb b/app/models/remote_mirror.rb index 880970b72a8..1bde2727c44 100644 --- a/app/models/remote_mirror.rb +++ b/app/models/remote_mirror.rb @@ -207,7 +207,7 @@ class RemoteMirror < ApplicationRecord end def safe_url - super(usernames_whitelist: %w[git]) + super(allowed_usernames: %w[git]) end def bare_url diff --git a/app/views/users/terms/index.html.haml b/app/views/users/terms/index.html.haml index da8b73fd4fd..73d0f51f9ac 100644 --- a/app/views/users/terms/index.html.haml +++ b/app/views/users/terms/index.html.haml @@ -6,11 +6,11 @@ .card-footer.footer-block.clearfix - if can?(current_user, :accept_terms, @term) .float-right - = button_to accept_term_path(@term, redirect_params), class: 'gl-button btn btn-success gl-ml-3', data: { qa_selector: 'accept_terms_button' } do + = button_to accept_term_path(@term, redirect_params), class: 'gl-button btn btn-confirm gl-ml-3', data: { qa_selector: 'accept_terms_button' } do = _('Accept terms') - else .float-right - = link_to root_path, class: 'gl-button btn btn-success gl-ml-3' do + = link_to root_path, class: 'gl-button btn btn-confirm gl-ml-3' do = _('Continue') - if can?(current_user, :decline_terms, @term) .float-right diff --git a/changelogs/unreleased/btn-confirm-users.yml b/changelogs/unreleased/btn-confirm-users.yml new file mode 100644 index 00000000000..2ebab7cea7d --- /dev/null +++ b/changelogs/unreleased/btn-confirm-users.yml @@ -0,0 +1,5 @@ +--- +title: Move from btn-success to btn-confirm in users directory +merge_request: 56945 +author: Yogi (@yo) +type: changed diff --git a/changelogs/unreleased/qmnguyen0711-expand-a-histogram-bucket.yml b/changelogs/unreleased/qmnguyen0711-expand-a-histogram-bucket.yml new file mode 100644 index 00000000000..fe906d02551 --- /dev/null +++ b/changelogs/unreleased/qmnguyen0711-expand-a-histogram-bucket.yml @@ -0,0 +1,5 @@ +--- +title: Adjust gitlab_database_transaction_seconds histogram bucket +merge_request: 56952 +author: +type: changed diff --git a/doc/administration/pages/index.md b/doc/administration/pages/index.md index 3728948ef28..8e4e2f34fc5 100644 --- a/doc/administration/pages/index.md +++ b/doc/administration/pages/index.md @@ -600,8 +600,9 @@ the below steps to do a no downtime transfer to a new storage location. ## Configure listener for reverse proxy requests -Follow the steps below to configure the proxy listener of GitLab Pages. [Introduced](https://gitlab.com/gitlab-org/omnibus-gitlab/-/merge_requests/2533) in -Omnibus GitLab 11.1. +> [Introduced](https://gitlab.com/gitlab-org/omnibus-gitlab/-/merge_requests/2533) in Omnibus GitLab 11.1. + +Follow the steps below to configure the proxy listener of GitLab Pages. 1. By default the listener is configured to listen for requests on `localhost:8090`. diff --git a/doc/development/licensing.md b/doc/development/licensing.md index c467fe81755..1ab56e60a0b 100644 --- a/doc/development/licensing.md +++ b/doc/development/licensing.md @@ -18,9 +18,6 @@ Some gems may not include their license information in their `gemspec` file, and ### License Finder commands -NOTE: -License Finder currently uses GitLab misused terms of `whitelist` and `blacklist`. As a result, the commands below reference those terms. We've created an [issue on their project](https://github.com/pivotal/LicenseFinder/issues/745) to propose that they rename their commands. - There are a few basic commands License Finder provides that you need in order to manage license detection. To verify that the checks are passing, and/or to see what dependencies are causing the checks to fail: @@ -32,13 +29,13 @@ bundle exec license_finder To allowlist a new license: ```shell -license_finder whitelist add MIT +license_finder permitted_licenses add MIT ``` To denylist a new license: ```shell -license_finder blacklist add Unlicense +license_finder restricted_licenses add Unlicense ``` To tell License Finder about a dependency's license if it isn't auto-detected: diff --git a/lib/gitlab/metrics/subscribers/active_record.rb b/lib/gitlab/metrics/subscribers/active_record.rb index 5eefef02507..6de180337fc 100644 --- a/lib/gitlab/metrics/subscribers/active_record.rb +++ b/lib/gitlab/metrics/subscribers/active_record.rb @@ -11,13 +11,16 @@ module Gitlab DB_COUNTERS = %i{db_count db_write_count db_cached_count}.freeze SQL_COMMANDS_WITH_COMMENTS_REGEX = /\A(\/\*.*\*\/\s)?((?!(.*[^\w'"](DELETE|UPDATE|INSERT INTO)[^\w'"])))(WITH.*)?(SELECT)((?!(FOR UPDATE|FOR SHARE)).)*$/i.freeze - DURATION_BUCKET = [0.05, 0.1, 0.25].freeze + SQL_DURATION_BUCKET = [0.05, 0.1, 0.25].freeze + TRANSACTION_DURATION_BUCKET = [0.1, 0.25, 1].freeze # This event is published from ActiveRecordBaseTransactionMetrics and # used to record a database transaction duration when calling # ActiveRecord::Base.transaction {} block. def transaction(event) - observe(:gitlab_database_transaction_seconds, event) + observe(:gitlab_database_transaction_seconds, event) do + buckets TRANSACTION_DURATION_BUCKET + end end def sql(event) @@ -33,7 +36,9 @@ module Gitlab increment(:db_cached_count) if cached_query?(payload) increment(:db_write_count) unless select_sql_command?(payload) - observe(:gitlab_sql_duration_seconds, event) + observe(:gitlab_sql_duration_seconds, event) do + buckets SQL_DURATION_BUCKET + end end def self.db_counter_payload @@ -66,10 +71,8 @@ module Gitlab Gitlab::SafeRequestStore[counter] = Gitlab::SafeRequestStore[counter].to_i + 1 end - def observe(histogram, event) - current_transaction&.observe(histogram, event.duration / 1000.0) do - buckets DURATION_BUCKET - end + def observe(histogram, event, &block) + current_transaction&.observe(histogram, event.duration / 1000.0, &block) end def current_transaction diff --git a/spec/lib/gitlab/metrics/subscribers/active_record_spec.rb b/spec/lib/gitlab/metrics/subscribers/active_record_spec.rb index dffd37eeb9d..9939e680fa9 100644 --- a/spec/lib/gitlab/metrics/subscribers/active_record_spec.rb +++ b/spec/lib/gitlab/metrics/subscribers/active_record_spec.rb @@ -8,65 +8,145 @@ RSpec.describe Gitlab::Metrics::Subscribers::ActiveRecord do let(:env) { {} } let(:subscriber) { described_class.new } let(:connection) { double(:connection) } - let(:payload) { { sql: 'SELECT * FROM users WHERE id = 10', connection: connection } } - - let(:event) do - double( - :event, - name: 'sql.active_record', - duration: 2, - payload: payload - ) - end - # Emulate Marginalia pre-pending comments - def sql(query, comments: true) - if comments && !%w[BEGIN COMMIT].include?(query) - "/*application:web,controller:badges,action:pipeline,correlation_id:01EYN39K9VMJC56Z7808N7RSRH*/ #{query}" - else - query + describe '#transaction' do + let(:web_transaction) { double('Gitlab::Metrics::WebTransaction') } + let(:background_transaction) { double('Gitlab::Metrics::WebTransaction') } + + let(:event) do + double( + :event, + name: 'transaction.active_record', + duration: 230, + payload: { connection: connection } + ) end - end - shared_examples 'track generic sql events' do - where(:name, :sql_query, :record_query, :record_write_query, :record_cached_query) do - 'SQL' | 'SELECT * FROM users WHERE id = 10' | true | false | false - 'SQL' | 'WITH active_milestones AS (SELECT COUNT(*), state FROM milestones GROUP BY state) SELECT * FROM active_milestones' | true | false | false - 'SQL' | 'SELECT * FROM users WHERE id = 10 FOR UPDATE' | true | true | false - 'SQL' | 'WITH archived_rows AS (SELECT * FROM users WHERE archived = true) INSERT INTO products_log SELECT * FROM archived_rows' | true | true | false - 'SQL' | 'DELETE FROM users where id = 10' | true | true | false - 'SQL' | 'INSERT INTO project_ci_cd_settings (project_id) SELECT id FROM projects' | true | true | false - 'SQL' | 'UPDATE users SET admin = true WHERE id = 10' | true | true | false - 'CACHE' | 'SELECT * FROM users WHERE id = 10' | true | false | true - 'SCHEMA' | "SELECT attr.attname FROM pg_attribute attr INNER JOIN pg_constraint cons ON attr.attrelid = cons.conrelid AND attr.attnum = any(cons.conkey) WHERE cons.contype = 'p' AND cons.conrelid = '\"projects\"'::regclass" | false | false | false - nil | 'BEGIN' | false | false | false - nil | 'COMMIT' | false | false | false + before do + allow(background_transaction).to receive(:observe) + allow(web_transaction).to receive(:observe) end - with_them do - let(:payload) { { name: name, sql: sql(sql_query, comments: comments), connection: connection } } + context 'when both web and background transaction are available' do + before do + allow(::Gitlab::Metrics::WebTransaction).to receive(:current) + .and_return(web_transaction) + allow(::Gitlab::Metrics::BackgroundTransaction).to receive(:current) + .and_return(background_transaction) + end + + it 'captures the metrics for web only' do + expect(web_transaction).to receive(:observe).with(:gitlab_database_transaction_seconds, 0.23) - it 'marks the current thread as using the database' do - # since it would already have been toggled by other specs - Thread.current[:uses_db_connection] = nil + expect(background_transaction).not_to receive(:observe) + expect(background_transaction).not_to receive(:increment) - expect { subscriber.sql(event) }.to change { Thread.current[:uses_db_connection] }.from(nil).to(true) + subscriber.transaction(event) end + end + + context 'when web transaction is available' do + let(:web_transaction) { double('Gitlab::Metrics::WebTransaction') } + + before do + allow(::Gitlab::Metrics::WebTransaction).to receive(:current) + .and_return(web_transaction) + allow(::Gitlab::Metrics::BackgroundTransaction).to receive(:current) + .and_return(nil) + end + + it 'captures the metrics for web only' do + expect(web_transaction).to receive(:observe).with(:gitlab_database_transaction_seconds, 0.23) - it_behaves_like 'record ActiveRecord metrics' - it_behaves_like 'store ActiveRecord info in RequestStore' + expect(background_transaction).not_to receive(:observe) + expect(background_transaction).not_to receive(:increment) + + subscriber.transaction(event) + end end - end - context 'without Marginalia comments' do - let(:comments) { false } + context 'when background transaction is available' do + let(:background_transaction) { double('Gitlab::Metrics::BackgroundTransaction') } + + before do + allow(::Gitlab::Metrics::WebTransaction).to receive(:current) + .and_return(nil) + allow(::Gitlab::Metrics::BackgroundTransaction).to receive(:current) + .and_return(background_transaction) + end - it_behaves_like 'track generic sql events' + it 'captures the metrics for web only' do + expect(background_transaction).to receive(:observe).with(:gitlab_database_transaction_seconds, 0.23) + + expect(web_transaction).not_to receive(:observe) + expect(web_transaction).not_to receive(:increment) + + subscriber.transaction(event) + end + end end - context 'with Marginalia comments' do - let(:comments) { true } + describe '#sql' do + let(:payload) { { sql: 'SELECT * FROM users WHERE id = 10', connection: connection } } - it_behaves_like 'track generic sql events' + let(:event) do + double( + :event, + name: 'sql.active_record', + duration: 2, + payload: payload + ) + end + + # Emulate Marginalia pre-pending comments + def sql(query, comments: true) + if comments && !%w[BEGIN COMMIT].include?(query) + "/*application:web,controller:badges,action:pipeline,correlation_id:01EYN39K9VMJC56Z7808N7RSRH*/ #{query}" + else + query + end + end + + shared_examples 'track generic sql events' do + where(:name, :sql_query, :record_query, :record_write_query, :record_cached_query) do + 'SQL' | 'SELECT * FROM users WHERE id = 10' | true | false | false + 'SQL' | 'WITH active_milestones AS (SELECT COUNT(*), state FROM milestones GROUP BY state) SELECT * FROM active_milestones' | true | false | false + 'SQL' | 'SELECT * FROM users WHERE id = 10 FOR UPDATE' | true | true | false + 'SQL' | 'WITH archived_rows AS (SELECT * FROM users WHERE archived = true) INSERT INTO products_log SELECT * FROM archived_rows' | true | true | false + 'SQL' | 'DELETE FROM users where id = 10' | true | true | false + 'SQL' | 'INSERT INTO project_ci_cd_settings (project_id) SELECT id FROM projects' | true | true | false + 'SQL' | 'UPDATE users SET admin = true WHERE id = 10' | true | true | false + 'CACHE' | 'SELECT * FROM users WHERE id = 10' | true | false | true + 'SCHEMA' | "SELECT attr.attname FROM pg_attribute attr INNER JOIN pg_constraint cons ON attr.attrelid = cons.conrelid AND attr.attnum = any(cons.conkey) WHERE cons.contype = 'p' AND cons.conrelid = '\"projects\"'::regclass" | false | false | false + nil | 'BEGIN' | false | false | false + nil | 'COMMIT' | false | false | false + end + + with_them do + let(:payload) { { name: name, sql: sql(sql_query, comments: comments), connection: connection } } + + it 'marks the current thread as using the database' do + # since it would already have been toggled by other specs + Thread.current[:uses_db_connection] = nil + + expect { subscriber.sql(event) }.to change { Thread.current[:uses_db_connection] }.from(nil).to(true) + end + + it_behaves_like 'record ActiveRecord metrics' + it_behaves_like 'store ActiveRecord info in RequestStore' + end + end + + context 'without Marginalia comments' do + let(:comments) { false } + + it_behaves_like 'track generic sql events' + end + + context 'with Marginalia comments' do + let(:comments) { true } + + it_behaves_like 'track generic sql events' + end end end diff --git a/spec/models/concerns/safe_url_spec.rb b/spec/models/concerns/safe_url_spec.rb index 3d38c05bf11..c298e56b1b1 100644 --- a/spec/models/concerns/safe_url_spec.rb +++ b/spec/models/concerns/safe_url_spec.rb @@ -26,14 +26,16 @@ RSpec.describe SafeUrl do context 'when URL contains credentials' do let(:url) { 'http://foo:bar@example.com' } - it { is_expected.to eq('http://*****:*****@example.com')} + it 'masks username and password' do + is_expected.to eq('http://*****:*****@example.com') + end - context 'when username is whitelisted' do - subject { test_class.safe_url(usernames_whitelist: usernames_whitelist) } + context 'when username is allowed' do + subject { test_class.safe_url(allowed_usernames: usernames) } - let(:usernames_whitelist) { %w[foo] } + let(:usernames) { %w[foo] } - it 'does expect the whitelisted username not to be masked' do + it 'masks the password, but not the username' do is_expected.to eq('http://foo:*****@example.com') end end |