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

New feature to add AWS tags to the log group #199

Closed
paulsuh opened this issue May 29, 2024 · 5 comments
Closed

New feature to add AWS tags to the log group #199

paulsuh opened this issue May 29, 2024 · 5 comments

Comments

@paulsuh
Copy link
Contributor

paulsuh commented May 29, 2024

AWS has the ability to add tags to the log group, both through the console and through API calls. These are important in the context of a large organization -- they help with governance and attribution, so that we can mark things like who is responsible for a resource or the level of sensitivity of certain data.

I forked the repo and created code that is more-or-less ready, but the docs need to be updated plus I have a question about a design choice.

The simple way to do the tagging is to use boto3's tag_log_group() call. However, it has an ominous warning at the top of the documentation:

Warning
The TagLogGroup operation is on the path to deprecation. We recommend that you use TagResource instead.

The problem is that tag_log_group() uses the IAM permission logs:TagLogGroup, while tag_resource() uses the IAM permission logs:TagResource (which is new and a lot of people won't have it in their policies). This will cause a problem if a user tries to tag a log group. I originally just wrote the code using tag_resource() to be future-proof, but discovered the problem when I was testing.

I can see a few possible ways forward:

  1. Use tag_log_group() until it is deprecated, then change it.
  2. Use tag_resource() and document the necessary change in IAM permissions.
  3. Set up the code to try tag_log_group() first, then automatically try tag_resource() if it fails.
  4. Have the code for both and add an optional boolean flag letting the user choose which method to use, defaulting to one or the other.

Since this is your package I'd like to get your opinion on which path to take, or if there is an alternative that would be better.

@kislyuk
Copy link
Owner

kislyuk commented Jun 3, 2024

Thank you for your interest in watchtower. I am generally open to the idea of specifying log group tags to apply to the log group at startup time. The deprecated API should not be used nor is there a need to mention it - only mention the details of what is needed going forward.

@paulsuh
Copy link
Contributor Author

paulsuh commented Jun 19, 2024

Created PR #201. All unit tests are passing on local machine.

@paulsuh
Copy link
Contributor Author

paulsuh commented Jun 23, 2024

Cool, thanks! (I didn't know you had a separate structure for Changes.rst in the Makefile, but I'm glad that the change log is now back in the right place. Also, I can't believe I didn't catch the dang auto-incorrect docstring --> doctoring in the PR text. :-P )

@paulsuh paulsuh closed this as completed Jun 23, 2024
@paulsuh
Copy link
Contributor Author

paulsuh commented Aug 19, 2024

Hi, would it be possible to do a Watchtower release soon? We got a directive from above that all of our log groups MUST be tagged ASAP.

@kislyuk
Copy link
Owner

kislyuk commented Aug 19, 2024

For sure. Released in v3.3.0, sorry about the delay.

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

No branches or pull requests

2 participants