`azurerm_kubernetes_cluster` - fix tests #25200

ms-henglu · 2024-03-11T08:05:51Z

This PR fixes the tests related with this gh issue: #24020

ms-henglu · 2024-03-13T02:02:09Z

I believe the failed check is a false positive

stephybun · 2024-03-15T08:45:38Z

internal/services/containers/kubernetes_cluster_auth_resource_test.go

@@ -845,6 +848,9 @@ resource "azurerm_kubernetes_cluster" "test" {
    name       = "default"
    node_count = 1
    vm_size    = "Standard_DS2_v2"
+    upgrade_settings {
+      max_surge = "10%%"


Can you also update the default value for this property for 4.0?

The max_surge = 10% is only for the clusters created with k8s version = 1.28+, so I think we can't add the default value for this field.

The window for supported versions will continue to move so at some point clusters can only be created with k8s version 1.28+. I don't see what is preventing us from having a default value in place for that field for v4.0.

Is it correct 10%% with double %% ?

Yes, this syntax is required for % values in the test config

stephybun · 2024-03-15T14:09:51Z

internal/services/containers/kubernetes_cluster_auth_resource_test.go

@@ -142,6 +142,7 @@ func TestAccKubernetesCluster_roleBasedAccessControlDisabled(t *testing.T) {
 }

 func TestAccKubernetesCluster_roleBasedAccessControlAAD(t *testing.T) {
+	t.Skip("Azure AD Integration (legacy) (https://aka.ms/aks/aad-legacy) is deprecated, the cluster could not be created with the Azure AD integration (legacy) enabled.")


Should we also add a deprecation message to the affected properties?

The deprecated use case is specifying managed = false, so I think we couldn't add the deprecated message to the field.

refs: https://registry.terraform.io/providers/hashicorp/azurerm/latest/docs/resources/kubernetes_cluster#managed

If AKS doesn't support clusters where managed = false i.e. where client_app_id, server_app_id, server_app_secret can be specified for that use case, then those properties (including managed) should have deprecation messages and be removed in v4.0

Got it! I'll fix it in the next commit.

stephybun

Could you also put the removal of the deprecated properties behind the FourPointOhBeta flag and update the remaining deprecation messages with the suggestions below?

stephybun · 2024-03-20T06:35:12Z

internal/services/containers/kubernetes_cluster_resource.go

@@ -308,6 +308,7 @@ func resourceKubernetesCluster() *pluginsdk.Resource {
 				Elem: &pluginsdk.Resource{
 					Schema: map[string]*pluginsdk.Schema{
 						"client_app_id": {
+							Deprecated:   "Azure AD Integration (legacy) (https://aka.ms/aks/aad-legacy) is deprecated, the cluster could not be created with the Azure AD integration (legacy) enabled. This field will be removed in 4.0 release.",


Suggested change

Deprecated: "Azure AD Integration (legacy) (https://aka.ms/aks/aad-legacy) is deprecated, the cluster could not be created with the Azure AD integration (legacy) enabled. This field will be removed in 4.0 release.",

Deprecated: "Azure AD Integration (legacy) (https://aka.ms/aks/aad-legacy) is deprecated and clusters can no longer be created with the Azure AD integration (legacy) enabled. This field will be removed in v4.0 of the AzureRM Provider.",

stephybun

Looks like there are a lot of failing tests:

------- Stdout: -------
=== RUN   TestAccKubernetesCluster_roleBasedAccessControlAADManaged
=== PAUSE TestAccKubernetesCluster_roleBasedAccessControlAADManaged
=== CONT  TestAccKubernetesCluster_roleBasedAccessControlAADManaged
    testcase.go:113: Step 1/4 error: After applying this test step, the plan was not empty.
        stdout:
        Terraform used the selected providers to generate the following execution
        plan. Resource actions are indicated with the following symbols:
          ~ update in-place
        Terraform will perform the following actions:
          # azurerm_kubernetes_cluster.test will be updated in-place
          ~ resource "azurerm_kubernetes_cluster" "test" {
                id                                  = "/subscriptions/*******/resourceGroups/acctestRG-aks-240402104316331075/providers/Microsoft.ContainerService/managedClusters/acctestaks240402104316331075"
                name                                = "acctestaks240402104316331075"
                # (27 unchanged attributes hidden)
              ~ azure_active_directory_role_based_access_control {
                  + admin_group_object_ids = []
                    # (3 unchanged attributes hidden)
                }
              ~ default_node_pool {
                    name                         = "default"
                  + node_taints                  = []
                  + tags                         = {}
                  + zones                        = []
                    # (20 unchanged attributes hidden)
                  - upgrade_settings {
                      - max_surge = "10%" -> null
                    }
                }
              ~ identity {
                  + identity_ids = []
                    # (3 unchanged attributes hidden)
                }
                # (3 unchanged blocks hidden)
            }
        Plan: 0 to add, 1 to change, 0 to destroy.
--- FAIL: TestAccKubernetesCluster_roleBasedAccessControlAADManaged (661.41s)
FAIL

ms-henglu · 2024-04-08T07:12:19Z

Hi @stephybun , it seems that the default Kubernetes version is 1.28 now, so all tests need to be updated to add the max_surge parameter. Is it possible to make the upgrade_settings as a computed field? Or should I update all tests?

stephybun · 2024-04-08T07:44:46Z

@ms-henglu I'd prefer the tests updated over setting Computed on upgrade_settings

ms-henglu · 2024-04-09T02:00:56Z

I also opened this PR to fix the tests in the generated resources: hashicorp/pandora#4050

ms-henglu · 2024-04-15T02:55:55Z

Hi @stephybun , the conflicts are resolved.

stephybun

Thanks @ms-henglu LGTM 👍

…5581 #25405 #25594 #25533

<Actions> <action id="f410411e63aff4bb73a81c2aec1d373cf8a903e63b30dee2006b0030d8a94cc8"> <h3>Bump Terraform `azurerm` provider version</h3> <details id="1d9343c012f5434ac9fe8a98135bae3667b399259be16d9b14302ea3bd424a24"> <summary>Update Terraform lock file</summary> <p>changes detected:
	"hashicorp/azurerm" updated from "3.99.0" to "3.100.0" in file ".terraform.lock.hcl"</p> <details> <summary>3.100.0</summary> <pre>Changelog retrieved from:
	https://github.com/hashicorp/terraform-provider-azurerm/releases/tag/v3.100.0
ENHANCEMENTS:

* dependencies: updating `hashicorp/go-azure-sdk` to `v0.20240417.1084633` ([#25659](hashicorp/terraform-provider-azurerm#25659 `compute` - update Virtual Machine and Virtual Machine Scale Set resources and data sources to use `hashicorp/go-azure-sdk` ([#25533](hashicorp/terraform-provider-azurerm#25533 `machine_learning` - Add new `machine_learning` block that supports `purge_soft_deleted_workspace_on_destroy` ([#25624](hashicorp/terraform-provider-azurerm#25624 `loganalytics` - update cluster resource to use `hashicorp/go-azure-sdk` ([#23373](hashicorp/terraform-provider-azurerm#23373 Data Source: `azurerm_management_group` - now exports the `tenant_scoped_id` attribute ([#25555](hashicorp/terraform-provider-azurerm#25555 `azurerm_container_app` - the `ingress.ip_security_restriction.ip_address_range` property will now accept an IP address as valid input ([#25609](hashicorp/terraform-provider-azurerm#25609 `azurerm_container_group` - the `identity` block can now be updated ([#25543](hashicorp/terraform-provider-azurerm#25543 `azurerm_express_route_connection` - support for the `private_link_fast_path_enabled` property ([#25596](hashicorp/terraform-provider-azurerm#25596 `azurerm_hdinsight_hadoop_cluster` - support for the `private_link_configuration` block ([#25629](hashicorp/terraform-provider-azurerm#25629 `azurerm_hdinsight_hbase_cluster` - support for the `private_link_configuration` block ([#25629](hashicorp/terraform-provider-azurerm#25629 `azurerm_hdinsight_interactive_query_cluster` - support for the `private_link_configuration` block ([#25629](hashicorp/terraform-provider-azurerm#25629 `azurerm_hdinsight_kafka_cluster` - support for the `private_link_configuration` block ([#25629](hashicorp/terraform-provider-azurerm#25629 `azurerm_hdinsight_spark_cluster` - support for the `private_link_configuration` block ([#25629](hashicorp/terraform-provider-azurerm#25629 `azurerm_management_group` - now exports the `tenant_scoped_id` attribute ([#25555](hashicorp/terraform-provider-azurerm#25555 `azurerm_monitor_activity_log_alert` - support for the `location` property ([#25389](hashicorp/terraform-provider-azurerm#25389 `azurerm_mysql_flexible_server` - update validating regex for `sku_name` ([#25642](hashicorp/terraform-provider-azurerm#25642 `azurerm_postgresql_flexible_server` - support for the `GeoRestore` `create_mode` ([#25664](hashicorp/terraform-provider-azurerm#25664 `azurerm_virtual_network_gateway_connection` - support for the `private_link_fast_path_enabled` property ([#25650](hashicorp/terraform-provider-azurerm#25650 `azurerm_windows_web_app` - support for the `handler_mapping` block ([#25631](hashicorp/terraform-provider-azurerm#25631 `azurerm_windows_web_app_slot` - support for the `handler_mapping` block ([#25631](https://github.com/hashicorp/terraform-provider-azurerm/issues/25631))

BUG FIXES:

* storage: prevent a bug causing the second storage account key to be used for authentication instead of the first ([#25652](hashicorp/terraform-provider-azurerm#25652 `azurerm_active_directory_domain_service` - prevent an issue where `filtered_sync_enabled` was not being updated ([#25594](hashicorp/terraform-provider-azurerm#25594 `azurerm_application_insights` - add a state migration to fix the resource ID casing of Application Insights resources ([#25628](hashicorp/terraform-provider-azurerm#25628 `azurerm_function_app_hybrid_connection` - can now use relay resources created in a different resource group ([#25541](hashicorp/terraform-provider-azurerm#25541 `azurerm_kubernetes_cluster_node_pool` - prevent plan diff when the `windows_profile.outbound_nat_enabled` property is unset ([#25644](hashicorp/terraform-provider-azurerm#25644 `azurerm_machine_learning_compute_cluster` - fix location to point to parent resource for computes ([#25643](hashicorp/terraform-provider-azurerm#25643 `azurerm_machine_learning_compute_instance` - fix location to point to parent resource for computes ([#25643](hashicorp/terraform-provider-azurerm#25643 `azurerm_storage_account` - check replication type when evaluating support level for shares and queues for V1 storage accounts ([#25581](hashicorp/terraform-provider-azurerm#25581 `azurerm_storage_account` - added a sanity check for `dns_endpoint_type` and `blob_properties.restore_policy` ([#25450](hashicorp/terraform-provider-azurerm#25450 `azurerm_web_app_hybrid_connection` - can now use relay resources created in a different resource group ([#25541](hashicorp/terraform-provider-azurerm#25541 `azurerm_windows_web_app` - prevent removal of `site_config.application_stack.node_version` when `app_settings` are updated ([#25488](hashicorp/terraform-provider-azurerm#25488 `azurerm_windows_web_app_slot` - prevent removal of `site_config.application_stack.node_version` when `app_settings` are updated ([#25489](https://github.com/hashicorp/terraform-provider-azurerm/issues/25489))

DEPRECATIONS:

* `logz` - the Logz resources are deprecated and will be removed in v4.0 of the AzureRM Provider since the API no longer allows new instances to be created ([#25405](hashicorp/terraform-provider-azurerm#25405 `azurerm_machine_learning_compute_instance` - marked the `location` field as deprecated in v4.0 of the provider ([#25643](hashicorp/terraform-provider-azurerm#25643 `azurerm_kubernetes_cluster` - the following properties have been deprecated since the API no longer supports cluster creation with legacy Azure Entra integration: `client_app_id`, `server_app_id`, `server_app_secret` and `managed` ([#25200](https://github.com/hashicorp/terraform-provider-azurerm/issues/25200))


</pre> </details> </details> <a href="https://infra.ci.jenkins.io/job/updatecli/job/azure/job/main/117/">Jenkins pipeline link</a> </action> </Actions> --- <table> <tr> <td width="77"> <img src="https://www.updatecli.io/images/updatecli.png" alt="Updatecli logo" width="50" height="50"> </td> <td> <p> Created automatically by <a href="https://www.updatecli.io/">Updatecli</a> </p> <details><summary>Options:</summary> <br /> <p>Most of Updatecli configuration is done via <a href="https://www.updatecli.io/docs/prologue/quick-start/">its manifest(s)</a>.</p> <ul> <li>If you close this pull request, Updatecli will automatically reopen it, the next time it runs.</li> <li>If you close this pull request and delete the base branch, Updatecli will automatically recreate it, erasing all previous commits made.</li> </ul> <p> Feel free to report any issues at <a href="https://github.com/updatecli/updatecli/issues">github.com/updatecli/updatecli</a>.<br /> If you find this tool useful, do not hesitate to star <a href="https://github.com/updatecli/updatecli/stargazers">our GitHub repository</a> as a sign of appreciation, and/or to tell us directly on our <a href="https://matrix.to/#/#Updatecli_community:gitter.im">chat</a>! </p> </details> </td> </tr> </table> Co-authored-by: Jenkins Infra Bot (updatecli) <60776566+jenkins-infra-bot@users.noreply.github.com>

adamzeyinsurity · 2024-04-19T22:08:11Z

Perhaps I'm missing something, but what's the correct path to solve this warning message as a user? If you set managed = true, you get a warning. If you delete the field, it defaults to false, which as you'd expect throws errors. There doesn't appear to be any path to use the current AAD implementation without the warning message, users are required to set the field to true, and setting it to true produces a warning on every plan or apply.

There is also no mention in the documentation that the field defaults to false, or that it's even deprecated in the first place. This sent me down a rabbit hole of trying to figure out how to resolve the warning, only to realize that it can't be resolved and that everybody using AAD will just always get this warning until v4.0 lets us remove the field.

rdtechie · 2024-04-22T07:11:35Z

Perhaps I'm missing something, but what's the correct path to solve this warning message as a user? If you set managed = true, you get a warning. If you delete the field, it defaults to false, which as you'd expect throws errors. There doesn't appear to be any path to use the current AAD implementation without the warning message, users are required to set the field to true, and setting it to true produces a warning on every plan or apply.

There is also no mention in the documentation that the field defaults to false, or that it's even deprecated in the first place. This sent me down a rabbit hole of trying to figure out how to resolve the warning, only to realize that it can't be resolved and that everybody using AAD will just always get this warning until v4.0 lets us remove the field.

I second this. This is confusing for endusers (like me).

adamzeyinsurity · 2024-04-25T16:27:14Z

This warning is now being flagged in Visual Studio Code by the HashiCorp Terraform extension (with, again, no way to remove the warning).

github-actions · 2024-05-26T02:10:45Z

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

github-actions bot added size/M waiting-response and removed waiting-response labels Mar 11, 2024

ms-henglu marked this pull request as ready for review March 12, 2024 09:15

github-actions bot removed the waiting-response label Mar 13, 2024

stephybun requested changes Mar 15, 2024

View reviewed changes

stephybun added service/kubernetes-cluster tests labels Mar 15, 2024

github-actions bot added the waiting-response label Mar 15, 2024

stephybun reviewed Mar 15, 2024

View reviewed changes

github-actions bot added waiting-response and removed waiting-response labels Mar 19, 2024

stephybun reviewed Mar 20, 2024

View reviewed changes

github-actions bot added size/L waiting-response and removed waiting-response size/M labels Mar 20, 2024

ms-henglu added 6 commits March 20, 2024 12:20

azurerm_kubernetes_cluster - fix tests

1c29aa0

fix more

15e5271

add deprecated message for Azure AD Integration (legacy)

862b86f

add default value for max_surge

810ad36

update

2b0d7ca

update

1586c1b

stephybun force-pushed the issue-24020 branch from 0e969d5 to 1586c1b Compare March 20, 2024 11:22

github-actions bot removed waiting-response size/L labels Mar 20, 2024

github-actions bot removed the waiting-response label Apr 1, 2024

stephybun requested changes Apr 2, 2024

View reviewed changes

github-actions bot added the waiting-response label Apr 2, 2024

github-actions bot removed the waiting-response label Apr 8, 2024

update more tests

89db74d

github-actions bot added the waiting-response label Apr 8, 2024

github-actions bot removed the waiting-response label Apr 9, 2024

update more tests

8c10e6c

github-actions bot added the waiting-response label Apr 9, 2024

Merge branch 'main' into issue-24020

303bbfe

github-actions bot added waiting-response and removed waiting-response labels Apr 15, 2024

github-actions bot removed the waiting-response label Apr 15, 2024

stephybun approved these changes Apr 15, 2024

View reviewed changes

stephybun merged commit 99c6b37 into hashicorp:main Apr 15, 2024
29 of 30 checks passed

github-actions bot added this to the v3.100.0 milestone Apr 15, 2024

stephybun added a commit that referenced this pull request Apr 16, 2024

Update for #25389 #25596 #25628 #25609 #25450 #25541 #25200 #25543 #2…

ffd0273

…5581 #25405 #25594 #25533

ms-henglu mentioned this pull request Apr 17, 2024

azurerm_kubernetes_cluster_node_pool - fix windows_profile.outbound_nat_enabled plan-diff #25644

Merged

14 tasks

adamzeyinsurity mentioned this pull request Apr 25, 2024

AKS block azure_active_directory_role_based_access_control always produces a warning due to managed field being both required and deprecated #25756

Closed

1 task

ms-henglu mentioned this pull request May 7, 2024

dependencies: updating containerservice to 2023-09-02-preview #25663

Merged

14 tasks

github-actions bot locked as resolved and limited conversation to collaborators May 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`azurerm_kubernetes_cluster` - fix tests #25200

`azurerm_kubernetes_cluster` - fix tests #25200

ms-henglu commented Mar 11, 2024 •

edited

Loading

ms-henglu commented Mar 13, 2024

stephybun Mar 15, 2024

ms-henglu Mar 18, 2024

stephybun Mar 18, 2024

zioproto Mar 27, 2024

stephybun Mar 28, 2024

stephybun Mar 15, 2024

ms-henglu Mar 18, 2024 •

edited

Loading

stephybun Mar 18, 2024

ms-henglu Mar 19, 2024

stephybun left a comment •

edited

Loading

stephybun Mar 20, 2024

stephybun left a comment

ms-henglu commented Apr 8, 2024

stephybun commented Apr 8, 2024

ms-henglu commented Apr 9, 2024

ms-henglu commented Apr 15, 2024

stephybun left a comment

adamzeyinsurity commented Apr 19, 2024 •

edited

Loading

rdtechie commented Apr 22, 2024

adamzeyinsurity commented Apr 25, 2024

github-actions bot commented May 26, 2024

	Deprecated: "Azure AD Integration (legacy) (https://aka.ms/aks/aad-legacy) is deprecated, the cluster could not be created with the Azure AD integration (legacy) enabled. This field will be removed in 4.0 release.",
	Deprecated: "Azure AD Integration (legacy) (https://aka.ms/aks/aad-legacy) is deprecated and clusters can no longer be created with the Azure AD integration (legacy) enabled. This field will be removed in v4.0 of the AzureRM Provider.",

azurerm_kubernetes_cluster - fix tests #25200

azurerm_kubernetes_cluster - fix tests #25200

Conversation

ms-henglu commented Mar 11, 2024 • edited Loading

ms-henglu commented Mar 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ms-henglu Mar 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephybun left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephybun left a comment

Choose a reason for hiding this comment

ms-henglu commented Apr 8, 2024

stephybun commented Apr 8, 2024

ms-henglu commented Apr 9, 2024

ms-henglu commented Apr 15, 2024

stephybun left a comment

Choose a reason for hiding this comment

adamzeyinsurity commented Apr 19, 2024 • edited Loading

rdtechie commented Apr 22, 2024

adamzeyinsurity commented Apr 25, 2024

github-actions bot commented May 26, 2024

`azurerm_kubernetes_cluster` - fix tests #25200

`azurerm_kubernetes_cluster` - fix tests #25200

ms-henglu commented Mar 11, 2024 •

edited

Loading

ms-henglu Mar 18, 2024 •

edited

Loading

stephybun left a comment •

edited

Loading

adamzeyinsurity commented Apr 19, 2024 •

edited

Loading