-
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
Fixes for handling the dns_prefix
property of an AKS cluster
#2611
Conversation
If you try to change the DNS prefix with the configuration before this change, `terraform apply` will fail with a message to this effect.
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 @tomasaschan
Thanks for this PR :)
I've taken a look through and this mostly LGTM, if we can fix up the minor comment (and the tests pass) this otherwise LGTM 👍
Thanks!
This validation is not done in the CLI, so creating a cluster with a DNS prefix that does not fulfill these requirements will not fail. However, in some cases such a cluster will be badly broken; for example, if the specified DNS prefix has subdomain parts (e.g. foo.bar.baz), certificate validation will fail when communicating with the cluster, resulting in broken behavior for commands such as kubectl logs. Because of the seriously broken state a cluster can end up in, and since the cluster will have to be completely recreated in order to change the DNS prefix, I think it's worthwhile to implement this validation even before the CLI has been patched to include it. The source for the validation rules (and error message) is the client- side validation error message in the Azure Portal, where validation is already implemented.
a131436
to
d16b533
Compare
@tombuildsstuff Thanks! In the meantime I got my local go env working again and noticed a build error, so I force-pushed a new commit that includes a previously missing |
Finally figured out how to use the provider locally as well, so now I have tested it on my setup and it seems to do what I intended it to :) I haven't touched the tests, and couldn't find anything in them about the Thanks for the speedy review - I'm already looking forward to the release of 1.21! |
@tomasaschan thanks for pushing those changes - would you mind reverting that from the changelog? in the larger providers (AWS/Azure/GCP) we only change that directly on master to try and reduce conflicts |
5810467
to
d16b533
Compare
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 @tomasaschan,
Thanks for the changes, the one last thing is fixing the build. Seems linting is throwing this error:
$ make lint
==> Checking source code against linters...
azurerm/resource_arm_kubernetes_cluster.go:1130:3:warning: should use raw string (`...`) with regexp.MustCompile to avoid having to escape twice (S1007) (megacheck)
make: *** [lint] Error 1
The command "make lint" exited with 2.
Should be a quick fix 🙂
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 hope you don't mind but I fixed the line myself, this LGTM now 🙂
@katbyte Not at all, thanks for the help! :) |
Fix the documentation issue for in application_insight.
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! |
I got help from Microsoft Support today because one of my clusters were badly broken. Eventually, we figured out that it was because of a bad
dns_prefix
.When I tried to use Terraform to change the prefix it didn't complain, until the underlying ARM call failed because the
dns_prefix
can only be set when the cluster is created, and then never changed.This PR fixes both these issues: it forces recreation of the cluster if the
dns_prefix
changes, and it adds the missing validation for DNS prefixes. See the respective commit messages for more details on each change.