Repository: GoogleCloudPlatform/terraform-validator
Pull request #306 (2021-09-16 22:09:30)

Piped zap loggers throughout the conversion process
With this change, all of our error / status logging should use the same zap logger, allowing us to enable structured logging for everything. This is a follow-up for https://github.com/GoogleCloudPlatform/terraform-validator/pull/304

melinath: FWIW it's errorLogger in opposition to the outputLogger because it outputs to stderr (and the structured logging schema we're using treats everything that's going to stderr as an "error"). But outputLogger only gets used in the cmd package, so maybe it just ends up being confusing? For now I think I'll merge this as-is for consistency but we can re-evaluate later. It will at least be a searchable term.

Patch:
diff --git a/ancestrymanager/ancestrymanager.go b/ancestrymanager/ancestrymanager.go
index 9835f956..aeab332c 100644
--- a/ancestrymanager/ancestrymanager.go
+++ b/ancestrymanager/ancestrymanager.go
@@ -3,12 +3,12 @@ package ancestrymanager
 
 import (
 	"fmt"
-	"log"
 	"strings"
 
 	"google.golang.org/api/cloudresourcemanager/v1"
 
 	converter "github.com/GoogleCloudPlatform/terraform-google-conversion/google"
+	"go.uber.org/zap"
 )
 
 // AncestryManager is the interface that wraps the GetAncestry method.
@@ -27,11 +27,12 @@ type ClientRetriever interface {
 
 // resourceAncestryManager provides common methods for retrieving ancestry from resources
 type resourceAncestryManager struct {
+	errorLogger *zap.Logger
 }
 
 func (m *resourceAncestryManager) getFolderAncestry(folderID string) ([]*cloudresourcemanager.Ancestor, error) {
 	// TODO(morgantep): Incorporate folders.GetAncestry from v2alpha1 API
-	log.Printf("[INFO] Retrieve ancestry for folder: %s", folderID)
+	m.errorLogger.Info(fmt.Sprintf("Retrieving ancestry for folder: %s", folderID))
 
 	return []*cloudresourcemanager.Ancestor{
 		&cloudresourcemanager.Ancestor{
@@ -50,7 +51,7 @@ func (m *resourceAncestryManager) getFolderAncestry(folderID string) ([]*cloudre
 }
 
 func (m *resourceAncestryManager) getAncestryFromResource(tfData converter.TerraformResourceData, cai converter.Asset) ([]*cloudresourcemanager.Ancestor, bool) {
-	log.Printf("[INFO] Retrieving ancestry from resource (type=%s)", cai.Type)
+	m.errorLogger.Info(fmt.Sprintf("Retrieving ancestry from resource (type=%s)", cai.Type))
 
 	switch cai.Type {
 	case "cloudresourcemanager.googleapis.com/Project", "cloudbilling.googleapis.com/ProjectBillingInfo":
@@ -88,14 +89,14 @@ func (m *resourceAncestryManager) getAncestryFromResource(tfData converter.Terra
 		if ok && folderID != "" {
 			folderAncestry, err := m.getFolderAncestry(folderID.(string))
 			if err != nil {
-				log.Printf("[ERROR] Failed to retrieve folder ancestry: %s", err)
+				m.errorLogger.Error(fmt.Sprintf("Failed to retrieve folder ancestry: %s", err))
 				return nil, false
 			}
 			return append(ancestry, folderAncestry...), true
 		}
 		return nil, false
 	default:
-		log.Printf("[INFO] Resource of type %s does not include sufficient data for ancestry retrieval", cai.Type)
+		m.errorLogger.Info(fmt.Sprintf("Resource of type %s does not include sufficient data for ancestry retrieval", cai.Type))
 		return nil, false
 	}
 }
@@ -138,7 +139,7 @@ func (m *onlineAncestryManager) GetAncestryWithResource(project string, tfData c
 		return "", err
 	}
 	path = ancestryPath(ancestry)
-	log.Printf("[INFO] [Online] Retrieved ancestry for %s: %s", project, path)
+	m.errorLogger.Info(fmt.Sprintf("[Online] Retrieved ancestry for %s: %s", project, path))
 	m.store(project, path)
 	return path, nil
 }
@@ -170,7 +171,7 @@ func (m *offlineAncestryManager) GetAncestryWithResource(project string, tfData
 	ancestry, ok := m.getAncestryFromResource(tfData, cai)
 	if ok {
 		path := ancestryPath(ancestry)
-		log.Printf("[INFO] [Offline] Retrieved ancestry for %s: %s", project, path)
+		m.errorLogger.Info(fmt.Sprintf("[Offline] Retrieved ancestry for %s: %s", project, path))
 		return path, nil
 	}
 
@@ -183,14 +184,14 @@ func (m *offlineAncestryManager) GetAncestryWithResource(project string, tfData
 }
 
 // New returns AncestryManager that can be used to fetch ancestry information for a project.
-func New(retriever ClientRetriever, project, ancestry, userAgent string, offline bool) (AncestryManager, error) {
+func New(retriever ClientRetriever, project, ancestry, userAgent string, offline bool, errorLogger *zap.Logger) (AncestryManager, error) {
 	if ancestry != "" {
 		ancestry = fmt.Sprintf("%s/project/%s", ancestry, project)
 	}
 	if offline {
-		return &offlineAncestryManager{project: project, ancestry: ancestry}, nil
+		return &offlineAncestryManager{project: project, ancestry: ancestry, resourceAncestryManager: resourceAncestryManager{errorLogger: errorLogger}}, nil
 	}
-	am := &onlineAncestryManager{ancestryCache: map[string]string{}}
+	am := &onlineAncestryManager{ancestryCache: map[string]string{}, resourceAncestryManager: resourceAncestryManager{errorLogger: errorLogger}}
 	am.store(project, ancestry)
 	rm := retriever.NewResourceManagerClient(userAgent)
 	am.resourceManager = rm
diff --git a/cmd/convert.go b/cmd/convert.go
index bb1bc64c..557fe3f2 100644
--- a/cmd/convert.go
+++ b/cmd/convert.go
@@ -89,7 +89,7 @@ func (o *convertOptions) validateArgs(args []string) error {
 
 func (o *convertOptions) run(plan string) error {
 	ctx := context.Background()
-	assets, err := o.readPlannedAssets(ctx, plan, o.project, o.ancestry, o.offline, false)
+	assets, err := o.readPlannedAssets(ctx, plan, o.project, o.ancestry, o.offline, false, o.errorLogger)
 	if err != nil {
 		if errors.Cause(err) == tfgcv.ErrParsingProviderProject {
 			return errors.New("unable to parse provider project, please use --project flag")
diff --git a/cmd/validate.go b/cmd/validate.go
index 5d2caca8..2fa3b6e5 100644
--- a/cmd/validate.go
+++ b/cmd/validate.go
@@ -103,7 +103,7 @@ func (o *validateOptions) validateArgs(args []string) error {
 
 func (o *validateOptions) run(plan string) error {
 	ctx := context.Background()
-	assets, err := o.readPlannedAssets(ctx, plan, o.project, o.ancestry, o.offline, false)
+	assets, err := o.readPlannedAssets(ctx, plan, o.project, o.ancestry, o.offline, false, o.errorLogger)
 	if err != nil {
 		if errors.Cause(err) == tfgcv.ErrParsingProviderProject {
 			return errors.New("unable to parse provider project, please use --project flag")
diff --git a/converters/google/convert.go b/converters/google/convert.go
index 2b335292..c41bbe62 100644
--- a/converters/google/convert.go
+++ b/converters/google/convert.go
@@ -21,7 +21,6 @@ import (
 	"strings"
 	"time"
 
-	"github.com/golang/glog"
 	tfjson "github.com/hashicorp/terraform-json"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
 	provider "github.com/hashicorp/terraform-provider-google/v3/google"
@@ -30,6 +29,7 @@ import (
 	converter "github.com/GoogleCloudPlatform/terraform-google-conversion/google"
 	"github.com/GoogleCloudPlatform/terraform-validator/ancestrymanager"
 	"github.com/GoogleCloudPlatform/terraform-validator/tfplan"
+	"go.uber.org/zap"
 )
 
 var ErrDuplicateAsset = errors.New("duplicate asset")
@@ -126,7 +126,7 @@ type RestoreDefault struct {
 }
 
 // NewConverter is a factory function for Converter.
-func NewConverter(cfg *converter.Config, ancestryManager ancestrymanager.AncestryManager, offline bool, convertUnchanged bool) *Converter {
+func NewConverter(cfg *converter.Config, ancestryManager ancestrymanager.AncestryManager, offline bool, convertUnchanged bool, errorLogger *zap.Logger) *Converter {
 	return &Converter{
 		schema:           provider.Provider(),
 		mapperFuncs:      converter.Mappers(),
@@ -135,6 +135,7 @@ func NewConverter(cfg *converter.Config, ancestryManager ancestrymanager.Ancestr
 		ancestryManager:  ancestryManager,
 		assets:           make(map[string]Asset),
 		convertUnchanged: convertUnchanged,
+		errorLogger:      errorLogger,
 	}
 }
 
@@ -158,6 +159,9 @@ type Converter struct {
 
 	// When set, Converter will convert ResourceChanges with no-op "actions".
 	convertUnchanged bool
+
+	// For logging error / status information that doesn't warrant an outright failure
+	errorLogger *zap.Logger
 }
 
 // Schemas exposes the schemas of resources this converter knows about.
@@ -187,13 +191,13 @@ func (c *Converter) AddResourceChanges(changes []*tfjson.ResourceChange) error {
 	for _, rc := range changes {
 		// skip unknown resources
 		if _, ok := c.schema.ResourcesMap[rc.Type]; !ok {
-			glog.Infof("unknown resource: %s", rc.Type)
+			c.errorLogger.Info(fmt.Sprintf("unknown resource: %s", rc.Type))
 			continue
 		}
 
 		// Skip unsupported resources
 		if _, ok := c.mapperFuncs[rc.Type]; !ok {
-			glog.Infof("unsupported resource: %s", rc.Type)
+			c.errorLogger.Info(fmt.Sprintf("unsupported resource: %s", rc.Type))
 			continue
 		}
 
@@ -209,7 +213,7 @@ func (c *Converter) AddResourceChanges(changes []*tfjson.ResourceChange) error {
 	for _, rc := range createOrUpdateOrNoops {
 		if err := c.addCreateOrUpdateOrNoop(rc); err != nil {
 			if errorssyslib.Is(err, ErrDuplicateAsset) {
-				glog.Warningf("adding resource change: %v", err)
+				c.errorLogger.Warn(fmt.Sprintf("adding resource change: %v", err))
 			} else {
 				return fmt.Errorf("adding resource create/update/no-op %w", err)
 			}
@@ -252,7 +256,7 @@ func (c *Converter) addDelete(rc *tfjson.ResourceChange) error {
 			} else if !c.offline {
 				asset, err := mapper.Fetch(&rd, c.cfg)
 				if errors.Cause(err) == converter.ErrEmptyIdentityField {
-					glog.Warningf("%s did not return a value for ID field. Skipping asset fetch.", key)
+					c.errorLogger.Warn(fmt.Sprintf("%s did not return a value for ID field. Skipping asset fetch.", key))
 					existingConverterAsset = nil
 				} else if err != nil {
 					return errors.Wrap(err, "fetching asset")
@@ -303,7 +307,7 @@ func (c *Converter) addCreateOrUpdateOrNoop(rc *tfjson.ResourceChange) error {
 			} else if mapper.Fetch != nil && !c.offline {
 				asset, err := mapper.Fetch(&rd, c.cfg)
 				if errors.Cause(err) == converter.ErrEmptyIdentityField {
-					glog.Warningf("%s did not return a value for ID field. Skipping asset fetch.", key)
+					c.errorLogger.Warn(fmt.Sprintf("%s did not return a value for ID field. Skipping asset fetch.", key))
 					existingConverterAsset = nil
 				} else if err != nil {
 					return errors.Wrap(err, "fetching asset")
@@ -350,7 +354,7 @@ func (c *Converter) Assets() []Asset {
 
 // augmentAsset adds data to an asset that is not set by the conversion library.
 func (c *Converter) augmentAsset(tfData converter.TerraformResourceData, cfg *converter.Config, cai converter.Asset) (Asset, error) {
-	project, err := getProject(tfData, cfg, cai)
+	project, err := getProject(tfData, cfg, cai, c.errorLogger)
 	if err != nil {
 		return Asset{}, fmt.Errorf("getting project for %v: %w", cai.Name, err)
 	}
diff --git a/converters/google/vendor_utils.go b/converters/google/vendor_utils.go
index 8e46dcb6..6401d269 100644
--- a/converters/google/vendor_utils.go
+++ b/converters/google/vendor_utils.go
@@ -16,9 +16,9 @@ package google
 
 import (
 	"fmt"
-	"log"
 
 	converter "github.com/GoogleCloudPlatform/terraform-google-conversion/google"
+	"go.uber.org/zap"
 )
 
 // NOTE: These functions were pulled from github.com/terraform-providers/terraform-provider-google. They can go away when the functionality they are providing is implemented in the future github.com/GoogleCloudPlatform/terraform-converters package.
@@ -26,7 +26,8 @@ import (
 // getProject reads the "project" field from the given resource data and falls
 // back to the provider's value if not given. If the provider's value is not
 // given, an error is returned.
-func getProject(d converter.TerraformResourceData, config *converter.Config, cai converter.Asset) (string, error) {
+func getProject(d converter.TerraformResourceData, config *converter.Config, cai converter.Asset, errorLogger *zap.Logger) (string, error) {
+
 	switch cai.Type {
 	case "cloudresourcemanager.googleapis.com/Project",
 		"cloudbilling.googleapis.com/ProjectBillingInfo":
@@ -39,7 +40,7 @@ func getProject(d converter.TerraformResourceData, config *converter.Config, cai
 		if ok {
 			return res.(string), nil
 		} else {
-			log.Printf("[WARN] Failed to retrieve project_id for %s from resource", cai.Name)
+			errorLogger.Warn(fmt.Sprintf("Failed to retrieve project_id for %s from resource", cai.Name))
 		}
 	case "storage.googleapis.com/Bucket":
 		if cai.Resource != nil {
@@ -48,7 +49,7 @@ func getProject(d converter.TerraformResourceData, config *converter.Config, cai
 				return res.(string), nil
 			}
 		}
-		log.Printf("[WARN] Failed to retrieve project_id for %s from cai resource", cai.Name)
+		errorLogger.Warn(fmt.Sprintf("Failed to retrieve project_id for %s from cai resource", cai.Name))
 	}
 
 	return getProjectFromSchema("project", d, config)
diff --git a/tfgcv/planned_assets.go b/tfgcv/planned_assets.go
index 69bc8a9d..5a2b84cc 100644
--- a/tfgcv/planned_assets.go
+++ b/tfgcv/planned_assets.go
@@ -25,9 +25,10 @@ import (
 	"github.com/GoogleCloudPlatform/terraform-validator/converters/google"
 	"github.com/GoogleCloudPlatform/terraform-validator/tfplan"
 	"github.com/pkg/errors"
+	"go.uber.org/zap"
 )
 
-type ReadPlannedAssetsFunc func(ctx context.Context, path, project, ancestry string, offline, convertUnchanged bool) ([]google.Asset, error)
+type ReadPlannedAssetsFunc func(ctx context.Context, path, project, ancestry string, offline, convertUnchanged bool, errorLogger *zap.Logger) ([]google.Asset, error)
 
 // ReadPlannedAssets extracts CAI assets from a terraform plan file.
 // If ancestry path is provided, it assumes the project is in that path rather
@@ -36,8 +37,8 @@ type ReadPlannedAssetsFunc func(ctx context.Context, path, project, ancestry str
 // are also reported in the output, otherwise only resources that are going to
 // be changed are reported.
 // It ignores non-supported resources.
-func ReadPlannedAssets(ctx context.Context, path, project, ancestry string, offline, convertUnchanged bool) ([]google.Asset, error) {
-	converter, err := newConverter(ctx, path, project, ancestry, offline, convertUnchanged)
+func ReadPlannedAssets(ctx context.Context, path, project, ancestry string, offline, convertUnchanged bool, errorLogger *zap.Logger) ([]google.Asset, error) {
+	converter, err := newConverter(ctx, path, project, ancestry, offline, convertUnchanged, errorLogger)
 	if err != nil {
 		return nil, err
 	}
@@ -60,17 +61,17 @@ func ReadPlannedAssets(ctx context.Context, path, project, ancestry string, offl
 	return converter.Assets(), nil
 }
 
-func newConverter(ctx context.Context, path, project, ancestry string, offline, convertUnchanged bool) (*google.Converter, error) {
+func newConverter(ctx context.Context, path, project, ancestry string, offline, convertUnchanged bool, errorLogger *zap.Logger) (*google.Converter, error) {
 	cfg, err := cnvconfig.GetConfig(ctx, project, offline)
 	if err != nil {
 		return nil, errors.Wrap(err, "building google configuration")
 	}
 	ua := fmt.Sprintf("config-validator-tf/%s", BuildVersion())
-	ancestryManager, err := ancestrymanager.New(cfg, project, ancestry, ua, offline)
+	ancestryManager, err := ancestrymanager.New(cfg, project, ancestry, ua, offline, errorLogger)
 	if err != nil {
 		return nil, errors.Wrap(err, "building google ancestry manager")
 	}
-	converter := google.NewConverter(cfg, ancestryManager, offline, convertUnchanged)
+	converter := google.NewConverter(cfg, ancestryManager, offline, convertUnchanged, errorLogger)
 	return converter, nil
 }
 


Test patch:
diff --git a/ancestrymanager/ancestrymanager_test.go b/ancestrymanager/ancestrymanager_test.go
index 1720d4aa..5022a696 100644
--- a/ancestrymanager/ancestrymanager_test.go
+++ b/ancestrymanager/ancestrymanager_test.go
@@ -7,13 +7,15 @@ import (
 	"regexp"
 	"testing"
 
+	"go.uber.org/zap"
 	cloudresourcemanager "google.golang.org/api/cloudresourcemanager/v1"
 	"google.golang.org/api/option"
 )
 
 type testRetriever struct {
-	online bool
-	opts   []option.ClientOption
+	online      bool
+	opts        []option.ClientOption
+	errorLogger *zap.Logger
 }
 
 func (tr *testRetriever) NewResourceManagerClient(userAgent string) *cloudresourcemanager.Service {
@@ -94,13 +96,13 @@ func TestGetAncestry(t *testing.T) {
 
 	// 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)
+	amOnline, err := New(trOnline, ownerProject, ownerAncestry, "", false, zap.NewExample())
 	if err != nil {
 		t.Fatalf("failed to create online ancestry manager: %s", err)
 	}
 
 	trOffline := &testRetriever{online: false}
-	amOffline, err := New(trOffline, ownerProject, ownerAncestry, "", true)
+	amOffline, err := New(trOffline, ownerProject, ownerAncestry, "", true, zap.NewExample())
 	if err != nil {
 		t.Fatalf("failed to create offline ancestry manager: %s", err)
 	}
diff --git a/cmd/convert_test.go b/cmd/convert_test.go
index 47ea785b..b931dcb4 100644
--- a/cmd/convert_test.go
+++ b/cmd/convert_test.go
@@ -6,9 +6,10 @@ import (
 
 	"github.com/GoogleCloudPlatform/terraform-validator/converters/google"
 	"github.com/stretchr/testify/assert"
+	"go.uber.org/zap"
 )
 
-func MockReadPlannedAssets(ctx context.Context, path, project, ancestry string, offline, convertUnchanged bool) ([]google.Asset, error) {
+func MockReadPlannedAssets(ctx context.Context, path, project, ancestry string, offline, convertUnchanged bool, errorLogger *zap.Logger) ([]google.Asset, error) {
 	return []google.Asset{
 		google.Asset{
 			Name: "//compute.googleapis.com/projects/my-project/zones/us-central1-a/disks/test-disk",
diff --git a/converters/google/convert_test.go b/converters/google/convert_test.go
index 0e337816..57e5bba7 100644
--- a/converters/google/convert_test.go
+++ b/converters/google/convert_test.go
@@ -25,6 +25,7 @@ import (
 	tfjson "github.com/hashicorp/terraform-json"
 	"github.com/pkg/errors"
 	"github.com/stretchr/testify/assert"
+	"go.uber.org/zap"
 )
 
 const testProject = "test-project"
@@ -39,11 +40,12 @@ func newTestConverter(convertUnchanged bool) (*Converter, error) {
 	if err != nil {
 		return nil, errors.Wrap(err, "constructing configuration")
 	}
-	ancestryManager, err := ancestrymanager.New(cfg, project, ancestry, ua, offline)
+	errorLogger := zap.NewExample()
+	ancestryManager, err := ancestrymanager.New(cfg, project, ancestry, ua, offline, errorLogger)
 	if err != nil {
 		return nil, errors.Wrap(err, "constructing resource ancestryManager")
 	}
-	c := NewConverter(cfg, ancestryManager, offline, convertUnchanged)
+	c := NewConverter(cfg, ancestryManager, offline, convertUnchanged, errorLogger)
 	return c, nil
 }
 
diff --git a/test/read_test.go b/test/read_test.go
index 4bec634f..85b5e736 100644
--- a/test/read_test.go
+++ b/test/read_test.go
@@ -13,6 +13,7 @@ import (
 
 	"github.com/GoogleCloudPlatform/terraform-validator/converters/google"
 	"github.com/GoogleCloudPlatform/terraform-validator/tfgcv"
+	"go.uber.org/zap"
 )
 
 func TestReadPlannedAssetsCoverage(t *testing.T) {
@@ -92,7 +93,7 @@ func TestReadPlannedAssetsCoverage(t *testing.T) {
 
 			planfile := filepath.Join(dir, c.name+".tfplan.json")
 			ctx := context.Background()
-			got, err := tfgcv.ReadPlannedAssets(ctx, planfile, data.Provider["project"], data.Ancestry, true, false)
+			got, err := tfgcv.ReadPlannedAssets(ctx, planfile, data.Provider["project"], data.Ancestry, true, false, zap.NewExample())
 			if err != nil {
 				t.Fatalf("ReadPlannedAssets(%s, %s, %s, %t): %v", planfile, data.Provider["project"], data.Ancestry, true, err)
 			}
diff --git a/tfgcv/planned_assets_test.go b/tfgcv/planned_assets_test.go
index b91f7b26..2d385816 100644
--- a/tfgcv/planned_assets_test.go
+++ b/tfgcv/planned_assets_test.go
@@ -2,6 +2,7 @@ package tfgcv
 
 import (
 	"context"
+	"go.uber.org/zap"
 	"path/filepath"
 	"testing"
 )
@@ -76,7 +77,7 @@ func TestReadPlannedAssets(t *testing.T) {
 			var offline bool
 			offline = true
 			ctx := context.Background()
-			got, err := ReadPlannedAssets(ctx, testFile, tt.args.project, tt.args.ancestry, offline, tt.args.convertUnchanged)
+			got, err := ReadPlannedAssets(ctx, testFile, tt.args.project, tt.args.ancestry, offline, tt.args.convertUnchanged, zap.NewExample())
 			if (err != nil) != tt.wantErr {
 				t.Errorf("ReadPlannedAssets() error = %v, wantErr %v", err, tt.wantErr)
 				return