feat(logs): custom Role for Kinesis destination#13553
feat(logs): custom Role for Kinesis destination#13553mergify[bot] merged 19 commits intoaws:masterfrom gmeligio:master
Conversation
|
|
|
I was able to unit test |
rix0rrr
left a comment
There was a problem hiding this comment.
It looks like what you're trying to do is preventing the Kinesis destination from creating a Role for you. I'm assuming because your organization does not allow the creation of new Roles by developers.
Right now it's the case that the ILogSubscriptionDestination is responsible for creating the Roles. If you want it to stop doing that, the easiest way is to make a pre-existing Role configurable on all the individual subclasses of ILogSubscriptionDestination, starting with the Kinesis one. Create a new (optional) Props object with a single role?: Role field, and use that one if given instead of creating a new one. This is the pattern used all across the CDK.
The next question might be--does this make sense? Should the Role not be configured by the subscription filter itself instead of its destination?
A case could definitely be made for that. That requires a bigger refactor, where we add the role?: Role argument to SubscriptionFilterOptions, and we need to add some protocol between the SubscriptionFilter and the Destination to indicate whether or not the filter already HAS a role or needs one configured (and even if one already exists, permissions may still need to be added to them!).
You could also take on that, I'm not opposed to it.
In neither case is a change to LogGroup necessary, or does LogGroup implicitly get a Role or something. You are using the LogGroup object to transport a single variable somewhere else -- that doesn't seem necessargy.
packages/@aws-cdk/aws-logs/README.md
Outdated
| logGroup, | ||
| destination: new LogsDestinations.LambdaDestination(fn), | ||
| filterPattern: FilterPattern.allTerms("ERROR", "MainThread") | ||
| roleArn: role.roleArn |
There was a problem hiding this comment.
It's not roleArn though, right?
| * @default No role assumed | ||
| * @attribute | ||
| */ | ||
| readonly role?: iam.IRole; |
There was a problem hiding this comment.
No need to expose this here.
924c117 to
ebfd5f2
Compare
…ination subclasses
|
|
|
I was trying to solve this issue, but I don't have this problem in particular in my organization. Thank you for the guidance @rix0rrr . I studied other packages in the CDK to understand your points. I decided to go with option one and customize destination subclasses as the pattern used all across the CDK. It seems like it's suitable for the issue use case. Still, maybe I can implement the second option if it's necessary with the role creation protocol between When adding the role property to destination subclasses, I couldn't find a way to do it for |
Pull request has been modified.
Pull request has been modified.
Pull request has been modified.
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
* [x] Testing - Unit test added. * [x] Docs - __jsdocs__: All public APIs documented - __README__: README and/or documentation topic updated When creating a logs destination, an optional `role` can be passed to be assumed when writing logs in the destination. Closes aws#7661. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
When creating a logs destination, an optional
rolecan be passed to be assumed when writing logs in the destination.Closes #7661.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license