fix: remove transit_encryption != null, auth_token rotation support

cloudposse / terraform-aws-elasticache-redis

Terraform module to provision an ElastiCache Redis Cluster

https://cloudposse.com/accelerate

Apache License 2.0

141 stars 246 forks source link

fix: remove transit_encryption != null, auth_token rotation support #195

Closed Steve-Louie-Bose closed 4 months ago

Steve-Louie-Bose commented 1 year ago

what

This conditional causes this module to not support dynamic auth_token rotation in a nice way. This conditional forces us into a destroy/create instead of an in place modify

why

Avoiding a destroy / create when the aws providers supports this behavior is worth it. It would allow one to use random_password and feed that into an auth_token and rotate it gracefully.

references

closes #194

gusse commented 4 months ago

I guess this isn't going to be merged?

Gowiem commented 4 months ago

@gusse not necessarily. This seems like a valid change... we just get a TON of PRs and a lot of the module PR review is done by volunteers (myself included). That said... are you using @Steve-Louie-Bose's fork? Can you confirm it works as you would expect?

Gowiem commented 4 months ago

/terratest

gusse commented 4 months ago

@gusse not necessarily. This seems like a valid change... we just get a TON of PRs and a lot of the module PR review is done by volunteers (myself included). That said... are you using @Steve-Louie-Bose's fork? Can you confirm it works as you would expect?

No worries, I understand there are tons of things to do and I do appreciate your hard work, thank you for that 🙏 Was just wondering if more work is needed where I could possibly help

I'm indeed using @Steve-Louie-Bose's fork+branch and it seems to be working as expected. In my scenario, I'm updating the module from 0.42.0 to 1.2.0 and the problem was that cluster recreation is forced as I have the auth_token defined.

Gowiem commented 4 months ago

@gusse good to know, thanks for the double check. On these "random removal of logic" PRs, we don't typically have the context of why that might be there, so it can be scary change. Considering it's working for both of you though... I'm inclined to say we move it forward.

@Steve-Louie-Bose unfortunately, we need to run some automation to pass our tests. Mind doing the following locally, adding + committing the result, and pushing to your branch?

make init
make readme

Once you do that, ping me and I'll provide a review. Thanks!

Steve-Louie-Bose commented 4 months ago

@Gowiem and @gusse - updated the readme following your instructions

I was surprised to see so much change in the README. I even rebased on main in the upstream and ended up with the same result.

Steve-Louie-Bose commented 4 months ago

@gusse - I did post a work around that should solve your use case. You'd need to define your auth_token in state first and then run a follow up TF plan that can swap the value in place without destroy/create. (if this takes time to get merged and released)

Gowiem commented 4 months ago

/terratest