Fix panic from a null resources module in a state file #8120

jbardin · 2016-08-10T19:57:03Z

https://gist.github.com/davidneudorfer/39bfdd1854384e57537fc0a5e3a4fc65

Make sure all maps and slices are initialized when reading a state file to prevent nil panics like above.

If we read a state file with "null" objects in a module and they become
initialized to an empty map the state file may be written out with empty
objects rather than "null", changing the checksum. If we can detect
this, increment the serial number to prevent a conflict in atlas.

Our fakeAtlas test server now needs to decode the state directly rather
than using the ReadState function, so as to be able to read the state
unaltered.

The terraform.State data structures have initialization spread out
throughout the package. More thoroughly initialize State during
ReadState, and add a call to init() during WriteState as another
normalization safeguard.

Expose State.init through an exported Init() method, so that a new State
can be completely realized outside of the terraform package.
Additionally, the internal init now completely walks all internal state
structures ensuring that all maps and slices are initialized. While it
was mentioned before that the init() methods are problematic with too
many call sites, expanding this out better exposes the entry points that
will need to be refactored later for improved concurrency handling.

The State structures had a mix of omitempty fields. Remove omitempty
for all maps and slices as part of this normalization process. Make
Lineage mandatory, which is now explicitly set in some tests.

jbardin · 2016-08-12T15:05:57Z

I ended up squashing all the commits down here, since the there were too many change done and undone by the series of commits. This commit is a little large, but much easier to understand in isolation.

Fix checksum issue with remote state If we read a state file with "null" objects in a module and they become initialized to an empty map the state file may be written out with empty objects rather than "null", changing the checksum. If we can detect this, increment the serial number to prevent a conflict in atlas. Our fakeAtlas test server now needs to decode the state directly rather than using the ReadState function, so as to be able to read the state unaltered. The terraform.State data structures have initialization spread out throughout the package. More thoroughly initialize State during ReadState, and add a call to init() during WriteState as another normalization safeguard. Expose State.init through an exported Init() method, so that a new State can be completely realized outside of the terraform package. Additionally, the internal init now completely walks all internal state structures ensuring that all maps and slices are initialized. While it was mentioned before that the `init()` methods are problematic with too many call sites, expanding this out better exposes the entry points that will need to be refactored later for improved concurrency handling. The State structures had a mix of `omitempty` fields. Remove omitempty for all maps and slices as part of this normalization process. Make Lineage mandatory, which is now explicitly set in some tests.

jbardin · 2016-08-12T15:54:18Z

cc @phinze and/or @jen20 for review

phinze · 2016-08-12T18:16:25Z

terraform/state.go

@@ -713,7 +752,7 @@ type ModuleState struct {
 	// Terraform. If Terraform doesn't find a matching ID in the
 	// overall state, then it assumes it isn't managed and doesn't
 	// worry about it.
-	Dependencies []string `json:"depends_on,omitempty"`
+	Dependencies []string `json:"depends_on"`


The normalization-serial-bump below will also cover these changes as well, yes?

yes, since every ReadState checks WriteState for changes, we'll always bump the serial on a change.

phinze · 2016-08-12T18:21:19Z

Looking good on first pass!

Trying to structure a "rule of thumb" going forward: never create a terraform.State, ResourceState, or InstanceState without Init(). Does that sound right?

jbardin · 2016-08-12T18:34:54Z

I don't want to commit yet to a global pattern for handling these, as I'd rather not have this api cruft leak out into provider-land.

Right now it should be the case that init() is called everywhere it matters, and you only need to call it manually for changes within the terraform package or tests that build their own state. I was going to do further review when I get back, as the init() calls all over place are part of the concurrency issues we have, and need to be cleaned up. It's going to take a little planning to not have this affect providers.

phinze · 2016-08-12T18:42:47Z

Cool that all makes sense. This LGTM, but let's also get @jen20 to do a triple check here before merge.

phinze · 2016-08-15T19:26:55Z

cc @jen20 or @mitchellh for double check review then I'll merge it 👍

mitchellh · 2016-08-15T19:44:09Z

LGTM.

One thing we should probably look into in the future is exposing test helpers via testing.go or testing_state.go (our convention is just prefixing the file with testing) for public APIs to get variously formed state files that can be used in and out of the core package that handles this in various ways.

I see there are a lot of ways that that wouldn't have helped but at least for "I need a ready to go state" those cases should ideally be centralized in a single function.

This PR LGTM though!

Ninir · 2016-08-16T09:59:46Z

command/refresh_test.go

@@ -742,6 +743,20 @@ func TestRefresh_displaysOutputs(t *testing.T) {
 	}
 }

+// When creating an InstaneState for direct comparison to one contained in
+// terraform.State, all fields must be inintialized (duplicating the


Small typo here: initialized, awesome work otherwise! :-)

Good catch! Will fix on master.

ghost · 2020-04-23T02:12:59Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

jbardin added the core label Aug 10, 2016

jbardin changed the title ~~Fix panic from a null resources module in a state file~~ WIP: Fix panic from a null resources module in a state file Aug 10, 2016

jbardin force-pushed the jbardin/null-state-module branch 4 times, most recently from af4be59 to 2e69ec5 Compare August 11, 2016 14:45

jbardin changed the title ~~WIP: Fix panic from a null resources module in a state file~~ Fix panic from a null resources module in a state file Aug 11, 2016

jbardin force-pushed the jbardin/null-state-module branch 6 times, most recently from f955f98 to 4d55eab Compare August 12, 2016 15:04

jbardin force-pushed the jbardin/null-state-module branch from 4d55eab to 9fddcb7 Compare August 12, 2016 15:08

jbardin force-pushed the jbardin/null-state-module branch from 9fddcb7 to cdb80f6 Compare August 12, 2016 15:10

phinze reviewed Aug 12, 2016
View reviewed changes

phinze self-assigned this Aug 15, 2016

Ninir reviewed Aug 16, 2016
View reviewed changes

phinze merged commit 50df583 into master Aug 16, 2016

phinze deleted the jbardin/null-state-module branch August 16, 2016 14:45

dtolnay mentioned this pull request Sep 8, 2016

core: Allow refresh of local state with no resources #7320

Merged

ghost locked and limited conversation to collaborators Apr 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix panic from a null resources module in a state file #8120

Fix panic from a null resources module in a state file #8120

jbardin commented Aug 10, 2016 •

edited

Loading

jbardin commented Aug 12, 2016

jbardin commented Aug 12, 2016

phinze Aug 12, 2016

jbardin Aug 12, 2016

phinze commented Aug 12, 2016

jbardin commented Aug 12, 2016 •

edited

Loading

phinze commented Aug 12, 2016

phinze commented Aug 15, 2016

mitchellh commented Aug 15, 2016

Ninir Aug 16, 2016

phinze Aug 16, 2016

ghost commented Apr 23, 2020

Fix panic from a null resources module in a state file #8120

Fix panic from a null resources module in a state file #8120

Conversation

jbardin commented Aug 10, 2016 • edited Loading

jbardin commented Aug 12, 2016

jbardin commented Aug 12, 2016

phinze Aug 12, 2016

Choose a reason for hiding this comment

jbardin Aug 12, 2016

Choose a reason for hiding this comment

phinze commented Aug 12, 2016

jbardin commented Aug 12, 2016 • edited Loading

phinze commented Aug 12, 2016

phinze commented Aug 15, 2016

mitchellh commented Aug 15, 2016

Ninir Aug 16, 2016

Choose a reason for hiding this comment

phinze Aug 16, 2016

Choose a reason for hiding this comment

ghost commented Apr 23, 2020

jbardin commented Aug 10, 2016 •

edited

Loading

jbardin commented Aug 12, 2016 •

edited

Loading