Fix cross account upload using --os-storage-url

Removes an account stat from the object upload path.
This stat fails when user is not account admin even
though the user may have container ACL permission to
write objects.

Reduces the severity of the CLI output message when
upload fails to create the given container (this is
not an error since the container may exist - the user
just does not have permission to PUT or POST the
container).

Changes the 'swift upload' exit return code from 1 to
0 if container PUT fails but object PUT succeeds.

For segment uploads, makes the attempt to create the
segment container conditional on it not being the same
as the manifest container. This avoids an unnecessary
container PUT.

Fixes another bug that became apparent: with segmented
upload a container HEAD may be attempted to determine
the policy to be used for the segment container. When
this failed the result dict has headers=None which was
causing an exception in the shell result handler.

Add unit tests for object upload/download and container
list with --os-storage-url option.

Closes-Bug: #1371650
Change-Id: If1f8a02ee7459ea2158ffa6e958f67d299ec529e
This commit is contained in:
Alistair Coles
2014-09-29 18:26:33 +01:00
parent 5d57018707
commit 9593d4b58a
5 changed files with 447 additions and 80 deletions

View File

@@ -96,10 +96,16 @@ class OutputManager(object):
item = item.encode('utf8') item = item.encode('utf8')
print(item, file=stream) print(item, file=stream)
def _print_error(self, item): def _print_error(self, item, count=1):
self.error_count += 1 self.error_count += count
return self._print(item, stream=self.error_stream) return self._print(item, stream=self.error_stream)
def warning(self, msg, *fmt_args):
# print to error stream but do not increment error count
if fmt_args:
msg = msg % fmt_args
self.error_print_pool.submit(self._print_error, msg, count=0)
class MultiThreadingManager(object): class MultiThreadingManager(object):
""" """

View File

@@ -1175,11 +1175,6 @@ class SwiftService(object):
except ValueError: except ValueError:
raise SwiftError('Segment size should be an integer value') raise SwiftError('Segment size should be an integer value')
# Does the account exist?
account_stat = self.stat(options=options)
if not account_stat["success"]:
raise account_stat["error"]
# Try to create the container, just in case it doesn't exist. If this # Try to create the container, just in case it doesn't exist. If this
# fails, it might just be because the user doesn't have container PUT # fails, it might just be because the user doesn't have container PUT
# permissions, so we'll ignore any error. If there's really a problem, # permissions, so we'll ignore any error. If there's really a problem,
@@ -1206,28 +1201,29 @@ class SwiftService(object):
seg_container = container + '_segments' seg_container = container + '_segments'
if options['segment_container']: if options['segment_container']:
seg_container = options['segment_container'] seg_container = options['segment_container']
if not policy_header: if seg_container != container:
# Since no storage policy was specified on the command line, if not policy_header:
# rather than just letting swift pick the default storage # Since no storage policy was specified on the command
# policy, we'll try to create the segments container with the # line, rather than just letting swift pick the default
# same as the upload container # storage policy, we'll try to create the segments
create_containers = [ # container with the same policy as the upload container
self.thread_manager.container_pool.submit( create_containers = [
self._create_container_job, seg_container, self.thread_manager.container_pool.submit(
policy_source=container self._create_container_job, seg_container,
) policy_source=container
] )
else: ]
create_containers = [ else:
self.thread_manager.container_pool.submit( create_containers = [
self._create_container_job, seg_container, self.thread_manager.container_pool.submit(
headers=policy_header self._create_container_job, seg_container,
) headers=policy_header
] )
]
for r in interruptable_as_completed(create_containers): for r in interruptable_as_completed(create_containers):
res = r.result() res = r.result()
yield res yield res
# We maintain a results queue here and a separate thread to monitor # We maintain a results queue here and a separate thread to monitor
# the futures because we want to get results back from potential # the futures because we want to get results back from potential

View File

@@ -817,16 +817,14 @@ def st_upload(parser, args, output_manager):
) )
else: else:
error = r['error'] error = r['error']
if isinstance(error, SwiftError): if 'action' in r and r['action'] == "create_container":
output_manager.error("%s" % error) # it is not an error to be unable to create the
elif isinstance(error, ClientException): # container so print a warning and carry on
if r['action'] == "create_container": if isinstance(error, ClientException):
if 'X-Storage-Policy' in r['headers']: if (r['headers'] and
output_manager.error( 'X-Storage-Policy' in r['headers']):
'Error trying to create container %s with ' msg = ' with Storage Policy %s' % \
'Storage Policy %s', container, r['headers']['X-Storage-Policy'].strip()
r['headers']['X-Storage-Policy'].strip()
)
else: else:
msg = ' '.join(str(x) for x in ( msg = ' '.join(str(x) for x in (
error.http_status, error.http_reason) error.http_status, error.http_reason)
@@ -835,20 +833,15 @@ def st_upload(parser, args, output_manager):
if msg: if msg:
msg += ': ' msg += ': '
msg += error.http_response_content[:60] msg += error.http_response_content[:60]
output_manager.error( msg = ': %s' % msg
'Error trying to create container %r: %s',
container, msg
)
else: else:
output_manager.error("%s" % error) msg = ': %s' % error
output_manager.warning(
'Warning: failed to create container '
'%r%s', container, msg
)
else: else:
if r['action'] == "create_container": output_manager.error("%s" % error)
output_manager.error(
'Error trying to create container %r: %s',
container, error
)
else:
output_manager.error("%s" % error)
except SwiftError as e: except SwiftError as e:
output_manager.error("%s" % e) output_manager.error("%s" % e)

View File

@@ -12,7 +12,9 @@
# implied. # implied.
# See the License for the specific language governing permissions and # See the License for the specific language governing permissions and
# limitations under the License. # limitations under the License.
from genericpath import getmtime
import hashlib
import mock import mock
import os import os
import tempfile import tempfile
@@ -83,6 +85,21 @@ def _make_env(opts, os_opts):
return env return env
def _make_cmd(cmd, opts, os_opts, use_env=False, flags=None, cmd_args=None):
flags = flags or []
if use_env:
# set up fake environment variables and make a minimal command line
env = _make_env(opts, os_opts)
args = _make_args(cmd, {}, {}, separator='-', flags=flags,
cmd_args=cmd_args)
else:
# set up empty environment and make full command line
env = {}
args = _make_args(cmd, opts, os_opts, separator='-', flags=flags,
cmd_args=cmd_args)
return args, env
@mock.patch.dict(os.environ, mocked_os_environ) @mock.patch.dict(os.environ, mocked_os_environ)
class TestShell(unittest.TestCase): class TestShell(unittest.TestCase):
def __init__(self, *args, **kwargs): def __init__(self, *args, **kwargs):
@@ -388,6 +405,28 @@ class TestShell(unittest.TestCase):
'x-object-meta-mtime': mock.ANY}, 'x-object-meta-mtime': mock.ANY},
response_dict={}) response_dict={})
@mock.patch('swiftclient.service.Connection')
def test_upload_segments_to_same_container(self, connection):
# Upload in segments to same container
connection.return_value.head_object.return_value = {
'content-length': '0'}
connection.return_value.attempts = 0
argv = ["", "upload", "container", self.tmpfile, "-S", "10",
"-C", "container"]
with open(self.tmpfile, "wb") as fh:
fh.write(b'12345678901234567890')
swiftclient.shell.main(argv)
connection.return_value.put_container.assert_called_once_with(
'container', {}, response_dict={})
connection.return_value.put_object.assert_called_with(
'container',
self.tmpfile.lstrip('/'),
'',
content_length=0,
headers={'x-object-manifest': mock.ANY,
'x-object-meta-mtime': mock.ANY},
response_dict={})
@mock.patch('swiftclient.service.Connection') @mock.patch('swiftclient.service.Connection')
def test_delete_account(self, connection): def test_delete_account(self, connection):
connection.return_value.get_account.side_effect = [ connection.return_value.get_account.side_effect = [
@@ -692,10 +731,11 @@ class TestSubcommandHelp(unittest.TestCase):
self.assertEqual(out.strip('\n'), expected) self.assertEqual(out.strip('\n'), expected)
class TestParsing(unittest.TestCase): class TestBase(unittest.TestCase):
"""
def setUp(self): Provide some common methods to subclasses
super(TestParsing, self).setUp() """
def _remove_swift_env_vars(self):
self._environ_vars = {} self._environ_vars = {}
keys = list(os.environ.keys()) keys = list(os.environ.keys())
for k in keys: for k in keys:
@@ -703,9 +743,20 @@ class TestParsing(unittest.TestCase):
or k.startswith('OS_')): or k.startswith('OS_')):
self._environ_vars[k] = os.environ.pop(k) self._environ_vars[k] = os.environ.pop(k)
def tearDown(self): def _replace_swift_env_vars(self):
os.environ.update(self._environ_vars) os.environ.update(self._environ_vars)
class TestParsing(TestBase):
def setUp(self):
super(TestParsing, self).setUp()
self._remove_swift_env_vars()
def tearDown(self):
self._replace_swift_env_vars()
super(TestParsing, self).tearDown()
def _make_fake_command(self, result): def _make_fake_command(self, result):
def fake_command(parser, args, thread_manager): def fake_command(parser, args, thread_manager):
result[0], result[1] = swiftclient.shell.parse_args(parser, args) result[0], result[1] = swiftclient.shell.parse_args(parser, args)
@@ -1339,3 +1390,294 @@ class TestAuth(MockHttpTest):
'x-auth-token': token + '_new', 'x-auth-token': token + '_new',
}), }),
]) ])
class TestCrossAccountObjectAccess(TestBase, MockHttpTest):
"""
Tests to verify use of --os-storage-url will actually
result in the object request being sent despite account
read/write access and container write access being denied.
"""
def setUp(self):
super(TestCrossAccountObjectAccess, self).setUp()
self._remove_swift_env_vars()
temp_file = tempfile.NamedTemporaryFile(delete=False)
temp_file.file.write(b'01234567890123456789')
temp_file.file.flush()
self.obj = temp_file.name
self.url = 'http://alternate.com:8080/v1'
# account tests will attempt to access
self.account = 'AUTH_alice'
# keystone returns endpoint for another account
fake_ks = FakeKeystone(endpoint='http://example.com:8080/v1/AUTH_bob',
token='bob_token')
self.fake_ks_import = _make_fake_import_keystone_client(fake_ks)
self.cont = 'c1'
self.cont_path = '/v1/%s/%s' % (self.account, self.cont)
self.obj_path = '%s%s' % (self.cont_path, self.obj)
self.os_opts = {'username': 'bob',
'password': 'password',
'project-name': 'proj_bob',
'auth-url': 'http://example.com:5000/v3',
'storage-url': '%s/%s' % (self.url, self.account)}
self.opts = {'auth-version': '3'}
def tearDown(self):
try:
os.remove(self.obj)
except OSError:
pass
self._replace_swift_env_vars()
super(TestCrossAccountObjectAccess, self).tearDown()
def _make_cmd(self, cmd, cmd_args=None):
return _make_cmd(cmd, self.opts, self.os_opts, cmd_args=cmd_args)
def _fake_cross_account_auth(self, read_ok, write_ok):
def on_request(method, path, *args, **kwargs):
"""
Modify response code to 200 if cross account permissions match.
"""
status = 403
if (path.startswith('/v1/%s/%s' % (self.account, self.cont))
and read_ok and method in ('GET', 'HEAD')):
status = 200
elif (path.startswith('/v1/%s/%s%s'
% (self.account, self.cont, self.obj))
and write_ok and method in ('PUT', 'POST', 'DELETE')):
status = 200
return status
return on_request
def test_upload_with_read_write_access(self):
req_handler = self._fake_cross_account_auth(True, True)
fake_conn = self.fake_http_connection(403, 403,
on_request=req_handler)
args, env = self._make_cmd('upload', cmd_args=[self.cont, self.obj,
'--leave-segments'])
with mock.patch('swiftclient.client._import_keystone_client',
self.fake_ks_import):
with mock.patch('swiftclient.client.http_connection', fake_conn):
with mock.patch.dict(os.environ, env):
with CaptureOutput() as out:
try:
swiftclient.shell.main(args)
except SystemExit as e:
self.fail('Unexpected SystemExit: %s' % e)
self.assertRequests([('PUT', self.cont_path),
('PUT', self.obj_path)])
self.assertEqual(self.obj, out.strip())
expected_err = 'Warning: failed to create container %r: 403 Fake' \
% self.cont
self.assertEqual(expected_err, out.err.strip())
def test_upload_with_write_only_access(self):
req_handler = self._fake_cross_account_auth(False, True)
fake_conn = self.fake_http_connection(403, 403,
on_request=req_handler)
args, env = self._make_cmd('upload', cmd_args=[self.cont, self.obj,
'--leave-segments'])
with mock.patch('swiftclient.client._import_keystone_client',
self.fake_ks_import):
with mock.patch('swiftclient.client.http_connection', fake_conn):
with mock.patch.dict(os.environ, env):
with CaptureOutput() as out:
try:
swiftclient.shell.main(args)
except SystemExit as e:
self.fail('Unexpected SystemExit: %s' % e)
self.assertRequests([('PUT', self.cont_path),
('PUT', self.obj_path)])
self.assertEqual(self.obj, out.strip())
expected_err = 'Warning: failed to create container %r: 403 Fake' \
% self.cont
self.assertEqual(expected_err, out.err.strip())
def test_segment_upload_with_write_only_access(self):
req_handler = self._fake_cross_account_auth(False, True)
fake_conn = self.fake_http_connection(403, 403, 403, 403,
on_request=req_handler)
args, env = self._make_cmd('upload',
cmd_args=[self.cont, self.obj,
'--leave-segments',
'--segment-size=10',
'--segment-container=%s'
% self.cont])
with mock.patch('swiftclient.client._import_keystone_client',
self.fake_ks_import):
with mock.patch('swiftclient.client.http_connection', fake_conn):
with mock.patch.dict(os.environ, env):
with CaptureOutput() as out:
try:
swiftclient.shell.main(args)
except SystemExit as e:
self.fail('Unexpected SystemExit: %s' % e)
segment_time = getmtime(self.obj)
segment_path_0 = '%s/%f/20/10/00000000' % (self.obj_path, segment_time)
segment_path_1 = '%s/%f/20/10/00000001' % (self.obj_path, segment_time)
# Note that the order of segment PUTs cannot be asserted, so test for
# existence in request log individually
self.assert_request(('PUT', self.cont_path))
self.assert_request(('PUT', segment_path_0))
self.assert_request(('PUT', segment_path_1))
self.assert_request(('PUT', self.obj_path))
self.assertTrue(self.obj in out.out)
expected_err = 'Warning: failed to create container %r: 403 Fake' \
% self.cont
self.assertEqual(expected_err, out.err.strip())
def test_upload_with_no_access(self):
fake_conn = self.fake_http_connection(403, 403)
args, env = self._make_cmd('upload', cmd_args=[self.cont, self.obj,
'--leave-segments'])
with mock.patch('swiftclient.client._import_keystone_client',
self.fake_ks_import):
with mock.patch('swiftclient.client.http_connection', fake_conn):
with mock.patch.dict(os.environ, env):
with CaptureOutput() as out:
try:
swiftclient.shell.main(args)
self.fail('Expected SystemExit')
except SystemExit:
pass
self.assertRequests([('PUT', self.cont_path),
('PUT', self.obj_path)])
expected_err = 'Object PUT failed: http://1.2.3.4%s 403 Fake' \
% self.obj_path
self.assertTrue(expected_err in out.err)
self.assertEqual('', out)
def test_download_with_read_write_access(self):
req_handler = self._fake_cross_account_auth(True, True)
empty_str_etag = 'd41d8cd98f00b204e9800998ecf8427e'
fake_conn = self.fake_http_connection(403, on_request=req_handler,
etags=[empty_str_etag])
args, env = self._make_cmd('download', cmd_args=[self.cont,
self.obj.lstrip('/'),
'--no-download'])
with mock.patch('swiftclient.client._import_keystone_client',
self.fake_ks_import):
with mock.patch('swiftclient.client.http_connection', fake_conn):
with mock.patch.dict(os.environ, env):
with CaptureOutput() as out:
try:
swiftclient.shell.main(args)
except SystemExit as e:
self.fail('Unexpected SystemExit: %s' % e)
self.assertRequests([('GET', self.obj_path)])
self.assertTrue(out.out.startswith(self.obj.lstrip('/')))
self.assertEqual('', out.err)
def test_download_with_read_only_access(self):
req_handler = self._fake_cross_account_auth(True, False)
empty_str_etag = 'd41d8cd98f00b204e9800998ecf8427e'
fake_conn = self.fake_http_connection(403, on_request=req_handler,
etags=[empty_str_etag])
args, env = self._make_cmd('download', cmd_args=[self.cont,
self.obj.lstrip('/'),
'--no-download'])
with mock.patch('swiftclient.client._import_keystone_client',
self.fake_ks_import):
with mock.patch('swiftclient.client.http_connection', fake_conn):
with mock.patch.dict(os.environ, env):
with CaptureOutput() as out:
try:
swiftclient.shell.main(args)
except SystemExit as e:
self.fail('Unexpected SystemExit: %s' % e)
self.assertRequests([('GET', self.obj_path)])
self.assertTrue(out.out.startswith(self.obj.lstrip('/')))
self.assertEqual('', out.err)
def test_download_with_no_access(self):
fake_conn = self.fake_http_connection(403)
args, env = self._make_cmd('download', cmd_args=[self.cont,
self.obj.lstrip('/'),
'--no-download'])
with mock.patch('swiftclient.client._import_keystone_client',
self.fake_ks_import):
with mock.patch('swiftclient.client.http_connection', fake_conn):
with mock.patch.dict(os.environ, env):
with CaptureOutput() as out:
try:
swiftclient.shell.main(args)
self.fail('Expected SystemExit')
except SystemExit:
pass
self.assertRequests([('GET', self.obj_path)])
path = '%s%s' % (self.cont, self.obj)
expected_err = 'Error downloading object %r' % path
self.assertTrue(out.err.startswith(expected_err))
self.assertEqual('', out)
def test_list_with_read_access(self):
req_handler = self._fake_cross_account_auth(True, False)
resp_body = '{}'
m = hashlib.md5()
m.update(resp_body.encode())
etag = m.hexdigest()
fake_conn = self.fake_http_connection(403, on_request=req_handler,
etags=[etag],
body=resp_body)
args, env = self._make_cmd('download', cmd_args=[self.cont])
with mock.patch('swiftclient.client._import_keystone_client',
self.fake_ks_import):
with mock.patch('swiftclient.client.http_connection', fake_conn):
with mock.patch.dict(os.environ, env):
with CaptureOutput() as out:
try:
swiftclient.shell.main(args)
except SystemExit as e:
self.fail('Unexpected SystemExit: %s' % e)
self.assertRequests([('GET', '%s?format=json' % self.cont_path)])
self.assertEqual('', out)
self.assertEqual('', out.err)
def test_list_with_no_access(self):
fake_conn = self.fake_http_connection(403)
args, env = self._make_cmd('download', cmd_args=[self.cont])
with mock.patch('swiftclient.client._import_keystone_client',
self.fake_ks_import):
with mock.patch('swiftclient.client.http_connection', fake_conn):
with mock.patch.dict(os.environ, env):
with CaptureOutput() as out:
try:
swiftclient.shell.main(args)
self.fail('Expected SystemExit')
except SystemExit:
pass
self.assertRequests([('GET', '%s?format=json' % self.cont_path)])
self.assertEqual('', out)
self.assertTrue(out.err.startswith('Container GET failed:'))
class TestCrossAccountObjectAccessUsingEnv(TestCrossAccountObjectAccess):
"""
Repeat super-class tests using environment variables rather than command
line to set options.
"""
def _make_cmd(self, cmd, cmd_args=None):
return _make_cmd(cmd, self.opts, self.os_opts, cmd_args=cmd_args,
use_env=True)

View File

@@ -216,6 +216,7 @@ class MockHttpTest(testtools.TestCase):
storage_url = kwargs.get('storage_url') storage_url = kwargs.get('storage_url')
auth_token = kwargs.get('auth_token') auth_token = kwargs.get('auth_token')
exc = kwargs.get('exc') exc = kwargs.get('exc')
on_request = kwargs.get('on_request')
def wrapper(url, proxy=None, cacert=None, insecure=False, def wrapper(url, proxy=None, cacert=None, insecure=False,
ssl_compression=True): ssl_compression=True):
@@ -245,6 +246,9 @@ class MockHttpTest(testtools.TestCase):
conn.resp.has_been_read = True conn.resp.has_been_read = True
return _orig_read(*args, **kwargs) return _orig_read(*args, **kwargs)
conn.resp.read = read conn.resp.read = read
if on_request:
status = on_request(method, url, *args, **kwargs)
conn.resp.status = status
if auth_token: if auth_token:
headers = args[1] headers = args[1]
self.assertTrue('X-Auth-Token' in headers) self.assertTrue('X-Auth-Token' in headers)
@@ -258,7 +262,12 @@ class MockHttpTest(testtools.TestCase):
if exc: if exc:
raise exc raise exc
return conn.resp return conn.resp
def putrequest(path, data=None, headers=None, **kwargs):
request('PUT', path, data, headers, **kwargs)
conn.request = request conn.request = request
conn.putrequest = putrequest
def getresponse(): def getresponse():
return conn.resp return conn.resp
@@ -288,6 +297,34 @@ class MockHttpTest(testtools.TestCase):
orig_assertEqual = unittest.TestCase.assertEqual orig_assertEqual = unittest.TestCase.assertEqual
def assert_request_equal(self, expected, real_request):
method, path = expected[:2]
if urlparse(path).scheme:
match_path = real_request['full_path']
else:
match_path = real_request['path']
self.assertEqual((method, path), (real_request['method'],
match_path))
if len(expected) > 2:
body = expected[2]
real_request['expected'] = body
err_msg = 'Body mismatch for %(method)s %(path)s, ' \
'expected %(expected)r, and got %(body)r' % real_request
self.orig_assertEqual(body, real_request['body'], err_msg)
if len(expected) > 3:
headers = expected[3]
for key, value in headers.items():
real_request['key'] = key
real_request['expected_value'] = value
real_request['value'] = real_request['headers'].get(key)
err_msg = (
'Header mismatch on %(key)r, '
'expected %(expected_value)r and got %(value)r '
'for %(method)s %(path)s %(headers)r' % real_request)
self.orig_assertEqual(value, real_request['value'],
err_msg)
def assertRequests(self, expected_requests): def assertRequests(self, expected_requests):
""" """
Make sure some requests were made like you expected, provide a list of Make sure some requests were made like you expected, provide a list of
@@ -295,33 +332,26 @@ class MockHttpTest(testtools.TestCase):
""" """
real_requests = self.iter_request_log() real_requests = self.iter_request_log()
for expected in expected_requests: for expected in expected_requests:
method, path = expected[:2]
real_request = next(real_requests) real_request = next(real_requests)
if urlparse(path).scheme: self.assert_request_equal(expected, real_request)
match_path = real_request['full_path']
else:
match_path = real_request['path']
self.assertEqual((method, path), (real_request['method'],
match_path))
if len(expected) > 2:
body = expected[2]
real_request['expected'] = body
err_msg = 'Body mismatch for %(method)s %(path)s, ' \
'expected %(expected)r, and got %(body)r' % real_request
self.orig_assertEqual(body, real_request['body'], err_msg)
if len(expected) > 3: def assert_request(self, expected_request):
headers = expected[3] """
for key, value in headers.items(): Make sure a request was made as expected. Provide the
real_request['key'] = key expected request in the form of [(method, path), ...]
real_request['expected_value'] = value """
real_request['value'] = real_request['headers'].get(key) real_requests = self.iter_request_log()
err_msg = ( for real_request in real_requests:
'Header mismatch on %(key)r, ' try:
'expected %(expected_value)r and got %(value)r ' self.assert_request_equal(expected_request, real_request)
'for %(method)s %(path)s %(headers)r' % real_request) break
self.orig_assertEqual(value, real_request['value'], except AssertionError:
err_msg) pass
else:
raise AssertionError(
"Expected request %s not found in actual requests %s"
% (expected_request, self.request_log)
)
def validateMockedRequestsConsumed(self): def validateMockedRequestsConsumed(self):
if not self.fake_connect: if not self.fake_connect: