From c5f76e37700b02ca92beb32b4af1a265c617b115 Mon Sep 17 00:00:00 2001 From: Maxime Orefice Date: Tue, 6 Dec 2022 14:44:15 +0100 Subject: [PATCH 1/4] Prefix partition_id to Ci::Build#token Ensure unique constraint is enforce accross all future partitions. --- app/models/ci/build.rb | 11 +++++++- .../ci_build_partition_id_token_prefix.yml | 8 ++++++ spec/models/ci/build_spec.rb | 25 +++++++++++++++++++ 3 files changed, 43 insertions(+), 1 deletion(-) create mode 100644 config/feature_flags/development/ci_build_partition_id_token_prefix.yml diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 34b5b637422fb4..c1cc313ef986d5 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -170,7 +170,7 @@ class Build < Ci::Processable acts_as_taggable - add_authentication_token_field :token, encrypted: :required + add_authentication_token_field :token, encrypted: :required, prefix: ->(instance) { instance.partition_id_token_prefix } after_save :stick_build_if_status_changed @@ -1135,6 +1135,15 @@ def test_suite_name end end + def partition_id_token_prefix + Feature.enabled?(:ci_build_partition_id_token_prefix, project) ? partition_id : '' + end + + override :format_token + def format_token(token) + "#{partition_id_token_prefix}#{token}" + end + protected def run_status_commit_hooks! diff --git a/config/feature_flags/development/ci_build_partition_id_token_prefix.yml b/config/feature_flags/development/ci_build_partition_id_token_prefix.yml new file mode 100644 index 00000000000000..5e46ee2623bd92 --- /dev/null +++ b/config/feature_flags/development/ci_build_partition_id_token_prefix.yml @@ -0,0 +1,8 @@ +--- +name: ci_build_partition_id_token_prefix +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/106179 +rollout_issue_url: +milestone: '15.7' +type: development +group: group::pipeline execution +default_enabled: false diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 5f1937d078c12b..234447d8113d53 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -5643,4 +5643,29 @@ def run_job_without_exception expect(ci_build.job_variables.second.partition_id).to eq(ci_testing_partition_id) end end + + describe 'assigning token' do + include Ci::PartitioningHelpers + + let(:new_pipeline) { create(:ci_pipeline) } + let(:ci_build) { create(:ci_build, pipeline: new_pipeline) } + + before do + stub_current_partition_id + end + + it 'includes partition_id as a token prefix' do + expect(ci_build.token).to start_with(ci_testing_partition_id.to_s) + end + + context 'when ci_build_partition_id_token_prefix is disabled' do + before do + stub_feature_flags(ci_build_partition_id_token_prefix: false) + end + + it 'does not include partition_id as a token prefix' do + expect(ci_build).not_to start_with(ci_testing_partition_id.to_s) + end + end + end end -- GitLab From c9794d45c2838e8d4f005b9d3f9982af8818e6af Mon Sep 17 00:00:00 2001 From: Maxime Orefice Date: Mon, 12 Dec 2022 12:42:21 +0100 Subject: [PATCH 2/4] Convert prefix in hex and use delimiter --- app/models/ci/build.rb | 16 +++++++++++++--- .../ci_build_partition_id_token_prefix.yml | 2 +- .../ci_build_partition_id_token_prefix_check.yml | 8 ++++++++ spec/models/ci/build_spec.rb | 10 +++++++--- 4 files changed, 29 insertions(+), 7 deletions(-) create mode 100644 config/feature_flags/development/ci_build_partition_id_token_prefix_check.yml diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index c1cc313ef986d5..f576e293b45c1f 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -170,7 +170,15 @@ class Build < Ci::Processable acts_as_taggable - add_authentication_token_field :token, encrypted: :required, prefix: ->(instance) { instance.partition_id_token_prefix } + add_authentication_token_field :token, + encrypted: :required, + prefix: -> (instance) { + if Feature.enabled?(:ci_build_partition_id_token_prefix_check, instance.project) + instance.partition_id_token_prefix + else + '' + end + } after_save :stick_build_if_status_changed @@ -1136,12 +1144,14 @@ def test_suite_name end def partition_id_token_prefix - Feature.enabled?(:ci_build_partition_id_token_prefix, project) ? partition_id : '' + Feature.enabled?(:ci_build_partition_id_token_prefix, project) ? partition_id.to_s(16) : '' end override :format_token def format_token(token) - "#{partition_id_token_prefix}#{token}" + return token if partition_id_token_prefix.blank? + + "#{partition_id_token_prefix}_#{token}" end protected diff --git a/config/feature_flags/development/ci_build_partition_id_token_prefix.yml b/config/feature_flags/development/ci_build_partition_id_token_prefix.yml index 5e46ee2623bd92..5b3cd22a489bbd 100644 --- a/config/feature_flags/development/ci_build_partition_id_token_prefix.yml +++ b/config/feature_flags/development/ci_build_partition_id_token_prefix.yml @@ -1,7 +1,7 @@ --- name: ci_build_partition_id_token_prefix introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/106179 -rollout_issue_url: +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/385401 milestone: '15.7' type: development group: group::pipeline execution diff --git a/config/feature_flags/development/ci_build_partition_id_token_prefix_check.yml b/config/feature_flags/development/ci_build_partition_id_token_prefix_check.yml new file mode 100644 index 00000000000000..1e66bea0d8e8d3 --- /dev/null +++ b/config/feature_flags/development/ci_build_partition_id_token_prefix_check.yml @@ -0,0 +1,8 @@ +--- +name: ci_build_partition_id_token_prefix_check +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/106179 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/385411 +milestone: '15.7' +type: development +group: group::pipeline execution +default_enabled: false diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 234447d8113d53..5fbfe9b3bd6fc9 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -5644,7 +5644,7 @@ def run_job_without_exception end end - describe 'assigning token' do + describe 'assigning token', :ci_partitionable do include Ci::PartitioningHelpers let(:new_pipeline) { create(:ci_pipeline) } @@ -5655,7 +5655,9 @@ def run_job_without_exception end it 'includes partition_id as a token prefix' do - expect(ci_build.token).to start_with(ci_testing_partition_id.to_s) + prefix = ci_build.token.split('_').first.to_i(16) + + expect(prefix).to eq(ci_testing_partition_id) end context 'when ci_build_partition_id_token_prefix is disabled' do @@ -5664,7 +5666,9 @@ def run_job_without_exception end it 'does not include partition_id as a token prefix' do - expect(ci_build).not_to start_with(ci_testing_partition_id.to_s) + prefix = ci_build.token.split('_').first.to_i(16) + + expect(prefix).not_to eq(ci_testing_partition_id) end end end -- GitLab From 6155072a98394155a5745faffca377305be4bb2e Mon Sep 17 00:00:00 2001 From: Maxime Orefice Date: Tue, 13 Dec 2022 09:39:58 +0100 Subject: [PATCH 3/4] Apply code review feedback --- app/models/ci/build.rb | 10 +--------- .../ci_build_partition_id_token_prefix_check.yml | 8 -------- 2 files changed, 1 insertion(+), 17 deletions(-) delete mode 100644 config/feature_flags/development/ci_build_partition_id_token_prefix_check.yml diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index f576e293b45c1f..7c34260f4fd932 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -170,15 +170,7 @@ class Build < Ci::Processable acts_as_taggable - add_authentication_token_field :token, - encrypted: :required, - prefix: -> (instance) { - if Feature.enabled?(:ci_build_partition_id_token_prefix_check, instance.project) - instance.partition_id_token_prefix - else - '' - end - } + add_authentication_token_field :token, encrypted: :required after_save :stick_build_if_status_changed diff --git a/config/feature_flags/development/ci_build_partition_id_token_prefix_check.yml b/config/feature_flags/development/ci_build_partition_id_token_prefix_check.yml deleted file mode 100644 index 1e66bea0d8e8d3..00000000000000 --- a/config/feature_flags/development/ci_build_partition_id_token_prefix_check.yml +++ /dev/null @@ -1,8 +0,0 @@ ---- -name: ci_build_partition_id_token_prefix_check -introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/106179 -rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/385411 -milestone: '15.7' -type: development -group: group::pipeline execution -default_enabled: false -- GitLab From 00880eab9fa0bc6342127ebb60d29d2174ec0928 Mon Sep 17 00:00:00 2001 From: Maxime Orefice Date: Tue, 13 Dec 2022 16:12:30 +0100 Subject: [PATCH 4/4] Apply code review feedback --- app/models/ci/build.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 7c34260f4fd932..7f42b21bc8722f 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -1136,12 +1136,12 @@ def test_suite_name end def partition_id_token_prefix - Feature.enabled?(:ci_build_partition_id_token_prefix, project) ? partition_id.to_s(16) : '' + partition_id.to_s(16) if Feature.enabled?(:ci_build_partition_id_token_prefix, project) end override :format_token def format_token(token) - return token if partition_id_token_prefix.blank? + return token if partition_id_token_prefix.nil? "#{partition_id_token_prefix}_#{token}" end -- GitLab