Merge branch '220028-increase-the-value-size-limit-for-instance-level-variables' into 'master'

Increase size limit for instance-level variables

See merge request gitlab-org/gitlab!35063

app/models/ci/instance_variable.rb

@@ -14,12 +14,14 @@ module Ci
     alias_attribute :secret_value, :value
 
     validates :key, uniqueness: {
-      message: "(%{value}) has already been taken"
+      message: -> (object, data) { _("(%{value}) has already been taken") }
     }
 
-    validates :encrypted_value, length: {
-      maximum: 1024,
-      too_long: 'The encrypted value of the provided variable exceeds %{count} bytes. Variables over 700 characters risk exceeding the limit.'
+    validates :value, length: {
+      maximum: 10_000,
+      too_long: -> (object, data) do
+        _('The value of the provided variable exceeds the %{count} character limit')
+      end
     }
 
     scope :unprotected, -> { where(protected: false) }

changelogs/unreleased/220028-increase-the-value-size-limit-for-instance-level-variables.yml

+---
+title: Increase CI instance variable value limit
+merge_request: 35063
+author:
+type: changed

db/migrate/20200625193358_increase_size_on_instance_level_variable_values.rb

+# frozen_string_literal: true
+
+class IncreaseSizeOnInstanceLevelVariableValues < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    existing_constraint_name = text_limit_name(:ci_instance_variables, :encrypted_value)
+    new_constraint_name = check_constraint_name(:ci_instance_variables, :encrypted_value, :char_length_updated)
+
+    add_text_limit(:ci_instance_variables, :encrypted_value, 13_579, constraint_name: new_constraint_name)
+    remove_check_constraint(:ci_instance_variables, existing_constraint_name)
+  end
+
+  def down
+    # no-op
+  end
+end

db/schema_migrations/20200625193358

+e691c94223b44a5866f34aab96cc9a8c5857a6302115f977542bacb36e7c010e
\ No newline at end of file

db/structure.sql

@@ -9990,7 +9990,7 @@ CREATE TABLE public.ci_instance_variables (
     encrypted_value_iv text,
     CONSTRAINT check_07a45a5bcb CHECK ((char_length(encrypted_value_iv) <= 255)),
     CONSTRAINT check_5aede12208 CHECK ((char_length(key) <= 255)),
-    CONSTRAINT check_5ebd0515a0 CHECK ((char_length(encrypted_value) <= 1024))
+    CONSTRAINT check_956afd70f1 CHECK ((char_length(encrypted_value) <= 13579))
 );
 
 CREATE SEQUENCE public.ci_instance_variables_id_seq

doc/api/instance_level_ci_variables.md

@@ -73,7 +73,7 @@ POST /admin/ci/variables
 | Attribute       | Type    | required | Description           |
 |-----------------|---------|----------|-----------------------|
 | `key`           | string  | yes      | The `key` of a variable. Max 255 characters, only `A-Z`, `a-z`, `0-9`, and `_` are allowed. |
-| `value`         | string  | yes      | The `value` of a variable. Around 700 characters allowed. |
+| `value`         | string  | yes      | The `value` of a variable. 10,000 characters allowed. [Since GitLab 13.3] |
 | `variable_type` | string  | no       | The type of a variable. Available types are: `env_var` (default) and `file`. |
 | `protected`     | boolean | no       | Whether the variable is protected. |
 | `masked`        | boolean | no       | Whether the variable is masked. |
@@ -103,7 +103,7 @@ PUT /admin/ci/variables/:key
 | Attribute       | Type    | required | Description             |
 |-----------------|---------|----------|-------------------------|
 | `key`           | string  | yes      | The `key` of a variable.   |
-| `value`         | string  | yes      | The `value` of a variable. |
+| `value`         | string  | yes      | The `value` of a variable. [Since GitLab 13.3](https://gitlab.com/gitlab-org/gitlab/-/issues/220028), around 10,000 characters allowed. Previously 700 characters. |
 | `variable_type` | string  | no       | The type of a variable. Available types are: `env_var` (default) and `file`. |
 | `protected`     | boolean | no       | Whether the variable is protected. |
 | `masked`        | boolean | no       | Whether the variable is masked. |

doc/ci/variables/README.md

@@ -445,7 +445,7 @@ To add an instance-level variable:
 1. Click the **Add variable** button, and fill in the details:
 
    - **Key**: Must be one line, using only letters, numbers, or `_` (underscore), with no spaces.
-    - **Value**: 700 characters allowed.
+   - **Value**: [Since GitLab 13.3](https://gitlab.com/gitlab-org/gitlab/-/issues/220028), 10,000 characters allowed. This is also bounded by the limits of the selected Runner operating system. In GitLab 13.0 to 13.2, 700 characters allowed.
    - **Type**: `File` or `Variable`.
    - **Protect variable** (Optional): If selected, the variable will only be available in pipelines that run on protected branches or tags.
    - **Mask variable** (Optional): If selected, the variable's **Value** will not be shown in job logs. The variable will not be saved if the value does not meet the [masking requirements](#masked-variable-requirements).

locale/gitlab.pot

@@ -794,6 +794,9 @@ msgstr[1] ""
 msgid "(%{mrCount} merged)"
 msgstr ""
 
+msgid "(%{value}) has already been taken"
+msgstr ""
+
 msgid "(No changes)"
 msgstr ""
 
@@ -23876,6 +23879,9 @@ msgstr ""
 msgid "The value lying at the midpoint of a series of observed values. E.g., between 3, 5, 9, the median is 5. Between 3, 5, 7, 8, the median is (5+7)/2 = 6."
 msgstr ""
 
+msgid "The value of the provided variable exceeds the %{count} character limit"
+msgstr ""
+
 msgid "The vulnerability is no longer detected. Verify the vulnerability has been fixed or removed before changing its status."
 msgstr ""
 

spec/models/ci/instance_variable_spec.rb

@@ -9,12 +9,39 @@ RSpec.describe Ci::InstanceVariable do
 
   it { is_expected.to include_module(Ci::Maskable) }
   it { is_expected.to validate_uniqueness_of(:key).with_message(/\(\w+\) has already been taken/) }
-  it { is_expected.to validate_length_of(:encrypted_value).is_at_most(1024).with_message(/Variables over 700 characters risk exceeding the limit/) }
+  it { is_expected.to validate_length_of(:value).is_at_most(10_000).with_message(/The value of the provided variable exceeds the 10000 character limit/) }
 
   it_behaves_like 'includes Limitable concern' do
     subject { build(:ci_instance_variable) }
   end
 
+  describe '#value' do
+    context 'without application limit' do
+      # Ensures breakage if encryption algorithm changes
+      let(:variable) { build(:ci_instance_variable, key: 'too_long', value: value) }
+
+      before do
+        allow(variable).to receive(:valid?).and_return(true)
+      end
+
+      context 'when value is over the limit' do
+        let(:value) { SecureRandom.alphanumeric(10_002) }
+
+        it 'raises a database level error' do
+          expect { variable.save }.to raise_error(ActiveRecord::StatementInvalid)
+        end
+      end
+
+      context 'when value is under the limit' do
+        let(:value) { SecureRandom.alphanumeric(10_000) }
+
+        it 'does not raise database level error' do
+          expect { variable.save }.not_to raise_error
+        end
+      end
+    end
+  end
+
   describe '.unprotected' do
     subject { described_class.unprotected }
 

spec/requests/api/admin/ci/variables_spec.rb

@@ -110,20 +110,19 @@ RSpec.describe ::API::Admin::Ci::Variables do
         expect(response).to have_gitlab_http_status(:bad_request)
       end
 
-      it 'does not allow values above 700 characters' do
+      it 'does not allow values above 10,000 characters' do
         too_long_message = <<~MESSAGE.strip
-          The encrypted value of the provided variable exceeds 1024 bytes. \
-          Variables over 700 characters risk exceeding the limit.
+          The value of the provided variable exceeds the 10000 character limit
         MESSAGE
 
         expect do
           post api('/admin/ci/variables', admin),
-            params: { key: 'too_long', value: SecureRandom.hex(701) }
+            params: { key: 'too_long', value: SecureRandom.hex(10_001) }
         end.not_to change { ::Ci::InstanceVariable.count }
 
         expect(response).to have_gitlab_http_status(:bad_request)
         expect(json_response).to match('message' =>
-          a_hash_including('encrypted_value' => [too_long_message]))
+          a_hash_including('value' => [too_long_message]))
       end
     end