-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
azurerm_api_management: invert ssl tls flags #4226
azurerm_api_management: invert ssl tls flags #4226
Conversation
pull upstream
disable_backend*** and disable_frontend**** terraform properties for APIM are all the wrong way round - disable in terraform should not enable them in AzureRM Fixes hashicorp#3125 See hashicorp#3125
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 @alastairtree,
Thank you for this PR. This should fix it, however it is a breaking change and then the boolean value doesn't match what is passed to the API. A better solution here might be to add new properties enable_backend_ect
and deprecate the old ones. WDYT?
New properties makes sense so it lines up with the ARM API, but I kinda think somewhere has to make a breaking change or fix it properly - it is currently a broken security issue. In my case I found it because a security audit noticed that the old SSL we thought we had disabled was actually wide open. Not convinced deprecating the old properties is really good enough - they insecurely do the exact opposit of what they say they should! Although I understand that some people might have deliberatley passed in the inverted wrong value to achieve the desired outcome, i suspect more people are just getting the wrong behaviour because the code is wrong and needs fixing. Either way the experience is not good, so someone just needs to make a decision. |
@alastairtree, if we flip it we'll break anyone using it property. By creating new properties and deprecating the old it means users will get a message stating exactly why they should switch, and then when we release 2.0 in the upcoming months we can remove the incorrect ones entirely. |
Hi @alastairtree, as i haven't received a response nor can push to you branch i have opened #4226 with my suggested fix and am going to close this in favour of it. Thanks again for the PR! |
This has been released in version 2.0.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example: provider "azurerm" {
version = "~> 2.0.0"
}
# ... other configuration ... |
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. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks! |
This is a naive, untested fix, but I think it should correct the wrongly inverted properties?
disable_backend*** and disable_frontend**** terraform properties for APIM are all the wrong way round - disable in terraform should not enable them in AzureRM
Fixes #3125