From 2bc908cd4086a6bc9b0e6ebc09760cc4f2c0a2f3 Mon Sep 17 00:00:00 2001
From: Stephen Shelton <steve@brewcraft.org>
Date: Tue, 28 Apr 2020 09:37:22 -0600
Subject: [PATCH] Remove [dns]:local-dns in favor of [dns]:bind

---
 llarp/config/config.cpp | 36 +++++++++++++++++-------------------
 llarp/config/config.hpp |  6 ++----
 llarp/handlers/exit.cpp | 22 ++++------------------
 llarp/router/router.cpp |  7 -------
 4 files changed, 23 insertions(+), 48 deletions(-)

diff --git a/llarp/config/config.cpp b/llarp/config/config.cpp
index 6357438b5..ddc19e632 100644
--- a/llarp/config/config.cpp
+++ b/llarp/config/config.cpp
@@ -177,27 +177,25 @@ namespace llarp
   {
     (void)params;
 
-    // TODO: make sure this is documented
-    // TODO: refactor to remove freehand options map
-    conf.defineOption<std::string>("dns", "upstream", false, true, "", [this](std::string arg) {
-      m_options.emplace("upstream", std::move(arg));
-    });
+    auto parseAddr = [](std::string input) {
+      Addr addr;
+      bool success = addr.from_char_array(input);
+      if (not success)
+        throw std::invalid_argument(stringify(input, " is not a valid address"));
 
-    // TODO: the m_options is fixed in another branch/PR, this will conflict when merged
-    //       you're welcome
+      // default port if it wasn't specified
+      if (input.find(":") == std::string::npos)
+        addr.port(53);
 
-    // TODO: make sure this is documented
-    conf.defineOption<std::string>("dns", "local-dns", false, true, "", [this](std::string arg) {
-      m_options.emplace("local-dns", arg);
-      m_options.emplace("bind", arg);
-    });
+      return addr;
+    };
 
-    // TODO: we'll only support "bind" going forward, for now make sure bind and local-dns are
-    //       equivalent
-    conf.defineOption<std::string>("dns", "bind", false, true, "", [this](std::string arg) {
-      m_options.emplace("local-dns", arg);
-      m_options.emplace("bind", arg);
+    conf.defineOption<std::string>("dns", "upstream-dns", true, "", [=](std::string arg) {
+      m_upstreamDNS.push_back(parseAddr(arg));
     });
+
+    conf.defineOption<std::string>(
+        "dns", "bind", false, "", [=](std::string arg) { m_bind = parseAddr(arg); });
   }
 
   LinksConfig::LinkInfo
@@ -756,9 +754,9 @@ namespace llarp
 
     def.addOptionComments(
         "dns",
-        "upstream",
+        "upstream-dns",
         {
-            "Upstream resolver to use as fallback for non-loki addresses.",
+            "Upstream resolver(s) to use as fallback for non-loki addresses.",
             "Multiple values accepted.",
         });
 
diff --git a/llarp/config/config.hpp b/llarp/config/config.hpp
index 4f9c066ad..b0dd9fe35 100644
--- a/llarp/config/config.hpp
+++ b/llarp/config/config.hpp
@@ -82,10 +82,8 @@ namespace llarp
 
   struct DnsConfig
   {
-    std::string m_localDNS;
-    std::string m_upstreamDNS;
-
-    FreehandOptions m_options;
+    Addr m_bind;
+    std::vector<Addr> m_upstreamDNS;
 
     void
     defineConfigOptions(ConfigDefinition& conf, const ConfigGenParameters& params);
diff --git a/llarp/handlers/exit.cpp b/llarp/handlers/exit.cpp
index d5b7f0985..3eacac2fd 100644
--- a/llarp/handlers/exit.cpp
+++ b/llarp/handlers/exit.cpp
@@ -541,21 +541,8 @@ namespace llarp
       }
        */
 
-      // TODO: parse this in config, use a proper type for addr+port
-      auto parseAddr = [](std::string input) {
-        uint16_t port = 53;
-        auto pos = input.find(":");
-        if (pos != std::string::npos)
-        {
-          input = input.substr(0, pos);
-          port = std::atoi(input.substr(pos + 1).c_str());
-        }
-        return Addr(input, port);
-      };
-
-      m_LocalResolverAddr = parseAddr(dnsConfig.m_localDNS);
-
-      m_UpstreamResolvers.push_back(parseAddr(dnsConfig.m_upstreamDNS));
+      m_LocalResolverAddr = dnsConfig.m_bind;
+      m_UpstreamResolvers = dnsConfig.m_upstreamDNS;
 
       if (!m_OurRange.FromString(networkConfig.m_ifaddr))
       {
@@ -568,7 +555,6 @@ namespace llarp
       {
         throw std::invalid_argument(
             stringify(Name(), " ifaddr is not a cidr: ", networkConfig.m_ifaddr));
-        return false;
       }
       std::string nmask_str = networkConfig.m_ifaddr.substr(1 + pos);
       std::string host_str = networkConfig.m_ifaddr.substr(0, pos);
@@ -592,8 +578,8 @@ namespace llarp
 
       if (networkConfig.m_ifname.length() >= sizeof(m_Tun.ifname))
       {
-        LogError(Name() + " ifname '", networkConfig.m_ifname, "' is too long");
-        return false;
+        throw std::invalid_argument(
+            stringify(Name() + " ifname '", networkConfig.m_ifname, "' is too long"));
       }
       strncpy(m_Tun.ifname, networkConfig.m_ifname.c_str(), sizeof(m_Tun.ifname) - 1);
       LogInfo(Name(), " set ifname to ", m_Tun.ifname);
diff --git a/llarp/router/router.cpp b/llarp/router/router.cpp
index 1bdf62347..862e0dcbb 100644
--- a/llarp/router/router.cpp
+++ b/llarp/router/router.cpp
@@ -602,10 +602,6 @@ namespace llarp
         conf->router.m_nickname,
         diskworker());
 
-    // TODO: clean this up. it appears that we're dumping the [dns] "options" into the
-    //       [network] "options"
-    conf->network.m_options.insert(conf->dns.m_options.begin(), conf->dns.m_options.end());
-
     return true;
   }
 
@@ -864,9 +860,6 @@ namespace llarp
 
     if (networkConfig.m_ifaddr.empty())
       networkConfig.m_ifaddr = llarp::FindFreeRange();
-
-    if (dnsConfig.m_localDNS.empty())
-      dnsConfig.m_localDNS = "127.0.0.1:53";
   }
 
   bool