-
Notifications
You must be signed in to change notification settings - Fork 330
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
feat: Add option to use CMK and set the resources retention #573
Conversation
@@ -17,15 +15,13 @@ export class SageMakerModel extends Construct { | |||
const { model } = props; | |||
this.modelId = model.modelId; | |||
|
|||
if (model.type == DeploymentType.Container) { | |||
const { endpoint } = deployContainerModel(this, props, model); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed because it was never used.
removalPolicy: cdk.RemovalPolicy.DESTROY, | ||
storageEncryptionKey: props.shared.kmsKey, | ||
// Always setting it to true would be a breaking change. | ||
storageEncrypted: props.shared.kmsKey ? true : false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My preference would be to always enable it but it would trigger the recreation of the cluster
encryption: props.kmsKey | ||
? dynamodb.TableEncryption.CUSTOMER_MANAGED | ||
: dynamodb.TableEncryption.AWS_MANAGED, | ||
encryptionKey: props.kmsKey, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if props.kmsKey does not exist? Same comment for other constructs like ChatBotS3Buckets, RealtimeResolvers, ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It passes undefined. (property is ignored)
enforceSSL: true, | ||
encryption: s3.BucketEncryption.S3_MANAGED, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does logs bucket encrypted with S3 managed key and files bucket has options?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not supported.
https://docs.aws.amazon.com/AmazonS3/latest/userguide/enable-server-access-logging.html
"You can use default bucket encryption on the destination bucket only if you use server-side encryption with Amazon S3 managed keys (SSE-S3), which uses the 256-bit Advanced Encryption Standard (AES-256). Default server-side encryption with AWS Key Management Service (AWS KMS) keys (SSE-KMS) is not supported."
Issue #, if available:
Description of changes:
Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.