Fix WIF api call options (#174)

* Fix retry options to Google API calls that are causing errors in GIT * Cater for the max elapsed retry durection for all retries in the token expiry leeway * Fix syntax used for enabling/disabling cop Style/ClassVars * Fix out of date Gemfile.lock * Hold durations as integers in constants to avoid use of ActiveSupport * Fix events_client test stub for WIF
DFE-Digital · Nov 25, 2024 · 75d06fb · 75d06fb
1 parent 2feaba1
commit 75d06fb
Show file tree

Hide file tree

Showing 7 changed files with 92 additions and 71 deletions.
diff --git a/Gemfile.lock b/Gemfile.lock
@@ -10,65 +10,66 @@ PATH
 GEM
   remote: https://rubygems.org/
   specs:
-    actioncable (8.0.0)
-      actionpack (= 8.0.0)
-      activesupport (= 8.0.0)
+    actioncable (7.2.2)
+      actionpack (= 7.2.2)
+      activesupport (= 7.2.2)
       nio4r (~> 2.0)
       websocket-driver (>= 0.6.1)
       zeitwerk (~> 2.6)
-    actionmailbox (8.0.0)
-      actionpack (= 8.0.0)
-      activejob (= 8.0.0)
-      activerecord (= 8.0.0)
-      activestorage (= 8.0.0)
-      activesupport (= 8.0.0)
+    actionmailbox (7.2.2)
+      actionpack (= 7.2.2)
+      activejob (= 7.2.2)
+      activerecord (= 7.2.2)
+      activestorage (= 7.2.2)
+      activesupport (= 7.2.2)
       mail (>= 2.8.0)
-    actionmailer (8.0.0)
-      actionpack (= 8.0.0)
-      actionview (= 8.0.0)
-      activejob (= 8.0.0)
-      activesupport (= 8.0.0)
+    actionmailer (7.2.2)
+      actionpack (= 7.2.2)
+      actionview (= 7.2.2)
+      activejob (= 7.2.2)
+      activesupport (= 7.2.2)
       mail (>= 2.8.0)
       rails-dom-testing (~> 2.2)
-    actionpack (8.0.0)
-      actionview (= 8.0.0)
-      activesupport (= 8.0.0)
+    actionpack (7.2.2)
+      actionview (= 7.2.2)
+      activesupport (= 7.2.2)
       nokogiri (>= 1.8.5)
-      rack (>= 2.2.4)
+      racc
+      rack (>= 2.2.4, < 3.2)
       rack-session (>= 1.0.1)
       rack-test (>= 0.6.3)
       rails-dom-testing (~> 2.2)
       rails-html-sanitizer (~> 1.6)
       useragent (~> 0.16)
-    actiontext (8.0.0)
-      actionpack (= 8.0.0)
-      activerecord (= 8.0.0)
-      activestorage (= 8.0.0)
-      activesupport (= 8.0.0)
+    actiontext (7.2.2)
+      actionpack (= 7.2.2)
+      activerecord (= 7.2.2)
+      activestorage (= 7.2.2)
+      activesupport (= 7.2.2)
       globalid (>= 0.6.0)
       nokogiri (>= 1.8.5)
-    actionview (8.0.0)
-      activesupport (= 8.0.0)
+    actionview (7.2.2)
+      activesupport (= 7.2.2)
       builder (~> 3.1)
       erubi (~> 1.11)
       rails-dom-testing (~> 2.2)
       rails-html-sanitizer (~> 1.6)
-    activejob (8.0.0)
-      activesupport (= 8.0.0)
+    activejob (7.2.2)
+      activesupport (= 7.2.2)
       globalid (>= 0.3.6)
-    activemodel (8.0.0)
-      activesupport (= 8.0.0)
-    activerecord (8.0.0)
-      activemodel (= 8.0.0)
-      activesupport (= 8.0.0)
+    activemodel (7.2.2)
+      activesupport (= 7.2.2)
+    activerecord (7.2.2)
+      activemodel (= 7.2.2)
+      activesupport (= 7.2.2)
       timeout (>= 0.4.0)
-    activestorage (8.0.0)
-      actionpack (= 8.0.0)
-      activejob (= 8.0.0)
-      activerecord (= 8.0.0)
-      activesupport (= 8.0.0)
+    activestorage (7.2.2)
+      actionpack (= 7.2.2)
+      activejob (= 7.2.2)
+      activerecord (= 7.2.2)
+      activesupport (= 7.2.2)
       marcel (~> 1.0)
-    activesupport (8.0.0)
+    activesupport (7.2.2)
       base64
       benchmark (>= 0.3)
       bigdecimal
@@ -80,7 +81,6 @@ GEM
       minitest (>= 5.1)
       securerandom (>= 0.3)
       tzinfo (~> 2.0, >= 2.0.5)
-      uri (>= 0.13.1)
     addressable (2.8.7)
       public_suffix (>= 2.0.2, < 7.0)
     appraisal (2.5.0)
@@ -286,30 +286,30 @@ GEM
       rack (>= 1.3)
     rackup (2.2.0)
       rack (>= 3)
-    rails (8.0.0)
-      actioncable (= 8.0.0)
-      actionmailbox (= 8.0.0)
-      actionmailer (= 8.0.0)
-      actionpack (= 8.0.0)
-      actiontext (= 8.0.0)
-      actionview (= 8.0.0)
-      activejob (= 8.0.0)
-      activemodel (= 8.0.0)
-      activerecord (= 8.0.0)
-      activestorage (= 8.0.0)
-      activesupport (= 8.0.0)
+    rails (7.2.2)
+      actioncable (= 7.2.2)
+      actionmailbox (= 7.2.2)
+      actionmailer (= 7.2.2)
+      actionpack (= 7.2.2)
+      actiontext (= 7.2.2)
+      actionview (= 7.2.2)
+      activejob (= 7.2.2)
+      activemodel (= 7.2.2)
+      activerecord (= 7.2.2)
+      activestorage (= 7.2.2)
+      activesupport (= 7.2.2)
       bundler (>= 1.15.0)
-      railties (= 8.0.0)
+      railties (= 7.2.2)
     rails-dom-testing (2.2.0)
       activesupport (>= 5.0.0)
       minitest
       nokogiri (>= 1.6)
     rails-html-sanitizer (1.6.0)
       loofah (~> 2.21)
       nokogiri (~> 1.14)
-    railties (8.0.0)
-      actionpack (= 8.0.0)
-      activesupport (= 8.0.0)
+    railties (7.2.2)
+      actionpack (= 7.2.2)
+      activesupport (= 7.2.2)
       irb (~> 1.13)
       rackup (>= 1.0.0)
       rake (>= 12.2)
@@ -341,7 +341,7 @@ GEM
     rspec-mocks (3.13.2)
       diff-lcs (>= 1.2.0, < 2.0)
       rspec-support (~> 3.13.0)
-    rspec-rails (7.0.1)
+    rspec-rails (7.1.0)
       actionpack (>= 7.0)
       activesupport (>= 7.0)
       railties (>= 7.0)
@@ -432,7 +432,7 @@ GEM
     with_model (2.1.7)
       activerecord (>= 6.0)
     yard (0.9.37)
-    zeitwerk (2.7.1)
+    zeitwerk (2.6.18)
 
 PLATFORMS
   aarch64-linux

diff --git a/lib/dfe/analytics/azure_federated_auth.rb b/lib/dfe/analytics/azure_federated_auth.rb
@@ -8,7 +8,7 @@ module Analytics
     class AzureFederatedAuth
       DEFAULT_AZURE_SCOPE = 'api://AzureADTokenExchange/.default'
       DEFAULT_GCP_SCOPE   = 'https://www.googleapis.com/auth/cloud-platform'
-      ACCESS_TOKEN_EXPIRE_TIME_LEEWAY = 10.seconds
+      ACCESS_TOKEN_EXPIRE_TIME_LEEWAY = 10
 
       def self.gcp_client_credentials
         return @gcp_client_credentials if @gcp_client_credentials && !@gcp_client_credentials.expired?
@@ -19,7 +19,9 @@ def self.gcp_client_credentials
 
         google_token, expire_time = google_access_token(azure_google_exchange_token)
 
-        expire_time_with_leeway = expire_time.to_datetime - ACCESS_TOKEN_EXPIRE_TIME_LEEWAY
+        # Expire time with leeway that includes the max time for any retries
+        expire_time_with_leeway =
+          expire_time.to_datetime - ACCESS_TOKEN_EXPIRE_TIME_LEEWAY.seconds - DfE::Analytics::BigQueryApi::ALL_RETRIES_MAX_ELASPED_TIME.seconds
 
         @gcp_client_credentials = Google::Auth::UserRefreshCredentials.new(access_token: google_token, expires_at: expire_time_with_leeway)
       end

diff --git a/lib/dfe/analytics/big_query_api.rb b/lib/dfe/analytics/big_query_api.rb
@@ -2,8 +2,14 @@
 
 module DfE
   module Analytics
-    # For use with for workload identity federeation
+    # For use with for workload identity federation
     class BigQueryApi
+      # All times are in seconds
+      ALL_RETRIES_MAX_ELASPED_TIME = 120
+      RETRY_INITIAL_BASE_INTERVAL = 15
+      RETRY_MAX_INTERVAL = 60
+      RETRY_INTERVAL_MULTIPLIER = 2
+
       def self.events_client
         @events_client ||= begin
           missing_config = %i[
@@ -27,9 +33,16 @@ def self.events_client
       end
 
       def self.insert(events)
-        rows         = events.map { |event| { json: event } }
-        data_request = Google::Apis::BigqueryV2::InsertAllTableDataRequest.new(rows: rows, skip_invalid_rows: true)
-        options      = Google::Apis::RequestOptions.new(retries: DfE::Analytics.config.bigquery_retries)
+        rows            = events.map { |event| { json: event } }
+        data_request    = Google::Apis::BigqueryV2::InsertAllTableDataRequest.new(rows: rows, skip_invalid_rows: true)
+        options         = Google::Apis::RequestOptions.default
+
+        options.authorization    = events_client.authorization
+        options.retries          = DfE::Analytics.config.bigquery_retries
+        options.max_elapsed_time = ALL_RETRIES_MAX_ELASPED_TIME
+        options.base_interval    = RETRY_INITIAL_BASE_INTERVAL
+        options.max_interval     = RETRY_MAX_INTERVAL
+        options.multiplier       = RETRY_INTERVAL_MULTIPLIER
 
         response =
           events_client.insert_all_table_data(

diff --git a/lib/dfe/analytics/initialisation_events.rb b/lib/dfe/analytics/initialisation_events.rb
@@ -7,8 +7,8 @@ module Analytics
     # - Event contains the dfe analytics version, config and other items
     class InitialisationEvents
       # Disable rubocop class variable warnings for class - class variable required to control sending of event
-      # rubocop:disable Style:ClassVars
-      @@initialisation_events_sent = false # rubocop:disable Style:ClassVars
+      # rubocop:disable Style/ClassVars
+      @@initialisation_events_sent = false # rubocop:disable Style/ClassVars
 
       def self.trigger_initialisation_events
         new.send_initialisation_events
@@ -19,7 +19,7 @@ def self.initialisation_events_sent?
       end
 
       def self.initialisation_events_sent=(value)
-        @@initialisation_events_sent = value # rubocop:disable Style:ClassVars
+        @@initialisation_events_sent = value # rubocop:disable Style/ClassVars
       end
 
       def send_initialisation_events
@@ -32,7 +32,7 @@ def send_initialisation_events
 
         DfE::Analytics::SendEvents.perform_for([initialise_analytics_event])
 
-        @@initialisation_events_sent = true # rubocop:disable Style:ClassVars
+        @@initialisation_events_sent = true # rubocop:disable Style/ClassVars
       end
 
       private
@@ -45,7 +45,7 @@ def initialise_analytics_data
           }
         }
       end
-      # rubocop:enable Style:ClassVars
+      # rubocop:enable Style/ClassVars
     end
   end
 end
diff --git a/lib/dfe/analytics/testing.rb b/lib/dfe/analytics/testing.rb
@@ -66,9 +66,11 @@ def events_client
     class StubClient
       Response = Struct.new(:insert_errors)
 
-      def insert(*)
+      def insert_all_table_data(*)
         Response.new([])
       end
+
+      def authorization; end
     end
 
     module TestOverrides

diff --git a/spec/dfe/analytics/azure_federated_auth_spec.rb b/spec/dfe/analytics/azure_federated_auth_spec.rb
@@ -105,8 +105,11 @@
     it 'returns the expected client credentials' do
       expect(described_class.gcp_client_credentials).to be_an_instance_of(Google::Auth::UserRefreshCredentials)
       expect(described_class.gcp_client_credentials.access_token).to eq(google_access_token)
-      expect(described_class.gcp_client_credentials.expires_at)
-        .to be_within(DfE::Analytics::AzureFederatedAuth::ACCESS_TOKEN_EXPIRE_TIME_LEEWAY).of(future_expire_time)
+
+      expiry_leeway_duration =
+        DfE::Analytics::BigQueryApi::ALL_RETRIES_MAX_ELASPED_TIME.seconds + DfE::Analytics::AzureFederatedAuth::ACCESS_TOKEN_EXPIRE_TIME_LEEWAY.seconds
+
+      expect(described_class.gcp_client_credentials.expires_at).to be_within(expiry_leeway_duration).of(future_expire_time)
     end
 
     context 'token expiry' do

diff --git a/spec/dfe/analytics/big_query_api_spec.rb b/spec/dfe/analytics/big_query_api_spec.rb
@@ -20,6 +20,7 @@
     allow(Google::Apis::BigqueryV2::BigqueryService).to receive(:new).and_return(events_client)
     allow(DfE::Analytics::AzureFederatedAuth).to receive(:gcp_client_credentials).and_return(authorization)
     allow(events_client).to receive(:authorization=).and_return(authorization)
+    allow(events_client).to receive(:authorization).and_return(authorization)
 
     DfE::Analytics::Testing.webmock!
   end