From dd113ab25a3089fa19c8e824c2d89db0ca3db0fa Mon Sep 17 00:00:00 2001 From: Alistair Coles Date: Tue, 5 Dec 2017 10:36:38 -0800 Subject: [PATCH] Refactor proxy-server conf loading to a helper function There were two middlewares using a common pattern to load the proxy-server app config section. The existing pattern fails to recognise option overrides that are declared using paste-deploy's 'set' notation, as illustrated by the change to test_dlo.py in this patch. This patch replaces the existing code with a helper function that loads the proxy-server config using the paste-deploy loader. The resulting config dict is therefore exactly the same as that used to initialise the proxy-server app. Change-Id: Ib58ce03e2010f41e7eb11f1a6dc78b0b7f55d466 --- swift/common/middleware/copy.py | 28 ++------ swift/common/middleware/dlo.py | 27 ++----- swift/common/wsgi.py | 18 +++++ test/unit/common/middleware/test_copy.py | 3 + test/unit/common/middleware/test_dlo.py | 11 ++- test/unit/common/test_wsgi.py | 91 ++++++++++++++++++++++++ 6 files changed, 132 insertions(+), 46 deletions(-) diff --git a/swift/common/middleware/copy.py b/swift/common/middleware/copy.py index 49e85c2b1a..c3fe0a20f1 100644 --- a/swift/common/middleware/copy.py +++ b/swift/common/middleware/copy.py @@ -114,20 +114,18 @@ greater than 5GB. """ -import os -from six.moves.configparser import ConfigParser, NoSectionError, NoOptionError from six.moves.urllib.parse import quote, unquote from swift.common import utils from swift.common.utils import get_logger, \ - config_true_value, FileLikeIter, read_conf_dir, close_if_possible + config_true_value, FileLikeIter, close_if_possible from swift.common.swob import Request, HTTPPreconditionFailed, \ HTTPRequestEntityTooLarge, HTTPBadRequest, HTTPException from swift.common.http import HTTP_MULTIPLE_CHOICES, is_success, HTTP_OK from swift.common.constraints import check_account_format, MAX_FILE_SIZE from swift.common.request_helpers import copy_header_subset, remove_items, \ is_sys_meta, is_sys_or_user_meta, is_object_transient_sysmeta -from swift.common.wsgi import WSGIContext, make_subrequest +from swift.common.wsgi import WSGIContext, make_subrequest, load_app_config def _check_path_header(req, name, length, error_msg): @@ -263,25 +261,9 @@ class ServerSideCopyMiddleware(object): # This takes preference over the one set in proxy app return - cp = ConfigParser() - if os.path.isdir(conf['__file__']): - read_conf_dir(cp, conf['__file__']) - else: - cp.read(conf['__file__']) - - try: - pipe = cp.get("pipeline:main", "pipeline") - except (NoSectionError, NoOptionError): - return - - proxy_name = pipe.rsplit(None, 1)[-1] - proxy_section = "app:" + proxy_name - - try: - conf['object_post_as_copy'] = cp.get(proxy_section, - 'object_post_as_copy') - except (NoSectionError, NoOptionError): - pass + proxy_conf = load_app_config(conf['__file__']) + if 'object_post_as_copy' in proxy_conf: + conf['object_post_as_copy'] = proxy_conf['object_post_as_copy'] def __call__(self, env, start_response): req = Request(env) diff --git a/swift/common/middleware/dlo.py b/swift/common/middleware/dlo.py index 21752ba26e..91728a5989 100644 --- a/swift/common/middleware/dlo.py +++ b/swift/common/middleware/dlo.py @@ -119,10 +119,8 @@ Here's an example using ``curl`` with tiny 1-byte segments:: """ import json -import os import six -from six.moves.configparser import ConfigParser, NoSectionError, NoOptionError from six.moves.urllib.parse import unquote from hashlib import md5 @@ -132,10 +130,9 @@ from swift.common.http import is_success from swift.common.swob import Request, Response, \ HTTPRequestedRangeNotSatisfiable, HTTPBadRequest, HTTPConflict from swift.common.utils import get_logger, \ - RateLimitedIterator, read_conf_dir, quote, close_if_possible, \ - closing_if_possible + RateLimitedIterator, quote, close_if_possible, closing_if_possible from swift.common.request_helpers import SegmentedIterable -from swift.common.wsgi import WSGIContext, make_subrequest +from swift.common.wsgi import WSGIContext, make_subrequest, load_app_config class GetContext(WSGIContext): @@ -381,26 +378,12 @@ class DynamicLargeObject(object): '__file__' not in conf): return - cp = ConfigParser() - if os.path.isdir(conf['__file__']): - read_conf_dir(cp, conf['__file__']) - else: - cp.read(conf['__file__']) - - try: - pipe = cp.get("pipeline:main", "pipeline") - except (NoSectionError, NoOptionError): - return - - proxy_name = pipe.rsplit(None, 1)[-1] - proxy_section = "app:" + proxy_name + proxy_conf = load_app_config(conf['__file__']) for setting in ('rate_limit_after_segment', 'rate_limit_segments_per_sec', 'max_get_time'): - try: - conf[setting] = cp.get(proxy_section, setting) - except (NoSectionError, NoOptionError): - pass + if setting in proxy_conf: + conf[setting] = proxy_conf[setting] def __call__(self, env, start_response): """ diff --git a/swift/common/wsgi.py b/swift/common/wsgi.py index add551c6f8..c078ac9d7b 100644 --- a/swift/common/wsgi.py +++ b/swift/common/wsgi.py @@ -397,6 +397,24 @@ def loadapp(conf_file, global_conf=None, allow_modify_pipeline=True): return ctx.create() +def load_app_config(conf_file): + """ + Read the app config section from a config file. + + :param conf_file: path to a config file + :return: a dict + """ + app_conf = {} + try: + ctx = loadcontext(loadwsgi.APP, conf_file) + except LookupError: + pass + else: + app_conf.update(ctx.app_context.global_conf) + app_conf.update(ctx.app_context.local_conf) + return app_conf + + def run_server(conf, logger, sock, global_conf=None): # Ensure TZ environment variable exists to avoid stat('/etc/localtime') on # some platforms. This locks in reported times to UTC. diff --git a/test/unit/common/middleware/test_copy.py b/test/unit/common/middleware/test_copy.py index a7b44e4a0e..26fb41630a 100644 --- a/test/unit/common/middleware/test_copy.py +++ b/test/unit/common/middleware/test_copy.py @@ -1309,6 +1309,9 @@ class TestServerSideCopyConfiguration(unittest.TestCase): [pipeline:main] pipeline = catch_errors copy ye-olde-proxy-server + [filter:catch_errors] + use = egg:swift#catch_errors + [filter:copy] use = egg:swift#copy diff --git a/test/unit/common/middleware/test_dlo.py b/test/unit/common/middleware/test_dlo.py index e0ed2d029d..d0a4ccc8d6 100644 --- a/test/unit/common/middleware/test_dlo.py +++ b/test/unit/common/middleware/test_dlo.py @@ -811,6 +811,9 @@ class TestDloConfiguration(unittest.TestCase): [pipeline:main] pipeline = catch_errors dlo ye-olde-proxy-server + [filter:catch_errors] + use = egg:swift#catch_errors + [filter:dlo] use = egg:swift#dlo max_get_time = 3600 @@ -845,13 +848,16 @@ class TestDloConfiguration(unittest.TestCase): [pipeline:main] pipeline = catch_errors dlo ye-olde-proxy-server + [filter:catch_errors] + use = egg:swift#catch_errors + [filter:dlo] use = egg:swift#dlo [app:ye-olde-proxy-server] use = egg:swift#proxy rate_limit_after_segment = 13 - max_get_time = 2900 + set max_get_time = 2900 """) conffile = tempfile.NamedTemporaryFile() @@ -878,6 +884,9 @@ class TestDloConfiguration(unittest.TestCase): """) proxy_conf2 = dedent(""" + [filter:catch_errors] + use = egg:swift#catch_errors + [filter:dlo] use = egg:swift#dlo diff --git a/test/unit/common/test_wsgi.py b/test/unit/common/test_wsgi.py index 5fed4901d9..7750e138ce 100644 --- a/test/unit/common/test_wsgi.py +++ b/test/unit/common/test_wsgi.py @@ -198,6 +198,97 @@ class TestWSGI(unittest.TestCase): app = wsgi.loadapp(wsgi.ConfigString(conf_body)) self.assertTrue(isinstance(app, obj_server.ObjectController)) + @with_tempdir + def test_load_app_config(self, tempdir): + conf_file = os.path.join(tempdir, 'file.conf') + + def _write_and_load_conf_file(conf): + with open(conf_file, 'wb') as fd: + fd.write(dedent(conf)) + return wsgi.load_app_config(conf_file) + + # typical case - DEFAULT options override same option in other sections + conf_str = """ + [DEFAULT] + dflt_option = dflt-value + + [pipeline:main] + pipeline = proxy-logging proxy-server + + [filter:proxy-logging] + use = egg:swift#proxy_logging + + [app:proxy-server] + use = egg:swift#proxy + proxy_option = proxy-value + dflt_option = proxy-dflt-value + """ + + proxy_conf = _write_and_load_conf_file(conf_str) + self.assertEqual('proxy-value', proxy_conf['proxy_option']) + self.assertEqual('dflt-value', proxy_conf['dflt_option']) + + # 'set' overrides DEFAULT option + conf_str = """ + [DEFAULT] + dflt_option = dflt-value + + [pipeline:main] + pipeline = proxy-logging proxy-server + + [filter:proxy-logging] + use = egg:swift#proxy_logging + + [app:proxy-server] + use = egg:swift#proxy + proxy_option = proxy-value + set dflt_option = proxy-dflt-value + """ + + proxy_conf = _write_and_load_conf_file(conf_str) + self.assertEqual('proxy-value', proxy_conf['proxy_option']) + self.assertEqual('proxy-dflt-value', proxy_conf['dflt_option']) + + # actual proxy server app name is dereferenced + conf_str = """ + [pipeline:main] + pipeline = proxy-logging proxyserverapp + + [filter:proxy-logging] + use = egg:swift#proxy_logging + + [app:proxyserverapp] + use = egg:swift#proxy + proxy_option = proxy-value + dflt_option = proxy-dflt-value + """ + proxy_conf = _write_and_load_conf_file(conf_str) + self.assertEqual('proxy-value', proxy_conf['proxy_option']) + self.assertEqual('proxy-dflt-value', proxy_conf['dflt_option']) + + # no pipeline + conf_str = """ + [filter:proxy-logging] + use = egg:swift#proxy_logging + + [app:proxy-server] + use = egg:swift#proxy + proxy_option = proxy-value + """ + proxy_conf = _write_and_load_conf_file(conf_str) + self.assertEqual({}, proxy_conf) + + # no matching section + conf_str = """ + [pipeline:main] + pipeline = proxy-logging proxy-server + + [filter:proxy-logging] + use = egg:swift#proxy_logging + """ + proxy_conf = _write_and_load_conf_file(conf_str) + self.assertEqual({}, proxy_conf) + def test_init_request_processor_from_conf_dir(self): config_dir = { 'proxy-server.conf.d/pipeline.conf': """