From 03b21215830a9d14b2f6f53c025295ad57bc6808 Mon Sep 17 00:00:00 2001 From: Riley Karson Date: Thu, 3 Oct 2019 14:56:59 -0700 Subject: [PATCH] Add support for renamed services in google_project_service* resources. --- .../resource_google_project_service.go | 36 +++++- .../resource_google_project_services.go | 84 +++++++++++-- .../resource_google_project_service_test.go | 40 ++++++ .../resource_google_project_services_test.go | 114 +++++++++++++++++- .../terraform/utils/serviceusage_batching.go | 30 ++++- third_party/terraform/utils/utils.go.erb | 22 ++++ 6 files changed, 312 insertions(+), 14 deletions(-) diff --git a/third_party/terraform/resources/resource_google_project_service.go b/third_party/terraform/resources/resource_google_project_service.go index e0991b71358a..c3d8fdd1808e 100644 --- a/third_party/terraform/resources/resource_google_project_service.go +++ b/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 } diff --git a/third_party/terraform/resources/resource_google_project_services.go b/third_party/terraform/resources/resource_google_project_services.go index 975f29915cbf..7cff208b187d 100644 --- a/third_party/terraform/resources/resource_google_project_services.go +++ b/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,33 @@ 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 any values set in prior state. If none were set, only store + // the old value. + 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 +156,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 +280,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 +307,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 diff --git a/third_party/terraform/tests/resource_google_project_service_test.go b/third_party/terraform/tests/resource_google_project_service_test.go index dc8b00c6a518..6d8dfd81ff3f 100644 --- a/third_party/terraform/tests/resource_google_project_service_test.go +++ b/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) +} diff --git a/third_party/terraform/tests/resource_google_project_services_test.go b/third_party/terraform/tests/resource_google_project_services_test.go index 1ea2c986fadc..3b351a1cef0c 100644 --- a/third_party/terraform/tests/resource_google_project_services_test.go +++ b/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) diff --git a/third_party/terraform/utils/serviceusage_batching.go b/third_party/terraform/utils/serviceusage_batching.go index 7d7204423d3b..f0921c5be6b7 100644 --- a/third_party/terraform/utils/serviceusage_batching.go +++ b/third_party/terraform/utils/serviceusage_batching.go @@ -2,8 +2,10 @@ package google import ( "fmt" - "github.com/hashicorp/terraform-plugin-sdk/helper/schema" + "log" "time" + + "github.com/hashicorp/terraform-plugin-sdk/helper/schema" ) const ( @@ -13,10 +15,32 @@ const ( // BatchRequestEnableServices can be used to batch requests to enable services // across resource nodes, i.e. to batch creation of several // google_project_service(s) resources. -func BatchRequestEnableServices(services []string, project string, d *schema.ResourceData, config *Config) error { +func BatchRequestEnableServices(services map[string]struct{}, project string, d *schema.ResourceData, config *Config) error { + // renamed service create calls are relatively likely to fail, so break out + // of the batched call to avoid failing that as well + for k := range services { + if v, ok := renamedServicesByOldAndNewServiceNames[k]; ok { + log.Printf("[DEBUG] found renamed service %s (with alternate name %s)", k, v) + delete(services, k) + // also remove the other name, so we don't enable it 2x in a row + delete(services, v) + + // use a short timeout- failures are likely + log.Printf("[DEBUG] attempting user-specified name %s", k) + err := enableServiceUsageProjectServices([]string{k}, project, config, 1*time.Minute) + if err != nil { + log.Printf("[DEBUG] saw error %s. attempting alternate name %v", err, v) + err2 := enableServiceUsageProjectServices([]string{v}, project, config, 1*time.Minute) + if err2 != nil { + return fmt.Errorf("Saw 2 subsequent errors attempting to enable a renamed service: %s / %s", err, err2) + } + } + } + } + req := &BatchRequest{ ResourceName: project, - Body: services, + Body: stringSliceFromGolangSet(services), CombineF: combineServiceUsageServicesBatches, SendF: sendBatchFuncEnableServices(config, d.Timeout(schema.TimeoutCreate)), DebugId: fmt.Sprintf("Enable Project Services %s: %+v", project, services), diff --git a/third_party/terraform/utils/utils.go.erb b/third_party/terraform/utils/utils.go.erb index 31495e6366bb..f8544ed0970f 100644 --- a/third_party/terraform/utils/utils.go.erb +++ b/third_party/terraform/utils/utils.go.erb @@ -312,6 +312,28 @@ func stringSliceFromGolangSet(sset map[string]struct{}) []string { return ls } +func reverseStringMap(m map[string]string) map[string]string { + o := map[string]string{} + for k, v := range m { + o[v] = k + } + return o +} + +func mergeStringMaps(a, b map[string]string) map[string]string { + merged := make(map[string]string) + + for k, v := range a { + merged[k] = v + } + + for k, v := range b { + merged[k] = v + } + + return merged +} + func mergeSchemas(a, b map[string]*schema.Schema) map[string]*schema.Schema { merged := make(map[string]*schema.Schema)