diff options
| -rw-r--r-- | Gemfile | 2 | ||||
| -rw-r--r-- | Gemfile.lock | 4 | ||||
| -rw-r--r-- | app/controllers/health_controller.rb | 4 | ||||
| -rw-r--r-- | app/controllers/projects/branches_controller.rb | 19 | ||||
| -rw-r--r-- | changelogs/unreleased/ak-health-check-custom-error.yml | 5 | ||||
| -rw-r--r-- | changelogs/unreleased/sh-limit-diverging-commit-counts.yml | 5 | ||||
| -rw-r--r-- | doc/user/permissions.md | 1 | ||||
| -rw-r--r-- | qa/qa/page/project/issue/show.rb | 8 | ||||
| -rw-r--r-- | qa/qa/resource/issue.rb | 1 | ||||
| -rw-r--r-- | spec/controllers/health_controller_spec.rb | 13 | ||||
| -rw-r--r-- | spec/controllers/projects/branches_controller_spec.rb | 50 |
11 files changed, 107 insertions, 5 deletions
@@ -405,7 +405,7 @@ group :test do gem 'webmock', '~> 3.5.1' gem 'rails-controller-testing' gem 'concurrent-ruby', '~> 1.1' - gem 'test-prof', '~> 0.2.5' + gem 'test-prof', '~> 0.10.0' gem 'rspec_junit_formatter' end diff --git a/Gemfile.lock b/Gemfile.lock index 404c2d550bd..4a2016c8a8b 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -994,7 +994,7 @@ GEM temple (0.8.1) terminal-table (1.8.0) unicode-display_width (~> 1.1, >= 1.1.1) - test-prof (0.2.5) + test-prof (0.10.0) text (1.3.1) thin (1.7.2) daemons (~> 1.0, >= 1.0.9) @@ -1314,7 +1314,7 @@ DEPENDENCIES stackprof (~> 0.2.10) state_machines-activerecord (~> 0.5.1) sys-filesystem (~> 1.1.6) - test-prof (~> 0.2.5) + test-prof (~> 0.10.0) thin (~> 1.7.0) timecop (~> 0.8.0) toml-rb (~> 1.0.0) diff --git a/app/controllers/health_controller.rb b/app/controllers/health_controller.rb index c97057c08cb..84b4932ba7a 100644 --- a/app/controllers/health_controller.rb +++ b/app/controllers/health_controller.rb @@ -41,6 +41,10 @@ class HealthController < ActionController::Base info[:labels] = r.labels if r.labels [name, info] end + + # disable static error pages at the gitlab-workhorse level, we want to see this error response even in production + headers["X-GitLab-Custom-Error"] = 1 unless success + render json: response.to_h, status: success ? :ok : :service_unavailable end end diff --git a/app/controllers/projects/branches_controller.rb b/app/controllers/projects/branches_controller.rb index c125ed3605a..578a3d451a7 100644 --- a/app/controllers/projects/branches_controller.rb +++ b/app/controllers/projects/branches_controller.rb @@ -11,6 +11,7 @@ class Projects::BranchesController < Projects::ApplicationController # Support legacy URLs before_action :redirect_for_legacy_index_sort_or_search, only: [:index] + before_action :limit_diverging_commit_counts!, only: [:diverging_commit_counts] def index respond_to do |format| @@ -125,6 +126,24 @@ class Projects::BranchesController < Projects::ApplicationController private + # It can be expensive to calculate the diverging counts for each + # branch. Normally the frontend should be specifying a set of branch + # names, but prior to + # https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/32496, the + # frontend could omit this set. To prevent excessive I/O, we require + # that a list of names be specified. + def limit_diverging_commit_counts! + return unless Feature.enabled?(:limit_diverging_commit_counts, default_enabled: true) + + limit = Kaminari.config.default_per_page + + # If we don't have many branches in the repository, then go ahead. + return if project.repository.branch_count <= limit + return if params[:names].present? && Array(params[:names]).length <= limit + + render json: { error: "Specify at least one and at most #{limit} branch names" }, status: :unprocessable_entity + end + def ref if params[:ref] ref_escaped = strip_tags(sanitize(params[:ref])) diff --git a/changelogs/unreleased/ak-health-check-custom-error.yml b/changelogs/unreleased/ak-health-check-custom-error.yml new file mode 100644 index 00000000000..dd9ef8f9c7a --- /dev/null +++ b/changelogs/unreleased/ak-health-check-custom-error.yml @@ -0,0 +1,5 @@ +--- +title: Disable gitlab-workhorse static error page on health endpoints +merge_request: 17770 +author: +type: fixed diff --git a/changelogs/unreleased/sh-limit-diverging-commit-counts.yml b/changelogs/unreleased/sh-limit-diverging-commit-counts.yml new file mode 100644 index 00000000000..8f0acf879cf --- /dev/null +++ b/changelogs/unreleased/sh-limit-diverging-commit-counts.yml @@ -0,0 +1,5 @@ +--- +title: Limit diverging commit counts requests +merge_request: 16737 +author: +type: performance diff --git a/doc/user/permissions.md b/doc/user/permissions.md index c911336d274..70191100e9e 100644 --- a/doc/user/permissions.md +++ b/doc/user/permissions.md @@ -222,6 +222,7 @@ group. | Edit epic comments (posted by any user) **(ULTIMATE)** | | | | ✓ (2) | ✓ (2) | | View group Audit Events | | | | | ✓ | | Disable notification emails | | | | | ✓ | +| View/manage group-level Kubernetes cluster | | | | ✓ | ✓ | - (1): Groups can be set to [allow either Owners or Owners and Maintainers to create subgroups](group/subgroups/index.md#creating-a-subgroup) diff --git a/qa/qa/page/project/issue/show.rb b/qa/qa/page/project/issue/show.rb index 9264289b603..0aa53b1487a 100644 --- a/qa/qa/page/project/issue/show.rb +++ b/qa/qa/page/project/issue/show.rb @@ -35,6 +35,10 @@ module QA element :more_assignees_link end + view 'app/assets/javascripts/vue_shared/components/issue/related_issuable_item.vue' do + element :remove_issue_button + end + view 'app/helpers/dropdowns_helper.rb' do element :dropdown_input_field end @@ -76,6 +80,10 @@ module QA click_element(:milestone_link) end + def click_remove_issue_button + click_element(:remove_issue_button) + end + # Adds a comment to an issue # attachment option should be an absolute path def comment(text, attachment: nil, filter: :all_activities) diff --git a/qa/qa/resource/issue.rb b/qa/qa/resource/issue.rb index 9d0a5e159e0..0817a9de06f 100644 --- a/qa/qa/resource/issue.rb +++ b/qa/qa/resource/issue.rb @@ -15,6 +15,7 @@ module QA end attribute :id + attribute :iid attribute :assignee_ids attribute :labels attribute :title diff --git a/spec/controllers/health_controller_spec.rb b/spec/controllers/health_controller_spec.rb index 8c44ae4e3de..ae05573af2e 100644 --- a/spec/controllers/health_controller_spec.rb +++ b/spec/controllers/health_controller_spec.rb @@ -30,6 +30,19 @@ describe HealthController do expect(json_response['shared_state_check']['status']).to eq('ok') expect(json_response['gitaly_check']['status']).to eq('ok') end + + it 'responds with readiness checks data when a failure happens' do + allow(Gitlab::HealthChecks::Redis::RedisCheck).to receive(:readiness).and_return(Gitlab::HealthChecks::Result.new(false, "check error")) + + subject + + expect(json_response['redis_check']['status']).to eq('failed') + expect(json_response['redis_check']['message']).to eq('check error') + expect(json_response['cache_check']['status']).to eq('ok') + + expect(response.status).to eq(503) + expect(response.headers['X-GitLab-Custom-Error']).to eq(1) + end end context 'accessed from whitelisted ip' do diff --git a/spec/controllers/projects/branches_controller_spec.rb b/spec/controllers/projects/branches_controller_spec.rb index f5bcea4a097..affe0e0f970 100644 --- a/spec/controllers/projects/branches_controller_spec.rb +++ b/spec/controllers/projects/branches_controller_spec.rb @@ -578,7 +578,9 @@ describe Projects::BranchesController do describe 'GET diverging_commit_counts' do before do sign_in(user) + end + it 'returns the commit counts behind and ahead of default branch' do get :diverging_commit_counts, format: :json, params: { @@ -586,14 +588,58 @@ describe Projects::BranchesController do project_id: project, names: ['fix', 'add-pdf-file', 'branch-merged'] } - end - it 'returns the commit counts behind and ahead of default branch' do + expect(response).to have_gitlab_http_status(200) expect(json_response).to eq( "fix" => { "behind" => 29, "ahead" => 2 }, "branch-merged" => { "behind" => 1, "ahead" => 0 }, "add-pdf-file" => { "behind" => 0, "ahead" => 3 } ) end + + it 'returns the commits counts with no names provided' do + allow_any_instance_of(Repository).to receive(:branch_count).and_return(Kaminari.config.default_per_page) + + get :diverging_commit_counts, + format: :json, + params: { + namespace_id: project.namespace, + project_id: project + } + + expect(response).to have_gitlab_http_status(200) + expect(json_response.count).to be > 1 + end + + describe 'with many branches' do + before do + allow_any_instance_of(Repository).to receive(:branch_count).and_return(Kaminari.config.default_per_page + 1) + end + + it 'returns 422 if no names are specified' do + get :diverging_commit_counts, + format: :json, + params: { + namespace_id: project.namespace, + project_id: project + } + + expect(response).to have_gitlab_http_status(422) + expect(json_response['error']).to eq("Specify at least one and at most #{Kaminari.config.default_per_page} branch names") + end + + it 'returns the list of counts' do + get :diverging_commit_counts, + format: :json, + params: { + namespace_id: project.namespace, + project_id: project, + names: ['fix', 'add-pdf-file', 'branch-merged'] + } + + expect(response).to have_gitlab_http_status(200) + expect(json_response.count).to be > 1 + end + end end end |