[Terraform] fixes to updating node versions (#1247)

Merged PR #1247.
GoogleCloudPlatform · Jan 18, 2019 · 79e732c · 79e732c
1 parent 7112e6c
commit 79e732c
Show file tree

Hide file tree

Showing 6 changed files with 146 additions and 54 deletions.
diff --git a/build/terraform b/build/terraform
diff --git a/build/terraform-beta b/build/terraform-beta
diff --git a/third_party/terraform/resources/resource_container_cluster.go.erb b/third_party/terraform/resources/resource_container_cluster.go.erb
@@ -1004,56 +1004,6 @@ func resourceContainerClusterUpdate(d *schema.ResourceData, meta interface{}) er
 		d.SetPartial("master_authorized_networks_config")
 	}
 
-	// The master must be updated before the nodes
-	if d.HasChange("min_master_version") {
-		desiredMasterVersion := d.Get("min_master_version").(string)
-		currentMasterVersion := d.Get("master_version").(string)
-		des, err := version.NewVersion(desiredMasterVersion)
-		if err != nil {
-			return err
-		}
-		cur, err := version.NewVersion(currentMasterVersion)
-		if err != nil {
-			return err
-		}
-
-		// Only upgrade the master if the current version is lower than the desired version
-		if cur.LessThan(des) {
-			req := &containerBeta.UpdateClusterRequest{
-				Update: &containerBeta.ClusterUpdate{
-					DesiredMasterVersion: desiredMasterVersion,
-				},
-			}
-
-			updateF := updateFunc(req, "updating GKE master version")
-			// Call update serially.
-			if err := lockedCall(lockKey, updateF); err != nil {
-				return err
-			}
-			log.Printf("[INFO] GKE cluster %s: master has been updated to %s", d.Id(), desiredMasterVersion)
-		}
-		d.SetPartial("min_master_version")
-	}
-
-	if d.HasChange("node_version") {
-		desiredNodeVersion := d.Get("node_version").(string)
-		req := &containerBeta.UpdateClusterRequest{
-			Update: &containerBeta.ClusterUpdate{
-				DesiredNodeVersion: desiredNodeVersion,
-			},
-		}
-
-		updateF := updateFunc(req, "updating GKE node version")
-		// Call update serially.
-		if err := lockedCall(lockKey, updateF); err != nil {
-			return err
-		}
-		log.Printf("[INFO] GKE cluster %s: nodes have been updated to %s", d.Id(),
-			desiredNodeVersion)
-
-		d.SetPartial("node_version")
-	}
-
 	if d.HasChange("addons_config") {
 		if ac, ok := d.GetOk("addons_config"); ok {
 			req := &containerBeta.UpdateClusterRequest{
@@ -1353,6 +1303,71 @@ func resourceContainerClusterUpdate(d *schema.ResourceData, meta interface{}) er
 		d.SetPartial("node_pool")
 	}
 
+	// The master must be updated before the nodes
+	if d.HasChange("min_master_version") {
+		desiredMasterVersion := d.Get("min_master_version").(string)
+		currentMasterVersion := d.Get("master_version").(string)
+		des, err := version.NewVersion(desiredMasterVersion)
+		if err != nil {
+			return err
+		}
+		cur, err := version.NewVersion(currentMasterVersion)
+		if err != nil {
+			return err
+		}
+
+		// Only upgrade the master if the current version is lower than the desired version
+		if cur.LessThan(des) {
+			req := &containerBeta.UpdateClusterRequest{
+				Update: &containerBeta.ClusterUpdate{
+					DesiredMasterVersion: desiredMasterVersion,
+				},
+			}
+
+			updateF := updateFunc(req, "updating GKE master version")
+			// Call update serially.
+			if err := lockedCall(lockKey, updateF); err != nil {
+				return err
+			}
+			log.Printf("[INFO] GKE cluster %s: master has been updated to %s", d.Id(), desiredMasterVersion)
+		}
+		d.SetPartial("min_master_version")
+	}
+
+	// It's not super important that this come after updating the node pools, but it still seems like a better
+	// idea than doing it before.
+	if d.HasChange("node_version") {
+		foundDefault := false
+		if n, ok := d.GetOk("node_pool.#"); ok {
+			for i := 0; i < n.(int); i++ {
+				key := fmt.Sprintf("node_pool.%d.", i)
+				if d.Get(key+"name").(string) == "default-pool" {
+					desiredNodeVersion := d.Get("node_version").(string)
+					req := &containerBeta.UpdateClusterRequest{
+						Update: &containerBeta.ClusterUpdate{
+							DesiredNodeVersion: desiredNodeVersion,
+							DesiredNodePoolId:  "default-pool",
+						},
+					}
+					updateF := updateFunc(req, "updating GKE default node pool node version")
+					// Call update serially.
+					if err := lockedCall(lockKey, updateF); err != nil {
+						return err
+					}
+					log.Printf("[INFO] GKE cluster %s: default node pool has been updated to %s", d.Id(),
+						desiredNodeVersion)
+					foundDefault = true
+				}
+			}
+		}
+
+		if !foundDefault {
+			return fmt.Errorf("node_version was updated but default-pool was not found. To update the version for a non-default pool, use the version attribute on that pool.")
+		}
+
+		d.SetPartial("node_version")
+	}
+
 	if d.HasChange("node_config") {
 		if d.HasChange("node_config.0.image_type") {
 			it := d.Get("node_config.0.image_type").(string)

diff --git a/third_party/terraform/resources/resource_container_node_pool.go.erb b/third_party/terraform/resources/resource_container_node_pool.go.erb
@@ -713,7 +713,7 @@ func nodePoolUpdate(d *schema.ResourceData, meta interface{}, nodePoolInfo *Node
 	if d.HasChange(prefix + "version") {
 		req := &containerBeta.UpdateNodePoolRequest{
 			NodePoolId:  name,
-			NodeVersion: d.Get("version").(string),
+			NodeVersion: d.Get(prefix + "version").(string),
 		}
 		updateF := func() error {
 			op, err := config.clientContainerBeta.Projects.

diff --git a/third_party/terraform/tests/resource_container_cluster_test.go.erb b/third_party/terraform/tests/resource_container_cluster_test.go.erb
@@ -989,6 +989,41 @@ func TestAccContainerCluster_withNodePoolBasic(t *testing.T) {
 	})
 }
 
+func TestAccContainerCluster_withNodePoolUpdateVersion(t *testing.T) {
+	t.Parallel()
+
+	clusterName := fmt.Sprintf("tf-cluster-nodepool-test-%s", acctest.RandString(10))
+	npName := fmt.Sprintf("tf-cluster-nodepool-test-%s", acctest.RandString(10))
+
+	resource.Test(t, resource.TestCase{
+		PreCheck:     func() { testAccPreCheck(t) },
+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckContainerClusterDestroy,
+		Steps: []resource.TestStep{
+			{
+				Config: testAccContainerCluster_withNodePoolLowerVersion(clusterName, npName),
+			},
+			{
+				ResourceName:            "google_container_cluster.with_node_pool",
+				ImportStateIdPrefix:     "us-central1-a/",
+				ImportState:             true,
+				ImportStateVerify:       true,
+				ImportStateVerifyIgnore: []string{"min_master_version"},
+			},
+			{
+				Config: testAccContainerCluster_withNodePoolUpdateVersion(clusterName, npName),
+			},
+			{
+				ResourceName:            "google_container_cluster.with_node_pool",
+				ImportStateIdPrefix:     "us-central1-a/",
+				ImportState:             true,
+				ImportStateVerify:       true,
+				ImportStateVerifyIgnore: []string{"min_master_version"},
+			},
+		},
+	})
+}
+
 func TestAccContainerCluster_withNodePoolResize(t *testing.T) {
 	t.Parallel()
 
@@ -2339,6 +2374,46 @@ resource "google_container_cluster" "with_node_pool" {
 }`, cluster, nodePool)
 }
 
+func testAccContainerCluster_withNodePoolLowerVersion(cluster, nodePool string) string {
+	return fmt.Sprintf(`
+data "google_container_engine_versions" "central1a" {
+	zone = "us-central1-a"
+}
+
+resource "google_container_cluster" "with_node_pool" {
+	name = "%s"
+	zone = "us-central1-a"
+
+	min_master_version = "${data.google_container_engine_versions.central1a.valid_master_versions.1}"
+
+	node_pool {
+		name               = "%s"
+		initial_node_count = 2
+		version            = "${data.google_container_engine_versions.central1a.valid_node_versions.2}"
+	}
+}`, cluster, nodePool)
+}
+
+func testAccContainerCluster_withNodePoolUpdateVersion(cluster, nodePool string) string {
+	return fmt.Sprintf(`
+data "google_container_engine_versions" "central1a" {
+	zone = "us-central1-a"
+}
+
+resource "google_container_cluster" "with_node_pool" {
+	name = "%s"
+	zone = "us-central1-a"
+
+	min_master_version = "${data.google_container_engine_versions.central1a.valid_master_versions.1}"
+
+	node_pool {
+		name               = "%s"
+		initial_node_count = 2
+		version            = "${data.google_container_engine_versions.central1a.valid_node_versions.1}"
+	}
+}`, cluster, nodePool)
+}
+
 func testAccContainerCluster_withNodePoolAdditionalZones(cluster, nodePool string) string {
 	return fmt.Sprintf(`
 resource "google_container_cluster" "with_node_pool" {

diff --git a/third_party/terraform/website/docs/r/container_cluster.html.markdown b/third_party/terraform/website/docs/r/container_cluster.html.markdown
@@ -176,7 +176,9 @@ to the datasource. A `region` can have a different set of supported versions tha
 
 * `node_version` - (Optional) The Kubernetes version on the nodes. Must either be unset
     or set to the same value as `min_master_version` on create. Defaults to the default
-    version set by GKE which is not necessarily the latest version.
+    version set by GKE which is not necessarily the latest version. This only affects
+    nodes in the default node pool. To update nodes in other node pools, use the `version`
+    attribute on the node pool.
 
 * `pod_security_policy_config` - (Optional, [Beta](https://terraform.io/docs/providers/google/provider_versions.html)) Configuration for the
     [PodSecurityPolicy](https://cloud.google.com/kubernetes-engine/docs/how-to/pod-security-policies) feature.