Not satisfied with the code complexity/ readability/ maintainability, so refactor a bit. Fix a couple bugs in the process. Add unit tests.

This commit is contained in:
Kris Raney 2016-01-21 10:43:43 -06:00
parent d699df9c3d
commit 4ad20f6217
4 changed files with 517 additions and 112 deletions

View File

@ -245,14 +245,16 @@ def describe(Bucket,
del(data['ResponseMetadata'])
result[key] = data
result['Tagging'] = {}
tags = {}
try:
data = conn.get_bucket_tagging(Bucket=Bucket)
for tagdef in data.get('TagSet'):
result['Tagging'][tagdef.get('Key')] = tagdef.get('Value')
tags[tagdef.get('Key')] = tagdef.get('Value')
except ClientError as e:
if not e.response.get('Error', {}).get('Code') == 'NoSuchTagSet':
raise
if tags:
result['Tagging'] = tags
return {'bucket': result}
except ClientError as e:
err = salt.utils.boto3.get_error(e)

View File

@ -170,6 +170,7 @@ def _acl_to_grant(ACL, owner_canonical_id):
for item in ret.get('Grants'):
if 'Type' in item.get('Grantee',()):
del item['Grantee']['Type']
# If AccessControlPolicy is set, other options are not allowed
return ret
ret = {
'Grants': [{
@ -234,11 +235,17 @@ def _acl_to_grant(ACL, owner_canonical_id):
}
elif kind == 'id':
grantee = {
# No API provides this info, so the result will never
# match, and we will always update. Result is still
# idempotent
# 'DisplayName': ???,
'ID': val
}
else:
grantee = {
# No API provides this info, so the result will never
# match, and we will always update. Result is still
# idempotent
# 'DisplayName': ???,
# 'ID': ???
}
@ -263,6 +270,48 @@ def _get_role_arn(name, region=None, key=None, keyid=None, profile=None):
return 'arn:aws:iam::{0}:role/{1}'.format(account_id, name)
def _compare_json(current, desired, region, key, keyid, profile):
return json_objs_equal(current, desired)
def _compare_acl(current, desired, region, key, keyid, profile):
'''
ACLs can be specified using macro-style names that get expanded to
something more complex. There's no predictable way to reverse it.
So expand all syntactic sugar in our input, and compare against that
rather than the input itself.
'''
ocid = _get_canonical_id(region, key, keyid, profile)
return json_objs_equal(current, _acl_to_grant(desired, ocid))
def _compare_policy(current, desired, region, key, keyid, profile):
'''
Policy discription is always returned as a JSON string. Comparison
should be object-to-object, since order is not significant in JSON
'''
if isinstance(desired, string_types):
desired = json.loads(desired)
if current is not None:
temp = current.get('Policy')
if isinstance(temp, string_types):
current = {'Policy': json.loads(temp)}
else:
current = None
return json_objs_equal(current, desired)
def _compare_replication(current, desired, region, key, keyid, profile):
'''
Replication accepts a non-ARN role name, but always returns an ARN
'''
if desired is not None and desired.get('Role'):
desired = deepcopy(desired)
desired['Role'] = _get_role_arn(desired['Role'],
region=region, key=key, keyid=keyid, profile=profile)
return json_objs_equal(current, desired)
def present(name, Bucket,
LocationConstraint=None,
ACL=None,
@ -374,7 +423,7 @@ def present(name, Bucket,
ret['comment'] = 'Failed to create bucket: {0}.'.format(r['error']['message'])
return ret
for func, testval, funcargs in (
for setter, testval, funcargs in (
('put_acl', ACL, ACL),
('put_cors', CORSRules, {"CORSRules": CORSRules}),
('put_lifecycle_configuration', LifecycleConfiguration, {"Rules":LifecycleConfiguration}),
@ -389,7 +438,7 @@ def present(name, Bucket,
('put_website', Website, Website),
):
if testval is not None:
r = __salt__['boto_s3_bucket.{0}'.format(func)](Bucket=Bucket,
r = __salt__['boto_s3_bucket.{0}'.format(setter)](Bucket=Bucket,
region=region, key=key, keyid=keyid, profile=profile,
**funcargs)
if not r.get('updated'):
@ -422,71 +471,44 @@ def present(name, Bucket,
if not bool(Versioning) and _describe.get('Versioning') is not None:
Versioning = {'Status': 'Suspended'}
# Policy discription is always returned as a JSON string, but may be input
# as a loaded JSON object
if isinstance(Policy, string_types):
policy_compare = None
else:
policy_compare = _describe.get('Policy',{}).get('Policy')
if isinstance(policy_compare, string_types):
policy_compare = {'Policy': json.loads(policy_compare)}
else:
policy_compare = None
# Replication accepts a non-ARN role name, but always returns an ARN
replication_compare = None
if Replication is not None and Replication.get('Role'):
replication_compare = deepcopy(Replication)
replication_compare['Role'] = _get_role_arn(replication_compare['Role'],
region=region, key=key, keyid=keyid, profile=profile)
ocid = _get_canonical_id(region, key, keyid, profile)
config_items = [
# ACLs can be specified using macro-style names that get expanded to
# something more complex. There's no predictable way to reverse it.
# So expand our input, and compare against that rather than the
# input itself. Also, ACL can't be deleted, only updated
('ACL', 'put_acl', _describe.get('ACL'),
None,
_acl_to_grant(ACL, ocid),
ACL,
None),
('CORS', 'put_cors', _describe.get('CORS'),
None,
None,
{"CORSRules": CORSRules} if CORSRules else None,
'delete_cors'),
('LifecycleConfiguration', 'put_lifecycle_configuration', _describe.get('LifecycleConfiguration'),
None,
None,
{"Rules": LifecycleConfiguration} if LifecycleConfiguration else None,
'delete_lifecycle_configuration'),
('Logging', 'put_logging', _describe.get('Logging',{}).get('LoggingEnabled'),
None,
None,
Logging,
None),
('NotificationConfiguration', 'put_notification_configuration', _describe.get('NotificationConfiguration'),
None,
None,
NotificationConfiguration,
None),
# Load JSON returned value to an object before comparing to desired
# state
('Policy', 'put_policy', _describe.get('Policy'),
policy_compare,
None,
{"Policy": Policy} if Policy else None,
'delete_policy'),
('RequestPayment', 'put_request_payment', _describe.get('RequestPayment'), None, None, RequestPayment, None),
('Tagging', 'put_tagging', Tagging, None, None, Tagging, 'delete_tagging'),
('Website', 'put_website', Website, None, None, Website, 'delete_website'),
('ACL', 'put_acl',
_describe.get('ACL'), _compare_acl, ACL,
None),
('CORS', 'put_cors',
_describe.get('CORS'), _compare_json, {"CORSRules": CORSRules} if CORSRules else None,
'delete_cors'),
('LifecycleConfiguration', 'put_lifecycle_configuration',
_describe.get('LifecycleConfiguration'), _compare_json, {"Rules": LifecycleConfiguration} if LifecycleConfiguration else None,
'delete_lifecycle_configuration'),
('Logging', 'put_logging',
_describe.get('Logging',{}).get('LoggingEnabled'), _compare_json, Logging,
None),
('NotificationConfiguration', 'put_notification_configuration',
_describe.get('NotificationConfiguration'), _compare_json, NotificationConfiguration,
None),
('Policy', 'put_policy',
_describe.get('Policy'), _compare_policy, {"Policy": Policy} if Policy else None,
'delete_policy'),
('RequestPayment', 'put_request_payment',
_describe.get('RequestPayment'), _compare_json, RequestPayment,
None),
('Tagging', 'put_tagging',
_describe.get('Tagging'), _compare_json, Tagging,
'delete_tagging'),
('Website', 'put_website',
_describe.get('Website'), _compare_json, Website,
'delete_website'),
]
versioning_item = ('Versioning', 'put_versioning', _describe.get('Versioning'), None, None, Versioning, None)
versioning_item = ('Versioning', 'put_versioning',
_describe.get('Versioning'), _compare_json, Versioning,
None)
# Substitute full ARN into desired state for comparison
replication_item = ('Replication', 'put_replication', _describe.get('Replication',{}).get('ReplicationConfiguration'), None, replication_compare, Replication, 'delete_replication')
replication_item = ('Replication', 'put_replication',
_describe.get('Replication',{}).get('ReplicationConfiguration'), _compare_replication, Replication,
'delete_replication')
# versioning must be turned on before replication can be on, but replication
# versioning must be turned on before replication can be on, thus replication
# must be turned off before versioning can be off
if Replication is not None:
# replication will be on, must deal with versioning first
@ -497,8 +519,8 @@ def present(name, Bucket,
config_items.append(replication_item)
config_items.append(versioning_item)
for varname, func, current, modded, compare, desired, deleter in config_items:
if not json_objs_equal(modded or current, compare or desired):
for varname, setter, current, comparator, desired, deleter in config_items:
if not comparator(current, desired, region, key, keyid, profile):
# current state and desired state differ
if __opts__['test']:
msg = 'S3 bucket {0} set to be modified.'.format(Bucket)
@ -518,7 +540,7 @@ def present(name, Bucket,
ret['changes'] = {}
return ret
else:
r = __salt__['boto_s3_bucket.{0}'.format(func)](Bucket=Bucket,
r = __salt__['boto_s3_bucket.{0}'.format(setter)](Bucket=Bucket,
region=region, key=key, keyid=keyid, profile=profile,
**(desired or {}))
if not r.get('updated'):

View File

@ -102,63 +102,61 @@ if _has_required_boto():
}
config_ret = {
'get_bucket_acl': {
'Grants': {
'Grants': [{
'Grantee': {
'DisplayName': 'testuser',
'ID': 'aaaaaaaaaaabbbbbbbbbbbbbbbbbbbbbbbbbbc255ce8ac627c5a8c35e9273589',
'DisplayName': 'testowner',
'ID': 'sdfghjklqwertyuiopzxcvbnm'
},
'Permission': 'FULL_CONTROL'
},
}, {
'Grantee': {
'URI': 'http://acs.amazonaws.com/groups/global/AllUsers'
},
'Permission': 'READ'
}],
'Owner': {
'DisplayName': 'testuser',
'ID': 'aaaaaaaaaaabbbbbbbbbbbbbbbbbbbbbbbbbbc255ce8ac627c5a8c35e9273589'
'DisplayName': 'testowner',
'ID': 'sdfghjklqwertyuiopzxcvbnm'
}
},
'get_bucket_cors': {
'Grants': {
'Grantee': {
'DisplayName': 'testuser',
'ID': 'aaaaaaaaaaabbbbbbbbbbbbbbbbbbbbbbbbbbc255ce8ac627c5a8c35e9273589',
},
'Permission': 'FULL_CONTROL'
},
'Owner': {
'DisplayName': 'testuser',
'ID': 'aaaaaaaaaaabbbbbbbbbbbbbbbbbbbbbbbbbbc255ce8ac627c5a8c35e9273589'
}
'CORSRules': [{
'AllowedMethods': ["GET"],
'AllowedOrigins': ["*"],
}]
},
'get_bucket_lifecycle_configuration': {
'Rules': [{
'Expiration': {'Days': 10},
'ID': 'Yjc0NGIyNDUtNmVhZi00OTM2LThhYWEtZDRmZGU0NzA0ZWIw',
'Prefix': 'expiring',
'Status': 'Enabled'
'Expiration': {
'Days': 1
},
'Prefix': 'prefix',
'Status': 'Enabled',
'ID': 'asdfghjklpoiuytrewq'
}]
},
'get_bucket_location': {
'LocationConstraint': None
'LocationConstraint': 'EU'
},
'get_bucket_logging': {
'LoggingEnabled': {
'TargetBucket': 'auditinfo',
'TargetGrants': None,
'TargetPrefix': 'logging',
'TargetBucket': 'my-bucket',
'TargetPrefix': 'prefix'
}
},
'get_bucket_notification_configuration': {
'LambdaFunctionConfigurations': [{
'Events': ['s3:ObjectCreated:*'],
'Filter': {
'Key': {
'FilterRules': [{
'Name': 'Prefix',
'Value': 'lambda'
}]
}
},
'Id': 'MGQ3MGYxYTEtZmRiYS00N2RkLWFhYTItMDRmYTRhNGUwMmZl',
'LambdaFunctionArn':
'arn:aws:lambda:us-east-1:213454234522:function:myfunction',
'LambdaFunctionArn': 'arn:aws:lambda:us-east-1:111111222222:function:my-function',
'Id': 'zxcvbnmlkjhgfdsa',
'Events': ["s3:ObjectCreated:*"],
'Filter': {
'Key': {
'FilterRules': [{
'Name': 'prefix',
'Value': 'string'
}]
}
}
}]
},
'get_bucket_policy': {
@ -167,14 +165,14 @@ if _has_required_boto():
},
'get_bucket_replication': {
'ReplicationConfiguration': {
'Role': 'arn:aws:iam::111111222222:role/my-role',
'Role': 'arn:aws:iam::11111222222:my-role',
'Rules': [{
'Destination': {
'Bucket': 'arn:aws:s3:::my-bucket-2'
},
'ID': 'r1',
'Prefix': 'repl',
'Status': 'Enabled'
'ID': "r1",
'Prefix': "prefix",
'Status': "Enabled",
'Destination': {
'Bucket': "arn:aws:s3:::my-bucket"
}
}]
}
},
@ -192,6 +190,9 @@ if _has_required_boto():
'Status': 'Enabled'
},
'get_bucket_website': {
'ErrorDocument': {
'Key': 'error.html'
},
'IndexDocument': {
'Suffix': 'index.html'
}

View File

@ -0,0 +1,380 @@
# -*- coding: utf-8 -*-
# Import Python libs
from __future__ import absolute_import
from distutils.version import LooseVersion # pylint: disable=import-error,no-name-in-module
from copy import deepcopy
# Import Salt Testing libs
from salttesting.unit import skipIf, TestCase
from salttesting.mock import NO_MOCK, NO_MOCK_REASON, patch
from salttesting.helpers import ensure_in_syspath
ensure_in_syspath('../../')
# Import Salt libs
import salt.config
import salt.loader
# Import 3rd-party libs
import logging
# Import Mock libraries
from salttesting.mock import NO_MOCK, NO_MOCK_REASON, MagicMock, patch
# pylint: disable=import-error,no-name-in-module,unused-import
from unit.modules.boto_s3_bucket_test import BotoS3BucketTestCaseMixin
# Import 3rd-party libs
try:
import boto
import boto3
from botocore.exceptions import ClientError
HAS_BOTO = True
except ImportError:
HAS_BOTO = False
# pylint: enable=import-error,no-name-in-module,unused-import
# the boto_s3_bucket module relies on the connect_to_region() method
# which was added in boto 2.8.0
# https://github.com/boto/boto/commit/33ac26b416fbb48a60602542b4ce15dcc7029f12
required_boto3_version = '1.2.1'
log = logging.getLogger(__name__)
opts = salt.config.DEFAULT_MINION_OPTS
context = {}
utils = salt.loader.utils(opts, whitelist=['boto3'], context=context)
serializers = salt.loader.serializers(opts)
funcs = salt.loader.minion_mods(opts, context=context, utils=utils, whitelist=['boto_s3_bucket'])
salt_states = salt.loader.states(opts=opts, functions=funcs, utils=utils, whitelist=['boto_s3_bucket'], serializers=serializers)
def _has_required_boto():
'''
Returns True/False boolean depending on if Boto is installed and correct
version.
'''
if not HAS_BOTO:
return False
elif LooseVersion(boto3.__version__) < LooseVersion(required_boto3_version):
return False
else:
return True
if _has_required_boto():
region = 'us-east-1'
access_key = 'GKTADJGHEIQSXMKKRBJ08H'
secret_key = 'askdjghsdfjkghWupUjasdflkdfklgjsdfjajkghs'
conn_parameters = {'region': region, 'key': access_key, 'keyid': secret_key, 'profile': {}}
error_message = 'An error occurred (101) when calling the {0} operation: Test-defined error'
not_found_error = ClientError({
'Error': {
'Code': '404',
'Message': "Test-defined error"
}
}, 'msg')
error_content = {
'Error': {
'Code': 101,
'Message': "Test-defined error"
}
}
list_ret = {
'Buckets': [{
'Name': 'mybucket',
'CreationDate': None
}],
'Owner': {
'DisplayName': 'testuser',
'ID': '111111222222'
},
'ResponseMetadata': {'Key': 'Value'}
}
config_in = {
'LocationConstraint': 'EU',
'ACL': {
'ACL': 'public-read'
},
'CORSRules': [{
'AllowedMethods': ["GET"],
'AllowedOrigins': ["*"],
}],
'LifecycleConfiguration': [{
'Expiration': {
'Days': 1
},
'Prefix': 'prefix',
'Status': 'Enabled',
'ID': 'asdfghjklpoiuytrewq'
}],
'Logging': {
'TargetBucket': 'my-bucket',
'TargetPrefix': 'prefix'
},
'NotificationConfiguration': {
'LambdaFunctionConfigurations': [{
'LambdaFunctionArn': 'arn:aws:lambda:us-east-1:111111222222:function:my-function',
'Id': 'zxcvbnmlkjhgfdsa',
'Events': ["s3:ObjectCreated:*"],
'Filter': {
'Key': {
'FilterRules': [{
'Name': 'prefix',
'Value': 'string'
}]
}
}
}]
},
'Policy': {
'Version': "2012-10-17",
'Statement': [{
'Sid': "",
'Effect': "Allow",
'Principal': {
'AWS': "arn:aws:iam::111111222222:root"
},
'Action': "s3:PutObject",
'Resource': "arn:aws:s3:::my-bucket/*"
}]
},
'Replication': {
'Role': 'arn:aws:iam::11111222222:my-role',
'Rules': [{
'ID': "r1",
'Prefix': "prefix",
'Status': "Enabled",
'Destination': {
'Bucket': "arn:aws:s3:::my-bucket"
}
}]
},
'RequestPayment': {
'Payer': 'Requester'
},
'Tagging': {
'a': 'b',
'c': 'd'
},
'Versioning': {
'Status': 'Enabled'
},
'Website': {
'ErrorDocument': {
'Key': 'error.html'
},
'IndexDocument': {
'Suffix': 'index.html'
}
}
}
config_ret = {
'get_bucket_acl': {
'Grants': [{
'Grantee': {
'DisplayName': 'testuser',
'ID': '111111222222'
},
'Permission': 'FULL_CONTROL'
}, {
'Grantee': {
'URI': 'http://acs.amazonaws.com/groups/global/AllUsers'
},
'Permission': 'READ'
}],
'Owner': {
'DisplayName': 'testuser',
'ID': '111111222222'
}
},
'get_bucket_cors': {
'CORSRules': [{
'AllowedMethods': ["GET"],
'AllowedOrigins': ["*"],
}]
},
'get_bucket_lifecycle_configuration': {
'Rules': [{
'Expiration': {
'Days': 1
},
'Prefix': 'prefix',
'Status': 'Enabled',
'ID': 'asdfghjklpoiuytrewq'
}]
},
'get_bucket_location': {
'LocationConstraint': 'EU'
},
'get_bucket_logging': {
'LoggingEnabled': {
'TargetBucket': 'my-bucket',
'TargetPrefix': 'prefix'
}
},
'get_bucket_notification_configuration': {
'LambdaFunctionConfigurations': [{
'LambdaFunctionArn': 'arn:aws:lambda:us-east-1:111111222222:function:my-function',
'Id': 'zxcvbnmlkjhgfdsa',
'Events': ["s3:ObjectCreated:*"],
'Filter': {
'Key': {
'FilterRules': [{
'Name': 'prefix',
'Value': 'string'
}]
}
}
}]
},
'get_bucket_policy': {
'Policy':
'{"Version":"2012-10-17","Statement":[{"Sid":"","Effect":"Allow","Principal":{"AWS":"arn:aws:iam::111111222222:root"},"Action":"s3:PutObject","Resource":"arn:aws:s3:::my-bucket/*"}]}'
},
'get_bucket_replication': {
'ReplicationConfiguration': {
'Role': 'arn:aws:iam::11111222222:my-role',
'Rules': [{
'ID': "r1",
'Prefix': "prefix",
'Status': "Enabled",
'Destination': {
'Bucket': "arn:aws:s3:::my-bucket"
}
}]
}
},
'get_bucket_request_payment': {'Payer': 'Requester'},
'get_bucket_tagging': {
'TagSet': [{
'Key': 'c',
'Value': 'd'
}, {
'Key': 'a',
'Value': 'b',
}]
},
'get_bucket_versioning': {
'Status': 'Enabled'
},
'get_bucket_website': {
'ErrorDocument': {
'Key': 'error.html'
},
'IndexDocument': {
'Suffix': 'index.html'
}
}
}
bucket_ret = {
'Location': 'EU'
}
class BotoS3BucketStateTestCaseBase(TestCase):
conn = None
# Set up MagicMock to replace the boto3 session
def setUp(self):
context.clear()
self.patcher = patch('boto3.session.Session')
self.addCleanup(self.patcher.stop)
mock_session = self.patcher.start()
session_instance = mock_session.return_value
self.conn = MagicMock()
session_instance.client.return_value = self.conn
@skipIf(HAS_BOTO is False, 'The boto module must be installed.')
@skipIf(_has_required_boto() is False, 'The boto3 module must be greater than'
' or equal to version {0}'
.format(required_boto3_version))
@skipIf(NO_MOCK, NO_MOCK_REASON)
class BotoS3BucketTestCase(BotoS3BucketStateTestCaseBase, BotoS3BucketTestCaseMixin):
'''
TestCase for salt.modules.boto_s3_bucket state.module
'''
def test_present_when_bucket_does_not_exist(self):
'''
Tests present on a bucket that does not exist.
'''
self.conn.head_bucket.side_effect = [not_found_error, None]
self.conn.list_buckets.return_value = deepcopy(list_ret)
self.conn.create_bucket.return_value = bucket_ret
for key, value in config_ret.iteritems():
getattr(self.conn, key).return_value = deepcopy(value)
with patch.dict(funcs, {'boto_iam.get_account_id': MagicMock(return_value='111111222222')}):
result = salt_states['boto_s3_bucket.present'](
'bucket present',
Bucket='testbucket',
**config_in
)
self.assertTrue(result['result'])
self.assertEqual(result['changes']['new']['bucket']['Location'],config_ret['get_bucket_location'])
def test_present_when_bucket_exists_no_mods(self):
self.conn.list_buckets.return_value = deepcopy(list_ret)
for key, value in config_ret.iteritems():
getattr(self.conn, key).return_value = deepcopy(value)
with patch.dict(funcs, {'boto_iam.get_account_id': MagicMock(return_value='111111222222')}):
result = salt_states['boto_s3_bucket.present'](
'bucket present',
Bucket='testbucket',
**config_in
)
self.assertTrue(result['result'])
self.assertEqual(result['changes'], {})
def test_present_when_bucket_exists_all_mods(self):
self.conn.list_buckets.return_value = deepcopy(list_ret)
for key, value in config_ret.iteritems():
getattr(self.conn, key).return_value = deepcopy(value)
with patch.dict(funcs, {'boto_iam.get_account_id': MagicMock(return_value='111111222222')}):
result = salt_states['boto_s3_bucket.present'](
'bucket present',
Bucket='testbucket',
LocationConstraint=config_in['LocationConstraint']
)
self.assertTrue(result['result'])
self.assertNotEqual(result['changes'], {})
def test_present_with_failure(self):
self.conn.head_bucket.side_effect = [not_found_error, None]
self.conn.list_buckets.return_value = deepcopy(list_ret)
self.conn.create_bucket.side_effect = ClientError(error_content, 'create_bucket')
with patch.dict(funcs, {'boto_iam.get_account_id': MagicMock(return_value='111111222222')}):
result = salt_states['boto_s3_bucket.present'](
'bucket present',
Bucket='testbucket',
**config_in
)
self.assertFalse(result['result'])
self.assertTrue('An error occurred' in result['comment'])
def test_absent_when_bucket_does_not_exist(self):
'''
Tests absent on a bucket that does not exist.
'''
self.conn.head_bucket.side_effect = [not_found_error, None]
result = salt_states['boto_s3_bucket.absent']('test', 'mybucket')
self.assertTrue(result['result'])
self.assertEqual(result['changes'], {})
def test_absent_when_bucket_exists(self):
result = salt_states['boto_s3_bucket.absent']('test', 'testbucket')
self.assertTrue(result['result'])
self.assertEqual(result['changes']['new']['bucket'], None)
def test_absent_with_failure(self):
self.conn.delete_bucket.side_effect = ClientError(error_content, 'delete_bucket')
result = salt_states['boto_s3_bucket.absent']('test', 'testbucket')
self.assertFalse(result['result'])
self.assertTrue('An error occurred' in result['comment'])