Fix add_tag behaviour in dns filter

The filter should only modify the event's fields and tags if and only if
all resolves/reverses succeed. So we clone the event, modify the new
copy and return it if all operations succeed. Otherwise the original
event is not modified.

For performance reasons we could reverse the clone logic: clone the
event, modify the original event and, it case of failure, return the
backup.

Note: this changes the dns filter behaviour towards add_tag

Fixes #1795

lib/logstash/filters/dns.rb

@@ -70,11 +70,14 @@ class LogStash::Filters::DNS < LogStash::Filters::Base
   def filter(event)
     return unless filter?(event)
 
+    new_event = event.clone
+
     if @resolve
       begin
         status = Timeout::timeout(@timeout) {
-          resolve(event)
+          resolve(new_event)
         }
+        return if status.nil?
       rescue Timeout::Error
         @logger.debug("DNS: resolve action timed out")
         return
@@ -84,15 +87,18 @@ class LogStash::Filters::DNS < LogStash::Filters::Base
     if @reverse
       begin
         status = Timeout::timeout(@timeout) {
-          reverse(event)
+          reverse(new_event)
         }
+        return if status.nil?
       rescue Timeout::Error
         @logger.debug("DNS: reverse action timed out")
         return
       end
     end
 
-    filter_matched(event)
+    filter_matched(new_event)
+    yield new_event
+    event.cancel
   end
 
   private

spec/filters/dns.rb

@@ -73,14 +73,72 @@ describe LogStash::Filters::DNS do
     config <<-CONFIG
       filter {
         dns {
-          resolve => "host"
+          resolve => ["host"]
           action => "replace"
+          add_tag => ["success"]
         }
       }
     CONFIG
 
     sample("host" => "carrera.databits.net") do
       insist { subject["host"] } == "199.192.228.250"
+      insist { subject["tags"] } == ["success"]
+    end
+  end
+
+  describe "dns fail resolve lookup, don't add tag" do
+    config <<-CONFIG
+      filter {
+        dns {
+          resolve => ["host1", "host2"]
+          action => "replace"
+          add_tag => ["success"]
+        }
+      }
+    CONFIG
+
+    sample("host1" => "carrera.databits.net", "host2" => "nonexistanthostname###.net") do
+      insist { subject["tags"] }.nil?
+      insist { subject["host1"] } == "carrera.databits.net"
+      insist { subject["host2"] } == "nonexistanthostname###.net"
+    end
+  end
+
+  describe "dns resolves lookups, adds tag" do
+    config <<-CONFIG
+      filter {
+        dns {
+          resolve => ["host1", "host2"]
+          action => "replace"
+          add_tag => ["success"]
+        }
+      }
+    CONFIG
+
+    sample("host1" => "carrera.databits.net", "host2" => "carrera.databits.net") do
+      insist { subject["tags"] } == ["success"]
+    end
+  end
+
+  describe "dns resolves and reverses, fails last, no tag" do
+    config <<-CONFIG
+      filter {
+        dns {
+          resolve => ["host1"]
+          reverse => ["ip1", "ip2"]
+          action => "replace"
+          add_tag => ["success"]
+        }
+      }
+    CONFIG
+
+    sample("host1" => "carrera.databits.net",
+           "ip1" => "127.0.0.1",
+           "ip2" => "128.0.0.1") do
+      insist { subject["tags"] }.nil?
+      insist { subject["host1"] } == "carrera.databits.net"
+      insist { subject["ip1"] } == "127.0.0.1"
+      insist { subject["ip2"] } == "128.0.0.1"
     end
   end