Add tag support to LogGroup resource #53

gruebel · 2020-10-27T17:01:05Z

Fixes aws-cloudformation/cloudformation-coverage-roadmap#77

Description of changes:
I added the possibility to add tags to the LogGroup resource.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

wbingli · 2020-11-03T07:55:27Z

Thanks for your contribution. We are prioritizing this PR review.

petewilcock · 2020-11-08T19:10:29Z

+1 on this PR, we're awaiting this functionality 💯

miparnisari · 2020-11-11T06:46:39Z

aws-logs-loggroup/src/main/java/software/amazon/logs/loggroup/Translator.java

@@ -128,4 +159,19 @@ static String buildResourceDoesNotExistErrorMessage(final String resourceIdentif
 ResourceModel.TYPE_NAME,
 resourceIdentifier);
 }
+
+ static Map<String, String> translateTagsToSdk(final Set<Tag> tags) {
+ if (tags == null || tags.isEmpty()) {


The tags.isEmpty() check is redundant. It should be handled by the stream() call

will result in same error as below

miparnisari · 2020-11-11T06:48:49Z

aws-logs-loggroup/src/main/java/software/amazon/logs/loggroup/Translator.java

+
+ static Map<String, String> translateTagsToSdk(final Set<Tag> tags) {
+ if (tags == null || tags.isEmpty()) {
+ return null;


Instead of returning null can we return an empty map?

if I don't return null here some of the contract tests will fail with

{'status': 'FAILED', 'errorCode': 'GeneralServiceException', 'message': "1 validation error detected: Value '{}' at 'tags' failed to satisfy constraint: Member must have length greater than or equal to 1 (Service: CloudWatchLogs, Status Code: 400, Request ID: 710f70c1-0dad-4f89-91ad-3eadd83b6e9c, Extended Request ID: null)", 'callbackDelaySeconds': 0}

miparnisari · 2020-11-11T06:54:54Z

aws-logs-loggroup/src/main/java/software/amazon/logs/loggroup/UpdateHandler.java

+ ResourceModel.TYPE_NAME, model.getLogGroupName(), tagsToAdd);
+ logger.log(message);
+ }
+ } catch (final ResourceNotFoundException e) {


Not sure whether we need to catch a few more exceptions https://docs.aws.amazon.com/AmazonCloudWatch/latest/APIReference/API_TagResource.html#API_TagResource_Errors

aws-cloudformation/cloudformation-cli-java-plugin#306 should handle AwsServiceExceptions automatically but the contract seems to still require some to be explicitly mapped

I added one more exception handling, because the link provided is actually the wrong one, it is for CloudWatch not CloudWatch Logs. That's the correct one 😄
https://docs.aws.amazon.com/AmazonCloudWatchLogs/latest/APIReference/API_TagLogGroup.html#API_TagLogGroup_Errors

aws-logs-loggroup/src/main/java/software/amazon/logs/loggroup/UpdateHandler.java

…test

PatMyron · 2020-11-15T19:25:18Z

aws-logs-loggroup/pom.xml

@@ -43,7 +43,7 @@
 <dependency>
 <groupId>software.amazon.awssdk</groupId>
 <artifactId>cloudwatchlogs</artifactId>
- <version>2.13.18</version>
+ <version>2.15.26</version>


There have been issues with resource providers using newer AWS SDK versions than the Java plugin is using:
aws-cloudformation/cloudformation-cli-java-plugin#267 (comment)
aws-cloudformation/cloudformation-cli-java-plugin#324

Is the current AWS SDK version the Java plugin is using sufficient?

Suggested change

<version>2.15.26</version>

<version>2.15.19</version>

sure, changed it and moved the version to the properties section

gruebel · 2020-11-24T20:45:55Z

@PatMyron is there anything else I should change or more feedback?

wbingli · 2020-11-24T21:26:20Z

aws-logs-loggroup/src/main/java/software/amazon/logs/loggroup/ReadHandler.java

 try {
 response = proxy.injectCredentialsAndInvokeV2(Translator.translateToReadRequest(model),
- ClientBuilder.getClient()::describeLogGroups);
+ ClientBuilder.getClient()::describeLogGroups);
+ tagsResponse = proxy.injectCredentialsAndInvokeV2(Translator.translateToListTagsLogGroupRequest(model.getLogGroupName()),


To have backward compatibility, the ListTagsLogGroups call should be soft-fail if no permission for tagging. Basically catch the permission error of this API call and ignore it.

sure, I can add it, but I don't get why I should support here backward compatibility? Shouldn't I then do it everywhere, where I interact with tags in some way? I'm just really curious about it, hope to get some more insights 😄

particularly issues with ReadHandlers since permissions expand without customer changes. We have an internal issue to eventually find a platform-wide solution to this problem

@gruebel Some users may not have tagging permissions while creating the log group resource before, especially they use stack role and restrict the permissions to operate a stack .

In this way, it will break people if this change go out. It makes senses to fail if users specify tags during create or update time. However, if they creates the log group successfully before, it will be bad experience to break them. BTW, the READ will be used for GetAtt function to get the attribute data as well as for drift detection for the resource.

Thanks for the insight. Makes sense with the stach role permission for stack updates. I also used it in my previous work.

What I'm still curious about is, what is actually the List handler used for. The Read one I understand and it is used also during stack resource import, but the List one?!

This List handler is not used in CloudFormation yet, it intends for future features, so having it hard fail for permission issue is right to do.

wbingli · 2020-11-24T21:31:49Z

aws-logs-loggroup/src/main/java/software/amazon/logs/loggroup/UpdateHandler.java

 final ResourceModel model = request.getDesiredResourceState();
 final ResourceModel previousModel = request.getPreviousResourceState();
 final boolean retentionChanged = ! retentionUnchanged(previousModel, model);
 final boolean kmsKeyChanged = ! kmsKeyUnchanged(previousModel, model);
+ final boolean tagsChanged = ! tagsUnchanged(previousModel, model);


Should use getDesiredResourceTags which will combine both stack tags and resource tag. And getPreviousResourceTags for both previous stack tags and resource tags.

You will need to override the resourceDefinedTags (example) so that the getDesiredResourceTags will merge the stacks and resource defined tags together.

Good point, makes sense and will try to add it.

gruebel · 2020-12-16T14:06:38Z

@wbingli would be great to get some feedback for my new changes 😄

wbingli · 2021-02-01T19:18:27Z

aws-logs-loggroup/src/main/java/software/amazon/logs/loggroup/UpdateHandler.java

+ }
+ } catch (final ResourceNotFoundException e) {
+ throwNotFoundException(model);
+ } catch (final InvalidParameterException e) {


We need to soft fail the Tag permission as well for Update and Create. The reason is that the Tagging change would comes from stack tags as well. The scenario is that some users don't give Tag permission to the stack. Before this change, the Log group resource can be update successful while doing stack tag update, as this resource just ignore any tag update.

With this change, it will now update the tag for cloudformation stack tags and it can run into permission error and fails the resource update. Soft fail with it as same as read handler can avoid breaking change to those users.

aws-logs-loggroup/src/main/java/software/amazon/logs/loggroup/CreateHandler.java

aws-logs-loggroup/src/main/java/software/amazon/logs/loggroup/Configuration.java

jaglade · 2021-02-22T15:27:53Z

Is there any update on this PR? We are eagerly awaiting this functionality.

tata9001 · 2021-04-01T15:50:58Z

Any update on this PR?

Aleksandr-Filichkin · 2021-04-12T11:12:04Z

Any update on this?

vitali-pozniak · 2021-04-16T13:29:17Z

+1

mgmarino · 2021-04-23T06:45:39Z

It'd be great to have input from either the reviewers or @gruebel (who has already put in a lot of work here, thanks!) about whether help from the community is desired to get this over the finish line. This is a very important feature improvement.

JoryUK · 2021-05-07T17:51:25Z

+1

wbingli · 2021-05-10T18:46:23Z

@sfbaker7 Any comment to this PR? I have only one comment here (#53 (comment)) to address.

gruebel · 2021-05-10T21:25:11Z

sorry, everyone. I totally forgot about my PR 🙈

@wbingli I added the silent fail behaviour for add and remove tags, when trying to update a log group. Tagging permissions are not needed when creating a log group with tags.

wbingli · 2021-05-12T20:46:24Z

@gruebel Thanks for your contribution. Will merge it once we have other reviewer approve it.

shoddyknight · 2021-05-13T15:10:54Z

I am eagerly awaitiing this being merged so I can tag and find my cfn-init logs easily

samjarrett · 2021-07-01T03:28:53Z

Is this still yet to be deployed? It definitely isn't documented and doesn't work if you specify Tags: as a property...

sfbaker7 · 2021-07-01T18:56:23Z

Hi, we are expecting this to be available in all regions by the end of next week. Documentation change will follow after

Will update this thread once deployed, apologies for the delay!

glc-froussel · 2021-07-16T07:32:50Z

Hello there, any update on this ?

bool3012 · 2021-07-26T15:37:59Z

Any update?? This feature still doesn't work, at least in eu regions..

felixfang · 2021-07-27T16:32:29Z

Hi this is Felix from AWS, to add support of tags, our team need to go through security review to release it. We are currently working on the review.

bencovi · 2021-08-04T17:49:16Z

Hi @felixfang - any updates?

felixfang · 2021-08-06T18:39:30Z

Hi we are still working on the security review. We are aiming to have it ready in September

felixfang · 2021-09-27T23:59:42Z

Hi Sam, just and update. The security review is in good progress, we just need some more weeks to close it so we can pull your request into our code. We will try complete Tag support in Oct. Sorry for delaying on this.

Karthi96 · 2021-11-08T13:57:43Z

@felixfang any update on this, when this can be released?

reply2srij · 2022-01-03T14:37:28Z

Can we get an update on this please?

glc-froussel · 2022-01-03T14:41:58Z

Can we get an update on this please?

It has been released on November 22 ;)

SeijiSuenaga · 2022-01-05T01:58:46Z

Is it just me, or are stack-level tags not always applied to log group resources in the stack? I'm seeing inconsistent results where log groups don't always (but sometimes do) have all the tags that are on the stack.

glc-froussel · 2022-01-05T09:15:46Z

Is it just me, or are stack-level tags not always applied to log group resources in the stack? I'm seeing inconsistent results where log groups don't always (but sometimes do) have all the tags that are on the stack.

I had the same behaviour and the cause was that I had to explicitly create the log group resource in the CF template instead of having the related resource create it (aka, the log group must be part of your stack resources).

Maybe you are facing the same issue ?

mgmarino · 2022-01-05T09:28:20Z

Is it just me, or are stack-level tags not always applied to log group resources in the stack? I'm seeing inconsistent results where log groups don't always (but sometimes do) have all the tags that are on the stack.

CF will only propagate changes. In other words, if you have stack-level tags and they have not changed since this feature was introduced, they will not get propagated to the log groups. For already existing stacks, this was an issue for us since we have some tags that do not regularly change on deployments. (Some, like the Version, do, and these were getting propagated as expected.) To force the tags to get propagated, we had to do some manual deployments setting the tags to a temporary value (note, just omitting them will not "delete" them), and then a subsequent deployment with the correct tag values.

SeijiSuenaga · 2022-01-06T00:31:23Z

Is it just me, or are stack-level tags not always applied to log group resources in the stack? I'm seeing inconsistent results where log groups don't always (but sometimes do) have all the tags that are on the stack.

CF will only propagate changes. In other words, if you have stack-level tags and they have not changed since this feature was introduced, they will not get propagated to the log groups. For already existing stacks, this was an issue for us since we have some tags that do not regularly change on deployments. (Some, like the Version, do, and these were getting propagated as expected.) To force the tags to get propagated, we had to do some manual deployments setting the tags to a temporary value (note, just omitting them will not "delete" them), and then a subsequent deployment with the correct tag values.

Thank you, that's exactly what's happening in my case, so I'll have to do some manual shenanigans too unfortunately.

Add tag support to LogGroup resource

0406d2b

wbingli requested review from miparnisari and cdebojit November 3, 2020 07:45

miparnisari removed the request for review from cdebojit November 3, 2020 17:42

miparnisari reviewed Nov 11, 2020

View reviewed changes

miparnisari requested a review from PatMyron November 11, 2020 07:02

PatMyron reviewed Nov 12, 2020

View reviewed changes

aws-logs-loggroup/src/main/java/software/amazon/logs/loggroup/UpdateHandler.java Outdated Show resolved Hide resolved

gruebel added 2 commits November 12, 2020 22:27

Add missing exception handling

d35f9a4

removed Tag patterns and upgraded dependency, due to failing cotract …

a1ab815

…test

PatMyron reviewed Nov 15, 2020

View reviewed changes

pin awssdk version to same as Java plugin

b79f187

wbingli reviewed Nov 24, 2020

View reviewed changes

gruebel added 2 commits December 9, 2020 17:41

Silent fail when list tags permission is missing

e8b0238

Add missing stack tags

f5fd0dc

gruebel force-pushed the add-tags branch from ba049f3 to f5fd0dc Compare December 16, 2020 14:00

gruebel requested a review from wbingli January 29, 2021 10:39

wbingli suggested changes Feb 1, 2021

View reviewed changes

aws-logs-loggroup/src/main/java/software/amazon/logs/loggroup/CreateHandler.java Show resolved Hide resolved

aws-logs-loggroup/src/main/java/software/amazon/logs/loggroup/Configuration.java Show resolved Hide resolved

Silent fail when add or remove tags permission is missing

e427257

gruebel requested a review from wbingli May 11, 2021 09:32

wbingli approved these changes May 12, 2021

View reviewed changes

wbingli requested a review from ammokhov May 12, 2021 20:45

ammokhov approved these changes May 12, 2021

View reviewed changes

ammokhov merged commit 5e7acf4 into aws-cloudformation:master May 20, 2021

samjarrett mentioned this pull request Sep 14, 2021

AWS::Logs::LogGroup - Tags aws-cloudformation/cloudformation-coverage-roadmap#919

Closed

rittneje mentioned this pull request Oct 18, 2021

(logs): Adding Tags to LogGroups Construct Not working aws/aws-cdk#11814

Closed

Add tag support to LogGroup resource #53

Add tag support to LogGroup resource #53

Conversation

gruebel commented Oct 27, 2020

wbingli commented Nov 3, 2020

petewilcock commented Nov 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gruebel commented Nov 24, 2020

wbingli Nov 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PatMyron Nov 24, 2020 • edited Loading

Choose a reason for hiding this comment

wbingli Dec 3, 2020 • edited Loading

Choose a reason for hiding this comment

gruebel Dec 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gruebel commented Dec 16, 2020

wbingli Feb 1, 2021 • edited Loading

Choose a reason for hiding this comment

jaglade commented Feb 22, 2021

tata9001 commented Apr 1, 2021

Aleksandr-Filichkin commented Apr 12, 2021

vitali-pozniak commented Apr 16, 2021

mgmarino commented Apr 23, 2021

JoryUK commented May 7, 2021

wbingli commented May 10, 2021

gruebel commented May 10, 2021

wbingli commented May 12, 2021

shoddyknight commented May 13, 2021

samjarrett commented Jul 1, 2021

sfbaker7 commented Jul 1, 2021 • edited Loading

glc-froussel commented Jul 16, 2021

bool3012 commented Jul 26, 2021

felixfang commented Jul 27, 2021

bencovi commented Aug 4, 2021

felixfang commented Aug 6, 2021 • edited Loading

felixfang commented Sep 27, 2021 • edited Loading

Karthi96 commented Nov 8, 2021

reply2srij commented Jan 3, 2022

glc-froussel commented Jan 3, 2022

SeijiSuenaga commented Jan 5, 2022

glc-froussel commented Jan 5, 2022

mgmarino commented Jan 5, 2022 • edited Loading

SeijiSuenaga commented Jan 6, 2022

wbingli Nov 24, 2020 •

edited

Loading

PatMyron Nov 24, 2020 •

edited

Loading

wbingli Dec 3, 2020 •

edited

Loading

gruebel Dec 3, 2020 •

edited

Loading

wbingli Feb 1, 2021 •

edited

Loading

sfbaker7 commented Jul 1, 2021 •

edited

Loading

felixfang commented Aug 6, 2021 •

edited

Loading

felixfang commented Sep 27, 2021 •

edited

Loading

mgmarino commented Jan 5, 2022 •

edited

Loading