From c760e00b6ad25d79a2dd715ef449691043c6380c Mon Sep 17 00:00:00 2001 From: jackofallops Date: Wed, 15 Jan 2020 17:10:00 +0000 Subject: [PATCH 1/5] added functionality to update RBAC AAD Profile without rebuild of cluster --- .../resource_arm_kubernetes_cluster.go | 38 ++++--- ...source_arm_kubernetes_cluster_auth_test.go | 103 ++++++++++++++++++ 2 files changed, 127 insertions(+), 14 deletions(-) diff --git a/azurerm/internal/services/containers/resource_arm_kubernetes_cluster.go b/azurerm/internal/services/containers/resource_arm_kubernetes_cluster.go index eee3f945fa68..d0e86457273b 100644 --- a/azurerm/internal/services/containers/resource_arm_kubernetes_cluster.go +++ b/azurerm/internal/services/containers/resource_arm_kubernetes_cluster.go @@ -436,7 +436,7 @@ func resourceArmKubernetesCluster() *schema.Resource { Type: schema.TypeList, Optional: true, Computed: true, - ForceNew: true, + //ForceNew: true, MaxItems: 1, Elem: &schema.Resource{ Schema: map[string]*schema.Schema{ @@ -448,27 +448,23 @@ func resourceArmKubernetesCluster() *schema.Resource { "azure_active_directory": { Type: schema.TypeList, Optional: true, - ForceNew: true, MaxItems: 1, Elem: &schema.Resource{ Schema: map[string]*schema.Schema{ "client_app_id": { Type: schema.TypeString, Required: true, - ForceNew: true, ValidateFunc: validate.UUID, }, "server_app_id": { Type: schema.TypeString, Required: true, - ForceNew: true, ValidateFunc: validate.UUID, }, "server_app_secret": { Type: schema.TypeString, - ForceNew: true, Required: true, Sensitive: true, ValidateFunc: validate.NoEmptyStrings, @@ -478,7 +474,6 @@ func resourceArmKubernetesCluster() *schema.Resource { Type: schema.TypeString, Optional: true, Computed: true, - ForceNew: true, // OrEmpty since this can be sourced from the client config if it's not specified ValidateFunc: validate.UUIDOrEmpty, }, @@ -789,6 +784,29 @@ func resourceArmKubernetesClusterUpdate(d *schema.ResourceData, meta interface{} // since there's multiple reasons why we could be called into Update, we use this to only update if something's changed that's not SP/Version updateCluster := false + // RBAC profile updates need to be handled atomically before any call to createUpdate as a diff there will create a PropertyChangeNotAllowed error + if d.HasChange("role_based_access_control") { + rbacRaw := d.Get("role_based_access_control").([]interface{}) + rbacEnabled, azureADProfile := expandKubernetesClusterRoleBasedAccessControl(rbacRaw, tenantId) + // changing rbacEnabled must still force cluster recreation + if *existing.ManagedClusterProperties.EnableRBAC == rbacEnabled { + existing.ManagedClusterProperties.AadProfile = azureADProfile + existing.ManagedClusterProperties.EnableRBAC = utils.Bool(rbacEnabled) + + log.Printf("[DEBUG] Updating the RBAC AAD profile") + future, err := clusterClient.ResetAADProfile(ctx, resourceGroup, name, *existing.ManagedClusterProperties.AadProfile) + if err != nil { + return fmt.Errorf("Error updating Managed Kubernetes Cluster AAD Profile in cluster %q (Resource Group %q): %+v", name, resourceGroup, err) + } + + if err = future.WaitForCompletionRef(ctx, clusterClient.Client); err != nil { + return fmt.Errorf("Error waiting for update of RBAC AAD profile of Managed Cluster %q (Resource Group %q):, %+v", name, resourceGroup, err) + } + } else { + updateCluster = true + } + } + if d.HasChange("addon_profile") { updateCluster = true addOnProfilesRaw := d.Get("addon_profile").([]interface{}) @@ -826,14 +844,6 @@ func resourceArmKubernetesClusterUpdate(d *schema.ResourceData, meta interface{} existing.ManagedClusterProperties.NetworkProfile = networkProfile } - if d.HasChange("role_based_access_control") { - updateCluster = true - rbacRaw := d.Get("role_based_access_control").([]interface{}) - rbacEnabled, azureADProfile := expandKubernetesClusterRoleBasedAccessControl(rbacRaw, tenantId) - existing.ManagedClusterProperties.AadProfile = azureADProfile - existing.ManagedClusterProperties.EnableRBAC = utils.Bool(rbacEnabled) - } - if d.HasChange("tags") { updateCluster = true t := d.Get("tags").(map[string]interface{}) diff --git a/azurerm/internal/services/containers/tests/resource_arm_kubernetes_cluster_auth_test.go b/azurerm/internal/services/containers/tests/resource_arm_kubernetes_cluster_auth_test.go index bd12298ba569..04f5d92a7c96 100644 --- a/azurerm/internal/services/containers/tests/resource_arm_kubernetes_cluster_auth_test.go +++ b/azurerm/internal/services/containers/tests/resource_arm_kubernetes_cluster_auth_test.go @@ -183,6 +183,60 @@ func testAccAzureRMKubernetesCluster_roleBasedAccessControlAAD(t *testing.T) { }) } +func TestAccAzureRMKubernetesCluster_updateRoleBasedAccessControlAAD(t *testing.T) { + checkIfShouldRunTestsIndividually(t) + testAccAzureRMKubernetesCluster_updateRoleBaseAccessControlAAD(t) +} + +func testAccAzureRMKubernetesCluster_updateRoleBaseAccessControlAAD(t *testing.T) { + data := acceptance.BuildTestData(t, "azurerm_kubernetes_cluster", "test") + clientId := os.Getenv("ARM_CLIENT_ID") + clientSecret := os.Getenv("ARM_CLIENT_SECRET") + tenantId := os.Getenv("ARM_TENANT_ID") + // Currently this only tests a change of password on the aadProfile + // TODO: find or create a suitable replacement client_id to use to extend the test and set ARM_CLIENT_ID_ALT in the CI job + updateClientId := os.Getenv("ARM_CLIENT_ID_ALT") + updateClientSecret := os.Getenv("ARM_CLIENT_SECRET_ALT") + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { acceptance.PreCheck(t) }, + Providers: acceptance.SupportedProviders, + CheckDestroy: testCheckAzureRMKubernetesClusterDestroy, + Steps: []resource.TestStep{ + { + Config: testAccAzureRMKubernetesCluster_roleBasedAccessControlAADConfig(data, clientId, clientSecret, tenantId), + Check: resource.ComposeTestCheckFunc( + testCheckAzureRMKubernetesClusterExists(data.ResourceName), + resource.TestCheckResourceAttr(data.ResourceName, "role_based_access_control.#", "1"), + resource.TestCheckResourceAttr(data.ResourceName, "role_based_access_control.0.enabled", "true"), + resource.TestCheckResourceAttr(data.ResourceName, "role_based_access_control.0.azure_active_directory.#", "1"), + resource.TestCheckResourceAttrSet(data.ResourceName, "role_based_access_control.0.azure_active_directory.0.client_app_id"), + resource.TestCheckResourceAttrSet(data.ResourceName, "role_based_access_control.0.azure_active_directory.0.server_app_id"), + resource.TestCheckResourceAttrSet(data.ResourceName, "role_based_access_control.0.azure_active_directory.0.server_app_secret"), + resource.TestCheckResourceAttrSet(data.ResourceName, "role_based_access_control.0.azure_active_directory.0.tenant_id"), + resource.TestCheckResourceAttr(data.ResourceName, "kube_admin_config.#", "1"), + resource.TestCheckResourceAttrSet(data.ResourceName, "kube_admin_config_raw"), + ), + }, + { + Config: testAccAzureRMKubernetesCluster_updateRoleBasedAccessControlAADConfig(data, clientId, clientSecret, updateClientId, updateClientSecret, tenantId), + Check: resource.ComposeTestCheckFunc( + testCheckAzureRMKubernetesClusterExists(data.ResourceName), + resource.TestCheckResourceAttr(data.ResourceName, "role_based_access_control.#", "1"), + resource.TestCheckResourceAttr(data.ResourceName, "role_based_access_control.0.enabled", "true"), + resource.TestCheckResourceAttr(data.ResourceName, "role_based_access_control.0.azure_active_directory.#", "1"), + resource.TestCheckResourceAttr(data.ResourceName, "role_based_access_control.0.azure_active_directory.0.client_app_id", updateClientId), + resource.TestCheckResourceAttr(data.ResourceName, "role_based_access_control.0.azure_active_directory.0.server_app_id", updateClientId), + resource.TestCheckResourceAttr(data.ResourceName, "role_based_access_control.0.azure_active_directory.0.server_app_secret", updateClientSecret), + resource.TestCheckResourceAttr(data.ResourceName, "role_based_access_control.0.azure_active_directory.0.tenant_id", tenantId), + resource.TestCheckResourceAttr(data.ResourceName, "kube_admin_config.#", "1"), + resource.TestCheckResourceAttrSet(data.ResourceName, "kube_admin_config_raw"), + ), + }, + }, + }) +} + func testAccAzureRMKubernetesCluster_apiServerAuthorizedIPRangesConfig(data acceptance.TestData, clientId string, clientSecret string) string { return fmt.Sprintf(` resource "azurerm_resource_group" "test" { @@ -388,3 +442,52 @@ resource "azurerm_kubernetes_cluster" "test" { } `, tenantId, data.RandomInteger, data.Locations.Primary, data.RandomInteger, data.RandomInteger, data.RandomInteger, clientId, clientSecret, clientId, clientSecret, clientId) } +func testAccAzureRMKubernetesCluster_updateRoleBasedAccessControlAADConfig(data acceptance.TestData, clientId, clientSecret, altClientId, altClientSecret, tenantId string) string { + return fmt.Sprintf(` +variable "tenant_id" { + default = "%s" +} + +resource "azurerm_resource_group" "test" { + name = "acctestRG-%d" + location = "%s" +} + +resource "azurerm_kubernetes_cluster" "test" { + name = "acctestaks%d" + location = "${azurerm_resource_group.test.location}" + resource_group_name = "${azurerm_resource_group.test.name}" + dns_prefix = "acctestaks%d" + + linux_profile { + admin_username = "acctestuser%d" + + ssh_key { + key_data = "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQCqaZoyiz1qbdOQ8xEf6uEu1cCwYowo5FHtsBhqLoDnnp7KUTEBN+L2NxRIfQ781rxV6Iq5jSav6b2Q8z5KiseOlvKA/RF2wqU0UPYqQviQhLmW6THTpmrv/YkUCuzxDpsH7DUDhZcwySLKVVe0Qm3+5N2Ta6UYH3lsDf9R9wTP2K/+vAnflKebuypNlmocIvakFWoZda18FOmsOoIVXQ8HWFNCuw9ZCunMSN62QGamCe3dL5cXlkgHYv7ekJE15IA9aOJcM7e90oeTqo+7HTcWfdu0qQqPWY5ujyMw/llas8tsXY85LFqRnr3gJ02bAscjc477+X+j/gkpFoN1QEmt terraform@demo.tld" + } + } + + default_node_pool { + name = "default" + node_count = 1 + vm_size = "Standard_DS2_v2" + } + + service_principal { + client_id = "%s" + client_secret = "%s" + } + + role_based_access_control { + enabled = true + + azure_active_directory { + server_app_id = "%s" + server_app_secret = "%s" + client_app_id = "%s" + tenant_id = var.tenant_id + } + } +} +`, tenantId, data.RandomInteger, data.Locations.Primary, data.RandomInteger, data.RandomInteger, data.RandomInteger, clientId, clientSecret, altClientId, altClientSecret, altClientId) +} From 2c85864b82e652825d9c8d47f0fbb55d30821e25 Mon Sep 17 00:00:00 2001 From: jackofallops Date: Wed, 15 Jan 2020 17:15:44 +0000 Subject: [PATCH 2/5] removed commented out Forcenew --- .../services/containers/resource_arm_kubernetes_cluster.go | 1 - 1 file changed, 1 deletion(-) diff --git a/azurerm/internal/services/containers/resource_arm_kubernetes_cluster.go b/azurerm/internal/services/containers/resource_arm_kubernetes_cluster.go index d0e86457273b..799317ebae02 100644 --- a/azurerm/internal/services/containers/resource_arm_kubernetes_cluster.go +++ b/azurerm/internal/services/containers/resource_arm_kubernetes_cluster.go @@ -436,7 +436,6 @@ func resourceArmKubernetesCluster() *schema.Resource { Type: schema.TypeList, Optional: true, Computed: true, - //ForceNew: true, MaxItems: 1, Elem: &schema.Resource{ Schema: map[string]*schema.Schema{ From 16992a29968132e4c85ba8b1587d6149a5b04184 Mon Sep 17 00:00:00 2001 From: jackofallops Date: Wed, 15 Jan 2020 17:18:27 +0000 Subject: [PATCH 3/5] removed out of date comment --- .../tests/resource_arm_kubernetes_cluster_auth_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/azurerm/internal/services/containers/tests/resource_arm_kubernetes_cluster_auth_test.go b/azurerm/internal/services/containers/tests/resource_arm_kubernetes_cluster_auth_test.go index 04f5d92a7c96..caf89cb57962 100644 --- a/azurerm/internal/services/containers/tests/resource_arm_kubernetes_cluster_auth_test.go +++ b/azurerm/internal/services/containers/tests/resource_arm_kubernetes_cluster_auth_test.go @@ -193,7 +193,6 @@ func testAccAzureRMKubernetesCluster_updateRoleBaseAccessControlAAD(t *testing.T clientId := os.Getenv("ARM_CLIENT_ID") clientSecret := os.Getenv("ARM_CLIENT_SECRET") tenantId := os.Getenv("ARM_TENANT_ID") - // Currently this only tests a change of password on the aadProfile // TODO: find or create a suitable replacement client_id to use to extend the test and set ARM_CLIENT_ID_ALT in the CI job updateClientId := os.Getenv("ARM_CLIENT_ID_ALT") updateClientSecret := os.Getenv("ARM_CLIENT_SECRET_ALT") From 57afb50979c0e6995e1547596e2e9a4fae966e3a Mon Sep 17 00:00:00 2001 From: jackofallops Date: Fri, 17 Jan 2020 10:34:59 +0000 Subject: [PATCH 4/5] Added import test steps --- .../tests/resource_arm_kubernetes_cluster_auth_test.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/azurerm/internal/services/containers/tests/resource_arm_kubernetes_cluster_auth_test.go b/azurerm/internal/services/containers/tests/resource_arm_kubernetes_cluster_auth_test.go index caf89cb57962..9ed8b5c2de43 100644 --- a/azurerm/internal/services/containers/tests/resource_arm_kubernetes_cluster_auth_test.go +++ b/azurerm/internal/services/containers/tests/resource_arm_kubernetes_cluster_auth_test.go @@ -217,6 +217,10 @@ func testAccAzureRMKubernetesCluster_updateRoleBaseAccessControlAAD(t *testing.T resource.TestCheckResourceAttrSet(data.ResourceName, "kube_admin_config_raw"), ), }, + data.ImportStep( + "service_principal.0.client_secret", + "role_based_access_control.0.azure_active_directory.0.server_app_secret", + ), { Config: testAccAzureRMKubernetesCluster_updateRoleBasedAccessControlAADConfig(data, clientId, clientSecret, updateClientId, updateClientSecret, tenantId), Check: resource.ComposeTestCheckFunc( @@ -232,6 +236,10 @@ func testAccAzureRMKubernetesCluster_updateRoleBaseAccessControlAAD(t *testing.T resource.TestCheckResourceAttrSet(data.ResourceName, "kube_admin_config_raw"), ), }, + data.ImportStep( + "service_principal.0.client_secret", + "role_based_access_control.0.azure_active_directory.0.server_app_secret", + ), }, }) } From 8eb16c2c48ca791c2cc4fa5f5e7b5a22b88c7828 Mon Sep 17 00:00:00 2001 From: jackofallops Date: Fri, 17 Jan 2020 10:48:58 +0000 Subject: [PATCH 5/5] added nil check for current enableRBAC, updated docs --- .../resource_arm_kubernetes_cluster.go | 13 +++++++++---- website/docs/r/kubernetes_cluster.html.markdown | 16 +++++++++------- 2 files changed, 18 insertions(+), 11 deletions(-) diff --git a/azurerm/internal/services/containers/resource_arm_kubernetes_cluster.go b/azurerm/internal/services/containers/resource_arm_kubernetes_cluster.go index 799317ebae02..ee48956fbca0 100644 --- a/azurerm/internal/services/containers/resource_arm_kubernetes_cluster.go +++ b/azurerm/internal/services/containers/resource_arm_kubernetes_cluster.go @@ -785,15 +785,20 @@ func resourceArmKubernetesClusterUpdate(d *schema.ResourceData, meta interface{} // RBAC profile updates need to be handled atomically before any call to createUpdate as a diff there will create a PropertyChangeNotAllowed error if d.HasChange("role_based_access_control") { + props := existing.ManagedClusterProperties + // check if we can determine current EnableRBAC state - don't do anything destructive if we can't be sure + if props.EnableRBAC == nil { + return fmt.Errorf("Error reading current state of RBAC Enabled, expected bool got %+v", props.EnableRBAC) + } rbacRaw := d.Get("role_based_access_control").([]interface{}) rbacEnabled, azureADProfile := expandKubernetesClusterRoleBasedAccessControl(rbacRaw, tenantId) // changing rbacEnabled must still force cluster recreation - if *existing.ManagedClusterProperties.EnableRBAC == rbacEnabled { - existing.ManagedClusterProperties.AadProfile = azureADProfile - existing.ManagedClusterProperties.EnableRBAC = utils.Bool(rbacEnabled) + if *props.EnableRBAC == rbacEnabled { + props.AadProfile = azureADProfile + props.EnableRBAC = utils.Bool(rbacEnabled) log.Printf("[DEBUG] Updating the RBAC AAD profile") - future, err := clusterClient.ResetAADProfile(ctx, resourceGroup, name, *existing.ManagedClusterProperties.AadProfile) + future, err := clusterClient.ResetAADProfile(ctx, resourceGroup, name, *props.AadProfile) if err != nil { return fmt.Errorf("Error updating Managed Kubernetes Cluster AAD Profile in cluster %q (Resource Group %q): %+v", name, resourceGroup, err) } diff --git a/website/docs/r/kubernetes_cluster.html.markdown b/website/docs/r/kubernetes_cluster.html.markdown index 7047ff33cf91..f420c64a59fa 100644 --- a/website/docs/r/kubernetes_cluster.html.markdown +++ b/website/docs/r/kubernetes_cluster.html.markdown @@ -103,7 +103,9 @@ The following arguments are supported: -> **NOTE:** Azure requires that a new, non-existent Resource Group is used, as otherwise the provisioning of the Kubernetes Service will fail. -* `role_based_access_control` - (Optional) A `role_based_access_control` block. Changing this forces a new resource to be created. +* `role_based_access_control` - (Optional) A `role_based_access_control` block. + +-> **NOTE:** Adding this block to, or removing it from, an existing cluster configuration will recreate the cluster. * `tags` - (Optional) A mapping of tags to assign to the resource. @@ -121,7 +123,7 @@ A `aci_connector_linux` block supports the following: * `subnet_name` - (Optional) The subnet name for the virtual nodes to run. This is required when `aci_connector_linux` `enabled` argument is set to `true`. --> **Note:** AKS will add a delegation to the subnet named here. To prevent further runs from failing you should make sure that the subnet you create for virtual nodes has a delegation, like so. +-> **NOTE:** AKS will add a delegation to the subnet named here. To prevent further runs from failing you should make sure that the subnet you create for virtual nodes has a delegation, like so. ``` resource "azurerm_subnet" "virtual" { @@ -200,13 +202,13 @@ A `agent_pool_profile` block supports the following: A `azure_active_directory` block supports the following: -* `client_app_id` - (Required) The Client ID of an Azure Active Directory Application. Changing this forces a new resource to be created. +* `client_app_id` - (Required) The Client ID of an Azure Active Directory Application. -* `server_app_id` - (Required) The Server ID of an Azure Active Directory Application. Changing this forces a new resource to be created. +* `server_app_id` - (Required) The Server ID of an Azure Active Directory Application. -* `server_app_secret` - (Required) The Server Secret of an Azure Active Directory Application. Changing this forces a new resource to be created. +* `server_app_secret` - (Required) The Server Secret of an Azure Active Directory Application. -* `tenant_id` - (Optional) The Tenant ID used for Azure Active Directory Application. If this isn't specified the Tenant ID of the current Subscription is used. Changing this forces a new resource to be created. +* `tenant_id` - (Optional) The Tenant ID used for Azure Active Directory Application. If this isn't specified the Tenant ID of the current Subscription is used. --- @@ -325,7 +327,7 @@ A `oms_agent` block supports the following: A `role_based_access_control` block supports the following: -* `azure_active_directory` - (Optional) An `azure_active_directory` block. Changing this forces a new resource to be created. +* `azure_active_directory` - (Optional) An `azure_active_directory` block. * `enabled` - (Required) Is Role Based Access Control Enabled? Changing this forces a new resource to be created.