-
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_kubernetes_cluster - add support for private link #5161
Conversation
Add support for aks private link (public preview). Required version upgrade of the azure-sdk to use the new API, vendor updated
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 @jlpedrosa
Thanks for this PR :)
Taking a look through this is looking pretty good - I've left some (minor) comments inline but if we can fix those up (and the tests pass) then this should otherwise be good to merge 👍
Thanks!
I've fixed the code according to your comments in a new commit. Before this small changes I ran the accTests, they were ok. Let me know what you 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 @jlpedrosa
Thanks for pushing those changes - I've taken a look through and other than a few of the original changes needing to also be made to the Resource this otherwise LGTM 👍
Thanks!
Fixed the code to match your comments. |
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.
one minor question but this otherwise LGTM 👍
@@ -748,7 +766,9 @@ func resourceArmKubernetesClusterUpdate(d *schema.ResourceData, meta interface{} | |||
if d.HasChange("api_server_authorized_ip_ranges") { | |||
updateCluster = true | |||
apiServerAuthorizedIPRangesRaw := d.Get("api_server_authorized_ip_ranges").(*schema.Set).List() | |||
existing.APIServerAuthorizedIPRanges = utils.ExpandStringSlice(apiServerAuthorizedIPRangesRaw) | |||
existing.ManagedClusterProperties.APIServerAccessProfile = &containerservice.ManagedClusterAPIServerAccessProfile{ | |||
AuthorizedIPRanges: utils.ExpandStringSlice(apiServerAuthorizedIPRangesRaw), |
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.
presumably we also need to pass in private_link_enabled
here too, even if it's not updatable, so when updating a cluster with a value of true
the API doesn't return an error?
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've added it just in case. Also added to the acc tests (Ran OK):
- Create and Update with privateLink On
- Create and Update with privateLink Off.
I aded update tests in the acc test and include the private link just in case (they seemed to work fine regardless if we send it or not) but better be safe. |
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 👍 - thanks for pushing those changes @jlpedrosa
(just a heads up - this is good to merge we're just waiting for the test suite to pass prior to merging) |
Ran travis tess locally |
This has been released in version 1.40.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.40.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! |
Add support for aks private link (public preview).
Required version upgrade of the azure-sdk to use the new API, (2019-06-01) -> (2019-10-01).
During the development I saw that the datasource was lacking some fileds added to the resource in other PRs (api_server_authorized_ip_ranges) So i've added them too as in the new API it goes together.
Fixes: #5148