From 6f20fc2b0ef7dde5822347ae910d266bed06e25d Mon Sep 17 00:00:00 2001
From: Chou Yu Ta <lawrencechou1024@gmail.com>
Date: Sat, 21 Dec 2024 13:51:54 +0000
Subject: [PATCH] Revert "Rename `ServerInfoPresenter` to
`Gitlab::Kas::ServerInfo`"
This reverts commit 45c0eb6f93a4e54c0e5f15c38f6a699c75ddc21c.
---
app/controllers/admin/dashboard_controller.rb | 2 +-
app/models/app_config/kas_metadata.rb | 2 +-
.../gitlab/kas/server_info_presenter.rb | 28 +-----
doc/api/metadata.md | 2 +-
lib/gitlab/kas/server_info.rb | 44 +++++++++
spec/lib/gitlab/kas/server_info_spec.rb | 95 +++++++++++++++++++
spec/models/app_config/kas_metadata_spec.rb | 7 +-
.../gitlab/kas/server_info_presenter_spec.rb | 81 ++++------------
.../api/graphql/metadata_query_spec.rb | 43 ++++++++-
.../admin/dashboard/index.html.haml_spec.rb | 11 ++-
10 files changed, 215 insertions(+), 100 deletions(-)
create mode 100644 lib/gitlab/kas/server_info.rb
create mode 100644 spec/lib/gitlab/kas/server_info_spec.rb
diff --git a/app/controllers/admin/dashboard_controller.rb b/app/controllers/admin/dashboard_controller.rb
index d657b1de1055..bad28c4f49cf 100644
--- a/app/controllers/admin/dashboard_controller.rb
+++ b/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
diff --git a/app/models/app_config/kas_metadata.rb b/app/models/app_config/kas_metadata.rb
index 917eefaaf9b9..496c1407b31d 100644
--- a/app/models/app_config/kas_metadata.rb
+++ b/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
diff --git a/app/presenters/gitlab/kas/server_info_presenter.rb b/app/presenters/gitlab/kas/server_info_presenter.rb
index 4096e705ae61..3aa114d4053f 100644
--- a/app/presenters/gitlab/kas/server_info_presenter.rb
+++ b/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
diff --git a/doc/api/metadata.md b/doc/api/metadata.md
index 6dfe6b7ab880..a144166696c7 100644
--- a/doc/api/metadata.md
+++ b/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:
diff --git a/lib/gitlab/kas/server_info.rb b/lib/gitlab/kas/server_info.rb
new file mode 100644
index 000000000000..c2dd98f77404
--- /dev/null
+++ b/lib/gitlab/kas/server_info.rb
@@ -0,0 +1,44 @@
+# 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
diff --git a/spec/lib/gitlab/kas/server_info_spec.rb b/spec/lib/gitlab/kas/server_info_spec.rb
new file mode 100644
index 000000000000..edfc4ac8d322
--- /dev/null
+++ b/spec/lib/gitlab/kas/server_info_spec.rb
@@ -0,0 +1,95 @@
+# 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
diff --git a/spec/models/app_config/kas_metadata_spec.rb b/spec/models/app_config/kas_metadata_spec.rb
index 3c8a490bdecf..e863913a37d3 100644
--- a/spec/models/app_config/kas_metadata_spec.rb
+++ b/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
)
diff --git a/spec/presenters/gitlab/kas/server_info_presenter_spec.rb b/spec/presenters/gitlab/kas/server_info_presenter_spec.rb
index 35219da6afd5..76b5e41732f6 100644
--- a/spec/presenters/gitlab/kas/server_info_presenter_spec.rb
+++ b/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
diff --git a/spec/requests/api/graphql/metadata_query_spec.rb b/spec/requests/api/graphql/metadata_query_spec.rb
index d19fe9da797f..096515543cac 100644
--- a/spec/requests/api/graphql/metadata_query_spec.rb
+++ b/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
diff --git a/spec/views/admin/dashboard/index.html.haml_spec.rb b/spec/views/admin/dashboard/index.html.haml_spec.rb
index 7a682b3bc952..9a18da14d66b 100644
--- a/spec/views/admin/dashboard/index.html.haml_spec.rb
+++ b/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
--
GitLab