feat(import): support import blocks inside child modules by mildwonkey · Pull Request #38352 · hashicorp/terraform

mildwonkey · 2026-04-07T17:07:49Z

This PR adds support for import blocks inside child modules. Duplicate blocks will overwrite prior imports (I copied this behavior from the refactoring code, but we could instead return an error if multiple import blocks target the same abs config resource).

I believe I found a bug in import when the resource is deferred and we are generating config - I've added a comment to the line in the file. There was one check that would fail if there was no config - but it wasn't checking if that was because config was being generated. (does this commit also need a changelog entry for the bug? I need to add one for the feature)

Note for reviewers - the changes in node_resource_plan look very dramatic in the default view. If you haven't already, I recommend switching to "Hide whitespace" to review this PR, or at least that file.

Fixes #33474

Target Release

1.16.x

Rollback Plan

If a change needs to be reverted, we will roll out an update to the code within 7 days.

Changes to Security Controls

Are there any changes to security controls (access controls, encryption, logging) in this pull request? If so, explain.

CHANGELOG entry

This change is user-facing and I added a changelog entry.
This change is not user-facing.

mildwonkey · 2026-04-10T12:52:37Z


 	// If we're importing and generating config, generate it now.
-	if n.Config == nil {
+	if n.Config == nil && n.generateConfigPath == "" {


I believe this fixes a (theoretical) bug - if we're generating config, config will be nil, and that's ok at this point.

mildwonkey · 2026-04-10T13:05:12Z

+// Copyright IBM Corp. 2014, 2026
+// SPDX-License-Identifier: BUSL-1.1
+
+package refactoring


I'm not super sold on putting this file in refactoring, but this package was the inspiration for it so 🤷🏻 happy to move it or nah.

yeah 🤔, I don't think prior to this PR that I would have thought of import as a "refactoring" operation but it doesn't seem too far off in concept I guess. The only other location I can think that would make sense is just closer to the caller/tests for this logic back in internal/terraform 😆

austinvalle

Very cool! 🎸 I mostly just left questions and a bug I ran into when trying the PR out 👍🏻

austinvalle · 2026-04-17T18:42:52Z

+// Copyright IBM Corp. 2014, 2026
+// SPDX-License-Identifier: BUSL-1.1
+
+package refactoring


yeah 🤔, I don't think prior to this PR that I would have thought of import as a "refactoring" operation but it doesn't seem too far off in concept I guess. The only other location I can think that would make sense is just closer to the caller/tests for this logic back in internal/terraform 😆

austinvalle · 2026-04-17T19:25:58Z

+			// get the context from the module the import was defined in
+			ctx = evalContextForModuleInstance(ctx, mod)


Maybe already known, but there are other areas I think we'll need to update to get the right context for evaluating. I ran into this one when testing (when using an input variable on a child module, it evaluated with the root module context and raised an error):

terraform/internal/terraform/node_resource_validate.go

Lines 658 to 660 in 97265cf

// Import blocks are only valid within the root module, and must be

// evaluated within that context

ctx = evalContextForModuleInstance(ctx, addrs.RootModuleInstance)

this was "known" as in "i knew i was missing something important" 😭 thank you, i'll get more tests and get to work here!

I added the Validate() call to my tests and got some errors 🎉

We don't have full module instance context during validate - no expansion yet - so for now I just noped out of validating import statements that weren't in the root module. We make similiar choices elsewhere, but I do want to poke around a bit and see what I can do to improve that situation

austinvalle · 2026-04-17T19:42:08Z

+		// If we already have an import statement for this ConfigResource, it
+		// must have come from a parent module, because duplicate import
+		// blocks in the same module result in an error.
+		// The import block in the parent module overrides the block in the
+		// child module.


It's not immediately clear to me what the use-case for overriding an import block inside a module would be, maybe just a situation where a module author doesn't provide a proper way to influence the import command? 🤔 (with like a variable for example)

Although, I'm also not well versed in why the existing moved block allows that either 🙂

Agreed; this was weird so I copied the moved behavior but I could also return an error if we get this.

Though ... never mind overriding an import block, to be honest I am not very comfortable with this feature at all. the only use-cases I can actually come up with are solved by data sources, not importing resources into your state from nested modules. I can't think of many instances where I'd want to blanket import something in to every instance of a module that's being used multiple places, even back when I was managing relatively well contained modules and environments. If I owned everything, I could write the import statement in the root module, and if I don't own everything ... I don't want imports I didn't write. But I also came from a fairly specific way of working so I tried to let all that go.

.... though now that I'm thinking about it, wearing my infra hat, I'd prefer want the ability to control this behavior (ie, block nested-module imports/fail/etc) from the root module (as a security stance), but I guess a sentinel policy could cover that.

austinvalle · 2026-04-17T19:57:50Z

-	if len(mod.Import) > 0 {
-		diags = diags.Append(&hcl.Diagnostic{
-			Severity: hcl.DiagError,
-			Summary:  "Invalid import configuration",
-			Detail:   fmt.Sprintf("An import block was detected in %q. Import blocks are only allowed in the root module.", cfg.Path),
-			Subject:  mod.Import[0].DeclRange.Ptr(),
-		})
-	}


Are we changing any user assumptions by allowing modules to now use import blocks by default? (and if so, would there be value in only allowing local modules initially?)

Yep, and I think it could be a security concern (see my above comment). On some level I didn't think of it because I wouldn't use terraform modules I didn't write or code review myself 🤔

…dation needs to wait

mildwonkey force-pushed the mildwonkey/import-in-module branch from 6934539 to a9fa5e3 Compare April 7, 2026 18:44

mildwonkey commented Apr 10, 2026

View reviewed changes

mildwonkey force-pushed the mildwonkey/import-in-module branch from 21cc434 to 9b1473f Compare April 10, 2026 12:55

mildwonkey marked this pull request as ready for review April 10, 2026 13:03

mildwonkey requested a review from a team as a code owner April 10, 2026 13:03

mildwonkey commented Apr 10, 2026

View reviewed changes

jbardin reviewed Apr 13, 2026

View reviewed changes

Comment thread internal/refactoring/import_statement.go Outdated

mildwonkey force-pushed the mildwonkey/import-in-module branch 2 times, most recently from 5def6e8 to 1f45cc1 Compare April 14, 2026 13:16

mildwonkey requested a review from jbardin April 14, 2026 13:17

austinvalle reviewed Apr 17, 2026

View reviewed changes

mildwonkey added 7 commits April 22, 2026 08:07

feat(import): support import blocks inside child modules

044a9ad

copyrightfix

58c07f7

figure out a deferrel related test and maybe found a bug?

2ec7f39

changelog

9d74c30

use the correct context when evaluating imports

e085a3c

typo

a8010ba

we don't have module instances during validate, so nested import vali…

de3cfda

…dation needs to wait

mildwonkey force-pushed the mildwonkey/import-in-module branch from 1f45cc1 to de3cfda Compare April 22, 2026 12:39

		// get the context from the module the import was defined in
		ctx = evalContextForModuleInstance(ctx, mod)

	// Import blocks are only valid within the root module, and must be
	// evaluated within that context
	ctx = evalContextForModuleInstance(ctx, addrs.RootModuleInstance)

Conversation

mildwonkey commented Apr 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Target Release

Rollback Plan

Changes to Security Controls

CHANGELOG entry

Uh oh!

mildwonkey Apr 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

austinvalle left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mildwonkey commented Apr 7, 2026 •

edited

Loading

mildwonkey Apr 10, 2026 •

edited

Loading