feat(events-targets): Add tagging for ECS tasks triggered by an event #23838

rdbatch · 2023-01-25T20:05:15Z

This adds the ability to pass a tagList and the propagateTags flag to EC2 and Fargate ECS tasks triggered by an event. Users can leverage either or both of these attributes to apply tags an ECS task that's triggered through an Event. Both of these attributes are defined in the EcsParameters for a Rule https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-events-rule-ecsparameters.html.

This closes #9823.

This is a non-breaking change. These new fields are added as optional with default values such that it won't impact existing stacks. Also note that these are two separate fields: a user can pass either or both but they're not dependent on each other since they have different uses. propagateTags will copy tags from the task definition, while tagList provides an explicit set of tags to be applied to the task but that are not on the task definition. In either case, these tags are separate from the tags on the EventBridge bus or Rule.

I did see that there was previously a PR opened for this in the past which was closed for staleness (#19583). There was a comment on there asking if this could be added to the existing tag system (#19583 (review)). I'm not sure if that would make sense in this case since the resource being created at deploy-time by CloudFormation isn't the resource being tagged. The event rule itself is instead being given the flag to pass along tags onto the Task that it's creating at some time in the future when the schedule triggers.I did see that there was previously a PR opened for this in the past which was closed for staleness (#19583). There was a comment on there asking if this could be added to the existing tag system (#19583 (review)). I'm not sure if that would make sense in this case since the resource being created at deploy-time by CloudFormation isn't the resource being tagged. The event rule itself is instead being given the flag to pass along tags onto the Task that it's creating at some time in the future when the schedule triggers.

All Submissions:

[x ] Have you followed the guidelines in our Contributing guide?

Adding new Construct Runtime Dependencies:

This PR adds new construct runtime dependencies following the process described here

New Features

[x ] Have you added the new feature to an integration test?
- [x ] Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

…ed event Add the ability to set tags for ECS tasks triggered by a scheduled event

gitpod-io · 2023-01-25T20:05:21Z

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

rdbatch · 2023-01-25T20:33:55Z

I'll get a README update added here, it looks like that got lost in the shuffle as this was cut over from #23666

Fixed a few minor issues on existing README entries that Rosetta threw errors on

✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.

rdbatch · 2023-01-25T21:11:04Z

packages/@aws-cdk/aws-events-targets/README.md

-  logEvent: targets.LogGroupTargetInput({
-    timestamp: events.EventField.from('$.time'),
-    message: events.EventField.from('$.detail-type'),
+  logEvent: targets.LogGroupTargetInput.fromObject({


Heads up to reviewers: these updates that aren't specifically related to ECS were required because they would not compile when running Rosetta.

…ed in a separate chore PR later

TheRealAmazonKendra · 2023-01-26T04:04:31Z

Thanks @TheRealAmazonKendra for taking a look! I understand where you're coming from in terms of splitting it up into two separate PRs (one for the changes to aws-events-targets and one for aws-ecs-patterns). I've submitted the first one as #23838 and I can submit the other one pending the first being merged since it's dependent on those changes.

My initial feedback, however, is that propagateTags seems to rely on tagList having values supplied. That interdependency should be baked into the contract.

I've clarified this in the description for the second PR, but these are separate fields that can be passed independently of each other and aren't interdependent.

Also, just a question here but in my quick scroll, I see that you're using CfnTag instead of a Tag resource. Would it be feasible to implement the resource so that we didn't have use the underlying L1 there?

Good call out, this was a miss on my part and was updated in the new PR. Thanks.

With that, we can probably close out this PR and move any subsequent conversation to the new one.

Pulling in this convo from the last PR to continue. Firstly, thank you for splitting this up! I appreciate the extra effort so we can provide better feedback!

Can you help me understand why propagateTags wouldn't rely on tagList? This isn't one of my areas of expertise so it would seem to me that you'd have to have values in tagList to propagate them, but that's a bit on an assumption on my part.

TheRealAmazonKendra · 2023-01-26T04:23:15Z

@Mergifyio update

mergify · 2023-01-26T04:23:51Z

update

✅ Branch has been successfully updated

TheRealAmazonKendra · 2023-01-26T04:10:20Z

packages/@aws-cdk/aws-events-targets/lib/ecs-task.ts

@@ -118,12 +148,17 @@ export class EcsTask implements events.IRuleTarget {
    this.taskDefinition = props.taskDefinition;
    this.taskCount = props.taskCount ?? 1;
    this.platformVersion = props.platformVersion;
+    this.propagateTags = props.propagateTags === true ? ecs.PropagatedTagSource.TASK_DEFINITION : undefined ;


This is giving me pause. If CloudFormation expects something other than a true or false here, I think we're potentially setting ourselves up for breaking changes if they ever add more allowed values here.

I can see how this could become problematic even though this is the only allowed value today. I can update this so it takes a value of type ecs.PropagatedTagSource and then validates that you passed the correct value with some helpful messaging. That way if a new source is ever added, the validation can be removed or modified accordingly and existing code won't break.

TheRealAmazonKendra · 2023-01-26T04:26:41Z

I did see that there was previously a PR opened for this in the past which was closed for staleness (#19583). There was a comment on there asking if this could be added to the existing tag system (#19583 (review)). I'm not sure if that would make sense in this case since the resource being created at deploy-time by CloudFormation isn't the resource being tagged. The event rule itself is instead being given the flag to pass along tags onto the Task that it's creating at some time in the future when the schedule triggers.

Happy to discuss this further if the CDK team disagrees, but from an implementation perspective it seems like it would make sense to keep the tagList and propagateTags fields as configuration attributes on scheduled task to more closely map to how it's being created by CloudFormation.

This comment also gives me some pause simply because I'm not sure of the answer here. Asking for some additional feedback from the team.

rdbatch · 2023-01-26T12:17:12Z

Can you help me understand why propagateTags wouldn't rely on tagList? This isn't one of my areas of expertise so it would seem to me that you'd have to have values in tagList to propagate them, but that's a bit on an assumption on my part.

Yep, so when an ECS Task is triggered by an event, propagateTags will pull across the tags from the Task Definition to apply to the Task. These tags are inherently applied to the Task Definition itself and you just want them copied to the tasks that get spawned. tagList is a separate set of tags that get applied to every task triggered by this particluar event rule.

A use case example: I might want to tag all of my tasks with my team's cost allocation tags, so I'll apply those to the task definition and set them to propagate when triggered by an event using propagateTags. I might trigger these tasks through a varienty of different means (different schedules, manual jobs, etc.) so I'd add additional tags using tagList to indicate which tasks were triggered by a scheduled event so that the task will have both sets of tags applied to it.

Ultimately this rolls into the discussion of why I don't think this makes sense to roll up into the CDK tagging model as well. The code that was modified here creates an Event Rule. But what's being tagged isn't that Rule, it's the ECS Task that's created at some point in the future whenever the Rule fires.

…olean to avoid breaking changes should the allowed values change in the future

Pull request has been modified.

rdbatch · 2023-02-16T12:56:40Z

This comment also gives me some pause simply because I'm not sure of the answer here. Asking for some additional feedback from the team.

@TheRealAmazonKendra Have you had a chance to talk with the team and take another look at this PR?

TheRealAmazonKendra · 2023-03-02T01:20:37Z

Hey, sorry for the delay on this! One more question about the potential interdependency between the two props. Would you have a tagsList even when you aren't using propagateTags? I'm getting the impression that the answer is yes from previous comments but I just want to validate that assumption.

TheRealAmazonKendra · 2023-03-02T01:20:44Z

@Mergifyio update

mergify · 2023-03-02T01:20:54Z

update

❌ Base branch update has failed

refusing to allow a GitHub App to create or update workflow .github/workflows/issue-label-assign.yml without workflows permission
err-code: 544A0

rdbatch · 2023-03-02T03:13:40Z

Hey, sorry for the delay on this! One more question about the potential interdependency between the two props. Would you have a tagsList even when you aren't using propagateTags? I'm getting the impression that the answer is yes from previous comments but I just want to validate that assumption.

Yep, you could pass either parameter separately or both together. So for example you might pass a tagList alone if you want all tasks triggered by an event to have a tag like triggeredBy: schedule and you don't want to propagate all of the tags from the task definition.

For this example I'd set it up something like:

rule.addTarget(
   new targets.EcsTask( {
       cluster: cluster,
       taskDefinition: taskDefinition,
       tagList: [
         {
           key: 'triggeredBy',
           value: 'schedule',
         },
       ],
     })
 );

Now even if my task definition has say 25 other tags on it, the only tag that gets applied to the running task that was triggered by this event rule will be triggeredBy.

rdbatch · 2023-03-28T11:08:44Z

Hey @TheRealAmazonKendra, have you had a chance to take another look at this and my response above?

TheRealAmazonKendra · 2023-03-29T04:12:51Z

Hey @TheRealAmazonKendra, have you had a chance to take another look at this and my response above?

Apologies for the delay. I was unexpectedly out of the office for a few weeks. I will re-review this in the next day or two.

aws-cdk-automation · 2023-04-01T00:08:14Z

This PR has been in the MERGE CONFLICTS state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

aws-cdk-automation · 2023-04-07T00:07:32Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

corymhall

Just a couple more comments and then I think we are good!

corymhall · 2023-04-17T14:23:36Z

packages/aws-cdk-lib/aws-events-targets/lib/ecs-task.ts

@@ -7,6 +7,20 @@ import { Construct } from 'constructs';
 import { ContainerOverride } from './ecs-task-properties';
 import { addToDeadLetterQueueResourcePolicy, bindBaseTargetConfig, singletonEventRole, TargetBaseProps } from './util';

+/**
+  * Tag


Can you update this description to give details on what this is? Also the
docstring for key and value

Updated the docstrings here

packages/aws-cdk-lib/aws-events-targets/lib/ecs-task.ts

corymhall · 2023-04-17T14:32:24Z

packages/aws-cdk-lib/aws-events-targets/lib/ecs-task.ts

+     *
+     * @default - No tags are applied to the task
+     */
+  readonly tagList?: Tag[]


What about just tags? I don't think we need to say List since the type tells
you it is a list.

I agree that tags is probably better here. I chose tagList initially to match the CloudFormation parameter name https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-events-rule-ecsparameters.html#cfn-events-rule-ecsparameters-taglist. Since this is a level 2 construct though I suppose there's no need to faithfully match that naming and will get that update applied.

corymhall · 2023-04-17T14:33:04Z

packages/aws-cdk-lib/aws-events-targets/README.md

@@ -337,3 +337,33 @@ rule.addTarget(new targets.EventBus(
  ),
 ));
 ```
+
+## Run an ECS Task


Can you add a little section here about tags? Based on the conversation so far
on this PR I think it would help users to understand the different types of tags
and how they are used.

Yep that sounds good, I'm not sure why GitHub isn't showing this diff as outdated now but I've gone ahead and added some clarifying notes about tagging here.

aws-cdk-automation · 2023-04-18T00:07:37Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

Rename tagList to tags

Pull request has been modified.

mergify · 2023-04-18T17:03:35Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2023-04-18T17:42:33Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: b55edb1
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2023-04-18T17:43:01Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

This was enabled by PR #23838 which adds support for the `propagateTags` and `tags` attributes to the ecs-patterns module. With the changes in this PR, tags can now be applied to tasks that are launched as scheduled tasks with the proper tags passed through to the event target. This PR contributes to (but does not completely close) #25106 . ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

Add the ability to propagate tags on ECS tasks triggered by a schedul…

680dcc5

…ed event Add the ability to set tags for ECS tasks triggered by a scheduled event

github-actions bot added the beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK label Jan 25, 2023

aws-cdk-automation requested a review from a team January 25, 2023 20:05

github-actions bot added effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p1 labels Jan 25, 2023

aws-cdk-automation previously requested changes Jan 25, 2023

View reviewed changes

rdbatch mentioned this pull request Jan 25, 2023

feat(aws-ecs-patterns): Add tagList and propagateTags to scheduled tasks #23666

Closed

4 tasks

rdbatch changed the title ~~feat(aws-events-targets): Add tagging for ECS tasks triggered by an event~~ feat(events-targets): Add tagging for ECS tasks triggered by an event Jan 25, 2023

Added README snippet for triggering an ECS task from an Event

66dc94e

Fixed a few minor issues on existing README entries that Rosetta threw errors on

rdbatch commented Jan 25, 2023

View reviewed changes

Remove README cleanups that aren't required for Rosetta, to be re-add…

20b1ddc

…ed in a separate chore PR later

Merge branch 'main' into events-targets-ecs-propagate-tags

9395fd0

TheRealAmazonKendra previously requested changes Jan 26, 2023

View reviewed changes

Update propagateTags to take the enum of valid values instead of a bo…

7230673

…olean to avoid breaking changes should the allowed values change in the future

rdbatch requested a review from TheRealAmazonKendra January 26, 2023 14:31

Merge branch 'main' into events-targets-ecs-propagate-tags

d3fe4bd

Merge branch 'main' into events-targets-ecs-propagate-tags

0b0644f

Merge branch 'main' into events-targets-ecs-propagate-tags

652373d

Merge branch 'main' into events-targets-ecs-propagate-tags

c012487

corymhall previously requested changes Apr 17, 2023

View reviewed changes

corymhall self-assigned this Apr 17, 2023

rdbatch added 3 commits April 18, 2023 07:24

Merge branch 'main' into events-targets-ecs-propagate-tags

81c729e

Update docstrings per PR feedback

acf5cf2

Rename tagList to tags

Merge branch 'aws:main' into events-targets-ecs-propagate-tags

ab72e36

rdbatch added 3 commits April 18, 2023 10:31

Merge branch 'main' into events-targets-ecs-propagate-tags

58e31cc

Update organization around tagging comment

373ada2

Merge branch 'main' into events-targets-ecs-propagate-tags

f509634

corymhall approved these changes Apr 18, 2023

View reviewed changes

Merge branch 'main' into events-targets-ecs-propagate-tags

b55edb1

mergify bot merged commit e3bc59a into aws:main Apr 18, 2023

blimmer mentioned this pull request Apr 20, 2023

aws-ecs-patterns: Support setting EcsParameters for a scheduled task. #25106

Open

2 tasks

rdbatch mentioned this pull request Apr 20, 2023

feat(ecs-patterns): Tagging support for scheduled tasks #25222

Merged

rdbatch deleted the events-targets-ecs-propagate-tags branch April 21, 2023 13:09

filletofish mentioned this pull request Jul 2, 2023

feat(scheduler): ScheduleGroup #26196

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(events-targets): Add tagging for ECS tasks triggered by an event #23838

feat(events-targets): Add tagging for ECS tasks triggered by an event #23838

rdbatch commented Jan 25, 2023 •

edited

Loading

gitpod-io bot commented Jan 25, 2023

aws-cdk-automation left a comment

rdbatch commented Jan 25, 2023

rdbatch Jan 25, 2023

TheRealAmazonKendra commented Jan 26, 2023

TheRealAmazonKendra commented Jan 26, 2023

mergify bot commented Jan 26, 2023

TheRealAmazonKendra Jan 26, 2023

rdbatch Jan 26, 2023 •

edited

Loading

rdbatch Jan 26, 2023

TheRealAmazonKendra commented Jan 26, 2023

rdbatch commented Jan 26, 2023

rdbatch commented Feb 16, 2023

TheRealAmazonKendra commented Mar 2, 2023

TheRealAmazonKendra commented Mar 2, 2023

mergify bot commented Mar 2, 2023

rdbatch commented Mar 2, 2023

rdbatch commented Mar 28, 2023

TheRealAmazonKendra commented Mar 29, 2023

aws-cdk-automation commented Apr 1, 2023

aws-cdk-automation commented Apr 7, 2023

corymhall left a comment

corymhall Apr 17, 2023

rdbatch Apr 18, 2023

corymhall Apr 17, 2023

rdbatch Apr 18, 2023 •

edited

Loading

corymhall Apr 17, 2023

rdbatch Apr 18, 2023

aws-cdk-automation commented Apr 18, 2023

mergify bot commented Apr 18, 2023

aws-cdk-automation commented Apr 18, 2023

mergify bot commented Apr 18, 2023

feat(events-targets): Add tagging for ECS tasks triggered by an event #23838

feat(events-targets): Add tagging for ECS tasks triggered by an event #23838

Conversation

rdbatch commented Jan 25, 2023 • edited Loading

All Submissions:

Adding new Construct Runtime Dependencies:

New Features

gitpod-io bot commented Jan 25, 2023

aws-cdk-automation left a comment

Choose a reason for hiding this comment

rdbatch commented Jan 25, 2023

Choose a reason for hiding this comment

TheRealAmazonKendra commented Jan 26, 2023

TheRealAmazonKendra commented Jan 26, 2023

mergify bot commented Jan 26, 2023

✅ Branch has been successfully updated

Choose a reason for hiding this comment

rdbatch Jan 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheRealAmazonKendra commented Jan 26, 2023

rdbatch commented Jan 26, 2023

rdbatch commented Feb 16, 2023

TheRealAmazonKendra commented Mar 2, 2023

TheRealAmazonKendra commented Mar 2, 2023

mergify bot commented Mar 2, 2023

❌ Base branch update has failed

rdbatch commented Mar 2, 2023

rdbatch commented Mar 28, 2023

TheRealAmazonKendra commented Mar 29, 2023

aws-cdk-automation commented Apr 1, 2023

aws-cdk-automation commented Apr 7, 2023

corymhall left a 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

rdbatch Apr 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aws-cdk-automation commented Apr 18, 2023

mergify bot commented Apr 18, 2023

aws-cdk-automation commented Apr 18, 2023

AWS CodeBuild CI Report

mergify bot commented Apr 18, 2023

rdbatch commented Jan 25, 2023 •

edited

Loading

rdbatch Jan 26, 2023 •

edited

Loading

rdbatch Apr 18, 2023 •

edited

Loading