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

Add tagging support to AWS IAM Policies #6751

Merged
merged 3 commits into from Jul 6, 2021

Conversation

trastle
Copy link
Contributor

@trastle trastle commented Jun 14, 2021

Add tag loading and tagging to AWS IAM Policies.
Resolves: #6679

@trastle
Copy link
Contributor Author

trastle commented Jun 14, 2021

I've fixed the tests that needed additional or changed resources in the test fixtures.
I have, currently, resolved these by making the minimum change rather than replacing the existing recording.

There is one test, the remaining failure, that I am uncertain how to fix as I'm uncertain exactly what its testing:

 =================================== FAILURES ===================================
______________ PolicyMetaLint.test_resource_shadow_source_augment ______________
[gw1] linux -- Python 3.6.13 /home/runner/work/cloud-custodian/cloud-custodian/.tox/py36/bin/python
Traceback (most recent call last):
  File "/home/runner/work/cloud-custodian/cloud-custodian/tests/test_policy.py", line 175, in test_resource_shadow_source_augment
    % (", ".join(shadowed))
  File "/opt/hostedtoolcache/Python/3.6.13/x64/lib/python3.6/unittest/case.py", line 670, in fail
    raise self.failureException(msg)
AssertionError: iam-policy have resource managers shadowing source augments

@ajkerrigan
Copy link
Member

Thanks for the PR!

There is one test, the remaining failure, that I am uncertain how to fix as I'm uncertain exactly what its testing:
...
AssertionError: iam-policy have resource managers shadowing source augments

Ok, that's happening because augments can be source-specific. By default, the resource manager defers to the source to handle augments. This test sees that the iam-policy resource has a Config source defined, but defines a custom augment at the resource manager level. I think that's actually what we want here, but that shadow test goes back a long while and seems like a good general idea for consistency and to avoid surprises. (Config sources also get tags automatically for some resources, though that doesn't seem to be the case here.)

Given all of that context, I think the smoothest way forward would be to define augments per source. Which would mean something like this for the existing DescribePolicy source, and then a new custom Config source with the same augment.

@kapilt
Copy link
Collaborator

kapilt commented Jun 28, 2021

the shadow augment catch is valid here, config source already has tags, so redundantly fetching them when that's the source on a manager augment is not good, this should be on a describe source subclass overriding the augment method on it.

@trastle
Copy link
Contributor Author

trastle commented Jul 5, 2021

Sorry for the slow reply on this one folks. I added the previous patch to my workflow and forgot a little about the PR.

I've implemented the above suggested change, moving the augment to the existing DescribePolicy class from the Policy object itself.

This has made the automated tests happy (a rebase was also needed) and I can confirm this works in my environment also.

Copy link
Collaborator

@kapilt kapilt left a comment

Choose a reason for hiding this comment

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

lgtm, thanks

@kapilt
Copy link
Collaborator

kapilt commented Jul 6, 2021

i do wonder if we want to do a follow up branch to default to only customer managed policies wrt to server side query.

@kapilt kapilt merged commit 7a3cd95 into cloud-custodian:master Jul 6, 2021
@trastle
Copy link
Contributor Author

trastle commented Jul 7, 2021

i do wonder if we want to do a follow up branch to default to only customer managed policies wrt to server side query.

I cannot comment for all users of this but I'm certainly filtering down to just my customer policies when I am running my policies. A default or some intuitive way to do this would be welcome.

@trastle trastle deleted the CC-6679-policytags branch July 7, 2021 05:48
aq17 pushed a commit to draios/cloud-custodian that referenced this pull request Jul 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The iam.iam-policy is not obeying tag absence filters
3 participants