From 37239452963bb510a7a4e973480edbcb9d2b3872 Mon Sep 17 00:00:00 2001 From: meejah Date: Sat, 19 May 2018 20:10:39 -0600 Subject: [PATCH 01/11] Actually validate configs when loading them An indenting problem meant the validators weren't being called, which revealed some follow-on errors. --- src/allmydata/client.py | 1 + src/allmydata/node.py | 2 +- src/allmydata/test/no_network.py | 4 ++-- src/allmydata/test/test_client.py | 4 ++-- src/allmydata/test/test_node.py | 10 +++++----- 5 files changed, 11 insertions(+), 10 deletions(-) diff --git a/src/allmydata/client.py b/src/allmydata/client.py index 71397f961..d9b51a3d2 100644 --- a/src/allmydata/client.py +++ b/src/allmydata/client.py @@ -72,6 +72,7 @@ def _valid_config_sections(): "expire.override_lease_duration", "readonly", "reserved_space", + "storage_dir", ), "sftpd": ( "accounts.file", diff --git a/src/allmydata/node.py b/src/allmydata/node.py index 318f0764e..05cf146bc 100644 --- a/src/allmydata/node.py +++ b/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) diff --git a/src/allmydata/test/no_network.py b/src/allmydata/test/no_network.py index 2c0644fc1..26199cd92 100644 --- a/src/allmydata/test/no_network.py +++ b/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) diff --git a/src/allmydata/test/test_client.py b/src/allmydata/test/test_client.py index b99769d3c..3bfd30a87 100644 --- a/src/allmydata/test/test_client.py +++ b/src/allmydata/test/test_client.py @@ -71,7 +71,7 @@ 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 +93,7 @@ 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]) diff --git a/src/allmydata/test/test_node.py b/src/allmydata/test/test_node.py index 93d38e5ca..257e0dc88 100644 --- a/src/allmydata/test/test_node.py +++ b/src/allmydata/test/test_node.py @@ -16,7 +16,7 @@ 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 import allmydata.test.common_util as testutil @@ -30,7 +30,7 @@ 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 +262,7 @@ 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 +377,7 @@ 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 +403,7 @@ 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() From 8e0e96da0134d83e3e086979dd1e86042bc8b785 Mon Sep 17 00:00:00 2001 From: meejah Date: Tue, 29 May 2018 15:51:59 -0600 Subject: [PATCH 02/11] improve user experience on config errors --- src/allmydata/scripts/tahoe_daemonize.py | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/allmydata/scripts/tahoe_daemonize.py b/src/allmydata/scripts/tahoe_daemonize.py index 3c511c9c6..e04408fc0 100644 --- a/src/allmydata/scripts/tahoe_daemonize.py +++ b/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,15 @@ 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() + except Exception as e: + sys.stderr.write("\nError building service:\n{}\n\n".format(e)) + reactor.stop() from twisted.internet import reactor reactor.callWhenRunning(start) From 4aec12a92f2722a08982cb30dbe22821c1dbe74a Mon Sep 17 00:00:00 2001 From: meejah Date: Tue, 29 May 2018 15:52:09 -0600 Subject: [PATCH 03/11] whitespace on long lines --- src/allmydata/test/test_client.py | 16 ++++++++++++++-- src/allmydata/test/test_node.py | 24 ++++++++++++++++++++---- 2 files changed, 34 insertions(+), 6 deletions(-) diff --git a/src/allmydata/test/test_client.py b/src/allmydata/test/test_client.py index 3bfd30a87..77cda2d73 100644 --- a/src/allmydata/test/test_client.py +++ b/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", _valid_config_sections=client._valid_config_sections) + 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", _valid_config_sections=client._valid_config_sections) + 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]) diff --git a/src/allmydata/test/test_node.py b/src/allmydata/test/test_node.py index 257e0dc88..094c3fe12 100644 --- a/src/allmydata/test/test_node.py +++ b/src/allmydata/test/test_node.py @@ -30,7 +30,11 @@ class TestNode(Node): CERTFILE='DEFAULT_CERTFILE_BLANK' def __init__(self, basedir): - config = read_config(basedir, 'DEFAULT_PORTNUMFILE_BLANK', _valid_config_sections=_valid_config_sections) + config = read_config( + basedir, + 'DEFAULT_PORTNUMFILE_BLANK', + _valid_config_sections=_valid_config_sections, + ) Node.__init__(self, config, basedir) @@ -262,7 +266,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", _valid_config_sections=_valid_config_sections) + 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 +385,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", _valid_config_sections=_valid_config_sections) + 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 +415,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", _valid_config_sections=_valid_config_sections) + n.config = read_config( + n.basedir, + "client.port", + _valid_config_sections=_valid_config_sections, + ) n.check_privacy() n.services = [] i2p_ep = object() From bd63a4354b049965539055e3c629f51bcd1a7627 Mon Sep 17 00:00:00 2001 From: meejah Date: Tue, 29 May 2018 17:09:46 -0600 Subject: [PATCH 04/11] fixup for errors --- src/allmydata/scripts/tahoe_daemonize.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/allmydata/scripts/tahoe_daemonize.py b/src/allmydata/scripts/tahoe_daemonize.py index e04408fc0..f9b26ab15 100644 --- a/src/allmydata/scripts/tahoe_daemonize.py +++ b/src/allmydata/scripts/tahoe_daemonize.py @@ -132,9 +132,6 @@ class DaemonizeTheRealService(Service): except UnknownConfigError as e: sys.stderr.write("\nConfiguration error:\n{}\n\n".format(e)) reactor.stop() - except Exception as e: - sys.stderr.write("\nError building service:\n{}\n\n".format(e)) - reactor.stop() from twisted.internet import reactor reactor.callWhenRunning(start) From e9879abc938e208547646d7a07f732eb3d2a3ae5 Mon Sep 17 00:00:00 2001 From: meejah Date: Tue, 29 May 2018 17:11:28 -0600 Subject: [PATCH 05/11] add unit-tests --- src/allmydata/test/test_node.py | 39 +++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/src/allmydata/test/test_node.py b/src/allmydata/test/test_node.py index 094c3fe12..99791db26 100644 --- a/src/allmydata/test/test_node.py +++ b/src/allmydata/test/test_node.py @@ -19,6 +19,7 @@ from allmydata.introducer.server import create_introducer 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 @@ -483,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), + ) From 423208f391d804f6aad3669fa38e5ae5283342da Mon Sep 17 00:00:00 2001 From: meejah Date: Tue, 29 May 2018 18:09:23 -0600 Subject: [PATCH 06/11] add a 'tahoe run' unit-test for config errors --- src/allmydata/test/cli/test_start.py | 43 ++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/src/allmydata/test/cli/test_start.py b/src/allmydata/test/cli/test_start.py index 34ca15032..c2dd8c834 100644 --- a/src/allmydata/test/cli/test_start.py +++ b/src/allmydata/test/cli/test_start.py @@ -1,4 +1,5 @@ import os +import sys import shutil import subprocess from os.path import join @@ -207,3 +208,45 @@ class RunStartTests(unittest.TestCase): e.getvalue() ) self.assertEqual([1], exit_code) + + +class RunTests(unittest.TestCase): + + def setUp(self): + d = super(RunTests, self).setUp() + self.node_dir = self.mktemp() + os.mkdir(self.node_dir) + return d + + def test_run_invalid_config(self): + + 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) + + # should print out the collected logs and an error-code + self.assertIn( + "invalid section", + o.getvalue(), + ) + # 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)) From 0607b7331f9fcdc43d5782d4b3a36d4a2fac544f Mon Sep 17 00:00:00 2001 From: meejah Date: Wed, 27 Jun 2018 03:04:58 -0600 Subject: [PATCH 07/11] fix tests by overriding/patching reactor.stop in tests --- src/allmydata/test/cli/test_daemonize.py | 4 ++++ src/allmydata/test/cli/test_start.py | 6 +++++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/src/allmydata/test/cli/test_daemonize.py b/src/allmydata/test/cli/test_daemonize.py index 3853054e4..51d39b395 100644 --- a/src/allmydata/test/cli/test_daemonize.py +++ b/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) diff --git a/src/allmydata/test/cli/test_start.py b/src/allmydata/test/cli/test_start.py index c2dd8c834..01c8a1c60 100644 --- a/src/allmydata/test/cli/test_start.py +++ b/src/allmydata/test/cli/test_start.py @@ -218,7 +218,11 @@ class RunTests(unittest.TestCase): os.mkdir(self.node_dir) return d - def test_run_invalid_config(self): + @patch('twisted.internet.reactor') + def test_run_invalid_config(self, reactor): + def cwr(fn, *args, **kw): + fn() + reactor.callWhenRunning = cwr with open(os.path.join(self.node_dir, "client.tac"), "w") as f: f.write('test') From 03712c9ccab28b3e0b01844e6d02ed8cce2322eb Mon Sep 17 00:00:00 2001 From: meejah Date: Thu, 5 Jul 2018 10:58:08 -0600 Subject: [PATCH 08/11] add docstrings --- src/allmydata/test/cli/test_start.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/allmydata/test/cli/test_start.py b/src/allmydata/test/cli/test_start.py index 01c8a1c60..99d4c7d33 100644 --- a/src/allmydata/test/cli/test_start.py +++ b/src/allmydata/test/cli/test_start.py @@ -211,6 +211,9 @@ class RunStartTests(unittest.TestCase): class RunTests(unittest.TestCase): + """ + Tests confirming end-user behavior of CLI commands + """ def setUp(self): d = super(RunTests, self).setUp() @@ -220,6 +223,10 @@ class RunTests(unittest.TestCase): @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() reactor.callWhenRunning = cwr From 1af71e6ba39941857c541d699f09ae9ac7b07ee3 Mon Sep 17 00:00:00 2001 From: meejah Date: Thu, 5 Jul 2018 10:58:19 -0600 Subject: [PATCH 09/11] confirm .stop() called --- src/allmydata/test/cli/test_start.py | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/src/allmydata/test/cli/test_start.py b/src/allmydata/test/cli/test_start.py index 99d4c7d33..725d06a8b 100644 --- a/src/allmydata/test/cli/test_start.py +++ b/src/allmydata/test/cli/test_start.py @@ -229,7 +229,12 @@ class RunTests(unittest.TestCase): 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') @@ -251,13 +256,20 @@ class RunTests(unittest.TestCase): 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", - o.getvalue(), + 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) From 74420fb764f914ceec25c1794b562a7cb16c30d3 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Tue, 3 Jul 2018 10:57:24 -0400 Subject: [PATCH 10/11] Try to preserve the working directory --- src/allmydata/test/cli/test_start.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/allmydata/test/cli/test_start.py b/src/allmydata/test/cli/test_start.py index 725d06a8b..d1966cd33 100644 --- a/src/allmydata/test/cli/test_start.py +++ b/src/allmydata/test/cli/test_start.py @@ -5,6 +5,7 @@ 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 @@ -217,6 +218,7 @@ class RunTests(unittest.TestCase): 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 From 0cf71486a5e4245f412abc42a225ef9863d09245 Mon Sep 17 00:00:00 2001 From: meejah Date: Fri, 17 Aug 2018 16:27:28 -0600 Subject: [PATCH 11/11] add a newsfragment --- newsfragments/2935.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 newsfragments/2935.bugfix diff --git a/newsfragments/2935.bugfix b/newsfragments/2935.bugfix new file mode 100644 index 000000000..23b985259 --- /dev/null +++ b/newsfragments/2935.bugfix @@ -0,0 +1 @@ +Configuration-checking code wasn't being called due to indenting \ No newline at end of file