Finalize import validation

Remove the redundant validation from the config transformers, making
sure most of the validation is now done by NodeValidatableResource.
Inset Validate calls into import tests to ensure both that existing
errors are still found, and that existing plans don't fail the new
validation code.

internal/terraform/context_apply_deferred_test.go

@@ -2915,7 +2915,7 @@ import {
 				wantDeferred: make(map[string]ExpectedDeferred),
 				wantDiagnostic: func(diags tfdiags.Diagnostics) bool {
 					for _, diag := range diags {
-						if diag.Description().Summary == "Configuration for import target does not exist" {
+						if diag.Description().Summary == "Use of import for_each in an invalid context" {
 							return true
 						}
 					}
@@ -3236,9 +3236,16 @@ func TestContextApply_deferredActions(t *testing.T) {
 
 					var plan *plans.Plan
 					t.Run("plan", func(t *testing.T) {
-
 						var diags tfdiags.Diagnostics
-						plan, diags = ctx.Plan(cfg, state, opts)
+
+						// Validate is run by default for any plan from the CLI
+						diags = diags.Append(ctx.Validate(cfg, &ValidateOpts{}))
+						// Plan won't proceed if validate failed
+						if !diags.HasErrors() {
+							p, pDiags := ctx.Plan(cfg, state, opts)
+							diags = diags.Append(pDiags)
+							plan = p
+						}
 
 						if stage.wantDiagnostic == nil {
 							// We expect the correct planned changes and no diagnostics.
@@ -3249,7 +3256,7 @@ func TestContextApply_deferredActions(t *testing.T) {
 							}
 						} else {
 							if !stage.wantDiagnostic(diags) {
-								t.Fatalf("missing expected diagnostics: %s", diags)
+								t.Fatalf("missing expected diagnostics: %s", diags.ErrWithWarnings())
 							} else {
 								// We don't want to make any further assertions in this case.
 								// If diagnostics are expected it's valid that no plan may be returned.

internal/terraform/context_plan_import_test.go

@@ -1093,14 +1093,6 @@ import {
 	}
 
 	diags := ctx.Validate(m, &ValidateOpts{})
-	if diags.HasErrors() {
-		t.Fatalf("unexpected errors\n%s", diags.Err().Error())
-	}
-
-	_, diags = ctx.Plan(m, states.NewState(), &PlanOpts{
-		Mode:               plans.NormalMode,
-		GenerateConfigPath: "generated.tf",
-	})
 	if !diags.HasErrors() {
 		t.Fatalf("expected plan to error, but it did not")
 	}
@@ -1597,20 +1589,8 @@ import {
 	})
 
 	diags := ctx.Validate(m, &ValidateOpts{})
-	if diags.HasErrors() {
-		t.Fatalf("unexpected errors\n%s", diags.Err().Error())
-	}
-
-	plan, diags := ctx.Plan(m, states.NewState(), &PlanOpts{
-		Mode:               plans.NormalMode,
-		GenerateConfigPath: "generated.tf",
-	})
-	if diags.HasErrors() {
-		t.Fatalf("unexpected errors\n%s", diags.Err().Error())
-	}
-
-	if len(plan.Changes.Resources) != 0 {
-		t.Fatal("expected no resource changes")
+	if !diags.HasErrors() {
+		t.Fatal("expected errors, got none")
 	}
 }
 

internal/terraform/node_resource_plan.go

@@ -282,6 +282,20 @@ func (n *nodeExpandPlannableResource) validateExpandedImportTargets(expandedImpo
 			))
 			return diags
 		}
+
+		if n.Config == nil && n.generateConfigPath == "" && len(n.importTargets) == 1 {
+			// we have no config and aren't generating any. This isn't caught during
+			// validation because generateConfigPath is only a plan option. If we
+			// got this far however, it means this node is eligible for config
+			// generation, so suggest it to the user.
+			diags = diags.Append(&hcl.Diagnostic{
+				Severity: hcl.DiagError,
+				Summary:  "Configuration for import target does not exist",
+				Detail:   fmt.Sprintf("The configuration for the given import target %s does not exist. If you wish to automatically generate config for this resource, use the -generate-config-out option within terraform plan. Otherwise, make sure the target resource exists within your configuration. For example:\n\n  terraform plan -generate-config-out=generated.tf", n.Addr),
+				Subject:  n.importTargets[0].Config.To.Range().Ptr(),
+			})
+			return diags
+		}
 	}
 
 	return diags

internal/terraform/node_resource_validate.go

@@ -46,6 +46,9 @@ func (n *NodeValidatableResource) Path() addrs.ModuleInstance {
 
 // GraphNodeEvalable
 func (n *NodeValidatableResource) Execute(ctx EvalContext, op walkOperation) (diags tfdiags.Diagnostics) {
+	// this is done first since there may not be config if we are generating it
+	diags = diags.Append(n.validateImportTargets(ctx))
+
 	if n.Config == nil {
 		return diags
 	}
@@ -54,8 +57,6 @@ func (n *NodeValidatableResource) Execute(ctx EvalContext, op walkOperation) (di
 
 	diags = diags.Append(n.validateCheckRules(ctx, n.Config))
 
-	diags = diags.Append(n.validateImportTargets(ctx))
-
 	if managed := n.Config.Managed; managed != nil {
 		// Validate all the provisioners
 		for _, p := range managed.Provisioners {
@@ -527,6 +528,8 @@ func (n *NodeValidatableResource) validateImportTargets(ctx EvalContext) tfdiags
 		return diags
 	}
 
+	diags = diags.Append(n.validateConfigGen(ctx))
+
 	// Import blocks are only valid within the root module, and must be
 	// evaluated within that context
 	ctx = evalContextForModuleInstance(ctx, addrs.RootModuleInstance)
@@ -543,8 +546,11 @@ func (n *NodeValidatableResource) validateImportTargets(ctx EvalContext) tfdiags
 			return diags
 		}
 
+		// Resource config might be nil here since we are also validating config generation.
+		expanded := n.Config != nil && (n.Config.ForEach != nil || n.Config.Count != nil)
+
 		if imp.Config.ForEach != nil {
-			if n.Config.ForEach == nil && n.Config.Count == nil {
+			if !expanded {
 				diags = diags.Append(&hcl.Diagnostic{
 					Severity: hcl.DiagError,
 					Summary:  "Use of import for_each in an invalid context",
@@ -605,9 +611,12 @@ func (n *NodeValidatableResource) validateImportTargets(ctx EvalContext) tfdiags
 func (n *NodeValidatableResource) validateImportTargetExpansion(to addrs.AbsResourceInstance, toExpr hcl.Expression) tfdiags.Diagnostics {
 	var diags tfdiags.Diagnostics
 
+	forEach := n.Config != nil && n.Config.ForEach != nil
+	count := n.Config != nil && n.Config.Count != nil
+
 	switch to.Resource.Key.(type) {
 	case addrs.StringKey:
-		if n.Config.ForEach == nil {
+		if !forEach {
 			diags = diags.Append(&hcl.Diagnostic{
 				Severity: hcl.DiagError,
 				Summary:  "Invalid import 'to' expression",
@@ -616,7 +625,7 @@ func (n *NodeValidatableResource) validateImportTargetExpansion(to addrs.AbsReso
 			})
 		}
 	case addrs.IntKey:
-		if n.Config.Count == nil {
+		if !count {
 			diags = diags.Append(&hcl.Diagnostic{
 				Severity: hcl.DiagError,
 				Summary:  "Invalid import 'to' expression",
@@ -625,20 +634,20 @@ func (n *NodeValidatableResource) validateImportTargetExpansion(to addrs.AbsReso
 			})
 		}
 	default:
-		if n.Config.Count != nil {
+		if forEach {
 			diags = diags.Append(&hcl.Diagnostic{
 				Severity: hcl.DiagError,
 				Summary:  "Invalid import 'to' expression",
-				Detail:   fmt.Sprintf("The target resource is using count."),
+				Detail:   fmt.Sprintf("The target resource is using for_each."),
 				Subject:  toExpr.Range().Ptr(),
 			})
 		}
 
-		if n.Config.ForEach != nil {
+		if count {
 			diags = diags.Append(&hcl.Diagnostic{
 				Severity: hcl.DiagError,
 				Summary:  "Invalid import 'to' expression",
-				Detail:   fmt.Sprintf("The target resource is using for_each."),
+				Detail:   fmt.Sprintf("The target resource is using count."),
 				Subject:  toExpr.Range().Ptr(),
 			})
 		}
@@ -647,6 +656,51 @@ func (n *NodeValidatableResource) validateImportTargetExpansion(to addrs.AbsReso
 	return diags
 }
 
+// validate imports with no config for possible config generation
+func (n *NodeValidatableResource) validateConfigGen(ctx EvalContext) tfdiags.Diagnostics {
+	var diags tfdiags.Diagnostics
+
+	if n.Config != nil {
+		return diags
+	}
+
+	// We won't have the config generation output path during validate, so only
+	// check if generation is at all possible.
+
+	for _, imp := range n.importTargets {
+		if !n.Addr.Module.IsRoot() {
+			diags = diags.Append(&hcl.Diagnostic{
+				Severity: hcl.DiagError,
+				Summary:  "Configuration for import target does not exist",
+				Detail:   fmt.Sprintf("Resource %s not found. Only resources within the root module are eligible for config generation.", n.Addr),
+				Subject:  imp.Config.To.Range().Ptr(),
+			})
+			continue
+		}
+
+		var toDiags tfdiags.Diagnostics
+		traversal, hd := hcl.AbsTraversalForExpr(imp.Config.To)
+		toDiags = toDiags.Append(hd)
+		to, td := addrs.ParseAbsResourceInstance(traversal)
+		toDiags = toDiags.Append(td)
+
+		if toDiags.HasErrors() {
+			// these will be caught elsewhere with better context
+			continue
+		}
+
+		if to.Resource.Key != addrs.NoKey || imp.Config.ForEach != nil {
+			diags = diags.Append(&hcl.Diagnostic{
+				Severity: hcl.DiagError,
+				Summary:  "Configuration for import target does not exist",
+				Detail:   "The given import block is not compatible with config generation. The -generate-config-out option cannot be used with import blocks which use for_each, or resources which use for_each or count.",
+				Subject:  imp.Config.To.Range().Ptr(),
+			})
+		}
+	}
+	return diags
+}
+
 func validateCount(ctx EvalContext, expr hcl.Expression) (diags tfdiags.Diagnostics) {
 	val, countDiags := evaluateCountExpressionValue(expr, ctx)
 	// If the value isn't known then that's the best we can do for now, but

internal/terraform/transform_attach_config_resource.go

@@ -48,6 +48,7 @@ func (t *AttachResourceConfigTransformer) Transform(g *Graph) error {
 
 		// Get the configuration.
 		config := t.Config.Descendent(addr.Module)
+
 		if config == nil {
 			log.Printf("[TRACE] AttachResourceConfigTransformer: %q (%T) has no configuration available", dag.VertexName(v), v)
 			continue

internal/terraform/transform_config.go

@@ -62,12 +62,8 @@ func (t *ConfigTransformer) Transform(g *Graph) error {
 		return nil
 	}
 
-	if t.op != walkValidate {
-		// We can't generate config during validate, so we need to accept
-		// missing targets.
-		if err := t.validateImportTargets(); err != nil {
-			return err
-		}
+	if err := t.validateImportTargets(); err != nil {
+		return err
 	}
 
 	// Start the transformation process
@@ -179,54 +175,31 @@ func (t *ConfigTransformer) transformSingle(g *Graph, config *configs.Config) er
 		g.Add(node)
 	}
 
-	// If any import targets were not claimed by resources and we are
-	// generating configuration, then let's add them into the graph now.
+	// If any import targets were not claimed by resources we may be
+	// generating configuration. Add them to the graph for validation.
 	for _, i := range importTargets {
-		if path.IsRoot() {
-			// If we have a single instance import target in the root module, we
-			// can suggest config generation.
-			// We do need to make sure there are no dynamic expressions here
-			// and we can parse this at all.
-			var toDiags tfdiags.Diagnostics
-			traversal, hd := hcl.AbsTraversalForExpr(i.Config.To)
-			toDiags = toDiags.Append(hd)
-			to, td := addrs.ParseAbsResourceInstance(traversal)
-			toDiags = toDiags.Append(td)
-
-			// Check if we can generate config for this target. During
-			// validation we won't have a valid config path, but we should still
-			// add the Node since it will only be used for further validation.
-			canGenerate := !toDiags.HasErrors() && to.Resource.Key == addrs.NoKey &&
-				(t.op == walkValidate || t.generateConfigPathForImportTargets != "")
-
-			if canGenerate {
-				log.Printf("[DEBUG] ConfigTransformer: adding config generation node for %s", i.Config.ToResource)
-
-				// TODO: if config generation is ever supported for for_each
-				// resources, this will add multiple nodes for the same
-				// resource
-				abstract := &NodeAbstractResource{
-					Addr:               i.Config.ToResource,
-					importTargets:      []*ImportTarget{i},
-					generateConfigPath: t.generateConfigPathForImportTargets,
-				}
-				var node dag.Vertex = abstract
-				if f := t.Concrete; f != nil {
-					node = f(abstract)
-				}
-
-				g.Add(node)
-				continue
-			}
+		log.Printf("[DEBUG] ConfigTransformer: adding config generation node for %s", i.Config.ToResource)
+
+		// TODO: if config generation is ever supported for for_each
+		// resources, this will add multiple nodes for the same
+		// resource
+		abstract := &NodeAbstractResource{
+			Addr:               i.Config.ToResource,
+			importTargets:      []*ImportTarget{i},
+			generateConfigPath: t.generateConfigPathForImportTargets,
+		}
+		var node dag.Vertex = abstract
+		if f := t.Concrete; f != nil {
+			node = f(abstract)
 		}
+
+		g.Add(node)
 	}
 	return nil
 }
 
-// validateImportTargets ensures that the import target resources exist in the
-// configuration. We do this here rather than statically during config loading
-// to have any CLI imports included, and to provide feedback about possible
-// config generation.
+// validateImportTargets ensures that the import target module exists in the
+// configuration. Individual resources will be check by the validation node.
 func (t *ConfigTransformer) validateImportTargets() error {
 	var diags tfdiags.Diagnostics
 
@@ -240,53 +213,14 @@ func (t *ConfigTransformer) validateImportTargets() error {
 		}
 
 		moduleCfg := t.Config.Root.Descendent(toResource.Module)
-		if moduleCfg != nil {
-			res := moduleCfg.Module.ResourceByAddr(toResource.Resource)
-			if res != nil {
-				// the target config exists, which is all we're looking for at this point.
-				continue
-			}
+		if moduleCfg == nil {
+			diags = diags.Append(&hcl.Diagnostic{
+				Severity: hcl.DiagError,
+				Summary:  "Configuration for import target does not exist",
+				Detail:   fmt.Sprintf("The configuration for the given import target %s does not exist. All target instances must have an associated configuration to be imported.", i.Config.ToResource),
+				Subject:  i.Config.To.Range().Ptr(),
+			})
 		}
-
-		if toResource.Module.IsRoot() {
-			var toDiags tfdiags.Diagnostics
-			traversal, hd := hcl.AbsTraversalForExpr(i.Config.To)
-			toDiags = toDiags.Append(hd)
-			to, td := addrs.ParseAbsResourceInstance(traversal)
-			toDiags = toDiags.Append(td)
-			canGenerate := !toDiags.HasErrors() && to.Resource.Key == addrs.NoKey
-
-			if t.generateConfigPathForImportTargets != "" {
-				if canGenerate {
-					continue
-				}
-
-				diags = diags.Append(&hcl.Diagnostic{
-					Severity: hcl.DiagError,
-					Summary:  "Cannot generate configuration",
-					Detail:   "The given import block is not compatible with config generation. The -generate-config-out option cannot be used with import blocks which use for_each, or resources which use for_each or count.",
-					Subject:  i.Config.To.Range().Ptr(),
-				})
-				continue
-			}
-
-			if canGenerate {
-				diags = diags.Append(&hcl.Diagnostic{
-					Severity: hcl.DiagError,
-					Summary:  "Configuration for import target does not exist",
-					Detail:   fmt.Sprintf("The configuration for the given import target %s does not exist. If you wish to automatically generate config for this resource, use the -generate-config-out option within terraform plan. Otherwise, make sure the target resource exists within your configuration. For example:\n\n  terraform plan -generate-config-out=generated.tf", to),
-					Subject:  i.Config.To.Range().Ptr(),
-				})
-				continue
-			}
-		}
-
-		diags = diags.Append(&hcl.Diagnostic{
-			Severity: hcl.DiagError,
-			Summary:  "Configuration for import target does not exist",
-			Detail:   fmt.Sprintf("The configuration for the given import target %s does not exist. All target instances must have an associated configuration to be imported.", i.Config.ToResource),
-			Subject:  i.Config.To.Range().Ptr(),
-		})
 	}
 
 	return diags.Err()