diff --git a/changes/unreleased/Fixed-20231122-134818.yaml b/changes/unreleased/Fixed-20231122-134818.yaml new file mode 100644 index 00000000..e5072be5 --- /dev/null +++ b/changes/unreleased/Fixed-20231122-134818.yaml @@ -0,0 +1,3 @@ +kind: Fixed +body: phantom attributes on multi-resources +time: 2023-11-22T13:48:18.832055057+01:00 diff --git a/pkg/hcl_interpreter/accessor.go b/pkg/hcl_interpreter/accessor.go new file mode 100644 index 00000000..a60a72cd --- /dev/null +++ b/pkg/hcl_interpreter/accessor.go @@ -0,0 +1,116 @@ +package hcl_interpreter + +import ( + "fmt" + "strconv" + "strings" + + "github.com/hashicorp/hcl/v2" + "github.com/zclconf/go-cty/cty" +) + +// accessor represents paths in HCL that can contain both string or int parts, +// e.g. "foo.bar[3].qux". +type accessor []interface{} + +func (a accessor) toString() string { + buf := &strings.Builder{} + for i, p := range a { + switch p := p.(type) { + case int: + fmt.Fprintf(buf, "[%d]", p) + case string: + if i == 0 { + fmt.Fprintf(buf, "%s", p) + } else { + fmt.Fprintf(buf, ".%s", p) + } + } + } + return buf.String() +} + +func stringToAccessor(input string) (accessor, error) { + parts := []interface{}{} + for len(input) > 0 { + if input[0] == '[' { + end := strings.IndexByte(input, ']') + if end < 0 { + return nil, fmt.Errorf("unmatched [") + } + num, err := strconv.Atoi(input[1:end]) + if err != nil { + return nil, err + } + parts = append(parts, num) + input = input[end+1:] + if len(input) > 0 && input[0] == '.' { + input = input[1:] // Consume extra '.' after ']' + } + } else { + end := strings.IndexAny(input, ".[") + if end < 0 { + parts = append(parts, input) + input = "" + } else { + parts = append(parts, input[:end]) + if input[end] == '.' { + input = input[end+1:] + } else { + input = input[end:] + } + } + } + } + return parts, nil +} + +func traversalToAccessor(traversal hcl.Traversal) (accessor, error) { + parts := make(accessor, 0) + for _, traverser := range traversal { + switch t := traverser.(type) { + case hcl.TraverseRoot: + parts = append(parts, t.Name) + case hcl.TraverseAttr: + parts = append(parts, t.Name) + case hcl.TraverseIndex: + val := t.Key + if val.IsKnown() { + if val.Type() == cty.Number { + n := val.AsBigFloat() + if n.IsInt() { + i, _ := n.Int64() + parts = append(parts, int(i)) + } else { + return nil, fmt.Errorf("Non-int number type in TraverseIndex") + } + } else if val.Type() == cty.String { + parts = append(parts, val.AsString()) + } else { + return nil, fmt.Errorf("Unsupported type in TraverseIndex: %s", val.Type().GoString()) + } + } else { + return nil, fmt.Errorf("Unknown value in TraverseIndex") + } + } + } + return parts, nil +} + +// toLocalName tries to convert the accessor to a local name starting from the +// front. As soon as a non-string part is encountered, we stop and return the +// trailing accessor as well. +func (a accessor) toLocalName() (LocalName, accessor) { + name := make(LocalName, 0) + trailing := make(accessor, len(a)) + copy(trailing, a) + for len(trailing) > 0 { + if str, ok := trailing[0].(string); ok { + name = append(name, str) + trailing = trailing[1:] + } else { + break + } + } + return name, trailing +} diff --git a/pkg/hcl_interpreter/accessor_test.go b/pkg/hcl_interpreter/accessor_test.go new file mode 100644 index 00000000..dec920c1 --- /dev/null +++ b/pkg/hcl_interpreter/accessor_test.go @@ -0,0 +1,93 @@ +package hcl_interpreter + +import ( + "fmt" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestAccessorToString(t *testing.T) { + tests := []struct { + input accessor + expected string + }{ + { + input: accessor{"foo", "bar", 3, "qux"}, + expected: "foo.bar[3].qux", + }, + } + for i, test := range tests { + t.Run(fmt.Sprintf("case%02d", i), func(t *testing.T) { + actual := test.input.toString() + assert.Equal(t, test.expected, actual) + }) + } +} + +func TestStringToAccessor(t *testing.T) { + tests := []struct { + input string + expected accessor + err bool + }{ + { + input: "foo.bar[3].qux", + expected: accessor{"foo", "bar", 3, "qux"}, + }, + { + input: "[1][2][3]", + expected: accessor{1, 2, 3}, + }, + { + input: "foo[3.qux", + err: true, + }, + { + input: "foo.bar[three].qux", + err: true, + }, + } + for i, test := range tests { + t.Run(fmt.Sprintf("case%02d", i), func(t *testing.T) { + actual, err := stringToAccessor(test.input) + if test.err { + assert.Error(t, err) + } else { + assert.NoError(t, err) + assert.Equal(t, test.expected, actual) + } + }) + } +} + +func TestAccessorToLocalName(t *testing.T) { + tests := []struct { + input accessor + expected LocalName + trailing accessor + }{ + { + input: accessor{"aws_s3_bucket", "my_bucket", 0, "id"}, + expected: LocalName{"aws_s3_bucket", "my_bucket"}, + trailing: accessor{0, "id"}, + }, + { + input: accessor{}, + expected: LocalName{}, + trailing: accessor{}, + }, + { + input: accessor{"aws_s3_bucket", "my_bucket", "id"}, + expected: LocalName{"aws_s3_bucket", "my_bucket", "id"}, + trailing: accessor{}, + }, + } + for i, test := range tests { + t.Run(fmt.Sprintf("case%02d", i), func(t *testing.T) { + actual, trailing := test.input.toLocalName() + assert.Equal(t, test.expected, actual) + assert.Equal(t, test.trailing, trailing) + }) + } +} diff --git a/pkg/hcl_interpreter/hcl_interpreter.go b/pkg/hcl_interpreter/hcl_interpreter.go index 9a1e7207..55af7a77 100644 --- a/pkg/hcl_interpreter/hcl_interpreter.go +++ b/pkg/hcl_interpreter/hcl_interpreter.go @@ -102,7 +102,7 @@ func (v *Analysis) dependencies(name FullName, term Term) []dependency { deps := []dependency{} for _, termDependency := range term.Dependencies() { traversal := termDependency.Traversal - local, err := TraversalToLocalName(traversal) + local, _, err := TraversalToLocalName(traversal) if err != nil { v.badKeys[TraversalToString(traversal)] = struct{}{} continue @@ -224,11 +224,15 @@ type Evaluation struct { } func EvaluateAnalysis(analysis *Analysis) (*Evaluation, error) { + isMultiResource := func(name FullName) bool { + meta, ok := analysis.Resources[name.ToString()] + return ok && meta.Multiple + } eval := &Evaluation{ Analysis: analysis, Modules: map[string]cty.Value{}, resourceAttributes: map[string]cty.Value{}, - phantomAttrs: newPhantomAttrs(), + phantomAttrs: newPhantomAttrs(isMultiResource), } for moduleKey := range analysis.Modules { @@ -306,11 +310,7 @@ func (v *Evaluation) evaluate() error { } v.resourceAttributes[name.ToString()] = val - patchMultiple := false - if resourceMeta, ok := v.Analysis.Resources[name.ToString()]; ok { - patchMultiple = resourceMeta.Multiple - } - val = v.phantomAttrs.add(name, patchMultiple, val) + val = v.phantomAttrs.add(name, val) singleton := NestVal(name.Local, val) v.Modules[moduleKey] = MergeVal(v.Modules[moduleKey], singleton) } diff --git a/pkg/hcl_interpreter/names.go b/pkg/hcl_interpreter/names.go index b785ae0c..38ceafa9 100644 --- a/pkg/hcl_interpreter/names.go +++ b/pkg/hcl_interpreter/names.go @@ -78,7 +78,7 @@ func ChildModuleName(moduleName ModuleName, childName string) ModuleName { type LocalName []string var ( - // Supported fixed paths can be checked using Equals. + // Supported fixed paths can be checked using Equals. PathModuleName = LocalName{"path", "module"} PathRootName = LocalName{"path", "root"} PathCwdName = LocalName{"path", "cwd"} @@ -101,6 +101,14 @@ func (name LocalName) Equals(other LocalName) bool { return true } +func (name LocalName) ToAccessor() accessor { + accessor := make(accessor, len(name)) + for i := range name { + accessor[i] = name[i] + } + return accessor +} + type FullName struct { Module ModuleName Local LocalName @@ -217,34 +225,16 @@ func (name FullName) AsResourceName() (*FullName, LocalName) { return nil, nil } -// TODO: Refactor to TraversalToName? -func TraversalToLocalName(traversal hcl.Traversal) (LocalName, error) { - parts := make([]string, 0) - - for _, traverser := range traversal { - switch t := traverser.(type) { - case hcl.TraverseRoot: - parts = append(parts, t.Name) - case hcl.TraverseAttr: - parts = append(parts, t.Name) - case hcl.TraverseIndex: - val := t.Key - if val.IsKnown() { - if val.Type() == cty.Number { - // The other parts must be trailing accessors. - return parts, nil - } else if val.Type() == cty.String { - parts = append(parts, val.AsString()) - } else { - return nil, fmt.Errorf("Unsupported type in TraverseIndex: %s", val.Type().GoString()) - } - } else { - return nil, fmt.Errorf("Unknown value in TraverseIndex") - } - } +// TraversalToLocalName returns the leading LocalName (strictly-string) part of +// a traversal as well as the trailing part (string-or-int). +func TraversalToLocalName(traversal hcl.Traversal) (LocalName, accessor, error) { + // Just delegate the conversion to the accessor. + accessor, err := traversalToAccessor(traversal) + if err != nil { + return nil, nil, err } - - return parts, nil + name, trailing := accessor.toLocalName() + return name, trailing, nil } func TraversalToString(traversal hcl.Traversal) string { diff --git a/pkg/hcl_interpreter/phantom_attrs.go b/pkg/hcl_interpreter/phantom_attrs.go index ba106314..be7b6ff8 100644 --- a/pkg/hcl_interpreter/phantom_attrs.go +++ b/pkg/hcl_interpreter/phantom_attrs.go @@ -26,13 +26,18 @@ import ( ) type phantomAttrs struct { + // Whether or not this is a multiresource (count or for_each). + isMultiResource func(FullName) bool // A set of phantom attributes per FullName. attrs map[string]map[string]struct{} } -func newPhantomAttrs() *phantomAttrs { +func newPhantomAttrs( + isMultiResource func(FullName) bool, +) *phantomAttrs { return &phantomAttrs{ - attrs: map[string]map[string]struct{}{}, + isMultiResource: isMultiResource, + attrs: map[string]map[string]struct{}{}, } } @@ -52,17 +57,32 @@ func (pa *phantomAttrs) analyze(name FullName, term Term) { term.VisitExpressions(func(expr hcl.Expression) { exprAttrs := exprAttributes(expr) for _, traversal := range expr.Variables() { - local, err := TraversalToLocalName(traversal) + local, localTrailing, err := TraversalToLocalName(traversal) if err != nil { continue } full := FullName{Module: name.Module, Local: local} - if asResourceName, trailing := full.AsResourceName(); asResourceName != nil { + if asResourceName, resourceTrailing := full.AsResourceName(); asResourceName != nil { attrs := map[string]struct{}{} - if len(trailing) > 0 { + + // Construct the trailing part from the resourceTrailing + // and localTrailing parts defined above. + trailingAccessor := resourceTrailing.ToAccessor() + trailingAccessor = append(trailingAccessor, localTrailing...) + + // We need to strip one element from the index if this is a + // multi-resource to drop the index or key. + if pa.isMultiResource(*asResourceName) && len(trailingAccessor) > 0 { + trailingAccessor = trailingAccessor[1:] + } + + // Store the trailing part that was accessed in attrs. + if trailing, _ := trailingAccessor.toLocalName(); len(trailing) > 0 { attrs[LocalNameToString(trailing)] = struct{}{} } + + // Store the other attrs. for _, attr := range exprAttrs { attrs[LocalNameToString(attr)] = struct{}{} } @@ -81,8 +101,9 @@ func (pa *phantomAttrs) analyze(name FullName, term Term) { }) } -func (pa *phantomAttrs) add(name FullName, multiple bool, val cty.Value) cty.Value { +func (pa *phantomAttrs) add(name FullName, val cty.Value) cty.Value { rk := name.ToString() + multiple := pa.isMultiResource(name) var patch func(LocalName, string, cty.Value) cty.Value patch = func(local LocalName, ref string, val cty.Value) cty.Value { @@ -136,7 +157,7 @@ func exprAttributes(expr hcl.Expression) []LocalName { hclsyntax.VisitAll(syn, func(node hclsyntax.Node) hcl.Diagnostics { switch e := node.(type) { case *hclsyntax.RelativeTraversalExpr: - if name, err := TraversalToLocalName(e.Traversal); err == nil { + if name, _, err := TraversalToLocalName(e.Traversal); err == nil { names = append(names, name) } case *hclsyntax.IndexExpr: diff --git a/pkg/input/golden_test/tf/count-ref.json b/pkg/input/golden_test/tf/count-ref-01.json similarity index 85% rename from pkg/input/golden_test/tf/count-ref.json rename to pkg/input/golden_test/tf/count-ref-01.json index 8c747b6e..d145dc21 100644 --- a/pkg/input/golden_test/tf/count-ref.json +++ b/pkg/input/golden_test/tf/count-ref-01.json @@ -4,14 +4,14 @@ "input_type": "tf_hcl", "environment_provider": "iac", "meta": { - "filepath": "golden_test/tf/count-ref/main.tf" + "filepath": "golden_test/tf/count-ref-01/main.tf" }, "resources": { "aws_s3_bucket": { "aws_s3_bucket.not_working_1[0]": { "id": "aws_s3_bucket.not_working_1[0]", "resource_type": "aws_s3_bucket", - "namespace": "golden_test/tf/count-ref/main.tf", + "namespace": "golden_test/tf/count-ref-01/main.tf", "meta": { "region": "eu-west-1", "terraform": { @@ -30,7 +30,7 @@ "aws_s3_bucket_public_access_block.not_working_1_block[0]": { "id": "aws_s3_bucket_public_access_block.not_working_1_block[0]", "resource_type": "aws_s3_bucket_public_access_block", - "namespace": "golden_test/tf/count-ref/main.tf", + "namespace": "golden_test/tf/count-ref-01/main.tf", "meta": { "region": "eu-west-1", "terraform": { @@ -50,6 +50,6 @@ } }, "scope": { - "filepath": "golden_test/tf/count-ref/main.tf" + "filepath": "golden_test/tf/count-ref-01/main.tf" } } \ No newline at end of file diff --git a/pkg/input/golden_test/tf/count-ref/main.tf b/pkg/input/golden_test/tf/count-ref-01/main.tf similarity index 100% rename from pkg/input/golden_test/tf/count-ref/main.tf rename to pkg/input/golden_test/tf/count-ref-01/main.tf diff --git a/pkg/input/golden_test/tf/count-ref-02.json b/pkg/input/golden_test/tf/count-ref-02.json new file mode 100644 index 00000000..675ab40d --- /dev/null +++ b/pkg/input/golden_test/tf/count-ref-02.json @@ -0,0 +1,64 @@ +{ + "format": "", + "format_version": "", + "input_type": "tf_hcl", + "environment_provider": "iac", + "meta": { + "filepath": "golden_test/tf/count-ref-02/main.tf" + }, + "resources": { + "aws_s3_bucket": { + "aws_s3_bucket.febucket[one]": { + "id": "aws_s3_bucket.febucket[one]", + "resource_type": "aws_s3_bucket", + "namespace": "golden_test/tf/count-ref-02/main.tf", + "meta": {}, + "attributes": { + "bucket": "test" + } + }, + "aws_s3_bucket.nativebucket[0]": { + "id": "aws_s3_bucket.nativebucket[0]", + "resource_type": "aws_s3_bucket", + "namespace": "golden_test/tf/count-ref-02/main.tf", + "meta": {}, + "attributes": { + "bucket": "test" + } + } + }, + "aws_s3_bucket_versioning": { + "aws_s3_bucket_versioning.febucket[one]": { + "id": "aws_s3_bucket_versioning.febucket[one]", + "resource_type": "aws_s3_bucket_versioning", + "namespace": "golden_test/tf/count-ref-02/main.tf", + "meta": {}, + "attributes": { + "bucket": "aws_s3_bucket.febucket[one]", + "versioning_configuration": [ + { + "status": "Enabled" + } + ] + } + }, + "aws_s3_bucket_versioning.nativebucket[0]": { + "id": "aws_s3_bucket_versioning.nativebucket[0]", + "resource_type": "aws_s3_bucket_versioning", + "namespace": "golden_test/tf/count-ref-02/main.tf", + "meta": {}, + "attributes": { + "bucket": "aws_s3_bucket.nativebucket[0]", + "versioning_configuration": [ + { + "status": "Enabled" + } + ] + } + } + } + }, + "scope": { + "filepath": "golden_test/tf/count-ref-02/main.tf" + } +} \ No newline at end of file diff --git a/pkg/input/golden_test/tf/count-ref-02/main.tf b/pkg/input/golden_test/tf/count-ref-02/main.tf new file mode 100644 index 00000000..fb66efe6 --- /dev/null +++ b/pkg/input/golden_test/tf/count-ref-02/main.tf @@ -0,0 +1,25 @@ +resource "aws_s3_bucket" "nativebucket" { + count = 1 + bucket = "test" +} + +resource "aws_s3_bucket_versioning" "nativebucket" { + count = 1 + bucket = aws_s3_bucket.nativebucket[0].id + versioning_configuration { + status = "Enabled" + } +} + +resource "aws_s3_bucket" "febucket" { + for_each = {"one": "one"} + bucket = "test" +} + +resource "aws_s3_bucket_versioning" "febucket" { + for_each = {"one": "one"} + bucket = aws_s3_bucket.febucket["one"].id + versioning_configuration { + status = "Enabled" + } +}