Revert "Rename `ServerInfoPresenter` to `Gitlab::Kas::ServerInfo`"

This reverts commit 45c0eb6f93a4e54c0e5f15c38f6a699c75ddc21c.

app/controllers/admin/dashboard_controller.rb

@@ -15,7 +15,7 @@ def index
     @users = User.order_id_desc.limit(10)
     @groups = Group.order_id_desc.with_route.limit(10)
     @notices = Gitlab::ConfigChecker::ExternalDatabaseChecker.check
-    @kas_server_info = Gitlab::Kas::ServerInfoPresenter.new if Gitlab::Kas.enabled?
+    @kas_server_info = Gitlab::Kas::ServerInfo.new.present if Gitlab::Kas.enabled?
     @redis_versions = Gitlab::Redis::ALL_CLASSES.map(&:version).uniq
   end
 

app/models/app_config/kas_metadata.rb

@@ -10,7 +10,7 @@ def self.declarative_policy_class
 
     def initialize
       @enabled = Gitlab::Kas.enabled?
-      @version = Gitlab::Kas.version if @enabled
+      @version = Gitlab::Kas::ServerInfo.new.version_info if @enabled
       @external_url = Gitlab::Kas.external_url if @enabled
       @external_k8s_proxy_url = Gitlab::Kas.tunnel_url if @enabled
     end

app/presenters/gitlab/kas/server_info_presenter.rb

@@ -2,45 +2,27 @@
 
 module Gitlab
   module Kas
-    class ServerInfoPresenter
+    class ServerInfoPresenter < Gitlab::View::Presenter::Delegated
+      presents ::Gitlab::Kas::ServerInfo, as: :server_info
+
       GROUP = 'gitlab-org/cluster-integration'
       PROJECT = 'gitlab-agent'
       private_constant :GROUP, :PROJECT
 
-      delegate :git_ref, to: :server_info, private: true
-      delegate :version, to: :server_info
-
-      def initialize
-        @server_info = fetch_server_info
-      end
-
-      def retrieved_server_info?
-        server_info.present?
-      end
-
       def git_ref_for_display
-        return unless git_ref.present?
+        return unless retrieved_server_info? && git_ref.present?
 
         commit&.short_id || git_ref
       end
 
       def git_ref_url
-        return unless git_ref.present?
+        return unless retrieved_server_info? && git_ref.present?
 
         build_git_ref_url
       end
 
       private
 
-      attr_reader :server_info
-
-      def fetch_server_info
-        Gitlab::Kas::Client.new.get_server_info
-      rescue StandardError => e
-        Gitlab::ErrorTracking.track_exception(e)
-        nil
-      end
-
       def build_git_ref_url
         Gitlab::Utils.append_path(Gitlab::Saas.com_url, git_ref_path)
       end

doc/api/metadata.md

@@ -30,7 +30,7 @@ Response body attributes:
 | `kas.enabled`             | boolean        | Indicates whether KAS is enabled.                                                                                             |
 | `kas.externalUrl`         | string or null | URL used by the agents to communicate with KAS. It's `null` if `kas.enabled` is `false`.                                      |
 | `kas.externalK8sProxyUrl` | string or null | URL used by the Kubernetes tooling to communicate with the KAS Kubernetes API proxy. It's `null` if `kas.enabled` is `false`. |
-| `kas.version`             | string or null | Version of KAS. It's `null` if `kas.enabled` is `false`.                                                                      |
+| `kas.version`             | string or null | Version of KAS. It's `null` if `kas.enabled` is `false` or when GitLab instance failed to fetch server info from KAS.         |
 | `enterprise`              | boolean        | Indicates whether GitLab instance is Enterprise Edition.                                                                      |
 
 Example request:

lib/gitlab/kas/server_info.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Kas
+    class ServerInfo
+      include Presentable
+
+      delegate :git_ref, :version, to: :fetched_server_info
+
+      def initialize
+        @fetched_server_info = fetch_server_info
+      end
+
+      def retrieved_server_info?
+        fetched_server_info.present?
+      end
+
+      def version_info
+        return unless retrieved_server_info? && version.present?
+        return Gitlab::VersionInfo.parse("#{version}+#{git_ref}", parse_suffix: true) if valid_commit_ref?
+
+        Gitlab::VersionInfo.parse(version)
+      end
+
+      def valid_commit_ref?
+        return unless retrieved_server_info?
+
+        ::Gitlab::Git::Commit.valid?(git_ref) &&
+          git_ref.match?(::Gitlab::Git::COMMIT_ID)
+      end
+
+      private
+
+      attr_reader :fetched_server_info
+
+      def fetch_server_info
+        Gitlab::Kas::Client.new.get_server_info
+      rescue StandardError => e
+        Gitlab::ErrorTracking.track_exception(e)
+        nil
+      end
+    end
+  end
+end

spec/lib/gitlab/kas/server_info_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Kas::ServerInfo, feature_category: :deployment_management do
+  subject(:server_info) { described_class.new }
+
+  let(:git_ref) { '6a0281c68969d9ce8f36fdaf242b4f6e0503d940' }
+
+  before do
+    allow(Gitlab::Kas).to receive(:enabled?).and_return(true)
+
+    # rubocop:disable RSpec/VerifiedDoubles -- Avoiding the false positive 'the Gitlab::Agent::ServerInfo::ServerInfo
+    # class does not implement the instance method: version' when using instance_double(), which is only because
+    # Ruby protobuf handle these message through method_missing instead of actually defining instance methods.
+    response = double(Gitlab::Agent::ServerInfo::ServerInfo, version: '17.4.0-rc1', git_ref: git_ref)
+    # rubocop:enable RSpec/VerifiedDoubles
+
+    allow_next_instance_of(Gitlab::Kas::Client) do |instance|
+      allow(instance).to receive(:get_server_info).and_return(response)
+    end
+  end
+
+  shared_examples 'logs kas error' do
+    it 'logs the error' do
+      expect(Gitlab::ErrorTracking).to receive(:track_exception).with(error)
+
+      subject
+    end
+  end
+
+  context 'when kas client initialization fails' do
+    let(:error) { Gitlab::Kas::Client::ConfigurationError.new('boom') }
+
+    before do
+      allow(Gitlab::Kas::Client).to receive(:new).and_raise(error)
+    end
+
+    it_behaves_like 'logs kas error'
+  end
+
+  context 'when kas rpc fail' do
+    let(:error) { GRPC::Unavailable.new("failed to connect to all addresses") }
+
+    before do
+      allow_next_instance_of(Gitlab::Kas::Client) do |instance|
+        allow(instance).to receive(:get_server_info).and_raise(error)
+      end
+    end
+
+    it_behaves_like 'logs kas error'
+  end
+
+  describe '#version' do
+    subject { server_info.version }
+
+    it 'returns version' do
+      is_expected.to eq('17.4.0-rc1')
+    end
+  end
+
+  describe '#retrieved_server_info?' do
+    it { is_expected.to be_retrieved_server_info }
+
+    it 'returns false when server info is not retrieved' do
+      allow_next_instance_of(Gitlab::Kas::Client) do |instance|
+        allow(instance).to receive(:get_server_info).and_raise(Gitlab::Kas::Client::ConfigurationError)
+      end
+
+      is_expected.not_to be_retrieved_server_info
+    end
+  end
+
+  describe '#version_info' do
+    subject { server_info.version_info.to_s }
+
+    context 'when git ref is a commit' do
+      let(:git_ref) { '6a0281c68969d9ce8f36fdaf242b4f6e0503d940' }
+
+      it { is_expected.to eq('17.4.0-rc1+6a0281c68969d9ce8f36fdaf242b4f6e0503d940') }
+    end
+
+    context 'when git ref is a tag' do
+      let(:git_ref) { 'v17.4.0-rc1' }
+
+      it { is_expected.to eq('17.4.0') }
+    end
+
+    context 'when git ref is empty' do
+      let(:git_ref) { '' }
+
+      it { is_expected.to eq('17.4.0') }
+    end
+  end
+end

spec/models/app_config/kas_metadata_spec.rb

@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 RSpec.describe AppConfig::KasMetadata, feature_category: :api do
+  let(:kas_version_info) { instance_double(Gitlab::VersionInfo) }
+
   it 'has InstanceMetadataPolicy as declarative policy' do
     expect(described_class.declarative_policy_class).to eq("AppConfig::InstanceMetadataPolicy")
   end
@@ -10,10 +12,13 @@
   context 'when KAS is enabled' do
     it 'has the correct properties' do
       allow(Gitlab::Kas).to receive(:enabled?).and_return(true)
+      allow_next_instance_of(Gitlab::Kas::ServerInfo) do |instance|
+        allow(instance).to receive(:version_info).and_return(kas_version_info)
+      end
 
       expect(described_class.new).to have_attributes(
         enabled: Gitlab::Kas.enabled?,
-        version: Gitlab::Kas.version,
+        version: kas_version_info,
         external_url: Gitlab::Kas.external_url,
         external_k8s_proxy_url: Gitlab::Kas.tunnel_url
       )

spec/presenters/gitlab/kas/server_info_presenter_spec.rb

@@ -4,71 +4,14 @@
 
 RSpec.describe Gitlab::Kas::ServerInfoPresenter, feature_category: :deployment_management do
   let(:git_ref) { '6a0281c68969d9ce8f36fdaf242b4f6e0503d940' }
-  let(:presenter) { described_class.new }
-
-  before do
-    allow(Gitlab::Kas).to receive(:enabled?).and_return(true)
-
-    # rubocop:disable RSpec/VerifiedDoubles -- Avoiding the false positive 'the Gitlab::Agent::ServerInfo::ServerInfo
-    # class does not implement the instance method: version' when using instance_double(), which is only because
-    # Ruby protobuf handle these message through method_missing instead of actually defining instance methods.
-    response = double(Gitlab::Agent::ServerInfo::ServerInfo, version: '17.4.0-rc1', git_ref: git_ref)
-    # rubocop:enable RSpec/VerifiedDoubles
-
-    allow_next_instance_of(Gitlab::Kas::Client) do |instance|
-      allow(instance).to receive(:get_server_info).and_return(response)
-    end
-  end
-
-  shared_examples 'logs kas error' do
-    it 'logs the error' do
-      expect(Gitlab::ErrorTracking).to receive(:track_exception).with(error)
-
-      presenter
-    end
-  end
-
-  context 'when kas client initialization fails' do
-    let(:error) { Gitlab::Kas::Client::ConfigurationError.new('boom') }
-
-    before do
-      allow(Gitlab::Kas::Client).to receive(:new).and_raise(error)
-    end
-
-    it_behaves_like 'logs kas error'
-  end
-
-  context 'when kas rpc fail' do
-    let(:error) { GRPC::Unavailable.new("failed to connect to all addresses") }
-
-    before do
-      allow_next_instance_of(Gitlab::Kas::Client) do |instance|
-        allow(instance).to receive(:get_server_info).and_raise(error)
-      end
-    end
-
-    it_behaves_like 'logs kas error'
+  let(:server_info) do
+    instance_double(Gitlab::Kas::ServerInfo,
+      retrieved_server_info?: true,
+      version: '17.4.0-rc1',
+      git_ref: git_ref)
   end
 
-  describe '#version' do
-    it 'returns version' do
-      expect(presenter.version).to eq('17.4.0-rc1')
-    end
-  end
-
-  describe '#retrieved_server_info?' do
-    it 'returns true when server info is retrieved' do
-      expect(presenter.retrieved_server_info?).to be(true)
-    end
-
-    it 'returns false when server info is not retrieved' do
-      allow_next_instance_of(Gitlab::Kas::Client) do |instance|
-        allow(instance).to receive(:get_server_info).and_raise(Gitlab::Kas::Client::ConfigurationError)
-      end
-
-      expect(presenter.retrieved_server_info?).to be(false)
-    end
-  end
+  let(:presenter) { described_class.new(server_info) }
 
   describe '#git_ref_for_display' do
     subject { presenter.git_ref_for_display }
@@ -90,6 +33,12 @@
 
       it { is_expected.to be_nil }
     end
+
+    context 'when server_info is not retrieved' do
+      let(:server_info) { instance_double(Gitlab::Kas::ServerInfo, retrieved_server_info?: false) }
+
+      it { is_expected.to be_nil }
+    end
   end
 
   describe '#git_ref_url' do
@@ -120,5 +69,11 @@
 
       it { is_expected.to be_nil }
     end
+
+    context 'when server_info is not retrieved' do
+      let(:server_info) { instance_double(Gitlab::Kas::ServerInfo, retrieved_server_info?: false) }
+
+      it { is_expected.to be_nil }
+    end
   end
 end

spec/requests/api/graphql/metadata_query_spec.rb

@@ -27,18 +27,44 @@
     end
 
     context 'kas is enabled' do
-      let(:expected_kas_version) { Gitlab::Kas.version }
+      let(:kas_version_info) { Gitlab::VersionInfo.new(1, 2, 3) }
+      let(:expected_kas_version) { kas_version_info.to_s }
       let(:expected_kas_external_url) { Gitlab::Kas.external_url }
       let(:expected_kas_external_k8s_proxy_url) { Gitlab::Kas.tunnel_url }
 
       before do
         allow(Gitlab::Kas).to receive(:enabled?).and_return(true)
-        post_graphql(query, current_user: current_user)
       end
 
-      it 'returns version, revision, kas_enabled, kas_version, kas_external_url' do
-        expect(graphql_errors).to be_nil
-        expect(graphql_data).to eq(expected_data)
+      context 'when kas server info fetched successfully' do
+        before do
+          allow_next_instance_of(Gitlab::Kas::ServerInfo) do |server_info|
+            allow(server_info).to receive(:version_info).and_return(kas_version_info)
+          end
+          post_graphql(query, current_user: current_user)
+        end
+
+        it 'returns version, revision, kas_enabled, kas_version, kas_external_url' do
+          expect(graphql_errors).to be_nil
+          expect(graphql_data).to eq(expected_data)
+        end
+      end
+
+      context 'when failed to fetch kas server info' do
+        let(:expected_kas_version) { nil }
+
+        before do
+          allow_next_instance_of(Gitlab::Kas::ServerInfo) do |server_info|
+            # Upon RPC failure, Gitlab::Kas::ServerInfo#version_info could return nil after reporting the error.
+            allow(server_info).to receive(:version_info).and_return nil
+          end
+          post_graphql(query, current_user: current_user)
+        end
+
+        it 'returns nil as kas version' do
+          expect(graphql_errors).to be_nil
+          expect(graphql_data).to eq(expected_data)
+        end
       end
     end
 
@@ -70,6 +96,7 @@
     let(:query) { graphql_query_for('metadata', {}, feature_flags_field) }
 
     before do
+      allow(Gitlab::Kas).to receive(:enabled?).and_return(false)
       stub_feature_flag_definition('foo')
       stub_feature_flag_definition('ipsum')
       stub_feature_flag_definition('dolar')
@@ -115,5 +142,11 @@
       expect(graphql_errors).to be_nil
       expect(graphql_data).to eq('metadata' => nil)
     end
+
+    it 'avoids unnecessary kas query' do
+      post_graphql(query, current_user: nil)
+
+      expect(Gitlab::Kas).not_to receive(:enabled?)
+    end
   end
 end

spec/views/admin/dashboard/index.html.haml_spec.rb

@@ -81,13 +81,14 @@
 
       before do
         server_info = instance_double(
-          Gitlab::Kas::ServerInfoPresenter,
+          Gitlab::Kas::ServerInfo,
           retrieved_server_info?: retrieved_server_info?,
-          version: '17.4.0-rc1',
-          git_ref_for_display: '6a0281c6896',
-          git_ref_url: 'some/url'
+          version: '17.4.0-rc1'
         )
-        assign(:kas_server_info, server_info)
+        presenter = Gitlab::Kas::ServerInfoPresenter.new(server_info)
+        allow(presenter).to receive(:git_ref_for_display).and_return('6a0281c6896')
+        allow(presenter).to receive(:git_ref_url).and_return('some/url')
+        assign(:kas_server_info, presenter)
       end
 
       context 'when successfully fetched KAS version' do