Merge branch '41722-track-gcp-billing-enabled-project-changes' into 'master'

Resolve "Track GCP Billing enabled project changes"

Closes #41722

See merge request gitlab-org/gitlab-ce!16962

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'
+    when nil
-      return
-    when 'false'
-      flash[:alert] = _('Please <a href=%{link_to_billing} target="_blank" rel="noopener noreferrer">enable billing for one of your projects to be able to create a Kubernetes cluster</a>, 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
       flash[:alert] = _('We could not verify that one of your projects on GCP has billing enabled. Please try again.')
+    when false
+      flash[:alert] = _('Please <a href=%{link_to_billing} target="_blank" rel="noopener noreferrer">enable billing for one of your projects to be able to create a Kubernetes cluster</a>, 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|
+    CheckGcpProjectBillingWorker.get_billing_state(token_in_session)
-      redis.get(CheckGcpProjectBillingWorker.redis_shared_state_key_for(token_in_session))
-    end
   end
 
   def token_in_session

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)
+  def self.get_billing_state(token)
-    "gitlab:gcp:#{Digest::SHA1.hexdigest(token)}:billing_enabled"
+    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)
+    billing_enabled_state = !CheckGcpProjectBillingService.new.execute(token).empty?
-    Gitlab::Redis::SharedState.with do |redis|
+    update_billing_change_counter(self.class.get_billing_state(token), billing_enabled_state)
-      redis.set(self.class.redis_shared_state_key_for(token),
+    self.class.set_billing_state(token, billing_enabled_state)
-        !billing_enabled_projects.empty?,
-        ex: BILLING_TIMEOUT)
-    end
   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

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)
 

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(described_class).to receive(:set_billing_state).with(token, true)
-          expect(redis_double).to receive(:set).with(described_class.redis_shared_state_key_for(token), anything, anything)
 
           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