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

kubernetes_cluster: Add support for private_dns_zone_id #10201

Merged
merged 16 commits into from
Feb 11, 2021
Merged

kubernetes_cluster: Add support for private_dns_zone_id #10201

merged 16 commits into from
Feb 11, 2021

Conversation

favoretti
Copy link
Collaborator

@favoretti favoretti commented Jan 15, 2021

Fixes #10193
Blocked on #7979 #8737

Copy link
Member

@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 @favoretti

Thanks for this PR :)

I've taken a look through and left a few minor comments inline but this otherwise LGTM - I've merged #8737, so if we can rebase this then the tests should be good to run 👍

Thanks!

Copy link
Member

@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 👍

Copy link
Member

@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 @favoretti

I've run the tests for this and it appears the configuration is slightly out - but I think if we can update the field name then we should otherwise be good here 👍

Thanks!

@favoretti
Copy link
Collaborator Author

@tombuildsstuff This should work, although I haven't tested all the options.

@tombuildsstuff tombuildsstuff modified the milestones: v2.45.0, v2.46.0 Jan 28, 2021
@dansanabria
Copy link

We need this! initial testing looks promising

Copy link
Member

@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 @favoretti

Thanks for pushing those changes - I've taken a look through and this is looking good, if we can remove the None option and add an acceptance test covering System (which I think'll require adjusting the validation logic in the Create) - this should otherwise be good to go 👍

Thanks!

@ghost ghost added size/L and removed size/M labels Feb 3, 2021
@favoretti
Copy link
Collaborator Author

$ TF_ACC=1 go test -v ./azurerm/internal/services/containers/ -timeout=1000m -run TestAccKubernetesCluster_privateClusterOnWithPrivateDNSZoneSystem
=== RUN   TestAccKubernetesCluster_privateClusterOnWithPrivateDNSZoneSystem
=== PAUSE TestAccKubernetesCluster_privateClusterOnWithPrivateDNSZoneSystem
=== CONT  TestAccKubernetesCluster_privateClusterOnWithPrivateDNSZoneSystem
--- PASS: TestAccKubernetesCluster_privateClusterOnWithPrivateDNSZoneSystem (834.53s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/containers	835.738s

@favoretti
Copy link
Collaborator Author

$ TF_ACC=1 go test -v ./azurerm/internal/services/containers/ -timeout=1000m -run TestAccKubernetesCluster_privateClusterOnWithPrivateDNSZone
=== RUN   TestAccKubernetesCluster_privateClusterOnWithPrivateDNSZone
=== PAUSE TestAccKubernetesCluster_privateClusterOnWithPrivateDNSZone
=== RUN   TestAccKubernetesCluster_privateClusterOnWithPrivateDNSZoneSystem
=== PAUSE TestAccKubernetesCluster_privateClusterOnWithPrivateDNSZoneSystem
=== CONT  TestAccKubernetesCluster_privateClusterOnWithPrivateDNSZone
=== CONT  TestAccKubernetesCluster_privateClusterOnWithPrivateDNSZoneSystem
--- PASS: TestAccKubernetesCluster_privateClusterOnWithPrivateDNSZone (802.86s)
--- PASS: TestAccKubernetesCluster_privateClusterOnWithPrivateDNSZoneSystem (829.75s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/containers	830.968s

Copy link
Member

@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 👍

@tombuildsstuff
Copy link
Member

Running the tests for this, it appears that Private Clusters may need to explicitly set private_dns_zone_id to System, but that public clusters are unaffected - whilst I've run a subset of the tests, I think we need a full suite run for the Kubernetes package to be sure

@favoretti
Copy link
Collaborator Author

Push it to 2.47 otherwise? I can add more acceptance tests as well to see whether not setting it to anything works.

@tombuildsstuff
Copy link
Member

Yeah, I have a feeling this only affects Private Clusters (I spotted this for TestAccKubernetesCluster_privateClusterOn) - I'm running the suite now but I have a feeling this'll need an upgrade note explaining this, so I'm going to push this back to next week's release

@tombuildsstuff tombuildsstuff modified the milestones: v2.46.0, v2.47.0 Feb 4, 2021
@EppO
Copy link
Contributor

EppO commented Feb 5, 2021

Running the tests for this, it appears that Private Clusters may need to explicitly set private_dns_zone_id to System, but that public clusters are unaffected - whilst I've run a subset of the tests, I think we need a full suite run for the Kubernetes package to be sure

Private AKS clusters should be able to use private_dns_zone_id with the Resource ID of the private DNS zone per the docs

"Custom private dns zone name" should be in this format for azure global cloud: privatelink..azmk8s.io. You will need the Resource Id of that Private DNS Zone. Additionally, you will need a user assigned identity or service principal with at least the private dns zone contributor role to the custom private dns zone.

So I assume the workflow would be to:

  • create a SP or a user assigned identity
  • create a private DNS zone
  • assign to the SP or user assigned identity a private DNS zone contributor role on the private DNS zone
  • create the AKS cluster with the private_dns_zone_id and the SP's client_id/client_secret or user assigned identity's user_assigned_identity_id

@tombuildsstuff tombuildsstuff self-assigned this Feb 11, 2021
favoretti and others added 16 commits February 11, 2021 16:51
…ce.go

Co-authored-by: Tom Harvey <tombuildsstuff@users.noreply.github.com>
…ce.go

Co-authored-by: Tom Harvey <tombuildsstuff@users.noreply.github.com>
…ce.go

Co-authored-by: Tom Harvey <tombuildsstuff@users.noreply.github.com>
…network_resource_test.go

Co-authored-by: Tom Harvey <tombuildsstuff@users.noreply.github.com>
…resource_test.go

Co-authored-by: Tom Harvey <tombuildsstuff@users.noreply.github.com>
…ce.go

Co-authored-by: Tom Harvey <tombuildsstuff@users.noreply.github.com>
Co-authored-by: Tom Harvey <tombuildsstuff@users.noreply.github.com>
@tombuildsstuff tombuildsstuff merged commit c746dec into hashicorp:master Feb 11, 2021
tombuildsstuff added a commit that referenced this pull request Feb 11, 2021
@ghost
Copy link

ghost commented Feb 11, 2021

This has been released in version 2.47.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.47.0"
}
# ... other configuration ...

@ghost
Copy link

ghost commented Mar 14, 2021

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 as resolved and limited conversation to collaborators Mar 14, 2021
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 Custom Private DNS Zone for AKS
4 participants