Skip to content

Commit

Permalink
handle lookup misses without exceptions
Browse files Browse the repository at this point in the history
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#60
  • Loading branch information
yaauie committed Jan 4, 2020
1 parent 1db63f8 commit c739549
Show file tree
Hide file tree
Showing 2 changed files with 56 additions and 21 deletions.
58 changes: 45 additions & 13 deletions lib/logstash/filters/dns.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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.",
Expand Down Expand Up @@ -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.",
Expand Down Expand Up @@ -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
19 changes: 11 additions & 8 deletions spec/filters/dns_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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

Expand Down

0 comments on commit c739549

Please sign in to comment.