From c73954966de22bf61492f48d7783497430f63bc0 Mon Sep 17 00:00:00 2001 From: Ry Biesemeyer Date: Fri, 3 Jan 2020 19:48:43 +0000 Subject: [PATCH] handle lookup misses without exceptions By using `Resolv#each_name` and `Resolv#each_address`, and returning the first match from the block or returning nil if no matches were found, we can mirror the behavior of `Resolv#getname` and `Resolv#getaddress` without raising exceptions on lookup misses. Adjusts flow-control to handle a nil return value as a lookup miss instead of relying on exception handlers. Resolves: logstash-plugins/logstash-filter-dns#60 --- lib/logstash/filters/dns.rb | 58 ++++++++++++++++++++++++++++--------- spec/filters/dns_spec.rb | 19 +++++++----- 2 files changed, 56 insertions(+), 21 deletions(-) diff --git a/lib/logstash/filters/dns.rb b/lib/logstash/filters/dns.rb index ad8dfbe..cbdfd89 100644 --- a/lib/logstash/filters/dns.rb +++ b/lib/logstash/filters/dns.rb @@ -195,11 +195,12 @@ def resolve(event) else address = retriable_getaddress(raw) end - rescue Resolv::ResolvError => e - @failed_cache[raw] = true if @failed_cache - @logger.debug("DNS: couldn't resolve the hostname.", - :field => field, :value => raw, :message => e.message) - return + if address.nil? + @failed_cache[raw] = true if @failed_cache + @logger.debug("DNS: couldn't resolve the hostname.", + :field => field, :value => raw) + return + end rescue Resolv::ResolvTimeout, Timeout::Error @failed_cache[raw] = true if @failed_cache @logger.warn("DNS: timeout on resolving the hostname.", @@ -275,11 +276,12 @@ def reverse(event) else hostname = retriable_getname(raw) end - rescue Resolv::ResolvError => e - @failed_cache[raw] = true if @failed_cache - @logger.debug("DNS: couldn't resolve the address.", - :field => field, :value => raw, :message => e.message) - return + if hostname.nil? + @failed_cache[raw] = true if @failed_cache + @logger.debug("DNS: couldn't resolve the address.", + :field => field, :value => raw) + return + end rescue Resolv::ResolvTimeout, Timeout::Error @failed_cache[raw] = true if @failed_cache @logger.warn("DNS: timeout on resolving address.", @@ -350,13 +352,43 @@ def retriable_getaddress(name) private def getname(address) - name = @resolv.getname(address).force_encoding(Encoding::UTF_8) - IDN.toUnicode(name) + name = resolv_getname_or_nil(@resolv, address) + name && name.force_encoding(Encoding::UTF_8) + name && IDN.toUnicode(name) end private def getaddress(name) idn = IDN.toASCII(name) - @resolv.getaddress(idn).force_encoding(Encoding::UTF_8) + address = resolv_getaddress_or_nil(@resolv, idn) + address && address.force_encoding(Encoding::UTF_8) + end + + private + def resolv_getname_or_nil(resolver, address) + # `Resolv#each_name` yields to the provided block zero or more times; + # to prevent it from yielding multiple times when more than one match + # is found, we return directly in the block. + # See also `Resolv#getname` + resolver.each_name(address) do |name| + return name + end + + # If no match was found, we return nil. + return nil + end + + private + def resolv_getaddress_or_nil(resolver, name) + # `Resolv#each_address` yields to the provided block zero or more times; + # to prevent it from yielding multiple times when more than one match + # is found, we return directly in the block. + # See also `Resolv#getaddress` + resolver.each_address(name) do |address| + return address + end + + # If no match was found, we return nil. + return nil end end # class LogStash::Filters::DNS diff --git a/spec/filters/dns_spec.rb b/spec/filters/dns_spec.rb index 04147d5..8dc8f06 100644 --- a/spec/filters/dns_spec.rb +++ b/spec/filters/dns_spec.rb @@ -6,13 +6,16 @@ describe LogStash::Filters::DNS do describe "with stubbed Resolv" do before(:each) do - allow_any_instance_of(Resolv).to receive(:getaddress).with("carrera.databits.net").and_return("199.192.228.250") - allow_any_instance_of(Resolv).to receive(:getaddress).with("does.not.exist").and_raise(Resolv::ResolvError) - allow_any_instance_of(Resolv).to receive(:getaddress).with("nonexistanthostname###.net").and_raise(Resolv::ResolvError) - allow_any_instance_of(Resolv).to receive(:getname).with("199.192.228.250").and_return("carrera.databits.net") - allow_any_instance_of(Resolv).to receive(:getname).with("127.0.0.1").and_return("localhost") - allow_any_instance_of(Resolv).to receive(:getname).with("128.0.0.1").and_raise(Resolv::ResolvError) - allow_any_instance_of(Resolv).to receive(:getname).with("199.192.228.250").and_return("carrera.databits.net") + # We use `Resolv#each_address` and `Resolv#each_name`, which have + # undefined return values but _yield_ once per result, so our stubs + # need to either yield a result or not yield at all if there is no result. + allow_any_instance_of(Resolv).to receive(:each_address).with("carrera.databits.net").and_yield("199.192.228.250") + allow_any_instance_of(Resolv).to receive(:each_address).with("does.not.exist") # no yield + allow_any_instance_of(Resolv).to receive(:each_address).with("nonexistanthostname###.net") # no yield + allow_any_instance_of(Resolv).to receive(:each_name).with("199.192.228.250").and_yield("carrera.databits.net") + allow_any_instance_of(Resolv).to receive(:each_name).with("127.0.0.1").and_yield("localhost") + allow_any_instance_of(Resolv).to receive(:each_name).with("128.0.0.1") # no yield + allow_any_instance_of(Resolv).to receive(:each_name).with("199.192.228.250").and_yield("carrera.databits.net") end describe "dns reverse lookup, replace (on a field)" do @@ -305,7 +308,7 @@ let(:event2) { LogStash::Event.new("message" => "unkownhost") } before(:each) do - allow(subject).to receive(:getaddress).and_raise Resolv::ResolvError + allow(subject).to receive(:getaddress).and_return(nil) subject.register end