Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

helper/validation: Prevented panics with ToDiagFunc() function when used inside Schema type Elem field #915

Merged
merged 2 commits into from
Mar 28, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions .changelog/915.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:bug
helper/validation: Prevented panics with `ToDiagFunc()` function when used inside `Schema` type `Elem` field, such as validating `TypeList` elements
```
17 changes: 15 additions & 2 deletions helper/validation/meta.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,21 @@ func ToDiagFunc(validator schema.SchemaValidateFunc) schema.SchemaValidateDiagFu
return func(i interface{}, p cty.Path) diag.Diagnostics {
var diags diag.Diagnostics

attr := p[len(p)-1].(cty.GetAttrStep)
ws, es := validator(i, attr.Name)
// A practitioner-friendly key for any SchemaValidateFunc output.
// Generally this should be the last attribute name on the path.
// If not found for some unexpected reason, an empty string is fine
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you provide a bit of details about why it wouldn't not be, in some cases, that the last element in the path is not the one that implements GetAttrStep?
And if it's not, then what are those? IndexStep?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI I dug a bit into go-cty to try to understand this a bit: I do get that those *Step structs are representation of operations that can be done on a Path, but I don't fully follow how they get assembled and why we would hit a scenario like the one described in the comment you added.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why it wouldn't not be, in some cases, that the last element in the path is not the one that implements GetAttrStep?

An IndexStep in cty represents a path inside a list, map, or set for one of the elements. For example, the path to the first element in a list is 0.

I don't fully follow how they get assembled

Paths are assembled as part of decoding a value. The core folks would be best to ask how this works in terms of how a configuration/schema gets decoded to certain types and paths.

why we would hit a scenario like the one described in the comment you added

If a new path type is added to cty. Just coding for future safety because the runtime code for providers should never introduce an unnecessary panic.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried out the forcetypeassert linter in golangci-lint and it catches some other potentially problematic code, but unfortunately doesn't catch this particular code pattern of a type assertion on a slice element. Not sure its worth the effort of enabling it in this project, but maybe some of the others.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Big 👍 from me on adding this linter everywhere.

// as the diagnostic will have the full attribute path anyways.
var key string

// Reverse search for last cty.GetAttrStep
for i := len(p) - 1; i >= 0; i-- {
if pathStep, ok := p[i].(cty.GetAttrStep); ok {
key = pathStep.Name
break
}
}

ws, es := validator(i, key)

for _, w := range ws {
diags = append(diags, diag.Diagnostic{
Expand Down
79 changes: 70 additions & 9 deletions helper/validation/meta_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@ package validation
import (
"regexp"
"testing"

"github.com/hashicorp/go-cty/cty"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
)

func TestValidationNoZeroValues(t *testing.T) {
Expand Down Expand Up @@ -101,36 +104,94 @@ func TestValidationAny(t *testing.T) {
}

func TestToDiagFunc(t *testing.T) {
runDiagTestCases(t, []diagTestCase{
{
t.Parallel()

testCases := map[string]struct {
path cty.Path
val interface{}
f schema.SchemaValidateDiagFunc
expectedErr *regexp.Regexp
}{
"success-GetAttrStep-int": {
path: cty.Path{
cty.GetAttrStep{Name: "test_property"},
},
val: 43,
f: ToDiagFunc(Any(
IntAtLeast(42),
IntAtMost(5),
)),
},
{
"success-GetAttrStep-string": {
path: cty.Path{
cty.GetAttrStep{Name: "test_property"},
},
val: "foo",
f: ToDiagFunc(All(
StringLenBetween(1, 10),
StringIsNotWhiteSpace,
)),
},
{
"success-IndexStep-string": {
path: cty.Path{
cty.GetAttrStep{Name: "test_property"},
cty.IndexStep{Key: cty.NumberIntVal(0)},
},
val: "foo",
f: ToDiagFunc(StringLenBetween(1, 10)),
},
"error-GetAttrStep-int-first": {
path: cty.Path{
cty.GetAttrStep{Name: "test_property"},
},
val: 7,
f: ToDiagFunc(Any(
IntAtLeast(42),
IntAtMost(5),
)),
expectedErr: regexp.MustCompile(`expected [\w]+ to be at least \(42\), got 7`),
expectedErr: regexp.MustCompile(`expected test_property to be at least \(42\), got 7`),
},
{
"error-GetAttrStep-int-second": {
path: cty.Path{
cty.GetAttrStep{Name: "test_property"},
},
val: 7,
f: ToDiagFunc(Any(
IntAtLeast(42),
IntAtMost(5),
)),
expectedErr: regexp.MustCompile(`expected [\w]+ to be at most \(5\), got 7`),
},
})
expectedErr: regexp.MustCompile(`expected test_property to be at most \(5\), got 7`),
},
"error-IndexStep-int": {
path: cty.Path{
cty.GetAttrStep{Name: "test_property"},
cty.IndexStep{Key: cty.NumberIntVal(0)},
},
val: 7,
f: ToDiagFunc(IntAtLeast(42)),
expectedErr: regexp.MustCompile(`expected test_property to be at least \(42\), got 7`),
},
}

for name, testCase := range testCases {
name, testCase := name, testCase

t.Run(name, func(t *testing.T) {
t.Parallel()

diags := testCase.f(testCase.val, testCase.path)

if !diags.HasError() && testCase.expectedErr == nil {
return
}

if diags.HasError() && testCase.expectedErr == nil {
t.Fatalf("expected to produce no errors, got %v", diags)
}

if !matchAnyDiagSummary(diags, testCase.expectedErr) {
t.Fatalf("expected to produce error matching %q, got %v", testCase.expectedErr, diags)
}
})
}
}
30 changes: 0 additions & 30 deletions helper/validation/testing.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import (

testing "github.com/mitchellh/go-testing-interface"

"github.com/hashicorp/go-cty/cty"
"github.com/hashicorp/terraform-plugin-sdk/v2/diag"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
)
Expand All @@ -16,12 +15,6 @@ type testCase struct {
expectedErr *regexp.Regexp
}

type diagTestCase struct {
val interface{}
f schema.SchemaValidateDiagFunc
expectedErr *regexp.Regexp
}

func runTestCases(t testing.T, cases []testCase) {
t.Helper()

Expand Down Expand Up @@ -52,29 +45,6 @@ func matchAnyError(errs []error, r *regexp.Regexp) bool {
return false
}

func runDiagTestCases(t testing.T, cases []diagTestCase) {
t.Helper()

for i, tc := range cases {
p := cty.Path{
cty.GetAttrStep{Name: "test_property"},
}
diags := tc.f(tc.val, p)

if !diags.HasError() && tc.expectedErr == nil {
continue
}

if diags.HasError() && tc.expectedErr == nil {
t.Fatalf("expected test case %d to produce no errors, got %v", i, diags)
}

if !matchAnyDiagSummary(diags, tc.expectedErr) {
t.Fatalf("expected test case %d to produce error matching \"%s\", got %v", i, tc.expectedErr, diags)
}
}
}

func matchAnyDiagSummary(ds diag.Diagnostics, r *regexp.Regexp) bool {
for _, d := range ds {
if r.MatchString(d.Summary) {
Expand Down