Merge pull request #507 from meejah/validate-configs.2

Actually validate configuration (an indentation error meant we weren't before). Also some unit-tests.

newsfragments/2935.bugfix

@@ -0,0 +1 @@
+Configuration-checking code wasn't being called due to indenting

src/allmydata/client.py

@@ -72,6 +72,7 @@ def _valid_config_sections():
             "expire.override_lease_duration",
             "readonly",
             "reserved_space",
+            "storage_dir",
         ),
         "sftpd": (
             "accounts.file",

src/allmydata/node.py

@@ -146,7 +146,7 @@ def read_config(basedir, portnumfile, generated_files=[], _valid_config_sections
     except EnvironmentError:
         if os.path.exists(config_fname):
             raise
-        configutil.validate_config(config_fname, parser, _valid_config_sections())
+    configutil.validate_config(config_fname, parser, _valid_config_sections())
     return _Config(parser, portnumfile, config_fname)
 
 

src/allmydata/scripts/tahoe_daemonize.py

@@ -8,6 +8,7 @@ from allmydata.scripts.default_nodedir import _default_nodedir
 from allmydata.util import fileutil
 from allmydata.node import read_config
 from allmydata.util.encodingutil import listdir_unicode, quote_local_unicode_path
+from allmydata.util.configutil import UnknownConfigError
 from twisted.application.service import Service
 
 
@@ -125,8 +126,12 @@ class DaemonizeTheRealService(Service):
             except KeyError:
                 raise ValueError("unknown nodetype %s" % self.nodetype)
 
-            srv = service_factory()
-            srv.setServiceParent(self.parent)
+            try:
+                srv = service_factory()
+                srv.setServiceParent(self.parent)
+            except UnknownConfigError as e:
+                sys.stderr.write("\nConfiguration error:\n{}\n\n".format(e))
+                reactor.stop()
 
         from twisted.internet import reactor
         reactor.callWhenRunning(start)

src/allmydata/test/cli/test_daemonize.py

@@ -37,6 +37,7 @@ class Util(unittest.TestCase):
         with patch('twisted.internet.reactor') as r:
             def call(fn, *args, **kw):
                 fn()
+            r.stop = lambda: None
             r.callWhenRunning = call
             service = plug.makeService(None)
             service.parent = Mock()
@@ -52,6 +53,7 @@ class Util(unittest.TestCase):
             def call(fn, *args, **kw):
                 fn()
             r.callWhenRunning = call
+            r.stop = lambda: None
             service = plug.makeService(None)
             service.parent = Mock()
             with self.assertRaises(ValueError) as ctx:
@@ -68,6 +70,7 @@ class Util(unittest.TestCase):
         with patch('twisted.internet.reactor') as r:
             def call(fn, *args, **kw):
                 fn()
+            r.stop = lambda: None
             r.callWhenRunning = call
             service = plug.makeService(None)
             service.parent = Mock()
@@ -97,6 +100,7 @@ class RunDaemonizeTests(unittest.TestCase):
         self._working = os.path.abspath('.')
         d = super(RunDaemonizeTests, self).setUp()
         self._reactor = patch('twisted.internet.reactor')
+        self._reactor.stop = lambda: None
         self._twistd = patch('allmydata.scripts.tahoe_daemonize.twistd')
         self.node_dir = self.mktemp()
         os.mkdir(self.node_dir)

src/allmydata/test/cli/test_start.py

@@ -1,9 +1,11 @@
 import os
+import sys
 import shutil
 import subprocess
 from os.path import join
 from mock import patch
 from StringIO import StringIO
+from functools import partial
 
 from twisted.trial import unittest
 from allmydata.scripts import runner
@@ -207,3 +209,69 @@ class RunStartTests(unittest.TestCase):
             e.getvalue()
         )
         self.assertEqual([1], exit_code)
+
+
+class RunTests(unittest.TestCase):
+    """
+    Tests confirming end-user behavior of CLI commands
+    """
+
+    def setUp(self):
+        d = super(RunTests, self).setUp()
+        self.addCleanup(partial(os.chdir, os.getcwd()))
+        self.node_dir = self.mktemp()
+        os.mkdir(self.node_dir)
+        return d
+
+    @patch('twisted.internet.reactor')
+    def test_run_invalid_config(self, reactor):
+        """
+        Configuration that's invalid should be obvious to the user
+        """
+
+        def cwr(fn, *args, **kw):
+            fn()
+
+        def stop(*args, **kw):
+            stopped.append(None)
+        stopped = []
+        reactor.callWhenRunning = cwr
+        reactor.stop = stop
+
+        with open(os.path.join(self.node_dir, "client.tac"), "w") as f:
+            f.write('test')
+
+        with open(os.path.join(self.node_dir, "tahoe.cfg"), "w") as f:
+            f.write(
+                "[invalid section]\n"
+                "foo = bar\n"
+            )
+
+        config = runner.parse_or_exit_with_explanation([
+            # have to do this so the tests don't muck around in
+            # ~/.tahoe (the default)
+            '--node-directory', self.node_dir,
+            'run',
+        ])
+
+        i, o, e = StringIO(), StringIO(), StringIO()
+        with patch.object(sys, 'stdout', o), patch.object(sys, 'stderr', e):
+            runner.dispatch(config, i, o, e)
+
+        output = o.getvalue()
+        # should print out the collected logs and an error-code
+        self.assertIn(
+            "invalid section",
+            output,
+        )
+        self.assertIn(
+            "Configuration error:",
+            output,
+        )
+        # this is SystemExit(0) for some reason I can't understand,
+        # while running on the command-line, "echo $?" shows "1" on
+        # this same error (some config exception)...
+        errs = self.flushLoggedErrors(SystemExit)
+        self.assertEqual(1, len(errs))
+        # ensure reactor.stop was actually called
+        self.assertEqual([None], stopped)

src/allmydata/test/no_network.py

@@ -26,7 +26,7 @@ import treq
 from allmydata.util.assertutil import _assert
 
 from allmydata import uri as tahoe_uri
-from allmydata.client import _Client
+from allmydata.client import _Client, _valid_config_sections
 from allmydata.storage.server import StorageServer, storage_index_to_dir
 from allmydata.util import fileutil, idlib, hashutil
 from allmydata.util.hashutil import permute_server_hash
@@ -188,7 +188,7 @@ def NoNetworkClient(basedir):
     # XXX FIXME this is just to avoid massive search-replace for now;
     # should be create_nonetwork_client() or something...
     from allmydata.node import read_config
-    config = read_config(basedir, u'client.port')
+    config = read_config(basedir, u'client.port', _valid_config_sections=_valid_config_sections)
     return _NoNetworkClient(config, basedir=basedir)
 
 

src/allmydata/test/test_client.py

@@ -71,7 +71,13 @@ class Basic(testutil.ReallyEqualMixin, testutil.NonASCIIPathMixin, unittest.Test
         old_mode = os.stat(fn).st_mode
         os.chmod(fn, 0)
         try:
-            e = self.assertRaises(EnvironmentError, read_config, basedir, "client.port")
+            e = self.assertRaises(
+                EnvironmentError,
+                read_config,
+                basedir,
+                "client.port",
+                _valid_config_sections=client._valid_config_sections,
+            )
             self.assertIn("Permission denied", str(e))
         finally:
             # don't leave undeleteable junk lying around
@@ -93,7 +99,13 @@ class Basic(testutil.ReallyEqualMixin, testutil.NonASCIIPathMixin, unittest.Test
         logged_messages = []
         self.patch(twisted.python.log, 'msg', logged_messages.append)
 
-        e = self.failUnlessRaises(OldConfigError, read_config, basedir, "client.port")
+        e = self.failUnlessRaises(
+            OldConfigError,
+            read_config,
+            basedir,
+            "client.port",
+            _valid_config_sections=client._valid_config_sections,
+        )
         abs_basedir = fileutil.abspath_expanduser_unicode(unicode(basedir)).encode(sys.getfilesystemencoding())
         self.failUnlessIn(os.path.join(abs_basedir, "introducer.furl"), e.args[0])
         self.failUnlessIn(os.path.join(abs_basedir, "no_storage"), e.args[0])

src/allmydata/test/test_node.py

@@ -16,9 +16,10 @@ import foolscap.logging.log
 from twisted.application import service
 from allmydata.node import Node, formatTimeTahoeStyle, MissingConfigEntry, read_config, config_from_string
 from allmydata.introducer.server import create_introducer
-from allmydata.client import create_client
+from allmydata.client import create_client, _valid_config_sections
 from allmydata.util import fileutil, iputil
 from allmydata.util.namespace import Namespace
+from allmydata.util.configutil import UnknownConfigError
 import allmydata.test.common_util as testutil
 
 
@@ -30,7 +31,11 @@ class TestNode(Node):
     CERTFILE='DEFAULT_CERTFILE_BLANK'
 
     def __init__(self, basedir):
-        config = read_config(basedir, 'DEFAULT_PORTNUMFILE_BLANK')
+        config = read_config(
+            basedir,
+            'DEFAULT_PORTNUMFILE_BLANK',
+            _valid_config_sections=_valid_config_sections,
+        )
         Node.__init__(self, config, basedir)
 
 
@@ -262,7 +267,11 @@ class PortLocation(unittest.TestCase):
         n = EmptyNode()
         basedir = os.path.join("test_node/portlocation/%s/%s" % (tp, tl))
         fileutil.make_dirs(basedir)
-        config = n.config = read_config(basedir, "node.port")
+        config = n.config = read_config(
+            basedir,
+            "node.port",
+            _valid_config_sections=_valid_config_sections,
+        )
         n._reveal_ip = True
 
         if exp in ("ERR1", "ERR2", "ERR3", "ERR4"):
@@ -377,7 +386,11 @@ class Listeners(unittest.TestCase):
             f.write("tub.location = %s\n" % location)
         # we're doing a lot of calling-into-setup-methods here, it might be
         # better to just create a real Node instance, I'm not sure.
-        n.config = read_config(n.basedir, "client.port")
+        n.config = read_config(
+            n.basedir,
+            "client.port",
+            _valid_config_sections=_valid_config_sections,
+        )
         n.check_privacy()
         n.services = []
         n.create_i2p_provider()
@@ -403,7 +416,11 @@ class Listeners(unittest.TestCase):
             f.write("tub.location = tcp:example.org:1234\n")
         # we're doing a lot of calling-into-setup-methods here, it might be
         # better to just create a real Node instance, I'm not sure.
-        n.config = read_config(n.basedir, "client.port")
+        n.config = read_config(
+            n.basedir,
+            "client.port",
+            _valid_config_sections=_valid_config_sections,
+        )
         n.check_privacy()
         n.services = []
         i2p_ep = object()
@@ -467,3 +484,41 @@ class IntroducerNotListening(unittest.TestCase):
         f.close()
         e = self.assertRaises(ValueError, create_introducer, basedir)
         self.assertIn("we are Introducer, but tub is not listening", str(e))
+
+class Configuration(unittest.TestCase):
+
+    def setUp(self):
+        self.basedir = self.mktemp()
+        fileutil.make_dirs(self.basedir)
+
+    def test_read_invalid_config(self):
+        with open(os.path.join(self.basedir, 'tahoe.cfg'), 'w') as f:
+            f.write(
+                '[invalid section]\n'
+                'foo = bar\n'
+            )
+        with self.assertRaises(UnknownConfigError) as ctx:
+            read_config(
+                self.basedir,
+                "client.port",
+                _valid_config_sections=_valid_config_sections,
+            )
+
+        self.assertIn(
+            "invalid section",
+            str(ctx.exception),
+        )
+
+    def test_create_client_invalid_config(self):
+        with open(os.path.join(self.basedir, 'tahoe.cfg'), 'w') as f:
+            f.write(
+                '[invalid section]\n'
+                'foo = bar\n'
+            )
+        with self.assertRaises(UnknownConfigError) as ctx:
+            create_client(self.basedir)
+
+        self.assertIn(
+            "invalid section",
+            str(ctx.exception),
+        )