-
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
New Resource: azurerm_arc_kubernetes_cluster
#15401
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.
hi @ms-zhenhua
Thanks for this PR - taking a look through here whilst this is off to a good start and I've left some comments inline, it appears that there's some more fields needed to make this usable. Out of interest have you tried this with a Kubernetes Cluster connected to it?
Thanks!
internal/services/hybridkubernetes/hybrid_kubernetes_connected_cluster_resource.go
Outdated
Show resolved
Hide resolved
internal/services/hybridkubernetes/hybrid_kubernetes_connected_cluster_resource.go
Outdated
Show resolved
Hide resolved
internal/services/hybridkubernetes/hybrid_kubernetes_connected_cluster_resource.go
Outdated
Show resolved
Hide resolved
internal/services/hybridkubernetes/hybrid_kubernetes_connected_cluster_resource.go
Outdated
Show resolved
Hide resolved
internal/services/hybridkubernetes/hybrid_kubernetes_connected_cluster_resource.go
Outdated
Show resolved
Hide resolved
website/docs/r/hybrid_kubernetes_connected_cluster.html.markdown
Outdated
Show resolved
Hide resolved
website/docs/r/hybrid_kubernetes_connected_cluster.html.markdown
Outdated
Show resolved
Hide resolved
website/docs/r/hybrid_kubernetes_connected_cluster.html.markdown
Outdated
Show resolved
Hide resolved
website/docs/r/hybrid_kubernetes_connected_cluster.html.markdown
Outdated
Show resolved
Hide resolved
internal/services/hybridkubernetes/hybrid_kubernetes_connected_cluster_resource.go
Outdated
Show resolved
Hide resolved
Hi @tombuildsstuff , thank you for reviewing my code. I have updated the code and re-run the test cases with no error. Kindly help review it again. And I've also created a k8s cluster and successfully connected it to Azure. BTW, the Golang linting failed because of timeout. But it ran successfully on my own repo. Kindly help re-run it. Thanks. |
Thanks for making those changes @ms-zhenhua. I took a closer look at this and even though we can manage this resource in Terraform we aren't able to actually connect it to a Kubernetes cluster which is the whole point of this exercise. I've raised an issue over on Azure/azure-cli-extensions#4756 regarding this and am going to mark this as Blocked for the time being. |
Hi @stephybun, I saw your question was answered in this issue. Could this PR be unblocked now? |
azurerm_arc_kubernetes_cluster
This comment was marked as off-topic.
This comment was marked as off-topic.
Temporarily close this PR to make some modifications. Will reopen it later. |
Hi @stephybun, I updated the PR to let it support installing the arc agent. I extracted the code for installing arc agent from Azure Cli extension and the code can be run in a Linux VM through remote-exec provisioner. Now all test cases can be run automatically. Could you kindly help take another review? Thanks. |
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.
Thanks @ms-zhenhua.
In addition to the comments left in-line I still have concerns about how a user can establish connectivity to the Arc resource provisioned through Terraform.
Although the logic to install the agent can be taken from the Azure CLI, this isn't documented publicly anywhere for us to reference and is left up to the user to go digging for it. Could we get this process documented on the MSFT learn site as well as add a valid example to the examples folder for this resource?
internal/services/arckubernetes/sdk/2021-10-01/hybridkubernetes/client.go
Outdated
Show resolved
Hide resolved
Hi @stephybun, your concerns are reasonable. In fact, there already exists a document for agent deployment. I have also created a requirement issue to ask service team to make the document easier for users to understand and will keep tracking it. I have also added a note to remind users to install agents when creating this resource and added an example to the example folder. |
This comment was marked as off-topic.
This comment was marked as off-topic.
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.
Thanks for making those updates and for adding the complete example @ms-zhenhua. A few final remarks but it looks like this will be good to go once those have been addressed.
internal/services/arckubernetes/arc_kubernetes_cluster_resource.go
Outdated
Show resolved
Hide resolved
internal/services/arckubernetes/arc_kubernetes_cluster_resource.go
Outdated
Show resolved
Hide resolved
Hi @stephybun, thank you for reviewing. I have updated the code and document. Kindly take another review. |
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.
Thanks for your effort on this @ms-zhenhua. LGTM 🥳
This functionality has been released in v3.51.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you! |
Fixes #12612
swagger: https://github.com/Azure/azure-rest-api-specs/blob/main/specification/hybridkubernetes/resource-manager/Microsoft.Kubernetes/stable/2021-10-01/connectedClusters.json
Test Case Result: