import: fix crash when referencing the to-be-imported resource from the 'id' field (#35420)

Author: Liam Cervante

internal/terraform/context_plan_import_test.go

@@ -1564,6 +1564,7 @@ import {
 		t.Fatalf("unexpected errors\n%s", diags.Err().Error())
 	}
 }
+
 func TestContext2Plan_importDuringDestroy(t *testing.T) {
 	m := testModuleInline(t, map[string]string{
 		"main.tf": `
@@ -1628,3 +1629,130 @@ func TestContext2Plan_importDuringDestroy(t *testing.T) {
 		t.Fatalf("unexpected errors\n%s", diags.Err().Error())
 	}
 }
+
+func TestContext2Plan_importSelfReference(t *testing.T) {
+	m := testModuleInline(t, map[string]string{
+		"main.tf": `
+import {
+  to = test_object.a
+  id = test_object.a.test_string
+}
+
+resource "test_object" "a" {
+  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", len(diags))
+	}
+
+	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_importSelfReferenceInst(t *testing.T) {
+	m := testModuleInline(t, map[string]string{
+		"main.tf": `
+import {
+  to = test_object.a[0]
+  id = test_object.a.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."
+	if cmp.Diff(want, got) != "" {
+		t.Fatalf("unexpected error\n%s", cmp.Diff(want, got))
+	}
+}
+
+func TestContext2Plan_importSelfReferenceInModule(t *testing.T) {
+	m := testModuleInline(t, map[string]string{
+		"main.tf": `
+import {
+  to = module.mod.test_object.a
+  id = module.mod.foo
+}
+
+module "mod" {
+  source = "./mod"
+}
+`,
+		"mod/main.tf": `
+resource "test_object" "a" {
+  test_string = "foo"
+}
+
+output "foo" {
+  value = test_object.a.test_string
+}
+`,
+	})
+
+	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", len(diags))
+	}
+
+	// Terraform detects this case as a cycle, and the order of rendering the
+	// cycle if non-deterministic, so we can't do a straight string match.
+
+	got := diags.Err().Error()
+	if !strings.Contains(got, "Cycle:") {
+		t.Errorf("should have reported a cycle error, but got %s", got)
+	}
+	if !strings.Contains(got, "module.mod.output.foo (expand)") {
+		t.Errorf("should have reported the cycle to contain the module output, but got %s", got)
+	}
+	if !strings.Contains(got, "module.mod.test_object.a (expand)") {
+		t.Errorf("should have reported the cycle to contain the target resource, but got %s", got)
+	}
+}

internal/terraform/eval_import.go

@@ -16,13 +16,9 @@ import (
 	"github.com/hashicorp/terraform/internal/tfdiags"
 )
 
-func evaluateImportIdExpression(expr hcl.Expression, ctx EvalContext, keyData instances.RepetitionData, allowUnknown bool) (cty.Value, tfdiags.Diagnostics) {
+func evaluateImportIdExpression(expr hcl.Expression, target addrs.AbsResourceInstance, ctx EvalContext, keyData instances.RepetitionData, allowUnknown bool) (cty.Value, tfdiags.Diagnostics) {
 	var diags tfdiags.Diagnostics
 
-	// import blocks only exist in the root module, and must be evaluated in
-	// that context.
-	ctx = evalContextForModuleInstance(ctx, addrs.RootModuleInstance)
-
 	if expr == nil {
 		return cty.NilVal, diags.Append(&hcl.Diagnostic{
 			Severity: hcl.DiagError,
@@ -32,6 +28,14 @@ func evaluateImportIdExpression(expr hcl.Expression, ctx EvalContext, keyData in
 		})
 	}
 
+	diags = diags.Append(validateSelfRefFromImport(target.Resource.Resource, expr))
+	if diags.HasErrors() {
+		return cty.NilVal, diags
+	}
+
+	// import blocks only exist in the root module, and must be evaluated in
+	// that context.
+	ctx = evalContextForModuleInstance(ctx, addrs.RootModuleInstance)
 	scope := ctx.EvaluationScope(nil, nil, keyData)
 	importIdVal, evalDiags := scope.EvalExpr(expr, cty.String)
 	diags = diags.Append(evalDiags)

internal/terraform/node_resource_plan.go

@@ -177,11 +177,6 @@ func (n *nodeExpandPlannableResource) expandResourceImports(ctx EvalContext, all
 		}
 
 		if imp.Config.ForEach == nil {
-			importID, evalDiags := evaluateImportIdExpression(imp.Config.ID, ctx, EvalDataForNoInstanceKey, allowUnknown)
-			diags = diags.Append(evalDiags)
-			if diags.HasErrors() {
-				return knownImports, unknownImports, diags
-			}
 
 			traversal, hds := hcl.AbsTraversalForExpr(imp.Config.To)
 			diags = diags.Append(hds)
@@ -191,6 +186,12 @@ func (n *nodeExpandPlannableResource) expandResourceImports(ctx EvalContext, all
 				return knownImports, unknownImports, diags
 			}
 
+			importID, evalDiags := evaluateImportIdExpression(imp.Config.ID, to, ctx, EvalDataForNoInstanceKey, allowUnknown)
+			diags = diags.Append(evalDiags)
+			if diags.HasErrors() {
+				return knownImports, unknownImports, diags
+			}
+
 			knownImports.Put(to, importID)
 
 			log.Printf("[TRACE] expandResourceImports: found single import target %s", to)
@@ -242,7 +243,7 @@ func (n *nodeExpandPlannableResource) expandResourceImports(ctx EvalContext, all
 				return knownImports, unknownImports, diags
 			}
 
-			importID, evalDiags := evaluateImportIdExpression(imp.Config.ID, ctx, keyData, allowUnknown)
+			importID, evalDiags := evaluateImportIdExpression(imp.Config.ID, res, ctx, keyData, allowUnknown)
 			diags = diags.Append(evalDiags)
 			if diags.HasErrors() {
 				return knownImports, unknownImports, diags

internal/terraform/validate_selfref.go

@@ -89,6 +89,38 @@ func validateSelfRefInExpr(addr addrs.Referenceable, expr hcl.Expression) tfdiag
 	return diags
 }
 
+// validateSelfRefFromImport is similar to validateSelfRefInExpr 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
+
+	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:  "Invalid import id argument",
+					Detail:   "The import ID cannot reference the resource being imported.",
+					Subject:  ref.SourceRange.ToHCL().Ptr(),
+				})
+			}
+		}
+	}
+
+	return diags
+}
+
 // Legacy provisioner configurations may refer to single instances using the
 // resource address. We need to filter these out from the reported references
 // to prevent cycles.