From 09e0dffe6c75e4571d982c2fe9fe4b0d8db0ca1b Mon Sep 17 00:00:00 2001 From: Joshua Feierman Date: Tue, 31 Jan 2023 13:51:40 -0500 Subject: [PATCH 1/5] wip: failing unit test for condition --- internal/states/remote/state_test.go | 258 ++++++++++++++++++--------- 1 file changed, 171 insertions(+), 87 deletions(-) diff --git a/internal/states/remote/state_test.go b/internal/states/remote/state_test.go index 1d348549db86..85fba5939283 100644 --- a/internal/states/remote/state_test.go +++ b/internal/states/remote/state_test.go @@ -49,19 +49,23 @@ type testCase struct { name string // A function to mutate state and return a cleanup function mutationFunc func(*State) (*states.State, func()) - // The expected request to have taken place - expectedRequest mockClientRequest + // The expected requests to have taken place + expectedRequests []mockClientRequest // Mark this case as not having a request noRequest bool + // The expected current state + currentState []byte } // isRequested ensures a test that is specified as not having // a request doesn't have one by checking if a method exists // on the expectedRequest. func (tc testCase) isRequested(t *testing.T) bool { - hasMethod := tc.expectedRequest.Method != "" - if tc.noRequest && hasMethod { - t.Fatalf("expected no content for %q but got: %v", tc.name, tc.expectedRequest) + for _, expectedMethod := range tc.expectedRequests { + hasMethod := expectedMethod.Method != "" + if tc.noRequest && hasMethod { + t.Fatalf("expected no content for %q but got: %v", tc.name, expectedMethod) + } } return !tc.noRequest } @@ -74,17 +78,20 @@ func TestStatePersist(t *testing.T) { mutationFunc: func(mgr *State) (*states.State, func()) { return mgr.State(), func() {} }, - expectedRequest: mockClientRequest{ - Method: "Get", - Content: map[string]interface{}{ - "version": 4.0, // encoding/json decodes this as float64 by default - "lineage": "mock-lineage", - "serial": 1.0, // encoding/json decodes this as float64 by default - "terraform_version": "0.0.0", - "outputs": map[string]interface{}{}, - "resources": []interface{}{}, + expectedRequests: []mockClientRequest{ + { + Method: "Get", + Content: map[string]interface{}{ + "version": 4.0, // encoding/json decodes this as float64 by default + "lineage": "mock-lineage", + "serial": 1.0, // encoding/json decodes this as float64 by default + "terraform_version": "0.0.0", + "outputs": map[string]interface{}{}, + "resources": []interface{}{}, + }, }, }, + currentState: nil, }, { name: "change lineage", @@ -95,18 +102,21 @@ func TestStatePersist(t *testing.T) { mgr.lineage = originalLineage } }, - expectedRequest: mockClientRequest{ - Method: "Put", - Content: map[string]interface{}{ - "version": 4.0, // encoding/json decodes this as float64 by default - "lineage": "some-new-lineage", - "serial": 2.0, // encoding/json decodes this as float64 by default - "terraform_version": version.Version, - "outputs": map[string]interface{}{}, - "resources": []interface{}{}, - "check_results": nil, + expectedRequests: []mockClientRequest{ + { + Method: "Put", + Content: map[string]interface{}{ + "version": 4.0, // encoding/json decodes this as float64 by default + "lineage": "some-new-lineage", + "serial": 2.0, // encoding/json decodes this as float64 by default + "terraform_version": version.Version, + "outputs": map[string]interface{}{}, + "resources": []interface{}{}, + "check_results": nil, + }, }, }, + currentState: nil, }, { name: "change serial", @@ -117,18 +127,21 @@ func TestStatePersist(t *testing.T) { mgr.serial = originalSerial } }, - expectedRequest: mockClientRequest{ - Method: "Put", - Content: map[string]interface{}{ - "version": 4.0, // encoding/json decodes this as float64 by default - "lineage": "mock-lineage", - "serial": 4.0, // encoding/json decodes this as float64 by default - "terraform_version": version.Version, - "outputs": map[string]interface{}{}, - "resources": []interface{}{}, - "check_results": nil, + expectedRequests: []mockClientRequest{ + { + Method: "Put", + Content: map[string]interface{}{ + "version": 4.0, // encoding/json decodes this as float64 by default + "lineage": "mock-lineage", + "serial": 4.0, // encoding/json decodes this as float64 by default + "terraform_version": version.Version, + "outputs": map[string]interface{}{}, + "resources": []interface{}{}, + "check_results": nil, + }, }, }, + currentState: nil, }, { name: "add output to state", @@ -137,23 +150,26 @@ func TestStatePersist(t *testing.T) { s.RootModule().SetOutputValue("foo", cty.StringVal("bar"), false) return s, func() {} }, - expectedRequest: mockClientRequest{ - Method: "Put", - Content: map[string]interface{}{ - "version": 4.0, // encoding/json decodes this as float64 by default - "lineage": "mock-lineage", - "serial": 3.0, // encoding/json decodes this as float64 by default - "terraform_version": version.Version, - "outputs": map[string]interface{}{ - "foo": map[string]interface{}{ - "type": "string", - "value": "bar", + expectedRequests: []mockClientRequest{ + { + Method: "Put", + Content: map[string]interface{}{ + "version": 4.0, // encoding/json decodes this as float64 by default + "lineage": "mock-lineage", + "serial": 3.0, // encoding/json decodes this as float64 by default + "terraform_version": version.Version, + "outputs": map[string]interface{}{ + "foo": map[string]interface{}{ + "type": "string", + "value": "bar", + }, }, + "resources": []interface{}{}, + "check_results": nil, }, - "resources": []interface{}{}, - "check_results": nil, }, }, + currentState: nil, }, { name: "mutate state bar -> baz", @@ -162,23 +178,26 @@ func TestStatePersist(t *testing.T) { s.RootModule().SetOutputValue("foo", cty.StringVal("baz"), false) return s, func() {} }, - expectedRequest: mockClientRequest{ - Method: "Put", - Content: map[string]interface{}{ - "version": 4.0, // encoding/json decodes this as float64 by default - "lineage": "mock-lineage", - "serial": 4.0, // encoding/json decodes this as float64 by default - "terraform_version": version.Version, - "outputs": map[string]interface{}{ - "foo": map[string]interface{}{ - "type": "string", - "value": "baz", + expectedRequests: []mockClientRequest{ + { + Method: "Put", + Content: map[string]interface{}{ + "version": 4.0, // encoding/json decodes this as float64 by default + "lineage": "mock-lineage", + "serial": 4.0, // encoding/json decodes this as float64 by default + "terraform_version": version.Version, + "outputs": map[string]interface{}{ + "foo": map[string]interface{}{ + "type": "string", + "value": "baz", + }, }, + "resources": []interface{}{}, + "check_results": nil, }, - "resources": []interface{}{}, - "check_results": nil, }, }, + currentState: nil, }, { name: "nothing changed", @@ -194,21 +213,77 @@ func TestStatePersist(t *testing.T) { mgr.serial = 2 return mgr.State(), func() {} }, - expectedRequest: mockClientRequest{ - Method: "Put", - Content: map[string]interface{}{ - "version": 4.0, // encoding/json decodes this as float64 by default - "lineage": "mock-lineage", - "serial": 3.0, // encoding/json decodes this as float64 by default - "terraform_version": version.Version, - "outputs": map[string]interface{}{ - "foo": map[string]interface{}{ - "type": "string", - "value": "baz", + expectedRequests: []mockClientRequest{ + { + Method: "Put", + Content: map[string]interface{}{ + "version": 4.0, // encoding/json decodes this as float64 by default + "lineage": "mock-lineage", + "serial": 3.0, // encoding/json decodes this as float64 by default + "terraform_version": version.Version, + "outputs": map[string]interface{}{ + "foo": map[string]interface{}{ + "type": "string", + "value": "baz", + }, }, + "resources": []interface{}{}, + "check_results": nil, + }, + }, + }, + currentState: nil, + }, + { + name: "first state persistence", + mutationFunc: func(mgr *State) (*states.State, func()) { + mgr.readState = nil + return mgr.State(), func() {} + }, + currentState: []byte(` + { + "version": 4, + "lineage": "", + "serial": 0, + "terraform_version":"0.0.0", + "outputs": {}, + "resources": [] + } + `), + expectedRequests: []mockClientRequest{ + { + Method: "Get", + Content: map[string]interface{}{ + "version": 4.0, // encoding/json decodes this as float64 by default + "lineage": "mock-lineage", + "serial": 3.0, // encoding/json decodes this as float64 by default + "terraform_version": version.Version, + "outputs": map[string]interface{}{ + "foo": map[string]interface{}{ + "type": "string", + "value": "baz", + }, + }, + "resources": []interface{}{}, + "check_results": nil, + }, + }, + { + Method: "Put", + Content: map[string]interface{}{ + "version": 4.0, // encoding/json decodes this as float64 by default + "lineage": "mock-lineage", + "serial": 3.0, // encoding/json decodes this as float64 by default + "terraform_version": version.Version, + "outputs": map[string]interface{}{ + "foo": map[string]interface{}{ + "type": "string", + "value": "baz", + }, + }, + "resources": []interface{}{}, + "check_results": nil, }, - "resources": []interface{}{}, - "check_results": nil, }, }, }, @@ -220,15 +295,15 @@ func TestStatePersist(t *testing.T) { mgr := &State{ Client: &mockClient{ current: []byte(` - { - "version": 4, - "lineage": "mock-lineage", - "serial": 1, - "terraform_version":"0.0.0", - "outputs": {}, - "resources": [] - } - `), + { + "version": 4, + "lineage": "mock-lineage", + "serial": 1, + "terraform_version":"0.0.0", + "outputs": {}, + "resources": [] + } + `), }, } @@ -252,6 +327,13 @@ func TestStatePersist(t *testing.T) { // Run tests in order. for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { + if tc.currentState != nil { + originalState := mockClient.current + defer func(){ + mockClient.current = originalState + }() + mockClient.current = tc.currentState + } s, cleanup := tc.mutationFunc(mgr) if err := mgr.WriteState(s); err != nil { @@ -267,10 +349,12 @@ func TestStatePersist(t *testing.T) { if logIdx >= len(mockClient.log) { t.Fatalf("request lock and index are out of sync on %q: idx=%d len=%d", tc.name, logIdx, len(mockClient.log)) } - loggedRequest := mockClient.log[logIdx] - logIdx++ - if diff := cmp.Diff(tc.expectedRequest, loggedRequest); len(diff) > 0 { - t.Fatalf("incorrect client requests for %q:\n%s", tc.name, diff) + for expectedRequestIdx := 0; expectedRequestIdx < len(tc.expectedRequests); expectedRequestIdx++ { + loggedRequest := mockClient.log[logIdx] + logIdx++ + if diff := cmp.Diff(tc.expectedRequests[expectedRequestIdx], loggedRequest); len(diff) > 0 { + t.Fatalf("incorrect client requests for %q:\n%s", tc.name, diff) + } } } cleanup() @@ -278,7 +362,7 @@ func TestStatePersist(t *testing.T) { } logCnt := len(mockClient.log) if logIdx != logCnt { - log.Fatalf("not all requests were read. Expected logIdx to be %d but got %d", logCnt, logIdx) + t.Fatalf("not all requests were read. Expected logIdx to be %d but got %d", logCnt, logIdx) } } From 68e227d93de99d1ca205d2a91bcf43678b31a8eb Mon Sep 17 00:00:00 2001 From: Joshua Feierman Date: Tue, 31 Jan 2023 16:37:09 -0500 Subject: [PATCH 2/5] wip: refactored tests --- internal/states/remote/state_test.go | 148 +++++++++++++-------------- 1 file changed, 69 insertions(+), 79 deletions(-) diff --git a/internal/states/remote/state_test.go b/internal/states/remote/state_test.go index 85fba5939283..a260ac31e3bc 100644 --- a/internal/states/remote/state_test.go +++ b/internal/states/remote/state_test.go @@ -6,8 +6,11 @@ import ( "testing" "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" "github.com/zclconf/go-cty/cty" + tfaddr "github.com/hashicorp/terraform-registry-address" + "github.com/hashicorp/terraform/internal/addrs" "github.com/hashicorp/terraform/internal/states" "github.com/hashicorp/terraform/internal/states/statefile" "github.com/hashicorp/terraform/internal/states/statemgr" @@ -72,32 +75,77 @@ func (tc testCase) isRequested(t *testing.T) bool { func TestStatePersist(t *testing.T) { testCases := []testCase{ - // Refreshing state before we run the test loop causes a GET { - name: "refresh state", + name: "first state persistence", mutationFunc: func(mgr *State) (*states.State, func()) { - return mgr.State(), func() {} + mgr.state = &states.State{ + Modules: map[string]*states.Module{"": {}}, + } + s := mgr.State() + s.RootModule().SetResourceInstanceCurrent( + addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Name: "myfile", + Type: "local_file", + }.Instance(addrs.NoKey), + &states.ResourceInstanceObjectSrc{ + AttrsFlat: map[string]string{ + "filename": "file.txt", + }, + Status: states.ObjectReady, + }, + addrs.AbsProviderConfig{ + Provider: tfaddr.Provider{Namespace: "local"}, + }, + ) + mgr.readState = nil + return s, func() {} }, expectedRequests: []mockClientRequest{ { Method: "Get", + Content: nil, + }, + { + Method: "Get", + Content: nil, + }, + { + Method: "Put", Content: map[string]interface{}{ "version": 4.0, // encoding/json decodes this as float64 by default "lineage": "mock-lineage", - "serial": 1.0, // encoding/json decodes this as float64 by default - "terraform_version": "0.0.0", - "outputs": map[string]interface{}{}, - "resources": []interface{}{}, + "serial": 0.0, // encoding/json decodes this as float64 by default + "terraform_version": version.Version, + "outputs": map[string]interface{}{}, + "resources": []interface{}{ + map[string]interface{}{ + "instances": []interface{}{ + map[string]interface{}{ + "attributes_flat": map[string]interface{}{ + "filename":"file.txt", + }, + "schema_version": 0.0, + "sensitive_attributes": []interface{}{}, + }, + }, + "mode": "managed", + "name": "myfile", + "provider": `provider["/local/"]`, + "type": "local_file", + }, + }, + "check_results": nil, }, }, }, - currentState: nil, }, { name: "change lineage", mutationFunc: func(mgr *State) (*states.State, func()) { originalLineage := mgr.lineage mgr.lineage = "some-new-lineage" + mgr.state.RootModule().Resources = map[string]*states.Resource{} return mgr.State(), func() { mgr.lineage = originalLineage } @@ -108,7 +156,7 @@ func TestStatePersist(t *testing.T) { Content: map[string]interface{}{ "version": 4.0, // encoding/json decodes this as float64 by default "lineage": "some-new-lineage", - "serial": 2.0, // encoding/json decodes this as float64 by default + "serial": 1.0, // encoding/json decodes this as float64 by default "terraform_version": version.Version, "outputs": map[string]interface{}{}, "resources": []interface{}{}, @@ -133,7 +181,7 @@ func TestStatePersist(t *testing.T) { Content: map[string]interface{}{ "version": 4.0, // encoding/json decodes this as float64 by default "lineage": "mock-lineage", - "serial": 4.0, // encoding/json decodes this as float64 by default + "serial": 3.0, // encoding/json decodes this as float64 by default "terraform_version": version.Version, "outputs": map[string]interface{}{}, "resources": []interface{}{}, @@ -156,7 +204,7 @@ func TestStatePersist(t *testing.T) { Content: map[string]interface{}{ "version": 4.0, // encoding/json decodes this as float64 by default "lineage": "mock-lineage", - "serial": 3.0, // encoding/json decodes this as float64 by default + "serial": 2.0, // encoding/json decodes this as float64 by default "terraform_version": version.Version, "outputs": map[string]interface{}{ "foo": map[string]interface{}{ @@ -184,7 +232,7 @@ func TestStatePersist(t *testing.T) { Content: map[string]interface{}{ "version": 4.0, // encoding/json decodes this as float64 by default "lineage": "mock-lineage", - "serial": 4.0, // encoding/json decodes this as float64 by default + "serial": 3.0, // encoding/json decodes this as float64 by default "terraform_version": version.Version, "outputs": map[string]interface{}{ "foo": map[string]interface{}{ @@ -234,77 +282,13 @@ func TestStatePersist(t *testing.T) { }, currentState: nil, }, - { - name: "first state persistence", - mutationFunc: func(mgr *State) (*states.State, func()) { - mgr.readState = nil - return mgr.State(), func() {} - }, - currentState: []byte(` - { - "version": 4, - "lineage": "", - "serial": 0, - "terraform_version":"0.0.0", - "outputs": {}, - "resources": [] - } - `), - expectedRequests: []mockClientRequest{ - { - Method: "Get", - Content: map[string]interface{}{ - "version": 4.0, // encoding/json decodes this as float64 by default - "lineage": "mock-lineage", - "serial": 3.0, // encoding/json decodes this as float64 by default - "terraform_version": version.Version, - "outputs": map[string]interface{}{ - "foo": map[string]interface{}{ - "type": "string", - "value": "baz", - }, - }, - "resources": []interface{}{}, - "check_results": nil, - }, - }, - { - Method: "Put", - Content: map[string]interface{}{ - "version": 4.0, // encoding/json decodes this as float64 by default - "lineage": "mock-lineage", - "serial": 3.0, // encoding/json decodes this as float64 by default - "terraform_version": version.Version, - "outputs": map[string]interface{}{ - "foo": map[string]interface{}{ - "type": "string", - "value": "baz", - }, - }, - "resources": []interface{}{}, - "check_results": nil, - }, - }, - }, - }, } // Initial setup of state just to give us a fixed starting point for our // test assertions below, or else we'd need to deal with // random lineage. mgr := &State{ - Client: &mockClient{ - current: []byte(` - { - "version": 4, - "lineage": "mock-lineage", - "serial": 1, - "terraform_version":"0.0.0", - "outputs": {}, - "resources": [] - } - `), - }, + Client: &mockClient{}, } // In normal use (during a Terraform operation) we always refresh and read @@ -352,8 +336,14 @@ func TestStatePersist(t *testing.T) { for expectedRequestIdx := 0; expectedRequestIdx < len(tc.expectedRequests); expectedRequestIdx++ { loggedRequest := mockClient.log[logIdx] logIdx++ - if diff := cmp.Diff(tc.expectedRequests[expectedRequestIdx], loggedRequest); len(diff) > 0 { - t.Fatalf("incorrect client requests for %q:\n%s", tc.name, diff) + if diff := cmp.Diff(tc.expectedRequests[expectedRequestIdx], loggedRequest, cmpopts.IgnoreMapEntries(func(key string, value interface{}) bool { + if key == "lineage" && value != "mock-lineage" { + return true + } + return false + })); len(diff) > 0 { + t.Logf("incorrect client requests for %q:\n%s", tc.name, diff) + t.Fail() } } } From 2576544db8ebeb78bba41651734a0a941603ac8d Mon Sep 17 00:00:00 2001 From: Joshua Feierman Date: Wed, 1 Feb 2023 12:49:55 -0500 Subject: [PATCH 3/5] fix: remote state behavior This makes the behavior of remote state consistent with local state in regards to the initial serial number of the generated / pushed state. Previously remote state's initial push would have a serial number of 0, whereas local state had a serial of > 0. This causes issues with the logic around, for example, ensuring that a plan file cannot be applied if state is stale (see https://github.com/hashicorp/terraform/issues/30670 for example). --- internal/backend/local/backend_local.go | 3 +++ internal/states/remote/state.go | 8 +++++++- internal/states/remote/state_test.go | 20 ++++++++++++++------ 3 files changed, 24 insertions(+), 7 deletions(-) diff --git a/internal/backend/local/backend_local.go b/internal/backend/local/backend_local.go index 312b31ee2cfe..2e0e3445e7ac 100644 --- a/internal/backend/local/backend_local.go +++ b/internal/backend/local/backend_local.go @@ -284,7 +284,10 @@ func (b *Local) localRunForPlanFile(op *backend.Operation, pf *planfile.Reader, )) return nil, snap, diags } + log.Printf("[DEBUG] backend/local: priorStateFile: %v", *priorStateFile) + if currentStateMeta != nil { + log.Printf("[DEBUG] backend/local: currentStateMeta: %v", *currentStateMeta) // If the caller sets this, we require that the stored prior state // has the same metadata, which is an extra safety check that nothing // has changed since the plan was created. (All of the "real-world" diff --git a/internal/states/remote/state.go b/internal/states/remote/state.go index 412adc3eb4f4..ebff37c1cf08 100644 --- a/internal/states/remote/state.go +++ b/internal/states/remote/state.go @@ -3,6 +3,7 @@ package remote import ( "bytes" "fmt" + "log" "sync" uuid "github.com/hashicorp/go-uuid" @@ -158,6 +159,9 @@ func (s *State) PersistState(schemas *terraform.Schemas) error { s.mu.Lock() defer s.mu.Unlock() + log.Printf("[DEBUG] states/remote: state read serial is: %d; serial is: %d", s.readSerial, s.serial) + log.Printf("[DEBUG] states/remote: state read lineage is: %s; lineage is: %s", s.readLineage, s.lineage) + if s.readState != nil { lineageUnchanged := s.readLineage != "" && s.lineage == s.readLineage serialUnchanged := s.readSerial != 0 && s.serial == s.readSerial @@ -175,13 +179,15 @@ func (s *State) PersistState(schemas *terraform.Schemas) error { if err != nil { return fmt.Errorf("failed checking for existing remote state: %s", err) } + log.Printf("[DEBUG] states/remote: after refresh, state read serial is: %d; serial is: %d", s.readSerial, s.serial) + log.Printf("[DEBUG] states/remote: after refresh, state read lineage is: %s; lineage is: %s", s.readLineage, s.lineage) if s.lineage == "" { // indicates that no state snapshot is present yet lineage, err := uuid.GenerateUUID() if err != nil { return fmt.Errorf("failed to generate initial lineage: %v", err) } s.lineage = lineage - s.serial = 0 + s.serial = 1 } } diff --git a/internal/states/remote/state_test.go b/internal/states/remote/state_test.go index a260ac31e3bc..57ec93377c87 100644 --- a/internal/states/remote/state_test.go +++ b/internal/states/remote/state_test.go @@ -98,24 +98,26 @@ func TestStatePersist(t *testing.T) { Provider: tfaddr.Provider{Namespace: "local"}, }, ) - mgr.readState = nil return s, func() {} }, expectedRequests: []mockClientRequest{ + // Expect an initial refresh, which returns nothing since there is no remote state. { Method: "Get", Content: nil, }, + // Expect a second refresh, since the read state is nil { Method: "Get", Content: nil, }, + // Expect an initial push with values and a serial of 1 { Method: "Put", Content: map[string]interface{}{ "version": 4.0, // encoding/json decodes this as float64 by default "lineage": "mock-lineage", - "serial": 0.0, // encoding/json decodes this as float64 by default + "serial": 1.0, // encoding/json decodes this as float64 by default "terraform_version": version.Version, "outputs": map[string]interface{}{}, "resources": []interface{}{ @@ -140,6 +142,7 @@ func TestStatePersist(t *testing.T) { }, }, }, + // If lineage changes, expect the serial to increment { name: "change lineage", mutationFunc: func(mgr *State) (*states.State, func()) { @@ -156,7 +159,7 @@ func TestStatePersist(t *testing.T) { Content: map[string]interface{}{ "version": 4.0, // encoding/json decodes this as float64 by default "lineage": "some-new-lineage", - "serial": 1.0, // encoding/json decodes this as float64 by default + "serial": 2.0, // encoding/json decodes this as float64 by default "terraform_version": version.Version, "outputs": map[string]interface{}{}, "resources": []interface{}{}, @@ -166,6 +169,7 @@ func TestStatePersist(t *testing.T) { }, currentState: nil, }, + // If the remote serial is incremented, then we increment it once more. { name: "change serial", mutationFunc: func(mgr *State) (*states.State, func()) { @@ -181,7 +185,7 @@ func TestStatePersist(t *testing.T) { Content: map[string]interface{}{ "version": 4.0, // encoding/json decodes this as float64 by default "lineage": "mock-lineage", - "serial": 3.0, // encoding/json decodes this as float64 by default + "serial": 4.0, // encoding/json decodes this as float64 by default "terraform_version": version.Version, "outputs": map[string]interface{}{}, "resources": []interface{}{}, @@ -191,6 +195,7 @@ func TestStatePersist(t *testing.T) { }, currentState: nil, }, + // Adding an output should cause the serial to increment as well. { name: "add output to state", mutationFunc: func(mgr *State) (*states.State, func()) { @@ -204,7 +209,7 @@ func TestStatePersist(t *testing.T) { Content: map[string]interface{}{ "version": 4.0, // encoding/json decodes this as float64 by default "lineage": "mock-lineage", - "serial": 2.0, // encoding/json decodes this as float64 by default + "serial": 3.0, // encoding/json decodes this as float64 by default "terraform_version": version.Version, "outputs": map[string]interface{}{ "foo": map[string]interface{}{ @@ -219,6 +224,7 @@ func TestStatePersist(t *testing.T) { }, currentState: nil, }, + // ...as should changing an output { name: "mutate state bar -> baz", mutationFunc: func(mgr *State) (*states.State, func()) { @@ -232,7 +238,7 @@ func TestStatePersist(t *testing.T) { Content: map[string]interface{}{ "version": 4.0, // encoding/json decodes this as float64 by default "lineage": "mock-lineage", - "serial": 3.0, // encoding/json decodes this as float64 by default + "serial": 4.0, // encoding/json decodes this as float64 by default "terraform_version": version.Version, "outputs": map[string]interface{}{ "foo": map[string]interface{}{ @@ -255,6 +261,8 @@ func TestStatePersist(t *testing.T) { }, noRequest: true, }, + // If the remote state's serial is less (force push), then we + // increment it once from there. { name: "reset serial (force push style)", mutationFunc: func(mgr *State) (*states.State, func()) { From d45ebfbdef745f4c518f5ef8f26dbd2d96458a94 Mon Sep 17 00:00:00 2001 From: Joshua Feierman Date: Wed, 1 Feb 2023 13:08:38 -0500 Subject: [PATCH 4/5] chore: clean-up tests & logging --- internal/backend/local/backend_local.go | 2 - internal/states/remote/state_test.go | 98 +++++++++++++++---------- 2 files changed, 58 insertions(+), 42 deletions(-) diff --git a/internal/backend/local/backend_local.go b/internal/backend/local/backend_local.go index 2e0e3445e7ac..84646ab48297 100644 --- a/internal/backend/local/backend_local.go +++ b/internal/backend/local/backend_local.go @@ -284,10 +284,8 @@ func (b *Local) localRunForPlanFile(op *backend.Operation, pf *planfile.Reader, )) return nil, snap, diags } - log.Printf("[DEBUG] backend/local: priorStateFile: %v", *priorStateFile) if currentStateMeta != nil { - log.Printf("[DEBUG] backend/local: currentStateMeta: %v", *currentStateMeta) // If the caller sets this, we require that the stored prior state // has the same metadata, which is an extra safety check that nothing // has changed since the plan was created. (All of the "real-world" diff --git a/internal/states/remote/state_test.go b/internal/states/remote/state_test.go index 57ec93377c87..71e86c65703c 100644 --- a/internal/states/remote/state_test.go +++ b/internal/states/remote/state_test.go @@ -56,8 +56,6 @@ type testCase struct { expectedRequests []mockClientRequest // Mark this case as not having a request noRequest bool - // The expected current state - currentState []byte } // isRequested ensures a test that is specified as not having @@ -84,9 +82,9 @@ func TestStatePersist(t *testing.T) { s := mgr.State() s.RootModule().SetResourceInstanceCurrent( addrs.Resource{ - Mode: addrs.ManagedResourceMode, - Name: "myfile", - Type: "local_file", + Mode: addrs.ManagedResourceMode, + Name: "myfile", + Type: "local_file", }.Instance(addrs.NoKey), &states.ResourceInstanceObjectSrc{ AttrsFlat: map[string]string{ @@ -103,12 +101,12 @@ func TestStatePersist(t *testing.T) { expectedRequests: []mockClientRequest{ // Expect an initial refresh, which returns nothing since there is no remote state. { - Method: "Get", + Method: "Get", Content: nil, }, // Expect a second refresh, since the read state is nil { - Method: "Get", + Method: "Get", Content: nil, }, // Expect an initial push with values and a serial of 1 @@ -116,25 +114,25 @@ func TestStatePersist(t *testing.T) { Method: "Put", Content: map[string]interface{}{ "version": 4.0, // encoding/json decodes this as float64 by default - "lineage": "mock-lineage", + "lineage": "some meaningless value", "serial": 1.0, // encoding/json decodes this as float64 by default "terraform_version": version.Version, - "outputs": map[string]interface{}{}, + "outputs": map[string]interface{}{}, "resources": []interface{}{ map[string]interface{}{ "instances": []interface{}{ map[string]interface{}{ "attributes_flat": map[string]interface{}{ - "filename":"file.txt", + "filename": "file.txt", }, - "schema_version": 0.0, + "schema_version": 0.0, "sensitive_attributes": []interface{}{}, }, }, - "mode": "managed", - "name": "myfile", + "mode": "managed", + "name": "myfile", "provider": `provider["/local/"]`, - "type": "local_file", + "type": "local_file", }, }, "check_results": nil, @@ -146,28 +144,61 @@ func TestStatePersist(t *testing.T) { { name: "change lineage", mutationFunc: func(mgr *State) (*states.State, func()) { - originalLineage := mgr.lineage - mgr.lineage = "some-new-lineage" - mgr.state.RootModule().Resources = map[string]*states.Resource{} - return mgr.State(), func() { - mgr.lineage = originalLineage - } + mgr.lineage = "mock-lineage" + return mgr.State(), func() {} }, expectedRequests: []mockClientRequest{ { Method: "Put", Content: map[string]interface{}{ "version": 4.0, // encoding/json decodes this as float64 by default - "lineage": "some-new-lineage", + "lineage": "mock-lineage", "serial": 2.0, // encoding/json decodes this as float64 by default "terraform_version": version.Version, "outputs": map[string]interface{}{}, + "resources": []interface{}{ + map[string]interface{}{ + "instances": []interface{}{ + map[string]interface{}{ + "attributes_flat": map[string]interface{}{ + "filename": "file.txt", + }, + "schema_version": 0.0, + "sensitive_attributes": []interface{}{}, + }, + }, + "mode": "managed", + "name": "myfile", + "provider": `provider["/local/"]`, + "type": "local_file", + }, + }, + "check_results": nil, + }, + }, + }, + }, + // removing resources should increment the serial + { + name: "remove resources", + mutationFunc: func(mgr *State) (*states.State, func()) { + mgr.state.RootModule().Resources = map[string]*states.Resource{} + return mgr.State(), func() {} + }, + expectedRequests: []mockClientRequest{ + { + Method: "Put", + Content: map[string]interface{}{ + "version": 4.0, // encoding/json decodes this as float64 by default + "lineage": "mock-lineage", + "serial": 3.0, // encoding/json decodes this as float64 by default + "terraform_version": version.Version, + "outputs": map[string]interface{}{}, "resources": []interface{}{}, "check_results": nil, }, }, }, - currentState: nil, }, // If the remote serial is incremented, then we increment it once more. { @@ -185,7 +216,7 @@ func TestStatePersist(t *testing.T) { Content: map[string]interface{}{ "version": 4.0, // encoding/json decodes this as float64 by default "lineage": "mock-lineage", - "serial": 4.0, // encoding/json decodes this as float64 by default + "serial": 5.0, // encoding/json decodes this as float64 by default "terraform_version": version.Version, "outputs": map[string]interface{}{}, "resources": []interface{}{}, @@ -193,7 +224,6 @@ func TestStatePersist(t *testing.T) { }, }, }, - currentState: nil, }, // Adding an output should cause the serial to increment as well. { @@ -209,7 +239,7 @@ func TestStatePersist(t *testing.T) { Content: map[string]interface{}{ "version": 4.0, // encoding/json decodes this as float64 by default "lineage": "mock-lineage", - "serial": 3.0, // encoding/json decodes this as float64 by default + "serial": 4.0, // encoding/json decodes this as float64 by default "terraform_version": version.Version, "outputs": map[string]interface{}{ "foo": map[string]interface{}{ @@ -222,7 +252,6 @@ func TestStatePersist(t *testing.T) { }, }, }, - currentState: nil, }, // ...as should changing an output { @@ -238,7 +267,7 @@ func TestStatePersist(t *testing.T) { Content: map[string]interface{}{ "version": 4.0, // encoding/json decodes this as float64 by default "lineage": "mock-lineage", - "serial": 4.0, // encoding/json decodes this as float64 by default + "serial": 5.0, // encoding/json decodes this as float64 by default "terraform_version": version.Version, "outputs": map[string]interface{}{ "foo": map[string]interface{}{ @@ -251,7 +280,6 @@ func TestStatePersist(t *testing.T) { }, }, }, - currentState: nil, }, { name: "nothing changed", @@ -288,7 +316,6 @@ func TestStatePersist(t *testing.T) { }, }, }, - currentState: nil, }, } @@ -319,13 +346,6 @@ func TestStatePersist(t *testing.T) { // Run tests in order. for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - if tc.currentState != nil { - originalState := mockClient.current - defer func(){ - mockClient.current = originalState - }() - mockClient.current = tc.currentState - } s, cleanup := tc.mutationFunc(mgr) if err := mgr.WriteState(s); err != nil { @@ -345,10 +365,8 @@ func TestStatePersist(t *testing.T) { loggedRequest := mockClient.log[logIdx] logIdx++ if diff := cmp.Diff(tc.expectedRequests[expectedRequestIdx], loggedRequest, cmpopts.IgnoreMapEntries(func(key string, value interface{}) bool { - if key == "lineage" && value != "mock-lineage" { - return true - } - return false + // This is required since the initial state creation causes the lineage to be a UUID that is not known at test time. + return tc.name == "first state persistence" && key == "lineage" })); len(diff) > 0 { t.Logf("incorrect client requests for %q:\n%s", tc.name, diff) t.Fail() From 2d9e3da9832fd16139198809cf1776e2ccae96b4 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 7 Feb 2023 17:35:56 -0500 Subject: [PATCH 5/5] Update internal/states/remote/state.go Fix from review Co-authored-by: Nathan Mische --- internal/states/remote/state.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/states/remote/state.go b/internal/states/remote/state.go index ebff37c1cf08..d8da9d827adb 100644 --- a/internal/states/remote/state.go +++ b/internal/states/remote/state.go @@ -187,7 +187,7 @@ func (s *State) PersistState(schemas *terraform.Schemas) error { return fmt.Errorf("failed to generate initial lineage: %v", err) } s.lineage = lineage - s.serial = 1 + s.serial++ } }