Repository: GoogleCloudPlatform/terraform-validator
Pull request #283 (2021-08-04 23:21:26)

update ancestry manager to use cfg
closes https://github.com/GoogleCloudPlatform/terraform-validator/issues/279

credentials/token were not being used for resourceManager initialization. 


Patch:
diff --git a/ancestrymanager/ancestrymanager.go b/ancestrymanager/ancestrymanager.go
index 55f6e834..9835f956 100644
--- a/ancestrymanager/ancestrymanager.go
+++ b/ancestrymanager/ancestrymanager.go
@@ -2,14 +2,11 @@
 package ancestrymanager
 
 import (
-	"context"
 	"fmt"
 	"log"
 	"strings"
 
-	"github.com/pkg/errors"
 	"google.golang.org/api/cloudresourcemanager/v1"
-	"google.golang.org/api/option"
 
 	converter "github.com/GoogleCloudPlatform/terraform-google-conversion/google"
 )
@@ -22,6 +19,12 @@ type AncestryManager interface {
 	GetAncestryWithResource(project string, tfData converter.TerraformResourceData, cai converter.Asset) (string, error)
 }
 
+// ClientRetriever is the interface that returns an instance of various clients.
+type ClientRetriever interface {
+	// NewResourceManagerClient returns an initialized *cloudresourcemanager.Service
+	NewResourceManagerClient(userAgent string) *cloudresourcemanager.Service
+}
+
 // resourceAncestryManager provides common methods for retrieving ancestry from resources
 type resourceAncestryManager struct {
 }
@@ -180,7 +183,7 @@ func (m *offlineAncestryManager) GetAncestryWithResource(project string, tfData
 }
 
 // New returns AncestryManager that can be used to fetch ancestry information for a project.
-func New(ctx context.Context, project, ancestry string, offline bool, opts ...option.ClientOption) (AncestryManager, error) {
+func New(retriever ClientRetriever, project, ancestry, userAgent string, offline bool) (AncestryManager, error) {
 	if ancestry != "" {
 		ancestry = fmt.Sprintf("%s/project/%s", ancestry, project)
 	}
@@ -189,10 +192,7 @@ func New(ctx context.Context, project, ancestry string, offline bool, opts ...op
 	}
 	am := &onlineAncestryManager{ancestryCache: map[string]string{}}
 	am.store(project, ancestry)
-	rm, err := cloudresourcemanager.NewService(ctx, opts...)
-	if err != nil {
-		return nil, errors.Wrap(err, "constructing resource manager client")
-	}
+	rm := retriever.NewResourceManagerClient(userAgent)
 	am.resourceManager = rm
 	return am, nil
 }
diff --git a/cnvconfig/getconfig.go b/cnvconfig/getconfig.go
new file mode 100644
index 00000000..70f029e4
--- /dev/null
+++ b/cnvconfig/getconfig.go
@@ -0,0 +1,48 @@
+// Copyright 2021 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package cnvconfig
+
+import (
+	"context"
+
+	converter "github.com/GoogleCloudPlatform/terraform-google-conversion/google"
+	"github.com/pkg/errors"
+)
+
+func GetConfig(ctx context.Context, project string, offline bool) (*converter.Config, error) {
+	cfg := &converter.Config{
+		Project: project,
+	}
+
+	// Search for default credentials
+	cfg.Credentials = multiEnvSearch([]string{
+		"GOOGLE_CREDENTIALS",
+		"GOOGLE_CLOUD_KEYFILE_JSON",
+		"GCLOUD_KEYFILE_JSON",
+	})
+
+	cfg.AccessToken = multiEnvSearch([]string{
+		"GOOGLE_OAUTH_ACCESS_TOKEN",
+	})
+
+	if !offline {
+		converter.ConfigureBasePaths(cfg)
+		if err := cfg.LoadAndValidate(ctx); err != nil {
+			return nil, errors.Wrap(err, "load and validate config")
+		}
+	}
+
+	return cfg, nil
+}
diff --git a/converters/google/utils.go b/cnvconfig/utils.go
similarity index 97%
rename from converters/google/utils.go
rename to cnvconfig/utils.go
index 26631e20..5640e5ef 100644
--- a/converters/google/utils.go
+++ b/cnvconfig/utils.go
@@ -11,7 +11,7 @@
 // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 // See the License for the specific language governing permissions and
 // limitations under the License.
-package google
+package cnvconfig
 
 import (
 	"os"
diff --git a/converters/google/convert.go b/converters/google/convert.go
index cb8980f2..2b335292 100644
--- a/converters/google/convert.go
+++ b/converters/google/convert.go
@@ -15,7 +15,6 @@
 package google
 
 import (
-	"context"
 	errorssyslib "errors"
 	"fmt"
 	"sort"
@@ -127,27 +126,7 @@ type RestoreDefault struct {
 }
 
 // NewConverter is a factory function for Converter.
-func NewConverter(ctx context.Context, ancestryManager ancestrymanager.AncestryManager, project string, offline, convertUnchanged bool) (*Converter, error) {
-	cfg := &converter.Config{
-		Project: project,
-	}
-	// Search for default credentials
-	cfg.Credentials = multiEnvSearch([]string{
-		"GOOGLE_CREDENTIALS",
-		"GOOGLE_CLOUD_KEYFILE_JSON",
-		"GCLOUD_KEYFILE_JSON",
-	})
-
-	cfg.AccessToken = multiEnvSearch([]string{
-		"GOOGLE_OAUTH_ACCESS_TOKEN",
-	})
-	if !offline {
-		converter.ConfigureBasePaths(cfg)
-		if err := cfg.LoadAndValidate(ctx); err != nil {
-			return nil, errors.Wrap(err, "load and validate config")
-		}
-	}
-
+func NewConverter(cfg *converter.Config, ancestryManager ancestrymanager.AncestryManager, offline bool, convertUnchanged bool) *Converter {
 	return &Converter{
 		schema:           provider.Provider(),
 		mapperFuncs:      converter.Mappers(),
@@ -156,7 +135,7 @@ func NewConverter(ctx context.Context, ancestryManager ancestrymanager.AncestryM
 		ancestryManager:  ancestryManager,
 		assets:           make(map[string]Asset),
 		convertUnchanged: convertUnchanged,
-	}, nil
+	}
 }
 
 // Converter knows how to convert terraform resources to their
diff --git a/tfgcv/planned_assets.go b/tfgcv/planned_assets.go
index b07b4f99..31d9b868 100644
--- a/tfgcv/planned_assets.go
+++ b/tfgcv/planned_assets.go
@@ -20,9 +20,8 @@ import (
 	"io/ioutil"
 	"path/filepath"
 
-	"google.golang.org/api/option"
-
 	"github.com/GoogleCloudPlatform/terraform-validator/ancestrymanager"
+	"github.com/GoogleCloudPlatform/terraform-validator/cnvconfig"
 	"github.com/GoogleCloudPlatform/terraform-validator/converters/google"
 	"github.com/GoogleCloudPlatform/terraform-validator/tfplan"
 	"github.com/pkg/errors"
@@ -60,15 +59,16 @@ func ReadPlannedAssets(ctx context.Context, path, project, ancestry string, offl
 }
 
 func newConverter(ctx context.Context, path, project, ancestry string, offline, convertUnchanged bool) (*google.Converter, error) {
-	ua := option.WithUserAgent(fmt.Sprintf("config-validator-tf/%s", BuildVersion()))
-	ancestryManager, err := ancestrymanager.New(context.Background(), project, ancestry, offline, ua)
+	cfg, err := cnvconfig.GetConfig(ctx, project, offline)
 	if err != nil {
-		return nil, errors.Wrap(err, "constructing resource manager client")
+		return nil, errors.Wrap(err, "building google configuration")
 	}
-	converter, err := google.NewConverter(ctx, ancestryManager, project, offline, convertUnchanged)
+	ua := fmt.Sprintf("config-validator-tf/%s", BuildVersion())
+	ancestryManager, err := ancestrymanager.New(cfg, project, ancestry, ua, offline)
 	if err != nil {
-		return nil, errors.Wrap(err, "building google converter")
+		return nil, errors.Wrap(err, "building google ancestry manager")
 	}
+	converter := google.NewConverter(cfg, ancestryManager, offline, convertUnchanged)
 	return converter, nil
 }
 


Test patch:
diff --git a/ancestrymanager/ancestrymanager_test.go b/ancestrymanager/ancestrymanager_test.go
index 382c8ba5..1720d4aa 100644
--- a/ancestrymanager/ancestrymanager_test.go
+++ b/ancestrymanager/ancestrymanager_test.go
@@ -11,6 +11,24 @@ import (
 	"google.golang.org/api/option"
 )
 
+type testRetriever struct {
+	online bool
+	opts   []option.ClientOption
+}
+
+func (tr *testRetriever) NewResourceManagerClient(userAgent string) *cloudresourcemanager.Service {
+	ctx := context.Background()
+
+	if tr.online {
+		rm, err := cloudresourcemanager.NewService(ctx, tr.opts...)
+		if err != nil {
+			panic(err)
+		}
+		return rm
+	}
+	return nil
+}
+
 func TestAncestryPath(t *testing.T) {
 	cases := []struct {
 		name           string
@@ -53,7 +71,6 @@ func TestAncestryPath(t *testing.T) {
 }
 
 func TestGetAncestry(t *testing.T) {
-	ctx := context.Background()
 	ownerProject := "foo"
 	ownerAncestry := "organization/qux/folder/bar"
 	ownerAncestryPath := "organization/qux/folder/bar/project/foo"
@@ -75,11 +92,15 @@ func TestGetAncestry(t *testing.T) {
 	ts := newAncestryManagerMockServer(t, cache)
 	defer ts.Close()
 
-	amOnline, err := New(ctx, ownerProject, "", false, option.WithEndpoint(ts.URL), option.WithoutAuthentication())
+	// option.WithEndpoint(ts.URL), option.WithoutAuthentication()
+	trOnline := &testRetriever{online: true, opts: []option.ClientOption{option.WithEndpoint(ts.URL), option.WithoutAuthentication()}}
+	amOnline, err := New(trOnline, ownerProject, ownerAncestry, "", false)
 	if err != nil {
 		t.Fatalf("failed to create online ancestry manager: %s", err)
 	}
-	amOffline, err := New(ctx, ownerProject, ownerAncestry, true)
+
+	trOffline := &testRetriever{online: false}
+	amOffline, err := New(trOffline, ownerProject, ownerAncestry, "", true)
 	if err != nil {
 		t.Fatalf("failed to create offline ancestry manager: %s", err)
 	}
diff --git a/cnvconfig/getconfig_test.go b/cnvconfig/getconfig_test.go
new file mode 100644
index 00000000..68888ad5
--- /dev/null
+++ b/cnvconfig/getconfig_test.go
@@ -0,0 +1,84 @@
+package cnvconfig
+
+import (
+	"context"
+	"os"
+	"testing"
+
+	converter "github.com/GoogleCloudPlatform/terraform-google-conversion/google"
+	"github.com/stretchr/testify/assert"
+)
+
+type configAttrGetter func(cfg *converter.Config) string
+
+func getCredentials(cfg *converter.Config) string {
+	return cfg.Credentials
+}
+func getAccessToken(cfg *converter.Config) string {
+	return cfg.AccessToken
+}
+
+func TestNewConverterCredentials(t *testing.T) {
+	ctx := context.Background()
+	offline := true
+	cases := []struct {
+		name           string
+		envKey         string
+		envValue       string
+		getConfigValue configAttrGetter
+	}{
+		{
+			name:           "GOOGLE_CREDENTIALS",
+			envKey:         "GOOGLE_CREDENTIALS",
+			envValue:       "whatever",
+			getConfigValue: getCredentials,
+		},
+		{
+			name:           "GOOGLE_CLOUD_KEYFILE_JSON",
+			envKey:         "GOOGLE_CLOUD_KEYFILE_JSON",
+			envValue:       "whatever",
+			getConfigValue: getCredentials,
+		},
+		{
+			name:           "GCLOUD_KEYFILE_JSON",
+			envKey:         "GCLOUD_KEYFILE_JSON",
+			envValue:       "whatever",
+			getConfigValue: getCredentials,
+		},
+		{
+			name:           "GOOGLE_OAUTH_ACCESS_TOKEN",
+			envKey:         "GOOGLE_OAUTH_ACCESS_TOKEN",
+			envValue:       "whatever",
+			getConfigValue: getAccessToken,
+		},
+	}
+
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			originalValue, isSet := os.LookupEnv(c.envKey)
+			err := os.Setenv(c.envKey, c.envValue)
+			if err != nil {
+				t.Fatalf("error setting env var %s=%s: %s", c.envKey, c.envValue, err)
+			}
+
+			cfg, err := GetConfig(ctx, "project", offline)
+			if err != nil {
+				t.Fatalf("error building converter: %s", err)
+			}
+
+			assert.EqualValues(t, c.getConfigValue(cfg), c.envValue)
+
+			if isSet {
+				err = os.Setenv(c.envKey, originalValue)
+				if err != nil {
+					t.Fatalf("error setting env var %s=%s: %s", c.envKey, originalValue, err)
+				}
+			} else {
+				err = os.Unsetenv(c.envKey)
+				if err != nil {
+					t.Fatalf("error unsetting env var %s: %s", c.envKey, err)
+				}
+			}
+		})
+	}
+}
diff --git a/converters/google/convert_test.go b/converters/google/convert_test.go
index 594f11e6..0e337816 100644
--- a/converters/google/convert_test.go
+++ b/converters/google/convert_test.go
@@ -16,13 +16,12 @@ package google
 
 import (
 	"context"
-	"os"
 	"sort"
 	"testing"
 	"time"
 
-	converter "github.com/GoogleCloudPlatform/terraform-google-conversion/google"
 	"github.com/GoogleCloudPlatform/terraform-validator/ancestrymanager"
+	"github.com/GoogleCloudPlatform/terraform-validator/cnvconfig"
 	tfjson "github.com/hashicorp/terraform-json"
 	"github.com/pkg/errors"
 	"github.com/stretchr/testify/assert"
@@ -33,91 +32,21 @@ const testProject = "test-project"
 func newTestConverter(convertUnchanged bool) (*Converter, error) {
 	ctx := context.Background()
 	ancestry := ""
+	ua := ""
 	project := testProject
 	offline := true
-	ancestryManager, err := ancestrymanager.New(context.Background(), project, ancestry, offline)
+	cfg, err := cnvconfig.GetConfig(ctx, project, offline)
 	if err != nil {
-		return nil, errors.Wrap(err, "constructing resource manager client")
+		return nil, errors.Wrap(err, "constructing configuration")
 	}
-	c, err := NewConverter(ctx, ancestryManager, project, offline, convertUnchanged)
+	ancestryManager, err := ancestrymanager.New(cfg, project, ancestry, ua, offline)
 	if err != nil {
-		return nil, errors.Wrap(err, "building converter")
+		return nil, errors.Wrap(err, "constructing resource ancestryManager")
 	}
+	c := NewConverter(cfg, ancestryManager, offline, convertUnchanged)
 	return c, nil
 }
 
-type configAttrGetter func(cfg *converter.Config) string
-
-func getCredentials(cfg *converter.Config) string {
-	return cfg.Credentials
-}
-func getAccessToken(cfg *converter.Config) string {
-	return cfg.AccessToken
-}
-
-func TestNewConverterCredentials(t *testing.T) {
-	cases := []struct {
-		name           string
-		envKey         string
-		envValue       string
-		getConfigValue configAttrGetter
-	}{
-		{
-			name:           "GOOGLE_CREDENTIALS",
-			envKey:         "GOOGLE_CREDENTIALS",
-			envValue:       "whatever",
-			getConfigValue: getCredentials,
-		},
-		{
-			name:           "GOOGLE_CLOUD_KEYFILE_JSON",
-			envKey:         "GOOGLE_CLOUD_KEYFILE_JSON",
-			envValue:       "whatever",
-			getConfigValue: getCredentials,
-		},
-		{
-			name:           "GCLOUD_KEYFILE_JSON",
-			envKey:         "GCLOUD_KEYFILE_JSON",
-			envValue:       "whatever",
-			getConfigValue: getCredentials,
-		},
-		{
-			name:           "GOOGLE_OAUTH_ACCESS_TOKEN",
-			envKey:         "GOOGLE_OAUTH_ACCESS_TOKEN",
-			envValue:       "whatever",
-			getConfigValue: getAccessToken,
-		},
-	}
-
-	for _, c := range cases {
-		t.Run(c.name, func(t *testing.T) {
-			originalValue, isSet := os.LookupEnv(c.envKey)
-			err := os.Setenv(c.envKey, c.envValue)
-			if err != nil {
-				t.Fatalf("error setting env var %s=%s: %s", c.envKey, c.envValue, err)
-			}
-
-			converter, err := newTestConverter(false)
-			if err != nil {
-				t.Fatalf("error building converter: %s", err)
-			}
-
-			assert.EqualValues(t, c.getConfigValue(converter.cfg), c.envValue)
-
-			if isSet {
-				err = os.Setenv(c.envKey, originalValue)
-				if err != nil {
-					t.Fatalf("error setting env var %s=%s: %s", c.envKey, originalValue, err)
-				}
-			} else {
-				err = os.Unsetenv(c.envKey)
-				if err != nil {
-					t.Fatalf("error unsetting env var %s: %s", c.envKey, err)
-				}
-			}
-		})
-	}
-}
-
 func TestSortByName(t *testing.T) {
 	cases := []struct {
 		name           string