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

Jump to bottom

[needs love] feat(@aws-cdk/s3): add support for bucket replication. #184

Closed
wants to merge 3 commits into from
Closed

[needs love] feat(@aws-cdk/s3): add support for bucket replication. #184

wants to merge 3 commits into from

Conversation

rix0rrr
Copy link
Contributor

@rix0rrr rix0rrr commented Jun 28, 2018

You can now call source.enableBucketReplication(dest) to replicate
one bucket to a different one.

Note that the two buckets must live in different regions.

By submitting this pull request, I confirm that my contribution is made under
the terms of the beta license.

feat(@aws-cdk/s3): add support for bucket replication.
686c874 
You can now call `source.enableBucketReplication(dest)` to replicate
one bucket to a different one.

Note that the two buckets must live in different regions.
eladb
eladb approved these changes Jun 28, 2018
View reviewed changes
packages/@aws-cdk/s3/lib/bucket.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/s3/lib/bucket.ts
* Can specify details about prefixes to replicate by giving rules. If no rules are given,
* all objects are replicated with default settings.
*
* Note that the indicated bucket MUST reside in a different region! Bucket replication
Copy link
Contributor

Choose a reason for hiding this comment

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

Add jsdocs for params

packages/@aws-cdk/s3/lib/bucket.ts
/**
* Establish bidirectional grant between identity and KMS key for the given actions.
*
* Normally we can do it one way, but for KMS keys we must add the grants on
Copy link
Contributor

Choose a reason for hiding this comment

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

😛

@rix0rrr
Copy link
Contributor Author

rix0rrr commented Jun 29, 2018 •
edited
Loading

This PR is still broken in the face of encryption.

The Console lets me pick a Source key for decryption, but the model doesn't have that field.

The Console lets me pick a Destination key, but in CDK it has to be an alias, which I can put but doesn't show in the console?

And then there are permissions which are tricky to get right. Policy permissions must be cross-stack, given on the key. However, keys ALWAYS have a generated ID, so we have no solution for this. Aliases have fixed names, but we cannot give permissions on aliases.

@rix0rrr
Copy link
Contributor Author

rix0rrr commented Jun 29, 2018

A policy like the following on the key will work and I think doesn't compromise too much:

    {
      "Action": [
        "kms:Encrypt"
      ],
      "Effect": "Allow",
      "Principal": "*",     <--- might also be account root of same account to be safer
      "Condition": {
        "StringLike": {
          "kms:ViaService": "s3.us-east-1.amazonaws.com",
          "kms:EncryptionContext:aws:s3:arn": [
            "arn:aws:s3:::cdk-central-events-bucket/*"
          ]
        }
      },
      "Resource": "*"
    }

@rix0rrr
Copy link
Contributor Author

rix0rrr commented Jun 29, 2018

Parking this for later, needs some work but it's not high prio.

Also export/import encryption key as part of Bucket export
336528f 
Incorporate progressing insight on how bucket replication works in combination
with encryption.

- KMS-encrypted objects require a KMS key during the replication operation.
- The IAM role must have permissions to use this KMS key (which must be
  set bidirectionally).

Still work left to do, parking for now.
@rix0rrr rix0rrr changed the title feat(@aws-cdk/s3): add support for bucket replication. [needs love] feat(@aws-cdk/s3): add support for bucket replication. Jul 2, 2018
@rix0rrr rix0rrr added the parked label Aug 20, 2018
@rix0rrr
Copy link
Contributor Author

rix0rrr commented Aug 21, 2018

This PR will be a great motivating example for cross-stack references.

@eladb
Copy link
Contributor

eladb commented Aug 28, 2018

What's the follow up on this?

@rix0rrr
Copy link
Contributor Author

rix0rrr commented Aug 28, 2018

Cross stack references first, then this. Still percolating in the back of my head, will start working on it as soon as I have time.

@moofish32
Copy link
Contributor

@rix0rrr -- bucket replication is cross region, cross stack references are in region? Is there something to make all that work here?

@rix0rrr
Copy link
Contributor Author

rix0rrr commented Aug 28, 2018

Not yet, but there will be 😊

@eladb
Copy link
Contributor

eladb commented Feb 4, 2019

@rix0rrr what should we do with this?

@eladb eladb mentioned this pull request Feb 5, 2019
Closed
@eladb
Copy link
Contributor

eladb commented Feb 5, 2019

Opened #1680

@eladb eladb closed this Feb 5, 2019
@RomainMuller RomainMuller deleted the huijbers/s3-bucket-replication branch August 10, 2019 00:38
@NGL321 NGL321 added the contribution/core This is a PR that came from AWS. label Sep 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eladb eladb eladb approved these changes

Assignees
No one assigned
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@rix0rrr @eladb @moofish32 @srchase @NGL321