Fix the lock-yourself-out prevention code and specs for Geo

(cherry picked from commit 1eeaf62b12ba76818e7e38932e5d612f69e59ad0)

app/models/geo_node.rb

@@ -23,6 +23,7 @@ class GeoNode < ActiveRecord::Base
   validates :encrypted_secret_access_key, presence: true
 
   validates :geo_node_key, presence: true, if: :secondary?
+  validate :check_not_adding_primary_as_secondary, if: :secondary?
 
   after_initialize :build_dependents
   after_save :expire_cache!
@@ -216,12 +217,12 @@ class GeoNode < ActiveRecord::Base
     end
   end
 
-  def validate(record)
-    # Prevent locking yourself out
-    if record.host == Gitlab.config.gitlab.host &&
-        record.port == Gitlab.config.gitlab.port &&
-        record.relative_url_root == Gitlab.config.gitlab.relative_url_root && !record.primary
-      record.errors[:base] << 'Current node must be the primary node or you will be locking yourself out'
+  # Prevent locking yourself out
+  def check_not_adding_primary_as_secondary
+    if host == Gitlab.config.gitlab.host &&
+        port == Gitlab.config.gitlab.port &&
+        relative_url_root == Gitlab.config.gitlab.relative_url_root
+      errors.add(:base, 'Current node must be the primary node or you will be locking yourself out')
     end
   end
 

changelogs/unreleased-ee/cherry-pick-1eeaf62b.yml

+---
+title: Fix a Geo node validation, preventing admins from locking themselves out
+merge_request: 3040
+author:
+type: fixed

spec/ee/spec/features/projects/geo_clone_instructions_spec.rb

@@ -9,7 +9,6 @@ feature 'Geo clone instructions', :js do
   background do
     primary = create(:geo_node, :primary, schema: 'https', host: 'primary.domain.com', port: 443)
     primary.update_attribute(:clone_url_prefix, 'git@primary.domain.com:')
-    create(:geo_node, :current)
     allow(Gitlab::Geo).to receive(:secondary?).and_return(true)
 
     project.team << [developer, :developer]

spec/ee/spec/migrations/remove_system_hook_from_geo_nodes_spec.rb

@@ -8,7 +8,7 @@ describe RemoveSystemHookFromGeoNodes, :migration do
     allow_any_instance_of(WebHookService).to receive(:execute)
 
     create(:system_hook)
-    geo_nodes.create! attributes_for(:geo_node, :primary, :current)
+    geo_nodes.create! attributes_for(:geo_node, :primary)
     geo_nodes.create! attributes_for(:geo_node, system_hook_id: create(:system_hook).id)
   end
 

spec/ee/spec/models/ee/namespace_spec.rb

@@ -67,7 +67,7 @@ describe Namespace do
 
   describe '#move_dir' do
     context 'when running on a primary node' do
-      let!(:geo_node) { create(:geo_node, :primary, :current) }
+      let!(:geo_node) { create(:geo_node, :primary) }
       let(:gitlab_shell) { Gitlab::Shell.new }
 
       it 'logs the Geo::RepositoryRenamedEvent for each project inside namespace' do

spec/ee/spec/models/ee/project_spec.rb

@@ -749,7 +749,7 @@ describe Project do
 
   describe '#rename_repo' do
     context 'when running on a primary node' do
-      let!(:geo_node) { create(:geo_node, :primary, :current) }
+      let!(:geo_node) { create(:geo_node, :primary) }
       let(:project) { create(:project, :repository) }
       let(:gitlab_shell) { Gitlab::Shell.new }
 

spec/ee/spec/services/ee/projects/create_service_spec.rb

@@ -161,7 +161,7 @@ describe Projects::CreateService, '#execute' do
   end
 
   context 'when running on a primary node' do
-    let!(:geo_node) { create(:geo_node, :primary, :current) }
+    let!(:geo_node) { create(:geo_node, :primary) }
 
     it 'logs an event to the Geo event log' do
       expect { create_project(user, opts) }.to change(Geo::RepositoryCreatedEvent, :count).by(1)

spec/ee/spec/services/ee/projects/destroy_service_spec.rb

@@ -31,7 +31,7 @@ describe Projects::DestroyService do
   end
 
   context 'when running on a primary node' do
-    let!(:geo_node) { create(:geo_node, :primary, :current) }
+    let!(:geo_node) { create(:geo_node, :primary) }
 
     it 'logs an event to the Geo event log' do
       # Run Sidekiq immediately to check that renamed repository will be removed

spec/ee/spec/services/ee/projects/transfer_service_spec.rb

@@ -12,7 +12,7 @@ describe Projects::TransferService do
   end
 
   context 'when running on a primary node' do
-    let!(:geo_node) { create(:geo_node, :primary, :current) }
+    let!(:geo_node) { create(:geo_node, :primary) }
 
     it 'logs an event to the Geo event log' do
       expect { subject.execute(group) }.to change(Geo::RepositoryRenamedEvent, :count).by(1)

spec/factories/geo_nodes.rb

@@ -9,9 +9,5 @@ FactoryGirl.define do
       port { Gitlab.config.gitlab.port }
       geo_node_key nil
     end
-
-    trait :current do
-      port { Gitlab.config.gitlab.port }
-    end
   end
 end

spec/lib/gitlab/geo/health_check_spec.rb

 require 'spec_helper'
 
 describe Gitlab::Geo::HealthCheck, :postgresql do
-  let!(:secondary) { create(:geo_node, :current) }
+  set(:secondary) { create(:geo_node) }
 
   subject { described_class }
 
+  before do
+    allow(Gitlab::Geo).to receive(:current_node).and_return(secondary)
+  end
+
   describe '.perform_checks' do
     it 'returns a string if database is not fully migrated' do
-      allow(Gitlab::Geo).to receive(:secondary?) { true }
       allow(described_class).to receive(:geo_database_configured?).and_return(true)
       allow(described_class).to receive(:database_secondary?).and_return(true)
       allow(described_class).to receive(:get_database_version).and_return('20170101')
@@ -27,7 +30,6 @@ describe Gitlab::Geo::HealthCheck, :postgresql do
     end
 
     it 'returns an error when database is not configured for streaming replication' do
-      allow(Gitlab::Geo).to receive(:secondary?) { true }
       allow(Gitlab::Geo).to receive(:configured?) { true }
       allow(Gitlab::Database).to receive(:postgresql?) { true }
       allow(described_class).to receive(:database_secondary?) { false }
@@ -36,7 +38,6 @@ describe Gitlab::Geo::HealthCheck, :postgresql do
     end
 
     it 'returns an error when streaming replication is not working' do
-      allow(Gitlab::Geo).to receive(:secondary?) { true }
       allow(Gitlab::Geo).to receive(:configured?) { true }
       allow(Gitlab::Database).to receive(:postgresql?) { true }
       allow(described_class).to receive(:database_secondary?) { false }

spec/lib/gitlab/geo/log_cursor/daemon_spec.rb

@@ -2,7 +2,11 @@ require 'spec_helper'
 
 describe Gitlab::Geo::LogCursor::Daemon, :postgresql do
   describe '#run!' do
-    let!(:geo_node) { create(:geo_node, :current) }
+    set(:geo_node) { create(:geo_node) }
+
+    before do
+      allow(Gitlab::Geo).to receive(:current_node).and_return(geo_node)
+    end
 
     it 'traps signals' do
       allow(subject).to receive(:exit?) { true }
@@ -125,8 +129,6 @@ describe Gitlab::Geo::LogCursor::Daemon, :postgresql do
       end
 
       it 'schedules a GeoRepositoryDestroyWorker when event node is the current node' do
-        allow(Gitlab::Geo).to receive(:current_node).and_return(geo_node)
-
         expect(Geo::RepositoriesCleanUpWorker).to receive(:perform_in).with(within(5.minutes).of(1.hour), geo_node.id)
 
         subject.run!
@@ -142,7 +144,6 @@ describe Gitlab::Geo::LogCursor::Daemon, :postgresql do
     end
 
     context 'when node has namespace restrictions' do
-      let(:geo_node) { create(:geo_node, :current) }
       let(:group_1) { create(:group) }
       let(:group_2) { create(:group) }
       let(:project) { create(:project, group: group_1) }

spec/models/geo_node_spec.rb

@@ -53,14 +53,17 @@ describe GeoNode, type: :model do
   end
 
   context 'prevent locking yourself out' do
-    subject do
-      GeoNode.new(host: Gitlab.config.gitlab.host,
-                  port: Gitlab.config.gitlab.port,
-                  relative_url_root: Gitlab.config.gitlab.relative_url_root)
-    end
-
     it 'does not accept adding a non primary node with same details as current_node' do
-      expect(subject).not_to be_valid
+      node = GeoNode.new(
+        host: Gitlab.config.gitlab.host,
+        port: Gitlab.config.gitlab.port,
+        relative_url_root: Gitlab.config.gitlab.relative_url_root,
+        geo_node_key: build(:geo_node_key)
+      )
+
+      expect(node).not_to be_valid
+      expect(node.errors.full_messages.count).to eq(1)
+      expect(node.errors[:base].first).to match('locking yourself out')
     end
   end
 

spec/models/geo_node_status_spec.rb

 require 'spec_helper'
 
 describe GeoNodeStatus do
-  let!(:geo_node) { create(:geo_node, :current) }
-  let(:group)      { create(:group) }
-  let!(:project_1) { create(:project, group: group) }
-  let!(:project_2) { create(:project, group: group) }
-  let!(:project_3) { create(:project) }
-  let!(:project_4) { create(:project) }
+  set(:geo_node)  { create(:geo_node, :primary) }
+  set(:group)     { create(:group) }
+  set(:project_1) { create(:project, group: group) }
+  set(:project_2) { create(:project, group: group) }
+  set(:project_3) { create(:project) }
+  set(:project_4) { create(:project) }
 
   subject { described_class.new }
 
@@ -99,7 +99,8 @@ describe GeoNodeStatus do
   end
 
   describe '#db_replication_lag' do
-    it 'returns the set replication lag' do
+    it 'returns the set replication lag if secondary' do
+      allow(Gitlab::Geo).to receive(:secondary?).and_return(true)
       allow(Gitlab::Geo::HealthCheck).to receive(:db_replication_lag).and_return(1000)
 
       expect(subject.db_replication_lag).to eq(1000)
@@ -107,7 +108,6 @@ describe GeoNodeStatus do
 
     it "doesn't attempt to set replication lag if primary" do
       expect(Gitlab::Geo::HealthCheck).not_to receive(:db_replication_lag)
-      expect(Gitlab::Geo).to receive(:secondary?).and_return(false)
 
       expect(subject.db_replication_lag).to eq(nil)
     end
@@ -198,7 +198,8 @@ describe GeoNodeStatus do
       expect(subject.cursor_last_event_date).to be_nil
     end
 
-    it 'returns the latest event ID' do
+    it 'returns the latest event ID if secondary' do
+      allow(Gitlab::Geo).to receive(:secondary?).and_return(true)
       event = create(:geo_event_log_state)
 
       expect(subject.cursor_last_event_id).to eq(event.event_id)
@@ -206,7 +207,6 @@ describe GeoNodeStatus do
 
     it "doesn't attempt to retrieve cursor if primary" do
       create(:geo_event_log_state)
-      expect(Gitlab::Geo).to receive(:secondary?).exactly(2).times.and_return(false)
 
       expect(subject.cursor_last_event_date).to eq(nil)
       expect(subject.cursor_last_event_id).to eq(nil)

spec/services/geo/file_upload_service_spec.rb

 require 'spec_helper'
 
 describe Geo::FileUploadService do
-  let!(:node) { create(:geo_node, :current) }
+  set(:node) { create(:geo_node, :primary) }
 
   describe '#execute' do
     context 'user avatar' do

spec/services/geo/node_update_service_spec.rb

@@ -2,7 +2,7 @@ require 'spec_helper'
 
 describe Geo::NodeUpdateService do
   let(:group) { create(:group) }
-  let!(:primary) { create(:geo_node, :primary, :current) }
+  let!(:primary) { create(:geo_node, :primary) }
   let(:geo_node) { create(:geo_node) }
   let(:geo_node_with_restrictions) { create(:geo_node, namespace_ids: [group.id]) }
 

spec/workers/geo/file_download_dispatch_worker_spec.rb

 require 'spec_helper'
 
 describe Geo::FileDownloadDispatchWorker do
-  let!(:primary)   { create(:geo_node, :primary, host: 'primary-geo-node') }
-  let!(:secondary) { create(:geo_node, :current) }
+  set(:primary)   { create(:geo_node, :primary, host: 'primary-geo-node') }
+  set(:secondary) { create(:geo_node) }
 
   before do
-    allow(Gitlab::Geo).to receive(:secondary?).and_return(true)
+    allow(Gitlab::Geo).to receive(:current_node).and_return(secondary)
     allow_any_instance_of(Gitlab::ExclusiveLease).to receive(:try_obtain).and_return(true)
     allow_any_instance_of(Gitlab::ExclusiveLease).to receive(:renew).and_return(true)
     allow_any_instance_of(described_class).to receive(:over_time?).and_return(false)

spec/workers/geo/repository_sync_worker_spec.rb

 require 'spec_helper'
 
 describe Geo::RepositorySyncWorker, :postgresql do
-  let!(:primary) { create(:geo_node, :primary, host: 'primary-geo-node') }
-  let!(:secondary) { create(:geo_node, :current) }
-  let(:synced_group) { create(:group) }
-  let!(:project_in_synced_group) { create(:project, group: synced_group) }
-  let!(:unsynced_project) { create(:project) }
+  set(:primary) { create(:geo_node, :primary, host: 'primary-geo-node') }
+  set(:secondary) { create(:geo_node) }
+  set(:synced_group) { create(:group) }
+  set(:project_in_synced_group) { create(:project, group: synced_group) }
+  set(:unsynced_project) { create(:project) }
 
   subject { described_class.new }
 
+  before do
+    allow(Gitlab::Geo).to receive(:current_node).and_return(secondary)
+  end
+
   describe '#perform' do
     before do
       allow_any_instance_of(Gitlab::ExclusiveLease).to receive(:try_obtain) { true }