[2157] fix opcode handling and added some documentation

src/bin/auth/auth_srv.cc

@@ -526,6 +526,8 @@ AuthSrv::processMessage(const IOMessage& io_message, Message& message,
     }
 
     const Opcode& opcode = message.getOpcode();
+    // Get opcode at this point; for all requests regardless of message body
+    // sanity check.
     stats_attrs.setRequestOpCode(opcode);
 
     try {

src/bin/auth/b10-auth.xml.pre

@@ -196,6 +196,25 @@
 
 <!-- ### STATISTICS DATA PLACEHOLDER ### -->
 
+    <note>
+      <para>
+        Opcode of a request message will not be counted if:
+        <itemizedlist>
+          <listitem><para>
+            The request message is too short to parse the message header
+          </para></listitem>
+          <listitem><para>
+            The request message is a response (i.e. QR bit is set)
+          </para></listitem>
+        </itemizedlist>
+      </para>
+  
+      <para>
+        Request attributes except for opcode will not be counted if signature
+        validation failed as they are not reliable.
+      </para>
+    </note>
+
   </refsect1>
 
   <refsect1>

src/bin/auth/statistics.cc.pre

@@ -131,35 +131,37 @@ Counters::incRequest(const MessageAttributes& msgattrs) {
         server_msg_counter_.inc(MSG_REQUEST_TCP);
     }
 
-    // request TSIG
+    // Opcode
+    const boost::optional<isc::dns::Opcode>& opcode =
+        msgattrs.getRequestOpCode();
+    // Increment opcode counter only if the opcode exists; opcode can be empty
+    // if a short message which does not contain DNS header is received, or
+    // a response message (i.e. QR bit is set) is received.
+    if (opcode) {
+        server_msg_counter_.inc(opcode_to_msgcounter[opcode.get().getCode()]);
+    }
+
+    // TSIG
     if (msgattrs.requestHasTSIG()) {
         server_msg_counter_.inc(MSG_REQUEST_TSIG);
     }
     if (msgattrs.requestHasBadSig()) {
         server_msg_counter_.inc(MSG_REQUEST_BADSIG);
-        // If signature validation is failed, no other query attributes are
-        // reliable. Skip processing of the rest of query counters.
+        // If signature validation failed, no other request attributes (except
+        // for opcode) are reliable. Skip processing of the rest of request
+        // counters.
         return;
     }
 
-    // request EDNS
+    // EDNS0
     if (msgattrs.requestHasEDNS0()) {
         server_msg_counter_.inc(MSG_REQUEST_EDNS0);
     }
 
-    // request DNSSEC
+    // DNSSEC OK bit
     if (msgattrs.requestHasDO()) {
         server_msg_counter_.inc(MSG_REQUEST_DNSSEC_OK);
     }
-
-    // OPCODE
-    const boost::optional<isc::dns::Opcode>& opcode =
-        msgattrs.getRequestOpCode();
-    // 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()]);
-    }
 }
 
 void

src/bin/auth/tests/auth_srv_unittest.cc

@@ -419,6 +419,7 @@ TEST_F(AuthSrvTest, TSIGSignedBadKey) {
     expect["request.tsig"] = 1;
     expect["request.badsig"] = 1;
     expect["request.udp"] = 1;
+    expect["opcode.query"] = 1;
     expect["responses"] = 1;
     expect["response.tsig"] = 1;
     expect["rcode.notauth"] = 1;
@@ -464,6 +465,7 @@ TEST_F(AuthSrvTest, TSIGBadSig) {
     expect["request.tsig"] = 1;
     expect["request.badsig"] = 1;
     expect["request.udp"] = 1;
+    expect["opcode.query"] = 1;
     expect["responses"] = 1;
     expect["response.tsig"] = 1;
     expect["rcode.notauth"] = 1;
@@ -512,6 +514,7 @@ TEST_F(AuthSrvTest, TSIGCheckFirst) {
     expect["request.tsig"] = 1;
     expect["request.badsig"] = 1;
     expect["request.udp"] = 1;
+    expect["opcode.other"] = 1;
     expect["responses"] = 1;
     expect["response.tsig"] = 1;
     expect["rcode.notauth"] = 1;

src/bin/auth/tests/statistics_unittest.cc.pre

@@ -344,7 +344,7 @@ TEST_F(CountersTest, incrementTSIG) {
         expect.clear();
         expect["request.v4"] = i+1;
         expect["request.udp"] = i+1;
-        expect["opcode.query"] = i+1 - count_badsig;
+        expect["opcode.query"] = i+1;
         expect["request.edns0"] = i+1 - count_badsig;
         expect["request.badednsver"] = 0;
         expect["request.dnssec_ok"] = i+1 - count_badsig;