Add support for renamed services in google_project_service* resources. (

#2424)

Merged PR #2424.

third_party/terraform/resources/resource_google_project_service.go

@@ -17,6 +17,40 @@ var ignoredProjectServices = []string{"dataproc-control.googleapis.com", "source
 // so don't bother storing them in the config or using them for diffing.
 var ignoredProjectServicesSet = golangSetFromStringSlice(ignoredProjectServices)
 
+// Service Renames
+// we expect when a service is renamed:
+// - both service names will continue to be able to be set
+// - setting one will effectively enable the other as a dependent
+// - GET will return whichever service name is requested
+// - LIST responses will not contain the old service name
+// renames may be reverted, though, so we should canonicalise both ways until
+// the old service is fully removed from the provider
+//
+// We handle service renames in the provider by pretending that we've read both
+// the old and new service names from the API if we see either, and only setting
+// the one(s) that existed in prior state in config (if any). If neither exists,
+// we'll set the new service name in state.
+// Additionally, in case of service rename rollbacks or unexpected early
+// removals of services, if we fail to create or delete a service that's been
+// renamed we'll retry using an alternate name.
+// We try creation by the user-specified value followed by the other value.
+// We try deletion by the old value followed by the new value.
+
+// map from old -> new names of services that have been renamed
+// these should be removed during major provider versions. comment here with
+// "DEPRECATED FOR {{version}} next to entries slated for removal in {{version}}
+// upon removal, we should disallow the old name from being used even if it's
+// not gone from the underlying API yet
+var renamedServices = map[string]string{
+	"bigquery-json.googleapis.com": "bigquery.googleapis.com", // DEPRECATED FOR 3.0.0
+}
+
+// renamedServices in reverse (new -> old)
+var renamedServicesByNewServiceNames = reverseStringMap(renamedServices)
+
+// renamedServices expressed as both old -> new and new -> old
+var renamedServicesByOldAndNewServiceNames = mergeStringMaps(renamedServices, renamedServicesByNewServiceNames)
+
 func resourceGoogleProjectService() *schema.Resource {
 	return &schema.Resource{
 		Create: resourceGoogleProjectServiceCreate,
@@ -82,7 +116,7 @@ func resourceGoogleProjectServiceCreate(d *schema.ResourceData, meta interface{}
 	}
 
 	srv := d.Get("service").(string)
-	err = BatchRequestEnableServices([]string{srv}, project, d, config)
+	err = BatchRequestEnableServices(map[string]struct{}{srv: {}}, project, d, config)
 	if err != nil {
 		return err
 	}

third_party/terraform/resources/resource_google_project_services.go

@@ -8,6 +8,7 @@ import (
 	"google.golang.org/api/googleapi"
 	"google.golang.org/api/serviceusage/v1"
 	"log"
+	"strings"
 	"time"
 )
 
@@ -90,10 +91,35 @@ func resourceGoogleProjectServicesRead(d *schema.ResourceData, meta interface{})
 	if err != nil {
 		return err
 	}
+
+	// use old services to set the correct renamed service names in state
+	s, _ := expandServiceUsageProjectServicesServices(d.Get("services"), d, config)
+	log.Printf("[DEBUG] Saw services in state on Read: %s ", s)
+	sset := golangSetFromStringSlice(s)
+	for ov, nv := range renamedServices {
+		_, ook := sset[ov]
+		_, nok := sset[nv]
+
+		// preserve the values set in prior state if they're identical. If none
+		// were set, we  delete the new value if it exists. By doing that that
+		// we only store the old value if the service is enabled, and no value
+		// if it isn't.
+		if ook && nok {
+			continue
+		} else if ook {
+			delete(enabledSet, nv)
+		} else if nok {
+			delete(enabledSet, ov)
+		} else {
+			delete(enabledSet, nv)
+		}
+	}
+
 	services := stringSliceFromGolangSet(enabledSet)
 
 	d.Set("project", d.Id())
 	d.Set("services", flattenServiceUsageProjectServicesServices(services, d))
+
 	return nil
 }
 
@@ -132,25 +158,59 @@ func setServiceUsageProjectEnabledServices(services []string, project string, d
 		return err
 	}
 
-	toEnable := make([]string, 0, len(services))
+	toEnable := map[string]struct{}{}
 	for _, srv := range services {
 		// We don't have to enable a service if it's already enabled.
 		if _, ok := currentlyEnabled[srv]; !ok {
-			toEnable = append(toEnable, srv)
+			toEnable[srv] = struct{}{}
 		}
 	}
 
-	if err := BatchRequestEnableServices(toEnable, project, d, config); err != nil {
-		return fmt.Errorf("unable to enable Project Services %s (%+v): %s", project, services, err)
+	if len(toEnable) > 0 {
+		log.Printf("[DEBUG] Enabling services: %s", toEnable)
+		if err := BatchRequestEnableServices(toEnable, project, d, config); err != nil {
+			return fmt.Errorf("unable to enable Project Services %s (%+v): %s", project, services, err)
+		}
+	} else {
+		log.Printf("[DEBUG] No services to enable.")
 	}
 
 	srvSet := golangSetFromStringSlice(services)
+
+	srvSetWithRenames := map[string]struct{}{}
+
+	// we'll always list both names for renamed services, so allow both forms if
+	// we see both.
+	for k := range srvSet {
+		srvSetWithRenames[k] = struct{}{}
+		if v, ok := renamedServicesByOldAndNewServiceNames[k]; ok {
+			srvSetWithRenames[v] = struct{}{}
+		}
+	}
+
 	for srv := range currentlyEnabled {
 		// Disable any services that are currently enabled for project but are not
 		// in our list of acceptable services.
-		if _, ok := srvSet[srv]; !ok {
+		if _, ok := srvSetWithRenames[srv]; !ok {
+			// skip deleting services by their new names and prefer the old name.
+			if _, ok := renamedServicesByNewServiceNames[srv]; ok {
+				continue
+			}
+
 			log.Printf("[DEBUG] Disabling project %s service %s", project, srv)
-			if err := disableServiceUsageProjectService(srv, project, d, config, true); err != nil {
+			err := disableServiceUsageProjectService(srv, project, d, config, true)
+			if err != nil {
+				log.Printf("[DEBUG] Saw error %s deleting service %s", err, srv)
+
+				// if we got the right error and the service is renamed, delete by the new name
+				if n, ok := renamedServices[srv]; ok && strings.Contains(err.Error(), "not found or permission denied.") {
+					log.Printf("[DEBUG] Failed to delete service %s, it doesn't exist. Trying %s", srv, n)
+					err = disableServiceUsageProjectService(n, project, d, config, true)
+					if err == nil {
+						return nil
+					}
+				}
+
 				return fmt.Errorf("unable to disable unwanted Project Service %s %s): %s", project, srv, err)
 			}
 		}
@@ -222,6 +282,9 @@ func expandServiceUsageProjectServicesServices(v interface{}, d TerraformResourc
 }
 
 // Retrieve a project's services from the API
+// if a service has been renamed, this function will list both the old and new
+// forms of the service. LIST responses are expected to return only the old or
+// new form, but we'll always return both.
 func listCurrentlyEnabledServices(project string, config *Config, timeout time.Duration) (map[string]struct{}, error) {
 	// Verify project for services still exists
 	p, err := config.clientResourceManager.Projects.Get(project).Do()
@@ -246,10 +309,19 @@ func listCurrentlyEnabledServices(project string, config *Config, timeout time.D
 			Filter("state:ENABLED").
 			Pages(ctx, func(r *serviceusage.ListServicesResponse) error {
 				for _, v := range r.Services {
-					// services are returned as "projects/PROJECT/services/NAME"
+					// services are returned as "projects/{{project}}/services/{{name}}"
 					name := GetResourceNameFromSelfLink(v.Name)
+
+					// if name not in ignoredProjectServicesSet
 					if _, ok := ignoredProjectServicesSet[name]; !ok {
 						apiServices[name] = struct{}{}
+
+						// if a service has been renamed, set both. We'll deal
+						// with setting the right values later.
+						if v, ok := renamedServicesByOldAndNewServiceNames[name]; ok {
+							log.Printf("[DEBUG] Adding service alias for %s to enabled services: %s", name, v)
+							apiServices[v] = struct{}{}
+						}
 					}
 				}
 				return nil

third_party/terraform/tests/resource_google_project_service_test.go

@@ -132,6 +132,28 @@ func TestAccProjectService_handleNotFound(t *testing.T) {
 	})
 }
 
+func TestAccProjectService_renamedService(t *testing.T) {
+	t.Parallel()
+
+	org := getTestOrgFromEnv(t)
+	pid := "terraform-" + acctest.RandString(10)
+	resource.Test(t, resource.TestCase{
+		PreCheck:  func() { testAccPreCheck(t) },
+		Providers: testAccProviders,
+		Steps: []resource.TestStep{
+			{
+				Config: testAccProjectService_single("bigquery-json.googleapis.com", pid, pname, org),
+			},
+			{
+				ResourceName:            "google_project_service.test",
+				ImportState:             true,
+				ImportStateVerify:       true,
+				ImportStateVerifyIgnore: []string{"disable_on_destroy", "disable_dependent_services"},
+			},
+		},
+	})
+}
+
 func testAccCheckProjectService(services []string, pid string, expectEnabled bool) resource.TestCheckFunc {
 	return func(s *terraform.State) error {
 		config := testAccProvider.Meta().(*Config)
@@ -270,3 +292,21 @@ resource "google_project_service" "test" {
 }
 `, pid, service)
 }
+
+func testAccProjectService_single(service string, pid, name, org string) string {
+	return fmt.Sprintf(`
+resource "google_project" "acceptance" {
+  project_id = "%s"
+  name       = "%s"
+  org_id     = "%s"
+}
+
+resource "google_project_service" "test" {
+  project = "${google_project.acceptance.project_id}"
+  service = "%s"
+
+  disable_dependent_services = true
+}
+
+`, pid, name, org, service)
+}

third_party/terraform/tests/resource_google_project_services_test.go

@@ -179,9 +179,7 @@ func TestAccProjectServices_ignoreUnenablableServices(t *testing.T) {
 		Steps: []resource.TestStep{
 			{
 				Config: testAccProjectAssociateServicesBasic_withBilling(services, pid, pname, org, billingId),
-				Check: resource.ComposeTestCheckFunc(
-					testProjectServicesMatch(services, pid),
-				),
+				Check:  resource.ComposeTestCheckFunc(testProjectServicesMatch(services, pid)),
 			},
 		},
 	})
@@ -269,6 +267,105 @@ func TestAccProjectServices_pagination(t *testing.T) {
 	})
 }
 
+func TestAccProjectServices_renamedServices(t *testing.T) {
+	t.Parallel()
+
+	org := getTestOrgFromEnv(t)
+	pid := "terraform-" + acctest.RandString(10)
+	resource.Test(t, resource.TestCase{
+		PreCheck:  func() { testAccPreCheck(t) },
+		Providers: testAccProviders,
+		Steps: []resource.TestStep{
+			{
+				// create new
+				Config: testAccProjectAssociateServicesBasic([]string{
+					"bigquery.googleapis.com",
+					"bigquerystorage.googleapis.com",
+					"iam.googleapis.com",
+					"iamcredentials.googleapis.com",
+					"oslogin.googleapis.com",
+				}, pid, pname, org),
+			},
+			{
+				// transition to old
+				Config: testAccProjectAssociateServicesBasic([]string{
+					"bigquery-json.googleapis.com",
+					"bigquerystorage.googleapis.com",
+					"iam.googleapis.com",
+					"iamcredentials.googleapis.com",
+					"oslogin.googleapis.com",
+				}, pid, pname, org),
+			},
+			{
+				// transition to new
+				Config: testAccProjectAssociateServicesBasic([]string{
+					"bigquery.googleapis.com",
+					"bigquerystorage.googleapis.com",
+					"iam.googleapis.com",
+					"iamcredentials.googleapis.com",
+					"oslogin.googleapis.com",
+				}, pid, pname, org),
+			},
+			{
+				// remove new
+				Config: testAccProjectAssociateServicesBasic([]string{
+					"iam.googleapis.com",
+					"iamcredentials.googleapis.com",
+					"oslogin.googleapis.com",
+				}, pid, pname, org),
+			},
+			{
+				// create both
+				Config: testAccProjectAssociateServicesBasic([]string{
+					"bigquery.googleapis.com",
+					"bigquery-json.googleapis.com",
+					"bigquerystorage.googleapis.com",
+					"iam.googleapis.com",
+					"iamcredentials.googleapis.com",
+					"oslogin.googleapis.com",
+				}, pid, pname, org),
+			},
+			{
+				// remove new
+				Config: testAccProjectAssociateServicesBasic([]string{
+					"bigquery-json.googleapis.com",
+					"bigquerystorage.googleapis.com",
+					"iam.googleapis.com",
+					"iamcredentials.googleapis.com",
+					"oslogin.googleapis.com",
+				}, pid, pname, org),
+			},
+			{
+				// import imports old
+				ResourceName:            "google_project_services.acceptance",
+				ImportState:             true,
+				ImportStateId:           pid,
+				ImportStateVerify:       true,
+				ImportStateVerifyIgnore: []string{"disable_on_destroy"},
+			},
+			{
+				// transition to both
+				Config: testAccProjectAssociateServicesBasic([]string{
+					"bigquery.googleapis.com",
+					"bigquery-json.googleapis.com",
+					"bigquerystorage.googleapis.com",
+					"iam.googleapis.com",
+					"iamcredentials.googleapis.com",
+					"oslogin.googleapis.com",
+				}, pid, pname, org),
+			},
+			{
+				// remove both
+				Config: testAccProjectAssociateServicesBasic([]string{
+					"iam.googleapis.com",
+					"iamcredentials.googleapis.com",
+					"oslogin.googleapis.com",
+				}, pid, pname, org),
+			},
+		},
+	})
+}
+
 func testAccProjectAssociateServicesBasic(services []string, pid, name, org string) string {
 	return fmt.Sprintf(`
 resource "google_project" "acceptance" {
@@ -309,6 +406,17 @@ func testProjectServicesMatch(services []string, pid string) resource.TestCheckF
 			return fmt.Errorf("Error listing services for project %q: %v", pid, err)
 		}
 
+		servicesSet := golangSetFromStringSlice(services)
+		// add renamed service aliases because listCurrentlyEnabledServices will
+		// have both
+		for k := range servicesSet {
+			if v, ok := renamedServicesByOldAndNewServiceNames[k]; ok {
+				servicesSet[v] = struct{}{}
+			}
+		}
+
+		services = stringSliceFromGolangSet(servicesSet)
+
 		apiServices := stringSliceFromGolangSet(currentlyEnabled)
 		sort.Strings(services)
 		sort.Strings(apiServices)