Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wip - aws.ecr - log warning on access denied instead of raising #3618

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

thisisshi
Copy link
Member

ECR's resource level IAM policies causes custodian to error out if a single repository denies access to custodian's role. This pr logs the error as a warning in the logs and continues on to the rest of the resources. Also fixes this for get lifecycle policy

Traceback (most recent call last):
File "/src/c7n/policy.py", line 232, in run
resources = self.policy.resource_manager.resources()
File "/src/c7n/query.py", line 421, in resources
resources = self.filter_resources(resources)
File "/src/c7n/manager.py", line 105, in filter_resources
resources = f.process(resources, event)
File "/src/c7n/resources/ecr.py", line 116, in process
resources = list(filter(None, w.map(_augment, resources)))
File "/usr/local/lib/python3.7/concurrent/futures/_base.py", line 586, in result_iterator
yield fs.pop().result()
File "/usr/local/lib/python3.7/concurrent/futures/_base.py", line 432, in result
return self.__get_result()
File "/usr/local/lib/python3.7/concurrent/futures/_base.py", line 384, in __get_result
raise self._exception
File "/usr/local/lib/python3.7/concurrent/futures/thread.py", line 57, in run
result = self.fn(*self.args, **self.kwargs)
File "/src/c7n/resources/ecr.py", line 109, in _augment
repositoryName=r['repositoryName'])['policyText']
File "/usr/local/lib/python3.7/site-packages/botocore/client.py", line 357, in _api_call
return self._make_api_call(operation_name, kwargs)
File "/usr/local/lib/python3.7/site-packages/botocore/client.py", line 661, in _make_api_call
raise error_class(parsed_response, operation_name)
botocore.exceptions.ClientError: An error occurred (AccessDeniedException) when calling the GetRepositoryPolicy operation: User: arn:aws:sts::xxxxxxxxxxxx:assumed-role/CloudCustodian is not authorized to perform: ecr:GetRepositoryPolicy on resource: arn:aws:ecr:us-east-1:xxxxxxxxxxxx:repository/test with an explicit deny

@kapilt
Copy link
Collaborator

kapilt commented Mar 4, 2019

maybe not for this pr, but i feel like we really need to have these captured and a way to track signal a policy's failure on a partial like this. warning log messages just haven't been cutting at it scale, i don't get the sense any one looks at the logs typically.

@thisisshi
Copy link
Member Author

@kapilt yeah, I was originally going to add in the c7n:DeniedMethods attribute to cross-account but in the case that the resource fails to get a repository policy, you would never be able to see it in the output. Additionally, when you wrap the cross account filter in a not block, the annotated key is removed.

@kapilt
Copy link
Collaborator

kapilt commented Mar 5, 2019

wrt to tests, perhaps worth looking at how i was doing it for ebs snapshot not found when tagging pr

c7n/resources/ecr.py Outdated Show resolved Hide resolved
c7n/resources/ecr.py Outdated Show resolved Hide resolved
@kapilt kapilt changed the title aws.ecr - log warning on access denied instead of raising [wip] aws.ecr - log warning on access denied instead of raising Mar 25, 2019
@kapilt kapilt changed the title [wip] aws.ecr - log warning on access denied instead of raising wip - aws.ecr - log warning on access denied instead of raising Mar 25, 2019
if e.response['Error']['Code'] == 'AccessDeniedException':
self.log.warning('Access Denied on GetLifecyclePolicy for repository: %s' %
r['repositoryName'])
r[self.policy_annotation] = {}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some changes got pushed to this that now unconditionally and without logging swallow all other exceptions ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants