-
Notifications
You must be signed in to change notification settings - Fork 9.1k
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
RDS Aurora Cross Region replication for encrypted clusters #2111
RDS Aurora Cross Region replication for encrypted clusters #2111
Conversation
Any objections to merging this? This is really holding my work back as far as being able to deploy encrypted cross region RDS clusters. |
@radeksimko I believe there should also be a bug label on this since it was missing source_region causing it to fail when trying to deploy a cross region replication cluster that's encrypted. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @mitchelldavis44
thanks for the PR.
In addition to what I mentioned in the code, do you mind adding an acceptance test or modifying and existing one to exercise this new field?
aws/resource_aws_rds_cluster.go
Outdated
"source_region": { | ||
Type: schema.TypeString, | ||
Optional: true, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we should mark this as ConflictsWith: []string{"snapshot_identifier"}
because this field will be ignored if the DB is restored, instead of created from scratch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also it seems it's not updatable, so we should mark it as ForceNew: true
.
ab29929
to
c423a24
Compare
Hi @radeksimko I have made the changes you've requested, I've never added an acceptance test before but I tried my hand at adding one. Let me know if you need anything else, thanks! |
@radeksimko is this ready to merge, or are there more changes needed? |
@mitchelldavis44 thanks for modifying the schema, I think we'll need a bit more testing there - i.e. a separate test which spins up an Aurora cluster with both encryption and |
8b5cd56
to
7874ae7
Compare
@radeksimko I added a completely new test for cross region replicas that checks that |
CheckDestroy: testAccCheckAWSClusterDestroy, | ||
Steps: []resource.TestStep{ | ||
{ | ||
Config: testAccAWSClusterConfig(acctest.RandInt()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we'll also need to create a config which sets up the cross-region replication, testAccAWSClusterConfig
doesn't do it. Any minimalistic configuration will do in this context, but it should actually setup the replication and test this new behaviour/field - ideally we'd be checking the value of source_region
, not just whether it's set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a little out of my comfort zone. Unfortunately I don't really have the experience to do what you're asking, I'm not very experienced in writing go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@radeksimko is there any way we can have this merged? I'm not the guy to make more complex tests, but this is definitely a needed flag in terraform.
It looks like there are 2 conflicting PR's solving the same issue - This one, and another #2808 by @dselvan. Both are requesting help with the test - although @dselvan has an almost working Terraform test setup, it doesn't work with both providers (which is necessary to deploy to both east and west for cross-region replication). Is there a different way this test can be composed - is it somehow related to I have tried searching the tests to find an example of doing something similar (2 providers while calling |
This is getting kinda ridiculous, is there someone that can help with the tests or at the very least a timetable on when you guys will release this? The code I have in this pull request works, for both east and west and we've been using our "own" provider since this hasn't been merged. Again, cross region replication works completely with this pull request, any assistance on the test would be greatly appreciated. |
This has been released in version 1.10.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. |
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks! |
This fixes issue RDS Aurora Cross-Region replication for encrypted cluster failing #630. Issue was it was missing source_region within the module which was making it fail when trying to create a cross region replication cluster that was encrypted with the errror "aws_rds_cluster.replica: InvalidParameterCombination: Source cluster arn:aws:rds:us-east-1::cluster:<cluster_name> is encrypted; pre-signed URL has to be specified status code: 400, request id: b59ab0b3-812b-11e7-8467-c192637e53bf"