internal_client: Remove allow_modify_pipeline option

The internal client is suppose to be internal to the cluster, and as
such we rely on it to not remove any headers we decide to send. However
if the allow_modify_pipeline option is set the gatekeeper middleware is
added to the internal client's proxy pipeline.

So firstly, this patch removes the allow_modify_pipeline option from the
internal client constructor. And when calling loadapp
allow_modify_pipeline is always passed with a False.

Further, an op could directly put the gatekeeper middleware into the
internal client config. The internal client constructor will now check
the pipeline and raise a ValueError if one has been placed in the
pipeline.

To do this, there is now a check_gatekeeper_loaded staticmethod that will
walk the pipeline which called from the InternalClient.__init__ method.
Enabling this walking through the pipeline, we are now stashing the wsgi
pipeline in each filter so that we don't have to rely on 'app' naming
conventions to iterate the pipeline.

Co-Authored-By: Alistair Coles <alistairncoles@gmail.com>
Change-Id: Idcca7ac0796935c8883de9084d612d64159d9f92

3 files changed, 67 insertions, 4 deletions

diff --git a/test/unit/common/test_internal_client.py b/test/unit/common/test_internal_client.py
index d26ef0e2d..1ccbf30f0 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 d43f6730b..5cddc7164 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 f73f20254..c101f93cd 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)