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

Independent S3 Bucket Replication Configuration Resource #20777

Conversation

dkujawski
Copy link
Contributor

@dkujawski dkujawski commented Sep 2, 2021

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Closes #20360

S3 Bucket Replication Configuration

Currently the S3 bucket replication configuration is managed as an internal object of the aws_s3_bucket resource. When attempting to configure multi-directional replication between many buckets, a circular dependency error is detected and triggered by Terraform.

The Fix

  • The fix here is to map the existing internal object to an independent resource called aws_s3_bucket_replication_configuration to define replication configurations between s3 buckets.

Proposed HCL representations

Here are implementation examples showing what the HCL will look like. The first code blocks define the aws_s3_bucket configurations assumed to be present in the rest of the examples. Note the lifecycle additions needed to avoid conflicts between existing replication configuration blocks that may exist in the aws_s3_bucket and the new independent aws_s3_bucket_replication_configuration resource.

resource aws_s3_bucket source {
    ...
    versioning {
        enabled = true
    }
    lifecycle {
        ignore_changes = [
            replication_configuration
        ]
    }
}

resource aws_s3_bucket destination {
    ...
    versioning {
        enabled = true
     }
    lifecycle {
        ignore_changes = [
            replication_configuration
        ]
    }
}

Align with the Go SDK v2 structs for PutBucketReplicationInput

In this example we are working to map the structures as they are defined in the Go AWS SDK v2. Following the lead of AWS may be the safest way forward to avoid any potential drift and take advantage of any forward thinking that they have built into the current API.

resource "aws_s3_bucket_replication_configuration" "config" {
  bucket = aws_s3_bucket.source.arn
  role   = aws_iam_role.replication.arn
  rules {
    id       = "some_id_value"
    status   = "Enabled"
    priority = 1
    delete_marker_replication {
      status = "Enabled"
    }
    existing_object_replication {
      status = "Enabled"
    }
    filter {
      prefix = "baz"
      tags {
        key = "value"
      }
    }
    source_selection_criteria {
      replica_modifications {
        status = "Enabled"
      }
      sse_kms_encrypted_objects {
        status = "Enabled"
      }
    }
    destination {
      bucket        = aws_s3_bucket.destination.arn
      storage_class = "STANDARD"
      account_id    = local.destination_bucket_owner_id
      access_control_translation {
        owner = "Destination"
      }
      encryption_configuration { 
        # change the kms key used for SSE
        replica_kms_key_id = aws_kms_key.key.id
      }
      metrics {
        status = "Enabled"  
        event_threshold { 
          minutes = 15  
        }
      }
      replication_time {
        status = "Enabled"
        time {
          minutes = 15 
        }
      }
    }
  }
  rules {...}
}

New Functionality

Considerations

  • As discussed with Hashicorp, the existing replication_configuration child block in the aws_s3_bucket will continue to function exactly as it currently does. To avoid unexpected conflicts or changes, when using the independent aws_s3_bucket_replication_configuration resource, the aws_s3_bucket.replication_configuration of the original aws_s3_bucket child block will need to be included in the ignore_changes lifecycle list. Documentation will need to be added to explicitly advise practitioners to remove the replication configuration definitions from within the the aws_s3_bucket resource or use the lifecycle meta-argument configuration option.
  • Documentation updates will be needed to advise practitioners that conflicts, flapping, and cyclical dependencies may present errors during Plan/Apply operations. Solution to this will be to include the replication_configuration in a lifecycle meta-argument ignore_changes list on the aws_s3_bucket. (This is a work-around until the upcoming major release fixes this)
  • Provide upgrade documentation to advise that using the new aws_s3_bucket_replication_configuration resources will require a depends_on within the aws_s3_bucket_object terraform configurations to ensure they are created only after replication is enabled on the bucket.
  • From a source code perspective no changes will be made to the aws_s3_bucket resource. A new resource will be implemented with stand-alone logic being sensitive to expected code changes moving into the next major release. These changes will continue to use the AWS Go SDK v2.

Imperfections

Note: These imperfections are expected to be resolved in the next major version release when the internal configuration blocks are set to a read-only state.

  • Lots of opportunity for conflict if/when replication configuration settings are defined in both the aws_s3_bucket and this new aws_s3_bucket_replication_configuration resource.
  • Conflicts will require manual configuration update(s)
  • Errors in apply operations may not be immediately obvious to practitioners

Related issues

#16539
#12223
#4418

Output from acceptance testing:

$> make testacc TESTARGS='-run=TestAccAWSS3BucketReplicationConfig'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSS3BucketReplicationConfig -timeout 180m
=== RUN   TestAccAWSS3BucketReplicationConfig_basic
=== PAUSE TestAccAWSS3BucketReplicationConfig_basic
=== RUN   TestAccAWSS3BucketReplicationConfig_multipleDestinationsEmptyFilter
=== PAUSE TestAccAWSS3BucketReplicationConfig_multipleDestinationsEmptyFilter
=== RUN   TestAccAWSS3BucketReplicationConfig_multipleDestinationsNonEmptyFilter
=== PAUSE TestAccAWSS3BucketReplicationConfig_multipleDestinationsNonEmptyFilter
=== RUN   TestAccAWSS3BucketReplicationConfig_twoDestination
=== PAUSE TestAccAWSS3BucketReplicationConfig_twoDestination
=== RUN   TestAccAWSS3BucketReplicationConfig_configurationRuleDestinationAccessControlTranslation
=== PAUSE TestAccAWSS3BucketReplicationConfig_configurationRuleDestinationAccessControlTranslation
=== RUN   TestAccAWSS3BucketReplicationConfig_configurationRuleDestinationAddAccessControlTranslation
=== PAUSE TestAccAWSS3BucketReplicationConfig_configurationRuleDestinationAddAccessControlTranslation
=== RUN   TestAccAWSS3BucketReplicationConfig_replicationTimeControl
=== PAUSE TestAccAWSS3BucketReplicationConfig_replicationTimeControl
=== RUN   TestAccAWSS3BucketReplicationConfig_replicaModifications
=== PAUSE TestAccAWSS3BucketReplicationConfig_replicaModifications
=== RUN   TestAccAWSS3BucketReplicationConfig_withoutStorageClass
=== PAUSE TestAccAWSS3BucketReplicationConfig_withoutStorageClass
=== RUN   TestAccAWSS3BucketReplicationConfig_schemaV2
=== PAUSE TestAccAWSS3BucketReplicationConfig_schemaV2
=== RUN   TestAccAWSS3BucketReplicationConfig_schemaV2SameRegion
=== PAUSE TestAccAWSS3BucketReplicationConfig_schemaV2SameRegion
=== RUN   TestAccAWSS3BucketReplicationConfig_existingObjectReplication
--- PASS: TestAccAWSS3BucketReplicationConfig_existingObjectReplication (0.00s)
=== CONT  TestAccAWSS3BucketReplicationConfig_basic
=== CONT  TestAccAWSS3BucketReplicationConfig_replicationTimeControl
=== CONT  TestAccAWSS3BucketReplicationConfig_twoDestination
=== CONT  TestAccAWSS3BucketReplicationConfig_configurationRuleDestinationAddAccessControlTranslation
=== CONT  TestAccAWSS3BucketReplicationConfig_schemaV2SameRegion
=== CONT  TestAccAWSS3BucketReplicationConfig_withoutStorageClass
=== CONT  TestAccAWSS3BucketReplicationConfig_multipleDestinationsNonEmptyFilter
=== CONT  TestAccAWSS3BucketReplicationConfig_multipleDestinationsEmptyFilter
=== CONT  TestAccAWSS3BucketReplicationConfig_schemaV2
=== CONT  TestAccAWSS3BucketReplicationConfig_configurationRuleDestinationAccessControlTranslation
=== CONT  TestAccAWSS3BucketReplicationConfig_replicaModifications
--- PASS: TestAccAWSS3BucketReplicationConfig_schemaV2SameRegion (31.84s)
--- PASS: TestAccAWSS3BucketReplicationConfig_replicationTimeControl (37.45s)
--- PASS: TestAccAWSS3BucketReplicationConfig_replicaModifications (39.08s)
--- PASS: TestAccAWSS3BucketReplicationConfig_multipleDestinationsEmptyFilter (40.85s)
--- PASS: TestAccAWSS3BucketReplicationConfig_twoDestination (41.44s)
--- PASS: TestAccAWSS3BucketReplicationConfig_withoutStorageClass (42.02s)
--- PASS: TestAccAWSS3BucketReplicationConfig_multipleDestinationsNonEmptyFilter (42.62s)
--- PASS: TestAccAWSS3BucketReplicationConfig_configurationRuleDestinationAddAccessControlTranslation (58.12s)
--- PASS: TestAccAWSS3BucketReplicationConfig_configurationRuleDestinationAccessControlTranslation (62.94s)
--- PASS: TestAccAWSS3BucketReplicationConfig_basic (78.49s)
--- PASS: TestAccAWSS3BucketReplicationConfig_schemaV2 (118.23s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       118.898s

...

Blocking out general structure for new independent resource for managing
the s3 bucket replication configuration settings

Pulling over logic from resource s3 bucket to start with
…thub.com:dkujawski/terraform-provider-aws into dk-FCC-332-bi-directional-s3-replication-support
Rename resource names to reflect new position in configuration scope of
the independent resource.

Use literal strings instead of fmt.Sprint in hcl concatination
Ensure that the source bucket name is configured in the HCL

Ensure that when importing the bucket name is passed in to the process
as the import id value
Relocate replication testing helper functions out of the s3 bucket tests
and into the replication configuration testing file.

Remove s3 bucket existance checks from replication testing per does not
apply to the replication resource logic.
Adding schema for ExistingObjectReplication configuration

Adding read logic to identify ExistingObjectReplication configurations
added to replication rules

Adding update logic to include ExistingObjectReplicaiton configuration
in the PutBucketReplicaiton input
In order for ExistingObjectReplication to work on s3 buckets, a request
to AWS Technical Support needs to be made.  Once they allow the
configuration the test will operate as expected.
new schema definition for "replication_time" along with update and read
logic.

tracking upstream changes, adopt "waiter" module
Metrics are a requirement for the Replication Time Control
functionality.  Adding it here.

Restructure the configuration read logic for Replication Time to be more
correct and inline with expected data structures

Update tests to reflect changes
Update the the source_selection_criteria configuration to include the
replica_modificaions.

Refactored sse_kms_encrypted_objects schema to map closer to the actual
AWS SDK structure.
@github-actions github-actions bot added needs-triage Waiting for first response or review from a maintainer. provider Pertains to the provider itself, rather than any interaction with AWS. service/s3 Issues and PRs that pertain to the s3 service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. size/XL Managed by automation to categorize the size of a PR. labels Sep 2, 2021
@dkujawski dkujawski changed the title Dk fcc 332 bi directional s3 replication support Independent S3 Bucket Replication Configuration Resource Sep 2, 2021
@github-actions github-actions bot removed service/acm Issues and PRs that pertain to the acm service. service/acmpca Issues and PRs that pertain to the acmpca service. client-connections Pertains to the AWS Client and service connections. generators Relates to code generators. repository Repository modifications; GitHub Actions, developer docs, issue templates, codeowners, changelog. pre-service-packages Includes pre-Service Packages aspects. service/apigateway Issues and PRs that pertain to the apigateway service. tags Pertains to resource tagging. service/amplify Issues and PRs that pertain to the amplify service. sweeper Pertains to changes to or issues with the sweeper. dependencies Used to indicate dependency changes. service/accessanalyzer Issues and PRs that pertain to the accessanalyzer service. labels Nov 3, 2021
@lee5i3
Copy link

lee5i3 commented Nov 5, 2021

Hashicorp, please MERGE this!!!!

@dkujawski
Copy link
Contributor Author

Updated tests to reflect upstream changes and V2 related requirements from AWS.

$> TF_ACC=1 go test ./internal/service/s3 -v -count 1 -run=TestAccAWSS3BucketReplicationConfig_ -timeout 180m
=== RUN   TestAccAWSS3BucketReplicationConfig_basic
=== PAUSE TestAccAWSS3BucketReplicationConfig_basic
=== RUN   TestAccAWSS3BucketReplicationConfig_multipleDestinationsEmptyFilter
=== PAUSE TestAccAWSS3BucketReplicationConfig_multipleDestinationsEmptyFilter
=== RUN   TestAccAWSS3BucketReplicationConfig_multipleDestinationsNonEmptyFilter
=== PAUSE TestAccAWSS3BucketReplicationConfig_multipleDestinationsNonEmptyFilter
=== RUN   TestAccAWSS3BucketReplicationConfig_twoDestination
=== PAUSE TestAccAWSS3BucketReplicationConfig_twoDestination
=== RUN   TestAccAWSS3BucketReplicationConfig_configurationRuleDestinationAccessControlTranslation
=== PAUSE TestAccAWSS3BucketReplicationConfig_configurationRuleDestinationAccessControlTranslation
=== RUN   TestAccAWSS3BucketReplicationConfig_configurationRuleDestinationAddAccessControlTranslation
=== PAUSE TestAccAWSS3BucketReplicationConfig_configurationRuleDestinationAddAccessControlTranslation
=== RUN   TestAccAWSS3BucketReplicationConfig_replicationTimeControl
=== PAUSE TestAccAWSS3BucketReplicationConfig_replicationTimeControl
=== RUN   TestAccAWSS3BucketReplicationConfig_replicaModifications
=== PAUSE TestAccAWSS3BucketReplicationConfig_replicaModifications
=== RUN   TestAccAWSS3BucketReplicationConfig_withoutStorageClass
=== PAUSE TestAccAWSS3BucketReplicationConfig_withoutStorageClass
=== RUN   TestAccAWSS3BucketReplicationConfig_schemaV2
=== PAUSE TestAccAWSS3BucketReplicationConfig_schemaV2
=== RUN   TestAccAWSS3BucketReplicationConfig_schemaV2SameRegion
=== PAUSE TestAccAWSS3BucketReplicationConfig_schemaV2SameRegion
=== RUN   TestAccAWSS3BucketReplicationConfig_existingObjectReplication
--- PASS: TestAccAWSS3BucketReplicationConfig_existingObjectReplication (0.00s)
=== RUN   TestAccAWSS3BucketReplicationConfig_delete
=== PAUSE TestAccAWSS3BucketReplicationConfig_delete
=== CONT  TestAccAWSS3BucketReplicationConfig_basic
=== CONT  TestAccAWSS3BucketReplicationConfig_replicationTimeControl
=== CONT  TestAccAWSS3BucketReplicationConfig_delete
=== CONT  TestAccAWSS3BucketReplicationConfig_schemaV2SameRegion
=== CONT  TestAccAWSS3BucketReplicationConfig_schemaV2
=== CONT  TestAccAWSS3BucketReplicationConfig_withoutStorageClass
=== CONT  TestAccAWSS3BucketReplicationConfig_replicaModifications
=== CONT  TestAccAWSS3BucketReplicationConfig_twoDestination
=== CONT  TestAccAWSS3BucketReplicationConfig_configurationRuleDestinationAddAccessControlTranslation
=== CONT  TestAccAWSS3BucketReplicationConfig_configurationRuleDestinationAccessControlTranslation
=== CONT  TestAccAWSS3BucketReplicationConfig_multipleDestinationsEmptyFilter
=== CONT  TestAccAWSS3BucketReplicationConfig_multipleDestinationsNonEmptyFilter
--- PASS: TestAccAWSS3BucketReplicationConfig_schemaV2SameRegion (30.61s)
--- PASS: TestAccAWSS3BucketReplicationConfig_replicaModifications (39.92s)
--- PASS: TestAccAWSS3BucketReplicationConfig_replicationTimeControl (40.08s)
--- PASS: TestAccAWSS3BucketReplicationConfig_twoDestination (40.18s)
--- PASS: TestAccAWSS3BucketReplicationConfig_multipleDestinationsEmptyFilter (42.36s)
--- PASS: TestAccAWSS3BucketReplicationConfig_multipleDestinationsNonEmptyFilter (42.66s)
--- PASS: TestAccAWSS3BucketReplicationConfig_schemaV2 (43.13s)
--- PASS: TestAccAWSS3BucketReplicationConfig_withoutStorageClass (43.58s)
--- PASS: TestAccAWSS3BucketReplicationConfig_delete (57.84s)
--- PASS: TestAccAWSS3BucketReplicationConfig_configurationRuleDestinationAddAccessControlTranslation (63.64s)
--- PASS: TestAccAWSS3BucketReplicationConfig_configurationRuleDestinationAccessControlTranslation (64.24s)
--- PASS: TestAccAWSS3BucketReplicationConfig_basic (79.08s)
PASS
ok  	github.com/hashicorp/terraform-provider-aws/internal/service/s3	79.865s

Copy link
Contributor

@anGie44 anGie44 left a comment

Choose a reason for hiding this comment

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

Hi @dkujawski -- overall this is looking pretty great! With the service package refactor, some naming conventions have changed just a bit in our test files so I'm going to give a quick re-look and push up a commit to get those inline and have this ready for merging for this week's release

@anGie44
Copy link
Contributor

anGie44 commented Nov 18, 2021

Hi again @dkujawski -- I've come across strange test errors when running against terraform CLI 0.14.0 so i'm going to double-check if there's any additional work needed, as the tests are certainly all passing when using the latest version of terraform (1.0.11) and as far back as 0.14.7

Example test errors (terraform CLI 0.14.0:

=== CONT  TestAccS3BucketReplicationConfiguration_basic
    bucket_replication_configuration_test.go:29: Step 1/3 error: Error running post-apply refresh: exit status 1
        
        Error: expected replication_configuration.0.rules.0.delete_marker_replication_status to be one of [Enabled], got 
        
          on terraform_plugin_test.tf line 37, in resource "aws_s3_bucket" "source":
          37: resource "aws_s3_bucket" "source" {
        
        
        
        Error: "replication_configuration.0.rules.0.destination.0.account_id" doesn't look like AWS Account ID (exactly 12 digits): ""
        
          on terraform_plugin_test.tf line 37, in resource "aws_s3_bucket" "source":
          37: resource "aws_s3_bucket" "source" {
        
        
--- FAIL: TestAccS3BucketReplicationConfiguration_basic (350.71s)

Update: since the errors are coming from 0.1.4.0 but look to be fixed in later versions of 0.14.x, we'll consider them as upstream and not hold off on the merge

@anGie44 anGie44 added this to the v3.66.0 milestone Nov 18, 2021
@anGie44 anGie44 merged commit 8020a3f into hashicorp:main Nov 18, 2021
@github-actions
Copy link

This functionality has been released in v3.66.0 of the Terraform AWS Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

@github-actions
Copy link

github-actions bot commented Jun 8, 2022

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Introduces or discusses updates to documentation. new-resource Introduces a new resource. provider Pertains to the provider itself, rather than any interaction with AWS. service/s3 Issues and PRs that pertain to the s3 service. size/XL Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Separate Resource for S3 Replication Configuration
5 participants