From 2df3fbbc602861654d8797da654f578fd2b60303 Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Fri, 21 Oct 2022 06:09:25 +0000 Subject: [PATCH] Add latest changes from gitlab-org/gitlab@master --- app/services/ci/register_job_service.rb | 2 +- app/views/projects/network/show.html.haml | 3 +- doc/ci/environments/index.md | 5 ++ doc/ci/pipelines/settings.md | 2 +- lib/api/admin/ci/variables.rb | 2 + lib/api/api.rb | 5 +- lib/api/ci/variables.rb | 2 + lib/api/group_variables.rb | 2 + .../loggers/filter_parameters.rb | 33 ++++++++++ .../loggers/filter_parameters_spec.rb | 62 +++++++++++++++++++ spec/requests/api/admin/ci/variables_spec.rb | 18 ++++++ spec/requests/api/ci/variables_spec.rb | 20 +++++- spec/requests/api/group_variables_spec.rb | 18 ++++++ spec/services/ci/register_job_service_spec.rb | 13 ++-- 14 files changed, 176 insertions(+), 11 deletions(-) create mode 100644 lib/gitlab/grape_logging/loggers/filter_parameters.rb create mode 100644 spec/lib/gitlab/grape_logging/loggers/filter_parameters_spec.rb diff --git a/app/services/ci/register_job_service.rb b/app/services/ci/register_job_service.rb index 0bd4bf8cc86..814fbf5e565 100644 --- a/app/services/ci/register_job_service.rb +++ b/app/services/ci/register_job_service.rb @@ -42,7 +42,7 @@ module Ci if !db_all_caught_up && !result.build metrics.increment_queue_operation(:queue_replication_lag) - ::Ci::RegisterJobService::Result.new(nil, false) # rubocop:disable Cop/AvoidReturnFromBlocks + ::Ci::RegisterJobService::Result.new(nil, nil, false) # rubocop:disable Cop/AvoidReturnFromBlocks else result end diff --git a/app/views/projects/network/show.html.haml b/app/views/projects/network/show.html.haml index b6700c9ed1e..2a3171e9fd8 100644 --- a/app/views/projects/network/show.html.haml +++ b/app/views/projects/network/show.html.haml @@ -6,8 +6,7 @@ .controls.gl-bg-gray-50.gl-p-2.gl-font-base.gl-text-gray-400.gl-border-b-1.gl-border-b-solid.gl-border-b-gray-300 = form_tag project_network_path(@project, @id), method: :get, class: 'form-inline network-form' do |f| = text_field_tag :extended_sha1, @options[:extended_sha1], placeholder: _("Git revision"), class: 'search-input form-control gl-form-input input-mx-250 search-sha gl-mr-2' - = button_tag class: 'btn gl-button btn-confirm btn-icon' do - = sprite_icon('search') + = render Pajamas::ButtonComponent.new(type: :submit, variant: :confirm, icon: 'search') .inline.gl-ml-5 .form-check.light = check_box_tag :filter_ref, 1, @options[:filter_ref], class: 'form-check-input' diff --git a/doc/ci/environments/index.md b/doc/ci/environments/index.md index 2a92503f5c9..29aa3bf932f 100644 --- a/doc/ci/environments/index.md +++ b/doc/ci/environments/index.md @@ -383,6 +383,11 @@ To retry or rollback a deployment: - To retry a deployment, select **Re-deploy to environment**. - To roll back to a deployment, next to a previously successful deployment, select **Rollback environment**. +NOTE: +If you have [prevented outdated deployment jobs](deployment_safety.md#prevent-outdated-deployment-jobs) in your project, +the rollback buttons might be hidden or disabled. +In this case, see [how to rollback to an outdated deployment](deployment_safety.md#how-to-rollback-to-an-outdated-deployment). + ### Environment URL > - [Fixed](https://gitlab.com/gitlab-org/gitlab/-/issues/337417) to persist arbitrary URLs in GitLab 15.2 [with a flag](../../administration/feature_flags.md) named `soft_validation_on_external_url`. Disabled by default. diff --git a/doc/ci/pipelines/settings.md b/doc/ci/pipelines/settings.md index 246285eedcb..f04a9e549f5 100644 --- a/doc/ci/pipelines/settings.md +++ b/doc/ci/pipelines/settings.md @@ -101,7 +101,7 @@ To avoid this scenario: 1. Select the **Prevent outdated deployment jobs** checkbox. 1. Select **Save changes**. -For more information, see [Deployment safety](../environments/deployment_safety.md). +For more information, see [Deployment safety](../environments/deployment_safety.md#prevent-outdated-deployment-jobs). ## Specify a custom CI/CD configuration file diff --git a/lib/api/admin/ci/variables.rb b/lib/api/admin/ci/variables.rb index 0462878c90c..be73fd496c5 100644 --- a/lib/api/admin/ci/variables.rb +++ b/lib/api/admin/ci/variables.rb @@ -43,6 +43,7 @@ module API desc 'Create a new instance-level variable' do success Entities::Ci::Variable end + route_setting :log_safety, { safe: %w[key], unsafe: %w[value] } params do requires :key, type: String, @@ -80,6 +81,7 @@ module API desc 'Update an existing instance-variable' do success Entities::Ci::Variable end + route_setting :log_safety, { safe: %w[key], unsafe: %w[value] } params do optional :key, type: String, diff --git a/lib/api/api.rb b/lib/api/api.rb index 933c3f69075..981260b9429 100644 --- a/lib/api/api.rb +++ b/lib/api/api.rb @@ -13,13 +13,14 @@ module API USER_REQUIREMENTS = { user_id: NO_SLASH_URL_PART_REGEX }.freeze LOG_FILTERS = ::Rails.application.config.filter_parameters + [/^output$/] LOG_FORMATTER = Gitlab::GrapeLogging::Formatters::LogrageWithTimestamp.new + LOGGER = Logger.new(LOG_FILENAME) insert_before Grape::Middleware::Error, GrapeLogging::Middleware::RequestLogger, - logger: Logger.new(LOG_FILENAME), + logger: LOGGER, formatter: LOG_FORMATTER, include: [ - GrapeLogging::Loggers::FilterParameters.new(LOG_FILTERS), + Gitlab::GrapeLogging::Loggers::FilterParameters.new(LOG_FILTERS), Gitlab::GrapeLogging::Loggers::ClientEnvLogger.new, Gitlab::GrapeLogging::Loggers::RouteLogger.new, Gitlab::GrapeLogging::Loggers::UserLogger.new, diff --git a/lib/api/ci/variables.rb b/lib/api/ci/variables.rb index c9e1d115d03..25a06719783 100644 --- a/lib/api/ci/variables.rb +++ b/lib/api/ci/variables.rb @@ -49,6 +49,7 @@ module API desc 'Create a new variable in a project' do success Entities::Ci::Variable end + route_setting :log_safety, { safe: %w[key], unsafe: %w[value] } params do requires :key, type: String, desc: 'The key of the variable' requires :value, type: String, desc: 'The value of the variable' @@ -74,6 +75,7 @@ module API desc 'Update an existing variable from a project' do success Entities::Ci::Variable end + route_setting :log_safety, { safe: %w[key], unsafe: %w[value] } params do optional :key, type: String, desc: 'The key of the variable' optional :value, type: String, desc: 'The value of the variable' diff --git a/lib/api/group_variables.rb b/lib/api/group_variables.rb index 2235746b254..cc24fb6fc4d 100644 --- a/lib/api/group_variables.rb +++ b/lib/api/group_variables.rb @@ -43,6 +43,7 @@ module API desc 'Create a new variable in a group' do success Entities::Ci::Variable end + route_setting :log_safety, { safe: %w[key], unsafe: %w[value] } params do requires :key, type: String, desc: 'The key of the variable' requires :value, type: String, desc: 'The value of the variable' @@ -74,6 +75,7 @@ module API desc 'Update an existing variable from a group' do success Entities::Ci::Variable end + route_setting :log_safety, { safe: %w[key], unsafe: %w[value] } params do optional :key, type: String, desc: 'The key of the variable' optional :value, type: String, desc: 'The value of the variable' diff --git a/lib/gitlab/grape_logging/loggers/filter_parameters.rb b/lib/gitlab/grape_logging/loggers/filter_parameters.rb new file mode 100644 index 00000000000..ae9df203544 --- /dev/null +++ b/lib/gitlab/grape_logging/loggers/filter_parameters.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +module Gitlab + module GrapeLogging + module Loggers + # In the CI variables APIs, the POST or PUT parameters will always be + # literally 'key' and 'value'. Rails' default filters_parameters will + # always incorrectly mask the value of param 'key' when it should mask the + # value of the param 'value'. + # See: https://gitlab.com/gitlab-org/gitlab/-/issues/353857 + class FilterParameters < ::GrapeLogging::Loggers::FilterParameters + private + + def safe_parameters(request) + loggable_params = super + settings = request.env[Grape::Env::API_ENDPOINT]&.route&.settings + + return loggable_params unless settings&.key?(:log_safety) + + settings[:log_safety][:safe].each do |key| + loggable_params[key] = request.params[key] if loggable_params.key?(key) + end + + settings[:log_safety][:unsafe].each do |key| + loggable_params[key] = @replacement if loggable_params.key?(key) + end + + loggable_params + end + end + end + end +end diff --git a/spec/lib/gitlab/grape_logging/loggers/filter_parameters_spec.rb b/spec/lib/gitlab/grape_logging/loggers/filter_parameters_spec.rb new file mode 100644 index 00000000000..15c842c9f44 --- /dev/null +++ b/spec/lib/gitlab/grape_logging/loggers/filter_parameters_spec.rb @@ -0,0 +1,62 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::GrapeLogging::Loggers::FilterParameters do + subject { described_class.new } + + describe ".parameters" do + let(:route) { instance_double('Grape::Router::Route', settings: settings) } + let(:endpoint) { instance_double('Grape::Endpoint', route: route) } + + let(:env) do + { 'rack.input' => '', Grape::Env::API_ENDPOINT => endpoint } + end + + let(:mock_request) { ActionDispatch::Request.new(env) } + + before do + mock_request.params['key'] = 'some key' + mock_request.params['foo'] = 'wibble' + mock_request.params['value'] = 'some value' + mock_request.params['oof'] = 'wobble' + mock_request.params['other'] = 'Unaffected' + end + + context 'when the log_safety setting is provided' do + let(:settings) { { log_safety: { safe: %w[foo bar key], unsafe: %w[oof rab value] } } } + + it 'includes safe parameters, and filters unsafe ones' do + data = subject.parameters(mock_request, nil) + + expect(data).to eq( + params: { + 'key' => 'some key', + 'foo' => 'wibble', + 'value' => '[FILTERED]', + 'oof' => '[FILTERED]', + 'other' => 'Unaffected' + } + ) + end + end + + context 'when the log_safety is not provided' do + let(:settings) { {} } + + it 'behaves like the normal parameter filter' do + data = subject.parameters(mock_request, nil) + + expect(data).to eq( + params: { + 'key' => '[FILTERED]', + 'foo' => 'wibble', + 'value' => 'some value', + 'oof' => 'wobble', + 'other' => 'Unaffected' + } + ) + end + end + end +end diff --git a/spec/requests/api/admin/ci/variables_spec.rb b/spec/requests/api/admin/ci/variables_spec.rb index f89964411f8..93036bfe413 100644 --- a/spec/requests/api/admin/ci/variables_spec.rb +++ b/spec/requests/api/admin/ci/variables_spec.rb @@ -83,6 +83,15 @@ RSpec.describe ::API::Admin::Ci::Variables do expect(json_response['variable_type']).to eq('env_var') end + it 'masks the new value when logging' do + masked_params = { 'key' => 'VAR_KEY', 'value' => '[FILTERED]', 'protected' => 'true', 'masked' => 'true' } + + expect(::API::API::LOGGER).to receive(:info).with(include(params: include(masked_params))) + + post api("/admin/ci/variables", user), + params: { key: 'VAR_KEY', value: 'SENSITIVE', protected: true, masked: true } + end + it 'creates variable with optional attributes', :aggregate_failures do expect do post api('/admin/ci/variables', admin), @@ -163,6 +172,15 @@ RSpec.describe ::API::Admin::Ci::Variables do expect(json_response['masked']).to be_truthy end + it 'masks the new value when logging' do + masked_params = { 'value' => '[FILTERED]', 'protected' => 'true', 'masked' => 'true' } + + expect(::API::API::LOGGER).to receive(:info).with(include(params: include(masked_params))) + + put api("/admin/ci/variables/#{variable.key}", admin), + params: { value: 'SENSITIVE', protected: true, masked: true } + end + it 'responds with 404 Not Found if requesting non-existing variable' do put api('/admin/ci/variables/non_existing_variable', admin) diff --git a/spec/requests/api/ci/variables_spec.rb b/spec/requests/api/ci/variables_spec.rb index 74ed8c1551d..99e3541acdd 100644 --- a/spec/requests/api/ci/variables_spec.rb +++ b/spec/requests/api/ci/variables_spec.rb @@ -126,9 +126,18 @@ RSpec.describe API::Ci::Variables do expect(json_response['variable_type']).to eq('env_var') end + it 'masks the new value when logging' do + masked_params = { 'key' => 'VAR_KEY', 'value' => '[FILTERED]', 'protected' => 'true', 'masked' => 'true' } + + expect(::API::API::LOGGER).to receive(:info).with(include(params: include(masked_params))) + + post api("/projects/#{project.id}/variables", user), + params: { key: 'VAR_KEY', value: 'SENSITIVE', protected: true, masked: true } + end + it 'creates variable with optional attributes' do expect do - post api("/projects/#{project.id}/variables", user), params: { variable_type: 'file', key: 'TEST_VARIABLE_2', value: 'VALUE_2' } + post api("/projects/#{project.id}/variables", user), params: { variable_type: 'file', key: 'TEST_VARIABLE_2', value: 'VALUE_2' } end.to change { project.variables.count }.by(1) expect(response).to have_gitlab_http_status(:created) @@ -206,6 +215,15 @@ RSpec.describe API::Ci::Variables do expect(updated_variable.variable_type).to eq('file') end + it 'masks the new value when logging' do + masked_params = { 'value' => '[FILTERED]', 'protected' => 'true' } + + expect(::API::API::LOGGER).to receive(:info).with(include(params: include(masked_params))) + + put api("/projects/#{project.id}/variables/#{variable.key}", user), + params: { value: 'SENSITIVE', protected: true } + end + it 'responds with 404 Not Found if requesting non-existing variable' do put api("/projects/#{project.id}/variables/non_existing_variable", user) diff --git a/spec/requests/api/group_variables_spec.rb b/spec/requests/api/group_variables_spec.rb index 4fed7dd7624..979a9d61d15 100644 --- a/spec/requests/api/group_variables_spec.rb +++ b/spec/requests/api/group_variables_spec.rb @@ -102,6 +102,15 @@ RSpec.describe API::GroupVariables do expect(json_response['environment_scope']).to eq('*') end + it 'masks the new value when logging' do + masked_params = { 'key' => 'VAR_KEY', 'value' => '[FILTERED]', 'protected' => 'true', 'masked' => 'true' } + + expect(::API::API::LOGGER).to receive(:info).with(include(params: include(masked_params))) + + post api("/groups/#{group.id}/variables", user), + params: { key: 'VAR_KEY', value: 'SENSITIVE', protected: true, masked: true } + end + it 'creates variable with optional attributes' do expect do post api("/groups/#{group.id}/variables", user), params: { variable_type: 'file', key: 'TEST_VARIABLE_2', value: 'VALUE_2' } @@ -164,6 +173,15 @@ RSpec.describe API::GroupVariables do expect(json_response['masked']).to be_truthy end + it 'masks the new value when logging' do + masked_params = { 'value' => '[FILTERED]', 'protected' => 'true', 'masked' => 'true' } + + expect(::API::API::LOGGER).to receive(:info).with(include(params: include(masked_params))) + + put api("/groups/#{group.id}/variables/#{variable.key}", user), + params: { value: 'SENSITIVE', protected: true, masked: true } + end + it 'responds with 404 Not Found if requesting non-existing variable' do put api("/groups/#{group.id}/variables/non_existing_variable", user) diff --git a/spec/services/ci/register_job_service_spec.rb b/spec/services/ci/register_job_service_spec.rb index e2e760b9812..f40f5cc5a62 100644 --- a/spec/services/ci/register_job_service_spec.rb +++ b/spec/services/ci/register_job_service_spec.rb @@ -14,25 +14,29 @@ module Ci let!(:pending_job) { create(:ci_build, :pending, :queued, pipeline: pipeline) } describe '#execute' do - context 'checks database loadbalancing stickiness' do - subject { described_class.new(shared_runner).execute } + subject { described_class.new(shared_runner).execute } + context 'checks database loadbalancing stickiness' do before do project.update!(shared_runners_enabled: false) end - it 'result is valid if replica did caught-up' do + it 'result is valid if replica did caught-up', :aggregate_failures do expect(ApplicationRecord.sticking).to receive(:all_caught_up?) .with(:runner, shared_runner.id) { true } expect(subject).to be_valid + expect(subject.build).to be_nil + expect(subject.build_json).to be_nil end - it 'result is invalid if replica did not caught-up' do + it 'result is invalid if replica did not caught-up', :aggregate_failures do expect(ApplicationRecord.sticking).to receive(:all_caught_up?) .with(:runner, shared_runner.id) { false } expect(subject).not_to be_valid + expect(subject.build).to be_nil + expect(subject.build_json).to be_nil end end @@ -954,6 +958,7 @@ module Ci expect(result).not_to be_valid expect(result.build).to be_nil + expect(result.build_json).to be_nil end end