From f7ecd481f936dbef0a893dda17ab4b5b5faf556b Mon Sep 17 00:00:00 2001 From: Matthew Riddle Date: Mon, 23 Mar 2020 14:46:53 +0100 Subject: [PATCH] ID validation This patch adds `auto_increment_increment` validation to the following scenarios: * Connection initalization * Generating one ID * Generating multiple IDs These changes should have no noticeable impact on performance and are thread safe. The intent is to prevent data corruption when a server is misconfigured and in the event that one is found, it's removed from the pool of available alloc servers allowing creation to continue with the valid alloc servers. Validation during initialization is done by comparing the client configuration and with what's set on each server. If the server is misconfigured, an error is reported via the `notifier` and it's left out of the pool. Validation during creation happens by comparing each pair of identifiers and ensuring they've been incremented correctly by the configured amount. If the `auto_increment_increment` changes while the clients are using the server, the alloc server is removed from the list of servers and an error is reported via the `notifier`. An InvalidIncrementException is raised if all servers are misconfigured. --- global_uid.gemspec | 1 + lib/global_uid.rb | 3 + lib/global_uid/allocations.rb | 28 +++++++ lib/global_uid/base.rb | 31 +++++++- test/global_uid_test.rb | 136 +++++++++++++++++++++++++++++++--- test/models.rb | 1 + test/test_helper.rb | 9 +-- 7 files changed, 187 insertions(+), 22 deletions(-) create mode 100644 lib/global_uid/allocations.rb diff --git a/global_uid.gemspec b/global_uid.gemspec index eb20b12..5e40504 100644 --- a/global_uid.gemspec +++ b/global_uid.gemspec @@ -18,6 +18,7 @@ Gem::Specification.new 'global_uid', '3.7.1' do |s| s.add_development_dependency('minitest-rg') s.add_development_dependency('minitest-line') s.add_development_dependency('mocha') + s.add_development_dependency('benchmark-ips') s.add_development_dependency('bump') s.add_development_dependency('phenix') s.add_development_dependency('pry') diff --git a/lib/global_uid.rb b/lib/global_uid.rb index 9a0e7f5..ab368ce 100644 --- a/lib/global_uid.rb +++ b/lib/global_uid.rb @@ -1,5 +1,6 @@ # frozen_string_literal: true require "global_uid/base" +require "global_uid/allocations" require "global_uid/active_record_extension" require "global_uid/has_and_belongs_to_many_builder_extension" require "global_uid/migration_extension" @@ -9,6 +10,8 @@ module GlobalUid class NoServersAvailableException < StandardError ; end class ConnectionTimeoutException < StandardError ; end class TimeoutException < StandardError ; end + class InvalidIncrementException < StandardError ; end + class InvalidIncrementPreventionException < StandardError ; end end ActiveRecord::Base.send(:include, GlobalUid::ActiveRecordExtension) diff --git a/lib/global_uid/allocations.rb b/lib/global_uid/allocations.rb new file mode 100644 index 0000000..1e48126 --- /dev/null +++ b/lib/global_uid/allocations.rb @@ -0,0 +1,28 @@ +class Allocations + attr_reader :allocations, :max_size, :incrementing_by + + def initialize(max_size: 5, incrementing_by:) + @allocations = [] + @max_size = max_size + @incrementing_by = incrementing_by + end + + def add(identifier) + allocations.shift if allocations.size >= max_size + allocations << identifier + valid_increment? + end + alias_method :<<, :add + + def to_s + allocations.to_s + end + + private + + def valid_increment? + allocations[1..-1].all? do |identifier| + (identifier - allocations[0]) % incrementing_by == 0 + end + end +end diff --git a/lib/global_uid/base.rb b/lib/global_uid/base.rb index 1498094..f4b5407 100644 --- a/lib/global_uid/base.rb +++ b/lib/global_uid/base.rb @@ -9,7 +9,7 @@ class Base GLOBAL_UID_DEFAULTS = { :connection_timeout => 3, :connection_retry => 10.minutes, - :notifier => Proc.new { |exception, message| ActiveRecord::Base.logger.error("GlobalUID error: #{exception} #{message}") }, + :notifier => Proc.new { |exception, message| ActiveRecord::Base.logger.error("GlobalUID error: #{exception.class} #{message}") }, :query_timeout => 10, :increment_by => 5, # This will define the maximum number of servers that you can have :disabled => false, @@ -84,6 +84,7 @@ def self.init_server_info info[:retry_at] = nil info[:rand] = rand info[:new?] = true + info[:allocated_ids] = Allocations.new(incrementing_by: increment_by) info end end @@ -94,6 +95,7 @@ def self.disconnect! def self.setup_connections! connection_timeout = self.global_uid_options[:connection_timeout] + increment_by = self.global_uid_options[:increment_by] if self.servers.nil? self.servers = init_server_info @@ -107,9 +109,18 @@ def self.setup_connections! if info[:new?] || ( info[:retry_at] && Time.now > info[:retry_at] ) info[:new?] = false - connection = new_connection(info[:name], connection_timeout) - info[:cx] = connection - info[:retry_at] = Time.now + self.global_uid_options[:connection_retry] if connection.nil? + begin + connection = new_connection(info[:name], connection_timeout) + if !connection.nil? && increment_by != (db_increment = connection.select_value("SELECT @@auto_increment_increment")) + raise InvalidIncrementPreventionException, "Configured: '#{increment_by}', Found: '#{db_increment}' on '#{connection.current_database}'" + end + + info[:cx] = connection + info[:retry_at] = Time.now + self.global_uid_options[:connection_retry] if connection.nil? + rescue InvalidIncrementPreventionException => e + notify e, "#{e.message}" + info[:cx] = nil + end end end @@ -158,10 +169,19 @@ def self.get_connections with_connections {} end + def self.validate_increment!(new_record_id, connection) + server = self.servers.find { |s| connection.current_database.include?(s[:name]) } + + unless server[:allocated_ids] << new_record_id + raise InvalidIncrementException, "Recently allocated IDs: #{server[:allocated_ids]} on '#{connection.current_database}'" + end + end + def self.get_uid_for_class(klass) with_connections do |connection| Timeout.timeout(self.global_uid_options[:query_timeout], TimeoutException) do id = connection.insert("REPLACE INTO #{klass.global_uid_table} (stub) VALUES ('a')") + validate_increment!(id, connection) return id end end @@ -173,6 +193,9 @@ def self.get_many_uids_for_class(klass, count) with_connections do |connection| Timeout.timeout(self.global_uid_options[:query_timeout], TimeoutException) do increment_by = connection.select_value("SELECT @@auto_increment_increment") + if increment_by != self.global_uid_options[:increment_by] + raise InvalidIncrementPreventionException, "Configured: '#{self.global_uid_options[:increment_by]}', Found: '#{increment_by}' on '#{connection.current_database}'" + end start_id = connection.insert("REPLACE INTO #{klass.global_uid_table} (stub) VALUES " + (["('a')"] * count).join(',')) return start_id.step(start_id + (count-1) * increment_by, increment_by).to_a end diff --git a/test/global_uid_test.rb b/test/global_uid_test.rb index 0f0cd7b..1808271 100644 --- a/test/global_uid_test.rb +++ b/test/global_uid_test.rb @@ -205,6 +205,33 @@ def table_exists?(connection, table) end end + describe "Performance" do + before do + CreateWithNoParams.up + CreateWithoutGlobalUIDs.up + end + + it "has a negligible performance impact" do + report = Benchmark.ips do |x| + x.report(WithGlobalUID) { WithGlobalUID.create! } + x.report(WithoutGlobalUID) { WithoutGlobalUID.create! } + x.compare! + end + + with_global_uid = report.entries.find { |e| e.label == WithGlobalUID }.stats + without_global_uid = report.entries.find { |e| e.label == WithoutGlobalUID }.stats + + performance_impact, _ = with_global_uid.slowdown(without_global_uid) + # assert performance_impact < 2 # The results vary too much on CI :( + end + + after do + reset_connections! + CreateWithNoParams.down + CreateWithoutGlobalUIDs.down + end + end + describe "With GlobalUID" do before do CreateWithNoParams.up @@ -222,6 +249,90 @@ def table_exists?(connection, table) it "get a unique id" do test_unique_ids end + + describe 'when the auto_increment_increment changes' do + before do + @notifications = [] + GlobalUid::Base.global_uid_options[:notifier] = Proc.new do |exception, message| + GlobalUid::Base::GLOBAL_UID_DEFAULTS[:notifier].call(exception, message) + @notifications << exception + end + end + + describe "and all servers report a value other than what's configured" do + it "raises an exception when configuration incorrect during initialization" do + GlobalUid::Base.global_uid_options[:increment_by] = 42 + reset_connections! + assert_raises(GlobalUid::NoServersAvailableException) { test_unique_ids(10) } + assert_includes(@notifications, GlobalUid::InvalidIncrementPreventionException) + end + + it "raises an exception, preventing duplicate ID generation" do + GlobalUid::Base.with_connections do |con| + con.execute("SET SESSION auto_increment_increment = 42") + end + + assert_raises(GlobalUid::NoServersAvailableException) { test_unique_ids(10) } + assert_includes(@notifications, GlobalUid::InvalidIncrementException) + end + + it "raises an exception before attempting to generate many UIDs" do + GlobalUid::Base.with_connections do |con| + con.execute("SET SESSION auto_increment_increment = 42") + end + + assert_raises GlobalUid::NoServersAvailableException do + WithGlobalUID.generate_many_uids(10) + end + assert_includes(@notifications, GlobalUid::InvalidIncrementPreventionException) + end + + it "doesn't cater for increment_by being increased by a factor of x" do + GlobalUid::Base.with_connections do |connection| + connection.execute("SET SESSION auto_increment_increment = #{GlobalUid::Base::GLOBAL_UID_DEFAULTS[:increment_by] * 2}") + end + # Due to multiple processes and threads sharing the same alloc server, identifiers may be provisioned + # before the current thread receives its next one. We rely on the gap being divisible by the configured increment + test_unique_ids(10) + assert_empty(@notifications) + end + end + + describe "and only one server reports a value other than what's configured" do + it "notifies the client when configuration incorrect during initialization" do + modified_connection = Proc.new do |name, connection_timeout, offset, increment_by| + config = ActiveRecord::Base.configurations.to_h[name] + ActiveRecord::Base.mysql2_connection(config).tap do |connection| + connection.execute("SET SESSION auto_increment_increment = 42") if name == "test_id_server_1" + end + end + + GlobalUid::Base.stub :new_connection, modified_connection do + reset_connections! + test_unique_ids(32) + assert_includes(@notifications, GlobalUid::InvalidIncrementPreventionException) + end + end + + it "notifies the client and continues with the other connection" do + con = GlobalUid::Base.get_connections.first + con.execute("SET SESSION auto_increment_increment = 42") + + # Trigger the exception, one call may not hit the server, there's still a 1/(2^32) chance of failure. + test_unique_ids(32) + assert_includes(@notifications, GlobalUid::InvalidIncrementException) + end + + it "notifies the client and continues when attempting to generate many UIDs" do + con = GlobalUid::Base.get_connections.first + con.execute("SET SESSION auto_increment_increment = 42") + + # Trigger the exception, one call may not hit the server, there's still a 1/(2^32) chance of failure. + 32.times { WithGlobalUID.generate_many_uids(10) } + assert_includes(@notifications, GlobalUid::InvalidIncrementPreventionException) + end + end + end end describe "With a timing out server" do @@ -394,15 +505,15 @@ def parent_child_fork_values private - def test_unique_ids + def test_unique_ids(amount = 10) seen = {} - (0..10).each do - foo = WithGlobalUID.new - foo.save - refute_nil foo.id - assert_nil foo.description - refute seen.has_key?(foo.id) - seen[foo.id] = 1 + amount.times do + WithGlobalUID.create!.tap do |record| + refute_nil record.id + assert_nil record.description + refute seen.has_key?(record.id) + seen[record.id] = true + end end end @@ -411,8 +522,13 @@ def reset_connections! end def restore_defaults! - GlobalUid::Base.global_uid_options[:storage_engine] = nil - GlobalUid::Base.global_uid_options[:disabled] = false + GlobalUid::Base.global_uid_options = { + :disabled => false, + :id_servers => [ + "test_id_server_1", + "test_id_server_2" + ] + } end def show_create_sql(klass, table) diff --git a/test/models.rb b/test/models.rb index 1d8ab94..e3e7399 100644 --- a/test/models.rb +++ b/test/models.rb @@ -3,6 +3,7 @@ class WithGlobalUID < ActiveRecord::Base end class WithoutGlobalUID < ActiveRecord::Base + disable_global_uid end class Parent < ActiveRecord::Base diff --git a/test/test_helper.rb b/test/test_helper.rb index 4500321..cd12c1e 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true require 'bundler/setup' require "active_record" +require 'benchmark/ips' require 'minitest/autorun' require 'minitest/rg' require 'minitest/line/describe_track' @@ -9,14 +10,6 @@ require 'phenix' require 'pry' -GlobalUid::Base.global_uid_options = { - :disabled => false, - :id_servers => [ - "test_id_server_1", - "test_id_server_2" - ] -} - Phenix.configure do |config| config.database_config_path = File.join(File.dirname(__FILE__), "config/database.yml") end