Repository: GoogleCloudPlatform/terraform-validator
Pull request #770 (2022-06-15 18:01:35)

Check folder change before using v3 API


Patch:
diff --git a/ancestrymanager/ancestrymanager.go b/ancestrymanager/ancestrymanager.go
index 2a0d4689..dfdefe68 100644
--- a/ancestrymanager/ancestrymanager.go
+++ b/ancestrymanager/ancestrymanager.go
@@ -170,6 +170,14 @@ func (m *manager) fetchAncestors(config *resources.Config, tfData resources.Terr
 			return ancestors, nil
 		}
 		if folderOK {
+			// Get ancestry from project level first, if folder changed, then proceed
+			// with folder. This is to avoid requiring folder level permission if
+			// there is no folder change.
+
+			// trigger v1 API to get current ancestors and update the cache
+			m.getAncestorsWithCache(fmt.Sprintf("projects/%s", projectID))
+
+			// if folder changed, then it goes with v3 API, else it will use cache.
 			key = folderKey
 			ret, err := m.getAncestorsWithCache(key)
 			if err != nil {
diff --git a/go.mod b/go.mod
index 1c87c25e..67a24d09 100644
--- a/go.mod
+++ b/go.mod
@@ -13,7 +13,7 @@ require (
 	github.com/hashicorp/go-cleanhttp v0.5.2
 	github.com/hashicorp/terraform-json v0.13.0
 	github.com/hashicorp/terraform-plugin-sdk/v2 v2.16.0
-	github.com/hashicorp/terraform-provider-google v1.20.1-0.20220614183843-d84f3bf1bf09
+	github.com/hashicorp/terraform-provider-google v1.20.1-0.20220615190539-8520a6e717db
 	github.com/kr/pretty v0.3.0 // indirect
 	github.com/mitchellh/go-homedir v1.1.0
 	github.com/onsi/gomega v1.17.0 // indirect
diff --git a/go.sum b/go.sum
index 603c6d7f..c437018d 100644
--- a/go.sum
+++ b/go.sum
@@ -690,8 +690,8 @@ github.com/hashicorp/terraform-plugin-log v0.4.0 h1:F3eVnm8r2EfQCe2k9blPIiF/r2TT
 github.com/hashicorp/terraform-plugin-log v0.4.0/go.mod h1:9KclxdunFownr4pIm1jdmwKRmE4d6HVG2c9XDq47rpg=
 github.com/hashicorp/terraform-plugin-sdk/v2 v2.16.0 h1:9fjPgCenJqnbjo95SDcbJ+YdLyEC1N35cwKWcRWhJTQ=
 github.com/hashicorp/terraform-plugin-sdk/v2 v2.16.0/go.mod h1:hLa0sTiySU/AWEgV2GxJh0/pQIqcCmm30IPja9N9lTg=
-github.com/hashicorp/terraform-provider-google v1.20.1-0.20220614183843-d84f3bf1bf09 h1:7Ijc7VuR4+k+qFZJHccQOFQ++UvaP68MP4H6/5Mcy+s=
-github.com/hashicorp/terraform-provider-google v1.20.1-0.20220614183843-d84f3bf1bf09/go.mod h1:mqsL8g69G+YPPRC8tz+GhLxDQBuVQlUjboPt1Pvme4s=
+github.com/hashicorp/terraform-provider-google v1.20.1-0.20220615190539-8520a6e717db h1:KwK1q5bjlbxZhKvbKMnZ4kf2CNCR4XTQKNr8yggMXzI=
+github.com/hashicorp/terraform-provider-google v1.20.1-0.20220615190539-8520a6e717db/go.mod h1:mqsL8g69G+YPPRC8tz+GhLxDQBuVQlUjboPt1Pvme4s=
 github.com/hashicorp/terraform-registry-address v0.0.0-20210412075316-9b2996cce896 h1:1FGtlkJw87UsTMg5s8jrekrHmUPUJaMcu6ELiVhQrNw=
 github.com/hashicorp/terraform-registry-address v0.0.0-20210412075316-9b2996cce896/go.mod h1:bzBPnUIkI0RxauU8Dqo+2KrZZ28Cf48s8V6IHt3p4co=
 github.com/hashicorp/terraform-svchost v0.0.0-20200729002733-f050f53b9734 h1:HKLsbzeOsfXmKNpr3GiT18XAblV0BjCbzL8KQAMZGa0=


Test patch:
diff --git a/ancestrymanager/ancestrymanager_test.go b/ancestrymanager/ancestrymanager_test.go
index 0f8238f7..0cbf7330 100644
--- a/ancestrymanager/ancestrymanager_test.go
+++ b/ancestrymanager/ancestrymanager_test.go
@@ -50,7 +50,7 @@ func TestGetAncestors(t *testing.T) {
 		},
 	}
 
-	ts := newAncestryManagerMockServer(t, v3Responses, v1Responses)
+	ts := newTestServer(t, v1Responses, v3Responses)
 	defer ts.Close()
 	mockV1Client, err := crmv1.NewService(context.Background(), option.WithEndpoint(ts.URL), option.WithoutAuthentication())
 	if err != nil {
@@ -414,6 +414,233 @@ func TestGetAncestors(t *testing.T) {
 	}
 }
 
+type testServer struct {
+	*httptest.Server
+	v1Count int
+	v3Count int
+}
+
+func newTestServer(t *testing.T, v1Responses map[string][]*crmv1.Ancestor, v3Responses map[string]*crmv3.Project) *testServer {
+	t.Helper()
+	ts := &testServer{}
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if strings.HasPrefix(r.URL.Path, "/v3/") {
+			name := strings.TrimPrefix(r.URL.Path, "/v3/")
+			resp, ok := v3Responses[name]
+			if !ok {
+				w.WriteHeader(http.StatusForbidden)
+				w.Write([]byte(fmt.Sprintf("no response for request path %s", "/v3/"+name)))
+				return
+			}
+
+			ts.v3Count++
+			payload, err := resp.MarshalJSON()
+			if err != nil {
+				w.WriteHeader(http.StatusInternalServerError)
+				w.Write([]byte(fmt.Sprintf("failed to MarshalJSON: %s", err)))
+				return
+			}
+			w.Write(payload)
+		} else if strings.HasPrefix(r.URL.Path, "/v1/") {
+			re := regexp.MustCompile(`([^/]*):getAncestry`)
+			path := re.FindStringSubmatch(r.URL.Path)
+			if path == nil || v1Responses[path[1]] == nil {
+				w.WriteHeader(http.StatusForbidden)
+				return
+			}
+
+			ts.v1Count++
+			payload, err := (&crmv1.GetAncestryResponse{Ancestor: v1Responses[path[1]]}).MarshalJSON()
+			if err != nil {
+				w.WriteHeader(http.StatusInternalServerError)
+				w.Write([]byte(fmt.Sprintf("failed to MarshalJSON: %s", err)))
+				return
+			}
+			w.Write(payload)
+		} else {
+			w.WriteHeader(http.StatusForbidden)
+			w.Write([]byte(fmt.Sprintf("no response for url path %s", r.URL.Path)))
+			return
+		}
+	}))
+	ts.Server = server
+	return ts
+}
+
+func TestGetAncestors_Folder(t *testing.T) {
+	// v1 API is used to fetch ancestry first, and compared with the resource data
+	// to find out whether there is a folder ID change.
+	p := provider.Provider()
+	cases := []struct {
+		name        string
+		data        resources.TerraformResourceData
+		asset       *resources.Asset
+		v1Responses map[string][]*crmv1.Ancestor
+		v3Responses map[string]*crmv3.Project
+		want        []string
+		parent      string
+		v3Count     int
+		v1Count     int
+	}{
+		{
+			name: "folder not changed",
+			data: tfdata.NewFakeResourceData(
+				"google_project",
+				p.ResourcesMap["google_project"].Schema,
+				map[string]interface{}{
+					"project_id": "foo",
+					"folder_id":  "folders/bar",
+				},
+			),
+			asset: &resources.Asset{
+				Type: "cloudresourcemanager.googleapis.com/Project",
+			},
+			v3Responses: map[string]*crmv3.Project{},
+			v1Responses: map[string][]*crmv1.Ancestor{
+				"foo": {
+					{ResourceId: &crmv1.ResourceId{Id: "foo", Type: "project"}},
+					{ResourceId: &crmv1.ResourceId{Id: "bar", Type: "folder"}},
+					{ResourceId: &crmv1.ResourceId{Id: "qux", Type: "organization"}},
+				},
+			},
+			want:    []string{"projects/foo", "folders/bar", "organizations/qux"},
+			parent:  "//cloudresourcemanager.googleapis.com/folders/bar",
+			v1Count: 1,
+		},
+		{
+			// project moving from organizations/qux/folders/bar to organizations/qux2/folders/bar2
+			name: "project moved from a top-level folder in one org to a top-level folder in a different org",
+			data: tfdata.NewFakeResourceData(
+				"google_project",
+				p.ResourcesMap["google_project"].Schema,
+				map[string]interface{}{
+					"project_id": "foo",
+					"folder_id":  "folders/bar2",
+				},
+			),
+			asset: &resources.Asset{
+				Type: "cloudresourcemanager.googleapis.com/Project",
+			},
+			v3Responses: map[string]*crmv3.Project{
+				"folders/bar2":       {Name: "folders/bar2", Parent: "organizations/qux2"},
+				"organizations/qux2": {Name: "organizations/qux2", Parent: ""},
+			},
+			v1Responses: map[string][]*crmv1.Ancestor{
+				"foo": {
+					{ResourceId: &crmv1.ResourceId{Id: "foo", Type: "project"}},
+					{ResourceId: &crmv1.ResourceId{Id: "bar", Type: "folder"}},
+					{ResourceId: &crmv1.ResourceId{Id: "qux", Type: "organization"}},
+				},
+			},
+			want:    []string{"projects/foo", "folders/bar2", "organizations/qux2"},
+			parent:  "//cloudresourcemanager.googleapis.com/folders/bar2",
+			v3Count: 1,
+			v1Count: 1,
+		},
+		{
+			// project moving from folders/bar2/folders/bar to folders/bar2
+			name: "project moved from child folder to parent folder",
+			data: tfdata.NewFakeResourceData(
+				"google_project",
+				p.ResourcesMap["google_project"].Schema,
+				map[string]interface{}{
+					"project_id": "foo",
+					"folder_id":  "folders/bar2",
+				},
+			),
+			asset: &resources.Asset{
+				Type: "cloudresourcemanager.googleapis.com/Project",
+			},
+			v3Responses: map[string]*crmv3.Project{},
+			v1Responses: map[string][]*crmv1.Ancestor{
+				"foo": {
+					{ResourceId: &crmv1.ResourceId{Id: "foo", Type: "project"}},
+					{ResourceId: &crmv1.ResourceId{Id: "bar", Type: "folder"}},
+					{ResourceId: &crmv1.ResourceId{Id: "bar2", Type: "folder"}},
+					{ResourceId: &crmv1.ResourceId{Id: "qux", Type: "organization"}},
+				},
+			},
+			want:    []string{"projects/foo", "folders/bar2", "organizations/qux"},
+			parent:  "//cloudresourcemanager.googleapis.com/folders/bar2",
+			v1Count: 1,
+		},
+		{
+			// project moving from folders/bar2 to folders/bar2/folders/bar
+			name: "project moved from parent folder to child folder",
+			data: tfdata.NewFakeResourceData(
+				"google_project",
+				p.ResourcesMap["google_project"].Schema,
+				map[string]interface{}{
+					"project_id": "foo",
+					"folder_id":  "folders/bar",
+				},
+			),
+			asset: &resources.Asset{
+				Type: "cloudresourcemanager.googleapis.com/Project",
+			},
+			v3Responses: map[string]*crmv3.Project{
+				"folders/bar": {Name: "folders/bar", Parent: "folders/bar2"},
+			},
+			v1Responses: map[string][]*crmv1.Ancestor{
+				"foo": {
+					{ResourceId: &crmv1.ResourceId{Id: "foo", Type: "project"}},
+					{ResourceId: &crmv1.ResourceId{Id: "bar2", Type: "folder"}},
+					{ResourceId: &crmv1.ResourceId{Id: "qux", Type: "organization"}},
+				},
+			},
+			want:    []string{"projects/foo", "folders/bar", "folders/bar2", "organizations/qux"},
+			parent:  "//cloudresourcemanager.googleapis.com/folders/bar",
+			v1Count: 1,
+			v3Count: 1,
+		},
+	}
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+
+			ts := newTestServer(t, c.v1Responses, c.v3Responses)
+			defer ts.Close()
+
+			mockV1Client, err := crmv1.NewService(context.Background(), option.WithEndpoint(ts.URL), option.WithoutAuthentication())
+			if err != nil {
+				t.Fatal(err)
+			}
+			mockV3Client, err := crmv3.NewService(context.Background(), option.WithEndpoint(ts.URL), option.WithoutAuthentication())
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			cfg := &resources.Config{
+				Project: "foo",
+			}
+			ancestryManager := &manager{
+				errorLogger:       zap.NewExample(),
+				ancestorCache:     make(map[string][]string),
+				resourceManagerV3: mockV3Client,
+				resourceManagerV1: mockV1Client,
+			}
+			// empty cache
+			ancestryManager.initAncestryCache(map[string]string{})
+
+			got, parent, err := ancestryManager.Ancestors(cfg, c.data, c.asset)
+			if err != nil {
+				t.Fatalf("Ancestors(%v, %v, %v) = %s, want = nil", cfg, c.data, c.asset, err)
+			}
+			if parent != c.parent {
+				t.Errorf("Ancestors(%v, %v, %v) parent = %s, want = %s", cfg, c.data, c.asset, parent, c.parent)
+			}
+			if diff := cmp.Diff(c.want, got); diff != "" {
+				t.Errorf("Ancestors(%v, %v, %v) returned unexpected diff (-want +got):\n%s", cfg, c.data, c.asset, diff)
+			}
+			if ts.v3Count != c.v3Count {
+				t.Errorf("Ancestors(%v, %v, %v) v3 API called = %d, want = %d", cfg, c.data, c.asset, ts.v3Count, c.v3Count)
+			}
+			if ts.v1Count != c.v1Count {
+				t.Errorf("Ancestors(%v, %v, %v) v1 API called = %d, want = %d", cfg, c.data, c.asset, ts.v1Count, c.v1Count)
+			}
+		})
+	}
+}
+
 func TestGetAncestorsWithCache(t *testing.T) {
 	tests := []struct {
 		name        string
@@ -501,7 +728,7 @@ func TestGetAncestorsWithCache(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			ts := newAncestryManagerMockServer(t, test.v3Responses, test.v1Responses)
+			ts := newTestServer(t, test.v1Responses, test.v3Responses)
 			defer ts.Close()
 			mockV1Client, err := crmv1.NewService(context.Background(), option.WithEndpoint(ts.URL), option.WithoutAuthentication())
 			if err != nil {
@@ -553,7 +780,7 @@ func TestGetAncestorsWithCache_Fail(t *testing.T) {
 	}
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			ts := newAncestryManagerMockServer(t, test.v3Responses, test.v1Responses)
+			ts := newTestServer(t, test.v1Responses, test.v3Responses)
 			defer ts.Close()
 			mockV1Client, err := crmv1.NewService(context.Background(), option.WithEndpoint(ts.URL), option.WithoutAuthentication())
 			if err != nil {
@@ -578,48 +805,6 @@ func TestGetAncestorsWithCache_Fail(t *testing.T) {
 	}
 }
 
-func newAncestryManagerMockServer(t *testing.T, v3Responses map[string]*crmv3.Project, v1Responses map[string][]*crmv1.Ancestor) *httptest.Server {
-	t.Helper()
-	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		if strings.HasPrefix(r.URL.Path, "/v3/") {
-			name := strings.TrimPrefix(r.URL.Path, "/v3/")
-			resp, ok := v3Responses[name]
-			if !ok {
-				w.WriteHeader(http.StatusForbidden)
-				w.Write([]byte(fmt.Sprintf("no response for request path %s", "/v3/"+name)))
-				return
-			}
-			payload, err := resp.MarshalJSON()
-			if err != nil {
-				w.WriteHeader(http.StatusInternalServerError)
-				w.Write([]byte(fmt.Sprintf("failed to MarshalJSON: %s", err)))
-				return
-			}
-			w.Write(payload)
-		} else if strings.HasPrefix(r.URL.Path, "/v1/") {
-			re := regexp.MustCompile(`([^/]*):getAncestry`)
-			path := re.FindStringSubmatch(r.URL.Path)
-			if path == nil || v1Responses[path[1]] == nil {
-				w.WriteHeader(http.StatusForbidden)
-				return
-			}
-
-			payload, err := (&crmv1.GetAncestryResponse{Ancestor: v1Responses[path[1]]}).MarshalJSON()
-			if err != nil {
-				t.Errorf("failed to MarshalJSON: %s", err)
-				w.WriteHeader(http.StatusInternalServerError)
-				return
-			}
-			w.Write(payload)
-		} else {
-			w.WriteHeader(http.StatusForbidden)
-			w.Write([]byte(fmt.Sprintf("no response for url path %s", r.URL.Path)))
-			return
-		}
-	}))
-	return ts
-}
-
 func TestParseAncestryPath(t *testing.T) {
 	tests := []struct {
 		name string