Fix missing validation for count and for-each meta-arguments (#35432)

Author: Liam Cervante

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))
+			}
+		})
+	}
+
+}

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

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
 	}

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
 		}

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

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