Fix perma-diff in google_service_networking_connection for reserved_peering_ranges ordering (GoogleCloudPlatform#16685)

Author: SCSAndre

mmv1/third_party/terraform/services/servicenetworking/resource_service_networking_connection.go

@@ -4,7 +4,9 @@ import (
 	"fmt"
 	"log"
 	"net/url"
+	"reflect"
 	"regexp"
+	"sort"
 	"strings"
 	"time"
 
@@ -62,10 +64,11 @@ func ResourceServiceNetworkingConnection() *schema.Resource {
 				Description: `Provider peering service that is managing peering connectivity for a service provider organization. For Google services that support this functionality it is 'servicenetworking.googleapis.com'.`,
 			},
 			"reserved_peering_ranges": {
-				Type:        schema.TypeList,
-				Required:    true,
-				Elem:        &schema.Schema{Type: schema.TypeString},
-				Description: `Named IP address range(s) of PEERING type reserved for this service provider. Note that invoking this method with a different range when connection is already established will not reallocate already provisioned service producer subnetworks.`,
+				Type:             schema.TypeList,
+				Required:         true,
+				Elem:             &schema.Schema{Type: schema.TypeString},
+				DiffSuppressFunc: stringListDiffSuppress,
+				Description:      `Named IP address range(s) of PEERING type reserved for this service provider. Note that invoking this method with a different range when connection is already established will not reallocate already provisioned service producer subnetworks.`,
 			},
 			"deletion_policy": {
 				Type:         schema.TypeString,
@@ -227,9 +230,14 @@ func resourceServiceNetworkingConnectionRead(d *schema.ResourceData, meta interf
 	if err := d.Set("peering", connection.Peering); err != nil {
 		return fmt.Errorf("Error setting peering: %s", err)
 	}
+
+	// removed the intermediate `ranges` variable — it was a
+	// leftover from when sort.Strings() lived here. The DiffSuppressFunc
+	// already handles ordering, so we write the API response directly to state.
 	if err := d.Set("reserved_peering_ranges", connection.ReservedPeeringRanges); err != nil {
 		return fmt.Errorf("Error setting reserved_peering_ranges: %s", err)
 	}
+
 	return nil
 }
 
@@ -433,7 +441,6 @@ func RetrieveServiceNetworkingNetworkName(d *schema.ResourceData, config *transp
 
 	// return the network name formatting unique to this API
 	return fmt.Sprintf("projects/%v/global/networks/%v", project.ProjectNumber, networkName), nil
-
 }
 
 const parentServicePattern = "^services/.+$"
@@ -457,3 +464,37 @@ func init() {
 		Schema:      ResourceServiceNetworkingConnection(),
 	}.Register()
 }
+
+// stringListDiffSuppress suppresses diffs for TypeList fields where the
+// order of elements does not matter. It derives the root key from k by
+// stripping the element index suffix.
+func stringListDiffSuppress(k, old, new string, d *schema.ResourceData) bool {
+	root := k
+	if idx := strings.IndexByte(k, '.'); idx != -1 {
+		root = k[:idx]
+	}
+
+	o, n := d.GetChange(root)
+
+	oldList, ok1 := o.([]interface{})
+	newList, ok2 := n.([]interface{})
+
+	if !ok1 || !ok2 || len(oldList) != len(newList) {
+		return false
+	}
+
+	oldStrs := make([]string, len(oldList))
+	for i, v := range oldList {
+		oldStrs[i] = fmt.Sprintf("%v", v)
+	}
+
+	newStrs := make([]string, len(newList))
+	for i, v := range newList {
+		newStrs[i] = fmt.Sprintf("%v", v)
+	}
+
+	sort.Strings(oldStrs)
+	sort.Strings(newStrs)
+
+	return reflect.DeepEqual(oldStrs, newStrs)
+}

mmv1/third_party/terraform/services/servicenetworking/resource_service_networking_connection_test.go

@@ -312,3 +312,72 @@ resource "google_service_networking_connection" "foobar" {
 }
 `, addressRangeName, addressRangeName, org_id, billing_account, networkName, addressRangeName, serviceName)
 }
+
+func TestAccServiceNetworkingConnection_reorder(t *testing.T) {
+	t.Parallel()
+
+	network := fmt.Sprintf("tf-test-snc-%s", acctest.RandString(t, 10))
+	range1 := fmt.Sprintf("tf-test-range1-%s", acctest.RandString(t, 10))
+	range2 := fmt.Sprintf("tf-test-range2-%s", acctest.RandString(t, 10))
+
+	service := "servicenetworking.googleapis.com"
+
+	acctest.VcrTest(t, resource.TestCase{
+		PreCheck:                 func() { acctest.AccTestPreCheck(t) },
+		ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories(t),
+		CheckDestroy:             testServiceNetworkingConnectionDestroy(t, service, network),
+		Steps: []resource.TestStep{
+			{
+				Config: testAccServiceNetworkingConnectionOrder(network, range1, range2, service, "forward"),
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr("google_service_networking_connection.foobar", "reserved_peering_ranges.#", "2"),
+				),
+			},
+			{
+				ResourceName:      "google_service_networking_connection.foobar",
+				ImportState:       true,
+				ImportStateVerify: true,
+			},
+			{
+				Config:   testAccServiceNetworkingConnectionOrder(network, range1, range2, service, "reverse"),
+				PlanOnly: true,
+			},
+		},
+	})
+}
+
+func testAccServiceNetworkingConnectionOrder(networkName, r1, r2, serviceName, order string) string {
+	ranges := fmt.Sprintf(`["%s", "%s"]`, r1, r2)
+	if order == "reverse" {
+		ranges = fmt.Sprintf(`["%s", "%s"]`, r2, r1)
+	}
+
+	return fmt.Sprintf(`
+resource "google_compute_network" "servicenet" {
+  name = "%s"
+}
+
+resource "google_compute_global_address" "r1" {
+  name          = "%s"
+  purpose       = "VPC_PEERING"
+  address_type  = "INTERNAL"
+  prefix_length = 16
+  network       = google_compute_network.servicenet.self_link
+}
+
+resource "google_compute_global_address" "r2" {
+  name          = "%s"
+  purpose       = "VPC_PEERING"
+  address_type  = "INTERNAL"
+  prefix_length = 16
+  network       = google_compute_network.servicenet.self_link
+}
+
+resource "google_service_networking_connection" "foobar" {
+  network                 = google_compute_network.servicenet.self_link
+  service                 = "%s"
+  reserved_peering_ranges = %s
+  depends_on = [google_compute_global_address.r1, google_compute_global_address.r2]
+}
+`, networkName, r1, r2, serviceName, ranges)
+}