Merge branch 'add-client-id-application-context-metadata' into 'master'

Add client_id to Application Context

See merge request gitlab-org/gitlab!55683

Gemfile

@@ -313,7 +313,7 @@ gem 'pg_query', '~> 1.3.0'
 gem 'premailer-rails', '~> 1.10.3'
 
 # LabKit: Tracing and Correlation
-gem 'gitlab-labkit', '~> 0.16.0'
+gem 'gitlab-labkit', '~> 0.16.1'
 # Thrift is a dependency of gitlab-labkit, we want a version higher than 0.14.0
 # because of https://gitlab.com/gitlab-org/gitlab/-/issues/321900
 gem 'thrift', '>= 0.14.0'

Gemfile.lock

@@ -447,7 +447,7 @@ GEM
       fog-xml (~> 0.1.0)
       google-api-client (>= 0.44.2, < 0.51)
       google-cloud-env (~> 1.2)
-    gitlab-labkit (0.16.0)
+    gitlab-labkit (0.16.1)
       actionpack (>= 5.0.0, < 7.0.0)
       activesupport (>= 5.0.0, < 7.0.0)
       grpc (~> 1.19)
@@ -1416,7 +1416,7 @@ DEPENDENCIES
   gitlab-experiment (~> 0.5.0)
   gitlab-fog-azure-rm (~> 1.0.1)
   gitlab-fog-google (~> 1.13)
-  gitlab-labkit (~> 0.16.0)
+  gitlab-labkit (~> 0.16.1)
   gitlab-license (~> 1.3)
   gitlab-mail_room (~> 0.0.8)
   gitlab-markup (~> 1.7.1)

changelogs/unreleased/add-client-id-application-context-metadata.yml

+---
+title: Add client_id to application context
+merge_request: 55683
+author:
+type: added

ee/spec/lib/ee/gitlab/application_context_spec.rb

@@ -43,10 +43,12 @@ RSpec.describe Gitlab::ApplicationContext do
     using RSpec::Parameterized::TableSyntax
 
     where(:provided_options, :expected_context_keys) do
-      [:user, :namespace, :project] | [:user, :project, :root_namespace, :subscription_plan]
-      [:user, :project]             | [:user, :project, :root_namespace, :subscription_plan]
-      [:user, :namespace]           | [:user, :root_namespace, :subscription_plan]
-      [:user]                       | [:user]
+      [:user, :namespace, :project] | [:user, :project, :root_namespace, :client_id, :subscription_plan]
+      [:user, :project]             | [:user, :project, :root_namespace, :client_id, :subscription_plan]
+      [:user, :namespace]           | [:user, :root_namespace, :client_id, :subscription_plan]
+      [:user]                       | [:user, :client_id]
+      [:remote_ip]                  | [:remote_ip, :client_id]
+      [:runner]                     | [:project, :root_namespace, :client_id, :subscription_plan]
       [:caller_id]                  | [:caller_id]
       []                            | []
     end

lib/api/api.rb

@@ -58,6 +58,7 @@ module API
         user: -> { @current_user },
         project: -> { @project },
         namespace: -> { @group },
+        runner: -> { @current_runner || @runner },
         caller_id: route.origin,
         remote_ip: request.ip,
         feature_category: feature_category

lib/api/ci/runner.rb

@@ -44,12 +44,12 @@ module API
               forbidden!
             end
 
-          runner = ::Ci::Runner.create(attributes)
+          @runner = ::Ci::Runner.create(attributes)
 
-          if runner.persisted?
-            present runner, with: Entities::RunnerRegistrationDetails
+          if @runner.persisted?
+            present @runner, with: Entities::RunnerRegistrationDetails
           else
-            render_validation_error!(runner)
+            render_validation_error!(@runner)
           end
         end
 
@@ -62,9 +62,7 @@ module API
         delete '/' do
           authenticate_runner!
 
-          runner = ::Ci::Runner.find_by_token(params[:token])
-
-          destroy_conditionally!(runner)
+          destroy_conditionally!(current_runner)
         end
 
         desc 'Validates authentication credentials' do

lib/api/helpers/runner.rb

@@ -73,23 +73,12 @@ module API
       end
 
       def set_application_context
-        if current_job
-          Gitlab::ApplicationContext.push(
-            user: -> { current_job.user },
-            project: -> { current_job.project }
-          )
-        elsif current_runner&.project_type?
-          Gitlab::ApplicationContext.push(
-            project: -> do
-              projects = current_runner.projects.limit(2) # rubocop: disable CodeReuse/ActiveRecord
-              projects.first if projects.length == 1
-            end
-          )
-        elsif current_runner&.group_type?
-          Gitlab::ApplicationContext.push(
-            namespace: -> { current_runner.groups.first }
-          )
-        end
+        return unless current_job
+
+        Gitlab::ApplicationContext.push(
+          user: -> { current_job.user },
+          project: -> { current_job.project }
+        )
       end
     end
   end

lib/gitlab/application_context.rb

@@ -4,6 +4,7 @@ module Gitlab
   # A GitLab-rails specific accessor for `Labkit::Logging::ApplicationContext`
   class ApplicationContext
     include Gitlab::Utils::LazyAttributes
+    include Gitlab::Utils::StrongMemoize
 
     Attribute = Struct.new(:name, :type)
 
@@ -11,6 +12,7 @@ module Gitlab
       Attribute.new(:project, Project),
       Attribute.new(:namespace, Namespace),
       Attribute.new(:user, User),
+      Attribute.new(:runner, ::Ci::Runner),
       Attribute.new(:caller_id, String),
       Attribute.new(:remote_ip, String),
       Attribute.new(:related_class, String),
@@ -47,8 +49,9 @@ module Gitlab
     def to_lazy_hash
       {}.tap do |hash|
         hash[:user] = -> { username } if set_values.include?(:user)
-        hash[:project] = -> { project_path } if set_values.include?(:project)
+        hash[:project] = -> { project_path } if set_values.include?(:project) || set_values.include?(:runner)
         hash[:root_namespace] = -> { root_namespace_path } if include_namespace?
+        hash[:client_id] = -> { client } if include_client?
         hash[:caller_id] = caller_id if set_values.include?(:caller_id)
         hash[:remote_ip] = remote_ip if set_values.include?(:remote_ip)
         hash[:related_class] = related_class if set_values.include?(:related_class)
@@ -75,7 +78,8 @@ module Gitlab
     end
 
     def project_path
-      project&.full_path
+      associated_routable = project || runner_project
+      associated_routable&.full_path
     end
 
     def username
@@ -83,15 +87,43 @@ module Gitlab
     end
 
     def root_namespace_path
-      if namespace
-        namespace.full_path_components.first
+      associated_routable = namespace || project || runner_project || runner_group
+      associated_routable&.full_path_components&.first
+    end
+
+    def include_namespace?
+      set_values.include?(:namespace) || set_values.include?(:project) || set_values.include?(:runner)
+    end
+
+    def include_client?
+      set_values.include?(:user) || set_values.include?(:runner) || set_values.include?(:remote_ip)
+    end
+
+    def client
+      if user
+        "user/#{user.id}"
+      elsif runner
+        "runner/#{runner.id}"
       else
-        project&.full_path_components&.first
+        "ip/#{remote_ip}"
       end
     end
 
-    def include_namespace?
-      set_values.include?(:namespace) || set_values.include?(:project)
+    def runner_project
+      strong_memoize(:runner_project) do
+        next unless runner&.project_type?
+
+        projects = runner.projects.take(2) # rubocop: disable CodeReuse/ActiveRecord
+        projects.first if projects.one?
+      end
+    end
+
+    def runner_group
+      strong_memoize(:runner_group) do
+        next unless runner&.group_type?
+
+        runner.groups.first
+      end
     end
   end
 end

spec/lib/gitlab/application_context_spec.rb

@@ -30,7 +30,7 @@ RSpec.describe Gitlab::ApplicationContext do
   describe '.push' do
     it 'passes the expected context on to labkit' do
       fake_proc = duck_type(:call)
-      expected_context = { user: fake_proc }
+      expected_context = { user: fake_proc, client_id: fake_proc }
 
       expect(Labkit::Context).to receive(:push).with(expected_context)
 
@@ -92,6 +92,34 @@ RSpec.describe Gitlab::ApplicationContext do
       expect(result(context))
         .to include(project: project.full_path, root_namespace: project.full_path_components.first)
     end
+
+    describe 'setting the client' do
+      let_it_be(:remote_ip) { '127.0.0.1' }
+      let_it_be(:runner) { create(:ci_runner) }
+      let_it_be(:options) { { remote_ip: remote_ip, runner: runner, user: user } }
+
+      using RSpec::Parameterized::TableSyntax
+
+      where(:provided_options, :client) do
+        [:remote_ip]                 | :remote_ip
+        [:remote_ip, :runner]        | :runner
+        [:remote_ip, :runner, :user] | :user
+      end
+
+      with_them do
+        it 'sets the client_id to the expected value' do
+          context = described_class.new(**options.slice(*provided_options))
+
+          client_id = case client
+                      when :remote_ip then "ip/#{remote_ip}"
+                      when :runner then "runner/#{runner.id}"
+                      when :user then "user/#{user.id}"
+                      end
+
+          expect(result(context)[:client_id]).to eq(client_id)
+        end
+      end
+    end
   end
 
   describe '#use' do

spec/requests/api/api_spec.rb

@@ -112,6 +112,7 @@ RSpec.describe API::API do
                                        'meta.project' => project.full_path,
                                        'meta.root_namespace' => project.namespace.full_path,
                                        'meta.user' => user.username,
+                                       'meta.client_id' => an_instance_of(String),
                                        'meta.feature_category' => 'issue_tracking')
         end
       end
@@ -125,6 +126,7 @@ RSpec.describe API::API do
           expect(log_context).to match('correlation_id' => an_instance_of(String),
                                        'meta.caller_id' => '/api/:version/users',
                                        'meta.remote_ip' => an_instance_of(String),
+                                       'meta.client_id' => an_instance_of(String),
                                        'meta.feature_category' => 'users')
         end
       end

spec/requests/api/ci/runner/jobs_request_post_spec.rb

@@ -806,7 +806,7 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do
             subject { request_job(id: job.id) }
 
             it_behaves_like 'storing arguments in the application context' do
-              let(:expected_params) { { user: user.username, project: project.full_path } }
+              let(:expected_params) { { user: user.username, project: project.full_path, client_id: "user/#{user.id}" } }
             end
 
             it_behaves_like 'not executing any extra queries for the application context', 3 do
@@ -817,7 +817,7 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do
 
           context 'when the runner is of project type' do
             it_behaves_like 'storing arguments in the application context' do
-              let(:expected_params) { { project: project.full_path } }
+              let(:expected_params) { { project: project.full_path, client_id: "runner/#{runner.id}" } }
             end
 
             it_behaves_like 'not executing any extra queries for the application context', 2 do
@@ -831,7 +831,7 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do
             let(:runner) { create(:ci_runner, :group, groups: [group]) }
 
             it_behaves_like 'storing arguments in the application context' do
-              let(:expected_params) { { root_namespace: group.full_path_components.first } }
+              let(:expected_params) { { root_namespace: group.full_path_components.first, client_id: "runner/#{runner.id}" } }
             end
 
             it_behaves_like 'not executing any extra queries for the application context', 2 do

spec/requests/api/ci/runner/runners_delete_spec.rb

@@ -37,8 +37,10 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do
       context 'when valid token is provided' do
         let(:runner) { create(:ci_runner) }
 
+        subject { delete api('/runners'), params: { token: runner.token } }
+
         it 'deletes Runner' do
-          delete api('/runners'), params: { token: runner.token }
+          subject
 
           expect(response).to have_gitlab_http_status(:no_content)
           expect(::Ci::Runner.count).to eq(0)
@@ -48,6 +50,10 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do
           let(:request) { api('/runners') }
           let(:params) { { token: runner.token } }
         end
+
+        it_behaves_like 'storing arguments in the application context' do
+          let(:expected_params) { { client_id: "runner/#{runner.id}" } }
+        end
       end
     end
   end

spec/requests/api/ci/runner/runners_post_spec.rb

@@ -39,18 +39,32 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do
           post api('/runners'), params: { token: token }
         end
 
-        it 'creates runner with default values' do
-          post api('/runners'), params: { token: registration_token }
+        context 'with a registration token' do
+          let(:token) { registration_token }
 
-          runner = ::Ci::Runner.first
+          it 'creates runner with default values' do
+            request
 
-          expect(response).to have_gitlab_http_status(:created)
-          expect(json_response['id']).to eq(runner.id)
-          expect(json_response['token']).to eq(runner.token)
-          expect(runner.run_untagged).to be true
-          expect(runner.active).to be true
-          expect(runner.token).not_to eq(registration_token)
-          expect(runner).to be_instance_type
+            runner = ::Ci::Runner.first
+
+            expect(response).to have_gitlab_http_status(:created)
+            expect(json_response['id']).to eq(runner.id)
+            expect(json_response['token']).to eq(runner.token)
+            expect(runner.run_untagged).to be true
+            expect(runner.active).to be true
+            expect(runner.token).not_to eq(registration_token)
+            expect(runner).to be_instance_type
+          end
+
+          it_behaves_like 'storing arguments in the application context' do
+            subject { request }
+
+            let(:expected_params) { { client_id: "runner/#{::Ci::Runner.first.id}" } }
+          end
+
+          it_behaves_like 'not executing any extra queries for the application context' do
+            let(:subject_proc) { proc { request } }
+          end
         end
 
         context 'when project token is used' do
@@ -71,7 +85,7 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do
           it_behaves_like 'storing arguments in the application context' do
             subject { request }
 
-            let(:expected_params) { { project: project.full_path } }
+            let(:expected_params) { { project: project.full_path, client_id: "runner/#{::Ci::Runner.first.id}" } }
           end
 
           it_behaves_like 'not executing any extra queries for the application context' do
@@ -97,7 +111,7 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do
           it_behaves_like 'storing arguments in the application context' do
             subject { request }
 
-            let(:expected_params) { { root_namespace: group.full_path_components.first } }
+            let(:expected_params) { { root_namespace: group.full_path_components.first, client_id: "runner/#{::Ci::Runner.first.id}" } }
           end
 
           it_behaves_like 'not executing any extra queries for the application context' do

spec/requests/api/ci/runner/runners_verify_post_spec.rb

@@ -37,11 +37,17 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do
       end
 
       context 'when valid token is provided' do
+        subject { post api('/runners/verify'), params: { token: runner.token } }
+
         it 'verifies Runner credentials' do
-          post api('/runners/verify'), params: { token: runner.token }
+          subject
 
           expect(response).to have_gitlab_http_status(:ok)
         end
+
+        it_behaves_like 'storing arguments in the application context' do
+          let(:expected_params) { { client_id: "runner/#{runner.id}" } }
+        end
       end
     end
   end