fix(aws-cloudtrail): correct created log policy when sendToCloudWatchLogs is true #1966
Conversation
…Logs is set to true add the correct resource (log group) to the the log role policy instead of adding the log role as a resource make the created cloud trail depend on the logs policy when the prop sendToCloudWatchLogs is set to true fixes #1963
| @@ -150,7 +150,7 @@ export class CloudTrail extends cdk.Construct { | |||
|
|
|||
| logsRole = new iam.Role(this, 'LogsRole', { assumedBy: new iam.ServicePrincipal(cloudTrailPrincipal) }); | |||
|
|
|||
| const streamArn = `${logsRole.roleArn}:log-stream:*`; | |||
| const streamArn = `${logGroup.logGroupArn}:*`; | |||
There was a problem hiding this comment.
Why did you drop log-stream?
There was a problem hiding this comment.
Can you confirm that the :* suffix is actually necessary? I believe the logGroupArn will already end in :*.
There was a problem hiding this comment.
log-stream was dropped as the cloud trail fails to deploy with it. The reason been that the logs:CreateLogStream action should be on the log group resource and not on the (as yet to be created) log stream. I think the logs:PutLogEvents action could be scoped to just the log stream if we want to be stricter about the permissions.
I tried without :* and it works fine. I'll make the change.
|
|
||
| if (logsRole !== undefined) { | ||
| const logsRolePolicy = logsRole.node.findChild("DefaultPolicy").node.findChild("Resource"); | ||
| trail.node.addDependency(logsRolePolicy); |
There was a problem hiding this comment.
Can you add a comment explaining why we need this?
Also, I think dependencies have been lifted to the Construct level so you can reduce the amount of node traversal:
const logsRolePolicy = logsRole.node.findChild("DefaultPolicy");
trail.addDependency(logsRolePolicy);There was a problem hiding this comment.
Don't even need to do that, you can do:
trail.node.addDependency(logsRolePolicy);There was a problem hiding this comment.
I assume you mean
trail.node.addDependency(logsRole);I tried this and it works. It adds both role and the policy as dependencies.
There was a problem hiding this comment.
I'll add this as a comment:
If props.sendToCloudWatchLogs is set to true then the trail needs to depend on the created logsRole so that it can create the log stream for the log group. This ensures the logsRole is created and propagated before the trail tries to create the log stream.
Without the dependency the cloud trail fails to deploy with the same error as the referenced issue.
RobinsonAndrew
deleted the
RobinsonAndrew/fix-cloudtrail-send-logs-to-cloudwatch
branch
fix(aws-cloudtrail): correct created log policy when sendToCloudWatchLogs is true
add the correct resource (log group) to the the log role policy instead of adding the log role as a resource
make the created cloud trail depend on the logs policy when the prop sendToCloudWatchLogs is set to true
Fixes #1963
Pull Request Checklist
CLI change?: coordinate update of integration tests with teamcdk-init template change?: coordinated update of integration tests with teamjsdocs: All public APIs documentedREADME: README and/or documentation topic updatedBreaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"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)By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.