feat(cdk): adding tags parameter option to cdk deploy command

[IsmaelMartinez]

Adding tags parameter option to cdk deploy command to allow tagging full stacks and their associated resources.

Now it will be possible to:

const app = new App();

const stack1 = new Stack(app, 'stack1', { tags: { foo: 'bar' } });
const stack2 = new Stacl(app, 'stack2');

stack1.node.apply(new Tag('fii', 'bug'));
stack2.node.apply(new Tag('boo', 'bug'));

That will produce

• stack1 with tags foo bar and fii bug
• stack2 with tags boo bug

It is possible also to override constructor tags with the stack.node.apply.

So doing:

stack1.node.apply(new Tag('foo', 'newBar');

stack1 will have tags foo newBar and fii bug

Last, but not least, it is also possible to pass it via arguments (using yargs) as in the following example:

cdk deploy --tags foo=bar --tags myTag=myValue

That will produce a stack with tags foo barand myTag myValue

Important
That will ignore tags provided by the constructor and/or aspects.

Fixes #932

---

Pull Request Checklist

• Testing
  • Unit test added (prefer not to modify an existing test, otherwise, it's probably a breaking change)
  • CLI change?: coordinate update of integration tests with team
  • cdk-init template change?: coordinated update of integration tests with team
• Docs
  • jsdocs: All public APIs documented
  • README: README and/or documentation topic updated
• Title and Description
  • Change type: title prefixed with fix, feat will appear in changelog
  • Title: use lower-case and doesn't end with a period
  • Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
  • Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
• Sensitive Modules (requires 2 PR approvers)
  • IAM Policy Document (in @aws-cdk/aws-iam)
  • EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
  • Grant APIs (only if not based on official documentation with a reference)

---

Notes:

• This is my first pull request so I will probably be missing quite a few things. Apologies for this. I will add comments in the lines were we have take some assumptions on how to do it.

• I haven't found any tests/readme around the functionality that I am extending so not added any extra tests/documentation. Do point me out to documentation/test cases that should be covered if required.

• Manually tested that the following works with a stack with a dynamoDB in it:
  [x] Creating a new stack with the --tags argument creates tags for the stack and the dynamoDB resource
  [x] Creating a new stack without the --tags argument does not create tags for the stack nor modifies any dynamoDB tags generated with Ability to specify Tags across all resources #1516
  [x] Updating a stack with a tags but not changed, correctly detects that there are no changes
  [x] Updating a stack with tag values or keys changes, updated the stack tags and their associated resources.
  [x] Updating a stack removing tag values, updates the stack removing the removed tags.

---

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

[IsmaelMartinez]

I have unmarked the tasks because I suspect that is for the reviewer to complete/not me. Let me know if that is the actual process or not. Thanks!

[eladb]

I think it would make more sense to apply stack-level tags in code (similar to any other tag) and propagate that information through CDK metadata to the toolkit. This feels like what people would expect and will also be more robust when it comes to multi-stack apps.

For example, I would expect this to "Just Work":

const app = new App();

const stack1 = new Stack(app, 'stack1', { tags: { foo: 'bar' } });
const stack2 = new Stacl(app, 'stack2');

stack2.node.apply(new Tag('boo', 'bug'));

Thew result should be:

1. All resources in stack1, including the stack itself have the { "foo": "bar" } tag.
2. All resources in stack2, including the stack itself have the { "boo": "bug" } tag.

@moofish32 I am curious what you think. about this.

[eladb]

See note

[IsmaelMartinez]

That sounds like the best solution. I just wanted to allow to tag the stacks without needing to use the aws cli.

What about providing both options? I will appreciate a few pointers on where the code (area, not specific) to change to allow the changes you requested. :)

[eladb]

Allow both options sounds good to me.

A few pointers: the way a CDK app can communicate something like this to the toolkit is by attaching CDK metadata (this.node.addMetadata) on the stack object, and then the toolkit can look that up and apply the tags upon create/update of the stack.

I wonder if we should generalize the Tag aspect such that instead of checking if a node is a CfnResource it will just check if the implements ITaggble or something like that, and then each node can provide it's own tagging strategy (resources will add the tag to the CFN props and stacks will add metadata that will tell the toolkit to apply).

The owner question is: if I apply a tag at the stack level, and we know CFN will automatically apply them to all taggable resources, maybe the propagation should work differently.

@moofish32, thoughts?

[IsmaelMartinez]

Waiting for @moofish32 answer but updated the PR with part of the @eladb requirements.

The code changes now does the following:

• Checks if arguments are passed for tags.
• If no arguments found, checks if there are Tags in the stack metadata.

I have tested that you can add stack tags with the following (been stack a cdk.Stack):

stack.node.addMetadata('Tags', [new cdk.Tag("Team", "Isma"), new cdk.Tag("Group", "fdsajkl3")]);

Note: should I change the PR title from parameters to arguments? Might be better wait until there is a more clear knowledge of all the things this does.

[eladb]

I am expecting the standard Tag aspect to work seamlessly:

const s = new MyStack(...);
s.node.apply(new cdk.Tag('key, value'));

[IsmaelMartinez]

I am expecting the standard Tag aspect to work seamlessly:

const s = new MyStack(...);
s.node.apply(new cdk.Tag('key, value'));

Agree but I was waiting for confirmation from @moofish32. This was just to confirm I could create a stack, add the "Tag" metadata and that will pass to the CDK part (so a WIP)

As far as I understand, by adding the tags on the stack, there is no need to add the tags using the aspects. Does adding both adds a benefit that I don't see?

[IsmaelMartinez]

I will see if I can dig around the aspects tomorrow, but only if we are really going to use them.

If, by tagging the stack, we tag all the resources, there might be a reason to keep them separate? Just wondering if that is a valid reasoning or we just tag everything (stack and resources with the aspects)

Thanks and apologies for my ignorance on this project, 1st typescript work and 1st AWS PR.

[IsmaelMartinez]

Hi @eladb ,

Could you check my previous message? I am not sure why we will need to be able to do both types of tagging (all stack vs all the resources minus the stack).

Maybe I am missing something in here. I will see if I can have a look at the tag aspects today.

Soon I will be on holidays for a few weeks so I will like to wrap this beforehand.

Any news also from @moofish32 ?

[moofish32]

Sorry for my delay. The reason I prefer the aspect over the CLI is I don't really want to type out all the tags for the stack on the command line or in bash via Jenkins. Aspects let me commit that to VCS. If the next thing we want is to override that for specific environments the CDK has many methods for that (e.g. context, config object, or json/yaml prop file).

Now to @eladb's point a stack does not currently implement ITaggable, but that's very little code. I would think we just let aspects behave normally and in the collectMetadata we just render the tags from the TagManager. The tag format seems to be little different Key=Value we could just add a formatter for stack type. We do need to change the TagAspect to only care about ITaggable as well. I don't think we need to worry about CfnResource. I did test if the resource has a tag and the stack has the same tag no errors or mistakes occurred for my simple security group test.

We might want to move the tag interface out of CfnResource and on to CfnElement?

[IsmaelMartinez]

Hi @eladb and @moofish32

Will need some tidy up, but code is not that far from the final solution (IMO). I will appreciate a bit of feedback to see if i am going in the right direction.

I have added a few changes but still an early WIP. Prefer to see it functionally working and then do the tests rather than writing all up and then needing to redo it.

The current version does support passing the tags as tag properties, applying the tag aspect and passing it with yargs.

To notice, if passing via yargs (--tags key=value) that then ignores the aspects one.

So now is possible to:

const app = new App();

const stack1 = new Stack(app, 'stack1', { tags: { foo: 'bar' } });
const stack2 = new Stacl(app, 'stack2');

stack1.node.apply(new Tag('fii', 'bug'));
stack2.node.apply(new Tag('boo', 'bug'));

That will produce

• stack1 with tags foo bar and fii bug
• stack2 with tags boo bug

and

cdk deploy --tags foo=bar

I have also updated a bit the code (pull from master a couple of days) so hopefully the integration is easier.

Thanks

[moofish32]

A couple of comments, and questions that I think we need @eladb or @rix0rrr to help with.

[moofish32]

@rix0rrr -- we are still separating interfaces across the cxapi right?

[IsmaelMartinez]

@rix0rrr , any updates on this?

[eladb]

Why do we need this as a public API?

[eladb]

https://github.com/IsmaelMartinez can you take a look?

[IsmaelMartinez]

Oh dear, 1st of all apologies. I will see if I can have a look at it tonight if not tomorrow morning

[skinny85]

@AlexRex I see you're running cdk --version, but then npx cdk deploy. Can you make sure both CDK CLI versions (the one you used for init, and the one you used for deploy) are in the same version?

[IsmaelMartinez]

I will continue investigation and others in the open issue.

[AlexRex]

@skinny85 is the same version, tested out.

@IsmaelMartinez no worries, I just following the other issue. Thanks!

[iDVB]

Is there docs on what the net-net way is to implement these tags?
This thread jumped around a bit and I didn't see the Guide being updated as part of this merge?