docs(dynamodb): the documentation is wrong around table encryption#20938
docs(dynamodb): the documentation is wrong around table encryption#20938mergify[bot] merged 10 commits intoaws:mainfrom
Conversation
|
|
madeline-k
left a comment
There was a problem hiding this comment.
Did you do any test to confirm that server-side encryption is not enabled when the SSESpecification property is omitted from the Table properties cloudformation template? I deployed a stack with only the required props defined, and found that there is actually still some server-side encryption enabled on the resulting table:
new dynamodb.Table(this, 'myTable', {
partitionKey: {
name: 'myKey',
type: ddb.AttributeType.STRING,
}
})
The @default tag in the documentation should describe what happens when you deploy a Table omitting those properties.
| * | ||
| * @default - If `encryption` is set to `TableEncryption.CUSTOMER_MANAGED` and this | ||
| * property is undefined, a new KMS key will be created and associated with this table. | ||
| * property is undefined, the table will not be encrypted. |
There was a problem hiding this comment.
One of these things is not like the other.
- In the other properties, the
@defaultdescribes what DynamoDB does by default. - In this property, the
@defaultdescribes what our code does by default.
It could be that we're wrong about the first, but I'm pretty sure we're not wrong about the second. Or if we are, then that's a bug we should fix.
There was a problem hiding this comment.
Good point. @Tianyi-W ,
You need to test with setting this as well:
encryption: dynamodb.TableEncryption.CUSTOMER_MANAGED
And see what happens. This @default tag should describe the default behavior for the encryptionKey property both when encryption is undefined and when encryption is CUSTOMER_MANAGED.
Add correct doc to "serverSideEncryption?" and "encryption?", revert the doc for "encryptionKey?" to the existing one which is correct.
peterwoodworth
left a comment
There was a problem hiding this comment.
@Tianyi-W the build is failing because there are some trailing spaces in the documentation you've added (lines 227 and 191). If you remove these, then the build should pass!
madeline-k
left a comment
There was a problem hiding this comment.
Looks great! I just have a couple minor comments, and then this should be good to go.
Co-authored-by: Madeline Kusters <80541297+madeline-k@users.noreply.github.com>
Co-authored-by: Madeline Kusters <80541297+madeline-k@users.noreply.github.com>
madeline-k
left a comment
There was a problem hiding this comment.
The content of the change looks good, approving 👍
The PR build is failing, which I suspect is because of trailing space. Checkout the build logs in the link from the comment to see why your build is failing
Co-authored-by: Madeline Kusters <80541297+madeline-k@users.noreply.github.com>
commit suggested changes Co-authored-by: Madeline Kusters <80541297+madeline-k@users.noreply.github.com>
|
Thank you for contributing! Your pull request will be updated from main 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 main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
This pull request aims to resolve #16436. As mentioned in the issue, by default, no encryption is done, which is different from the documentation description. Now the documentation descriptions are updated around the table encryption as the default is for the Table to not be encrypted.
Closes #16436
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license