Commit 5e5f9047 authored by Marcin Siodelski's avatar Marcin Siodelski

[3269] Address review comments.

parent 434cc524
...@@ -2326,9 +2326,10 @@ Dhcpv6Srv::processConfirm(const Pkt6Ptr& confirm) { ...@@ -2326,9 +2326,10 @@ Dhcpv6Srv::processConfirm(const Pkt6Ptr& confirm) {
// Make sure that the necessary options are included. // Make sure that the necessary options are included.
copyDefaultOptions(confirm, reply); copyDefaultOptions(confirm, reply);
appendDefaultOptions(confirm, reply); appendDefaultOptions(confirm, reply);
// Number of addresses verified. If at the end it occurs that no addresses // Indicates if at least one address has been verified. If no addresses
// were verified we will need to dicard the message. // are verified it means that the client has sent no IA_NA options
int verified = 0; // or no IAAddr options and that client's message has to be discarded.
bool verified = false;
// Check if subnet can be selected for the message. If no subnet // Check if subnet can be selected for the message. If no subnet
// has been selected, the client is not on link. // has been selected, the client is not on link.
SubnetPtr subnet = selectSubnet(confirm); SubnetPtr subnet = selectSubnet(confirm);
...@@ -2342,28 +2343,29 @@ Dhcpv6Srv::processConfirm(const Pkt6Ptr& confirm) { ...@@ -2342,28 +2343,29 @@ Dhcpv6Srv::processConfirm(const Pkt6Ptr& confirm) {
for (OptionCollection::const_iterator opt = opts.begin(); for (OptionCollection::const_iterator opt = opts.begin();
opt != opts.end(); ++opt) { opt != opts.end(); ++opt) {
// Ignore options other than IAAddr. // Ignore options other than IAAddr.
if (opt->second->getType() != D6O_IAADDR) { if (opt->second->getType() == D6O_IAADDR) {
continue; // Check that the address is in range in the subnet selected.
} Option6IAAddrPtr iaaddr = boost::dynamic_pointer_cast<
// Check that the address is in range in the subnet selected. Option6IAAddr>(opt->second);
Option6IAAddrPtr iaaddr = boost::dynamic_pointer_cast< // If there is subnet selected and the address has been included
Option6IAAddr>(opt->second); // in IA_NA, mark it verified and verify that it belongs to the
// If there is subnet selected and the address has been included // subnet.
// in IA_NA, mark it verified and verify that it belongs to the if (iaaddr) {
// subnet. verified = true;
if (iaaddr && subnet) { // If at least one address is not in range, then return
// There is an address so we increase the counter of // the NotOnLink status code.
// addresses verified. if (subnet && !subnet->inRange(iaaddr->getAddress())) {
++verified; std::ostringstream status_msg;
// If at least one address is not in range, then return
// the NotOnLink status code.
if (!subnet->inRange(iaaddr->getAddress())) {
std::ostringstream status_msg;
status_msg << "Address " << iaaddr->getAddress() status_msg << "Address " << iaaddr->getAddress()
<< " is not on link."; << " is not on link.";
reply->addOption(createStatusCode(STATUS_NotOnLink, reply->addOption(createStatusCode(STATUS_NotOnLink,
status_msg.str())); status_msg.str()));
return (reply); return (reply);
}
} else {
isc_throw(Unexpected, "failed to cast the IA Address option"
" to the Option6IAAddrPtr. This is programmatic"
" error and should be reported");
} }
} }
} }
...@@ -2371,7 +2373,7 @@ Dhcpv6Srv::processConfirm(const Pkt6Ptr& confirm) { ...@@ -2371,7 +2373,7 @@ Dhcpv6Srv::processConfirm(const Pkt6Ptr& confirm) {
// It seems that the client hasn't included any addresses in which case // It seems that the client hasn't included any addresses in which case
// the Confirm must be discarded. // the Confirm must be discarded.
if (verified == 0) { if (!verified) {
return (Pkt6Ptr()); return (Pkt6Ptr());
} }
......
...@@ -32,11 +32,11 @@ namespace { ...@@ -32,11 +32,11 @@ namespace {
/// ///
/// - Configuration 0: /// - Configuration 0:
/// - only addresses (no prefixes) /// - only addresses (no prefixes)
/// - 2 subnets with 2001:db8:1::/64 and 2001:db8:2::64 /// - 2 subnets with 2001:db8:1::/64 and 2001:db8:2::/64
/// - 1 subnet for eth0 and 1 subnet for eth1 /// - 1 subnet for eth0 and 1 subnet for eth1
/// ///
/// - Configuration 1: /// - Configuration 1:
/// - similar to Configuration 0 and Configuration 1 /// - similar to Configuration 0
/// - pools configured: 3000:1::/64 and 3000:2::/64 /// - pools configured: 3000:1::/64 and 3000:2::/64
/// - this specific configuration is used by tests using relays /// - this specific configuration is used by tests using relays
/// ///
...@@ -97,7 +97,7 @@ public: ...@@ -97,7 +97,7 @@ public:
// Test that directly connected client's Confirm message is processed and Reply // Test that directly connected client's Confirm message is processed and Reply
// message is sent back. In this test case, the client sends Confirm for two // message is sent back. In this test case, the client sends Confirm for two
// addresses that belong to the same IAID and are sent within the same IA_NA // addresses that belong to the same IAID and are sent within the same IA_NA
// option. // option (RFC3315, section 18.2.2).
TEST_F(ConfirmTest, directClientSameIAID) { TEST_F(ConfirmTest, directClientSameIAID) {
Dhcp6Client client; Dhcp6Client client;
// Configure client to request IA_NA. // Configure client to request IA_NA.
...@@ -139,7 +139,7 @@ TEST_F(ConfirmTest, directClientSameIAID) { ...@@ -139,7 +139,7 @@ TEST_F(ConfirmTest, directClientSameIAID) {
// Test that directly connected client's Confirm message is processed and Reply // Test that directly connected client's Confirm message is processed and Reply
// message is sent back. In this test case, the client sends Confirm for two // message is sent back. In this test case, the client sends Confirm for two
// addresses that belong to different IAIDs and are sent within the different // addresses that belong to different IAIDs and are sent within the different
// IA_NA options. // IA_NA options (RFC3315, section 18.2.2).
TEST_F(ConfirmTest, directClientDifferentIAID) { TEST_F(ConfirmTest, directClientDifferentIAID) {
Dhcp6Client client; Dhcp6Client client;
// Configure client to request IA_NA. // Configure client to request IA_NA.
...@@ -185,7 +185,7 @@ TEST_F(ConfirmTest, directClientDifferentIAID) { ...@@ -185,7 +185,7 @@ TEST_F(ConfirmTest, directClientDifferentIAID) {
// Test that relayed client's Confirm message is processed and Reply message // Test that relayed client's Confirm message is processed and Reply message
// is sent back. // is sent back (RFC3315, section 18.2.2).
TEST_F(ConfirmTest, relayedClient) { TEST_F(ConfirmTest, relayedClient) {
Dhcp6Client client; Dhcp6Client client;
// Client to send relayed message. // Client to send relayed message.
...@@ -226,7 +226,8 @@ TEST_F(ConfirmTest, relayedClient) { ...@@ -226,7 +226,8 @@ TEST_F(ConfirmTest, relayedClient) {
EXPECT_TRUE(client.getContext().response_->getOption(D6O_CLIENTID)); EXPECT_TRUE(client.getContext().response_->getOption(D6O_CLIENTID));
} }
// Test that the Confirm message without any addresses is discarded. // Test that the Confirm message without any addresses is discarded
// (RFC3315, section 18.2.2).
TEST_F(ConfirmTest, relayedClientNoAddress) { TEST_F(ConfirmTest, relayedClientNoAddress) {
Dhcp6Client client; Dhcp6Client client;
// Configure the server. // Configure the server.
...@@ -242,8 +243,48 @@ TEST_F(ConfirmTest, relayedClientNoAddress) { ...@@ -242,8 +243,48 @@ TEST_F(ConfirmTest, relayedClientNoAddress) {
EXPECT_FALSE(client.getContext().response_); EXPECT_FALSE(client.getContext().response_);
} }
// This test checks that the server processes Confirm message correctly if
// the subnet can't be selected for the client (RFC3315, section 18.2.2).
TEST_F(ConfirmTest, relayedClientNoSubnet) {
Dhcp6Client client;
// Client to send relayed message.
client.useRelay();
// Configure client to request IA_NA.
client.useNA();
// Make 4-way exchange to get the lease.
ASSERT_NO_FATAL_FAILURE(requestLease(CONFIRM_CONFIGS[1], 2, client));
// Now that the client has a lease, let's remove any subnets to check
// how the server would respond to the Confirm.
ASSERT_NO_THROW(CfgMgr::instance().deleteSubnets6());
// Send Confirm message to the server.
ASSERT_NO_THROW(client.doConfirm());
// Client should have received a status code option and this option should
// indicate that the client is NotOnLink becuase subnet could not be
// selected.
ASSERT_TRUE(client.receivedStatusCode());
ASSERT_EQ(STATUS_NotOnLink, client.getStatusCode());
// Let's test another case that the client sends no addresses in the Confirm
// message. The subnet can't be selected for that client as in the previous
// case but this time the server must discard the client's message because
// it contains no addresses (is invalid).
// Set lifetimes to 0 so as the Confirm will ignore the specific address
// and send an empty IA_NA.
client.config_.leases_[0].lease_.preferred_lft_ = 0;
client.config_.leases_[0].lease_.valid_lft_ = 0;
ASSERT_NO_THROW(client.doConfirm());
EXPECT_FALSE(client.getContext().response_);
// Do similar test but this time remove the lease so as no IA_NA option
// is sent.
client.config_.clear();
ASSERT_NO_THROW(client.doConfirm());
EXPECT_FALSE(client.getContext().response_);
}
// This test checks that the relayed Confirm messsage is processed by the server // This test checks that the relayed Confirm messsage is processed by the server
// when sent to unicast address. // when sent to unicast address RFC3315, section 18.2.8).
TEST_F(ConfirmTest, relayedUnicast) { TEST_F(ConfirmTest, relayedUnicast) {
Dhcp6Client client; Dhcp6Client client;
// Client to send relayed message. // Client to send relayed message.
...@@ -256,7 +297,7 @@ TEST_F(ConfirmTest, relayedUnicast) { ...@@ -256,7 +297,7 @@ TEST_F(ConfirmTest, relayedUnicast) {
ASSERT_GT(client.getLeaseNum(), 0); ASSERT_GT(client.getLeaseNum(), 0);
client.setDestAddress(IOAddress("2001:db8:1::1")); client.setDestAddress(IOAddress("2001:db8:1::1"));
// Send Confirm message to the server. // Send Confirm message to the server.
ASSERT_NO_THROW(client.doConfirm()); ASSERT_NO_THROW (client.doConfirm());
// Client should have received a response. // Client should have received a response.
ASSERT_TRUE(client.getContext().response_); ASSERT_TRUE(client.getContext().response_);
// Client should have received a status code option and this option should // Client should have received a status code option and this option should
......
...@@ -178,7 +178,6 @@ Dhcp6Client::applyLease(const LeaseInfo& lease_info) { ...@@ -178,7 +178,6 @@ Dhcp6Client::applyLease(const LeaseInfo& lease_info) {
config_.leases_.push_back(lease_info); config_.leases_.push_back(lease_info);
} }
void void
Dhcp6Client::copyIAs(const Pkt6Ptr& source, const Pkt6Ptr& dest) { Dhcp6Client::copyIAs(const Pkt6Ptr& source, const Pkt6Ptr& dest) {
typedef OptionCollection Opts; typedef OptionCollection Opts;
...@@ -208,18 +207,21 @@ Dhcp6Client::copyIAsFromLeases(const Pkt6Ptr& dest) const { ...@@ -208,18 +207,21 @@ Dhcp6Client::copyIAsFromLeases(const Pkt6Ptr& dest) const {
opt->setT2(leases[0].t2_); opt->setT2(leases[0].t2_);
for (std::vector<Lease6>::const_iterator lease = leases.begin(); for (std::vector<Lease6>::const_iterator lease = leases.begin();
lease != leases.end(); ++lease) { lease != leases.end(); ++lease) {
if (lease->type_ == Lease::TYPE_NA) { if ((lease->preferred_lft_ != 0) && (lease->valid_lft_ != 0)) {
opt->addOption(Option6IAAddrPtr(new Option6IAAddr(D6O_IAADDR, if (lease->type_ == Lease::TYPE_NA) {
opt->addOption(Option6IAAddrPtr(new Option6IAAddr(
D6O_IAADDR,
lease->addr_, lease->addr_,
lease->preferred_lft_, lease->preferred_lft_,
lease->valid_lft_))); lease->valid_lft_)));
} else if (lease->type_ == Lease::TYPE_PD) { } else if (lease->type_ == Lease::TYPE_PD) {
opt->addOption(Option6IAAddrPtr(new Option6IAPrefix(D6O_IAPREFIX, opt->addOption(Option6IAAddrPtr(new Option6IAPrefix(
D6O_IAPREFIX,
lease->addr_, lease->addr_,
lease->prefixlen_, lease->prefixlen_,
lease->preferred_lft_, lease->preferred_lft_,
lease->valid_lft_))); lease->valid_lft_)));
}
} }
} }
dest->addOption(opt); dest->addOption(opt);
......
...@@ -412,8 +412,12 @@ private: ...@@ -412,8 +412,12 @@ private:
/// ///
/// This method iterates over existing leases that client acquired and /// This method iterates over existing leases that client acquired and
/// places corresponding IA_NA or IA_PD options into a specified message. /// places corresponding IA_NA or IA_PD options into a specified message.
/// This is useful to construct Renew or Rebind message from the existing /// This is useful to construct Renew, Rebind or Confirm message from the
/// configuration that client has obtained using 4-way exchange. /// existing configuration that client has obtained using 4-way exchange.
///
/// If there are no leases no IA options will be added. If the lease exists
/// but any of the lifetime values is set to 0, the IA option will be added
/// but the IAAddr (or IAPrefix) option will not be added.
/// ///
/// @param dest Message to which the IA options will be added. /// @param dest Message to which the IA options will be added.
void copyIAsFromLeases(const Pkt6Ptr& dest) const; void copyIAsFromLeases(const Pkt6Ptr& dest) const;
......
...@@ -70,7 +70,7 @@ Dhcpv6MessageTest::requestLease(const std::string& config, ...@@ -70,7 +70,7 @@ Dhcpv6MessageTest::requestLease(const std::string& config,
Lease6Ptr lease_server = checkLease(lease_client); Lease6Ptr lease_server = checkLease(lease_client);
ASSERT_TRUE(lease_server); ASSERT_TRUE(lease_server);
// And that status code was OK. // And that status code was OK.
EXPECT_EQ(STATUS_Success, client.getStatusCode(0)); ASSERT_EQ(STATUS_Success, client.getStatusCode(0));
} }
} }
......
...@@ -48,11 +48,14 @@ public: ...@@ -48,11 +48,14 @@ public:
/// ///
/// This function is called by @c bumpAddress and @c bumpSubnet. /// This function is called by @c bumpAddress and @c bumpSubnet.
/// ///
/// @param input_addr An input address. /// @warning This function is no-op if the byte index is out of range.
///
/// @param input_addr An input address
/// @param byte_num An index of the byte which value should be increased..
/// ///
/// @return New address. /// @return New address.
asiolink::IOAddress bumpByteInAddress(const asiolink::IOAddress& input_addr, asiolink::IOAddress bumpByteInAddress(const asiolink::IOAddress& input_addr,
const size_t byte_num); const size_t byte_num);
/// @brief Increases the first byte of the address. /// @brief Increases the first byte of the address.
/// ///
......
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