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

azurerm_kubernetes_cluster - add support for private link #5161

Merged
merged 9 commits into from
Dec 18, 2019

Conversation

jlpedrosa
Copy link
Contributor

@jlpedrosa jlpedrosa commented Dec 13, 2019

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

Add support for aks private link (public preview). Required version upgrade of the azure-sdk to use the new API, vendor updated
Copy link
Contributor

@tombuildsstuff tombuildsstuff left a 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!

azurerm/data_source_kubernetes_cluster.go Show resolved Hide resolved
azurerm/data_source_kubernetes_cluster_test.go Outdated Show resolved Hide resolved
azurerm/data_source_kubernetes_cluster_test.go Outdated Show resolved Hide resolved
azurerm/resource_arm_kubernetes_cluster.go Outdated Show resolved Hide resolved
azurerm/resource_arm_kubernetes_cluster.go Outdated Show resolved Hide resolved
website/docs/d/kubernetes_cluster.html.markdown Outdated Show resolved Hide resolved
website/docs/d/kubernetes_cluster.html.markdown Outdated Show resolved Hide resolved
website/docs/r/kubernetes_cluster.html.markdown Outdated Show resolved Hide resolved
website/docs/r/kubernetes_cluster.html.markdown Outdated Show resolved Hide resolved
website/docs/r/kubernetes_cluster.html.markdown Outdated Show resolved Hide resolved
@jlpedrosa
Copy link
Contributor Author

Hi @tombuildsstuff

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.

@ghost ghost removed the waiting-response label Dec 16, 2019
Copy link
Contributor

@tombuildsstuff tombuildsstuff left a 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!

azurerm/data_source_kubernetes_cluster.go Outdated Show resolved Hide resolved
azurerm/resource_arm_kubernetes_cluster.go Show resolved Hide resolved
@jlpedrosa
Copy link
Contributor Author

Hi @tombuildsstuff

Fixed the code to match your comments.

@ghost ghost removed the waiting-response label Dec 16, 2019
Copy link
Contributor

@tombuildsstuff tombuildsstuff left a 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),
Copy link
Contributor

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?

Copy link
Contributor Author

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):

  1. Create and Update with privateLink On
  2. Create and Update with privateLink Off.

@jlpedrosa
Copy link
Contributor Author

Hi @tombuildsstuff

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.

Copy link
Contributor

@tombuildsstuff tombuildsstuff left a 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

@tombuildsstuff
Copy link
Contributor

(just a heads up - this is good to merge we're just waiting for the test suite to pass prior to merging)

@ghost ghost removed the size/XXL label Dec 18, 2019
@ghost ghost added the size/XL label Dec 18, 2019
@katbyte
Copy link
Collaborator

katbyte commented Dec 18, 2019

Ran travis tess locally

@katbyte katbyte merged commit 87de8ae into hashicorp:master Dec 18, 2019
@katbyte katbyte changed the title Add support for AKS private link azurerm_kubernetes_cluster - add support for private link Dec 18, 2019
katbyte added a commit that referenced this pull request Dec 18, 2019
@ghost
Copy link

ghost commented Jan 8, 2020

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 ...

@ghost
Copy link

ghost commented Mar 28, 2020

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!

@ghost ghost locked and limited conversation to collaborators Mar 28, 2020
@jlpedrosa jlpedrosa deleted the aks_private_link branch March 28, 2020 17:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for Private Links in AKS
3 participants