-
Notifications
You must be signed in to change notification settings - Fork 35
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 tag support to LogGroup resource #53
Changes from 3 commits
0406d2b
d35f9a4
a1ab815
b79f187
e8b0238
f5fd0dc
e427257
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
package software.amazon.logs.loggroup; | ||
|
||
import software.amazon.awssdk.services.cloudwatchlogs.model.ListTagsLogGroupResponse; | ||
import software.amazon.cloudformation.proxy.AmazonWebServicesClientProxy; | ||
import software.amazon.cloudformation.proxy.Logger; | ||
import software.amazon.cloudformation.proxy.ProgressEvent; | ||
|
@@ -25,14 +26,17 @@ public ProgressEvent<ResourceModel, CallbackContext> handleRequest( | |
} | ||
|
||
DescribeLogGroupsResponse response = null; | ||
ListTagsLogGroupResponse tagsResponse = null; | ||
try { | ||
response = proxy.injectCredentialsAndInvokeV2(Translator.translateToReadRequest(model), | ||
ClientBuilder.getClient()::describeLogGroups); | ||
ClientBuilder.getClient()::describeLogGroups); | ||
tagsResponse = proxy.injectCredentialsAndInvokeV2(Translator.translateToListTagsLogGroupRequest(model.getLogGroupName()), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. particularly issues with There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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. |
||
ClientBuilder.getClient()::listTagsLogGroup); | ||
} catch (final ResourceNotFoundException e) { | ||
throwNotFoundException(model); | ||
} | ||
|
||
final ResourceModel modelFromReadResult = Translator.translateForRead(response); | ||
final ResourceModel modelFromReadResult = Translator.translateForRead(response, tagsResponse); | ||
if (modelFromReadResult.getLogGroupName() == null) { | ||
throwNotFoundException(model); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,14 +5,21 @@ | |
import software.amazon.awssdk.services.cloudwatchlogs.model.DeleteRetentionPolicyRequest; | ||
import software.amazon.awssdk.services.cloudwatchlogs.model.DescribeLogGroupsRequest; | ||
import software.amazon.awssdk.services.cloudwatchlogs.model.DescribeLogGroupsResponse; | ||
import software.amazon.awssdk.services.cloudwatchlogs.model.ListTagsLogGroupRequest; | ||
import software.amazon.awssdk.services.cloudwatchlogs.model.ListTagsLogGroupResponse; | ||
import software.amazon.awssdk.services.cloudwatchlogs.model.PutRetentionPolicyRequest; | ||
import software.amazon.awssdk.services.cloudwatchlogs.model.DisassociateKmsKeyRequest; | ||
import software.amazon.awssdk.services.cloudwatchlogs.model.AssociateKmsKeyRequest; | ||
import software.amazon.awssdk.services.cloudwatchlogs.model.TagLogGroupRequest; | ||
import software.amazon.awssdk.services.cloudwatchlogs.model.UntagLogGroupRequest; | ||
|
||
import java.util.Collection; | ||
import java.util.Collections; | ||
import java.util.List; | ||
import java.util.Map; | ||
import java.util.Objects; | ||
import java.util.Optional; | ||
import java.util.Set; | ||
import java.util.stream.Collectors; | ||
import java.util.stream.Stream; | ||
|
||
|
@@ -42,6 +49,7 @@ static CreateLogGroupRequest translateToCreateRequest(final ResourceModel model) | |
return CreateLogGroupRequest.builder() | ||
.logGroupName(model.getLogGroupName()) | ||
.kmsKeyId(model.getKmsKeyId()) | ||
.tags(translateTagsToSdk(model.getTags())) | ||
.build(); | ||
} | ||
|
||
|
@@ -71,7 +79,27 @@ static AssociateKmsKeyRequest translateToAssociateKmsKeyRequest(final ResourceMo | |
.build(); | ||
} | ||
|
||
static ResourceModel translateForRead(final DescribeLogGroupsResponse response) { | ||
static ListTagsLogGroupRequest translateToListTagsLogGroupRequest(final String logGroupName) { | ||
return ListTagsLogGroupRequest.builder() | ||
.logGroupName(logGroupName) | ||
.build(); | ||
} | ||
|
||
static TagLogGroupRequest translateToTagLogGroupRequest(final String logGroupName, final Set<Tag> tags) { | ||
return TagLogGroupRequest.builder() | ||
.logGroupName(logGroupName) | ||
.tags(translateTagsToSdk(tags)) | ||
.build(); | ||
} | ||
|
||
static UntagLogGroupRequest translateToUntagLogGroupRequest(final String logGroupName, final List<String> tagKeys) { | ||
return UntagLogGroupRequest.builder() | ||
.logGroupName(logGroupName) | ||
.tags(tagKeys) | ||
.build(); | ||
} | ||
|
||
static ResourceModel translateForRead(final DescribeLogGroupsResponse response, final ListTagsLogGroupResponse tagsResponse) { | ||
final String logGroupName = streamOfOrEmpty(response.logGroups()) | ||
.map(software.amazon.awssdk.services.cloudwatchlogs.model.LogGroup::logGroupName) | ||
.filter(Objects::nonNull) | ||
|
@@ -92,26 +120,29 @@ static ResourceModel translateForRead(final DescribeLogGroupsResponse response) | |
.filter(Objects::nonNull) | ||
.findAny() | ||
.orElse(null); | ||
final Set<Tag> tags = translateSdkToTags(tagsResponse.tags()); | ||
return ResourceModel.builder() | ||
.arn(logGroupArn) | ||
.logGroupName(logGroupName) | ||
.retentionInDays(retentionInDays) | ||
.kmsKeyId(kmsKeyId) | ||
.tags(tags) | ||
.build(); | ||
} | ||
|
||
static List<ResourceModel> translateForList(final DescribeLogGroupsResponse response) { | ||
static List<ResourceModel> translateForList(final DescribeLogGroupsResponse response, final Map<String, ListTagsLogGroupResponse> tagResponses) { | ||
return streamOfOrEmpty(response.logGroups()) | ||
.map(logGroup -> ResourceModel.builder() | ||
.arn(logGroup.arn()) | ||
.logGroupName(logGroup.logGroupName()) | ||
.retentionInDays(logGroup.retentionInDays()) | ||
.kmsKeyId(logGroup.kmsKeyId()) | ||
.tags(translateSdkToTags(tagResponses.get(logGroup.logGroupName()).tags())) | ||
.build()) | ||
.collect(Collectors.toList()); | ||
} | ||
|
||
private static <T> Stream<T> streamOfOrEmpty(final Collection<T> collection) { | ||
static <T> Stream<T> streamOfOrEmpty(final Collection<T> collection) { | ||
return Optional.ofNullable(collection) | ||
.map(Collection::stream) | ||
.orElseGet(Stream::empty); | ||
|
@@ -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()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. will result in same error as below |
||
return null; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of returning null can we return an empty map? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if I don't return
|
||
} | ||
return tags.stream().collect(Collectors.toMap(Tag::getKey, Tag::getValue)); | ||
} | ||
|
||
static Set<Tag> translateSdkToTags(final Map<String, String> tags) { | ||
if (tags == null || tags.isEmpty()) { | ||
return null; | ||
} | ||
return tags.entrySet().stream().map(tag -> new Tag(tag.getKey(), tag.getValue())) | ||
.collect(Collectors.toSet()); | ||
} | ||
} |
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,11 @@ public ProgressEvent<ResourceModel, CallbackContext> handleRequest( | |
final CallbackContext callbackContext, | ||
final Logger logger) { | ||
|
||
// Everything except RetentionPolicyInDays and KmsKeyId is createOnly | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should use You will need to override the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point, makes sense and will try to add it. |
||
if (retentionChanged && model.getRetentionInDays() == null) { | ||
deleteRetentionPolicy(proxy, request, logger); | ||
} else if (retentionChanged){ | ||
|
@@ -47,6 +53,10 @@ public ProgressEvent<ResourceModel, CallbackContext> handleRequest( | |
associateKmsKey(proxy, request, logger); | ||
} | ||
|
||
if (tagsChanged) { | ||
updateTags(proxy, previousModel, model, logger); | ||
} | ||
|
||
return ProgressEvent.defaultSuccessHandler(model); | ||
} | ||
|
||
|
@@ -148,6 +158,41 @@ 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. aws-cloudformation/cloudformation-cli-java-plugin#306 should handle There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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); | ||
} catch (final InvalidParameterException e) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
throw new CfnInternalFailureException(e); | ||
} | ||
} | ||
|
||
private void throwNotFoundException(final ResourceModel model) { | ||
throw new software.amazon.cloudformation.exceptions.ResourceNotFoundException(ResourceModel.TYPE_NAME, | ||
Objects.toString(model.getPrimaryIdentifier())); | ||
|
@@ -161,4 +206,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())); | ||
} | ||
} |
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.
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?
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, changed it and moved the version to the properties section