Use CloudTrail's regionalized policy templates #1167
Conversation
|
LGTM 🚢 I agree it would be nice if you could update to clients. But, at this time it will not be feasible for you until |
| @@ -204,8 +206,9 @@ def setup_new_bucket(self, bucket, prefix, policy_url=None): | |||
| if policy_url: | |||
| policy = requests.get(policy_url).text | |||
| else: | |||
| data = self.s3.GetObject(bucket='awscloudtrail', | |||
| key=S3_POLICY_TEMPLATE) | |||
| data = self.s3.GetObject( | |||
There was a problem hiding this comment.
It might be nice to provide a better error message if a policy hasn't been copied over to a specific region now that this is a new possible failure mode.
|
Overall looks good. Just a small comment on the new failure mode. |
|
@jamesls code has been refactored/updated to log an error message, then pass-through the underlying exception. If we want to expose a custom exception just for this case then I'm not opposed, but it's unclear to me which Botocore exceptions might happen during the |
| ' region %s: %s', self.region_name, key_name) | ||
| raise | ||
|
|
||
| return data['Body'].read().decode('utf-8') |
There was a problem hiding this comment.
This should probably be moved inside the try/except because we can still hit timeout/connection errors when we try to read the response body.
|
It would be good to add a test for this new code. |
|
@jamesls please take another look. The coverage decrease seems like a false alarm because I only added tests. |
| data = self.s3.GetObject( | ||
| bucket='awscloudtrail-policy-' + self.region_name, | ||
| key=key_name) | ||
| policy = data['Body'].read().decode('utf-8') |
There was a problem hiding this comment.
You can just return data['Body'].read().decode('utf-8') instead of needing the return policy on line 200.
|
Why not raise a specific exception and just use |
|
|
Thanks! Don't forget to update the changelog.This change switches away from always using an S3 bucket in `us-west-2` to fetch CloudTrail policy information for S3 and SNS when setting up new buckets/topics and instead uses the `awscloudtrail-policy-REGION` regionalized buckets. It also updates the policy version to use the most recent release. Tests are updated and a couple new ones make sure the regionalized buckets are used.
danielgtaylor
force-pushed
the
cloudtrail-regional-policy
branch
from
8b6344b to
f549785
Compare
Use CloudTrail's regionalized policy templates
This change switches away from always using an S3 bucket in
us-west-2to fetch CloudTrail policy information for S3 and SNS when setting up
new buckets/topics and instead uses the
awscloudtrail-policy-REGIONregionalized buckets. It also updates the policy version to use the
most recent release.
Tests are updated and a couple new ones make sure the regionalized
buckets are used.
cc @jamesls @kyleknap
Note: this code still needs to be updated to use clients, but that seems
like it should be a separate task.