[#3576] Swap order of template and subclass

/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc
    TEST_F(Dhcpv4SrvTest, subClassPrecedence) - new test

/src/bin/dhcp6/tests/classify_unittests.cc
    TEST_F(ClassifyTest, subClassPrecedence) - new test

/src/lib/dhcp/pkt.cc
    Pkt::addSubClass() - changed to add subclass then template to packet

/src/lib/dhcp/tests/pkt4_unittest.cc
    TEST_F(Pkt4Test, templateClasses) - test now verifies class order

/src/lib/dhcp/tests/pkt6_unittest.cc
    TEST_F(Pkt6Test, templateClasses) - test now verifies class order

diff --git a/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc b/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc
index 0dccec99148cae2ca8dbcac535c03520a7fba280..9c5f81cad9f88f6d4504b59cb4cdbcbf5b9f20e8 100644 (file)
--- a/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc
+++ b/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc
@@ -6848,6 +6848,113 @@ TEST_F(StashAgentOptionTest, badHwareAddress) {
     EXPECT_FALSE(query_->inClass("STASH_AGENT_OPTIONS"));
 }
 
+// Checks that sub-class options have precedence of template class options
+TEST_F(Dhcpv4SrvTest, subClassPrecedence) {
+    IfaceMgrTestConfig test_config(true);
+    IfaceMgr::instance().openSockets4();
+
+    NakedDhcpv4Srv srv(0);
+
+    string config = R"^(
+    {
+        "interfaces-config": { "interfaces": [ "*" ] },
+        "rebind-timer": 2000,
+        "renew-timer": 1000,
+        "valid-lifetime": 4000,
+        "subnet4": [
+        {
+            "id": 1,
+            "subnet": "192.0.2.0/24",
+            "pools": [{ "pool": "192.0.2.1 - 192.0.2.100" }]
+        }],
+        "option-def": [{
+            "name": "opt1",
+            "code": 249,
+            "type": "string"
+        },
+        {
+            "name": "opt2",
+            "code": 250,
+            "type": "string"
+
+        }],
+        "client-classes": [
+        {
+            "name": "template-client-id",
+            "template-test": "substring(option[61].hex,0,3)",
+            "option-data": [
+            {
+                "name": "opt1",
+                "data": "template one"
+            },
+            {
+                "name": "opt2",
+                "data": "template two"
+            }]
+        },
+        {
+            "name": "SPAWN_template-client-id_111",
+            "option-data": [
+            {
+                "name": "opt2",
+                "data": "spawn two"
+            }]
+        }]
+    }
+    )^";
+
+    ConstElementPtr json;
+    ASSERT_NO_THROW(json = parseDHCP4(config));
+    ConstElementPtr status;
+
+    // Configure the server and make sure the config is accepted
+    EXPECT_NO_THROW(status = Dhcpv4SrvTest::configure(srv, json));
+    ASSERT_TRUE(status);
+    comment_ = config::parseAnswer(rcode_, status);
+    ASSERT_EQ(0, rcode_);
+
+    CfgMgr::instance().commit();
+
+    // Create packets with enough to select the subnet
+    auto id = ClientId::fromText("31:31:31");
+    OptionPtr clientid = (OptionPtr(new Option(Option::V4,
+                                               DHO_DHCP_CLIENT_IDENTIFIER,
+                                               id->getClientId())));
+
+    Pkt4Ptr query1(new Pkt4(DHCPDISCOVER, 1234));
+    query1->setRemoteAddr(IOAddress("192.0.2.1"));
+    query1->addOption(clientid);
+    query1->setIface("eth1");
+    query1->setIndex(ETH1_INDEX);
+
+    // Create and add a PRL option to the first 2 queries
+    OptionUint8ArrayPtr prl(new OptionUint8Array(Option::V4,
+                                                 DHO_DHCP_PARAMETER_REQUEST_LIST));
+    prl->addValue(249);
+    prl->addValue(250);
+    query1->addOption(prl);
+
+    // Classify packets
+    srv.classifyPacket(query1);
+
+    // Verify class membership is as expected.
+    EXPECT_TRUE(query1->inClass("template-client-id"));
+    EXPECT_TRUE(query1->inClass("SPAWN_template-client-id_111"));
+
+    // Process query
+    Pkt4Ptr response1 = srv.processDiscover(query1);
+
+    // Verify that opt1 is inherited from the template.
+    OptionPtr opt = response1->getOption(249);
+    ASSERT_TRUE(opt);
+    EXPECT_EQ(opt->toString(), "template one");
+
+    // Verify that for opt2 subclass overrides the template.
+    opt = response1->getOption(250);
+    ASSERT_TRUE(opt);
+    EXPECT_EQ(opt->toString(), "spawn two");
+}
+
 /// @todo: lease ownership
 
 /// @todo: Implement proper tests for MySQL lease/host database,

diff --git a/src/bin/dhcp6/tests/classify_unittests.cc b/src/bin/dhcp6/tests/classify_unittests.cc
index 85b1ef7e920ac2a3013f92ec50f8f6ca9c946bf6..2d89e83a7ba91e86f2b4698daab5696a2ac55496 100644 (file)
--- a/src/bin/dhcp6/tests/classify_unittests.cc
+++ b/src/bin/dhcp6/tests/classify_unittests.cc
@@ -2630,4 +2630,101 @@ TEST_F(ClassifyTest, earlyDrop) {
     EXPECT_TRUE(client2.getContext().response_);
 }
 
+// Checks that sub-class options have precedence of template class options
+TEST_F(ClassifyTest, subClassPrecedence) {
+    IfaceMgrTestConfig test_config(true);
+
+    NakedDhcpv6Srv srv(0);
+
+    string config = R"^(
+    {
+        "interfaces-config": {
+          "interfaces": [ "*" ]
+        },
+        "preferred-lifetime": 3000,
+        "rebind-timer": 2000,
+        "renew-timer": 1000,
+        "valid-lifetime": 4000,
+        "subnet6": [
+        {   "pools": [ { "pool": "2001:db8:1::/64" } ],
+            "subnet": "2001:db8:1::/48",
+            "id": 1,
+            "interface": "eth1",
+        }],
+        "option-def": [{
+            "name": "opt1",
+            "code": 1249,
+            "type": "string"
+        },
+        {
+            "name": "opt2",
+            "code": 1250,
+            "type": "string"
+
+        }],
+        "client-classes": [
+        {
+            "name": "template-client-id",
+            "template-test": "substring(option[1].hex,0,3)",
+            "option-data": [
+            {
+                "name": "opt1",
+                "data": "template one"
+            },
+            {
+                "name": "opt2",
+                "data": "template two"
+            }]
+        },
+        {
+            "name": "SPAWN_template-client-id_def",
+            "option-data": [
+            {
+                "name": "opt2",
+                "data": "spawn two"
+            }]
+        }]
+    }
+    )^";
+
+    ASSERT_NO_THROW(configure(config));
+
+    // Create packets with enough to select the subnet
+    Pkt6Ptr query1 = createSolicit();
+
+    // Create and add an ORO option to the first 2 queries
+    OptionUint16ArrayPtr oro(new OptionUint16Array(Option::V6, D6O_ORO));
+    ASSERT_TRUE(oro);
+    oro->addValue(1249);
+    oro->addValue(1250);
+    query1->addOption(oro);
+
+    // Classify packets
+    srv.classifyPacket(query1);
+
+    // Verify class membership is as expected.
+    EXPECT_TRUE(query1->inClass("template-client-id"));
+    EXPECT_TRUE(query1->inClass("SPAWN_template-client-id_def"));
+
+    // Process queries
+    AllocEngine::ClientContext6 ctx1;
+    bool drop = !srv.earlyGHRLookup(query1, ctx1);
+    ASSERT_FALSE(drop);
+    ctx1.subnet_ = srv_.selectSubnet(query1, drop);
+    ASSERT_FALSE(drop);
+    srv.initContext(ctx1, drop);
+    ASSERT_FALSE(drop);
+    Pkt6Ptr response1 = srv.processSolicit(ctx1);
+
+    // Verify that opt1 is inherited from the template.
+    OptionPtr opt = response1->getOption(1249);
+    ASSERT_TRUE(opt);
+    EXPECT_EQ(opt->toString(), "template one");
+
+    // Verify that for opt2 subclass overrides the template.
+    opt = response1->getOption(1250);
+    ASSERT_TRUE(opt);
+    EXPECT_EQ(opt->toString(), "spawn two");
+}
+
 } // end of anonymous namespace

diff --git a/src/lib/dhcp/pkt.cc b/src/lib/dhcp/pkt.cc
index 195480db5840e62c96f0466e1c07b5e1fdf039cd..1276515df52a819cf206e1a5258a7ab6bc4e78da 100644 (file)
--- a/src/lib/dhcp/pkt.cc
+++ b/src/lib/dhcp/pkt.cc
@@ -133,14 +133,14 @@ Pkt::addClass(const ClientClass& client_class, bool required) {
 
 void
 Pkt::addSubClass(const ClientClass& class_def, const ClientClass& subclass) {
-    if (!classes_.contains(class_def)) {
-        classes_.insert(class_def);
-        static_cast<void>(subclasses_.push_back(SubClassRelation(class_def, subclass)));
-    }
     if (!classes_.contains(subclass)) {
         classes_.insert(subclass);
         static_cast<void>(subclasses_.push_back(SubClassRelation(subclass, subclass)));
     }
+    if (!classes_.contains(class_def)) {
+        classes_.insert(class_def);
+        static_cast<void>(subclasses_.push_back(SubClassRelation(class_def, subclass)));
+    }
 }
 
 void

diff --git a/src/lib/dhcp/tests/pkt4_unittest.cc b/src/lib/dhcp/tests/pkt4_unittest.cc
index 4b953e507816fa0edf911d324ec3ae7fa76c43ab..ad418609d3b09912f5e6b7613a04bef41d301df2 100644 (file)
--- a/src/lib/dhcp/tests/pkt4_unittest.cc
+++ b/src/lib/dhcp/tests/pkt4_unittest.cc
@@ -1086,6 +1086,21 @@ TEST_F(Pkt4Test, templateClasses) {
     EXPECT_TRUE(pkt.inClass("SPAWN_template-interface-name_eth0"));
     EXPECT_TRUE(pkt.inClass("SPAWN_template-interface-id_interface-id0"));
 
+    // Verify the order is as expected.
+    const ClientClasses& classes = pkt.getClasses();
+    auto cclass = classes.cbegin();
+    ASSERT_NE(cclass, classes.cend());
+    EXPECT_EQ("SPAWN_template-interface-name_eth0", (*cclass));
+    ++cclass;
+    ASSERT_NE(cclass, classes.cend());
+    EXPECT_EQ("template-interface-name", (*cclass));
+    ++cclass;
+    ASSERT_NE(cclass, classes.cend());
+    EXPECT_EQ("SPAWN_template-interface-id_interface-id0", (*cclass));
+    ++cclass;
+    ASSERT_NE(cclass, classes.cend());
+    EXPECT_EQ("template-interface-id", (*cclass));
+
     // Check that it's ok to add to the same subclass repeatedly
     EXPECT_NO_THROW(pkt.addSubClass("template-foo", "SPAWN_template-foo_bar"));
     EXPECT_NO_THROW(pkt.addSubClass("template-foo", "SPAWN_template-foo_bar"));

diff --git a/src/lib/dhcp/tests/pkt6_unittest.cc b/src/lib/dhcp/tests/pkt6_unittest.cc
index adb7c5f9661a0caaf1e5cae3e3ac28667cc6552f..6febc4d2585267e63ed0da5d26f7f34add36b5e0 100644 (file)
--- a/src/lib/dhcp/tests/pkt6_unittest.cc
+++ b/src/lib/dhcp/tests/pkt6_unittest.cc
@@ -1386,6 +1386,21 @@ TEST_F(Pkt6Test, templateClasses) {
     EXPECT_TRUE(pkt.inClass("SPAWN_template-interface-name_eth0"));
     EXPECT_TRUE(pkt.inClass("SPAWN_template-interface-id_interface-id0"));
 
+    // Verify the order is as expected.
+    const ClientClasses& classes = pkt.getClasses();
+    auto cclass = classes.cbegin();
+    ASSERT_NE(cclass, classes.cend());
+    EXPECT_EQ("SPAWN_template-interface-name_eth0", (*cclass));
+    ++cclass;
+    ASSERT_NE(cclass, classes.cend());
+    EXPECT_EQ("template-interface-name", (*cclass));
+    ++cclass;
+    ASSERT_NE(cclass, classes.cend());
+    EXPECT_EQ("SPAWN_template-interface-id_interface-id0", (*cclass));
+    ++cclass;
+    ASSERT_NE(cclass, classes.cend());
+    EXPECT_EQ("template-interface-id", (*cclass));
+
     // Check that it's ok to add to the same subclass repeatedly
     EXPECT_NO_THROW(pkt.addSubClass("template-foo", "SPAWN_template-foo_bar"));
     EXPECT_NO_THROW(pkt.addSubClass("template-foo", "SPAWN_template-foo_bar"));