diff --git a/app/controllers/projects/clusters/gcp_controller.rb b/app/controllers/projects/clusters/gcp_controller.rb index 94d33b91562..0f41af7d87b 100644 --- a/app/controllers/projects/clusters/gcp_controller.rb +++ b/app/controllers/projects/clusters/gcp_controller.rb @@ -39,12 +39,12 @@ class Projects::Clusters::GcpController < Projects::ApplicationController def verify_billing case google_project_billing_status - when 'true' - return - when 'false' - flash[:alert] = _('Please enable billing for one of your projects to be able to create a Kubernetes cluster, then try again.').html_safe % { link_to_billing: "https://console.cloud.google.com/freetrial?utm_campaign=2018_cpanel&utm_source=gitlab&utm_medium=referral" } - else + when nil flash[:alert] = _('We could not verify that one of your projects on GCP has billing enabled. Please try again.') + when false + flash[:alert] = _('Please enable billing for one of your projects to be able to create a Kubernetes cluster, then try again.').html_safe % { link_to_billing: "https://console.cloud.google.com/freetrial?utm_campaign=2018_cpanel&utm_source=gitlab&utm_medium=referral" } + when true + return end @cluster = ::Clusters::Cluster.new(create_params) @@ -81,9 +81,7 @@ class Projects::Clusters::GcpController < Projects::ApplicationController end def google_project_billing_status - Gitlab::Redis::SharedState.with do |redis| - redis.get(CheckGcpProjectBillingWorker.redis_shared_state_key_for(token_in_session)) - end + CheckGcpProjectBillingWorker.get_billing_state(token_in_session) end def token_in_session diff --git a/app/workers/check_gcp_project_billing_worker.rb b/app/workers/check_gcp_project_billing_worker.rb index 5466ccdda59..363f81590ab 100644 --- a/app/workers/check_gcp_project_billing_worker.rb +++ b/app/workers/check_gcp_project_billing_worker.rb @@ -7,6 +7,7 @@ class CheckGcpProjectBillingWorker LEASE_TIMEOUT = 3.seconds.to_i SESSION_KEY_TIMEOUT = 5.minutes BILLING_TIMEOUT = 1.hour + BILLING_CHANGED_LABELS = { state_transition: nil }.freeze def self.get_session_token(token_key) Gitlab::Redis::SharedState.with do |redis| @@ -22,8 +23,11 @@ class CheckGcpProjectBillingWorker end end - def self.redis_shared_state_key_for(token) - "gitlab:gcp:#{Digest::SHA1.hexdigest(token)}:billing_enabled" + def self.get_billing_state(token) + Gitlab::Redis::SharedState.with do |redis| + value = redis.get(redis_shared_state_key_for(token)) + ActiveRecord::Type::Boolean.new.type_cast_from_user(value) + end end def perform(token_key) @@ -33,12 +37,9 @@ class CheckGcpProjectBillingWorker return unless token return unless try_obtain_lease_for(token) - billing_enabled_projects = CheckGcpProjectBillingService.new.execute(token) - Gitlab::Redis::SharedState.with do |redis| - redis.set(self.class.redis_shared_state_key_for(token), - !billing_enabled_projects.empty?, - ex: BILLING_TIMEOUT) - end + billing_enabled_state = !CheckGcpProjectBillingService.new.execute(token).empty? + update_billing_change_counter(self.class.get_billing_state(token), billing_enabled_state) + self.class.set_billing_state(token, billing_enabled_state) end private @@ -51,9 +52,41 @@ class CheckGcpProjectBillingWorker "gitlab:gcp:session:#{token_key}" end + def self.redis_shared_state_key_for(token) + "gitlab:gcp:#{Digest::SHA1.hexdigest(token)}:billing_enabled" + end + + def self.set_billing_state(token, value) + Gitlab::Redis::SharedState.with do |redis| + redis.set(redis_shared_state_key_for(token), value, ex: BILLING_TIMEOUT) + end + end + def try_obtain_lease_for(token) Gitlab::ExclusiveLease .new("check_gcp_project_billing_worker:#{token.hash}", timeout: LEASE_TIMEOUT) .try_obtain end + + def billing_changed_counter + @billing_changed_counter ||= Gitlab::Metrics.counter( + :gcp_billing_change_count, + "Counts the number of times a GCP project changed billing_enabled state from false to true", + BILLING_CHANGED_LABELS + ) + end + + def state_transition(previous_state, current_state) + if previous_state.nil? && !current_state + 'no_billing' + elsif previous_state.nil? && current_state + 'with_billing' + elsif !previous_state && current_state + 'billing_configured' + end + end + + def update_billing_change_counter(previous_state, current_state) + billing_changed_counter.increment(state_transition: state_transition(previous_state, current_state)) + end end diff --git a/spec/features/projects/clusters/gcp_spec.rb b/spec/features/projects/clusters/gcp_spec.rb index 02dbd3380b3..4d47cdb500c 100644 --- a/spec/features/projects/clusters/gcp_spec.rb +++ b/spec/features/projects/clusters/gcp_spec.rb @@ -25,7 +25,7 @@ feature 'Gcp Cluster', :js do context 'when user has a GCP project with billing enabled' do before do allow_any_instance_of(Projects::Clusters::GcpController).to receive(:authorize_google_project_billing) - allow_any_instance_of(Projects::Clusters::GcpController).to receive(:google_project_billing_status).and_return('true') + allow_any_instance_of(Projects::Clusters::GcpController).to receive(:google_project_billing_status).and_return(true) end context 'when user does not have a cluster and visits cluster index page' do @@ -134,7 +134,7 @@ feature 'Gcp Cluster', :js do context 'when user does not have a GCP project with billing enabled' do before do allow_any_instance_of(Projects::Clusters::GcpController).to receive(:authorize_google_project_billing) - allow_any_instance_of(Projects::Clusters::GcpController).to receive(:google_project_billing_status).and_return('false') + allow_any_instance_of(Projects::Clusters::GcpController).to receive(:google_project_billing_status).and_return(false) visit project_clusters_path(project) diff --git a/spec/workers/check_gcp_project_billing_worker_spec.rb b/spec/workers/check_gcp_project_billing_worker_spec.rb index 7b7a7c1bc44..526ecf75921 100644 --- a/spec/workers/check_gcp_project_billing_worker_spec.rb +++ b/spec/workers/check_gcp_project_billing_worker_spec.rb @@ -6,6 +6,11 @@ describe CheckGcpProjectBillingWorker do subject { described_class.new.perform('token_key') } + before do + allow(described_class).to receive(:get_billing_state) + allow_any_instance_of(described_class).to receive(:update_billing_change_counter) + end + context 'when there is a token in redis' do before do allow(described_class).to receive(:get_session_token).and_return(token) @@ -23,11 +28,8 @@ describe CheckGcpProjectBillingWorker do end it 'stores billing status in redis' do - redis_double = double - expect(CheckGcpProjectBillingService).to receive_message_chain(:new, :execute).and_return([double]) - expect(Gitlab::Redis::SharedState).to receive(:with).and_yield(redis_double) - expect(redis_double).to receive(:set).with(described_class.redis_shared_state_key_for(token), anything, anything) + expect(described_class).to receive(:set_billing_state).with(token, true) subject end @@ -48,7 +50,7 @@ describe CheckGcpProjectBillingWorker do context 'when there is no token in redis' do before do - allow_any_instance_of(described_class).to receive(:get_session_token).and_return(nil) + allow(described_class).to receive(:get_session_token).and_return(nil) end it 'does not call the service' do @@ -58,4 +60,57 @@ describe CheckGcpProjectBillingWorker do end end end + + describe 'billing change counter' do + subject { described_class.new.perform('token_key') } + + before do + allow(described_class).to receive(:get_session_token).and_return('bogustoken') + allow_any_instance_of(described_class).to receive(:try_obtain_lease_for).and_return('randomuuid') + allow(described_class).to receive(:set_billing_state) + end + + context 'when previous state was false' do + before do + expect(described_class).to receive(:get_billing_state).and_return(false) + end + + context 'when the current state is false' do + before do + expect(CheckGcpProjectBillingService).to receive_message_chain(:new, :execute).and_return([]) + end + + it 'increments the billing change counter' do + expect_any_instance_of(described_class).to receive_message_chain(:billing_changed_counter, :increment) + + subject + end + end + + context 'when the current state is true' do + before do + expect(CheckGcpProjectBillingService).to receive_message_chain(:new, :execute).and_return([double]) + end + + it 'increments the billing change counter' do + expect_any_instance_of(described_class).to receive_message_chain(:billing_changed_counter, :increment) + + subject + end + end + end + + context 'when previous state was true' do + before do + expect(described_class).to receive(:get_billing_state).and_return(true) + expect(CheckGcpProjectBillingService).to receive_message_chain(:new, :execute).and_return([double]) + end + + it 'increment the billing change counter' do + expect_any_instance_of(described_class).to receive_message_chain(:billing_changed_counter, :increment) + + subject + end + end + end end