diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2021-11-22 18:14:13 +0300 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2021-11-22 18:14:13 +0300 |
commit | ad765b15d8b18af8ebf26a740f679a9b3c543c1a (patch) | |
tree | 5b7152906487f56c8c5c0a4f8ac5579a7ac66589 | |
parent | 860dab2f5f977c7bf14c13c01e43cae2757264f1 (diff) |
Add latest changes from gitlab-org/gitlab@master
-rw-r--r-- | .rubocop_manual_todo.yml | 1 | ||||
-rw-r--r-- | app/services/clusters/cleanup/project_namespace_service.rb | 6 | ||||
-rw-r--r-- | app/services/clusters/cleanup/service_account_service.rb | 4 | ||||
-rw-r--r-- | config/feature_flags/ops/feature_flag_state_logs.yml | 8 | ||||
-rw-r--r-- | danger/roulette/Dangerfile | 4 | ||||
-rw-r--r-- | doc/api/projects.md | 11 | ||||
-rw-r--r-- | doc/integration/mattermost/gitlab-mattermost.msc | 28 | ||||
-rw-r--r-- | doc/integration/mattermost/img/gitlab-mattermost.png | bin | 26656 -> 0 bytes | |||
-rw-r--r-- | doc/integration/mattermost/index.md | 22 | ||||
-rw-r--r-- | lib/feature.rb | 19 | ||||
-rw-r--r-- | lib/gitlab/lograge/custom_options.rb | 4 | ||||
-rw-r--r-- | spec/lib/feature_spec.rb | 22 | ||||
-rw-r--r-- | spec/lib/gitlab/experimentation/experiment_spec.rb | 1 | ||||
-rw-r--r-- | spec/lib/gitlab/lograge/custom_options_spec.rb | 42 | ||||
-rw-r--r-- | spec/services/clusters/cleanup/project_namespace_service_spec.rb | 26 | ||||
-rw-r--r-- | spec/services/clusters/cleanup/service_account_service_spec.rb | 14 |
16 files changed, 177 insertions, 35 deletions
diff --git a/.rubocop_manual_todo.yml b/.rubocop_manual_todo.yml index a7892e477b0..cc8fe9bb8fc 100644 --- a/.rubocop_manual_todo.yml +++ b/.rubocop_manual_todo.yml @@ -2560,7 +2560,6 @@ Style/OpenStructUse: - 'ee/spec/helpers/ee/blob_helper_spec.rb' - 'ee/spec/lib/gitlab/auth/group_saml/failure_handler_spec.rb' - 'ee/spec/lib/gitlab/legacy_github_import/project_creator_spec.rb' - - 'ee/spec/requests/api/ldap_spec.rb' - 'lib/api/wikis.rb' - 'lib/gitlab/git/diff_collection.rb' - 'lib/gitlab/import_export/after_export_strategies/base_after_export_strategy.rb' diff --git a/app/services/clusters/cleanup/project_namespace_service.rb b/app/services/clusters/cleanup/project_namespace_service.rb index 0173f93f625..80192aa14ab 100644 --- a/app/services/clusters/cleanup/project_namespace_service.rb +++ b/app/services/clusters/cleanup/project_namespace_service.rb @@ -26,8 +26,10 @@ module Clusters begin kubeclient_delete_namespace(kubernetes_namespace) - rescue Kubeclient::HttpError - next + rescue Kubeclient::HttpError => e + # unauthorized, forbidden: GitLab's access has been revoked + # certificate verify failed: Cluster is probably gone forever + raise unless e.message =~ /unauthorized|forbidden|certificate verify failed/i end kubernetes_namespace.destroy! diff --git a/app/services/clusters/cleanup/service_account_service.rb b/app/services/clusters/cleanup/service_account_service.rb index 53f968cd409..dce41d2a39c 100644 --- a/app/services/clusters/cleanup/service_account_service.rb +++ b/app/services/clusters/cleanup/service_account_service.rb @@ -24,6 +24,10 @@ module Clusters # The resources have already been deleted, possibly on a previous attempt that timed out rescue Gitlab::UrlBlocker::BlockedUrlError # User gave an invalid cluster from the start, or deleted the endpoint before this job ran + rescue Kubeclient::HttpError => e + # unauthorized, forbidden: GitLab's access has been revoked + # certificate verify failed: Cluster is probably gone forever + raise unless e.message =~ /unauthorized|forbidden|certificate verify failed/i end end end diff --git a/config/feature_flags/ops/feature_flag_state_logs.yml b/config/feature_flags/ops/feature_flag_state_logs.yml new file mode 100644 index 00000000000..7153aca3860 --- /dev/null +++ b/config/feature_flags/ops/feature_flag_state_logs.yml @@ -0,0 +1,8 @@ +--- +name: feature_flag_state_logs +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/73729 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/345888 +milestone: '14.6' +type: ops +group: group::incubation +default_enabled: false diff --git a/danger/roulette/Dangerfile b/danger/roulette/Dangerfile index 4eb1d987d84..6b0c196dc5d 100644 --- a/danger/roulette/Dangerfile +++ b/danger/roulette/Dangerfile @@ -49,8 +49,8 @@ for them. MARKDOWN def group_not_available_template(slack_channel, gitlab_group) - <<~TEMPLATE - No engineer is available for automated assignment, please reach out to `#{slack_channel}` slack channel or mention `#{gitlab_group}` for assistance. + <<~TEMPLATE.strip + No engineer is available for automated assignment, please reach out to the `#{slack_channel}` Slack channel or mention `#{gitlab_group}` for assistance. TEMPLATE end diff --git a/doc/api/projects.md b/doc/api/projects.md index d19019c9597..4caa4c4ffe6 100644 --- a/doc/api/projects.md +++ b/doc/api/projects.md @@ -1350,6 +1350,17 @@ where `password` is a public access key with the `api` scope enabled. PUT /projects/:id ``` +For example, to toggle the setting for +[shared runners on a GitLab.com project](../user/gitlab_com/index.md#shared-runner-cloud-runners): + +```shell +curl --request PUT --header "PRIVATE-TOKEN: <your-token>" \ + --url 'https://gitlab.com/api/v4/projects/<your-project-ID>' \ + --data "shared_runners_enabled=true" # to turn off: "shared_runners_enabled=false" +``` + +Supported attributes: + | Attribute | Type | Required | Description | |-------------------------------------------------------------|----------------|------------------------|-------------| | `allow_merge_on_skipped_pipeline` | boolean | **{dotted-circle}** No | Set whether or not merge requests can be merged with skipped jobs. | diff --git a/doc/integration/mattermost/gitlab-mattermost.msc b/doc/integration/mattermost/gitlab-mattermost.msc deleted file mode 100644 index f6d4bf7aa68..00000000000 --- a/doc/integration/mattermost/gitlab-mattermost.msc +++ /dev/null @@ -1,28 +0,0 @@ -msc { - # Use https://mscgen.js.org or mscgen to convert this into PNG - hscale="1.5", - wordwraparcs=on; - - user [ label="User", textbgcolor="blue", textcolor="white" ], - mattermost [ label="Mattermost", textbgcolor="red", textcolor="white"], - gitlab [ label="GitLab", textbgcolor="indigo", textcolor="white"]; - - user=>mattermost [label="GET https://mm.domain.com"]; - mattermost note gitlab [label="Obtain access code", textcolor="green"]; - mattermost=>gitlab [label="GET https://gitlab.domain.com/oauth/authorize", textcolor="indigo"]; - gitlab rbox user [label="GitLab user logs in (if necessary)"]; - gitlab rbox gitlab [label="GitLab verifies client_id matches an OAuth application"]; - gitlab=>user [label="GitLab asks user to authorize Mattermost OAuth app"]; - user=>gitlab [label="User clicks 'Allow'"]; - gitlab rbox gitlab [label="GitLab verifies redirect_uri matches list of valid URLs"]; - gitlab=>user [label="302 Redirect: https://mm.domain.com/signup/gitlab/complete"]; - user=>mattermost [label="GET https://mm.domain.com/signup/gitlab/complete", textcolor="red"]; - mattermost note gitlab [label="Exchange access code for access token", textcolor="green"]; - mattermost=>gitlab [label="POST http://gitlab.domain.com/oauth/token", textcolor="indigo"]; - gitlab=>gitlab [label="Doorkeeper::TokensController#create"]; - gitlab=>mattermost [label="Access token", textcolor="red"]; - mattermost note gitlab [label="Mattermost looks up GitLab user", textcolor="green"]; - mattermost=>gitlab [label="GET https://gitlab.domain.com/api/v4/user", textcolor="indigo"]; - gitlab=>mattermost [label="User details", textcolor="red"]; - mattermost=>user [label="Mattermost/GitLab user ready"]; -} diff --git a/doc/integration/mattermost/img/gitlab-mattermost.png b/doc/integration/mattermost/img/gitlab-mattermost.png Binary files differdeleted file mode 100644 index c3b019988d0..00000000000 --- a/doc/integration/mattermost/img/gitlab-mattermost.png +++ /dev/null diff --git a/doc/integration/mattermost/index.md b/doc/integration/mattermost/index.md index 0489ccd431c..a532e94e838 100644 --- a/doc/integration/mattermost/index.md +++ b/doc/integration/mattermost/index.md @@ -476,7 +476,27 @@ The following image is a sequence diagram for how GitLab works as an OAuth2 provider for Mattermost. You can use this to troubleshoot errors in getting the integration to work: -![sequence diagram](img/gitlab-mattermost.png) +```mermaid +sequenceDiagram + User->>Mattermost: GET https://mm.domain.com + Note over Mattermost, GitLab: Obtain access code + Mattermost->>GitLab: GET https://gitlab.domain.com/oauth/authorize + Note over User, GitLab: GitLab user signs in (if necessary) + Note over GitLab: GitLab verifies client_id matches an OAuth application + GitLab->>User: GitLab asks user to authorize Mattermost OAuth app + User->>GitLab: User selects 'Allow' + Note over GitLab: GitLab verifies redirect_uri matches list of valid URLs + GitLab->>User: 302 redirect: https://mm.domain.com/signup/gitlab/complete + User->>Mattermost: GET https://mm.domain.com/signup/gitlab/complete + Note over Mattermost, GitLab: Exchange access code for access token + Mattermost->>GitLab: POST http://gitlab.domain.com/oauth/token + GitLab->>GitLab: Doorkeeper::TokensController#35;create + GitLab->>Mattermost: Access token + Note over Mattermost, GitLab: Mattermost looks up GitLab user + Mattermost->>GitLab: GET https://gitlab.domain.com/api/v4/user + GitLab->>Mattermost: User details + Mattermost->>User: Mattermost/GitLab user ready +``` ## Troubleshooting the Mattermost CLI diff --git a/lib/feature.rb b/lib/feature.rb index 8186fbc40fa..1a299323dda 100644 --- a/lib/feature.rb +++ b/lib/feature.rb @@ -83,7 +83,12 @@ class Feature # `persisted?` can potentially generate DB queries and also checks for inclusion # in an array of feature names (177 at last count), possibly reducing performance by half. # So we only perform the `persisted` check if `default_enabled: true` - !default_enabled || Feature.persisted_name?(feature.name) ? feature.enabled?(thing) : true + feature_value = !default_enabled || Feature.persisted_name?(feature.name) ? feature.enabled?(thing) : true + + # If we don't filter out this flag here we will enter an infinite loop + log_feature_flag_state(key, feature_value) if log_feature_flag_states?(key) + + feature_value end def disabled?(key, thing = nil, type: :development, default_enabled: false) @@ -153,6 +158,18 @@ class Feature @logger ||= Feature::Logger.build end + def log_feature_flag_states?(key) + key != :feature_flag_state_logs && Feature.enabled?(:feature_flag_state_logs, type: :ops) + end + + def log_feature_flag_state(key, feature_value) + logged_states[key] ||= feature_value + end + + def logged_states + RequestStore.fetch(:feature_flag_events) { {} } + end + private def flipper diff --git a/lib/gitlab/lograge/custom_options.rb b/lib/gitlab/lograge/custom_options.rb index e6c9ba0773c..84ead5119d5 100644 --- a/lib/gitlab/lograge/custom_options.rb +++ b/lib/gitlab/lograge/custom_options.rb @@ -32,6 +32,10 @@ module Gitlab ::Gitlab::ExceptionLogFormatter.format!(exception, payload) + if Feature.enabled?(:feature_flag_state_logs, type: :ops) + payload[:feature_flag_states] = Feature.logged_states.map { |key, state| "#{key}:#{state ? 1 : 0}" } + end + payload end end diff --git a/spec/lib/feature_spec.rb b/spec/lib/feature_spec.rb index 58e7292c125..2c4b0a85537 100644 --- a/spec/lib/feature_spec.rb +++ b/spec/lib/feature_spec.rb @@ -127,6 +127,10 @@ RSpec.describe Feature, stub_feature_flags: false do end describe '.enabled?' do + before do + allow(Feature).to receive(:log_feature_flag_states?).and_return(false) + end + it 'returns false for undefined feature' do expect(described_class.enabled?(:some_random_feature_flag)).to be_falsey end @@ -179,6 +183,24 @@ RSpec.describe Feature, stub_feature_flags: false do expect(described_class.enabled?(:a_feature, default_enabled: fake_default)).to eq(fake_default) end + context 'logging is enabled', :request_store do + before do + allow(Feature).to receive(:log_feature_flag_states?).and_call_original + described_class.enable(:feature_flag_state_logs) + described_class.enable(:enabled_feature_flag) + described_class.enabled?(:enabled_feature_flag) + end + + it 'does not log feature_flag_state_logs' do + expect(described_class.logged_states).not_to have_key("feature_flag_state_logs") + end + + it 'logs other feature flags' do + expect(described_class.logged_states).to have_key(:enabled_feature_flag) + expect(described_class.logged_states[:enabled_feature_flag]).to be_truthy + end + end + context 'cached feature flag', :request_store do let(:flag) { :some_feature_flag } diff --git a/spec/lib/gitlab/experimentation/experiment_spec.rb b/spec/lib/gitlab/experimentation/experiment_spec.rb index d52ab3a8983..d9bf85460b3 100644 --- a/spec/lib/gitlab/experimentation/experiment_spec.rb +++ b/spec/lib/gitlab/experimentation/experiment_spec.rb @@ -16,6 +16,7 @@ RSpec.describe Gitlab::Experimentation::Experiment do before do skip_feature_flags_yaml_validation skip_default_enabled_yaml_check + allow(Feature).to receive(:log_feature_flag_states?).and_return(false) feature = double('FeatureFlag', percentage_of_time_value: percentage, enabled?: true) allow(Feature).to receive(:get).with(:experiment_key_experiment_percentage).and_return(feature) end diff --git a/spec/lib/gitlab/lograge/custom_options_spec.rb b/spec/lib/gitlab/lograge/custom_options_spec.rb index a4ae39a835a..5c9cd86ce7d 100644 --- a/spec/lib/gitlab/lograge/custom_options_spec.rb +++ b/spec/lib/gitlab/lograge/custom_options_spec.rb @@ -95,5 +95,47 @@ RSpec.describe Gitlab::Lograge::CustomOptions do expect(subject[correlation_id_key]).to eq('123456') end end + + context 'when feature flags are present', :request_store do + before do + allow(Feature::Definition).to receive(:valid_usage!).and_return(true) + + allow(Feature).to receive(:log_feature_flag_states?).and_return(false) + + Feature.enable(:enabled_feature) + Feature.disable(:disabled_feature) + + allow(Feature).to receive(:log_feature_flag_states?).with(:enabled_feature).and_call_original + allow(Feature).to receive(:log_feature_flag_states?).with(:disabled_feature).and_call_original + end + + context 'and :feature_flag_log_states is enabled' do + before do + Feature.enable(:feature_flag_state_logs) + end + + it 'adds feature flag events' do + Feature.enabled?(:enabled_feature) + Feature.enabled?(:disabled_feature) + + expect(subject).to have_key(:feature_flag_states) + expect(subject[:feature_flag_states]).to match_array(%w[enabled_feature:1 disabled_feature:0]) + end + end + + context 'and :feature_flag_log_states is disabled' do + before do + Feature.disable(:feature_flag_state_logs) + end + + it 'does not track or add feature flag events' do + Feature.enabled?(:enabled_feature) + Feature.enabled?(:disabled_feature) + + expect(subject).not_to have_key(:feature_flag_states) + expect(Feature).not_to receive(:log_feature_flag_state) + end + end + end end end diff --git a/spec/services/clusters/cleanup/project_namespace_service_spec.rb b/spec/services/clusters/cleanup/project_namespace_service_spec.rb index ec510b2e3c5..8d3ae217a9f 100644 --- a/spec/services/clusters/cleanup/project_namespace_service_spec.rb +++ b/spec/services/clusters/cleanup/project_namespace_service_spec.rb @@ -95,5 +95,31 @@ RSpec.describe Clusters::Cleanup::ProjectNamespaceService do subject end end + + context 'when there is a Kubeclient::HttpError' do + let(:kubeclient_instance_double) do + instance_double(Gitlab::Kubernetes::KubeClient) + end + + ['Unauthorized', 'forbidden', 'Certificate verify Failed'].each do |message| + it 'schedules ::ServiceAccountWorker with accepted errors' do + allow(kubeclient_instance_double) + .to receive(:delete_namespace) + .and_raise(Kubeclient::HttpError.new(401, message, nil)) + + expect(Clusters::Cleanup::ServiceAccountWorker).to receive(:perform_async).with(cluster.id) + + subject + end + end + + it 'raises error with unaccepted errors' do + allow(kubeclient_instance_double) + .to receive(:delete_namespace) + .and_raise(Kubeclient::HttpError.new(401, 'unexpected message', nil)) + + expect { subject }.to raise_error(Kubeclient::HttpError) + end + end end end diff --git a/spec/services/clusters/cleanup/service_account_service_spec.rb b/spec/services/clusters/cleanup/service_account_service_spec.rb index adcdbd84da0..769762237f9 100644 --- a/spec/services/clusters/cleanup/service_account_service_spec.rb +++ b/spec/services/clusters/cleanup/service_account_service_spec.rb @@ -52,5 +52,19 @@ RSpec.describe Clusters::Cleanup::ServiceAccountService do expect { subject }.to change { Clusters::Cluster.where(id: cluster.id).exists? }.from(true).to(false) end end + + context 'when there is a Kubeclient::HttpError' do + ['Unauthorized', 'forbidden', 'Certificate verify Failed'].each do |message| + before do + allow(kubeclient_instance_double) + .to receive(:delete_service_account) + .and_raise(Kubeclient::HttpError.new(401, message, nil)) + end + + it 'destroys cluster' do + expect { subject }.to change { Clusters::Cluster.where(id: cluster.id).exists? }.from(true).to(false) + end + end + end end end |