`azurerm_kubernetes_cluster` & `azurerm_kubernetes_cluster_node_pool`: Support for `node_labels` #5531

dintel · 2020-01-27T11:38:43Z

Add support for node_labels in azurerm_kubernetes_cluster and azurerm_kubernetes_cluster_node_pool resources.
Fixes #4968

mbfrahry

Hey @dintel, this looks good so far but I've made a couple minor suggestions. It'd also be good to add some tests to confirm that we can create, update, and remove them

mbfrahry · 2020-02-06T22:30:53Z

azurerm/internal/services/containers/data_source_kubernetes_cluster.go

@@ -176,6 +176,14 @@ func dataSourceArmKubernetesCluster() *schema.Resource {
 							Computed: true,
 						},

+						"node_labels": {
+							Type:     schema.TypeMap,
+							Optional: true,


It doesn't look like this is used to get the resource and as such should be swapped to

Suggested change

Optional: true,

Computed: true,

mbfrahry · 2020-02-06T22:40:21Z

azurerm/internal/services/containers/resource_arm_kubernetes_cluster.go

@@ -1230,6 +1238,10 @@ func expandKubernetesClusterAgentPoolProfiles(input []interface{}, isNewResource
 			return nil, fmt.Errorf("Can't create an AKS cluster with autoscaling enabled but not setting min_count or max_count")
 		}

+		if nodeLabels := utils.ExpandMapStringPtrString(config["node_labels"].(map[string]interface{})); len(nodeLabels) > 0 {


Do we want to empty node label struct to the api so people can remove the labels?

From my tests of Azure AKS API node labels can not be changed.
Whenever you try to alter node labels API accepts the update, but labels are never changed.

(as above) since the agent_pool_profile block is deprecated can we remove this? For the default_node_pool block we can make this ForceNew too - I'll leave a comment there

mbfrahry · 2020-02-06T22:40:55Z

azurerm/internal/services/containers/kubernetes_nodepool.go

@@ -286,6 +298,14 @@ func FlattenDefaultNodePool(input *[]containerservice.ManagedClusterAgentPoolPro
 		name = *agentPool.Name
 	}

+	var nodeLabels map[string]string
+	if agentPool.NodeLabels != nil {
+		nodeLabels = make(map[string]string)


Might be misreading here but can't we use your nifty flatten function here?

ExpandMapStringPtrString is almost the same, but it accepts map[string]interface{}, so it can not be used here.

azurerm/internal/services/containers/kubernetes_nodepool.go

mbfrahry · 2020-02-06T22:42:59Z

azurerm/utils/common_marshal.go

+	result := make(map[string]*string)
+	for k, v := range input {
+		s := v.(string)
+		result[k] = &s


I think we can consolidate this just a bit

Suggested change

result[k] = &s

result[k] = utils.String(v.(string))

mbfrahry · 2020-02-06T22:43:37Z

azurerm/utils/common_marshal.go

+func FlattenMapStringPtrString(input map[string]*string) map[string]interface{} {
+	result := make(map[string]interface{})
+	for k, v := range input {
+		result[k] = *v


We'll want to confirm that v isn't nil before pointing to it

tombuildsstuff

hey @dintel

Thanks for this PR / pushing those changes - I've taken a look through and left some other minor comments inline but this is otherwise looking good 👍

Thanks!

tombuildsstuff · 2020-02-10T07:24:17Z

azurerm/internal/services/containers/resource_arm_kubernetes_cluster.go

@@ -223,6 +223,14 @@ func resourceArmKubernetesCluster() *schema.Resource {
 							ForceNew: true,
 						},

+						"node_labels": {
+							Type:     schema.TypeMap,
+							Optional: true,


since these can't be updated we'll also need to make this:

Suggested change

Optional: true,

Optional: true,

ForceNew: true,

@tombuildsstuff since agent the agent_pool_profile block is deprecated, do not you want to drop it here too?

azurerm/internal/services/containers/kubernetes_nodepool.go

tombuildsstuff · 2020-02-10T07:26:13Z

azurerm/internal/services/containers/resource_arm_kubernetes_cluster_node_pool.go

+		nodeLabelsRaw := d.Get("node_labels").(map[string]interface{})
+		nodeLabels := utils.ExpandMapStringPtrString(nodeLabelsRaw)
+		props.NodeLabels = nodeLabels
+	}


since these can't be updated at this time could we remove this for the moment?

azurerm/internal/services/containers/resource_arm_kubernetes_cluster_node_pool.go

tombuildsstuff · 2020-02-10T07:55:03Z

azurerm/internal/services/containers/resource_arm_kubernetes_cluster.go

@@ -1230,6 +1238,10 @@ func expandKubernetesClusterAgentPoolProfiles(input []interface{}, isNewResource
 			return nil, fmt.Errorf("Can't create an AKS cluster with autoscaling enabled but not setting min_count or max_count")
 		}

+		if nodeLabels := utils.ExpandMapStringPtrString(config["node_labels"].(map[string]interface{})); len(nodeLabels) > 0 {


(as above) since the agent_pool_profile block is deprecated can we remove this? For the default_node_pool block we can make this ForceNew too - I'll leave a comment there

tombuildsstuff · 2020-02-10T07:56:33Z

website/docs/r/kubernetes_cluster.html.markdown

@@ -184,6 +184,8 @@ A `agent_pool_profile` block supports the following:

 * `max_pods` - (Optional) The maximum number of pods that can run on each agent. Changing this forces a new resource to be created.

+* `node_labels` - (Optional) A map of Kubernetes labels which should be applied to nodes in the agent pool (e.g `{"name": "value"}`).


(as above) since the agent_pool_profile block is deprecated could we remove this:

Suggested change

* `node_labels` - (Optional) A map of Kubernetes labels which should be applied to nodes in the agent pool (e.g `{"name": "value"}`).

tombuildsstuff · 2020-02-10T07:59:08Z

website/docs/r/kubernetes_cluster_node_pool.html.markdown

@@ -70,6 +70,8 @@ The following arguments are supported:

 * `max_pods` - (Optional) The maximum number of pods that can run on each agent. Changing this forces a new resource to be created.

+* `node_labels` - (Optional) A map of Kubernetes labels which should be applied to nodes in the agent pool (e.g `{"name": "value"}`).


could we make this:

Suggested change

* `node_labels` - (Optional) A map of Kubernetes labels which should be applied to nodes in the agent pool (e.g `{"name": "value"}`).

* `node_labels` - (Optional) A map of Kubernetes labels which should be applied to nodes in this Node Pool.

tombuildsstuff · 2020-02-10T08:03:39Z

website/docs/r/kubernetes_cluster.html.markdown

@@ -239,6 +241,8 @@ A `default_node_pool` block supports the following:

 * `max_pods` - (Optional) The maximum number of pods that can run on each agent. Changing this forces a new resource to be created.

+* `node_labels` - (Optional) A map of Kubernetes labels which should be applied to nodes in the agent pool (e.g `{"name": "value"}`).


could we make this:

Suggested change

* `node_labels` - (Optional) A map of Kubernetes labels which should be applied to nodes in the agent pool (e.g `{"name": "value"}`).

* `node_labels` - (Optional) A map of Kubernetes labels which should be applied to nodes in the Default Node Pool.

(since this is a Map users can imply it's:

foo = { Hello = "World" }

as such we can remove this example)

dintel · 2020-02-14T07:30:03Z

@tombuildsstuff @mbfrahry I made changes as you guys requested. Could you please review again?

…_kubernetes_cluster_node_pool resources hashicorp#4968

dintel · 2020-02-26T15:17:43Z

@tombuildsstuff @mbfrahry could you guys review this PR? I made some changes to reflect new version changes.

Fixing test

mbfrahry

Hey @dintel, I'm struggling to get a couple of the tests working.

--- FAIL: TestAccAzureRMKubernetesCluster_nodeLabels (80.00s)
    testing.go:640: Step 0 error: errors during apply:
        
        Error: Error creating Managed Kubernetes Cluster "acctestaks200304233511150885" (Resource Group "acctestRG-200304233511150885"): containerservice.ManagedClustersClient#CreateOrUpdate: Failure sending request: StatusCode=400 -- Original Error: Code="CanNotTaintAllAgentPools" Message="Operation failed due to insufficient nodes for system pod scheduling. Placing custom taints on all node pools is not supported. Retry the operation and leave at least one node pool untainted."

Looking at what we are sending to the api, it seems like the label is being appended to the taint attribute.

"nodeLabels": {},
"nodeTaints": ["key=value:PreferNoSchedule"],

And then the datasource config seems off

--- FAIL: TestAccDataSourceAzureRMKubernetesCluster_nodeLabels (0.02s)
    testing.go:640: Step 0 error: Missing item separator: On /opt/teamcity-agent/temp/buildTmp/tf-test852087197/main.tf line 19: Expected a comma to mark the beginning of the next item.

dintel · 2020-03-09T10:13:35Z

@mbfrahry Please check now.

mbfrahry

LGTM! Thanks for your taking the time to get this in

ghost · 2020-04-09T14:51:37Z

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 added size/M documentation labels Jan 27, 2020

dintel requested a review from tombuildsstuff January 27, 2020 11:44

tombuildsstuff requested review from a team and removed request for tombuildsstuff January 27, 2020 12:11

dintel force-pushed the master branch from 6c7bdb5 to f99aad1 Compare January 27, 2020 12:38

ghost added size/XXL dependencies and removed size/M labels Jan 27, 2020

dintel force-pushed the master branch 3 times, most recently from 6173692 to a6d9ef2 Compare January 27, 2020 13:24

ghost added size/XL and removed size/XXL labels Jan 27, 2020

dintel force-pushed the master branch from a6d9ef2 to 1477e75 Compare January 27, 2020 14:20

mbfrahry requested changes Feb 6, 2020

View reviewed changes

dintel force-pushed the master branch from 1477e75 to 0a1cc07 Compare February 7, 2020 13:07

tombuildsstuff added enhancement service/kubernetes-cluster labels Feb 10, 2020

tombuildsstuff suggested changes Feb 10, 2020

View reviewed changes

tombuildsstuff added the waiting-response label Feb 10, 2020

dintel force-pushed the master branch 4 times, most recently from 099f40a to 0c9a51b Compare February 13, 2020 20:30

ghost removed the waiting-response label Feb 14, 2020

dintel requested review from tombuildsstuff and mbfrahry February 19, 2020 19:27

Add support for node_labels in azurerm_kubernetes_cluster and azurerm…

01b0e37

…_kubernetes_cluster_node_pool resources hashicorp#4968

Add tests for kubernetes cluster node labels

7981e54

dintel force-pushed the master branch from 0c9a51b to 7981e54 Compare February 26, 2020 11:43

aLekSer mentioned this pull request Mar 3, 2020

AKS labels are not supported in the Terraform provider, wrong Controller placement googleforgames/agones#1383

Closed

Update resource_arm_kubernetes_cluster_node_pool_test.go

ad7b0de

Fixing test

mbfrahry requested changes Mar 5, 2020

View reviewed changes

katbyte added the waiting-response label Mar 5, 2020

Fix tests

4d0198a

dintel requested a review from mbfrahry March 9, 2020 10:13

ghost removed the waiting-response label Mar 9, 2020

Update resource_arm_kubernetes_cluster_test.go

80c7d5d

mbfrahry approved these changes Mar 10, 2020

View reviewed changes

mbfrahry changed the title ~~Add support of node_labels in AKS~~ azurerm_kubernetes_cluster & azurerm_kubernetes_cluster_node_pool: Support for node_labels Mar 10, 2020

mbfrahry merged commit 4c5ae77 into hashicorp:master Mar 10, 2020

mbfrahry added a commit that referenced this pull request Mar 10, 2020

Update changelog for #5531

13ca314

dstrebel mentioned this pull request Mar 11, 2020

Add support for nodepool-labels in azurerm_kubernetes_cluster_node_pool #6051

Closed

ghost locked and limited conversation to collaborators Apr 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`azurerm_kubernetes_cluster` & `azurerm_kubernetes_cluster_node_pool`: Support for `node_labels` #5531

`azurerm_kubernetes_cluster` & `azurerm_kubernetes_cluster_node_pool`: Support for `node_labels` #5531

dintel commented Jan 27, 2020 •

edited by tombuildsstuff

Loading

mbfrahry left a comment

mbfrahry Feb 6, 2020

mbfrahry Feb 6, 2020

dintel Feb 7, 2020 •

edited

Loading

tombuildsstuff Feb 10, 2020

mbfrahry Feb 6, 2020

dintel Feb 7, 2020

mbfrahry Feb 6, 2020

mbfrahry Feb 6, 2020

tombuildsstuff left a comment

tombuildsstuff Feb 10, 2020

dintel Feb 10, 2020

tombuildsstuff Feb 10, 2020

tombuildsstuff Feb 10, 2020

tombuildsstuff Feb 10, 2020

tombuildsstuff Feb 10, 2020

tombuildsstuff Feb 10, 2020

dintel commented Feb 14, 2020

dintel commented Feb 26, 2020

mbfrahry left a comment

dintel commented Mar 9, 2020

mbfrahry left a comment

ghost commented Apr 9, 2020

		@@ -184,6 +184,8 @@ A `agent_pool_profile` block supports the following:

		* `max_pods` - (Optional) The maximum number of pods that can run on each agent. Changing this forces a new resource to be created.

		* `node_labels` - (Optional) A map of Kubernetes labels which should be applied to nodes in the agent pool (e.g `{"name": "value"}`).

		@@ -70,6 +70,8 @@ The following arguments are supported:

		* `max_pods` - (Optional) The maximum number of pods that can run on each agent. Changing this forces a new resource to be created.

		* `node_labels` - (Optional) A map of Kubernetes labels which should be applied to nodes in the agent pool (e.g `{"name": "value"}`).

		@@ -239,6 +241,8 @@ A `default_node_pool` block supports the following:

		* `max_pods` - (Optional) The maximum number of pods that can run on each agent. Changing this forces a new resource to be created.

		* `node_labels` - (Optional) A map of Kubernetes labels which should be applied to nodes in the agent pool (e.g `{"name": "value"}`).

azurerm_kubernetes_cluster & azurerm_kubernetes_cluster_node_pool: Support for node_labels #5531

azurerm_kubernetes_cluster & azurerm_kubernetes_cluster_node_pool: Support for node_labels #5531

Conversation

dintel commented Jan 27, 2020 • edited by tombuildsstuff Loading

mbfrahry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dintel Feb 7, 2020 • edited Loading

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

tombuildsstuff left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dintel commented Feb 14, 2020

dintel commented Feb 26, 2020

mbfrahry left a comment

Choose a reason for hiding this comment

dintel commented Mar 9, 2020

mbfrahry left a comment

Choose a reason for hiding this comment

ghost commented Apr 9, 2020

`azurerm_kubernetes_cluster` & `azurerm_kubernetes_cluster_node_pool`: Support for `node_labels` #5531

`azurerm_kubernetes_cluster` & `azurerm_kubernetes_cluster_node_pool`: Support for `node_labels` #5531

dintel commented Jan 27, 2020 •

edited by tombuildsstuff

Loading

dintel Feb 7, 2020 •

edited

Loading