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.

Sign up for GitHub

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

Jump to bottom

new resource azurerm_arc_kubernetes_provisioned_cluster_instance #27143

Open
wants to merge 22 commits into
base: main
Choose a base branch
from
Open

new resource azurerm_arc_kubernetes_provisioned_cluster_instance #27143

wants to merge 22 commits into from
+6,288 −9

Conversation

teowa
Copy link
Contributor

@teowa teowa commented Aug 22, 2024 •
edited
Loading

Community Note

  • Please vote on this PR by adding a 👍 reaction to the original PR to help the community and maintainers prioritize for review
  • Please do not leave comments along the lines of "+1", "me too" or "any updates", they generate extra noise for PR followers and do not help prioritize for review

Description

pending on #26916

new resource azurerm_arc_kubernetes_provisioned_cluster_instance

swagger: https://github.com/Azure/azure-rest-api-specs/blob/main/specification/hybridaks/resource-manager/Microsoft.HybridContainerService/stable/2024-01-01/provisionedClusterInstances.json

doc: https://learn.microsoft.com/en-us/azure/aks/hybrid/aks-create-clusters-cli?toc=%2Fazure-stack%2Fhci%2Ftoc.json&bc=%2Fazure-stack%2Fbreadcrumb%2Ftoc.json

PR Checklist

  • I have followed the guidelines in our Contributing Documentation.
  • I have checked to ensure there aren't other open Pull Requests for the same update/change.
  • I have checked if my changes close any open issues. If so please include appropriate closing keywords below.
  • I have updated/added Documentation as required written in a helpful and kind way to assist users that may be unfamiliar with the resource / data source.
  • I have used a meaningful PR title to help maintainers and other users understand this change and help prevent duplicate work.
    For example: “resource_name_here - description of change e.g. adding property new_property_name_here”

Changes to existing Resource / Data Source

  • I have added an explanation of what my changes do and why I'd like you to include them (This may be covered by linking to an issue above, but may benefit from additional explanation).
  • I have written new tests for my resource or datasource changes & updated any relevent documentation.
  • I have successfully run tests with my changes locally. If not, please provide details on testing challenges that prevented you running the tests.
  • (For changes that include a state migration only). I have manually tested the migration path between relevant versions of the provider.

Testing

  • My submission includes Test coverage as described in the Contribution Guide and the tests pass. (if this is not possible for any reason, please include details of why you did or could not add test coverage)

Tests depend on an existing custom location provisioned during HCI deployment

ARM_TEST_LOCATION=eastus ARM_TEST_STACK_HCI_CUSTOM_LOCATION_ID=/subscriptions/xxx/resourceGroups/acctest-hci-0828/providers/Microsoft.ExtendedLocation/customLocations/customlocation0828 make acctests SERVICE="arckubernetes" TESTARGS="-parallel 5 -run TestAccArcKubernetesProvisionedClusterInstance" TESTTIMEOUT=5h
==> Checking that code complies with gofmt requirements...
==> Checking that Custom Timeouts are used...
==> Checking that acceptance test packages are used...
TF_ACC=1 go test -v ./internal/services/arckubernetes -parallel 5 -run TestAccArcKubernetesProvisionedClusterInstance -timeout 5h -ldflags="-X=github.com/hashicorp/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccArcKubernetesProvisionedClusterInstance
=== RUN   TestAccArcKubernetesProvisionedClusterInstance/complete
=== RUN   TestAccArcKubernetesProvisionedClusterInstance/update
=== RUN   TestAccArcKubernetesProvisionedClusterInstance/requiresImport
=== RUN   TestAccArcKubernetesProvisionedClusterInstance/basic
--- PASS: TestAccArcKubernetesProvisionedClusterInstance (6168.82s)
    --- PASS: TestAccArcKubernetesProvisionedClusterInstance/complete (1257.68s)
    --- PASS: TestAccArcKubernetesProvisionedClusterInstance/update (3052.38s)
    --- PASS: TestAccArcKubernetesProvisionedClusterInstance/requiresImport (833.29s)
    --- PASS: TestAccArcKubernetesProvisionedClusterInstance/basic (1025.45s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/arckubernetes 6168.854s

Change Log

Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.

  • new resource azurerm_arc_kubernetes_provisioned_cluster_instance

This is a (please select all that apply):

  • Bug Fix
  • New Feature (ie adding a service, resource, or data source)
  • Enhancement
  • Breaking Change

Related Issue(s)

Fixes #0000

Note

If this PR changes meaningfully during the course of review please update the title and description as required.

@teowa teowa marked this pull request as draft August 24, 2024 02:24
@lukasosterheider
Copy link

Hi @teowa,
will this or your other PR enable the creation of a new Kubernetes cluster on an Azure Stack HCI cluster?
Your linked documentation (below) says so, but I am not sure.

Thank you!
Lukas

https://learn.microsoft.com/en-us/azure/aks/hybrid/aks-create-clusters-cli?toc=%2Fazure-stack%2Fhci%2Ftoc.json&bc=%2Fazure-stack%2Fbreadcrumb%2Ftoc.json

ms-zhenhua
ms-zhenhua suggested changes Sep 3, 2024
View reviewed changes
Copy link
Contributor

@ms-zhenhua ms-zhenhua left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @teowa,

Thanks for this PR - I've taken a look through and left some comments inline. If we can fix those up, this should be good to go 👍

internal/services/arckubernetes/arc_kubernetes_provisioned_cluster_resource.go Outdated
}

func (ArcKubernetesProvisionedClusterInstanceResource) ResourceType() string {
return "azurerm_arc_kubernetes_provisioned_cluster_instance"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be azurerm_arc_kubernetes_provisioned_cluster ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed

internal/services/arckubernetes/arc_kubernetes_provisioned_cluster_resource.go Outdated
ValidateFunc: validation.StringIsNotEmpty,
},
},
"os_sku": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what sku will be used if os_sku is not configured?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed to required

internal/services/arckubernetes/arc_kubernetes_provisioned_cluster_resource.go Outdated
MaxItems: 1,
Elem: &pluginsdk.Resource{
Schema: map[string]*pluginsdk.Schema{
"authorized_ip_ranges": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why it's not Required since it's the only sub property of cluster_vm_access_profile ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed to required

internal/services/arckubernetes/arc_kubernetes_provisioned_cluster_resource.go Outdated
ValidateFunc: validation.IsIPv4Address,
ConflictsWith: []string{"network_profile.0.load_balancer_profile"},
},
"vm_size": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please put Required fields ahead of Optional ones

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

internal/services/arckubernetes/arc_kubernetes_provisioned_cluster_resource.go Outdated
},
},

"kubernetes_version": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please put Required fields ahead of Optional ones

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

website/docs/r/arc_kubernetes_provisioned_cluster_instance.html.markdown Outdated

A `linux_profile` block supports the following:

* `ssh_key` - (Required) A list of certificate public key used to authenticate with VMs through SSH. The certificate must be in PEM format with or without headers. Changing this forces a new Arc Kubernetes Provisioned Cluster Instance to be created.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* `ssh_key` - (Required) A list of certificate public key used to authenticate with VMs through SSH. The certificate must be in PEM format with or without headers. Changing this forces a new Arc Kubernetes Provisioned Cluster Instance to be created.
* `ssh_key` - (Required) A list of certificate public keys used to authenticate with VMs through SSH. The certificate must be in PEM format with or without headers. Changing this forces a new Arc Kubernetes Provisioned Cluster Instance to be created.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

website/docs/r/arc_kubernetes_provisioned_cluster_instance.html.markdown Outdated

A `network_profile` block supports the following:

* `network_policy` - (Required) Network policy used for building Kubernetes network. Possible value is `calico`. Changing this forces a new Arc Kubernetes Provisioned Cluster Instance to be created.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* `network_policy` - (Required) Network policy used for building Kubernetes network. Possible value is `calico`. Changing this forces a new Arc Kubernetes Provisioned Cluster Instance to be created.
* `network_policy` - (Required) The network policy used for building Kubernetes network. The only possible value is `calico`. Changing this forces a new Arc Kubernetes Provisioned Cluster Instance to be created.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

website/docs/r/arc_kubernetes_provisioned_cluster_instance.html.markdown Outdated

* `pod_cidr` - (Required) A CIDR notation IP Address range from which to assign pod IPs. Changing this forces a new Arc Kubernetes Provisioned Cluster Instance to be created.

* `load_balancer_profile` - (Optional) A `load_balancer_profile` block as defined above.Conflicts with `control_plane_profile.0.host_ip`. Changing this forces a new Arc Kubernetes Provisioned Cluster Instance to be created.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* `load_balancer_profile` - (Optional) A `load_balancer_profile` block as defined above.Conflicts with `control_plane_profile.0.host_ip`. Changing this forces a new Arc Kubernetes Provisioned Cluster Instance to be created.
* `load_balancer_profile` - (Optional) A `load_balancer_profile` block as defined above. Changing this forces a new Arc Kubernetes Provisioned Cluster Instance to be created.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

website/docs/r/arc_kubernetes_provisioned_cluster_instance.html.markdown Outdated

A `storage_profile` block supports the following:

* `nfs_csi_driver_enabled` - (Optional) Should the NFS CSI Driver be enabled? Defaults to `true`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* `nfs_csi_driver_enabled` - (Optional) Should the NFS CSI Driver be enabled? Defaults to `true`.
* `nfs_csi_driver_enabled` - (Optional) Whether to enable the NFS CSI Driver. Defaults to `true`.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

website/docs/r/arc_kubernetes_provisioned_cluster_instance.html.markdown Outdated

* `nfs_csi_driver_enabled` - (Optional) Should the NFS CSI Driver be enabled? Defaults to `true`.

* `smb_csi_driver_enabled` - (Optional) Should the SMB CSI Driver be enabled? Defaults to `true`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* `smb_csi_driver_enabled` - (Optional) Should the SMB CSI Driver be enabled? Defaults to `true`.
* `smb_csi_driver_enabled` - (Optional) Whether to enable the SMB CSI Driver. Defaults to `true`.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

@teowa
Copy link
Contributor Author

teowa commented Sep 3, 2024

Hi @teowa, will this or your other PR enable the creation of a new Kubernetes cluster on an Azure Stack HCI cluster? Your linked documentation (below) says so, but I am not sure.

Thank you! Lukas

https://learn.microsoft.com/en-us/azure/aks/hybrid/aks-create-clusters-cli?toc=%2Fazure-stack%2Fhci%2Ftoc.json&bc=%2Fazure-stack%2Fbreadcrumb%2Ftoc.json

Hi @lukasosterheider , yes, refer to the template from https://github.com/Azure/Edge-infrastructure-quickstart-template/blob/main/modules/aks-arc/main.tf.
The AKS on HCI is composed with two resources, one is "Microsoft.Kubernetes/connectedClusters@2024-01-01" with kind ProvisionedCluster, the #26916 is going to support this, another is "Microsoft.HybridContainerService/provisionedClusterInstances@2024-01-01" in this PR.

@lukasosterheider
Copy link

Hi @teowa, will this or your other PR enable the creation of a new Kubernetes cluster on an Azure Stack HCI cluster? Your linked documentation (below) says so, but I am not sure.
Thank you! Lukas
https://learn.microsoft.com/en-us/azure/aks/hybrid/aks-create-clusters-cli?toc=%2Fazure-stack%2Fhci%2Ftoc.json&bc=%2Fazure-stack%2Fbreadcrumb%2Ftoc.json

Hi @lukasosterheider , yes, refer to the template from https://github.com/Azure/Edge-infrastructure-quickstart-template/blob/main/modules/aks-arc/main.tf. The AKS on HCI is composed with two resources, one is "Microsoft.Kubernetes/connectedClusters@2024-01-01" with kind ProvisionedCluster, the #26916 is going to support this, another is "Microsoft.HybridContainerService/provisionedClusterInstances@2024-01-01" in this PR.

Great - thank you! :)

@teowa teowa changed the title new resource azurerm_arc_kubernetes_provisioned_cluster_instance new resource azurerm_arc_kubernetes_provisioned_cluster Sep 3, 2024
@teowa
Copy link
Contributor Author

teowa commented Sep 4, 2024

image

ms-zhenhua
ms-zhenhua approved these changes Sep 9, 2024
View reviewed changes
Copy link
Contributor

@ms-zhenhua ms-zhenhua left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @teowa ,

Thank you for your updates. LGTM~

@lukasosterheider
Copy link

Hi @teowa,
as both PRs got approval from @ms-zhenhua, do you have already an idea of the ETA for both this and the #26916 PR?

Just looking very much forward to utilizing these resources :)

@katbyte
Copy link
Collaborator

katbyte commented Oct 17, 2024

@teowa - is this pr readyf or review?

@teowa
Copy link
Contributor Author

teowa commented Oct 21, 2024 •
edited
Loading

@katbyte , the PR is ready, however the resource depends on PR #26916.

@teowa teowa marked this pull request as ready for review October 21, 2024 06:32
@teowa teowa requested review from katbyte and a team as code owners October 21, 2024 06:32
@teowa
Copy link
Contributor Author

teowa commented Oct 24, 2024 •
edited
Loading

image

log

@teowa teowa mentioned this pull request Dec 5, 2024
Closed
14 tasks
@teowa teowa changed the title new resource azurerm_arc_kubernetes_provisioned_cluster new resource azurerm_arc_kubernetes_provisioned_cluster_instance Dec 9, 2024
@stephybun stephybun self-assigned this Dec 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ms-zhenhua ms-zhenhua ms-zhenhua approved these changes

@katbyte katbyte Awaiting requested review from katbyte

At least 1 approving review is required to merge this pull request.

Assignees

@stephybun stephybun

Labels
dependencies documentation new-resource size/XL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@teowa @lukasosterheider @katbyte @ms-zhenhua @stephybun @rcskosir