diff --git a/app/models/group.rb b/app/models/group.rb index 8a136590a64a62b5dd258d6d4026d85b210e1136..c0c7d04b930e4f7e4bf4b9185ca7716a0892c62a 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -32,6 +32,37 @@ class Group < Namespace joins(:ldap_group_links).where(ldap_group_links: { provider: provider }) end + state_machine :ldap_sync_status, initial: :ready do + state :ready + state :started + state :failed + + event :ldap_sync_start do + transition [:ready, :failed] => :started + end + + event :ldap_sync_finish do + transition started: :ready + end + + event :ldap_sync_fail do + transition started: :failed + 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 + class << self # Searches for groups matching the given query. # @@ -185,4 +216,9 @@ def system_hook_service def first_non_empty_project projects.detect{ |project| !project.empty_repo? } end + + def mark_ldap_sync_as_failed(error_message) + ldap_sync_fail + update_column(:ldap_sync_error, Gitlab::UrlSanitizer.sanitize(error_message)) + end end diff --git a/app/workers/ldap_group_sync_worker.rb b/app/workers/ldap_group_sync_worker.rb index 4f1a715126910b82f7cca8e9b450eb7326339ada..ccecb9688fbbabef6218b164b3af8a4edeefdae2 100644 --- a/app/workers/ldap_group_sync_worker.rb +++ b/app/workers/ldap_group_sync_worker.rb @@ -3,9 +3,9 @@ class LdapGroupSyncWorker sidekiq_options retry: false - def perform + def perform(group_id = nil) logger.info 'Started LDAP group sync' - Gitlab::LDAP::GroupSync.execute + Gitlab::LDAP::GroupSync.execute(group_id) logger.info 'Finished LDAP group sync' end end diff --git a/db/migrate/20160623160821_add_ldap_sync_state_to_groups.rb b/db/migrate/20160623160821_add_ldap_sync_state_to_groups.rb new file mode 100644 index 0000000000000000000000000000000000000000..0c1b87563b20fbd181bc2b27a8bfbfd17ba951bf --- /dev/null +++ b/db/migrate/20160623160821_add_ldap_sync_state_to_groups.rb @@ -0,0 +1,19 @@ +# Migration type: online without errors (works on previous version and new one) +class AddLdapSyncStateToGroups < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + disable_ddl_transaction! + + 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 + 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 + end +end diff --git a/db/migrate/20160623170821_add_ldap_sync_state_indices_to_groups.rb b/db/migrate/20160623170821_add_ldap_sync_state_indices_to_groups.rb new file mode 100644 index 0000000000000000000000000000000000000000..2c656994e686307f6cad0392dd6b1831c6530028 --- /dev/null +++ b/db/migrate/20160623170821_add_ldap_sync_state_indices_to_groups.rb @@ -0,0 +1,15 @@ +# Migration type: online without errors (works on previous version and new one) +class AddLdapSyncStateIndicesToGroups < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + disable_ddl_transaction! + + 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/20160623200908_remove_last_ldap_sync_status_index_from_groups.rb b/db/migrate/20160623200908_remove_last_ldap_sync_status_index_from_groups.rb new file mode 100644 index 0000000000000000000000000000000000000000..40ddb24bb0181ce6d3c305cf309b939c3f823344 --- /dev/null +++ b/db/migrate/20160623200908_remove_last_ldap_sync_status_index_from_groups.rb @@ -0,0 +1,13 @@ +# Migration type: online without errors (works on previous version and new one) +class RemoveLastLdapSyncStatusIndexFromGroups < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + disable_ddl_transaction! + + 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/20160623210606_remove_last_ldap_sync_status_from_groups.rb b/db/migrate/20160623210606_remove_last_ldap_sync_status_from_groups.rb new file mode 100644 index 0000000000000000000000000000000000000000..4ef4615178c410c1f3ff2553df38d4da46ce9e6c --- /dev/null +++ b/db/migrate/20160623210606_remove_last_ldap_sync_status_from_groups.rb @@ -0,0 +1,7 @@ +# Migration type: online without errors (works on previous version and new one) +class RemoveLastLdapSyncStatusFromGroups < ActiveRecord::Migration + + def change + remove_column :namespaces, :last_ldap_sync_at, :datetime + end +end diff --git a/db/schema.rb b/db/schema.rb index 9d64545bf8c913bb16e9f2a0dc4623b081a7f86f..eb9b620314e9f53d276eb1325f99f6d8963d1f2a 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20160621123729) do +ActiveRecord::Schema.define(version: 20160623210606) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -553,8 +553,8 @@ t.integer "weight" t.boolean "confidential", default: false t.datetime "deleted_at" - t.date "due_date" t.integer "moved_to_id" + t.date "due_date" end add_index "issues", ["assignee_id"], name: "index_issues_on_assignee_id", using: :btree @@ -745,22 +745,26 @@ add_index "milestones", ["title"], name: "index_milestones_on_title_trigram", using: :gin, opclasses: {"title"=>"gin_trgm_ops"} create_table "namespaces", force: :cascade do |t| - t.string "name", null: false - t.string "path", null: false + t.string "name", null: false + t.string "path", null: false t.integer "owner_id" t.datetime "created_at" t.datetime "updated_at" t.string "type" - t.string "description", default: "", null: false + t.string "description", default: "", null: false t.string "avatar" - 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 "membership_lock", default: false + t.boolean "share_with_group_lock", default: false + t.integer "visibility_level", default: 20, 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" 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/gitlab/ldap/group_sync.rb b/lib/gitlab/ldap/group_sync.rb index 88886759755d53d01cb8bb300ee4ebb86d9ba455..b71693c0b9ff3b321bfe70cd3945898927c28700 100644 --- a/lib/gitlab/ldap/group_sync.rb +++ b/lib/gitlab/ldap/group_sync.rb @@ -3,23 +3,23 @@ module Gitlab module LDAP class GroupSync - attr_reader :provider + attr_reader :provider, :group_id # Open a connection so we can run all queries through it. # It's more efficient than the default of opening/closing per LDAP query. - def self.open(provider, &block) + def self.open(provider, group_id, &block) Gitlab::LDAP::Adapter.open(provider) do |adapter| - block.call(self.new(provider, adapter)) + block.call(self.new(provider, adapter, group_id)) end end - def self.execute + def self.execute(group_id = nil) # Shuffle providers to prevent a scenario where sync fails after a time # and only the first provider or two get synced. This shuffles the order # so subsequent syncs should eventually get to all providers. Obviously # we should avoid failure, but this is an additional safeguard. Gitlab::LDAP::Config.providers.shuffle.each do |provider| - self.open(provider) do |group_sync| + self.open(provider, group_id) do |group_sync| group_sync.update_permissions end end @@ -27,9 +27,10 @@ def self.execute true end - def initialize(provider, adapter = nil) + def initialize(provider, adapter = nil, group_id = nil) @adapter = adapter @provider = provider + @group_id = group_id end def update_permissions @@ -41,7 +42,8 @@ def update_permissions logger.debug { "No `group_base` configured for '#{provider}' provider. Skipping" } end - if admin_group.present? + # Only run admin sync during full sync + if admin_group.present? && !group_id logger.debug { "Syncing admin users for '#{provider}' provider" } sync_admin_users logger.debug { "Finished syncing admin users for '#{provider}' provider" } @@ -64,14 +66,16 @@ def update_permissions # representing a user's highest access level among the LDAP links within # the same GitLab group. def sync_groups - # Order results by last_ldap_sync_at ASC so groups with older last - # sync time are handled first + # Order results by ldap_sync_last_successful_update_at ASC so groups + # with older last sync time are handled first groups_where_group_links_with_provider_ordered.each do |group| - lease = Gitlab::ExclusiveLease.new( - "ldap_group_sync:#{provider}:#{group.id}", - timeout: 3600 - ) - next unless lease.try_obtain + fail_stuck_group(group) + + unless group.ldap_sync_status == 'ready' + logger.debug { "Group '#{group.name}' is not ready. Skipping" } + next + end + group.ldap_sync_start logger.debug { "Syncing '#{group.name}' group" } @@ -89,7 +93,7 @@ def sync_groups update_existing_group_membership(group, access_levels) add_new_members(group, access_levels) - group.update(last_ldap_sync_at: Time.now) + group.ldap_sync_finish logger.debug { "Finished syncing '#{group.name}' group" } end @@ -377,12 +381,25 @@ def select_and_preload_group_members(group) end def groups_where_group_links_with_provider_ordered - ::Group.where_group_links_with_provider(provider) + Rails.logger.debug { "Syncing group(s): #{group_id.inspect}" } + + group_id_condition = { id: group_id } if group_id + + ::Group.where(group_id_condition) + .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 + def fail_stuck_group(group) + return unless group.ldap_sync_status == '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/spec/lib/gitlab/ldap/group_sync_spec.rb b/spec/lib/gitlab/ldap/group_sync_spec.rb index 46ff87a9172ab1ad94c70ced50f59b2e9d4d544c..629c15f9bee7d9be6bfb6becc5835c8cfaafe5d3 100644 --- a/spec/lib/gitlab/ldap/group_sync_spec.rb +++ b/spec/lib/gitlab/ldap/group_sync_spec.rb @@ -37,6 +37,20 @@ it { is_expected.to receive(:sync_admin_users) } it { is_expected.not_to receive(:sync_groups) } end + + context 'when syncing specific groups' do + before do + allow(group_sync) + .to receive_messages( + group_base: 'my-group-base', + admin_group: 'my-admin-group', + group_ids: [1, 2, 3] + ) + end + + it { is_expected.to receive(:sync_groups) } + it { is_expected.not_to receive(:sync_admin_groups) } + end end describe '#sync_groups' do @@ -297,6 +311,28 @@ expect(group2.members.pluck(:access_level).uniq).to eq([Gitlab::Access::MASTER]) end end + + context 'when syncing specific groups' do + let(:specific_group_sync) do + Gitlab::LDAP::GroupSync.new('ldapmain', nil, group1.id) + end + + it 'adds new members to group1' do + expect { specific_group_sync.sync_groups } + .to change { group1.members.where(user_id: user1.id).any? } + .from(false).to(true) + end + + it 'does not add new members to group2' do + expect { specific_group_sync.sync_groups } + .not_to change { + group2.members.where( + user_id: user2.id, + access_level: Gitlab::Access::OWNER + ).any? + } + end + end end # Test that membership can be resolved for all different type of LDAP groups