Merge "internal_client: Remove allow_modify_pipeline option"

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]

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,
+                 use_replication_network=False, global_conf=None, app=None,
-                 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):

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()

swift/container/sharder.py

@@ -896,7 +896,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)})

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):

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(*_, **__):

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)