Automatic merge of gitlab-org/gitlab-ce master

Gemfile

@@ -234,7 +234,7 @@ gem 'asana', '~> 0.8.1'
 gem 'ruby-fogbugz', '~> 0.2.1'
 
 # Kubernetes integration
-gem 'kubeclient', '~> 4.0.0'
+gem 'kubeclient', '~> 4.2.2'
 
 # Sanitize user input
 gem 'sanitize', '~> 4.6'

Gemfile.lock

@@ -453,7 +453,7 @@ GEM
     kgio (2.10.0)
     knapsack (1.17.0)
       rake
-    kubeclient (4.0.0)
+    kubeclient (4.2.2)
       http (~> 3.0)
       recursive-open-struct (~> 1.0, >= 1.0.4)
       rest-client (~> 2.0)
@@ -1090,7 +1090,7 @@ DEPENDENCIES
   jwt (~> 2.1.0)
   kaminari (~> 1.0)
   knapsack (~> 1.17)
-  kubeclient (~> 4.0.0)
+  kubeclient (~> 4.2.2)
   letter_opener_web (~> 1.3.0)
   license_finder (~> 5.4)
   licensee (~> 8.9)

changelogs/unreleased/40997-gitlab-pages-deploy-jobs-have-a-null-status.yml

+---
+title: Fix empty labels of CI builds for gitlab-pages on pipeline page
+merge_request: 24451
+author:
+type: fixed

changelogs/unreleased/54250-upstream-kubeclient-redirect-patch.yml

+---
+title: Upgrade kubeclient to 4.2.2 and swap out monkey-patch to disallow redirects
+merge_request: 24284
+author:
+type: other

changelogs/unreleased/an-opentracing-active-record-tracing.yml

+---
+title: Adds tracing support for ActiveRecord notifications
+merge_request: 24604
+author:
+type: other

config/initializers/kubeclient.rb

-class Kubeclient::Client
-  # Monkey patch to set `max_redirects: 0`, so that kubeclient
-  # does not follow redirects and expose internal services.
-  # See https://gitlab.com/gitlab-org/gitlab-ce/issues/53158
-  def create_rest_client(path = nil)
-    path ||= @api_endpoint.path
-    options = {
-      ssl_ca_file: @ssl_options[:ca_file],
-      ssl_cert_store: @ssl_options[:cert_store],
-      verify_ssl: @ssl_options[:verify_ssl],
-      ssl_client_cert: @ssl_options[:client_cert],
-      ssl_client_key: @ssl_options[:client_key],
-      proxy: @http_proxy_uri,
-      user: @auth_options[:username],
-      password: @auth_options[:password],
-      open_timeout: @timeouts[:open],
-      read_timeout: @timeouts[:read],
-      max_redirects: 0
-    }
-    RestClient::Resource.new(@api_endpoint.merge(path).to_s, options)
-  end
-end

config/initializers/tracing.rb

@@ -23,6 +23,9 @@ if Gitlab::Tracing.enabled?
     end
   end
 
+  # Instrument Rails
+  Gitlab::Tracing::Rails::ActiveRecordSubscriber.instrument
+
   # In multi-processed clustered architectures (puma, unicorn) don't
   # start tracing until the worker processes are spawned. This works
   # around issues when the opentracing implementation spawns threads

lib/gitlab/ci/status/external/common.rb

@@ -6,7 +6,7 @@ module Gitlab
       module External
         module Common
           def label
-            subject.description
+            subject.description.presence || super
           end
 
           def has_details?

lib/gitlab/kubernetes/kube_client.rb

@@ -76,9 +76,12 @@ module Gitlab
 
       attr_reader :api_prefix, :kubeclient_options
 
+      # We disable redirects through 'http_max_redirects: 0',
+      # so that KubeClient does not follow redirects and
+      # expose internal services.
       def initialize(api_prefix, **kubeclient_options)
         @api_prefix = api_prefix
-        @kubeclient_options = kubeclient_options
+        @kubeclient_options = kubeclient_options.merge(http_max_redirects: 0)
       end
 
       def create_or_update_cluster_role_binding(resource)

lib/gitlab/tracing/common.rb

 # frozen_string_literal: true
 
+require 'opentracing'
+
 module Gitlab
   module Tracing
     module Common
@@ -32,6 +34,14 @@ module Gitlab
         end
       end
 
+      def postnotify_span(operation_name, start_time, end_time, tags: nil, child_of: nil, exception: nil)
+        span = OpenTracing.start_span(operation_name, start_time: start_time, tags: tags, child_of: child_of)
+
+        log_exception_on_span(span, exception) if exception
+
+        span.finish(end_time: end_time)
+      end
+
       def log_exception_on_span(span, exception)
         span.set_tag('error', true)
         span.log_kv(kv_tags_for_exception(exception))
@@ -44,7 +54,7 @@ module Gitlab
             'event':      'error',
             'error.kind': exception.class.to_s,
             'message':    Gitlab::UrlSanitizer.sanitize(exception.message),
-            'stack':      exception.backtrace.join("\n")
+            'stack':      exception.backtrace&.join("\n")
           }
         else
           {

lib/gitlab/tracing/rails/active_record_subscriber.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Tracing
+    module Rails
+      class ActiveRecordSubscriber
+        include Gitlab::Tracing::Common
+
+        ACTIVE_RECORD_NOTIFICATION_TOPIC = 'sql.active_record'
+        DEFAULT_OPERATION_NAME = "sqlquery"
+
+        def self.instrument
+          subscriber = new
+
+          ActiveSupport::Notifications.subscribe(ACTIVE_RECORD_NOTIFICATION_TOPIC) do |_, start, finish, _, payload|
+            subscriber.notify(start, finish, payload)
+          end
+        end
+
+        # For more information on the payloads: https://guides.rubyonrails.org/active_support_instrumentation.html
+        def notify(start, finish, payload)
+          operation_name = payload[:name].presence || DEFAULT_OPERATION_NAME
+          exception = payload[:exception]
+          tags = {
+            'component' =>        'ActiveRecord',
+            'span.kind' =>        'client',
+            'db.type' =>          'sql',
+            'db.connection_id' => payload[:connection_id],
+            'db.cached' =>        payload[:cached] || false,
+            'db.statement' =>     payload[:sql]
+          }
+
+          postnotify_span("active_record:#{operation_name}", start, finish, tags: tags, exception: exception)
+        end
+      end
+    end
+  end
+end

spec/lib/gitlab/ci/status/external/common_spec.rb

@@ -11,7 +11,7 @@ describe Gitlab::Ci::Status::External::Common do
   end
 
   subject do
-    Gitlab::Ci::Status::Core
+    Gitlab::Ci::Status::Success
       .new(external_status, user)
       .extend(described_class)
   end
@@ -20,6 +20,22 @@ describe Gitlab::Ci::Status::External::Common do
     it 'returns description' do
       expect(subject.label).to eq external_description
     end
+
+    context 'when description is nil' do
+      let(:external_description) { nil }
+
+      it 'uses core status label' do
+        expect(subject.label).to eq('passed')
+      end
+    end
+
+    context 'when description is empty string' do
+      let(:external_description) { '' }
+
+      it 'uses core status label' do
+        expect(subject.label).to eq('passed')
+      end
+    end
   end
 
   describe '#has_action?' do

spec/lib/gitlab/kubernetes/kube_client_spec.rb

@@ -24,6 +24,32 @@ describe Gitlab::Kubernetes::KubeClient do
     end
   end
 
+  shared_examples 'redirection not allowed' do |method_name|
+    before do
+      redirect_url = 'https://not-under-our-control.example.com/api/v1/pods'
+
+      stub_request(:get, %r{\A#{api_url}/})
+        .to_return(status: 302, headers: { location: redirect_url })
+
+      stub_request(:get, redirect_url)
+        .to_return(status: 200, body: '{}')
+    end
+
+    it 'does not follow redirects' do
+      method_call = -> do
+        case method_name
+        when /\A(get_|delete_)/
+          client.public_send(method_name)
+        when /\A(create_|update_)/
+          client.public_send(method_name, {})
+        else
+          raise "Unknown method name #{method_name}"
+        end
+      end
+      expect { method_call.call }.to raise_error(Kubeclient::HttpError)
+    end
+  end
+
   describe '#core_client' do
     subject { client.core_client }
 
@@ -103,6 +129,8 @@ describe Gitlab::Kubernetes::KubeClient do
       :update_service_account
     ].each do |method|
       describe "##{method}" do
+        include_examples 'redirection not allowed', method
+
         it 'delegates to the core client' do
           expect(client).to delegate_method(method).to(:core_client)
         end
@@ -123,6 +151,8 @@ describe Gitlab::Kubernetes::KubeClient do
       :update_cluster_role_binding
     ].each do |method|
       describe "##{method}" do
+        include_examples 'redirection not allowed', method
+
         it 'delegates to the rbac client' do
           expect(client).to delegate_method(method).to(:rbac_client)
         end
@@ -139,6 +169,8 @@ describe Gitlab::Kubernetes::KubeClient do
     let(:extensions_client) { client.extensions_client }
 
     describe '#get_deployments' do
+      include_examples 'redirection not allowed', 'get_deployments'
+
       it 'delegates to the extensions client' do
         expect(client).to delegate_method(:get_deployments).to(:extensions_client)
       end

spec/lib/gitlab/tracing/rails/active_record_subscriber_spec.rb

+# frozen_string_literal: true
+
+require 'fast_spec_helper'
+require 'rspec-parameterized'
+
+describe Gitlab::Tracing::Rails::ActiveRecordSubscriber do
+  using RSpec::Parameterized::TableSyntax
+
+  describe '.instrument' do
+    it 'is unsubscribable' do
+      subscription = described_class.instrument
+
+      expect(subscription).not_to be_nil
+      expect { ActiveSupport::Notifications.unsubscribe(subscription) }.not_to raise_error
+    end
+  end
+
+  describe '#notify' do
+    subject { described_class.new }
+    let(:start) { Time.now }
+    let(:finish) { Time.now }
+
+    where(:name, :operation_name, :exception, :connection_id, :cached, :cached_response, :sql) do
+      nil         | "active_record:sqlquery"  | nil               | nil | nil   | false | nil
+      ""          | "active_record:sqlquery"  | nil               | nil | nil   | false | nil
+      "User Load" | "active_record:User Load" | nil               | nil | nil   | false | nil
+      "Repo Load" | "active_record:Repo Load" | StandardError.new | nil | nil   | false | nil
+      nil         | "active_record:sqlquery"  | nil               | 123 | nil   | false | nil
+      nil         | "active_record:sqlquery"  | nil               | nil | false | false | nil
+      nil         | "active_record:sqlquery"  | nil               | nil | true  | true  | nil
+      nil         | "active_record:sqlquery"  | nil               | nil | true  | true  | "SELECT * FROM users"
+    end
+
+    with_them do
+      def payload
+        {
+          name: name,
+          exception: exception,
+          connection_id: connection_id,
+          cached: cached,
+          sql: sql
+        }
+      end
+
+      def expected_tags
+        {
+          "component" =>        "ActiveRecord",
+          "span.kind" =>        "client",
+          "db.type" =>          "sql",
+          "db.connection_id" => connection_id,
+          "db.cached" =>        cached_response,
+          "db.statement" =>     sql
+        }
+      end
+
+      it 'should notify the tracer when the hash contains null values' do
+        expect(subject).to receive(:postnotify_span).with(operation_name, start, finish, tags: expected_tags, exception: exception)
+
+        subject.notify(start, finish, payload)
+      end
+
+      it 'should notify the tracer when the payload is missing values' do
+        expect(subject).to receive(:postnotify_span).with(operation_name, start, finish, tags: expected_tags, exception: exception)
+
+        subject.notify(start, finish, payload.compact)
+      end
+
+      it 'should not throw exceptions when with the default tracer' do
+        expect { subject.notify(start, finish, payload) }.not_to raise_error
+      end
+    end
+  end
+end