From eaaa3b2e44843b95154c540eabcb96fb0d69b409 Mon Sep 17 00:00:00 2001 From: Matt White <16320656+matt-FFFFFF@users.noreply.github.com> Date: Mon, 8 Apr 2024 15:47:23 +0100 Subject: [PATCH 1/5] feat: add location interface --- interfaces/interface_variable.go | 41 ++++++++++++-- interfaces/interfaces_simple_test.go | 83 +++++++++++++++++++++++++++- interfaces/interfaces_test.go | 4 +- interfaces/location.go | 20 +++++++ interfaces/location_test.go | 45 +++++++++++++++ 5 files changed, 185 insertions(+), 8 deletions(-) create mode 100644 interfaces/location.go create mode 100644 interfaces/location_test.go diff --git a/interfaces/interface_variable.go b/interfaces/interface_variable.go index cc8dd82..1f11cc0 100644 --- a/interfaces/interface_variable.go +++ b/interfaces/interface_variable.go @@ -105,8 +105,8 @@ func (vcr *InterfaceVarCheckRule) Check(r tflint.Runner) error { continue } - typeAttr, c := CheckWithReturnValue(NewChecker(), getAttr(vcr, r, b, "type")) - defaultAttr, c := CheckWithReturnValue(c, getAttr(vcr, r, b, "default")) + typeAttr, c := CheckWithReturnValue(NewChecker(), getAttr(vcr, r, b, "type", true)) + defaultAttr, c := CheckWithReturnValue(c, getAttr(vcr, r, b, "default", vcr.Default.IsKnown())) if c = c.Check(checkVarType(vcr, r, typeAttr)). Check(checkDefaultValue(vcr, r, b, defaultAttr)). Check(checkNullableValue(vcr, r, b)); c.err != nil { @@ -118,15 +118,22 @@ func (vcr *InterfaceVarCheckRule) Check(r tflint.Runner) error { return nil } -// getTypeAttr returns a function that will return the type attribute from a given hcl block. +// getAttr returns a function that will return the attribute from a given hcl block. // It is designed to be used with the CheckWithReturnValue function. -func getAttr(rule tflint.Rule, r tflint.Runner, b *hclext.Block, attrName string) func() (*hclext.Attribute, bool, error) { +func getAttr(rule tflint.Rule, r tflint.Runner, b *hclext.Block, attrName string, shouldExist bool) func() (*hclext.Attribute, bool, error) { return func() (*hclext.Attribute, bool, error) { attr, exists := b.Body.Attributes[attrName] - if !exists { + if shouldExist != exists { + var msg string + switch shouldExist { + case true: + msg = "not declared" + case false: + msg = "should not be declared" + } return attr, false, r.EmitIssue( rule, - fmt.Sprintf("`%s` %s not declared", b.Labels[0], attrName), + fmt.Sprintf("`%s` %s %s", b.Labels[0], attrName, msg), b.DefRange, ) } @@ -187,6 +194,16 @@ func checkVarType(vcr *InterfaceVarCheckRule, r tflint.Runner, typeAttr *hclext. func checkDefaultValue(vcr *InterfaceVarCheckRule, r tflint.Runner, b *hclext.Block, defaultAttr *hclext.Attribute) func() (bool, error) { return func() (bool, error) { // Check if the default value is correct. + if !vcr.Default.IsKnown() { + if defaultAttr != nil { + return true, r.EmitIssue( + vcr, + fmt.Sprintf("default value should not be set, see: %s", vcr.Link()), + defaultAttr.Range, + ) + } + return true, nil + } defaultVal, _ := defaultAttr.Expr.Value(nil) if !check.EqualCtyValue(defaultVal, vcr.Default) { return true, r.EmitIssue( @@ -242,3 +259,15 @@ func CheckWithReturnValue[TR any](c Checker, check func() (TR, bool, error)) (re err: err, } } + +func CheckBool(c Checker, check func() (bool, error)) (rc Checker) { + if c.err != nil || !c.continueCheck { + rc = c + return + } + continueCheck, err := check() + return Checker{ + continueCheck: continueCheck, + err: err, + } +} diff --git a/interfaces/interfaces_simple_test.go b/interfaces/interfaces_simple_test.go index 29ff582..f039956 100644 --- a/interfaces/interfaces_simple_test.go +++ b/interfaces/interfaces_simple_test.go @@ -2,9 +2,10 @@ package interfaces_test import ( "fmt" - "github.com/stretchr/testify/assert" "testing" + "github.com/stretchr/testify/assert" + "github.com/Azure/tflint-ruleset-avm/interfaces" "github.com/hashicorp/hcl/v2" "github.com/matt-FFFFFF/tfvarcheck/varcheck" @@ -30,6 +31,16 @@ var SimpleVar = interfaces.AvmInterface{ RuleSeverity: tflint.ERROR, } +// SimpleVar represents a simple variable type with no default value used for testing. +var SimpleVarNoDefault = interfaces.AvmInterface{ + VarCheck: varcheck.NewVarCheck(simpleType, cty.UnknownVal(cty.DynamicPseudoType), false), + RuleName: "simple", + VarTypeString: simpleVarTypeString, + RuleEnabled: true, + RuleLink: "https://simple", + RuleSeverity: tflint.ERROR, +} + func TestIncorrectInterfaceTypeStringShouldPanic(t *testing.T) { defer func() { err := recover() @@ -40,6 +51,76 @@ func TestIncorrectInterfaceTypeStringShouldPanic(t *testing.T) { t.Fatal("incorrect type should panic") } +func TestSimpleInterfaceNoDefault(t *testing.T) { + cases := []struct { + Name string + Content string + JSON bool + Expected helper.Issues + }{ + { + Name: "correct", + Content: toTerraformVarType(SimpleVarNoDefault), + Expected: helper.Issues{}, + }, + + { + Name: "incorrect - has default value", + Content: ` +variable "simple" { + type = object({ + kind = string + name = optional(string, null) + }) + default = {} +}`, + Expected: helper.Issues{ + { + Rule: interfaces.NewVarCheckRuleFromAvmInterface(SimpleVarNoDefault), + Message: "`simple` default should not be declared", + }, + }, + }, + { + Name: "incorrect - is nullable", + Content: ` +variable "simple" { + type = object({ + kind = string + name = optional(string, null) + }) +}`, + Expected: helper.Issues{ + { + Rule: interfaces.NewVarCheckRuleFromAvmInterface(SimpleVarNoDefault), + Message: "nullable should be set to false", + }, + }, + }, + } + + rule := interfaces.NewVarCheckRuleFromAvmInterface(SimpleVarNoDefault) + + for _, tc := range cases { + tc := tc + t.Run(tc.Name, func(t *testing.T) { + t.Parallel() + filename := "variables.tf" + if tc.JSON { + filename += ".json" + } + + runner := helper.TestRunner(t, map[string]string{filename: tc.Content}) + + if err := rule.Check(runner); err != nil { + t.Fatalf("Unexpected error occurred: %s", err) + } + + helper.AssertIssuesWithoutRange(t, tc.Expected, runner.Issues) + }) + } +} + func TestSimpleInterface(t *testing.T) { cases := []struct { Name string diff --git a/interfaces/interfaces_test.go b/interfaces/interfaces_test.go index bf89bcd..2aa9476 100644 --- a/interfaces/interfaces_test.go +++ b/interfaces/interfaces_test.go @@ -20,7 +20,9 @@ func toTerraformVarType(i interfaces.AvmInterface) string { SpacesBefore: 1, }, }) - varBody.SetAttributeValue("default", i.Default) + if i.Default.IsKnown() { + varBody.SetAttributeValue("default", i.Default) + } if !i.Nullable { varBody.SetAttributeValue("nullable", cty.False) } diff --git a/interfaces/location.go b/interfaces/location.go new file mode 100644 index 0000000..5a2ad3b --- /dev/null +++ b/interfaces/location.go @@ -0,0 +1,20 @@ +package interfaces + +import ( + "github.com/matt-FFFFFF/tfvarcheck/varcheck" + "github.com/terraform-linters/tflint-plugin-sdk/tflint" + "github.com/zclconf/go-cty/cty" +) + +var LocationTypeString = `string` + +var locationType = StringToTypeConstraintWithDefaults(LocationTypeString) + +var Location = AvmInterface{ + VarCheck: varcheck.NewVarCheck(locationType, cty.UnknownVal(cty.String), false), + RuleName: "location", + VarTypeString: LocationTypeString, + RuleEnabled: true, + RuleLink: "https://azure.github.io/Azure-Verified-Modules/specs/shared/interfaces/#resource-locks", + RuleSeverity: tflint.ERROR, +} diff --git a/interfaces/location_test.go b/interfaces/location_test.go new file mode 100644 index 0000000..993aa03 --- /dev/null +++ b/interfaces/location_test.go @@ -0,0 +1,45 @@ +package interfaces_test + +import ( + "testing" + + "github.com/Azure/tflint-ruleset-avm/interfaces" + "github.com/terraform-linters/tflint-plugin-sdk/helper" +) + +// TestLockTerraformVar tests Lock interface. +func TestTerraformLocationInterface(t *testing.T) { + cases := []struct { + Name string + Content string + JSON bool + Expected helper.Issues + }{ + { + Name: "correct", + Content: toTerraformVarType(interfaces.Location), + Expected: helper.Issues{}, + }, + } + + rule := interfaces.NewVarCheckRuleFromAvmInterface(interfaces.Location) + + for _, tc := range cases { + tc := tc + t.Run(tc.Name, func(t *testing.T) { + t.Parallel() + filename := "variables.tf" + if tc.JSON { + filename += ".json" + } + + runner := helper.TestRunner(t, map[string]string{filename: tc.Content}) + + if err := rule.Check(runner); err != nil { + t.Fatalf("Unexpected error occurred: %s", err) + } + + helper.AssertIssuesWithoutRange(t, tc.Expected, runner.Issues) + }) + } +} From a3cf8101da29aa0fa7120294f99d162cbb165f46 Mon Sep 17 00:00:00 2001 From: Matt White <16320656+matt-FFFFFF@users.noreply.github.com> Date: Mon, 8 Apr 2024 15:49:55 +0100 Subject: [PATCH 2/5] fix: forgot to add interface to Rules slice --- interfaces/interfaces.go | 1 + 1 file changed, 1 insertion(+) diff --git a/interfaces/interfaces.go b/interfaces/interfaces.go index 6d29d96..f1b0013 100644 --- a/interfaces/interfaces.go +++ b/interfaces/interfaces.go @@ -8,6 +8,7 @@ import ( var Rules = []tflint.Rule{ NewVarCheckRuleFromAvmInterface(CustomerManagedKey), NewVarCheckRuleFromAvmInterface(DiagnosticSettings), + NewVarCheckRuleFromAvmInterface(Location), NewVarCheckRuleFromAvmInterface(Lock), NewVarCheckRuleFromAvmInterface(ManagedIdentities), NewVarCheckRuleFromAvmInterface(RoleAssignments), From 3c24552845133acb50eaf93794a1288d46d063b1 Mon Sep 17 00:00:00 2001 From: Matt White <16320656+matt-FFFFFF@users.noreply.github.com> Date: Mon, 8 Apr 2024 16:11:30 +0100 Subject: [PATCH 3/5] fix: remove unused func --- interfaces/interface_variable.go | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/interfaces/interface_variable.go b/interfaces/interface_variable.go index 1f11cc0..87562ec 100644 --- a/interfaces/interface_variable.go +++ b/interfaces/interface_variable.go @@ -259,15 +259,3 @@ func CheckWithReturnValue[TR any](c Checker, check func() (TR, bool, error)) (re err: err, } } - -func CheckBool(c Checker, check func() (bool, error)) (rc Checker) { - if c.err != nil || !c.continueCheck { - rc = c - return - } - continueCheck, err := check() - return Checker{ - continueCheck: continueCheck, - err: err, - } -} From 70f2712a1f34342b18ecc17199c1289060a1cb23 Mon Sep 17 00:00:00 2001 From: Matt White <16320656+matt-FFFFFF@users.noreply.github.com> Date: Tue, 9 Apr 2024 08:33:39 +0100 Subject: [PATCH 4/5] chore: update link --- interfaces/location.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/interfaces/location.go b/interfaces/location.go index 5a2ad3b..f728e16 100644 --- a/interfaces/location.go +++ b/interfaces/location.go @@ -15,6 +15,6 @@ var Location = AvmInterface{ RuleName: "location", VarTypeString: LocationTypeString, RuleEnabled: true, - RuleLink: "https://azure.github.io/Azure-Verified-Modules/specs/shared/interfaces/#resource-locks", + RuleLink: "https://azure.github.io/Azure-Verified-Modules/specs/shared/#id-rmnfr2---category-inputs---parametervariable-naming", RuleSeverity: tflint.ERROR, } From b31c83abedf81d1348d94b8ead8a0080138aade5 Mon Sep 17 00:00:00 2001 From: lonegunmanb Date: Tue, 9 Apr 2024 16:38:06 +0800 Subject: [PATCH 5/5] refactor getAttr function, extract attribute non-existance check logic into new function attributeNotExist (#19) --- interfaces/interface_variable.go | 32 +++++++++++++++++----------- interfaces/interfaces_simple_test.go | 3 +-- 2 files changed, 21 insertions(+), 14 deletions(-) diff --git a/interfaces/interface_variable.go b/interfaces/interface_variable.go index 87562ec..19ade14 100644 --- a/interfaces/interface_variable.go +++ b/interfaces/interface_variable.go @@ -105,8 +105,13 @@ func (vcr *InterfaceVarCheckRule) Check(r tflint.Runner) error { continue } - typeAttr, c := CheckWithReturnValue(NewChecker(), getAttr(vcr, r, b, "type", true)) - defaultAttr, c := CheckWithReturnValue(c, getAttr(vcr, r, b, "default", vcr.Default.IsKnown())) + typeAttr, c := CheckWithReturnValue(NewChecker(), getAttr(vcr, r, b, "type")) + var defaultAttr *hclext.Attribute + if vcr.Default.IsKnown() { + defaultAttr, c = CheckWithReturnValue(c, getAttr(vcr, r, b, "default")) + } else { + c = c.Check(attributeNotExist(vcr, r, b, "default")) + } if c = c.Check(checkVarType(vcr, r, typeAttr)). Check(checkDefaultValue(vcr, r, b, defaultAttr)). Check(checkNullableValue(vcr, r, b)); c.err != nil { @@ -118,22 +123,25 @@ func (vcr *InterfaceVarCheckRule) Check(r tflint.Runner) error { return nil } +func attributeNotExist(vcr *InterfaceVarCheckRule, r tflint.Runner, b *hclext.Block, attrName string) func() (bool, error) { + return func() (bool, error) { + _, exist := b.Body.Attributes[attrName] + if exist { + return false, r.EmitIssue(vcr, fmt.Sprintf("`%s` %s should not be declared", b.Labels[0], attrName), b.DefRange) + } + return true, nil + } +} + // getAttr returns a function that will return the attribute from a given hcl block. // It is designed to be used with the CheckWithReturnValue function. -func getAttr(rule tflint.Rule, r tflint.Runner, b *hclext.Block, attrName string, shouldExist bool) func() (*hclext.Attribute, bool, error) { +func getAttr(rule tflint.Rule, r tflint.Runner, b *hclext.Block, attrName string) func() (*hclext.Attribute, bool, error) { return func() (*hclext.Attribute, bool, error) { attr, exists := b.Body.Attributes[attrName] - if shouldExist != exists { - var msg string - switch shouldExist { - case true: - msg = "not declared" - case false: - msg = "should not be declared" - } + if !exists { return attr, false, r.EmitIssue( rule, - fmt.Sprintf("`%s` %s %s", b.Labels[0], attrName, msg), + fmt.Sprintf("`%s` %s not declared", b.Labels[0], attrName), b.DefRange, ) } diff --git a/interfaces/interfaces_simple_test.go b/interfaces/interfaces_simple_test.go index f039956..e7c1951 100644 --- a/interfaces/interfaces_simple_test.go +++ b/interfaces/interfaces_simple_test.go @@ -4,11 +4,10 @@ import ( "fmt" "testing" - "github.com/stretchr/testify/assert" - "github.com/Azure/tflint-ruleset-avm/interfaces" "github.com/hashicorp/hcl/v2" "github.com/matt-FFFFFF/tfvarcheck/varcheck" + "github.com/stretchr/testify/assert" "github.com/terraform-linters/tflint-plugin-sdk/helper" "github.com/terraform-linters/tflint-plugin-sdk/tflint" "github.com/zclconf/go-cty/cty"