[1613] fix rcode counting, and add additional tests for that

src/bin/auth/auth.spec.pre.in

@@ -344,7 +344,7 @@
         "item_optional": true,
         "item_default": 0,
         "item_title": "Sent 'no such rrset' response",
-        "item_description": "The number of total responses with rcode 8 ()"
+        "item_description": "The number of total responses with rcode 8 (NXRRSET)"
       },
       {
         "item_name": "rcode.notauth",

src/bin/auth/auth_srv.cc

@@ -409,13 +409,13 @@ AuthSrv::processMessage(const IOMessage& io_message, MessagePtr message,
         // Ignore all responses.
         if (message->getHeaderFlag(Message::HEADERFLAG_QR)) {
             LOG_DEBUG(auth_logger, DBG_AUTH_DETAIL, AUTH_RESPONSE_RECEIVED);
-            server->resume(false);
+            resumeServer(server, message, false);
             return;
         }
     } catch (const Exception& ex) {
         LOG_DEBUG(auth_logger, DBG_AUTH_DETAIL, AUTH_HEADER_PARSE_FAIL)
                   .arg(ex.what());
-        server->resume(false);
+        resumeServer(server, message, false);
         return;
     }
 
@@ -426,13 +426,13 @@ AuthSrv::processMessage(const IOMessage& io_message, MessagePtr message,
         LOG_DEBUG(auth_logger, DBG_AUTH_DETAIL, AUTH_PACKET_PROTOCOL_ERROR)
                   .arg(error.getRcode().toText()).arg(error.what());
         makeErrorMessage(message, buffer, error.getRcode());
-        server->resume(true);
+        resumeServer(server, message, true);
         return;
     } catch (const Exception& ex) {
         LOG_DEBUG(auth_logger, DBG_AUTH_DETAIL, AUTH_PACKET_PARSE_ERROR)
                   .arg(ex.what());
         makeErrorMessage(message, buffer, Rcode::SERVFAIL());
-        server->resume(true);
+        resumeServer(server, message, true);
         return;
     } // other exceptions will be handled at a higher layer.
 
@@ -459,7 +459,7 @@ AuthSrv::processMessage(const IOMessage& io_message, MessagePtr message,
 
     if (tsig_error != TSIGError::NOERROR()) {
         makeErrorMessage(message, buffer, tsig_error.toRcode(), tsig_context);
-        server->resume(true);
+        resumeServer(server, message, true);
         return;
     }
 
@@ -492,10 +492,7 @@ AuthSrv::processMessage(const IOMessage& io_message, MessagePtr message,
         }
     }
 
-    // update per rcode statistics counter.
-    impl_->counters_.inc(message->getRcode());
-
-    server->resume(send_answer);
+    resumeServer(server, message, send_answer);
 }
 
 bool
@@ -786,6 +783,11 @@ AuthSrv::getCounter(const Opcode opcode) const {
     return (impl_->counters_.getCounter(opcode));
 }
 
+uint64_t
+AuthSrv::getCounter(const Rcode rcode) const {
+    return (impl_->counters_.getCounter(rcode));
+}
+
 const AddressList&
 AuthSrv::getListenAddresses() const {
     return (impl_->listen_addresses_);
@@ -805,3 +807,11 @@ void
 AuthSrv::setTSIGKeyRing(const boost::shared_ptr<TSIGKeyRing>* keyring) {
     impl_->keyring_ = keyring;
 }
+
+void
+AuthSrv::resumeServer(DNSServer* server, MessagePtr message, bool done) {
+    if (done) {
+        impl_->counters_.inc(message->getRcode());
+    }
+    server->resume(done);
+}

src/bin/auth/auth_srv.h

@@ -361,6 +361,20 @@ public:
     /// \return the value of the counter.
     uint64_t getCounter(const isc::dns::Opcode opcode) const;
 
+    /// \brief Get the value of per Rcode counter in the Auth Counters.
+    ///
+    /// This function calls AuthCounters::getCounter(isc::dns::Rcode) and
+    /// returns its return value.
+    ///
+    /// \note This is a tentative interface as an attempt of experimentally
+    /// supporting more statistics counters.  This should eventually be more
+    /// generalized.  In any case, this method is mainly for testing.
+    ///
+    /// \throw None
+    /// \param rcode The rcode of the counter to get the value of
+    /// \return the value of the counter.
+    uint64_t getCounter(const isc::dns::Rcode rcode) const;
+
     /**
      * \brief Set and get the addresses we listen on.
      */
@@ -382,6 +396,22 @@ public:
                         keyring);
 
 private:
+    /// \brief Resume the server
+    ///
+    /// This is a wrapper call for DNSServer::resume(done), if 'done' is true,
+    /// the Rcode set in the given Message is counted in the statistics
+    /// counter.
+    ///
+    /// This method is expected to be called by processMessage()
+    ///
+    /// \param server The DNSServer as passed to processMessage()
+    /// \param message The response as constructed by processMessage()
+    /// \param done If true, the Rcode from the given message is counted,
+    ///             this value is then passed to server->resume(bool)
+    void resumeServer(isc::asiodns::DNSServer* server,
+                      isc::dns::MessagePtr message,
+                      bool done);
+
     AuthSrvImpl* impl_;
     isc::asiolink::SimpleCallback* checkin_;
     isc::asiodns::DNSLookup* dns_lookup_;

src/bin/auth/tests/auth_srv_unittest.cc

@@ -84,6 +84,32 @@ protected:
         server.processMessage(*io_message, parse_message, response_obuffer,
                               &dnsserv);
     }
+
+    // Helper for checking Rcode statistic counters
+    void checkRcodeCounter(const Rcode& rcode, int expected_value) {
+        EXPECT_EQ(expected_value, server.getCounter(rcode)) <<
+                  "Expected Rcode count for " << rcode.toText() <<
+                  " " << expected_value << ", was: " <<
+                  server.getCounter(rcode);
+    }
+    // Checks whether all Rcode counters are set to zero
+    void checkAllRcodeCountersZero() {
+        for (int i = 0; i < 17; i++) {
+            checkRcodeCounter(Rcode(i), 0);
+        }
+    }
+    // Checks whether all Rcode counters are set to zero except the given
+    // rcode (it is checked to be set to 'value')
+    void checkAllRcodeCountersZeroExcept(const Rcode& rcode, int value) {
+        for (int i = 0; i < 17; i++) {
+            const Rcode rc(i);
+            if (rc == rcode) {
+                checkRcodeCounter(Rcode(i), value);
+            } else {
+                checkRcodeCounter(Rcode(i), 0);
+            }
+        }
+    }
     IOService ios_;
     DNSService dnss_;
     MockSession statistics_session;
@@ -145,6 +171,7 @@ TEST_F(AuthSrvTest, builtInQuery) {
                         response_obuffer->getData(),
                         response_obuffer->getLength(),
                         &response_data[0], response_data.size());
+    checkAllRcodeCountersZeroExcept(Rcode::NOERROR(), 1);
 }
 
 // Same test emulating the UDPServer class behavior (defined in libasiolink).
@@ -195,38 +222,46 @@ TEST_F(AuthSrvTest, iqueryViaDNSServer) {
 // Unsupported requests.  Should result in NOTIMP.
 TEST_F(AuthSrvTest, unsupportedRequest) {
     unsupportedRequest();
+    // unsupportedRequest tries 14 different opcodes
+    checkAllRcodeCountersZeroExcept(Rcode::NOTIMP(), 14);
 }
 
 // Multiple questions.  Should result in FORMERR.
 TEST_F(AuthSrvTest, multiQuestion) {
     multiQuestion();
+    checkAllRcodeCountersZeroExcept(Rcode::FORMERR(), 1);
 }
 
 // Incoming data doesn't even contain the complete header.  Must be silently
 // dropped.
 TEST_F(AuthSrvTest, shortMessage) {
     shortMessage();
+    checkAllRcodeCountersZero();
 }
 
 // Response messages.  Must be silently dropped, whether it's a valid response
 // or malformed or could otherwise cause a protocol error.
 TEST_F(AuthSrvTest, response) {
     response();
+    checkAllRcodeCountersZero();
 }
 
 // Query with a broken question
 TEST_F(AuthSrvTest, shortQuestion) {
     shortQuestion();
+    checkAllRcodeCountersZeroExcept(Rcode::FORMERR(), 1);
 }
 
 // Query with a broken answer section
 TEST_F(AuthSrvTest, shortAnswer) {
     shortAnswer();
+    checkAllRcodeCountersZeroExcept(Rcode::FORMERR(), 1);
 }
 
 // Query with unsupported version of EDNS.
 TEST_F(AuthSrvTest, ednsBadVers) {
     ednsBadVers();
+    checkAllRcodeCountersZeroExcept(Rcode::BADVERS(), 1);
 }
 
 TEST_F(AuthSrvTest, AXFROverUDP) {
@@ -244,6 +279,7 @@ TEST_F(AuthSrvTest, AXFRSuccess) {
     server.processMessage(*io_message, parse_message, response_obuffer, &dnsserv);
     EXPECT_FALSE(dnsserv.hasAnswer());
     EXPECT_TRUE(xfrout.isConnected());
+    checkAllRcodeCountersZero();
 }
 
 // Try giving the server a TSIG signed request and see it can anwer signed as
@@ -279,6 +315,8 @@ TEST_F(AuthSrvTest, TSIGSigned) {
                                    response_obuffer->getLength()));
     EXPECT_EQ(TSIGError::NOERROR(), error) <<
         "The server signed the response, but it doesn't seem to be valid";
+
+    checkAllRcodeCountersZeroExcept(Rcode::NOERROR(), 1);
 }
 
 // Give the server a signed request, but don't give it the key. It will
@@ -311,6 +349,8 @@ TEST_F(AuthSrvTest, TSIGSignedBadKey) {
     EXPECT_EQ(TSIGError::BAD_KEY_CODE, tsig->getRdata().getError());
     EXPECT_EQ(0, tsig->getRdata().getMACSize()) <<
         "It should be unsigned with this error";
+
+    checkAllRcodeCountersZeroExcept(Rcode::NOTAUTH(), 1);
 }
 
 // Give the server a signed request, but signed by a different key
@@ -344,6 +384,8 @@ TEST_F(AuthSrvTest, TSIGBadSig) {
     EXPECT_EQ(TSIGError::BAD_SIG_CODE, tsig->getRdata().getError());
     EXPECT_EQ(0, tsig->getRdata().getMACSize()) <<
         "It should be unsigned with this error";
+
+    checkAllRcodeCountersZeroExcept(Rcode::NOTAUTH(), 1);
 }
 
 // Give the server a signed unsupported request with a bad signature.
@@ -383,6 +425,8 @@ TEST_F(AuthSrvTest, TSIGCheckFirst) {
     // TSIG should have failed, and so the per opcode counter shouldn't be
     // incremented.
     EXPECT_EQ(0, server.getCounter(Opcode::RESERVED14()));
+
+    checkAllRcodeCountersZeroExcept(Rcode::NOTAUTH(), 1);
 }
 
 TEST_F(AuthSrvTest, AXFRConnectFail) {
@@ -531,6 +575,8 @@ TEST_F(AuthSrvTest, notify) {
     EXPECT_EQ(Name("example.com"), question->getName());
     EXPECT_EQ(RRClass::IN(), question->getClass());
     EXPECT_EQ(RRType::SOA(), question->getType());
+
+    checkAllRcodeCountersZeroExcept(Rcode::NOERROR(), 1);
 }
 
 TEST_F(AuthSrvTest, notifyForCHClass) {
@@ -799,6 +845,10 @@ TEST_F(AuthSrvTest, queryCounterUDPNormal) {
                           &dnsserv);
     // After processing UDP query, the counter should be 1.
     EXPECT_EQ(1, server.getCounter(AuthCounters::SERVER_UDP_QUERY));
+    // The counter for SUCCESS responses should also be one
+    EXPECT_EQ(1, server.getCounter(Opcode::QUERY()));
+    // The counter for REFUSED responses should also be one, the rest zero
+    checkAllRcodeCountersZeroExcept(Rcode::REFUSED(), 1);
 }
 
 // Submit TCP normal query and check query counter
@@ -814,6 +864,10 @@ TEST_F(AuthSrvTest, queryCounterTCPNormal) {
                           &dnsserv);
     // After processing TCP query, the counter should be 1.
     EXPECT_EQ(1, server.getCounter(AuthCounters::SERVER_TCP_QUERY));
+    // The counter for SUCCESS responses should also be one
+    EXPECT_EQ(1, server.getCounter(Opcode::QUERY()));
+    // The counter for REFUSED responses should also be one, the rest zero
+    checkAllRcodeCountersZeroExcept(Rcode::REFUSED(), 1);
 }
 
 // Submit TCP AXFR query and check query counter
@@ -829,6 +883,8 @@ TEST_F(AuthSrvTest, queryCounterTCPAXFR) {
     EXPECT_FALSE(dnsserv.hasAnswer());
     // After processing TCP AXFR query, the counter should be 1.
     EXPECT_EQ(1, server.getCounter(AuthCounters::SERVER_TCP_QUERY));
+    // No rcodes should be incremented
+    checkAllRcodeCountersZero();
 }
 
 // Submit TCP IXFR query and check query counter