[master] Merge branch 'trac3309'

doc/guide/bind10-guide.xml

@@ -3976,6 +3976,20 @@ Dhcp4/subnet4	[]	list	(default)
       <!-- @todo: describe record types -->
 
       <para>
+        The <xref linkend="dhcp4-custom-options"/> describes the configuration
+        syntax to create custom option definitions (formats). It is generally not
+        allowed to create custom definitions for standard options, even if the
+        definition being created matches the actual option format defined in the
+        RFCs. There is an exception from this rule for standard options for which
+        Kea does not provide a definition yet. In order to use such options,
+        a server administrator must create a definition as described in
+        <xref linkend="dhcp4-custom-options"/> in the 'dhcp4' option space. This
+        definition should match the option format described in the relevant
+        RFC but configuration mechanism would allow any option format as it has
+        no means to validate it at the moment.
+      </para>
+
+      <para>
         <table frame="all" id="dhcp4-std-options-list">
           <title>List of standard DHCPv4 options</title>
           <tgroup cols='4'>
@@ -4978,6 +4992,21 @@ Dhcp6/subnet6/	list
 
 <!-- @todo: describe record types -->
 
+      <para>
+        The <xref linkend="dhcp6-custom-options"/> describes the configuration
+        syntax to create custom option definitions (formats). It is generally not
+        allowed to create custom definitions for standard options, even if the
+        definition being created matches the actual option format defined in the
+        RFCs. There is an exception from this rule for standard options for which
+        Kea does not provide a definition yet. In order to use such options,
+        a server administrator must create a definition as described in
+        <xref linkend="dhcp6-custom-options"/> in the 'dhcp6' option space. This
+        definition should match the option format described in the relevant
+        RFC but configuration mechanism would allow any option format as it has
+        no means to validate it at the moment.
+      </para>
+
+
     <para>
       <table frame="all" id="dhcp6-std-options-list">
         <title>List of standard DHCPv6 options</title>

src/bin/dhcp4/tests/config_parser_unittest.cc

@@ -1425,14 +1425,13 @@ TEST_F(Dhcp4ParserTest, optionDefEncapsulateOwnSpace) {
 
 /// The purpose of this test is to verify that it is not allowed
 /// to override the standard option (that belongs to dhcp4 option
-/// space) and that it is allowed to define option in the dhcp4
-/// option space that has a code which is not used by any of the
-/// standard options.
+/// space and has its definition) and that it is allowed to define
+/// option in the dhcp4 option space that has a code which is not
+/// used by any of the standard options.
 TEST_F(Dhcp4ParserTest, optionStandardDefOverride) {
 
-    // Configuration string. The option code 109 is unassigned
-    // so it can be used for a custom option definition in
-    // dhcp4 option space.
+    // Configuration string. The option code 109 is unassigned so it
+    // can be used for a custom option definition in dhcp4 option space.
     std::string config =
         "{ \"option-def\": [ {"
         "      \"name\": \"foo\","
@@ -1465,15 +1464,14 @@ TEST_F(Dhcp4ParserTest, optionStandardDefOverride) {
     EXPECT_EQ(OPT_STRING_TYPE, def->getType());
     EXPECT_FALSE(def->getArrayType());
 
-    // The combination of option space and code is
-    // invalid. The 'dhcp4' option space groups
-    // standard options and the code 100 is reserved
-    // for one of them.
+    // The combination of option space and code is invalid. The 'dhcp4' option
+    // space groups standard options and the code 3 is reserved for one of
+    // them.
     config =
         "{ \"option-def\": [ {"
-        "      \"name\": \"foo\","
-        "      \"code\": 100,"
-        "      \"type\": \"string\","
+        "      \"name\": \"routers\","
+        "      \"code\": 3,"
+        "      \"type\": \"ipv4-address\","
         "      \"array\": False,"
         "      \"record-types\": \"\","
         "      \"space\": \"dhcp4\","
@@ -1487,6 +1485,40 @@ TEST_F(Dhcp4ParserTest, optionStandardDefOverride) {
     ASSERT_TRUE(status);
     // Expecting parsing error (error code 1).
     checkResult(status, 1);
+
+    /// @todo The option 65 is a standard DHCPv4 option. However, at this point
+    /// there is no definition for this option in libdhcp++, so it should be
+    /// allowed to define it from the configuration interface. This test will
+    /// have to be removed once definitions for remaining standard options are
+    /// created.
+    config =
+        "{ \"option-def\": [ {"
+        "      \"name\": \"nis-server-addr\","
+        "      \"code\": 65,"
+        "      \"type\": \"ipv4-address\","
+        "      \"array\": False,"
+        "      \"record-types\": \"\","
+        "      \"space\": \"dhcp4\","
+        "      \"encapsulate\": \"\""
+        "  } ]"
+        "}";
+    json = Element::fromJSON(config);
+
+    // Use the configuration string to create new option definition.
+    EXPECT_NO_THROW(status = configureDhcp4Server(*srv_, json));
+    ASSERT_TRUE(status);
+    // Expecting success.
+    checkResult(status, 0);
+
+    def = CfgMgr::instance().getOptionDef("dhcp4", 65);
+    ASSERT_TRUE(def);
+
+    // Check the option data.
+    EXPECT_EQ("nis-server-addr", def->getName());
+    EXPECT_EQ(65, def->getCode());
+    EXPECT_EQ(OPT_IPV4_ADDRESS_TYPE, def->getType());
+    EXPECT_FALSE(def->getArrayType());
+
 }
 
 // Goal of this test is to verify that global option

src/bin/dhcp6/tests/config_parser_unittest.cc

@@ -1746,9 +1746,9 @@ TEST_F(Dhcp6ParserTest, optionDefEncapsulateOwnSpace) {
 
 /// The purpose of this test is to verify that it is not allowed
 /// to override the standard option (that belongs to dhcp6 option
-/// space) and that it is allowed to define option in the dhcp6
-/// option space that has a code which is not used by any of the
-/// standard options.
+/// space and has its definition) and that it is allowed to define
+/// option in the dhcp6 option space that has a code which is not
+/// used by any of the standard options.
 TEST_F(Dhcp6ParserTest, optionStandardDefOverride) {
 
     // Configuration string. The option code 100 is unassigned
@@ -1786,9 +1786,8 @@ TEST_F(Dhcp6ParserTest, optionStandardDefOverride) {
     EXPECT_EQ(OPT_STRING_TYPE, def->getType());
     EXPECT_FALSE(def->getArrayType());
 
-    // The combination of option space and code is
-    // invalid. The 'dhcp6' option space groups
-    // standard options and the code 3 is reserved
+    // The combination of option space and code is invalid. The 'dhcp6'
+    // option space groups standard options and the code 3 is reserved
     // for one of them.
     config =
         "{ \"option-def\": [ {"
@@ -1808,6 +1807,39 @@ TEST_F(Dhcp6ParserTest, optionStandardDefOverride) {
     ASSERT_TRUE(status);
     // Expecting parsing error (error code 1).
     checkResult(status, 1);
+
+    /// @todo The option 59 is a standard DHCPv6 option. However, at this point
+    /// there is no definition for this option in libdhcp++, so it should be
+    /// allowed to define it from the configuration interface. This test will
+    /// have to be removed once definitions for remaining standard options are
+    /// created.
+    config =
+        "{ \"option-def\": [ {"
+        "      \"name\": \"boot-file-name\","
+        "      \"code\": 59,"
+        "      \"type\": \"string\","
+        "      \"array\": False,"
+        "      \"record-types\": \"\","
+        "      \"space\": \"dhcp6\","
+        "      \"encapsulate\": \"\""
+        "  } ]"
+        "}";
+    json = Element::fromJSON(config);
+
+    // Use the configuration string to create new option definition.
+    EXPECT_NO_THROW(status = configureDhcp6Server(srv_, json));
+    ASSERT_TRUE(status);
+    // Expecting success.
+    checkResult(status, 0);
+
+    def = CfgMgr::instance().getOptionDef("dhcp6", 59);
+    ASSERT_TRUE(def);
+
+    // Check the option data.
+    EXPECT_EQ("boot-file-name", def->getName());
+    EXPECT_EQ(59, def->getCode());
+    EXPECT_EQ(OPT_STRING_TYPE, def->getType());
+    EXPECT_FALSE(def->getArrayType());
 }
 
 // Goal of this test is to verify that global option

src/lib/dhcpsrv/cfgmgr.cc

@@ -74,12 +74,15 @@ CfgMgr::addOptionDef(const OptionDefinitionPtr& def,
         isc_throw(DuplicateOptionDefinition, "option definition already added"
                   << " to option space " << option_space);
 
+    // We must not override standard (assigned) option for which there is a
+    // definition in libdhcp++. The standard options belong to dhcp4 or dhcp6
+    // option space.
     } else if ((option_space == "dhcp4" &&
-                LibDHCP::isStandardOption(Option::V4, def->getCode())) ||
+                LibDHCP::isStandardOption(Option::V4, def->getCode()) &&
+                LibDHCP::getOptionDef(Option::V4, def->getCode())) ||
                (option_space == "dhcp6" &&
-                LibDHCP::isStandardOption(Option::V6, def->getCode()))) {
-        // We must not override standard (assigned) option. The standard options
-        // belong to dhcp4 or dhcp6 option space.
+                LibDHCP::isStandardOption(Option::V6, def->getCode()) &&
+                LibDHCP::getOptionDef(Option::V6, def->getCode()))) {
         isc_throw(BadValue, "unable to override definition of option '"
                   << def->getCode() << "' in standard option space '"
                   << option_space << "'.");

src/lib/dhcpsrv/tests/cfgmgr_unittest.cc

@@ -325,18 +325,46 @@ TEST_F(CfgMgrTest, getOptionDef) {
 
 }
 
+// This test verifies that it is not allowed to override a definition of the
+// standard option which has its definition defined in libdhcp++, but it is
+// allowed to create a definition for the standard option which doesn't have
+// its definition in libdhcp++.
+TEST_F(CfgMgrTest, overrideStdOptionDef) {
+    CfgMgr& cfg_mgr = CfgMgr::instance();
+
+    OptionDefinitionPtr def;
+    // There is a definition for routers option in libdhcp++, so an attempt
+    // to add (override) another definition for this option should fail.
+    def.reset(new OptionDefinition("routers", DHO_ROUTERS, "uint32"));
+    EXPECT_THROW(cfg_mgr.addOptionDef(def, "dhcp4"), isc::BadValue);
+
+    /// @todo There is no definition for the NIS Server Addr option in
+    /// libdhcp++. Once it is implemented it should be not allowed to
+    /// add a custom definition for it. At the moment, it should be ok
+    /// to add a definition for this option (using configuration mechanism)
+    /// because we haven't implemented the one in libdhcp++.
+    def.reset(new OptionDefinition("nis-server-addr", 65, "uint16"));
+    EXPECT_NO_THROW(cfg_mgr.addOptionDef(def, "dhcp4"));
+
+    // It is not allowed to override the definition of the option which
+    // has its definition in the libdhcp++.
+    def.reset(new OptionDefinition("sntp-servers", D6O_SNTP_SERVERS,
+                                   "ipv4-address"));
+    EXPECT_THROW(cfg_mgr.addOptionDef(def, "dhcp6"), isc::BadValue);
+    // There is no definition for option 59 in libdhcp++ yet, so it should
+    // be possible provide a custom definition.
+    def.reset(new OptionDefinition("bootfile-url", 59, "uint32"));
+    EXPECT_NO_THROW(cfg_mgr.addOptionDef(def, "dhcp6"));
+
+}
+
 // This test verifies that the function that adds new option definition
 // throws exceptions when arguments are invalid.
 TEST_F(CfgMgrTest, addOptionDefNegative) {
     CfgMgr& cfg_mgr = CfgMgr::instance();
-    // The option code 65 is reserved for standard options either in
-    // DHCPv4 or DHCPv6. Thus we expect that adding an option to this
-    // option space fails.
-    OptionDefinitionPtr def(new OptionDefinition("option-foo", 65, "uint16"));
-
-    // Try reserved option space names.
-    ASSERT_THROW(cfg_mgr.addOptionDef(def, "dhcp4"), isc::BadValue);
-    ASSERT_THROW(cfg_mgr.addOptionDef(def, "dhcp6"), isc::BadValue);
+
+    OptionDefinitionPtr def(new OptionDefinition("option-foo", 1000, "uint16"));
+
     // Try empty option space name.
     ASSERT_THROW(cfg_mgr.addOptionDef(def, ""), isc::BadValue);
     // Try NULL option definition.