feat(logs): Add KMS key support to LogGroup #11363
Conversation
|
|
| "KmsKeyId": { | ||
| "Documentation": "https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-logs-loggroup.html#cfn-logs-loggroup-kmskeyid", | ||
| "PrimitiveType": "String", | ||
| "Required": false, | ||
| "UpdateType": "Mutable" | ||
| }, |
There was a problem hiding this comment.
spec changes are automated and not manually made -
the spec import is being done in #11319 ... let's wait for that to get merged and keep this change isolated to making the capability available
There was a problem hiding this comment.
Sounds good. Thanks 👍
There was a problem hiding this comment.
@nicklaw5 it's been merged so updating your PR should allow us to simply consume the spec 🙂
| * | ||
| * @default No KMS key | ||
| */ | ||
| readonly kmsKey?: IKey; |
There was a problem hiding this comment.
we commonly call this encryptionKey unless there's a need for multiple keys in which case we tend to prefix it with the qualifier that the key applies to.
| readonly kmsKey?: IKey; | |
| readonly encryptionKey?: IKey; |
| /** | ||
| * The KMS Key to encrypt the log group with. | ||
| * | ||
| * @default No KMS key |
There was a problem hiding this comment.
does this use the default master key if unspecified?
the default text should describe the behaviour - feel free to adjust if i'm reading it incorrectly.
| * @default No KMS key | |
| * @default - log group is encrypted with the default master key |
packages/@aws-cdk/aws-logs/README.md
Outdated
| const kmsKey = new kms.Key(this, 'Key'); | ||
|
|
||
| new LogGroup(this, 'LogGroup', { | ||
| kmsKey, | ||
| }); |
There was a problem hiding this comment.
| const kmsKey = new kms.Key(this, 'Key'); | |
| new LogGroup(this, 'LogGroup', { | |
| kmsKey, | |
| }); | |
| import * as kms from '@aws-cdk/aws-kms'; | |
| new LogGroup(this, 'LogGroup', { | |
| kmsKey: new kms.Key(this, 'Key'), | |
| }); |
nicklaw5
force-pushed
the
feature-logs-add-kmskey
branch
from
d4e086c
to
791ed3f
Compare
mergify
bot
dismissed
shivlaks’s stale review
Pull request has been modified.
nicklaw5
force-pushed
the
feature-logs-add-kmskey
branch
from
791ed3f
to
0a95722
Compare
|
@shivlaks I've updated as per your review. |
| @@ -363,6 +371,7 @@ export class LogGroup extends LogGroupBase { | |||
| } | |||
|
|
|||
| const resource = new CfnLogGroup(this, 'Resource', { | |||
| kmsKeyId: props.encryptionKey?.keyId, | |||
There was a problem hiding this comment.
should this be using the ARN?
The docs indicate that it can. It would be more robust to use an ARN because the key might be in another account (some services allow it, I'm not sure about whether that's valid usage in this case)
There was a problem hiding this comment.
I agree. Let me make that adjustment also.
mergify
bot
dismissed
shivlaks’s stale review
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). |
This PR updates the
LogGroupconstruct to support the ability to encrypt log groups on creation.Closes #11211
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license