Replace InstanceInfo call with Instances call for regional reliability (

hashicorp#8971) (hashicorp#15900) * Replace InstanceInfo call with Instances call for regional reliability * Handle unavailable error when no overlap * Handle unavailable error when no overlap * Add tests * Add tests * Add tests * Add tests Signed-off-by: Modular Magician <[email protected]>
opentofu · Sep 19, 2023 · 9f02f46 · 9f02f46
1 parent a8aa378
commit 9f02f46
Show file tree

Hide file tree

Showing 3 changed files with 131 additions and 9 deletions.
diff --git a/.changelog/8971.txt b/.changelog/8971.txt
@@ -0,0 +1,3 @@
+```release-note:bug
+bigtable: improved regional reliability when instance overlaps a downed region in the resource `google_bigtable_instance`
+```
diff --git a/google/services/bigtable/resource_bigtable_instance.go b/google/services/bigtable/resource_bigtable_instance.go
@@ -260,25 +260,26 @@ func resourceBigtableInstanceRead(d *schema.ResourceData, meta interface{}) erro
 
 	ctxWithTimeout, cancel := context.WithTimeout(ctx, d.Timeout(schema.TimeoutRead))
 	defer cancel()
-	instance, err := c.InstanceInfo(ctxWithTimeout, instanceName)
-	if err != nil {
-		if tpgresource.IsNotFoundGrpcError(err) {
-			log.Printf("[WARN] Removing %s because it's gone", instanceName)
-			d.SetId("")
-			return nil
-		}
+	instancesResponse, err := c.Instances(ctxWithTimeout)
+	instance, stop, err := getInstanceFromResponse(instancesResponse, instanceName, err, d)
+	if stop {
 		return err
 	}
 
 	if err := d.Set("project", project); err != nil {
 		return fmt.Errorf("Error setting project: %s", err)
 	}
 
-	clusters, err := c.Clusters(ctxWithTimeout, instance.Name)
+	clusters, err := c.Clusters(ctxWithTimeout, instanceName)
 	if err != nil {
 		partiallyUnavailableErr, ok := err.(bigtable.ErrPartiallyUnavailable)
-
 		if !ok {
+			// Clusters() fails with 404 if instance does not exist.
+			if tpgresource.IsNotFoundGrpcError(err) {
+				log.Printf("[WARN] Removing %s because it's gone", instanceName)
+				d.SetId("")
+				return nil
+			}
 			return fmt.Errorf("Error retrieving instance clusters. %s", err)
 		}
 
@@ -432,6 +433,42 @@ func flattenBigtableCluster(c *bigtable.ClusterInfo) map[string]interface{} {
 	return cluster
 }
 
+func getInstanceFromResponse(instances []*bigtable.InstanceInfo, instanceName string, err error, d *schema.ResourceData) (*bigtable.InstanceInfo, bool, error) {
+	// Fail on any error other than ParrtiallyUnavailable.
+	isPartiallyUnavailableError := false
+	if err != nil {
+		_, isPartiallyUnavailableError = err.(bigtable.ErrPartiallyUnavailable)
+
+		if !isPartiallyUnavailableError {
+			return nil, true, fmt.Errorf("Error retrieving instance. %s", err)
+		}
+	}
+
+	// Get instance from response.
+	var instanceInfo *bigtable.InstanceInfo
+	for _, instance := range instances {
+		if instance.Name == instanceName {
+			instanceInfo = instance
+		}
+	}
+
+	// If instance found, it either wasn't affected by the outage, or there is no outage.
+	if instanceInfo != nil {
+		return instanceInfo, false, nil
+	}
+
+	// If instance wasn't found and error is PartiallyUnavailable,
+	// continue to clusters call that will reveal overlap between instance regions and unavailable regions.
+	if isPartiallyUnavailableError {
+		return nil, false, nil
+	}
+
+	// If instance wasn't found and error is not PartiallyUnavailable, instance doesn't exist.
+	log.Printf("[WARN] Removing %s because it's gone", instanceName)
+	d.SetId("")
+	return nil, true, nil
+}
+
 func getUnavailableClusterZones(clusters []interface{}, unavailableZones []string) []string {
 	var zones []string
 

diff --git a/google/services/bigtable/resource_bigtable_instance_internal_test.go b/google/services/bigtable/resource_bigtable_instance_internal_test.go
@@ -3,8 +3,13 @@
 package bigtable
 
 import (
+	"fmt"
 	"reflect"
+	"strings"
 	"testing"
+
+	"cloud.google.com/go/bigtable"
+	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
 )
 
 func TestGetUnavailableClusterZones(t *testing.T) {
@@ -50,3 +55,80 @@ func TestGetUnavailableClusterZones(t *testing.T) {
 		}
 	}
 }
+
+func TestGetInstanceFromResponse(t *testing.T) {
+	instanceName := "test-instance"
+	originalId := "original_value"
+	cases := map[string]struct {
+		instanceNames      []string
+		listInstancesError error
+
+		wantError        string
+		wantInstanceName string
+		wantStop         bool
+		wantId           string
+	}{
+		"not found": {
+			instanceNames:      []string{"wrong", "also_wrong"},
+			listInstancesError: nil,
+
+			wantError:        "",
+			wantStop:         true,
+			wantInstanceName: "",
+			wantId:           "",
+		},
+		"found": {
+			instanceNames:      []string{"wrong", "also_wrong", instanceName},
+			listInstancesError: nil,
+
+			wantError:        "",
+			wantStop:         false,
+			wantInstanceName: instanceName,
+			wantId:           originalId,
+		},
+		"error": {
+			instanceNames:      nil,
+			listInstancesError: fmt.Errorf("some error"),
+
+			wantError:        "Error retrieving instance.",
+			wantStop:         true,
+			wantInstanceName: "",
+			wantId:           originalId,
+		},
+		"unavailble error": {
+			instanceNames:      []string{"wrong", "also_wrong"},
+			listInstancesError: bigtable.ErrPartiallyUnavailable{[]string{"some", "location"}},
+
+			wantError:        "",
+			wantStop:         false,
+			wantInstanceName: "",
+			wantId:           originalId,
+		}}
+	for tn, tc := range cases {
+		instancesResponse := []*bigtable.InstanceInfo{}
+		for _, existingInstance := range tc.instanceNames {
+			instancesResponse = append(instancesResponse, &bigtable.InstanceInfo{Name: existingInstance})
+		}
+		d := &schema.ResourceData{}
+		d.SetId(originalId)
+		gotInstance, gotStop, gotErr := getInstanceFromResponse(instancesResponse, instanceName, tc.listInstancesError, d)
+
+		if gotStop != tc.wantStop {
+			t.Errorf("bad stop: %s, got %v, want %v", tn, gotStop, tc.wantStop)
+		}
+		if (gotErr != nil && tc.wantError == "") ||
+			(gotErr == nil && tc.wantError != "") ||
+			(gotErr != nil && !strings.Contains(gotErr.Error(), tc.wantError)) {
+			t.Errorf("bad error: %s, got %q, want %q", tn, gotErr, tc.wantError)
+		}
+		if (gotInstance == nil && tc.wantInstanceName != "") ||
+			(gotInstance != nil && tc.wantInstanceName == "") ||
+			(gotInstance != nil && gotInstance.Name != tc.wantInstanceName) {
+			t.Errorf("bad instance: %s, got %v, want %q", tn, gotInstance, tc.wantInstanceName)
+		}
+		gotId := d.Id()
+		if gotId != tc.wantId {
+			t.Errorf("bad ID: %s, got %v, want %q", tn, gotId, tc.wantId)
+		}
+	}
+}