Add tag support to LogGroup resource #53
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| package software.amazon.logs.loggroup; | ||
|
|
||
| import com.google.common.collect.Sets; | ||
| import software.amazon.cloudformation.exceptions.CfnInternalFailureException; | ||
| import software.amazon.cloudformation.exceptions.CfnResourceConflictException; | ||
| import software.amazon.cloudformation.exceptions.CfnServiceInternalErrorException; | ||
|
|
@@ -16,7 +17,12 @@ | |
| import software.amazon.awssdk.services.cloudwatchlogs.model.ResourceNotFoundException; | ||
| import software.amazon.awssdk.services.cloudwatchlogs.model.ServiceUnavailableException; | ||
|
|
||
| import java.util.HashSet; | ||
| import java.util.List; | ||
| import java.util.Objects; | ||
| import java.util.Optional; | ||
| import java.util.Set; | ||
| import java.util.stream.Collectors; | ||
|
|
||
| public class UpdateHandler extends BaseHandler<CallbackContext> { | ||
|
|
||
|
|
@@ -27,11 +33,12 @@ public ProgressEvent<ResourceModel, CallbackContext> handleRequest( | |
| final CallbackContext callbackContext, | ||
| final Logger logger) { | ||
|
|
||
| // Everything except RetentionPolicyInDays, KmsKeyId and Tags is createOnly | ||
gruebel marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| 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); | ||
|
There was a problem hiding this comment. Should use You will need to override the There was a problem hiding this comment. Good point, makes sense and will try to add it. |
||
| if (retentionChanged && model.getRetentionInDays() == null) { | ||
| deleteRetentionPolicy(proxy, request, logger); | ||
| } else if (retentionChanged){ | ||
|
|
@@ -47,6 +54,10 @@ public ProgressEvent<ResourceModel, CallbackContext> handleRequest( | |
| associateKmsKey(proxy, request, logger); | ||
| } | ||
|
|
||
| if (tagsChanged) { | ||
| updateTags(proxy, previousModel, model, logger); | ||
| } | ||
|
|
||
| return ProgressEvent.defaultSuccessHandler(model); | ||
| } | ||
|
|
||
|
|
@@ -148,6 +159,39 @@ private void associateKmsKey(final AmazonWebServicesClientProxy proxy, | |
| logger.log(kmsKeyMessage); | ||
| } | ||
|
|
||
| private void updateTags(final AmazonWebServicesClientProxy proxy, | ||
| final ResourceModel previousModel, | ||
| final ResourceModel model, | ||
| final Logger logger) { | ||
| final Set<Tag> previousTags = Optional.ofNullable(previousModel).map(ResourceModel::getTags).orElse(new HashSet<>()); | ||
| final Set<Tag> newTags = Optional.ofNullable(model.getTags()).orElse(new HashSet<>()); | ||
| final Set<Tag> tagsToRemove = Sets.difference(previousTags, newTags); | ||
| final Set<Tag> tagsToAdd = Sets.difference(newTags, previousTags); | ||
| try { | ||
| if (!tagsToRemove.isEmpty()) { | ||
| final List<String> tagKeys = tagsToRemove.stream().map(Tag::getKey).collect(Collectors.toList()); | ||
| proxy.injectCredentialsAndInvokeV2(Translator.translateToUntagLogGroupRequest(model.getLogGroupName(), tagKeys), | ||
| ClientBuilder.getClient()::untagLogGroup); | ||
|
|
||
| final String message = | ||
| String.format("%s [%s] successfully removed tags: [%s]", | ||
| ResourceModel.TYPE_NAME, model.getLogGroupName(), tagKeys); | ||
| logger.log(message); | ||
| } | ||
| if(!tagsToAdd.isEmpty()) { | ||
| proxy.injectCredentialsAndInvokeV2(Translator.translateToTagLogGroupRequest(model.getLogGroupName(), tagsToAdd), | ||
| ClientBuilder.getClient()::tagLogGroup); | ||
|
|
||
| final String message = | ||
| String.format("%s [%s] successfully added tags: [%s]", | ||
| ResourceModel.TYPE_NAME, model.getLogGroupName(), tagsToAdd); | ||
| logger.log(message); | ||
| } | ||
| } catch (final ResourceNotFoundException e) { | ||
|
There was a problem hiding this comment. 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 There was a problem hiding this comment. aws-cloudformation/cloudformation-cli-java-plugin#306 should handle There was a problem hiding this comment. 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 😄 |
||
| throwNotFoundException(model); | ||
| } | ||
| } | ||
|
|
||
| private void throwNotFoundException(final ResourceModel model) { | ||
| throw new software.amazon.cloudformation.exceptions.ResourceNotFoundException(ResourceModel.TYPE_NAME, | ||
| Objects.toString(model.getPrimaryIdentifier())); | ||
|
|
@@ -161,4 +205,11 @@ private static boolean retentionUnchanged(final ResourceModel previousModel, fin | |
| private static boolean kmsKeyUnchanged(final ResourceModel previousModel, final ResourceModel model) { | ||
| return (previousModel != null && Objects.equals(model.getKmsKeyId(), previousModel.getKmsKeyId())); | ||
| } | ||
|
|
||
| private static boolean tagsUnchanged(final ResourceModel previousModel, final ResourceModel model) { | ||
| if (previousModel == null && model.getTags() == null) { | ||
| return true; | ||
| } | ||
| return (previousModel != null && Objects.equals(model.getTags(), previousModel.getTags())); | ||
| } | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
particularly issues with
ReadHandlerssince permissions expand without customer changes. We have an internal issue to eventually find a platform-wide solution to this problemThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
GetAttfunction to get the attribute data as well as for drift detection for the resource.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the insight. Makes sense with the stach role permission for stack updates. I also used it in my previous work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.