Repository: GoogleCloudPlatform/terraform-validator
Pull request #177 (2021-02-09 22:32:48)

Fully moved responsibility for iterating over and filtering resource changes to converter
This change allows the converter to filter out unsupported resource changes (this was partially handled by `tfplan.jsonToResources` prior to #176) and to have a specific internal function that is only responsible for adding a resource change that is known to be a create or update to the converter's internal asset map (which means we will be able to have a separate internal function to handle merging in deletes, and a separate internal function for pre-fetching things like IAM.)

This is slightly different than the design doc, which called for detecting create&update vs delete inside AddResourceChange; however, I think this is a better separation of concerns.


Patch:
diff --git a/converters/google/convert.go b/converters/google/convert.go
index 02369d0a..96c29f11 100644
--- a/converters/google/convert.go
+++ b/converters/google/convert.go
@@ -21,6 +21,7 @@ import (
 	"time"
 
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
+	"github.com/golang/glog"
 	provider "github.com/hashicorp/terraform-provider-google/v3/google"
 	"github.com/pkg/errors"
 
@@ -31,26 +32,6 @@ import (
 
 var ErrDuplicateAsset = errors.New("duplicate asset")
 
-// TerraformResource represents the required methods needed to convert a terraform
-// resource into an Asset type.
-type TerraformResource interface {
-	Id() string
-	Kind() string
-	Get(string) interface{}
-	GetOk(string) (interface{}, bool)
-	GetOkExists(string) (interface{}, bool)
-}
-
-// nonImplementedResourceData represents the non-getter fields that are
-// passed to terraform providers that are not implemented here but are
-// required (and not used) by downstream mappers.
-type nonImplementedResourceData struct{}
-
-func (nonImplementedResourceData) HasChange(string) bool             { return false }
-func (nonImplementedResourceData) Set(string, interface{}) error     { return nil }
-func (nonImplementedResourceData) SetId(string)                      {}
-func (nonImplementedResourceData) GetProviderMeta(interface{}) error { return nil }
-
 // Asset contains the resource data and metadata in the same format as
 // Google CAI (Cloud Asset Inventory).
 type Asset struct {
@@ -179,39 +160,49 @@ func (c *Converter) Schemas() map[string]*schema.Resource {
 	return supported
 }
 
-// AddResource converts a terraform resource and stores the converted asset.
+// Compatibility shim: maintain support for ComposeTF12Resources -> AddResource pipeline
 func (c *Converter) AddResource(rc *tfplan.ResourceChange) error {
-	// Compatibility shim: silently do nothing if this resource isn't
-	// being added or changed.
-	if !(rc.IsCreate() || rc.IsUpdate() || rc.IsDeleteCreate()) {
-		return nil
-	}
+	return c.AddResourceChanges([]tfplan.ResourceChange{*rc})
+}
 
-	// Compatibility shim: silently skip unknown resources
-	resource, ok := c.schema.ResourcesMap[rc.Type]
-	if !ok {
-		return nil
-	}
+func (c *Converter) AddResourceChanges(changes []tfplan.ResourceChange) error {
+	for _, rc := range changes {
+		// skip unknown resources
+		if _, ok := c.schema.ResourcesMap[rc.Type]; !ok {
+			glog.Infof("unknown resource: %s", rc.Type)
+			continue
+		}
 
-	// Compatibility shim: silently skip unsupported resources
-	if _, ok := c.mapperFuncs[rc.Type]; !ok {
-		return nil
+		// Skip unsupported resources
+		if _, ok := c.mapperFuncs[rc.Type]; !ok {
+			glog.Infof("unsupported resource: %s", rc.Type)
+			continue
+		}
+
+		if rc.IsCreate() || rc.IsUpdate() || rc.IsDeleteCreate() {
+			if err := c.addCreateOrUpdate(rc); err != nil {
+				if errors.Cause(err) == ErrDuplicateAsset {
+					glog.Warningf("adding resource change: %v", err)
+				} else {
+					return errors.Wrapf(err, "adding resource change")
+				}
+			}
+		}
 	}
 
+	return nil
+}
+
+func (c *Converter) addCreateOrUpdate(rc tfplan.ResourceChange) error {
+	resource, _ := c.schema.ResourcesMap[rc.Type]
 	rd := NewFakeResourceData(
 		rc.Type,
 		resource.Schema,
 		rc.Change.After,
 	)
-	r := &rd
-
-	for _, mapper := range c.mapperFuncs[r.Kind()] {
-		data := struct {
-			TerraformResource
-			nonImplementedResourceData
-		}{TerraformResource: r}
 
-		converted, err := mapper.convert(data, c.cfg)
+	for _, mapper := range c.mapperFuncs[rd.Kind()] {
+		converted, err := mapper.convert(&rd, c.cfg)
 		if err != nil {
 			if errors.Cause(err) == converter.ErrNoConversion {
 				continue
@@ -234,7 +225,7 @@ func (c *Converter) AddResource(rc *tfplan.ResourceChange) error {
 			}
 		}
 
-		augmented, err := c.augmentAsset(data, c.cfg, converted)
+		augmented, err := c.augmentAsset(&rd, c.cfg, converted)
 		if err != nil {
 			return errors.Wrap(err, "augmenting asset")
 		}
diff --git a/converters/google/fake_resource_data.go b/converters/google/fake_resource_data.go
index 86bdcdeb..d5121dfc 100644
--- a/converters/google/fake_resource_data.go
+++ b/converters/google/fake_resource_data.go
@@ -71,6 +71,13 @@ func (d *FakeResourceData) GetOkExists(name string) (interface{}, bool) {
 	return d.GetOk(name)
 }
 
+// These methods are required by some mappers but we don't actually have (or need)
+// implementations for them.
+func (d *FakeResourceData) HasChange(string) bool             { return false }
+func (d *FakeResourceData) Set(string, interface{}) error     { return nil }
+func (d *FakeResourceData) SetId(string)                      {}
+func (d *FakeResourceData) GetProviderMeta(interface{}) error { return nil }
+
 func NewFakeResourceData(kind string, resourceSchema map[string]*schema.Schema, values map[string]interface{}) FakeResourceData {
 	state := map[string]string{}
 	var address []string
diff --git a/tfgcv/planned_assets.go b/tfgcv/planned_assets.go
index 4eac64c1..83c16c66 100644
--- a/tfgcv/planned_assets.go
+++ b/tfgcv/planned_assets.go
@@ -25,7 +25,6 @@ import (
 	"github.com/GoogleCloudPlatform/terraform-validator/ancestrymanager"
 	"github.com/GoogleCloudPlatform/terraform-validator/converters/google"
 	"github.com/GoogleCloudPlatform/terraform-validator/tfplan"
-	"github.com/golang/glog"
 	"github.com/pkg/errors"
 )
 
@@ -44,19 +43,14 @@ func ReadPlannedAssets(ctx context.Context, path, project, ancestry string, offl
 		return nil, err
 	}
 
-	resources, err := tfplan.ComposeTF12Resources(data, converter.Schemas())
+	changes, err := tfplan.ReadResourceChanges(data)
 	if err != nil {
 		return nil, errors.Wrap(err, "reading resource changes")
 	}
 
-	for _, r := range resources {
-		if err := converter.AddResource(&r); err != nil {
-			if errors.Cause(err) == google.ErrDuplicateAsset {
-				glog.Warningf("converting resource: %v", err)
-			} else {
-				return nil, errors.Wrapf(err, "converting resource %v", r.Kind())
-			}
-		}
+	err = converter.AddResourceChanges(changes)
+	if err != nil {
+		return nil, errors.Wrap(err, "adding resource changes to converter")
 	}
 
 	return converter.Assets(), nil
diff --git a/tfplan/json_plan.go b/tfplan/json_plan.go
index 79ad2269..98441181 100644
--- a/tfplan/json_plan.go
+++ b/tfplan/json_plan.go
@@ -26,25 +26,27 @@ type jsonPlan struct {
 	ResourceChanges []ResourceChange `json:"resource_changes"`
 }
 
+type ChangeRepresentation struct {
+	// Valid actions values are:
+	//    ["no-op"]
+	//    ["create"]
+	//    ["read"]
+	//    ["update"]
+	//    ["delete", "create"]
+	//    ["create", "delete"]
+	//    ["delete"]
+	Actions []string               `json:"actions"`
+	Before  map[string]interface{} `json:"before"`
+	After   map[string]interface{} `json:"after"`
+}
+
 type ResourceChange struct {
-	Address      string `json:"address"`
-	Mode         string `json:"mode"`
-	Type         string `json:"type"`
-	Name         string `json:"name"`
-	ProviderName string `json:"provider_name"`
-	Change       struct {
-		// Valid actions values are:
-		//    ["no-op"]
-		//    ["create"]
-		//    ["read"]
-		//    ["update"]
-		//    ["delete", "create"]
-		//    ["create", "delete"]
-		//    ["delete"]
-		Actions []string               `json:"actions"`
-		Before  map[string]interface{} `json:"before"`
-		After   map[string]interface{} `json:"after"`
-	} `json:"change"`
+	Address      string               `json:"address"`
+	Mode         string               `json:"mode"`
+	Type         string               `json:"type"`
+	Name         string               `json:"name"`
+	ProviderName string               `json:"provider_name"`
+	Change       ChangeRepresentation `json:"change"`
 }
 
 func (c *ResourceChange) IsCreate() bool {


Test patch:
diff --git a/converters/google/convert_test.go b/converters/google/convert_test.go
index 3d5f2e38..40341c94 100644
--- a/converters/google/convert_test.go
+++ b/converters/google/convert_test.go
@@ -15,12 +15,34 @@
 package google
 
 import (
+	"context"
 	"sort"
 	"testing"
 
+	"github.com/GoogleCloudPlatform/terraform-validator/ancestrymanager"
+	"github.com/GoogleCloudPlatform/terraform-validator/tfplan"
+	"github.com/pkg/errors"
 	"github.com/stretchr/testify/assert"
 )
 
+const testProject = "test-project"
+
+func newTestConverter() (*Converter, error) {
+	ctx := context.Background()
+	ancestry := ""
+	project := testProject
+	offline := true
+	ancestryManager, err := ancestrymanager.New(context.Background(), project, ancestry, offline)
+	if err != nil {
+		return nil, errors.Wrap(err, "constructing resource manager client")
+	}
+	c, err := NewConverter(ctx, ancestryManager, project, "", offline)
+	if err != nil {
+		return nil, errors.Wrap(err, "building converter")
+	}
+	return c, nil
+}
+
 func TestSortByName(t *testing.T) {
 	cases := []struct {
 		name           string
@@ -47,3 +69,144 @@ func TestSortByName(t *testing.T) {
 		})
 	}
 }
+
+func TestAddResourceChanges_unknownResourceIgnored(t *testing.T) {
+	rc := tfplan.ResourceChange{
+		Address:      "whatever.google_unknown.foo",
+		Mode:         "managed",
+		Type:         "google_unknown",
+		Name:         "foo",
+		ProviderName: "google",
+		Change: tfplan.ChangeRepresentation{
+			Actions: []string{"change"},
+			Before:  nil,
+			After:   nil,
+		},
+	}
+	c, err := newTestConverter()
+	assert.Nil(t, err)
+	err = c.AddResourceChanges([]tfplan.ResourceChange{rc})
+	assert.Nil(t, err)
+	assert.EqualValues(t, map[string]Asset{}, c.assets)
+}
+
+func TestAddResourceChanges_unsupportedResourceIgnored(t *testing.T) {
+	rc := tfplan.ResourceChange{
+		Address:      "whatever.google_unknown.foo",
+		Mode:         "managed",
+		Type:         "google_unsupported",
+		Name:         "foo",
+		ProviderName: "google",
+		Change: tfplan.ChangeRepresentation{
+			Actions: []string{"change"},
+			Before:  nil,
+			After:   nil,
+		},
+	}
+	c, err := newTestConverter()
+	assert.Nil(t, err)
+
+	// fake that this resource is known to the provider; it will never be "supported" by the
+	// converter.
+	c.schema.ResourcesMap[rc.Type] = c.schema.ResourcesMap["google_compute_disk"]
+
+	err = c.AddResourceChanges([]tfplan.ResourceChange{rc})
+	assert.Nil(t, err)
+	assert.EqualValues(t, map[string]Asset{}, c.assets)
+}
+
+func TestAddResourceChanges_noopIgnored(t *testing.T) {
+	rc := tfplan.ResourceChange{
+		Address:      "whatever.google_compute_disk.foo",
+		Mode:         "managed",
+		Type:         "google_compute_disk",
+		Name:         "foo",
+		ProviderName: "google",
+		Change: tfplan.ChangeRepresentation{
+			Actions: []string{"no-op"},
+			Before:  nil,
+			After:   nil,
+		},
+	}
+	c, err := newTestConverter()
+	assert.Nil(t, err)
+
+	err = c.AddResourceChanges([]tfplan.ResourceChange{rc})
+	assert.Nil(t, err)
+	assert.EqualValues(t, map[string]Asset{}, c.assets)
+}
+
+func TestAddResourceChanges_deleteIgnored(t *testing.T) {
+	rc := tfplan.ResourceChange{
+		Address:      "whatever.google_compute_disk.foo",
+		Mode:         "managed",
+		Type:         "google_compute_disk",
+		Name:         "foo",
+		ProviderName: "google",
+		Change: tfplan.ChangeRepresentation{
+			Actions: []string{"delete"},
+			Before:  nil,
+			After:   nil,
+		},
+	}
+	c, err := newTestConverter()
+	assert.Nil(t, err)
+
+	err = c.AddResourceChanges([]tfplan.ResourceChange{rc})
+	assert.Nil(t, err)
+	assert.EqualValues(t, map[string]Asset{}, c.assets)
+}
+
+func TestAddResourceChanges_createOrUpdateOrDeleteCreateProcessed(t *testing.T) {
+	cases := []struct {
+		name    string
+		actions []string
+	}{
+		{
+			name:    "Create",
+			actions: []string{"create"},
+		},
+		{
+			name:    "Update",
+			actions: []string{"update"},
+		},
+		{
+			name:    "DeleteCreate",
+			actions: []string{"delete", "create"},
+		},
+	}
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			rc := tfplan.ResourceChange{
+				Address:      "whatever.google_compute_disk.foo",
+				Mode:         "managed",
+				Type:         "google_compute_disk",
+				Name:         "foo",
+				ProviderName: "google",
+				Change: tfplan.ChangeRepresentation{
+					Actions: c.actions,
+					Before:  nil, // Ignore Before because it's unused
+					After: map[string]interface{}{
+						"project": testProject,
+						"name":    "test-disk",
+						"type":    "pd-ssd",
+						"zone":    "us-central1-a",
+						"image":   "projects/debian-cloud/global/images/debian-8-jessie-v20170523",
+						"labels": map[string]interface{}{
+							"environment": "dev",
+						},
+						"physical_block_size_bytes": 4096,
+					},
+				},
+			}
+			c, err := newTestConverter()
+			assert.Nil(t, err)
+
+			err = c.AddResourceChanges([]tfplan.ResourceChange{rc})
+			assert.Nil(t, err)
+
+			caiKey := "compute.googleapis.com/Disk//compute.googleapis.com/projects/test-project/zones/us-central1-a/disks/test-disk"
+			assert.Contains(t, c.assets, caiKey)
+		})
+	}
+}