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

Support Bucket Replication #10

Merged
merged 7 commits into from
Oct 9, 2017
Merged

Conversation

goruha
Copy link
Member

@goruha goruha commented Sep 27, 2017

What

  • Added ability to grant sufficient IAM permissions to accept replication from master bucket

Why

  • We want s3 bucket to be the destination in replication process

README.md Outdated
@@ -68,7 +68,7 @@ module "website_with_cname" {
| `logs_standard_transition_days` | `30` | Number of days to persist in the standard storage tier before moving to the glacier tier | No |
| `logs_glacier_transition_days` | `60` | Number of days after which to move the data to the glacier storage tier | No |
| `logs_expiration_days` | `90` | Number of days after which to expunge the objects | No |

| `masters` | `{}` | Source bucket name and AWS IAM ID of owner for replication. In format map { "$id" = "$bucket_name" } | No |
Copy link
Member

Choose a reason for hiding this comment

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

What is the $id?

Copy link
Member

Choose a reason for hiding this comment

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

let's rename what you call $id to $aws_account_id

Copy link
Member

Choose a reason for hiding this comment

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

Should we invert the map? Bucket names are globally unique, but multiple buckets might exist in the same account. To permit multiple masters in the same account, we should invert.

@osterman osterman changed the title Feature allow replicate to bucket Support Bucket Replication Sep 27, 2017
]

resources = [
"${aws_s3_bucket.default.arn}",
Copy link
Member

Choose a reason for hiding this comment

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

In the original version, this was the source bucket arn. Now this is the destination bucket arn.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

It was mistake

main.tf Outdated
statement = ["${flatten(data.aws_iam_policy_document.master.*.statement)}"]
}

data "aws_iam_policy_document" "master" {
Copy link
Member

Choose a reason for hiding this comment

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

I can't see where you associate this policy with the bucket.

Also, heads up: hashicorp/terraform#10543 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

Here it is #10 (diff)
We do not attach policy, we get array of statements and include it to default policy document

README.md Outdated
@@ -68,6 +68,7 @@ module "website_with_cname" {
| `logs_standard_transition_days` | `30` | Number of days to persist in the standard storage tier before moving to the glacier tier | No |
| `logs_glacier_transition_days` | `60` | Number of days after which to move the data to the glacier storage tier | No |
| `logs_expiration_days` | `90` | Number of days after which to expunge the objects | No |
| `master_aws_account_ids | `[]` | List of AWS IAM ID of owner for replication source bucket. | No |
Copy link
Member

Choose a reason for hiding this comment

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

I prefer we use ARNs for consistency. See deployment_arns

@goruha goruha merged commit 234fca4 into master Oct 9, 2017
@goruha goruha deleted the feature-allow-replicate-to-bucket branch October 9, 2017 05:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants