New Checks: semantics validation on attribute reference #163

magodo · 2020-04-12T07:17:56Z

This is similar to S035-S037, except rather than syntax checks, it applies a semantics validation. The terraform acc test could do some semantics validation on attribute reference, while it failed to test for other cases like referring to attribute to multi nested blocks.

For example, the following is the explanation of the S038 implemented in this PR:

S038

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

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

Flagged Code

// Attribute reference in multi nested block 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

// 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

&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.

//lintignore:S038
&schema.Resource{
    // ...
}

…OneOf)

bflad

Hi @magodo 👋 Thanks for submitting this. Please see the below initial feedback and let me know if you have any questions.

bflad · 2020-04-14T05:34:12Z

passes/S038/README.md

@@ -0,0 +1,86 @@
+# S038
+
+The S038 analyzer reports cases of Schemas which include `ConflictsWith` and have invalid schema attribute references.


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.

Sorry for the mistake 😅

bflad · 2020-04-14T05:41:45Z

helper/terraformtype/helper/schema/attributes.go

+			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


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

}

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.

bflad · 2020-04-14T05:45:14Z

helper/terraformtype/helper/schema/attributes.go

+			// 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


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

}

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).

bflad · 2020-04-14T05:48:02Z

passes/S038/README.md

+
+The S038 analyzer reports cases of Schemas which include `ConflictsWith` and have invalid schema attribute references.
+
+NOTE: This pass only works with Terraform resources that are fully defined in a single function.


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.

You're right

bflad · 2020-04-14T05:49:01Z

passes/S038/README.md

+## Flagged Code
+
+```go
+// Attribute reference in multi nested block is not supported


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

bflad · 2020-04-14T05:53:17Z

passes/S038/testdata/src/a/main.go

+			},
+		},
+	}
+	// Failing


Let's ensure the following additional failure cases are tested:

TypeSet

Root attribute referencing non-existing root attribute

Root attribute referencing existing configuration block attribute with non-existing nested attribute

Configuration block nested attribute referencing non-existing root attribute

Configuration block nested attribute referencing existing configuration block attribute with non-existing nested attribute

Good point, and I will also add the corresponding pass test cases for those scenarios.

bflad · 2020-04-14T05:57:08Z

passes/S038/testdata/src/a/main.go

+		},
+	}
+
+	// Passing


Let's ensure the following additional passing cases are tested:

Root attribute referencing existing root attribute (e.g. https://github.com/terraform-providers/terraform-provider-aws/blob/2cb4e3f7f017feb338e0ba2ffdf409d6b0c91793/aws/resource_aws_cognito_user_pool.go#L90)

Root attribute referencing existing configuration block attribute with existing nested attribute (e.g. https://github.com/terraform-providers/terraform-provider-aws/blob/2cb4e3f7f017feb338e0ba2ffdf409d6b0c91793/aws/resource_aws_cognito_user_pool.go#L176)

Configuration block nested attribute referencing existing root attribute (e.g. https://github.com/terraform-providers/terraform-provider-aws/blob/2cb4e3f7f017feb338e0ba2ffdf409d6b0c91793/aws/resource_aws_cognito_user_pool.go#L517)

Configuration block nested attribute referencing existing configuration block attribute with existing nested attribute (e.g. https://github.com/terraform-providers/terraform-provider-aws/blob/2cb4e3f7f017feb338e0ba2ffdf409d6b0c91793/aws/resource_aws_cognito_user_pool.go#L307)

Deeper level configuration block attribute references (e.g. https://github.com/terraform-providers/terraform-provider-aws/blob/2cb4e3f7f017feb338e0ba2ffdf409d6b0c91793/aws/resource_aws_appmesh_virtual_node.go#L227)

1. Add more test cases for different scenarios, both pass and failure cases. 2. Add type assertion to avoid crash on unepxected schema declarations. 3. Modify the signature of `ValidateAttributeReference()` to only return the validation resutl.

magodo · 2020-04-15T03:01:18Z

@bflad Thank you for the quick response👍
I have made the changes per your comment, please take a look!

bflad · 2020-08-05T13:15:06Z

Hi again @magodo 👋 After much thought about this and the recent release of Terraform Plugin SDK v2, which includes unit testing for AtLeastOneOf/ConflictsWith/ExactlyOneOf/RequiredWhen attribute reference semantics, I'm going to opt to close this in preference of the SDK itself providing validation using the fully built schema. With increasing inter-connection between AST nodes and the various forms they can take, attempting to reconstruct the intended source code representation from purely the Go AST will always be a challenge, and we would prefer to avoid that complexity when unit testing the assembled schema will cover all cases and potentially be more accurate. Thanks again for the contribution and sorry that we are going the other route here.

magodo · 2020-08-06T01:14:23Z

@bflad No worry 🙃 Also thanks for the unit testing news in the new SDK!

magodo mentioned this pull request Apr 12, 2020

Link Resource and Schema to fill each other && test basic Schema #129

Merged

New Check: S038 for invalid attribute reference semantically (AtLeast…

2a7b858

…OneOf)

magodo force-pushed the attribute_reference_semantics_validation branch from bfc47a3 to 2a7b858 Compare April 14, 2020 03:19

magodo marked this pull request as ready for review April 14, 2020 03:20

bflad requested changes Apr 14, 2020

View reviewed changes

bflad added ast AST Handling check/schema Schema Check new-check New Lint Check labels Apr 14, 2020

bflad mentioned this pull request Apr 24, 2020

Spot Fleet Launch Template Support hashicorp/terraform-provider-aws#12732

Merged

bflad closed this Aug 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Checks: semantics validation on attribute reference #163

New Checks: semantics validation on attribute reference #163

magodo commented Apr 12, 2020

bflad left a comment

bflad Apr 14, 2020

magodo Apr 15, 2020

bflad Apr 14, 2020

magodo Apr 15, 2020

bflad Apr 14, 2020

magodo Apr 15, 2020

bflad Apr 14, 2020

magodo Apr 15, 2020

bflad Apr 14, 2020

bflad Apr 14, 2020

magodo Apr 15, 2020

bflad Apr 14, 2020 •

edited

Loading

magodo Apr 15, 2020

magodo commented Apr 15, 2020

bflad commented Aug 5, 2020

magodo commented Aug 6, 2020

		@@ -0,0 +1,86 @@
		# S038

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

	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.

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

New Checks: semantics validation on attribute reference #163

New Checks: semantics validation on attribute reference #163

Conversation

magodo commented Apr 12, 2020

S038

Flagged Code

Passing Code

Ignoring Reports

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bflad Apr 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magodo commented Apr 15, 2020

bflad commented Aug 5, 2020

magodo commented Aug 6, 2020

bflad Apr 14, 2020 •

edited

Loading