S3 lifecycle fixes #1831
S3 lifecycle fixes #1831
Conversation
…le so that a single bucket with access problems doesn't kill the entire run.
|
@kapilt - I have tested the latest changes and VOK they work! Thanks Scott |
| if e.response['Error']['Code'] == 'AccessDenied': | ||
| log.warning("Access Denied Bucket:%s while applying lifecycle" % bucket['Name']) | ||
| else: | ||
| raise e |
There was a problem hiding this comment.
@kapilt - Is this appropriate to log and skip over any buckets that cannot be accessed? If so, the set-bucket-encryption action probably needs similar logic.
There was a problem hiding this comment.
yes thats appropriate, ideally is if we can return a set of those buckets names on denied {'denied': [bucket_names]}, action results are serialized as a json output file.
There was a problem hiding this comment.
The error catching is handled in process. The reason it isn't working is because .exception is called before .result. process needs to be changed to properly handle exceptions so that it doesn't error out when a single bucket has an exception. I don't completely understand why the behavior is that way though.
There was a problem hiding this comment.
Ok I see, exception and result can't be used together. Exception will return return if an exception is raised, however, that does not stop result from also raising the exception if result is called later.
There was a problem hiding this comment.
@kapilt - is it appropriate to change the lifecycle action to be a subclass of ScanBucket? That will work with the denied_buckets. Otherwise I can move the denied_buckets logic up a level into BucketActionBase
There was a problem hiding this comment.
scanbucket is for actions that are looking at objects, so it wouldn't be appropriate. simply returning a dictionary from process on the action method would suffice.
c7n/resources/s3.py
Outdated
| @@ -2582,7 +2582,7 @@ def process_bucket(self, bucket): | |||
| s3 = bucket_client(local_session(self.manager.session_factory), bucket) | |||
|
|
|||
| # Adjust the existing lifecycle by adding/deleting/overwriting rules as necessary | |||
| config = (bucket['Lifecycle'] or {}).get('Rules', []) | |||
| config = (bucket.get('Lifecycle') or {}).get('Rules', []) | |||
There was a problem hiding this comment.
@kapilt - This is necessary because in the S3 augment step, if the permission fails for a given method then it doesn't set the key. I wonder if it should set the default instead. This is around line 438
There was a problem hiding this comment.
we should probably filter out buckets before processing if we don't have access to get lifecycle configuration, it will be listed in an an annotation deniedmethods (sp?) per the bucket augment.
There was a problem hiding this comment.
Yes - good point. Fixed.
It would be odd to have permissions allowing a person to put the lifecycle but not get it. But if that does happen this would end up deleting the previous lifecycle.
| @@ -2498,15 +2498,15 @@ class Lifecycle(BucketActionBase): | |||
| 'And': { | |||
| 'type': 'object', | |||
| 'additionalProperties': False, | |||
| 'items': { | |||
| 'properties': { | |||
There was a problem hiding this comment.
so i'm a bit surprised that this got through unit test runs, ci runs with schema validation on for unit test executed policies.
There was a problem hiding this comment.
looks like it passed because the lifecycle options with problematic schemas weren't being tested.
There was a problem hiding this comment.
@kapilt - there are a ton of options to the lifecycle and they can't all be specified together in a valid policy so I am not exercising every option. I could have one fake test just for validating the schema that tries to utilize every single option and just validates the policy instead of running it. Would that be useful?
@kapilt - @jtroberts83 helped me fix two issues with the S3 lifecycle code. One question inline.
itemsinstead ofpropertiesfor four of the objects. This PR fixes it.Issue #1830