diff --git a/lib/mongo/client.rb b/lib/mongo/client.rb index 70f5768628..2bd46df4dc 100644 --- a/lib/mongo/client.rb +++ b/lib/mongo/client.rb @@ -356,7 +356,9 @@ def hash # that the driver will communicate with for sharded topologies. If this # option is 0, then there will be no maximum number of mongoses. If the # given URI resolves to more hosts than ``:srv_max_hosts``, the client - # will ramdomly choose an ``:srv_max_hosts`` sized subset of hosts. + # will randomly choose an ``:srv_max_hosts`` sized subset of hosts. If + # srvMaxHosts is provided in the URI options, it takes precedence over this + # option. # @option options [ String ] :srv_service_name The service name to use in # the SRV DNS query. # @option options [ true, false ] :ssl Whether to use TLS. diff --git a/lib/mongo/srv/monitor.rb b/lib/mongo/srv/monitor.rb index 71911acdb2..2aa13f529c 100644 --- a/lib/mongo/srv/monitor.rb +++ b/lib/mongo/srv/monitor.rb @@ -72,7 +72,11 @@ def do_work def scan! begin last_result = Timeout.timeout(timeout) do - @resolver.get_records(@srv_uri.query_hostname) + @resolver.get_records( + @srv_uri.query_hostname, + @srv_uri.uri_options[:srv_service_name] || options[:srv_service_name], + @srv_uri.uri_options[:srv_max_hosts] || @options[:srv_max_hosts] + ) end rescue Resolv::ResolvTimeout => e log_warn("SRV monitor: timed out trying to resolve hostname #{@srv_uri.query_hostname}: #{e.class}: #{e}") diff --git a/lib/mongo/uri/srv_protocol.rb b/lib/mongo/uri/srv_protocol.rb index 4336d1c814..d1d6c0afa7 100644 --- a/lib/mongo/uri/srv_protocol.rb +++ b/lib/mongo/uri/srv_protocol.rb @@ -122,11 +122,13 @@ def raise_invalid_error!(details) end # Gets the SRV resolver. + # If domain verification fails or no SRV records are found, + # an error must not be raised per the spec; instead, a warning is logged. # # @return [ Mongo::Srv::Resolver ] def resolver @resolver ||= Srv::Resolver.new( - raise_on_invalid: true, + raise_on_invalid: false, resolv_options: options[:resolv_options], timeout: options[:connect_timeout], ) @@ -149,7 +151,11 @@ def parse!(remaining) log_debug "attempting to resolve #{hostname}" - @srv_result = resolver.get_records(hostname, uri_options[:srv_service_name], uri_options[:srv_max_hosts]) + @srv_result = resolver.get_records( + hostname, + uri_options[:srv_service_name] || options[:srv_service_name], + uri_options[:srv_max_hosts] || options[:srv_max_hosts] + ) if srv_result.empty? raise Error::NoSRVRecords.new(NO_SRV_RECORDS % hostname) end diff --git a/spec/mongo/srv/monitor_spec.rb b/spec/mongo/srv/monitor_spec.rb index aaa50c16f4..e62d02a550 100644 --- a/spec/mongo/srv/monitor_spec.rb +++ b/spec/mongo/srv/monitor_spec.rb @@ -31,202 +31,281 @@ end end - let(:srv_uri) do - Mongo::URI.get("mongodb+srv://this.is.not.used") - end - - let(:cluster) do - Mongo::Cluster.new(hosts, Mongo::Monitoring.new, monitoring_io: false) - end - - let(:monitor) do - described_class.new(cluster, srv_uri: srv_uri) - end + context 'when srv_max_hosts is not specified' do + let(:srv_uri) do + Mongo::URI.get("mongodb+srv://this.is.not.used") + end - before do - # monitor instantiation triggers cluster instantiation which - # performs real SRV lookups for the hostname. - # The next lookup (the one performed when cluster is already set up) - # is using our doubles. - RSpec::Mocks.with_temporary_scope do - allow(uri_resolver).to receive(:get_txt_options_string) - expect(Mongo::Srv::Resolver).to receive(:new).ordered.and_return(uri_resolver) - allow(resolver).to receive(:get_txt_options_string) - expect(Mongo::Srv::Resolver).to receive(:new).ordered.and_return(resolver) - monitor.send(:scan!) + let(:cluster) do + Mongo::Cluster.new(hosts, Mongo::Monitoring.new, monitoring_io: false) end - end - context 'when a new DNS record is added' do - let(:new_hosts) do - hosts + ['localhost.test.build.10gen.cc:27019'] + let(:monitor) do + described_class.new(cluster, srv_uri: srv_uri) end - let(:new_result) do - double('result').tap do |result| - allow(result).to receive(:hostname).and_return(hostname) - allow(result).to receive(:address_strs).and_return(new_hosts) - allow(result).to receive(:empty?).and_return(false) - allow(result).to receive(:min_ttl).and_return(nil) + before do + # monitor instantiation triggers cluster instantiation which + # performs real SRV lookups for the hostname. + # The next lookup (the one performed when cluster is already set up) + # is using our doubles. + RSpec::Mocks.with_temporary_scope do + allow(uri_resolver).to receive(:get_txt_options_string) + expect(Mongo::Srv::Resolver).to receive(:new).ordered.and_return(uri_resolver) + allow(resolver).to receive(:get_txt_options_string) + expect(Mongo::Srv::Resolver).to receive(:new).ordered.and_return(resolver) + monitor.send(:scan!) end end - let(:resolver) do - double('monitor resolver').tap do |resolver| - expect(resolver).to receive(:get_records).and_return(new_result) + context 'when a new DNS record is added' do + let(:new_hosts) do + hosts + ['localhost.test.build.10gen.cc:27019'] end - end - it 'adds the new host to the cluster' do - expect(cluster.servers_list.map(&:address).map(&:to_s).sort).to eq(new_hosts.sort) - end - end + let(:new_result) do + double('result').tap do |result| + allow(result).to receive(:hostname).and_return(hostname) + allow(result).to receive(:address_strs).and_return(new_hosts) + allow(result).to receive(:empty?).and_return(false) + allow(result).to receive(:min_ttl).and_return(nil) + end + end - context 'when a DNS record is removed' do - let(:new_hosts) do - hosts - ['test1.test.build.10gen.cc:27018'] - end + let(:resolver) do + double('monitor resolver').tap do |resolver| + expect(resolver).to receive(:get_records).and_return(new_result) + end + end - let(:new_result) do - double('result').tap do |result| - allow(result).to receive(:hostname).and_return(hostname) - allow(result).to receive(:address_strs).and_return(new_hosts) - allow(result).to receive(:empty?).and_return(false) - allow(result).to receive(:min_ttl).and_return(nil) + it 'adds the new host to the cluster' do + expect(cluster.servers_list.map(&:address).map(&:to_s).sort).to eq(new_hosts.sort) end end - let(:resolver) do - double('resolver').tap do |resolver| - allow(resolver).to receive(:get_records).and_return(new_result) + context 'when a DNS record is removed' do + let(:new_hosts) do + hosts - ['test1.test.build.10gen.cc:27018'] end - end - it 'adds the new host to the cluster' do - expect(cluster.addresses.map(&:to_s).sort).to eq(new_hosts.sort) - end - end + let(:new_result) do + double('result').tap do |result| + allow(result).to receive(:hostname).and_return(hostname) + allow(result).to receive(:address_strs).and_return(new_hosts) + allow(result).to receive(:empty?).and_return(false) + allow(result).to receive(:min_ttl).and_return(nil) + end + end - context 'when a single DNS record is replaced' do - let(:new_hosts) do - hosts - ['test1.test.build.10gen.cc:27018'] + ['test1.test.build.10gen.cc:27019'] - end + let(:resolver) do + double('resolver').tap do |resolver| + allow(resolver).to receive(:get_records).and_return(new_result) + end + end - let(:new_result) do - double('result').tap do |result| - allow(result).to receive(:hostname).and_return(hostname) - allow(result).to receive(:address_strs).and_return(new_hosts) - allow(result).to receive(:empty?).and_return(false) - allow(result).to receive(:min_ttl).and_return(nil) + it 'adds the new host to the cluster' do + expect(cluster.addresses.map(&:to_s).sort).to eq(new_hosts.sort) end end - let(:resolver) do - double('resolver').tap do |resolver| - allow(resolver).to receive(:get_records).and_return(new_result) + context 'when a single DNS record is replaced' do + let(:new_hosts) do + hosts - ['test1.test.build.10gen.cc:27018'] + ['test1.test.build.10gen.cc:27019'] end - end - it 'adds the new host to the cluster' do - expect(cluster.addresses.map(&:to_s).sort).to eq(new_hosts.sort) - end - end + let(:new_result) do + double('result').tap do |result| + allow(result).to receive(:hostname).and_return(hostname) + allow(result).to receive(:address_strs).and_return(new_hosts) + allow(result).to receive(:empty?).and_return(false) + allow(result).to receive(:min_ttl).and_return(nil) + end + end - context 'when all DNS result are replaced with a single record' do - let(:new_hosts) do - ['test1.test.build.10gen.cc:27019'] - end + let(:resolver) do + double('resolver').tap do |resolver| + allow(resolver).to receive(:get_records).and_return(new_result) + end + end - let(:new_result) do - double('result').tap do |result| - allow(result).to receive(:hostname).and_return(hostname) - allow(result).to receive(:address_strs).and_return(new_hosts) - allow(result).to receive(:empty?).and_return(false) - allow(result).to receive(:min_ttl).and_return(nil) + it 'adds the new host to the cluster' do + expect(cluster.addresses.map(&:to_s).sort).to eq(new_hosts.sort) end end - let(:resolver) do - double('resolver').tap do |resolver| - expect(resolver).to receive(:get_records).and_return(new_result) + context 'when all DNS result are replaced with a single record' do + let(:new_hosts) do + ['test1.test.build.10gen.cc:27019'] end - end - it 'adds the new host to the cluster' do - expect(cluster.addresses.map(&:to_s).sort).to eq(new_hosts.sort) - end - end + let(:new_result) do + double('result').tap do |result| + allow(result).to receive(:hostname).and_return(hostname) + allow(result).to receive(:address_strs).and_return(new_hosts) + allow(result).to receive(:empty?).and_return(false) + allow(result).to receive(:min_ttl).and_return(nil) + end + end - context 'when all DNS result are replaced with multiple result' do - let(:new_hosts) do - [ - 'test1.test.build.10gen.cc:27019', - 'test1.test.build.10gen.cc:27020', - ] - end + let(:resolver) do + double('resolver').tap do |resolver| + expect(resolver).to receive(:get_records).and_return(new_result) + end + end - let(:new_result) do - double('result').tap do |result| - allow(result).to receive(:hostname).and_return(hostname) - allow(result).to receive(:address_strs).and_return(new_hosts) - allow(result).to receive(:empty?).and_return(false) - allow(result).to receive(:min_ttl).and_return(nil) + it 'adds the new host to the cluster' do + expect(cluster.addresses.map(&:to_s).sort).to eq(new_hosts.sort) end end - let(:resolver) do - double('resolver').tap do |resolver| - allow(resolver).to receive(:get_records).and_return(new_result) + context 'when all DNS result are replaced with multiple result' do + let(:new_hosts) do + [ + 'test1.test.build.10gen.cc:27019', + 'test1.test.build.10gen.cc:27020', + ] + end + + let(:new_result) do + double('result').tap do |result| + allow(result).to receive(:hostname).and_return(hostname) + allow(result).to receive(:address_strs).and_return(new_hosts) + allow(result).to receive(:empty?).and_return(false) + allow(result).to receive(:min_ttl).and_return(nil) + end + end + + let(:resolver) do + double('resolver').tap do |resolver| + allow(resolver).to receive(:get_records).and_return(new_result) + end + end + + it 'adds the new host to the cluster' do + expect(cluster.addresses.map(&:to_s).sort).to eq(new_hosts.sort) end end - it 'adds the new host to the cluster' do - expect(cluster.addresses.map(&:to_s).sort).to eq(new_hosts.sort) + context 'when the DNS lookup times out' do + let(:resolver) do + double('resolver').tap do |resolver| + expect(resolver).to receive(:get_records).and_raise(Resolv::ResolvTimeout) + end + end + + it 'does not add or remove any hosts from the cluster' do + expect(cluster.addresses.map(&:to_s).sort).to eq(hosts.sort) + end end - end - context 'when the DNS lookup times out' do + context 'when the DNS lookup is unable to resolve the hostname' do let(:resolver) do - double('resolver').tap do |resolver| - expect(resolver).to receive(:get_records).and_raise(Resolv::ResolvTimeout) + double('resolver').tap do |resolver| + allow(resolver).to receive(:get_records).and_raise(Resolv::ResolvError) + end end - end - it 'does not add or remove any hosts from the cluster' do - expect(cluster.addresses.map(&:to_s).sort).to eq(hosts.sort) + it 'does not add or remove any hosts from the cluster' do + expect(cluster.addresses.map(&:to_s).sort).to eq(hosts.sort) + end end - end - context 'when the DNS lookup is unable to resolve the hostname' do - let(:resolver) do - double('resolver').tap do |resolver| - allow(resolver).to receive(:get_records).and_raise(Resolv::ResolvError) + context 'when no DNS result are returned' do + let(:new_result) do + double('result').tap do |result| + allow(result).to receive(:hostname).and_return(hostname) + allow(result).to receive(:address_strs).and_return([]) + allow(result).to receive(:empty?).and_return(true) + allow(result).to receive(:min_ttl).and_return(nil) + end + end + + let(:resolver) do + double('resolver').tap do |resolver| + allow(resolver).to receive(:get_records).and_return(new_result) + end end - end - it 'does not add or remove any hosts from the cluster' do - expect(cluster.addresses.map(&:to_s).sort).to eq(hosts.sort) + it 'does not add or remove any hosts from the cluster' do + expect(cluster.addresses.map(&:to_s).sort).to eq(hosts.sort) + end end end - context 'when no DNS result are returned' do - let(:new_result) do + context 'when srv_max_hosts is specified' do + let(:cluster) do + Mongo::Cluster.new(hosts, Mongo::Monitoring.new, monitoring_io: false) + end + + let(:limited_result) do double('result').tap do |result| allow(result).to receive(:hostname).and_return(hostname) - allow(result).to receive(:address_strs).and_return([]) - allow(result).to receive(:empty?).and_return(true) + allow(result).to receive(:address_strs).and_return([hosts.first]) + allow(result).to receive(:empty?).and_return(false) allow(result).to receive(:min_ttl).and_return(nil) end end let(:resolver) do double('resolver').tap do |resolver| - allow(resolver).to receive(:get_records).and_return(new_result) + allow(resolver).to receive(:get_txt_options_string) + expect(resolver).to receive(:get_records).at_least(:once).with( + anything, + anything, + 1 # Verify srv_max_hosts=1 is passed + ).and_return(limited_result) end end - it 'does not add or remove any hosts from the cluster' do - expect(cluster.addresses.map(&:to_s).sort).to eq(hosts.sort) + context 'when srv_max_hosts=1 in the URI' do + let(:srv_uri_with_max_hosts) do + Mongo::URI.get("mongodb+srv://this.is.not.used/?srvMaxHosts=1") + end + + let(:monitor_with_max_hosts) do + described_class.new(cluster, srv_uri: srv_uri_with_max_hosts) + end + + before do + # monitor instantiation triggers cluster instantiation which + # performs real SRV lookups for the hostname. + # The next lookup (the one performed when cluster is already set up) + # is using our doubles. + RSpec::Mocks.with_temporary_scope do + allow(uri_resolver).to receive(:get_txt_options_string) + expect(Mongo::Srv::Resolver).to receive(:new).ordered.and_return(uri_resolver) + allow(resolver).to receive(:get_txt_options_string) + expect(Mongo::Srv::Resolver).to receive(:new).ordered.and_return(resolver) + monitor_with_max_hosts.send(:scan!) + end + end + + it 'limits the number of hosts in the cluster and passes srv_max_hosts to resolver' do + expect(cluster.servers_list.size).to eq(1) + end + end + + context 'when srv_max_hosts is set on client' do + it 'creates client with srv_max_hosts and passes it to resolver on scan' do + client = nil + expect(Mongo::Srv::Resolver).to receive(:new).at_least(:once).and_return(resolver) + + client = Mongo::Client.new('mongodb+srv://test.example.com/', srv_max_hosts: 1, monitoring_io: false, connect: :sharded) + + # Start SRV monitor but don't run the background thread + client.cluster.send(:start_stop_srv_monitor) + + srv_monitor = client.cluster.instance_variable_get(:@srv_monitor) + + # Stop the background thread if it's running + srv_monitor.stop! if srv_monitor.running? + + srv_monitor.instance_variable_set(:@resolver, resolver) + + srv_monitor.send(:scan!) + + expect(client.cluster.servers_list.size).to eq(1) + end end end end