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

New Checks: semantics validation on attribute reference #163

Closed
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 24 additions & 4 deletions helper/analysisutils/schema_analyzers.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,16 @@ package analysisutils

import (
"fmt"
"github.com/bflad/tfproviderlint/passes/helper/schema/resourceinfo"

"github.com/bflad/tfproviderlint/passes/commentignore"
"github.com/bflad/tfproviderlint/passes/helper/schema/schemainfo"
"golang.org/x/tools/go/analysis"
)

// SchemaAttributeReferencesAnalyzer returns an Analyzer for fields that use schema attribute references
func SchemaAttributeReferencesAnalyzer(analyzerName string, fieldName string) *analysis.Analyzer {
doc := fmt.Sprintf(`check for Schema with invalid %[2]s references
// SchemaAttributeReferencesSyntaxAnalyzer returns an Analyzer for fields that use schema attribute references
func SchemaAttributeReferencesSyntaxAnalyzer(analyzerName string, fieldName string) *analysis.Analyzer {
doc := fmt.Sprintf(`check for Schema %[2]s references with invalid syntax

The %[1]s analyzer ensures schema attribute references in the Schema %[2]s
field use valid syntax. The Terraform Plugin SDK can unit test attribute
Expand All @@ -24,6 +25,25 @@ references to verify the references against the full schema.
commentignore.Analyzer,
schemainfo.Analyzer,
},
Run: SchemaAttributeReferencesRunner(analyzerName, fieldName),
Run: SchemaAttributeReferencesSyntaxRunner(analyzerName, fieldName),
}
}

// SchemaAttributeReferencesSemanticsAnalyzer returns an Analyzer for fields that use schema attribute references
func SchemaAttributeReferencesSemanticsAnalyzer(analyzerName string, fieldName string) *analysis.Analyzer {
doc := fmt.Sprintf(`check for Schema %[2]s references with invalid semantics

The %[1]s analyzer ensures schema attribute references in the Schema %[2]s
field refers to valid attribute.
`, analyzerName, fieldName)

return &analysis.Analyzer{
Name: analyzerName,
Doc: doc,
Requires: []*analysis.Analyzer{
commentignore.Analyzer,
resourceinfo.Analyzer,
},
Run: SchemaAttributeReferencesSemanticsRunner(analyzerName, fieldName),
}
}
74 changes: 71 additions & 3 deletions helper/analysisutils/schema_runners.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package analysisutils

import (
"github.com/bflad/tfproviderlint/passes/helper/schema/resourceinfo"
"go/ast"

"github.com/bflad/tfproviderlint/helper/astutils"
Expand All @@ -10,8 +11,8 @@ import (
"golang.org/x/tools/go/analysis"
)

// SchemaAttributeReferencesRunner returns an Analyzer runner for fields that use schema attribute references
func SchemaAttributeReferencesRunner(analyzerName string, fieldName string) func(*analysis.Pass) (interface{}, error) {
// SchemaAttributeReferencesSyntaxRunner returns an Analyzer runner for fields that use schema attribute references
func SchemaAttributeReferencesSyntaxRunner(analyzerName string, fieldName string) func(*analysis.Pass) (interface{}, error) {
return func(pass *analysis.Pass) (interface{}, error) {
ignorer := pass.ResultOf[commentignore.Analyzer].(*commentignore.Ignorer)
schemaInfos := pass.ResultOf[schemainfo.Analyzer].([]*schema.SchemaInfo)
Expand Down Expand Up @@ -39,7 +40,74 @@ func SchemaAttributeReferencesRunner(analyzerName string, fieldName string) func
}

if _, err := schema.ParseAttributeReference(*attributeReference); err != nil {
pass.Reportf(elt.Pos(), "%s: invalid %s attribute reference: %s", analyzerName, fieldName, err)
pass.Reportf(elt.Pos(), "%s: invalid %s attribute reference syntax: %s", analyzerName, fieldName, err)
}
}
}
}

return nil, nil
}
}

// SchemaAttributeReferencesSemanticsRunner returns an Analyzer runner for fields that use schema attribute references
func SchemaAttributeReferencesSemanticsRunner(analyzerName string, fieldName string) func(*analysis.Pass) (interface{}, error) {
return func(pass *analysis.Pass) (interface{}, error) {
ignorer := pass.ResultOf[commentignore.Analyzer].(*commentignore.Ignorer)
resourceInfos := pass.ResultOf[resourceinfo.Analyzer].([]*schema.ResourceInfo)

for _, resourceInfo := range resourceInfos {
// Handle "root" schema.Resource only since we will use the resourceInfo of that to validate attribute reference.
if !resourceInfo.IsDataSource() && !resourceInfo.IsResource() {
continue
}

// Collection schemaInfo under current resourceInfo
var schemaInfos []*schema.SchemaInfo
ast.Inspect(resourceInfo.AstCompositeLit, func(n ast.Node) bool {
compositeLit, ok := n.(*ast.CompositeLit)

if !ok {
return true
}

if schema.IsMapStringSchema(compositeLit, pass.TypesInfo) {
for _, mapSchema := range schema.GetSchemaMapSchemas(compositeLit) {
schemaInfos = append(schemaInfos, schema.NewSchemaInfo(mapSchema, pass.TypesInfo))
}
} else if schema.IsTypeSchema(pass.TypesInfo.TypeOf(compositeLit.Type)) {
schemaInfos = append(schemaInfos, schema.NewSchemaInfo(compositeLit, pass.TypesInfo))
}

return true
})

// Validate each schemaInfo under current resourceInfo
for _, schemaInfo := range schemaInfos {
if ignorer.ShouldIgnore(analyzerName, schemaInfo.AstCompositeLit) {
continue
}

if !schemaInfo.DeclaresField(fieldName) {
continue
}

switch value := schemaInfo.Fields[fieldName].Value.(type) {
case *ast.CompositeLit:
if !astutils.IsExprTypeArrayString(value.Type) {
continue
}

for _, elt := range value.Elts {
attributeReference := astutils.ExprStringValue(elt)

if attributeReference == nil {
continue
}

if _, err := schema.ValidateAttributeReference(resourceInfo.Resource, *attributeReference); err != nil {
pass.Reportf(elt.Pos(), "%s: invalid %s attribute reference semantics: %s", analyzerName, fieldName, err)
}
}
}
}
Expand Down
40 changes: 40 additions & 0 deletions helper/terraformtype/helper/schema/attributes.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package schema

import (
"fmt"
tfschema "github.com/hashicorp/terraform-plugin-sdk/helper/schema"
"math"
"regexp"
"strconv"
Expand Down Expand Up @@ -59,3 +60,42 @@ func ParseAttributeReference(reference string) ([]string, error) {

return attributeReferenceParts, nil
}

// ValidateAttributeReference validates schema attribute reference.
// Attribute references are used in Schema fields such as AtLeastOneOf, ConflictsWith, and ExactlyOneOf.
func ValidateAttributeReference(tfresource *tfschema.Resource, reference string) ([]string, error) {
var curSchemaOrResource interface{} = tfresource
attributeReferenceParts := strings.Split(reference, ".")
for idx, attributeReferencePart := range attributeReferenceParts {
attributeReference := strings.Join(attributeReferenceParts[:idx+1], ".")
if math.Mod(float64(idx), 2) == 1 {
// For odd part, ensure it is a 0 and the containing schema has `MaxItems` set to `1`. This is because
// reference among multiple instances of the same nested block is not supported in current plugin SDK.
attributeReferencePartInt, err := strconv.Atoi(attributeReferencePart)

configurationBlockReferenceErr := fmt.Errorf("%q configuration block attribute references must be separated by .0", attributeReference)
if err != nil {
return nil, configurationBlockReferenceErr
}
if attributeReferencePartInt != 0 {
return nil, configurationBlockReferenceErr
}

curSchema := curSchemaOrResource.(*tfschema.Schema)
if curSchema.MaxItems != 1 || curSchema.Type != tfschema.TypeList {
return nil, fmt.Errorf("%q configuration block attribute references are only valid for TypeList and MaxItems: 1 attributes", attributeReference)
}
curSchemaOrResource = curSchema.Elem
Copy link
Owner

Choose a reason for hiding this comment

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

Please introduce type assertion safety here with a switch statement utilizing .(type):

Suggested change
curSchema := curSchemaOrResource.(*tfschema.Schema)
if curSchema.MaxItems != 1 || curSchema.Type != tfschema.TypeList {
return nil, fmt.Errorf("%q configuration block attribute references are only valid for TypeList and MaxItems: 1 attributes", attributeReference)
}
curSchemaOrResource = curSchema.Elem
switch current := curSchemaOrResource.(type) {
case *tfschema.Schema:
if current.MaxItems != 1 || current.Type != tfschema.TypeList {
return nil, fmt.Errorf("%q configuration block attribute references are only valid for TypeList and MaxItems: 1 attributes", attributeReference)
}
curSchemaOrResource = current.Elem
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Also for the default case (which means we are not capable to deal with for now), I think we shall just return nil to avoid false positive.

} else {
// For even part, ensure it references to defined attribute
schema := curSchemaOrResource.(*tfschema.Resource).Schema[attributeReferencePart]
if schema == nil {
return nil, fmt.Errorf("%q references to unknown attribute", attributeReference)
}
curSchemaOrResource = schema
Copy link
Owner

Choose a reason for hiding this comment

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

Same here. We will want to be careful for unexpected types and missing key references:

Suggested change
// For even part, ensure it references to defined attribute
schema := curSchemaOrResource.(*tfschema.Resource).Schema[attributeReferencePart]
if schema == nil {
return nil, fmt.Errorf("%q references to unknown attribute", attributeReference)
}
curSchemaOrResource = schema
// For even part, ensure it references to defined attribute
switch current := curSchemaOrResource.(type) {
case *tfschema.Resource:
if current.Schema == nil {
return nil, fmt.Errorf("%q references attribute without schema")
}
schema, ok := current.Schema[attributeReferencePart]
if !ok || schema == nil {
return nil, fmt.Errorf("%q references to unknown attribute", attributeReference)
}
curSchemaOrResource = schema
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A little improvement of this might be just using schema := current.Schema[attributeReferencePart] instead, and in absent key case, it will return the zero value of the Value, in which case it is nil (since we do not differ between absent key and nil value).

}
}

return attributeReferenceParts, nil
}

2 changes: 1 addition & 1 deletion passes/S035/S035.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,4 @@ import (
"github.com/bflad/tfproviderlint/helper/terraformtype/helper/schema"
)

var Analyzer = analysisutils.SchemaAttributeReferencesAnalyzer("S035", schema.SchemaFieldAtLeastOneOf)
var Analyzer = analysisutils.SchemaAttributeReferencesSyntaxAnalyzer("S035", schema.SchemaFieldAtLeastOneOf)
2 changes: 1 addition & 1 deletion passes/S036/S036.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,4 @@ import (
"github.com/bflad/tfproviderlint/helper/terraformtype/helper/schema"
)

var Analyzer = analysisutils.SchemaAttributeReferencesAnalyzer("S036", schema.SchemaFieldConflictsWith)
var Analyzer = analysisutils.SchemaAttributeReferencesSyntaxAnalyzer("S036", schema.SchemaFieldConflictsWith)
2 changes: 1 addition & 1 deletion passes/S037/S037.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,4 @@ import (
"github.com/bflad/tfproviderlint/helper/terraformtype/helper/schema"
)

var Analyzer = analysisutils.SchemaAttributeReferencesAnalyzer("S037", schema.SchemaFieldExactlyOneOf)
var Analyzer = analysisutils.SchemaAttributeReferencesSyntaxAnalyzer("S037", schema.SchemaFieldExactlyOneOf)
86 changes: 86 additions & 0 deletions passes/S038/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
# S038

The S038 analyzer reports cases of Schemas which include `ConflictsWith` and have invalid schema attribute references.
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
The S038 analyzer reports cases of Schemas which include `ConflictsWith` and have invalid schema attribute references.
The S038 analyzer reports cases of Schemas which include `AtLeastOneOf` and have invalid schema attribute references.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the mistake 😅


NOTE: This pass only works with Terraform resources that are fully defined in a single function.
Copy link
Owner

Choose a reason for hiding this comment

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

Terraform resources that meet the criteria for this check can come from a variety of source code, e.g. variables, inline declarations in schema.Provider.ResourcesMap, etc.

Suggested change
NOTE: This pass only works with Terraform resources that are fully defined in a single function.
NOTE: This pass only works with `schema.Resource` that are wholly declared.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right


## Flagged Code

```go
// Attribute reference in multi nested block is not supported
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
// Attribute reference in multi nested block is not supported
// Attribute reference in configuration block with potentially more than one element is not supported

&schema.Resource{
Schema: map[string]*schema.Schema{
"x": {
Type: schema.TypeList,
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"foo": {
AtLeastOneOf: []string{"x.0.bar"},
},
"bar": {
AtLeastOneOf: []string{"x.0.foo"},
},
},
},
},
},
}
```

or

```go
// Non-existed attribute reference
&schema.Resource{
Schema: map[string]*schema.Schema{
"x": {
Type: schema.TypeList,
MaxItems: 1,
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"foo": {
AtLeastOneOf: []string{"x.1.bar"},
},
"bar": {
AtLeastOneOf: []string{"x.1.foo"},
},
},
},
},
},
}
```

## Passing Code

```go
&schema.Resource{
Schema: map[string]*schema.Schema{
"x": {
Type: schema.TypeList,
MaxItems: 1,
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"foo": {
AtLeastOneOf: []string{"x.0.bar"},
},
"bar": {
AtLeastOneOf: []string{"x.0.foo"},
},
},
},
},
},
}
```

## Ignoring Reports

Singular reports can be ignored by adding the a `//lintignore:S038` Go code comment at the end of the offending line or on the line immediately proceding, e.g.

```go
//lintignore:S038
&schema.Resource{
// ...
}
```
8 changes: 8 additions & 0 deletions passes/S038/S038.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
package S038

import (
"github.com/bflad/tfproviderlint/helper/analysisutils"
"github.com/bflad/tfproviderlint/helper/terraformtype/helper/schema"
)

var Analyzer = analysisutils.SchemaAttributeReferencesSemanticsAnalyzer("S038", schema.SchemaFieldAtLeastOneOf)
12 changes: 12 additions & 0 deletions passes/S038/S038_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package S038

import (
"testing"

"golang.org/x/tools/go/analysis/analysistest"
)

func TestS038(t *testing.T) {
testdata := analysistest.TestData()
analysistest.Run(t, testdata, Analyzer, "a")
}
26 changes: 26 additions & 0 deletions passes/S038/testdata/src/a/alias.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
package a

import (
s "github.com/hashicorp/terraform-plugin-sdk/helper/schema"
)

func falias() {
_ = &s.Resource{
Read: func(*s.ResourceData, interface{}) error { return nil },
Schema: map[string]*s.Schema{
"x": {
Type: s.TypeList,
Elem: &s.Resource{
Schema: map[string]*s.Schema{
"foo": {
AtLeastOneOf: []string{"x.0.bar"}, // want `S038: invalid AtLeastOneOf attribute reference semantics: "x.0" configuration block attribute references are only valid for TypeList and MaxItems: 1 attributes`
},
"bar": {
AtLeastOneOf: []string{"x.0.foo"}, // want `S038: invalid AtLeastOneOf attribute reference semantics: "x.0" configuration block attribute references are only valid for TypeList and MaxItems: 1 attributes`
},
},
},
},
},
}
}
Loading