Merge branch 'sh-disable-geo-primary-system-hook' into 'master'

Delete primary Geo system hook

Closes #1752

See merge request !1679

app/models/geo_node.rb

@@ -130,8 +130,7 @@ class GeoNode < ActiveRecord::Base
 
   def build_dependents
     unless persisted?
-      self.build_geo_node_key if geo_node_key.nil?
-      update_system_hook!
+      self.build_geo_node_key unless geo_node_key.present?
     end
   end
 
@@ -167,6 +166,8 @@ class GeoNode < ActiveRecord::Base
   end
 
   def update_system_hook!
+    return if self.primary?
+
     self.build_system_hook if system_hook.nil?
     self.system_hook.token = SecureRandom.hex(20) unless self.system_hook.token.present?
     self.system_hook.url = geo_events_url if uri.present?

db/migrate/20170620063751_remove_geo_primary_system_hook.rb

+class RemoveGeoPrimarySystemHook < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  # Older versions of GitLab added a system hook for the primary Geo node.
+  # This would cause all events to be fired for the primary as well.
+  def up
+    return unless geo_enabled?
+
+    execute <<-SQL
+      DELETE FROM web_hooks WHERE
+      type = 'SystemHook' AND
+      id IN (
+      SELECT system_hook_id FROM geo_nodes WHERE
+        "primary" = #{true_value}
+      );
+    SQL
+  end
+
+  def geo_enabled?
+    select_all("SELECT 1 FROM geo_nodes").present?
+  end
+end

spec/models/geo_node_spec.rb

 require 'spec_helper'
 
 describe GeoNode, type: :model do
-  subject(:new_node) { described_class.new(schema: 'https', host: 'localhost', port: 3000, relative_url_root: 'gitlab') }
-  subject(:new_primary_node) { described_class.new(schema: 'https', host: 'localhost', port: 3000, relative_url_root: 'gitlab', primary: true) }
+  subject(:new_node) { create(:geo_node, schema: 'https', host: 'localhost', port: 3000, relative_url_root: 'gitlab', primary: false) }
+  subject(:new_primary_node) { create(:geo_node, schema: 'https', host: 'localhost', port: 3000, relative_url_root: 'gitlab', primary: true) }
   subject(:empty_node) { described_class.new }
-  subject(:primary_node) { FactoryGirl.create(:geo_node, :primary) }
-  subject(:node) { FactoryGirl.create(:geo_node) }
+  subject(:primary_node) { create(:geo_node, :primary) }
+  subject(:node) { create(:geo_node) }
 
   let(:dummy_url) { 'https://localhost:3000/gitlab' }
   let(:url_helpers) { Gitlab::Application.routes.url_helpers }
@@ -46,6 +46,19 @@ describe GeoNode, type: :model do
     end
   end
 
+  context 'system hooks' do
+    it 'primary does not create a system hook' do
+      expect(primary_node.system_hook).to be_nil
+    end
+
+    it 'secondary creates a system hook with repository update events' do
+      hook = new_node.system_hook
+      expect(hook.push_events).to be_falsey
+      expect(hook.tag_push_events).to be_falsey
+      expect(hook.repository_update_events).to be_truthy
+    end
+  end
+
   context 'prevent locking yourself out' do
     subject do
       GeoNode.new(host: Gitlab.config.gitlab.host,
@@ -151,6 +164,10 @@ describe GeoNode, type: :model do
     end
 
     context 'when required fields are not filled' do
+      it 'returns an initialized Geo node key' do
+        expect(empty_node.geo_node_key).not_to be_nil
+      end
+
       it 'returns an URI object' do
         expect(empty_node.uri).to be_a URI
       end

spec/requests/api/geo_spec.rb

@@ -6,12 +6,13 @@ describe API::Geo, api: true do
   let(:admin) { create(:admin) }
   let(:user) { create(:user) }
   let!(:primary_node) { create(:geo_node, :primary) }
+  let!(:secondary_node) { create(:geo_node) }
   let(:geo_token_header) do
-    { 'X-Gitlab-Token' => primary_node.system_hook.token }
+    { 'X-Gitlab-Token' => secondary_node.system_hook.token }
   end
 
   before(:each) do
-    allow(Gitlab::Geo).to receive(:current_node) { primary_node }
+    allow(Gitlab::Geo).to receive(:current_node) { secondary_node }
   end
 
   describe 'POST /geo/receive_events authentication' do
@@ -30,7 +31,7 @@ describe API::Geo, api: true do
 
   describe 'POST /geo/refresh_wikis disabled node' do
     it 'responds with forbidden' do
-      primary_node.enabled = false
+      secondary_node.enabled = false
 
       post api('/geo/refresh_wikis', admin), nil
 
@@ -40,7 +41,7 @@ describe API::Geo, api: true do
 
   describe 'POST /geo/receive_events disabled node' do
     it 'responds with forbidden' do
-      primary_node.enabled = false
+      secondary_node.enabled = false
 
       post api('/geo/receive_events'), nil, geo_token_header