From 3f19041c99fd86fefb8a43071bab279d37f324af Mon Sep 17 00:00:00 2001 From: Samuel Merritt Date: Thu, 27 Feb 2014 18:30:15 -0800 Subject: [PATCH] Don't lard up InternalClient with extra middleware One can argue that it makes sense for the client-facing proxy server to have certain middlewares like gatekeeper in its pipeline, but that is not desirable for InternalClient. In particular, it prevents you from passing in sysmeta headers using InternalClient, and I found myself wanting to do that earlier today. Now InternalClient's proxy application gets exactly what's configured; no more, no less. This will mean that the object expirer can read and write sysmeta headers, but I think we can trust it to keep our secrets. Change-Id: I17b4a89c24e600754701ee1645b40406421fa6f3 --- swift/common/internal_client.py | 6 ++++-- swift/common/wsgi.py | 4 ++-- test/unit/common/test_internal_client.py | 3 ++- test/unit/common/test_wsgi.py | 16 ++++++++++++++++ test/unit/obj/test_expirer.py | 12 ++++++------ 5 files changed, 30 insertions(+), 11 deletions(-) diff --git a/swift/common/internal_client.py b/swift/common/internal_client.py index 8394706a09..b19ba33b03 100644 --- a/swift/common/internal_client.py +++ b/swift/common/internal_client.py @@ -134,8 +134,10 @@ class InternalClient(object): gives up. """ - def __init__(self, conf_path, user_agent, request_tries): - self.app = loadapp('config:' + conf_path) + def __init__(self, conf_path, user_agent, request_tries, + allow_modify_pipeline=False): + self.app = loadapp('config:' + conf_path, + allow_modify_pipeline=allow_modify_pipeline) self.user_agent = user_agent self.request_tries = request_tries diff --git a/swift/common/wsgi.py b/swift/common/wsgi.py index 7feb25d0b2..a6004b638a 100644 --- a/swift/common/wsgi.py +++ b/swift/common/wsgi.py @@ -305,7 +305,7 @@ def loadcontext(object_type, uri, name=None, relative_to=None, global_conf=global_conf) -def loadapp(conf_file, global_conf): +def loadapp(conf_file, global_conf, allow_modify_pipeline=True): """ Loads a context from a config file, and if the context is a pipeline then presents the app with the opportunity to modify the pipeline. @@ -315,7 +315,7 @@ def loadapp(conf_file, global_conf): # give app the opportunity to modify the pipeline context app = ctx.app_context.create() func = getattr(app, 'modify_wsgi_pipeline', None) - if func: + if func and allow_modify_pipeline: func(PipelineWrapper(ctx)) return ctx.create() diff --git a/test/unit/common/test_internal_client.py b/test/unit/common/test_internal_client.py index 4772688d62..c94ccfeb37 100644 --- a/test/unit/common/test_internal_client.py +++ b/test/unit/common/test_internal_client.py @@ -185,9 +185,10 @@ class TestInternalClient(unittest.TestCase): self.conf_path = conf_path self.load_called = 0 - def load(self, uri): + def load(self, uri, allow_modify_pipeline=True): self.load_called += 1 self.test.assertEquals('config:' + conf_path, uri) + self.test.assertFalse(allow_modify_pipeline) return self conf_path = 'some_path' diff --git a/test/unit/common/test_wsgi.py b/test/unit/common/test_wsgi.py index f7af28357f..b54c86c2e0 100644 --- a/test/unit/common/test_wsgi.py +++ b/test/unit/common/test_wsgi.py @@ -819,6 +819,22 @@ class TestPipelineModification(unittest.TestCase): exp = swift.proxy.server.Application self.assertTrue(isinstance(app.app.app, exp), app.app.app) + # make sure you can turn off the pipeline modification if you want + def blow_up(*_, **__): + raise self.fail("needs more struts") + + with mock.patch( + 'swift.proxy.server.Application.modify_wsgi_pipeline', + blow_up): + app = wsgi.loadapp(conf_file, global_conf={}, + allow_modify_pipeline=False) + + # the pipeline was untouched + exp = swift.common.middleware.healthcheck.HealthCheckMiddleware + self.assertTrue(isinstance(app, exp), app) + exp = swift.proxy.server.Application + self.assertTrue(isinstance(app.app, exp), app.app) + def test_proxy_unmodified_wsgi_pipeline(self): # Make sure things are sane even when we modify nothing config = """ diff --git a/test/unit/obj/test_expirer.py b/test/unit/obj/test_expirer.py index 4329eef211..fd8100d7f9 100644 --- a/test/unit/obj/test_expirer.py +++ b/test/unit/obj/test_expirer.py @@ -46,7 +46,7 @@ class TestObjectExpirer(TestCase): self.old_loadapp = internal_client.loadapp self.old_sleep = internal_client.sleep - internal_client.loadapp = lambda x: None + internal_client.loadapp = lambda *a, **kw: None internal_client.sleep = not_sleep def teardown(self): @@ -618,7 +618,7 @@ class TestObjectExpirer(TestCase): start_response('204 No Content', [('Content-Length', '0')]) return [] - internal_client.loadapp = lambda x: fake_app + internal_client.loadapp = lambda *a, **kw: fake_app x = expirer.ObjectExpirer({}) ts = '1234' @@ -635,7 +635,7 @@ class TestObjectExpirer(TestCase): start_response('204 No Content', [('Content-Length', '0')]) return [] - internal_client.loadapp = lambda x: fake_app + internal_client.loadapp = lambda *a, **kw: fake_app x = expirer.ObjectExpirer({}) ts = '1234' @@ -649,7 +649,7 @@ class TestObjectExpirer(TestCase): start_response('404 Not Found', [('Content-Length', '0')]) return [] - internal_client.loadapp = lambda x: fake_app + internal_client.loadapp = lambda *a, **kw: fake_app x = expirer.ObjectExpirer({}) x.delete_actual_object('/path/to/object', '1234') @@ -661,7 +661,7 @@ class TestObjectExpirer(TestCase): [('Content-Length', '0')]) return [] - internal_client.loadapp = lambda x: fake_app + internal_client.loadapp = lambda *a, **kw: fake_app x = expirer.ObjectExpirer({}) x.delete_actual_object('/path/to/object', '1234') @@ -674,7 +674,7 @@ class TestObjectExpirer(TestCase): [('Content-Length', '0')]) return [] - internal_client.loadapp = lambda x: fake_app + internal_client.loadapp = lambda *a, **kw: fake_app x = expirer.ObjectExpirer({}) exc = None