Commit 1eb7487b authored by Robert Speicher's avatar Robert Speicher

Merge branch 'cherry-pick-1eeaf62b' into 'master'

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

Closes #3506

See merge request gitlab-org/gitlab-ee!3040
parents 20e2d7eb 50f00309
......@@ -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
......
---
title: Fix a Geo node validation, preventing admins from locking themselves out
merge_request: 3040
author:
type: fixed
......@@ -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]
......
......@@ -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
......
......@@ -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
......
......@@ -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 }
......
......@@ -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)
......
......@@ -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
......
......@@ -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)
......
module EE
module GeoHelpers
def stub_current_geo_node(node)
allow(::Gitlab::Geo).to receive(:current_node).and_return(node)
end
end
end
......@@ -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
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 }
......
require 'spec_helper'
describe Gitlab::Geo::LogCursor::Daemon, :postgresql do
include ::EE::GeoHelpers
describe '#run!' do
let!(:geo_node) { create(:geo_node, :current) }
set(:geo_node) { create(:geo_node) }
before do
stub_current_geo_node(geo_node)
end
it 'traps signals' do
allow(subject).to receive(:exit?) { true }
......@@ -125,15 +131,13 @@ 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!
end
it 'does not schedule a GeoRepositoryDestroyWorker when event node is not the current node' do
allow(Gitlab::Geo).to receive(:current_node).and_return(build(:geo_node))
stub_current_geo_node(build(:geo_node))
expect(Geo::RepositoriesCleanUpWorker).not_to receive(:perform_in)
......@@ -142,7 +146,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) }
......
require 'spec_helper'
describe Gitlab::Geo::Transfer do
let!(:primary_node) { FactoryGirl.create(:geo_node, :primary) }
let!(:secondary_node) { FactoryGirl.create(:geo_node) }
let(:lfs_object) { create(:lfs_object, :with_file) }
include ::EE::GeoHelpers
set(:primary_node) { create(:geo_node, :primary) }
set(:secondary_node) { create(:geo_node) }
set(:lfs_object) { create(:lfs_object, :with_file) }
let(:url) { primary_node.geo_transfers_url(:lfs, lfs_object.id.to_s) }
let(:content) { StringIO.new("1\n2\n3") }
let(:size) { File.stat(lfs_object.file.path).size }
......@@ -20,7 +22,8 @@ describe Gitlab::Geo::Transfer do
end
it '#download_from_primary' do
allow(Gitlab::Geo).to receive(:current_node) { secondary_node }
stub_current_geo_node(secondary_node)
response = double(success?: true)
expect(HTTParty).to receive(:get).and_return(response)
......
require 'spec_helper'
describe Gitlab::Geo do
let(:primary_node) { FactoryGirl.create(:geo_node, :primary) }
let(:secondary_node) { FactoryGirl.create(:geo_node) }
include ::EE::GeoHelpers
describe 'current_node' do
before do
primary_node
end
set(:primary_node) { create(:geo_node, :primary) }
set(:secondary_node) { create(:geo_node) }
describe 'current_node' do
it 'returns a GeoNode instance' do
expect(described_class.current_node).to eq(primary_node)
end
end
describe 'primary_node' do
before do
primary_node
secondary_node
end
it 'returns a GeoNode primary instance' do
expect(described_class.current_node).to eq(primary_node)
expect(described_class.primary_node).to eq(primary_node)
end
end
describe 'primary?' do
context 'when current node is a primary node' do
before do
primary_node
end
it 'returns true' do
expect(described_class.primary?).to be_truthy
end
......@@ -46,12 +35,12 @@ describe Gitlab::Geo do
describe 'primary_node_configured?' do
context 'when current node is a primary node' do
it 'returns true' do
primary_node
expect(described_class.primary_node_configured?).to be_truthy
end
it 'returns false when primary does not exist' do
primary_node.destroy
expect(described_class.primary_node_configured?).to be_falsey
end
end
......@@ -60,8 +49,7 @@ describe Gitlab::Geo do
describe 'secondary?' do
context 'when current node is a secondary node' do
before do
secondary_node
allow(described_class).to receive(:current_node) { secondary_node }
stub_current_geo_node(secondary_node)
end
it 'returns true' do
......@@ -78,16 +66,16 @@ describe Gitlab::Geo do
describe 'enabled?' do
context 'when any GeoNode exists' do
before do
secondary_node
end
it 'returns true' do
expect(described_class.enabled?).to be_truthy
end
end
context 'when no GeoNode exists' do
before do
GeoNode.delete_all
end
it 'returns false' do
expect(described_class.enabled?).to be_falsey
end
......@@ -111,17 +99,10 @@ describe Gitlab::Geo do
end
end
context 'with RequestStore enabled' do
before do
RequestStore.begin!
end
after do
RequestStore.end!
RequestStore.clear!
end
context 'with RequestStore enabled', :request_store do
it 'return false when no GeoNode exists' do
GeoNode.delete_all
expect(GeoNode).to receive(:exists?).once.and_call_original
2.times { expect(described_class.enabled?).to be_falsey }
......@@ -131,23 +112,14 @@ describe Gitlab::Geo do
describe 'readonly?' do
context 'when current node is secondary' do
before do
secondary_node
end
it 'returns true' do
allow(described_class).to receive(:current_node) { secondary_node }
stub_current_geo_node(secondary_node)
expect(described_class.secondary?).to be_truthy
end
end
context 'current node is primary' do
before do
primary_node
end
it 'returns false when ' do
allow(described_class).to receive(:current_node) { primary_node }
it 'returns false ' do
expect(described_class.secondary?).to be_falsey
end
end
......@@ -211,9 +183,6 @@ describe Gitlab::Geo do
end
it 'activates cron jobs for primary' do
allow(described_class).to receive(:primary?).and_return(true)
allow(described_class).to receive(:secondary?).and_return(false)
described_class.configure_cron_jobs!
expect(described_class.repository_sync_job).not_to be_enabled
......@@ -221,9 +190,8 @@ describe Gitlab::Geo do
expect(Sidekiq::Cron::Job.find('ldap_test')).to be_enabled
end
it 'activates cron jobs for secondary' do
allow(described_class).to receive(:primary?).and_return(false)
allow(described_class).to receive(:secondary?).and_return(true)
it 'does not activate cron jobs for secondary' do
stub_current_geo_node(secondary_node)
described_class.configure_cron_jobs!
......@@ -233,8 +201,7 @@ describe Gitlab::Geo do
end
it 'deactivates all jobs when Geo is not active' do
allow(described_class).to receive(:primary?).and_return(false)
allow(described_class).to receive(:secondary?).and_return(false)
GeoNode.update_all(enabled: false)
described_class.configure_cron_jobs!
......@@ -244,13 +211,11 @@ describe Gitlab::Geo do
end
it 'reactivates cron jobs when node turns off Geo' do
allow(described_class).to receive(:primary?).and_return(false)
allow(described_class).to receive(:secondary?).and_return(true)
stub_current_geo_node(secondary_node)
described_class.configure_cron_jobs!
expect(Sidekiq::Cron::Job.find('ldap_test')).not_to be_enabled
allow(described_class).to receive(:primary?).and_return(false)
allow(described_class).to receive(:secondary?).and_return(false)
described_class.configure_cron_jobs!
......
require 'spec_helper'
describe GeoNode, type: :model do
include ::EE::GeoHelpers
let(:new_node) { create(:geo_node, schema: 'https', host: 'localhost', port: 3000, relative_url_root: 'gitlab') }
let(:new_primary_node) { create(:geo_node, :primary, schema: 'https', host: 'localhost', port: 3000, relative_url_root: 'gitlab') }
let(:empty_node) { described_class.new }
......@@ -53,14 +55,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
......@@ -119,13 +124,13 @@ describe GeoNode, type: :model do
subject { described_class.new }
it 'returns true when node is the current node' do
allow(Gitlab::Geo).to receive(:current_node) { subject }
stub_current_geo_node(subject)
expect(subject.current?).to eq true
end
it 'returns false when node is not the current node' do
allow(Gitlab::Geo).to receive(:current_node) { double }
stub_current_geo_node(double)
expect(subject.current?).to eq false
end
......
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)
......
......@@ -2,21 +2,21 @@ require 'spec_helper'
describe API::Geo do
include ApiHelpers
include ::EE::GeoHelpers
let(:admin) { create(:admin) }
let(:user) { create(:user) }
let!(:primary_node) { create(:geo_node, :primary) }
let!(:secondary_node) { create(:geo_node) }
set(:admin) { create(:admin) }
set(:user) { create(:user) }
set(:primary_node) { create(:geo_node, :primary) }
set(:secondary_node) { create(:geo_node) }
let(:geo_token_header) do
{ 'X-Gitlab-Token' => secondary_node.system_hook.token }
end
before do
allow(Gitlab::Geo).to receive(:current_node) { secondary_node }
stub_current_geo_node(secondary_node)
end
describe 'GET /geo/transfers/attachment/1' do
let!(:secondary_node) { create(:geo_node) }
let(:note) { create(:note, :with_attachment) }
let(:upload) { Upload.find_by(model: note, uploader: 'AttachmentUploader') }
let(:transfer) { Gitlab::Geo::FileTransfer.new(:attachment, upload) }
......@@ -52,7 +52,6 @@ describe API::Geo do
end
describe 'GET /geo/transfers/avatar/1' do
let!(:secondary_node) { create(:geo_node) }
let(:user) { create(:user, avatar: fixture_file_upload(Rails.root + 'spec/fixtures/dk.png', 'image/png')) }
let(:upload) { Upload.find_by(model: user, uploader: 'AvatarUploader') }
let(:transfer) { Gitlab::Geo::FileTransfer.new(:avatar, upload) }
......@@ -88,7 +87,6 @@ describe API::Geo do
end
describe 'GET /geo/transfers/file/1' do
let!(:secondary_node) { create(:geo_node) }
let(:project) { create(:project) }
let(:upload) { Upload.find_by(model: project, uploader: 'FileUploader') }
let(:transfer) { Gitlab::Geo::FileTransfer.new(:file, upload) }
......@@ -125,7 +123,6 @@ describe API::Geo do
end
describe 'GET /geo/transfers/lfs/1' do
let!(:secondary_node) { create(:geo_node) }
let(:lfs_object) { create(:lfs_object, :with_file) }
let(:req_header) do
transfer = Gitlab::Geo::LfsTransfer.new(lfs_object)
......@@ -161,14 +158,9 @@ describe API::Geo do
end
end
describe 'GET /geo/status' do
let!(:secondary_node) { create(:geo_node) }
describe 'GET /geo/status', :postgresql do
let(:request) { Gitlab::Geo::BaseRequest.new }
before do
skip("Not using PostgreSQL") unless Gitlab::Database.postgresql?
end
it 'responds with 401 with invalid auth header' do
get api('/geo/status'), nil, Authorization: 'Test'
......@@ -185,7 +177,7 @@ describe API::Geo do
context 'when requesting secondary node with valid auth header' do
before do
allow(Gitlab::Geo).to receive(:current_node) { secondary_node }
stub_current_geo_node(secondary_node)
allow(request).to receive(:requesting_node) { primary_node }
end
......@@ -199,7 +191,7 @@ describe API::Geo do
context 'when requesting primary node with valid auth header' do
before do
allow(Gitlab::Geo).to receive(:current_node) { primary_node }
stub_current_geo_node(primary_node)
allow(request).to receive(:requesting_node) { secondary_node }
end
......
require 'spec_helper'
describe Geo::FileDownloadService do
let!(:primary) { create(:geo_node, :primary) }
let(:secondary) { create(:geo_node) }
include ::EE::GeoHelpers
set(:primary) { create(:geo_node, :primary) }
set(:secondary) { create(:geo_node) }
before do
allow(described_class).to receive(:current_node) { secondary }
stub_current_geo_node(secondary)
end
describe '#execute' do
......
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
......
require 'spec_helper'
describe Geo::NodeStatusService do
let!(:primary) { create(:geo_node, :primary) }
let(:secondary) { create(:geo_node) }
set(:primary) { create(:geo_node, :primary) }
set(:secondary) { create(:geo_node) }
subject { described_class.new }
before do
allow(described_class).to receive(:current_node) { primary }
end
describe '#call' do
it 'parses a 401 response' do
request = double(success?: false,
......@@ -75,8 +71,6 @@ describe Geo::NodeStatusService do
end
it 'returns meaningful error message when primary uses incorrect db key' do
secondary # create it before mocking GeoNode#secret_access_key
allow_any_instance_of(GeoNode).to receive(:secret_access_key).and_raise(OpenSSL::Cipher::CipherError)
status = subject.call(secondary)
......@@ -85,7 +79,7 @@ describe Geo::NodeStatusService do
end
it 'gracefully handles case when primary is deleted' do
primary.destroy
primary.destroy!
status = subject.call(secondary)
......
......@@ -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]) }
......
require 'spec_helper'
describe Geo::FileDownloadDispatchWorker do
let!(:primary) { create(:geo_node, :primary, host: 'primary-geo-node') }
let!(:secondary) { create(:geo_node, :current) }
include ::EE::GeoHelpers
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)
stub_current_geo_node(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)
......
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) }
include ::EE::GeoHelpers
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
stub_current_geo_node(secondary)
end
describe '#perform' do
before do
allow_any_instance_of(Gitlab::ExclusiveLease).to receive(:try_obtain) { true }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment