diff --git a/command/meta_backend_test.go b/command/meta_backend_test.go index 2adfbabb6..15995de8f 100644 --- a/command/meta_backend_test.go +++ b/command/meta_backend_test.go @@ -238,7 +238,6 @@ func TestMetaBackend_configureInterpolation(t *testing.T) { // Newly configured backend func TestMetaBackend_configureNew(t *testing.T) { - // Create a temporary working directory that is empty td := tempDir(t) copy.CopyDir(testFixturePath("backend-new"), td) defer os.RemoveAll(td) @@ -1532,7 +1531,6 @@ func TestMetaBackend_planLocal(t *testing.T) { // A plan with a custom state save path func TestMetaBackend_planLocalStatePath(t *testing.T) { - // Create a temporary working directory that is empty td := tempDir(t) copy.CopyDir(testFixturePath("backend-plan-local"), td) defer os.RemoveAll(td) diff --git a/states/statemgr/filesystem.go b/states/statemgr/filesystem.go index 47ea17b25..3a8e79110 100644 --- a/states/statemgr/filesystem.go +++ b/states/statemgr/filesystem.go @@ -56,7 +56,6 @@ type Filesystem struct { file *statefile.File readFile *statefile.File backupFile *statefile.File - written bool writtenBackup bool } @@ -115,9 +114,6 @@ func (s *Filesystem) State() *states.State { if s.file == nil { return nil } - if s.backupPath != "" && s.backupFile == nil { - s.backupFile = s.file.DeepCopy() - } return s.file.DeepCopy().State } @@ -128,36 +124,19 @@ func (s *Filesystem) WriteState(state *states.State) error { // writing to a temp file on the same filesystem, and renaming the file over // the original. + defer s.mutex()() + if s.readFile == nil { - err := s.RefreshState() + err := s.refreshState() if err != nil { return err } } - defer s.mutex()() return s.writeState(state, nil) } func (s *Filesystem) writeState(state *states.State, meta *SnapshotMeta) error { - // We'll try to write our backup first, so we can be sure we've created - // it successfully before clobbering the original file it came from. - if !s.writtenBackup && s.backupFile != nil && s.backupPath != "" && !statefile.StatesMarshalEqual(state, s.backupFile.State) { - log.Printf("[TRACE] statemgr.Filesystem: creating backup snapshot at %s", s.backupPath) - bfh, err := os.Create(s.backupPath) - if err != nil { - return fmt.Errorf("failed to create local state backup file: %s", err) - } - defer bfh.Close() - - err = statefile.Write(s.backupFile, bfh) - if err != nil { - return fmt.Errorf("failed to write to local state backup file: %s", err) - } - - s.writtenBackup = true - } - if s.stateFileOut == nil { if err := s.createStateFiles(); err != nil { return nil @@ -165,13 +144,46 @@ func (s *Filesystem) writeState(state *states.State, meta *SnapshotMeta) error { } defer s.stateFileOut.Sync() + // We'll try to write our backup first, so we can be sure we've created + // it successfully before clobbering the original file it came from. + if !s.writtenBackup && s.backupFile != nil && s.backupPath != "" { + if !statefile.StatesMarshalEqual(state, s.backupFile.State) { + log.Printf("[TRACE] statemgr.Filesystem: creating backup snapshot at %s", s.backupPath) + bfh, err := os.Create(s.backupPath) + if err != nil { + return fmt.Errorf("failed to create local state backup file: %s", err) + } + defer bfh.Close() + + err = statefile.Write(s.backupFile, bfh) + if err != nil { + return fmt.Errorf("failed to write to local state backup file: %s", err) + } + + s.writtenBackup = true + } else { + log.Print("[TRACE] statemgr.Filesystem: not making a backup, because the new snapshot is identical to the old") + } + } else { + // This branch is all just logging, to help understand why we didn't make a backup. + switch { + case s.backupPath == "": + log.Print("[TRACE] statemgr.Filesystem: state file backups are disabled") + case s.writtenBackup: + log.Printf("[TRACE] statemgr.Filesystem: have already backed up original %s to %s on a previous write", s.path, s.backupPath) + case s.backupFile == nil: + log.Printf("[TRACE] statemgr.Filesystem: no original state snapshot to back up") + default: + log.Printf("[TRACE] statemgr.Filesystem: not creating a backup for an unknown reason") + } + } + s.file = s.file.DeepCopy() if s.file == nil { s.file = NewStateFile() } s.file.State = state.DeepCopy() - log.Print("[TRACE] statemgr.Filesystem: truncating the state file") if _, err := s.stateFileOut.Seek(0, os.SEEK_SET); err != nil { return err } @@ -204,7 +216,8 @@ func (s *Filesystem) writeState(state *states.State, meta *SnapshotMeta) error { return err } - s.written = true + // Any future reads must come from the file we've now updated + s.readPath = s.path return nil } @@ -217,7 +230,10 @@ func (s *Filesystem) PersistState() error { // RefreshState is an implementation of Refresher. func (s *Filesystem) RefreshState() error { defer s.mutex()() + return s.refreshState() +} +func (s *Filesystem) refreshState() error { var reader io.Reader // The s.readPath file is only OK to read if we have not written any state out @@ -227,10 +243,9 @@ func (s *Filesystem) RefreshState() error { // This is important for Windows, as if the input file is the same as the // output file, and the output file has been locked already, we can't open // the file again. - if !s.written && (s.stateFileOut == nil || s.readPath != s.path) { - log.Printf("[TRACE] statemgr.Filesystem: reading initial snapshot from %s", s.readPath) - + if s.stateFileOut == nil || s.readPath != s.path { // we haven't written a state file yet, so load from readPath + log.Printf("[TRACE] statemgr.Filesystem: reading initial snapshot from %s", s.readPath) f, err := os.Open(s.readPath) if err != nil { // It is okay if the file doesn't exist; we'll treat that as a nil state. @@ -247,11 +262,9 @@ func (s *Filesystem) RefreshState() error { reader = f } } else { - log.Printf("[TRACE] statemgr.Filesystem: reading snapshot from %s", s.path) - + log.Printf("[TRACE] statemgr.Filesystem: reading latest snapshot from %s", s.path) // no state to refresh if s.stateFileOut == nil { - log.Printf("[TRACE] statemgr.Filesystem: no state snapshot has been written yet") return nil } @@ -261,23 +274,20 @@ func (s *Filesystem) RefreshState() error { } f, err := statefile.Read(reader) - - // nothing to backup if there's no initial state - if f == nil { - log.Printf("[TRACE] statemgr.Filesystem: no initial state, so will skip writing a backup") - s.writtenBackup = true - } - - // if there's no state we just assign the nil return value - if err != nil && err != statefile.ErrNoState { - log.Printf("[TRACE] statemgr.Filesystem: state snapshot is nil") - return err + // if there's no state then a nil file is fine + if err != nil { + if err != statefile.ErrNoState { + return err + } + log.Printf("[TRACE] statemgr.Filesystem: snapshot file has nil snapshot, but that's okay") } s.file = f s.readFile = s.file.DeepCopy() if s.file != nil { log.Printf("[TRACE] statemgr.Filesystem: read snapshot with lineage %q serial %d", s.file.Lineage, s.file.Serial) + } else { + log.Print("[TRACE] statemgr.Filesystem: read nil snapshot") } return nil } @@ -378,13 +388,14 @@ func (s *Filesystem) StateForMigration() *statefile.File { // WriteStateForMigration is part of our implementation of Migrator. func (s *Filesystem) WriteStateForMigration(f *statefile.File, force bool) error { + defer s.mutex()() + if s.readFile == nil { - err := s.RefreshState() + err := s.refreshState() if err != nil { return err } } - defer s.mutex()() if !force { err := CheckValidImport(f, s.readFile) @@ -436,6 +447,20 @@ func (s *Filesystem) createStateFiles() error { } s.stateFileOut = f + + // If the file already existed with content then that'll be the content + // of our backup file if we write a change later. + s.backupFile, err = statefile.Read(s.stateFileOut) + if err != nil { + if err != statefile.ErrNoState { + return err + } + log.Printf("[TRACE] statemgr.Filesystem: no previously-stored snapshot exists") + } else { + log.Printf("[TRACE] statemgr.Filesystem: existing snapshot has lineage %q serial %d", s.backupFile.Lineage, s.backupFile.Serial) + } + + // Refresh now, to load in the snapshot if the file already existed return nil } diff --git a/states/statemgr/filesystem_test.go b/states/statemgr/filesystem_test.go index 7977a6f8d..c21acd34e 100644 --- a/states/statemgr/filesystem_test.go +++ b/states/statemgr/filesystem_test.go @@ -4,13 +4,17 @@ import ( "io/ioutil" "os" "os/exec" + "path/filepath" "strings" "sync" "testing" "github.com/go-test/deep" version "github.com/hashicorp/go-version" + "github.com/zclconf/go-cty/cty" + "github.com/hashicorp/terraform/addrs" + "github.com/hashicorp/terraform/states" "github.com/hashicorp/terraform/states/statefile" tfversion "github.com/hashicorp/terraform/version" ) @@ -57,7 +61,7 @@ func TestFilesystemLocks(t *testing.T) { t.Fatal("unexpected lock failure", err, string(out)) } - if string(out) != "lock failed" { + if !strings.Contains(string(out), "lock failed") { t.Fatal("expected 'locked failed', got", string(out)) } @@ -172,6 +176,116 @@ func TestFilesystem_backup(t *testing.T) { } } +// This test verifies a particularly tricky behavior where the input file +// is overridden and backups are enabled at the same time. This combination +// requires special care because we must ensure that when we create a backup +// it is of the original contents of the output file (which we're overwriting), +// not the contents of the input file (which is left unchanged). +func TestFilesystem_backupAndReadPath(t *testing.T) { + defer testOverrideVersion(t, "1.2.3")() + + workDir, err := ioutil.TempDir("", "tf") + if err != nil { + t.Fatalf("failed to create temporary directory: %s", err) + } + defer os.RemoveAll(workDir) + + markerOutput := addrs.OutputValue{Name: "foo"}.Absolute(addrs.RootModuleInstance) + + outState := states.BuildState(func(ss *states.SyncState) { + ss.SetOutputValue( + markerOutput, + cty.StringVal("from-output-state"), + false, // not sensitive + ) + }) + outFile, err := os.Create(filepath.Join(workDir, "output.tfstate")) + if err != nil { + t.Fatalf("failed to create temporary outFile %s", err) + } + defer outFile.Close() + err = statefile.Write(&statefile.File{ + Lineage: "-", + Serial: 0, + TerraformVersion: version.Must(version.NewVersion("1.2.3")), + State: outState, + }, outFile) + if err != nil { + t.Fatalf("failed to write initial outfile state to %s: %s", outFile.Name(), err) + } + + inState := states.BuildState(func(ss *states.SyncState) { + ss.SetOutputValue( + markerOutput, + cty.StringVal("from-input-state"), + false, // not sensitive + ) + }) + inFile, err := os.Create(filepath.Join(workDir, "input.tfstate")) + if err != nil { + t.Fatalf("failed to create temporary inFile %s", err) + } + defer inFile.Close() + err = statefile.Write(&statefile.File{ + Lineage: "-", + Serial: 0, + TerraformVersion: version.Must(version.NewVersion("1.2.3")), + State: inState, + }, inFile) + if err != nil { + t.Fatalf("failed to write initial infile state to %s: %s", inFile.Name(), err) + } + + backupPath := outFile.Name() + ".backup" + + ls := NewFilesystemBetweenPaths(inFile.Name(), outFile.Name()) + ls.SetBackupPath(backupPath) + + newState := states.BuildState(func(ss *states.SyncState) { + ss.SetOutputValue( + markerOutput, + cty.StringVal("from-new-state"), + false, // not sensitive + ) + }) + err = ls.WriteState(newState) + if err != nil { + t.Fatalf("failed to write new state: %s", err) + } + + // The backup functionality should've saved a copy of the original contents + // of the _output_ file, even though the first snapshot was read from + // the _input_ file. + t.Run("backup file", func (t *testing.T) { + bfh, err := os.Open(backupPath) + if err != nil { + t.Fatal(err) + } + bf, err := statefile.Read(bfh) + if err != nil { + t.Fatal(err) + } + os := bf.State.OutputValue(markerOutput) + if got, want := os.Value, cty.StringVal("from-output-state"); !want.RawEquals(got) { + t.Errorf("wrong marker value in backup state file\ngot: %#v\nwant: %#v", got, want) + } + }) + t.Run("output file", func (t *testing.T) { + ofh, err := os.Open(outFile.Name()) + if err != nil { + t.Fatal(err) + } + of, err := statefile.Read(ofh) + if err != nil { + t.Fatal(err) + } + os := of.State.OutputValue(markerOutput) + if got, want := os.Value, cty.StringVal("from-new-state"); !want.RawEquals(got) { + t.Errorf("wrong marker value in backup state file\ngot: %#v\nwant: %#v", got, want) + } + }) +} + func TestFilesystem_nonExist(t *testing.T) { defer testOverrideVersion(t, "1.2.3")() ls := NewFilesystem("ishouldntexist")