Skip to content
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

s3 notifications: be able to tag custom resource #1497

Closed
manrueda opened this issue Jan 8, 2019 · 4 comments · Fixed by #1451 or #1724
Closed

s3 notifications: be able to tag custom resource #1497

manrueda opened this issue Jan 8, 2019 · 4 comments · Fixed by #1451 or #1724
Labels
@aws-cdk/aws-s3 Related to Amazon S3 feature-request A feature should be added or improved.

Comments

@manrueda
Copy link

manrueda commented Jan 8, 2019

When S3 notifications is used, a custom Resource is created to allow this with a Lambda Function backing this custom resource (fixed logicalId: BucketNotificationsHandler050a0587b7544547bf325f094a3db834).

In our AWS structure we have to tag all resource with some values to be compliant with our internal rules. This internal Lambda resource is not expose in any way now (the only way is to copy the fixed logicalId and get it from the root with tryFindChild).

I was thinking in exposing this fixed id or the resource instance itself can be a good idea so tags can be added to the resource.

I am open to create a PR with the proposed changed.

Thanks!

@eladb
Copy link
Contributor

eladb commented Feb 4, 2019

@moofish32's PR #1451 should address this concern without needing to expose the underlying resource. @manrueda you'll be able to simply apply a tag at any level and it will automatically propagate to all underlying resources. Would that address your use case?

@eladb eladb added the feature-request A feature should be added or improved. label Feb 4, 2019
@eladb eladb changed the title S3 Notifications: Customize notifications lambda s3 notifications: be able to tag custom resource Feb 4, 2019
@eladb eladb added the @aws-cdk/aws-s3 Related to Amazon S3 label Feb 4, 2019
eladb pushed a commit that referenced this issue Feb 6, 2019
A generalized aspect framework is added. Aspects can be used to affect the construct tree without modifying the class hierarchy. This framework is designed to be generic for future use cases. Tagging is the first implementation.

Tagging has been reimplemented to leverage the aspect framework and simplify the original tag design. Tag Manager still exists, but is no longer intended for use by L2 construct authors. There are two new classes `cdk.Tag` and `cdk.RemoveTag`. As new resources are added tag support will be automatic as long as they implement one of the existing tag specifications. All L2 constructs have removed the TagManager.

Code generation has been modified to add tag support to any CloudFormation Resource with a matching specification, by creating a Tag Manager for that resource as a `tags` attribute. The schema code now includes the ability to detect 3 forms of tags which include the current CloudFormation Resources.

BREAKING CHANGE: if you are using TagManager the API for this object has completely changed. You should no longer use TagManager directly, but instead replace this with Tag Aspects. `cdk.Tag` has been renamed to `cdk.CfnTag` to enable `cdk.Tag` to be the Tag Aspect.

Fixes #1136
Fixes #1497 
Related #360
@manrueda
Copy link
Author

manrueda commented Feb 7, 2019

It should cover my case but it's not happening. I am updating all my stack to use the tag propagation, the tags are correctly propagated to all resources but this Lambda created by BucketNotifications.

Here is a chunk of my cdk diff after my code update where i remove all the manual tagging.

[~] AWS::Lambda::Function BucketNotificationsHandler050a0587b7544547bf325f094a3db834 BucketNotificationsHandler050a0587b7544547bf325f094a3db8347ECC3691
 └─ [-] Tags
     └─ [{"Key":"XXXX","Value":"XXXX"}...]
[~] AWS::Lambda::Function XXXXXX XXXXXAF403C3B

@moofish32
Copy link
Contributor

@manrueda let me verify a reproduction and add a test case here.

@moofish32
Copy link
Contributor

moofish32 commented Feb 9, 2019

@eladb -- we need to reopen this; it's a bug. I didn't see this line:

https://github.com/awslabs/aws-cdk/blob/master/packages/%40aws-cdk/aws-s3/lib/notifications-resource/notifications-resource-handler.ts#L18

Since this is not actually a Lambda resource it doesn't get the tags attribute. I'll put in a quick fix, but not sure if we want to take a hard look at this cycle issue?

moofish32 added a commit to moofish32/aws-cdk that referenced this issue Feb 10, 2019
Add tag support to the inline Lambda by extending `cdk.Resource` and
adding tags. Implemented `renderProperties` to properly format tags.

Fixes aws#1497
@eladb eladb reopened this Feb 10, 2019
eladb pushed a commit that referenced this issue Feb 11, 2019
Add tag support to the inline Lambda by extending `cdk.Resource` and
adding tags. Implemented `renderProperties` to properly format tags.

Fixes #1497
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-s3 Related to Amazon S3 feature-request A feature should be added or improved.
Projects
None yet
3 participants