Import block ID validation (#2973)

Signed-off-by: Ilia Gogotchuri <ilia.gogotchuri0@gmail.com>

CHANGELOG.md

@@ -17,6 +17,7 @@ BUG FIXES:
 
 * The `tofu.rc` configuration file now properly takes precedence over `terraform.rc` on Windows ([#2891](https://github.com/opentofu/opentofu/pull/2891))
 * S3 backend now correctly sends the `x-amz-server-side-encryption` header for the lockfile ([#2870](https://github.com/opentofu/opentofu/issues/2970))
+* The `import` block now correctly validates the `id` property. ([#2416](https://github.com/opentofu/opentofu/issues/2416)
 * Allow function calls in test variable blocks ([#2947](https://github.com/opentofu/opentofu/pull/2947))
 * The `issensitive` function now returns an unknown result when its argument is unknown, since a sensitive unknown value can potentially become non-sensitive once more information is available. ([#3008](https://github.com/opentofu/opentofu/pull/3008))
 * Provider references like "null.some_alias[each.key]" in .tf.json files are now correctly parsed ([#2915](https://github.com/opentofu/opentofu/issues/2915))

internal/command/testdata/validate-invalid/import_undefined_var/main.tf

@@ -0,0 +1,7 @@
+resource "test_instance" "web" {
+}
+
+import {
+  to = test_instance.web
+  id = var.undefined
+}

internal/command/testdata/validate-valid/import_id_defined_var/main.tf

@@ -0,0 +1,9 @@
+variable "defined" {
+  type = string
+  default = "example_id"
+}
+
+import {
+  to = test_instance.web
+  id = var.defined
+}

internal/command/testdata/validate-valid/import_undefined_resource/main.tf

@@ -0,0 +1,4 @@
+import {
+  to = test_instance.web
+  id = "test2"
+}

internal/command/validate_test.go

@@ -166,6 +166,32 @@ func TestSameImportTargetMultipleTimesShouldFail(t *testing.T) {
 	}
 }
 
+func TestUndefinedVariableAsImportIDShouldFail(t *testing.T) {
+	output, code := setupTest(t, "validate-invalid/import_undefined_var")
+	if code != 1 {
+		t.Fatalf("Should have failed: %d\n\n%s", code, output.Stderr())
+	}
+	wantError := `Error: Reference to undeclared input variable`
+	if !strings.Contains(output.Stderr(), wantError) {
+		t.Fatalf("Missing error string %q\n\n'%s'", wantError, output.Stderr())
+	}
+}
+
+func TestUndefinedResourceAsImportTargetShouldSucceed(t *testing.T) {
+	// -generate-config-out is the reason we can have undefined resources as targets
+	output, code := setupTest(t, "validate-valid/import_undefined_resource")
+	if code != 0 {
+		t.Fatalf("Should have succeeded: %d\n\n%s", code, output.Stderr())
+	}
+}
+
+func TestDefinedVarAsImportIDShouldSucceed(t *testing.T) {
+	output, code := setupTest(t, "validate-valid/import_id_defined_var")
+	if code != 0 {
+		t.Fatalf("Should have succeeded: %d\n\n%s", code, output.Stderr())
+	}
+}
+
 func TestOutputWithoutValueShouldFail(t *testing.T) {
 	output, code := setupTest(t, "validate-invalid/outputs")
 	if code != 1 {

internal/tofu/context_import.go

@@ -108,6 +108,54 @@ func NewImportResolver() *ImportResolver {
 	return &ImportResolver{imports: make(map[string]EvaluatedConfigImportTarget)}
 }
 
+// ValidateImportIDs is used during the validation phase to validate the import IDs of all import targets.
+// This function works similarly to ExpandAndResolveImport, but it only validates the IDs of the import targets and does not modify the EvalContext.
+// We only validate the IDs during the validation phase. Otherwise, we might cause a false positive,
+// since we do not know if the user intends to use the '-generate-config-out' option to generate additional configuration, which would make invalid Addresses valid
+func (ri *ImportResolver) ValidateImportIDs(ctx context.Context, importTarget *ImportTarget, evalCtx EvalContext) tfdiags.Diagnostics {
+	var diags tfdiags.Diagnostics
+
+	// The import block expressions are declared within the root module.
+	// We need to explicitly use the context with the path of the root module, so that all references will be
+	// relative to the root module
+	rootCtx := evalCtx.WithPath(addrs.RootModuleInstance)
+
+	if importTarget.Config.ForEach != nil {
+		const unknownsNotAllowed = false
+		const tupleAllowed = true
+
+		// The import target has a for_each attribute, so we need to expand it
+		forEachVal, evalDiags := evaluateForEachExpressionValue(ctx, importTarget.Config.ForEach, rootCtx, unknownsNotAllowed, tupleAllowed, nil)
+		diags = diags.Append(evalDiags)
+		if diags.HasErrors() {
+			return diags
+		}
+
+		// We are building an instances.RepetitionData based on each for_each key and val combination
+		var repetitions []instances.RepetitionData
+
+		it := forEachVal.ElementIterator()
+		for it.Next() {
+			k, v := it.Element()
+			repetitions = append(repetitions, instances.RepetitionData{
+				EachKey:   k,
+				EachValue: v,
+			})
+		}
+
+		for _, keyData := range repetitions {
+			_, evalDiags = evaluateImportIdExpression(importTarget.Config.ID, evalCtx, keyData)
+			diags = diags.Append(evalDiags)
+		}
+	} else {
+		// The import target is singular, no need to expand
+		_, evalDiags := evaluateImportIdExpression(importTarget.Config.ID, evalCtx, EvalDataForNoInstanceKey)
+		diags = diags.Append(evalDiags)
+	}
+
+	return diags
+}
+
 // ExpandAndResolveImport is responsible for two operations:
 // 1. Expands the ImportTarget (originating from an import block) if it contains a 'for_each' attribute.
 // 2. Goes over the expanded imports and resolves the ID and address, when we have the context necessary to resolve

internal/tofu/context_validate.go

@@ -67,6 +67,8 @@ func (c *Context) Validate(ctx context.Context, config *configs.Config) tfdiags.
 		}
 	}
 
+	importTargets := c.findImportTargets(config)
+
 	providerFunctionTracker := make(ProviderFunctionMapping)
 
 	graph, moreDiags := (&PlanGraphBuilder{
@@ -76,6 +78,10 @@ func (c *Context) Validate(ctx context.Context, config *configs.Config) tfdiags.
 		RootVariableValues:      varValues,
 		Operation:               walkValidate,
 		ProviderFunctionTracker: providerFunctionTracker,
+		ImportTargets:           importTargets,
+		// Setting GenerateConfigPath is required to correctly validate cases where the users would use '-generate-config-out' during the plan phase
+		// and generate config on the fly. Otherwise, we hit false positive at https://github.com/opentofu/opentofu/blob/f8900fdc757fee3eace8c57013d411a0398369b1/internal/tofu/transform_config.go#L178
+		GenerateConfigPath: ".validate_config_path",
 	}).Build(ctx, addrs.RootModuleInstance)
 	diags = diags.Append(moreDiags)
 	if moreDiags.HasErrors() {

internal/tofu/eval_import.go

@@ -35,6 +35,10 @@ func evaluateImportIdExpression(expr hcl.Expression, evalCtx EvalContext, keyDat
 	importIdVal, evalDiags := evaluateExprWithRepetitionData(context.TODO(), evalCtx, expr, cty.String, keyData)
 	diags = diags.Append(evalDiags)
 
+	if diags.HasErrors() {
+		return "", diags
+	}
+
 	if importIdVal.IsNull() {
 		return "", diags.Append(&hcl.Diagnostic{
 			Severity: hcl.DiagError,

internal/tofu/node_resource_validate.go

@@ -93,6 +93,9 @@ func (n *NodeValidatableResource) Execute(ctx context.Context, evalCtx EvalConte
 			}
 		}
 	}
+	importDiags := n.validateImportIDs(ctx, evalCtx)
+	diags = diags.Append(importDiags)
+
 	return diags
 }
 
@@ -351,6 +354,19 @@ func (n *NodeValidatableResource) validateResource(ctx context.Context, evalCtx
 	return diags
 }
 
+func (n *NodeValidatableResource) validateImportIDs(ctx context.Context, evalCtx EvalContext) tfdiags.Diagnostics {
+	importResolver := evalCtx.ImportResolver()
+	var diags tfdiags.Diagnostics
+	for _, importTarget := range n.importTargets {
+		err := importResolver.ValidateImportIDs(ctx, importTarget, evalCtx)
+		if err != nil {
+			diags = diags.Append(err)
+		}
+	}
+	return diags
+
+}
+
 func (n *NodeValidatableResource) evaluateExpr(ctx context.Context, evalCtx EvalContext, expr hcl.Expression, wantTy cty.Type, self addrs.Referenceable, keyData instances.RepetitionData) (cty.Value, tfdiags.Diagnostics) {
 	var diags tfdiags.Diagnostics