[#642,!373] Global parameters may be associated with server tags.

src/hooks/dhcp/mysql_cb/mysql_cb_impl.cc

@@ -250,21 +250,79 @@ MySqlConfigBackendImpl::getGlobalParameters(const int index,
         MySqlBinding::createString(SERVER_TAG_BUF_LENGTH) // server_tag
     };
 
+    // This remembers id of the last entry processed.
+    uint64_t last_id = 0;
+
     conn_.selectQuery(index, in_bindings, out_bindings,
-                      [&parameters] (MySqlBindingCollection& out_bindings) {
-        if (!out_bindings[1]->getString().empty()) {
-
-            // Convert value read as string from the database to the actual
-            // data type known from the database as binding #3.
-            StampedValuePtr stamped_value =
-                StampedValue::create(out_bindings[1]->getString(),
-                                     out_bindings[2]->getString(),
-                                     static_cast<Element::types>
-                                     (out_bindings[3]->getInteger<uint8_t>()));
-
-            stamped_value->setModificationTime(out_bindings[4]->getTimestamp());
-            stamped_value->setServerTag(out_bindings[5]->getString());
-            parameters.insert(stamped_value);
+                      [&last_id, &parameters] (MySqlBindingCollection& out_bindings) {
+
+        // Store id of the current entry.
+        uint64_t id = out_bindings[0]->getInteger<uint64_t>();
+
+        // If we're starting or if this is new parameter being processed...
+        if ((last_id == 0) || (last_id != id)) {
+
+            // parameter name
+            std::string name = out_bindings[1]->getString();
+
+            if (!name.empty()) {
+
+                // Remember last processed entry.
+                last_id = id;
+
+                // The server may use one of the two values present in the
+                // database, i.e. the value specified for the particular
+                // server tag or the value specified for all servers. The
+                // former takes precedence. Therefore, we check here if the
+                // value already present in the parameters collection is
+                // specified for all servers or selected server.
+                auto& index = parameters.get<StampedValueNameIndexTag>();
+                auto existing = index.find(name);
+                if (existing != index.end()) {
+                    // The value of this parameter has been already seen.
+                    // Let's check if the value we stored is for all
+                    // servers or one server.
+                    try {
+                        ServerTag existing_tag((*existing)->getServerTag());
+                        ServerTag new_tag(out_bindings[5]->getString());
+                        if (!existing_tag.amAll() && new_tag.amAll()) {
+                            // The stored value is for one server and the
+                            // currently fetched value is for all servers,
+                            // so let's drop the currently processed value.
+                            return;
+                        }
+
+                    } catch (...) {
+                        // This shouldn't occur because the value comes
+                        // from the database and should have been validated.
+                        return;
+                    }
+                }
+
+                // Convert value read as string from the database to the actual
+                // data type known from the database as binding #3.
+                StampedValuePtr stamped_value =
+                    StampedValue::create(out_bindings[1]->getString(),
+                                         out_bindings[2]->getString(),
+                                         static_cast<Element::types>
+                                         (out_bindings[3]->getInteger<uint8_t>()));
+
+                stamped_value->setId(id);
+                stamped_value->setModificationTime(out_bindings[4]->getTimestamp());
+                stamped_value->setServerTag(out_bindings[5]->getString());
+
+                // If the parameter is already stored, it means that the
+                // stored value is for all servers and the one we're fetching
+                // is for a server tag. Let's replace the value.
+                if (existing != index.end()) {
+                    parameters.replace(existing, stamped_value);
+
+                } else {
+                    // The parameter doesn't exist, so insert it whether
+                    // it is for all or one server.
+                    parameters.insert(stamped_value);
+                }
+            }
         }
     });
 }

src/hooks/dhcp/mysql_cb/tests/mysql_cb_dhcp4_unittest.cc

@@ -638,6 +638,117 @@ TEST_F(MySqlConfigBackendDHCPv4Test, createUpdateDeleteGlobalParameter4) {
     }
 }
 
+// This test verifies that it is possible to differentiate between the
+// global parameters by server tag and that the value specified for the
+// particular server overrides the value specified for all servers.
+TEST_F(MySqlConfigBackendDHCPv4Test, globalParameters4WithServerTags) {
+    // Create three global parameters having the same name.
+    StampedValuePtr global_parameter1 = StampedValue::create("global", "value1");
+    StampedValuePtr global_parameter2 = StampedValue::create("global", "value2");
+    StampedValuePtr global_parameter3 = StampedValue::create("global", "value3");
+
+    // Try to insert one of them and associate with non-existing server.
+    // This should fail because the server must be inserted first.
+    EXPECT_THROW(cbptr_->createUpdateGlobalParameter4(ServerSelector::ONE("server1"),
+                                                      global_parameter1),
+                 DbOperationError);
+
+    // Create two servers.
+    EXPECT_NO_THROW(cbptr_->createUpdateServer4(test_servers_[1]));
+    EXPECT_NO_THROW(cbptr_->createUpdateServer4(test_servers_[2]));
+
+    // This time inserting the global parameters for the server1 and server2 should
+    // be successful.
+    EXPECT_NO_THROW(cbptr_->createUpdateGlobalParameter4(ServerSelector::ONE("server1"),
+                                                         global_parameter1));
+
+    EXPECT_NO_THROW(cbptr_->createUpdateGlobalParameter4(ServerSelector::ONE("server2"),
+                                                         global_parameter2));
+
+    // The last parameter is associated with all servers.
+    EXPECT_NO_THROW(cbptr_->createUpdateGlobalParameter4(ServerSelector::ALL(),
+                                                         global_parameter3));
+
+    StampedValuePtr returned_global;
+
+    // Try to fetch the value specified for all servers.
+    EXPECT_NO_THROW(
+        returned_global = cbptr_->getGlobalParameter4(ServerSelector::ALL(),
+                                                      "global")
+    );
+    ASSERT_TRUE(returned_global);
+    EXPECT_EQ(global_parameter3->getValue(), returned_global->getValue());
+    EXPECT_EQ("all", returned_global->getServerTag());
+
+    // Try to fetch the value specified for the server1. This should override the
+    // value specified for all servers.
+    EXPECT_NO_THROW(
+        returned_global = cbptr_->getGlobalParameter4(ServerSelector::ONE("server1"),
+                                                      "global")
+    );
+    ASSERT_TRUE(returned_global);
+    EXPECT_EQ(global_parameter1->getValue(), returned_global->getValue());
+    EXPECT_EQ("server1", returned_global->getServerTag());
+
+    // The same in case of the server2.
+    EXPECT_NO_THROW(
+        returned_global = cbptr_->getGlobalParameter4(ServerSelector::ONE("server2"),
+                                                      "global")
+    );
+    ASSERT_TRUE(returned_global);
+    EXPECT_EQ(global_parameter2->getValue(), returned_global->getValue());
+    EXPECT_EQ("server2", returned_global->getServerTag());
+
+    StampedValueCollection returned_globals;
+
+    // Try to fetch the collection of globals for the server2. It should contain
+    // server specific values.
+    EXPECT_NO_THROW(
+        returned_globals = cbptr_->getAllGlobalParameters4(ServerSelector::ONE("server2"))
+    );
+    ASSERT_EQ(1, returned_globals.size());
+    returned_global = *returned_globals.begin();
+    EXPECT_EQ(global_parameter2->getValue(), returned_global->getValue());
+    EXPECT_EQ("server2", returned_global->getServerTag());
+
+    // Try to fetch the collection of global parameters specified for all servers.
+    // This excludes the values specific to server1 and server2. It returns only the
+    // common ones.
+    EXPECT_NO_THROW(
+        returned_globals = cbptr_->getAllGlobalParameters4(ServerSelector::ALL())
+    );
+    ASSERT_EQ(1, returned_globals.size());
+    returned_global = *returned_globals.begin();
+    EXPECT_EQ(global_parameter3->getValue(), returned_global->getValue());
+    EXPECT_EQ("all", returned_global->getServerTag());
+
+    // Delete the server1. It should remove associations of this server with the
+    // global parameters.
+    EXPECT_NO_THROW(cbptr_->deleteServer4(ServerTag("server1")));
+    EXPECT_NO_THROW(
+        returned_globals = cbptr_->getAllGlobalParameters4(ServerSelector::ONE("server1"))
+    );
+    ASSERT_EQ(1, returned_globals.size());
+    returned_global = *returned_globals.begin();
+    // As a result, the value fetched for the server1 should be the one available for
+    // all servers, rather than the one dedicated for server1. The association of
+    // the server1 specific value with the server1 should be gone.
+    EXPECT_EQ(global_parameter3->getValue(), returned_global->getValue());
+    EXPECT_EQ("all", returned_global->getServerTag());
+
+    // Delete all servers, except 'all'.
+    EXPECT_NO_THROW(cbptr_->deleteAllServers4());
+    EXPECT_NO_THROW(
+        returned_globals = cbptr_->getAllGlobalParameters4(ServerSelector::ALL())
+    );
+    ASSERT_EQ(1, returned_globals.size());
+    returned_global = *returned_globals.begin();
+    // The common value for all servers should still be available because 'all'
+    // logical server should not be deleted.
+    EXPECT_EQ(global_parameter3->getValue(), returned_global->getValue());
+    EXPECT_EQ("all", returned_global->getServerTag());
+}
+
 // This test verifies that all global parameters can be retrieved and deleted.
 TEST_F(MySqlConfigBackendDHCPv4Test, getAllGlobalParameters4) {
     // Create 3 parameters and put them into the database.

src/hooks/dhcp/mysql_cb/tests/mysql_cb_dhcp6_unittest.cc

@@ -678,6 +678,117 @@ TEST_F(MySqlConfigBackendDHCPv6Test, createUpdateDeleteGlobalParameter6) {
     }
 }
 
+// This test verifies that it is possible to differentiate between the
+// global parameters by server tag and that the value specified for the
+// particular server overrides the value specified for all servers.
+TEST_F(MySqlConfigBackendDHCPv6Test, globalParameters6WithServerTags) {
+    // Create three global parameters having the same name.
+    StampedValuePtr global_parameter1 = StampedValue::create("global", "value1");
+    StampedValuePtr global_parameter2 = StampedValue::create("global", "value2");
+    StampedValuePtr global_parameter3 = StampedValue::create("global", "value3");
+
+    // Try to insert one of them and associate with non-existing server.
+    // This should fail because the server must be inserted first.
+    EXPECT_THROW(cbptr_->createUpdateGlobalParameter6(ServerSelector::ONE("server1"),
+                                                      global_parameter1),
+                 DbOperationError);
+
+    // Create two servers.
+    EXPECT_NO_THROW(cbptr_->createUpdateServer6(test_servers_[1]));
+    EXPECT_NO_THROW(cbptr_->createUpdateServer6(test_servers_[2]));
+
+    // This time inserting the global parameters for the server1 and server2 should
+    // be successful.
+    EXPECT_NO_THROW(cbptr_->createUpdateGlobalParameter6(ServerSelector::ONE("server1"),
+                                                         global_parameter1));
+
+    EXPECT_NO_THROW(cbptr_->createUpdateGlobalParameter6(ServerSelector::ONE("server2"),
+                                                         global_parameter2));
+
+    // The last parameter is associated with all servers.
+    EXPECT_NO_THROW(cbptr_->createUpdateGlobalParameter6(ServerSelector::ALL(),
+                                                         global_parameter3));
+
+    StampedValuePtr returned_global;
+
+    // Try to fetch the value specified for all servers.
+    EXPECT_NO_THROW(
+        returned_global = cbptr_->getGlobalParameter6(ServerSelector::ALL(),
+                                                      "global")
+    );
+    ASSERT_TRUE(returned_global);
+    EXPECT_EQ(global_parameter3->getValue(), returned_global->getValue());
+    EXPECT_EQ("all", returned_global->getServerTag());
+
+    // Try to fetch the value specified for the server1. This should override the
+    // value specified for all servers.
+    EXPECT_NO_THROW(
+        returned_global = cbptr_->getGlobalParameter6(ServerSelector::ONE("server1"),
+                                                      "global")
+    );
+    ASSERT_TRUE(returned_global);
+    EXPECT_EQ(global_parameter1->getValue(), returned_global->getValue());
+    EXPECT_EQ("server1", returned_global->getServerTag());
+
+    // The same in case of the server2.
+    EXPECT_NO_THROW(
+        returned_global = cbptr_->getGlobalParameter6(ServerSelector::ONE("server2"),
+                                                      "global")
+    );
+    ASSERT_TRUE(returned_global);
+    EXPECT_EQ(global_parameter2->getValue(), returned_global->getValue());
+    EXPECT_EQ("server2", returned_global->getServerTag());
+
+    StampedValueCollection returned_globals;
+
+    // Try to fetch the collection of globals for the server2. It should contain
+    // server specific values.
+    EXPECT_NO_THROW(
+        returned_globals = cbptr_->getAllGlobalParameters6(ServerSelector::ONE("server2"))
+    );
+    ASSERT_EQ(1, returned_globals.size());
+    returned_global = *returned_globals.begin();
+    EXPECT_EQ(global_parameter2->getValue(), returned_global->getValue());
+    EXPECT_EQ("server2", returned_global->getServerTag());
+
+    // Try to fetch the collection of global parameters specified for all servers.
+    // This excludes the values specific to server1 and server2. It returns only the
+    // common ones.
+    EXPECT_NO_THROW(
+        returned_globals = cbptr_->getAllGlobalParameters6(ServerSelector::ALL())
+    );
+    ASSERT_EQ(1, returned_globals.size());
+    returned_global = *returned_globals.begin();
+    EXPECT_EQ(global_parameter3->getValue(), returned_global->getValue());
+    EXPECT_EQ("all", returned_global->getServerTag());
+
+    // Delete the server1. It should remove associations of this server with the
+    // global parameters.
+    EXPECT_NO_THROW(cbptr_->deleteServer6(ServerTag("server1")));
+    EXPECT_NO_THROW(
+        returned_globals = cbptr_->getAllGlobalParameters6(ServerSelector::ONE("server1"))
+    );
+    ASSERT_EQ(1, returned_globals.size());
+    returned_global = *returned_globals.begin();
+    // As a result, the value fetched for the server1 should be the one available for
+    // all servers, rather than the one dedicated for server1. The association of
+    // the server1 specific value with the server1 should be gone.
+    EXPECT_EQ(global_parameter3->getValue(), returned_global->getValue());
+    EXPECT_EQ("all", returned_global->getServerTag());
+
+    // Delete all servers, except 'all'.
+    EXPECT_NO_THROW(cbptr_->deleteAllServers6());
+    EXPECT_NO_THROW(
+        returned_globals = cbptr_->getAllGlobalParameters6(ServerSelector::ALL())
+    );
+    ASSERT_EQ(1, returned_globals.size());
+    returned_global = *returned_globals.begin();
+    // The common value for all servers should still be available because 'all'
+    // logical server should not be deleted.
+    EXPECT_EQ(global_parameter3->getValue(), returned_global->getValue());
+    EXPECT_EQ("all", returned_global->getServerTag());
+}
+
 // This test verifies that all global parameters can be retrieved and deleted.
 TEST_F(MySqlConfigBackendDHCPv6Test, getAllGlobalParameters6) {
     // Create 3 parameters and put them into the database.

src/lib/database/server_selector.h

@@ -53,30 +53,30 @@ public:
     };
 
     /// @brief Factory returning "unassigned" server selector.
-    static ServerSelector& UNASSIGNED() {
-        static ServerSelector selector(Type::UNASSIGNED);
+    static ServerSelector UNASSIGNED() {
+        ServerSelector selector(Type::UNASSIGNED);
         return (selector);
     }
 
     /// @brief Factory returning "all servers" selector.
-    static ServerSelector& ALL() {
-        static ServerSelector selector(Type::ALL);
+    static ServerSelector ALL() {
+        ServerSelector selector(Type::ALL);
         return (selector);
     }
 
     /// @brief Factory returning selector of one server.
     ///
     /// @param server_tag tag of the single server to be selected.
-    static ServerSelector& ONE(const std::string& server_tag) {
-        static ServerSelector selector(server_tag);
+    static ServerSelector ONE(const std::string& server_tag) {
+        ServerSelector selector(server_tag);
         return (selector);
     }
 
     /// @brief Factory returning "multiple servers" selector.
     ///
     /// @param server_tags set of server tags to be selected.
-    static ServerSelector& MULTIPLE(const std::set<std::string>& server_tags) {
-        static ServerSelector selector(server_tags);
+    static ServerSelector MULTIPLE(const std::set<std::string>& server_tags) {
+        ServerSelector selector(server_tags);
         return (selector);
     }
 

src/lib/process/cb_ctl_base.h

@@ -183,7 +183,7 @@ public:
 
         // Use the server_tag if set, otherwise use ALL.
         std::string server_tag = srv_cfg->getServerTag();
-        db::ServerSelector& server_selector =
+        db::ServerSelector server_selector =
             (server_tag.empty()? db::ServerSelector::ALL() : db::ServerSelector::ONE(server_tag));
 
         // This collection will hold the audit entries since the last update if