Improve `short_sha` uniqueness for routable runner token
This was discovered at !186679 (comment 2437665833) and explained in the next comment.
The current suggestion is:
My suggestion would be decoding the routable token, and extract the random bytes, and use that random bytes to calculate the short sha. This should ensure that it preserves the same uniqueness even when it's not fully guaranteed.
I am filing this as a bug because this is a regression for short_sha that they're less unique being routable.
Once the problem is addressed, we can re-enable routable tokens for runners:
index 8dc898e53478..06b8cb361882 100644
--- a/app/models/ci/runner.rb
+++ b/app/models/ci/runner.rb
@@ -25,8 +25,7 @@ class Runner < Ci::ApplicationRecord
expires_at: :compute_token_expiration,
format_with_prefix: :prefix_for_new_and_legacy_runner,
routable_token: {
- # NOTE: Disabled until we can fix https://gitlab.com/gitlab-org/gitlab/-/issues/534685
- if: ->(_token_owner_record) { false },
+ if: ->(token_owner_record) { token_owner_record.owner },
payload: {
o: ->(token_owner_record) { token_owner_record.owner.try(:organization_id) },
g: ->(token_owner_record) { token_owner_record.group_type? ? token_owner_record.sharding_key_id : nil },
index b6d3e55f8677..00c38d311423 100644
--- a/spec/models/ci/runner_spec.rb
+++ b/spec/models/ci/runner_spec.rb
@@ -1871,6 +1871,27 @@ def does_db_update
it { is_expected.not_to start_with(described_class::REGISTRATION_RUNNER_TOKEN_PREFIX) }
it { is_expected.not_to start_with(described_class::CREATED_RUNNER_TOKEN_PREFIX) }
end
+
+ context 'when many runners are created' do
+ let(:project_runners) { build_list(:ci_runner, 5, :project, projects: [project]) }
+ let(:group_runners) { build_list(:ci_runner, 5, :group, groups: [group]) }
+ let(:instance_runners) { build_list(:ci_runner, 5) }
+ let(:runners) { instance_runners + group_runners + project_runners }
+
+ before do
+ runners.each(&:ensure_token)
+ end
+
+ # NOTE: It is important for runners to have (mostly) unique values, since these are used in same places
+ # as runner identifiers (namely in Prometheus metrics and Docker Machine executor machine identification)
+ # The goal is for this test to pass when no FF exists.
+ it 'does not generate duplicate short_sha values', :aggregate_failures do
+ expect(runners.map(&:short_sha).uniq).to match_array(runners.map(&:short_sha))
+ expect(project_runners.map(&:short_sha).uniq).to match_array(project_runners.map(&:short_sha))
+ expect(group_runners.map(&:short_sha).uniq).to match_array(group_runners.map(&:short_sha))
+ expect(instance_runners.map(&:short_sha).uniq).to match_array(instance_runners.map(&:short_sha))
+ end
+ end
end
it_behaves_like 'TokenAuthenticatable' do
Edited by Pedro Pombeiro