openstack/swift
Code Issues Proposed changes

internal_client: Remove allow_modify_pipeline option

Browse Source 
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
This commit is contained in:
Matthew Oliver 2023-03-31 16:48:01 +11:00 committed by Alistair Coles
parent d2153f5d5a
commit e5105ffa09
7 changed files with 96 additions and 10 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
etc/internal-client.conf-sample
View File

@ -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]

25
swift/common/internal_client.py
View File

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

8
swift/common/wsgi.py
View File

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

1
swift/container/sharder.py
View File

@ -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)})

61
test/unit/common/test_internal_client.py
View File

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

6
test/unit/common/test_wsgi.py
View File

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

4
test/unit/container/test_sharder.py
View File

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