Refactor ExitEndpoint's config, mv dns ops from [network] to [dns]

llarp/config/config.hpp

@@ -74,7 +74,6 @@ namespace llarp
     std::string m_strictConnect;
     std::string m_ifname;
     std::string m_ifaddr;
-    std::string m_localDNS;
     FreehandOptions m_options;
 
     void
@@ -83,6 +82,9 @@ namespace llarp
 
   struct DnsConfig
   {
+    std::string m_localDNS;
+    std::string m_upstreamDNS;
+
     FreehandOptions m_options;
 
     void

llarp/exit/context.cpp

@@ -1,5 +1,6 @@
 #include <exit/context.hpp>
 #include <memory>
+#include <stdexcept>
 
 namespace llarp
 {
@@ -96,48 +97,21 @@ namespace llarp
       return false;
     }
 
-    bool
+    void
     Context::AddExitEndpoint(
         const std::string& name, const NetworkConfig& networkConfig, const DnsConfig& dnsConfig)
     {
-      throw std::runtime_error("FIXME");
+      if (m_Exits.find(name) != m_Exits.end())
-      /*
+        throw std::invalid_argument(stringify("An exit with name ", name, " already exists"));
-       * TODO: no more SetOption()
+
-       *
+      auto endpoint = std::make_unique<handlers::ExitEndpoint>(name, m_Router);
-      // check for duplicate exit by name
+      endpoint->Configure(networkConfig, dnsConfig);
-      {
+
-        auto itr = m_Exits.find(name);
-        if (itr != m_Exits.end())
-        {
-          LogError("duplicate exit with name ", name);
-          return false;
-        }
-      }
-      std::unique_ptr<handlers::ExitEndpoint> endpoint;
-      // make new endpoint
-      endpoint = std::make_unique<handlers::ExitEndpoint>(name, m_Router);
-      // configure
-      {
-        auto itr = conf.begin();
-        while (itr != conf.end())
-        {
-          if (!endpoint->SetOption(itr->first, itr->second))
-          {
-            LogWarn("Couldn't set option ", itr->first, " to  ", itr->second);
-            return false;
-          }
-          ++itr;
-        }
-      }
       // add endpoint
       if (!endpoint->Start())
-      {
+        throw std::runtime_error(stringify("Failed to start endpoint ", name));
-        LogWarn("Couldn't start exit endpoint");
+
-        return false;
-      }
       m_Exits.emplace(name, std::move(endpoint));
-      return true;
-       */
     }
 
   }  // namespace exit

llarp/exit/context.hpp

@@ -13,8 +13,6 @@ namespace llarp
     /// owner of all the exit endpoints
     struct Context
     {
-      using Config_t = std::unordered_multimap<std::string, std::string>;
-
       Context(AbstractRouter* r);
       ~Context();
 
@@ -31,7 +29,7 @@ namespace llarp
       void
       Stop();
 
-      bool
+      void
       AddExitEndpoint(
           const std::string& name, const NetworkConfig& networkConfig, const DnsConfig& dnsConfig);
 

llarp/handlers/exit.cpp

@@ -524,9 +524,11 @@ namespace llarp
       return true;
     }
 
-    bool
+    void
-    ExitEndpoint::SetOption(const std::string& k, const std::string& v)
+    ExitEndpoint::Configure(const NetworkConfig& networkConfig, const DnsConfig& dnsConfig)
     {
+      /*
+       * TODO
       if (k == "type" && v == "null")
       {
         m_ShouldInitTun = false;
@@ -537,89 +539,67 @@ namespace llarp
         m_PermitExit = IsTrueValue(v.c_str());
         return true;
       }
-      if (k == "local-dns")
+       */
-      {
+
-        std::string resolverAddr = v;
+      // TODO: parse this in config, use a proper type for addr+port
-        uint16_t dnsport = 53;
+      auto parseAddr = [](std::string input) {
-        auto pos = v.find(":");
+        uint16_t port = 53;
-        if (pos != std::string::npos)
+        auto pos = input.find(":");
-        {
-          resolverAddr = v.substr(0, pos);
-          dnsport = std::atoi(v.substr(pos + 1).c_str());
-        }
-        m_LocalResolverAddr = Addr(resolverAddr, dnsport);
-        LogInfo(Name(), " local dns set to ", m_LocalResolverAddr);
-      }
-      if (k == "upstream-dns")
-      {
-        std::string resolverAddr = v;
-        uint16_t dnsport = 53;
-        auto pos = v.find(":");
         if (pos != std::string::npos)
         {
-          resolverAddr = v.substr(0, pos);
+          input = input.substr(0, pos);
-          dnsport = std::atoi(v.substr(pos + 1).c_str());
+          port = std::atoi(input.substr(pos + 1).c_str());
         }
-        m_UpstreamResolvers.emplace_back(resolverAddr, dnsport);
+        return Addr(input, port);
-        LogInfo(Name(), " adding upstream dns set to ", resolverAddr, ":", dnsport);
+      };
-      }
+
-      if (k == "ifaddr")
+      m_LocalResolverAddr = parseAddr(dnsConfig.m_localDNS);
-      {
+
-        if (!m_OurRange.FromString(v))
+      m_UpstreamResolvers.push_back(parseAddr(dnsConfig.m_upstreamDNS));
-        {
+
-          LogError(Name(), " has invalid address range: ", v);
+      if (!m_OurRange.FromString(networkConfig.m_ifaddr))
-          return false;
-        }
-        auto pos = v.find("/");
-        if (pos == std::string::npos)
-        {
-          LogError(Name(), " ifaddr is not a cidr: ", v);
-          return false;
-        }
-        std::string nmask_str = v.substr(1 + pos);
-        std::string host_str = v.substr(0, pos);
-        // string, or just a plain char array?
-        strncpy(m_Tun.ifaddr, host_str.c_str(), sizeof(m_Tun.ifaddr) - 1);
-        m_Tun.netmask = std::atoi(nmask_str.c_str());
-        m_IfAddr = m_OurRange.addr;
-        m_NextAddr = m_IfAddr;
-        m_HigestAddr = m_OurRange.HighestAddr();
-        LogInfo(
-            Name(),
-            " set ifaddr range to ",
-            m_Tun.ifaddr,
-            "/",
-            m_Tun.netmask,
-            " lo=",
-            m_IfAddr,
-            " hi=",
-            m_HigestAddr);
-        m_UseV6 = false;
-      }
-      if (k == "ifname")
       {
-        if (v.length() >= sizeof(m_Tun.ifname))
+        throw std::invalid_argument(
-        {
+            stringify(Name(), " has invalid address range: ", networkConfig.m_ifaddr));
-          LogError(Name() + " ifname '", v, "' is too long");
-          return false;
-        }
-        strncpy(m_Tun.ifname, v.c_str(), sizeof(m_Tun.ifname) - 1);
-        LogInfo(Name(), " set ifname to ", m_Tun.ifname);
       }
-      if (k == "exit-whitelist")
+      // TODO: clean this up (make a util function for handling CIDR, etc.)
+      auto pos = networkConfig.m_ifaddr.find("/");
+      if (pos == std::string::npos)
       {
-        // add exit policy whitelist rule
+        throw std::invalid_argument(
-        // TODO: implement me
+            stringify(Name(), " ifaddr is not a cidr: ", networkConfig.m_ifaddr));
-        return true;
+        return false;
       }
-      if (k == "exit-blacklist")
+      std::string nmask_str = networkConfig.m_ifaddr.substr(1 + pos);
+      std::string host_str = networkConfig.m_ifaddr.substr(0, pos);
+      // string, or just a plain char array?
+      strncpy(m_Tun.ifaddr, host_str.c_str(), sizeof(m_Tun.ifaddr) - 1);
+      m_Tun.netmask = std::atoi(nmask_str.c_str());
+      m_IfAddr = m_OurRange.addr;
+      m_NextAddr = m_IfAddr;
+      m_HigestAddr = m_OurRange.HighestAddr();
+      LogInfo(
+          Name(),
+          " set ifaddr range to ",
+          m_Tun.ifaddr,
+          "/",
+          m_Tun.netmask,
+          " lo=",
+          m_IfAddr,
+          " hi=",
+          m_HigestAddr);
+      m_UseV6 = false;
+
+      if (networkConfig.m_ifname.length() >= sizeof(m_Tun.ifname))
       {
-        // add exit policy blacklist rule
+        LogError(Name() + " ifname '", networkConfig.m_ifname, "' is too long");
-        // TODO: implement me
+        return false;
-        return true;
       }
+      strncpy(m_Tun.ifname, networkConfig.m_ifname.c_str(), sizeof(m_Tun.ifname) - 1);
+      LogInfo(Name(), " set ifname to ", m_Tun.ifname);
 
-      return true;
+      // TODO: "exit-whitelist" and "exit-blacklist"
+      //       (which weren't originally implemented)
     }
 
     huint128_t

llarp/handlers/exit.hpp

@@ -19,8 +19,8 @@ namespace llarp
       void
       Tick(llarp_time_t now);
 
-      bool
+      void
-      SetOption(const std::string& k, const std::string& v);
+      Configure(const NetworkConfig& networkConfig, const DnsConfig& dnsConfig);
 
       std::string
       Name() const;

llarp/router/router.cpp

@@ -857,7 +857,7 @@ namespace llarp
 
   /// Populates some default values on networkConfig if they are absent
   void
-  Router::PopulateNetworkConfigDefaults()
+  Router::PopulateConfigDefaults()
   {
     if (networkConfig.m_ifname.empty())
       networkConfig.m_ifname = llarp::FindFreeTun();
@@ -865,8 +865,8 @@ namespace llarp
     if (networkConfig.m_ifaddr.empty())
       networkConfig.m_ifaddr = llarp::FindFreeRange();
 
-    if (networkConfig.m_localDNS.empty())
+    if (dnsConfig.m_localDNS.empty())
-      networkConfig.m_localDNS = "127.0.0.1:53";
+      dnsConfig.m_localDNS = "127.0.0.1:53";
   }
 
   bool
@@ -993,7 +993,7 @@ namespace llarp
       return false;
     }
 
-    PopulateNetworkConfigDefaults();
+    PopulateConfigDefaults();
 
     if (IsServiceNode())
     {
@@ -1167,7 +1167,8 @@ namespace llarp
     LogInfo("accepting transit traffic");
     paths.AllowTransit();
     llarp_dht_allow_transit(dht());
-    return _exitContext.AddExitEndpoint("default-connectivity", networkConfig, dnsConfig);
+    _exitContext.AddExitEndpoint("default-connectivity", networkConfig, dnsConfig);
+    return true;
   }
 
   bool

llarp/router/router.hpp

@@ -501,7 +501,7 @@ namespace llarp
     MessageSent(const RouterID& remote, SendStatus status);
 
     void
-    PopulateNetworkConfigDefaults();
+    PopulateConfigDefaults();
   };
 
 }  // namespace llarp

test/exit/test_llarp_exit_context.cpp

@@ -33,7 +33,7 @@ TEST_F(ExitTest, AddMultipleIP)
   llarp::NetworkConfig networkConfig;
   networkConfig.m_ifaddr = "10.0.0.1/24";
 
-  ASSERT_TRUE(context.AddExitEndpoint("test-exit", networkConfig, {}));
+  ASSERT_NO_THROW(context.AddExitEndpoint("test-exit", networkConfig, {}));
   ASSERT_TRUE(context.ObtainNewExit(pk, firstPath, true));
   ASSERT_TRUE(context.ObtainNewExit(pk, secondPath, true));
   ASSERT_TRUE(context.FindEndpointForPath(firstPath)->LocalIP()