diff --git a/internal/terraform/context_plan2_test.go b/internal/terraform/context_plan2_test.go index 6b91a4d4b3..0496b0a7f7 100644 --- a/internal/terraform/context_plan2_test.go +++ b/internal/terraform/context_plan2_test.go @@ -5783,3 +5783,67 @@ resource "test_object" "obj" { t.Errorf("unexpected diags\n%s", diags) } } + +func TestContext2Plan_selfReferences(t *testing.T) { + tcs := []struct { + attribute string + }{ + // Note here, the type returned by the lookup doesn't really matter as + // we should safely fail before we even get to type checking. + { + attribute: "count = test_object.a[0].test_string", + }, + { + attribute: "count = test_object.a[*].test_string", + }, + { + attribute: "for_each = test_object.a[0].test_string", + }, + { + attribute: "for_each = test_object.a[*].test_string", + }, + // Even though the can and try functions might normally allow some + // fairly crazy things, we're still going to put a stop to a self + // reference since it is more akin to a compilation error than some kind + // of dynamic exception. + { + attribute: "for_each = can(test_object.a[0].test_string) ? 0 : 1", + }, + { + attribute: "count = try(test_object.a[0].test_string, 0)", + }, + } + for _, tc := range tcs { + t.Run(tc.attribute, func(t *testing.T) { + tmpl := ` +resource "test_object" "a" { + %%attribute%% +} +` + module := strings.ReplaceAll(tmpl, "%%attribute%%", tc.attribute) + m := testModuleInline(t, map[string]string{ + "main.tf": module, + }) + + p := simpleMockProvider() + ctx := testContext2(t, &ContextOpts{ + Providers: map[addrs.Provider]providers.Factory{ + // The providers never actually going to get called here, we should + // catch the error long before anything happens. + addrs.NewDefaultProvider("test"): testProviderFuncFixed(p), + }, + }) + + _, diags := ctx.Plan(m, states.NewState(), DefaultPlanOpts) + if len(diags) != 1 { + t.Fatalf("expected one diag, got %d: %s", len(diags), diags.ErrWithWarnings()) + } + + got, want := diags.Err().Error(), "Self-referential block: Configuration for test_object.a may not refer to itself." + if cmp.Diff(want, got) != "" { + t.Fatalf("unexpected error\n%s", cmp.Diff(want, got)) + } + }) + } + +} diff --git a/internal/terraform/context_plan_import_test.go b/internal/terraform/context_plan_import_test.go index fef0949ba4..25baa8840a 100644 --- a/internal/terraform/context_plan_import_test.go +++ b/internal/terraform/context_plan_import_test.go @@ -1657,7 +1657,44 @@ resource "test_object" "a" { // We're expecting exactly one diag, which is the self-reference error. if len(diags) != 1 { - t.Fatalf("expected one diag, got %d", len(diags)) + t.Fatalf("expected one diag, got %d: %s", len(diags), diags.ErrWithWarnings()) + } + + got, want := diags.Err().Error(), "Invalid import id argument: The import ID cannot reference the resource being imported." + if cmp.Diff(want, got) != "" { + t.Fatalf("unexpected error\n%s", cmp.Diff(want, got)) + } +} + +func TestContext2Plan_importSelfReferenceInstanceRef(t *testing.T) { + m := testModuleInline(t, map[string]string{ + "main.tf": ` +import { + to = test_object.a + id = test_object.a[0].test_string +} + +resource "test_object" "a" { + count = 1 + test_string = "foo" +} +`, + }) + + p := simpleMockProvider() + ctx := testContext2(t, &ContextOpts{ + Providers: map[addrs.Provider]providers.Factory{ + // The providers never actually going to get called here, we should + // catch the error long before anything happens. + addrs.NewDefaultProvider("test"): testProviderFuncFixed(p), + }, + }) + + _, diags := ctx.Plan(m, states.NewState(), DefaultPlanOpts) + + // We're expecting exactly one diag, which is the self-reference error. + if len(diags) != 1 { + t.Fatalf("expected one diag, got %d: %s", len(diags), diags.ErrWithWarnings()) } got, want := diags.Err().Error(), "Invalid import id argument: The import ID cannot reference the resource being imported." @@ -1739,7 +1776,7 @@ output "foo" { // We're expecting exactly one diag, which is the self-reference error. if len(diags) != 1 { - t.Fatalf("expected one diag, got %d", len(diags)) + t.Fatalf("expected one diag, got %d: %s", len(diags), diags.ErrWithWarnings()) } // Terraform detects this case as a cycle, and the order of rendering the diff --git a/internal/terraform/eval_import.go b/internal/terraform/eval_import.go index 66f796855b..2a5aa6f87d 100644 --- a/internal/terraform/eval_import.go +++ b/internal/terraform/eval_import.go @@ -28,7 +28,7 @@ func evaluateImportIdExpression(expr hcl.Expression, target addrs.AbsResourceIns }) } - diags = diags.Append(validateSelfRefFromImport(target.Resource.Resource, expr)) + diags = diags.Append(validateImportSelfRef(target.Resource.Resource, expr)) if diags.HasErrors() { return cty.NilVal, diags } diff --git a/internal/terraform/node_resource_plan.go b/internal/terraform/node_resource_plan.go index 0738550fcc..c0329cd6b7 100644 --- a/internal/terraform/node_resource_plan.go +++ b/internal/terraform/node_resource_plan.go @@ -103,8 +103,8 @@ func (n *nodeExpandPlannableResource) DynamicExpand(ctx EvalContext) (*Graph, tf // resource. The config maybe nil if we are generating configuration, or // deleting a resource. if n.Config != nil { - diags = diags.Append(validateSelfRefInExpr(n.Addr.Resource, n.Config.Count)) - diags = diags.Append(validateSelfRefInExpr(n.Addr.Resource, n.Config.ForEach)) + diags = diags.Append(validateMetaSelfRef(n.Addr.Resource, n.Config.Count)) + diags = diags.Append(validateMetaSelfRef(n.Addr.Resource, n.Config.ForEach)) if diags.HasErrors() { return nil, diags } diff --git a/internal/terraform/validate_selfref.go b/internal/terraform/validate_selfref.go index 3cc5c5e93c..521ecae16b 100644 --- a/internal/terraform/validate_selfref.go +++ b/internal/terraform/validate_selfref.go @@ -58,63 +58,69 @@ func validateSelfRef(addr addrs.Referenceable, config hcl.Body, providerSchema p return diags } -// validateSelfRefInExpr checks to ensure that a specific expression does not -// reference the same block that it is contained within. -func validateSelfRefInExpr(addr addrs.Referenceable, expr hcl.Expression) tfdiags.Diagnostics { - var diags tfdiags.Diagnostics - - addrStrs := make([]string, 0, 1) - addrStrs = append(addrStrs, addr.String()) - switch tAddr := addr.(type) { - case addrs.ResourceInstance: - // A resource instance may not refer to its containing resource either. - addrStrs = append(addrStrs, tAddr.ContainingResource().String()) - } - - refs, _ := langrefs.ReferencesInExpr(addrs.ParseRef, expr) - for _, ref := range refs { - - for _, addrStr := range addrStrs { - if ref.Subject.String() == addrStr { - diags = diags.Append(&hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Self-referential block", - Detail: fmt.Sprintf("Configuration for %s may not refer to itself.", addrStr), - Subject: ref.SourceRange.ToHCL().Ptr(), - }) - } +// validateMetaSelfRef checks to ensure that a specific meta expression (count / +// for_each) does not reference the resource it is attached to. The behaviour +// is slightly different from validateSelfRef in that this function is only ever +// called from static contexts (ie. before expansion) and as such the address is +// always a Resource. +// +// This also means that often the references will be to instances of the +// resource, so we need to unpack these to the containing resource to compare +// against the static resource. From the perspective of this function +// `test_resource.foo[4]` is considered to be a self reference to +// `test_resource.foo`, in which is a significant behaviour change to +// validateSelfRef. +func validateMetaSelfRef(addr addrs.Resource, expr hcl.Expression) tfdiags.Diagnostics { + return validateSelfRefFromExprInner(addr, expr, func(ref *addrs.Reference) *hcl.Diagnostic { + return &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Self-referential block", + Detail: fmt.Sprintf("Configuration for %s may not refer to itself.", addr.String()), + Subject: ref.SourceRange.ToHCL().Ptr(), } - } - - return diags + }) } -// validateSelfRefFromImport is similar to validateSelfRefInExpr except it +// validateImportSelfRef is similar to validateMetaSelfRef except it // tweaks the error message slightly to reflect the self-reference is coming -// from an import block instead of directly from the resource. -func validateSelfRefFromImport(addr addrs.Referenceable, expr hcl.Expression) tfdiags.Diagnostics { - var diags tfdiags.Diagnostics +// from an import block instead of directly from the resource. All the same +// caveats apply as validateMetaSelfRef. +func validateImportSelfRef(addr addrs.Resource, expr hcl.Expression) tfdiags.Diagnostics { + return validateSelfRefFromExprInner(addr, expr, func(ref *addrs.Reference) *hcl.Diagnostic { + return &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Invalid import id argument", + Detail: "The import ID cannot reference the resource being imported.", + Subject: ref.SourceRange.ToHCL().Ptr(), + } + }) +} - addrStrs := make([]string, 0, 1) - addrStrs = append(addrStrs, addr.String()) - switch tAddr := addr.(type) { - case addrs.ResourceInstance: - // A resource instance may not refer to its containing resource either. - addrStrs = append(addrStrs, tAddr.ContainingResource().String()) - } +// validateSelfRefFromExprInner is a helper function that takes an address and +// an expression and returns diagnostics for self-references in the expression. +// +// This should only be called via validateMetaSelfRef and validateImportSelfRef, +// do not access this function directly. +func validateSelfRefFromExprInner(addr addrs.Resource, expr hcl.Expression, diag func(ref *addrs.Reference) *hcl.Diagnostic) tfdiags.Diagnostics { + var diags tfdiags.Diagnostics refs, _ := langrefs.ReferencesInExpr(addrs.ParseRef, expr) for _, ref := range refs { + var target addrs.Resource + switch t := ref.Subject.(type) { + case addrs.ResourceInstance: + // Automatically unpack an instance reference to its containing + // resource, since we're only comparing against the static resource. + target = t.Resource + case addrs.Resource: + target = t + default: + // Anything else cannot be a self-reference. + continue + } - for _, addrStr := range addrStrs { - if ref.Subject.String() == addrStr { - diags = diags.Append(&hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Invalid import id argument", - Detail: "The import ID cannot reference the resource being imported.", - Subject: ref.SourceRange.ToHCL().Ptr(), - }) - } + if target.Equal(addr) { + diags = diags.Append(diag(ref)) } } diff --git a/internal/terraform/validate_selfref_test.go b/internal/terraform/validate_selfref_test.go index 8228871a1c..c6a4a8626c 100644 --- a/internal/terraform/validate_selfref_test.go +++ b/internal/terraform/validate_selfref_test.go @@ -109,10 +109,55 @@ func TestValidateSelfRef(t *testing.T) { t.Errorf("unexpected error\n\n%s", diags.Err()) } } + }) + } +} + +func TestValidateSelfInExpr(t *testing.T) { + rAddr := addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "aws_instance", + Name: "foo", + } - // We can use the same expressions to test the expression - // validation. - diags = validateSelfRefInExpr(test.Addr, test.Expr) + tests := []struct { + Name string + Addr addrs.Resource + Expr hcl.Expression + Err bool + }{ + { + "no references at all", + rAddr, + hcltest.MockExprLiteral(cty.StringVal("bar")), + false, + }, + + { + "non self reference", + rAddr, + hcltest.MockExprTraversalSrc("aws_instance.bar.id"), + false, + }, + + { + "self reference", + rAddr, + hcltest.MockExprTraversalSrc("aws_instance.foo.id"), + true, + }, + + { + "self reference other index", + rAddr, + hcltest.MockExprTraversalSrc("aws_instance.foo[4].id"), + true, + }, + } + + for i, test := range tests { + t.Run(fmt.Sprintf("%d-%s", i, test.Name), func(t *testing.T) { + diags := validateMetaSelfRef(test.Addr, test.Expr) if diags.HasErrors() != test.Err { if test.Err { t.Errorf("unexpected success; want error")