Support project numbers in monitored project (#8454) (#15305)

Signed-off-by: Modular Magician <[email protected]>

.changelog/8454.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+monitoring: fixed an issue in `google_monitoring_monitored_project` where project numbers were not accepted for `name`
+```

google/resource_monitoring_monitored_project_test.go

@@ -0,0 +1,293 @@
+// Copyright (c) HashiCorp, Inc.
+// SPDX-License-Identifier: MPL-2.0
+package google
+
+import (
+	"fmt"
+	"testing"
+
+	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
+	"github.com/hashicorp/terraform-provider-google/google/acctest"
+	"github.com/hashicorp/terraform-provider-google/google/envvar"
+	"github.com/hashicorp/terraform-provider-google/google/services/monitoring"
+	"github.com/hashicorp/terraform-provider-google/google/tpgresource"
+)
+
+func TestAccMonitoringMonitoredProject_projectNumLongForm(t *testing.T) {
+	t.Parallel()
+
+	context := map[string]interface{}{
+		"org_id":        envvar.GetTestOrgFromEnv(t),
+		"project_id":    envvar.GetTestProjectFromEnv(),
+		"random_suffix": acctest.RandString(t, 10),
+	}
+
+	acctest.VcrTest(t, resource.TestCase{
+		PreCheck:                 func() { acctest.AccTestPreCheck(t) },
+		ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories(t),
+		CheckDestroy:             testAccCheckMonitoringMonitoredProjectDestroyProducer(t),
+		Steps: []resource.TestStep{
+			{
+				Config: testAccMonitoringMonitoredProject_projectNumLongForm(context),
+			},
+			{
+				ResourceName:            "google_monitoring_monitored_project.primary",
+				ImportState:             true,
+				ImportStateVerify:       true,
+				ImportStateVerifyIgnore: []string{"metrics_scope"},
+			},
+		},
+	})
+}
+
+func TestAccMonitoringMonitoredProject_projectNumShortForm(t *testing.T) {
+	t.Parallel()
+
+	context := map[string]interface{}{
+		"org_id":        envvar.GetTestOrgFromEnv(t),
+		"project_id":    envvar.GetTestProjectFromEnv(),
+		"random_suffix": acctest.RandString(t, 10),
+	}
+
+	acctest.VcrTest(t, resource.TestCase{
+		PreCheck:                 func() { acctest.AccTestPreCheck(t) },
+		ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories(t),
+		CheckDestroy:             testAccCheckMonitoringMonitoredProjectDestroyProducer(t),
+		Steps: []resource.TestStep{
+			{
+				Config: testAccMonitoringMonitoredProject_projectNumShortForm(context),
+			},
+			{
+				ResourceName:            "google_monitoring_monitored_project.primary",
+				ImportState:             true,
+				ImportStateVerify:       true,
+				ImportStateVerifyIgnore: []string{"metrics_scope"},
+			},
+		},
+	})
+}
+
+func testAccMonitoringMonitoredProject_projectNumLongForm(context map[string]interface{}) string {
+	return acctest.Nprintf(`
+resource "google_monitoring_monitored_project" "primary" {
+  metrics_scope = "%{project_id}"
+  name          = "locations/global/metricsScopes/%{project_id}/projects/${google_project.basic.number}"
+}
+
+resource "google_project" "basic" {
+  project_id = "tf-test-m-id%{random_suffix}"
+  name       = "tf-test-m-id%{random_suffix}-display"
+  org_id     = "%{org_id}"
+}
+`, context)
+}
+
+func testAccMonitoringMonitoredProject_projectNumShortForm(context map[string]interface{}) string {
+	return acctest.Nprintf(`
+resource "google_monitoring_monitored_project" "primary" {
+  metrics_scope = "%{project_id}"
+  name          = "${google_project.basic.number}"
+}
+
+resource "google_project" "basic" {
+  project_id = "tf-test-m-id%{random_suffix}"
+  name       = "tf-test-m-id%{random_suffix}-display"
+  org_id     = "%{org_id}"
+}
+`, context)
+}
+
+func TestUnitMonitoringMonitoredProject_nameDiffSuppress(t *testing.T) {
+	for _, tc := range monitoringMonitoredProjectDiffSuppressTestCases {
+		tc.Test(t)
+	}
+}
+
+type MonitoringMonitoredProjectDiffSuppressTestCase struct {
+	Name           string
+	KeysToSuppress []string
+	Before         map[string]interface{}
+	After          map[string]interface{}
+}
+
+var monitoringMonitoredProjectDiffSuppressTestCases = []MonitoringMonitoredProjectDiffSuppressTestCase{
+	// Project Id -> project Id
+	{
+		Name:           "short project id to long project id suppressed",
+		KeysToSuppress: []string{"name"},
+		Before: map[string]interface{}{
+			"name": "sameId",
+		},
+		After: map[string]interface{}{
+			"name": "locations/global/metricsScopes/projectId/projects/sameId",
+		},
+	},
+	{
+		Name:           "long project id to short project id suppressed",
+		KeysToSuppress: []string{"name"},
+		Before: map[string]interface{}{
+			"name": "locations/global/metricsScopes/projectId/projects/sameId",
+		},
+		After: map[string]interface{}{
+			"name": "sameId",
+		},
+	},
+	{
+		Name:           "short project id to long project id show diff",
+		KeysToSuppress: []string{},
+		Before: map[string]interface{}{
+			"name": "oldId",
+		},
+		After: map[string]interface{}{
+			"name": "locations/global/metricsScopes/projectId/projects/newId",
+		},
+	},
+	{
+		Name:           "long project id to short project id show diff",
+		KeysToSuppress: []string{},
+		Before: map[string]interface{}{
+			"name": "locations/global/metricsScopes/projectId/projects/oldId",
+		},
+		After: map[string]interface{}{
+			"name": "newId",
+		},
+	},
+
+	// Project Num -> Project Num
+	{
+		Name:           "short project num to long project num suppressed",
+		KeysToSuppress: []string{"name"},
+		Before: map[string]interface{}{
+			"name": "000000000000",
+		},
+		After: map[string]interface{}{
+			"name": "locations/global/metricsScopes/projectId/projects/000000000000",
+		},
+	},
+	{
+		Name:           "long project num to short project num suppressed",
+		KeysToSuppress: []string{"name"},
+		Before: map[string]interface{}{
+			"name": "locations/global/metricsScopes/projectId/projects/000000000000",
+		},
+		After: map[string]interface{}{
+			"name": "000000000000",
+		},
+	},
+	{
+		Name:           "short project num to long project num show diff",
+		KeysToSuppress: []string{},
+		Before: map[string]interface{}{
+			"name": "000000000000",
+		},
+		After: map[string]interface{}{
+			"name": "locations/global/metricsScopes/projectId/projects/111111111111",
+		},
+	},
+	{
+		Name:           "long project num to short project num show diff",
+		KeysToSuppress: []string{},
+		Before: map[string]interface{}{
+			"name": "locations/global/metricsScopes/projectId/projects/000000000000",
+		},
+		After: map[string]interface{}{
+			"name": "111111111111",
+		},
+	},
+
+	// Project id <--> Project num
+	// Every variation of this should be suppressed. We cannot detect
+	// if the project number matches the id within a diff suppress
+	{
+		Name:           "short project id to long project num suppressed",
+		KeysToSuppress: []string{"name"},
+		Before: map[string]interface{}{
+			"name": "oldId",
+		},
+		After: map[string]interface{}{
+			"name": "locations/global/metricsScopes/projectId/projects/111111111111",
+		},
+	},
+	{
+		Name:           "long project id to short project num suppressed",
+		KeysToSuppress: []string{"name"},
+		Before: map[string]interface{}{
+			"name": "locations/global/metricsScopes/projectId/projects/oldId",
+		},
+		After: map[string]interface{}{
+			"name": "111111111111",
+		},
+	},
+	{
+		Name:           "short project num to long project id suppressed",
+		KeysToSuppress: []string{"name"},
+		Before: map[string]interface{}{
+			"name": "000000000000",
+		},
+		After: map[string]interface{}{
+			"name": "locations/global/metricsScopes/projectId/projects/newId",
+		},
+	},
+	{
+		Name:           "long project num to short project id suppressed",
+		KeysToSuppress: []string{"name"},
+		Before: map[string]interface{}{
+			"name": "locations/global/metricsScopes/projectId/projects/000000000000",
+		},
+		After: map[string]interface{}{
+			"name": "newId",
+		},
+	},
+
+	// Empty -> anything (resource creation)
+	{
+		Name:           "empty name to anything shows diff",
+		KeysToSuppress: []string{},
+		Before: map[string]interface{}{
+			"name": "",
+		},
+		After: map[string]interface{}{
+			"name": "newId",
+		},
+	},
+}
+
+func (tc *MonitoringMonitoredProjectDiffSuppressTestCase) Test(t *testing.T) {
+	mockResourceDiff := &tpgresource.ResourceDiffMock{
+		Before: tc.Before,
+		After:  tc.After,
+	}
+
+	keySuppressionMap := map[string]bool{}
+	for key := range tc.Before {
+		keySuppressionMap[key] = false
+	}
+	for key := range tc.After {
+		keySuppressionMap[key] = false
+	}
+
+	for _, key := range tc.KeysToSuppress {
+		keySuppressionMap[key] = true
+	}
+
+	for key, tcSuppress := range keySuppressionMap {
+		oldValue, ok := tc.Before[key]
+		if !ok {
+			oldValue = ""
+		}
+		newValue, ok := tc.After[key]
+		if !ok {
+			newValue = ""
+		}
+		suppressed := monitoring.ResourceMonitoringMonitoredProjectNameDiffSuppressFunc(key, fmt.Sprintf("%v", oldValue), fmt.Sprintf("%v", newValue), mockResourceDiff)
+		if suppressed != tcSuppress {
+			var expectation string
+			if tcSuppress {
+				expectation = "be"
+			} else {
+				expectation = "not be"
+			}
+			t.Errorf("Test %s: expected key `%s` to %s suppressed", tc.Name, key, expectation)
+		}
+	}
+}

google/services/monitoring/resource_monitoring_monitored_project.go

@@ -31,6 +31,33 @@ import (
 	transport_tpg "github.com/hashicorp/terraform-provider-google/google/transport"
 )
 
+func ResourceMonitoringMonitoredProjectNameDiffSuppressFunc(k, old, new string, d tpgresource.TerraformResourceDataChange) bool {
+	// Don't suppress if values are empty strings
+	if old == "" || new == "" {
+		return false
+	}
+
+	oldShort := tpgresource.GetResourceNameFromSelfLink(old)
+	newShort := tpgresource.GetResourceNameFromSelfLink(new)
+
+	// Suppress if short names are equal
+	if oldShort == newShort {
+		return true
+	}
+
+	_, isOldNumErr := tpgresource.StringToFixed64(oldShort)
+	isOldNumber := isOldNumErr == nil
+	_, isNewNumErr := tpgresource.StringToFixed64(newShort)
+	isNewNumber := isNewNumErr == nil
+
+	// Suppress if comparing a project number to project id
+	return isOldNumber != isNewNumber
+}
+
+func resourceMonitoringMonitoredProjectNameDiffSuppress(k, old, new string, d *schema.ResourceData) bool {
+	return ResourceMonitoringMonitoredProjectNameDiffSuppressFunc(k, old, new, d)
+}
+
 func ResourceMonitoringMonitoredProject() *schema.Resource {
 	return &schema.Resource{
 		Create: resourceMonitoringMonitoredProjectCreate,
@@ -68,7 +95,7 @@ func ResourceMonitoringMonitoredProject() *schema.Resource {
 				Type:             schema.TypeString,
 				Required:         true,
 				ForceNew:         true,
-				DiffSuppressFunc: tpgresource.CompareResourceNames,
+				DiffSuppressFunc: resourceMonitoringMonitoredProjectNameDiffSuppress,
 				Description:      `Immutable. The resource name of the 'MonitoredProject'. On input, the resource name includes the scoping project ID and monitored project ID. On output, it contains the equivalent project numbers. Example: 'locations/global/metricsScopes/{SCOPING_PROJECT_ID_OR_NUMBER}/projects/{MONITORED_PROJECT_ID_OR_NUMBER}'`,
 			},
 			"create_time": {
@@ -363,9 +390,18 @@ func resourceMonitoringMonitoredProjectFindNestedObjectInList(d *schema.Resource
 }
 func resourceMonitoringMonitoredProjectDecoder(d *schema.ResourceData, meta interface{}, res map[string]interface{}) (map[string]interface{}, error) {
 	config := meta.(*transport_tpg.Config)
+
+	expectedName, _ := expandNestedMonitoringMonitoredProjectName(d.Get("name"), d, config)
+	expectedFlattenedName := flattenNestedMonitoringMonitoredProjectName(expectedName, d, config)
+	_, isNumErr := tpgresource.StringToFixed64(expectedFlattenedName.(string))
+	expectProjectNumber := isNumErr == nil
+
 	name := res["name"].(string)
 	name = tpgresource.GetResourceNameFromSelfLink(name)
-	if name != "" {
+
+	if expectProjectNumber {
+		res["name"] = name
+	} else if name != "" {
 		project, err := config.NewResourceManagerClient(config.UserAgent).Projects.Get(name).Do()
 		if err != nil {
 			return nil, err