Commit a6a4cbf0 authored by Marcin Siodelski's avatar Marcin Siodelski
Browse files

[3747] Addressed review comments.

parent 61a24e4f
......@@ -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<int>(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<int>(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:
......
......@@ -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:
......
......@@ -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.
......
......@@ -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");
......
// 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 <asiolink/io_address.h>
#include <dhcp/duid.h>
#include <dhcpsrv/lease.h>
#include <util/pointer_util.h>
#include <gtest/gtest.h>
#include <vector>
#include <sstream>
......@@ -96,8 +97,8 @@ TEST_F(Lease4Test, constructor) {
EXPECT_EQ(ADDRESS[i], static_cast<uint32_t>(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
......
......@@ -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<typename T>
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<typename T>
bool nullOrEqualValues(const T& ptr1, const T& ptr2) {
inline bool nullOrEqualValues(const T& ptr1, const T& ptr2) {
return ((!ptr1 && !ptr2) || equalValues(ptr1, ptr2));
}
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment