S3 Replication Improvements #93

alexjurkiewicz · 2021-06-24T10:37:44Z

Terraform plan impact

In order to support multiple S3 Bucket replication destinations, we must use the filter in the replication rule, even if there is nothing to filter. The filter, even if empty, conflicts with the prefix attribute of the rule (a v1 feature replaced in v2 with the filter). So we moved all prefix settings into the filter. Therefore, you may see Terraform make a change like this:

Click to show plan

- rules {
  - id       = "replication-test"
  - prefix   = "/main"
  - priority = 0
  - status   = "Enabled"

  - destination {
    - bucket        = "arn:aws:s3:::replication-target"
    - storage_class = "STANDARD"
  }

  - filter {}
- }

+ rules {
    + id       = "replication-test"
    + priority = 0
    + status   = "Enabled"

    + destination {
        + bucket        = "arn:aws:s3:::replication-target"
        + storage_class = "STANDARD"
      }

    + filter {
        + prefix = "/main"
        + tags   = {}
    + }
+ }

This change is harmless and can be applied without impact.

Deprecation

To provide consistency in naming, the replication_rules input has been deprecated in favor of s3_replication_rules. Existing code will continue to work, but new users should use s3_replication_rules and current users of replication_rules should update their code to use s3_replication_rules at their convenience.

what

@alexjurkiewicz

Add support for multi-bucket S3 replication
Add support for easily adding cross-account replication destination bucket policy statements

@korenyoni

Add test for S3 bucket replication configuration to provide better code coverage

@Nuru

Convert v1 prefix to v2 filter to support multiple replication destinations
Rename replication_rules to s3_replication_rules for consistency
Move testing to us-east-2 region because that is where Cloud Posse prefers to do testing

why

Module currently does not support multi-bucket S3 replication
Adding cross-account replication destination bucket policy statements is currently a manual task with no site-specific uniqueness

references

Replication was added in Adding support for s3 bucket replication configuration #56
https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/s3_bucket

Allows easy management of buckets intended as cross-account S3 replication destinations.

This removes var.s3_replica_bucket_arn and lets you set it per-replication rule.

No longer required. Replication is implicitly enabled if there are any replication rules.

alexjurkiewicz · 2021-06-24T11:21:13Z

I tested this myself with cross-account replication and confirmed it works:

source bucket:

module "source" {
  source = "github.com/alexjurkiewicz/terraform-aws-s3-bucket?ref=9cc2a02e2d2796110ba894d090b86356459c681b"
  name    = "ajtest-replication-source"
  sse_algorithm           = "AES256"
  replication_rules = [
    {
      id       = "destination"
      priority = 0
      status   = "Enabled"
      filter   = {}
      destination = {
        bucket        = "arn:aws:s3:::ajtest-replication-destination"
        storage_class = "ONEZONE_IA"
      }
    },
  ]
}

destination bucket:

module "destination" {
  source = "github.com/alexjurkiewicz/terraform-aws-s3-bucket?ref=9cc2a02e2d2796110ba894d090b86356459c681b"
  name    = "ajtest-replication-destination"
  sse_algorithm                = "AES256"
  replication_source_roles = [
    # From oneoff stack
    "arn:aws:iam::XXX:role/ajtest-replication-source"
  ]
}

korenyoni · 2021-06-24T11:23:38Z

/test all

korenyoni

LGTM except for the addition of a secondary locals block in main.tf.

I also need to weigh in on @Nuru and @osterman to decide on how to provide the best upgrade path for our users with these two variables removed.

main.tf

korenyoni · 2021-06-24T11:36:38Z

variables.tf

-variable "s3_replication_enabled" {
-  type        = bool
-  default     = false
-  description = "Set this to true and specify `s3_replica_bucket_arn` to enable replication. `versioning_enabled` must also be `true`."
-}
-
-variable "s3_replica_bucket_arn" {
-  type        = string
-  default     = ""
-  description = "The ARN of the S3 replica bucket (destination)"
-}
-


I will need to weigh in on @Nuru and @osterman to decide on whether removing these outright is the best upgrade path for our module's users.

In this particular case I feel like var.s3_replication_enabled could be deprecated instead of outright removed, whereas var.s3_replica_bucket_arn cannot be. But in that case, maybe it doesn't make sense to deprecate and keep one without the other.

Let me know.

As someone who went through the change of lifecycle rules from individual variables to the lifecycle_rules combined structure, my experience was that the transition was significantly harder because the old variables still existed but did nothing. I think it's easier for users to simply break compatibility and describe the migration path in release notes.

main.tf

variables.tf

main.tf

This Pull Request has been updated, so we're dismissing all reviews.

korenyoni · 2021-06-24T13:40:54Z

/test all

korenyoni

After speaking with @Nuru , we are leaning towards deprecating the variables. Even if this PR isn't the best example of variables that are straight forward to deprecate, it's nonetheless an effort we are attempting to do throughout all of our cloudposse modules.

I've requested the changes to make this happen.

After digging into these changes, I also realized that we do not have a proper test suite for replication. I think it's appropriate for this PR to add that. I've taken the time to build it out and test it:

examples/complete/main.tf (this file was changed to support replication, but only when it's enabled):

locals {
  replication_enabled = length(var.replication_rules) > 0
}

provider "aws" {
  region = var.region
}

module "s3_bucket" {
  source = "../../"

  user_enabled                 = true
  acl                          = var.acl
  force_destroy                = var.force_destroy
  grants                       = var.grants
  lifecycle_rules              = var.lifecycle_rules
  versioning_enabled           = var.versioning_enabled
  allow_encrypted_uploads_only = var.allow_encrypted_uploads_only
  allowed_bucket_actions       = var.allowed_bucket_actions
  bucket_name                  = var.bucket_name
  object_lock_configuration    = var.object_lock_configuration
  replication_rules            = local.replication_enabled ? [for rule in var.replication_rules : merge(rule, {destination: {bucket: module.s3_bucket_replication_target[0].bucket_arn}})] : []

  context = module.this.context
}

module "replication_target_label" {
  count   = local.replication_enabled ? 1 : 0

  source  = "cloudposse/label/null"
  version = "0.24.1"

  attributes = ["target"]
  context    = module.this.context
}

module "s3_bucket_replication_target" {
  count   = local.replication_enabled ? 1 : 0

  source = "../../"

  user_enabled                 = true
  acl                          = "private"
  force_destroy                = true
  versioning_enabled           = true
  bucket_name                  = module.replication_target_label[0].id
  replication_source_roles     = [module.s3_bucket.replication_role_arn]

  context = module.replication_target_label[0].context
}

examples/complete/replication.us-west-1.tfvars (new file):

enabled = true

region = "us-west-1"

namespace = "eg"

stage = "test"

name = "s3-replication-test"

acl = "private"

force_destroy = true

versioning_enabled = true

allow_encrypted_uploads_only = true

allowed_bucket_actions = ["s3:PutObject", "s3:PutObjectAcl", "s3:GetObject", "s3:DeleteObject", "s3:ListBucket", "s3:ListBucketMultipartUploads", "s3:GetBucketLocation", "s3:AbortMultipartUpload"]

# This variable is going to be manipulated to get the ID of the replication target bucket
replication_rules = [
  {
    id     = "replication-test"
    status = "Enabled"
    prefix = "/"
  }
]

examples/complete/variables.tf (right before var.restrict_public_buckets):

variable "replication_rules" {
  default     = []
  description = "S3 replication rules"
}

At the bottom of test/src/examples_complete_test.go:

func TestExamplesCompleteWithReplication(t *testing.T) {
	rand.Seed(time.Now().UnixNano())

	attributes := []string{strconv.Itoa(rand.Intn(100000))}
	rootFolder := "../../"
	terraformFolderRelativeToRoot := "examples/complete"
	varFiles := []string{"replication.us-west-1.tfvars"}

	tempTestFolder := test_structure.CopyTerraformFolderToTemp(t, rootFolder, terraformFolderRelativeToRoot)

	terraformOptions := &terraform.Options{
		// The path to where our Terraform code is located
		TerraformDir: tempTestFolder,
		Upgrade:      true,
		// Variables to pass to our Terraform code using -var-file options
		VarFiles: varFiles,
		Vars: map[string]interface{}{
			"attributes": attributes,
		},
	}

	// At the end of the test, run `terraform destroy` to clean up any resources that were created
	defer terraform.Destroy(t, terraformOptions)

	// This will run `terraform init` and `terraform apply` and fail the test if there are any errors
	terraform.InitAndApply(t, terraformOptions)

	// Run `terraform output` to get the value of an output variable
	userName := terraform.Output(t, terraformOptions, "user_name")

	expectedUserName := "eg-test-s3-replication-test-" + attributes[0]
	// Verify we're getting back the outputs we expect
	assert.Equal(t, expectedUserName, userName)

	// Run `terraform output` to get the value of an output variable
	s3BucketId := terraform.Output(t, terraformOptions, "bucket_id")

	expectedS3BucketId := "eg-test-s3-replication-test-" + attributes[0]
	// Verify we're getting back the outputs we expect
	assert.Equal(t, expectedS3BucketId, s3BucketId)
}

korenyoni · 2021-06-25T15:32:11Z

main.tf

@@ -1,5 +1,7 @@
 locals {
-  bucket_name = var.bucket_name != null && var.bucket_name != "" ? var.bucket_name : module.this.id
+  bucket_name         = var.bucket_name != null && var.bucket_name != "" ? var.bucket_name : module.this.id
+  replication_enabled = length(var.replication_rules) > 0


Suggested change

replication_enabled = length(var.replication_rules) > 0

# the following also takes into account the deprecated var.s3_replication_enabled variable (which is null by default)

replication_enabled = length(var.replication_rules) > 0 || var.s3_replication_enabled == true

korenyoni · 2021-06-25T15:32:46Z

main.tf

@@ -154,7 +156,7 @@ resource "aws_s3_bucket" "default" {
          status   = try(rules.value.status, null)

          destination {
-            bucket             = var.s3_replica_bucket_arn
+            bucket             = rules.value.destination.bucket


Suggested change

bucket = rules.value.destination.bucket

# The following takes into account the deprecated var.s3_replica_bucket_arn variable

bucket = try(rules.value.destination.bucket, var.s3_replica_bucket_arn)```

korenyoni · 2021-06-25T15:33:44Z

variables.tf

-variable "s3_replication_enabled" {
-  type        = bool
-  default     = false
-  description = "Set this to true and specify `s3_replica_bucket_arn` to enable replication. `versioning_enabled` must also be `true`."
-}
-
-variable "s3_replica_bucket_arn" {
-  type        = string
-  default     = ""
-  description = "The ARN of the S3 replica bucket (destination)"
-}
-


Please add this to the bottom of variables.tf

# The variables below are DEPRECATED and should not be used anymore variable "s3_replication_enabled" { type = bool default = null description = "DEPRECATED. Use `replication_rules` instead." } variable "s3_replica_bucket_arn" { type = string default = null description = "DEPRECATED. Use `replication_rules` instead." }

Nuru · 2021-06-25T17:47:07Z

@alexjurkiewicz I agree with @korenyoni but given how much additional work we are asking for, I am going to take over and finish this PR building on both of your work.

This Pull Request has been updated, so we're dismissing all reviews.

Nuru · 2021-06-25T20:14:04Z

/test all

korenyoni

Looks good to me. I think the level of precedence set fors3_replica_bucket_arn is logical. I wish the names of var.replication_rules and var.replication_source_roles were more consistent with var.s3_repliction_enabled and var.s3_replica_bucket_arn, but all of these are old variables, so there's nothing we can really do.

korenyoni · 2021-06-27T12:25:07Z

/test all

korenyoni

@Nuru please see my comment on cloudposse/actions, terratest and AWS_SECRET_KEY

korenyoni · 2021-06-27T12:50:23Z

test/src/Makefile

+	# Verify we have valid AWS credential
+	if command -v aws >/dev/null; then \
+	  (unset AWS_PROFILE; aws sts get-caller-identity >/dev/null); \
+	fi
+	go test -v -timeout 10m -run TestExamplesComplete


@Nuru I'm not 100% sure entirely why this is, but since cloudposse/actions sets AWS_SECRET_KEY instead of the newer AWS_SECRET_ACCESS_KEY (see https://github.com/cloudposse/actions/blob/tf-v1/.github/workflows/test-command.yml#L403) aws sts get-caller-identity will fail, even though terratest will not.

I think this is because the AWS golang SDK maintains backwards compatibility between AWS_SECRET_ACCESS_KEY and AWS_SECRET_KEY, but the cli does not.

So, aws sts get-caller-identity before invoking terratest will cause the run to fail

https://github.com/cloudposse/actions/runs/2925169210?check_suite_focus=true

Run make -C test/src make -C test/src shell: bash -e -o pipefail {0} env: MAKE_INCLUDES: Makefile AWS_ACCESS_KEY_ID: *** AWS_SECRET_KEY: *** make: Entering directory '/__w/actions/actions/test/src' # Verify we have valid AWS credential if command -v aws >/dev/null; then \ (unset AWS_PROFILE; aws sts get-caller-identity >/dev/null); \ fi Partial credentials found in env, missing: AWS_SECRET_ACCESS_KEY make: *** [Makefile:18: test] Error 255 make: Leaving directory '/__w/actions/actions/test/src' Error: Process completed with exit code 2.

Nuru · 2021-06-27T22:05:39Z

/test terratest

This Pull Request has been updated, so we're dismissing all reviews.

Nuru · 2021-06-28T00:13:54Z

/test all

korenyoni

LGTM, but requested a minor change to a variable description.

korenyoni · 2021-06-28T13:27:40Z

variables.tf

@@ -169,21 +169,29 @@ variable "restrict_public_buckets" {
 variable "s3_replication_enabled" {
  type        = bool
  default     = false
-  description = "Set this to true and specify `s3_replica_bucket_arn` to enable replication. `versioning_enabled` must also be `true`."
+  description = "Set this to true and specify `replication_rules` to enable replication. `versioning_enabled` must also be `true`."


The description references a deprecated variable

Suggested change

description = "Set this to true and specify `replication_rules` to enable replication. `versioning_enabled` must also be `true`."

description = "Set this to true and specify `s3_replication_rules` to enable replication. `versioning_enabled` must also be `true`."

korenyoni · 2021-06-28T13:29:06Z

examples/complete/variables.tf

+variable "replication_rules" {
+  default     = []
+  description = "S3 replication rules"
+}


nit: we could add the s3_ prefix to this variable in examples/complete to be more consistent with the newly-added variables in root module.

bridgecrew

Bridgecrew has found 0 infrastructure configuration error in this PR ⬇️

This Pull Request has been updated, so we're dismissing all reviews.

bridgecrew

Bridgecrew has found 0 infrastructure configuration error in this PR ⬇️

korenyoni · 2021-06-28T19:48:24Z

/test all

korenyoni

LGTM, minor typo though.

variables.tf

Co-authored-by: Yonatan Koren <[email protected]>

bridgecrew

Bridgecrew has found 0 infrastructure configuration error in this PR ⬇️

bridgecrew

Bridgecrew has found 0 infrastructure configuration error in this PR ⬇️

This Pull Request has been updated, so we're dismissing all reviews.

bridgecrew

Bridgecrew has found 0 infrastructure configuration error in this PR ⬇️

Nuru · 2021-06-28T19:56:16Z

/test all

korenyoni

Looks good to me.

alexjurkiewicz · 2021-06-30T04:20:05Z

thanks for all the merge help @korenyoni and @Nuru !

Pull out common string into a local variable

241cb24

alexjurkiewicz changed the title ~~## what * Describe high-level what changed as a result of these commits (i.e. in plain-english, what do these changes mean?) * Use bullet points to be concise and to the point.~~ S3 Replication Improvements Jun 24, 2021

alexjurkiewicz and others added 4 commits June 24, 2021 20:48

Add var.replication_source_roles

66ef539

Allows easy management of buckets intended as cross-account S3 replication destinations.

Support replication to multiple buckets

9062254

This removes var.s3_replica_bucket_arn and lets you set it per-replication rule.

Remove var.s3_replication_enabled

9c6cb9d

No longer required. Replication is implicitly enabled if there are any replication rules.

Auto Format

9cc2a02

alexjurkiewicz marked this pull request as ready for review June 24, 2021 11:13

alexjurkiewicz requested review from a team as code owners June 24, 2021 11:13

alexjurkiewicz requested review from dotCipher and 3h4x June 24, 2021 11:13

korenyoni previously requested changes Jun 24, 2021

View reviewed changes

korenyoni reviewed Jun 24, 2021

View reviewed changes

variables.tf Outdated Show resolved Hide resolved

Address review feedback

7c3a690

korenyoni reviewed Jun 24, 2021

View reviewed changes

main.tf Show resolved Hide resolved

Auto Format

31d44aa

korenyoni previously requested changes Jun 25, 2021

View reviewed changes

korenyoni added the enhancement New feature or request label Jun 25, 2021

Nuru added the wip - DO NOT MERGE label Jun 25, 2021

Ensure backward compatibility

d6ee58c

Nuru removed the wip - DO NOT MERGE label Jun 25, 2021

Ignore AWS_PROFILE when testing for AWS credentials

53140de

Support empty s3_replica_bucket_arn in bucket policy

a101eea

korenyoni approved these changes Jun 27, 2021

View reviewed changes

korenyoni previously requested changes Jun 27, 2021

View reviewed changes

Various fixes for multiple replication destinations

3c79dd8

Nuru requested a review from korenyoni June 28, 2021 00:13

korenyoni previously requested changes Jun 28, 2021

View reviewed changes

Reviewer suggestions

a681f1a

bridgecrew bot reviewed Jun 28, 2021

View reviewed changes

Nuru requested a review from korenyoni June 28, 2021 19:44

Auto Format

cc83775

bridgecrew bot reviewed Jun 28, 2021

View reviewed changes

korenyoni previously requested changes Jun 28, 2021

View reviewed changes

variables.tf Outdated Show resolved Hide resolved

Update variables.tf

6a03310

Co-authored-by: Yonatan Koren <[email protected]>

bridgecrew bot reviewed Jun 28, 2021

View reviewed changes

Auto Format

4f5d382

bridgecrew bot reviewed Jun 28, 2021

View reviewed changes

korenyoni approved these changes Jun 28, 2021

View reviewed changes

Nuru merged commit a6f9942 into cloudposse:master Jun 28, 2021

alexjurkiewicz deleted the replication-improvements branch June 30, 2021 04:19

alexjurkiewicz mentioned this pull request Jun 30, 2021

Properly type the default replication filter object #96

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

S3 Replication Improvements #93

S3 Replication Improvements #93

alexjurkiewicz commented Jun 24, 2021 •

edited by Nuru

Loading

alexjurkiewicz commented Jun 24, 2021 •

edited

Loading

korenyoni commented Jun 24, 2021

korenyoni left a comment

korenyoni Jun 24, 2021

alexjurkiewicz Jun 24, 2021

korenyoni commented Jun 24, 2021

korenyoni left a comment •

edited

Loading

korenyoni Jun 25, 2021

korenyoni Jun 25, 2021

korenyoni Jun 25, 2021

Nuru commented Jun 25, 2021 •

edited

Loading

Nuru commented Jun 25, 2021

korenyoni left a comment

korenyoni commented Jun 27, 2021

korenyoni left a comment •

edited

Loading

korenyoni Jun 27, 2021 •

edited

Loading

Nuru commented Jun 27, 2021

Nuru commented Jun 28, 2021

korenyoni left a comment

korenyoni Jun 28, 2021

korenyoni Jun 28, 2021

bridgecrew bot left a comment

bridgecrew bot left a comment

korenyoni commented Jun 28, 2021

korenyoni left a comment

bridgecrew bot left a comment

bridgecrew bot left a comment

bridgecrew bot left a comment

Nuru commented Jun 28, 2021

korenyoni left a comment

alexjurkiewicz commented Jun 30, 2021

	replication_enabled = length(var.replication_rules) > 0
	# the following also takes into account the deprecated var.s3_replication_enabled variable (which is null by default)
	replication_enabled = length(var.replication_rules) > 0 \|\| var.s3_replication_enabled == true

	bucket = rules.value.destination.bucket
	# The following takes into account the deprecated var.s3_replica_bucket_arn variable
	bucket = try(rules.value.destination.bucket, var.s3_replica_bucket_arn)```

	description = "Set this to true and specify `replication_rules` to enable replication. `versioning_enabled` must also be `true`."
	description = "Set this to true and specify `s3_replication_rules` to enable replication. `versioning_enabled` must also be `true`."

S3 Replication Improvements #93

S3 Replication Improvements #93

Conversation

alexjurkiewicz commented Jun 24, 2021 • edited by Nuru Loading

Terraform plan impact

Deprecation

what

why

references

alexjurkiewicz commented Jun 24, 2021 • edited Loading

korenyoni commented Jun 24, 2021

korenyoni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

korenyoni commented Jun 24, 2021

korenyoni left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nuru commented Jun 25, 2021 • edited Loading

Nuru commented Jun 25, 2021

korenyoni left a comment

Choose a reason for hiding this comment

korenyoni commented Jun 27, 2021

korenyoni left a comment • edited Loading

Choose a reason for hiding this comment

korenyoni Jun 27, 2021 • edited Loading

Choose a reason for hiding this comment

Nuru commented Jun 27, 2021

Nuru commented Jun 28, 2021

korenyoni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bridgecrew bot left a comment

Choose a reason for hiding this comment

bridgecrew bot left a comment

Choose a reason for hiding this comment

korenyoni commented Jun 28, 2021

korenyoni left a comment

Choose a reason for hiding this comment

bridgecrew bot left a comment

Choose a reason for hiding this comment

bridgecrew bot left a comment

Choose a reason for hiding this comment

bridgecrew bot left a comment

Choose a reason for hiding this comment

Nuru commented Jun 28, 2021

korenyoni left a comment

Choose a reason for hiding this comment

alexjurkiewicz commented Jun 30, 2021

alexjurkiewicz commented Jun 24, 2021 •

edited by Nuru

Loading

alexjurkiewicz commented Jun 24, 2021 •

edited

Loading

korenyoni left a comment •

edited

Loading

Nuru commented Jun 25, 2021 •

edited

Loading

korenyoni left a comment •

edited

Loading

korenyoni Jun 27, 2021 •

edited

Loading