rib: remote prefix registration

resolve 3 bugs:

There are redundant registrations/unregistrations if loading the
config file multiple times.

Remote registration/unregistration will fail if localhop_security
is enabled.

Unstable RemoteRegistrator/UnregisterAdvanced test case.

Change-Id: I4437292e9f6c0e340c761ef7556a9bdc703ac06c
refs: #2294

nfd.conf.sample.in

@@ -288,6 +288,16 @@ rib
   ;   ; }
   ; }
 
+  ; The following localhop_security should be enabled when NFD runs on a hub,
+  ; which accepts all remote registrations and is a short-term solution.
+  ; localhop_security
+  ; {
+  ;   trust-anchor
+  ;   {
+  ;     type any
+  ;   }
+  ; }
+
   remote_register
   {
     cost 15 ; forwarding cost of prefix registered on remote router

rib/remote-registrator.cpp

@@ -35,10 +35,9 @@ NFD_LOG_INIT("RemoteRegistrator");
 using ndn::nfd::ControlParameters;
 using ndn::nfd::CommandOptions;
 
-
-const Name RemoteRegistrator::RM_LOCAL_PREFIX = "/localhost";
-const Name RemoteRegistrator::RM_HUB_PREFIX = "/localhop/nfd";
-const name::Component RemoteRegistrator::RM_IGNORE_COMMPONENT("rib");
+const Name RemoteRegistrator::LOCAL_REGISTRATION_PREFIX = "/localhost";
+const Name RemoteRegistrator::REMOTE_HUB_PREFIX = "/localhop/nfd/rib";
+const name::Component RemoteRegistrator::IGNORE_COMMPONENT("rib");
 
 RemoteRegistrator::RemoteRegistrator(ndn::nfd::Controller& controller,
                                      ndn::KeyChain& keyChain,
@@ -100,8 +99,14 @@ RemoteRegistrator::loadConfig(const ConfigSection& configSection)
      .setOrigin(ndn::nfd::ROUTE_ORIGIN_CLIENT)// set origin to client.
      .setFaceId(0);// the remote hub will take the input face as the faceId.
 
+   Name commandPrefix = REMOTE_HUB_PREFIX;
+   if (IGNORE_COMMPONENT == commandPrefix.at(-1))
+     {
+       commandPrefix = commandPrefix.getPrefix(-1);
+     }
+
    m_commandOptions
-     .setPrefix(RM_HUB_PREFIX)
+     .setPrefix(commandPrefix)
      .setTimeout(time::milliseconds(timeout));
 
    m_nRetries = retry;
@@ -116,16 +121,39 @@ RemoteRegistrator::loadConfig(const ConfigSection& configSection)
    m_refreshInterval = time::seconds(interval);
 }
 
+void
+RemoteRegistrator::enable()
+{
+  // do remote registration after an entry is inserted into the RIB.
+  m_afterInsertConnection =
+    m_rib.afterInsertEntry.connect([this] (const Name& prefix) {
+        registerPrefix(prefix);
+      });
+
+  // do remote unregistration after an entry is erased from the RIB.
+  m_afterEraseConnection =
+    m_rib.afterEraseEntry.connect([this] (const Name& prefix) {
+        unregisterPrefix(prefix);
+      });
+}
+
+void
+RemoteRegistrator::disable()
+{
+  m_afterInsertConnection.disconnect();
+  m_afterEraseConnection.disconnect();
+}
+
 void
 RemoteRegistrator::registerPrefix(const Name& prefix)
 {
-  if (RM_LOCAL_PREFIX.isPrefixOf(prefix))
+  if (LOCAL_REGISTRATION_PREFIX.isPrefixOf(prefix))
     {
       NFD_LOG_INFO("local registration only for " << prefix);
       return;
     }
 
-  bool isHubPrefix = prefix == RM_HUB_PREFIX;
+  bool isHubPrefix = prefix == REMOTE_HUB_PREFIX;
 
   if (isHubPrefix)
     {
@@ -181,7 +209,7 @@ RemoteRegistrator::registerPrefix(const Name& prefix)
 void
 RemoteRegistrator::unregisterPrefix(const Name& prefix)
 {
-  if (prefix == RM_HUB_PREFIX)
+  if (prefix == REMOTE_HUB_PREFIX)
     {
       NFD_LOG_INFO("disconnected to hub with prefix: " << prefix);
 
@@ -267,7 +295,7 @@ RemoteRegistrator::findIdentityForRegistration(const Name& prefix)
   // longest prefix matching to all indenties.
   for (auto&& i : identities)
     {
-      if (!i.empty() && RM_IGNORE_COMMPONENT == i.at(-1))
+      if (!i.empty() && IGNORE_COMMPONENT == i.at(-1))
         {
           isPrefix = i.getPrefix(-1).isPrefixOf(prefix);
           curLength = i.size() - 1;
@@ -360,9 +388,9 @@ RemoteRegistrator::onRegFailure(uint32_t code, const std::string& reason,
                                 const CommandOptions& options,
                                 int nRetries)
 {
-  NFD_LOG_INFO("fail to unregister " << parameters.getName()
-                                     << "\n\t reason:" << reason
-                                     << "\n\t remain retries:" << nRetries);
+  NFD_LOG_INFO("fail to register " << parameters.getName()
+                                   << "\n\t reason:" << reason
+                                   << "\n\t remain retries:" << nRetries);
 
   if (nRetries > 0)
     {

rib/remote-registrator.hpp

@@ -36,6 +36,7 @@
 #include <ndn-cxx/management/nfd-control-command.hpp>
 #include <ndn-cxx/management/nfd-control-parameters.hpp>
 #include <ndn-cxx/management/nfd-command-options.hpp>
+#include <ndn-cxx/util/signal.hpp>
 
 namespace nfd {
 namespace rib {
@@ -71,6 +72,20 @@ public:
   void
   loadConfig(const ConfigSection& configSection);
 
+  /**
+   * @brief enable remote registration/unregistration.
+   *
+   */
+  void
+  enable();
+
+  /**
+   * @brief disable remote registration/unregistration.
+   *
+   */
+  void
+  disable();
+
   /**
    * @brief register a prefix to remote hub(s).
    *
@@ -197,7 +212,6 @@ private:
   void
   clearRefreshEvents();
 
-
 PUBLIC_WITH_TESTS_ELSE_PRIVATE:
   /**
    * When a locally registered prefix triggles remote
@@ -217,16 +231,17 @@ private:
   ndn::nfd::Controller& m_nfdController;
   ndn::KeyChain& m_keyChain;
   Rib& m_rib;
-
+  ndn::util::signal::ScopedConnection m_afterInsertConnection;
+  ndn::util::signal::ScopedConnection m_afterEraseConnection;
   ndn::nfd::ControlParameters m_controlParameters;
   ndn::nfd::CommandOptions m_commandOptions;
   time::seconds m_refreshInterval;
   bool m_hasConnectedHub;
   int m_nRetries;
 
-  static const Name RM_LOCAL_PREFIX; // /localhost
-  static const Name RM_HUB_PREFIX; // /localhop/nfd
-  static const name::Component RM_IGNORE_COMMPONENT; // rib
+  static const Name LOCAL_REGISTRATION_PREFIX; // /localhost
+  static const Name REMOTE_HUB_PREFIX; // /localhop/nfd/rib
+  static const name::Component IGNORE_COMMPONENT; // rib
 };
 
 } // namespace rib

rib/rib-manager.cpp

@@ -144,6 +144,8 @@ RibManager::onConfig(const ConfigSection& configSection,
                      bool isDryRun,
                      const std::string& filename)
 {
+  bool isRemoteRegisterEnabled = false;
+
   for (ConfigSection::const_iterator i = configSection.begin();
        i != configSection.end(); ++i)
     {
@@ -157,21 +159,23 @@ RibManager::onConfig(const ConfigSection& configSection,
       else if (i->first == "remote_register")
         {
           m_remoteRegistrator.loadConfig(i->second);
+          isRemoteRegisterEnabled = true;
+          // avoid other actions when isDryRun == true
+          if (isDryRun)
+            {
+              continue;
+            }
 
-          // register callback to the RIB.
-          // do remote registration after an entry is inserted into the RIB.
-          // do remote unregistration after an entry is erased from the RIB.
-          m_managedRib.afterInsertEntry += [this] (const Name& prefix) {
-            m_remoteRegistrator.registerPrefix(prefix);
-          };
-
-          m_managedRib.afterEraseEntry += [this] (const Name& prefix) {
-            m_remoteRegistrator.unregisterPrefix(prefix);
-          };
+          m_remoteRegistrator.enable();
         }
       else
         throw Error("Unrecognized rib property: " + i->first);
     }
+
+  if (!isRemoteRegisterEnabled)
+    {
+      m_remoteRegistrator.disable();
+    }
 }
 
 void
@@ -200,16 +204,7 @@ void
 RibManager::onLocalhostRequest(const Interest& request)
 {
   const Name& command = request.getName();
-
-  if (command.size() <= COMMAND_PREFIX.size())
-    {
-      // command is too short to have a verb
-      NFD_LOG_DEBUG("command result: malformed");
-      sendResponse(command, 400, "Malformed command");
-      return;
-    }
-
-  const Name::Component& verb = command.at(COMMAND_PREFIX.size());
+  const Name::Component& verb = command.get(COMMAND_PREFIX.size());
 
   UnsignedVerbDispatchTable::const_iterator unsignedVerbProcessor = m_unsignedVerbDispatch.find(verb);
 

rib/rib.hpp

@@ -30,6 +30,7 @@
 #include "fib-update.hpp"
 #include "common.hpp"
 #include <ndn-cxx/management/nfd-control-command.hpp>
+#include <ndn-cxx/util/signal.hpp>
 
 namespace nfd {
 namespace rib {
@@ -137,8 +138,8 @@ private:
   removeInheritedFacesFromEntry(RibEntry& entry, const Rib::FaceSet& facesToRemove);
 
 public:
-  ndn::util::EventEmitter<Name> afterInsertEntry;
-  ndn::util::EventEmitter<Name> afterEraseEntry;
+  ndn::util::signal::Signal<Rib, Name> afterInsertEntry;
+  ndn::util::signal::Signal<Rib, Name> afterEraseEntry;
 
 private:
   RibTable m_rib;

tests/rib/remote-registrator.cpp

@@ -34,10 +34,11 @@ namespace rib {
 namespace tests {
 
 class RemoteRegistratorFixture : public nfd::tests::IdentityManagementFixture
+                               , public nfd::tests::UnitTestTimeFixture
 {
 public:
   RemoteRegistratorFixture()
-    : face(ndn::util::makeDummyClientFace())
+    : face(ndn::util::makeDummyClientFace(getGlobalIoService()))
     , controller(make_shared<ndn::nfd::Controller>(std::ref(*face), m_keyChain))
     , remoteRegistrator(make_shared<RemoteRegistrator>(std::ref(*controller),
                                                        m_keyChain,
@@ -48,41 +49,52 @@ public:
   {
     readConfig();
 
-    registerCallback();
+    remoteRegistrator->enable();
 
-    face->processEvents(time::milliseconds(1));
+    advanceClocks(time::milliseconds(1));
     face->sentInterests.clear();
   }
 
   void
-  readConfig()
+  readConfig(bool isSetRetry = false)
   {
     ConfigFile config;
     config.addSectionHandler("remote_register",
                              bind(&RemoteRegistrator::loadConfig, remoteRegistrator, _1));
 
-    const std::string CONFIG_STRING =
-    "remote_register\n"
-    "{\n"
-    "  cost 15\n"
-    "  timeout 10000\n"
-    "  retry 2\n"
-    "  refresh_interval 5\n"
-    "}";
 
-    config.parse(CONFIG_STRING, true, "test-remote-register");
+    if (isSetRetry)
+      {
+        const std::string CONFIG_STRING =
+        "remote_register\n"
+        "{\n"
+        "  cost 15\n"
+        "  timeout 1000\n"
+        "  retry 1\n"
+        "  refresh_interval 5\n"
+        "}";
+
+        config.parse(CONFIG_STRING, true, "test-remote-register");
+      }
+    else
+      {
+        const std::string CONFIG_STRING =
+        "remote_register\n"
+        "{\n"
+        "  cost 15\n"
+        "  timeout 100000\n"
+        "  retry 0\n"
+        "  refresh_interval 5\n"
+        "}";
+
+        config.parse(CONFIG_STRING, true, "test-remote-register");
+      }
   }
 
   void
-  registerCallback()
+  waitForTimeout()
   {
-    rib.afterInsertEntry += [this] (const Name& prefix) {
-      remoteRegistrator->registerPrefix(prefix);
-    };
-
-    rib.afterEraseEntry += [this] (const Name& prefix) {
-      remoteRegistrator->unregisterPrefix(prefix);
-    };
+    advanceClocks(time::milliseconds(100), time::seconds(1));
   }
 
   void
@@ -97,7 +109,7 @@ public:
 
     rib.insert(identity.append(appName), faceEntry);
 
-    face->processEvents(time::milliseconds(1));
+    advanceClocks(time::milliseconds(1));
   }
 
   void
@@ -110,7 +122,7 @@ public:
 
     rib.insert(identity.append(appName), faceEntry);
 
-    face->processEvents(time::milliseconds(1));
+    advanceClocks(time::milliseconds(1));
   }
 
   void
@@ -125,7 +137,7 @@ public:
 
     rib.erase(identity.append(appName), faceEntry);
 
-    face->processEvents(time::milliseconds(1));
+    advanceClocks(time::milliseconds(1));
   }
 
   void
@@ -138,7 +150,7 @@ public:
 
     rib.erase(identity.append(appName), faceEntry);
 
-    face->processEvents(time::milliseconds(1));
+    advanceClocks(time::milliseconds(1));
   }
 
   void
@@ -146,23 +158,23 @@ public:
   {
     rib.erase(faceId);
 
-    face->processEvents(time::milliseconds(1));
+    advanceClocks(time::milliseconds(1));
   }
 
   void
   connectToHub()
   {
-    rib.insert(Name("/localhop/nfd"), FaceEntry());
+    rib.insert(COMMAND_PREFIX, FaceEntry());
 
-    face->processEvents(time::milliseconds(1));
+    advanceClocks(time::milliseconds(1));
   }
 
   void
   disconnectToHub()
   {
-    rib.erase(Name("/localhop/nfd"), FaceEntry());
+    rib.erase(COMMAND_PREFIX, FaceEntry());
 
-    face->processEvents(time::milliseconds(1));
+    advanceClocks(time::milliseconds(1));
   }
 
   void
@@ -270,6 +282,55 @@ BOOST_FIXTURE_TEST_CASE(RegisterAdvanced, RemoteRegistratorFixture)
   BOOST_CHECK_EQUAL(extractedParameters.getName(), identity);
 }
 
+BOOST_FIXTURE_TEST_CASE(RegisterWithRedundantCallback, RemoteRegistratorFixture)
+{
+  remoteRegistrator->enable();
+
+  connectToHub();
+
+  Name identity("/remote/register");
+  insertEntryWithIdentity(identity);
+
+  BOOST_REQUIRE_EQUAL(face->sentInterests.size(), 1);
+
+  Interest& request = face->sentInterests[0];
+
+  ndn::nfd::ControlParameters extractedParameters;
+  Name::Component verb;
+  extractParameters(request, verb, extractedParameters);
+
+  BOOST_CHECK_EQUAL(verb, REGISTER_VERB);
+  BOOST_CHECK_EQUAL(extractedParameters.getName(), identity);
+}
+
+BOOST_FIXTURE_TEST_CASE(RegisterRetry, RemoteRegistratorFixture)
+{
+  // setRetry
+  readConfig(true);
+
+  connectToHub();
+
+  Name identity("/remote/register");
+  insertEntryWithIdentity(identity);
+
+  waitForTimeout();
+
+  BOOST_REQUIRE_EQUAL(face->sentInterests.size(), 2);
+
+  Interest& requestFirst  = face->sentInterests[0];
+  Interest& requestSecond = face->sentInterests[1];
+
+  ndn::nfd::ControlParameters extractedParametersFirst, extractedParametersSecond;
+  Name::Component verbFirst, verbSecond;
+  extractParameters(requestFirst,  verbFirst,  extractedParametersFirst);
+  extractParameters(requestSecond, verbSecond, extractedParametersSecond);
+
+  BOOST_CHECK_EQUAL(verbFirst,  REGISTER_VERB);
+  BOOST_CHECK_EQUAL(verbSecond, REGISTER_VERB);
+  BOOST_CHECK_EQUAL(extractedParametersFirst.getName(),  identity);
+  BOOST_CHECK_EQUAL(extractedParametersSecond.getName(), identity);
+}
+
 BOOST_FIXTURE_TEST_CASE(UnregisterWithoutInsert, RemoteRegistratorFixture)
 {
   connectToHub();