[2157] increment opcode counter if message header parse succeeded

diff --git a/src/bin/auth/auth_srv.cc b/src/bin/auth/auth_srv.cc
index 6a58caebab6940f7c376da90d2055f6b7ca8a01e..81ae05d22dd13fc1f6e840a86b9aec56933b1d41 100644 (file)
--- a/src/bin/auth/auth_srv.cc
+++ b/src/bin/auth/auth_srv.cc
@@ -525,6 +525,9 @@ AuthSrv::processMessage(const IOMessage& io_message, Message& message,
         return;
     }
 
+    const Opcode opcode = message.getOpcode();
+    stats_attrs.setRequestOpCode(opcode);
+
     try {
         // Parse the message.
         message.fromWire(request_buffer);
@@ -573,7 +576,6 @@ AuthSrv::processMessage(const IOMessage& io_message, Message& message,
         return;
     }
 
-    const Opcode opcode = message.getOpcode();
     bool send_answer = true;
     try {
         // note: This can only be reliable after TSIG check succeeds.
@@ -584,8 +586,6 @@ AuthSrv::processMessage(const IOMessage& io_message, Message& message,
         }
 
         // note: This can only be reliable after TSIG check succeeds.
-        stats_attrs.setRequestOpCode(opcode);
-
         if (opcode == Opcode::NOTIFY()) {
             send_answer = impl_->processNotify(io_message, message, buffer,
                                                tsig_context, stats_attrs);

diff --git a/src/bin/auth/b10-auth.xml.pre b/src/bin/auth/b10-auth.xml.pre
index 0ecc49cf2f6eec3466feedc9435658d5dd22616a..45fb68472c686631e0332af9fcfe9346e779a24f 100644 (file)
--- a/src/bin/auth/b10-auth.xml.pre
+++ b/src/bin/auth/b10-auth.xml.pre
@@ -196,21 +196,6 @@
 
 <!-- ### STATISTICS DATA PLACEHOLDER ### -->
 
-    <note><para>
-      DNS parameters (e.g. opcode) in the request message will not be
-      considered if <command>b10-auth</command> failed to parse message,
-      including but not limited to the following circumstance:
-      <itemizedlist>
-        <listitem><para>
-          EDNS version of a message is unknown to
-          <command>b10-auth</command>.
-        </para></listitem>
-        <listitem><para>
-          TSIG signature verification fails.
-        </para></listitem>
-      </itemizedlist>
-    </para></note>
-
   </refsect1>
 
   <refsect1>

diff --git a/src/bin/auth/statistics.cc.pre b/src/bin/auth/statistics.cc.pre
index 199068e719e085775ffc1e3d6ca53dfba4c3975c..7df39de07d8f5a809bff4f5b0e7c65a64ff6c9ad 100644 (file)
--- a/src/bin/auth/statistics.cc.pre
+++ b/src/bin/auth/statistics.cc.pre
@@ -155,7 +155,8 @@ Counters::incRequest(const MessageAttributes& msgattrs) {
     // OPCODE
     const boost::optional<isc::dns::Opcode>& opcode =
         msgattrs.getRequestOpCode();
-    // Increment opcode counter only if the opcode exists.
+    // Increment opcode counter only if the opcode exists; it can happen if
+    // short message which does not contain DNS header received.
     if (opcode) {
         server_msg_counter_.inc(opcode_to_msgcounter[opcode.get().getCode()]);
     }
@@ -199,7 +200,13 @@ Counters::incResponse(const MessageAttributes& msgattrs,
 
     const boost::optional<isc::dns::Opcode>& opcode =
         msgattrs.getRequestOpCode();
-    if (opcode && opcode.get() == Opcode::QUERY()) {
+    if (!opcode) {
+        isc_throw(isc::Unexpected, "Opcode of the request is empty while it is"
+                                   " responded");
+    }
+    if (!msgattrs.getRequestSigBadSig() &&
+               opcode.get() == Opcode::QUERY())
+    {
         // compound attributes
         const unsigned int answer_rrs =
             response.getRRCount(Message::SECTION_ANSWER);

diff --git a/src/bin/auth/statistics.h b/src/bin/auth/statistics.h
index 18d2f83f094adb6b9d29b894db3b5a5cdef57c61..c845a6eee22d2780fbb63661cd8d920e810dcbed 100644 (file)
--- a/src/bin/auth/statistics.h
+++ b/src/bin/auth/statistics.h
@@ -286,7 +286,7 @@ public:
     /// \param msgattrs DNS message attributes.
     /// \param response DNS response message.
     /// \param done DNS response was sent to the client.
-    /// \throw None
+    /// \throw isc::Unexpected Internal condition check failed.
     void inc(const MessageAttributes& msgattrs,
              const isc::dns::Message& response, const bool done);
 

diff --git a/src/bin/auth/tests/auth_srv_unittest.cc b/src/bin/auth/tests/auth_srv_unittest.cc
index 70f98fb03d39c368c15aebf627374b676d882b38..aede681e8163b5549b4c4e13dce0ff3e9cc62797 100644 (file)
--- a/src/bin/auth/tests/auth_srv_unittest.cc
+++ b/src/bin/auth/tests/auth_srv_unittest.cc
@@ -309,13 +309,31 @@ TEST_F(AuthSrvTest, response) {
 // Query with a broken question
 TEST_F(AuthSrvTest, shortQuestion) {
     shortQuestion();
-    checkAllRcodeCountersZeroExcept(Rcode::FORMERR(), 1);
+    ConstElementPtr stats_after = server.getStatistics()->get("zones")->
+        get("_SERVER_");
+    std::map<std::string, int> expect;
+    expect["request.v4"] = 1;
+    expect["request.udp"] = 1;
+    expect["opcode.query"] = 1;
+    expect["responses"] = 1;
+    expect["rcode.formerr"] = 1;
+    expect["qrynoauthans"] = 1;
+    checkStatisticsCounters(stats_after, expect);
 }
 
 // Query with a broken answer section
 TEST_F(AuthSrvTest, shortAnswer) {
     shortAnswer();
-    checkAllRcodeCountersZeroExcept(Rcode::FORMERR(), 1);
+    ConstElementPtr stats_after = server.getStatistics()->get("zones")->
+        get("_SERVER_");
+    std::map<std::string, int> expect;
+    expect["request.v4"] = 1;
+    expect["request.udp"] = 1;
+    expect["opcode.query"] = 1;
+    expect["responses"] = 1;
+    expect["rcode.formerr"] = 1;
+    expect["qrynoauthans"] = 1;
+    checkStatisticsCounters(stats_after, expect);
 }
 
 // Query with unsupported version of EDNS.
@@ -328,8 +346,10 @@ TEST_F(AuthSrvTest, ednsBadVers) {
     expect["request.v4"] = 1;
     expect["request.badednsver"] = 1;
     expect["request.udp"] = 1;
+    expect["opcode.query"] = 1;
     expect["responses"] = 1;
     expect["rcode.badvers"] = 1;
+    expect["qrynoauthans"] = 1;
     checkStatisticsCounters(stats_after, expect);
 }