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.

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(ecr): add server-side encryption configuration #16966

Merged
merged 7 commits into from
Jan 31, 2022
Merged

feat(ecr): add server-side encryption configuration #16966

merged 7 commits into from
Jan 31, 2022

Conversation

Wurstnase
Copy link
Contributor

@Wurstnase Wurstnase commented Oct 13, 2021

fixes #15400

With this request you will be able to configure the encryption of your ECR Repository.

Before this patch you need to use a L1-Construct and add it via:

Python:

repo = ecr.Repository(stack, 'Repo')
cfn_repo = repo.node.default_child
cfn_repo.encryption_configuration = CfnRepository.EncryptionConfigurationProperty(encryption_type="KMS")

Now this becomes:

repo = ecr.Repository(stack, 'Repo', encryption_type=ecr.RepositoryEncryption.KMS)

When using a KMS Key, the encryption_type is set automatically to KMS.

kms_key = kms.Key(stack, 'Key')
ecr.Repository(stack, 'Repo', encryption_key=kms_key)

Similar to #15571


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@gitpod-io
Copy link

gitpod-io bot commented Oct 13, 2021

@github-actions github-actions bot added the @aws-cdk/aws-ecr Related to Amazon Elastic Container Registry label Oct 21, 2021
@peterwoodworth peterwoodworth changed the title feat(ecr): add server-side encryption configuration feat(ecr): add server-side encryption configuration Oct 21, 2021
@Wurstnase
Copy link
Contributor Author

@madeline-k Do you have some time to review my PR?

@bafonso
Copy link

bafonso commented Dec 15, 2021

This would be a very useful addition indeed!

@Wurstnase
Copy link
Contributor Author

The rosetta test is pretty nice.

Looks like I fixed all issues for now. A review would be very nice.

Copy link
Contributor

@madeline-k madeline-k left a comment

Choose a reason for hiding this comment

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

Thank you for opening this PR @Wurstnase! It looks great, just needs a few updates.

packages/@aws-cdk/aws-ecr/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-ecr/README.md Show resolved Hide resolved
packages/@aws-cdk/aws-ecr/README.md Show resolved Hide resolved
packages/@aws-cdk/aws-ecr/lib/repository.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-ecr/lib/repository.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-ecr/lib/repository.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-ecr/lib/repository.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-ecr/lib/repository.ts Outdated Show resolved Hide resolved
@mergify mergify bot dismissed madeline-k’s stale review January 11, 2022 07:51

Pull request has been modified.

@Wurstnase
Copy link
Contributor Author

@madeline-k Thanks for your review.

I take your suggestions. I'm currently fighting with my dev container.
I will ping you when I finish all your review comments.

@Wurstnase
Copy link
Contributor Author

Hi @madeline-k , I just fixed all your comments. Please take a look.

Copy link
Contributor

@madeline-k madeline-k left a comment

Choose a reason for hiding this comment

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

Just one last comment on the README. Otherwise, this looks ready to me. 😄

packages/@aws-cdk/aws-ecr/README.md Outdated Show resolved Hide resolved
@mergify mergify bot dismissed madeline-k’s stale review January 13, 2022 07:39

Pull request has been modified.

@Wurstnase
Copy link
Contributor Author

@madeline-k This PR should be ready 😀

Copy link
Contributor

@madeline-k madeline-k left a comment

Choose a reason for hiding this comment

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

This looks great, thank you so much @Wurstnase!

Unfortunately, you will probably encounter some merge conflicts when you merge the latest from master now, and will definitely need to update the unit tests. This week, we migrated this module to use the new Assertions unit test framework. The work to update should be minimal and pretty obvious after you merge from master. Let me know if you have questions!

Wurstnase and others added 6 commits January 21, 2022 20:19
Co-authored-by: Madeline Kusters <80541297+madeline-k@users.noreply.github.com>
Co-authored-by: Madeline Kusters <80541297+madeline-k@users.noreply.github.com>
@Wurstnase
Copy link
Contributor Author

As you've said. It wasn't that hard 👍

Thanks alot for your review, @madeline-k.

Copy link
Contributor

@madeline-k madeline-k left a comment

Choose a reason for hiding this comment

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

Approving, @Wurstnase!

@mergify
Copy link
Contributor

mergify bot commented Jan 31, 2022

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-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: d879cea
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify mergify bot merged commit c46acd5 into aws:master Jan 31, 2022
@mergify
Copy link
Contributor

mergify bot commented Jan 31, 2022

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).

kornicameister added a commit to kornicameister/aws-cdk that referenced this pull request Feb 3, 2022
* origin/master: (74 commits)
  chore: remove reliance on `decdk` in build system (aws#18760)
  chore: add repository directory for all packages.json  (aws#17203)
  docs(ecs): correct comment documentation for NetworkMode (aws#17841)
  feat(ecs): expose image name in container definition (aws#17793)
  feat(ecr): add server-side encryption configuration  (aws#16966)
  chore(region-info): ap-southeast-3 (Jakarta) ROUTE_53_BUCKET_WEBSITE_ZONE_ID (aws#18110)
  chore: reassign njlynch's ownership areas (aws#18751)
  chore(ecs-service-extensions): migrate tests to assertions (aws#18649)
  chore(s3): Fixed documentation for `InventoryFormat.ORC` (aws#18717)
  feat(iot): add Action to republish MQTT messages to another MQTT topic (aws#18661)
  chore(rds): add support for PostgreSQL 14 (aws#18713)
  fix(core): correctly reference versionless secure parameters (aws#18730)
  fix(ec2): `UserData.addSignalOnExitCommand` does not work in combination with `userDataCausesReplacement` (aws#18726)
  fix(vpc): Vpc.fromLookup should throw if subnet group name tag is explicitly given and does not exist (aws#18714)
  docs(dynamodb): add note around table encryption (aws#18721)
  chore: override `markdown-it` version (aws#18723)
  docs(cfnspec): update CloudFormation documentation (aws#18741)
  chore(release): 1.142.0
  chore(lambda-layer-awscli): contains a CLI version with a CVE (aws#18727)
  chore(lambda-python): remove Pillow dependency (aws#18722)
  ...
@udondan udondan mentioned this pull request Feb 4, 2022
2 tasks
TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this pull request Feb 21, 2022
fixes aws#15400

With this request you will be able to configure the encryption of your ECR Repository.

Before this patch you need to use a L1-Construct and add it via:

Python:
```python
repo = ecr.Repository(stack, 'Repo')
cfn_repo = repo.node.default_child
cfn_repo.encryption_configuration = CfnRepository.EncryptionConfigurationProperty(encryption_type="KMS")
```
Now this becomes:
```python
repo = ecr.Repository(stack, 'Repo', encryption_type=ecr.RepositoryEncryption.KMS)
```

When using a KMS Key, the `encryption_type` is set automatically to `KMS`.
```python
kms_key = kms.Key(stack, 'Key')
ecr.Repository(stack, 'Repo', encryption_key=kms_key)
```

Similar to aws#15571

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-ecr Related to Amazon Elastic Container Registry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(aws-ecr): Repository construct is missing EncryptionConfiguration
4 participants