[2591] Code cleanup in Dhcp4SrvTest.processRequest and .processDiscover.

src/bin/dhcp4/tests/dhcp4_srv_unittest.cc

@@ -72,6 +72,73 @@ public:
         CfgMgr::instance().addSubnet4(subnet_);
     }
 
+    /// @brief Add 'Paramter Request List' option to the packet.
+    ///
+    /// This function PRL option comprising the following
+    /// option codes:
+    /// - 5 - Name Server
+    /// - 15 - Domain Name
+    /// - 7 - Log Server
+    /// - 8 - Quotes Server
+    /// - 9 - LPR Server
+    ///
+    /// @param pkt packet to add PRL option to.
+    void addPrlOption(Pkt4Ptr& pkt) {
+
+        OptionUint8ArrayPtr option_prl =
+            OptionUint8ArrayPtr(new OptionUint8Array(Option::V4,
+                                                     DHO_DHCP_PARAMETER_REQUEST_LIST));
+
+        std::vector<uint8_t> opts;
+        // Let's request options that have been configured for the subnet.
+        opts.push_back(DHO_DOMAIN_NAME_SERVERS);
+        opts.push_back(DHO_DOMAIN_NAME);
+        opts.push_back(DHO_LOG_SERVERS);
+        opts.push_back(DHO_COOKIE_SERVERS);
+        // Let's also request the option that hasn't been configured. In such
+        // case server should ignore request for this particular option.
+        opts.push_back(DHO_LPR_SERVERS);
+        // Put the requested option codes into the 'Parameter Request List'.
+        option_prl->setValues(opts);
+        // And add 'Parameter Request List' option into the DISCOVER packet.
+        pkt->addOption(option_prl);
+    }
+
+    /// @brief Configures options being requested in the PRL option.
+    ///
+    /// The lpr-servers option is NOT configured here altough it is
+    /// added to the 'Parameter Request List' option in the
+    /// \ref addPrlOption. When requested option is not configured
+    /// the server should not return it in its rensponse. The goal
+    /// of not configuring the requested option is to verify that
+    /// the server will not return it.
+    void configureRequestedOptions() {
+        // dns-servers
+        Option4AddrLstPtr
+            option_dns_servers(new Option4AddrLst(DHO_DOMAIN_NAME_SERVERS));
+        option_dns_servers->addAddress(IOAddress("192.0.2.1"));
+        option_dns_servers->addAddress(IOAddress("192.0.2.100"));
+        ASSERT_NO_THROW(subnet_->addOption(option_dns_servers, false, "dhcp4"));
+
+        // domain-name
+        OptionDefinition def("domain-name", DHO_DOMAIN_NAME, OPT_FQDN_TYPE);
+        boost::shared_ptr<OptionCustom>
+            option_domain_name(new OptionCustom(def, Option::V4));
+        option_domain_name->writeFqdn("example.com");
+        subnet_->addOption(option_domain_name, false, "dhcp4");
+
+        // log-servers
+        Option4AddrLstPtr option_log_servers(new Option4AddrLst(DHO_LOG_SERVERS));
+        option_log_servers->addAddress(IOAddress("192.0.2.2"));
+        option_log_servers->addAddress(IOAddress("192.0.2.10"));
+        ASSERT_NO_THROW(subnet_->addOption(option_log_servers, false, "dhcp4"));
+
+        // cookie-servers
+        Option4AddrLstPtr option_cookie_servers(new Option4AddrLst(DHO_COOKIE_SERVERS));
+        option_cookie_servers->addAddress(IOAddress("192.0.2.1"));
+        ASSERT_NO_THROW(subnet_->addOption(option_cookie_servers, false, "dhcp4"));
+    }
+
     /// @brief checks that the response matches request
     /// @param q query (client's message)
     /// @param a answer (server's message)
@@ -85,20 +152,41 @@ public:
         EXPECT_EQ(q->getIndex(),  a->getIndex());
         EXPECT_EQ(q->getGiaddr(), a->getGiaddr());
 
-        // Check that bare minimum of required options are there
+        // Check that bare minimum of required options are there.
+        // We don't check options requested by a client. Those
+        // are checked elsewhere.
         EXPECT_TRUE(a->getOption(DHO_SUBNET_MASK));
         EXPECT_TRUE(a->getOption(DHO_ROUTERS));
         EXPECT_TRUE(a->getOption(DHO_DHCP_SERVER_IDENTIFIER));
         EXPECT_TRUE(a->getOption(DHO_DHCP_LEASE_TIME));
         EXPECT_TRUE(a->getOption(DHO_SUBNET_MASK));
         EXPECT_TRUE(a->getOption(DHO_ROUTERS));
-        EXPECT_TRUE(a->getOption(DHO_DOMAIN_NAME));
-        EXPECT_TRUE(a->getOption(DHO_DOMAIN_NAME_SERVERS));
 
         // Check that something is offered
         EXPECT_TRUE(a->getYiaddr().toText() != "0.0.0.0");
     }
 
+    /// @brief Check that requested options are present.
+    ///
+    /// @param pkt packet to be checked.
+    void optionsCheck(const Pkt4Ptr& pkt) {
+        // Check that the requested and configured options are returned
+        // in the ACK message.
+        EXPECT_TRUE(pkt->getOption(DHO_DOMAIN_NAME))
+            << "domain-name not present in the response";
+        EXPECT_TRUE(pkt->getOption(DHO_DOMAIN_NAME_SERVERS))
+            << "dns-servers not present in the response";
+        EXPECT_TRUE(pkt->getOption(DHO_LOG_SERVERS))
+            << "log-servers not present in the response";
+        EXPECT_TRUE(pkt->getOption(DHO_COOKIE_SERVERS))
+            << "cookie-servers not present in the response";
+        // Check that the requested but not configured options are not
+        // returned in the ACK message.
+        EXPECT_FALSE(pkt->getOption(DHO_LPR_SERVERS))
+            << "domain-name present in the response but it is"
+            << " expected not to be present";
+    }
+
     /// @brief generates client-id option
     ///
     /// Generate client-id option of specified length
@@ -316,47 +404,10 @@ TEST_F(Dhcpv4SrvTest, processDiscover) {
     pkt->setRemotePort(DHCP4_SERVER_PORT);
 
     // We are going to test that certain options are returned
-    // in the OFFER message when requested using 'Parameter
-    // Request List' option. Let's configure those options that
-    // are returned when requested.
-
-    // dns-servers
-    OptionPtr option_dns_servers(new Option4AddrLst(DHO_DOMAIN_NAME_SERVERS));
-    ASSERT_NO_THROW(subnet_->addOption(option_dns_servers, false, "dhcp4"));
-    // domain-name
-    OptionDefinition def("domain-name", DHO_DOMAIN_NAME, OPT_FQDN_TYPE);
-    boost::shared_ptr<OptionCustom>
-        option_domain_name(new OptionCustom(def, Option::V4));
-    option_domain_name->writeFqdn("example.com");
-    OptionPtr opt_domain = boost::dynamic_pointer_cast<Option>(option_domain_name);
-    subnet_->addOption(opt_domain, false, "dhcp4");
-    // log-servers
-    OptionPtr option_log_servers(new Option4AddrLst(DHO_LOG_SERVERS));
-    ASSERT_NO_THROW(subnet_->addOption(option_log_servers, false, "dhcp4"));
-    // cookie-servers
-    OptionPtr option_cookie_servers(new Option4AddrLst(DHO_COOKIE_SERVERS));
-    ASSERT_NO_THROW(subnet_->addOption(option_cookie_servers, false, "dhcp4"));
-
-    // Add 'Parameter Request List' option. In this option we are going
-    // specify which options we request to be retured in the OFFER
-    // message.
-    OptionUint8ArrayPtr option_prl =
-        OptionUint8ArrayPtr(new OptionUint8Array(Option::V4,
-                                                 DHO_DHCP_PARAMETER_REQUEST_LIST));
-
-    std::vector<uint8_t> opts;
-    // Let's request options that have been configured for the subnet.
-    opts.push_back(DHO_DOMAIN_NAME_SERVERS);
-    opts.push_back(DHO_DOMAIN_NAME);
-    opts.push_back(DHO_LOG_SERVERS);
-    opts.push_back(DHO_COOKIE_SERVERS);
-    // Let's also request the option that hasn't been configured. In such
-    // case server should ignore request for this particular option.
-    opts.push_back(DHO_LPR_SERVERS);
-    // Put the requested option codes into the 'Parameter Request List'.
-    option_prl->setValues(opts);
-    // And add 'Parameter Request List' option into the DISCOVER packet.
-    pkt->addOption(option_prl);
+    // (or not returned) in the OFFER message when requested
+    // using 'Parameter Request List' option. Let's configure
+    // those options that are returned when requested.
+    configureRequestedOptions();
 
     // Should not throw
     EXPECT_NO_THROW(
@@ -373,6 +424,35 @@ TEST_F(Dhcpv4SrvTest, processDiscover) {
 
     messageCheck(pkt, offer);
 
+    // We did not request any options so they should not be present
+    // in the OFFER.
+    EXPECT_FALSE(offer->getOption(DHO_DOMAIN_NAME));
+    EXPECT_FALSE(offer->getOption(DHO_DOMAIN_NAME_SERVERS));
+    EXPECT_FALSE(offer->getOption(DHO_LOG_SERVERS));
+    EXPECT_FALSE(offer->getOption(DHO_COOKIE_SERVERS));
+    EXPECT_FALSE(offer->getOption(DHO_LPR_SERVERS));
+
+    // Add 'Parameter Request List' option.
+    addPrlOption(pkt);
+
+    // Now repeat the test but request some options.
+    EXPECT_NO_THROW(
+        offer = srv->processDiscover(pkt);
+    );
+
+    // Should return something
+    ASSERT_TRUE(offer);
+
+    EXPECT_EQ(DHCPOFFER, offer->getType());
+
+    // This is relayed message. It should be sent back to relay address.
+    EXPECT_EQ(pkt->getGiaddr(), offer->getRemoteAddr());
+
+    messageCheck(pkt, offer);
+
+    // Check that the requested options are returned.
+    optionsCheck(offer);
+
     // Now repeat the test for directly sent message
     pkt->setHops(0);
     pkt->setGiaddr(IOAddress("0.0.0.0"));
@@ -393,13 +473,8 @@ TEST_F(Dhcpv4SrvTest, processDiscover) {
 
     messageCheck(pkt, offer);
 
-    // Check that the requested and configured options are returned
-    // in the OFFER message.
-    EXPECT_TRUE(offer->getOption(DHO_LOG_SERVERS));
-    EXPECT_TRUE(offer->getOption(DHO_COOKIE_SERVERS));
-    // Check that the requested but not configured options are not
-    // returned in the OFFER message.
-    EXPECT_FALSE(offer->getOption(DHO_LPR_SERVERS));
+    // Check that the requested options are returned.
+    optionsCheck(offer);
 
     delete srv;
 }
@@ -427,6 +502,12 @@ TEST_F(Dhcpv4SrvTest, processRequest) {
     req->setRemoteAddr(IOAddress("192.0.2.56"));
     req->setGiaddr(IOAddress("192.0.2.67"));
 
+    // We are going to test that certain options are returned
+    // in the ACK message when requested using 'Parameter
+    // Request List' option. Let's configure those options that
+    // are returned when requested.
+    configureRequestedOptions();
+
     // Should not throw
     ASSERT_NO_THROW(
         ack = srv->processRequest(req);
@@ -442,6 +523,33 @@ TEST_F(Dhcpv4SrvTest, processRequest) {
 
     messageCheck(req, ack);
 
+    // We did not request any options so they should not be present
+    // in the ACK.
+    EXPECT_FALSE(ack->getOption(DHO_DOMAIN_NAME));
+    EXPECT_FALSE(ack->getOption(DHO_DOMAIN_NAME_SERVERS));
+    EXPECT_FALSE(ack->getOption(DHO_LOG_SERVERS));
+    EXPECT_FALSE(ack->getOption(DHO_COOKIE_SERVERS));
+    EXPECT_FALSE(ack->getOption(DHO_LPR_SERVERS));
+
+    // Add 'Parameter Request List' option.
+    addPrlOption(req);
+
+    // Repeat the test but request some options.
+    ASSERT_NO_THROW(
+        ack = srv->processRequest(req);
+    );
+
+    // Should return something
+    ASSERT_TRUE(ack);
+
+    EXPECT_EQ(DHCPACK, ack->getType());
+
+    // This is relayed message. It should be sent back to relay address.
+    EXPECT_EQ(req->getGiaddr(), ack->getRemoteAddr());
+
+    // Check that the requested options are returned.
+    optionsCheck(ack);
+
     // Now repeat the test for directly sent message
     req->setHops(0);
     req->setGiaddr(IOAddress("0.0.0.0"));
@@ -462,6 +570,9 @@ TEST_F(Dhcpv4SrvTest, processRequest) {
 
     messageCheck(req, ack);
 
+    // Check that the requested options are returned.
+    optionsCheck(ack);
+
     delete srv;
 }
 

src/lib/dhcpsrv/subnet.cc

@@ -44,7 +44,7 @@ bool Subnet::inRange(const isc::asiolink::IOAddress& addr) const {
 }
 
 void
-Subnet::addOption(OptionPtr& option, bool persistent,
+Subnet::addOption(const OptionPtr& option, bool persistent,
                   const std::string& option_space) {
     // @todo Once the #2313 is merged we need to use the OptionSpace object to
     // validate the option space name here. For now, let's check that the name

src/lib/dhcpsrv/subnet.h

@@ -69,7 +69,7 @@ public:
         ///
         /// @param opt option
         /// @param persist if true option is always sent.
-        OptionDescriptor(OptionPtr& opt, bool persist)
+        OptionDescriptor(const OptionPtr& opt, bool persist)
             : option(opt), persistent(persist) {};
 
         /// @brief Constructor
@@ -225,7 +225,7 @@ public:
     /// @param option_space name of the option space to add an option to.
     ///
     /// @throw isc::BadValue if invalid option provided.
-    void addOption(OptionPtr& option, bool persistent,
+    void addOption(const OptionPtr& option, bool persistent,
                    const std::string& option_space);
 
     /// @brief Delete all options configured for the subnet.