From beb014b0b1627e5ea8b1e7c90004b01765402647 Mon Sep 17 00:00:00 2001 From: Tim Burke Date: Tue, 29 May 2018 12:47:47 -0700 Subject: [PATCH] Read-only middleware fixups - Expose cluster-wide read-only status in /info - Use a set instead of list for write_methods - Provide a more explicit response when returning 405 - Consider self.read_only in account_read_only - Use return_values instead of callables for more mocks Change-Id: I45387067a69919706800df3a8ca12ae8f5d16d90 --- swift/common/middleware/read_only.py | 29 ++++++++++--------- test/unit/common/middleware/test_read_only.py | 28 ++++-------------- 2 files changed, 22 insertions(+), 35 deletions(-) diff --git a/swift/common/middleware/read_only.py b/swift/common/middleware/read_only.py index f064e70497..c62959c8eb 100644 --- a/swift/common/middleware/read_only.py +++ b/swift/common/middleware/read_only.py @@ -14,7 +14,8 @@ # limitations under the License. from swift.common.constraints import check_account_format from swift.common.swob import HTTPMethodNotAllowed, Request -from swift.common.utils import get_logger, config_true_value +from swift.common.utils import get_logger, config_true_value, \ + register_swift_info from swift.proxy.controllers.base import get_info """ @@ -67,9 +68,9 @@ class ReadOnlyMiddleware(object): self.app = app self.logger = logger or get_logger(conf, log_route='read_only') self.read_only = config_true_value(conf.get('read_only')) - self.write_methods = ['COPY', 'POST', 'PUT'] + self.write_methods = {'COPY', 'POST', 'PUT'} if not config_true_value(conf.get('allow_deletes')): - self.write_methods += ['DELETE'] + self.write_methods.add('DELETE') def __call__(self, env, start_response): req = Request(env) @@ -86,24 +87,23 @@ class ReadOnlyMiddleware(object): dest_account = req.headers.get('Destination-Account') account = check_account_format(req, dest_account) - account_read_only = self.account_read_only(req, account) - if account_read_only is False: - return self.app(env, start_response) - - if self.read_only or account_read_only: - return HTTPMethodNotAllowed()(env, start_response) + if self.account_read_only(req, account): + msg = 'Writes are disabled for this account.' + return HTTPMethodNotAllowed(body=msg)(env, start_response) return self.app(env, start_response) def account_read_only(self, req, account): """ - Returns None if X-Account-Sysmeta-Read-Only is not set. - Returns True or False otherwise. + Check whether an account should be read-only. + + This considers both the cluster-wide config value as well as the + per-account override in X-Account-Sysmeta-Read-Only. """ info = get_info(self.app, req.environ, account, swift_source='RO') read_only = info.get('sysmeta', {}).get('read-only', '') - if read_only == '': - return None + if not read_only: + return self.read_only return config_true_value(read_only) @@ -114,6 +114,9 @@ def filter_factory(global_conf, **local_conf): conf = global_conf.copy() conf.update(local_conf) + if config_true_value(conf.get('read_only')): + register_swift_info('read_only') + def read_only_filter(app): return ReadOnlyMiddleware(app, conf) diff --git a/test/unit/common/middleware/test_read_only.py b/test/unit/common/middleware/test_read_only.py index bdaefb2edb..4b6b63c49a 100644 --- a/test/unit/common/middleware/test_read_only.py +++ b/test/unit/common/middleware/test_read_only.py @@ -32,8 +32,7 @@ def start_response(*args): read_methods = 'GET HEAD'.split() write_methods = 'COPY DELETE POST PUT'.split() -ro_resp = ['

Method Not Allowed

The method is not ' + - 'allowed for this resource.

'] +ro_resp = ['Writes are disabled for this account.'] class TestReadOnly(unittest.TestCase): @@ -81,11 +80,8 @@ class TestReadOnly(unittest.TestCase): ro = read_only.filter_factory(conf)(FakeApp()) ro.logger = FakeLogger() - def get_fake_read_only(*args, **kwargs): - return {'sysmeta': {'read-only': 'true'}} - with mock.patch('swift.common.middleware.read_only.get_info', - get_fake_read_only): + return_value={'sysmeta': {'read-only': 'true'}}): for method in read_methods: req = Request.blank('/v/a') req.method = method @@ -104,11 +100,8 @@ class TestReadOnly(unittest.TestCase): ro = read_only.filter_factory(conf)(FakeApp()) ro.logger = FakeLogger() - def get_fake_read_only(*args, **kwargs): - return {'sysmeta': {'read-only': 'false'}} - with mock.patch('swift.common.middleware.read_only.get_info', - get_fake_read_only): + return_value={'sysmeta': {'read-only': 'false'}}): for method in read_methods + write_methods: req = Request.blank('/v/a') req.method = method @@ -123,11 +116,8 @@ class TestReadOnly(unittest.TestCase): ro = read_only.filter_factory(conf)(FakeApp()) ro.logger = FakeLogger() - def get_fake_read_only(*args, **kwargs): - return {'sysmeta': {'read-only': 'false'}} - with mock.patch('swift.common.middleware.read_only.get_info', - get_fake_read_only): + return_value={'sysmeta': {'read-only': 'false'}}): for method in read_methods + write_methods: req = Request.blank('/v/a') req.method = method @@ -158,11 +148,8 @@ class TestReadOnly(unittest.TestCase): ro = read_only.filter_factory(conf)(FakeApp()) ro.logger = FakeLogger() - def get_fake_read_only(*args, **kwargs): - return {'sysmeta': {'read-only': 'on'}} - with mock.patch('swift.common.middleware.read_only.get_info', - get_fake_read_only): + return_value={'sysmeta': {'read-only': 'on'}}): req = Request.blank('/v/a') req.method = "DELETE" resp = ro(req.environ, start_response) @@ -235,13 +222,10 @@ class TestReadOnly(unittest.TestCase): ro = read_only.filter_factory(conf)(FakeApp()) ro.logger = FakeLogger() - def fake_account_read_only(*args, **kwargs): - return 'true' - with mock.patch( 'swift.common.middleware.read_only.ReadOnlyMiddleware.' + 'account_read_only', - fake_account_read_only): + return_value='true'): headers = {'Destination-Account': 'b'} req = Request.blank('/v/a', headers=headers) req.method = "COPY"