Merge branch '8765-geo-gitlab-geo-should-not-serialize-activerecord-objects' into 'master'

Add Rails.version to the Geo cache keys

See merge request gitlab-org/gitlab-ee!8775

ee/changelogs/unreleased/8765-geo-gitlab-geo-should-not-serialize-activerecord-objects.yml

+---
+title: Add Rails.version to the Geo cache keys
+merge_request: 8775
+author:
+type: fixed

ee/lib/gitlab/geo.rb

+# frozen_string_literal: true
+
 module Gitlab
   module Geo
     OauthApplicationUndefinedError = Class.new(StandardError)
@@ -6,24 +8,22 @@ module Gitlab
     InvalidSignatureTimeError = Class.new(StandardError)
 
     CACHE_KEYS = %i(
-      geo_primary_node
-      geo_secondary_nodes
-      geo_node_enabled
-      geo_node_primary
-      geo_node_secondary
-      geo_oauth_application
+      primary_node
+      secondary_nodes
+      node_enabled
+      oauth_application
     ).freeze
 
     def self.current_node
-      self.cache_value(:geo_node_current) { GeoNode.current_node }
+      self.cache_value(:current_node) { GeoNode.current_node }
     end
 
     def self.primary_node
-      self.cache_value(:geo_primary_node) { GeoNode.primary_node }
+      self.cache_value(:primary_node) { GeoNode.primary_node }
     end
 
     def self.secondary_nodes
-      self.cache_value(:geo_secondary_nodes) { GeoNode.secondary_nodes }
+      self.cache_value(:secondary_nodes) { GeoNode.secondary_nodes }
     end
 
     def self.connected?
@@ -31,7 +31,7 @@ module Gitlab
     end
 
     def self.enabled?
-      cache_value(:geo_node_enabled) { GeoNode.exists? }
+      cache_value(:node_enabled) { GeoNode.exists? }
     end
 
     def self.primary?
@@ -74,24 +74,34 @@ module Gitlab
     def self.oauth_authentication
       return false unless Gitlab::Geo.secondary?
 
-      self.cache_value(:geo_oauth_application) do
+      self.cache_value(:oauth_application) do
         Gitlab::Geo.current_node.oauth_application || raise(OauthApplicationUndefinedError)
       end
     end
 
-    def self.cache_value(key, &block)
-      return yield unless RequestStore.active?
+    def self.cache_key_for(key)
+      "geo:#{key}:#{Rails.version}"
+    end
+
+    def self.cache_value(raw_key, &block)
+      return yield unless Gitlab::SafeRequestStore.active?
 
-      # We need a short expire time as we can't manually expire on a secondary node
-      RequestStore.fetch(key) { Rails.cache.fetch(key, expires_in: 15.seconds) { yield } }
+      key = cache_key_for(raw_key)
+
+      Gitlab::SafeRequestStore.fetch(key) do
+        # We need a short expire time as we can't manually expire on a secondary node
+        Rails.cache.fetch(key, expires_in: 15.seconds) { yield }
+      end
     end
 
     def self.expire_cache!
-      return true unless RequestStore.active?
+      return true unless Gitlab::SafeRequestStore.active?
+
+      CACHE_KEYS.each do |raw_key|
+        key = cache_key_for(raw_key)
 
-      CACHE_KEYS.each do |key|
         Rails.cache.delete(key)
-        RequestStore.delete(key)
+        Gitlab::SafeRequestStore.delete(key)
       end
 
       true

ee/spec/lib/gitlab/geo_spec.rb

@@ -6,19 +6,36 @@ describe Gitlab::Geo, :geo do
   set(:primary_node)   { create(:geo_node, :primary) }
   set(:secondary_node) { create(:geo_node) }
 
-  describe 'current_node' do
+  shared_examples 'a Geo cached value' do |method, key|
+    it 'includes Rails.version in the cache key', :request_store do
+      expect(Rails.cache).to receive(:fetch)
+        .with("geo:#{key}:#{Rails.version}", expires_in: 15.seconds)
+
+      described_class.public_send(method)
+    end
+  end
+
+  describe '.current_node' do
     it 'returns a GeoNode instance' do
       expect(described_class.current_node).to eq(primary_node)
     end
+
+    it_behaves_like 'a Geo cached value', :current_node, :current_node
   end
 
-  describe 'primary_node' do
+  describe '.primary_node' do
     it 'returns a GeoNode primary instance' do
       expect(described_class.primary_node).to eq(primary_node)
     end
+
+    it_behaves_like 'a Geo cached value', :primary_node, :primary_node
+  end
+
+  describe '.secondary_nodes' do
+    it_behaves_like 'a Geo cached value', :secondary_nodes, :secondary_nodes
   end
 
-  describe 'primary?' do
+  describe '.primary?' do
     context 'when current node is a primary node' do
       it 'returns true' do
         expect(described_class.primary?).to be_truthy
@@ -32,7 +49,7 @@ describe Gitlab::Geo, :geo do
     end
   end
 
-  describe 'primary_node_configured?' do
+  describe '.primary_node_configured?' do
     context 'when current node is a primary node' do
       it 'returns true' do
         expect(described_class.primary_node_configured?).to be_truthy
@@ -46,7 +63,7 @@ describe Gitlab::Geo, :geo do
     end
   end
 
-  describe 'secondary?' do
+  describe '.secondary?' do
     context 'when current node is a secondary node' do
       before do
         stub_current_geo_node(secondary_node)
@@ -64,7 +81,9 @@ describe Gitlab::Geo, :geo do
     end
   end
 
-  describe 'enabled?' do
+  describe '.enabled?' do
+    it_behaves_like 'a Geo cached value', :enabled?, :node_enabled
+
     context 'when any GeoNode exists' do
       it 'returns true' do
         expect(described_class.enabled?).to be_truthy
@@ -92,7 +111,15 @@ describe Gitlab::Geo, :geo do
     end
   end
 
-  describe 'connected?' do
+  describe '.oauth_authentication' do
+    before do
+      stub_secondary_node
+    end
+
+    it_behaves_like 'a Geo cached value', :oauth_authentication, :oauth_application
+  end
+
+  describe '.connected?' do
     context 'when there is a database issue' do
       it 'returns false when database connection is down' do
         allow(GeoNode).to receive(:connected?) { false }
@@ -114,7 +141,7 @@ describe Gitlab::Geo, :geo do
     end
   end
 
-  describe 'secondary?' do
+  describe '.secondary?' do
     context 'when current node is secondary' do
       it 'returns true' do
         stub_current_geo_node(secondary_node)
@@ -129,7 +156,17 @@ describe Gitlab::Geo, :geo do
     end
   end
 
-  describe 'license_allows?' do
+  describe '.expire_cache!' do
+    it 'clears the Geo cache keys', :request_store do
+      described_class::CACHE_KEYS.each do |raw_key|
+        expect(Rails.cache).to receive(:delete).with("geo:#{raw_key}:#{Rails.version}")
+      end
+
+      described_class.expire_cache!
+    end
+  end
+
+  describe '.license_allows?' do
     it 'returns true if license has Geo addon' do
       stub_licensed_features(geo: true)
       expect(described_class.license_allows?).to be_truthy