diff --git a/google-beta/resource_container_cluster.go b/google-beta/resource_container_cluster.go index 69af8dee70..3c881eaa74 100644 --- a/google-beta/resource_container_cluster.go +++ b/google-beta/resource_container_cluster.go @@ -10,6 +10,7 @@ import ( "github.com/hashicorp/errwrap" "github.com/hashicorp/go-version" + "github.com/hashicorp/terraform/helper/customdiff" "github.com/hashicorp/terraform/helper/resource" "github.com/hashicorp/terraform/helper/schema" "github.com/hashicorp/terraform/helper/validation" @@ -54,7 +55,10 @@ func resourceContainerCluster() *schema.Resource { Update: resourceContainerClusterUpdate, Delete: resourceContainerClusterDelete, - CustomizeDiff: resourceContainerClusterIpAllocationCustomizeDiff, + CustomizeDiff: customdiff.All( + resourceContainerClusterIpAllocationCustomizeDiff, + resourceNodeConfigEmptyGuestAccelerator, + ), Timeouts: &schema.ResourceTimeout{ Create: schema.DefaultTimeout(30 * time.Minute), @@ -753,6 +757,60 @@ func resourceContainerCluster() *schema.Resource { } } +// Setting a guest accelerator block to count=0 is the equivalent to omitting the block: it won't get +// sent to the API and it won't be stored in state. This diffFunc will try to compare the old + new state +// by only comparing the blocks with a positive count and ignoring those with count=0 +// +// One quirk with this approach is that configs with mixed count=0 and count>0 accelerator blocks will +// show a confusing diff if one of there are config changes that result in a legitimate diff as the count=0 +// blocks will not be in state. +// +// This could also be modelled by setting `guest_accelerator = []` in the config. However since the +// previous syntax requires that schema.SchemaConfigModeAttr is set on the field it is advisable that +// we have a work around for removing guest accelerators. Also Terraform 0.11 cannot use dynamic blocks +// so this isn't a solution for module authors who want to dynamically omit guest accelerators +// See https://github.com/terraform-providers/terraform-provider-google/issues/3786 +func resourceNodeConfigEmptyGuestAccelerator(diff *schema.ResourceDiff, meta interface{}) error { + old, new := diff.GetChange("node_config.0.guest_accelerator") + oList := old.([]interface{}) + nList := new.([]interface{}) + + if len(nList) == len(oList) || len(nList) == 0 { + return nil + } + var hasAcceleratorWithEmptyCount bool + // the list of blocks in a desired state with count=0 accelerator blocks in it + // will be longer than the current state. + // this index tracks the location of positive count accelerator blocks + index := 0 + for i, item := range nList { + accel := item.(map[string]interface{}) + if accel["count"].(int) == 0 { + hasAcceleratorWithEmptyCount = true + // Ignore any 'empty' accelerators because they aren't sent to the API + continue + } + if index >= len(oList) { + // Return early if there are more positive count accelerator blocks in the desired state + // than the current state since a difference in 'legit' blocks is a valid diff. + // This will prevent array index overruns + return nil + } + if !reflect.DeepEqual(nList[i], oList[index]) { + return nil + } + index += 1 + } + + if hasAcceleratorWithEmptyCount && index == len(oList) { + // If the number of count>0 blocks match, there are count=0 blocks present and we + // haven't already returned due to a legitimate diff + diff.Clear("node_config.0.guest_accelerator") + } + + return nil +} + func resourceContainerClusterIpAllocationCustomizeDiff(diff *schema.ResourceDiff, meta interface{}) error { // separate func to allow unit testing return resourceContainerClusterIpAllocationCustomizeDiffFunc(diff) diff --git a/google-beta/resource_container_node_pool.go b/google-beta/resource_container_node_pool.go index ee7fdf51e0..cad4daa354 100644 --- a/google-beta/resource_container_node_pool.go +++ b/google-beta/resource_container_node_pool.go @@ -6,6 +6,7 @@ import ( "strings" "time" + "github.com/hashicorp/terraform/helper/customdiff" "github.com/hashicorp/terraform/helper/resource" "github.com/hashicorp/terraform/helper/schema" "github.com/hashicorp/terraform/helper/validation" @@ -33,6 +34,10 @@ func resourceContainerNodePool() *schema.Resource { State: resourceContainerNodePoolStateImporter, }, + CustomizeDiff: customdiff.All( + resourceNodeConfigEmptyGuestAccelerator, + ), + Schema: mergeSchemas( schemaNodePool, map[string]*schema.Schema{ diff --git a/google-beta/resource_container_node_pool_test.go b/google-beta/resource_container_node_pool_test.go index 2a061c0ae3..4f4115920f 100644 --- a/google-beta/resource_container_node_pool_test.go +++ b/google-beta/resource_container_node_pool_test.go @@ -570,6 +570,53 @@ func TestAccContainerNodePool_012_ConfigModeAttr(t *testing.T) { }) } +func TestAccContainerNodePool_EmptyGuestAccelerator(t *testing.T) { + t.Parallel() + + cluster := fmt.Sprintf("tf-nodepool-test-%s", acctest.RandString(10)) + np := fmt.Sprintf("tf-nodepool-test-%s", acctest.RandString(10)) + + resource.Test(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testAccCheckContainerNodePoolDestroy, + Steps: []resource.TestStep{ + { + // Test alternative way to specify an empty node pool + Config: testAccContainerNodePool_EmptyGuestAccelerator(cluster, np), + }, + { + ResourceName: "google_container_node_pool.np", + ImportState: true, + ImportStateVerify: true, + ImportStateVerifyIgnore: []string{"max_pods_per_node"}, + }, + { + // Test alternative way to specify an empty node pool + Config: testAccContainerNodePool_PartialEmptyGuestAccelerator(cluster, np, 1), + }, + { + ResourceName: "google_container_node_pool.np", + ImportState: true, + ImportStateVerify: true, + ImportStateVerifyIgnore: []string{"max_pods_per_node"}, + }, + { + // Assert that changes in count from 1 result in a diff + Config: testAccContainerNodePool_PartialEmptyGuestAccelerator(cluster, np, 2), + ExpectNonEmptyPlan: true, + PlanOnly: true, + }, + { + // Assert that adding another accelerator block will also result in a diff + Config: testAccContainerNodePool_PartialEmptyGuestAccelerator2(cluster, np), + ExpectNonEmptyPlan: true, + PlanOnly: true, + }, + }, + }) +} + func testAccCheckContainerNodePoolDestroy(s *terraform.State) error { config := testAccProvider.Meta().(*Config) @@ -1163,3 +1210,87 @@ resource "google_container_node_pool" "np" { } }`, cluster, np) } + +func testAccContainerNodePool_EmptyGuestAccelerator(cluster, np string) string { + return fmt.Sprintf(` +resource "google_container_cluster" "cluster" { + name = "%s" + zone = "us-central1-f" + initial_node_count = 3 +} + +resource "google_container_node_pool" "np" { + name = "%s" + zone = "us-central1-f" + cluster = "${google_container_cluster.cluster.name}" + initial_node_count = 1 + + node_config { + guest_accelerator { + count = 0 + type = "nvidia-tesla-p100" + } + } +}`, cluster, np) +} + +func testAccContainerNodePool_PartialEmptyGuestAccelerator(cluster, np string, count int) string { + return fmt.Sprintf(` +resource "google_container_cluster" "cluster" { + name = "%s" + zone = "us-central1-f" + initial_node_count = 3 +} + +resource "google_container_node_pool" "np" { + name = "%s" + zone = "us-central1-f" + cluster = "${google_container_cluster.cluster.name}" + initial_node_count = 1 + + node_config { + guest_accelerator { + count = 0 + type = "nvidia-tesla-p100" + } + + guest_accelerator { + count = %d + type = "nvidia-tesla-p100" + } + } +}`, cluster, np, count) +} + +func testAccContainerNodePool_PartialEmptyGuestAccelerator2(cluster, np string) string { + return fmt.Sprintf(` +resource "google_container_cluster" "cluster" { + name = "%s" + zone = "us-central1-f" + initial_node_count = 3 +} + +resource "google_container_node_pool" "np" { + name = "%s" + zone = "us-central1-f" + cluster = "${google_container_cluster.cluster.name}" + initial_node_count = 1 + + node_config { + guest_accelerator { + count = 0 + type = "nvidia-tesla-p100" + } + + guest_accelerator { + count = 1 + type = "nvidia-tesla-p100" + } + + guest_accelerator { + count = 1 + type = "nvidia-tesla-p9000" + } + } +}`, cluster, np) +}