From 91feab6b5d08c434a71d53e817eb11bf2d62ea62 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 12 Feb 2025 18:59:41 -0500 Subject: [PATCH] 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 | 41 +++++++++++++++--------- internal/lang/ephemeral/validate_test.go | 32 ++++++++++++++---- 2 files changed, 51 insertions(+), 22 deletions(-) diff --git a/internal/lang/ephemeral/validate.go b/internal/lang/ephemeral/validate.go index 5aa4a922f5..680e2a9ee2 100644 --- a/internal/lang/ephemeral/validate.go +++ b/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 } diff --git a/internal/lang/ephemeral/validate_test.go b/internal/lang/ephemeral/validate_test.go index 011062cf5c..e82f172f5a 100644 --- a/internal/lang/ephemeral/validate_test.go +++ b/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))