-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Add support to disable authentication for Azure Redis caches #3389
Conversation
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.
hey @code-haven
Thanks for this PR :)
Taking a look through I believe it'd make more sense to invert this value so that it's a boolean field called authentication_enabled
with a default value of true
, and then cast to/from the yes/no
required in the Azure API, to make this a more Terraform-ey user experience - what do you think?
Thanks!
c1be47a
to
e182b2f
Compare
…tion to disable authentication
@tombuildsstuff Apologies for the force pushes -- wanted to keep the changes under a single commit. I have made the changes that were suggested. |
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.
hey @code-haven
Thanks for pushing those updates - besides the test naming (which needs the underscore to match the other tests to ensure it's consistent for some tooling - which I'll push a commit for, I hope you don't mind) this LGTM 👍
Thanks!
Some tooling will care about the presence of an underscore between the Resource Name and the test name in the near future, just ensuring this is consistent
@code-haven no worries about the force pushes, I hope you don't mind I had to push a commit to master; for whatever reason we can push to rebases to branches of a fork, but if we do that to the master branch it breaks the PR, so this was simplest 🤷♂ |
@tombuildsstuff That's fine. Thank you! However, there is a problem(or bug ?) I discovered yesterday while testing this and was confirmed by the solutions architect from Microsoft I'm working with at work: If we have a subnet that has a Redis cache launched with
However, we can launch the second one with |
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.
looks like we are getting test failures with the new property:
------- Stdout: -------
=== RUN TestAccAzureRMRedisCache_PatchSchedule
=== PAUSE TestAccAzureRMRedisCache_PatchSchedule
=== CONT TestAccAzureRMRedisCache_PatchSchedule
--- FAIL: TestAccAzureRMRedisCache_PatchSchedule (68.77s)
testing.go:568: Step 0 error: errors during apply:
Error: Error issuing create request for Redis Cache acctestRedis-190510050856814028 (resource group acctestRG-190510050856814028): redis.Client#Create: Failure sending request: StatusCode=400 -- Original Error: Code="InvalidRequestBody" Message="The value of the parameter 'properties.redisConfiguration.authnotrequired' is invalid.\r\nRequestID=73d98b42-6902-4d52-bd85-3f118435952a"
on /opt/teamcity-agent/temp/buildTmp/tf-test619733293/main.tf line 7:
(source code not available)
FAIL
WRT to that bug there, it sounds like it is on the azure side. Perhaps it would be best to open an issue on the azure-sdk-for-go and link to it here and add a note to the docs explaining the limitation (as well as linking to the issue). WDYT?
@katbyte @tombuildsstuff As a solution, I made What do you guys think? |
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.
hey @code-haven
Thanks for pushing those changes, that makes sense to me - aside from one minor fix for the documentation (to ensure it matches the other resources, which I hope you don't mind but I'll push a commit to fix) - this now LGTM 👍
Thanks!
dismissing since changes have been pushed
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.
LGTM 👍
This has been released in version 1.28.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 = "~> 1.28.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 👉 [email protected]. Thanks! |
By default, Azure Redis cache instances have authentication enabled. To disable it, we'll need to set
authnotrequired=yes
as part ofredis_configuration
. This PR adds the changes for that.