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

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

Merged
merged 2 commits into from
Feb 22, 2024

Conversation

Steve-Louie-Bose
Copy link
Contributor

@Steve-Louie-Bose Steve-Louie-Bose commented Apr 26, 2023

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

@Steve-Louie-Bose Steve-Louie-Bose requested review from a team as code owners April 26, 2023 14:13
@Steve-Louie-Bose Steve-Louie-Bose requested review from jamengual and woz5999 and removed request for a team April 26, 2023 14:13
@gusse
Copy link
Contributor

gusse commented Feb 19, 2024

I guess this isn't going to be merged?

@Gowiem
Copy link
Member

Gowiem commented Feb 19, 2024

@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 Gowiem self-requested a review February 19, 2024 19:02
@Gowiem
Copy link
Member

Gowiem commented Feb 19, 2024

/terratest

@gusse
Copy link
Contributor

gusse commented Feb 20, 2024

@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
Copy link
Member

Gowiem commented Feb 20, 2024

@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
Copy link
Contributor Author

Steve-Louie-Bose commented Feb 22, 2024

@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
Copy link
Contributor Author

Steve-Louie-Bose commented Feb 22, 2024

@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 Gowiem added the patch A minor, backward compatible change label Feb 22, 2024
@Gowiem
Copy link
Member

Gowiem commented Feb 22, 2024

/terratest

Copy link
Member

@Gowiem Gowiem left a comment

Choose a reason for hiding this comment

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

LGTM

@Gowiem Gowiem enabled auto-merge (squash) February 22, 2024 17:47
@Gowiem Gowiem merged commit a49c8e6 into cloudposse:main Feb 22, 2024
18 checks passed
@Steve-Louie-Bose Steve-Louie-Bose deleted the auth_token-rotate branch February 22, 2024 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch A minor, backward compatible change
Projects
None yet
3 participants