hashicorp / terraform

Terraform enables you to safely and predictably create, change, and improve infrastructure. It is a source-available tool that codifies APIs into declarative configuration files that can be shared amongst team members, treated as code, edited, reviewed, and versioned.
https://www.terraform.io/
Other
42.44k stars 9.51k forks source link

Missing validation for incorrectly quoted moved addresses #34041

Open rbtcollins opened 11 months ago

rbtcollins commented 11 months ago

Terraform Version

1.5.3

Terraform Configuration Files

I can't tell whats relevant, and our configuration is uhm, sizeable. Also internal.

Debug Output

.

Expected Behavior

Terraform should not have crashed.

Actual Behavior

running "/atlantis/bin/terraform1.5.3 init -input=false -upgrade" in "/atlantis/repos/cognitedata/terraform/8272/default/cdf/greenfield": exit status 11

!!!!!!!!!!!!!!!!!!!!!!!!!!! TERRAFORM CRASH !!!!!!!!!!!!!!!!!!!!!!!!!!!!

Terraform crashed! This is always indicative of a bug within Terraform.
Please report the crash with Terraform[1] so that we can fix this.

When reporting bugs, please include your terraform version, the stack trace
shown below, and any additional information which may help replicate the issue.

[1]: https://github.com/hashicorp/terraform/issues

!!!!!!!!!!!!!!!!!!!!!!!!!!! TERRAFORM CRASH !!!!!!!!!!!!!!!!!!!!!!!!!!!!

runtime error: invalid memory address or nil pointer dereference
goroutine 1 [running]:
runtime/debug.Stack()
    /opt/hostedtoolcache/go/1.20.0/x64/src/runtime/debug/stack.go:24 +0x65
runtime/debug.PrintStack()
    /opt/hostedtoolcache/go/1.20.0/x64/src/runtime/debug/stack.go:16 +0x19
github.com/hashicorp/terraform/internal/logging.PanicHandler()
    /home/runner/work/terraform/terraform/internal/logging/panic.go:58 +0x153
panic({0x242c520, 0x41dc420})
    /opt/hostedtoolcache/go/1.20.0/x64/src/runtime/panic.go:890 +0x263
github.com/hashicorp/terraform/internal/addrs.(*MoveEndpoint).String(...)
    /home/runner/work/terraform/terraform/internal/addrs/move_endpoint.go:54
github.com/hashicorp/terraform/internal/configs.(*Module).appendFile(0xc000b01d40, 0xc000164c00)
    /home/runner/work/terraform/terraform/internal/configs/module.go:443 +0x3dd8
github.com/hashicorp/terraform/internal/configs.NewModule({0xc00013e3e0, 0x4, 0x0?}, {0x0, 0x0, 0x122f616?})
    /home/runner/work/terraform/terraform/internal/configs/module.go:154 +0x475
github.com/hashicorp/terraform/internal/configs.(*Parser).LoadConfigDir(0xc000154000?, {0x2db13b8, 0x1})
    /home/runner/work/terraform/terraform/internal/configs/parser_config_dir.go:45 +0x265
github.com/hashicorp/terraform/internal/command.(*Meta).loadSingleModule(0xc000064234?, {0xc000064234?, 0xc0001541a4?})
    /home/runner/work/terraform/terraform/internal/command/meta_config.go:71 +0x9b
github.com/hashicorp/terraform/internal/command.(*InitCommand).Run(0xc000154000, {0xc0000500a0, 0x2, 0x2})
    /home/runner/work/terraform/terraform/internal/command/init.go:161 +0xe8b
github.com/mitchellh/cli.(*CLI).Run(0xc00013c280)
    /home/runner/go/pkg/mod/github.com/mitchellh/cli@v1.1.5/cli.go:262 +0x5f8
main.realMain()
    /home/runner/work/terraform/terraform/main.go:318 +0x1729
main.main()
    /home/runner/work/terraform/terraform/main.go:61 +0x19

Steps to Reproduce

terraform1.5.3 init -input=false -upgrade

Additional Context

No response

References

No response

jbardin commented 11 months ago

Hi @rbtcollins,

Thanks for filing the issue! Can you verify that the crash still happens in a current release version, to make sure it hasn't already been patched? I'm not sure what would trigger this yet, but my first guess us that there is a moved block with an invalid to or from address that somehow slips through.

Thanks!

rbtcollins commented 11 months ago
running "/atlantis/bin/terraform1.6.1 init -input=false -upgrade" in "/atlantis/repos/....": exit status 11

!!!!!!!!!!!!!!!!!!!!!!!!!!! TERRAFORM CRASH !!!!!!!!!!!!!!!!!!!!!!!!!!!!

Terraform crashed! This is always indicative of a bug within Terraform.
Please report the crash with Terraform[1] so that we can fix this.

When reporting bugs, please include your terraform version, the stack trace
shown below, and any additional information which may help replicate the issue.

[1]: https://github.com/hashicorp/terraform/issues

!!!!!!!!!!!!!!!!!!!!!!!!!!! TERRAFORM CRASH !!!!!!!!!!!!!!!!!!!!!!!!!!!!

runtime error: invalid memory address or nil pointer dereference
goroutine 1 [running]:
runtime/debug.Stack()
    /opt/hostedtoolcache/go/1.21.1/x64/src/runtime/debug/stack.go:24 +0x5e
runtime/debug.PrintStack()
    /opt/hostedtoolcache/go/1.21.1/x64/src/runtime/debug/stack.go:16 +0x13
github.com/hashicorp/terraform/internal/logging.PanicHandler()
    /home/runner/work/terraform/terraform/internal/logging/panic.go:58 +0x13b
panic({0x2cd7d40?, 0x50ab790?})
    /opt/hostedtoolcache/go/1.21.1/x64/src/runtime/panic.go:920 +0x270
github.com/hashicorp/terraform/internal/addrs.(*MoveEndpoint).String(...)
    /home/runner/work/terraform/terraform/internal/addrs/move_endpoint.go:54
github.com/hashicorp/terraform/internal/configs.(*Module).appendFile(0xc0005771e0, 0xc000574180)
    /home/runner/work/terraform/terraform/internal/configs/module.go:456 +0x3d75
github.com/hashicorp/terraform/internal/configs.NewModule({0xc0002e1f00, 0x5, 0x1?}, {0x0, 0x0, 0x1?})
    /home/runner/work/terraform/terraform/internal/configs/module.go:167 +0x4d5
github.com/hashicorp/terraform/internal/configs.NewModuleWithTests(...)
    /home/runner/work/terraform/terraform/internal/configs/module.go:100
github.com/hashicorp/terraform/internal/configs.(*Parser).LoadConfigDirWithTests(0xc000996e00?, {0x38bf420, 0x1}, {0x317b3ae?, 0x41?})
    /home/runner/work/terraform/terraform/internal/configs/parser_config_dir.go:73 +0x3a5
github.com/hashicorp/terraform/internal/command.(*Meta).loadSingleModuleWithTests(0xc000062234?, {0xc000062234?, 0xc0009a9c20?}, {0x317b3ae, 0x5})
    /home/runner/work/terraform/terraform/internal/command/meta_config.go:108 +0xac
github.com/hashicorp/terraform/internal/command.(*InitCommand).Run(0xc000996e00, {0xc0000500a0, 0x2, 0x2})
    /home/runner/work/terraform/terraform/internal/command/init.go:176 +0x125a
github.com/mitchellh/cli.(*CLI).Run(0xc0002fc3c0)
    /home/runner/go/pkg/mod/github.com/mitchellh/cli@v1.1.5/cli.go:262 +0x5b8
main.realMain()
    /home/runner/work/terraform/terraform/main.go:339 +0x1eab
main.main()
    /home/runner/work/terraform/terraform/main.go:64 +0x13
rbtcollins commented 11 months ago

Yes, I have a move block. I'm refactoring a configuration from:

root module +- childone +- childtwo

to root module

in order to remove highly duplicated code in our fleet - we have many root modules that are 99% identical code (providers, marshalling outputs of childone to childtwo).

moved {
  from = "module.childone"
  to   = "module.unify.module.childone[0]"
}
moved {
  from = "module.childtwo"
  to   = "module.unify.module.childtwo"
}
rbtcollins commented 11 months ago

Just finding my way at the moment,

=> 421:         m.Moved = append(m.Moved, file.Moved...)
   422:
   423:         for _, i := range file.Import {
   424:                 for _, mi := range m.Import {
   425:                         if i.To.Equal(mi.To) {
   426:                                 diags = append(diags, &hcl.Diagnostic{
(dlv) p file.Moved
[]*github.com/hashicorp/terraform/internal/configs.Moved len: 2, cap: 2, [
        *{
                From: *github.com/hashicorp/terraform/internal/addrs.MoveEndpoint nil,
                To: *github.com/hashicorp/terraform/internal/addrs.MoveEndpoint nil,
                DeclRange: (*"github.com/hashicorp/hcl/v2.Range")(0xc0000c3230),},
        *{
                From: *github.com/hashicorp/terraform/internal/addrs.MoveEndpoint nil,
                To: *github.com/hashicorp/terraform/internal/addrs.MoveEndpoint nil,
                DeclRange: (*"github.com/hashicorp/hcl/v2.Range")(0xc0000c3280),},
]

This seems very wrong - it is as far as I can tell the right file - it has the module invocation in it; the code looks like that in my previous comment, but has endpoint values of nil

rbtcollins commented 11 months ago

The problem was the quotation marks around the resource addresses. Removing them prevented the crash. I can't offer a patch - nothing ideological, just the overheads of a CLA for a one-off don't add up; I would suggest that both the parser extracting addresses and the appendFile method that is consuming the slice of Moved's should be validating the datastructure's basic properies. E.g. pre conditions or other such assertions (since golang doesn't permit encoding this in the type system).

jbardin commented 11 months ago

No worries @rbtcollins, thanks for adding the configuration example!

kmoe commented 11 months ago

The panic occurs when there is an import block and a moved block without valid to and from addresses in the same module.

Repro:

moved {
  to = null_resource.foo
  from = "null_resource.bar"
}

import {}

Minimal repro:

moved {}
import {}

The cross-validation of the import block and moved block args happens whether or not there were errors parsing the args, hence panic.