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

Implement aws_efs_replication_configuration resource #22844

Merged

Conversation

wedge-jarrad
Copy link
Contributor

@wedge-jarrad wedge-jarrad commented Jan 31, 2022

There is still a little bit of cleanup to do but I think the functionality is complete. No tests yet but I've tested the very basic cases manually. Writing tests and documentation are next on my list. Any early feedback on the code while I'm writing the tests would be appreciated. I'm a novice at this so if you find yourself wondering if I'm trying to do something clever or if I'm being dumb, the answer is definitely that I'm being dumb :)

One interesting thing about this resource is that creating a replication configuration causes another EFS file system to be created implicitly. Deleting the replication configuration does not delete that file system. This is intentional on AWS' part. Deleting the replication configuration is how you "promote" the read-only replica to be a fully stand-alone read-writable file system. That's something that the user needs to be aware of (and I'll be sure to make note of it in the user docs) but it also means that cleaning up from acceptance tests will be interesting. I haven't quite figured out how that's going to work yet. I assume sweepers will be the key, but haven't dug into those enough just yet to be sure. If anyone is aware of another resource that has a similar pattern please point it out so that I can use it as an example.

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 #22770

Output from acceptance testing:

$ gmake testacc TESTS=TestAccEFSReplicationConfiguration PKG=efs
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/efs/... -v -count 1 -parallel 20 -run='TestAccEFSReplicationConfiguration'  -timeout 180m
=== RUN   TestAccEFSReplicationConfiguration_basic
=== PAUSE TestAccEFSReplicationConfiguration_basic
=== RUN   TestAccEFSReplicationConfiguration_allAttributes
=== PAUSE TestAccEFSReplicationConfiguration_allAttributes
=== RUN   TestAccEFSReplicationConfiguration_disappears
=== PAUSE TestAccEFSReplicationConfiguration_disappears
=== CONT  TestAccEFSReplicationConfiguration_basic
=== CONT  TestAccEFSReplicationConfiguration_allAttributes
=== CONT  TestAccEFSReplicationConfiguration_disappears
--- PASS: TestAccEFSReplicationConfiguration_basic (674.15s)
--- PASS: TestAccEFSReplicationConfiguration_disappears (687.35s)
--- PASS: TestAccEFSReplicationConfiguration_allAttributes (687.46s)
PASS
ok  	github.com/hashicorp/terraform-provider-aws/internal/service/efs	687.611s

@github-actions github-actions bot added size/XL Managed by automation to categorize the size of a PR. needs-triage Waiting for first response or review from a maintainer. provider Pertains to the provider itself, rather than any interaction with AWS. service/efs Issues and PRs that pertain to the efs service. labels Jan 31, 2022
@edmundcraske-bjss
Copy link
Contributor

I commented over on the issue - #22770 (comment)

@github-actions github-actions bot added tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. and removed tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels Feb 5, 2022
@wedge-jarrad
Copy link
Contributor Author

Added basic and disappears tests. Note that currently the tests will leave orphaned EFS file systems in the alternate region. I still haven't figured out how to best handle that. Suggestions very welcome!

Next up is documentation. After that I will look at what other test cases might be needed and try to figure out the orphan resource issue.

@edmundcraske-bjss I'm not ignoring your comment, but given how the AWS API works on this one I'm not sure how what you propose would be possible (it sure would be nice, though, because it would solve the orphan resource issue). I'll let the maintainers comment on the issue if they see an alternate path forward.

@github-actions github-actions bot added documentation Introduces or discusses updates to documentation. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels Feb 8, 2022
@wedge-jarrad wedge-jarrad marked this pull request as ready for review February 10, 2022 07:01
@wedge-jarrad
Copy link
Contributor Author

Marking this as ready for review. Biggest outstanding issue is the fact that the acceptance tests leave file systems behind in the alternate region. I would appreciate any guidance on that point. Is just implementing a sweeper enough, or is there a good way to handle this in the test itself?

I'm on the fence as to whether or not a data source would be useful so I've left it out for now. Only use for it that I could think of would be to find the details of the replica file system that it created, but at that point you'd probably be better of using a data source for the replica file system directly. Let me know if anyone thinks otherwise.

There's a quirk with importing where you need to omit the availability_zone_name and kms_key_id from your configuration when you do so. The reason for this is that the API does not return those details when you run DescribeReplicationConfigurations. Configuring those values in your config will therefore result in a diff the next time you do a plan since the imported resource would expect those to be empty. Any changes require replacement of the replication configuration. I included this in the documentation.

I thought to query those details from the replicated file system (the code is still in there, commented out) but this creates its own problems. The KMS key is returned by the API as an ARN whereas the user has the option of configuring it as an ARN, key id, alias, or alias ARN. Converting between and ARN and ID is simple enough, but if the user had configured it as an alias or alias ARN then additional queries to the KMS service would be required to determine if their configured value matched what was returned by the lookup or not. The user could have also omitted the kms_key_id completely, in which case the default EFS key for that region would have been used and would be returned by the lookup. It would be difficult to say for sure whether the user's configuration should rightfully produce a diff on the next plan or not - particularly in the latter case where they omit kms_key_id because then you'd have to figure out if the key used on the replica is the default for that region and, if so, omit it from the state. The ability to compare those things in the first place assumes that there is a way to access the user's configuration during import, and what I could find suggests that only d.Id() is available during import.

Anyway, the complexity of implementing that safely started to get high enough that I thought it best to simply note that those two attributes should be left out and leave it at that. Happy to revisit if anyone has any ideas for handling it better.

Last point: I am pretty new at this so please feel free to nitpick. I mean it! It will only help me learn. :)

@justinretzolk justinretzolk added enhancement Requests to existing resources that expand the functionality or scope. and removed needs-triage Waiting for first response or review from a maintainer. labels Feb 16, 2022
@ewbankkit ewbankkit self-assigned this May 12, 2022
Copy link
Contributor

@ewbankkit ewbankkit left a comment

Choose a reason for hiding this comment

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

LGTM 🚀.

% make testacc TESTARGS='-run=TestAccEFSReplicationConfiguration_' PKG=efs ACCTEST_PARALLELISM=2              
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/efs/... -v -count 1 -parallel 2  -run=TestAccEFSReplicationConfiguration_ -timeout 180m
=== RUN   TestAccEFSReplicationConfiguration_basic
=== PAUSE TestAccEFSReplicationConfiguration_basic
=== RUN   TestAccEFSReplicationConfiguration_disappears
=== PAUSE TestAccEFSReplicationConfiguration_disappears
=== RUN   TestAccEFSReplicationConfiguration_allAttributes
=== PAUSE TestAccEFSReplicationConfiguration_allAttributes
=== CONT  TestAccEFSReplicationConfiguration_basic
=== CONT  TestAccEFSReplicationConfiguration_allAttributes
--- PASS: TestAccEFSReplicationConfiguration_basic (536.04s)
=== CONT  TestAccEFSReplicationConfiguration_disappears
--- PASS: TestAccEFSReplicationConfiguration_allAttributes (546.79s)
--- PASS: TestAccEFSReplicationConfiguration_disappears (533.62s)
PASS
ok  	github.com/hashicorp/terraform-provider-aws/internal/service/efs	1073.739s

@ewbankkit
Copy link
Contributor

% make providerlint && make golangci-lint
==> Checking source code with providerlint...
==> Checking source code with golangci-lint...

@ewbankkit ewbankkit merged commit 0636a64 into hashicorp:main May 24, 2022
@github-actions github-actions bot added this to the v4.16.0 milestone May 24, 2022
@github-actions
Copy link

This functionality has been released in v4.16.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

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 28, 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. enhancement Requests to existing resources that expand the functionality or scope. provider Pertains to the provider itself, rather than any interaction with AWS. service/efs Issues and PRs that pertain to the efs 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.

Replication for Amazon Elastic File System (EFS)
4 participants