Adapt new IAM provider to use new Tags concept#13884
Conversation
Test Results (amd64) - Integration, Bootstrap 5 files ± 0 5 suites ±0 11m 53s ⏱️ - 2h 29m 35s Results for commit b5e4fe2. ± Comparison against base commit 2770dfd. ♻️ This comment has been updated with latest results. |
bentsku
left a comment
There was a problem hiding this comment.
LGTM, this is nice to see tags being centralized! This is gonna make it easier to interact with them from outside 👍
One note: tagging functionality does not need to be put into particular methods anymore, as they do not need to be overridden.
Using the store directly if it equals to the same logic makes sense 👍 except if we want to keep consistency, so it's a matter of personal preference in the end, on how the provider is structured. Just wanted to mention that it was not a requirement anymore 😄
Jus have some comments about some docstrings that seem to be out of date, but logically it seems all good 👍
One thing to always keep track of is to clean up tags when deleting resources, but it seems to be taken care of in this PR 👍
\cc @viren-nadkarni @aidehn for a Tags migration, if you want to have a look
| ACCOUNT_ALIAS: str | None = CrossRegionAttribute(default=None) | ||
|
|
||
| # Centralized tag storage for all IAM resources | ||
| TAGS: Tags = CrossRegionAttribute(default=Tags) |
There was a problem hiding this comment.
note: usually, Tagging is a LocalAttribute. This is because from testing in ResourceGroupsTaggingApi, trying to list the tags of something will only yield depending on the region of the caller. But as IAM is a truly global service, I think this does not apply 👍
There was a problem hiding this comment.
With a LocalAttribute we would have had a problem with store lookup should, somehow, a request be made to a different region. To avoid this incompatibility, I chose a CrossRegionAttribute here as well :)
There was a problem hiding this comment.
Yes, in other services, we use the resource ARN to do the store lookup for tags in particular, because it seems are scoped by region somehow, as showed by RGTA testing.
For ex, for Route53, you can list tags in RGTA if the originating request is in us-east-1, otherwise it returns no tags.
Same for S3, the originating request needs to be in the same as the region.
So we do not want to have all tags from all regions when trying to list tags per region.
But coming back to Route53, then we just have hardcoded region in RGTA, etc 👍 or you can filter it in the RGTA plugin itself. You can see rgta.py in Route53 and S3 for weird examples, and KMS for a normal one
|
|
||
| :param store: IamStore instance | ||
| :param arn: Resource ARN | ||
| :param case_sensitive: Not used for get, but kept for API consistency |
There was a problem hiding this comment.
nit: this param isn't there anymore
There was a problem hiding this comment.
Claude added that, I removed it again and missed that 😅
| def _delete_all_tags(self, store: IamStore, arn: str) -> None: | ||
| """ | ||
| Delete all tags for a resource (used when the resource is deleted). | ||
|
|
||
| :param store: IamStore instance | ||
| :param arn: Resource ARN | ||
| """ | ||
| store.TAGS.delete_all_tags(arn) |
There was a problem hiding this comment.
I think as this function does not hold more logic, it can be removed and the provider can call the store directly
There was a problem hiding this comment.
Agreed, but I am inclined to keep it, if there are any more iterations on the tags storage, we only have to change it in a central location 😅
| def _store_initial_tags( | ||
| self, store: IamStore, arn: str, tags: list[dict] | None, case_sensitive: bool = True | ||
| ) -> None: | ||
| """ | ||
| Store initial tags when creating a resource. | ||
|
|
||
| :param store: IamStore instance | ||
| :param arn: Resource ARN | ||
| :param tags: Tags in list format [{"Key": ..., "Value": ...}] | ||
| :param case_sensitive: Whether to treat keys case-sensitively (True) or case-insensitively (False) | ||
| """ | ||
| if tags: | ||
| self._update_tags(store, arn, tags, case_sensitive) |
There was a problem hiding this comment.
same with this function, it can probably directly in the provider? except if we don't want to repeat the if tags I guess, this is up to you
There was a problem hiding this comment.
This one is truly unnecessary, agreed! Will just use _update_tags directly everywhere
…ry method, fix typing
Motivation
With #13821 a new concept for tagging was introduced, a Tags class to store tags for a provider.
This PR will migrate the new IAM provider to use this new mechanism.
Changes
Tests
Related
Closes UNC-293