From 2f8f30269b2bed61ef139539a1303d2b8536a783 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Wed, 10 Jul 2024 19:24:37 +0200 Subject: [PATCH 01/13] Invalidate local terraform state only when lineage match --- bundle/deploy/terraform/state_pull.go | 113 +++++++++----- bundle/deploy/terraform/state_pull_test.go | 172 ++++++++++++--------- bundle/deploy/terraform/state_push_test.go | 2 +- bundle/deploy/terraform/state_test.go | 6 +- bundle/deploy/terraform/util.go | 22 +-- bundle/deploy/terraform/util_test.go | 65 ++++---- 6 files changed, 212 insertions(+), 168 deletions(-) diff --git a/bundle/deploy/terraform/state_pull.go b/bundle/deploy/terraform/state_pull.go index cc7d342747a..7825c5718ea 100644 --- a/bundle/deploy/terraform/state_pull.go +++ b/bundle/deploy/terraform/state_pull.go @@ -1,8 +1,8 @@ package terraform import ( - "bytes" "context" + "encoding/json" "errors" "io" "io/fs" @@ -12,10 +12,14 @@ import ( "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/deploy" "github.com/databricks/cli/libs/diag" - "github.com/databricks/cli/libs/filer" "github.com/databricks/cli/libs/log" ) +type tfState struct { + Serial int `json:"serial"` + Lineage string `json:"lineage"` +} + type statePull struct { filerFactory deploy.FilerFactory } @@ -24,71 +28,102 @@ func (l *statePull) Name() string { return "terraform:state-pull" } -func (l *statePull) remoteState(ctx context.Context, f filer.Filer) (*bytes.Buffer, error) { - // Download state file from filer to local cache directory. - remote, err := f.Read(ctx, TerraformStateFileName) +func (l *statePull) remoteState(ctx context.Context, b *bundle.Bundle) (*tfState, []byte, error) { + f, err := l.filerFactory(b) if err != nil { - // On first deploy this state file doesn't yet exist. - if errors.Is(err, fs.ErrNotExist) { - return nil, nil - } - return nil, err + return nil, nil, err } + remote, err := f.Read(ctx, TerraformStateFileName) + if err != nil { + return nil, nil, err + } defer remote.Close() - var buf bytes.Buffer - _, err = io.Copy(&buf, remote) + remoteContent, err := io.ReadAll(remote) if err != nil { - return nil, err + return nil, nil, err + } + + remoteState := &tfState{} + err = json.Unmarshal(remoteContent, remoteState) + if err != nil { + return nil, nil, err } - return &buf, nil + return remoteState, remoteContent, nil } -func (l *statePull) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { - f, err := l.filerFactory(b) +func (l *statePull) localState(ctx context.Context, b *bundle.Bundle) (*tfState, error) { + dir, err := Dir(ctx, b) if err != nil { - return diag.FromErr(err) + return nil, err } - dir, err := Dir(ctx, b) + localStatePath := filepath.Join(dir, TerraformStateFileName) + local, err := os.Open(localStatePath) if err != nil { - return diag.FromErr(err) + return nil, err } + defer local.Close() - // Download state file from filer to local cache directory. - log.Infof(ctx, "Opening remote state file") - remote, err := l.remoteState(ctx, f) + localContent, err := io.ReadAll(local) if err != nil { - log.Infof(ctx, "Unable to open remote state file: %s", err) - return diag.FromErr(err) + return nil, err } - if remote == nil { - log.Infof(ctx, "Remote state file does not exist") - return nil + + localState := &tfState{} + err = json.Unmarshal(localContent, localState) + if err != nil { + return nil, err } - // Expect the state file to live under dir. - local, err := os.OpenFile(filepath.Join(dir, TerraformStateFileName), os.O_CREATE|os.O_RDWR, 0600) + return localState, nil +} + +func (l *statePull) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { + dir, err := Dir(ctx, b) if err != nil { return diag.FromErr(err) } - defer local.Close() - if !IsLocalStateStale(local, bytes.NewReader(remote.Bytes())) { - log.Infof(ctx, "Local state is the same or newer, ignoring remote state") + localStatePath := filepath.Join(dir, TerraformStateFileName) + + // Case: remote state file does not exist. In this case we should not use the + // local state file because we cannot guarantee it corresponds to the same deployment, + // that is the same root_path and workspace host. + remoteState, remoteContent, err := l.remoteState(ctx, b) + if errors.Is(err, fs.ErrNotExist) { + log.Infof(ctx, "Remote state file does not exist. Invalidating local terraform state.") + os.Remove(localStatePath) return nil } + if err != nil { + return diag.Errorf("failed to read remote state file: %v", err) + } - // Truncating the file before writing - local.Truncate(0) - local.Seek(0, 0) - - // Write file to disk. - log.Infof(ctx, "Writing remote state file to local cache directory") - _, err = io.Copy(local, bytes.NewReader(remote.Bytes())) + // Case: Local host does not exist. In this case we should rely on the remote state file. + localState, err := l.localState(ctx, b) + if errors.Is(err, fs.ErrNotExist) { + log.Infof(ctx, "Local state file does not exist. Using remote terraform state. Invalidating local terraform state.") + err := os.WriteFile(localStatePath, remoteContent, 0600) + return diag.FromErr(err) + } if err != nil { + return diag.Errorf("failed to read local state file: %v", err) + } + + // If the lineages do not match, the terraform state files do not correspond to the same deployment. + if localState.Lineage != remoteState.Lineage { + log.Infof(ctx, "Remote and local state lineages do not match. Using remote terraform state. Invalidating local terraform state.") + err := os.WriteFile(localStatePath, remoteContent, 0600) + return diag.FromErr(err) + } + + // If the remote state is newer than the local state, we should use the remote state. + if remoteState.Serial > localState.Serial { + log.Infof(ctx, "Remote state is newer than local state. Using remote terraform state.") + err := os.WriteFile(localStatePath, remoteContent, 0600) return diag.FromErr(err) } diff --git a/bundle/deploy/terraform/state_pull_test.go b/bundle/deploy/terraform/state_pull_test.go index 26297bfcbeb..960ecddf0ba 100644 --- a/bundle/deploy/terraform/state_pull_test.go +++ b/bundle/deploy/terraform/state_pull_test.go @@ -17,7 +17,7 @@ import ( "github.com/stretchr/testify/mock" ) -func mockStateFilerForPull(t *testing.T, contents map[string]int, merr error) filer.Filer { +func mockStateFilerForPull(t *testing.T, contents map[string]any, merr error) filer.Filer { buf, err := json.Marshal(contents) assert.NoError(t, err) @@ -41,86 +41,104 @@ func statePullTestBundle(t *testing.T) *bundle.Bundle { } } -func TestStatePullLocalMissingRemoteMissing(t *testing.T) { - m := &statePull{ - identityFiler(mockStateFilerForPull(t, nil, os.ErrNotExist)), - } - - ctx := context.Background() - b := statePullTestBundle(t) - diags := bundle.Apply(ctx, b, m) - assert.NoError(t, diags.Error()) - - // Confirm that no local state file has been written. - _, err := os.Stat(localStateFile(t, ctx, b)) - assert.ErrorIs(t, err, fs.ErrNotExist) -} +func TestStatePullLocal(t *testing.T) { + tcases := []struct { + name string -func TestStatePullLocalMissingRemotePresent(t *testing.T) { - m := &statePull{ - identityFiler(mockStateFilerForPull(t, map[string]int{"serial": 5}, nil)), - } - - ctx := context.Background() - b := statePullTestBundle(t) - diags := bundle.Apply(ctx, b, m) - assert.NoError(t, diags.Error()) + // remote state before applying the pull mutators + remote map[string]any - // Confirm that the local state file has been updated. - localState := readLocalState(t, ctx, b) - assert.Equal(t, map[string]int{"serial": 5}, localState) -} + // local state before applying the pull mutators + local map[string]any -func TestStatePullLocalStale(t *testing.T) { - m := &statePull{ - identityFiler(mockStateFilerForPull(t, map[string]int{"serial": 5}, nil)), - } - - ctx := context.Background() - b := statePullTestBundle(t) - - // Write a stale local state file. - writeLocalState(t, ctx, b, map[string]int{"serial": 4}) - diags := bundle.Apply(ctx, b, m) - assert.NoError(t, diags.Error()) - - // Confirm that the local state file has been updated. - localState := readLocalState(t, ctx, b) - assert.Equal(t, map[string]int{"serial": 5}, localState) -} - -func TestStatePullLocalEqual(t *testing.T) { - m := &statePull{ - identityFiler(mockStateFilerForPull(t, map[string]int{"serial": 5, "some_other_key": 123}, nil)), + // expected local state after applying the pull mutators + expected map[string]any + }{ + { + name: "local missing, remote missing", + remote: nil, + local: nil, + expected: nil, + }, + { + name: "local missing, remote present", + remote: map[string]any{"serial": 5}, + local: nil, + expected: map[string]any{"serial": float64(5)}, + }, + { + name: "local stale", + remote: map[string]any{"serial": 5}, + local: map[string]any{"serial": 4}, + expected: map[string]any{"serial": float64(5)}, + }, + { + name: "local equal", + remote: map[string]any{"serial": 5, "some_other_key": 123}, + local: map[string]any{"serial": 5}, + expected: map[string]any{"serial": float64(5)}, + }, + { + name: "local newer", + remote: map[string]any{"serial": 5, "some_other_key": 123}, + local: map[string]any{"serial": 6}, + expected: map[string]any{"serial": float64(6)}, + }, + { + name: "remote missing, local present", + remote: nil, + local: map[string]any{"serial": 5, "some_other_key": 123}, + // local state should be invalidated if remote state is missing. + expected: nil, + }, + { + name: "remote and local have different lineages", + remote: map[string]any{"serial": 5, "lineage": "aaaa"}, + local: map[string]any{"serial": 10, "lineage": "bbbb"}, + expected: map[string]any{"serial": float64(5), "lineage": "aaaa"}, + }, + { + name: "remote and local have the same lineage, local newer", + remote: map[string]any{"serial": 5, "lineage": "aaaa", "some_other_key": 123}, + local: map[string]any{"serial": 10, "lineage": "aaaa"}, + expected: map[string]any{"serial": float64(10), "lineage": "aaaa"}, + }, + { + name: "remote and local have the same lineage, remote newer", + remote: map[string]any{"serial": 10, "lineage": "aaaa", "some_other_key": 123}, + local: map[string]any{"serial": 5, "lineage": "aaaa"}, + expected: map[string]any{"serial": float64(10), "lineage": "aaaa", "some_other_key": float64(123)}, + }, } - ctx := context.Background() - b := statePullTestBundle(t) - - // Write a local state file with the same serial as the remote. - writeLocalState(t, ctx, b, map[string]int{"serial": 5}) - diags := bundle.Apply(ctx, b, m) - assert.NoError(t, diags.Error()) - - // Confirm that the local state file has not been updated. - localState := readLocalState(t, ctx, b) - assert.Equal(t, map[string]int{"serial": 5}, localState) -} - -func TestStatePullLocalNewer(t *testing.T) { - m := &statePull{ - identityFiler(mockStateFilerForPull(t, map[string]int{"serial": 5, "some_other_key": 123}, nil)), + for _, tc := range tcases { + t.Run(tc.name, func(t *testing.T) { + m := &statePull{} + if tc.remote == nil { + // nil represents no remote state file. + m.filerFactory = identityFiler(mockStateFilerForPull(t, nil, os.ErrNotExist)) + } else { + m.filerFactory = identityFiler(mockStateFilerForPull(t, tc.remote, nil)) + } + + ctx := context.Background() + b := statePullTestBundle(t) + if tc.local != nil { + writeLocalState(t, ctx, b, tc.local) + } + + diags := bundle.Apply(ctx, b, m) + assert.NoError(t, diags.Error()) + + if tc.expected == nil { + // nil represents no local state file is expected. + _, err := os.Stat(localStateFile(t, ctx, b)) + assert.ErrorIs(t, err, fs.ErrNotExist) + } else { + localState := readLocalState(t, ctx, b) + assert.Equal(t, tc.expected, localState) + + } + }) } - - ctx := context.Background() - b := statePullTestBundle(t) - - // Write a local state file with a newer serial as the remote. - writeLocalState(t, ctx, b, map[string]int{"serial": 6}) - diags := bundle.Apply(ctx, b, m) - assert.NoError(t, diags.Error()) - - // Confirm that the local state file has not been updated. - localState := readLocalState(t, ctx, b) - assert.Equal(t, map[string]int{"serial": 6}, localState) } diff --git a/bundle/deploy/terraform/state_push_test.go b/bundle/deploy/terraform/state_push_test.go index e054773f310..ac74f345d2f 100644 --- a/bundle/deploy/terraform/state_push_test.go +++ b/bundle/deploy/terraform/state_push_test.go @@ -55,7 +55,7 @@ func TestStatePush(t *testing.T) { b := statePushTestBundle(t) // Write a stale local state file. - writeLocalState(t, ctx, b, map[string]int{"serial": 4}) + writeLocalState(t, ctx, b, map[string]any{"serial": 4}) diags := bundle.Apply(ctx, b, m) assert.NoError(t, diags.Error()) } diff --git a/bundle/deploy/terraform/state_test.go b/bundle/deploy/terraform/state_test.go index ff325062553..73d7cb0dee4 100644 --- a/bundle/deploy/terraform/state_test.go +++ b/bundle/deploy/terraform/state_test.go @@ -26,19 +26,19 @@ func localStateFile(t *testing.T, ctx context.Context, b *bundle.Bundle) string return filepath.Join(dir, TerraformStateFileName) } -func readLocalState(t *testing.T, ctx context.Context, b *bundle.Bundle) map[string]int { +func readLocalState(t *testing.T, ctx context.Context, b *bundle.Bundle) map[string]any { f, err := os.Open(localStateFile(t, ctx, b)) require.NoError(t, err) defer f.Close() - var contents map[string]int + var contents map[string]any dec := json.NewDecoder(f) err = dec.Decode(&contents) require.NoError(t, err) return contents } -func writeLocalState(t *testing.T, ctx context.Context, b *bundle.Bundle, contents map[string]int) { +func writeLocalState(t *testing.T, ctx context.Context, b *bundle.Bundle, contents map[string]any) { f, err := os.Create(localStateFile(t, ctx, b)) require.NoError(t, err) defer f.Close() diff --git a/bundle/deploy/terraform/util.go b/bundle/deploy/terraform/util.go index 1a8a83ac73b..360bd69b0b6 100644 --- a/bundle/deploy/terraform/util.go +++ b/bundle/deploy/terraform/util.go @@ -22,10 +22,6 @@ type resourcesState struct { const SupportedStateVersion = 4 -type serialState struct { - Serial int `json:"serial"` -} - type stateResource struct { Type string `json:"type"` Name string `json:"name"` @@ -41,26 +37,12 @@ type stateInstanceAttributes struct { ID string `json:"id"` } -func IsLocalStateStale(local io.Reader, remote io.Reader) bool { - localState, err := loadState(local) - if err != nil { - return true - } - - remoteState, err := loadState(remote) - if err != nil { - return false - } - - return localState.Serial < remoteState.Serial -} - -func loadState(input io.Reader) (*serialState, error) { +func loadState(input io.Reader) (*tfState, error) { content, err := io.ReadAll(input) if err != nil { return nil, err } - var s serialState + var s tfState err = json.Unmarshal(content, &s) if err != nil { return nil, err diff --git a/bundle/deploy/terraform/util_test.go b/bundle/deploy/terraform/util_test.go index 8949ebca82b..786d47081b8 100644 --- a/bundle/deploy/terraform/util_test.go +++ b/bundle/deploy/terraform/util_test.go @@ -2,47 +2,56 @@ package terraform import ( "context" - "fmt" "os" "path/filepath" - "strings" "testing" - "testing/iotest" "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/config" "github.com/stretchr/testify/assert" ) -func TestLocalStateIsNewer(t *testing.T) { - local := strings.NewReader(`{"serial": 5}`) - remote := strings.NewReader(`{"serial": 4}`) - assert.False(t, IsLocalStateStale(local, remote)) -} +// func TestLocalStateIsNewer(t *testing.T) { +// local := strings.NewReader(`{"serial": 5}`) +// remote := strings.NewReader(`{"serial": 4}`) +// assert.False(t, IsLocalStateStale(local, remote)) +// } -func TestLocalStateIsOlder(t *testing.T) { - local := strings.NewReader(`{"serial": 5}`) - remote := strings.NewReader(`{"serial": 6}`) - assert.True(t, IsLocalStateStale(local, remote)) -} +// func TestLocalStateIsOlder(t *testing.T) { +// local := strings.NewReader(`{"serial": 5}`) +// remote := strings.NewReader(`{"serial": 6}`) +// assert.True(t, IsLocalStateStale(local, remote)) +// } -func TestLocalStateIsTheSame(t *testing.T) { - local := strings.NewReader(`{"serial": 5}`) - remote := strings.NewReader(`{"serial": 5}`) - assert.False(t, IsLocalStateStale(local, remote)) -} +// func TestLocalStateIsTheSame(t *testing.T) { +// local := strings.NewReader(`{"serial": 5}`) +// remote := strings.NewReader(`{"serial": 5}`) +// assert.False(t, IsLocalStateStale(local, remote)) +// } -func TestLocalStateMarkStaleWhenFailsToLoad(t *testing.T) { - local := iotest.ErrReader(fmt.Errorf("Random error")) - remote := strings.NewReader(`{"serial": 5}`) - assert.True(t, IsLocalStateStale(local, remote)) -} +// func TestLocalStateMarkStaleWhenFailsToLoad(t *testing.T) { +// local := iotest.ErrReader(fmt.Errorf("Random error")) +// remote := strings.NewReader(`{"serial": 5}`) +// assert.True(t, IsLocalStateStale(local, remote)) +// } -func TestLocalStateMarkNonStaleWhenRemoteFailsToLoad(t *testing.T) { - local := strings.NewReader(`{"serial": 5}`) - remote := iotest.ErrReader(fmt.Errorf("Random error")) - assert.False(t, IsLocalStateStale(local, remote)) -} +// func TestLocalStateMarkNonStaleWhenRemoteFailsToLoad(t *testing.T) { +// local := strings.NewReader(`{"serial": 5}`) +// remote := iotest.ErrReader(fmt.Errorf("Random error")) +// assert.False(t, IsLocalStateStale(local, remote)) +// } + +// func TestLocalStateIsFromAnotherDeployment(t *testing.T) { +// local := strings.NewReader(`{"serial": 5, "lineage": "alice"}`) +// remote := strings.NewReader(`{"serial": 5, "lineage": "bob"}`) +// assert.True(t, IsLocalStateStale(local, remote)) +// } + +// func TestLocalStateIsFromTheSameDeployment(t *testing.T) { +// local := strings.NewReader(`{"serial": 5, "lineage": "alice"}`) +// remote := strings.NewReader(`{"serial": 5, "lineage": "alice"}`) +// assert.False(t, IsLocalStateStale(local, remote)) +// } func TestParseResourcesStateWithNoFile(t *testing.T) { b := &bundle.Bundle{ From 479ae9684a85f5fd9e5cbbd7c7abc2d4656acae9 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Wed, 10 Jul 2024 19:25:14 +0200 Subject: [PATCH 02/13] - --- bundle/deploy/terraform/util_test.go | 42 ---------------------------- 1 file changed, 42 deletions(-) diff --git a/bundle/deploy/terraform/util_test.go b/bundle/deploy/terraform/util_test.go index 786d47081b8..251a7c256a3 100644 --- a/bundle/deploy/terraform/util_test.go +++ b/bundle/deploy/terraform/util_test.go @@ -11,48 +11,6 @@ import ( "github.com/stretchr/testify/assert" ) -// func TestLocalStateIsNewer(t *testing.T) { -// local := strings.NewReader(`{"serial": 5}`) -// remote := strings.NewReader(`{"serial": 4}`) -// assert.False(t, IsLocalStateStale(local, remote)) -// } - -// func TestLocalStateIsOlder(t *testing.T) { -// local := strings.NewReader(`{"serial": 5}`) -// remote := strings.NewReader(`{"serial": 6}`) -// assert.True(t, IsLocalStateStale(local, remote)) -// } - -// func TestLocalStateIsTheSame(t *testing.T) { -// local := strings.NewReader(`{"serial": 5}`) -// remote := strings.NewReader(`{"serial": 5}`) -// assert.False(t, IsLocalStateStale(local, remote)) -// } - -// func TestLocalStateMarkStaleWhenFailsToLoad(t *testing.T) { -// local := iotest.ErrReader(fmt.Errorf("Random error")) -// remote := strings.NewReader(`{"serial": 5}`) -// assert.True(t, IsLocalStateStale(local, remote)) -// } - -// func TestLocalStateMarkNonStaleWhenRemoteFailsToLoad(t *testing.T) { -// local := strings.NewReader(`{"serial": 5}`) -// remote := iotest.ErrReader(fmt.Errorf("Random error")) -// assert.False(t, IsLocalStateStale(local, remote)) -// } - -// func TestLocalStateIsFromAnotherDeployment(t *testing.T) { -// local := strings.NewReader(`{"serial": 5, "lineage": "alice"}`) -// remote := strings.NewReader(`{"serial": 5, "lineage": "bob"}`) -// assert.True(t, IsLocalStateStale(local, remote)) -// } - -// func TestLocalStateIsFromTheSameDeployment(t *testing.T) { -// local := strings.NewReader(`{"serial": 5, "lineage": "alice"}`) -// remote := strings.NewReader(`{"serial": 5, "lineage": "alice"}`) -// assert.False(t, IsLocalStateStale(local, remote)) -// } - func TestParseResourcesStateWithNoFile(t *testing.T) { b := &bundle.Bundle{ RootPath: t.TempDir(), From 529366dc1a3e523731b347828992adf294756f01 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Wed, 10 Jul 2024 19:33:17 +0200 Subject: [PATCH 03/13] remove unused function --- bundle/deploy/terraform/util.go | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/bundle/deploy/terraform/util.go b/bundle/deploy/terraform/util.go index 360bd69b0b6..64d667b5f6e 100644 --- a/bundle/deploy/terraform/util.go +++ b/bundle/deploy/terraform/util.go @@ -4,7 +4,6 @@ import ( "context" "encoding/json" "errors" - "io" "os" "path/filepath" @@ -37,20 +36,6 @@ type stateInstanceAttributes struct { ID string `json:"id"` } -func loadState(input io.Reader) (*tfState, error) { - content, err := io.ReadAll(input) - if err != nil { - return nil, err - } - var s tfState - err = json.Unmarshal(content, &s) - if err != nil { - return nil, err - } - - return &s, nil -} - func ParseResourcesState(ctx context.Context, b *bundle.Bundle) (*resourcesState, error) { cacheDir, err := Dir(ctx, b) if err != nil { From 1f0d7100d3273cd98fefa5b257cf3dcefdb3f9ad Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Wed, 10 Jul 2024 19:43:25 +0200 Subject: [PATCH 04/13] nit --- bundle/deploy/terraform/state_pull.go | 29 ++++++++++----------------- 1 file changed, 11 insertions(+), 18 deletions(-) diff --git a/bundle/deploy/terraform/state_pull.go b/bundle/deploy/terraform/state_pull.go index 7825c5718ea..d52ea2e9135 100644 --- a/bundle/deploy/terraform/state_pull.go +++ b/bundle/deploy/terraform/state_pull.go @@ -34,24 +34,24 @@ func (l *statePull) remoteState(ctx context.Context, b *bundle.Bundle) (*tfState return nil, nil, err } - remote, err := f.Read(ctx, TerraformStateFileName) + r, err := f.Read(ctx, TerraformStateFileName) if err != nil { return nil, nil, err } - defer remote.Close() + defer r.Close() - remoteContent, err := io.ReadAll(remote) + content, err := io.ReadAll(r) if err != nil { return nil, nil, err } - remoteState := &tfState{} - err = json.Unmarshal(remoteContent, remoteState) + state := &tfState{} + err = json.Unmarshal(content, state) if err != nil { return nil, nil, err } - return remoteState, remoteContent, nil + return state, content, nil } func (l *statePull) localState(ctx context.Context, b *bundle.Bundle) (*tfState, error) { @@ -60,25 +60,18 @@ func (l *statePull) localState(ctx context.Context, b *bundle.Bundle) (*tfState, return nil, err } - localStatePath := filepath.Join(dir, TerraformStateFileName) - local, err := os.Open(localStatePath) - if err != nil { - return nil, err - } - defer local.Close() - - localContent, err := io.ReadAll(local) + content, err := os.ReadFile(filepath.Join(dir, TerraformStateFileName)) if err != nil { return nil, err } - localState := &tfState{} - err = json.Unmarshal(localContent, localState) + state := &tfState{} + err = json.Unmarshal(content, state) if err != nil { return nil, err } - return localState, nil + return state, nil } func (l *statePull) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { @@ -102,7 +95,7 @@ func (l *statePull) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostic return diag.Errorf("failed to read remote state file: %v", err) } - // Case: Local host does not exist. In this case we should rely on the remote state file. + // Case: Local state file does not exist. In this case we should rely on the remote state file. localState, err := l.localState(ctx, b) if errors.Is(err, fs.ErrNotExist) { log.Infof(ctx, "Local state file does not exist. Using remote terraform state. Invalidating local terraform state.") From 4a29ea53f39d1965d49e5f4e21b65cf956cbcf38 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 15 Jul 2024 14:50:10 +0200 Subject: [PATCH 05/13] correct log statement --- bundle/deploy/terraform/state_pull.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bundle/deploy/terraform/state_pull.go b/bundle/deploy/terraform/state_pull.go index d52ea2e9135..88d78a8b43b 100644 --- a/bundle/deploy/terraform/state_pull.go +++ b/bundle/deploy/terraform/state_pull.go @@ -98,7 +98,7 @@ func (l *statePull) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostic // Case: Local state file does not exist. In this case we should rely on the remote state file. localState, err := l.localState(ctx, b) if errors.Is(err, fs.ErrNotExist) { - log.Infof(ctx, "Local state file does not exist. Using remote terraform state. Invalidating local terraform state.") + log.Infof(ctx, "Local state file does not exist. Using remote terraform state.") err := os.WriteFile(localStatePath, remoteContent, 0600) return diag.FromErr(err) } From 71e493cee01326b9ff55222553b1296af02a4e76 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 15 Jul 2024 19:09:49 +0200 Subject: [PATCH 06/13] error if remote lineage is missing' --- bundle/deploy/terraform/state_pull.go | 5 ++ bundle/deploy/terraform/state_pull_test.go | 59 ++++++++++++---------- 2 files changed, 38 insertions(+), 26 deletions(-) diff --git a/bundle/deploy/terraform/state_pull.go b/bundle/deploy/terraform/state_pull.go index 88d78a8b43b..ccb425524a1 100644 --- a/bundle/deploy/terraform/state_pull.go +++ b/bundle/deploy/terraform/state_pull.go @@ -95,6 +95,11 @@ func (l *statePull) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostic return diag.Errorf("failed to read remote state file: %v", err) } + // Invariant: remote state file has a lineage UUID. + if remoteState.Lineage == "" { + return diag.Errorf("remote state file does not have a lineage") + } + // Case: Local state file does not exist. In this case we should rely on the remote state file. localState, err := l.localState(ctx, b) if errors.Is(err, fs.ErrNotExist) { diff --git a/bundle/deploy/terraform/state_pull_test.go b/bundle/deploy/terraform/state_pull_test.go index 960ecddf0ba..7e16e6275cd 100644 --- a/bundle/deploy/terraform/state_pull_test.go +++ b/bundle/deploy/terraform/state_pull_test.go @@ -41,6 +41,18 @@ func statePullTestBundle(t *testing.T) *bundle.Bundle { } } +func TestStatePullLocalErrorWhenRemoteListFails(t *testing.T) { + m := &statePull{} + + // setup remote state. + m.filerFactory = identityFiler(mockStateFilerForPull(t, map[string]any{"serial": 5}, nil)) + + ctx := context.Background() + b := statePullTestBundle(t) + diags := bundle.Apply(ctx, b, m) + assert.EqualError(t, diags.Error(), "remote state file does not have a lineage") +} + func TestStatePullLocal(t *testing.T) { tcases := []struct { name string @@ -66,30 +78,30 @@ func TestStatePullLocal(t *testing.T) { local: nil, expected: map[string]any{"serial": float64(5)}, }, + { + name: "remote missing, local present", + remote: nil, + local: map[string]any{"serial": 5, "lineage": "aaaa"}, + // local state should be invalidated if remote state is missing. + expected: nil, + }, { name: "local stale", - remote: map[string]any{"serial": 5}, - local: map[string]any{"serial": 4}, - expected: map[string]any{"serial": float64(5)}, + remote: map[string]any{"serial": 10, "lineage": "aaaa", "some_other_key": 123}, + local: map[string]any{"serial": 5, "lineage": "aaaa"}, + expected: map[string]any{"serial": float64(10), "lineage": "aaaa", "some_other_key": float64(123)}, }, { name: "local equal", - remote: map[string]any{"serial": 5, "some_other_key": 123}, - local: map[string]any{"serial": 5}, - expected: map[string]any{"serial": float64(5)}, + remote: map[string]any{"serial": 5, "lineage": "aaaa", "some_other_key": 123}, + local: map[string]any{"serial": 5, "lineage": "aaaa"}, + expected: map[string]any{"serial": float64(5), "lineage": "aaaa"}, }, { name: "local newer", - remote: map[string]any{"serial": 5, "some_other_key": 123}, - local: map[string]any{"serial": 6}, - expected: map[string]any{"serial": float64(6)}, - }, - { - name: "remote missing, local present", - remote: nil, - local: map[string]any{"serial": 5, "some_other_key": 123}, - // local state should be invalidated if remote state is missing. - expected: nil, + remote: map[string]any{"serial": 5, "lineage": "aaaa", "some_other_key": 123}, + local: map[string]any{"serial": 6, "lineage": "aaaa"}, + expected: map[string]any{"serial": float64(6), "lineage": "aaaa"}, }, { name: "remote and local have different lineages", @@ -98,16 +110,11 @@ func TestStatePullLocal(t *testing.T) { expected: map[string]any{"serial": float64(5), "lineage": "aaaa"}, }, { - name: "remote and local have the same lineage, local newer", - remote: map[string]any{"serial": 5, "lineage": "aaaa", "some_other_key": 123}, - local: map[string]any{"serial": 10, "lineage": "aaaa"}, - expected: map[string]any{"serial": float64(10), "lineage": "aaaa"}, - }, - { - name: "remote and local have the same lineage, remote newer", - remote: map[string]any{"serial": 10, "lineage": "aaaa", "some_other_key": 123}, - local: map[string]any{"serial": 5, "lineage": "aaaa"}, - expected: map[string]any{"serial": float64(10), "lineage": "aaaa", "some_other_key": float64(123)}, + name: "local is missing lineage", + remote: map[string]any{"serial": 5, "lineage": "aaaa"}, + local: map[string]any{"serial": 10}, + // use remote, since lineages do not match. + expected: map[string]any{"serial": float64(5), "lineage": "aaaa"}, }, } From 2e037d66a234e5bbedf3e64a01ad78dc6321cebc Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 15 Jul 2024 19:18:27 +0200 Subject: [PATCH 07/13] fix tests --- bundle/deploy/terraform/state_pull_test.go | 36 +++++++++++++--------- 1 file changed, 22 insertions(+), 14 deletions(-) diff --git a/bundle/deploy/terraform/state_pull_test.go b/bundle/deploy/terraform/state_pull_test.go index 7e16e6275cd..aeeb2e5f5ed 100644 --- a/bundle/deploy/terraform/state_pull_test.go +++ b/bundle/deploy/terraform/state_pull_test.go @@ -44,13 +44,27 @@ func statePullTestBundle(t *testing.T) *bundle.Bundle { func TestStatePullLocalErrorWhenRemoteListFails(t *testing.T) { m := &statePull{} - // setup remote state. - m.filerFactory = identityFiler(mockStateFilerForPull(t, map[string]any{"serial": 5}, nil)) - - ctx := context.Background() - b := statePullTestBundle(t) - diags := bundle.Apply(ctx, b, m) - assert.EqualError(t, diags.Error(), "remote state file does not have a lineage") + t.Run("no local state", func(t *testing.T) { + // setup remote state. + m.filerFactory = identityFiler(mockStateFilerForPull(t, map[string]any{"serial": 5}, nil)) + + ctx := context.Background() + b := statePullTestBundle(t) + diags := bundle.Apply(ctx, b, m) + assert.EqualError(t, diags.Error(), "remote state file does not have a lineage") + }) + + t.Run("local state with lineage", func(t *testing.T) { + // setup remote state. + m.filerFactory = identityFiler(mockStateFilerForPull(t, map[string]any{"serial": 5}, nil)) + + ctx := context.Background() + b := statePullTestBundle(t) + writeLocalState(t, ctx, b, map[string]any{"serial": 5, "lineage": "aaaa"}) + + diags := bundle.Apply(ctx, b, m) + assert.EqualError(t, diags.Error(), "remote state file does not have a lineage") + }) } func TestStatePullLocal(t *testing.T) { @@ -67,17 +81,11 @@ func TestStatePullLocal(t *testing.T) { expected map[string]any }{ { - name: "local missing, remote missing", + name: "remote missing, local missing", remote: nil, local: nil, expected: nil, }, - { - name: "local missing, remote present", - remote: map[string]any{"serial": 5}, - local: nil, - expected: map[string]any{"serial": float64(5)}, - }, { name: "remote missing, local present", remote: nil, From 4d40d6d509df3024307ae29326d441be8f98e759 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Wed, 17 Jul 2024 14:42:37 +0200 Subject: [PATCH 08/13] int64 --- bundle/deploy/terraform/state_pull.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bundle/deploy/terraform/state_pull.go b/bundle/deploy/terraform/state_pull.go index ccb425524a1..de6e0db1c99 100644 --- a/bundle/deploy/terraform/state_pull.go +++ b/bundle/deploy/terraform/state_pull.go @@ -16,7 +16,7 @@ import ( ) type tfState struct { - Serial int `json:"serial"` + Serial int64 `json:"serial"` Lineage string `json:"lineage"` } From 3c69bd673e0e61cb7df66969a5d8c7b076eb1d1b Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Wed, 17 Jul 2024 14:43:28 +0200 Subject: [PATCH 09/13] - --- bundle/deploy/terraform/state_pull.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bundle/deploy/terraform/state_pull.go b/bundle/deploy/terraform/state_pull.go index de6e0db1c99..32443e88bc0 100644 --- a/bundle/deploy/terraform/state_pull.go +++ b/bundle/deploy/terraform/state_pull.go @@ -16,7 +16,7 @@ import ( ) type tfState struct { - Serial int64 `json:"serial"` + Serial int64 `json:"serial"` Lineage string `json:"lineage"` } @@ -111,7 +111,7 @@ func (l *statePull) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostic return diag.Errorf("failed to read local state file: %v", err) } - // If the lineages do not match, the terraform state files do not correspond to the same deployment. + // If the lineage does not match, the terraform state files do not correspond to the same deployment. if localState.Lineage != remoteState.Lineage { log.Infof(ctx, "Remote and local state lineages do not match. Using remote terraform state. Invalidating local terraform state.") err := os.WriteFile(localStatePath, remoteContent, 0600) From ab6cf20c78252e023346c19ba0d3e58bb38beae1 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Wed, 17 Jul 2024 14:46:03 +0200 Subject: [PATCH 10/13] cap terraform --- bundle/deploy/terraform/state_pull.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/bundle/deploy/terraform/state_pull.go b/bundle/deploy/terraform/state_pull.go index 32443e88bc0..e12352ab150 100644 --- a/bundle/deploy/terraform/state_pull.go +++ b/bundle/deploy/terraform/state_pull.go @@ -87,7 +87,7 @@ func (l *statePull) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostic // that is the same root_path and workspace host. remoteState, remoteContent, err := l.remoteState(ctx, b) if errors.Is(err, fs.ErrNotExist) { - log.Infof(ctx, "Remote state file does not exist. Invalidating local terraform state.") + log.Infof(ctx, "Remote state file does not exist. Invalidating local Terraform state.") os.Remove(localStatePath) return nil } @@ -103,7 +103,7 @@ func (l *statePull) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostic // Case: Local state file does not exist. In this case we should rely on the remote state file. localState, err := l.localState(ctx, b) if errors.Is(err, fs.ErrNotExist) { - log.Infof(ctx, "Local state file does not exist. Using remote terraform state.") + log.Infof(ctx, "Local state file does not exist. Using remote Terraform state.") err := os.WriteFile(localStatePath, remoteContent, 0600) return diag.FromErr(err) } @@ -111,16 +111,16 @@ func (l *statePull) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostic return diag.Errorf("failed to read local state file: %v", err) } - // If the lineage does not match, the terraform state files do not correspond to the same deployment. + // If the lineage does not match, the Terraform state files do not correspond to the same deployment. if localState.Lineage != remoteState.Lineage { - log.Infof(ctx, "Remote and local state lineages do not match. Using remote terraform state. Invalidating local terraform state.") + log.Infof(ctx, "Remote and local state lineages do not match. Using remote Terraform state. Invalidating local Terraform state.") err := os.WriteFile(localStatePath, remoteContent, 0600) return diag.FromErr(err) } // If the remote state is newer than the local state, we should use the remote state. if remoteState.Serial > localState.Serial { - log.Infof(ctx, "Remote state is newer than local state. Using remote terraform state.") + log.Infof(ctx, "Remote state is newer than local state. Using remote Terraform state.") err := os.WriteFile(localStatePath, remoteContent, 0600) return diag.FromErr(err) } From 25c61a217b27ca38c15f2e44252ebbe0fa338536 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Wed, 17 Jul 2024 14:57:00 +0200 Subject: [PATCH 11/13] modify comments --- bundle/deploy/terraform/state_pull.go | 9 +++--- bundle/deploy/terraform/state_pull_test.go | 36 ++++++++++++---------- 2 files changed, 24 insertions(+), 21 deletions(-) diff --git a/bundle/deploy/terraform/state_pull.go b/bundle/deploy/terraform/state_pull.go index e12352ab150..e88a05792cb 100644 --- a/bundle/deploy/terraform/state_pull.go +++ b/bundle/deploy/terraform/state_pull.go @@ -82,13 +82,12 @@ func (l *statePull) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostic localStatePath := filepath.Join(dir, TerraformStateFileName) - // Case: remote state file does not exist. In this case we should not use the - // local state file because we cannot guarantee it corresponds to the same deployment, - // that is the same root_path and workspace host. + // Case: Remote state file does not exist. In this case we fallback to using the + // local Terraform state. This allows users to change the "root_path" their bundle is + // configured with. remoteState, remoteContent, err := l.remoteState(ctx, b) if errors.Is(err, fs.ErrNotExist) { - log.Infof(ctx, "Remote state file does not exist. Invalidating local Terraform state.") - os.Remove(localStatePath) + log.Infof(ctx, "Remote state file does not exist. Using local Terraform state.") return nil } if err != nil { diff --git a/bundle/deploy/terraform/state_pull_test.go b/bundle/deploy/terraform/state_pull_test.go index aeeb2e5f5ed..39937a3cc2c 100644 --- a/bundle/deploy/terraform/state_pull_test.go +++ b/bundle/deploy/terraform/state_pull_test.go @@ -41,7 +41,7 @@ func statePullTestBundle(t *testing.T) *bundle.Bundle { } } -func TestStatePullLocalErrorWhenRemoteListFails(t *testing.T) { +func TestStatePullLocalErrorWhenRemoteHasNoLineage(t *testing.T) { m := &statePull{} t.Run("no local state", func(t *testing.T) { @@ -90,38 +90,42 @@ func TestStatePullLocal(t *testing.T) { name: "remote missing, local present", remote: nil, local: map[string]any{"serial": 5, "lineage": "aaaa"}, - // local state should be invalidated if remote state is missing. - expected: nil, + // fallback to local state, since remote state is missing. + expected: map[string]any{"serial": float64(5), "lineage": "aaaa"}, }, { - name: "local stale", - remote: map[string]any{"serial": 10, "lineage": "aaaa", "some_other_key": 123}, - local: map[string]any{"serial": 5, "lineage": "aaaa"}, + name: "local stale", + remote: map[string]any{"serial": 10, "lineage": "aaaa", "some_other_key": 123}, + local: map[string]any{"serial": 5, "lineage": "aaaa"}, + // use remote, since remote is newer. expected: map[string]any{"serial": float64(10), "lineage": "aaaa", "some_other_key": float64(123)}, }, { - name: "local equal", - remote: map[string]any{"serial": 5, "lineage": "aaaa", "some_other_key": 123}, - local: map[string]any{"serial": 5, "lineage": "aaaa"}, + name: "local equal", + remote: map[string]any{"serial": 5, "lineage": "aaaa", "some_other_key": 123}, + local: map[string]any{"serial": 5, "lineage": "aaaa"}, + // use local state, since they are equal in terms of serial sequence. expected: map[string]any{"serial": float64(5), "lineage": "aaaa"}, }, { - name: "local newer", - remote: map[string]any{"serial": 5, "lineage": "aaaa", "some_other_key": 123}, - local: map[string]any{"serial": 6, "lineage": "aaaa"}, + name: "local newer", + remote: map[string]any{"serial": 5, "lineage": "aaaa", "some_other_key": 123}, + local: map[string]any{"serial": 6, "lineage": "aaaa"}, + // use local state, since local is newer. expected: map[string]any{"serial": float64(6), "lineage": "aaaa"}, }, { - name: "remote and local have different lineages", - remote: map[string]any{"serial": 5, "lineage": "aaaa"}, - local: map[string]any{"serial": 10, "lineage": "bbbb"}, + name: "remote and local have different lineages", + remote: map[string]any{"serial": 5, "lineage": "aaaa"}, + local: map[string]any{"serial": 10, "lineage": "bbbb"}, + // use remote, since lineages do not match. expected: map[string]any{"serial": float64(5), "lineage": "aaaa"}, }, { name: "local is missing lineage", remote: map[string]any{"serial": 5, "lineage": "aaaa"}, local: map[string]any{"serial": 10}, - // use remote, since lineages do not match. + // use remote, since local does not have lineage. expected: map[string]any{"serial": float64(5), "lineage": "aaaa"}, }, } From 890a294553babb25781feccc37ab13d6759590ba Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Wed, 17 Jul 2024 15:40:52 +0200 Subject: [PATCH 12/13] comments --- bundle/deploy/terraform/state_pull.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/bundle/deploy/terraform/state_pull.go b/bundle/deploy/terraform/state_pull.go index e88a05792cb..d3992176eba 100644 --- a/bundle/deploy/terraform/state_pull.go +++ b/bundle/deploy/terraform/state_pull.go @@ -94,7 +94,8 @@ func (l *statePull) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostic return diag.Errorf("failed to read remote state file: %v", err) } - // Invariant: remote state file has a lineage UUID. + // Expected invariant: remote state file should have a lineage UUID. Error + // if that's not the case. if remoteState.Lineage == "" { return diag.Errorf("remote state file does not have a lineage") } @@ -124,6 +125,8 @@ func (l *statePull) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostic return diag.FromErr(err) } + // default: local state is newer or equal to remote state in terms of serial sequence. + // It is also of the same lineage. Keep using the local state. return nil } From eea82c377de300a99f91ac976945a8313b6dd6f3 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Wed, 17 Jul 2024 15:50:14 +0200 Subject: [PATCH 13/13] fmt --- bundle/deploy/terraform/state_pull.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bundle/deploy/terraform/state_pull.go b/bundle/deploy/terraform/state_pull.go index d3992176eba..9a5b9100766 100644 --- a/bundle/deploy/terraform/state_pull.go +++ b/bundle/deploy/terraform/state_pull.go @@ -126,7 +126,7 @@ func (l *statePull) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostic } // default: local state is newer or equal to remote state in terms of serial sequence. - // It is also of the same lineage. Keep using the local state. + // It is also of the same lineage. Keep using the local state. return nil }