diff --git a/etc/internal-client.conf-sample b/etc/internal-client.conf-sample index 7ded5fd8ad..d9ed5e24b2 100644 --- a/etc/internal-client.conf-sample +++ b/etc/internal-client.conf-sample @@ -26,6 +26,7 @@ # log_statsd_metric_prefix = [pipeline:main] +# Note: gatekeeper middleware is not allowed in the internal client pipeline pipeline = catch_errors proxy-logging cache symlink proxy-server [app:proxy-server] diff --git a/swift/common/internal_client.py b/swift/common/internal_client.py index 2c1c99cc0e..d82035c398 100644 --- a/swift/common/internal_client.py +++ b/swift/common/internal_client.py @@ -28,6 +28,7 @@ from zlib import compressobj from swift.common.exceptions import ClientException from swift.common.http import (HTTP_NOT_FOUND, HTTP_MULTIPLE_CHOICES, is_client_error, is_server_error) +from swift.common.middleware.gatekeeper import GatekeeperMiddleware from swift.common.request_helpers import USE_REPLICATION_NETWORK_HEADER from swift.common.swob import Request, bytes_to_wsgi from swift.common.utils import quote, close_if_possible, drain_and_close @@ -144,6 +145,8 @@ class InternalClient(object): :param user_agent: User agent to be sent to requests to Swift. :param request_tries: Number of tries before InternalClient.make_request() gives up. + :param use_replication_network: Force the client to use the replication + network over the cluster. :param global_conf: a dict of options to update the loaded proxy config. Options in ``global_conf`` will override those in ``conf_path`` except where the ``conf_path`` option is preceded by ``set``. @@ -151,12 +154,15 @@ class InternalClient(object): """ def __init__(self, conf_path, user_agent, request_tries, - allow_modify_pipeline=False, use_replication_network=False, - global_conf=None, app=None): + use_replication_network=False, global_conf=None, app=None, + **kwargs): if request_tries < 1: raise ValueError('request_tries must be positive') + # Internal clients don't use the gatekeeper and the pipeline remains + # static so we never allow anything to modify the proxy pipeline. self.app = app or loadapp(conf_path, global_conf=global_conf, - allow_modify_pipeline=allow_modify_pipeline,) + allow_modify_pipeline=False,) + self.check_gatekeeper_not_loaded(self.app) self.user_agent = \ self.app._pipeline_final_app.backend_user_agent = user_agent self.request_tries = request_tries @@ -167,6 +173,19 @@ class InternalClient(object): self.auto_create_account_prefix = \ self.app._pipeline_final_app.auto_create_account_prefix + @staticmethod + def check_gatekeeper_not_loaded(app): + # the Gatekeeper middleware would prevent an InternalClient passing + # X-Backend-* headers to the proxy app, so ensure it's not present + try: + for app in app._pipeline: + if isinstance(app, GatekeeperMiddleware): + raise ValueError( + "Gatekeeper middleware is not allowed in the " + "InternalClient proxy pipeline") + except AttributeError: + pass + def make_request( self, method, path, headers, acceptable_statuses, body_file=None, params=None): diff --git a/swift/common/wsgi.py b/swift/common/wsgi.py index 4fa4946dd7..7c39a89e25 100644 --- a/swift/common/wsgi.py +++ b/swift/common/wsgi.py @@ -361,10 +361,14 @@ def loadapp(conf_file, global_conf=None, allow_modify_pipeline=True): if func and allow_modify_pipeline: func(PipelineWrapper(ctx)) filters = [c.create() for c in reversed(ctx.filter_contexts)] + pipeline = [ultimate_app] + ultimate_app._pipeline = pipeline + ultimate_app._pipeline_final_app = ultimate_app app = ultimate_app - app._pipeline_final_app = ultimate_app for filter_app in filters: - app = filter_app(app) + app = filter_app(pipeline[0]) + pipeline.insert(0, app) + app._pipeline = pipeline app._pipeline_final_app = ultimate_app return app return ctx.create() diff --git a/swift/container/sharder.py b/swift/container/sharder.py index 378bad7c94..ca6ee05662 100644 --- a/swift/container/sharder.py +++ b/swift/container/sharder.py @@ -895,7 +895,6 @@ class ContainerSharder(ContainerSharderConf, ContainerReplicator): internal_client_conf_path, 'Swift Container Sharder', request_tries, - allow_modify_pipeline=False, use_replication_network=True, global_conf={'log_name': '%s-ic' % conf.get( 'log_name', self.log_route)}) diff --git a/test/unit/common/test_internal_client.py b/test/unit/common/test_internal_client.py index d26ef0e2d1..1ccbf30f08 100644 --- a/test/unit/common/test_internal_client.py +++ b/test/unit/common/test_internal_client.py @@ -30,6 +30,7 @@ from swift.common import exceptions, internal_client, request_helpers, swob, \ from swift.common.header_key_dict import HeaderKeyDict from swift.common.storage_policy import StoragePolicy from swift.common.middleware.proxy_logging import ProxyLoggingMiddleware +from swift.common.middleware.gatekeeper import GatekeeperMiddleware from test.debug_logger import debug_logger from test.unit import with_tempdir, write_fake_ring, patch_policies @@ -392,6 +393,21 @@ class TestInternalClient(unittest.TestCase): conf_path, user_agent, request_tries=0) mock_loadapp.assert_not_called() + # if we load it with the gatekeeper middleware then we also get a + # value error + gate_keeper_app = GatekeeperMiddleware(app, {}) + gate_keeper_app._pipeline_final_app = app + gate_keeper_app._pipeline = [gate_keeper_app, app] + with mock.patch.object( + internal_client, 'loadapp', return_value=gate_keeper_app) \ + as mock_loadapp, self.assertRaises(ValueError) as err: + internal_client.InternalClient( + conf_path, user_agent, request_tries) + self.assertEqual( + str(err.exception), + ('Gatekeeper middleware is not allowed in the InternalClient ' + 'proxy pipeline')) + with mock.patch.object( internal_client, 'loadapp', return_value=app) as mock_loadapp: client = internal_client.InternalClient( @@ -421,6 +437,51 @@ class TestInternalClient(unittest.TestCase): self.assertEqual(request_tries, client.request_tries) self.assertTrue(client.use_replication_network) + def test_gatekeeper_not_loaded(self): + app = FakeSwift() + pipeline = [app] + + class RandomMiddleware(object): + def __init__(self, app): + self.app = app + self._pipeline_final_app = app + self._pipeline = pipeline + self._pipeline.insert(0, self) + + # if there is no Gatekeeper middleware then it's false + # just the final app + self.assertFalse( + internal_client.InternalClient.check_gatekeeper_not_loaded(app)) + + # now with a bunch of middlewares + app_no_gatekeeper = app + for i in range(5): + app_no_gatekeeper = RandomMiddleware(app_no_gatekeeper) + self.assertFalse( + internal_client.InternalClient.check_gatekeeper_not_loaded( + app_no_gatekeeper)) + + # But if we put the gatekeeper on the end, it will be found + app_with_gatekeeper = GatekeeperMiddleware(app_no_gatekeeper, {}) + pipeline.insert(0, app_with_gatekeeper) + app_with_gatekeeper._pipeline = pipeline + with self.assertRaises(ValueError) as err: + internal_client.InternalClient.check_gatekeeper_not_loaded( + app_with_gatekeeper) + self.assertEqual(str(err.exception), + ('Gatekeeper middleware is not allowed in the ' + 'InternalClient proxy pipeline')) + + # even if we bury deep into the pipeline + for i in range(5): + app_with_gatekeeper = RandomMiddleware(app_with_gatekeeper) + with self.assertRaises(ValueError) as err: + internal_client.InternalClient.check_gatekeeper_not_loaded( + app_with_gatekeeper) + self.assertEqual(str(err.exception), + ('Gatekeeper middleware is not allowed in the ' + 'InternalClient proxy pipeline')) + def test_make_request_sets_user_agent(self): class FakeApp(FakeSwift): def __init__(self, test): diff --git a/test/unit/common/test_wsgi.py b/test/unit/common/test_wsgi.py index d43f6730b0..5cddc7164f 100644 --- a/test/unit/common/test_wsgi.py +++ b/test/unit/common/test_wsgi.py @@ -1642,6 +1642,12 @@ class TestPipelineModification(unittest.TestCase): self.assertIs(app.app.app, app._pipeline_final_app) self.assertIs(app.app.app, app.app._pipeline_final_app) self.assertIs(app.app.app, app.app.app._pipeline_final_app) + exp_pipeline = [app, app.app, app.app.app] + self.assertEqual(exp_pipeline, app._pipeline) + self.assertEqual(exp_pipeline, app.app._pipeline) + self.assertEqual(exp_pipeline, app.app.app._pipeline) + self.assertIs(app._pipeline, app.app._pipeline) + self.assertIs(app._pipeline, app.app.app._pipeline) # make sure you can turn off the pipeline modification if you want def blow_up(*_, **__): diff --git a/test/unit/container/test_sharder.py b/test/unit/container/test_sharder.py index f73f202545..c101f93cd0 100644 --- a/test/unit/container/test_sharder.py +++ b/test/unit/container/test_sharder.py @@ -203,7 +203,6 @@ class TestSharder(BaseTestSharder): 'container-sharder', sharder.logger.logger.name) mock_ic.assert_called_once_with( '/etc/swift/internal-client.conf', 'Swift Container Sharder', 3, - allow_modify_pipeline=False, use_replication_network=True, global_conf={'log_name': 'container-sharder-ic'}) @@ -218,7 +217,6 @@ class TestSharder(BaseTestSharder): sharder, mock_ic = self._do_test_init(conf, expected) mock_ic.assert_called_once_with( '/etc/swift/internal-client.conf', 'Swift Container Sharder', 3, - allow_modify_pipeline=False, use_replication_network=True, global_conf={'log_name': 'container-sharder-ic'}) @@ -280,7 +278,6 @@ class TestSharder(BaseTestSharder): sharder, mock_ic = self._do_test_init(conf, expected) mock_ic.assert_called_once_with( '/etc/swift/my-sharder-ic.conf', 'Swift Container Sharder', 2, - allow_modify_pipeline=False, use_replication_network=True, global_conf={'log_name': 'container-sharder-ic'}) self.assertEqual(self.logger.get_lines_for_level('warning'), [ @@ -418,7 +415,6 @@ class TestSharder(BaseTestSharder): mock_ic.assert_called_once_with( '/etc/swift/internal-client.conf', 'Swift Container Sharder', 3, - allow_modify_pipeline=False, global_conf={'log_name': exp_internal_client_log_name}, use_replication_network=True)