[3981] Compilation fixes, unit-tests now passing.

src/bin/dhcp4/dhcp4_messages.mes

@@ -177,6 +177,17 @@ This message is printed when DHCPv4 server disables an interface from being
 used to receive DHCPv4 traffic. Sockets on this interface will not be opened
 by the Interface Manager until interface is enabled.
 
+% DHCP4_DECLINE_LEASE Address %1 was declined by client %2. The lease will be unavailable for %3 seconds.
+This informational message is printed when a client received an address, but
+discovered that it is used by some other device and notified the server by
+sending DHCPDECLINE message. The server checked that this address really was
+leased to a given client and marked this address as unusable for a certain
+amount of time. This message may indicate a misconfiguration in a network,
+as there is either a buggy client or more likely a device that is using an
+address that it is not supposed to. The server will fully recover from this
+situation, but if the underlying problem of misconfigured or rogue device
+is not solved, this address may be declined again in the future.
+
 % DHCP4_DECLINE_LEASE_NOT_FOUND Received DHCPDECLINE for addr %1, but no such lease found. Client: %2
 This warning message indicates that a client reported that his address was
 detected as a duplicate (i.e. other device in the network is using this address).

src/bin/dhcp4/dhcp4_srv.cc

@@ -1844,14 +1844,15 @@ void
 Dhcpv4Srv::processDecline(Pkt4Ptr& decline) {
 
     // Server-id is mandatory in DHCPDECLINE (see table 5, RFC2131)
-    sanityCheck(decline, MANDATORY);
+    /// @todo Uncomment this (see ticket #3116)
+    // sanityCheck(decline, MANDATORY);
 
     // Client is supposed to specify the address being declined in
     // Requested IP address option, but must not set its ciaddr.
     // (again, see table 5 in RFC2131).
 
     OptionCustomPtr opt_requested_address = boost::dynamic_pointer_cast<
-        OptionCustom>(query->getOption(DHO_DHCP_REQUESTED_ADDRESS));
+        OptionCustom>(decline->getOption(DHO_DHCP_REQUESTED_ADDRESS));
     if (!opt_requested_address) {
 
         isc_throw(RFCViolation, "Mandatory 'Requested IP address' option missing"
@@ -1864,7 +1865,7 @@ Dhcpv4Srv::processDecline(Pkt4Ptr& decline) {
 
     // Now we need to check whether this address really belongs to the client
     // that attempts to decline it.
-    Lease4Ptr lease = LeaseMgrFactory::instance().getLease4(addr);
+    const Lease4Ptr lease = LeaseMgrFactory::instance().getLease4(addr);
 
     if (!lease) {
         // Client tried to decline an address, but we don't have a lease for
@@ -1875,39 +1876,42 @@ Dhcpv4Srv::processDecline(Pkt4Ptr& decline) {
         // opportunity for that to be useful is small and the attack vector
         // would be pretty severe.
         LOG_WARN(dhcp4_logger, DHCP4_DECLINE_LEASE_NOT_FOUND)
-            .arg(addr->toText()).arg(decline->getLabel());
+            .arg(addr.toText()).arg(decline->getLabel());
         return;
     }
 
     // Get client-id, if available.
-    OptionPtr opt_clientid = query->getOption(DHO_DHCP_CLIENT_IDENTIFIER);
+    OptionPtr opt_clientid = decline->getOption(DHO_DHCP_CLIENT_IDENTIFIER);
     ClientIdPtr client_id;
     if (opt_clientid) {
         client_id.reset(new ClientId(opt_clientid->getData()));
     }
 
     // Check if the client attempted to decline a lease it doesn't own.
-    if (!lease->belongsToClient(ctx.hwaddr_, ctx.clientid_)) {
+    if (!lease->belongsToClient(decline->getHWAddr(), client_id)) {
 
         // Get printable hardware addresses
         string client_hw = decline->getHWAddr() ?
             decline->getHWAddr()->toText(false) : "(none)";
         string lease_hw = lease->hwaddr_ ?
-            lease->hwaddr_->getHWAddr()->toText(false) : "(none)";
+            lease->hwaddr_->toText(false) : "(none)";
 
         // Get printable client-ids
-        string client_id = client_id ? client_id->toText() : "(none)";
-        string lease_id = lease->client_id ? lease->client_id_->toText() : "(none)";
+        string client_id_txt = client_id ? client_id->toText() : "(none)";
+        string lease_id_txt = lease->client_id_ ?
+            lease->client_id_->toText() : "(none)";
 
+        // Print the warning and we're done here.
         LOG_WARN(dhcp4_logger, DHCP4_DECLINE_LEASE_MISMATCH)
-            .arg(addr->toText()).arg(decline->getLabel())
-            .arg(client_hw).arg(lease_hw).arg(client_id).arg(lease_id);
+            .arg(addr.toText()).arg(decline->getLabel())
+            .arg(client_hw).arg(lease_hw).arg(client_id_txt).arg(lease_id_txt);
+
         return;
     }
 
     // Ok, all is good. The client is reporting its own address. Let's
     // process it.
-    declineLease(lease);
+    declineLease(lease, decline->getLabel());
 }
 
 void
@@ -1922,30 +1926,30 @@ Dhcpv4Srv::declineLease(const Lease4Ptr& lease, const std::string& descr) {
     }
 
     // Bump up the statistics.
-    isc::stats::StatsMgr::instance().addValue("pkt4-parse-failed",
-                                              static_cast<int64_t>(1));
+    std::stringstream name;
+    name << "subnet[" << lease->subnet_id_ << "].declined-addresses";
+    isc::stats::StatsMgr::instance().addValue(name.str(), static_cast<int64_t>(1));
 
     // @todo: Call hooks.
 
     // We need to disassociate the lease from the client. Once we move a lease
     // to declined state, it is no longer associated with the client in any
     // way.
-    lease->hwaddr_.resize(0);
-    lease->hwaddr_.client_id_.reset();
+    lease->hwaddr_.reset(new HWAddr());
+    lease->client_id_.reset();
     lease->t1_ = 0;
     lease->t2_ = 0;
-    lease->valid_lft_ = CfgMgr::instance().getCurrentCfg()->getDelinedPeriod();
-    lease->cltt_ = now();
+    lease->valid_lft_ = CfgMgr::instance().getCurrentCfg()->getDeclinePeriod();
+    lease->cltt_ = time(NULL);
     lease->hostname_ = string("");
     lease->fqdn_fwd_ = false;
     lease->fqdn_rev_ = false;
 
-    lease->state = DECLINED;
-    LeaseMgrFactory::instance().updateLease4();
+    lease->state_ = Lease::STATE_DECLINED;
+    LeaseMgrFactory::instance().updateLease4(lease);
 
-    LOG_INFO(dhcp4_logger, DHCP4_DECLINE_ADDR)
-        .arg(lease->addr_->toText()).arg(descr)
-        .arg(lease->valid_lft_);
+    LOG_INFO(dhcp4_logger, DHCP4_DECLINE_LEASE).arg(lease->addr_.toText())
+        .arg(descr).arg(lease->valid_lft_);
 }
 
 Pkt4Ptr

src/bin/dhcp4/dhcp4_srv.h

@@ -535,6 +535,20 @@ private:
     /// server's response.
     void processHostnameOption(Dhcpv4Exchange& ex);
 
+    /// @brief Marks lease as declined.
+    ///
+    /// This method moves a lease to declined state with all the steps involved:
+    /// - trigger DNS removal (if necessary)
+    /// - disassociate the client information
+    /// - update lease in the database (switch to DECLINED state)
+    /// - increase necessary statistics
+    /// - call appropriate hook (@todo)
+    ///
+    /// @param lease lease to be declined
+    /// @param descr textual description of the client (will be used for logging)
+    void
+    declineLease(const Lease4Ptr& lease, const std::string& descr);
+
 protected:
 
     /// @brief Creates NameChangeRequests which correspond to the lease

src/bin/dhcp4/tests/decline_unittest.cc

@@ -38,7 +38,7 @@ namespace {
 /// @brief Set of JSON configurations used throughout the Decline tests.
 ///
 /// - Configuration 0:
-///   - Used for testing Release message processing
+///   - Used for testing Decline message processing
 ///   - 1 subnet: 10.0.0.0/24
 ///   - 1 pool: 10.0.0.10-10.0.0.100
 ///   - Router option present: 10.0.0.200 and 10.0.0.201
@@ -63,18 +63,16 @@ const char* DECLINE_CONFIGS[] = {
     "}"
 };
 
-/// @brief Test fixture class for testing 4-way (DORA) exchanges.
+/// @brief Test fixture class for testing DHCPDECLINE message handling.
 ///
-/// @todo Currently there is a limit number of test cases covered here.
-/// In the future it is planned that the tests from the
-/// dhcp4_srv_unittest.cc will be migrated here and will use the
-/// @c Dhcp4Client class.
+/// @todo This class is very similar to ReleaseTest. Maybe we could
+/// merge those two classes one day and use derived classes?
 class DeclineTest : public Dhcpv4SrvTest {
 public:
 
     enum ExpectedResult {
-        SHOULD_PASS,
-        SHOULD_FAIL
+        SHOULD_PASS, // pass = accept decline, move lease to declined state.
+        SHOULD_FAIL  // fail = reject the decline
     };
 
     /// @brief Constructor.
@@ -88,6 +86,8 @@ public:
 
     /// @brief Performs 4-way exchange to obtain new lease.
     ///
+    /// This is used as a preparatory step for Decline operation.
+    ///
     /// @param client Client to be used to obtain a lease.
     void acquireLease(Dhcp4Client& client);
 
@@ -98,8 +98,8 @@ public:
     /// @param hw_address_2 HW Address to be used to decline the lease.
     /// @param client_id_2 Client id to be used to decline the lease.
     /// @param expected_result SHOULD_PASS if the lease is expected to
-    /// be successfully released, or SHOULD_FAIL if the lease is expected
-    /// to not be released.
+    /// be successfully declined, or SHOULD_FAIL if the lease is expected
+    /// to not be declined.
     void acquireAndDecline(const std::string& hw_address_1,
                            const std::string& client_id_1,
                            const std::string& hw_address_2,
@@ -137,10 +137,22 @@ DeclineTest::acquireAndDecline(const std::string& hw_address_1,
                                const std::string& hw_address_2,
                                const std::string& client_id_2,
                                ExpectedResult expected_result) {
+
+    // Let's get the subnet-id and generate statistics name out of it.
+    const Subnet4Collection* subnets =
+        CfgMgr::instance().getCurrentCfg()->getCfgSubnets4()->getAll();
+    ASSERT_EQ(1, subnets->size());
+    std::stringstream name;
+    name << "subnet[" << subnets->at(0)->getID() << "].declined-addresses";
+
+    // Set this statistic explicitly to zero.
+    isc::stats::StatsMgr::instance().setValue(name.str(), static_cast<int64_t>(0));
+
+    // Ok, do the normal lease aquisition.
     CfgMgr::instance().clear();
     Dhcp4Client client(Dhcp4Client::SELECTING);
     // Configure DHCP server.
-    configure(RELEASE_CONFIGS[0], *client.getServer());
+    configure(DECLINE_CONFIGS[0], *client.getServer());
     // Explicitly set the client id.
     client.includeClientId(client_id_1);
     // Explicitly set the HW address.
@@ -148,14 +160,7 @@ DeclineTest::acquireAndDecline(const std::string& hw_address_1,
     // Perform 4-way exchange to obtain a new lease.
     acquireLease(client);
 
-    std::stringstream name;
-
-    // Let's get the subnet-id and generate statistics name out of it
-    const Subnet4Collection* subnets =
-        CfgMgr::instance().getCurrentCfg()->getCfgSubnets4()->getAll();
-    ASSERT_EQ(1, subnets->size());
-    name << "subnet[" << subnets->at(0)->getID() << "].declined-addresses";
-
+    // Check the delined-addresses statistic before the Decline operation.
     ObservationPtr declined_cnt = StatsMgr::instance().getObservation(name.str());
     ASSERT_TRUE(declined_cnt);
     uint64_t before = declined_cnt->getInteger().first;
@@ -163,12 +168,12 @@ DeclineTest::acquireAndDecline(const std::string& hw_address_1,
     // Remember the acquired address.
     IOAddress declined_address = client.config_.lease_.addr_;
 
-    // Explicitly set the client id for DHCPRELEASE.
+    // Explicitly set the client id for DHCPDECLINE.
     client.includeClientId(client_id_2);
-    // Explicitly set the HW address for DHCPRELEASE.
+    // Explicitly set the HW address for DHCPDECLINE.
     client.setHWAddress(hw_address_2);
 
-    // Send the release and make sure that the lease is removed from the
+    // Send the decline and make sure that the lease is removed from the
     // server.
     ASSERT_NO_THROW(client.doDecline());
     Lease4Ptr lease = LeaseMgrFactory::instance().getLease4(declined_address);
@@ -181,14 +186,14 @@ DeclineTest::acquireAndDecline(const std::string& hw_address_1,
     // We check if the deline process was successful by checking if the
     // lease is in the database and what is its state.
     if (expected_result == SHOULD_PASS) {
-        EXPECT_EQ(DECLINED, lease->state);
+        EXPECT_EQ(Lease::STATE_DECLINED, lease->state_);
 
         // The decline succeded, so the declined-addresses statistic should
         // be increased by one
         EXPECT_EQ(after, before + 1);
     } else {
         // the decline was supposed, to be rejected.
-        EXPECT_EQ(DEFAULT, lease->state);
+        EXPECT_EQ(Lease::STATE_DEFAULT, lease->state_);
 
         // The decline failed, so the declined-addresses should be the same
         // as before
@@ -196,42 +201,42 @@ DeclineTest::acquireAndDecline(const std::string& hw_address_1,
     }
 }
 
-// This test checks that the client can acquire and release the lease.
-TEST_F(DeclineTest, releaseNoIdentifierChange) {
+// This test checks that the client can acquire and decline the lease.
+TEST_F(DeclineTest, declineNoIdentifierChange) {
     acquireAndDecline("01:02:03:04:05:06", "12:14",
                       "01:02:03:04:05:06", "12:14",
                       SHOULD_PASS);
 }
 
-// This test verifies the release correctness in the following case:
+// This test verifies the decline correctness in the following case:
 // - Client acquires new lease using HW address only
-// - Client sends the DHCPRELEASE with valid HW address and without
+// - Client sends the DHCPDECLINE with valid HW address and without
 //   client identifier.
-// - The server successfully releases the lease.
-TEST_F(DeclineTest, releaseHWAddressOnly) {
+// - The server successfully declines the lease.
+TEST_F(DeclineTest, declineHWAddressOnly) {
     acquireAndDecline("01:02:03:04:05:06", "",
                       "01:02:03:04:05:06", "",
                       SHOULD_PASS);
 }
 
-// This test verifies the release correctness in the following case:
+// This test verifies the decline correctness in the following case:
 // - Client acquires new lease using the client identifier and HW address
-// - Client sends the DHCPRELEASE with valid HW address but with no
+// - Client sends the DHCPDECLINE with valid HW address but with no
 //   client identifier.
-// - The server successfully releases the lease.
-TEST_F(DeclineTest, releaseNoClientId) {
+// - The server successfully declines the lease.
+TEST_F(DeclineTest, declineNoClientId) {
     acquireAndDecline("01:02:03:04:05:06", "12:14",
                       "01:02:03:04:05:06", "",
                       SHOULD_PASS);
 }
 
-// This test verifies the release correctness in the following case:
+// This test verifies the decline correctness in the following case:
 // - Client acquires new lease using HW address
-// - Client sends the DHCPRELEASE with valid HW address and some
+// - Client sends the DHCPDECLINE with valid HW address and some
 //   client identifier.
-// - The server identifies the lease using HW address and releases
+// - The server identifies the lease using HW address and declines
 //   this lease.
-TEST_F(DeclineTest, releaseNoClientId2) {
+TEST_F(DeclineTest, declineNoClientId2) {
     acquireAndDecline("01:02:03:04:05:06", "",
                       "01:02:03:04:05:06", "12:14",
                       SHOULD_PASS);
@@ -239,10 +244,10 @@ TEST_F(DeclineTest, releaseNoClientId2) {
 
 // This test checks the server's behavior in the following case:
 // - Client acquires new lease using the client identifier and HW address
-// - Client sends the DHCPRELEASE with the valid HW address but with invalid
+// - Client sends the DHCPDECLINE with the valid HW address but with invalid
 //   client identifier.
 // - The server should not remove the lease.
-TEST_F(DeclineTest, releaseNonMatchingClientId) {
+TEST_F(DeclineTest, declineNonMatchingClientId) {
     acquireAndDecline("01:02:03:04:05:06", "12:14",
                       "01:02:03:04:05:06", "12:15:16",
                       SHOULD_FAIL);
@@ -250,11 +255,11 @@ TEST_F(DeclineTest, releaseNonMatchingClientId) {
 
 // This test checks the server's behavior in the following case:
 // - Client acquires new lease using client identifier and HW address
-// - Client sends the DHCPRELEASE with the same client identifier but
+// - Client sends the DHCPDECLINE with the same client identifier but
 //   different HW address.
 // - The server uses client identifier to find the client's lease and
-//   releases it.
-TEST_F(DeclineTest, releaseNonMatchingHWAddress) {
+//   declines it.
+TEST_F(DeclineTest, declineNonMatchingHWAddress) {
     acquireAndDecline("01:02:03:04:05:06", "12:14",
                       "06:06:06:06:06:06", "12:14",
                       SHOULD_PASS);
@@ -262,30 +267,30 @@ TEST_F(DeclineTest, releaseNonMatchingHWAddress) {
 
 // This test checks the server's behavior in the following case:
 // - Client acquires new lease.
-// - Client sends DHCPRELEASE with the ciaddr set to a different
+// - Client sends DHCPDECLINE with the ciaddr set to a different
 //   address than it has acquired from the server.
-// - Server determines that the client is trying to release a
-//   wrong address and will refuse to release.
-TEST_F(DeclineTest, releaseNonMatchingIPAddress) {
+// - Server determines that the client is trying to decline a
+//   wrong address and will refuse to decline.
+TEST_F(DeclineTest, declineNonMatchingIPAddress) {
     Dhcp4Client client(Dhcp4Client::SELECTING);
     // Configure DHCP server.
-    configure(RELEASE_CONFIGS[0], *client.getServer());
+    configure(DECLINE_CONFIGS[0], *client.getServer());
     // Perform 4-way exchange to obtain a new lease.
     acquireLease(client);
 
     // Remember the acquired address.
     IOAddress leased_address = client.config_.lease_.addr_;
 
-    // Modify the client's address to force it to release a different address
+    // Modify the client's address to force it to decline a different address
     // than it has obtained from the server.
     client.config_.lease_.addr_ = IOAddress(static_cast<uint32_t>(leased_address) + 1);
 
-    // Send DHCPRELEASE and make sure it was unsuccessful, i.e. the lease
+    // Send DHCPDECLINE and make sure it was unsuccessful, i.e. the lease
     // remains in the database.
     ASSERT_NO_THROW(client.doDecline());
     Lease4Ptr lease = LeaseMgrFactory::instance().getLease4(leased_address);
     ASSERT_TRUE(lease);
-    EXPECT_EQ(DEFAULT, lease->state_);
+    EXPECT_EQ(Lease::STATE_DEFAULT, lease->state_);
 }
 
 } // end of anonymous namespace

src/bin/dhcp4/tests/dhcp4_client.cc

@@ -102,6 +102,13 @@ Dhcp4Client::appendClientId() {
 }
 
 void
+Dhcp4Client::appendServerId() {
+    OptionPtr opt(new Option4AddrLst(DHO_DHCP_SERVER_IDENTIFIER,
+                                     config_.serverid_));
+    context_.query_->addOption(opt);
+}
+
+void
 Dhcp4Client::appendName() {
     if (!context_.query_) {
         isc_throw(Dhcp4ClientError, "pointer to the query must not be NULL"
@@ -286,6 +293,7 @@ Dhcp4Client::doDecline() {
         isc_throw(Dhcp4ClientError, "failed to send the decline"
                   " message because client doesn't have a lease");
     }
+
     context_.query_ = createMsg(DHCPDECLINE);
 
     // Set ciaddr to 0.
@@ -297,6 +305,9 @@ Dhcp4Client::doDecline() {
     // Include client identifier.
     appendClientId();
 
+    // Incluer server identifier.
+    appendServerId();
+
     // Remove configuration.
     config_.reset();
 

src/bin/dhcp4/tests/dhcp4_client.h

@@ -390,6 +390,12 @@ private:
     /// option in the client's message to the server.
     void appendClientId();
 
+    /// @brief Includes the Server Identifier option in the client's message.
+    ///
+    /// This function creates an instance of the Server Identifier option.
+    /// It uses whatever information is stored in config_.serverid_.
+    void appendServerId();
+
     /// @brief Includes FQDN or Hostname option in the client's message.
     ///
     /// This method checks if @c fqdn_ or @c hostname_ is specified and

src/bin/dhcp4/tests/dhcp4_srv_unittest.cc

@@ -450,14 +450,6 @@ TEST_F(Dhcpv4SrvTest, processRelease) {
     EXPECT_NO_THROW(srv.processRelease(pkt));
 }
 
-TEST_F(Dhcpv4SrvTest, processDecline) {
-    NakedDhcpv4Srv srv;
-    Pkt4Ptr pkt(new Pkt4(DHCPDECLINE, 1234));
-
-    // Should not throw
-    EXPECT_NO_THROW(srv.processDecline(pkt));
-}
-
 // This test verifies that incoming DISCOVER can be handled properly, that an
 // OFFER is generated, that the response has an address and that address
 // really belongs to the configured pool.