From 49bb3e79b416407ef56a54b6c687775cb840b7f7 Mon Sep 17 00:00:00 2001 From: Drew Blessing Date: Fri, 29 Jul 2016 16:24:23 -0500 Subject: [PATCH] Replace LDAP group sync exclusive lease with state machine --- CHANGELOG-EE | 1 + app/models/ee/group.rb | 54 ++++++++++++ app/models/group.rb | 3 + ...718210912_add_ldap_sync_state_to_groups.rb | 23 +++++ ...9_add_ldap_sync_state_indices_to_groups.rb | 17 ++++ ...last_ldap_sync_status_index_from_groups.rb | 15 ++++ ...emove_last_ldap_sync_status_from_groups.rb | 9 ++ db/schema.rb | 9 +- lib/ee/gitlab/ldap/sync/group.rb | 22 +++-- lib/ee/gitlab/ldap/sync/groups.rb | 2 +- spec/lib/ee/gitlab/ldap/sync/group_spec.rb | 39 +++++++- spec/models/ee/group_spec.rb | 88 +++++++++++++++++++ 12 files changed, 270 insertions(+), 12 deletions(-) create mode 100644 app/models/ee/group.rb create mode 100644 db/migrate/20160718210912_add_ldap_sync_state_to_groups.rb create mode 100644 db/migrate/20160718210939_add_ldap_sync_state_indices_to_groups.rb create mode 100644 db/migrate/20160718211006_remove_last_ldap_sync_status_index_from_groups.rb create mode 100644 db/migrate/20160718211059_remove_last_ldap_sync_status_from_groups.rb create mode 100644 spec/models/ee/group_spec.rb diff --git a/CHANGELOG-EE b/CHANGELOG-EE index 4e8f46ec9547ca..9d5c6710b6d55a 100644 --- a/CHANGELOG-EE +++ b/CHANGELOG-EE @@ -21,6 +21,7 @@ v 8.10.0 - Rename Git Hooks to Push Rules - Fix EE keys fingerprint add index migration if came from CE - Add todos for MR approvers !547 + - Replace LDAP group sync exclusive lease with state machine - Prevent the author of an MR from being on the approvers list - Isolate EE LDAP library code in EE module (Part 1) !511 - Make Elasticsearch indexer run as an async task diff --git a/app/models/ee/group.rb b/app/models/ee/group.rb new file mode 100644 index 00000000000000..1ff674e9221aba --- /dev/null +++ b/app/models/ee/group.rb @@ -0,0 +1,54 @@ +# Group EE mixin +# +# This module is intended to encapsulate EE-specific model logic +# and be included in the `Group` model +module EE + module Group + extend ActiveSupport::Concern + + included do + state_machine :ldap_sync_status, namespace: :ldap_sync, initial: :ready do + state :ready + state :started + state :failed + + event :start do + transition [:ready, :failed] => :started + end + + event :finish do + transition started: :ready + end + + event :fail do + transition started: :failed + end + + after_transition ready: :started do |group, _| + group.ldap_sync_last_sync_at = DateTime.now + group.save + end + + after_transition started: :ready do |group, _| + current_time = DateTime.now + group.ldap_sync_last_update_at = current_time + group.ldap_sync_last_successful_update_at = current_time + group.ldap_sync_error = nil + group.save + end + + after_transition started: :failed do |group, _| + group.ldap_sync_last_update_at = DateTime.now + group.save + end + end + end + + def mark_ldap_sync_as_failed(error_message) + return false unless ldap_sync_started? + + fail_ldap_sync + update_column(:ldap_sync_error, ::Gitlab::UrlSanitizer.sanitize(error_message)) + end + end +end diff --git a/app/models/group.rb b/app/models/group.rb index 7fc2d9f9982128..1e83a7fb791ea5 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -1,6 +1,9 @@ require 'carrierwave/orm/activerecord' +# Contains methods common to both GitLab CE and EE. +# All EE methods should be in `EE::Group` only. class Group < Namespace + include EE::Group include Gitlab::ConfigHelper include Gitlab::VisibilityLevel include AccessRequestable diff --git a/db/migrate/20160718210912_add_ldap_sync_state_to_groups.rb b/db/migrate/20160718210912_add_ldap_sync_state_to_groups.rb new file mode 100644 index 00000000000000..67df4d62bd2125 --- /dev/null +++ b/db/migrate/20160718210912_add_ldap_sync_state_to_groups.rb @@ -0,0 +1,23 @@ +# Migration type: online without errors (works on previous version and new one) +class AddLdapSyncStateToGroups < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + disable_ddl_transaction! + + DOWNTIME = false + + def up + add_column_with_default :namespaces, :ldap_sync_status, :string, default: 'ready' + add_column :namespaces, :ldap_sync_error, :string + add_column :namespaces, :ldap_sync_last_update_at, :datetime + add_column :namespaces, :ldap_sync_last_successful_update_at, :datetime + add_column :namespaces, :ldap_sync_last_sync_at, :datetime + end + + def down + remove_column :namespaces, :ldap_sync_status + remove_column :namespaces, :ldap_sync_error + remove_column :namespaces, :ldap_sync_last_update_at + remove_column :namespaces, :ldap_sync_last_successful_update_at + remove_column :namespaces, :ldap_sync_last_sync_at + end +end diff --git a/db/migrate/20160718210939_add_ldap_sync_state_indices_to_groups.rb b/db/migrate/20160718210939_add_ldap_sync_state_indices_to_groups.rb new file mode 100644 index 00000000000000..8bee8af7b7769d --- /dev/null +++ b/db/migrate/20160718210939_add_ldap_sync_state_indices_to_groups.rb @@ -0,0 +1,17 @@ +# Migration type: online without errors (works on previous version and new one) +class AddLdapSyncStateIndicesToGroups < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + disable_ddl_transaction! + + DOWNTIME = false + + def up + add_concurrent_index :namespaces, :ldap_sync_last_update_at + add_concurrent_index :namespaces, :ldap_sync_last_successful_update_at + end + + def down + remove_index :namespaces, column: :ldap_sync_last_update_at if index_exists?(:namespaces, :ldap_sync_last_update_at) + remove_index :namespaces, column: :ldap_sync_last_successful_update_at if index_exists?(:namespaces, :ldap_sync_last_successful_update_at) + end +end diff --git a/db/migrate/20160718211006_remove_last_ldap_sync_status_index_from_groups.rb b/db/migrate/20160718211006_remove_last_ldap_sync_status_index_from_groups.rb new file mode 100644 index 00000000000000..44c53d8c44fa15 --- /dev/null +++ b/db/migrate/20160718211006_remove_last_ldap_sync_status_index_from_groups.rb @@ -0,0 +1,15 @@ +# Migration type: online without errors (works on previous version and new one) +class RemoveLastLdapSyncStatusIndexFromGroups < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + disable_ddl_transaction! + + DOWNTIME = false + + def up + remove_index :namespaces, column: :last_ldap_sync_at if index_exists?(:namespaces, :last_ldap_sync_at) + end + + def down + add_concurrent_index :namespaces, :last_ldap_sync_at + end +end diff --git a/db/migrate/20160718211059_remove_last_ldap_sync_status_from_groups.rb b/db/migrate/20160718211059_remove_last_ldap_sync_status_from_groups.rb new file mode 100644 index 00000000000000..6a794e5b43c16a --- /dev/null +++ b/db/migrate/20160718211059_remove_last_ldap_sync_status_from_groups.rb @@ -0,0 +1,9 @@ +# Migration type: online without errors (works on previous version and new one) +class RemoveLastLdapSyncStatusFromGroups < ActiveRecord::Migration + + DOWNTIME = false + + def change + remove_column :namespaces, :last_ldap_sync_at, :datetime + end +end diff --git a/db/schema.rb b/db/schema.rb index 7fcf569d51771c..833c0151d9286e 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -756,12 +756,17 @@ t.boolean "membership_lock", default: false t.boolean "share_with_group_lock", default: false t.integer "visibility_level", default: 20, null: false - t.datetime "last_ldap_sync_at" t.boolean "request_access_enabled", default: true, null: false + t.string "ldap_sync_status", default: "ready", null: false + t.string "ldap_sync_error" + t.datetime "ldap_sync_last_update_at" + t.datetime "ldap_sync_last_successful_update_at" + t.datetime "ldap_sync_last_sync_at" end add_index "namespaces", ["created_at", "id"], name: "index_namespaces_on_created_at_and_id", using: :btree - add_index "namespaces", ["last_ldap_sync_at"], name: "index_namespaces_on_last_ldap_sync_at", using: :btree + add_index "namespaces", ["ldap_sync_last_successful_update_at"], name: "index_namespaces_on_ldap_sync_last_successful_update_at", using: :btree + add_index "namespaces", ["ldap_sync_last_update_at"], name: "index_namespaces_on_ldap_sync_last_update_at", using: :btree add_index "namespaces", ["name"], name: "index_namespaces_on_name", unique: true, using: :btree add_index "namespaces", ["name"], name: "index_namespaces_on_name_trigram", using: :gin, opclasses: {"name"=>"gin_trgm_ops"} add_index "namespaces", ["owner_id"], name: "index_namespaces_on_owner_id", using: :btree diff --git a/lib/ee/gitlab/ldap/sync/group.rb b/lib/ee/gitlab/ldap/sync/group.rb index 551ae3b9af0946..9e9217cc35a461 100644 --- a/lib/ee/gitlab/ldap/sync/group.rb +++ b/lib/ee/gitlab/ldap/sync/group.rb @@ -16,11 +16,13 @@ def initialize(group, proxy) end def update_permissions - lease = ::Gitlab::ExclusiveLease.new( - "ldap_group_sync:#{provider}:#{group.id}", - timeout: 3600 - ) - return unless lease.try_obtain + fail_stuck_group(group) + + if group.ldap_sync_started? + logger.debug { "Group '#{group.name}' is not ready for LDAP sync. Skipping" } + return + end + group.start_ldap_sync logger.debug { "Syncing '#{group.name}' group" } @@ -38,7 +40,7 @@ def update_permissions update_existing_group_membership(group, access_levels) add_new_members(group, access_levels) - group.update(last_ldap_sync_at: Time.now) + group.finish_ldap_sync logger.debug { "Finished syncing '#{group.name}' group" } end @@ -143,6 +145,14 @@ def select_and_preload_group_members(group) .with_identity_provider(provider).preload(:user) end + def fail_stuck_group(group) + return false unless group.ldap_sync_started? + + if group.ldap_sync_last_sync_at < 1.hour.ago + group.mark_ldap_sync_as_failed('The sync took too long to complete.') + end + end + def logger Rails.logger end diff --git a/lib/ee/gitlab/ldap/sync/groups.rb b/lib/ee/gitlab/ldap/sync/groups.rb index 6c8211395c9725..84aa69a3992942 100644 --- a/lib/ee/gitlab/ldap/sync/groups.rb +++ b/lib/ee/gitlab/ldap/sync/groups.rb @@ -73,7 +73,7 @@ def sync_external_users def groups_where_group_links_with_provider_ordered ::Group.where_group_links_with_provider(provider) .preload(:ldap_group_links) - .reorder('last_ldap_sync_at ASC, namespaces.id ASC') + .reorder('ldap_sync_last_successful_update_at ASC, namespaces.id ASC') .distinct end diff --git a/spec/lib/ee/gitlab/ldap/sync/group_spec.rb b/spec/lib/ee/gitlab/ldap/sync/group_spec.rb index 2e86ead0b5ec30..ed38c5d2edcbd9 100644 --- a/spec/lib/ee/gitlab/ldap/sync/group_spec.rb +++ b/spec/lib/ee/gitlab/ldap/sync/group_spec.rb @@ -7,9 +7,6 @@ let(:user) { create(:user) } before do - allow_any_instance_of(::Gitlab::ExclusiveLease) - .to receive(:try_obtain).and_return(true) - create(:identity, user: user, extern_uid: user_dn(user.username)) stub_ldap_config(active_directory: false) @@ -27,6 +24,35 @@ context 'with basic add/update actions' do let(:ldap_group1) { ldap_group_entry(user_dn(user.username)) } + it 'fails a stuck group older than 1 hour' do + group.start_ldap_sync + group.update_column(:ldap_sync_last_sync_at, 61.minutes.ago) + + expect(group).to receive(:mark_ldap_sync_as_failed) + + sync_group.update_permissions + end + + context 'when the group ldap sync is already started' do + it 'logs a debug message' do + group.start_ldap_sync + + expect(Rails.logger).to receive(:debug) do |&block| + expect(block.call).to match /^Group '\w*' is not ready for LDAP sync. Skipping/ + end.at_least(1).times + + sync_group.update_permissions + end + + it 'does not add new members' do + group.start_ldap_sync + + sync_group.update_permissions + + expect(group.members).not_to include(user) + end + end + it 'adds new members' do sync_group.update_permissions @@ -54,6 +80,13 @@ expect(group.members.find_by(user_id: user.id).access_level) .to eq(::Gitlab::Access::DEVELOPER) end + + it 'uses the ldap sync state machine' do + expect(group).to receive(:start_ldap_sync) + expect(group).to receive(:finish_ldap_sync) + + sync_group.update_permissions + end end context 'when existing user is no longer in LDAP group' do diff --git a/spec/models/ee/group_spec.rb b/spec/models/ee/group_spec.rb new file mode 100644 index 00000000000000..f8916d61bf6c3a --- /dev/null +++ b/spec/models/ee/group_spec.rb @@ -0,0 +1,88 @@ +require 'spec_helper' + +describe Group, models: true do + let(:group) { create(:group) } + + it { is_expected.to include_module(EE::Group) } + + describe 'states' do + it { is_expected.to be_ldap_sync_ready } + + context 'after the start transition' do + it 'sets the last sync timestamp' do + expect { group.start_ldap_sync }.to change { group.ldap_sync_last_sync_at } + end + end + + context 'after the finish transition' do + it 'sets the state to started' do + group.start_ldap_sync + + expect(group).to be_ldap_sync_started + + group.finish_ldap_sync + end + + it 'sets last update and last successful update to the same timestamp' do + group.start_ldap_sync + + group.finish_ldap_sync + + expect(group.ldap_sync_last_update_at) + .to eq(group.ldap_sync_last_successful_update_at) + end + + it 'clears previous error message on success' do + group.start_ldap_sync + group.mark_ldap_sync_as_failed('Error') + group.start_ldap_sync + + group.finish_ldap_sync + + expect(group.ldap_sync_error).to be_nil + end + end + + context 'after the fail transition' do + it 'sets the state to failed' do + group.start_ldap_sync + + group.fail_ldap_sync + + expect(group).to be_ldap_sync_failed + end + + it 'sets last update timestamp but not last successful update timestamp' do + group.start_ldap_sync + + group.fail_ldap_sync + + expect(group.ldap_sync_last_update_at) + .not_to eq(group.ldap_sync_last_successful_update_at) + end + end + end + + describe '#mark_ldap_sync_as_failed' do + it 'sets the state to failed' do + group.start_ldap_sync + + group.mark_ldap_sync_as_failed('Error') + + expect(group).to be_ldap_sync_failed + end + + it 'sets the error message' do + group.start_ldap_sync + + group.mark_ldap_sync_as_failed('Something went wrong') + + expect(group.ldap_sync_error).to eq('Something went wrong') + end + + it 'is graceful when current state is not valid for the fail transition' do + expect(group).to be_ldap_sync_ready + expect { group.mark_ldap_sync_as_failed('Error') }.not_to raise_error + end + end +end -- GitLab