From: Marcin Siodelski Date: Tue, 19 May 2015 08:46:01 +0000 (+0200) Subject: [3747] Addressed review comments. X-Git-Tag: trac3867_base~2^2 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=a6a4cbf0b471f78b8f87466ef513bd820b45cc3f;p=thirdparty%2Fkea.git [3747] Addressed review comments. --- diff --git a/src/bin/dhcp4/tests/dora_unittest.cc b/src/bin/dhcp4/tests/dora_unittest.cc index d8d10a4eaf..ca7c2f7d2a 100644 --- a/src/bin/dhcp4/tests/dora_unittest.cc +++ b/src/bin/dhcp4/tests/dora_unittest.cc @@ -726,6 +726,7 @@ TEST_F(DORATest, ignoreChangingClientId) { Pkt4Ptr resp = client.getContext().response_; // Make sure that the server has responded with DHCPACK. ASSERT_EQ(DHCPACK, static_cast(resp->getType())); + EXPECT_FALSE(client.config_.lease_.client_id_); // Remember address which the client has obtained. IOAddress leased_address = client.config_.lease_.addr_; @@ -745,6 +746,8 @@ TEST_F(DORATest, ignoreChangingClientId) { // Make sure that the server assigned the same address, even though the // client id has changed. EXPECT_EQ(leased_address, client.config_.lease_.addr_); + // Check that the client id is not present in the lease. + EXPECT_FALSE(client.config_.lease_.client_id_); } // This test checks that the match-client-id parameter doesn't have @@ -762,6 +765,8 @@ TEST_F(DORATest, changingHWAddress) { Pkt4Ptr resp = client.getContext().response_; // Make sure that the server has responded with DHCPACK. ASSERT_EQ(DHCPACK, static_cast(resp->getType())); + // Check that the client id is not present in the lease. + EXPECT_FALSE(client.config_.lease_.client_id_); // Remember address which the client has obtained. IOAddress leased_address = client.config_.lease_.addr_; @@ -781,6 +786,8 @@ TEST_F(DORATest, changingHWAddress) { // Client must assign different address because the client id is // ignored and the HW address was changed. EXPECT_NE(client.config_.lease_.addr_, leased_address); + // Check that the client id is not present in the lease. + EXPECT_FALSE(client.config_.lease_.client_id_); } // This test checks the following scenario: diff --git a/src/bin/dhcp4/tests/release_unittest.cc b/src/bin/dhcp4/tests/release_unittest.cc index 7547929f1e..f0e904b98f 100644 --- a/src/bin/dhcp4/tests/release_unittest.cc +++ b/src/bin/dhcp4/tests/release_unittest.cc @@ -68,6 +68,11 @@ const char* RELEASE_CONFIGS[] = { class ReleaseTest : public Dhcpv4SrvTest { public: + enum ExpectedResult { + SHOULD_PASS, + SHOULD_FAIL + }; + /// @brief Constructor. /// /// Sets up fake interfaces. @@ -82,27 +87,20 @@ public: /// @param client Client to be used to obtain a lease. void acquireLease(Dhcp4Client& client); - /// @brief Test successful release for different identifiers. + /// @brief Tests if the acquired lease is or is not released. /// /// @param hw_address_1 HW Address to be used to acquire the lease. /// @param client_id_1 Client id to be used to acquire the lease. /// @param hw_address_2 HW Address to be used to release the lease. /// @param client_id_2 Client id to be used to release the lease. - void successfulRelease(const std::string& hw_address_1, + /// @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. + void acquireAndRelease(const std::string& hw_address_1, const std::string& client_id_1, const std::string& hw_address_2, - const std::string& client_id_2); - - /// @brief Test unsuccessful release for different identifiers. - /// - /// @param hw_address_1 HW Address to be used to acquire the lease. - /// @param client_id_1 Client id to be used to acquire the lease. - /// @param hw_address_2 HW Address to be used to release the lease. - /// @param client_id_2 Client id to be used to release the lease. - void unsuccessfulRelease(const std::string& hw_address_1, - const std::string& client_id_1, - const std::string& hw_address_2, - const std::string& client_id_2); + const std::string& client_id_2, + ExpectedResult expected_result); /// @brief Interface Manager's fake configuration control. IfaceMgrTestConfig iface_mgr_test_config_; @@ -130,10 +128,11 @@ ReleaseTest::acquireLease(Dhcp4Client& client) { } void -ReleaseTest::successfulRelease(const std::string& hw_address_1, +ReleaseTest::acquireAndRelease(const std::string& hw_address_1, const std::string& client_id_1, const std::string& hw_address_2, - const std::string& client_id_2) { + const std::string& client_id_2, + ExpectedResult expected_result) { CfgMgr::instance().clear(); Dhcp4Client client(Dhcp4Client::SELECTING); // Configure DHCP server. @@ -157,44 +156,23 @@ ReleaseTest::successfulRelease(const std::string& hw_address_1, // server. ASSERT_NO_THROW(client.doRelease()); Lease4Ptr lease = LeaseMgrFactory::instance().getLease4(leased_address); - ASSERT_FALSE(lease); -} -void -ReleaseTest::unsuccessfulRelease(const std::string& hw_address_1, - const std::string& client_id_1, - const std::string& hw_address_2, - const std::string& client_id_2) { - Dhcp4Client client(Dhcp4Client::SELECTING); - // Configure DHCP server. - configure(RELEASE_CONFIGS[0], *client.getServer()); - // Explicitly set the client id. - client.includeClientId(client_id_1); - // Explicitly set the HW address. - client.setHWAddress(hw_address_1); - // Perform 4-way exchange to obtain a new lease. - acquireLease(client); - - // Remember the acquired address. - IOAddress leased_address = client.config_.lease_.addr_; - - // Explicitly set the client id for DHCPRELEASE. - client.includeClientId(client_id_2); - // Explicitly set the HW address for DHCPRELEASE. - client.setHWAddress(hw_address_2); + // We check if the release process was successful by checking if the + // lease is in the database. It is expected that it is not present, + // i.e. has been deleted with the release. + if (expected_result == SHOULD_PASS) { + EXPECT_FALSE(lease); - // Send DHCPRELEASE and make sure it was unsuccessful, i.e. the lease - // remains in the database. - ASSERT_NO_THROW(client.doRelease()); - Lease4Ptr lease = LeaseMgrFactory::instance().getLease4(leased_address); - ASSERT_TRUE(lease); + } else { + EXPECT_TRUE(lease); + } } - // This test checks that the client can acquire and release the lease. TEST_F(ReleaseTest, releaseNoIdentifierChange) { - successfulRelease("01:02:03:04:05:06", "12:14", - "01:02:03:04:05:06", "12:14"); + acquireAndRelease("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: @@ -203,8 +181,9 @@ TEST_F(ReleaseTest, releaseNoIdentifierChange) { // client identifier. // - The server successfully releases the lease. TEST_F(ReleaseTest, releaseHWAddressOnly) { - successfulRelease("01:02:03:04:05:06", "", - "01:02:03:04:05:06", ""); + acquireAndRelease("01:02:03:04:05:06", "", + "01:02:03:04:05:06", "", + SHOULD_PASS); } // This test verifies the release correctness in the following case: @@ -213,8 +192,9 @@ TEST_F(ReleaseTest, releaseHWAddressOnly) { // client identifier. // - The server successfully releases the lease. TEST_F(ReleaseTest, releaseNoClientId) { - successfulRelease("01:02:03:04:05:06", "12:14", - "01:02:03:04:05:06", ""); + acquireAndRelease("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: @@ -224,8 +204,9 @@ TEST_F(ReleaseTest, releaseNoClientId) { // - The server identifies the lease using HW address and releases // this lease. TEST_F(ReleaseTest, releaseNoClientId2) { - successfulRelease("01:02:03:04:05:06", "", - "01:02:03:04:05:06", "12:14"); + acquireAndRelease("01:02:03:04:05:06", "", + "01:02:03:04:05:06", "12:14", + SHOULD_PASS); } // This test checks the server's behavior in the following case: @@ -234,8 +215,9 @@ TEST_F(ReleaseTest, releaseNoClientId2) { // client identifier. // - The server should not remove the lease. TEST_F(ReleaseTest, releaseNonMatchingClientId) { - unsuccessfulRelease("01:02:03:04:05:06", "12:14", - "01:02:03:04:05:06", "12:15:16"); + acquireAndRelease("01:02:03:04:05:06", "12:14", + "01:02:03:04:05:06", "12:15:16", + SHOULD_FAIL); } // This test checks the server's behavior in the following case: @@ -245,8 +227,9 @@ TEST_F(ReleaseTest, releaseNonMatchingClientId) { // - The server uses client identifier to find the client's lease and // releases it. TEST_F(ReleaseTest, releaseNonMatchingHWAddress) { - successfulRelease("01:02:03:04:05:06", "12:14", - "06:06:06:06:06:06", "12:14"); + acquireAndRelease("01:02:03:04:05:06", "12:14", + "06:06:06:06:06:06", "12:14", + SHOULD_PASS); } // This test checks the server's behavior in the following case: diff --git a/src/lib/dhcpsrv/lease.h b/src/lib/dhcpsrv/lease.h index bbff3386fc..a4faeee3c8 100644 --- a/src/lib/dhcpsrv/lease.h +++ b/src/lib/dhcpsrv/lease.h @@ -296,28 +296,29 @@ struct Lease4 : public Lease { /// cases: /// - client didn't generate client identifier and is only using the chaddr /// field to identify itself. - /// - server's administrator configured the server to ignore client identifier, - /// the client obtained the new lease, and the administrator reconfigured - /// the server to NOT ignore the client identifier. The client is trying - /// to renew its lease and both the client identifier and HW address is - /// used for matching the lease which doesn't have the record of the - /// client identifier. + /// - server's administrator configured the server to NOT match client + /// identifiers, the client obtained the new lease, and the administrator + /// reconfigured the server to match the client identifiers. The client + /// is trying to renew its lease and both the client identifier and HW + /// address is used for matching the lease which doesn't have the record + /// of the client identifier. /// - client obtained the lease using the HW address and client identifier, - /// the server's administrator configured the server to ignore the client - /// identifier, and the client returns to renew the lease. This time, the - /// lease has a record of both client identifier and the HW address but - /// only the HW address is used for matching the client to the lease. + /// the server's administrator configured the server to NOT match the + /// client identifiers, and the client returns to renew the lease. This + /// time, the lease has a record of both client identifier and the HW + /// address but only the HW address is used for matching the client to + /// the lease. /// /// Note that the typical case when the server's administrator may want to - /// force ignoring client identifier passed in the client's message is when - /// the client is performing multi-stage boot. In such case, the client - /// identifiers may change on various stages of the boot, but the HW address - /// will remain stable. The server's administrator prefers to use the - /// HW address for client identification in this case. - /// - /// It may also be useful to ignore client identifiers to mitigate the - /// problem of broken client implementations which generate new client - /// identifiers every time they connect to the network. + /// disable matching the client identifier passed in the client's message + /// is when the client is performing multi-stage boot. In such case, the + /// client identifiers may change on various stages of the boot, but the + /// HW address will remain stable. The server's administrator prefers + /// using the HW address for client identification in this case. + /// + /// It may also be useful to disable matching client identifiers to + /// mitigate the problem of broken client implementations which generate + /// new client identifiers every time they connect to the network. /// /// @param hw_address Pointer to the HW address of the client. /// @param client_id Pointer to the client identifier structure. diff --git a/src/lib/dhcpsrv/tests/alloc_engine4_unittest.cc b/src/lib/dhcpsrv/tests/alloc_engine4_unittest.cc index 4c36408624..01b678cca7 100644 --- a/src/lib/dhcpsrv/tests/alloc_engine4_unittest.cc +++ b/src/lib/dhcpsrv/tests/alloc_engine4_unittest.cc @@ -554,6 +554,7 @@ TEST_F(AllocEngine4Test, identifyClientLease) { EXPECT_EQ("192.0.2.101", identified_lease->addr_.toText()); ctx.hwaddr_ = hwaddr2_; + ctx.clientid_ = clientid_; identified_lease = engine.allocateLease4(ctx); ASSERT_TRUE(identified_lease); EXPECT_EQ("192.0.2.101", identified_lease->addr_.toText()); @@ -564,12 +565,14 @@ TEST_F(AllocEngine4Test, identifyClientLease) { ASSERT_TRUE(identified_lease); EXPECT_NE(identified_lease->addr_.toText(), "192.0.2.101"); + ctx.hwaddr_ = hwaddr_; ctx.clientid_.reset(); identified_lease = engine.allocateLease4(ctx); ASSERT_TRUE(identified_lease); EXPECT_EQ("192.0.2.101", identified_lease->addr_.toText()); ctx.hwaddr_ = hwaddr2_; + ctx.clientid_.reset(); identified_lease = engine.allocateLease4(ctx); ASSERT_TRUE(identified_lease); EXPECT_NE(identified_lease->addr_.toText(), "192.0.2.101"); @@ -583,13 +586,14 @@ TEST_F(AllocEngine4Test, identifyClientLease) { ASSERT_TRUE(identified_lease); EXPECT_EQ("192.0.2.101", identified_lease->addr_.toText()); + ctx.hwaddr_ = hwaddr_; ctx.clientid_.reset(); identified_lease = engine.allocateLease4(ctx); ASSERT_TRUE(identified_lease); EXPECT_EQ("192.0.2.101", identified_lease->addr_.toText()); - ctx.clientid_ = clientid_; ctx.hwaddr_ = hwaddr2_; + ctx.clientid_ = clientid_; identified_lease = engine.allocateLease4(ctx); ASSERT_TRUE(identified_lease); EXPECT_NE(identified_lease->addr_.toText(), "192.0.2.101"); diff --git a/src/lib/dhcpsrv/tests/lease_unittest.cc b/src/lib/dhcpsrv/tests/lease_unittest.cc index c9c4e934cd..67eed2e700 100644 --- a/src/lib/dhcpsrv/tests/lease_unittest.cc +++ b/src/lib/dhcpsrv/tests/lease_unittest.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2013-2014 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2013-2015 Internet Systems Consortium, Inc. ("ISC") // // Permission to use, copy, modify, and/or distribute this software for any // purpose with or without fee is hereby granted, provided that the above @@ -16,6 +16,7 @@ #include #include #include +#include #include #include #include @@ -96,8 +97,8 @@ TEST_F(Lease4Test, constructor) { EXPECT_EQ(ADDRESS[i], static_cast(lease.addr_)); EXPECT_EQ(0, lease.ext_); - EXPECT_TRUE(hwaddr_ == lease.hwaddr_); - EXPECT_TRUE(*clientid_ == *lease.client_id_); + EXPECT_TRUE(util::equalValues(hwaddr_, lease.hwaddr_)); + EXPECT_TRUE(util::equalValues(clientid_, lease.client_id_)); EXPECT_EQ(0, lease.t1_); EXPECT_EQ(0, lease.t2_); EXPECT_EQ(VALID_LIFETIME, lease.valid_lft_); @@ -188,90 +189,62 @@ TEST_F(Lease4Test, operatorAssign) { // belongs to the particular client identified by the client identifier // and hw address. TEST_F(Lease4Test, leaseBelongsToClient) { + // Client identifier that matches the one in the lease. + ClientIdPtr matching_client_id = ClientId::fromText("01:02:03:04"); + // Client identifier that doesn't match the one in the lease. + ClientIdPtr diff_client_id = ClientId::fromText("01:02:03:05"); + // Null (no) client identifier. + ClientIdPtr null_client_id; + + // HW Address that matches the one in the lease. + HWAddrPtr matching_hw(new HWAddr(HWAddr::fromText("00:01:02:03:04:05", + HTYPE_ETHER))); + // HW Address that doesn't match the one in the lease. + HWAddrPtr diff_hw(new HWAddr(HWAddr::fromText("00:01:02:03:04:06", + HTYPE_ETHER))); + // Null HW Address. + HWAddrPtr null_hw; + // Create the lease with MAC address and Client Identifier. - Lease4 lease(IOAddress("192.0.2.1"), - HWAddrPtr(new HWAddr(HWAddr::fromText("00:01:02:03:04:05", HTYPE_ETHER))), - ClientId::fromText("01:02:03:04"), + Lease4 lease(IOAddress("192.0.2.1"), matching_hw, matching_client_id, 60, time(NULL), 0, 0, 1); - // Create HW address and Client Id objects which match those held by the - // lease. This is a full match, so the lease belongs to the client. - HWAddrPtr hwaddr(new HWAddr(HWAddr::fromText("00:01:02:03:04:05", HTYPE_ETHER))); - ClientIdPtr clientid = ClientId::fromText("01:02:03:04"); - EXPECT_TRUE(lease.belongsToClient(hwaddr, clientid)); - - // Modify the HW address. The lease should still belong to the client - // because the client identifier matches. - hwaddr.reset(new HWAddr(HWAddr::fromText("00:01:02:03:04:06", HTYPE_ETHER))); - EXPECT_TRUE(lease.belongsToClient(hwaddr, clientid)); - - // Use the correct HW address, but modify the client identifier. The client - // identifier must match if present. If it doesn't match, the lease doesn't - // belong to the client. - hwaddr.reset(new HWAddr(HWAddr::fromText("00:01:02:03:04:05", HTYPE_ETHER))); - clientid = ClientId::fromText("01:02:03:05"); - EXPECT_FALSE(lease.belongsToClient(hwaddr, clientid)); - - // Set client id to null and leave only HW address. It coveres the case when - // the client id is ignored by the server (e.g. multi-stage boot case), but - // the client has a lease already. - clientid.reset(); - EXPECT_TRUE(lease.belongsToClient(hwaddr, clientid)); - - // Now use the correct client id and the null HW address. The client id - // should be sufficient to match the lease. - clientid = ClientId::fromText("01:02:03:04"); - hwaddr.reset(); - EXPECT_TRUE(lease.belongsToClient(hwaddr, clientid)); - - // Use both HW address and client id set to null. There must be no match. - hwaddr.reset(); - clientid.reset(); - EXPECT_FALSE(lease.belongsToClient(hwaddr, clientid)); - - // This time both HW address and client identifier are different than - // those held by the lease. There should be no match. - hwaddr.reset(new HWAddr(HWAddr::fromText("00:01:02:03:04:06", HTYPE_ETHER))); - clientid = ClientId::fromText("01:02:03:05"); - EXPECT_FALSE(lease.belongsToClient(hwaddr, clientid)); - - // Use the correct HW address and client id to match the lease, but set the - // client id for the lease to null. This covers the case when the lease - // has been acquired without client identifier but then the client is - // renewing using some client identifier (or server is configured to - // not ignore the client identifier). The client should obtain the lease. - hwaddr.reset(new HWAddr(HWAddr::fromText("00:01:02:03:04:05", HTYPE_ETHER))); - lease.client_id_.reset(); - EXPECT_TRUE(lease.belongsToClient(hwaddr, clientid)); - - // Change HW address. This time, the HW address doesn't match and client id - // can't be compared so it is considered as no match. - hwaddr.reset(new HWAddr(HWAddr::fromText("00:01:02:03:04:06", HTYPE_ETHER))); - EXPECT_FALSE(lease.belongsToClient(hwaddr, clientid)); - - // The lease now holds the client id by the HW address is null for the lease. - // Also, we're using correct client id to match the lease and some HW address. - // There should be a match because client id is ok. - lease.client_id_ = ClientId::fromText("01:02:03:04"); - lease.hwaddr_.reset(); - clientid = ClientId::fromText("01:02:03:04"); - hwaddr.reset(new HWAddr(HWAddr::fromText("00:01:02:03:04:05", HTYPE_ETHER))); - EXPECT_TRUE(lease.belongsToClient(hwaddr, clientid)); - - // If client id is wrong, there should be no match. - clientid = ClientId::fromText("01:02:03:05"); - EXPECT_FALSE(lease.belongsToClient(hwaddr, clientid)); - - // Setting HW address to null shouldn't matter and we should still have a - // match if the client id is correct. - clientid = ClientId::fromText("01:02:03:04"); - hwaddr.reset(); - EXPECT_TRUE(lease.belongsToClient(hwaddr, clientid)); - - // But with the null HW address and non-matching client id there should be - // no match whatsoever. - clientid = ClientId::fromText("01:02:03:06"); - EXPECT_FALSE(lease.belongsToClient(hwaddr, clientid)); + // Verify cases for lease that has both hw address and client identifier. + EXPECT_TRUE(lease.belongsToClient(matching_hw, matching_client_id)); + EXPECT_FALSE(lease.belongsToClient(matching_hw, diff_client_id)); + EXPECT_TRUE(lease.belongsToClient(matching_hw, null_client_id)); + EXPECT_TRUE(lease.belongsToClient(diff_hw, matching_client_id)); + EXPECT_FALSE(lease.belongsToClient(diff_hw, diff_client_id)); + EXPECT_FALSE(lease.belongsToClient(diff_hw, null_client_id)); + EXPECT_TRUE(lease.belongsToClient(null_hw, matching_client_id)); + EXPECT_FALSE(lease.belongsToClient(null_hw, diff_client_id)); + EXPECT_FALSE(lease.belongsToClient(null_hw, null_client_id)); + + + // Verify cases for lease that has only HW address. + lease.client_id_ = null_client_id; + EXPECT_TRUE(lease.belongsToClient(matching_hw, matching_client_id)); + EXPECT_TRUE(lease.belongsToClient(matching_hw, diff_client_id)); + EXPECT_TRUE(lease.belongsToClient(matching_hw, null_client_id)); + EXPECT_FALSE(lease.belongsToClient(diff_hw, matching_client_id)); + EXPECT_FALSE(lease.belongsToClient(diff_hw, diff_client_id)); + EXPECT_FALSE(lease.belongsToClient(diff_hw, null_client_id)); + EXPECT_FALSE(lease.belongsToClient(null_hw, matching_client_id)); + EXPECT_FALSE(lease.belongsToClient(null_hw, diff_client_id)); + EXPECT_FALSE(lease.belongsToClient(null_hw, null_client_id)); + + // Verify cases for lease that has only client identifier. + lease.client_id_ = matching_client_id; + lease.hwaddr_ = null_hw; + EXPECT_TRUE(lease.belongsToClient(matching_hw, matching_client_id)); + EXPECT_FALSE(lease.belongsToClient(matching_hw, diff_client_id)); + EXPECT_FALSE(lease.belongsToClient(matching_hw, null_client_id)); + EXPECT_TRUE(lease.belongsToClient(diff_hw, matching_client_id)); + EXPECT_FALSE(lease.belongsToClient(diff_hw, diff_client_id)); + EXPECT_FALSE(lease.belongsToClient(diff_hw, null_client_id)); + EXPECT_TRUE(lease.belongsToClient(null_hw, matching_client_id)); + EXPECT_FALSE(lease.belongsToClient(null_hw, diff_client_id)); + EXPECT_FALSE(lease.belongsToClient(null_hw, null_client_id)); } /// @brief Lease4 Equality Test diff --git a/src/lib/util/pointer_util.h b/src/lib/util/pointer_util.h index 527aea791e..886b929e82 100644 --- a/src/lib/util/pointer_util.h +++ b/src/lib/util/pointer_util.h @@ -32,7 +32,7 @@ namespace util { /// @return true only if both pointers are non-null and the values which they /// point to are equal. template -bool equalValues(const T& ptr1, const T& ptr2) { +inline bool equalValues(const T& ptr1, const T& ptr2) { return (ptr1 && ptr2 && (*ptr1 == *ptr2)); } @@ -47,7 +47,7 @@ bool equalValues(const T& ptr1, const T& ptr2) { /// @return true if both pointers are null or if they are both non-null /// and the values pointed to are equal. template -bool nullOrEqualValues(const T& ptr1, const T& ptr2) { +inline bool nullOrEqualValues(const T& ptr1, const T& ptr2) { return ((!ptr1 && !ptr2) || equalValues(ptr1, ptr2)); }