diff options
| -rw-r--r-- | app/controllers/projects/variables_controller.rb | 15 | ||||
| -rw-r--r-- | app/models/ci/build.rb | 21 | ||||
| -rw-r--r-- | app/models/project.rb | 3 | ||||
| -rw-r--r-- | doc/ci/variables/README.md | 11 | ||||
| -rw-r--r-- | doc/downgrade_ee_to_ce/README.md | 13 | ||||
| -rw-r--r-- | lib/api/variables.rb | 8 | ||||
| -rw-r--r-- | lib/gitlab/regex.rb | 6 | ||||
| -rw-r--r-- | lib/gitlab/sql/glob.rb | 22 | ||||
| -rw-r--r-- | spec/lib/gitlab/sql/glob_spec.rb | 53 | ||||
| -rw-r--r-- | spec/models/ci/build_spec.rb | 7 | ||||
| -rw-r--r-- | spec/models/ci/variable_spec.rb | 4 | ||||
| -rw-r--r-- | spec/models/project_spec.rb | 12 |
12 files changed, 140 insertions, 35 deletions
diff --git a/app/controllers/projects/variables_controller.rb b/app/controllers/projects/variables_controller.rb index 573d1c05622..326d31ecec2 100644 --- a/app/controllers/projects/variables_controller.rb +++ b/app/controllers/projects/variables_controller.rb @@ -14,7 +14,7 @@ class Projects::VariablesController < Projects::ApplicationController def update @variable = @project.variables.find(params[:id]) - if @variable.update_attributes(project_params) + if @variable.update_attributes(variable_params) redirect_to project_variables_path(project), notice: 'Variable was successfully updated.' else render action: "show" @@ -22,9 +22,9 @@ class Projects::VariablesController < Projects::ApplicationController end def create - @variable = Ci::Variable.new(project_params) + @variable = @project.variables.new(variable_params) - if @variable.valid? && @project.variables << @variable + if @variable.save flash[:notice] = 'Variables were successfully updated.' redirect_to project_settings_ci_cd_path(project) else @@ -43,8 +43,11 @@ class Projects::VariablesController < Projects::ApplicationController private - def project_params - params.require(:variable) - .permit([:id, :key, :value, :protected, :_destroy]) + def variable_params + params.require(:variable).permit(*variable_params_attributes) + end + + def variable_params_attributes + %i[id key value protected _destroy] end end diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 2e7a80d308b..ef92f990032 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -186,6 +186,12 @@ module Ci # Variables whose value does not depend on environment def simple_variables + variables(environment: nil) + end + + # All variables, including those dependent on environment, which could + # contain unexpanded variables. + def variables(environment: persisted_environment) variables = predefined_variables variables += project.predefined_variables variables += pipeline.predefined_variables @@ -194,15 +200,11 @@ module Ci variables += project.deployment_variables if has_environment? variables += yaml_variables variables += user_variables - variables += project.secret_variables_for(ref).map(&:to_runner_variable) + variables += secret_variables(environment: environment) variables += trigger_request.user_variables if trigger_request - variables - end + variables += persisted_environment_variables if environment - # All variables, including those dependent on environment, which could - # contain unexpanded variables. - def variables - simple_variables.concat(persisted_environment_variables) + variables end def merge_request @@ -370,6 +372,11 @@ module Ci ] end + def secret_variables(environment: persisted_environment) + project.secret_variables_for(ref: ref, environment: environment) + .map(&:to_runner_variable) + end + def steps [Gitlab::Ci::Build::Step.from_commands(self), Gitlab::Ci::Build::Step.from_after_script(self)].compact diff --git a/app/models/project.rb b/app/models/project.rb index 3a5a01db518..6b2b4cf56cb 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1326,7 +1326,8 @@ class Project < ActiveRecord::Base variables end - def secret_variables_for(ref) + def secret_variables_for(ref:, environment: nil) + # EE would use the environment if protected_for?(ref) variables else diff --git a/doc/ci/variables/README.md b/doc/ci/variables/README.md index ee23ac0adbe..3501aae75ec 100644 --- a/doc/ci/variables/README.md +++ b/doc/ci/variables/README.md @@ -160,7 +160,7 @@ Secret variables can be added by going to your project's Once you set them, they will be available for all subsequent pipelines. -## Protected secret variables +### Protected secret variables >**Notes:** This feature requires GitLab 9.3 or higher. @@ -426,10 +426,11 @@ export CI_REGISTRY_PASSWORD="longalfanumstring" ``` [ce-13784]: https://gitlab.com/gitlab-org/gitlab-ce/issues/13784 -[runner]: https://docs.gitlab.com/runner/ -[triggered]: ../triggers/README.md -[triggers]: ../triggers/README.md#pass-job-variables-to-a-trigger +[eep]: https://about.gitlab.com/gitlab-ee/ "Available only in GitLab Enterprise Edition Premium" +[envs]: ../environments.md [protected branches]: ../../user/project/protected_branches.md [protected tags]: ../../user/project/protected_tags.md +[runner]: https://docs.gitlab.com/runner/ [shellexecutors]: https://docs.gitlab.com/runner/executors/ -[eep]: https://about.gitlab.com/gitlab-ee/ "Available only in GitLab Enterprise Edition Premium" +[triggered]: ../triggers/README.md +[triggers]: ../triggers/README.md#pass-job-variables-to-a-trigger diff --git a/doc/downgrade_ee_to_ce/README.md b/doc/downgrade_ee_to_ce/README.md index fe4b6d73771..75bae324585 100644 --- a/doc/downgrade_ee_to_ce/README.md +++ b/doc/downgrade_ee_to_ce/README.md @@ -46,6 +46,19 @@ $ sudo gitlab-rails runner "Service.where(type: ['JenkinsService', 'JenkinsDepre $ bundle exec rails runner "Service.where(type: ['JenkinsService', 'JenkinsDeprecatedService']).delete_all" production ``` +### Secret variables environment scopes + +If you're using this feature and there are variables sharing the same +key, but they have different scopes in a project, then you might want to +revisit the environment scope setting for those variables. + +In CE, environment scopes are completely ignored, therefore you could +accidentally get a variable which you're not expecting for a particular +environment. Make sure that you have the right variables in this case. + +Data is completely preserved, so you could always upgrade back to EE and +restore the behavior if you leave it alone. + ## Downgrade to CE After performing the above mentioned steps, you are now ready to downgrade your diff --git a/lib/api/variables.rb b/lib/api/variables.rb index 10374995497..7fa528fb2d3 100644 --- a/lib/api/variables.rb +++ b/lib/api/variables.rb @@ -45,7 +45,9 @@ module API optional :protected, type: String, desc: 'Whether the variable is protected' end post ':id/variables' do - variable = user_project.variables.create(declared_params(include_missing: false)) + variable_params = declared_params(include_missing: false) + + variable = user_project.variables.create(variable_params) if variable.valid? present variable, with: Entities::Variable @@ -67,7 +69,9 @@ module API return not_found!('Variable') unless variable - if variable.update(declared_params(include_missing: false).except(:key)) + variable_params = declared_params(include_missing: false).except(:key) + + if variable.update(variable_params) present variable, with: Entities::Variable else render_validation_error!(variable) diff --git a/lib/gitlab/regex.rb b/lib/gitlab/regex.rb index 057f32eaef7..c1ee20b6977 100644 --- a/lib/gitlab/regex.rb +++ b/lib/gitlab/regex.rb @@ -30,8 +30,12 @@ module Gitlab @container_repository_regex ||= %r{\A[a-z0-9]+(?:[-._/][a-z0-9]+)*\Z} end + def environment_name_regex_chars + 'a-zA-Z0-9_/\\$\\{\\}\\. -' + end + def environment_name_regex - @environment_name_regex ||= /\A[a-zA-Z0-9_\\\/\${}. -]+\z/.freeze + @environment_name_regex ||= /\A[#{environment_name_regex_chars}]+\z/.freeze end def environment_name_regex_message diff --git a/lib/gitlab/sql/glob.rb b/lib/gitlab/sql/glob.rb new file mode 100644 index 00000000000..5e89e12b2b1 --- /dev/null +++ b/lib/gitlab/sql/glob.rb @@ -0,0 +1,22 @@ +module Gitlab + module SQL + module Glob + extend self + + # Convert a simple glob pattern with wildcard (*) to SQL LIKE pattern + # with SQL expression + def to_like(pattern) + <<~SQL + REPLACE(REPLACE(REPLACE(#{pattern}, + #{q('%')}, #{q('\\%')}), + #{q('_')}, #{q('\\_')}), + #{q('*')}, #{q('%')}) + SQL + end + + def q(string) + ActiveRecord::Base.connection.quote(string) + end + end + end +end diff --git a/spec/lib/gitlab/sql/glob_spec.rb b/spec/lib/gitlab/sql/glob_spec.rb new file mode 100644 index 00000000000..451c583310d --- /dev/null +++ b/spec/lib/gitlab/sql/glob_spec.rb @@ -0,0 +1,53 @@ +require 'spec_helper' + +describe Gitlab::SQL::Glob, lib: true do + describe '.to_like' do + it 'matches * as %' do + expect(glob('apple', '*')).to be(true) + expect(glob('apple', 'app*')).to be(true) + expect(glob('apple', 'apple*')).to be(true) + expect(glob('apple', '*pple')).to be(true) + expect(glob('apple', 'ap*le')).to be(true) + + expect(glob('apple', '*a')).to be(false) + expect(glob('apple', 'app*a')).to be(false) + expect(glob('apple', 'ap*l')).to be(false) + end + + it 'matches % literally' do + expect(glob('100%', '100%')).to be(true) + + expect(glob('100%', '%')).to be(false) + end + + it 'matches _ literally' do + expect(glob('^_^', '^_^')).to be(true) + + expect(glob('^A^', '^_^')).to be(false) + end + end + + def glob(string, pattern) + match(string, subject.to_like(quote(pattern))) + end + + def match(string, pattern) + value = query("SELECT #{quote(string)} LIKE #{pattern}") + .rows.flatten.first + + case value + when 't', 1 + true + else + false + end + end + + def query(sql) + ActiveRecord::Base.connection.select_all(sql) + end + + def quote(string) + ActiveRecord::Base.connection.quote(string) + end +end diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index a7ba3a7c43e..2b10791ad6d 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -1496,9 +1496,10 @@ describe Ci::Build, :models do allow(pipeline).to receive(:predefined_variables) { [pipeline_pre_var] } allow(build).to receive(:yaml_variables) { [build_yaml_var] } - allow(project).to receive(:secret_variables_for).with(build.ref) do - [create(:ci_variable, key: 'secret', value: 'value')] - end + allow(project).to receive(:secret_variables_for) + .with(ref: 'master', environment: nil) do + [create(:ci_variable, key: 'secret', value: 'value')] + end end it do diff --git a/spec/models/ci/variable_spec.rb b/spec/models/ci/variable_spec.rb index 50f7c029af8..4ffbfa6c130 100644 --- a/spec/models/ci/variable_spec.rb +++ b/spec/models/ci/variable_spec.rb @@ -8,10 +8,6 @@ describe Ci::Variable, models: true do describe 'validations' do it { is_expected.to include_module(HasVariable) } it { is_expected.to validate_uniqueness_of(:key).scoped_to(:project_id, :environment_scope) } - it { is_expected.to validate_length_of(:key).is_at_most(255) } - it { is_expected.to allow_value('foo').for(:key) } - it { is_expected.not_to allow_value('foo bar').for(:key) } - it { is_expected.not_to allow_value('foo/bar').for(:key) } end describe '.unprotected' do diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index f50b4aea411..a9855cf343b 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -1875,7 +1875,12 @@ describe Project, models: true do create(:ci_variable, :protected, value: 'protected', project: project) end - subject { project.secret_variables_for('ref') } + subject { project.secret_variables_for(ref: 'ref') } + + before do + stub_application_setting( + default_branch_protection: Gitlab::Access::PROTECTION_NONE) + end shared_examples 'ref is protected' do it 'contains all the variables' do @@ -1884,11 +1889,6 @@ describe Project, models: true do end context 'when the ref is not protected' do - before do - stub_application_setting( - default_branch_protection: Gitlab::Access::PROTECTION_NONE) - end - it 'contains only the secret variables' do is_expected.to contain_exactly(secret_variable) end |