From 196c4d239ece786aa29b86e6b48bef337ca330fc Mon Sep 17 00:00:00 2001 From: Tomek Mrugalski Date: Tue, 11 Dec 2012 15:57:45 +0100 Subject: [PATCH] [2325] Changes after review. --- src/bin/dhcp6/dhcp6_messages.mes | 14 +++++-- src/bin/dhcp6/dhcp6_srv.cc | 5 ++- src/bin/dhcp6/dhcp6_srv.h | 5 ++- src/bin/dhcp6/tests/dhcp6_srv_unittest.cc | 45 +++++++++++++---------- 4 files changed, 41 insertions(+), 28 deletions(-) diff --git a/src/bin/dhcp6/dhcp6_messages.mes b/src/bin/dhcp6/dhcp6_messages.mes index be505911ba..f93258d6b7 100644 --- a/src/bin/dhcp6/dhcp6_messages.mes +++ b/src/bin/dhcp6/dhcp6_messages.mes @@ -22,6 +22,11 @@ successfully established a session with the BIND 10 control channel. This debug message is issued just before the IPv6 DHCP server attempts to establish a session with the BIND 10 control channel. +% DHCP6_CLIENTID_MISSING mandatory client-id option is missing, message from %1 dropped +This error message indicates that received message does not include +mandatory client-id option that is necessary for address assignment, +so the message is being dropped. This likely indicates a buggy client. + % DHCP6_COMMAND_RECEIVED received command %1, arguments: %2 A debug message listing the command (and possible arguments) received from the BIND 10 control system by the IPv6 DHCP server. @@ -82,10 +87,11 @@ received REQUEST) a lease for a given client. There may be many reasons for such failure. Each specific failure is logged in a separate log entry. % DHCP6_REQUIRED_OPTIONS_CHECK_FAIL %1 message received from %2 failed the following check: %3 -This message indicates that received message has invalid number of options: -mandatory client-id option is missing, server-id forbidden in that particular -type of message is present, there is more than one instance of client-id -or server-id etc. Exact reason for rejecting the packed is printed. +This message indicates that received DHCPv6 packet is invalid. This may be due +to a number of reasons, e.g. the mandatory client-id option is missing, +the server-id forbidden in that particular type of message is present, +there is more than one instance of client-id or server-id present, +etc. The exact reason for rejecting the packet is included in the message. % DHCP6_NOT_RUNNING IPv6 DHCP server is not running A warning message is issued when an attempt is made to shut down the diff --git a/src/bin/dhcp6/dhcp6_srv.cc b/src/bin/dhcp6/dhcp6_srv.cc index 5edb6eab23..dd6bf2777c 100644 --- a/src/bin/dhcp6/dhcp6_srv.cc +++ b/src/bin/dhcp6/dhcp6_srv.cc @@ -433,7 +433,7 @@ void Dhcpv6Srv::assignLeases(const Pkt6Ptr& question, Pkt6Ptr& answer) { // perhaps this should be logged on some higher level? This is most likely // configuration bug. - LOG_DEBUG(dhcp6_logger, DBG_DHCP6_BASIC, DHCP6_SUBNET_SELECTION_FAILED); + LOG_ERROR(dhcp6_logger, DHCP6_SUBNET_SELECTION_FAILED); } else { LOG_DEBUG(dhcp6_logger, DBG_DHCP6_DETAIL_DATA, DHCP6_SUBNET_SELECTED) .arg(subnet->toText()); @@ -451,6 +451,7 @@ void Dhcpv6Srv::assignLeases(const Pkt6Ptr& question, Pkt6Ptr& answer) { if (opt_duid) { duid = DuidPtr(new DUID(opt_duid->getData())); } else { + LOG_DEBUG(dhcp6_logger, DBG_DHCP6_BASIC, DHCP6_CLIENTID_MISSING); // Let's drop the message. This client is not sane. isc_throw(RFCViolation, "Mandatory client-id is missing in received message"); } @@ -624,7 +625,7 @@ void Dhcpv6Srv::renewLeases(const Pkt6Ptr& renew, Pkt6Ptr& reply) { // perhaps this should be logged on some higher level? This is most likely // configuration bug. - LOG_DEBUG(dhcp6_logger, DBG_DHCP6_BASIC, DHCP6_SUBNET_SELECTION_FAILED); + LOG_ERROR(dhcp6_logger, DHCP6_SUBNET_SELECTION_FAILED); } else { LOG_DEBUG(dhcp6_logger, DBG_DHCP6_DETAIL_DATA, DHCP6_SUBNET_SELECTED) .arg(subnet->toText()); diff --git a/src/bin/dhcp6/dhcp6_srv.h b/src/bin/dhcp6/dhcp6_srv.h index 8490d9b8b3..b6b0306e0e 100644 --- a/src/bin/dhcp6/dhcp6_srv.h +++ b/src/bin/dhcp6/dhcp6_srv.h @@ -213,8 +213,8 @@ protected: /// @brief Renews specific IA_NA option /// /// Generates response to IA_NA. This typically includes finding a lease that - /// corresponds to the received address. If no such lease is found, IA_NA - /// response is generates with appropriate status code. + /// corresponds to the received address. If no such lease is found, an IA_NA + /// response is generated with an appropriate status code. /// /// @param subnet subnet the sender belongs to /// @param duid client's duid @@ -255,6 +255,7 @@ protected: /// /// It supports addresses (IA_NA) only. It does NOT support temporary /// addresses (IA_TA) nor prefixes (IA_PD). + /// @todo: Extend this method once TA and PD becomes supported /// /// @param question client's message (with requested IA_NA) /// @param answer server's message (IA_NA options will be added here) diff --git a/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc b/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc index b02bee4079..62f32cdd8f 100644 --- a/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc +++ b/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc @@ -796,12 +796,15 @@ TEST_F(Dhcpv6SrvTest, RenewBasic) { boost::scoped_ptr srv; ASSERT_NO_THROW( srv.reset(new NakedDhcpv6Srv(0)) ); - IOAddress addr("2001:db8:1:1::cafe:babe"); + const IOAddress addr("2001:db8:1:1::cafe:babe"); const uint32_t iaid = 234; - // generate client-id also duid_ + // Generate client-id also duid_ OptionPtr clientid = generateClientId(); + // Check that the address we are about to use is indeed in pool + ASSERT_TRUE(subnet_->inPool(addr)); + // Note that preferred, valid, T1 and T2 timers and CLTT are set to invalid // value on purpose. They should be updated during RENEW. Lease6Ptr lease(new Lease6(Lease6::LEASE_IA_NA, addr, duid_, iaid, @@ -809,11 +812,12 @@ TEST_F(Dhcpv6SrvTest, RenewBasic) { lease->cltt_ = 1234; ASSERT_TRUE(LeaseMgrFactory::instance().addLease(lease)); - // check that the lease is really in the database + // Check that the lease is really in the database Lease6Ptr l = LeaseMgrFactory::instance().getLease6(addr); ASSERT_TRUE(l); - // Check that T1, T2, preferred, valid and cltt really + // Check that T1, T2, preferred, valid and cltt really set and not using + // previous (500, 501, etc.) values EXPECT_NE(l->t1_, subnet_->getT1()); EXPECT_NE(l->t2_, subnet_->getT2()); EXPECT_NE(l->preferred_lft_, subnet_->getPreferred()); @@ -825,38 +829,37 @@ TEST_F(Dhcpv6SrvTest, RenewBasic) { req->setRemoteAddr(IOAddress("fe80::abcd")); boost::shared_ptr ia = generateIA(iaid, 1500, 3000); - ASSERT_TRUE(subnet_->inPool(addr)); OptionPtr renewed_addr_opt(new Option6IAAddr(D6O_IAADDR, addr, 300, 500)); ia->addOption(renewed_addr_opt); req->addOption(ia); req->addOption(clientid); - // server-id is mandatory in RENEW + // Server-id is mandatory in RENEW req->addOption(srv->getServerID()); // Pass it to the server and hope for a REPLY Pkt6Ptr reply = srv->processRenew(req); - // check if we get response at all + // Check if we get response at all checkResponse(reply, DHCPV6_REPLY, 1234); OptionPtr tmp = reply->getOption(D6O_IA_NA); ASSERT_TRUE(tmp); - // check that IA_NA was returned and that there's an address included + // Check that IA_NA was returned and that there's an address included boost::shared_ptr addr_opt = checkIA_NA(reply, 234, subnet_->getT1(), subnet_->getT2()); - // check that we've got the address we requested + // Check that we've got the address we requested checkIAAddr(addr_opt, addr, subnet_->getPreferred(), subnet_->getValid()); - // check DUIDs + // Check DUIDs checkServerId(reply, srv->getServerID()); checkClientId(reply, clientid); - // check that the lease is really in the database + // Check that the lease is really in the database l = checkLease(duid_, reply->getOption(D6O_IA_NA), addr_opt); - EXPECT_TRUE(l); + ASSERT_TRUE(l); // Check that T1, T2, preferred, valid and cltt were really updated EXPECT_EQ(l->t1_, subnet_->getT1()); @@ -864,10 +867,10 @@ TEST_F(Dhcpv6SrvTest, RenewBasic) { EXPECT_EQ(l->preferred_lft_, subnet_->getPreferred()); EXPECT_EQ(l->valid_lft_, subnet_->getValid()); - // checking for CLTT is a bit tricky if we want to avoid off by 1 errors + // Checking for CLTT is a bit tricky if we want to avoid off by 1 errors int32_t cltt = static_cast(l->cltt_); int32_t expected = static_cast(time(NULL)); - // 1 >= difference between cltt and expected + // equality or difference by 1 between cltt and expected is ok. EXPECT_GE(1, abs(cltt - expected)); EXPECT_TRUE(LeaseMgrFactory::instance().deleteLease6(addr_opt->getAddress())); @@ -886,18 +889,22 @@ TEST_F(Dhcpv6SrvTest, RenewBasic) { // - returned REPLY message has IA that includes STATUS-CODE // - No lease in LeaseMgr TEST_F(Dhcpv6SrvTest, RenewReject) { + boost::scoped_ptr srv; ASSERT_NO_THROW( srv.reset(new NakedDhcpv6Srv(0)) ); - IOAddress addr("2001:db8:1:1::dead"); + const IOAddress addr("2001:db8:1:1::dead"); const uint32_t transid = 1234; const uint32_t valid_iaid = 234; const uint32_t bogus_iaid = 456; - // generate client-id also duid_ + // Quick sanity check that the address we're about to use is ok + ASSERT_TRUE(subnet_->inPool(addr)); + + // GenerateClientId() also sets duid_ OptionPtr clientid = generateClientId(); - // check that the lease is really in the database + // Check that the lease is NOT in the database Lease6Ptr l = LeaseMgrFactory::instance().getLease6(addr); ASSERT_FALSE(l); @@ -906,13 +913,12 @@ TEST_F(Dhcpv6SrvTest, RenewReject) { req->setRemoteAddr(IOAddress("fe80::abcd")); boost::shared_ptr ia = generateIA(bogus_iaid, 1500, 3000); - ASSERT_TRUE(subnet_->inPool(addr)); OptionPtr renewed_addr_opt(new Option6IAAddr(D6O_IAADDR, addr, 300, 500)); ia->addOption(renewed_addr_opt); req->addOption(ia); req->addOption(clientid); - // server-id is mandatory in RENEW + // Server-id is mandatory in RENEW req->addOption(srv->getServerID()); // Case 1: No lease known to server @@ -972,7 +978,6 @@ TEST_F(Dhcpv6SrvTest, RenewReject) { ASSERT_TRUE(ia); checkRejectedIA_NA(ia, STATUS_NoAddrsAvail); - lease = LeaseMgrFactory::instance().getLease6(addr); ASSERT_TRUE(lease); // Verify that the lease was not updated.