Skip to content
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.

Sign up for GitHub

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

fix(aws-s3-deployment): fix server side encryption parameters #6006

Merged
merged 2 commits into from
Feb 4, 2020
Merged

fix(aws-s3-deployment): fix server side encryption parameters #6006

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fc8897d
fix(aws-s3-deployment): fix server side encryption parameters
mattsains Feb 3, 2020
b097e1c
Merge branch 'master' into master
mattsains Feb 4, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-s3-deployment/lambda/src/index.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ def create_metadata_args(raw_user_metadata, raw_system_metadata):
return []

format_system_metadata_key = lambda k: k.lower()
format_user_metadata_key = lambda k: k.lower() if k.lower().startswith("x-amzn-meta-") else f"x-amzn-meta-{k.lower()}"
format_user_metadata_key = lambda k: k.lower() if k.lower().startswith("x-amzn-meta-") else f"x-amzn-meta-{k.lower()}"

system_metadata = { format_system_metadata_key(k): v for k, v in raw_system_metadata.items() }
user_metadata = { format_user_metadata_key(k): v for k, v in raw_user_metadata.items() }
Expand Down
11 changes: 6 additions & 5 deletions packages/@aws-cdk/aws-s3-deployment/lib/bucket-deployment.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,8 +147,9 @@ export interface BucketDeploymentProps {
readonly serverSideEncryptionAwsKmsKeyId?: string;
/**
* System-defined x-amz-server-side-encryption-customer-algorithm metadata to be set on all objects in the deployment.
* Warning: This is not a useful parameter until this bug is fixed: https://github.com/aws/aws-cdk/issues/6080
* @default - Not set.
* @see https://docs.aws.amazon.com/AmazonS3/latest/dev/UsingMetadata.html#SysMetadata
* @see https://docs.aws.amazon.com/AmazonS3/latest/dev/ServerSideEncryptionCustomerKeys.html#sse-c-how-to-programmatically-intro
*/
readonly serverSideEncryptionCustomerAlgorithm?: string;
}
Expand Down Expand Up @@ -262,11 +263,11 @@ function mapSystemMetadata(metadata: BucketDeploymentProps) {
if (metadata.contentEncoding) { res["content-encoding"] = metadata.contentEncoding; }
if (metadata.contentLanguage) { res["content-language"] = metadata.contentLanguage; }
if (metadata.contentType) { res["content-type"] = metadata.contentType; }
if (metadata.serverSideEncryption) { res["server-side-encryption"] = metadata.serverSideEncryption; }
if (metadata.serverSideEncryption) { res.sse = metadata.serverSideEncryption; }
if (metadata.storageClass) { res["storage-class"] = metadata.storageClass; }
if (metadata.websiteRedirectLocation) { res["website-redirect-location"] = metadata.websiteRedirectLocation; }
if (metadata.serverSideEncryptionAwsKmsKeyId) { res["ssekms-key-id"] = metadata.serverSideEncryptionAwsKmsKeyId; }
if (metadata.serverSideEncryptionCustomerAlgorithm) { res["sse-customer-algorithm"] = metadata.serverSideEncryptionCustomerAlgorithm; }
if (metadata.websiteRedirectLocation) { res["website-redirect"] = metadata.websiteRedirectLocation; }
if (metadata.serverSideEncryptionAwsKmsKeyId) { res["sse-kms-key-id"] = metadata.serverSideEncryptionAwsKmsKeyId; }
if (metadata.serverSideEncryptionCustomerAlgorithm) { res["sse-c-copy-source"] = metadata.serverSideEncryptionCustomerAlgorithm; }

return Object.keys(res).length === 0 ? undefined : res;
}
Expand Down
18 changes: 9 additions & 9 deletions packages/@aws-cdk/aws-s3-deployment/test/integ.bucket-deployment-cloudfront.expected.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,7 @@
"Properties": {
"Code": {
"S3Bucket": {
"Ref": "AssetParameters6416c21be320b522db64c705872c0a54d788e3df57b34a5f0d1e8602d7521430S3Bucket13DFEC6A"
"Ref": "AssetParameters53a43f436014c307dccbd4ca1172459431a2a8dee8a2c318ee65991c2a8d4412S3Bucket1FAE0333"
},
"S3Key": {
"Fn::Join": [
Expand All @@ -261,7 +261,7 @@
"Fn::Split": [
"||",
{
"Ref": "AssetParameters6416c21be320b522db64c705872c0a54d788e3df57b34a5f0d1e8602d7521430S3VersionKeyED938FBC"
"Ref": "AssetParameters53a43f436014c307dccbd4ca1172459431a2a8dee8a2c318ee65991c2a8d4412S3VersionKeyE18B22C3"
}
]
}
Expand All @@ -274,7 +274,7 @@
"Fn::Split": [
"||",
{
"Ref": "AssetParameters6416c21be320b522db64c705872c0a54d788e3df57b34a5f0d1e8602d7521430S3VersionKeyED938FBC"
"Ref": "AssetParameters53a43f436014c307dccbd4ca1172459431a2a8dee8a2c318ee65991c2a8d4412S3VersionKeyE18B22C3"
}
]
}
Expand All @@ -301,17 +301,17 @@
}
},
"Parameters": {
"AssetParameters6416c21be320b522db64c705872c0a54d788e3df57b34a5f0d1e8602d7521430S3Bucket13DFEC6A": {
"AssetParameters53a43f436014c307dccbd4ca1172459431a2a8dee8a2c318ee65991c2a8d4412S3Bucket1FAE0333": {
"Type": "String",
"Description": "S3 bucket for asset \"6416c21be320b522db64c705872c0a54d788e3df57b34a5f0d1e8602d7521430\""
"Description": "S3 bucket for asset \"53a43f436014c307dccbd4ca1172459431a2a8dee8a2c318ee65991c2a8d4412\""
},
"AssetParameters6416c21be320b522db64c705872c0a54d788e3df57b34a5f0d1e8602d7521430S3VersionKeyED938FBC": {
"AssetParameters53a43f436014c307dccbd4ca1172459431a2a8dee8a2c318ee65991c2a8d4412S3VersionKeyE18B22C3": {
"Type": "String",
"Description": "S3 key for asset version \"6416c21be320b522db64c705872c0a54d788e3df57b34a5f0d1e8602d7521430\""
"Description": "S3 key for asset version \"53a43f436014c307dccbd4ca1172459431a2a8dee8a2c318ee65991c2a8d4412\""
},
"AssetParameters6416c21be320b522db64c705872c0a54d788e3df57b34a5f0d1e8602d7521430ArtifactHash55E30580": {
"AssetParameters53a43f436014c307dccbd4ca1172459431a2a8dee8a2c318ee65991c2a8d4412ArtifactHashA605283F": {
"Type": "String",
"Description": "Artifact hash for asset \"6416c21be320b522db64c705872c0a54d788e3df57b34a5f0d1e8602d7521430\""
"Description": "Artifact hash for asset \"53a43f436014c307dccbd4ca1172459431a2a8dee8a2c318ee65991c2a8d4412\""
},
"AssetParametersfc4481abf279255619ff7418faa5d24456fef3432ea0da59c95542578ff0222eS3Bucket9CD8B20A": {
"Type": "String",
Expand Down
18 changes: 9 additions & 9 deletions packages/@aws-cdk/aws-s3-deployment/test/integ.bucket-deployment.expected.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,7 @@
"Properties": {
"Code": {
"S3Bucket": {
"Ref": "AssetParameters6416c21be320b522db64c705872c0a54d788e3df57b34a5f0d1e8602d7521430S3Bucket13DFEC6A"
"Ref": "AssetParameters53a43f436014c307dccbd4ca1172459431a2a8dee8a2c318ee65991c2a8d4412S3Bucket1FAE0333"
},
"S3Key": {
"Fn::Join": [
Expand All @@ -271,7 +271,7 @@
"Fn::Split": [
"||",
{
"Ref": "AssetParameters6416c21be320b522db64c705872c0a54d788e3df57b34a5f0d1e8602d7521430S3VersionKeyED938FBC"
"Ref": "AssetParameters53a43f436014c307dccbd4ca1172459431a2a8dee8a2c318ee65991c2a8d4412S3VersionKeyE18B22C3"
}
]
}
Expand All @@ -284,7 +284,7 @@
"Fn::Split": [
"||",
{
"Ref": "AssetParameters6416c21be320b522db64c705872c0a54d788e3df57b34a5f0d1e8602d7521430S3VersionKeyED938FBC"
"Ref": "AssetParameters53a43f436014c307dccbd4ca1172459431a2a8dee8a2c318ee65991c2a8d4412S3VersionKeyE18B22C3"
}
]
}
Expand Down Expand Up @@ -374,17 +374,17 @@
}
},
"Parameters": {
"AssetParameters6416c21be320b522db64c705872c0a54d788e3df57b34a5f0d1e8602d7521430S3Bucket13DFEC6A": {
"AssetParameters53a43f436014c307dccbd4ca1172459431a2a8dee8a2c318ee65991c2a8d4412S3Bucket1FAE0333": {
"Type": "String",
"Description": "S3 bucket for asset \"6416c21be320b522db64c705872c0a54d788e3df57b34a5f0d1e8602d7521430\""
"Description": "S3 bucket for asset \"53a43f436014c307dccbd4ca1172459431a2a8dee8a2c318ee65991c2a8d4412\""
},
"AssetParameters6416c21be320b522db64c705872c0a54d788e3df57b34a5f0d1e8602d7521430S3VersionKeyED938FBC": {
"AssetParameters53a43f436014c307dccbd4ca1172459431a2a8dee8a2c318ee65991c2a8d4412S3VersionKeyE18B22C3": {
"Type": "String",
"Description": "S3 key for asset version \"6416c21be320b522db64c705872c0a54d788e3df57b34a5f0d1e8602d7521430\""
"Description": "S3 key for asset version \"53a43f436014c307dccbd4ca1172459431a2a8dee8a2c318ee65991c2a8d4412\""
},
"AssetParameters6416c21be320b522db64c705872c0a54d788e3df57b34a5f0d1e8602d7521430ArtifactHash55E30580": {
"AssetParameters53a43f436014c307dccbd4ca1172459431a2a8dee8a2c318ee65991c2a8d4412ArtifactHashA605283F": {
"Type": "String",
"Description": "Artifact hash for asset \"6416c21be320b522db64c705872c0a54d788e3df57b34a5f0d1e8602d7521430\""
"Description": "Artifact hash for asset \"53a43f436014c307dccbd4ca1172459431a2a8dee8a2c318ee65991c2a8d4412\""
},
"AssetParametersfc4481abf279255619ff7418faa5d24456fef3432ea0da59c95542578ff0222eS3Bucket9CD8B20A": {
"Type": "String",
Expand Down
41 changes: 36 additions & 5 deletions packages/@aws-cdk/aws-s3-deployment/test/test.bucket-deployment.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,32 +235,63 @@ export = {
sources: [s3deploy.Source.asset(path.join(__dirname, 'my-website.zip'))],
destinationBucket: bucket,
metadata: { "A": "1", "b": "2" },
});

// THEN
expect(stack).to(haveResource('Custom::CDKBucketDeployment', {
UserMetadata: { 'x-amzn-meta-a': '1', 'x-amzn-meta-b': '2' }
}));

test.done();
},

'system metadata is correctly transformed'(test: Test) {
// GIVEN
const stack = new cdk.Stack();
const bucket = new s3.Bucket(stack, 'Dest');

// WHEN
new s3deploy.BucketDeployment(stack, 'Deploy', {
sources: [s3deploy.Source.asset(path.join(__dirname, 'my-website.zip'))],
destinationBucket: bucket,
contentType: "text/html",
contentLanguage: "en",
storageClass: s3deploy.StorageClass.INTELLIGENT_TIERING,
contentDisposition: "inline",
serverSideEncryption: s3deploy.ServerSideEncryption.AES_256,
serverSideEncryption: s3deploy.ServerSideEncryption.AWS_KMS,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This kind of removes our test for the AES_256 use-case, which we don't want to give up (correct me if i'm missing something here).
Lets just add another test for the AWS_KMS case. Which I see is completely missing from this file.

In general, we always prefer adding tests, rather than extending or changing existing ones. Even if it means duplicating most of the test code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your philosophy might vary, but this is just checking that the code correctly translates the property keys. The actual values of these properties doesn't have any factor on the test. I just changed this to AWS_KMS so I could add an additional property to the assertion. I could have left it as AES_256 and although it doesn't really make sense to specify a KMS key ID along with AES_256, the test would still pass.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another thing to consider is what use case this test is testing: "object metadata can be given" - I haven't eroded this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your right, I didn't notice which exact test this was in. This test simply validates the given metadata: { "A": "1", "b": "2" } is translated into { 'x-amzn-meta-a': '1', 'x-amzn-meta-b': '2' }.

This means your added properties don't belong there (in fact none of the additional properties belong there).

What I would expect to see is an another test, perhaps call it 'props translate to system metadata', that validates the properties are translated to the expected keys. This test can than porbably be modified every time we add a property. Its important also not to mix test-cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry, I don't understand this piece of feedback. It seems like you're asserting that this case is testing the metadata -> UserMetadata mapping. However, there are far more fields in this test for everything else -> SystemMetadata in this test.

Anyway, I'll copy this test case to another test case and remove most of the fields from the original test.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I just realized the props translate to system metadata test im suggesting is in fact just a subset of the object metadata can be given test. Its just confusing because metadata is the name of property as well.

I'm still not liking the removal of that property value because in affect it does erode a test. Imagine for some reason we change this enum value, some test should fail. Currently, it looks like its this one.

Copy link
Contributor

@iliapolo iliapolo Feb 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, now the confusion if out of the way, hopefully.

What i would do is:

  • Leave the current test as is.
  • Add another test that simply validates translation of the missing keys. Why another test? because I do want to use the AWS_KMS value in the new one, but also leave the test for AES_256.

Or whatever combination that gets us to a point we are validating both enum values.

Does that make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a systemic problem with these tests. The following classes have values which are not tested:

  • CacheControl
  • StorageClass

For the purposes of getting this PR approved, I will add tests for the ServerSideEncryption enum

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated this PR with that test.

serverSideEncryptionAwsKmsKeyId: "mykey",
serverSideEncryptionCustomerAlgorithm: "rot13",
websiteRedirectLocation: "example",
cacheControl: [s3deploy.CacheControl.setPublic(), s3deploy.CacheControl.maxAge(cdk.Duration.hours(1))],
expires: s3deploy.Expires.after(cdk.Duration.hours(12))
expires: s3deploy.Expires.after(cdk.Duration.hours(12)),
});

// THEN
expect(stack).to(haveResource('Custom::CDKBucketDeployment', {
UserMetadata: { 'x-amzn-meta-a': '1', 'x-amzn-meta-b': '2' },
SystemMetadata: {
'content-type': 'text/html',
'content-language': 'en',
'content-disposition': 'inline',
'storage-class': 'INTELLIGENT_TIERING',
'server-side-encryption': 'AES256',
'sse': 'aws:kms',
'sse-kms-key-id': 'mykey',
'cache-control': 'public, max-age=3600',
'expires': s3deploy.Expires.after(cdk.Duration.hours(12)).value
'expires': s3deploy.Expires.after(cdk.Duration.hours(12)).value,
'sse-c-copy-source': 'rot13',
'website-redirect': 'example'
}
}));

test.done();
},

'server side encryption type has correct values'(test: Test) {
test.equal(s3deploy.ServerSideEncryption.AES_256, 'AES256');
test.equal(s3deploy.ServerSideEncryption.AWS_KMS, 'aws:kms');

test.done();
},

'distribution can be used to provide a CloudFront distribution for invalidation'(test: Test) {
// GIVEN
const stack = new cdk.Stack();
Expand Down