update LogGroup tagging impl and fix stack tag propagation

aws-logs-loggroup/aws-logs-loggroup.json

@@ -127,5 +127,7 @@
     "/properties/LogGroupName"
   ],
   "additionalProperties": false,
-  "taggable": true
+  "tagging": {
+    "taggable": true
+  }
 }

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

@@ -0,0 +1,163 @@
+package software.amazon.logs.loggroup;
+import software.amazon.logs.loggroup.Tag;
+
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+import com.google.common.collect.Sets;
+import org.apache.commons.collections.CollectionUtils;
+import org.apache.commons.collections.MapUtils;
+import org.apache.commons.lang3.ObjectUtils;
+import software.amazon.awssdk.awscore.AwsResponse;
+import software.amazon.awssdk.core.SdkClient;
+
+import software.amazon.cloudformation.proxy.AmazonWebServicesClientProxy;
+import software.amazon.cloudformation.proxy.Logger;
+import software.amazon.cloudformation.proxy.ProgressEvent;
+import software.amazon.cloudformation.proxy.ProxyClient;
+import software.amazon.cloudformation.proxy.ResourceHandlerRequest;
+
+public class TagHelper {
+    /**
+     * convertToMap
+     *
+     * Converts a collection of Tag objects to a tag-name -> tag-value map.
+     *
+     * Note: Tag objects with null tag values will not be included in the output
+     * map.
+     *
+     * @param tags Collection of tags to convert
+     * @return Converted Map of tags
+     */
+    public static Map<String, String> convertToMap(final Collection<Tag> tags) {
+        if (CollectionUtils.isEmpty(tags)) {
+            return Collections.emptyMap();
+        }
+        return tags.stream()
+            .filter(tag -> tag.getValue() != null)
+            .collect(Collectors.toMap(
+                Tag::getKey,
+                Tag::getValue,
+                (oldValue, newValue) -> newValue));
+    }
+
+    /**
+     * convertToSet
+     *
+     * Converts a tag map to a set of Tag objects.
+     *
+     * Note: Like convertToMap, convertToSet filters out value-less tag entries.
+     *
+     * @param tagMap Map of tags to convert
+     * @return Set of Tag objects
+     */
+    public static Set<Tag> convertToSet(final Map<String, String> tagMap) {
+        if (MapUtils.isEmpty(tagMap)) {
+            return Collections.emptySet();
+        }
+        return tagMap.entrySet().stream()
+            .filter(tag -> tag.getValue() != null)
+            .map(tag -> Tag.builder()
+                .key(tag.getKey())
+                .value(tag.getValue())
+                .build())
+            .collect(Collectors.toSet());
+    }
+
+    /**
+     * generateTagsForCreate
+     *
+     * Generate tags to put into resource creation request.
+     * This includes user defined tags and system tags as well.
+     */
+    public final Map<String, String> generateTagsForCreate(final ResourceModel resourceModel, final ResourceHandlerRequest<ResourceModel> handlerRequest) {
+        final Map<String, String> tagMap = new HashMap<>();
+
+        // merge system tags with desired resource tags if your service supports CloudFormation system tags
+        tagMap.putAll(handlerRequest.getSystemTags());
+
+        if (handlerRequest.getDesiredResourceTags() != null) {
+            tagMap.putAll(handlerRequest.getDesiredResourceTags());
+        }
+
+        // TODO: get tags from resource model based on your tag property name
+        // TODO: tagMap.putAll(convertToMap(resourceModel.getTags()));
+        return Collections.unmodifiableMap(tagMap);
+    }
+
+    /**
+     * shouldUpdateTags
+     *
+     * Determines whether user defined tags have been changed during update.
+     */
+    public static final boolean shouldUpdateTags(final ResourceModel resourceModel, final ResourceHandlerRequest<ResourceModel> handlerRequest) {
+        final Map<String, String> previousTags = getPreviouslyAttachedTags(handlerRequest);
+        final Map<String, String> desiredTags = getNewDesiredTags(resourceModel, handlerRequest);
+        return ObjectUtils.notEqual(previousTags, desiredTags);
+    }
+
+    /**
+     * getPreviouslyAttachedTags
+     *
+     * If stack tags and resource tags are not merged together in Configuration class,
+     * we will get previous attached user defined tags from both handlerRequest.getPreviousResourceTags (stack tags)
+     * and handlerRequest.getPreviousResourceState (resource tags).
+     */
+    public static Map<String, String> getPreviouslyAttachedTags(final ResourceHandlerRequest<ResourceModel> handlerRequest) {
+        // get previous stack level tags from handlerRequest
+        final Map<String, String> previousTags = handlerRequest.getPreviousResourceTags() != null ?
+            handlerRequest.getPreviousResourceTags() : Collections.emptyMap();
+
+        // TODO: get resource level tags from previous resource state based on your tag property name
+        // TODO: previousTags.putAll(handlerRequest.getPreviousResourceState().getTags());
+        return previousTags;
+    }
+
+    /**
+     * getNewDesiredTags
+     *
+     * If stack tags and resource tags are not merged together in Configuration class,
+     * we will get new user defined tags from both resource model and previous stack tags.
+     */
+    public static Map<String, String> getNewDesiredTags(final ResourceModel resourceModel, final ResourceHandlerRequest<ResourceModel> handlerRequest) {
+        // get new stack level tags from handlerRequest
+        final Map<String, String> desiredTags = handlerRequest.getDesiredResourceTags() != null ?
+            handlerRequest.getDesiredResourceTags() : Collections.emptyMap();
+
+        // TODO: get resource level tags from resource model based on your tag property name
+        // TODO: desiredTags.putAll(convertToMap(resourceModel.getTags()));
+        return desiredTags;
+    }
+
+    /**
+     * generateTagsToAdd
+     *
+     * Determines the tags the customer desired to define or redefine.
+     */
+    public static Map<String, String> generateTagsToAdd(final Map<String, String> previousTags, final Map<String, String> desiredTags) {
+        return desiredTags.entrySet().stream()
+            .filter(e -> !previousTags.containsKey(e.getKey()) || !Objects.equals(previousTags.get(e.getKey()), e.getValue()))
+            .collect(Collectors.toMap(
+                Map.Entry::getKey,
+                Map.Entry::getValue));
+    }
+
+    /**
+     * getTagsToRemove
+     *
+     * Determines the tags the customer desired to remove from the function.
+     */
+    public static Set<String> generateTagsToRemove(final Map<String, String> previousTags, final Map<String, String> desiredTags) {
+        final Set<String> desiredTagNames = desiredTags.keySet();
+
+        return previousTags.keySet().stream()
+            .filter(tagName -> !desiredTagNames.contains(tagName))
+            .collect(Collectors.toSet());
+    }
+}

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

@@ -1,7 +1,9 @@
 package software.amazon.logs.loggroup;
+import software.amazon.logs.loggroup.Tag;
 
 import com.google.common.collect.MapDifference;
 import com.google.common.collect.Maps;
+import software.amazon.awssdk.services.cloudwatchlogs.model.ListTagsLogGroupResponse;
 import software.amazon.awssdk.services.cloudwatchlogs.model.AssociateKmsKeyRequest;
 import software.amazon.awssdk.services.cloudwatchlogs.model.CloudWatchLogsException;
 import software.amazon.awssdk.services.cloudwatchlogs.model.DeleteRetentionPolicyRequest;
@@ -20,9 +22,11 @@
 import software.amazon.cloudformation.proxy.ResourceHandlerRequest;
 
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 import java.util.Objects;
 import java.util.Optional;
 import java.util.stream.Collectors;
@@ -39,12 +43,9 @@ public ProgressEvent<ResourceModel, CallbackContext> handleRequest(
 
         final ResourceModel model = request.getDesiredResourceState();
         final ResourceModel previousModel = request.getPreviousResourceState();
-        final Map<String, String> tags = request.getDesiredResourceTags();
-        final Map<String, String> previousTags = request.getPreviousResourceTags();
-
         final boolean retentionChanged = ! retentionUnchanged(previousModel, model);
         final boolean kmsKeyChanged = ! kmsKeyUnchanged(previousModel, model);
-        final boolean tagsChanged = ! tagsUnchanged(previousTags, tags);
+        final boolean tagsChanged =  TagHelper.shouldUpdateTags(model, request);
         if (retentionChanged && model.getRetentionInDays() == null) {
             deleteRetentionPolicy(proxy, request, logger);
         } else if (retentionChanged){
@@ -61,7 +62,7 @@ public ProgressEvent<ResourceModel, CallbackContext> handleRequest(
         }
 
         if (tagsChanged) {
-            updateTags(proxy, model, previousTags, tags, logger);
+            updateTags(proxy, model, request, logger);
         }
 
         return ProgressEvent.defaultSuccessHandler(model);
@@ -167,35 +168,40 @@ private void associateKmsKey(final AmazonWebServicesClientProxy proxy,
 
     private void updateTags(final AmazonWebServicesClientProxy proxy,
                             final ResourceModel model,
-                            final Map<String, String> previousTags,
-                            final Map<String, String> tags,
+                            final ResourceHandlerRequest<ResourceModel> request,
                             final Logger logger) {
-        MapDifference<String, String> tagsDifference = Maps.difference(Optional.ofNullable(previousTags).orElse(new HashMap<>()),
-                Optional.ofNullable(tags).orElse(new HashMap<>()));
-        final Map<String, String> tagsToRemove = tagsDifference.entriesOnlyOnLeft();
-        final Map<String, String> tagsToAdd = tagsDifference.entriesOnlyOnRight();
-        final Map<String, String> tagsToDiffer = tagsDifference.entriesDiffering().entrySet().stream()
-                .collect(Collectors.toMap(Map.Entry::getKey, tag -> tag.getValue().rightValue()));
-        final Map<String, String> tagsToUpdate = Stream.concat(tagsToAdd.entrySet().stream(), tagsToDiffer.entrySet().stream())
-                .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));
+
         try {
+            // Need to make a ListTagsLogGroup request here
+            // Since we launched tag support for LogGroup late, existing stack tags will not
+            // propagate to the LogGroup resource using getPreviouslyAttachedTags() which returns
+            // previous stack tags regardless if they are propagated to the resource or not.
+            final ListTagsLogGroupResponse listTagsResponse = proxy.injectCredentialsAndInvokeV2(Translator.translateToListTagsLogGroupRequest(model.getLogGroupName()),
+                    ClientBuilder.getClient()::listTagsLogGroup);
+
+            final Map<String, String> currentTags = listTagsResponse != null ? listTagsResponse.tags() : Collections.emptyMap();
+            final Map<String, String> desiredTags = TagHelper.getNewDesiredTags(model, request);
+
+            final Map<String, String> tagsToAdd = TagHelper.generateTagsToAdd(currentTags, desiredTags);
+            final Set<String> tagsToRemove = TagHelper.generateTagsToRemove(currentTags, desiredTags);
+
             if (!tagsToRemove.isEmpty()) {
-                final List<String> tagKeys = new ArrayList<>(tagsToRemove.keySet());
+                final List<String> tagKeys = new ArrayList<>(tagsToRemove);
                 proxy.injectCredentialsAndInvokeV2(Translator.translateToUntagLogGroupRequest(model.getLogGroupName(), tagKeys),
-                        ClientBuilder.getClient()::untagLogGroup);
-
+                    ClientBuilder.getClient()::untagLogGroup);
+                
                 final String message =
-                        String.format("%s [%s] successfully removed tags: [%s]",
-                                ResourceModel.TYPE_NAME, model.getLogGroupName(), tagKeys);
+                    String.format("%s [%s] successfully removed tags: [%s]",
+                            ResourceModel.TYPE_NAME, model.getLogGroupName(), tagKeys);
                 logger.log(message);
             }
-            if(!tagsToUpdate.isEmpty()) {
-                proxy.injectCredentialsAndInvokeV2(Translator.translateToTagLogGroupRequest(model.getLogGroupName(), tagsToUpdate),
-                        ClientBuilder.getClient()::tagLogGroup);
-
+            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(), tagsToUpdate);
+                    String.format("%s [%s] successfully added tags: [%s]",
+                        ResourceModel.TYPE_NAME, model.getLogGroupName(), tagsToAdd);
                 logger.log(message);
             }
         } catch (final ResourceNotFoundException e) {
@@ -225,11 +231,4 @@ 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 Map<String, String> previousTags, final Map<String, String> tags) {
-        if (previousTags == null && tags == null) {
-            return true;
-        }
-        return (previousTags != null && Objects.equals(previousTags, tags));
-    }
 }