From: Marcin Siodelski Date: Wed, 10 Jun 2015 08:21:49 +0000 (+0200) Subject: [3070] Addressed review comments. X-Git-Tag: trac3732a_base~15^2~1^2 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=7add208d6b4ab18cf49579fafa4ca3d8bb41f932;p=thirdparty%2Fkea.git [3070] Addressed review comments. - Minor changes in User Guide - Reordering DHCPv6 log messages - Improved comment for fake allocation - Cleanup in the config parser test - Added new test for Rapid Commit - Default value of rapid_commit_ in subnet. --- diff --git a/doc/guide/dhcp6-srv.xml b/doc/guide/dhcp6-srv.xml index 2ba177d9c4..d0f7b1eca9 100644 --- a/doc/guide/dhcp6-srv.xml +++ b/doc/guide/dhcp6-srv.xml @@ -1329,7 +1329,7 @@ should include options from the isc option space: - This setting is solely effective for a subnet for which the + This setting only affects the subnet for which the rapid-commit is set to true. For clients connected to other subnets, the server will ignore the Rapid Commit option sent by the client and will follow the 4-way diff --git a/src/bin/dhcp6/dhcp6_messages.mes b/src/bin/dhcp6/dhcp6_messages.mes index 845007f515..d5e00a5f23 100644 --- a/src/bin/dhcp6/dhcp6_messages.mes +++ b/src/bin/dhcp6/dhcp6_messages.mes @@ -429,18 +429,18 @@ as a hint for possible requested prefix. % DHCP6_QUERY_DATA received packet length %1, data length %2, data is %3 A debug message listing the data received from the client or relay. -% DHCP6_RELEASE_MISSING_CLIENTID client (address=%1) sent RELEASE message without mandatory client-id -This warning message indicates that client sent RELEASE message without -mandatory client-id option. This is most likely caused by a buggy client -(or a relay that malformed forwarded message). This request will not be -processed and a response with error status code will be sent back. - % DHCP6_RAPID_COMMIT %1: Rapid Commit option received, following 2-way exchange This debug messgage is issued when the server found Rapid Commit option in the client's message and the 2-way exchanges are supported by the server for the subnet, which the client is connected to. The argument specifies the client and transaction identification information. +% DHCP6_RELEASE_MISSING_CLIENTID client (address=%1) sent RELEASE message without mandatory client-id +This warning message indicates that client sent RELEASE message without +mandatory client-id option. This is most likely caused by a buggy client +(or a relay that malformed forwarded message). This request will not be +processed and a response with error status code will be sent back. + % DHCP6_RELEASE_NA address %1 belonging to client duid=%2, iaid=%3 was released properly This debug message indicates that an address was released properly. It is a normal operation during client shutdown. diff --git a/src/bin/dhcp6/dhcp6_srv.cc b/src/bin/dhcp6/dhcp6_srv.cc index dbbc858816..17c5039dfa 100644 --- a/src/bin/dhcp6/dhcp6_srv.cc +++ b/src/bin/dhcp6/dhcp6_srv.cc @@ -1312,12 +1312,17 @@ Dhcpv6Srv::assignIA_NA(const Pkt6Ptr& query, const Pkt6Ptr& answer, .arg(duid ? duid->toText() : "(no-duid)").arg(ia->getIAID()) .arg(hint_opt ? hint.toText() : "(no hint)"); - // "Fake" allocation is processing of SOLICIT message. We pretend to do an - // allocation, but we do not put the lease in the database. That is ok, - // because we do not guarantee that the user will get that exact lease. If - // the user selects this server to do actual allocation (i.e. sends REQUEST) - // it should include this hint. That will help us during the actual lease - // allocation. + // "Fake" allocation is the case when the server is processing the Solicit + // message without the Rapid Commit option and advertises a lease to + // the client, but doesn't commit this lease to the lease database. If + // the Solicit contains the Rapid Commit option and the server is + // configured to honor the Rapid Commit option, or the client has sent + // the Request message, the lease will be committed to the lease + // database. The type of the server's response may be used to determine + // if this is the fake allocation case or not. When the server sends + // Reply message it means that it is committing leases. Other message + // type (Advertise) means that server is not committing leases (fake + // allocation). bool fake_allocation = (answer->getType() != DHCPV6_REPLY); // Get DDNS update direction flags @@ -1467,12 +1472,17 @@ Dhcpv6Srv::assignIA_PD(const Pkt6Ptr& query, const Pkt6Ptr& answer, .arg(duid ? duid->toText() : "(no-duid)").arg(ia->getIAID()) .arg(hint_opt ? hint.toText() : "(no hint)"); - // "Fake" allocation is processing of SOLICIT message. We pretend to do an - // allocation, but we do not put the lease in the database. That is ok, - // because we do not guarantee that the user will get that exact lease. If - // the user selects this server to do actual allocation (i.e. sends REQUEST) - // it should include this hint. That will help us during the actual lease - // allocation. + // "Fake" allocation is the case when the server is processing the Solicit + // message without the Rapid Commit option and advertises a lease to + // the client, but doesn't commit this lease to the lease database. If + // the Solicit contains the Rapid Commit option and the server is + // configured to honor the Rapid Commit option, or the client has sent + // the Request message, the lease will be committed to the lease + // database. The type of the server's response may be used to determine + // if this is the fake allocation case or not. When the server sends + // Reply message it means that it is committing leases. Other message + // type (Advertise) means that server is not committing leases (fake + // allocation). bool fake_allocation = (answer->getType() != DHCPV6_REPLY); // Use allocation engine to pick a lease for this client. Allocation engine diff --git a/src/bin/dhcp6/tests/config_parser_unittest.cc b/src/bin/dhcp6/tests/config_parser_unittest.cc index 7c3e3b1bca..8f46c4f96c 100644 --- a/src/bin/dhcp6/tests/config_parser_unittest.cc +++ b/src/bin/dhcp6/tests/config_parser_unittest.cc @@ -525,6 +525,9 @@ public: // Check the Rapid Commit flag for the subnet. EXPECT_EQ(exp_rapid_commit, subnet->getRapidCommit()); + + // Clear any existing configuration. + CfgMgr::instance().clear(); } int rcode_; ///< Return code (see @ref isc::config::parseAnswer) @@ -1138,8 +1141,8 @@ TEST_F(Dhcp6ParserTest, subnetRapidCommit) { } { - SCOPED_TRACE("Enable Rapid Commit"); // rapid-commit explicitly set to true. + SCOPED_TRACE("Enable Rapid Commit"); testRapidCommit("{ \"preferred-lifetime\": 3000," "\"rebind-timer\": 2000, " "\"renew-timer\": 1000, " @@ -1153,8 +1156,8 @@ TEST_F(Dhcp6ParserTest, subnetRapidCommit) { } { - SCOPED_TRACE("Disable Rapid Commit"); // rapid-commit explicitly set to false. + SCOPED_TRACE("Disable Rapid Commit"); testRapidCommit("{ \"preferred-lifetime\": 3000," "\"rebind-timer\": 2000, " "\"renew-timer\": 1000, " diff --git a/src/bin/dhcp6/tests/dhcp6_client.cc b/src/bin/dhcp6/tests/dhcp6_client.cc index 988d3c67f5..08f37a234f 100644 --- a/src/bin/dhcp6/tests/dhcp6_client.cc +++ b/src/bin/dhcp6/tests/dhcp6_client.cc @@ -38,13 +38,14 @@ Dhcp6Client::Dhcp6Client() : dest_addr_(ALL_DHCP_RELAY_AGENTS_AND_SERVERS), duid_(generateDUID(DUID::DUID_LLT)), link_local_("fe80::3a60:77ff:fed5:cdef"), + iface_name_("eth0"), srv_(boost::shared_ptr(new NakedDhcpv6Srv(0))), use_na_(false), use_pd_(false), use_relay_(false), use_oro_(false), use_client_id_(true), - use_rapid_commit_(true), + use_rapid_commit_(false), prefix_hint_(), fqdn_() { } @@ -55,13 +56,14 @@ Dhcp6Client::Dhcp6Client(boost::shared_ptr& srv) : dest_addr_(ALL_DHCP_RELAY_AGENTS_AND_SERVERS), duid_(generateDUID(DUID::DUID_LLT)), link_local_("fe80::3a60:77ff:fed5:cdef"), + iface_name_("eth0"), srv_(srv), use_na_(false), use_pd_(false), use_relay_(false), use_oro_(false), use_client_id_(true), - use_rapid_commit_(true), + use_rapid_commit_(false), prefix_hint_(), fqdn_() { } @@ -532,7 +534,7 @@ Dhcp6Client::sendMsg(const Pkt6Ptr& msg) { msg->getBuffer().getLength())); msg_copy->setRemoteAddr(link_local_); msg_copy->setLocalAddr(dest_addr_); - msg_copy->setIface("eth0"); + msg_copy->setIface(iface_name_); srv_->fakeReceive(msg_copy); srv_->run(); } diff --git a/src/bin/dhcp6/tests/dhcp6_client.h b/src/bin/dhcp6/tests/dhcp6_client.h index 9a77672b71..f4150ccd40 100644 --- a/src/bin/dhcp6/tests/dhcp6_client.h +++ b/src/bin/dhcp6/tests/dhcp6_client.h @@ -384,6 +384,13 @@ public: dest_addr_ = dest_addr; } + /// @brief Sets the interface to be used by the client. + /// + /// @param iface_name Interface name. + void setInterface(const std::string& iface_name) { + iface_name_ = iface_name; + } + /// @brief Sets a prefix hint to be sent to a server. /// /// @param pref_lft Preferred lifetime. @@ -575,7 +582,7 @@ private: /// @biref Current transaction id (altered on each send). uint32_t curr_transid_; - /// @brief Currently use destination address. + /// @brief Currently used destination address. asiolink::IOAddress dest_addr_; /// @brief Currently used DUID. @@ -584,6 +591,9 @@ private: /// @brief Currently used link local address. asiolink::IOAddress link_local_; + /// @brief Currently used interface. + std::string iface_name_; + /// @brief Pointer to the server that the client is communicating with. boost::shared_ptr srv_; diff --git a/src/bin/dhcp6/tests/sarr_unittest.cc b/src/bin/dhcp6/tests/sarr_unittest.cc index 93201c66fb..143e002ffb 100644 --- a/src/bin/dhcp6/tests/sarr_unittest.cc +++ b/src/bin/dhcp6/tests/sarr_unittest.cc @@ -37,17 +37,12 @@ namespace { /// match the subnet prefix /// /// - Configuration 1: -/// - one subnet 2001:db8:1::/48 used on eth0 interface -/// - one pool in a range of 2001:db8:1::1 - 2001:db8:1::10 -/// - enables Rapid Commit for the subnet and can be used for testing -/// Rapid Commit option support -/// - DNS updates enabled -/// -/// - Configuration 2: -/// - one subnet 2001:db8:1::/48 used on eth0 interface -/// - one pool in a range of 2001:db8:1::1 - 2001:db8:1::10 -/// - disables Rapid Commit for the subnet and can be used for testing -/// that server ignores Rapid Commit option from the client. +/// - two subnets 2001:db8:1::/48 and 2001:db8:2::/48 +/// - first subnet assigned to interface eth0, another one assigned to eth1 +/// - one pool for subnet in a range of 2001:db8:X::1 - 2001:db8:X::10, +/// where X is 1 or 2 +/// - enables Rapid Commit for the first subnet and disables for the second +/// one /// - DNS updates enabled /// const char* CONFIGS[] = { @@ -82,6 +77,12 @@ const char* CONFIGS[] = { " \"subnet\": \"2001:db8:1::/48\", " " \"interface\": \"eth0\"," " \"rapid-commit\": True" + " }," + " {" + " \"pools\": [ { \"pool\": \"2001:db8:2::1 - 2001:db8:2::10\" } ]," + " \"subnet\": \"2001:db8:2::/48\", " + " \"interface\": \"eth1\"," + " \"rapid-commit\": False" " } ]," "\"valid-lifetime\": 4000," " \"dhcp-ddns\" : {" @@ -101,6 +102,12 @@ const char* CONFIGS[] = { " \"subnet\": \"2001:db8:1::/48\", " " \"interface\": \"eth0\"," " \"rapid-commit\": False" + " }," + " {" + " \"pools\": [ { \"pool\": \"2001:db8:2::1 - 2001:db8:2::10\" } ]," + " \"subnet\": \"2001:db8:2::/48\", " + " \"interface\": \"eth1\"," + " \"rapid-commit\": True" " } ]," "\"valid-lifetime\": 4000," " \"dhcp-ddns\" : {" @@ -202,7 +209,7 @@ TEST_F(SARRTest, rapidCommitEnable) { // Make sure we ended-up having expected number of subnets configured. const Subnet6Collection* subnets = CfgMgr::instance().getCurrentCfg()-> getCfgSubnets6()->getAll(); - ASSERT_EQ(1, subnets->size()); + ASSERT_EQ(2, subnets->size()); // Perform 2-way exchange. client.useRapidCommit(true); // Include FQDN to trigger generation of name change requests. @@ -229,19 +236,51 @@ TEST_F(SARRTest, rapidCommitEnable) { EXPECT_EQ(1, CfgMgr::instance().getD2ClientMgr().getQueueSize()); } +// Check that the server responds with Advertise if the client hasn't +// included the Rapid Commit option in the Solicit. +TEST_F(SARRTest, rapidCommitNoOption) { + Dhcp6Client client; + // Configure client to request IA_NA + client.useNA(); + configure(CONFIGS[1], *client.getServer()); + ASSERT_NO_THROW(client.getServer()->startD2()); + // Make sure we ended-up having expected number of subnets configured. + const Subnet6Collection* subnets = CfgMgr::instance().getCurrentCfg()-> + getCfgSubnets6()->getAll(); + ASSERT_EQ(2, subnets->size()); + // Include FQDN to test that the server will not create name change + // requests when it sends Advertise (Rapid Commit disabled). + ASSERT_NO_THROW(client.useFQDN(Option6ClientFqdn::FLAG_S, + "client-name.example.org", + Option6ClientFqdn::FULL)); + ASSERT_NO_THROW(client.doSolicit()); + // There should be no lease because the server should have responded + // with Advertise. + ASSERT_EQ(0, client.getLeaseNum()); + // Make sure that the server responded. + ASSERT_TRUE(client.getContext().response_); + EXPECT_EQ(DHCPV6_ADVERTISE, client.getContext().response_->getType()); + // Make sure that the Rapid Commit option is not included. + EXPECT_FALSE(client.getContext().response_->getOption(D6O_RAPID_COMMIT)); + // There should be no name change request generated. + EXPECT_EQ(0, CfgMgr::instance().getD2ClientMgr().getQueueSize()); +} + // Check that when the Rapid Commit support is disabled for the subnet // the server replies with an Advertise and ignores the Rapid Commit // option sent by the client. TEST_F(SARRTest, rapidCommitDisable) { Dhcp6Client client; + // The subnet assigned to eth1 has Rapid Commit disabled. + client.setInterface("eth1"); // Configure client to request IA_NA client.useNA(); - configure(CONFIGS[2], *client.getServer()); + configure(CONFIGS[1], *client.getServer()); ASSERT_NO_THROW(client.getServer()->startD2()); // Make sure we ended-up having expected number of subnets configured. const Subnet6Collection* subnets = CfgMgr::instance().getCurrentCfg()-> getCfgSubnets6()->getAll(); - ASSERT_EQ(1, subnets->size()); + ASSERT_EQ(2, subnets->size()); // Send Rapid Commit option to the server. client.useRapidCommit(true); // Include FQDN to test that the server will not create name change diff --git a/src/lib/dhcpsrv/subnet.h b/src/lib/dhcpsrv/subnet.h index 5a025dacf0..92c896db2a 100644 --- a/src/lib/dhcpsrv/subnet.h +++ b/src/lib/dhcpsrv/subnet.h @@ -679,6 +679,9 @@ private: /// @brief A flag indicating if Rapid Commit option is supported /// for this subnet. + /// + /// It's default value is false, which indicates that the Rapid + /// Commit is disabled for the subnet. bool rapid_commit_; };