use WriteOnlyPaths for validation

ValidateWriteOnlyAttributes did not properly descent through nested
blocks and structural attributes, and would allow invalid values to be
returned from the provider.

The schema already contains code to walk a value in conjunction
with its schema, WriteOnlyAttributes. Now that that method returns the
correct paths, we can just directly validate those paths and don't
need to recursively walk the value again.

internal/lang/ephemeral/validate.go

@@ -4,6 +4,8 @@
 package ephemeral
 
 import (
+	"fmt"
+
 	"github.com/hashicorp/terraform/internal/configs/configschema"
 	"github.com/hashicorp/terraform/internal/tfdiags"
 	"github.com/zclconf/go-cty/cty"
@@ -12,7 +14,15 @@ import (
 // ValidateWriteOnlyAttributes identifies all instances of write-only paths that contain non-null values
 // and returns a diagnostic for each instance
 func ValidateWriteOnlyAttributes(summary string, detail func(cty.Path) string, newVal cty.Value, schema *configschema.Block) (diags tfdiags.Diagnostics) {
-	if writeOnlyPaths := NonNullWriteOnlyPaths(newVal, schema, nil); len(writeOnlyPaths) != 0 {
+	writeOnlyPaths, err := nonNullWriteOnlyPaths(newVal, schema, nil)
+	if err != nil {
+		return diags.Append(tfdiags.Sourceless(
+			tfdiags.Error,
+			summary,
+			fmt.Sprintf("Error validating write-only attributes: %s.", err),
+		))
+	}
+	if len(writeOnlyPaths) != 0 {
 		for _, p := range writeOnlyPaths {
 			diags = diags.Append(tfdiags.Sourceless(
 				tfdiags.Error,
@@ -24,26 +34,27 @@ func ValidateWriteOnlyAttributes(summary string, detail func(cty.Path) string, n
 	return diags
 }
 
-// NonNullWriteOnlyPaths returns a list of paths to attributes that are write-only
+// nonNullWriteOnlyPaths returns a list of paths to attributes that are write-only
 // and non-null in the given value.
-func NonNullWriteOnlyPaths(val cty.Value, schema *configschema.Block, p cty.Path) (paths []cty.Path) {
+func nonNullWriteOnlyPaths(val cty.Value, schema *configschema.Block, p cty.Path) ([]cty.Path, error) {
 	if schema == nil {
-		return paths
+		panic("nonNullWriteOnlyPaths called wih nil schema")
 	}
+	var paths []cty.Path
 
-	for name, attr := range schema.Attributes {
-		attrPath := append(p, cty.GetAttrStep{Name: name})
-		attrVal, _ := attrPath.Apply(val)
-		if attr.WriteOnly && !attrVal.IsNull() {
-			paths = append(paths, attrPath)
+	for _, path := range schema.WriteOnlyPaths(val, nil) {
+		// Note that path.Apply will fail if the path traverses a set, but ephemeral
+		// values won't work in a set anyway, and they are prohibited by the
+		// plugin framework.
+		v, err := path.Apply(val)
+		if err != nil {
+			return nil, err
+		}
+		if !v.IsNull() {
+			paths = append(paths, path)
 		}
-	}
 
-	for name, blockS := range schema.BlockTypes {
-		blockPath := append(p, cty.GetAttrStep{Name: name})
-		x := NonNullWriteOnlyPaths(val, &blockS.Block, blockPath)
-		paths = append(paths, x...)
 	}
 
-	return paths
+	return paths, nil
 }

internal/lang/ephemeral/validate_test.go

@@ -69,20 +69,30 @@ func TestNonNullWriteOnlyPaths(t *testing.T) {
 
 		"write-only attributes in blocks": {
 			val: cty.ObjectVal(map[string]cty.Value{
-				"foo": cty.ObjectVal(map[string]cty.Value{
-					"valid-write-only": cty.NullVal(cty.String),
-					"valid":            cty.StringVal("valid"),
-					"id":               cty.StringVal("i-abc123"),
-					"bar": cty.ObjectVal(map[string]cty.Value{
+				"foo": cty.ListVal([]cty.Value{
+					cty.ObjectVal(map[string]cty.Value{
 						"valid-write-only": cty.NullVal(cty.String),
 						"valid":            cty.StringVal("valid"),
 						"id":               cty.StringVal("i-abc123"),
+						"bar": cty.ListVal([]cty.Value{
+							cty.ObjectVal(map[string]cty.Value{
+								"valid-write-only": cty.NullVal(cty.String),
+								"valid":            cty.StringVal("valid"),
+								"id":               cty.StringVal("i-abc123"),
+							}),
+							cty.ObjectVal(map[string]cty.Value{
+								"valid-write-only": cty.NullVal(cty.String),
+								"valid":            cty.StringVal("valid"),
+								"id":               cty.StringVal("i-xyz123"),
+							}),
+						}),
 					}),
 				}),
 			}),
 			schema: &configschema.Block{
 				BlockTypes: map[string]*configschema.NestedBlock{
 					"foo": {
+						Nesting: configschema.NestingList,
 						Block: configschema.Block{
 							Attributes: map[string]*configschema.Attribute{
 								"valid-write-only": {
@@ -102,6 +112,7 @@ func TestNonNullWriteOnlyPaths(t *testing.T) {
 							},
 							BlockTypes: map[string]*configschema.NestedBlock{
 								"bar": {
+									Nesting: configschema.NestingList,
 									Block: configschema.Block{
 										Attributes: map[string]*configschema.Attribute{
 											"valid-write-only": {
@@ -126,11 +137,18 @@ func TestNonNullWriteOnlyPaths(t *testing.T) {
 					},
 				},
 			},
-			expectedPaths: []cty.Path{cty.GetAttrPath("foo").GetAttr("id"), cty.GetAttrPath("foo").GetAttr("bar").GetAttr("id")},
+			expectedPaths: []cty.Path{
+				cty.GetAttrPath("foo").Index(cty.NumberIntVal(0)).GetAttr("id"),
+				cty.GetAttrPath("foo").Index(cty.NumberIntVal(0)).GetAttr("bar").Index(cty.NumberIntVal(0)).GetAttr("id"),
+				cty.GetAttrPath("foo").Index(cty.NumberIntVal(0)).GetAttr("bar").Index(cty.NumberIntVal(1)).GetAttr("id"),
+			},
 		},
 	} {
 		t.Run(name, func(t *testing.T) {
-			paths := NonNullWriteOnlyPaths(tc.val, tc.schema, nil)
+			paths, err := nonNullWriteOnlyPaths(tc.val, tc.schema, nil)
+			if err != nil {
+				t.Fatal(err)
+			}
 
 			if len(paths) != len(tc.expectedPaths) {
 				t.Fatalf("expected %d write-only paths, got %d", len(tc.expectedPaths), len(paths))