Commit 57dc2333 authored by Douwe Maan's avatar Douwe Maan

Merge branch 'osw-configurable-single-diff-file-limit' into 'master'

Make single diff patch limit configurable

Closes #48027

See merge request gitlab-org/gitlab-ce!21886
parents 8ed35734 4fbca2a3
...@@ -254,7 +254,8 @@ module ApplicationSettingsHelper ...@@ -254,7 +254,8 @@ module ApplicationSettingsHelper
:user_default_internal_regex, :user_default_internal_regex,
:user_oauth_applications, :user_oauth_applications,
:version_check_enabled, :version_check_enabled,
:web_ide_clientside_preview_enabled :web_ide_clientside_preview_enabled,
:diff_max_patch_bytes
] ]
end end
......
...@@ -182,6 +182,12 @@ class ApplicationSetting < ActiveRecord::Base ...@@ -182,6 +182,12 @@ class ApplicationSetting < ActiveRecord::Base
numericality: { less_than_or_equal_to: :gitaly_timeout_default }, numericality: { less_than_or_equal_to: :gitaly_timeout_default },
if: :gitaly_timeout_default if: :gitaly_timeout_default
validates :diff_max_patch_bytes,
presence: true,
numericality: { only_integer: true,
greater_than_or_equal_to: Gitlab::Git::Diff::DEFAULT_MAX_PATCH_BYTES,
less_than_or_equal_to: Gitlab::Git::Diff::MAX_PATCH_BYTES_UPPER_BOUND }
validates :user_default_internal_regex, js_regex: true, allow_nil: true validates :user_default_internal_regex, js_regex: true, allow_nil: true
SUPPORTED_KEY_TYPES.each do |type| SUPPORTED_KEY_TYPES.each do |type|
...@@ -293,7 +299,8 @@ class ApplicationSetting < ActiveRecord::Base ...@@ -293,7 +299,8 @@ class ApplicationSetting < ActiveRecord::Base
user_default_external: false, user_default_external: false,
user_default_internal_regex: nil, user_default_internal_regex: nil,
user_show_add_ssh_key_message: true, user_show_add_ssh_key_message: true,
usage_stats_set_by_user_id: nil usage_stats_set_by_user_id: nil,
diff_max_patch_bytes: Gitlab::Git::Diff::DEFAULT_MAX_PATCH_BYTES
} }
end end
......
= form_for @application_setting, url: admin_application_settings_path(anchor: 'js-merge-request-settings'), html: { class: 'fieldset-form' } do |f|
= form_errors(@application_setting)
%fieldset
.form-group
= f.label :diff_max_patch_bytes, 'Maximum diff patch size (Bytes)', class: 'label-light'
= f.number_field :diff_max_patch_bytes, class: 'form-control'
%span.form-text.text-muted
Diff files surpassing this limit will be presented as 'too large'
and won't be expandable.
= link_to icon('question-circle'),
help_page_path('user/admin_area/diff_limits',
anchor: 'maximum-diff-patch-size')
= f.submit _('Save changes'), class: 'btn btn-success'
...@@ -24,6 +24,17 @@ ...@@ -24,6 +24,17 @@
.settings-content .settings-content
= render 'account_and_limit' = render 'account_and_limit'
%section.settings.as-diff-limits.no-animate#js-merge-request-settings{ class: ('expanded' if expanded_by_default?) }
.settings-header
%h4
= _('Diff limits')
%button.btn.js-settings-toggle{ type: 'button' }
= expanded_by_default? ? _('Collapse') : _('Expand')
%p
= _('Diff content limits')
.settings-content
= render 'diff_limits'
%section.settings.as-signup.no-animate#js-signup-settings{ class: ('expanded' if expanded_by_default?) } %section.settings.as-signup.no-animate#js-signup-settings{ class: ('expanded' if expanded_by_default?) }
.settings-header .settings-header
%h4 %h4
......
---
title: Make single diff patch limit configurable
merge_request: 21886
author:
type: added
# frozen_string_literal: true
# See http://doc.gitlab.com/ce/development/migration_style_guide.html
# for more information on how to write migrations for GitLab.
class AddDiffMaxPatchBytesToApplicationSettings < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
# Set this constant to true if this migration requires downtime.
DOWNTIME = false
disable_ddl_transaction!
def up
add_column_with_default(:application_settings,
:diff_max_patch_bytes,
:integer,
default: 100.kilobytes,
allow_null: false)
end
def down
remove_column(:application_settings, :diff_max_patch_bytes)
end
end
...@@ -11,7 +11,7 @@ ...@@ -11,7 +11,7 @@
# #
# It's strongly recommended that you check this file into your version control system. # It's strongly recommended that you check this file into your version control system.
ActiveRecord::Schema.define(version: 20180917172041) do ActiveRecord::Schema.define(version: 20180924141949) do
# These are extensions that must be enabled in order to support this database # These are extensions that must be enabled in order to support this database
enable_extension "plpgsql" enable_extension "plpgsql"
...@@ -171,6 +171,7 @@ ActiveRecord::Schema.define(version: 20180917172041) do ...@@ -171,6 +171,7 @@ ActiveRecord::Schema.define(version: 20180917172041) do
t.boolean "user_show_add_ssh_key_message", default: true, null: false t.boolean "user_show_add_ssh_key_message", default: true, null: false
t.integer "usage_stats_set_by_user_id" t.integer "usage_stats_set_by_user_id"
t.integer "receive_max_input_size" t.integer "receive_max_input_size"
t.integer "diff_max_patch_bytes", default: 102400, null: false
end end
create_table "audit_events", force: :cascade do |t| create_table "audit_events", force: :cascade do |t|
......
...@@ -47,6 +47,7 @@ Learn how to install, configure, update, and maintain your GitLab instance. ...@@ -47,6 +47,7 @@ Learn how to install, configure, update, and maintain your GitLab instance.
- [Enforcing Terms of Service](../user/admin_area/settings/terms.md) - [Enforcing Terms of Service](../user/admin_area/settings/terms.md)
- [Third party offers](../user/admin_area/settings/third_party_offers.md) - [Third party offers](../user/admin_area/settings/third_party_offers.md)
- [Compliance](compliance.md): A collection of features from across the application that you may configure to help ensure that your GitLab instance and DevOps workflow meet compliance standards. - [Compliance](compliance.md): A collection of features from across the application that you may configure to help ensure that your GitLab instance and DevOps workflow meet compliance standards.
- [Diff limits](../user/admin_area/diff_limits.md): Configure the diff rendering size limits of branch comparison pages.
#### Customizing GitLab's appearance #### Customizing GitLab's appearance
......
...@@ -2,13 +2,10 @@ ...@@ -2,13 +2,10 @@
Currently we rely on different sources to present diffs, these include: Currently we rely on different sources to present diffs, these include:
- Rugged gem
- Gitaly service - Gitaly service
- Database (through `merge_request_diff_files`) - Database (through `merge_request_diff_files`)
- Redis (cached highlighted diffs) - Redis (cached highlighted diffs)
We're constantly moving Rugged calls to Gitaly and the progress can be followed through [Gitaly repo](https://gitlab.com/gitlab-org/gitaly).
## Architecture overview ## Architecture overview
### Merge request diffs ### Merge request diffs
...@@ -19,8 +16,9 @@ we fetch the comparison information using `Gitlab::Git::Compare`, which fetches ...@@ -19,8 +16,9 @@ we fetch the comparison information using `Gitlab::Git::Compare`, which fetches
The diffs fetching process _limits_ single file diff sizes and the overall size of the whole diff through a series of constant values. Raw diff files are The diffs fetching process _limits_ single file diff sizes and the overall size of the whole diff through a series of constant values. Raw diff files are
then persisted on `merge_request_diff_files` table. then persisted on `merge_request_diff_files` table.
Even though diffs higher than 10kb are collapsed (`Gitlab::Git::Diff::COLLAPSE_LIMIT`), we still keep them on Postgres. However, diff files over _safety limits_ Even though diffs larger than 10% of the value of `ApplicationSettings#diff_max_patch_bytes` are collapsed,
(see the [Diff limits section](#diff-limits)) are _not_ persisted. we still keep them on Postgres. However, diff files larger than defined _safety limits_
(see the [Diff limits section](#diff-limits)) are _not_ persisted in the database.
In order to present diffs information on the Merge Request diffs page, we: In order to present diffs information on the Merge Request diffs page, we:
...@@ -102,23 +100,20 @@ Gitaly will only return the safe amount of data to be persisted on `merge_reques ...@@ -102,23 +100,20 @@ Gitaly will only return the safe amount of data to be persisted on `merge_reques
Limits that act onto each diff file of a collection. Files number, lines number and files size are considered. Limits that act onto each diff file of a collection. Files number, lines number and files size are considered.
```ruby #### Expandable patches (collapsed)
Gitlab::Git::Diff::COLLAPSE_LIMIT = 10.kilobytes
```
File diff will be collapsed (but be expandable) if it is larger than 10 kilobytes. Diff patches are collapsed when surpassing 10% of the value set in `ApplicationSettings#diff_max_patch_bytes`.
That is, it's equivalent to 10kb if the maximum allowed value is 100kb.
The diff will still be persisted and expandable if the patch size doesn't
surpass `ApplicationSettings#diff_max_patch_bytes`.
*Note:* Although this nomenclature (Collapsing) is also used on Gitaly, this limit is only used on GitLab (hardcoded - not sent to Gitaly). *Note:* Although this nomenclature (Collapsing) is also used on Gitaly, this limit is only used on GitLab (hardcoded - not sent to Gitaly).
Gitaly will only return `Diff.Collapsed` (RPC) when surpassing collection limits. Gitaly will only return `Diff.Collapsed` (RPC) when surpassing collection limits.
```ruby #### Not expandable patches (too large)
Gitlab::Git::Diff::SIZE_LIMIT = 100.kilobytes
```
File diff will not be rendered if it's larger than 100 kilobytes.
*Note:* This limit is currently hardcoded and applied on Gitaly and the RPC returns `Diff.TooLarge` when this limit is surpassed. The patch not be rendered if it's larger than `ApplicationSettings#diff_max_patch_bytes`.
Although we're still also applying it on GitLab, we should remove the redundancy from GitLab once we're confident with the Gitaly integration. Users will see a `This source diff could not be displayed because it is too large` message.
```ruby ```ruby
Commit::DIFF_SAFE_LINES = Gitlab::Git::DiffCollection::DEFAULT_LIMITS[:max_lines] = 5000 Commit::DIFF_SAFE_LINES = Gitlab::Git::DiffCollection::DEFAULT_LIMITS[:max_lines] = 5000
......
# Diff limits administration
NOTE: **Note:**
Merge requests and branch comparison views will be affected.
CAUTION: **Caution:**
These settings are currently under experimental state. They'll
increase the resource consumption of your instance and should
be edited mindfully.
1. Access **Admin area > Settings > General**
1. Expand **Diff limits**
### Maximum diff patch size
This is the content size each diff file (patch) is allowed to reach before
it's collapsed, without the possibility of being expanded. A link redirecting
to the blob view will be presented for the patches that surpass this limit.
Patches surpassing 10% of this content size will be automatically collapsed,
but expandable (a link to expand the diff will be presented).
...@@ -19,13 +19,17 @@ module Gitlab ...@@ -19,13 +19,17 @@ module Gitlab
alias_method :expanded?, :expanded alias_method :expanded?, :expanded
SERIALIZE_KEYS = %i(diff new_path old_path a_mode b_mode new_file renamed_file deleted_file too_large).freeze # The default maximum content size to display a diff patch.
#
# If this value ever changes, make sure to create a migration to update
# current records, and default of `ApplicationSettings#diff_max_patch_bytes`.
DEFAULT_MAX_PATCH_BYTES = 100.kilobytes
# The maximum size of a diff to display. # This is a limitation applied on the source (Gitaly), therefore we don't allow
SIZE_LIMIT = 100.kilobytes # persisting limits over that.
MAX_PATCH_BYTES_UPPER_BOUND = 500.kilobytes
# The maximum size before a diff is collapsed. SERIALIZE_KEYS = %i(diff new_path old_path a_mode b_mode new_file renamed_file deleted_file too_large).freeze
COLLAPSE_LIMIT = 10.kilobytes
class << self class << self
def between(repo, head, base, options = {}, *paths) def between(repo, head, base, options = {}, *paths)
...@@ -105,6 +109,26 @@ module Gitlab ...@@ -105,6 +109,26 @@ module Gitlab
def binary_message(old_path, new_path) def binary_message(old_path, new_path)
"Binary files #{old_path} and #{new_path} differ\n" "Binary files #{old_path} and #{new_path} differ\n"
end end
# Returns the limit of bytes a single diff file can reach before it
# appears as 'collapsed' for end-users.
# By convention, it's 10% of the persisted `diff_max_patch_bytes`.
#
# Example: If we have 100k for the `diff_max_patch_bytes`, it will be 10k by
# default.
#
# Patches surpassing this limit should still be persisted in the database.
def patch_safe_limit_bytes
patch_hard_limit_bytes / 10
end
# Returns the limit for a single diff file (patch).
#
# Patches surpassing this limit shouldn't be persisted in the database
# and will be presented as 'too large' for end-users.
def patch_hard_limit_bytes
Gitlab::CurrentSettings.diff_max_patch_bytes
end
end end
def initialize(raw_diff, expanded: true) def initialize(raw_diff, expanded: true)
...@@ -150,7 +174,7 @@ module Gitlab ...@@ -150,7 +174,7 @@ module Gitlab
def too_large? def too_large?
if @too_large.nil? if @too_large.nil?
@too_large = @diff.bytesize >= SIZE_LIMIT @too_large = @diff.bytesize >= self.class.patch_hard_limit_bytes
else else
@too_large @too_large
end end
...@@ -168,7 +192,7 @@ module Gitlab ...@@ -168,7 +192,7 @@ module Gitlab
def collapsed? def collapsed?
return @collapsed if defined?(@collapsed) return @collapsed if defined?(@collapsed)
@collapsed = !expanded && @diff.bytesize >= COLLAPSE_LIMIT @collapsed = !expanded && @diff.bytesize >= self.class.patch_safe_limit_bytes
end end
def collapse! def collapse!
...@@ -219,30 +243,6 @@ module Gitlab ...@@ -219,30 +243,6 @@ module Gitlab
collapse! collapse!
end end
end end
# If the patch surpasses any of the diff limits it calls the appropiate
# prune method and returns true. Otherwise returns false.
def prune_large_patch(patch)
size = 0
patch.each_hunk do |hunk|
hunk.each_line do |line|
size += line.content.bytesize
if size >= SIZE_LIMIT
too_large!
return true # rubocop:disable Cop/AvoidReturnFromBlocks
end
end
end
if !expanded && size >= COLLAPSE_LIMIT
collapse!
return true
end
false
end
end end
end end
end end
...@@ -19,7 +19,7 @@ module Gitlab ...@@ -19,7 +19,7 @@ module Gitlab
limits[:safe_max_files] = [limits[:max_files], DEFAULT_LIMITS[:max_files]].min limits[:safe_max_files] = [limits[:max_files], DEFAULT_LIMITS[:max_files]].min
limits[:safe_max_lines] = [limits[:max_lines], DEFAULT_LIMITS[:max_lines]].min limits[:safe_max_lines] = [limits[:max_lines], DEFAULT_LIMITS[:max_lines]].min
limits[:safe_max_bytes] = limits[:safe_max_files] * 5.kilobytes # Average 5 KB per file limits[:safe_max_bytes] = limits[:safe_max_files] * 5.kilobytes # Average 5 KB per file
limits[:max_patch_bytes] = Gitlab::Git::Diff::SIZE_LIMIT limits[:max_patch_bytes] = Gitlab::Git::Diff.patch_hard_limit_bytes
OpenStruct.new(limits) OpenStruct.new(limits)
end end
......
...@@ -2292,6 +2292,12 @@ msgstr "" ...@@ -2292,6 +2292,12 @@ msgstr ""
msgid "Details" msgid "Details"
msgstr "" msgstr ""
msgid "Diff content limits"
msgstr ""
msgid "Diff limits"
msgstr ""
msgid "Diffs|No file name available" msgid "Diffs|No file name available"
msgstr "" msgstr ""
......
...@@ -592,4 +592,19 @@ describe ApplicationSetting do ...@@ -592,4 +592,19 @@ describe ApplicationSetting do
it { is_expected.to eq(result) } it { is_expected.to eq(result) }
end end
end end
context 'diff limit settings' do
describe '#diff_max_patch_bytes' do
context 'validations' do
it { is_expected.to validate_presence_of(:diff_max_patch_bytes) }
it do
is_expected.to validate_numericality_of(:diff_max_patch_bytes)
.only_integer
.is_greater_than_or_equal_to(Gitlab::Git::Diff::DEFAULT_MAX_PATCH_BYTES)
.is_less_than_or_equal_to(Gitlab::Git::Diff::MAX_PATCH_BYTES_UPPER_BOUND)
end
end
end
end
end end
...@@ -66,7 +66,8 @@ describe API::Settings, 'Settings' do ...@@ -66,7 +66,8 @@ describe API::Settings, 'Settings' do
enforce_terms: true, enforce_terms: true,
terms: 'Hello world!', terms: 'Hello world!',
performance_bar_allowed_group_path: group.full_path, performance_bar_allowed_group_path: group.full_path,
instance_statistics_visibility_private: true instance_statistics_visibility_private: true,
diff_max_patch_bytes: 150_000
expect(response).to have_gitlab_http_status(200) expect(response).to have_gitlab_http_status(200)
expect(json_response['default_projects_limit']).to eq(3) expect(json_response['default_projects_limit']).to eq(3)
...@@ -92,6 +93,7 @@ describe API::Settings, 'Settings' do ...@@ -92,6 +93,7 @@ describe API::Settings, 'Settings' do
expect(json_response['terms']).to eq('Hello world!') expect(json_response['terms']).to eq('Hello world!')
expect(json_response['performance_bar_allowed_group_id']).to eq(group.id) expect(json_response['performance_bar_allowed_group_id']).to eq(group.id)
expect(json_response['instance_statistics_visibility_private']).to be(true) expect(json_response['instance_statistics_visibility_private']).to be(true)
expect(json_response['diff_max_patch_bytes']).to eq(150_000)
end end
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment