From 470f52d2384d32eb955220982782c6101da97f83 Mon Sep 17 00:00:00 2001 From: Max Woolf Date: Tue, 23 Aug 2022 13:48:40 +0100 Subject: [PATCH 1/9] Jitsu/GitLab Connection PoC Adds basic building blocks to connect a GitLab project to a Jitsu analytics stack. Deployed behind a default-off feature flag. Changelog: added --- app/models/application_setting.rb | 1 + config/sidekiq_queues.yml | 2 + ...racking_columns_to_application_settings.rb | 15 ++ ...mns_to_application_settings_text_limits.rb | 19 +++ db/schema_migrations/20220818125332 | 1 + db/schema_migrations/20220818125703 | 1 + ee/app/models/ee/application_setting.rb | 7 +- ee/app/models/ee/project.rb | 6 + .../product_analytics/jitsu_authentication.rb | 84 +++++++++++ ee/app/workers/all_queues.yml | 9 ++ .../initialize_analytics_worker.rb | 19 +++ .../jitsu_connection_proof_of_concept.yml | 8 ++ .../jitsu_authentication_spec.rb | 132 ++++++++++++++++++ ee/spec/models/project_spec.rb | 16 +++ .../initialize_analytics_worker_spec.rb | 62 ++++++++ .../build_activity_graph_service_spec.rb | 2 +- 16 files changed, 382 insertions(+), 2 deletions(-) create mode 100644 db/migrate/20220818125332_add_jitsu_tracking_columns_to_application_settings.rb create mode 100644 db/migrate/20220818125703_add_jitsu_tracking_columns_to_application_settings_text_limits.rb create mode 100644 db/schema_migrations/20220818125332 create mode 100644 db/schema_migrations/20220818125703 create mode 100644 ee/app/models/product_analytics/jitsu_authentication.rb create mode 100644 ee/app/workers/product_analytics/initialize_analytics_worker.rb create mode 100644 ee/config/feature_flags/development/jitsu_connection_proof_of_concept.yml create mode 100644 ee/spec/models/product_analytics/jitsu_authentication_spec.rb create mode 100644 ee/spec/workers/product_analytics/initialize_analytics_worker_spec.rb diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index edb9a2053b1222..2dcee7f64d52b9 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -667,6 +667,7 @@ def self.kroki_formats_attributes attr_encrypted :arkose_labs_public_api_key, encryption_options_base_32_aes_256_gcm.merge(encode: false, encode_iv: false) attr_encrypted :arkose_labs_private_api_key, encryption_options_base_32_aes_256_gcm.merge(encode: false, encode_iv: false) attr_encrypted :cube_api_key, encryption_options_base_32_aes_256_gcm + attr_encrypted :jitsu_administrator_password, encryption_options_base_32_aes_256_gcm validates :disable_feed_token, inclusion: { in: [true, false], message: _('must be a boolean value') } diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml index 49109ec7142724..0e8f5d4a7d6f0d 100644 --- a/config/sidekiq_queues.yml +++ b/config/sidekiq_queues.yml @@ -379,6 +379,8 @@ - 5 - - process_commit - 3 +- - product_analytics_initialize_analytics + - 1 - - project_cache - 1 - - project_destroy diff --git a/db/migrate/20220818125332_add_jitsu_tracking_columns_to_application_settings.rb b/db/migrate/20220818125332_add_jitsu_tracking_columns_to_application_settings.rb new file mode 100644 index 00000000000000..9013168c2c57d9 --- /dev/null +++ b/db/migrate/20220818125332_add_jitsu_tracking_columns_to_application_settings.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +class AddJitsuTrackingColumnsToApplicationSettings < Gitlab::Database::Migration[2.0] + def change + # rubocop:disable Migration/AddLimitToTextColumns + # limit is added in 20220818125703_add_jitsu_tracking_columns_to_application_settings_text_limits.rb + add_column :application_settings, :jitsu_host, :text + add_column :application_settings, :jitsu_project_xid, :text + add_column :application_settings, :clickhouse_connection_string, :text + add_column :application_settings, :jitsu_administrator_email, :text + add_column :application_settings, :encrypted_jitsu_administrator_password, :binary + add_column :application_settings, :encrypted_jitsu_administrator_password_iv, :binary + # rubocop:enable Migration/AddLimitToTextColumns + end +end diff --git a/db/migrate/20220818125703_add_jitsu_tracking_columns_to_application_settings_text_limits.rb b/db/migrate/20220818125703_add_jitsu_tracking_columns_to_application_settings_text_limits.rb new file mode 100644 index 00000000000000..4949fa6464f0b3 --- /dev/null +++ b/db/migrate/20220818125703_add_jitsu_tracking_columns_to_application_settings_text_limits.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +class AddJitsuTrackingColumnsToApplicationSettingsTextLimits < Gitlab::Database::Migration[2.0] + disable_ddl_transaction! + + def up + add_text_limit :application_settings, :jitsu_host, 255 + add_text_limit :application_settings, :jitsu_project_xid, 255 + add_text_limit :application_settings, :clickhouse_connection_string, 255 + add_text_limit :application_settings, :jitsu_administrator_email, 255 + end + + def down + remove_text_limit :application_settings, :jitsu_host + remove_text_limit :application_settings, :jitsu_project_xid + remove_text_limit :application_settings, :clickhouse_connection_string + remove_text_limit :application_settings, :jitsu_administrator_email + end +end diff --git a/db/schema_migrations/20220818125332 b/db/schema_migrations/20220818125332 new file mode 100644 index 00000000000000..35c76c4318fd04 --- /dev/null +++ b/db/schema_migrations/20220818125332 @@ -0,0 +1 @@ +ebcf446aa6579d93c57c2e96e8b670a43bcb6e20216f33a7f535e1bed50ace62 \ No newline at end of file diff --git a/db/schema_migrations/20220818125703 b/db/schema_migrations/20220818125703 new file mode 100644 index 00000000000000..1bfebfc50ad7b2 --- /dev/null +++ b/db/schema_migrations/20220818125703 @@ -0,0 +1 @@ +b60f36cd83174ce257baba4a74f0fcba6cd462fa2af6530ff5a3341536058e12 \ No newline at end of file diff --git a/ee/app/models/ee/application_setting.rb b/ee/app/models/ee/application_setting.rb index 124c649dd8ad63..0c9d8327277b69 100644 --- a/ee/app/models/ee/application_setting.rb +++ b/ee/app/models/ee/application_setting.rb @@ -213,7 +213,12 @@ def defaults max_number_of_repository_downloads: 0, max_number_of_repository_downloads_within_time_period: 0, git_rate_limit_users_allowlist: [], - auto_ban_user_on_excessive_projects_download: false + auto_ban_user_on_excessive_projects_download: false, + jitsu_host: '', + jitsu_project_xid: '', + clickhouse_connection_string: '', + jitsu_administrator_email: '', + jitsu_administrator_password: '' ) end end diff --git a/ee/app/models/ee/project.rb b/ee/app/models/ee/project.rb index 5908e8d0ef3a26..d9ff4048ea5742 100644 --- a/ee/app/models/ee/project.rb +++ b/ee/app/models/ee/project.rb @@ -802,6 +802,12 @@ def merge_requests_disable_committers_approval? !!merge_requests_disable_committers_approval end + def initialize_analytics_stack + return unless ::Feature.enabled?(:jitsu_connection_proof_of_concept, self.group) + + ::ProductAnalytics::InitializeAnalyticsWorker.perform_async(id) + end + def license_compliance(pipeline = latest_pipeline_with_reports(::Ci::JobArtifact.of_report_type(:license_scanning))) SCA::LicenseCompliance.new(self, pipeline) end diff --git a/ee/app/models/product_analytics/jitsu_authentication.rb b/ee/app/models/product_analytics/jitsu_authentication.rb new file mode 100644 index 00000000000000..87b690a7c9c04f --- /dev/null +++ b/ee/app/models/product_analytics/jitsu_authentication.rb @@ -0,0 +1,84 @@ +# frozen_string_literal: true + +module ProductAnalytics + class JitsuAuthentication + def initialize(project) + @project = project + end + + def create_api_key! + response = Gitlab::HTTP.post( + "#{root_url}/api/v2/objects/#{Gitlab::CurrentSettings.jitsu_project_xid}/api_keys", + allow_local_requests: true, + headers: { + Authorization: "Bearer #{generate_access_token}" + }, + body: { + 'comment': @project.to_global_id.to_s, + 'jsAuth': SecureRandom.uuid + }.to_json + ) + + if response.success? + json = Gitlab::Json.parse(response.body) + + return { jsAuth: json['jsAuth'], uid: json['uid'] } + end + + nil + end + + def create_clickhouse_destination! + id = SecureRandom.uuid + + response = Gitlab::HTTP.post( + "#{root_url}/api/v2/objects/#{Gitlab::CurrentSettings.jitsu_project_xid}/destinations", + allow_local_requests: true, + headers: { + Authorization: "Bearer #{generate_access_token}" + }, + body: { + _type: 'clickhouse', + _onlyKeys: [create_api_key![:uid]], + _id: id, + _uid: SecureRandom.uuid, + _connectionTestOk: true, + _formData: { + ch_database: "gitlab_project_#{@project.id}", + mode: 'stream', + tableName: "jitsu", + ch_dsns_list: [clickhouse_connection_string] + } + }.to_json + ) + + response.success? ? id : nil + end + + def generate_access_token + response = Gitlab::HTTP.post( + "#{root_url}/api/v1/users/signin", + allow_local_requests: true, + headers: { 'Content-Type' => 'application/json' }, + body: { + 'email': Gitlab::CurrentSettings.jitsu_administrator_email, + 'password': Gitlab::CurrentSettings.jitsu_administrator_password + }.to_json + ) + + return Gitlab::Json.parse(response.body)['access_token'] if response.success? + + nil + end + + private + + def root_url + "#{Gitlab::CurrentSettings.jitsu_host}/configurator" + end + + def clickhouse_connection_string + Gitlab::CurrentSettings.clickhouse_connection_string + end + end +end diff --git a/ee/app/workers/all_queues.yml b/ee/app/workers/all_queues.yml index 9874b6ab56b763..ccb8974f0e4371 100644 --- a/ee/app/workers/all_queues.yml +++ b/ee/app/workers/all_queues.yml @@ -1335,6 +1335,15 @@ :weight: 1 :idempotent: true :tags: [] +- :name: product_analytics_initialize_analytics + :worker_name: ProductAnalytics::InitializeAnalyticsWorker + :feature_category: :product_analytics + :has_external_dependencies: true + :urgency: :low + :resource_boundary: :unknown + :weight: 1 + :idempotent: true + :tags: [] - :name: project_import_schedule :worker_name: ProjectImportScheduleWorker :feature_category: :source_code_management diff --git a/ee/app/workers/product_analytics/initialize_analytics_worker.rb b/ee/app/workers/product_analytics/initialize_analytics_worker.rb new file mode 100644 index 00000000000000..a06482f33847a5 --- /dev/null +++ b/ee/app/workers/product_analytics/initialize_analytics_worker.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +module ProductAnalytics + class InitializeAnalyticsWorker + include ApplicationWorker + + data_consistency :sticky + feature_category :product_analytics + idempotent! + worker_has_external_dependencies! + + def perform(project_id) + return if Gitlab::CurrentSettings.jitsu_host.nil? || Gitlab::CurrentSettings.jitsu_project_xid.nil? + + @project = Project.find(project_id) + ProductAnalytics::JitsuAuthentication.new(@project).create_clickhouse_destination! + end + end +end diff --git a/ee/config/feature_flags/development/jitsu_connection_proof_of_concept.yml b/ee/config/feature_flags/development/jitsu_connection_proof_of_concept.yml new file mode 100644 index 00000000000000..028e10f076348f --- /dev/null +++ b/ee/config/feature_flags/development/jitsu_connection_proof_of_concept.yml @@ -0,0 +1,8 @@ +--- +name: jitsu_connection_proof_of_concept +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/95999 +rollout_issue_url: +milestone: '15.4' +type: development +group: group::product analytics +default_enabled: false diff --git a/ee/spec/models/product_analytics/jitsu_authentication_spec.rb b/ee/spec/models/product_analytics/jitsu_authentication_spec.rb new file mode 100644 index 00000000000000..ecf36d461af295 --- /dev/null +++ b/ee/spec/models/product_analytics/jitsu_authentication_spec.rb @@ -0,0 +1,132 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ProductAnalytics::JitsuAuthentication do + let_it_be(:project) { create(:project) } + + let(:auth) { described_class.new(project) } + + before do + stub_application_setting( + jitsu_host: 'http://jitsu.dev', + jitsu_project_xid: 'testtesttesttestprj', + jitsu_administrator_email: 'test@test.com', + jitsu_administrator_password: 'testtest' + ) + end + + describe '#generate_access_token' do + subject { auth.generate_access_token } + + context 'when request is successful' do + before do + stub_signin_success + end + + it { is_expected.to eq('thisisanaccesstoken') } + end + + context 'when request is unsuccessful' do + before do + stub_signin_failure + end + + it { is_expected.to be_nil } + end + end + + describe '#create_api_key!' do + subject { auth.create_api_key! } + + context 'when request is successful' do + before do + stub_signin_success + stub_api_key_success + allow_next_instance_of(described_class) do |auth| + allow(auth).to receive(:access_token).and_return('testtoken') + end + end + + it { is_expected.to eq({ jsAuth: 'Mp1N4PYvRXNk1KIh2MLDE7BYghnSwdnt', uid: 'yijlmncqjot0xy9h6rv54p.s7zz20' }) } + end + + context 'when request is unsuccessful' do + before do + stub_signin_success + stub_api_key_failure + allow_next_instance_of(described_class) do |auth| + allow(auth).to receive(:access_token).and_return('testtoken') + end + end + + it { is_expected.to be_nil } + end + end + + describe '#create_clickhouse_destination' do + subject { auth.create_clickhouse_destination! } + + context 'when request is successful' do + before do + stub_signin_success + stub_clickhouse_success + stub_api_key_success + allow_next_instance_of(described_class) do |auth| + allow(auth).to receive(:access_token).and_return('testtoken') + end + end + + it { is_expected.not_to be_nil } + end + + context 'when request is unsuccessful' do + before do + stub_signin_success + stub_api_key_success + stub_clickhouse_failure + allow_next_instance_of(described_class) do |auth| + allow(auth).to receive(:access_token).and_return('testtoken') + end + end + + it { is_expected.to be_nil } + end + end + + private + + def stub_signin_success + stub_request(:post, "http://jitsu.dev/configurator/api/v1/users/signin") + .with(body: "{\"email\":\"test@test.com\",\"password\":\"testtest\"}") + .to_return(status: 200, body: { access_token: 'thisisanaccesstoken' }.to_json, headers: {}) + end + + def stub_signin_failure + stub_request(:post, "http://jitsu.dev/configurator/api/v1/users/signin") + .with(body: "{\"email\":\"test@test.com\",\"password\":\"testtest\"}") + .to_return(status: 500) + end + + def stub_api_key_success + stub_request(:post, "http://jitsu.dev/configurator/api/v2/objects/testtesttesttestprj/api_keys") + .to_return(status: 200, + body: "{\"jsAuth\":\"Mp1N4PYvRXNk1KIh2MLDE7BYghnSwdnt\",\"uid\":\"yijlmncqjot0xy9h6rv54p.s7zz20\"}", + headers: {}) + end + + def stub_api_key_failure + stub_request(:post, "http://jitsu.dev/configurator/api/v2/objects/testtesttesttestprj/api_keys") + .to_return(status: 500) + end + + def stub_clickhouse_success + stub_request(:post, "http://jitsu.dev/configurator/api/v2/objects/testtesttesttestprj/destinations") + .to_return(status: 200) + end + + def stub_clickhouse_failure + stub_request(:post, "http://jitsu.dev/configurator/api/v2/objects/testtesttesttestprj/destinations") + .to_return(status: 500) + end +end diff --git a/ee/spec/models/project_spec.rb b/ee/spec/models/project_spec.rb index 2349abb2a277a6..908d52df107fb3 100644 --- a/ee/spec/models/project_spec.rb +++ b/ee/spec/models/project_spec.rb @@ -859,6 +859,22 @@ end end + describe '#initialize_analytics_stack' do + subject { project.initialize_analytics_stack } + + context 'feature flag is enabled' do + before do + stub_feature_flags(jitsu_connection_proof_of_concept: true) + end + + it 'enqueues a job' do + expect(::ProductAnalytics::InitializeAnalyticsWorker).to receive(:perform_async).with(project.id).once + + subject + end + end + end + describe '#require_password_to_approve?' do it 'returns true when the resolver returns true' do allow_next_instance_of(ComplianceManagement::MergeRequestApprovalSettings::Resolver) do |resolver| diff --git a/ee/spec/workers/product_analytics/initialize_analytics_worker_spec.rb b/ee/spec/workers/product_analytics/initialize_analytics_worker_spec.rb new file mode 100644 index 00000000000000..337f06055b07d0 --- /dev/null +++ b/ee/spec/workers/product_analytics/initialize_analytics_worker_spec.rb @@ -0,0 +1,62 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ProductAnalytics::InitializeAnalyticsWorker do + let_it_be(:project) { create(:project) } + + shared_examples 'a worker that did not make any HTTP calls' do + it 'makes no HTTP calls to the Jitsu configurator API' do + subject + + expect(Gitlab::HTTP).not_to receive(:post) + end + end + + describe 'perform' do + subject { described_class.new.perform(project.id) } + + context 'when jitsu_host application setting is not defined' do + before do + stub_application_setting(jitsu_host: nil) + end + + it_behaves_like 'a worker that did not make any HTTP calls' + end + + context 'when jitsu_project_xid application setting is not defined' do + before do + stub_application_setting(jitsu_project_xid: nil) + end + + it_behaves_like 'a worker that did not make any HTTP calls' + end + + context 'when project does not exist' do + subject { described_class.new.perform(non_existing_record_id) } + + it 'raises a RecordNotFound error' do + expect { subject }.to raise_error(ActiveRecord::RecordNotFound) + end + end + + context 'when all application settings are defined' do + before do + stub_application_setting( + jitsu_host: 'http://jitsu.dev', + jitsu_project_xid: 'testtesttesttestprj', + jitsu_administrator_email: 'test@test.com', + jitsu_administrator_password: 'testtest' + ) + end + + it 'sends a HTTP request to create a clickhouse destination' do + expect_next_instance_of(ProductAnalytics::JitsuAuthentication) do |auth| + expect(auth).to receive(:create_clickhouse_destination!).once + end + + subject + end + end + end +end diff --git a/spec/services/product_analytics/build_activity_graph_service_spec.rb b/spec/services/product_analytics/build_activity_graph_service_spec.rb index e303656da34eac..e9841ce82d777b 100644 --- a/spec/services/product_analytics/build_activity_graph_service_spec.rb +++ b/spec/services/product_analytics/build_activity_graph_service_spec.rb @@ -17,7 +17,7 @@ ] end - let(:params) { { timerange: 7 } } + let(:params) { { timerange: 7 } }app/models/application_setting.rb subject { described_class.new(project, params).execute } -- GitLab From 220133a7cf5f8747d9dff78d10116eb5a6737aae Mon Sep 17 00:00:00 2001 From: Max Woolf Date: Wed, 14 Sep 2022 10:28:20 +0100 Subject: [PATCH 2/9] Refactor in to service object --- ...mns_to_application_settings_text_limits.rb | 2 +- ee/app/models/ee/project.rb | 6 ---- .../initialize_stack_service.rb | 11 +++++++ ee/spec/models/project_spec.rb | 16 ---------- .../initialize_stack_service_spec.rb | 31 +++++++++++++++++++ .../build_activity_graph_service_spec.rb | 2 +- 6 files changed, 44 insertions(+), 24 deletions(-) create mode 100644 ee/app/services/product_analytics/initialize_stack_service.rb create mode 100644 ee/spec/services/product_analytics/initialize_stack_service_spec.rb diff --git a/db/migrate/20220818125703_add_jitsu_tracking_columns_to_application_settings_text_limits.rb b/db/migrate/20220818125703_add_jitsu_tracking_columns_to_application_settings_text_limits.rb index 4949fa6464f0b3..41de6e34724120 100644 --- a/db/migrate/20220818125703_add_jitsu_tracking_columns_to_application_settings_text_limits.rb +++ b/db/migrate/20220818125703_add_jitsu_tracking_columns_to_application_settings_text_limits.rb @@ -6,7 +6,7 @@ class AddJitsuTrackingColumnsToApplicationSettingsTextLimits < Gitlab::Database: def up add_text_limit :application_settings, :jitsu_host, 255 add_text_limit :application_settings, :jitsu_project_xid, 255 - add_text_limit :application_settings, :clickhouse_connection_string, 255 + add_text_limit :application_settings, :clickhouse_connection_string, 1024 add_text_limit :application_settings, :jitsu_administrator_email, 255 end diff --git a/ee/app/models/ee/project.rb b/ee/app/models/ee/project.rb index d9ff4048ea5742..5908e8d0ef3a26 100644 --- a/ee/app/models/ee/project.rb +++ b/ee/app/models/ee/project.rb @@ -802,12 +802,6 @@ def merge_requests_disable_committers_approval? !!merge_requests_disable_committers_approval end - def initialize_analytics_stack - return unless ::Feature.enabled?(:jitsu_connection_proof_of_concept, self.group) - - ::ProductAnalytics::InitializeAnalyticsWorker.perform_async(id) - end - def license_compliance(pipeline = latest_pipeline_with_reports(::Ci::JobArtifact.of_report_type(:license_scanning))) SCA::LicenseCompliance.new(self, pipeline) end diff --git a/ee/app/services/product_analytics/initialize_stack_service.rb b/ee/app/services/product_analytics/initialize_stack_service.rb new file mode 100644 index 00000000000000..bfc042b74daeb1 --- /dev/null +++ b/ee/app/services/product_analytics/initialize_stack_service.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +module ProductAnalytics + class InitializeStackService < BaseContainerService + def execute + return unless ::Feature.enabled?(:jitsu_connection_proof_of_concept, container.group) + + ::ProductAnalytics::InitializeAnalyticsWorker.perform_async(container.id) + end + end +end diff --git a/ee/spec/models/project_spec.rb b/ee/spec/models/project_spec.rb index 908d52df107fb3..2349abb2a277a6 100644 --- a/ee/spec/models/project_spec.rb +++ b/ee/spec/models/project_spec.rb @@ -859,22 +859,6 @@ end end - describe '#initialize_analytics_stack' do - subject { project.initialize_analytics_stack } - - context 'feature flag is enabled' do - before do - stub_feature_flags(jitsu_connection_proof_of_concept: true) - end - - it 'enqueues a job' do - expect(::ProductAnalytics::InitializeAnalyticsWorker).to receive(:perform_async).with(project.id).once - - subject - end - end - end - describe '#require_password_to_approve?' do it 'returns true when the resolver returns true' do allow_next_instance_of(ComplianceManagement::MergeRequestApprovalSettings::Resolver) do |resolver| diff --git a/ee/spec/services/product_analytics/initialize_stack_service_spec.rb b/ee/spec/services/product_analytics/initialize_stack_service_spec.rb new file mode 100644 index 00000000000000..932f8ec1c4e10e --- /dev/null +++ b/ee/spec/services/product_analytics/initialize_stack_service_spec.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ProductAnalytics::InitializeStackService do + let_it_be(:project) { create(:project) } + + describe '#execute' do + subject { described_class.new(container: project).execute } + + context 'when feature flag is enabled' do + it 'enqueues a job' do + expect(::ProductAnalytics::InitializeAnalyticsWorker).to receive(:perform_async).with(project.id).once + + subject + end + end + + context 'when feature flag is disabled' do + before do + stub_feature_flags(jitsu_connection_proof_of_concept: false) + end + + it 'does not enqueues a job' do + expect(::ProductAnalytics::InitializeAnalyticsWorker).not_to receive(:perform_async) + + subject + end + end + end +end diff --git a/spec/services/product_analytics/build_activity_graph_service_spec.rb b/spec/services/product_analytics/build_activity_graph_service_spec.rb index e9841ce82d777b..e303656da34eac 100644 --- a/spec/services/product_analytics/build_activity_graph_service_spec.rb +++ b/spec/services/product_analytics/build_activity_graph_service_spec.rb @@ -17,7 +17,7 @@ ] end - let(:params) { { timerange: 7 } }app/models/application_setting.rb + let(:params) { { timerange: 7 } } subject { described_class.new(project, params).execute } -- GitLab From 25370296793460a1325ab05c7864593cad9d6c42 Mon Sep 17 00:00:00 2001 From: Robert Hunt Date: Thu, 22 Sep 2022 13:38:08 +0100 Subject: [PATCH 3/9] Rebase with master, updates structure.sql Signed-off-by: Robert Hunt --- db/structure.sql | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/db/structure.sql b/db/structure.sql index ed7bf933a969aa..489c7c78e36946 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -11473,6 +11473,12 @@ CREATE TABLE application_settings ( cube_api_base_url text, encrypted_cube_api_key bytea, encrypted_cube_api_key_iv bytea, + jitsu_host text, + jitsu_project_xid text, + clickhouse_connection_string text, + jitsu_administrator_email text, + encrypted_jitsu_administrator_password bytea, + encrypted_jitsu_administrator_password_iv bytea, dashboard_limit_enabled boolean DEFAULT false NOT NULL, dashboard_limit integer DEFAULT 0 NOT NULL, dashboard_notification_limit integer DEFAULT 0 NOT NULL, @@ -11514,12 +11520,16 @@ CREATE TABLE application_settings ( CONSTRAINT check_9c6c447a13 CHECK ((char_length(maintenance_mode_message) <= 255)), CONSTRAINT check_a5704163cc CHECK ((char_length(secret_detection_revocation_token_types_url) <= 255)), CONSTRAINT check_d03919528d CHECK ((char_length(container_registry_vendor) <= 255)), + CONSTRAINT check_d4865d70f3 CHECK ((char_length(clickhouse_connection_string) <= 1024)), CONSTRAINT check_d820146492 CHECK ((char_length(spam_check_endpoint_url) <= 255)), + CONSTRAINT check_dea8792229 CHECK ((char_length(jitsu_host) <= 255)), CONSTRAINT check_e2dd6e290a CHECK ((char_length(jira_connect_application_key) <= 255)), CONSTRAINT check_e5024c8801 CHECK ((char_length(elasticsearch_username) <= 255)), CONSTRAINT check_e5aba18f02 CHECK ((char_length(container_registry_version) <= 255)), + CONSTRAINT check_ec3ca9aa8d CHECK ((char_length(jitsu_administrator_email) <= 255)), CONSTRAINT check_ef6176834f CHECK ((char_length(encrypted_cloud_license_auth_token_iv) <= 255)), - CONSTRAINT check_f6563bc000 CHECK ((char_length(arkose_labs_verify_api_url) <= 255)) + CONSTRAINT check_f6563bc000 CHECK ((char_length(arkose_labs_verify_api_url) <= 255)), + CONSTRAINT check_fc732c181e CHECK ((char_length(jitsu_project_xid) <= 255)) ); COMMENT ON COLUMN application_settings.content_validation_endpoint_url IS 'JiHu-specific column'; @@ -30868,7 +30878,7 @@ CREATE INDEX tmp_index_on_vulnerabilities_non_dismissed ON vulnerabilities USING CREATE INDEX tmp_index_project_statistics_cont_registry_size ON project_statistics USING btree (project_id) WHERE (container_registry_size = 0); -CREATE INDEX tmp_index_system_note_metadata_on_attention_request_actions ON system_note_metadata USING btree (id) WHERE ((action)::text = ANY ((ARRAY['attention_requested'::character varying, 'attention_request_removed'::character varying])::text[])); +CREATE INDEX tmp_index_system_note_metadata_on_attention_request_actions ON system_note_metadata USING btree (id) WHERE ((action)::text = ANY (ARRAY[('attention_requested'::character varying)::text, ('attention_request_removed'::character varying)::text])); CREATE INDEX tmp_index_vulnerability_occurrences_on_id_and_scanner_id ON vulnerability_occurrences USING btree (id, scanner_id) WHERE (report_type = ANY (ARRAY[7, 99])); -- GitLab From f4be501aa6344865dfb77cefb98ec858b166c481 Mon Sep 17 00:00:00 2001 From: Robert Hunt Date: Thu, 22 Sep 2022 17:26:11 +0100 Subject: [PATCH 4/9] Handling feedback from backend maintainer - Updated app settings to be nil rather than blank strings - Updated jitsu_authentication.rb to pass jid - Updated initialize_analytics_worker.rb to pass jid - Updated feature flag to the correct milestone - Updated jitsu_authentication_spec.rb to: - Add the jid - Assign auth to the subject - Remove the allow_next_instance_of usages - Apply the correct status and body to signin failure stub - Removed .once from initialize_stack_service_spec.rb - Corrected grammar in spec title in initialize_stack_service_spec.rb - Added jid to initialize_analytics_worker_spec.rb - Fixed spec for non-existent project in initialize_analytics_worker_spec.rb Signed-off-by: Robert Hunt --- ee/app/models/ee/application_setting.rb | 10 ++++---- .../product_analytics/jitsu_authentication.rb | 3 ++- .../initialize_analytics_worker.rb | 2 +- .../jitsu_connection_proof_of_concept.yml | 2 +- .../jitsu_authentication_spec.rb | 23 +++++++---------- .../initialize_stack_service_spec.rb | 4 +-- .../initialize_analytics_worker_spec.rb | 25 ++++++++++++------- 7 files changed, 36 insertions(+), 33 deletions(-) diff --git a/ee/app/models/ee/application_setting.rb b/ee/app/models/ee/application_setting.rb index 0c9d8327277b69..f042ad34c28d24 100644 --- a/ee/app/models/ee/application_setting.rb +++ b/ee/app/models/ee/application_setting.rb @@ -214,11 +214,11 @@ def defaults max_number_of_repository_downloads_within_time_period: 0, git_rate_limit_users_allowlist: [], auto_ban_user_on_excessive_projects_download: false, - jitsu_host: '', - jitsu_project_xid: '', - clickhouse_connection_string: '', - jitsu_administrator_email: '', - jitsu_administrator_password: '' + jitsu_host: nil, + jitsu_project_xid: nil, + clickhouse_connection_string: nil, + jitsu_administrator_email: nil, + jitsu_administrator_password: nil ) end end diff --git a/ee/app/models/product_analytics/jitsu_authentication.rb b/ee/app/models/product_analytics/jitsu_authentication.rb index 87b690a7c9c04f..f56ef73a9e8e91 100644 --- a/ee/app/models/product_analytics/jitsu_authentication.rb +++ b/ee/app/models/product_analytics/jitsu_authentication.rb @@ -2,7 +2,8 @@ module ProductAnalytics class JitsuAuthentication - def initialize(project) + def initialize(jid, project) + @jid = jid @project = project end diff --git a/ee/app/workers/product_analytics/initialize_analytics_worker.rb b/ee/app/workers/product_analytics/initialize_analytics_worker.rb index a06482f33847a5..c89aa37e822341 100644 --- a/ee/app/workers/product_analytics/initialize_analytics_worker.rb +++ b/ee/app/workers/product_analytics/initialize_analytics_worker.rb @@ -13,7 +13,7 @@ def perform(project_id) return if Gitlab::CurrentSettings.jitsu_host.nil? || Gitlab::CurrentSettings.jitsu_project_xid.nil? @project = Project.find(project_id) - ProductAnalytics::JitsuAuthentication.new(@project).create_clickhouse_destination! + ProductAnalytics::JitsuAuthentication.new(jid, @project).create_clickhouse_destination! end end end diff --git a/ee/config/feature_flags/development/jitsu_connection_proof_of_concept.yml b/ee/config/feature_flags/development/jitsu_connection_proof_of_concept.yml index 028e10f076348f..7494e073844acc 100644 --- a/ee/config/feature_flags/development/jitsu_connection_proof_of_concept.yml +++ b/ee/config/feature_flags/development/jitsu_connection_proof_of_concept.yml @@ -2,7 +2,7 @@ name: jitsu_connection_proof_of_concept introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/95999 rollout_issue_url: -milestone: '15.4' +milestone: '15.5' type: development group: group::product analytics default_enabled: false diff --git a/ee/spec/models/product_analytics/jitsu_authentication_spec.rb b/ee/spec/models/product_analytics/jitsu_authentication_spec.rb index ecf36d461af295..9c5b41c9a16bad 100644 --- a/ee/spec/models/product_analytics/jitsu_authentication_spec.rb +++ b/ee/spec/models/product_analytics/jitsu_authentication_spec.rb @@ -3,9 +3,10 @@ require 'spec_helper' RSpec.describe ProductAnalytics::JitsuAuthentication do + let(:jid) { '12345678' } let_it_be(:project) { create(:project) } - let(:auth) { described_class.new(project) } + subject(:auth) { described_class.new(jid, project) } before do stub_application_setting( @@ -43,9 +44,7 @@ before do stub_signin_success stub_api_key_success - allow_next_instance_of(described_class) do |auth| - allow(auth).to receive(:access_token).and_return('testtoken') - end + allow(auth).to receive(:access_token).and_return('testtoken') end it { is_expected.to eq({ jsAuth: 'Mp1N4PYvRXNk1KIh2MLDE7BYghnSwdnt', uid: 'yijlmncqjot0xy9h6rv54p.s7zz20' }) } @@ -55,9 +54,7 @@ before do stub_signin_success stub_api_key_failure - allow_next_instance_of(described_class) do |auth| - allow(auth).to receive(:access_token).and_return('testtoken') - end + allow(auth).to receive(:access_token).and_return('testtoken') end it { is_expected.to be_nil } @@ -72,9 +69,7 @@ stub_signin_success stub_clickhouse_success stub_api_key_success - allow_next_instance_of(described_class) do |auth| - allow(auth).to receive(:access_token).and_return('testtoken') - end + allow(auth).to receive(:access_token).and_return('testtoken') end it { is_expected.not_to be_nil } @@ -85,9 +80,7 @@ stub_signin_success stub_api_key_success stub_clickhouse_failure - allow_next_instance_of(described_class) do |auth| - allow(auth).to receive(:access_token).and_return('testtoken') - end + allow(auth).to receive(:access_token).and_return('testtoken') end it { is_expected.to be_nil } @@ -105,7 +98,9 @@ def stub_signin_success def stub_signin_failure stub_request(:post, "http://jitsu.dev/configurator/api/v1/users/signin") .with(body: "{\"email\":\"test@test.com\",\"password\":\"testtest\"}") - .to_return(status: 500) + .to_return(status: 401, + body: { error: 'invalid password', message: 'Authorization failed: invalid password' }.to_json, + headers: {}) end def stub_api_key_success diff --git a/ee/spec/services/product_analytics/initialize_stack_service_spec.rb b/ee/spec/services/product_analytics/initialize_stack_service_spec.rb index 932f8ec1c4e10e..09cccf54486e30 100644 --- a/ee/spec/services/product_analytics/initialize_stack_service_spec.rb +++ b/ee/spec/services/product_analytics/initialize_stack_service_spec.rb @@ -10,7 +10,7 @@ context 'when feature flag is enabled' do it 'enqueues a job' do - expect(::ProductAnalytics::InitializeAnalyticsWorker).to receive(:perform_async).with(project.id).once + expect(::ProductAnalytics::InitializeAnalyticsWorker).to receive(:perform_async).with(project.id) subject end @@ -21,7 +21,7 @@ stub_feature_flags(jitsu_connection_proof_of_concept: false) end - it 'does not enqueues a job' do + it 'does not enqueue a job' do expect(::ProductAnalytics::InitializeAnalyticsWorker).not_to receive(:perform_async) subject diff --git a/ee/spec/workers/product_analytics/initialize_analytics_worker_spec.rb b/ee/spec/workers/product_analytics/initialize_analytics_worker_spec.rb index 337f06055b07d0..165e9001f3fea2 100644 --- a/ee/spec/workers/product_analytics/initialize_analytics_worker_spec.rb +++ b/ee/spec/workers/product_analytics/initialize_analytics_worker_spec.rb @@ -3,8 +3,15 @@ require 'spec_helper' RSpec.describe ProductAnalytics::InitializeAnalyticsWorker do + let(:jid) { '12345678' } let_it_be(:project) { create(:project) } + subject(:worker) { described_class.new } + + before do + allow(worker).to receive(:jid).and_return(jid) + end + shared_examples 'a worker that did not make any HTTP calls' do it 'makes no HTTP calls to the Jitsu configurator API' do subject @@ -14,7 +21,7 @@ end describe 'perform' do - subject { described_class.new.perform(project.id) } + subject { worker.perform(project.id) } context 'when jitsu_host application setting is not defined' do before do @@ -32,14 +39,6 @@ it_behaves_like 'a worker that did not make any HTTP calls' end - context 'when project does not exist' do - subject { described_class.new.perform(non_existing_record_id) } - - it 'raises a RecordNotFound error' do - expect { subject }.to raise_error(ActiveRecord::RecordNotFound) - end - end - context 'when all application settings are defined' do before do stub_application_setting( @@ -57,6 +56,14 @@ subject end + + context 'when project does not exist' do + subject { worker.perform(non_existing_record_id) } + + it 'raises a RecordNotFound error' do + expect { subject }.to raise_error(ActiveRecord::RecordNotFound) + end + end end end end -- GitLab From 8dffce6a3b1c15cde83f7b110c24d9445de8d59b Mon Sep 17 00:00:00 2001 From: Robert Hunt Date: Fri, 23 Sep 2022 11:27:46 +0100 Subject: [PATCH 5/9] Move current settings values to be initalised once Signed-off-by: Robert Hunt --- .../product_analytics/jitsu_authentication.rb | 28 ++++++++----------- 1 file changed, 12 insertions(+), 16 deletions(-) diff --git a/ee/app/models/product_analytics/jitsu_authentication.rb b/ee/app/models/product_analytics/jitsu_authentication.rb index f56ef73a9e8e91..d95594f8f61c91 100644 --- a/ee/app/models/product_analytics/jitsu_authentication.rb +++ b/ee/app/models/product_analytics/jitsu_authentication.rb @@ -5,11 +5,17 @@ class JitsuAuthentication def initialize(jid, project) @jid = jid @project = project + + @root_url = "#{Gitlab::CurrentSettings.jitsu_host}/configurator" + @clickhouse_connection_string = Gitlab::CurrentSettings.clickhouse_connection_string + @jitsu_project_xid = Gitlab::CurrentSettings.jitsu_project_xid + @jitsu_administrator_email = Gitlab::CurrentSettings.jitsu_administrator_email + @jitsu_administrator_password = Gitlab::CurrentSettings.jitsu_administrator_password end def create_api_key! response = Gitlab::HTTP.post( - "#{root_url}/api/v2/objects/#{Gitlab::CurrentSettings.jitsu_project_xid}/api_keys", + "#{@root_url}/api/v2/objects/#{@jitsu_project_xid}/api_keys", allow_local_requests: true, headers: { Authorization: "Bearer #{generate_access_token}" @@ -33,7 +39,7 @@ def create_clickhouse_destination! id = SecureRandom.uuid response = Gitlab::HTTP.post( - "#{root_url}/api/v2/objects/#{Gitlab::CurrentSettings.jitsu_project_xid}/destinations", + "#{@root_url}/api/v2/objects/#{@jitsu_project_xid}/destinations", allow_local_requests: true, headers: { Authorization: "Bearer #{generate_access_token}" @@ -48,7 +54,7 @@ def create_clickhouse_destination! ch_database: "gitlab_project_#{@project.id}", mode: 'stream', tableName: "jitsu", - ch_dsns_list: [clickhouse_connection_string] + ch_dsns_list: [@clickhouse_connection_string] } }.to_json ) @@ -58,12 +64,12 @@ def create_clickhouse_destination! def generate_access_token response = Gitlab::HTTP.post( - "#{root_url}/api/v1/users/signin", + "#{@root_url}/api/v1/users/signin", allow_local_requests: true, headers: { 'Content-Type' => 'application/json' }, body: { - 'email': Gitlab::CurrentSettings.jitsu_administrator_email, - 'password': Gitlab::CurrentSettings.jitsu_administrator_password + 'email': @jitsu_administrator_email, + 'password': @jitsu_administrator_password }.to_json ) @@ -71,15 +77,5 @@ def generate_access_token nil end - - private - - def root_url - "#{Gitlab::CurrentSettings.jitsu_host}/configurator" - end - - def clickhouse_connection_string - Gitlab::CurrentSettings.clickhouse_connection_string - end end end -- GitLab From 1b3ca7888de8827df6d26e620599ff3cbf169c72 Mon Sep 17 00:00:00 2001 From: Robert Hunt Date: Fri, 23 Sep 2022 15:58:36 +0100 Subject: [PATCH 6/9] Add error logging to jitsu API calls Signed-off-by: Robert Hunt --- .../product_analytics/jitsu_authentication.rb | 51 ++++++++++++++++--- .../jitsu_authentication_spec.rb | 43 +++++++++++++--- 2 files changed, 82 insertions(+), 12 deletions(-) diff --git a/ee/app/models/product_analytics/jitsu_authentication.rb b/ee/app/models/product_analytics/jitsu_authentication.rb index d95594f8f61c91..942d4c72925724 100644 --- a/ee/app/models/product_analytics/jitsu_authentication.rb +++ b/ee/app/models/product_analytics/jitsu_authentication.rb @@ -26,11 +26,21 @@ def create_api_key! }.to_json ) - if response.success? - json = Gitlab::Json.parse(response.body) + json = Gitlab::Json.parse(response.body) - return { jsAuth: json['jsAuth'], uid: json['uid'] } - end + return { jsAuth: json['jsAuth'], uid: json['uid'] } if response.success? + + Gitlab::AppLogger.error( + message: 'Jitsu API error', + error: json['error'], + jitsu_error_message: json['message'], + project: @project.id, + job_id: @jid + ) + + nil + rescue StandardError => e + Gitlab::ErrorTracking.track_exception(e) nil end @@ -59,7 +69,22 @@ def create_clickhouse_destination! }.to_json ) - response.success? ? id : nil + return id if response.success? + + json = Gitlab::Json.parse(response.body) + Gitlab::AppLogger.error( + message: 'Jitsu API error', + error: json['error'], + jitsu_error_message: json['message'], + project: @project.id, + job_id: @jid + ) + + nil + rescue StandardError => e + Gitlab::ErrorTracking.track_exception(e) + + nil end def generate_access_token @@ -73,7 +98,21 @@ def generate_access_token }.to_json ) - return Gitlab::Json.parse(response.body)['access_token'] if response.success? + json = Gitlab::Json.parse(response.body) + + return json['access_token'] if response.success? + + Gitlab::AppLogger.error( + message: 'Jitsu API error', + error: json['error'], + jitsu_error_message: json['message'], + project: @project.id, + job_id: @jid + ) + + nil + rescue StandardError => e + Gitlab::ErrorTracking.track_exception(e) nil end diff --git a/ee/spec/models/product_analytics/jitsu_authentication_spec.rb b/ee/spec/models/product_analytics/jitsu_authentication_spec.rb index 9c5b41c9a16bad..387e80a01c5de5 100644 --- a/ee/spec/models/product_analytics/jitsu_authentication_spec.rb +++ b/ee/spec/models/product_analytics/jitsu_authentication_spec.rb @@ -33,7 +33,16 @@ stub_signin_failure end - it { is_expected.to be_nil } + it 'returns nil and logs the error' do + expect(Gitlab::AppLogger).to receive(:error).with( + message: 'Jitsu API error', + error: 'invalid password', + jitsu_error_message: 'Authorization failed: invalid password', + project: project.id, + job_id: jid + ) + expect(subject).to be_nil + end end end @@ -57,7 +66,16 @@ allow(auth).to receive(:access_token).and_return('testtoken') end - it { is_expected.to be_nil } + it 'returns nil and logs the error' do + expect(Gitlab::AppLogger).to receive(:error).with( + message: 'Jitsu API error', + error: 'token required', + jitsu_error_message: 'Authorization failed: token required', + project: project.id, + job_id: jid + ) + expect(subject).to be_nil + end end end @@ -67,8 +85,8 @@ context 'when request is successful' do before do stub_signin_success - stub_clickhouse_success stub_api_key_success + stub_clickhouse_success allow(auth).to receive(:access_token).and_return('testtoken') end @@ -83,7 +101,16 @@ allow(auth).to receive(:access_token).and_return('testtoken') end - it { is_expected.to be_nil } + it 'returns nil and logs the error' do + expect(Gitlab::AppLogger).to receive(:error).with( + message: 'Jitsu API error', + error: 'token required', + jitsu_error_message: 'Authorization failed: token required', + project: project.id, + job_id: jid + ) + expect(subject).to be_nil + end end end @@ -112,7 +139,9 @@ def stub_api_key_success def stub_api_key_failure stub_request(:post, "http://jitsu.dev/configurator/api/v2/objects/testtesttesttestprj/api_keys") - .to_return(status: 500) + .to_return(status: 401, + body: { error: 'token required', message: 'Authorization failed: token required' }.to_json, + headers: {}) end def stub_clickhouse_success @@ -122,6 +151,8 @@ def stub_clickhouse_success def stub_clickhouse_failure stub_request(:post, "http://jitsu.dev/configurator/api/v2/objects/testtesttesttestprj/destinations") - .to_return(status: 500) + .to_return(status: 401, + body: { error: 'token required', message: 'Authorization failed: token required' }.to_json, + headers: {}) end end -- GitLab From caaf85bf847fd9c854af87e7f26cfc525988f905 Mon Sep 17 00:00:00 2001 From: Robert Hunt Date: Fri, 23 Sep 2022 16:04:43 +0100 Subject: [PATCH 7/9] Remove unrelated structure.sql change Signed-off-by: Robert Hunt --- db/structure.sql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/db/structure.sql b/db/structure.sql index 489c7c78e36946..3eefc98897a5ed 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -30878,7 +30878,7 @@ CREATE INDEX tmp_index_on_vulnerabilities_non_dismissed ON vulnerabilities USING CREATE INDEX tmp_index_project_statistics_cont_registry_size ON project_statistics USING btree (project_id) WHERE (container_registry_size = 0); -CREATE INDEX tmp_index_system_note_metadata_on_attention_request_actions ON system_note_metadata USING btree (id) WHERE ((action)::text = ANY (ARRAY[('attention_requested'::character varying)::text, ('attention_request_removed'::character varying)::text])); +CREATE INDEX tmp_index_system_note_metadata_on_attention_request_actions ON system_note_metadata USING btree (id) WHERE ((action)::text = ANY ((ARRAY['attention_requested'::character varying, 'attention_request_removed'::character varying])::text[])); CREATE INDEX tmp_index_vulnerability_occurrences_on_id_and_scanner_id ON vulnerability_occurrences USING btree (id, scanner_id) WHERE (report_type = ANY (ARRAY[7, 99])); -- GitLab From 92e4dd1ac44ed539516297d64e8f31c3080e8482 Mon Sep 17 00:00:00 2001 From: Robert Hunt Date: Fri, 23 Sep 2022 18:23:50 +0100 Subject: [PATCH 8/9] Add exception tests for each jitsu api method Signed-off-by: Robert Hunt --- .../jitsu_authentication_spec.rb | 54 +++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/ee/spec/models/product_analytics/jitsu_authentication_spec.rb b/ee/spec/models/product_analytics/jitsu_authentication_spec.rb index 387e80a01c5de5..32f897f1aa3236 100644 --- a/ee/spec/models/product_analytics/jitsu_authentication_spec.rb +++ b/ee/spec/models/product_analytics/jitsu_authentication_spec.rb @@ -44,6 +44,17 @@ expect(subject).to be_nil end end + + context 'when request throws an exception' do + before do + stub_signin_exception + end + + it 'logs the exception' do + expect(Gitlab::ErrorTracking).to receive(:track_exception).with(instance_of(Gitlab::HTTP::Error)) + expect(subject).to be_nil + end + end end describe '#create_api_key!' do @@ -77,6 +88,19 @@ expect(subject).to be_nil end end + + context 'when request throws an exception' do + before do + stub_signin_success + stub_api_key_exception + allow(auth).to receive(:access_token).and_return('testtoken') + end + + it 'logs the exception' do + expect(Gitlab::ErrorTracking).to receive(:track_exception).with(instance_of(Gitlab::HTTP::Error)) + expect(subject).to be_nil + end + end end describe '#create_clickhouse_destination' do @@ -112,6 +136,20 @@ expect(subject).to be_nil end end + + context 'when request throws an exception' do + before do + stub_signin_success + stub_api_key_success + stub_clickhouse_exception + allow(auth).to receive(:access_token).and_return('testtoken') + end + + it 'logs the exception' do + expect(Gitlab::ErrorTracking).to receive(:track_exception).with(instance_of(Gitlab::HTTP::Error)) + expect(subject).to be_nil + end + end end private @@ -130,6 +168,12 @@ def stub_signin_failure headers: {}) end + def stub_signin_exception + stub_request(:post, "http://jitsu.dev/configurator/api/v1/users/signin") + .with(body: "{\"email\":\"test@test.com\",\"password\":\"testtest\"}") + .to_raise(Gitlab::HTTP::Error) + end + def stub_api_key_success stub_request(:post, "http://jitsu.dev/configurator/api/v2/objects/testtesttesttestprj/api_keys") .to_return(status: 200, @@ -144,6 +188,11 @@ def stub_api_key_failure headers: {}) end + def stub_api_key_exception + stub_request(:post, "http://jitsu.dev/configurator/api/v2/objects/testtesttesttestprj/api_keys") + .to_raise(Gitlab::HTTP::Error) + end + def stub_clickhouse_success stub_request(:post, "http://jitsu.dev/configurator/api/v2/objects/testtesttesttestprj/destinations") .to_return(status: 200) @@ -155,4 +204,9 @@ def stub_clickhouse_failure body: { error: 'token required', message: 'Authorization failed: token required' }.to_json, headers: {}) end + + def stub_clickhouse_exception + stub_request(:post, "http://jitsu.dev/configurator/api/v2/objects/testtesttesttestprj/destinations") + .to_raise(Gitlab::HTTP::Error) + end end -- GitLab From 6b707b7b08de5950137e3496079d6ebeca2f9177 Mon Sep 17 00:00:00 2001 From: Robert Hunt Date: Tue, 27 Sep 2022 09:58:01 +0100 Subject: [PATCH 9/9] DRY up the error tracking implementation Signed-off-by: Robert Hunt --- .../product_analytics/jitsu_authentication.rb | 45 +++-------- .../jitsu_authentication_spec.rb | 79 +++++++++---------- 2 files changed, 47 insertions(+), 77 deletions(-) diff --git a/ee/app/models/product_analytics/jitsu_authentication.rb b/ee/app/models/product_analytics/jitsu_authentication.rb index 942d4c72925724..bdf6b71b9ea87e 100644 --- a/ee/app/models/product_analytics/jitsu_authentication.rb +++ b/ee/app/models/product_analytics/jitsu_authentication.rb @@ -28,21 +28,9 @@ def create_api_key! json = Gitlab::Json.parse(response.body) - return { jsAuth: json['jsAuth'], uid: json['uid'] } if response.success? - - Gitlab::AppLogger.error( - message: 'Jitsu API error', - error: json['error'], - jitsu_error_message: json['message'], - project: @project.id, - job_id: @jid - ) - - nil + response.success? ? { jsAuth: json['jsAuth'], uid: json['uid'] } : log_jitsu_api_error(json) rescue StandardError => e Gitlab::ErrorTracking.track_exception(e) - - nil end def create_clickhouse_destination! @@ -69,22 +57,9 @@ def create_clickhouse_destination! }.to_json ) - return id if response.success? - - json = Gitlab::Json.parse(response.body) - Gitlab::AppLogger.error( - message: 'Jitsu API error', - error: json['error'], - jitsu_error_message: json['message'], - project: @project.id, - job_id: @jid - ) - - nil + response.success? ? id : log_jitsu_api_error(Gitlab::Json.parse(response.body)) rescue StandardError => e Gitlab::ErrorTracking.track_exception(e) - - nil end def generate_access_token @@ -100,21 +75,21 @@ def generate_access_token json = Gitlab::Json.parse(response.body) - return json['access_token'] if response.success? + response.success? ? json['access_token'] : log_jitsu_api_error(json) + rescue StandardError => e + Gitlab::ErrorTracking.track_exception(e) + end + + private + def log_jitsu_api_error(json) Gitlab::AppLogger.error( message: 'Jitsu API error', error: json['error'], jitsu_error_message: json['message'], - project: @project.id, + project_id: @project.id, job_id: @jid ) - - nil - rescue StandardError => e - Gitlab::ErrorTracking.track_exception(e) - - nil end end end diff --git a/ee/spec/models/product_analytics/jitsu_authentication_spec.rb b/ee/spec/models/product_analytics/jitsu_authentication_spec.rb index 32f897f1aa3236..951a47ca0b2c5b 100644 --- a/ee/spec/models/product_analytics/jitsu_authentication_spec.rb +++ b/ee/spec/models/product_analytics/jitsu_authentication_spec.rb @@ -4,6 +4,8 @@ RSpec.describe ProductAnalytics::JitsuAuthentication do let(:jid) { '12345678' } + let(:error_message) { '' } + let(:jitsu_error_message) { '' } let_it_be(:project) { create(:project) } subject(:auth) { described_class.new(jid, project) } @@ -17,6 +19,26 @@ ) end + shared_examples 'returns nil and logs the API error' do + it do + expect(Gitlab::AppLogger).to receive(:error).with( + message: 'Jitsu API error', + error: error_message, + jitsu_error_message: jitsu_error_message, + project_id: project.id, + job_id: jid + ) + expect(subject).to be_nil + end + end + + shared_examples 'returns nil and logs the exception' do + it do + expect(Gitlab::ErrorTracking).to receive(:track_exception).with(instance_of(Gitlab::HTTP::Error)) + expect(subject).to be_nil + end + end + describe '#generate_access_token' do subject { auth.generate_access_token } @@ -29,20 +51,14 @@ end context 'when request is unsuccessful' do + let(:error_message) { 'invalid password' } + let(:jitsu_error_message) { 'Authorization failed: invalid password' } + before do stub_signin_failure end - it 'returns nil and logs the error' do - expect(Gitlab::AppLogger).to receive(:error).with( - message: 'Jitsu API error', - error: 'invalid password', - jitsu_error_message: 'Authorization failed: invalid password', - project: project.id, - job_id: jid - ) - expect(subject).to be_nil - end + it_behaves_like 'returns nil and logs the API error' end context 'when request throws an exception' do @@ -50,10 +66,7 @@ stub_signin_exception end - it 'logs the exception' do - expect(Gitlab::ErrorTracking).to receive(:track_exception).with(instance_of(Gitlab::HTTP::Error)) - expect(subject).to be_nil - end + it_behaves_like 'returns nil and logs the exception' end end @@ -71,22 +84,16 @@ end context 'when request is unsuccessful' do + let(:error_message) { 'token required' } + let(:jitsu_error_message) { 'Authorization failed: token required' } + before do stub_signin_success stub_api_key_failure allow(auth).to receive(:access_token).and_return('testtoken') end - it 'returns nil and logs the error' do - expect(Gitlab::AppLogger).to receive(:error).with( - message: 'Jitsu API error', - error: 'token required', - jitsu_error_message: 'Authorization failed: token required', - project: project.id, - job_id: jid - ) - expect(subject).to be_nil - end + it_behaves_like 'returns nil and logs the API error' end context 'when request throws an exception' do @@ -96,10 +103,7 @@ allow(auth).to receive(:access_token).and_return('testtoken') end - it 'logs the exception' do - expect(Gitlab::ErrorTracking).to receive(:track_exception).with(instance_of(Gitlab::HTTP::Error)) - expect(subject).to be_nil - end + it_behaves_like 'returns nil and logs the exception' end end @@ -118,6 +122,9 @@ end context 'when request is unsuccessful' do + let(:error_message) { 'token required' } + let(:jitsu_error_message) { 'Authorization failed: token required' } + before do stub_signin_success stub_api_key_success @@ -125,16 +132,7 @@ allow(auth).to receive(:access_token).and_return('testtoken') end - it 'returns nil and logs the error' do - expect(Gitlab::AppLogger).to receive(:error).with( - message: 'Jitsu API error', - error: 'token required', - jitsu_error_message: 'Authorization failed: token required', - project: project.id, - job_id: jid - ) - expect(subject).to be_nil - end + it_behaves_like 'returns nil and logs the API error' end context 'when request throws an exception' do @@ -145,10 +143,7 @@ allow(auth).to receive(:access_token).and_return('testtoken') end - it 'logs the exception' do - expect(Gitlab::ErrorTracking).to receive(:track_exception).with(instance_of(Gitlab::HTTP::Error)) - expect(subject).to be_nil - end + it_behaves_like 'returns nil and logs the exception' end end -- GitLab