Configuration of addresses to listen on

git-svn-id: svn://bind10.isc.org/svn/bind10/branches/vorner-recursor-config@3338 e5f2f494-b856-4b98-b285-d166d9295462

src/bin/recurse/main.cc

@@ -106,30 +106,10 @@ usage() {
 int
 main(int argc, char* argv[]) {
     int ch;
-    const char* port = DNSPORT;
-    const char* address = NULL;
     const char* uid = NULL;
-    bool use_ipv4 = true, use_ipv6 = true;
 
-    while ((ch = getopt(argc, argv, "46a:p:u:v")) != -1) {
+    while ((ch = getopt(argc, argv, "u:v")) != -1) {
         switch (ch) {
-        case '4':
-            // Note that -4 means "ipv4 only", we need to set "use_ipv6" here,
-            // not "use_ipv4".  We could use something like "ipv4_only", but
-            // we found the negatively named variable could confuse the code
-            // logic.
-            use_ipv6 = false;
-            break;
-        case '6':
-            // The same note as -4 applies.
-            use_ipv4 = false;
-            break;
-        case 'a':
-            address = optarg;
-            break;
-        case 'p':
-            port = optarg;
-            break;
         case 'u':
             uid = optarg;
             break;
@@ -146,18 +126,6 @@ main(int argc, char* argv[]) {
         usage();
     }
 
-    if (!use_ipv4 && !use_ipv6) {
-        cerr << "[b10-auth] Error: Cannot specify both -4 and -6 "
-             << "at the same time" << endl;
-        usage();
-    }
-
-    if ((!use_ipv4 || !use_ipv6) && address != NULL) {
-        cerr << "[b10-auth] Error: Cannot specify -4 or -6 "
-             << "at the same time as -a" << endl;
-        usage();
-    }
-
     int ret = 0;
 
     // XXX: we should eventually pass io_service here.
@@ -180,19 +148,7 @@ main(int argc, char* argv[]) {
         DNSLookup* lookup = recursor->getDNSLookupProvider();
         DNSAnswer* answer = recursor->getDNSAnswerProvider();
 
-        if (address != NULL) {
-            // XXX: we can only specify at most one explicit address.
-            // This also means the server cannot run in the dual address
-            // family mode if explicit addresses need to be specified.
-            // We don't bother to fix this problem, however.  The -a option
-            // is a short term workaround until we support dynamic listening
-            // port allocation.
-            io_service = new IOService(*port, *address,
-                                       checkin, lookup, answer);
-        } else {
-            io_service = new IOService(*port, use_ipv4, use_ipv6,
-                                       checkin, lookup, answer);
-        }
+        io_service = new IOService(checkin, lookup, answer);
         recursor->setIOService(*io_service);
         cout << "[b10-recurse] IOService created." << endl;
 
@@ -204,6 +160,7 @@ main(int argc, char* argv[]) {
                                              my_command_handler);
         cout << "[b10-recurse] Configuration channel established." << endl;
 
+        // FIXME: This does not belong here, but inside Boss
         if (uid != NULL) {
             changeUser(uid);
         }

src/bin/recurse/recurse.spec.pre.in

@@ -37,7 +37,11 @@
           {
             "address": "::1",
             "port": 5300
-          }
+          },
+          {
+            "address": "127.0.0.1",
+            "port": 5300
+          },
         ],
         "list_item_spec": {
           "item_name": "address",

src/bin/recurse/recursor.cc

@@ -58,6 +58,8 @@ using namespace isc::config;
 using namespace isc::xfr;
 using namespace asiolink;
 
+typedef pair<string, uint16_t> addr_t;
+
 class RecursorImpl {
 private:
     // prohibit copy
@@ -83,15 +85,15 @@ public:
         rec_query_ = NULL;
     }
 
-    void setForwardAddresses(const vector<pair<string, uint16_t> >& upstream,
+    void setForwardAddresses(const vector<addr_t>& upstream,
         IOService* ios)
     {
         queryShutdown();
         upstream_ = upstream;
         if (ios) {
             if (upstream_.empty()) {
-                cerr << "[b10-recurse] Asked to do full recursive." << endl <<
-                    ", but not implemented yet. I'll do nothing." << endl;
+                cerr << "[b10-recurse] Asked to do full recursive," << endl <<
+                    "but not implemented yet. I'll do nothing." << endl;
             } else {
                 querySetup(*ios);
             }
@@ -109,7 +111,7 @@ public:
     ModuleCCSession* config_session_;
     bool verbose_mode_;
     /// Addresses of the forward nameserver
-    vector<pair<string, uint16_t> > upstream_;
+    vector<addr_t> upstream_, listen_;
 
 private:
 
@@ -447,6 +449,46 @@ RecursorImpl::processNormalQuery(const Question& question, MessagePtr message,
     rec_query_->sendQuery(question, buffer, server);
 }
 
+namespace {
+
+vector<addr_t>
+parseAddresses(ConstElementPtr addresses) {
+    vector<addr_t> result;
+    if (addresses) {
+        if (addresses->getType() == Element::list) {
+            for (size_t i(0); i < addresses->size(); ++ i) {
+                ConstElementPtr addrPair(addresses->get(i));
+                ConstElementPtr addr(addrPair->get("address"));
+                ConstElementPtr port(addrPair->get("port"));
+                if (!addr || ! port) {
+                    isc_throw(BadValue, "Address must contain both the IP"
+                        "address and port");
+                }
+                try {
+                    IOAddress(addr->stringValue());
+                    if (port->intValue() < 0 ||
+                        port->intValue() > 0xffff) {
+                        isc_throw(BadValue, "Bad port value (" <<
+                            port->intValue() << ")");
+                    }
+                    result.push_back(addr_t(addr->stringValue(),
+                        port->intValue()));
+                }
+                catch (const TypeError &e) { // Better error message
+                    isc_throw(TypeError,
+                        "Address must be a string and port an integer");
+                }
+            }
+        } else if (addresses->getType() != Element::null) {
+            isc_throw(TypeError,
+                "forward_addresses config element must be a list");
+        }
+    }
+    return (result);
+}
+
+}
+
 ConstElementPtr
 Recursor::updateConfig(ConstElementPtr config) {
     if (impl_->verbose_mode_) {
@@ -454,41 +496,17 @@ Recursor::updateConfig(ConstElementPtr config) {
     }
     try {
         // Parse forward_addresses
-        vector<pair<string, uint16_t> > addresses;
-        ConstElementPtr forwardAddresses(config->get("forward_addresses"));
-        if (forwardAddresses) {
-            if (forwardAddresses->getType() == Element::list) {
-                for (size_t i(0); i < forwardAddresses->size(); ++ i) {
-                    ConstElementPtr addrPair(forwardAddresses->get(i));
-                    ConstElementPtr addr(addrPair->get("address"));
-                    ConstElementPtr port(addrPair->get("port"));
-                    if (!addr || ! port) {
-                        isc_throw(BadValue, "Address must contain both the IP"
-                            "address and port");
-                    }
-                    try {
-                        IOAddress(addr->stringValue());
-                        if (port->intValue() < 0 ||
-                            port->intValue() > 0xffff) {
-                            isc_throw(BadValue, "Bad port value (" <<
-                                port->intValue() << ")");
-                        }
-                        addresses.push_back(pair<string, uint16_t>(
-                            addr->stringValue(), port->intValue()));
-                    }
-                    catch (const TypeError &e) { // Better error message
-                        isc_throw(TypeError,
-                            "Address must be a string and port an integer");
-                    }
-                }
-            } else if (forwardAddresses->getType() != Element::null) {
-                isc_throw(TypeError,
-                    "forward_addresses config element must be a list");
-            }
-        }
+        ConstElementPtr forwardAddressesE(config->get("forward_addresses"));
+        vector<addr_t> forwardAddresses(parseAddresses(forwardAddressesE));
+        ConstElementPtr listenAddressesE(config->get("listen_addresses"));
+        vector<addr_t> listenAddresses(parseAddresses(listenAddressesE));
         // Everything OK, so commit the changes
-        if (forwardAddresses) {
-            setForwardAddresses(addresses);
+        // listenAddresses can fail to bind, so try them first
+        if (listenAddressesE) {
+            setListenAddresses(listenAddresses);
+        }
+        if (forwardAddressesE) {
+            setForwardAddresses(forwardAddresses);
         }
         return (isc::config::createAnswer());
     } catch (const isc::Exception& error) {
@@ -500,8 +518,7 @@ Recursor::updateConfig(ConstElementPtr config) {
 }
 
 void
-Recursor::setForwardAddresses(const vector<pair<string, uint16_t> >&
-    addresses)
+Recursor::setForwardAddresses(const vector<addr_t>& addresses)
 {
     impl_->setForwardAddresses(addresses, io_);
 }
@@ -511,6 +528,46 @@ Recursor::isForwarding() const {
     return (!impl_->upstream_.empty());
 }
 
-vector<pair<string, uint16_t> > Recursor::getForwardAddresses() const {
+vector<addr_t>
+Recursor::getForwardAddresses() const {
     return (impl_->upstream_);
 }
+
+namespace {
+
+void
+setAddresses(IOService *service, const vector<addr_t>&
+    addresses)
+{
+    service->clearServers();
+    BOOST_FOREACH(const addr_t &address, addresses) {
+        service->addServer(address.second, address.first);
+    }
+}
+
+}
+
+void
+Recursor::setListenAddresses(const vector<addr_t>&
+    addresses)
+{
+    try {
+        setAddresses(io_, addresses);
+        impl_->listen_ = addresses;
+    }
+    catch (const exception &e) {
+        /*
+         * We couldn't set it. So return it back. If that fails as well,
+         * we have a problem.
+         *
+         * FIXME: What to do in that case? Directly abort?
+         */
+        setAddresses(io_, impl_->listen_);
+        throw e;// Let it fly a little bit further
+    }
+}
+
+vector<addr_t>
+Recursor::getListenAddresses() const {
+    return (impl_->listen_);
+}

src/bin/recurse/recursor.h

@@ -111,6 +111,13 @@ public:
     /// Return if we are in forwarding mode (if not, we are in fully recursive)
     bool isForwarding() const;
 
+    /**
+     * Set and get the addresses we listen on.
+     */
+    void setListenAddresses(const std::vector<std::pair<std::string,
+        uint16_t> >& addresses);
+    std::vector<std::pair<std::string, uint16_t> > getListenAddresses() const;
+
 private:
     RecursorImpl* impl_;
     asiolink::IOService* io_;

src/bin/recurse/tests/recursor_unittest.cc

@@ -332,8 +332,14 @@ TEST_F(RecursorTest, notifyFail) {
 
 class RecursorConfig : public ::testing::Test {
     public:
+        IOService service;
         Recursor server;
-        void invalidForwardTest(const string &JOSN);
+        RecursorConfig() :
+            service(NULL, NULL, NULL)
+        {
+            server.setIOService(service);
+        }
+        void invalidTest(const string &JOSN);
 };
 
 TEST_F(RecursorConfig, forwardAddresses) {
@@ -387,7 +393,7 @@ TEST_F(RecursorConfig, forwardAddressConfig) {
     EXPECT_EQ(0, server.getForwardAddresses().size());
 }
 
-void RecursorConfig::invalidForwardTest(const string &JOSN) {
+void RecursorConfig::invalidTest(const string &JOSN) {
     ElementPtr config(Element::fromJSON(JOSN));
     EXPECT_FALSE(server.updateConfig(config)->equals(
         *isc::config::createAnswer())) << "Accepted config " << JOSN << endl;
@@ -395,27 +401,106 @@ void RecursorConfig::invalidForwardTest(const string &JOSN) {
 
 TEST_F(RecursorConfig, invalidForwardAddresses) {
     // Try torturing it with some invalid inputs
-    invalidForwardTest("{"
+    invalidTest("{"
         "\"forward_addresses\": \"error\""
         "}");
-    invalidForwardTest("{"
+    invalidTest("{"
         "\"forward_addresses\": [{}]"
         "}");
-    invalidForwardTest("{"
+    invalidTest("{"
         "\"forward_addresses\": [{"
         "   \"port\": 1.5,"
         "   \"address\": \"192.0.2.1\""
         "}]}");
-    invalidForwardTest("{"
+    invalidTest("{"
         "\"forward_addresses\": [{"
         "   \"port\": -5,"
         "   \"address\": \"192.0.2.1\""
         "}]}");
-    invalidForwardTest("{"
+    invalidTest("{"
         "\"forward_addresses\": [{"
         "   \"port\": 53,"
         "   \"address\": \"bad_address\""
         "}]}");
 }
 
+TEST_F(RecursorConfig, listenAddresses) {
+    // Default value should be fully recursive
+    EXPECT_TRUE(server.getListenAddresses().empty());
+
+    // Try putting there some addresses
+    vector<pair<string, uint16_t> > addresses;
+    addresses.push_back(pair<string, uint16_t>("127.0.0.1", 5300));
+    addresses.push_back(pair<string, uint16_t>("::1", 5300));
+    server.setListenAddresses(addresses);
+    EXPECT_EQ(2, server.getListenAddresses().size());
+    EXPECT_EQ("::1", server.getListenAddresses()[1].first);
+
+    // Is it independent from what we do with the vector later?
+    addresses.clear();
+    EXPECT_EQ(2, server.getListenAddresses().size());
+
+    // Did it return to fully recursive?
+    server.setListenAddresses(addresses);
+    EXPECT_TRUE(server.getListenAddresses().empty());
+}
+
+TEST_F(RecursorConfig, listenAddressConfig) {
+    // Try putting there some address
+    ElementPtr config(Element::fromJSON("{"
+        "\"listen_addresses\": ["
+        "   {"
+        "       \"address\": \"127.0.0.1\","
+        "       \"port\": 5300"
+        "   }"
+        "]"
+        "}"));
+    ConstElementPtr result(server.updateConfig(config));
+    EXPECT_EQ(result->toWire(), isc::config::createAnswer()->toWire());
+    ASSERT_EQ(1, server.getListenAddresses().size());
+    EXPECT_EQ("127.0.0.1", server.getListenAddresses()[0].first);
+    EXPECT_EQ(5300, server.getListenAddresses()[0].second);
+
+    // As this is example address, the machine should not have it on
+    // any interface
+    config = Element::fromJSON("{"
+        "\"listen_addresses\": ["
+        "   {"
+        "       \"address\": \"192.0.2.0\","
+        "       \"port\": 5300"
+        "   }"
+        "]"
+        "}");
+    result = server.updateConfig(config);
+    EXPECT_FALSE(result->equals(*isc::config::createAnswer()));
+    ASSERT_EQ(1, server.getListenAddresses().size());
+    EXPECT_EQ("127.0.0.1", server.getListenAddresses()[0].first);
+    EXPECT_EQ(5300, server.getListenAddresses()[0].second);
+}
+
+TEST_F(RecursorConfig, invalidListenAddresses) {
+    // Try torturing it with some invalid inputs
+    invalidTest("{"
+        "\"listen_addresses\": \"error\""
+        "}");
+    invalidTest("{"
+        "\"listen_addresses\": [{}]"
+        "}");
+    invalidTest("{"
+        "\"listen_addresses\": [{"
+        "   \"port\": 1.5,"
+        "   \"address\": \"192.0.2.1\""
+        "}]}");
+    invalidTest("{"
+        "\"listen_addresses\": [{"
+        "   \"port\": -5,"
+        "   \"address\": \"192.0.2.1\""
+        "}]}");
+    invalidTest("{"
+        "\"listen_addresses\": [{"
+        "   \"port\": 53,"
+        "   \"address\": \"bad_address\""
+        "}]}");
+}
+
 }