Possible bug with implicit moved blocks?

stuart-c · January 7, 2022, 12:22pm

An interesting one @apparentlymart we’ve just seen (latest Terraform version), which I’m not sure if it is a bug or just doing something wrong:

We have a module that recently had a change to add some optional resources (by adding count = var.xxx ? 1 : 0. This is a module stored in a separate git repo.

That module is then used within another module (another repo) and finally within a root module (third repo).

Running plan then gave us a set of errors about moved blocks not being allowed cross-repo. There are no explicit moved blocks, so I think it is the implicit ones that Terraform creates for the adding/removing count use case. However I’m not sure why it is giving the error? By any chance does it “put” the implicit moved blocks within the root module code, rather than “next to” the code which has changed?

jbardin · January 7, 2022, 6:24pm

Hi @stuart-c,

Can you try with the latest v1.1.3? There were a number of bug fixes that could relate to nested moves.

Thanks!

stuart-c · January 7, 2022, 7:46pm

This was with 1.1.3 already

jbardin · January 7, 2022, 7:52pm

Thanks. It’s obviously a problem with the implicit moves here, but no idea what exactly is causing it. If you can add a minimal example I’ll take a look, otherwise I’ll try to see if I can repro it here too.

apparentlymart · January 8, 2022, 12:32am

I would also be interested to see a reproduction case if you have one, but I can also imagine how this might occur and so we can maybe construct a reproduction case ourselves, if we can confirm this to be true:

When Terraform infers implied move blocks it currently treats them as if they were moved blocks written in the root module, because that’s then allows these implied statements to refer to resources in specific instances of child modules because not all instances of a module will necessarily need the same implied move statements, depending on the prior state.

It sounds like Terraform might be incorrectly including those implied statements when it does the cross-package move validation, and thus generating an error if there’s a module package boundary anywhere in the module path of the address.

Fortunately we do internally track whether each move statement was implied exactly so that we can make exceptions like this when needed, so if I’m right about the root cause here it should at least not be a big ordeal to fix it.

apparentlymart · January 8, 2022, 12:36am

Looking a bit deeper in the code just before I finish work for the week, it does look like there’s a missing exception for implied move statements in the validation code:

github.com

hashicorp/terraform/blob/1e9075b4fac9406c9248310114cb85deccc434ee/internal/refactoring/move_validate.go#L58

    
      
          	stmtTo := map[addrs.UniqueKey]AbsMoveEndpoint{}
          
          
	for _, stmt := range stmts {
          		// Earlier code that constructs MoveStatement values should ensure that
          		// both stmt.From and stmt.To always belong to the same statement and
          		// thus to the same module.
          		stmtMod, fromCallSteps := stmt.From.ModuleCallTraversals()
          		_, toCallSteps := stmt.To.ModuleCallTraversals()
          
          
		modCfg := rootCfg.Descendent(stmtMod)
          		if pkgAddr := callsThroughModulePackage(modCfg, fromCallSteps); pkgAddr != nil {
          			diags = diags.Append(&hcl.Diagnostic{
          				Severity: hcl.DiagError,
          				Summary:  "Cross-package move statement",
          				Detail: fmt.Sprintf(
          					"This statement declares a move from an object declared in external module package %q. Move statements can be only within a single module package.",
          					pkgAddr,
          				),
          				Subject: stmt.DeclRange.ToHCL().Ptr(),
          			})
          		}

@stuart-c would you mind opening a GitHub issue for this, so we can make sure this is visible to the whole engineering team? (Not everyone monitors this forum closely.) If you have one it’d be helpful to include a reproduction case we can test against, but I do understand that it’s often hard to produce a minimal reproduction case for a situation involving external modules. In the absence of a reproduction case, including a full exact error message would at least help show the “bones” of your situation so that we can try to craft a regression test case that matches it as closely as possible.

grimm26 · January 11, 2022, 1:41am

I hit this today, too. Updating a git::https module source that had a resource add a ternary count .

apparentlymart · January 11, 2022, 1:44am

In the interests of creating the reverse link, here’s the issue where we’re now tracking this bug:

github.com/hashicorp/terraform

Incorrect "Cross-package move statement" for move implied by changing "count" in an external module

opened 05:32PM - 10 Jan 22 UTC

dtroup-global

bug new

### Terraform Version  ``` Terraform v1.1.3 ``` ### Terraform Configuration Files  ```terraform resource "aws_cloudfront_distribution" "s3_distribution" { count = var.enabled ? 1 : 0 origin_group { origin_id = "groupS3" failover_criteria { status_codes = [403, 404, 500, 502] } member { origin_id = "primaryS3" } member { origin_id = "failoverS3" } } origin { domain_name = aws_s3_bucket.primary.bucket_regional_domain_name origin_id = "primaryS3" s3_origin_config { origin_access_identity = aws_cloudfront_origin_access_identity.default.cloudfront_access_identity_path } } origin { domain_name = aws_s3_bucket.failover.bucket_regional_domain_name origin_id = "failoverS3" s3_origin_config { origin_access_identity = aws_cloudfront_origin_access_identity.default.cloudfront_access_identity_path } } default_cache_behavior { # ... other configuration ... target_origin_id = "groupS3" } # ... other configuration ... } ``` ### Debug Output  ### Expected Behavior  Implicit move that Terraform creates for the adding/removing count ### Actual Behavior  Running plan then gave us a set of errors about moved blocks not being allowed cross-repo. ### Steps to Reproduce  1. Created objects using an older module with no count option set 2. terraform init/plan/apply 3. Update module with new count option 4. terraform init/plan ``` Error: Cross-package move statement on .terraform/modules/mymodule/main.tf line 108, in resource "aws_cloudfront_distribution" "s3_distribution": 108: count = var.enabled ? 1 : 0 This statement declares a move to an object declared in external module package "git::https://github.com/myorg/mymodule.git?ref=v0.1.0". Move statements can be only within a single module package. ``` ### Additional Context  ### References  Forum post re: logging this ticket https://discuss.hashicorp.com/t/possible-bug-with-implicit-moved-blocks/33957

vteran93 · January 14, 2022, 6:50pm

Hi, I am getting the same issue and I just saw that you merge a pr to fix it, could you tell me please when this change will be available on a release? It’s blocking me I have been asked on my job to do some upgrades on a project, but I got this error.

stuart-c · January 14, 2022, 7:43pm

You can do a manual terraform state mv in the meantime.

vteran93 · January 17, 2022, 2:37pm

Thank you for your recommendation Stuart. I am going to check it, the issue that is causing me curiosity is where I am going to move it? because my tree and the path in the module are the same, I saw other comments saying the same but it’s not clear to me.

stuart-c · January 18, 2022, 11:51am

So for example you might have changed a resource to add a count, which renames the resource to add [xxx] to the name. So in the case where you are adding a count to change a resource to only be conditionally created you might have

module.xxx.aws_resource.name → module.xxx.aws_resource.name[0]

Terraform should altomatically handle that renaming, but the bug is preventing that. So to manually fix you would run terraform state mv module.xxx.aws_resource.name module.xxx.aws_resource.name[0]

tspearconquest · January 22, 2022, 6:46am

Thanks for this @stuart-c! Your last comment solved the issue I was having after adding a count argument and ternary.