openstack/swift
Code Issues Proposed changes

Allow replication servers to handle all request methods

Browse Source 
Previously, the replication_server setting could take one of three
states:

 * If unspecified, the server would handle all available methods.
 * If "true", "yes", "on", etc. it would only handle replication
   methods (REPLICATE, SSYNC).
 * If any other value (including blank), it would only handle
   non-replication methods.

However, because SSYNC tunnels PUTs, POSTs, and DELETEs through
the same object-server app that's responding to SSYNC, setting
`replication_server = true` would break the protocol. This has
been the case ever since ssync was introduced.

Now, get rid of that second state -- operators can still set
`replication_server = false` as a principle-of-least-privilege guard
to ensure proxy-servers can't make replication requests, but replication
servers will be able to serve all traffic. This will allow replication
servers to be used as general internal-to-the-cluster endpoints, leaving
non-replication servers to handle client-driven traffic.

Closes-Bug: #1446873
Change-Id: Ica2b41a52d11cb10c94fa8ad780a201318c4fc87
This commit is contained in:
Tim Burke 2020-07-07 21:28:36 -07:00
parent 0dbf3d0a95
commit 9eb81f6e69
8 changed files with 73 additions and 67 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

14
etc/account-server.conf-sample
View File

@ -91,13 +91,13 @@ use = egg:swift#account
# set log_requests = true
# set log_address = /dev/log
#
# Configure parameter for creating specific server
# To handle all verbs, including replication verbs, do not specify
# "replication_server" (this is the default). To only handle replication,
# set to a True value (e.g. "True" or "1"). To handle only non-replication
# verbs, set to "False". Unless you have a separate replication network, you
# should not specify any value for "replication_server". Default is empty.
# replication_server = false
# You can disable REPLICATE handling (default is to allow it). When deploying
# a cluster with a separate replication network, you'll want multiple
# account-server processes running: one for client-driven traffic and another
# for replication traffic. The server handling client-driven traffic may set
# this to false. If there is only one account-server process, leave this as
# true.
# replication_server = true
#
# You can set scheduling priority of processes. Niceness values range from -20
# (most favorable to the process) to 19 (least favorable to the process).

14
etc/container-server.conf-sample
View File

@ -101,13 +101,13 @@ use = egg:swift#container
# conn_timeout = 0.5
# allow_versions = false
#
# Configure parameter for creating specific server
# To handle all verbs, including replication verbs, do not specify
# "replication_server" (this is the default). To only handle replication,
# set to a True value (e.g. "True" or "1"). To handle only non-replication
# verbs, set to "False". Unless you have a separate replication network, you
# should not specify any value for "replication_server".
# replication_server = false
# You can disable REPLICATE handling (default is to allow it). When deploying
# a cluster with a separate replication network, you'll want multiple
# container-server processes running: one for client-driven traffic and another
# for replication traffic. The server handling client-driven traffic may set
# this to false. If there is only one container-server process, leave this as
# true.
# replication_server = true
#
# You can set scheduling priority of processes. Niceness values range from -20
# (most favorable to the process) to 19 (least favorable to the process).

14
etc/object-server.conf-sample
View File

@ -156,13 +156,13 @@ use = egg:swift#object
#
# eventlet_tpool_num_threads = auto
# Configure parameter for creating specific server
# To handle all verbs, including replication verbs, do not specify
# "replication_server" (this is the default). To only handle replication,
# set to a True value (e.g. "True" or "1"). To handle only non-replication
# verbs, set to "False". Unless you have a separate replication network, you
# should not specify any value for "replication_server".
# replication_server = false
# You can disable REPLICATE and SSYNC handling (default is to allow it). When
# deploying a cluster with a separate replication network, you'll want multiple
# object-server processes running: one for client-driven traffic and another
# for replication traffic. The server handling client-driven traffic may set
# this to false. If there is only one object-server process, leave this as
# true.
# replication_server = true
#
# Set to restrict the number of concurrent incoming SSYNC requests
# Set to 0 for unlimited

29
swift/common/base_storage_server.py
View File

@ -27,10 +27,8 @@ class BaseStorageServer(object):
def __init__(self, conf, **kwargs):
self._allowed_methods = None
replication_server = conf.get('replication_server', None)
if replication_server is not None:
replication_server = config_true_value(replication_server)
self.replication_server = replication_server
self.replication_server = config_true_value(
conf.get('replication_server', 'true'))
self.log_format = conf.get('log_format', LOG_LINE_DEFAULT_FORMAT)
self.anonymization_method = conf.get('log_anonymization_method', 'md5')
self.anonymization_salt = conf.get('log_anonymization_salt', '')
@ -45,22 +43,13 @@ class BaseStorageServer(object):
if self._allowed_methods is None:
self._allowed_methods = []
all_methods = inspect.getmembers(self, predicate=callable)
if self.replication_server is True:
for name, m in all_methods:
if (getattr(m, 'publicly_accessible', False) and
getattr(m, 'replication', False)):
self._allowed_methods.append(name)
elif self.replication_server is False:
for name, m in all_methods:
if (getattr(m, 'publicly_accessible', False) and not
getattr(m, 'replication', False)):
self._allowed_methods.append(name)
elif self.replication_server is None:
for name, m in all_methods:
if getattr(m, 'publicly_accessible', False):
self._allowed_methods.append(name)
for name, m in all_methods:
if not getattr(m, 'publicly_accessible', False):
continue
if getattr(m, 'replication', False) and \
not self.replication_server:
continue
self._allowed_methods.append(name)
self._allowed_methods.sort()
return self._allowed_methods

11
test/unit/account/test_server.py
View File

@ -2456,7 +2456,7 @@ class TestAccountController(unittest.TestCase):
def test_serv_reserv(self):
# Test replication_server flag was set from configuration file.
conf = {'devices': self.testdir, 'mount_check': 'false'}
self.assertIsNone(AccountController(conf).replication_server)
self.assertTrue(AccountController(conf).replication_server)
for val in [True, '1', 'True', 'true']:
conf['replication_server'] = val
self.assertTrue(AccountController(conf).replication_server)
@ -2549,7 +2549,7 @@ class TestAccountController(unittest.TestCase):
response = self.controller.__call__(env, start_response)
self.assertEqual(response, answer)
def test_call_incorrect_replication_method(self):
def test_replicaiton_server_call_all_methods(self):
inbuf = BytesIO()
errbuf = StringIO()
outbuf = StringIO()
@ -2560,14 +2560,15 @@ class TestAccountController(unittest.TestCase):
def start_response(*args):
outbuf.write(args[0])
obj_methods = ['DELETE', 'PUT', 'HEAD', 'GET', 'POST', 'OPTIONS']
obj_methods = ['PUT', 'HEAD', 'GET', 'POST', 'DELETE', 'OPTIONS']
for method in obj_methods:
env = {'REQUEST_METHOD': method,
'SCRIPT_NAME': '',
'PATH_INFO': '/sda1/p/a/c',
'PATH_INFO': '/sda1/p/a',
'SERVER_NAME': '127.0.0.1',
'SERVER_PORT': '8080',
'SERVER_PROTOCOL': 'HTTP/1.0',
'HTTP_X_TIMESTAMP': next(self.ts).internal,
'CONTENT_LENGTH': '0',
'wsgi.version': (1, 0),
'wsgi.url_scheme': 'http',
@ -2578,7 +2579,7 @@ class TestAccountController(unittest.TestCase):
'wsgi.run_once': False}
self.controller(env, start_response)
self.assertEqual(errbuf.getvalue(), '')
self.assertEqual(outbuf.getvalue()[:4], '405 ')
self.assertIn(outbuf.getvalue()[:4], ('200 ', '201 ', '204 '))
def test__call__raise_timeout(self):
inbuf = WsgiBytesIO()

25
test/unit/common/test_base_storage_server.py
View File

@ -20,7 +20,7 @@ from swift.common.base_storage_server import BaseStorageServer
from tempfile import mkdtemp
from swift import __version__ as swift_version
from swift.common.swob import Request
from swift.common.utils import get_logger, public
from swift.common.utils import get_logger, public, replication
from shutil import rmtree
@ -40,6 +40,18 @@ class FakeANOTHER(FakeOPTIONS):
"""this is to test adding to allowed_methods"""
pass
@replication
@public
def REPLICATE(self):
"""this is to test replication_server"""
pass
@public
@replication
def REPLICATE2(self):
"""this is to test replication_server"""
pass
class TestBaseStorageServer(unittest.TestCase):
"""Test swift.common.base_storage_server"""
@ -73,18 +85,20 @@ class TestBaseStorageServer(unittest.TestCase):
# test that a subclass can add allowed methods
allowed_methods_test = FakeANOTHER(conf).allowed_methods
allowed_methods_test.sort()
self.assertEqual(allowed_methods_test, ['ANOTHER', 'OPTIONS'])
self.assertEqual(allowed_methods_test, [
'ANOTHER', 'OPTIONS'])
conf = {'devices': self.testdir, 'mount_check': 'false',
'replication_server': 'true'}
# test what's available in the base class
allowed_methods_test = FakeOPTIONS(conf).allowed_methods
self.assertEqual(allowed_methods_test, [])
self.assertEqual(allowed_methods_test, ['OPTIONS'])
# test that a subclass can add allowed methods
allowed_methods_test = FakeANOTHER(conf).allowed_methods
self.assertEqual(allowed_methods_test, [])
self.assertEqual(allowed_methods_test, [
'ANOTHER', 'OPTIONS', 'REPLICATE', 'REPLICATE2'])
conf = {'devices': self.testdir, 'mount_check': 'false'}
@ -95,7 +109,8 @@ class TestBaseStorageServer(unittest.TestCase):
# test that a subclass can add allowed methods
allowed_methods_test = FakeANOTHER(conf).allowed_methods
allowed_methods_test.sort()
self.assertEqual(allowed_methods_test, ['ANOTHER', 'OPTIONS'])
self.assertEqual(allowed_methods_test, [
'ANOTHER', 'OPTIONS', 'REPLICATE', 'REPLICATE2'])
def test_OPTIONS_error(self):
msg = 'Storage nodes have not implemented the Server type.'

9
test/unit/container/test_server.py
View File

@ -4819,7 +4819,7 @@ class TestContainerController(unittest.TestCase):
# Test replication_server flag was set from configuration file.
container_controller = container_server.ContainerController
conf = {'devices': self.testdir, 'mount_check': 'false'}
self.assertIsNone(container_controller(conf).replication_server)
self.assertTrue(container_controller(conf).replication_server)
for val in [True, '1', 'True', 'true']:
conf['replication_server'] = val
self.assertTrue(container_controller(conf).replication_server)
@ -4917,7 +4917,7 @@ class TestContainerController(unittest.TestCase):
self.assertEqual(response, answer)
self.assertEqual(outbuf.getvalue()[:4], '405 ')
def test_call_incorrect_replication_method(self):
def test_replication_server_call_all_methods(self):
inbuf = BytesIO()
errbuf = StringIO()
outbuf = StringIO()
@ -4929,7 +4929,7 @@ class TestContainerController(unittest.TestCase):
"""Sends args to outbuf"""
outbuf.writelines(status)
obj_methods = ['DELETE', 'PUT', 'HEAD', 'GET', 'POST', 'OPTIONS']
obj_methods = ['PUT', 'HEAD', 'GET', 'POST', 'DELETE', 'OPTIONS']
for method in obj_methods:
env = {'REQUEST_METHOD': method,
'SCRIPT_NAME': '',
@ -4937,6 +4937,7 @@ class TestContainerController(unittest.TestCase):
'SERVER_NAME': '127.0.0.1',
'SERVER_PORT': '8080',
'SERVER_PROTOCOL': 'HTTP/1.0',
'HTTP_X_TIMESTAMP': next(self.ts).internal,
'CONTENT_LENGTH': '0',
'wsgi.version': (1, 0),
'wsgi.url_scheme': 'http',
@ -4947,7 +4948,7 @@ class TestContainerController(unittest.TestCase):
'wsgi.run_once': False}
self.controller(env, start_response)
self.assertEqual(errbuf.getvalue(), '')
self.assertEqual(outbuf.getvalue()[:4], '405 ')
self.assertIn(outbuf.getvalue()[:4], ('200 ', '201 ', '204 '))
def test__call__raise_timeout(self):
inbuf = WsgiBytesIO()

24
test/unit/obj/test_server.py
View File

@ -3666,10 +3666,8 @@ class TestObjectController(unittest.TestCase):
def _create_ondisk_fragments(self, policy):
# Create some on disk files...
ts_iter = make_timestamp_iter()
# PUT at ts_0
ts_0 = next(ts_iter)
ts_0 = next(self.ts)
body = b'OLDER'
headers = {'X-Timestamp': ts_0.internal,
'Content-Length': '5',
@ -3689,7 +3687,7 @@ class TestObjectController(unittest.TestCase):
self.assertEqual(resp.status_int, 201)
# POST at ts_1
ts_1 = next(ts_iter)
ts_1 = next(self.ts)
headers = {'X-Timestamp': ts_1.internal,
'X-Backend-Storage-Policy-Index': int(policy)}
headers['X-Object-Meta-Test'] = 'abc'
@ -3700,7 +3698,7 @@ class TestObjectController(unittest.TestCase):
self.assertEqual(resp.status_int, 202)
# PUT again at ts_2 but without making the data file durable
ts_2 = next(ts_iter)
ts_2 = next(self.ts)
body = b'NEWER'
headers = {'X-Timestamp': ts_2.internal,
'Content-Length': '5',
@ -7165,8 +7163,8 @@ class TestObjectController(unittest.TestCase):
def test_serv_reserv(self):
# Test replication_server flag was set from configuration file.
conf = {'devices': self.testdir, 'mount_check': 'false'}
self.assertEqual(
object_server.ObjectController(conf).replication_server, None)
self.assertTrue(
object_server.ObjectController(conf).replication_server)
for val in [True, '1', 'True', 'true']:
conf['replication_server'] = val
self.assertTrue(
@ -7276,8 +7274,8 @@ class TestObjectController(unittest.TestCase):
' /sda1/p/a/c/o" 405 91 "-" "-" "-" 1.0000 "-"'
' 1234 -'])
def test_call_incorrect_replication_method(self):
inbuf = StringIO()
def test_replication_server_call_all_methods(self):
inbuf = WsgiBytesIO()
errbuf = StringIO()
outbuf = StringIO()
self.object_controller = object_server.ObjectController(
@ -7288,14 +7286,16 @@ class TestObjectController(unittest.TestCase):
"""Sends args to outbuf"""
outbuf.write(args[0])
obj_methods = ['DELETE', 'PUT', 'HEAD', 'GET', 'POST', 'OPTIONS']
obj_methods = ['PUT', 'HEAD', 'GET', 'POST', 'DELETE', 'OPTIONS']
for method in obj_methods:
env = {'REQUEST_METHOD': method,
'HTTP_X_TIMESTAMP': next(self.ts).internal,
'SCRIPT_NAME': '',
'PATH_INFO': '/sda1/p/a/c',
'PATH_INFO': '/sda1/p/a/c/o',
'SERVER_NAME': '127.0.0.1',
'SERVER_PORT': '8080',
'SERVER_PROTOCOL': 'HTTP/1.0',
'CONTENT_TYPE': 'text/plain',
'CONTENT_LENGTH': '0',
'wsgi.version': (1, 0),
'wsgi.url_scheme': 'http',
@ -7306,7 +7306,7 @@ class TestObjectController(unittest.TestCase):
'wsgi.run_once': False}
self.object_controller(env, start_response)
self.assertEqual(errbuf.getvalue(), '')
self.assertEqual(outbuf.getvalue()[:4], '405 ')
self.assertIn(outbuf.getvalue()[:4], ('201 ', '204 ', '200 '))
def test_create_reserved_namespace_object(self):
path = '/sda1/p/a/%sc/%so' % (utils.RESERVED_STR, utils.RESERVED_STR)