Repository: pulumi/pulumi-terraform-bridge (go)

Issue:
Include property name in Check errors
Currently, the `sdk-v2` and `tfplugin5` shims do not include the `AttributePath` of a Terraform `Diagnostic` in the error message reported back from `Check` in Pulumi.  For some validation errors, it is likely this information is necessary for a helpful/actionable error.

This is a follow-up to #310 and #309.

Pull request:
Initial attempt to add attribute info for check errors
Fixes #311

The PR currently gets additional context on check failures but still has some contextual gaps:
1. No insight into the user program's language so "pulumified" attributes may not directly match the user's program. Can't really use the provided input either since the problem may be due to required attributes not being specified.
2. Only supports v2 and tf5 shims. v1 doesn't seem to emit attribute information.

Patch:
diff --git a/pkg/tfbridge/names.go b/pkg/tfbridge/names.go
index ddac9f2e..2ee28125 100644
--- a/pkg/tfbridge/names.go
+++ b/pkg/tfbridge/names.go
@@ -55,7 +55,7 @@ func PulumiToTerraformName(name string, tfs shim.SchemaMap, ps map[string]*Schem
 			}
 		}
 
-		if ok && checkTfMaxItems(sch, false) || isPulmiMaxItemsOne(info) {
+		if ok && checkTfMaxItems(sch, false) || isPulumiMaxItemsOne(info) {
 			result = singularResult
 		}
 	}
@@ -74,7 +74,7 @@ func checkTfMaxItems(tfs shim.Schema, maxItemsOne bool) bool {
 	return (tfs.MaxItems() == 1) == maxItemsOne
 }
 
-func isPulmiMaxItemsOne(ps *SchemaInfo) bool {
+func isPulumiMaxItemsOne(ps *SchemaInfo) bool {
 	return ps != nil && ps.MaxItemsOne != nil && *ps.MaxItemsOne
 }
 
@@ -86,7 +86,7 @@ func TerraformToPulumiName(name string, sch shim.Schema, ps *SchemaInfo, upper b
 	var prev rune
 
 	// Pluralize names that will become array-shaped Pulumi values
-	if !isPulmiMaxItemsOne(ps) && checkTfMaxItems(sch, false) {
+	if !isPulumiMaxItemsOne(ps) && checkTfMaxItems(sch, false) {
 		pluralized := inflector.Pluralize(name)
 		if pluralized == name || inflector.Singularize(pluralized) == name {
 			//			contract.Assertf(
diff --git a/pkg/tfbridge/provider.go b/pkg/tfbridge/provider.go
index d6e97633..6dd941ed 100644
--- a/pkg/tfbridge/provider.go
+++ b/pkg/tfbridge/provider.go
@@ -17,6 +17,8 @@ package tfbridge
 import (
 	"encoding/json"
 	"fmt"
+	"github.com/hashicorp/go-cty/cty"
+	"github.com/pulumi/pulumi-terraform-bridge/v2/pkg/tfshim/diagnostics"
 	"log"
 	"regexp"
 	"strings"
@@ -555,7 +557,18 @@ func (p *Provider) Configure(ctx context.Context,
 // https://github.com/hashicorp/terraform/blob/7f5ffbfe9027c34c4ce1062a42b6e8d80b5504e0/helper/schema/schema.go#L1356
 var requiredFieldRegex = regexp.MustCompile("\"(.*?)\": required field is not set")
 
-func (p *Provider) formatFailureReason(res Resource, reason string) string {
+func (p *Provider) formatFailureReason(tokenType tokens.Type, res Resource, err error) string {
+	reason := err.Error()
+	var attributePath string
+	var d *diagnostics.ValidationError
+	if errors.As(err, &d) {
+		path := d.AttributePath
+		if len(path) > 0 {
+			attrPath := pathToAttributePath(path, tokenType, res)
+			attributePath = strings.Join(attrPath, "")
+		}
+	}
+
 	// Translate the name in missing-required-field error from TF to Pulumi naming scheme
 	parts := requiredFieldRegex.FindStringSubmatch(reason)
 	if len(parts) == 2 {
@@ -577,9 +590,56 @@ func (p *Provider) formatFailureReason(res Resource, reason string) string {
 		}
 	}
 
+	if attributePath != "" {
+		reason += fmt.Sprintf(". Examine values at '%s'.", attributePath)
+	}
 	return reason
 }
 
+// pathToAttributePath takes a cty.Path and translates it to a path compatible with the Pulumi schema.
+func pathToAttributePath(p cty.Path, tokenType tokens.Type, res Resource) []string {
+	res.Schema.GetTok()
+	ap := []string{tokenType.Name().String()}
+	var schema shim.Schema
+	var info *SchemaInfo
+	for _, step := range p {
+		switch selector := step.(type) {
+		case cty.GetAttrStep:
+			ap = append(ap, ".")
+			if schema == nil {
+				schema = getSchema(res.TF.Schema(), selector.Name)
+				info = res.Schema.Fields[selector.Name]
+			} else {
+				schema, info = elemSchemas(schema, info)
+			}
+			name := TerraformToPulumiName(selector.Name, schema, info, true)
+			ap = append(ap, name)
+		case cty.IndexStep:
+			// list type with max items 1 are collapsed.
+			if IsMaxItemsOne(schema, info) {
+				schema, info = elemSchemas(schema, info)
+				continue
+			}
+			key := selector.Key
+			if key.IsNull() {
+				continue
+			}
+			switch key.Type() {
+			case cty.String:
+				ap = append(ap, fmt.Sprintf("[\"%s\"]", key.AsString()))
+			case cty.Number:
+				i, _ := key.AsBigFloat().Int64()
+				ap = append(ap, fmt.Sprintf("[%d]", i))
+			default:
+				// We'll bail early if we encounter anything else, and just
+				// return the valid prefix.
+				return ap
+			}
+		}
+	}
+	return ap
+}
+
 // Check validates that the given property bag is valid for a resource of the given type.
 func (p *Provider) Check(ctx context.Context, req *pulumirpc.CheckRequest) (*pulumirpc.CheckResponse, error) {
 	p.setLoggingContext(ctx)
@@ -632,7 +692,7 @@ func (p *Provider) Check(ctx context.Context, req *pulumirpc.CheckRequest) (*pul
 	var failures []*pulumirpc.CheckFailure
 	for _, err := range errs {
 		failures = append(failures, &pulumirpc.CheckFailure{
-			Reason: p.formatFailureReason(res, err.Error()),
+			Reason: p.formatFailureReason(t, res, err),
 		})
 	}
 
diff --git a/pkg/tfshim/diagnostics/error.go b/pkg/tfshim/diagnostics/error.go
new file mode 100644
index 00000000..61b6d928
--- /dev/null
+++ b/pkg/tfshim/diagnostics/error.go
@@ -0,0 +1,20 @@
+package diagnostics
+
+import (
+	"fmt"
+	"github.com/hashicorp/go-cty/cty"
+)
+
+// ValidationError wraps validation errors reported by shims (currently shim v2 and tf5)
+type ValidationError struct {
+	AttributePath cty.Path
+	Summary       string
+	Detail        string
+}
+
+func (e ValidationError) Error() string {
+	if e.Detail != "" {
+		return fmt.Sprintf("%s: %s", e.Summary, e.Detail)
+	}
+	return e.Summary
+}
diff --git a/pkg/tfshim/sdk-v2/diagnostics.go b/pkg/tfshim/sdk-v2/diagnostics.go
index 6db266c9..3da00939 100644
--- a/pkg/tfshim/sdk-v2/diagnostics.go
+++ b/pkg/tfshim/sdk-v2/diagnostics.go
@@ -1,10 +1,9 @@
 package sdkv2
 
 import (
-	"fmt"
-
 	"github.com/hashicorp/go-multierror"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/diag"
+	"github.com/pulumi/pulumi-terraform-bridge/v2/pkg/tfshim/diagnostics"
 )
 
 func warningsAndErrors(diags diag.Diagnostics) ([]string, []error) {
@@ -13,11 +12,7 @@ func warningsAndErrors(diags diag.Diagnostics) ([]string, []error) {
 	for _, d := range diags {
 		switch d.Severity {
 		case diag.Error:
-			if d.Detail != "" {
-				errors = append(errors, fmt.Errorf("%s: %s", d.Summary, d.Detail))
-			} else {
-				errors = append(errors, fmt.Errorf("%s", d.Summary))
-			}
+			errors = append(errors, fromV2Diag(d))
 		case diag.Warning:
 			warnings = append(warnings, d.Summary)
 		}
@@ -29,12 +24,17 @@ func errors(diags diag.Diagnostics) error {
 	var err error
 	for _, d := range diags {
 		if d.Severity == diag.Error {
-			if d.Detail != "" {
-				err = multierror.Append(err, fmt.Errorf("%s: %s", d.Summary, d.Detail))
-			} else {
-				err = multierror.Append(err, fmt.Errorf("%s", d.Summary))
-			}
+			err = multierror.Append(err, fromV2Diag(d))
 		}
 	}
 	return err
 }
+
+func fromV2Diag(diagnostic diag.Diagnostic) error {
+	return &diagnostics.ValidationError{
+		AttributePath: diagnostic.AttributePath,
+		Summary:       diagnostic.Summary,
+		Detail:        diagnostic.Detail,
+	}
+}
+
diff --git a/pkg/tfshim/tfplugin5/diagnostics.go b/pkg/tfshim/tfplugin5/diagnostics.go
index e867437f..12568fb4 100644
--- a/pkg/tfshim/tfplugin5/diagnostics.go
+++ b/pkg/tfshim/tfplugin5/diagnostics.go
@@ -1,9 +1,9 @@
 package tfplugin5
 
 import (
-	"fmt"
-
+	"github.com/hashicorp/go-cty/cty"
 	"github.com/hashicorp/go-multierror"
+	"github.com/pulumi/pulumi-terraform-bridge/v2/pkg/tfshim/diagnostics"
 
 	"github.com/pulumi/pulumi-terraform-bridge/v2/pkg/tfshim/tfplugin5/proto"
 )
@@ -16,11 +16,7 @@ func unmarshalWarningsAndErrors(diags []*proto.Diagnostic) ([]string, []error) {
 	for _, d := range diags {
 		switch d.Severity {
 		case proto.Diagnostic_ERROR:
-			if d.Detail != "" {
-				errors = append(errors, fmt.Errorf("%s: %s", d.Summary, d.Detail))
-			} else {
-				errors = append(errors, fmt.Errorf("%s", d.Summary))
-			}
+			errors = append(errors, fromTF5ProtoDiag(d))
 		case proto.Diagnostic_WARNING:
 			warnings = append(warnings, d.Summary)
 		}
@@ -34,12 +30,34 @@ func unmarshalErrors(diags []*proto.Diagnostic) error {
 	var err error
 	for _, d := range diags {
 		if d.Severity == proto.Diagnostic_ERROR {
-			if d.Detail != "" {
-				err = multierror.Append(err, fmt.Errorf("%s: %s", d.Summary, d.Detail))
-			} else {
-				err = multierror.Append(err, fmt.Errorf("%s", d.Summary))
-			}
+			err = multierror.Append(err, fromTF5ProtoDiag(d))
 		}
 	}
 	return err
 }
+
+func fromTF5ProtoDiag(diagnostic *proto.Diagnostic) error {
+	return &diagnostics.ValidationError{
+		AttributePath: pathToCty(diagnostic.Attribute),
+		Summary:       diagnostic.Summary,
+		Detail:        diagnostic.Detail,
+	}
+}
+
+func pathToCty(path *proto.AttributePath) cty.Path {
+	var p cty.Path
+	if path == nil {
+		return p
+	}
+	for _, s := range path.Steps {
+		switch s := s.Selector.(type) {
+		case *proto.AttributePath_Step_AttributeName:
+			p = p.GetAttr(s.AttributeName)
+		case *proto.AttributePath_Step_ElementKeyString:
+			p = p.IndexString(s.ElementKeyString)
+		case *proto.AttributePath_Step_ElementKeyInt:
+			p = p.Index(cty.NumberIntVal(s.ElementKeyInt))
+		}
+	}
+	return p
+}


Test patch:
diff --git a/pkg/tfbridge/provider_test.go b/pkg/tfbridge/provider_test.go
index 457efd95..62c15f65 100644
--- a/pkg/tfbridge/provider_test.go
+++ b/pkg/tfbridge/provider_test.go
@@ -512,11 +512,11 @@ func TestProviderCheckV2(t *testing.T) {
 
 	failures := testCheckFailures(t, provider, "SecondResource")
 	sort.SliceStable(failures, func(i, j int) bool { return failures[i].Reason < failures[j].Reason })
-	assert.Equal(t, "ConflictsWith: \"conflicting_property\": conflicts with conflicting_property2", failures[0].Reason)
+	assert.Equal(t, "ConflictsWith: \"conflicting_property\": conflicts with conflicting_property2. Examine values at 'SecondResource.ConflictingProperty'.", failures[0].Reason)
 	assert.Equal(t, "", failures[0].Property)
-	assert.Equal(t, "ConflictsWith: \"conflicting_property2\": conflicts with conflicting_property", failures[1].Reason)
+	assert.Equal(t, "ConflictsWith: \"conflicting_property2\": conflicts with conflicting_property. Examine values at 'SecondResource.ConflictingProperty2'.", failures[1].Reason)
 	assert.Equal(t, "", failures[1].Property)
-	assert.Equal(t, "Required attribute is not set", failures[2].Reason)
+	assert.Equal(t, "Required attribute is not set. Examine values at 'SecondResource.ArrayPropertyValues'.", failures[2].Reason)
 	assert.Equal(t, "", failures[2].Property)
 }
 
diff --git a/pkg/tfshim/tfplugin5/diagnostics_test.go b/pkg/tfshim/tfplugin5/diagnostics_test.go
index 7c50421b..c9c3b1c8 100644
--- a/pkg/tfshim/tfplugin5/diagnostics_test.go
+++ b/pkg/tfshim/tfplugin5/diagnostics_test.go
@@ -1,7 +1,7 @@
 package tfplugin5
 
 import (
-	"fmt"
+	"github.com/pulumi/pulumi-terraform-bridge/v2/pkg/tfshim/diagnostics"
 	"testing"
 
 	"github.com/hashicorp/go-multierror"
@@ -29,11 +29,15 @@ func TestWarningsAndErrors(t *testing.T) {
 
 	warnings, errors = unmarshalWarningsAndErrors(errorsOnly)
 	assert.Empty(t, warnings)
-	assert.Equal(t, []error{fmt.Errorf("error 1"), fmt.Errorf("error 2")}, errors)
+	assert.Equal(t, errors, []error{&diagnostics.ValidationError{Summary: "error 1"}, &diagnostics.ValidationError{Summary: "error 2"}})
+	assert.EqualError(t, errors[0], "error 1")
+	assert.EqualError(t, errors[1], "error 2")
 
 	warnings, errors = unmarshalWarningsAndErrors(mixed)
 	assert.Equal(t, []string{"warning 1", "warning 2"}, warnings)
-	assert.Equal(t, []error{fmt.Errorf("error 1"), fmt.Errorf("error 2")}, errors)
+	assert.Equal(t, errors, []error{&diagnostics.ValidationError{Summary: "error 1"}, &diagnostics.ValidationError{Summary: "error 2"}})
+	assert.EqualError(t, errors[0], "error 1")
+	assert.EqualError(t, errors[1], "error 2")
 }
 
 func TestErrors(t *testing.T) {
@@ -41,8 +45,8 @@ func TestErrors(t *testing.T) {
 	assert.NoError(t, err)
 
 	err = unmarshalErrors(errorsOnly)
-	assert.Equal(t, multierror.Append(nil, fmt.Errorf("error 1"), fmt.Errorf("error 2")), err)
+	assert.Equal(t, multierror.Append(nil, &diagnostics.ValidationError{Summary: "error 1"}, &diagnostics.ValidationError{Summary: "error 2"}), err)
 
 	err = unmarshalErrors(mixed)
-	assert.Equal(t, multierror.Append(nil, fmt.Errorf("error 1"), fmt.Errorf("error 2")), err)
+	assert.Equal(t, multierror.Append(nil, &diagnostics.ValidationError{Summary: "error 1"}, &diagnostics.ValidationError{Summary: "error 2"}), err)
 }
diff --git a/pkg/tfshim/tfplugin5/provider_test.go b/pkg/tfshim/tfplugin5/provider_test.go
index c27c4523..4506bdf9 100644
--- a/pkg/tfshim/tfplugin5/provider_test.go
+++ b/pkg/tfshim/tfplugin5/provider_test.go
@@ -2,12 +2,14 @@ package tfplugin5
 
 import (
 	"bytes"
+	"github.com/pulumi/pulumi-terraform-bridge/v2/pkg/tfshim/diagnostics"
 	"io"
 	"log"
 	"os"
 	"os/exec"
 	"strconv"
 	"testing"
+	goerrors "errors"
 
 	"github.com/hashicorp/go-cty/cty"
 	"github.com/hashicorp/go-hclog"
@@ -732,6 +734,21 @@ func TestValidateResource(t *testing.T) {
 	}))
 	assert.Empty(t, warnings)
 	assert.Empty(t, errors)
+
+	var err *diagnostics.ValidationError
+	warnings, errors = p.ValidateResource("example_resource", p.NewResourceConfig(map[string]interface{}{
+		// missing required array_property_value
+	}))
+	assert.Empty(t, warnings)
+	assert.Len(t, errors, 1)
+	if goerrors.As(errors[0], &err) {
+		assert.Equal(t, &diagnostics.ValidationError{
+			Summary: "Required attribute is not set",
+			AttributePath: cty.GetAttrPath("array_property_value"),
+		}, err)
+	} else {
+		t.Error("Validate missing required property")
+	}
 }
 
 func TestValidateDataSource(t *testing.T) {