From f31607ded102c8fb8edca29b16af5994f78b999c Mon Sep 17 00:00:00 2001 From: sebres Date: Tue, 7 Oct 2014 14:07:50 +0200 Subject: [PATCH 01/12] test case for check the read of config files will be cached; Conflicts: fail2ban/tests/clientreadertestcase.py -- removed not needed time in imports --- fail2ban/tests/clientreadertestcase.py | 32 +++++++++++++++++++++++++- fail2ban/tests/utils.py | 4 ++++ 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/fail2ban/tests/clientreadertestcase.py b/fail2ban/tests/clientreadertestcase.py index 0ad3c66e..f65ef87a 100644 --- a/fail2ban/tests/clientreadertestcase.py +++ b/fail2ban/tests/clientreadertestcase.py @@ -21,7 +21,7 @@ __author__ = "Cyril Jaquier, Yaroslav Halchenko" __copyright__ = "Copyright (c) 2004 Cyril Jaquier, 2011-2013 Yaroslav Halchenko" __license__ = "GPL" -import os, glob, shutil, tempfile, unittest +import os, glob, shutil, tempfile, unittest, re from ..client.configreader import ConfigReader from ..client.jailreader import JailReader @@ -335,6 +335,36 @@ class FilterReaderTest(unittest.TestCase): self.assertRaises(ValueError, FilterReader.convert, filterReader) +class JailsReaderTestCache(LogCaptureTestCase): + + def testTestJailConfCache(self): + basedir = tempfile.mkdtemp("fail2ban_conf") + try: + shutil.rmtree(basedir) + shutil.copytree(CONFIG_DIR, basedir) + shutil.copy(CONFIG_DIR + '/jail.conf', basedir + '/jail.local') + shutil.copy(CONFIG_DIR + '/fail2ban.conf', basedir + '/fail2ban.local') + + # read whole configuration like a file2ban-client ... + configurator = Configurator() + configurator.setBaseDir(basedir) + configurator.readEarly() + configurator.getEarlyOptions() + configurator.readAll() + # from here we test a cache : + self.assertTrue(configurator.getOptions(None)) + cnt = 0 + for s in self.getLog().rsplit('\n'): + if re.match(r"^Reading files: .*jail.local", s): + cnt += 1 + # if cnt > 2: + # self.printLog() + self.assertFalse(cnt > 2, "Too many times reading of config files, cnt = %s" % cnt) + self.assertFalse(cnt <= 0) + finally: + shutil.rmtree(basedir) + + class JailsReaderTest(LogCaptureTestCase): def testProvidingBadBasedir(self): diff --git a/fail2ban/tests/utils.py b/fail2ban/tests/utils.py index 912c5a90..ad504703 100644 --- a/fail2ban/tests/utils.py +++ b/fail2ban/tests/utils.py @@ -111,6 +111,7 @@ def gatherTests(regexps=None, no_network=False): tests.addTest(unittest.makeSuite(clientreadertestcase.JailReaderTest)) tests.addTest(unittest.makeSuite(clientreadertestcase.FilterReaderTest)) tests.addTest(unittest.makeSuite(clientreadertestcase.JailsReaderTest)) + tests.addTest(unittest.makeSuite(clientreadertestcase.JailsReaderTestCache)) # CSocket and AsyncServer tests.addTest(unittest.makeSuite(sockettestcase.Socket)) # Misc helpers @@ -216,5 +217,8 @@ class LogCaptureTestCase(unittest.TestCase): def _is_logged(self, s): return s in self._log.getvalue() + def getLog(self): + return self._log.getvalue() + def printLog(self): print(self._log.getvalue()) From 0c5f11079c6d1cb817a4218f707be37cd1897398 Mon Sep 17 00:00:00 2001 From: Yaroslav Halchenko Date: Thu, 9 Oct 2014 10:46:17 -0400 Subject: [PATCH 02/12] ENH: keep spitting out logging to the screen in LogCaptureTestCases if HEAVYDEBUG --- fail2ban/tests/utils.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/fail2ban/tests/utils.py b/fail2ban/tests/utils.py index ad504703..b56c0988 100644 --- a/fail2ban/tests/utils.py +++ b/fail2ban/tests/utils.py @@ -25,6 +25,7 @@ __license__ = "GPL" import logging import os import re +import sys import time import unittest from StringIO import StringIO @@ -205,6 +206,8 @@ class LogCaptureTestCase(unittest.TestCase): # Let's log everything into a string self._log = StringIO() logSys.handlers = [logging.StreamHandler(self._log)] + if self._old_level < logging.DEBUG: # so if HEAVYDEBUG etc -- show them! + logSys.handlers += self._old_handlers logSys.setLevel(getattr(logging, 'DEBUG')) def tearDown(self): From b62ce14ccd4dd0307264493122caac824c0a9e49 Mon Sep 17 00:00:00 2001 From: sebres Date: Thu, 9 Oct 2014 18:00:45 +0200 Subject: [PATCH 03/12] Partially merge remote-tracking from 'sebres:cache-config-read-820': test cases extended, configurator.py adapted for test case. --- fail2ban/client/configurator.py | 4 +- fail2ban/tests/clientreadertestcase.py | 66 +++++++++++++++++++------- 2 files changed, 51 insertions(+), 19 deletions(-) diff --git a/fail2ban/client/configurator.py b/fail2ban/client/configurator.py index 0dd9f955..a29fe94d 100644 --- a/fail2ban/client/configurator.py +++ b/fail2ban/client/configurator.py @@ -33,11 +33,11 @@ logSys = getLogger(__name__) class Configurator: - def __init__(self): + def __init__(self, force_enable=False): self.__settings = dict() self.__streams = dict() self.__fail2ban = Fail2banReader() - self.__jails = JailsReader() + self.__jails = JailsReader(force_enable=force_enable) def setBaseDir(self, folderName): self.__fail2ban.setBaseDir(folderName) diff --git a/fail2ban/tests/clientreadertestcase.py b/fail2ban/tests/clientreadertestcase.py index f65ef87a..a24856da 100644 --- a/fail2ban/tests/clientreadertestcase.py +++ b/fail2ban/tests/clientreadertestcase.py @@ -21,7 +21,7 @@ __author__ = "Cyril Jaquier, Yaroslav Halchenko" __copyright__ = "Copyright (c) 2004 Cyril Jaquier, 2011-2013 Yaroslav Halchenko" __license__ = "GPL" -import os, glob, shutil, tempfile, unittest, re +import os, glob, shutil, tempfile, unittest, time, re from ..client.configreader import ConfigReader from ..client.jailreader import JailReader @@ -39,6 +39,8 @@ STOCK = os.path.exists(os.path.join('config','fail2ban.conf')) IMPERFECT_CONFIG = os.path.join(os.path.dirname(__file__), 'config') +LAST_WRITE_TIME = 0 + class ConfigReaderTest(unittest.TestCase): def setUp(self): @@ -57,7 +59,8 @@ class ConfigReaderTest(unittest.TestCase): d_ = os.path.join(self.d, d) if not os.path.exists(d_): os.makedirs(d_) - f = open("%s/%s" % (self.d, fname), "w") + fname = "%s/%s" % (self.d, fname) + f = open(fname, "w") if value is not None: f.write(""" [section] @@ -66,6 +69,14 @@ option = %s if content is not None: f.write(content) f.close() + # set modification time to another second to revalidate cache (if milliseconds not supported) : + global LAST_WRITE_TIME + mtime = os.path.getmtime(fname) + if LAST_WRITE_TIME == mtime: + mtime += 1 + os.utime(fname, (mtime, mtime)) + LAST_WRITE_TIME = mtime + def _remove(self, fname): os.unlink("%s/%s" % (self.d, fname)) @@ -91,7 +102,6 @@ option = %s # raise unittest.SkipTest("Skipping on %s -- access rights are not enforced" % platform) pass - def testOptionalDotDDir(self): self.assertFalse(self.c.read('c')) # nothing is there yet self._write("c.conf", "1") @@ -337,6 +347,23 @@ class FilterReaderTest(unittest.TestCase): class JailsReaderTestCache(LogCaptureTestCase): + def _readWholeConf(self, basedir, force_enable=False): + # read whole configuration like a file2ban-client ... + configurator = Configurator(force_enable=force_enable) + configurator.setBaseDir(basedir) + configurator.readEarly() + configurator.getEarlyOptions() + configurator.readAll() + # from here we test a cache with all includes / before / after : + self.assertTrue(configurator.getOptions(None)) + + def _getLoggedReadCount(self, filematch): + cnt = 0 + for s in self.getLog().rsplit('\n'): + if re.match(r"^Reading files?: .*/"+filematch, s): + cnt += 1 + return cnt + def testTestJailConfCache(self): basedir = tempfile.mkdtemp("fail2ban_conf") try: @@ -346,21 +373,26 @@ class JailsReaderTestCache(LogCaptureTestCase): shutil.copy(CONFIG_DIR + '/fail2ban.conf', basedir + '/fail2ban.local') # read whole configuration like a file2ban-client ... - configurator = Configurator() - configurator.setBaseDir(basedir) - configurator.readEarly() - configurator.getEarlyOptions() - configurator.readAll() - # from here we test a cache : - self.assertTrue(configurator.getOptions(None)) - cnt = 0 - for s in self.getLog().rsplit('\n'): - if re.match(r"^Reading files: .*jail.local", s): - cnt += 1 - # if cnt > 2: + self._readWholeConf(basedir) + # how many times jail.local was read: + cnt = self._getLoggedReadCount('jail.local') + # if cnt > 1: # self.printLog() - self.assertFalse(cnt > 2, "Too many times reading of config files, cnt = %s" % cnt) - self.assertFalse(cnt <= 0) + self.assertTrue(cnt == 1, "Unexpected count by reading of jail files, cnt = %s" % cnt) + + # read whole configuration like a file2ban-client, again ... + # but this time force enable all jails, to check filter and action cached also: + self._readWholeConf(basedir, force_enable=True) + cnt = self._getLoggedReadCount(r'jail\.local') + # still one (no more reads): + self.assertTrue(cnt == 1, "Unexpected count by second reading of jail files, cnt = %s" % cnt) + + # same with filter: + cnt = self._getLoggedReadCount(r'filter\.d/common\.conf') + self.assertTrue(cnt == 1, "Unexpected count by reading of filter files, cnt = %s" % cnt) + # same with action: + cnt = self._getLoggedReadCount(r'action\.d/iptables-common\.conf') + self.assertTrue(cnt == 1, "Unexpected count by reading of action files, cnt = %s" % cnt) finally: shutil.rmtree(basedir) From f67053c2ece28888f38c4f19d19c92e4cbfa91a1 Mon Sep 17 00:00:00 2001 From: sebres Date: Thu, 9 Oct 2014 19:01:49 +0200 Subject: [PATCH 04/12] ConfigReader/ConfigWrapper renamed as suggested from @yarikoptic; + code clarifying (suggested also); --- fail2ban/client/configparserinc.py | 6 ++--- fail2ban/client/configreader.py | 33 ++++++++++++++++---------- fail2ban/client/fail2banreader.py | 12 +++++----- fail2ban/client/jailreader.py | 12 +++++----- fail2ban/client/jailsreader.py | 10 ++++---- fail2ban/tests/clientreadertestcase.py | 6 ++--- 6 files changed, 44 insertions(+), 35 deletions(-) diff --git a/fail2ban/client/configparserinc.py b/fail2ban/client/configparserinc.py index 31945849..25e15213 100644 --- a/fail2ban/client/configparserinc.py +++ b/fail2ban/client/configparserinc.py @@ -92,7 +92,7 @@ class SafeConfigParserWithIncludes(object): return orig_attr @staticmethod - def _resource_mtime(resource): + def _get_resource_fingerprint(resource): mt = [] dirnames = [] for filename in resource: @@ -126,7 +126,7 @@ class SafeConfigParserWithIncludes(object): # check cache hashv = '\x01'.join(fileNamesFull) cr, ret, mtime = SCPWI.CFG_CACHE.get(hashv, (None, False, 0)) - curmt = SCPWI._resource_mtime(fileNamesFull) + curmt = SCPWI._get_resource_fingerprint(fileNamesFull) if cr is not None and mtime == curmt: self.__cr = cr logSys.debug("Cached config files: %s", resource) @@ -160,7 +160,7 @@ class SafeConfigParserWithIncludes(object): # check cache hashv = '///'.join(resources) cinc, mtime = SCPWI.CFG_INC_CACHE.get(hashv, (None, 0)) - curmt = SCPWI._resource_mtime(resources) + curmt = SCPWI._get_resource_fingerprint(resources) if cinc is not None and mtime == curmt: return cinc diff --git a/fail2ban/client/configreader.py b/fail2ban/client/configreader.py index d1cc7924..a3b5645e 100644 --- a/fail2ban/client/configreader.py +++ b/fail2ban/client/configreader.py @@ -32,9 +32,13 @@ from ..helpers import getLogger # Gets the instance of the logger. logSys = getLogger(__name__) -logLevel = 6 +_logLevel = 6 -class ConfigWrapper(): +class ConfigReader(): + """Config reader class (previously ConfigWrapper). + + Automatically shares or use already shared instance of ConfigReaderUnshared. + """ def __init__(self, use_config=None, share_config=None, **kwargs): # use given shared config if possible (see read): @@ -48,7 +52,7 @@ class ConfigWrapper(): self._cfg_share = share_config self._cfg_share_kwargs = kwargs else: - self._cfg = ConfigReader(**kwargs) + self._cfg = ConfigReaderUnshared(**kwargs) def setBaseDir(self, basedir): self._cfg.setBaseDir(basedir) @@ -61,7 +65,7 @@ class ConfigWrapper(): if not self._cfg and self._cfg_share is not None: self._cfg = self._cfg_share.get(name) if not self._cfg: - self._cfg = ConfigReader(**self._cfg_share_kwargs) + self._cfg = ConfigReaderUnshared(**self._cfg_share_kwargs) self._cfg_share[name] = self._cfg # performance feature - read once if using shared config reader: rc = self._cfg.read_cfg_files @@ -92,7 +96,12 @@ class ConfigWrapper(): return self._cfg.getOptions(*args, **kwargs) -class ConfigReader(SafeConfigParserWithIncludes): +class ConfigReaderUnshared(SafeConfigParserWithIncludes): + """Unshared config reader (previously ConfigReader). + + Does not use this class (internal not shared/cached represenation). + Use ConfigReader instead. + """ DEFAULT_BASEDIR = '/etc/fail2ban' @@ -103,7 +112,7 @@ class ConfigReader(SafeConfigParserWithIncludes): def setBaseDir(self, basedir): if basedir is None: - basedir = ConfigReader.DEFAULT_BASEDIR # stock system location + basedir = ConfigReaderUnshared.DEFAULT_BASEDIR # stock system location self._basedir = basedir.rstrip('/') def getBaseDir(self): @@ -181,15 +190,15 @@ class ConfigReader(SafeConfigParserWithIncludes): logSys.warning("'%s' not defined in '%s'. Using default one: %r" % (option[1], sec, option[2])) values[option[1]] = option[2] - elif logSys.getEffectiveLevel() <= logLevel: - logSys.log(logLevel, "Non essential option '%s' not defined in '%s'.", option[1], sec) + elif logSys.getEffectiveLevel() <= _logLevel: + logSys.log(_logLevel, "Non essential option '%s' not defined in '%s'.", option[1], sec) except ValueError: logSys.warning("Wrong value for '" + option[1] + "' in '" + sec + "'. Using default one: '" + `option[2]` + "'") values[option[1]] = option[2] return values -class DefinitionInitConfigReader(ConfigWrapper): +class DefinitionInitConfigReader(ConfigReader): """Config reader for files with options grouped in [Definition] and [Init] sections. @@ -201,7 +210,7 @@ class DefinitionInitConfigReader(ConfigWrapper): _configOpts = [] def __init__(self, file_, jailName, initOpts, **kwargs): - ConfigWrapper.__init__(self, **kwargs) + ConfigReader.__init__(self, **kwargs) self.setFile(file_) self.setJailName(jailName) self._initOpts = initOpts @@ -220,14 +229,14 @@ class DefinitionInitConfigReader(ConfigWrapper): return self._jailName def read(self): - return ConfigWrapper.read(self, self._file) + return ConfigReader.read(self, self._file) # needed for fail2ban-regex that doesn't need fancy directories def readexplicit(self): return SafeConfigParserWithIncludes.read(self, self._file) def getOptions(self, pOpts): - self._opts = ConfigWrapper.getOptions( + self._opts = ConfigReader.getOptions( self, "Definition", self._configOpts, pOpts) if self.has_section("Init"): diff --git a/fail2ban/client/fail2banreader.py b/fail2ban/client/fail2banreader.py index a151ebf0..4d46c156 100644 --- a/fail2ban/client/fail2banreader.py +++ b/fail2ban/client/fail2banreader.py @@ -24,32 +24,32 @@ __author__ = "Cyril Jaquier" __copyright__ = "Copyright (c) 2004 Cyril Jaquier" __license__ = "GPL" -from .configreader import ConfigWrapper +from .configreader import ConfigReader from ..helpers import getLogger # Gets the instance of the logger. logSys = getLogger(__name__) -class Fail2banReader(ConfigWrapper): +class Fail2banReader(ConfigReader): def __init__(self, **kwargs): self.__opts = None - ConfigWrapper.__init__(self, **kwargs) + ConfigReader.__init__(self, **kwargs) def read(self): - ConfigWrapper.read(self, "fail2ban") + ConfigReader.read(self, "fail2ban") def getEarlyOptions(self): opts = [["string", "socket", "/var/run/fail2ban/fail2ban.sock"], ["string", "pidfile", "/var/run/fail2ban/fail2ban.pid"]] - return ConfigWrapper.getOptions(self, "Definition", opts) + return ConfigReader.getOptions(self, "Definition", opts) def getOptions(self): opts = [["string", "loglevel", "INFO" ], ["string", "logtarget", "STDERR"], ["string", "dbfile", "/var/lib/fail2ban/fail2ban.sqlite3"], ["int", "dbpurgeage", 86400]] - self.__opts = ConfigWrapper.getOptions(self, "Definition", opts) + self.__opts = ConfigReader.getOptions(self, "Definition", opts) def convert(self): stream = list() diff --git a/fail2ban/client/jailreader.py b/fail2ban/client/jailreader.py index a3cc939c..e4394e59 100644 --- a/fail2ban/client/jailreader.py +++ b/fail2ban/client/jailreader.py @@ -27,7 +27,7 @@ __license__ = "GPL" import re, glob, os.path import json -from .configreader import ConfigReader, ConfigWrapper +from .configreader import ConfigReaderUnshared, ConfigReader from .filterreader import FilterReader from .actionreader import ActionReader from ..helpers import getLogger @@ -35,7 +35,7 @@ from ..helpers import getLogger # Gets the instance of the logger. logSys = getLogger(__name__) -class JailReader(ConfigWrapper): +class JailReader(ConfigReader): optionCRE = re.compile("^((?:\w|-|_|\.)+)(?:\[(.*)\])?$") optionExtractRE = re.compile( @@ -43,7 +43,7 @@ class JailReader(ConfigWrapper): def __init__(self, name, force_enable=False, cfg_share=None, **kwargs): # use shared config if possible: - ConfigWrapper.__init__(self, **kwargs) + ConfigReader.__init__(self, **kwargs) self.__name = name self.__filter = None self.__force_enable = force_enable @@ -62,7 +62,7 @@ class JailReader(ConfigWrapper): return self.__name def read(self): - out = ConfigWrapper.read(self, "jail") + out = ConfigReader.read(self, "jail") # Before returning -- verify that requested section # exists at all if not (self.__name in self.sections()): @@ -103,7 +103,7 @@ class JailReader(ConfigWrapper): ["string", "ignoreip", None], ["string", "filter", ""], ["string", "action", ""]] - self.__opts = ConfigWrapper.getOptions(self, self.__name, opts) + self.__opts = ConfigReader.getOptions(self, self.__name, opts) if not self.__opts: return False @@ -215,7 +215,7 @@ class JailReader(ConfigWrapper): if self.__filter: stream.extend(self.__filter.convert()) for action in self.__actions: - if isinstance(action, (ConfigReader, ConfigWrapper)): + if isinstance(action, (ConfigReaderUnshared, ConfigReader)): stream.extend(action.convert()) else: stream.append(action) diff --git a/fail2ban/client/jailsreader.py b/fail2ban/client/jailsreader.py index 37d37e01..3bb713f6 100644 --- a/fail2ban/client/jailsreader.py +++ b/fail2ban/client/jailsreader.py @@ -24,14 +24,14 @@ __author__ = "Cyril Jaquier" __copyright__ = "Copyright (c) 2004 Cyril Jaquier" __license__ = "GPL" -from .configreader import ConfigReader, ConfigWrapper +from .configreader import ConfigReader from .jailreader import JailReader from ..helpers import getLogger # Gets the instance of the logger. logSys = getLogger(__name__) -class JailsReader(ConfigWrapper): +class JailsReader(ConfigReader): def __init__(self, force_enable=False, **kwargs): """ @@ -42,7 +42,7 @@ class JailsReader(ConfigWrapper): It is for internal use """ # use shared config if possible: - ConfigWrapper.__init__(self, **kwargs) + ConfigReader.__init__(self, **kwargs) self.__cfg_share = dict() self.__jails = list() self.__force_enable = force_enable @@ -52,13 +52,13 @@ class JailsReader(ConfigWrapper): return self.__jails def read(self): - return ConfigWrapper.read(self, "jail") + return ConfigReader.read(self, "jail") def getOptions(self, section=None): """Reads configuration for jail(s) and adds enabled jails to __jails """ opts = [] - self.__opts = ConfigWrapper.getOptions(self, "Definition", opts) + self.__opts = ConfigReader.getOptions(self, "Definition", opts) if section is None: sections = self.sections() diff --git a/fail2ban/tests/clientreadertestcase.py b/fail2ban/tests/clientreadertestcase.py index a24856da..0aaa8890 100644 --- a/fail2ban/tests/clientreadertestcase.py +++ b/fail2ban/tests/clientreadertestcase.py @@ -21,9 +21,9 @@ __author__ = "Cyril Jaquier, Yaroslav Halchenko" __copyright__ = "Copyright (c) 2004 Cyril Jaquier, 2011-2013 Yaroslav Halchenko" __license__ = "GPL" -import os, glob, shutil, tempfile, unittest, time, re +import os, glob, shutil, tempfile, unittest, re -from ..client.configreader import ConfigReader +from ..client.configreader import ConfigReaderUnshared from ..client.jailreader import JailReader from ..client.filterreader import FilterReader from ..client.jailsreader import JailsReader @@ -46,7 +46,7 @@ class ConfigReaderTest(unittest.TestCase): def setUp(self): """Call before every test case.""" self.d = tempfile.mkdtemp(prefix="f2b-temp") - self.c = ConfigReader(basedir=self.d) + self.c = ConfigReaderUnshared(basedir=self.d) def tearDown(self): """Call after every test case.""" From 37952ab75f4cb34ffa9180a5a35344a2dfa53475 Mon Sep 17 00:00:00 2001 From: sebres Date: Thu, 9 Oct 2014 19:51:53 +0200 Subject: [PATCH 05/12] code review --- fail2ban/client/configreader.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/fail2ban/client/configreader.py b/fail2ban/client/configreader.py index a3b5645e..096f8e78 100644 --- a/fail2ban/client/configreader.py +++ b/fail2ban/client/configreader.py @@ -35,9 +35,9 @@ logSys = getLogger(__name__) _logLevel = 6 class ConfigReader(): - """Config reader class (previously ConfigWrapper). + """Generic config reader class. - Automatically shares or use already shared instance of ConfigReaderUnshared. + A caching adapter which automatically reuses already shared configuration. """ def __init__(self, use_config=None, share_config=None, **kwargs): @@ -200,12 +200,12 @@ class ConfigReaderUnshared(SafeConfigParserWithIncludes): class DefinitionInitConfigReader(ConfigReader): """Config reader for files with options grouped in [Definition] and - [Init] sections. + [Init] sections. - Is a base class for readers of filters and actions, where definitions - in jails might provide custom values for options defined in [Init] - section. - """ + Is a base class for readers of filters and actions, where definitions + in jails might provide custom values for options defined in [Init] + section. + """ _configOpts = [] From c35b4b24d21445690e4c51a335adf270136bd7d3 Mon Sep 17 00:00:00 2001 From: sebres Date: Fri, 10 Oct 2014 02:10:13 +0200 Subject: [PATCH 06/12] rewritten caching resp. sharing of ConfigReader and SafeConfigParserWithIncludes (v.2, first and second level cache, without fingerprinting etc.); --- fail2ban/client/configparserinc.py | 296 +++++++++++-------------- fail2ban/client/configreader.py | 43 ++-- fail2ban/client/configurator.py | 6 +- fail2ban/client/jailreader.py | 7 +- fail2ban/client/jailsreader.py | 3 +- fail2ban/tests/clientreadertestcase.py | 27 +-- 6 files changed, 178 insertions(+), 204 deletions(-) diff --git a/fail2ban/client/configparserinc.py b/fail2ban/client/configparserinc.py index 25e15213..d6ad4e34 100644 --- a/fail2ban/client/configparserinc.py +++ b/fail2ban/client/configparserinc.py @@ -65,136 +65,7 @@ logSys = getLogger(__name__) __all__ = ['SafeConfigParserWithIncludes'] -class SafeConfigParserWithIncludes(object): - - SECTION_NAME = "INCLUDES" - CFG_CACHE = {} - CFG_INC_CACHE = {} - CFG_EMPY_CFG = None - - def __init__(self): - self.__cr = None - - def __check_read(self, attr): - if self.__cr is None: - # raise RuntimeError("Access to wrapped attribute \"%s\" before read call" % attr) - if SafeConfigParserWithIncludes.CFG_EMPY_CFG is None: - SafeConfigParserWithIncludes.CFG_EMPY_CFG = _SafeConfigParserWithIncludes() - self.__cr = SafeConfigParserWithIncludes.CFG_EMPY_CFG - - def __getattr__(self,attr): - # check we access local implementation - try: - orig_attr = self.__getattribute__(attr) - except AttributeError: - self.__check_read(attr) - orig_attr = self.__cr.__getattribute__(attr) - return orig_attr - - @staticmethod - def _get_resource_fingerprint(resource): - mt = [] - dirnames = [] - for filename in resource: - if os.path.exists(filename): - s = os.stat(filename) - mt.append(s.st_mtime) - mt.append(s.st_mode) - mt.append(s.st_size) - dirname = os.path.dirname(filename) - if dirname not in dirnames: - dirnames.append(dirname) - for dirname in dirnames: - if os.path.exists(dirname): - s = os.stat(dirname) - mt.append(s.st_mtime) - mt.append(s.st_mode) - mt.append(s.st_size) - return mt - - def read(self, resource, get_includes=True, log_info=None): - SCPWI = SafeConfigParserWithIncludes - # check includes : - fileNamesFull = [] - if not isinstance(resource, list): - resource = [ resource ] - if get_includes: - for filename in resource: - fileNamesFull += SCPWI.getIncludes(filename) - else: - fileNamesFull = resource - # check cache - hashv = '\x01'.join(fileNamesFull) - cr, ret, mtime = SCPWI.CFG_CACHE.get(hashv, (None, False, 0)) - curmt = SCPWI._get_resource_fingerprint(fileNamesFull) - if cr is not None and mtime == curmt: - self.__cr = cr - logSys.debug("Cached config files: %s", resource) - #logSys.debug("Cached config files: %s", fileNamesFull) - return ret - # not yet in cache - create/read and add to cache: - if log_info is not None: - logSys.info(*log_info) - cr = _SafeConfigParserWithIncludes() - ret = cr.read(fileNamesFull) - SCPWI.CFG_CACHE[hashv] = (cr, ret, curmt) - self.__cr = cr - return ret - - def getOptions(self, *args, **kwargs): - self.__check_read('getOptions') - return self.__cr.getOptions(*args, **kwargs) - - @staticmethod - def getIncludes(resource, seen = []): - """ - Given 1 config resource returns list of included files - (recursively) with the original one as well - Simple loops are taken care about - """ - - # Use a short class name ;) - SCPWI = SafeConfigParserWithIncludes - - resources = seen + [resource] - # check cache - hashv = '///'.join(resources) - cinc, mtime = SCPWI.CFG_INC_CACHE.get(hashv, (None, 0)) - curmt = SCPWI._get_resource_fingerprint(resources) - if cinc is not None and mtime == curmt: - return cinc - - parser = SCPWI() - try: - # read without includes - parser.read(resource, get_includes=False) - except UnicodeDecodeError, e: - logSys.error("Error decoding config file '%s': %s" % (resource, e)) - return [] - - resourceDir = os.path.dirname(resource) - - newFiles = [ ('before', []), ('after', []) ] - if SCPWI.SECTION_NAME in parser.sections(): - for option_name, option_list in newFiles: - if option_name in parser.options(SCPWI.SECTION_NAME): - newResources = parser.get(SCPWI.SECTION_NAME, option_name) - for newResource in newResources.split('\n'): - if os.path.isabs(newResource): - r = newResource - else: - r = os.path.join(resourceDir, newResource) - if r in seen: - continue - option_list += SCPWI.getIncludes(r, resources) - # combine lists - cinc = newFiles[0][1] + [resource] + newFiles[1][1] - # cache and return : - SCPWI.CFG_INC_CACHE[hashv] = (cinc, curmt) - return cinc - #print "Includes list for " + resource + " is " + `resources` - -class _SafeConfigParserWithIncludes(SafeConfigParser, object): +class SafeConfigParserWithIncludes(SafeConfigParser): """ Class adds functionality to SafeConfigParser to handle included other configuration files (or may be urls, whatever in the future) @@ -223,14 +94,99 @@ after = 1.conf """ + SECTION_NAME = "INCLUDES" + if sys.version_info >= (3,2): # overload constructor only for fancy new Python3's - def __init__(self, *args, **kwargs): + def __init__(self, share_config=None, *args, **kwargs): kwargs = kwargs.copy() kwargs['interpolation'] = BasicInterpolationWithName() kwargs['inline_comment_prefixes'] = ";" - super(_SafeConfigParserWithIncludes, self).__init__( + super(SafeConfigParserWithIncludes, self).__init__( *args, **kwargs) + self._cfg_share = share_config + + else: + def __init__(self, share_config=None, *args, **kwargs): + SafeConfigParser.__init__(self, *args, **kwargs) + self._cfg_share = share_config + + @property + def share_config(self): + return self._cfg_share + + def _getSharedSCPWI(self, filename): + SCPWI = SafeConfigParserWithIncludes + # read single one, add to return list, use sharing if possible: + if self._cfg_share: + # cache/share each file as include (ex: filter.d/common could be included in each filter config): + hashv = 'inc:'+(filename if not isinstance(filename, list) else '\x01'.join(filename)) + cfg, i = self._cfg_share.get(hashv, (None, None)) + if cfg is None: + cfg = SCPWI(share_config=self._cfg_share) + i = cfg.read(filename, get_includes=False) + self._cfg_share[hashv] = (cfg, i) + else: + logSys.debug(" Shared file: %s", filename) + else: + # don't have sharing: + cfg = SCPWI() + i = cfg.read(filename, get_includes=False) + return (cfg, i) + + def _getIncludes(self, filenames, seen=[]): + if not isinstance(filenames, list): + filenames = [ filenames ] + # retrieve or cache include paths: + if self._cfg_share: + # cache/share include list: + hashv = 'inc-path:'+('\x01'.join(filenames)) + fileNamesFull = self._cfg_share.get(hashv) + if fileNamesFull is None: + fileNamesFull = [] + for filename in filenames: + fileNamesFull += self.__getIncludesUncached(filename, seen) + self._cfg_share[hashv] = fileNamesFull + return fileNamesFull + # don't have sharing: + fileNamesFull = [] + for filename in filenames: + fileNamesFull += self.__getIncludesUncached(filename, seen) + return fileNamesFull + + def __getIncludesUncached(self, resource, seen=[]): + """ + Given 1 config resource returns list of included files + (recursively) with the original one as well + Simple loops are taken care about + """ + SCPWI = SafeConfigParserWithIncludes + try: + parser, i = self._getSharedSCPWI(resource) + if not i: + return [] + except UnicodeDecodeError, e: + logSys.error("Error decoding config file '%s': %s" % (resource, e)) + return [] + + resourceDir = os.path.dirname(resource) + + newFiles = [ ('before', []), ('after', []) ] + if SCPWI.SECTION_NAME in parser.sections(): + for option_name, option_list in newFiles: + if option_name in parser.options(SCPWI.SECTION_NAME): + newResources = parser.get(SCPWI.SECTION_NAME, option_name) + for newResource in newResources.split('\n'): + if os.path.isabs(newResource): + r = newResource + else: + r = os.path.join(resourceDir, newResource) + if r in seen: + continue + s = seen + [resource] + option_list += self._getIncludes(r, s) + # combine lists + return newFiles[0][1] + [resource] + newFiles[1][1] def get_defaults(self): return self._defaults @@ -238,38 +194,52 @@ after = 1.conf def get_sections(self): return self._sections - def read(self, filenames): + def read(self, filenames, get_includes=True, log_info=None): if not isinstance(filenames, list): filenames = [ filenames ] - if len(filenames) > 1: - # read multiple configs: - ret = [] - alld = self.get_defaults() - alls = self.get_sections() - for filename in filenames: - # read single one, add to return list: - cfg = SafeConfigParserWithIncludes() - i = cfg.read(filename, get_includes=False) - if i: - ret += i - # merge defaults and all sections to self: - alld.update(cfg.get_defaults()) - for n, s in cfg.get_sections().iteritems(): - if isinstance(s, dict): - s2 = alls.get(n) - if isinstance(s2, dict): - s2.update(s) - else: - alls[n] = s.copy() - else: - alls[n] = s - - return ret - - # read one config : - logSys.debug("Reading file: %s", filenames[0]) - if sys.version_info >= (3,2): # pragma: no cover - return SafeConfigParser.read(self, filenames, encoding='utf-8') + # retrieve (and cache) includes: + fileNamesFull = [] + if get_includes: + fileNamesFull += self._getIncludes(filenames) else: - return SafeConfigParser.read(self, filenames) + fileNamesFull = filenames + + if self._cfg_share is not None: + logSys.debug(" Sharing files: %s", fileNamesFull) + + if len(fileNamesFull) > 1: + # read multiple configs: + ret = [] + alld = self.get_defaults() + alls = self.get_sections() + for filename in fileNamesFull: + # read single one, add to return list, use sharing if possible: + cfg, i = self._getSharedSCPWI(filename) + if i: + ret += i + # merge defaults and all sections to self: + alld.update(cfg.get_defaults()) + for n, s in cfg.get_sections().iteritems(): + if isinstance(s, dict): + s2 = alls.get(n) + if isinstance(s2, dict): + s2.update(s) + else: + alls[n] = s.copy() + else: + alls[n] = s + + return ret + + # read one config : + logSys.debug(" Reading file: %s", fileNamesFull[0]) + else: + # don't have sharing - read one or multiple at once: + logSys.debug(" Reading files: %s", fileNamesFull) + + # read file(s) : + if sys.version_info >= (3,2): # pragma: no cover + return SafeConfigParser.read(self, fileNamesFull, encoding='utf-8') + else: + return SafeConfigParser.read(self, fileNamesFull) diff --git a/fail2ban/client/configreader.py b/fail2ban/client/configreader.py index 096f8e78..657800fa 100644 --- a/fail2ban/client/configreader.py +++ b/fail2ban/client/configreader.py @@ -46,26 +46,38 @@ class ConfigReader(): self._cfg = None if use_config is not None: self._cfg = use_config - else: - # share config if possible: - if share_config is not None: - self._cfg_share = share_config - self._cfg_share_kwargs = kwargs - else: - self._cfg = ConfigReaderUnshared(**kwargs) + # share config if possible: + if share_config is not None: + self._cfg_share = share_config + self._cfg_share_kwargs = kwargs + self._cfg_share_basedir = None + elif self._cfg is None: + self._cfg = ConfigReaderUnshared(**kwargs) def setBaseDir(self, basedir): - self._cfg.setBaseDir(basedir) + if self._cfg: + self._cfg.setBaseDir(basedir) + else: + self._cfg_share_basedir = basedir def getBaseDir(self): - return self._cfg.getBaseDir() + if self._cfg: + return self._cfg.getBaseDir() + else: + return self._cfg_share_basedir + + @property + def share_config(self): + return self._cfg_share def read(self, name, once=True): # shared ? if not self._cfg and self._cfg_share is not None: self._cfg = self._cfg_share.get(name) if not self._cfg: - self._cfg = ConfigReaderUnshared(**self._cfg_share_kwargs) + self._cfg = ConfigReaderUnshared(share_config=self._cfg_share, **self._cfg_share_kwargs) + if self._cfg_share_basedir is not None: + self._cfg.setBaseDir(self._cfg_share_basedir) self._cfg_share[name] = self._cfg # performance feature - read once if using shared config reader: rc = self._cfg.read_cfg_files @@ -73,6 +85,8 @@ class ConfigReader(): return rc.get(name) # read: + if self._cfg_share is not None: + logSys.info("Sharing configs for %s under %s ", name, self._cfg.getBaseDir()) ret = self._cfg.read(name) # save already read: @@ -105,8 +119,8 @@ class ConfigReaderUnshared(SafeConfigParserWithIncludes): DEFAULT_BASEDIR = '/etc/fail2ban' - def __init__(self, basedir=None): - SafeConfigParserWithIncludes.__init__(self) + def __init__(self, basedir=None, *args, **kwargs): + SafeConfigParserWithIncludes.__init__(self, *args, **kwargs) self.read_cfg_files = dict() self.setBaseDir(basedir) @@ -123,7 +137,7 @@ class ConfigReaderUnshared(SafeConfigParserWithIncludes): raise ValueError("Base configuration directory %s does not exist " % self._basedir) basename = os.path.join(self._basedir, filename) - logSys.debug("Reading configs for %s under %s " , filename, self._basedir) + logSys.info("Reading configs for %s under %s " , filename, self._basedir) config_files = [ basename + ".conf" ] # possible further customizations under a .conf.d directory @@ -140,8 +154,7 @@ class ConfigReaderUnshared(SafeConfigParserWithIncludes): if len(config_files): # at least one config exists and accessible logSys.debug("Reading config files: %s", ', '.join(config_files)) - config_files_read = SafeConfigParserWithIncludes.read(self, config_files, - log_info=("Cache configs for %s under %s " , filename, self._basedir)) + config_files_read = SafeConfigParserWithIncludes.read(self, config_files) missed = [ cf for cf in config_files if cf not in config_files_read ] if missed: logSys.error("Could not read config files: %s", ', '.join(missed)) diff --git a/fail2ban/client/configurator.py b/fail2ban/client/configurator.py index a29fe94d..4c0c0428 100644 --- a/fail2ban/client/configurator.py +++ b/fail2ban/client/configurator.py @@ -33,11 +33,11 @@ logSys = getLogger(__name__) class Configurator: - def __init__(self, force_enable=False): + def __init__(self, force_enable=False, share_config=None): self.__settings = dict() self.__streams = dict() - self.__fail2ban = Fail2banReader() - self.__jails = JailsReader(force_enable=force_enable) + self.__fail2ban = Fail2banReader(share_config=share_config) + self.__jails = JailsReader(force_enable=force_enable, share_config=share_config) def setBaseDir(self, folderName): self.__fail2ban.setBaseDir(folderName) diff --git a/fail2ban/client/jailreader.py b/fail2ban/client/jailreader.py index e4394e59..99891c0f 100644 --- a/fail2ban/client/jailreader.py +++ b/fail2ban/client/jailreader.py @@ -41,13 +41,12 @@ class JailReader(ConfigReader): optionExtractRE = re.compile( r'([\w\-_\.]+)=(?:"([^"]*)"|\'([^\']*)\'|([^,]*))(?:,|$)') - def __init__(self, name, force_enable=False, cfg_share=None, **kwargs): + def __init__(self, name, force_enable=False, **kwargs): # use shared config if possible: ConfigReader.__init__(self, **kwargs) self.__name = name self.__filter = None self.__force_enable = force_enable - self.__cfg_share = cfg_share self.__actions = list() self.__opts = None @@ -113,7 +112,7 @@ class JailReader(ConfigReader): filterName, filterOpt = JailReader.extractOptions( self.__opts["filter"]) self.__filter = FilterReader( - filterName, self.__name, filterOpt, share_config=self.__cfg_share, basedir=self.getBaseDir()) + filterName, self.__name, filterOpt, share_config=self.share_config, basedir=self.getBaseDir()) ret = self.__filter.read() if ret: self.__filter.getOptions(self.__opts) @@ -143,7 +142,7 @@ class JailReader(ConfigReader): else: action = ActionReader( actName, self.__name, actOpt, - share_config=self.__cfg_share, basedir=self.getBaseDir()) + share_config=self.share_config, basedir=self.getBaseDir()) ret = action.read() if ret: action.getOptions(self.__opts) diff --git a/fail2ban/client/jailsreader.py b/fail2ban/client/jailsreader.py index 3bb713f6..7043aa67 100644 --- a/fail2ban/client/jailsreader.py +++ b/fail2ban/client/jailsreader.py @@ -43,7 +43,6 @@ class JailsReader(ConfigReader): """ # use shared config if possible: ConfigReader.__init__(self, **kwargs) - self.__cfg_share = dict() self.__jails = list() self.__force_enable = force_enable @@ -73,7 +72,7 @@ class JailsReader(ConfigReader): # use the cfg_share for filter/action caching and the same config for all # jails (use_config=...), therefore don't read it here: jail = JailReader(sec, force_enable=self.__force_enable, - cfg_share=self.__cfg_share, use_config=self._cfg) + share_config=self.share_config, use_config=self._cfg) ret = jail.getOptions() if ret: if jail.isEnabled(): diff --git a/fail2ban/tests/clientreadertestcase.py b/fail2ban/tests/clientreadertestcase.py index 0aaa8890..dadbef0f 100644 --- a/fail2ban/tests/clientreadertestcase.py +++ b/fail2ban/tests/clientreadertestcase.py @@ -39,8 +39,6 @@ STOCK = os.path.exists(os.path.join('config','fail2ban.conf')) IMPERFECT_CONFIG = os.path.join(os.path.dirname(__file__), 'config') -LAST_WRITE_TIME = 0 - class ConfigReaderTest(unittest.TestCase): def setUp(self): @@ -59,8 +57,7 @@ class ConfigReaderTest(unittest.TestCase): d_ = os.path.join(self.d, d) if not os.path.exists(d_): os.makedirs(d_) - fname = "%s/%s" % (self.d, fname) - f = open(fname, "w") + f = open("%s/%s" % (self.d, fname), "w") if value is not None: f.write(""" [section] @@ -69,14 +66,6 @@ option = %s if content is not None: f.write(content) f.close() - # set modification time to another second to revalidate cache (if milliseconds not supported) : - global LAST_WRITE_TIME - mtime = os.path.getmtime(fname) - if LAST_WRITE_TIME == mtime: - mtime += 1 - os.utime(fname, (mtime, mtime)) - LAST_WRITE_TIME = mtime - def _remove(self, fname): os.unlink("%s/%s" % (self.d, fname)) @@ -102,6 +91,7 @@ option = %s # raise unittest.SkipTest("Skipping on %s -- access rights are not enforced" % platform) pass + def testOptionalDotDDir(self): self.assertFalse(self.c.read('c')) # nothing is there yet self._write("c.conf", "1") @@ -347,9 +337,9 @@ class FilterReaderTest(unittest.TestCase): class JailsReaderTestCache(LogCaptureTestCase): - def _readWholeConf(self, basedir, force_enable=False): + def _readWholeConf(self, basedir, force_enable=False, share_config=None): # read whole configuration like a file2ban-client ... - configurator = Configurator(force_enable=force_enable) + configurator = Configurator(force_enable=force_enable, share_config=share_config) configurator.setBaseDir(basedir) configurator.readEarly() configurator.getEarlyOptions() @@ -360,7 +350,7 @@ class JailsReaderTestCache(LogCaptureTestCase): def _getLoggedReadCount(self, filematch): cnt = 0 for s in self.getLog().rsplit('\n'): - if re.match(r"^Reading files?: .*/"+filematch, s): + if re.match(r"^\s*Reading files?: .*/"+filematch, s): cnt += 1 return cnt @@ -372,8 +362,11 @@ class JailsReaderTestCache(LogCaptureTestCase): shutil.copy(CONFIG_DIR + '/jail.conf', basedir + '/jail.local') shutil.copy(CONFIG_DIR + '/fail2ban.conf', basedir + '/fail2ban.local') + # common sharing handle for this test: + share_cfg = dict() + # read whole configuration like a file2ban-client ... - self._readWholeConf(basedir) + self._readWholeConf(basedir, share_config=share_cfg) # how many times jail.local was read: cnt = self._getLoggedReadCount('jail.local') # if cnt > 1: @@ -382,7 +375,7 @@ class JailsReaderTestCache(LogCaptureTestCase): # read whole configuration like a file2ban-client, again ... # but this time force enable all jails, to check filter and action cached also: - self._readWholeConf(basedir, force_enable=True) + self._readWholeConf(basedir, force_enable=True, share_config=share_cfg) cnt = self._getLoggedReadCount(r'jail\.local') # still one (no more reads): self.assertTrue(cnt == 1, "Unexpected count by second reading of jail files, cnt = %s" % cnt) From e0eb4f2358049b4427282aaa2a8ddf65972ed05b Mon Sep 17 00:00:00 2001 From: sebres Date: Fri, 10 Oct 2014 02:47:42 +0200 Subject: [PATCH 07/12] code review: use the same code (corresponding test cases - with sharing on and without it); --- fail2ban/client/configparserinc.py | 57 +++++++++++++++--------------- 1 file changed, 29 insertions(+), 28 deletions(-) diff --git a/fail2ban/client/configparserinc.py b/fail2ban/client/configparserinc.py index d6ad4e34..196c2ae6 100644 --- a/fail2ban/client/configparserinc.py +++ b/fail2ban/client/configparserinc.py @@ -204,39 +204,40 @@ after = 1.conf else: fileNamesFull = filenames + if not fileNamesFull: + return [] + if self._cfg_share is not None: logSys.debug(" Sharing files: %s", fileNamesFull) - - if len(fileNamesFull) > 1: - # read multiple configs: - ret = [] - alld = self.get_defaults() - alls = self.get_sections() - for filename in fileNamesFull: - # read single one, add to return list, use sharing if possible: - cfg, i = self._getSharedSCPWI(filename) - if i: - ret += i - # merge defaults and all sections to self: - alld.update(cfg.get_defaults()) - for n, s in cfg.get_sections().iteritems(): - if isinstance(s, dict): - s2 = alls.get(n) - if isinstance(s2, dict): - s2.update(s) - else: - alls[n] = s.copy() - else: - alls[n] = s - - return ret - - # read one config : - logSys.debug(" Reading file: %s", fileNamesFull[0]) else: - # don't have sharing - read one or multiple at once: logSys.debug(" Reading files: %s", fileNamesFull) + if len(fileNamesFull) > 1: + # read multiple configs: + ret = [] + alld = self.get_defaults() + alls = self.get_sections() + for filename in fileNamesFull: + # read single one, add to return list, use sharing if possible: + cfg, i = self._getSharedSCPWI(filename) + if i: + ret += i + # merge defaults and all sections to self: + alld.update(cfg.get_defaults()) + for n, s in cfg.get_sections().iteritems(): + if isinstance(s, dict): + s2 = alls.get(n) + if isinstance(s2, dict): + s2.update(s) + else: + alls[n] = s.copy() + else: + alls[n] = s + + return ret + + # read one config : + logSys.debug(" Reading file: %s", fileNamesFull[0]) # read file(s) : if sys.version_info >= (3,2): # pragma: no cover return SafeConfigParser.read(self, fileNamesFull, encoding='utf-8') From 02a46d0901b99aedfebaae6b2eb6070d97ad4246 Mon Sep 17 00:00:00 2001 From: sebres Date: Fri, 10 Oct 2014 12:05:49 +0200 Subject: [PATCH 08/12] code review; more stable config sharing, configurator always shares it config readers now; --- fail2ban/client/configparserinc.py | 16 +++---- fail2ban/client/configreader.py | 64 +++++++++++++++----------- fail2ban/client/configurator.py | 3 ++ fail2ban/client/fail2banreader.py | 1 - fail2ban/client/jailreader.py | 1 - fail2ban/client/jailsreader.py | 1 - fail2ban/tests/clientreadertestcase.py | 7 ++- 7 files changed, 54 insertions(+), 39 deletions(-) diff --git a/fail2ban/client/configparserinc.py b/fail2ban/client/configparserinc.py index 196c2ae6..1687f87f 100644 --- a/fail2ban/client/configparserinc.py +++ b/fail2ban/client/configparserinc.py @@ -62,6 +62,7 @@ else: # pragma: no cover # Gets the instance of the logger. logSys = getLogger(__name__) +logLevel = 7 __all__ = ['SafeConfigParserWithIncludes'] @@ -126,8 +127,8 @@ after = 1.conf cfg = SCPWI(share_config=self._cfg_share) i = cfg.read(filename, get_includes=False) self._cfg_share[hashv] = (cfg, i) - else: - logSys.debug(" Shared file: %s", filename) + elif logSys.getEffectiveLevel() <= logLevel: + logSys.log(logLevel, " Shared file: %s", filename) else: # don't have sharing: cfg = SCPWI() @@ -207,10 +208,9 @@ after = 1.conf if not fileNamesFull: return [] - if self._cfg_share is not None: - logSys.debug(" Sharing files: %s", fileNamesFull) - else: - logSys.debug(" Reading files: %s", fileNamesFull) + if logSys.getEffectiveLevel() <= logLevel: + logSys.log(logLevel, (" Sharing files: %s" if self._cfg_share is not None else \ + " Reading files: %s"), fileNamesFull) if len(fileNamesFull) > 1: # read multiple configs: @@ -237,10 +237,10 @@ after = 1.conf return ret # read one config : - logSys.debug(" Reading file: %s", fileNamesFull[0]) + if logSys.getEffectiveLevel() <= logLevel: + logSys.log(logLevel, " Reading file: %s", fileNamesFull[0]) # read file(s) : if sys.version_info >= (3,2): # pragma: no cover return SafeConfigParser.read(self, fileNamesFull, encoding='utf-8') else: return SafeConfigParser.read(self, fileNamesFull) - diff --git a/fail2ban/client/configreader.py b/fail2ban/client/configreader.py index 657800fa..bd2e5f0c 100644 --- a/fail2ban/client/configreader.py +++ b/fail2ban/client/configreader.py @@ -27,12 +27,11 @@ __license__ = "GPL" import glob, os from ConfigParser import NoOptionError, NoSectionError -from .configparserinc import SafeConfigParserWithIncludes +from .configparserinc import SafeConfigParserWithIncludes, logLevel from ..helpers import getLogger # Gets the instance of the logger. logSys = getLogger(__name__) -_logLevel = 6 class ConfigReader(): """Generic config reader class. @@ -72,6 +71,22 @@ class ConfigReader(): def read(self, name, once=True): # shared ? + if not self._cfg: + self.touch(name) + # performance feature - read once if using shared config reader: + if once and self._cfg.read_cfg_files is not None: + return self._cfg.read_cfg_files + + # read: + if self._cfg_share is not None: + logSys.info("Sharing configs for %s under %s ", name, self._cfg.getBaseDir()) + ret = self._cfg.read(name) + + # save already read and return: + self._cfg.read_cfg_files = ret + return ret + + def touch(self, name = ''): if not self._cfg and self._cfg_share is not None: self._cfg = self._cfg_share.get(name) if not self._cfg: @@ -79,36 +94,33 @@ class ConfigReader(): if self._cfg_share_basedir is not None: self._cfg.setBaseDir(self._cfg_share_basedir) self._cfg_share[name] = self._cfg - # performance feature - read once if using shared config reader: - rc = self._cfg.read_cfg_files - if once and rc.get(name) is not None: - return rc.get(name) - - # read: - if self._cfg_share is not None: - logSys.info("Sharing configs for %s under %s ", name, self._cfg.getBaseDir()) - ret = self._cfg.read(name) - - # save already read: - if once: - rc[name] = ret - return ret + else: + self._cfg = ConfigReaderUnshared(**self._cfg_share_kwargs) def sections(self): - return self._cfg.sections() + if self._cfg is not None: + return self._cfg.sections() + return [] def has_section(self, sec): - return self._cfg.has_section(sec) + if self._cfg is not None: + return self._cfg.has_section(sec) + return False def options(self, *args): - return self._cfg.options(*args) + if self._cfg is not None: + return self._cfg.options(*args) + return {} def get(self, sec, opt): - return self._cfg.get(sec, opt) + if self._cfg is not None: + return self._cfg.get(sec, opt) + return None def getOptions(self, *args, **kwargs): - return self._cfg.getOptions(*args, **kwargs) - + if self._cfg is not None: + return self._cfg.getOptions(*args, **kwargs) + return {} class ConfigReaderUnshared(SafeConfigParserWithIncludes): """Unshared config reader (previously ConfigReader). @@ -121,7 +133,7 @@ class ConfigReaderUnshared(SafeConfigParserWithIncludes): def __init__(self, basedir=None, *args, **kwargs): SafeConfigParserWithIncludes.__init__(self, *args, **kwargs) - self.read_cfg_files = dict() + self.read_cfg_files = None self.setBaseDir(basedir) def setBaseDir(self, basedir): @@ -137,7 +149,7 @@ class ConfigReaderUnshared(SafeConfigParserWithIncludes): raise ValueError("Base configuration directory %s does not exist " % self._basedir) basename = os.path.join(self._basedir, filename) - logSys.info("Reading configs for %s under %s " , filename, self._basedir) + logSys.debug("Reading configs for %s under %s " , filename, self._basedir) config_files = [ basename + ".conf" ] # possible further customizations under a .conf.d directory @@ -203,8 +215,8 @@ class ConfigReaderUnshared(SafeConfigParserWithIncludes): logSys.warning("'%s' not defined in '%s'. Using default one: %r" % (option[1], sec, option[2])) values[option[1]] = option[2] - elif logSys.getEffectiveLevel() <= _logLevel: - logSys.log(_logLevel, "Non essential option '%s' not defined in '%s'.", option[1], sec) + elif logSys.getEffectiveLevel() <= logLevel: + logSys.log(logLevel, "Non essential option '%s' not defined in '%s'.", option[1], sec) except ValueError: logSys.warning("Wrong value for '" + option[1] + "' in '" + sec + "'. Using default one: '" + `option[2]` + "'") diff --git a/fail2ban/client/configurator.py b/fail2ban/client/configurator.py index 4c0c0428..b3ddc5d0 100644 --- a/fail2ban/client/configurator.py +++ b/fail2ban/client/configurator.py @@ -36,6 +36,9 @@ class Configurator: def __init__(self, force_enable=False, share_config=None): self.__settings = dict() self.__streams = dict() + # always share all config readers: + if share_config is None: + share_config = dict() self.__fail2ban = Fail2banReader(share_config=share_config) self.__jails = JailsReader(force_enable=force_enable, share_config=share_config) diff --git a/fail2ban/client/fail2banreader.py b/fail2ban/client/fail2banreader.py index 4d46c156..361a5e54 100644 --- a/fail2ban/client/fail2banreader.py +++ b/fail2ban/client/fail2banreader.py @@ -33,7 +33,6 @@ logSys = getLogger(__name__) class Fail2banReader(ConfigReader): def __init__(self, **kwargs): - self.__opts = None ConfigReader.__init__(self, **kwargs) def read(self): diff --git a/fail2ban/client/jailreader.py b/fail2ban/client/jailreader.py index 99891c0f..ffdc5e26 100644 --- a/fail2ban/client/jailreader.py +++ b/fail2ban/client/jailreader.py @@ -42,7 +42,6 @@ class JailReader(ConfigReader): r'([\w\-_\.]+)=(?:"([^"]*)"|\'([^\']*)\'|([^,]*))(?:,|$)') def __init__(self, name, force_enable=False, **kwargs): - # use shared config if possible: ConfigReader.__init__(self, **kwargs) self.__name = name self.__filter = None diff --git a/fail2ban/client/jailsreader.py b/fail2ban/client/jailsreader.py index 7043aa67..40255ce7 100644 --- a/fail2ban/client/jailsreader.py +++ b/fail2ban/client/jailsreader.py @@ -41,7 +41,6 @@ class JailsReader(ConfigReader): Passed to JailReader to force enable the jails. It is for internal use """ - # use shared config if possible: ConfigReader.__init__(self, **kwargs) self.__jails = list() self.__force_enable = force_enable diff --git a/fail2ban/tests/clientreadertestcase.py b/fail2ban/tests/clientreadertestcase.py index dadbef0f..e4dc2189 100644 --- a/fail2ban/tests/clientreadertestcase.py +++ b/fail2ban/tests/clientreadertestcase.py @@ -21,9 +21,9 @@ __author__ = "Cyril Jaquier, Yaroslav Halchenko" __copyright__ = "Copyright (c) 2004 Cyril Jaquier, 2011-2013 Yaroslav Halchenko" __license__ = "GPL" -import os, glob, shutil, tempfile, unittest, re - +import os, glob, shutil, tempfile, unittest, re, logging from ..client.configreader import ConfigReaderUnshared +from ..client import configparserinc from ..client.jailreader import JailReader from ..client.filterreader import FilterReader from ..client.jailsreader import JailsReader @@ -355,6 +355,8 @@ class JailsReaderTestCache(LogCaptureTestCase): return cnt def testTestJailConfCache(self): + saved_ll = configparserinc.logLevel + configparserinc.logLevel = logging.DEBUG basedir = tempfile.mkdtemp("fail2ban_conf") try: shutil.rmtree(basedir) @@ -388,6 +390,7 @@ class JailsReaderTestCache(LogCaptureTestCase): self.assertTrue(cnt == 1, "Unexpected count by reading of action files, cnt = %s" % cnt) finally: shutil.rmtree(basedir) + configparserinc.logLevel = saved_ll class JailsReaderTest(LogCaptureTestCase): From 95bdcdecaaa0419785614c22afc57165fcbb29dc Mon Sep 17 00:00:00 2001 From: sebres Date: Fri, 10 Oct 2014 16:49:08 +0200 Subject: [PATCH 09/12] cache-config-read-v2 merged; logging normalized, set log level for loading (read or use shared) file(s) to INFO; prevent to read some files twice by read inside "_getIncludes" and by "read" self (occurred by only one file); --- fail2ban/client/configparserinc.py | 10 ++++------ fail2ban/client/configreader.py | 5 ++--- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/fail2ban/client/configparserinc.py b/fail2ban/client/configparserinc.py index 1687f87f..d819281b 100644 --- a/fail2ban/client/configparserinc.py +++ b/fail2ban/client/configparserinc.py @@ -195,7 +195,7 @@ after = 1.conf def get_sections(self): return self._sections - def read(self, filenames, get_includes=True, log_info=None): + def read(self, filenames, get_includes=True): if not isinstance(filenames, list): filenames = [ filenames ] # retrieve (and cache) includes: @@ -208,11 +208,9 @@ after = 1.conf if not fileNamesFull: return [] - if logSys.getEffectiveLevel() <= logLevel: - logSys.log(logLevel, (" Sharing files: %s" if self._cfg_share is not None else \ - " Reading files: %s"), fileNamesFull) + logSys.info(" Loading files: %s", fileNamesFull) - if len(fileNamesFull) > 1: + if get_includes or len(fileNamesFull) > 1: # read multiple configs: ret = [] alld = self.get_defaults() @@ -238,7 +236,7 @@ after = 1.conf # read one config : if logSys.getEffectiveLevel() <= logLevel: - logSys.log(logLevel, " Reading file: %s", fileNamesFull[0]) + logSys.log(logLevel, " Reading file: %s", fileNamesFull[0]) # read file(s) : if sys.version_info >= (3,2): # pragma: no cover return SafeConfigParser.read(self, fileNamesFull, encoding='utf-8') diff --git a/fail2ban/client/configreader.py b/fail2ban/client/configreader.py index bd2e5f0c..9e3bf90f 100644 --- a/fail2ban/client/configreader.py +++ b/fail2ban/client/configreader.py @@ -77,9 +77,8 @@ class ConfigReader(): if once and self._cfg.read_cfg_files is not None: return self._cfg.read_cfg_files - # read: - if self._cfg_share is not None: - logSys.info("Sharing configs for %s under %s ", name, self._cfg.getBaseDir()) + # load: + logSys.info("Loading configs for %s under %s ", name, self._cfg.getBaseDir()) ret = self._cfg.read(name) # save already read and return: From 7f5d4aa7a6c5acc5f29893e9d0f5b4c78e5c4053 Mon Sep 17 00:00:00 2001 From: sebres Date: Fri, 10 Oct 2014 16:59:40 +0200 Subject: [PATCH 10/12] normalize tabs/spaces in docstrings; --- fail2ban/client/configreader.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/fail2ban/client/configreader.py b/fail2ban/client/configreader.py index 9e3bf90f..ea86a36f 100644 --- a/fail2ban/client/configreader.py +++ b/fail2ban/client/configreader.py @@ -36,8 +36,8 @@ logSys = getLogger(__name__) class ConfigReader(): """Generic config reader class. - A caching adapter which automatically reuses already shared configuration. - """ + A caching adapter which automatically reuses already shared configuration. + """ def __init__(self, use_config=None, share_config=None, **kwargs): # use given shared config if possible (see read): @@ -126,7 +126,7 @@ class ConfigReaderUnshared(SafeConfigParserWithIncludes): Does not use this class (internal not shared/cached represenation). Use ConfigReader instead. - """ + """ DEFAULT_BASEDIR = '/etc/fail2ban' @@ -224,12 +224,12 @@ class ConfigReaderUnshared(SafeConfigParserWithIncludes): class DefinitionInitConfigReader(ConfigReader): """Config reader for files with options grouped in [Definition] and - [Init] sections. + [Init] sections. - Is a base class for readers of filters and actions, where definitions - in jails might provide custom values for options defined in [Init] - section. - """ + Is a base class for readers of filters and actions, where definitions + in jails might provide custom values for options defined in [Init] + section. + """ _configOpts = [] From 73a06d55a8ed5996e959952a616c9d9a8b209dc7 Mon Sep 17 00:00:00 2001 From: sebres Date: Fri, 10 Oct 2014 18:50:24 +0200 Subject: [PATCH 11/12] reset share/cache storage (if we use 'reload' in client with interactive mode) --- bin/fail2ban-client | 1 + fail2ban/client/configurator.py | 7 ++++++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/bin/fail2ban-client b/bin/fail2ban-client index 89e0a903..0c6999c1 100755 --- a/bin/fail2ban-client +++ b/bin/fail2ban-client @@ -409,6 +409,7 @@ class Fail2banClient: # TODO: get away from stew of return codes and exception # handling -- handle via exceptions try: + self.__configurator.Reload() self.__configurator.readAll() ret = self.__configurator.getOptions(jail) self.__configurator.convertToProtocol() diff --git a/fail2ban/client/configurator.py b/fail2ban/client/configurator.py index b3ddc5d0..8667501c 100644 --- a/fail2ban/client/configurator.py +++ b/fail2ban/client/configurator.py @@ -39,9 +39,14 @@ class Configurator: # always share all config readers: if share_config is None: share_config = dict() + self.__share_config = share_config self.__fail2ban = Fail2banReader(share_config=share_config) self.__jails = JailsReader(force_enable=force_enable, share_config=share_config) - + + def Reload(self): + # clear all shared handlers: + self.__share_config.clear() + def setBaseDir(self, folderName): self.__fail2ban.setBaseDir(folderName) self.__jails.setBaseDir(folderName) From 7d3e6e9935a22de7c7225576e649ed8592d65e55 Mon Sep 17 00:00:00 2001 From: sebres Date: Fri, 10 Oct 2014 20:06:58 +0200 Subject: [PATCH 12/12] code review, change log entries added; --- ChangeLog | 6 ++++++ fail2ban/client/configreader.py | 18 ++++++++++++++---- 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/ChangeLog b/ChangeLog index d92aec4a..0515823a 100644 --- a/ChangeLog +++ b/ChangeLog @@ -16,6 +16,8 @@ ver. 0.9.1 (2014/xx/xx) - better, faster, stronger provides defaults for the chain, port, protocol and name tags - Fixes: + * start of file2ban aborted (on slow hosts, systemd considers the server has been + timed out and kills him), see gh-824 * UTF-8 fixes in pure-ftp thanks to Johannes Weberhofer. Closes gh-806. * systemd backend error on bad utf-8 in python3 * badips.py action error when logging HTTP error raised with badips request @@ -64,6 +66,10 @@ ver. 0.9.1 (2014/xx/xx) - better, faster, stronger - Added Cloudflare API action - Enhancements + * Start performance of fail2ban-client (and tests) increased, start time + and cpu usage rapidly reduced. Introduced a shared storage logic, to bypass + reading lots of config files (see gh-824). + Thanks to Joost Molenaar for good catch (reported gh-820). * Fail2ban-regex - add print-all-matched option. Closes gh-652 * Suppress fail2ban-client warnings for non-critical config options * Match non "Bye Bye" disconnect messages for sshd locked account regex diff --git a/fail2ban/client/configreader.py b/fail2ban/client/configreader.py index ea86a36f..82bf0fdc 100644 --- a/fail2ban/client/configreader.py +++ b/fail2ban/client/configreader.py @@ -70,7 +70,12 @@ class ConfigReader(): return self._cfg_share def read(self, name, once=True): - # shared ? + """ Overloads a default (not shared) read of config reader. + + To prevent mutiple reads of config files with it includes, reads into + the config reader, if it was not yet cached/shared by 'name'. + """ + # already shared ? if not self._cfg: self.touch(name) # performance feature - read once if using shared config reader: @@ -85,7 +90,12 @@ class ConfigReader(): self._cfg.read_cfg_files = ret return ret - def touch(self, name = ''): + def touch(self, name=''): + """ Allocates and share a config file by it name. + + Automatically allocates unshared or reuses shared handle by given 'name' and + init arguments inside a given shared storage. + """ if not self._cfg and self._cfg_share is not None: self._cfg = self._cfg_share.get(name) if not self._cfg: @@ -124,7 +134,7 @@ class ConfigReader(): class ConfigReaderUnshared(SafeConfigParserWithIncludes): """Unshared config reader (previously ConfigReader). - Does not use this class (internal not shared/cached represenation). + Do not use this class (internal not shared/cached represenation). Use ConfigReader instead. """ @@ -191,7 +201,7 @@ class ConfigReaderUnshared(SafeConfigParserWithIncludes): # 1 -> the name of the option # 2 -> the default value for the option - def getOptions(self, sec, options, pOptions = None): + def getOptions(self, sec, options, pOptions=None): values = dict() for option in options: try: