From 365db20275d7798c30f10ee03221561a28bb7a79 Mon Sep 17 00:00:00 2001 From: Alistair Coles Date: Thu, 14 Dec 2023 12:32:28 +0000 Subject: [PATCH] FakeSwift: use HTTPMethodNotAllowed not HTTPNotImplemented If a method is not allowed, real swift proxy server app will return an HTTPMethodNotAllowed response, whereas FakeSwift would previously *raise* HTTPNotImplemented. S3Api deliberately sends requests with method 'TEST' which is not allowed/implemented. To workaround the difference in real and fake swift behaviour, FakeSwift was configured to allow the 'TEST' method, and then in some tests an HTTPMethodNotAllowed response was registered for 'TEST' requests! This patch modifies FakeSwift to return an HTTPMethodNotAllowed response to the incoming request when the request method is not allowed. It is no longer necessary for FakeSwift to support extending the default list of allowed methods. Change-Id: I550d0174e14a5d5a05d26e5cbe9d3353f5da4e8a --- test/unit/common/middleware/helpers.py | 13 +++++------ test/unit/common/middleware/s3api/__init__.py | 6 +---- .../common/middleware/s3api/test_s3api.py | 10 ++++----- test/unit/common/middleware/test_helpers.py | 22 +++++-------------- 4 files changed, 16 insertions(+), 35 deletions(-) diff --git a/test/unit/common/middleware/helpers.py b/test/unit/common/middleware/helpers.py index 3736c47093..c58b6ae5ab 100644 --- a/test/unit/common/middleware/helpers.py +++ b/test/unit/common/middleware/helpers.py @@ -23,7 +23,7 @@ from swift.common.request_helpers import is_user_meta, \ is_object_transient_sysmeta, resolve_etag_is_at_header, \ resolve_ignore_range_header from swift.common.storage_policy import POLICIES -from swift.common.swob import HTTPNotImplemented +from swift.common.swob import HTTPMethodNotAllowed from swift.common.utils import split_path, md5 from test.debug_logger import debug_logger @@ -119,16 +119,13 @@ class FakeSwift(object): * received ``POST /v1/a/c/o?x=y``, if it matches a registered ``POST``, will update uploaded ``/v1/a/c/o`` """ - DEFAULT_ALLOWED_METHODS = [ + ALLOWED_METHODS = [ 'PUT', 'POST', 'DELETE', 'GET', 'HEAD', 'OPTIONS', 'REPLICATE', 'SSYNC', 'UPDATE'] container_existence_skip_cache = 0.0 account_existence_skip_cache = 0.0 - def __init__(self, allowed_methods=None): - self.allowed_methods = set(self.DEFAULT_ALLOWED_METHODS) - if allowed_methods: - self.allowed_methods.update(allowed_methods) + def __init__(self): self._calls = [] self.req_bodies = [] self._unclosed_req_keys = defaultdict(int) @@ -237,8 +234,8 @@ class FakeSwift(object): def __call__(self, env, start_response): method = env['REQUEST_METHOD'] - if method not in self.allowed_methods: - raise HTTPNotImplemented() + if method not in self.ALLOWED_METHODS: + return HTTPMethodNotAllowed()(env, start_response) path, acc, cont, obj = self._parse_path(env) diff --git a/test/unit/common/middleware/s3api/__init__.py b/test/unit/common/middleware/s3api/__init__.py index c6613c5d0e..4ec20255c2 100644 --- a/test/unit/common/middleware/s3api/__init__.py +++ b/test/unit/common/middleware/s3api/__init__.py @@ -39,7 +39,7 @@ class FakeApp(object): def __init__(self): self.remote_user = 'authorized' self._pipeline_final_app = self - self.swift = FakeSwift(allowed_methods=['TEST']) + self.swift = FakeSwift() self.logger = debug_logger() def _update_s3_path_info(self, env): @@ -281,10 +281,6 @@ class S3ApiTestCaseAcl(S3ApiTestCase): self.swift.register('HEAD', path, swob.HTTPNoContent, {}, None), self.swift.register('GET', path, swob.HTTPOk, {}, json.dumps([])), - for account in ('AUTH_test', 'AUTH_X'): - self.swift.register('TEST', '/v1/' + account, - swob.HTTPMethodNotAllowed, {}, None) - # setup sticky ACL headers... grants = [_gen_grant(perm) for perm in PERMISSIONS] self.default_owner = Owner('test:tester', 'test:tester') diff --git a/test/unit/common/middleware/s3api/test_s3api.py b/test/unit/common/middleware/s3api/test_s3api.py index a547505d1c..c76dfd03d6 100644 --- a/test/unit/common/middleware/s3api/test_s3api.py +++ b/test/unit/common/middleware/s3api/test_s3api.py @@ -1440,7 +1440,7 @@ class TestS3ApiMiddleware(S3ApiTestCase): self.s3api.logger.logger.statsd_client.get_increment_counts()) def test_s3api_with_only_s3_token(self): - self.swift = FakeSwift(allowed_methods=['TEST']) + self.swift = FakeSwift() self.keystone_auth = KeystoneAuth( self.swift, {'operator_roles': 'swift-user'}) self.s3_token = S3Token( @@ -1470,7 +1470,7 @@ class TestS3ApiMiddleware(S3ApiTestCase): req.environ['swift.backend_path']) def test_s3api_with_only_s3_token_v3(self): - self.swift = FakeSwift(allowed_methods=['TEST']) + self.swift = FakeSwift() self.keystone_auth = KeystoneAuth( self.swift, {'operator_roles': 'swift-user'}) self.s3_token = S3Token( @@ -1500,7 +1500,7 @@ class TestS3ApiMiddleware(S3ApiTestCase): req.environ['swift.backend_path']) def test_s3api_with_s3_token_and_auth_token(self): - self.swift = FakeSwift(allowed_methods=['TEST']) + self.swift = FakeSwift() self.keystone_auth = KeystoneAuth( self.swift, {'operator_roles': 'swift-user'}) self.auth_token = AuthProtocol( @@ -1555,7 +1555,7 @@ class TestS3ApiMiddleware(S3ApiTestCase): statsd_client.get_increment_counts()) def test_s3api_with_only_s3_token_in_s3acl(self): - self.swift = FakeSwift(allowed_methods=['TEST']) + self.swift = FakeSwift() self.keystone_auth = KeystoneAuth( self.swift, {'operator_roles': 'swift-user'}) self.s3_token = S3Token( @@ -1575,8 +1575,6 @@ class TestS3ApiMiddleware(S3ApiTestCase): # after PUT container so we need to register the resposne here self.swift.register('POST', '/v1/AUTH_TENANT_ID/bucket', swob.HTTPNoContent, {}, None) - self.swift.register('TEST', '/v1/AUTH_TENANT_ID', - swob.HTTPMethodNotAllowed, {}, None) with patch.object(self.s3_token, '_json_request') as mock_req: mock_resp = requests.Response() mock_resp._content = json.dumps(GOOD_RESPONSE_V2).encode('ascii') diff --git a/test/unit/common/middleware/test_helpers.py b/test/unit/common/middleware/test_helpers.py index cf055f36f4..1e2e84bc35 100644 --- a/test/unit/common/middleware/test_helpers.py +++ b/test/unit/common/middleware/test_helpers.py @@ -17,7 +17,7 @@ import unittest from swift.common.storage_policy import POLICIES from swift.common.swob import Request, HTTPOk, HTTPNotFound, \ - HTTPCreated, HeaderKeyDict, HTTPException + HTTPCreated, HeaderKeyDict from swift.common import request_helpers as rh from swift.common.middleware.s3api.utils import sysmeta_header from test.unit.common.middleware.helpers import FakeSwift @@ -26,29 +26,19 @@ from test.unit.common.middleware.helpers import FakeSwift class TestFakeSwift(unittest.TestCase): def test_allowed_methods(self): - def assert_allowed(swift, method): + def do_test(swift, method, exp_status): path = '/v1/a/c/o' swift.register(method, path, HTTPOk, {}, None) req = Request.blank(path) req.method = method - self.assertEqual(200, req.get_response(swift).status_int) - - def assert_disallowed(swift, method): - path = '/v1/a/c/o' - swift.register(method, path, HTTPOk, {}, None) - req = Request.blank(path) - req.method = method - with self.assertRaises(HTTPException) as cm: - req.get_response(swift) - self.assertEqual(501, cm.exception.status_int) + self.assertEqual(exp_status, req.get_response(swift).status_int) for method in ('PUT', 'POST', 'DELETE', 'GET', 'HEAD', 'OPTIONS', 'REPLICATE', 'SSYNC', 'UPDATE'): - assert_allowed(FakeSwift(), method) - assert_allowed(FakeSwift(allowed_methods=['TEST']), 'TEST') + do_test(FakeSwift(), method, 200) - assert_disallowed(FakeSwift(), 'TEST') - assert_allowed(FakeSwift(allowed_methods=['TEST']), 'TEST') + do_test(FakeSwift(), 'TEST', 405) + do_test(FakeSwift(), 'get', 405) def test_not_registered(self): swift = FakeSwift()