From b3f254e08dcc13e68468b912c27b99623cb34679 Mon Sep 17 00:00:00 2001 From: Nick Fagerlund Date: Wed, 21 Jun 2023 15:40:48 -0700 Subject: [PATCH 01/20] Upgrade to go-tfe v1.29.0 (for saved plan run support) --- go.mod | 2 +- go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index 2b58ea3808..5ba4f27b7d 100644 --- a/go.mod +++ b/go.mod @@ -41,7 +41,7 @@ require ( github.com/hashicorp/go-multierror v1.1.1 github.com/hashicorp/go-plugin v1.4.3 github.com/hashicorp/go-retryablehttp v0.7.4 - github.com/hashicorp/go-tfe v1.28.0 + github.com/hashicorp/go-tfe v1.29.0 github.com/hashicorp/go-uuid v1.0.3 github.com/hashicorp/go-version v1.6.0 github.com/hashicorp/hcl v1.0.0 diff --git a/go.sum b/go.sum index 1363526ebe..642d7d77ed 100644 --- a/go.sum +++ b/go.sum @@ -633,8 +633,8 @@ github.com/hashicorp/go-sockaddr v1.0.0/go.mod h1:7Xibr9yA9JjQq1JpNB2Vw7kxv8xerX github.com/hashicorp/go-sockaddr v1.0.2 h1:ztczhD1jLxIRjVejw8gFomI1BQZOe2WoVOu0SyteCQc= github.com/hashicorp/go-sockaddr v1.0.2/go.mod h1:rB4wwRAUzs07qva3c5SdrY/NEtAUjGlgmH/UkBUC97A= github.com/hashicorp/go-syslog v1.0.0/go.mod h1:qPfqrKkXGihmCqbJM2mZgkZGvKG1dFdvsLplgctolz4= -github.com/hashicorp/go-tfe v1.28.0 h1:YQNfHz5UPMiOD2idad4GCjzG3R2ExPww741PBPqMOIU= -github.com/hashicorp/go-tfe v1.28.0/go.mod h1:z0182DGE/63AKUaWblUVBIrt+xdSmsuuXg5AoxGqDF4= +github.com/hashicorp/go-tfe v1.29.0 h1:hVvgoKtLAWTkXl9p/8WnItCaW65VJwqpjLZkXe8R2AM= +github.com/hashicorp/go-tfe v1.29.0/go.mod h1:z0182DGE/63AKUaWblUVBIrt+xdSmsuuXg5AoxGqDF4= github.com/hashicorp/go-uuid v1.0.0/go.mod h1:6SBZvOh/SIDV7/2o3Jml5SYk/TvGqwFJ/bN7x4byOro= github.com/hashicorp/go-uuid v1.0.1/go.mod h1:6SBZvOh/SIDV7/2o3Jml5SYk/TvGqwFJ/bN7x4byOro= github.com/hashicorp/go-uuid v1.0.2/go.mod h1:6SBZvOh/SIDV7/2o3Jml5SYk/TvGqwFJ/bN7x4byOro= From 717a36036d7abbd9ca9e60ace7b4aee9a3757f7c Mon Sep 17 00:00:00 2001 From: Lauren Date: Wed, 24 May 2023 12:58:43 -0500 Subject: [PATCH 02/20] codify saved plan artifact contents (#33185) --- internal/cloud/backend_saved_plan.go | 38 +++++++++++++++++ internal/cloud/backend_saved_plan_test.go | 42 +++++++++++++++++++ .../testdata/plan-bookmark/bookmark.json | 5 +++ 3 files changed, 85 insertions(+) create mode 100644 internal/cloud/backend_saved_plan.go create mode 100644 internal/cloud/backend_saved_plan_test.go create mode 100644 internal/cloud/testdata/plan-bookmark/bookmark.json diff --git a/internal/cloud/backend_saved_plan.go b/internal/cloud/backend_saved_plan.go new file mode 100644 index 0000000000..f780ee811b --- /dev/null +++ b/internal/cloud/backend_saved_plan.go @@ -0,0 +1,38 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: MPL-2.0 + +package cloud + +import ( + "encoding/json" + "os" +) + +type SavedPlanBookmark struct { + RemotePlanFormat int `json:"remote_plan_format"` + RunID string `json:"run_id"` + Hostname string `json:"hostname"` +} + +func LoadSavedPlanBookmark(filepath string) (SavedPlanBookmark, error) { + bookmark := SavedPlanBookmark{} + data, err := os.ReadFile(filepath) + + if err != nil { + return bookmark, err + } + + err = json.Unmarshal([]byte(data), &bookmark) + return bookmark, err +} + +func (s *SavedPlanBookmark) Save(filepath string) error { + data, _ := json.Marshal(s) + + err := os.WriteFile(filepath, data, 0644) + if err != nil { + return err + } + + return nil +} diff --git a/internal/cloud/backend_saved_plan_test.go b/internal/cloud/backend_saved_plan_test.go new file mode 100644 index 0000000000..14a5d72c9a --- /dev/null +++ b/internal/cloud/backend_saved_plan_test.go @@ -0,0 +1,42 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: MPL-2.0 + +package cloud + +import ( + "path/filepath" + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/zclconf/go-cty/cty" +) + +func TestCloud_loadBasic(t *testing.T) { + bookmark := SavedPlanBookmark{ + RemotePlanFormat: 1, + RunID: "run-GXfuHMkbyHccAGUg", + Hostname: "app.terraform.io", + } + + result, err := LoadSavedPlanBookmark("./testdata/plan-bookmark/bookmark.json") + if err != nil { + t.Fatal(err) + } + + if diff := cmp.Diff(bookmark, result, cmp.Comparer(cty.Value.RawEquals)); diff != "" { + t.Errorf("wrong result\n%s", diff) + } +} + +func TestCloud_saveBasic(t *testing.T) { + tmp := t.TempDir() + bookmarkPath := filepath.Join(tmp, "saved-bookmark.json") + + b := &SavedPlanBookmark{ + RemotePlanFormat: 1, + RunID: "run-GXfuHMkbyHccAGUg", + Hostname: "app.terraform.io", + } + + b.Save(bookmarkPath) +} diff --git a/internal/cloud/testdata/plan-bookmark/bookmark.json b/internal/cloud/testdata/plan-bookmark/bookmark.json new file mode 100644 index 0000000000..0a1c73302a --- /dev/null +++ b/internal/cloud/testdata/plan-bookmark/bookmark.json @@ -0,0 +1,5 @@ +{ + "remote_plan_format": 1, + "run_id": "run-GXfuHMkbyHccAGUg", + "hostname": "app.terraform.io" +} From df7a1c821a971b9db54c8e465d8d60a0217b8d50 Mon Sep 17 00:00:00 2001 From: Nick Fagerlund Date: Thu, 25 May 2023 15:28:58 -0700 Subject: [PATCH 03/20] Move SavedPlanBookmark to dedicated `internal/cloud/cloudplan` package Having this sitting loose in `cloud` proved problematic because of circular import dependencies. Putting it in a sub-package under cloud frees us up to reference the type from places like `internal/backend`! --- .../cloud/{backend_saved_plan.go => cloudplan/saved_plan.go} | 2 +- .../saved_plan_test.go} | 2 +- .../cloud/{ => cloudplan}/testdata/plan-bookmark/bookmark.json | 0 3 files changed, 2 insertions(+), 2 deletions(-) rename internal/cloud/{backend_saved_plan.go => cloudplan/saved_plan.go} (97%) rename internal/cloud/{backend_saved_plan_test.go => cloudplan/saved_plan_test.go} (98%) rename internal/cloud/{ => cloudplan}/testdata/plan-bookmark/bookmark.json (100%) diff --git a/internal/cloud/backend_saved_plan.go b/internal/cloud/cloudplan/saved_plan.go similarity index 97% rename from internal/cloud/backend_saved_plan.go rename to internal/cloud/cloudplan/saved_plan.go index f780ee811b..ab136fe23c 100644 --- a/internal/cloud/backend_saved_plan.go +++ b/internal/cloud/cloudplan/saved_plan.go @@ -1,7 +1,7 @@ // Copyright (c) HashiCorp, Inc. // SPDX-License-Identifier: MPL-2.0 -package cloud +package cloudplan import ( "encoding/json" diff --git a/internal/cloud/backend_saved_plan_test.go b/internal/cloud/cloudplan/saved_plan_test.go similarity index 98% rename from internal/cloud/backend_saved_plan_test.go rename to internal/cloud/cloudplan/saved_plan_test.go index 14a5d72c9a..3813c8b341 100644 --- a/internal/cloud/backend_saved_plan_test.go +++ b/internal/cloud/cloudplan/saved_plan_test.go @@ -1,7 +1,7 @@ // Copyright (c) HashiCorp, Inc. // SPDX-License-Identifier: MPL-2.0 -package cloud +package cloudplan import ( "path/filepath" diff --git a/internal/cloud/testdata/plan-bookmark/bookmark.json b/internal/cloud/cloudplan/testdata/plan-bookmark/bookmark.json similarity index 100% rename from internal/cloud/testdata/plan-bookmark/bookmark.json rename to internal/cloud/cloudplan/testdata/plan-bookmark/bookmark.json From dcccd3b266afb39e7759cfa4f04f528acddc69df Mon Sep 17 00:00:00 2001 From: Lauren Date: Thu, 11 May 2023 17:04:50 -0400 Subject: [PATCH 04/20] Add error handling and tests for saved plan bookmark load/save (#33268) --- internal/cloud/cloudplan/saved_plan.go | 30 +++++++- internal/cloud/cloudplan/saved_plan_test.go | 69 +++++++++++++++++-- .../plan-bookmark/empty_hostname.json | 5 ++ .../testdata/plan-bookmark/empty_run_id.json | 5 ++ .../plan-bookmark/invalid_version.json | 5 ++ .../testdata/plan-bookmark/bookmark.json | 5 ++ 6 files changed, 110 insertions(+), 9 deletions(-) create mode 100644 internal/cloud/cloudplan/testdata/plan-bookmark/empty_hostname.json create mode 100644 internal/cloud/cloudplan/testdata/plan-bookmark/empty_run_id.json create mode 100644 internal/cloud/cloudplan/testdata/plan-bookmark/invalid_version.json create mode 100644 internal/cloud/testdata/plan-bookmark/bookmark.json diff --git a/internal/cloud/cloudplan/saved_plan.go b/internal/cloud/cloudplan/saved_plan.go index ab136fe23c..5bbe29ab96 100644 --- a/internal/cloud/cloudplan/saved_plan.go +++ b/internal/cloud/cloudplan/saved_plan.go @@ -1,13 +1,19 @@ // Copyright (c) HashiCorp, Inc. // SPDX-License-Identifier: MPL-2.0 - package cloudplan import ( "encoding/json" + "errors" + "io" "os" + "strings" ) +var ErrInvalidRemotePlanFormat = errors.New("invalid remote plan format, must be 1") +var ErrInvalidRunID = errors.New("invalid run ID") +var ErrInvalidHostname = errors.New("invalid hostname") + type SavedPlanBookmark struct { RemotePlanFormat int `json:"remote_plan_format"` RunID string `json:"run_id"` @@ -16,13 +22,31 @@ type SavedPlanBookmark struct { func LoadSavedPlanBookmark(filepath string) (SavedPlanBookmark, error) { bookmark := SavedPlanBookmark{} - data, err := os.ReadFile(filepath) + file, err := os.Open(filepath) + if err != nil { + return bookmark, err + } + defer file.Close() + + data, err := io.ReadAll(file) if err != nil { return bookmark, err } - err = json.Unmarshal([]byte(data), &bookmark) + err = json.Unmarshal(data, &bookmark) + if err != nil { + return bookmark, err + } + + if bookmark.RemotePlanFormat != 1 { + return bookmark, ErrInvalidRemotePlanFormat + } else if bookmark.Hostname == "" { + return bookmark, ErrInvalidHostname + } else if bookmark.RunID == "" || !strings.HasPrefix(bookmark.RunID, "run-") { + return bookmark, ErrInvalidRunID + } + return bookmark, err } diff --git a/internal/cloud/cloudplan/saved_plan_test.go b/internal/cloud/cloudplan/saved_plan_test.go index 3813c8b341..f02ccade13 100644 --- a/internal/cloud/cloudplan/saved_plan_test.go +++ b/internal/cloud/cloudplan/saved_plan_test.go @@ -4,6 +4,8 @@ package cloudplan import ( + "errors" + "os" "path/filepath" "testing" @@ -18,7 +20,8 @@ func TestCloud_loadBasic(t *testing.T) { Hostname: "app.terraform.io", } - result, err := LoadSavedPlanBookmark("./testdata/plan-bookmark/bookmark.json") + file := "./testdata/plan-bookmark/bookmark.json" + result, err := LoadSavedPlanBookmark(file) if err != nil { t.Fatal(err) } @@ -28,15 +31,69 @@ func TestCloud_loadBasic(t *testing.T) { } } -func TestCloud_saveBasic(t *testing.T) { - tmp := t.TempDir() - bookmarkPath := filepath.Join(tmp, "saved-bookmark.json") +func TestCloud_loadCheckRunID(t *testing.T) { + // Run ID must never be empty + file := "./testdata/plan-bookmark/empty_run_id.json" + _, err := LoadSavedPlanBookmark(file) + if !errors.Is(err, ErrInvalidRunID) { + t.Fatalf("expected %s but got %s", ErrInvalidRunID, err) + } +} +func TestCloud_loadCheckHostname(t *testing.T) { + // Hostname must never be empty + file := "./testdata/plan-bookmark/empty_hostname.json" + _, err := LoadSavedPlanBookmark(file) + if !errors.Is(err, ErrInvalidHostname) { + t.Fatalf("expected %s but got %s", ErrInvalidHostname, err) + } +} + +func TestCloud_loadCheckVersionNumberBasic(t *testing.T) { + // remote_plan_format must be set to 1 + // remote_plan_format and format version number are used interchangeably + file := "./testdata/plan-bookmark/invalid_version.json" + _, err := LoadSavedPlanBookmark(file) + if !errors.Is(err, ErrInvalidRemotePlanFormat) { + t.Fatalf("expected %s but got %s", ErrInvalidRemotePlanFormat, err) + } +} + +func TestCloud_saveWhenFileExistsBasic(t *testing.T) { + tmpDir := t.TempDir() + tmpFile, err := os.Create(filepath.Join(tmpDir, "saved-bookmark.json")) + if err != nil { + t.Fatal("File could not be created.", err) + } + defer tmpFile.Close() + + // verify the created path exists + // os.Stat() wants path to file + _, error := os.Stat(tmpFile.Name()) + if error != nil { + t.Fatal("Path to file does not exist.", error) + } else { + b := &SavedPlanBookmark{ + RemotePlanFormat: 1, + RunID: "run-GXfuHMkbyHccAGUg", + Hostname: "app.terraform.io", + } + err := b.Save(tmpFile.Name()) + if err != nil { + t.Fatal(err) + } + } +} + +func TestCloud_saveWhenFileDoesNotExistBasic(t *testing.T) { + tmpDir := t.TempDir() b := &SavedPlanBookmark{ RemotePlanFormat: 1, RunID: "run-GXfuHMkbyHccAGUg", Hostname: "app.terraform.io", } - - b.Save(bookmarkPath) + err := b.Save(filepath.Join(tmpDir, "create-new-file.txt")) + if err != nil { + t.Fatal(err) + } } diff --git a/internal/cloud/cloudplan/testdata/plan-bookmark/empty_hostname.json b/internal/cloud/cloudplan/testdata/plan-bookmark/empty_hostname.json new file mode 100644 index 0000000000..990267294f --- /dev/null +++ b/internal/cloud/cloudplan/testdata/plan-bookmark/empty_hostname.json @@ -0,0 +1,5 @@ +{ + "remote_plan_format": 1, + "run_id": "run-GXfuHMkbyHccAGUg", + "hostname": "" +} diff --git a/internal/cloud/cloudplan/testdata/plan-bookmark/empty_run_id.json b/internal/cloud/cloudplan/testdata/plan-bookmark/empty_run_id.json new file mode 100644 index 0000000000..712581aeae --- /dev/null +++ b/internal/cloud/cloudplan/testdata/plan-bookmark/empty_run_id.json @@ -0,0 +1,5 @@ +{ + "remote_plan_format": 1, + "run_id": "", + "hostname": "app.terraform.io" +} diff --git a/internal/cloud/cloudplan/testdata/plan-bookmark/invalid_version.json b/internal/cloud/cloudplan/testdata/plan-bookmark/invalid_version.json new file mode 100644 index 0000000000..59a89d9231 --- /dev/null +++ b/internal/cloud/cloudplan/testdata/plan-bookmark/invalid_version.json @@ -0,0 +1,5 @@ +{ + "remote_plan_format": 11, + "run_id": "run-GXfuHMkbyHccAGUg", + "hostname": "app.terraform.io" +} diff --git a/internal/cloud/testdata/plan-bookmark/bookmark.json b/internal/cloud/testdata/plan-bookmark/bookmark.json new file mode 100644 index 0000000000..0a1c73302a --- /dev/null +++ b/internal/cloud/testdata/plan-bookmark/bookmark.json @@ -0,0 +1,5 @@ +{ + "remote_plan_format": 1, + "run_id": "run-GXfuHMkbyHccAGUg", + "hostname": "app.terraform.io" +} From 9d85f189303cc9105c76cf7ca0ed6233896a9415 Mon Sep 17 00:00:00 2001 From: Nick Fagerlund Date: Wed, 21 Jun 2023 16:07:18 -0700 Subject: [PATCH 05/20] Add NewSavedPlanBookmark function so we don't have to remember the format version number --- internal/cloud/cloudplan/saved_plan.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/internal/cloud/cloudplan/saved_plan.go b/internal/cloud/cloudplan/saved_plan.go index 5bbe29ab96..0827f3a884 100644 --- a/internal/cloud/cloudplan/saved_plan.go +++ b/internal/cloud/cloudplan/saved_plan.go @@ -20,6 +20,14 @@ type SavedPlanBookmark struct { Hostname string `json:"hostname"` } +func NewSavedPlanBookmark(runID, hostname string) SavedPlanBookmark { + return SavedPlanBookmark{ + RemotePlanFormat: 1, + RunID: runID, + Hostname: hostname, + } +} + func LoadSavedPlanBookmark(filepath string) (SavedPlanBookmark, error) { bookmark := SavedPlanBookmark{} From f9d937a4ddde5d36b3699619521889687b0b7d2f Mon Sep 17 00:00:00 2001 From: Nick Fagerlund Date: Tue, 20 Jun 2023 19:06:18 -0700 Subject: [PATCH 06/20] Apply a confirmable run when given a saved cloud plan (#33270) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It displays a run header with link to web UI, like starting a new plan does, then confirms the run and streams the apply logs. If you can't apply the run (it's from a different workspace, is in an unconfirmable state, etc. etc.), it displays an error instead. Notable points along the way: * Implement `WrappedPlanFile` sum type, and update planfile consumers to use it instead of a plain `planfile.Reader`. * Enable applying a saved cloud plan * Update TFC mocks — add org name to workspace, and minimal support for includes on MockRuns.ReadWithOptions. --- internal/backend/backend.go | 2 +- internal/backend/local/backend_local.go | 9 +- internal/backend/local/backend_local_test.go | 37 +++- internal/backend/remote/backend_apply_test.go | 2 +- internal/backend/remote/backend_plan_test.go | 2 +- internal/cloud/backend_apply.go | 199 +++++++++++++----- internal/cloud/backend_apply_test.go | 77 ++++++- internal/cloud/backend_plan_test.go | 2 +- internal/cloud/testing.go | 1 + internal/cloud/tfe_client_mock.go | 21 +- internal/command/apply.go | 37 ++-- internal/command/graph.go | 2 +- internal/command/meta_backend.go | 8 +- internal/command/meta_backend_test.go | 6 +- internal/command/meta_new.go | 7 +- internal/plans/planfile/planfile_test.go | 40 +++- internal/plans/planfile/reader.go | 6 +- .../plans/planfile/testdata/cloudplan.json | 5 + internal/plans/planfile/wrapped.go | 90 ++++++++ 19 files changed, 461 insertions(+), 92 deletions(-) create mode 100644 internal/plans/planfile/testdata/cloudplan.json create mode 100644 internal/plans/planfile/wrapped.go diff --git a/internal/backend/backend.go b/internal/backend/backend.go index cee0f5015d..088e7ef5ec 100644 --- a/internal/backend/backend.go +++ b/internal/backend/backend.go @@ -270,7 +270,7 @@ type Operation struct { // Plan is a plan that was passed as an argument. This is valid for // plan and apply arguments but may not work for all backends. - PlanFile *planfile.Reader + PlanFile *planfile.WrappedPlanFile // The options below are more self-explanatory and affect the runtime // behavior of the operation. diff --git a/internal/backend/local/backend_local.go b/internal/backend/local/backend_local.go index 9bfad4e097..a38d5b033a 100644 --- a/internal/backend/local/backend_local.go +++ b/internal/backend/local/backend_local.go @@ -77,7 +77,12 @@ func (b *Local) localRun(op *backend.Operation) (*backend.LocalRun, *configload. var ctxDiags tfdiags.Diagnostics var configSnap *configload.Snapshot - if op.PlanFile != nil { + if op.PlanFile.IsCloud() { + diags = diags.Append(fmt.Errorf("error: using a saved cloud plan with the local backend is not supported")) + return nil, nil, nil, diags + } + + if lp, ok := op.PlanFile.Local(); ok { var stateMeta *statemgr.SnapshotMeta // If the statemgr implements our optional PersistentMeta interface then we'll // additionally verify that the state snapshot in the plan file has @@ -87,7 +92,7 @@ func (b *Local) localRun(op *backend.Operation) (*backend.LocalRun, *configload. stateMeta = &m } log.Printf("[TRACE] backend/local: populating backend.LocalRun from plan file") - ret, configSnap, ctxDiags = b.localRunForPlanFile(op, op.PlanFile, ret, &coreOpts, stateMeta) + ret, configSnap, ctxDiags = b.localRunForPlanFile(op, lp, ret, &coreOpts, stateMeta) if ctxDiags.HasErrors() { diags = diags.Append(ctxDiags) return nil, nil, nil, diags diff --git a/internal/backend/local/backend_local_test.go b/internal/backend/local/backend_local_test.go index f36097ad2a..82eefb634c 100644 --- a/internal/backend/local/backend_local_test.go +++ b/internal/backend/local/backend_local_test.go @@ -86,6 +86,41 @@ func TestLocalRun_error(t *testing.T) { assertBackendStateUnlocked(t, b) } +func TestLocalRun_cloudPlan(t *testing.T) { + configDir := "./testdata/apply" + b := TestLocal(t) + + _, configLoader, configCleanup := initwd.MustLoadConfigForTests(t, configDir) + defer configCleanup() + + planPath := "../../cloud/cloudplan/testdata/plan-bookmark/bookmark.json" + + planFile, err := planfile.OpenWrapped(planPath) + if err != nil { + t.Fatalf("unexpected error reading planfile: %s", err) + } + + streams, _ := terminal.StreamsForTesting(t) + view := views.NewView(streams) + stateLocker := clistate.NewLocker(0, views.NewStateLocker(arguments.ViewHuman, view)) + + op := &backend.Operation{ + ConfigDir: configDir, + ConfigLoader: configLoader, + PlanFile: planFile, + Workspace: backend.DefaultStateName, + StateLocker: stateLocker, + } + + _, _, diags := b.LocalRun(op) + if !diags.HasErrors() { + t.Fatal("unexpected success") + } + + // LocalRun() unlocks the state on failure + assertBackendStateUnlocked(t, b) +} + func TestLocalRun_stalePlan(t *testing.T) { configDir := "./testdata/apply" b := TestLocal(t) @@ -146,7 +181,7 @@ func TestLocalRun_stalePlan(t *testing.T) { if err := planfile.Create(planPath, planfileArgs); err != nil { t.Fatalf("unexpected error writing planfile: %s", err) } - planFile, err := planfile.Open(planPath) + planFile, err := planfile.OpenWrapped(planPath) if err != nil { t.Fatalf("unexpected error reading planfile: %s", err) } diff --git a/internal/backend/remote/backend_apply_test.go b/internal/backend/remote/backend_apply_test.go index 3b3dcac36c..c6ff31e49a 100644 --- a/internal/backend/remote/backend_apply_test.go +++ b/internal/backend/remote/backend_apply_test.go @@ -264,7 +264,7 @@ func TestRemote_applyWithPlan(t *testing.T) { op, configCleanup, done := testOperationApply(t, "./testdata/apply") defer configCleanup() - op.PlanFile = &planfile.Reader{} + op.PlanFile = planfile.NewWrappedLocal(&planfile.Reader{}) op.Workspace = backend.DefaultStateName run, err := b.Operation(context.Background(), op) diff --git a/internal/backend/remote/backend_plan_test.go b/internal/backend/remote/backend_plan_test.go index 8b95009777..fe18ca8e69 100644 --- a/internal/backend/remote/backend_plan_test.go +++ b/internal/backend/remote/backend_plan_test.go @@ -239,7 +239,7 @@ func TestRemote_planWithPlan(t *testing.T) { op, configCleanup, done := testOperationPlan(t, "./testdata/plan") defer configCleanup() - op.PlanFile = &planfile.Reader{} + op.PlanFile = planfile.NewWrappedLocal(&planfile.Reader{}) op.Workspace = backend.DefaultStateName run, err := b.Operation(context.Background(), op) diff --git a/internal/cloud/backend_apply.go b/internal/cloud/backend_apply.go index 4813d38e0f..d0cb672cfb 100644 --- a/internal/cloud/backend_apply.go +++ b/internal/cloud/backend_apply.go @@ -7,8 +7,10 @@ import ( "bufio" "context" "encoding/json" + "fmt" "io" "log" + "strings" tfe "github.com/hashicorp/go-tfe" "github.com/hashicorp/terraform/internal/backend" @@ -54,12 +56,12 @@ func (b *Cloud) opApply(stopCtx, cancelCtx context.Context, op *backend.Operatio )) } - if op.PlanFile != nil { + if op.PlanFile.IsLocal() { diags = diags.Append(tfdiags.Sourceless( tfdiags.Error, - "Applying a saved plan is currently not supported", - `Terraform Cloud currently requires configuration to be present and `+ - `does not accept an existing saved plan as an argument at this time.`, + "Applying a saved local plan is not supported", + `Terraform Cloud can apply a saved cloud plan, or create a new plan when `+ + `configuration is present. It cannot apply a saved local plan.`, )) } @@ -79,59 +81,107 @@ func (b *Cloud) opApply(stopCtx, cancelCtx context.Context, op *backend.Operatio return nil, diags.Err() } - // Run the plan phase. - r, err := b.plan(stopCtx, cancelCtx, op, w) - if err != nil { - return r, err - } + var r *tfe.Run + var err error - // This check is also performed in the plan method to determine if - // the policies should be checked, but we need to check the values - // here again to determine if we are done and should return. - if !r.HasChanges || r.Status == tfe.RunCanceled || r.Status == tfe.RunErrored { - return r, nil - } - - // Retrieve the run to get its current status. - r, err = b.client.Runs.Read(stopCtx, r.ID) - if err != nil { - return r, generalError("Failed to retrieve run", err) - } - - // Return if the run cannot be confirmed. - if !op.AutoApprove && !r.Actions.IsConfirmable { - return r, nil - } - - mustConfirm := (op.UIIn != nil && op.UIOut != nil) && !op.AutoApprove - - if mustConfirm && b.input { - opts := &terraform.InputOpts{Id: "approve"} - - if op.PlanMode == plans.DestroyMode { - opts.Query = "\nDo you really want to destroy all resources in workspace \"" + op.Workspace + "\"?" - opts.Description = "Terraform will destroy all your managed infrastructure, as shown above.\n" + - "There is no undo. Only 'yes' will be accepted to confirm." - } else { - opts.Query = "\nDo you want to perform these actions in workspace \"" + op.Workspace + "\"?" - opts.Description = "Terraform will perform the actions described above.\n" + - "Only 'yes' will be accepted to approve." + if cp, ok := op.PlanFile.Cloud(); ok { + log.Printf("[TRACE] Loading saved cloud plan for apply") + // Check hostname first, for a more actionable error than a generic 404 later + if cp.Hostname != b.hostname { + diags = diags.Append(tfdiags.Sourceless( + tfdiags.Error, + "Saved plan is for a different hostname", + fmt.Sprintf("The given saved plan refers to a run on %s, but the currently configured Terraform Cloud or Terraform Enterprise instance is %s.", cp.Hostname, b.hostname), + )) + return r, diags.Err() } + // Fetch the run referenced in the saved plan bookmark. + r, err = b.client.Runs.ReadWithOptions(stopCtx, cp.RunID, &tfe.RunReadOptions{ + Include: []tfe.RunIncludeOpt{tfe.RunWorkspace}, + }) - err = b.confirm(stopCtx, op, opts, r, "yes") - if err != nil && err != errRunApproved { + if err != nil { return r, err } - } else if mustConfirm && !b.input { - return r, errApplyNeedsUIConfirmation - } else { - // If we don't need to ask for confirmation, insert a blank - // line to separate the ouputs. + + if r.Workspace.ID != w.ID { + diags = diags.Append(tfdiags.Sourceless( + tfdiags.Error, + "Saved plan is for a different workspace", + fmt.Sprintf("The given saved plan does not refer to a run in the current workspace (%s/%s), so it cannot currently be applied. For more details, view this run in a browser at:\n%s", w.Organization.Name, w.Name, runURL(b.hostname, r.Workspace.Organization.Name, r.Workspace.Name, r.ID)), + )) + return r, diags.Err() + } + + if !r.Actions.IsConfirmable { + url := runURL(b.hostname, b.organization, op.Workspace, r.ID) + return r, unusableSavedPlanError(r.Status, url) + } + + // Since we're not calling plan(), we need to print a run header ourselves: if b.CLI != nil { - b.CLI.Output("") + b.CLI.Output(b.Colorize().Color(strings.TrimSpace(applySavedHeader) + "\n")) + b.CLI.Output(b.Colorize().Color(strings.TrimSpace(fmt.Sprintf( + runHeader, b.hostname, b.organization, r.Workspace.Name, r.ID)) + "\n")) + } + } else { + log.Printf("[TRACE] Running new cloud plan for apply") + // Run the plan phase. + r, err = b.plan(stopCtx, cancelCtx, op, w) + + if err != nil { + return r, err + } + + // This check is also performed in the plan method to determine if + // the policies should be checked, but we need to check the values + // here again to determine if we are done and should return. + if !r.HasChanges || r.Status == tfe.RunCanceled || r.Status == tfe.RunErrored { + return r, nil + } + + // Retrieve the run to get its current status. + r, err = b.client.Runs.Read(stopCtx, r.ID) + if err != nil { + return r, generalError("Failed to retrieve run", err) + } + + // Return if the run cannot be confirmed. + if !op.AutoApprove && !r.Actions.IsConfirmable { + return r, nil + } + + mustConfirm := (op.UIIn != nil && op.UIOut != nil) && !op.AutoApprove + + if mustConfirm && b.input { + opts := &terraform.InputOpts{Id: "approve"} + + if op.PlanMode == plans.DestroyMode { + opts.Query = "\nDo you really want to destroy all resources in workspace \"" + op.Workspace + "\"?" + opts.Description = "Terraform will destroy all your managed infrastructure, as shown above.\n" + + "There is no undo. Only 'yes' will be accepted to confirm." + } else { + opts.Query = "\nDo you want to perform these actions in workspace \"" + op.Workspace + "\"?" + opts.Description = "Terraform will perform the actions described above.\n" + + "Only 'yes' will be accepted to approve." + } + + err = b.confirm(stopCtx, op, opts, r, "yes") + if err != nil && err != errRunApproved { + return r, err + } + } else if mustConfirm && !b.input { + return r, errApplyNeedsUIConfirmation + } else { + // If we don't need to ask for confirmation, insert a blank + // line to separate the ouputs. + if b.CLI != nil { + b.CLI.Output("") + } } } + // Do the apply! if !op.AutoApprove && err != errRunApproved { if err = b.client.Runs.Apply(stopCtx, r.ID, tfe.RunApplyOptions{}); err != nil { return r, generalError("Failed to approve the apply command", err) @@ -222,6 +272,52 @@ func (b *Cloud) renderApplyLogs(ctx context.Context, run *tfe.Run) error { return nil } +func runURL(hostname, orgName, wsName, runID string) string { + return fmt.Sprintf("https://%s/app/%s/%s/runs/%s", hostname, orgName, wsName, runID) +} + +func unusableSavedPlanError(status tfe.RunStatus, url string) error { + var diags tfdiags.Diagnostics + var summary, reason string + + switch status { + case tfe.RunApplied: + summary = "Saved plan is already applied" + reason = "The given plan file was already successfully applied, and cannot be applied again." + case tfe.RunApplying, tfe.RunApplyQueued, tfe.RunConfirmed: + summary = "Saved plan is already confirmed" + reason = "The given plan file is already being applied, and cannot be applied again." + case tfe.RunCanceled: + summary = "Saved plan is canceled" + reason = "The given plan file can no longer be applied because the run was canceled via the Terraform Cloud UI or API." + case tfe.RunDiscarded: + summary = "Saved plan is discarded" + reason = "The given plan file can no longer be applied; either another run was applied first, or a user discarded it via the Terraform Cloud UI or API." + case tfe.RunErrored: + summary = "Saved plan is errored" + reason = "The given plan file refers to a plan that had errors and did not complete successfully. It cannot be applied." + case tfe.RunPlannedAndFinished: + // Note: planned and finished can also indicate a plan-only run, but + // terraform plan can't create a saved plan for a plan-only run, so we + // know it's no-changes in this case. + summary = "Saved plan has no changes" + reason = "The given plan file contains no changes, so it cannot be applied." + case tfe.RunPolicyOverride: + summary = "Saved plan requires policy override" + reason = "The given plan file has soft policy failures, and cannot be applied until a user with appropriate permissions overrides the policy check." + default: + summary = "Saved plan cannot be applied" + reason = "Terraform Cloud cannot apply the given plan file. This may mean the plan and checks have not yet completed, or may indicate another problem." + } + + diags = diags.Append(tfdiags.Sourceless( + tfdiags.Error, + summary, + fmt.Sprintf("%s For more details, view this run in a browser at:\n%s", reason, url), + )) + return diags.Err() +} + const applyDefaultHeader = ` [reset][yellow]Running apply in Terraform Cloud. Output will stream here. Pressing Ctrl-C will cancel the remote apply if it's still pending. If the apply started it @@ -229,3 +325,10 @@ will stop streaming the logs, but will not stop the apply running remotely.[rese Preparing the remote apply... ` + +const applySavedHeader = ` +[reset][yellow]Running apply in Terraform Cloud. Output will stream here. Pressing Ctrl-C +will stop streaming the logs, but will not stop the apply running remotely.[reset] + +Preparing the remote apply... +` diff --git a/internal/cloud/backend_apply_test.go b/internal/cloud/backend_apply_test.go index 32e5258697..3869232d19 100644 --- a/internal/cloud/backend_apply_test.go +++ b/internal/cloud/backend_apply_test.go @@ -22,6 +22,7 @@ import ( "github.com/hashicorp/terraform/internal/addrs" "github.com/hashicorp/terraform/internal/backend" + "github.com/hashicorp/terraform/internal/cloud/cloudplan" "github.com/hashicorp/terraform/internal/command/arguments" "github.com/hashicorp/terraform/internal/command/clistate" "github.com/hashicorp/terraform/internal/command/jsonformat" @@ -407,14 +408,15 @@ func TestCloud_applyWithParallelism(t *testing.T) { } } -func TestCloud_applyWithPlan(t *testing.T) { +// Apply with local plan file should fail. +func TestCloud_applyWithLocalPlan(t *testing.T) { b, bCleanup := testBackendWithName(t) defer bCleanup() op, configCleanup, done := testOperationApply(t, "./testdata/apply") defer configCleanup() - op.PlanFile = &planfile.Reader{} + op.PlanFile = planfile.NewWrappedLocal(&planfile.Reader{}) op.Workspace = testBackendSingleWorkspaceName run, err := b.Operation(context.Background(), op) @@ -432,11 +434,80 @@ func TestCloud_applyWithPlan(t *testing.T) { } errOutput := output.Stderr() - if !strings.Contains(errOutput, "saved plan is currently not supported") { + if !strings.Contains(errOutput, "saved local plan is not supported") { t.Fatalf("expected a saved plan error, got: %v", errOutput) } } +// Apply with bookmark to an existing cloud plan that's in a confirmable state +// should work. +func TestCloud_applyWithCloudPlan(t *testing.T) { + b, bCleanup := testBackendWithName(t) + defer bCleanup() + + op, configCleanup, done := testOperationApply(t, "./testdata/apply-json") + defer configCleanup() + defer done(t) + + op.UIOut = b.CLI + op.Workspace = testBackendSingleWorkspaceName + + mockSROWorkspace(t, b, op.Workspace) + + // Perform the plan before trying to apply it + ws, err := b.client.Workspaces.Read(context.Background(), b.organization, b.WorkspaceMapping.Name) + if err != nil { + t.Fatalf("Couldn't read workspace: %s", err) + } + + planRun, err := b.plan(context.Background(), context.Background(), op, ws) + if err != nil { + t.Fatalf("Couldn't perform plan: %s", err) + } + + // Synthesize a cloud plan file with the plan's run ID + pf := &cloudplan.SavedPlanBookmark{ + RemotePlanFormat: 1, + RunID: planRun.ID, + Hostname: b.hostname, + } + op.PlanFile = planfile.NewWrappedCloud(pf) + + // Start spying on the apply output (now that the plan's done) + stream, close := terminal.StreamsForTesting(t) + + b.renderer = &jsonformat.Renderer{ + Streams: stream, + Colorize: mockColorize(), + } + + // Try apply + run, err := b.Operation(context.Background(), op) + if err != nil { + t.Fatalf("error starting operation: %v", err) + } + + <-run.Done() + output := close(t) + if run.Result != backend.OperationSuccess { + t.Fatal("expected apply operation to succeed") + } + if run.PlanEmpty { + t.Fatalf("expected plan to not be empty") + } + + gotOut := output.Stdout() + if !strings.Contains(gotOut, "1 added, 0 changed, 0 destroyed") { + t.Fatalf("expected apply summary in output: %s", gotOut) + } + + stateMgr, _ := b.StateMgr(testBackendSingleWorkspaceName) + // An error suggests that the state was not unlocked after apply + if _, err := stateMgr.Lock(statemgr.NewLockInfo()); err != nil { + t.Fatalf("unexpected error locking state after apply: %s", err.Error()) + } +} + func TestCloud_applyWithoutRefresh(t *testing.T) { b, bCleanup := testBackendWithName(t) defer bCleanup() diff --git a/internal/cloud/backend_plan_test.go b/internal/cloud/backend_plan_test.go index b75207cb67..f46ed7a34b 100644 --- a/internal/cloud/backend_plan_test.go +++ b/internal/cloud/backend_plan_test.go @@ -337,7 +337,7 @@ func TestCloud_planWithPlan(t *testing.T) { op, configCleanup, done := testOperationPlan(t, "./testdata/plan") defer configCleanup() - op.PlanFile = &planfile.Reader{} + op.PlanFile = planfile.NewWrappedLocal(&planfile.Reader{}) op.Workspace = testBackendSingleWorkspaceName run, err := b.Operation(context.Background(), op) diff --git a/internal/cloud/testing.go b/internal/cloud/testing.go index 81def5c223..992a961de3 100644 --- a/internal/cloud/testing.go +++ b/internal/cloud/testing.go @@ -322,6 +322,7 @@ func testUnconfiguredBackend(t *testing.T) (*Cloud, func()) { b.client.Runs = mc.Runs b.client.RunEvents = mc.RunEvents b.client.StateVersions = mc.StateVersions + b.client.StateVersionOutputs = mc.StateVersionOutputs b.client.Variables = mc.Variables b.client.Workspaces = mc.Workspaces diff --git a/internal/cloud/tfe_client_mock.go b/internal/cloud/tfe_client_mock.go index a67ce3f7e3..02e8280860 100644 --- a/internal/cloud/tfe_client_mock.go +++ b/internal/cloud/tfe_client_mock.go @@ -1085,7 +1085,7 @@ func (m *MockRuns) Read(ctx context.Context, runID string) (*tfe.Run, error) { return m.ReadWithOptions(ctx, runID, nil) } -func (m *MockRuns) ReadWithOptions(ctx context.Context, runID string, _ *tfe.RunReadOptions) (*tfe.Run, error) { +func (m *MockRuns) ReadWithOptions(ctx context.Context, runID string, options *tfe.RunReadOptions) (*tfe.Run, error) { m.Lock() defer m.Unlock() @@ -1136,8 +1136,22 @@ func (m *MockRuns) ReadWithOptions(ctx context.Context, runID string, _ *tfe.Run if err != nil { panic(err) } + r = rc.(*tfe.Run) - return rc.(*tfe.Run), nil + // After copying, handle includes... or at least, any includes we're known to rely on. + if options != nil { + for _, n := range options.Include { + switch n { + case tfe.RunWorkspace: + ws, ok := m.client.Workspaces.workspaceIDs[r.Workspace.ID] + if ok { + r.Workspace = ws + } + } + } + } + + return r, nil } func (m *MockRuns) Apply(ctx context.Context, runID string, options tfe.RunApplyOptions) error { @@ -1534,6 +1548,9 @@ func (m *MockWorkspaces) Create(ctx context.Context, organization string, option CanQueueRun: true, CanForceDelete: tfe.Bool(true), }, + Organization: &tfe.Organization{ + Name: organization, + }, } if options.AutoApply != nil { w.AutoApply = *options.AutoApply diff --git a/internal/command/apply.go b/internal/command/apply.go index c9341d9013..263a2fe72c 100644 --- a/internal/command/apply.go +++ b/internal/command/apply.go @@ -150,8 +150,8 @@ func (c *ApplyCommand) Run(rawArgs []string) int { return 0 } -func (c *ApplyCommand) LoadPlanFile(path string) (*planfile.Reader, tfdiags.Diagnostics) { - var planFile *planfile.Reader +func (c *ApplyCommand) LoadPlanFile(path string) (*planfile.WrappedPlanFile, tfdiags.Diagnostics) { + var planFile *planfile.WrappedPlanFile var diags tfdiags.Diagnostics // Try to load plan if path is specified @@ -194,7 +194,7 @@ func (c *ApplyCommand) LoadPlanFile(path string) (*planfile.Reader, tfdiags.Diag return planFile, diags } -func (c *ApplyCommand) PrepareBackend(planFile *planfile.Reader, args *arguments.State, viewType arguments.ViewType) (backend.Enhanced, tfdiags.Diagnostics) { +func (c *ApplyCommand) PrepareBackend(planFile *planfile.WrappedPlanFile, args *arguments.State, viewType arguments.ViewType) (backend.Enhanced, tfdiags.Diagnostics) { var diags tfdiags.Diagnostics // FIXME: we need to apply the state arguments to the meta object here @@ -206,19 +206,8 @@ func (c *ApplyCommand) PrepareBackend(planFile *planfile.Reader, args *arguments // Load the backend var be backend.Enhanced var beDiags tfdiags.Diagnostics - if planFile == nil { - backendConfig, configDiags := c.loadBackendConfig(".") - diags = diags.Append(configDiags) - if configDiags.HasErrors() { - return nil, diags - } - - be, beDiags = c.Backend(&BackendOpts{ - Config: backendConfig, - ViewType: viewType, - }) - } else { - plan, err := planFile.ReadPlan() + if lp, ok := planFile.Local(); ok { + plan, err := lp.ReadPlan() if err != nil { diags = diags.Append(tfdiags.Sourceless( tfdiags.Error, @@ -236,7 +225,19 @@ func (c *ApplyCommand) PrepareBackend(planFile *planfile.Reader, args *arguments )) return nil, diags } - be, beDiags = c.BackendForPlan(plan.Backend) + be, beDiags = c.BackendForLocalPlan(plan.Backend) + } else { + // Both new plans and saved cloud plans load their backend from config. + backendConfig, configDiags := c.loadBackendConfig(".") + diags = diags.Append(configDiags) + if configDiags.HasErrors() { + return nil, diags + } + + be, beDiags = c.Backend(&BackendOpts{ + Config: backendConfig, + ViewType: viewType, + }) } diags = diags.Append(beDiags) @@ -250,7 +251,7 @@ func (c *ApplyCommand) OperationRequest( be backend.Enhanced, view views.Apply, viewType arguments.ViewType, - planFile *planfile.Reader, + planFile *planfile.WrappedPlanFile, args *arguments.Operation, autoApprove bool, ) (*backend.Operation, tfdiags.Diagnostics) { diff --git a/internal/command/graph.go b/internal/command/graph.go index f83ad0289e..a39bdd0425 100644 --- a/internal/command/graph.go +++ b/internal/command/graph.go @@ -55,7 +55,7 @@ func (c *GraphCommand) Run(args []string) int { } // Try to load plan if path is specified - var planFile *planfile.Reader + var planFile *planfile.WrappedPlanFile if planPath != "" { planFile, err = c.PlanFile(planPath) if err != nil { diff --git a/internal/command/meta_backend.go b/internal/command/meta_backend.go index 2d59a30c83..aaf20e9a7f 100644 --- a/internal/command/meta_backend.go +++ b/internal/command/meta_backend.go @@ -295,13 +295,13 @@ func (m *Meta) selectWorkspace(b backend.Backend) error { return m.SetWorkspace(workspace) } -// BackendForPlan is similar to Backend, but uses backend settings that were +// BackendForLocalPlan is similar to Backend, but uses backend settings that were // stored in a plan. // // The current workspace name is also stored as part of the plan, and so this // method will check that it matches the currently-selected workspace name // and produce error diagnostics if not. -func (m *Meta) BackendForPlan(settings plans.Backend) (backend.Enhanced, tfdiags.Diagnostics) { +func (m *Meta) BackendForLocalPlan(settings plans.Backend) (backend.Enhanced, tfdiags.Diagnostics) { var diags tfdiags.Diagnostics f := backendInit.Backend(settings.Type) @@ -310,7 +310,7 @@ func (m *Meta) BackendForPlan(settings plans.Backend) (backend.Enhanced, tfdiags return nil, diags } b := f() - log.Printf("[TRACE] Meta.BackendForPlan: instantiated backend of type %T", b) + log.Printf("[TRACE] Meta.BackendForLocalPlan: instantiated backend of type %T", b) schema := b.ConfigSchema() configVal, err := settings.Config.Decode(schema.ImpliedType()) @@ -361,7 +361,7 @@ func (m *Meta) BackendForPlan(settings plans.Backend) (backend.Enhanced, tfdiags // Otherwise, we'll wrap our state-only remote backend in the local backend // to cause any operations to be run locally. - log.Printf("[TRACE] Meta.Backend: backend %T does not support operations, so wrapping it in a local backend", b) + log.Printf("[TRACE] Meta.BackendForLocalPlan: backend %T does not support operations, so wrapping it in a local backend", b) cliOpts, err := m.backendCLIOpts() if err != nil { diags = diags.Append(err) diff --git a/internal/command/meta_backend_test.go b/internal/command/meta_backend_test.go index 6a6c94aa90..ee98b362a7 100644 --- a/internal/command/meta_backend_test.go +++ b/internal/command/meta_backend_test.go @@ -1550,7 +1550,7 @@ func TestMetaBackend_planLocal(t *testing.T) { m := testMetaBackend(t, nil) // Get the backend - b, diags := m.BackendForPlan(backendConfig) + b, diags := m.BackendForLocalPlan(backendConfig) if diags.HasErrors() { t.Fatal(diags.Err()) } @@ -1651,7 +1651,7 @@ func TestMetaBackend_planLocalStatePath(t *testing.T) { m.stateOutPath = statePath // Get the backend - b, diags := m.BackendForPlan(plannedBackend) + b, diags := m.BackendForLocalPlan(plannedBackend) if diags.HasErrors() { t.Fatal(diags.Err()) } @@ -1740,7 +1740,7 @@ func TestMetaBackend_planLocalMatch(t *testing.T) { m := testMetaBackend(t, nil) // Get the backend - b, diags := m.BackendForPlan(backendConfig) + b, diags := m.BackendForLocalPlan(backendConfig) if diags.HasErrors() { t.Fatal(diags.Err()) } diff --git a/internal/command/meta_new.go b/internal/command/meta_new.go index 0cb38bfbc9..1bc42ae184 100644 --- a/internal/command/meta_new.go +++ b/internal/command/meta_new.go @@ -27,14 +27,15 @@ func (m *Meta) Input() bool { return true } -// PlanFile returns a reader for the plan file at the given path. +// PlanFile loads the plan file at the given path, which might be either a local +// or cloud plan. // // If the return value and error are both nil, the given path exists but seems // to be a configuration directory instead. // // Error will be non-nil if path refers to something which looks like a plan // file and loading the file fails. -func (m *Meta) PlanFile(path string) (*planfile.Reader, error) { +func (m *Meta) PlanFile(path string) (*planfile.WrappedPlanFile, error) { fi, err := os.Stat(path) if err != nil { return nil, err @@ -45,5 +46,5 @@ func (m *Meta) PlanFile(path string) (*planfile.Reader, error) { return nil, nil } - return planfile.Open(path) + return planfile.OpenWrapped(path) } diff --git a/internal/plans/planfile/planfile_test.go b/internal/plans/planfile/planfile_test.go index dc6e864de3..1faf72dcb8 100644 --- a/internal/plans/planfile/planfile_test.go +++ b/internal/plans/planfile/planfile_test.go @@ -5,6 +5,7 @@ package planfile import ( "path/filepath" + "strings" "testing" "github.com/google/go-cmp/cmp" @@ -100,10 +101,17 @@ func TestRoundtrip(t *testing.T) { t.Fatalf("failed to create plan file: %s", err) } - pr, err := Open(planFn) + wpf, err := OpenWrapped(planFn) if err != nil { t.Fatalf("failed to open plan file for reading: %s", err) } + pr, ok := wpf.Local() + if !ok { + t.Fatalf("failed to open plan file as a local plan file") + } + if wpf.IsCloud() { + t.Fatalf("wrapped plan claims to be both kinds of plan at once") + } t.Run("ReadPlan", func(t *testing.T) { planOut, err := pr.ReadPlan() @@ -167,3 +175,33 @@ func TestRoundtrip(t *testing.T) { } }) } + +func TestWrappedError(t *testing.T) { + // Open something that isn't a cloud or local planfile: should error + wrongFile := "not a valid zip file" + _, err := OpenWrapped(filepath.Join("testdata", "test-config", "root.tf")) + if !strings.Contains(err.Error(), wrongFile) { + t.Fatalf("expected %q, got %q", wrongFile, err) + } + + // Open something that doesn't exist: should error + missingFile := "no such file or directory" + _, err = OpenWrapped(filepath.Join("testdata", "absent.tfplan")) + if !strings.Contains(err.Error(), missingFile) { + t.Fatalf("expected %q, got %q", missingFile, err) + } +} + +func TestWrappedCloud(t *testing.T) { + // Loading valid cloud plan results in a wrapped cloud plan + wpf, err := OpenWrapped(filepath.Join("testdata", "cloudplan.json")) + if err != nil { + t.Fatalf("failed to open valid cloud plan: %s", err) + } + if !wpf.IsCloud() { + t.Fatalf("failed to open cloud file as a cloud plan") + } + if wpf.IsLocal() { + t.Fatalf("wrapped plan claims to be both kinds of plan at once") + } +} diff --git a/internal/plans/planfile/reader.go b/internal/plans/planfile/reader.go index c55de5f398..ad6cbdb9a1 100644 --- a/internal/plans/planfile/reader.go +++ b/internal/plans/planfile/reader.go @@ -31,8 +31,10 @@ type Reader struct { zip *zip.ReadCloser } -// Open creates a Reader for the file at the given filename, or returns an -// error if the file doesn't seem to be a planfile. +// Open creates a Reader for the file at the given filename, or returns an error +// if the file doesn't seem to be a planfile. NOTE: Most commands that accept a +// plan file should use OpenWrapped instead, so they can support both local and +// cloud plan files. func Open(filename string) (*Reader, error) { r, err := zip.OpenReader(filename) if err != nil { diff --git a/internal/plans/planfile/testdata/cloudplan.json b/internal/plans/planfile/testdata/cloudplan.json new file mode 100644 index 0000000000..0a1c73302a --- /dev/null +++ b/internal/plans/planfile/testdata/cloudplan.json @@ -0,0 +1,5 @@ +{ + "remote_plan_format": 1, + "run_id": "run-GXfuHMkbyHccAGUg", + "hostname": "app.terraform.io" +} diff --git a/internal/plans/planfile/wrapped.go b/internal/plans/planfile/wrapped.go new file mode 100644 index 0000000000..5d87a0b425 --- /dev/null +++ b/internal/plans/planfile/wrapped.go @@ -0,0 +1,90 @@ +package planfile + +import ( + "fmt" + + "github.com/hashicorp/terraform/internal/cloud/cloudplan" +) + +// WrappedPlanFile is a sum type that represents a saved plan, loaded from a +// file path passed on the command line. If the specified file was a thick local +// plan file, the Local field will be populated; if it was a bookmark for a +// remote cloud plan, the Cloud field will be populated. In both cases, the +// other field is expected to be nil. Finally, the outer struct is also expected +// to be used as a pointer, so that a nil value can represent the absence of any +// plan file. +type WrappedPlanFile struct { + local *Reader + cloud *cloudplan.SavedPlanBookmark +} + +func (w *WrappedPlanFile) IsLocal() bool { + return w != nil && w.local != nil +} + +func (w *WrappedPlanFile) IsCloud() bool { + return w != nil && w.cloud != nil +} + +// Local checks whether the wrapped value is a local plan file, and returns it if available. +func (w *WrappedPlanFile) Local() (*Reader, bool) { + if w != nil && w.local != nil { + return w.local, true + } else { + return nil, false + } +} + +// Cloud checks whether the wrapped value is a cloud plan file, and returns it if available. +func (w *WrappedPlanFile) Cloud() (*cloudplan.SavedPlanBookmark, bool) { + if w != nil && w.cloud != nil { + return w.cloud, true + } else { + return nil, false + } +} + +// NewWrappedLocal constructs a WrappedPlanFile from an already loaded local +// plan file reader. Most cases should use OpenWrapped to load from disk +// instead. If the provided reader is nil, the returned pointer is nil. +func NewWrappedLocal(l *Reader) *WrappedPlanFile { + if l != nil { + return &WrappedPlanFile{local: l} + } else { + return nil + } +} + +// NewWrappedCloud constructs a WrappedPlanFile from an already loaded cloud +// plan file. Most cases should use OpenWrapped to load from disk +// instead. If the provided plan file is nil, the returned pointer is nil. +func NewWrappedCloud(c *cloudplan.SavedPlanBookmark) *WrappedPlanFile { + if c != nil { + return &WrappedPlanFile{cloud: c} + } else { + return nil + } +} + +// OpenWrapped loads a local or cloud plan file from a specified file path, or +// returns an error if the file doesn't seem to be a plan file of either kind. +// Most consumers should use this and switch behaviors based on the kind of plan +// they expected, rather than directly using Open. +func OpenWrapped(filename string) (*WrappedPlanFile, error) { + // First, try to load it as a local planfile. + local, localErr := Open(filename) + if localErr == nil { + return &WrappedPlanFile{local: local}, nil + } + // Then, try to load it as a cloud plan. + cloud, cloudErr := cloudplan.LoadSavedPlanBookmark(filename) + if cloudErr == nil { + return &WrappedPlanFile{cloud: &cloud}, nil + } + // If neither worked, return both errors. In general we don't care to give + // any advice about how to fix an internal problem in a plan file, since + // both formats are opaque, but we do want to give the user the best chance + // at resolving whatever their problem was. + combinedErr := fmt.Errorf("couldn't load the provided path as either a local plan file (%s) or a saved cloud plan (%s)", localErr, cloudErr) + return nil, combinedErr +} From da963a13b99af2f86c0c01d4adc2c1186f93e6dc Mon Sep 17 00:00:00 2001 From: Nick Fagerlund Date: Wed, 21 Jun 2023 16:29:28 -0700 Subject: [PATCH 07/20] Implement plan -out for Cloud - Don't save errored plans. - Call op.View.PlanNextStep for terraform plan in cloud mode (We never used to show this footer, because we didn't support -out.) - Create non-speculative config version if saving plan - Rewrite TestCloud_planWithPath to expect success! --- internal/cloud/backend_plan.go | 36 ++++++++++++++++++++--------- internal/cloud/backend_plan_test.go | 32 +++++++++++++++++-------- 2 files changed, 48 insertions(+), 20 deletions(-) diff --git a/internal/cloud/backend_plan.go b/internal/cloud/backend_plan.go index b01943d410..13cff76ee2 100644 --- a/internal/cloud/backend_plan.go +++ b/internal/cloud/backend_plan.go @@ -23,6 +23,7 @@ import ( version "github.com/hashicorp/go-version" "github.com/hashicorp/terraform/internal/backend" + "github.com/hashicorp/terraform/internal/cloud/cloudplan" "github.com/hashicorp/terraform/internal/command/jsonformat" "github.com/hashicorp/terraform/internal/configs" "github.com/hashicorp/terraform/internal/genconfig" @@ -65,15 +66,6 @@ func (b *Cloud) opPlan(stopCtx, cancelCtx context.Context, op *backend.Operation )) } - if op.PlanOutPath != "" { - diags = diags.Append(tfdiags.Sourceless( - tfdiags.Error, - "Saving a generated plan is currently not supported", - `Terraform Cloud does not support saving the generated execution `+ - `plan locally at this time.`, - )) - } - if !op.HasConfig() && op.PlanMode != plans.DestroyMode { diags = diags.Append(tfdiags.Sourceless( tfdiags.Error, @@ -95,7 +87,25 @@ func (b *Cloud) opPlan(stopCtx, cancelCtx context.Context, op *backend.Operation return nil, diags.Err() } - return b.plan(stopCtx, cancelCtx, op, w) + // If the run errored, exit before checking whether to save a plan file + run, err := b.plan(stopCtx, cancelCtx, op, w) + if err != nil { + return nil, err + } + + // Save plan file if -out was specified + if op.PlanOutPath != "" { + bookmark := cloudplan.NewSavedPlanBookmark(run.ID, b.hostname) + err = bookmark.Save(op.PlanOutPath) + if err != nil { + return nil, err + } + } + + // Everything succeded, so display next steps + op.View.PlanNextStep(op.PlanOutPath, op.GenerateConfigOut) + + return run, nil } func (b *Cloud) plan(stopCtx, cancelCtx context.Context, op *backend.Operation, w *tfe.Workspace) (*tfe.Run, error) { @@ -107,9 +117,12 @@ func (b *Cloud) plan(stopCtx, cancelCtx context.Context, op *backend.Operation, b.CLI.Output(b.Colorize().Color(strings.TrimSpace(header) + "\n")) } + // Plan-only means they ran terraform plan without -out. + planOnly := op.Type == backend.OperationTypePlan && op.PlanOutPath == "" + configOptions := tfe.ConfigurationVersionCreateOptions{ AutoQueueRuns: tfe.Bool(false), - Speculative: tfe.Bool(op.Type == backend.OperationTypePlan), + Speculative: tfe.Bool(planOnly), } cv, err := b.client.ConfigurationVersions.Create(stopCtx, w.ID, configOptions) @@ -206,6 +219,7 @@ in order to capture the filesystem context the remote workspace expects: Refresh: tfe.Bool(op.PlanRefresh), Workspace: w, AutoApply: tfe.Bool(op.AutoApprove), + SavePlan: tfe.Bool(op.PlanOutPath != ""), } switch op.PlanMode { diff --git a/internal/cloud/backend_plan_test.go b/internal/cloud/backend_plan_test.go index f46ed7a34b..e4d8c0e078 100644 --- a/internal/cloud/backend_plan_test.go +++ b/internal/cloud/backend_plan_test.go @@ -20,6 +20,7 @@ import ( "github.com/hashicorp/terraform/internal/addrs" "github.com/hashicorp/terraform/internal/backend" + "github.com/hashicorp/terraform/internal/cloud/cloudplan" "github.com/hashicorp/terraform/internal/command/arguments" "github.com/hashicorp/terraform/internal/command/clistate" "github.com/hashicorp/terraform/internal/command/jsonformat" @@ -366,8 +367,11 @@ func TestCloud_planWithPath(t *testing.T) { op, configCleanup, done := testOperationPlan(t, "./testdata/plan") defer configCleanup() + defer done(t) - op.PlanOutPath = "./testdata/plan" + tmpDir := t.TempDir() + pfPath := tmpDir + "/plan.tfplan" + op.PlanOutPath = pfPath op.Workspace = testBackendSingleWorkspaceName run, err := b.Operation(context.Background(), op) @@ -376,17 +380,27 @@ func TestCloud_planWithPath(t *testing.T) { } <-run.Done() - output := done(t) - if run.Result == backend.OperationSuccess { - t.Fatal("expected plan operation to fail") + if run.Result != backend.OperationSuccess { + t.Fatalf("operation failed: %s", b.CLI.(*cli.MockUi).ErrorWriter.String()) } - if !run.PlanEmpty { - t.Fatalf("expected plan to be empty") + if run.PlanEmpty { + t.Fatal("expected a non-empty plan") } - errOutput := output.Stderr() - if !strings.Contains(errOutput, "generated plan is currently not supported") { - t.Fatalf("expected a generated plan error, got: %v", errOutput) + output := b.CLI.(*cli.MockUi).OutputWriter.String() + if !strings.Contains(output, "Running plan in Terraform Cloud") { + t.Fatalf("expected TFC header in output: %s", output) + } + if !strings.Contains(output, "1 to add, 0 to change, 0 to destroy") { + t.Fatalf("expected plan summary in output: %s", output) + } + + plan, err := cloudplan.LoadSavedPlanBookmark(pfPath) + if err != nil { + t.Fatalf("error loading cloud plan file: %v", err) + } + if !strings.Contains(plan.RunID, "run-") || plan.Hostname != "app.terraform.io" { + t.Fatalf("unexpected contents in saved cloud plan: %v", plan) } } From 0df3c143bbf1cc62a190824ea9675b8a8f07c9ad Mon Sep 17 00:00:00 2001 From: Nick Fagerlund Date: Fri, 30 Jun 2023 12:24:57 -0700 Subject: [PATCH 08/20] New plans.Quality type for display-relevant facts about a plan This commit replaces the existing jsonformat.PlanRendererOpt type with a new type with identical semantics, located in the plans package. We needed to be able to exchange the facts represented by `jsonformat.PlanRendererOpt` across some package boundaries, but the jsonformat package is implicated in too many dependency chains to be safe for that purpose! So, we had to make a new one. The plans package seems safe to import from all the places that must emit or accept this info, and already contains plans.Mode, which is effectively a sibling of this type. --- internal/command/jsonformat/plan.go | 15 +++++---------- internal/command/jsonformat/renderer.go | 2 +- internal/command/views/operation.go | 6 +++--- internal/command/views/show.go | 6 +++--- internal/plans/quality.go | 20 ++++++++++++++++++++ internal/plans/quality_string.go | 24 ++++++++++++++++++++++++ 6 files changed, 56 insertions(+), 17 deletions(-) create mode 100644 internal/plans/quality.go create mode 100644 internal/plans/quality_string.go diff --git a/internal/command/jsonformat/plan.go b/internal/command/jsonformat/plan.go index cd92e1a44f..7a49ac235b 100644 --- a/internal/command/jsonformat/plan.go +++ b/internal/command/jsonformat/plan.go @@ -19,14 +19,9 @@ import ( "github.com/hashicorp/terraform/internal/plans" ) -type PlanRendererOpt int - const ( detectedDrift string = "drift" proposedChange string = "change" - - Errored PlanRendererOpt = iota - CanNotApply ) type Plan struct { @@ -51,8 +46,8 @@ func (plan Plan) getSchema(change jsonplan.ResourceChange) *jsonprovider.Schema } } -func (plan Plan) renderHuman(renderer Renderer, mode plans.Mode, opts ...PlanRendererOpt) { - checkOpts := func(target PlanRendererOpt) bool { +func (plan Plan) renderHuman(renderer Renderer, mode plans.Mode, opts ...plans.Quality) { + checkOpts := func(target plans.Quality) bool { for _, opt := range opts { if opt == target { return true @@ -102,7 +97,7 @@ func (plan Plan) renderHuman(renderer Renderer, mode plans.Mode, opts ...PlanRen // the plan is "applyable" and, if so, whether it had refresh changes // that we already would've presented above. - if checkOpts(Errored) { + if checkOpts(plans.Errored) { if haveRefreshChanges { renderer.Streams.Print(format.HorizontalRule(renderer.Colorize, renderer.Streams.Stdout.Columns())) renderer.Streams.Println() @@ -143,7 +138,7 @@ func (plan Plan) renderHuman(renderer Renderer, mode plans.Mode, opts ...PlanRen ) if haveRefreshChanges { - if !checkOpts(CanNotApply) { + if !checkOpts(plans.NoChanges) { // In this case, applying this plan will not change any // remote objects but _will_ update the state to match what // we detected during refresh, so we'll reassure the user @@ -210,7 +205,7 @@ func (plan Plan) renderHuman(renderer Renderer, mode plans.Mode, opts ...PlanRen } if len(changes) > 0 { - if checkOpts(Errored) { + if checkOpts(plans.Errored) { renderer.Streams.Printf("\nTerraform planned the following actions, but then encountered a problem:\n") } else { renderer.Streams.Printf("\nTerraform will perform the following actions:\n") diff --git a/internal/command/jsonformat/renderer.go b/internal/command/jsonformat/renderer.go index 6c8c1adcc1..9215e0e0ed 100644 --- a/internal/command/jsonformat/renderer.go +++ b/internal/command/jsonformat/renderer.go @@ -82,7 +82,7 @@ type Renderer struct { RunningInAutomation bool } -func (renderer Renderer) RenderHumanPlan(plan Plan, mode plans.Mode, opts ...PlanRendererOpt) { +func (renderer Renderer) RenderHumanPlan(plan Plan, mode plans.Mode, opts ...plans.Quality) { if incompatibleVersions(jsonplan.FormatVersion, plan.PlanFormatVersion) || incompatibleVersions(jsonprovider.FormatVersion, plan.ProviderFormatVersion) { renderer.Streams.Println(format.WordWrap( renderer.Colorize.Color("\n[bold][red]Warning:[reset][bold] This plan was generated using a different version of Terraform, the diff presented here may be missing representations of recent features."), diff --git a/internal/command/views/operation.go b/internal/command/views/operation.go index d5b5566a0f..a2411d4f4a 100644 --- a/internal/command/views/operation.go +++ b/internal/command/views/operation.go @@ -115,12 +115,12 @@ func (v *OperationHuman) Plan(plan *plans.Plan, schemas *terraform.Schemas) { } // Side load some data that we can't extract from the JSON plan. - var opts []jsonformat.PlanRendererOpt + var opts []plans.Quality if !plan.CanApply() { - opts = append(opts, jsonformat.CanNotApply) + opts = append(opts, plans.NoChanges) } if plan.Errored { - opts = append(opts, jsonformat.Errored) + opts = append(opts, plans.Errored) } renderer.RenderHumanPlan(jplan, plan.UIMode, opts...) diff --git a/internal/command/views/show.go b/internal/command/views/show.go index 23f8737597..add3115798 100644 --- a/internal/command/views/show.go +++ b/internal/command/views/show.go @@ -67,12 +67,12 @@ func (v *ShowHuman) Display(config *configs.Config, plan *plans.Plan, stateFile RelevantAttributes: attrs, } - var opts []jsonformat.PlanRendererOpt + var opts []plans.Quality if !plan.CanApply() { - opts = append(opts, jsonformat.CanNotApply) + opts = append(opts, plans.NoChanges) } if plan.Errored { - opts = append(opts, jsonformat.Errored) + opts = append(opts, plans.Errored) } renderer.RenderHumanPlan(jplan, plan.UIMode, opts...) diff --git a/internal/plans/quality.go b/internal/plans/quality.go new file mode 100644 index 0000000000..95646f369e --- /dev/null +++ b/internal/plans/quality.go @@ -0,0 +1,20 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: MPL-2.0 + +package plans + +// Quality represents facts about the nature of a plan that might be relevant +// when rendering it, like whether it errored or contains no changes. A plan can +// have multiple qualities. +type Quality int + +//go:generate go run golang.org/x/tools/cmd/stringer -type Quality + +const ( + // Errored plans did not successfully complete, and cannot be applied. + Errored Quality = iota + // NoChanges plans won't result in any actions on infrastructure, or any + // semantically meaningful updates to state. They can sometimes still affect + // the format of state if applied. + NoChanges +) diff --git a/internal/plans/quality_string.go b/internal/plans/quality_string.go new file mode 100644 index 0000000000..61a399a1e8 --- /dev/null +++ b/internal/plans/quality_string.go @@ -0,0 +1,24 @@ +// Code generated by "stringer -type Quality"; DO NOT EDIT. + +package plans + +import "strconv" + +func _() { + // An "invalid array index" compiler error signifies that the constant values have changed. + // Re-run the stringer command to generate them again. + var x [1]struct{} + _ = x[Errored-0] + _ = x[NoChanges-1] +} + +const _Quality_name = "ErroredNoChanges" + +var _Quality_index = [...]uint8{0, 7, 16} + +func (i Quality) String() string { + if i < 0 || i >= Quality(len(_Quality_index)-1) { + return "Quality(" + strconv.FormatInt(int64(i), 10) + ")" + } + return _Quality_name[_Quality_index[i]:_Quality_index[i+1]] +} From 2a08a5b46ec44f8c0bd6965e31f126fddfbc8e09 Mon Sep 17 00:00:00 2001 From: Nick Fagerlund Date: Mon, 26 Jun 2023 17:41:24 -0700 Subject: [PATCH 09/20] Cloud: Split private `readRedactedPlan` func into two Since `terraform show -json` needs to get a raw hunk of json bytes and sling it right back out again, it's going to be more convenient if plain `show` can ALSO take in raw json. In order for that to happen, I need a function that basically acts like `client.Plans.ReadJSONOutput()`, without eagerly unmarshalling that `jsonformat.Plan` struct. As a slight bonus, this also lets us make the tfe client mocks slightly stupider. --- internal/cloud/backend_common.go | 22 +++++++++++++++------- internal/cloud/backend_plan.go | 6 +++++- internal/cloud/testing.go | 5 ++--- internal/cloud/tfe_client_mock.go | 18 +++++------------- 4 files changed, 27 insertions(+), 24 deletions(-) diff --git a/internal/cloud/backend_common.go b/internal/cloud/backend_common.go index c4b4ee254f..ee7fd33dd5 100644 --- a/internal/cloud/backend_common.go +++ b/internal/cloud/backend_common.go @@ -5,6 +5,7 @@ package cloud import ( "bufio" + "bytes" "context" "encoding/json" "errors" @@ -550,11 +551,12 @@ func (b *Cloud) confirm(stopCtx context.Context, op *backend.Operation, opts *te return <-result } -// This method will fetch the redacted plan output and marshal the response into -// a struct the jsonformat.Renderer expects. +// This method will fetch the redacted plan output as a byte slice, mirroring +// the behavior of the similar client.Plans.ReadJSONOutput method. // -// Note: Apologies for the lengthy definition, this is a result of not being able to mock receiver methods -var readRedactedPlan func(context.Context, url.URL, string, string) (*jsonformat.Plan, error) = func(ctx context.Context, baseURL url.URL, token string, planID string) (*jsonformat.Plan, error) { +// Note: Apologies for the lengthy definition, this is a result of not being +// able to mock receiver methods +var readRedactedPlan func(context.Context, url.URL, string, string) ([]byte, error) = func(ctx context.Context, baseURL url.URL, token string, planID string) ([]byte, error) { client := retryablehttp.NewClient() client.RetryMax = 10 client.RetryWaitMin = 100 * time.Millisecond @@ -575,7 +577,6 @@ var readRedactedPlan func(context.Context, url.URL, string, string) (*jsonformat req.Header.Set("Authorization", "Bearer "+token) req.Header.Set("Accept", "application/json") - p := &jsonformat.Plan{} resp, err := client.Do(req) if err != nil { return nil, err @@ -586,10 +587,17 @@ var readRedactedPlan func(context.Context, url.URL, string, string) (*jsonformat return nil, err } - if err := json.NewDecoder(resp.Body).Decode(p); err != nil { + return io.ReadAll(resp.Body) +} + +// decodeRedactedPlan unmarshals a downloaded redacted plan into a struct the +// jsonformat.Renderer expects. +func decodeRedactedPlan(jsonBytes []byte) (*jsonformat.Plan, error) { + r := bytes.NewReader(jsonBytes) + p := &jsonformat.Plan{} + if err := json.NewDecoder(r).Decode(p); err != nil { return nil, err } - return p, nil } diff --git a/internal/cloud/backend_plan.go b/internal/cloud/backend_plan.go index 13cff76ee2..562307227d 100644 --- a/internal/cloud/backend_plan.go +++ b/internal/cloud/backend_plan.go @@ -509,10 +509,14 @@ func (b *Cloud) renderPlanLogs(ctx context.Context, op *backend.Operation, run * return err } if renderSRO || shouldGenerateConfig { - redactedPlan, err = readRedactedPlan(ctx, b.client.BaseURL(), b.token, run.Plan.ID) + jsonBytes, err := readRedactedPlan(ctx, b.client.BaseURL(), b.token, run.Plan.ID) if err != nil { return generalError("Failed to read JSON plan", err) } + redactedPlan, err = decodeRedactedPlan(jsonBytes) + if err != nil { + return generalError("Failed to decode JSON plan", err) + } } // Write any generated config before rendering the plan, so we can stop in case of errors diff --git a/internal/cloud/testing.go b/internal/cloud/testing.go index 992a961de3..0f6d89d42e 100644 --- a/internal/cloud/testing.go +++ b/internal/cloud/testing.go @@ -26,7 +26,6 @@ import ( "github.com/zclconf/go-cty/cty" "github.com/hashicorp/terraform/internal/backend" - "github.com/hashicorp/terraform/internal/command/jsonformat" "github.com/hashicorp/terraform/internal/configs" "github.com/hashicorp/terraform/internal/configs/configschema" "github.com/hashicorp/terraform/internal/httpclient" @@ -264,7 +263,7 @@ func testBackend(t *testing.T, obj cty.Value, handlers map[string]func(http.Resp } baseURL.Path = "/api/v2/" - readRedactedPlan = func(ctx context.Context, baseURL url.URL, token, planID string) (*jsonformat.Plan, error) { + readRedactedPlan = func(ctx context.Context, baseURL url.URL, token, planID string) ([]byte, error) { return mc.RedactedPlans.Read(ctx, baseURL.Hostname(), token, planID) } @@ -332,7 +331,7 @@ func testUnconfiguredBackend(t *testing.T) (*Cloud, func()) { } baseURL.Path = "/api/v2/" - readRedactedPlan = func(ctx context.Context, baseURL url.URL, token, planID string) (*jsonformat.Plan, error) { + readRedactedPlan = func(ctx context.Context, baseURL url.URL, token, planID string) ([]byte, error) { return mc.RedactedPlans.Read(ctx, baseURL.Hostname(), token, planID) } diff --git a/internal/cloud/tfe_client_mock.go b/internal/cloud/tfe_client_mock.go index 02e8280860..ecc7a96767 100644 --- a/internal/cloud/tfe_client_mock.go +++ b/internal/cloud/tfe_client_mock.go @@ -7,7 +7,6 @@ import ( "bytes" "context" "encoding/base64" - "encoding/json" "errors" "fmt" "io" @@ -22,7 +21,6 @@ import ( tfe "github.com/hashicorp/go-tfe" "github.com/mitchellh/copystructure" - "github.com/hashicorp/terraform/internal/command/jsonformat" tfversion "github.com/hashicorp/terraform/version" ) @@ -468,13 +466,13 @@ func (m *MockOrganizations) ReadRunQueue(ctx context.Context, name string, optio type MockRedactedPlans struct { client *MockClient - redactedPlans map[string]*jsonformat.Plan + redactedPlans map[string][]byte } func newMockRedactedPlans(client *MockClient) *MockRedactedPlans { return &MockRedactedPlans{ client: client, - redactedPlans: make(map[string]*jsonformat.Plan), + redactedPlans: make(map[string][]byte), } } @@ -495,23 +493,17 @@ func (m *MockRedactedPlans) create(cvID, workspaceID, planID string) error { return err } - raw, err := ioutil.ReadAll(redactedPlanFile) + raw, err := io.ReadAll(redactedPlanFile) if err != nil { return err } - redactedPlan := &jsonformat.Plan{} - err = json.Unmarshal(raw, redactedPlan) - if err != nil { - return err - } - - m.redactedPlans[planID] = redactedPlan + m.redactedPlans[planID] = raw return nil } -func (m *MockRedactedPlans) Read(ctx context.Context, hostname, token, planID string) (*jsonformat.Plan, error) { +func (m *MockRedactedPlans) Read(ctx context.Context, hostname, token, planID string) ([]byte, error) { if p, ok := m.redactedPlans[planID]; ok { return p, nil } From e0af3e25e0d2b2b37b76d4d527da1cdcf2429107 Mon Sep 17 00:00:00 2001 From: Nick Fagerlund Date: Mon, 26 Jun 2023 17:47:43 -0700 Subject: [PATCH 10/20] Add `cloudplan.RemotePlanJSON` wrapper struct for keeping plan metadata together --- internal/cloud/cloudplan/remote_plan_json.go | 39 ++++++++++++++++++++ 1 file changed, 39 insertions(+) create mode 100644 internal/cloud/cloudplan/remote_plan_json.go diff --git a/internal/cloud/cloudplan/remote_plan_json.go b/internal/cloud/cloudplan/remote_plan_json.go new file mode 100644 index 0000000000..9b378eee4f --- /dev/null +++ b/internal/cloud/cloudplan/remote_plan_json.go @@ -0,0 +1,39 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: MPL-2.0 + +package cloudplan + +import ( + "github.com/hashicorp/terraform/internal/plans" +) + +// RemotePlanJSON is a wrapper struct that associates a pre-baked JSON plan with +// several pieces of metadata that can't be derived directly from the JSON +// contents and must instead be discovered from a tfe.Run or tfe.Plan. The +// wrapper is useful for moving data between the Cloud backend (which is the +// only thing able to fetch the JSON and determine values for the metadata) and +// the command.ShowCommand and views.Show interface (which need to have all of +// this information together). +type RemotePlanJSON struct { + // The raw bytes of json we got from the API. + JSONBytes []byte + // Indicates whether the json bytes are the "redacted json plan" format, or + // the unredacted stable "external json plan" format. These formats are + // actually very different under the hood; the redacted one can be decoded + // directly into a jsonformat.Plan struct and is intended for formatting a + // plan for human consumption, while the unredacted one matches what is + // returned by the jsonplan.Marshal() function, cannot be directly decoded + // into a public type (it's actually a jsonplan.plan struct), and will + // generally be spat back out verbatim. + Redacted bool + // Normal/destroy/refresh. Required by (jsonformat.Renderer).RenderHumanPlan. + Mode plans.Mode + // Unchanged/errored. Required by (jsonformat.Renderer).RenderHumanPlan. + Qualities []plans.Quality + // A human-readable header with a link to view the associated run in the + // Terraform Cloud UI. + RunHeader string + // A human-readable footer with information relevant to the likely next + // actions for this plan. + RunFooter string +} From d5938f6b457c9a2bb19d87d6738e011dae93423f Mon Sep 17 00:00:00 2001 From: Nick Fagerlund Date: Mon, 26 Jun 2023 17:50:33 -0700 Subject: [PATCH 11/20] Add `(*Cloud).ShowPlanForRun` method to support `terraform show` To do the "human" version of showing a plan, we need more than just the redacted plan JSON itself; we also need a bunch of extra information that only the Cloud backend is in a position to find out (since it's the only one holding a configured go-tfe client instance). So, this method takes a run ID and hostname, finds out everything we're going to need, and returns it wrapped up in a RemotePlanJSON struct. --- internal/cloud/backend_show.go | 114 +++++++++++++++++++++++++++++++++ 1 file changed, 114 insertions(+) create mode 100644 internal/cloud/backend_show.go diff --git a/internal/cloud/backend_show.go b/internal/cloud/backend_show.go new file mode 100644 index 0000000000..c04cb39092 --- /dev/null +++ b/internal/cloud/backend_show.go @@ -0,0 +1,114 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: MPL-2.0 + +package cloud + +import ( + "context" + "fmt" + "strings" + + tfe "github.com/hashicorp/go-tfe" + "github.com/hashicorp/terraform/internal/cloud/cloudplan" + "github.com/hashicorp/terraform/internal/plans" +) + +// ShowPlanForRun downloads the JSON plan output for the specified cloud run +// (either the redacted or unredacted format, per the caller's request), and +// returns it in a cloudplan.RemotePlanJSON wrapper struct (along with various +// metadata required by terraform show). It's intended for use by the terraform +// show command, in order to format and display a saved cloud plan. +func (b *Cloud) ShowPlanForRun(ctx context.Context, runID, runHostname string, redacted bool) (*cloudplan.RemotePlanJSON, error) { + var jsonBytes []byte + mode := plans.NormalMode + var opts []plans.Quality + + // Bail early if wrong hostname + if runHostname != b.hostname { + return nil, fmt.Errorf("hostname for run (%s) does not match the configured cloud integration (%s)", runHostname, b.hostname) + } + + // Get run and plan + r, err := b.client.Runs.ReadWithOptions(ctx, runID, &tfe.RunReadOptions{Include: []tfe.RunIncludeOpt{tfe.RunPlan, tfe.RunWorkspace}}) + if err == tfe.ErrResourceNotFound { + return nil, fmt.Errorf("couldn't read information for cloud run %s; make sure you've run `terraform login` and that you have permission to view the run", runID) + } else if err != nil { + return nil, fmt.Errorf("couldn't read information for cloud run %s: %w", runID, err) + } + + // Sort out the run mode + if r.IsDestroy { + mode = plans.DestroyMode + } else if r.RefreshOnly { + mode = plans.RefreshOnlyMode + } + + // Check that the plan actually finished + switch r.Plan.Status { + case tfe.PlanErrored: + // Errored plans might still be displayable, but we want to mention it to the renderer. + opts = append(opts, plans.Errored) + case tfe.PlanFinished: + // Good to go, but alert the renderer if it has no changes. + if !r.Plan.HasChanges { + opts = append(opts, plans.NoChanges) + } + default: + // Bail, we can't use this. + err = fmt.Errorf("can't display a cloud plan that is currently %s", r.Plan.Status) + return nil, err + } + + // Fetch the json plan! + if redacted { + jsonBytes, err = readRedactedPlan(ctx, b.client.BaseURL(), b.token, r.Plan.ID) + } else { + jsonBytes, err = b.client.Plans.ReadJSONOutput(ctx, r.Plan.ID) + } + if err == tfe.ErrResourceNotFound { + if redacted { + return nil, fmt.Errorf("couldn't read plan data for cloud run %s; make sure you've run `terraform login` and that you have permission to view the run", runID) + } else { + return nil, fmt.Errorf("couldn't read unredacted JSON plan data for cloud run %s; make sure you've run `terraform login` and that you have admin permissions on the workspace", runID) + } + } else if err != nil { + return nil, fmt.Errorf("couldn't read plan data for cloud run %s: %w", runID, err) + } + + // Format a run header and footer + header := strings.TrimSpace(fmt.Sprintf(runHeader, b.hostname, b.organization, r.Workspace.Name, r.ID)) + footer := strings.TrimSpace(statusFooter(r.Status, r.Actions.IsConfirmable, r.Workspace.Locked)) + + out := &cloudplan.RemotePlanJSON{ + JSONBytes: jsonBytes, + Redacted: redacted, + Mode: mode, + Qualities: opts, + RunHeader: header, + RunFooter: footer, + } + + return out, nil +} + +func statusFooter(status tfe.RunStatus, isConfirmable, locked bool) string { + statusText := strings.ReplaceAll(string(status), "_", " ") + statusColor := "red" + statusNote := "not confirmable" + if isConfirmable { + statusColor = "green" + statusNote = "confirmable" + } + lockedColor := "green" + lockedText := "unlocked" + if locked { + lockedColor = "red" + lockedText = "locked" + } + return fmt.Sprintf(statusFooterText, statusColor, statusText, statusNote, lockedColor, lockedText) +} + +const statusFooterText = ` +[reset][%s]Run status: %s (%s)[reset] +[%s]Workspace is %s[reset] +` From 3a9ce2afb10e17d8e978903b07059c9b4e5bd2ed Mon Sep 17 00:00:00 2001 From: Nick Fagerlund Date: Mon, 26 Jun 2023 17:54:49 -0700 Subject: [PATCH 12/20] Update show command and view to support inspecting cloud plans One funny bit: We need to know the ViewType at the point where we ask the Cloud backend for the plan JSON, because we need to switch between two distinctly different formats for human show vs. `show -json`. I chose to pass that by stashing it on the command struct; passing it as an argument would also work, but one, the argument lists in these nested method calls were getting a little unwieldy, and two, many of these functions had to be receiver methods anyway in order to call methods on Meta. --- internal/command/show.go | 86 +++++++++++++++++++++++++--------- internal/command/views/show.go | 42 ++++++++++++++--- 2 files changed, 100 insertions(+), 28 deletions(-) diff --git a/internal/command/show.go b/internal/command/show.go index 3b83a3cc77..42f23ae0fa 100644 --- a/internal/command/show.go +++ b/internal/command/show.go @@ -4,11 +4,14 @@ package command import ( + "context" "fmt" "os" "strings" "github.com/hashicorp/terraform/internal/backend" + "github.com/hashicorp/terraform/internal/cloud" + "github.com/hashicorp/terraform/internal/cloud/cloudplan" "github.com/hashicorp/terraform/internal/command/arguments" "github.com/hashicorp/terraform/internal/command/views" "github.com/hashicorp/terraform/internal/configs" @@ -24,6 +27,7 @@ import ( // contents of a Terraform plan or state file. type ShowCommand struct { Meta + viewType arguments.ViewType } func (c *ShowCommand) Run(rawArgs []string) int { @@ -38,6 +42,7 @@ func (c *ShowCommand) Run(rawArgs []string) int { c.View.HelpPrompt("show") return 1 } + c.viewType = args.ViewType // Set up view view := views.NewShow(args.ViewType, c.View) @@ -51,7 +56,7 @@ func (c *ShowCommand) Run(rawArgs []string) int { } // Get the data we need to display - plan, stateFile, config, schemas, showDiags := c.show(args.Path) + plan, jsonPlan, stateFile, config, schemas, showDiags := c.show(args.Path) diags = diags.Append(showDiags) if showDiags.HasErrors() { view.Diagnostics(diags) @@ -59,7 +64,7 @@ func (c *ShowCommand) Run(rawArgs []string) int { } // Display the data - return view.Display(config, plan, stateFile, schemas) + return view.Display(config, plan, jsonPlan, stateFile, schemas) } func (c *ShowCommand) Help() string { @@ -83,9 +88,10 @@ func (c *ShowCommand) Synopsis() string { return "Show the current state or a saved plan" } -func (c *ShowCommand) show(path string) (*plans.Plan, *statefile.File, *configs.Config, *terraform.Schemas, tfdiags.Diagnostics) { +func (c *ShowCommand) show(path string) (*plans.Plan, *cloudplan.RemotePlanJSON, *statefile.File, *configs.Config, *terraform.Schemas, tfdiags.Diagnostics) { var diags, showDiags tfdiags.Diagnostics var plan *plans.Plan + var jsonPlan *cloudplan.RemotePlanJSON var stateFile *statefile.File var config *configs.Config var schemas *terraform.Schemas @@ -96,7 +102,7 @@ func (c *ShowCommand) show(path string) (*plans.Plan, *statefile.File, *configs. stateFile, showDiags = c.showFromLatestStateSnapshot() diags = diags.Append(showDiags) if showDiags.HasErrors() { - return plan, stateFile, config, schemas, diags + return plan, jsonPlan, stateFile, config, schemas, diags } } @@ -104,10 +110,10 @@ func (c *ShowCommand) show(path string) (*plans.Plan, *statefile.File, *configs. // so try to load the argument as a plan file first. // If that fails, try to load it as a statefile. if path != "" { - plan, stateFile, config, showDiags = c.showFromPath(path) + plan, jsonPlan, stateFile, config, showDiags = c.showFromPath(path) diags = diags.Append(showDiags) if showDiags.HasErrors() { - return plan, stateFile, config, schemas, diags + return plan, jsonPlan, stateFile, config, schemas, diags } } @@ -115,11 +121,11 @@ func (c *ShowCommand) show(path string) (*plans.Plan, *statefile.File, *configs. if config != nil || stateFile != nil { schemas, diags = c.MaybeGetSchemas(stateFile.State, config) if diags.HasErrors() { - return plan, stateFile, config, schemas, diags + return plan, jsonPlan, stateFile, config, schemas, diags } } - return plan, stateFile, config, schemas, diags + return plan, jsonPlan, stateFile, config, schemas, diags } func (c *ShowCommand) showFromLatestStateSnapshot() (*statefile.File, tfdiags.Diagnostics) { var diags tfdiags.Diagnostics @@ -149,17 +155,19 @@ func (c *ShowCommand) showFromLatestStateSnapshot() (*statefile.File, tfdiags.Di return stateFile, diags } -func (c *ShowCommand) showFromPath(path string) (*plans.Plan, *statefile.File, *configs.Config, tfdiags.Diagnostics) { +func (c *ShowCommand) showFromPath(path string) (*plans.Plan, *cloudplan.RemotePlanJSON, *statefile.File, *configs.Config, tfdiags.Diagnostics) { var diags tfdiags.Diagnostics var planErr, stateErr error var plan *plans.Plan + var jsonPlan *cloudplan.RemotePlanJSON var stateFile *statefile.File var config *configs.Config - // Try to get the plan file and associated data from - // the path argument. If that fails, try to get the - // statefile from the path argument. - plan, stateFile, config, planErr = getPlanFromPath(path) + // Path might be a local plan file, a bookmark to a saved cloud plan, or a + // state file. First, try to get a plan and associated data from a local + // plan file. If that fails, try to get a json plan from the path argument. + // If that fails, try to get the statefile from the path argument. + plan, jsonPlan, stateFile, config, planErr = c.getPlanFromPath(path) if planErr != nil { stateFile, stateErr = getStateFromPath(path) if stateErr != nil { @@ -170,21 +178,57 @@ func (c *ShowCommand) showFromPath(path string) (*plans.Plan, *statefile.File, * fmt.Sprintf("State read error: %s\n\nPlan read error: %s", stateErr, planErr), ), ) - return nil, nil, nil, diags + return nil, nil, nil, nil, diags } } - return plan, stateFile, config, diags + return plan, jsonPlan, stateFile, config, diags } -// getPlanFromPath returns a plan, statefile, and config if the user-supplied -// path points to a plan file. If both plan and error are nil, the path is likely -// a directory. An error could suggest that the given path points to a statefile. -func getPlanFromPath(path string) (*plans.Plan, *statefile.File, *configs.Config, error) { - planReader, err := planfile.Open(path) +// getPlanFromPath returns a plan, json plan, statefile, and config if the +// user-supplied path points to either a local or cloud plan file. Note that +// some of the return values will be nil no matter what; local plan files do not +// yield a json plan, and cloud plans do not yield real plan/state/config +// structs. An error generally suggests that the given path is either a +// directory or a statefile. +func (c *ShowCommand) getPlanFromPath(path string) (*plans.Plan, *cloudplan.RemotePlanJSON, *statefile.File, *configs.Config, error) { + var err error + var plan *plans.Plan + var jsonPlan *cloudplan.RemotePlanJSON + var stateFile *statefile.File + var config *configs.Config + + pf, err := planfile.OpenWrapped(path) if err != nil { - return nil, nil, nil, err + return nil, nil, nil, nil, err } + if lp, ok := pf.Local(); ok { + plan, stateFile, config, err = getDataFromPlanfileReader(lp) + } else if cp, ok := pf.Cloud(); ok { + redacted := c.viewType != arguments.ViewJSON + jsonPlan, err = c.getDataFromCloudPlan(cp, redacted) + } + + return plan, jsonPlan, stateFile, config, err +} + +func (c *ShowCommand) getDataFromCloudPlan(plan *cloudplan.SavedPlanBookmark, redacted bool) (*cloudplan.RemotePlanJSON, error) { + // Set up the backend + b, backendDiags := c.Backend(nil) + if backendDiags.HasErrors() { + return nil, backendDiags.Err() + } + // Cloud plans only work if we're cloud. + cl, ok := b.(*cloud.Cloud) + if !ok { + return nil, fmt.Errorf("can't show a saved cloud plan unless the current root module is connected to Terraform Cloud") + } + + return cl.ShowPlanForRun(context.Background(), plan.RunID, plan.Hostname, redacted) +} + +// getDataFromPlanfileReader returns a plan, statefile, and config, extracted from a local plan file. +func getDataFromPlanfileReader(planReader *planfile.Reader) (*plans.Plan, *statefile.File, *configs.Config, error) { // Get plan plan, err := planReader.ReadPlan() if err != nil { diff --git a/internal/command/views/show.go b/internal/command/views/show.go index add3115798..4c9f1bdae3 100644 --- a/internal/command/views/show.go +++ b/internal/command/views/show.go @@ -4,8 +4,11 @@ package views import ( + "bytes" + "encoding/json" "fmt" + "github.com/hashicorp/terraform/internal/cloud/cloudplan" "github.com/hashicorp/terraform/internal/command/arguments" "github.com/hashicorp/terraform/internal/command/jsonformat" "github.com/hashicorp/terraform/internal/command/jsonplan" @@ -20,7 +23,7 @@ import ( type Show interface { // Display renders the plan, if it is available. If plan is nil, it renders the statefile. - Display(config *configs.Config, plan *plans.Plan, stateFile *statefile.File, schemas *terraform.Schemas) int + Display(config *configs.Config, plan *plans.Plan, planJSON *cloudplan.RemotePlanJSON, stateFile *statefile.File, schemas *terraform.Schemas) int // Diagnostics renders early diagnostics, resulting from argument parsing. Diagnostics(diags tfdiags.Diagnostics) @@ -43,14 +46,31 @@ type ShowHuman struct { var _ Show = (*ShowHuman)(nil) -func (v *ShowHuman) Display(config *configs.Config, plan *plans.Plan, stateFile *statefile.File, schemas *terraform.Schemas) int { +func (v *ShowHuman) Display(config *configs.Config, plan *plans.Plan, planJSON *cloudplan.RemotePlanJSON, stateFile *statefile.File, schemas *terraform.Schemas) int { renderer := jsonformat.Renderer{ Colorize: v.view.colorize, Streams: v.view.streams, RunningInAutomation: v.view.runningInAutomation, } - if plan != nil { + // Prefer to display a pre-built JSON plan, if we got one; then, fall back + // to building one ourselves. + if planJSON != nil { + if !planJSON.Redacted { + v.view.streams.Eprintf("Didn't get renderable JSON plan format for human display") + return 1 + } + // The redacted json plan format can be decoded into a jsonformat.Plan + p := jsonformat.Plan{} + r := bytes.NewReader(planJSON.JSONBytes) + if err := json.NewDecoder(r).Decode(&p); err != nil { + v.view.streams.Eprintf("Couldn't decode renderable JSON plan format: %s", err) + } + + v.view.streams.Print(v.view.colorize.Color(planJSON.RunHeader + "\n")) + renderer.RenderHumanPlan(p, planJSON.Mode, planJSON.Qualities...) + v.view.streams.Print(v.view.colorize.Color("\n" + planJSON.RunFooter + "\n")) + } else if plan != nil { outputs, changed, drift, attrs, err := jsonplan.MarshalForRenderer(plan, schemas) if err != nil { v.view.streams.Eprintf("Failed to marshal plan to json: %s", err) @@ -111,15 +131,23 @@ type ShowJSON struct { var _ Show = (*ShowJSON)(nil) -func (v *ShowJSON) Display(config *configs.Config, plan *plans.Plan, stateFile *statefile.File, schemas *terraform.Schemas) int { - if plan != nil { - jsonPlan, err := jsonplan.Marshal(config, plan, stateFile, schemas) +func (v *ShowJSON) Display(config *configs.Config, plan *plans.Plan, planJSON *cloudplan.RemotePlanJSON, stateFile *statefile.File, schemas *terraform.Schemas) int { + // Prefer to display a pre-built JSON plan, if we got one; then, fall back + // to building one ourselves. + if planJSON != nil { + if planJSON.Redacted { + v.view.streams.Eprintf("Didn't get external JSON plan format") + return 1 + } + v.view.streams.Println(string(planJSON.JSONBytes)) + } else if plan != nil { + planJSON, err := jsonplan.Marshal(config, plan, stateFile, schemas) if err != nil { v.view.streams.Eprintf("Failed to marshal plan to json: %s", err) return 1 } - v.view.streams.Println(string(jsonPlan)) + v.view.streams.Println(string(planJSON)) } else { // It is possible that there is neither state nor a plan. // That's ok, we'll just return an empty object. From 12af8518ffb4a337a1af1a94e3cdb4dbb90359fd Mon Sep 17 00:00:00 2001 From: Nick Fagerlund Date: Mon, 26 Jun 2023 18:00:24 -0700 Subject: [PATCH 13/20] command/show_test: Update expected error text --- internal/command/show_test.go | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/internal/command/show_test.go b/internal/command/show_test.go index b3e3e6116c..c8100cf4a8 100644 --- a/internal/command/show_test.go +++ b/internal/command/show_test.go @@ -201,9 +201,13 @@ func TestShow_argsPlanFileDoesNotExist(t *testing.T) { } got := output.Stderr() - want := `Plan read error: open doesNotExist.tfplan:` - if !strings.Contains(got, want) { - t.Errorf("unexpected output\ngot: %s\nwant:\n%s", got, want) + want1 := `Plan read error: couldn't load the provided path` + want2 := `open doesNotExist.tfplan: no such file or directory` + if !strings.Contains(got, want1) { + t.Errorf("unexpected output\ngot: %s\nwant:\n%s", got, want1) + } + if !strings.Contains(got, want2) { + t.Errorf("unexpected output\ngot: %s\nwant:\n%s", got, want2) } } @@ -256,9 +260,13 @@ func TestShow_json_argsPlanFileDoesNotExist(t *testing.T) { } got := output.Stderr() - want := `Plan read error: open doesNotExist.tfplan:` - if !strings.Contains(got, want) { - t.Errorf("unexpected output\ngot: %s\nwant:\n%s", got, want) + want1 := `Plan read error: couldn't load the provided path` + want2 := `open doesNotExist.tfplan: no such file or directory` + if !strings.Contains(got, want1) { + t.Errorf("unexpected output\ngot: %s\nwant:\n%s", got, want1) + } + if !strings.Contains(got, want2) { + t.Errorf("unexpected output\ngot: %s\nwant:\n%s", got, want2) } } From ed27fa096eb41b731776359a2d7cb184bcd9d624 Mon Sep 17 00:00:00 2001 From: Nick Fagerlund Date: Mon, 26 Jun 2023 18:00:36 -0700 Subject: [PATCH 14/20] command/views/show_test: Update method arguments, add test cases --- internal/command/views/show_test.go | 53 +++++++++++++++++++++++++++-- 1 file changed, 51 insertions(+), 2 deletions(-) diff --git a/internal/command/views/show_test.go b/internal/command/views/show_test.go index 98cf69bbab..366a249b3e 100644 --- a/internal/command/views/show_test.go +++ b/internal/command/views/show_test.go @@ -5,10 +5,12 @@ package views import ( "encoding/json" + "os" "strings" "testing" "github.com/hashicorp/terraform/internal/addrs" + "github.com/hashicorp/terraform/internal/cloud/cloudplan" "github.com/hashicorp/terraform/internal/command/arguments" "github.com/hashicorp/terraform/internal/configs/configschema" "github.com/hashicorp/terraform/internal/initwd" @@ -23,8 +25,14 @@ import ( ) func TestShowHuman(t *testing.T) { + redactedPath := "../../cloud/testdata/plan-json-basic/plan-redacted.json" + redactedPlanJson, err := os.ReadFile(redactedPath) + if err != nil { + t.Fatalf("couldn't read json plan test data at %s for showing a cloud plan. Did the file get moved?", redactedPath) + } testCases := map[string]struct { plan *plans.Plan + jsonPlan *cloudplan.RemotePlanJSON stateFile *statefile.File schemas *terraform.Schemas wantExact bool @@ -33,11 +41,28 @@ func TestShowHuman(t *testing.T) { "plan file": { testPlan(t), nil, + nil, testSchemas(), false, "# test_resource.foo will be created", }, + "cloud plan file": { + nil, + &cloudplan.RemotePlanJSON{ + JSONBytes: redactedPlanJson, + Redacted: true, + Mode: plans.NormalMode, + Qualities: []plans.Quality{}, + RunHeader: "[reset][yellow]To view this run in a browser, visit:\nhttps://app.terraform.io/app/example_org/example_workspace/runs/run-run-bugsBUGSbugsBUGS[reset]", + RunFooter: "[reset][green]Run status: planned and saved (confirmable)[reset]\n[green]Workspace is unlocked[reset]", + }, + nil, + nil, + false, + "# null_resource.foo will be created", + }, "statefile": { + nil, nil, &statefile.File{ Serial: 0, @@ -49,6 +74,7 @@ func TestShowHuman(t *testing.T) { "# test_resource.foo:", }, "empty statefile": { + nil, nil, &statefile.File{ Serial: 0, @@ -63,6 +89,7 @@ func TestShowHuman(t *testing.T) { nil, nil, nil, + nil, true, "No state.\n", }, @@ -74,7 +101,7 @@ func TestShowHuman(t *testing.T) { view.Configure(&arguments.View{NoColor: true}) v := NewShow(arguments.ViewHuman, view) - code := v.Display(nil, testCase.plan, testCase.stateFile, testCase.schemas) + code := v.Display(nil, testCase.plan, testCase.jsonPlan, testCase.stateFile, testCase.schemas) if code != 0 { t.Errorf("expected 0 return code, got %d", code) } @@ -90,15 +117,35 @@ func TestShowHuman(t *testing.T) { } func TestShowJSON(t *testing.T) { + unredactedPath := "../testdata/show-json/basic-create/output.json" + unredactedPlanJson, err := os.ReadFile(unredactedPath) + if err != nil { + t.Fatalf("couldn't read json plan test data at %s for showing a cloud plan. Did the file get moved?", unredactedPath) + } testCases := map[string]struct { plan *plans.Plan + jsonPlan *cloudplan.RemotePlanJSON stateFile *statefile.File }{ "plan file": { testPlan(t), nil, + nil, + }, + "cloud plan file": { + nil, + &cloudplan.RemotePlanJSON{ + JSONBytes: unredactedPlanJson, + Redacted: false, + Mode: plans.NormalMode, + Qualities: []plans.Quality{}, + RunHeader: "[reset][yellow]To view this run in a browser, visit:\nhttps://app.terraform.io/app/example_org/example_workspace/runs/run-run-bugsBUGSbugsBUGS[reset]", + RunFooter: "[reset][green]Run status: planned and saved (confirmable)[reset]\n[green]Workspace is unlocked[reset]", + }, + nil, }, "statefile": { + nil, nil, &statefile.File{ Serial: 0, @@ -107,6 +154,7 @@ func TestShowJSON(t *testing.T) { }, }, "empty statefile": { + nil, nil, &statefile.File{ Serial: 0, @@ -117,6 +165,7 @@ func TestShowJSON(t *testing.T) { "nothing": { nil, nil, + nil, }, } @@ -147,7 +196,7 @@ func TestShowJSON(t *testing.T) { }, } - code := v.Display(config, testCase.plan, testCase.stateFile, schemas) + code := v.Display(config, testCase.plan, testCase.jsonPlan, testCase.stateFile, schemas) if code != 0 { t.Errorf("expected 0 return code, got %d", code) From 7f6b827987f10bb05be3197e9b809090202dfb52 Mon Sep 17 00:00:00 2001 From: Nick Fagerlund Date: Wed, 28 Jun 2023 16:47:08 -0700 Subject: [PATCH 15/20] Modify tfe client mocks to meet some new requirements - Add plausible unredacted plan json for `plan-json-{basic,full}` testdata -- Created by just running the relevant terraform commands locally. - Add plan-json-no-changes testdata -- The unredacted json was organically grown, but I edited the log and redacted json by hand to match what I observed from a real but unrelated planned-and-finished run in TFC. - Add plan-json-basic-no-unredacted testdata -- This mimics a lack of admin permissions, resulting in a 404. - Hook up `MockPlans.ReadJSONOutput` to test fixtures, when present. This method has been implemented for ages, and has had a backing store for unredacted plan json, but has been effectively a no-op since nothing ever fills that backing store. So, when creating a mock plan, make an attempt to read unredacted json and stow it in the mocks on success. - Make it possible to get the entire MockClient for a test backend In order to test some things, I'm going to need to mess with the internal state of runs and plans beyond what the go-tfe client API allows. I could add magic special-casing to the mock API methods, or I could locate the shenanigans next to the test that actually exploits it. The latter seems more comprehensible, but I need access to the full mock client struct in order to mess with its interior. - Fill in some missing expectations around HasChanges when retrieving a run + plan. --- internal/cloud/backend_test.go | 2 +- .../plan-json-basic-no-unredacted/main.tf | 1 + .../plan-redacted.json | 116 +++++++++++++++++ .../plan-json-basic-no-unredacted/plan.log | 3 + .../plan-json-basic/plan-unredacted.json | 1 + .../plan-json-full/plan-unredacted.json | 1 + .../testdata/plan-json-no-changes/main.tf | 1 + .../plan-json-no-changes/plan-redacted.json | 118 ++++++++++++++++++ .../plan-json-no-changes/plan-unredacted.json | 1 + .../testdata/plan-json-no-changes/plan.log | 2 + internal/cloud/testing.go | 18 ++- internal/cloud/tfe_client_mock.go | 20 ++- 12 files changed, 274 insertions(+), 10 deletions(-) create mode 100644 internal/cloud/testdata/plan-json-basic-no-unredacted/main.tf create mode 100644 internal/cloud/testdata/plan-json-basic-no-unredacted/plan-redacted.json create mode 100644 internal/cloud/testdata/plan-json-basic-no-unredacted/plan.log create mode 100644 internal/cloud/testdata/plan-json-basic/plan-unredacted.json create mode 100644 internal/cloud/testdata/plan-json-full/plan-unredacted.json create mode 100644 internal/cloud/testdata/plan-json-no-changes/main.tf create mode 100644 internal/cloud/testdata/plan-json-no-changes/plan-redacted.json create mode 100644 internal/cloud/testdata/plan-json-no-changes/plan-unredacted.json create mode 100644 internal/cloud/testdata/plan-json-no-changes/plan.log diff --git a/internal/cloud/backend_test.go b/internal/cloud/backend_test.go index 8591390033..89847350b2 100644 --- a/internal/cloud/backend_test.go +++ b/internal/cloud/backend_test.go @@ -650,7 +650,7 @@ func TestCloud_setUnavailableTerraformVersion(t *testing.T) { }), }) - b, bCleanup := testBackend(t, config, nil) + b, _, bCleanup := testBackend(t, config, nil) defer bCleanup() // Make sure the workspace doesn't exist yet -- otherwise, we can't test what diff --git a/internal/cloud/testdata/plan-json-basic-no-unredacted/main.tf b/internal/cloud/testdata/plan-json-basic-no-unredacted/main.tf new file mode 100644 index 0000000000..3911a2a9b2 --- /dev/null +++ b/internal/cloud/testdata/plan-json-basic-no-unredacted/main.tf @@ -0,0 +1 @@ +resource "null_resource" "foo" {} diff --git a/internal/cloud/testdata/plan-json-basic-no-unredacted/plan-redacted.json b/internal/cloud/testdata/plan-json-basic-no-unredacted/plan-redacted.json new file mode 100644 index 0000000000..9becf1bcd0 --- /dev/null +++ b/internal/cloud/testdata/plan-json-basic-no-unredacted/plan-redacted.json @@ -0,0 +1,116 @@ +{ + "plan_format_version": "1.1", + "resource_drift": [], + "resource_changes": [ + { + "address": "null_resource.foo", + "mode": "managed", + "type": "null_resource", + "name": "foo", + "provider_name": "registry.terraform.io/hashicorp/null", + "change": { + "actions": [ + "create" + ], + "before": null, + "after": { + "triggers": null + }, + "after_unknown": { + "id": true + }, + "before_sensitive": false, + "after_sensitive": {} + } + } + ], + "relevant_attributes": [], + "output_changes": {}, + "provider_schemas": { + "registry.terraform.io/hashicorp/null": { + "provider": { + "version": 0, + "block": { + "description_kind": "plain" + } + }, + "resource_schemas": { + "null_resource": { + "version": 0, + "block": { + "attributes": { + "id": { + "type": "string", + "description": "This is set to a random value at create time.", + "description_kind": "plain", + "computed": true + }, + "triggers": { + "type": [ + "map", + "string" + ], + "description": "A map of arbitrary strings that, when changed, will force the null resource to be replaced, re-running any associated provisioners.", + "description_kind": "plain", + "optional": true + } + }, + "description": "The `null_resource` resource implements the standard resource lifecycle but takes no further action.\n\nThe `triggers` argument allows specifying an arbitrary set of values that, when changed, will cause the resource to be replaced.", + "description_kind": "plain" + } + } + }, + "data_source_schemas": { + "null_data_source": { + "version": 0, + "block": { + "attributes": { + "has_computed_default": { + "type": "string", + "description": "If set, its literal value will be stored and returned. If not, its value defaults to `\"default\"`. This argument exists primarily for testing and has little practical use.", + "description_kind": "plain", + "optional": true, + "computed": true + }, + "id": { + "type": "string", + "description": "This attribute is only present for some legacy compatibility issues and should not be used. It will be removed in a future version.", + "description_kind": "plain", + "deprecated": true, + "computed": true + }, + "inputs": { + "type": [ + "map", + "string" + ], + "description": "A map of arbitrary strings that is copied into the `outputs` attribute, and accessible directly for interpolation.", + "description_kind": "plain", + "optional": true + }, + "outputs": { + "type": [ + "map", + "string" + ], + "description": "After the data source is \"read\", a copy of the `inputs` map.", + "description_kind": "plain", + "computed": true + }, + "random": { + "type": "string", + "description": "A random value. This is primarily for testing and has little practical use; prefer the [hashicorp/random provider](https://registry.terraform.io/providers/hashicorp/random) for more practical random number use-cases.", + "description_kind": "plain", + "computed": true + } + }, + "description": "The `null_data_source` data source implements the standard data source lifecycle but does not\ninteract with any external APIs.\n\nHistorically, the `null_data_source` was typically used to construct intermediate values to re-use elsewhere in configuration. The\nsame can now be achieved using [locals](https://www.terraform.io/docs/language/values/locals.html).\n", + "description_kind": "plain", + "deprecated": true + } + } + } + } + }, + "provider_format_version": "1.0" +} diff --git a/internal/cloud/testdata/plan-json-basic-no-unredacted/plan.log b/internal/cloud/testdata/plan-json-basic-no-unredacted/plan.log new file mode 100644 index 0000000000..6e7352ed44 --- /dev/null +++ b/internal/cloud/testdata/plan-json-basic-no-unredacted/plan.log @@ -0,0 +1,3 @@ +{"@level":"info","@message":"Terraform 1.3.7","@module":"terraform.ui","@timestamp":"2023-01-19T10:47:27.409143-05:00","terraform":"1.3.7","type":"version","ui":"1.0"} +{"@level":"info","@message":"null_resource.foo: Plan to create","@module":"terraform.ui","@timestamp":"2023-01-19T10:47:27.605841-05:00","change":{"resource":{"addr":"null_resource.foo","module":"","resource":"null_resource.foo","implied_provider":"null","resource_type":"null_resource","resource_name":"foo","resource_key":null},"action":"create"},"type":"planned_change"} +{"@level":"info","@message":"Plan: 1 to add, 0 to change, 0 to destroy.","@module":"terraform.ui","@timestamp":"2023-01-19T10:47:27.605906-05:00","changes":{"add":1,"change":0,"remove":0,"operation":"plan"},"type":"change_summary"} diff --git a/internal/cloud/testdata/plan-json-basic/plan-unredacted.json b/internal/cloud/testdata/plan-json-basic/plan-unredacted.json new file mode 100644 index 0000000000..e840d281f4 --- /dev/null +++ b/internal/cloud/testdata/plan-json-basic/plan-unredacted.json @@ -0,0 +1 @@ +{"format_version":"1.1","terraform_version":"1.4.4","planned_values":{"root_module":{"resources":[{"address":"null_resource.foo","mode":"managed","type":"null_resource","name":"foo","provider_name":"registry.terraform.io/hashicorp/null","schema_version":0,"values":{"triggers":null},"sensitive_values":{}}]}},"resource_changes":[{"address":"null_resource.foo","mode":"managed","type":"null_resource","name":"foo","provider_name":"registry.terraform.io/hashicorp/null","change":{"actions":["create"],"before":null,"after":{"triggers":null},"after_unknown":{"id":true},"before_sensitive":false,"after_sensitive":{}}}],"configuration":{"provider_config":{"null":{"name":"null","full_name":"registry.terraform.io/hashicorp/null"}},"root_module":{"resources":[{"address":"null_resource.foo","mode":"managed","type":"null_resource","name":"foo","provider_config_key":"null","schema_version":0}]}}} diff --git a/internal/cloud/testdata/plan-json-full/plan-unredacted.json b/internal/cloud/testdata/plan-json-full/plan-unredacted.json new file mode 100644 index 0000000000..2bf56bddc0 --- /dev/null +++ b/internal/cloud/testdata/plan-json-full/plan-unredacted.json @@ -0,0 +1 @@ +{"format_version":"1.1","terraform_version":"1.4.4","planned_values":{"root_module":{"resources":[{"address":"tfcoremock_complex_resource.example","mode":"managed","type":"tfcoremock_complex_resource","name":"example","provider_name":"registry.terraform.io/hashicorp/tfcoremock","schema_version":0,"values":{"bool":true,"float":0,"id":"my-complex-resource","integer":0,"list":[{"bool":null,"float":null,"integer":null,"list":null,"map":null,"number":null,"object":null,"set":null,"string":"list.one"},{"bool":null,"float":null,"integer":null,"list":null,"map":null,"number":null,"object":null,"set":null,"string":"list.two"}],"list_block":[{"bool":null,"float":null,"integer":null,"list":null,"list_block":[],"map":null,"number":null,"object":null,"set":null,"set_block":[],"string":"list_block.one"},{"bool":null,"float":null,"integer":null,"list":null,"list_block":[],"map":null,"number":null,"object":null,"set":null,"set_block":[],"string":"list_block.two"},{"bool":null,"float":null,"integer":null,"list":null,"list_block":[],"map":null,"number":null,"object":null,"set":null,"set_block":[],"string":"list_block.three"}],"map":{"one":{"bool":null,"float":null,"integer":null,"list":null,"map":null,"number":null,"object":null,"set":null,"string":"map.one"},"two":{"bool":null,"float":null,"integer":null,"list":null,"map":null,"number":null,"object":null,"set":null,"string":"map.two"}},"number":0,"object":{"bool":null,"float":null,"integer":null,"list":null,"map":null,"number":null,"object":{"bool":null,"float":null,"integer":null,"list":null,"map":null,"number":null,"object":null,"set":null,"string":"nested nested object"},"set":null,"string":"nested object"},"set":[{"bool":null,"float":null,"integer":null,"list":null,"map":null,"number":null,"object":null,"set":null,"string":"set.one"},{"bool":null,"float":null,"integer":null,"list":null,"map":null,"number":null,"object":null,"set":null,"string":"set.two"}],"set_block":[{"bool":null,"float":null,"integer":null,"list":null,"list_block":[],"map":null,"number":null,"object":null,"set":null,"set_block":[],"string":"set_block.one"},{"bool":null,"float":null,"integer":null,"list":null,"list_block":[],"map":null,"number":null,"object":null,"set":null,"set_block":[],"string":"set_block.two"}],"string":"Hello, world!"},"sensitive_values":{"list":[{},{}],"list_block":[{"list_block":[],"set_block":[]},{"list_block":[],"set_block":[]},{"list_block":[],"set_block":[]}],"map":{"one":{},"two":{}},"object":{"object":{}},"set":[{},{}],"set_block":[{"list_block":[],"set_block":[]},{"list_block":[],"set_block":[]}]}},{"address":"tfcoremock_simple_resource.example","mode":"managed","type":"tfcoremock_simple_resource","name":"example","provider_name":"registry.terraform.io/hashicorp/tfcoremock","schema_version":0,"values":{"bool":false,"float":0,"id":"my-simple-resource","integer":0,"number":0,"string":"Hello, world!"},"sensitive_values":{}}]}},"resource_changes":[{"address":"tfcoremock_complex_resource.example","mode":"managed","type":"tfcoremock_complex_resource","name":"example","provider_name":"registry.terraform.io/hashicorp/tfcoremock","change":{"actions":["create"],"before":null,"after":{"bool":true,"float":0,"id":"my-complex-resource","integer":0,"list":[{"bool":null,"float":null,"integer":null,"list":null,"map":null,"number":null,"object":null,"set":null,"string":"list.one"},{"bool":null,"float":null,"integer":null,"list":null,"map":null,"number":null,"object":null,"set":null,"string":"list.two"}],"list_block":[{"bool":null,"float":null,"integer":null,"list":null,"list_block":[],"map":null,"number":null,"object":null,"set":null,"set_block":[],"string":"list_block.one"},{"bool":null,"float":null,"integer":null,"list":null,"list_block":[],"map":null,"number":null,"object":null,"set":null,"set_block":[],"string":"list_block.two"},{"bool":null,"float":null,"integer":null,"list":null,"list_block":[],"map":null,"number":null,"object":null,"set":null,"set_block":[],"string":"list_block.three"}],"map":{"one":{"bool":null,"float":null,"integer":null,"list":null,"map":null,"number":null,"object":null,"set":null,"string":"map.one"},"two":{"bool":null,"float":null,"integer":null,"list":null,"map":null,"number":null,"object":null,"set":null,"string":"map.two"}},"number":0,"object":{"bool":null,"float":null,"integer":null,"list":null,"map":null,"number":null,"object":{"bool":null,"float":null,"integer":null,"list":null,"map":null,"number":null,"object":null,"set":null,"string":"nested nested object"},"set":null,"string":"nested object"},"set":[{"bool":null,"float":null,"integer":null,"list":null,"map":null,"number":null,"object":null,"set":null,"string":"set.one"},{"bool":null,"float":null,"integer":null,"list":null,"map":null,"number":null,"object":null,"set":null,"string":"set.two"}],"set_block":[{"bool":null,"float":null,"integer":null,"list":null,"list_block":[],"map":null,"number":null,"object":null,"set":null,"set_block":[],"string":"set_block.one"},{"bool":null,"float":null,"integer":null,"list":null,"list_block":[],"map":null,"number":null,"object":null,"set":null,"set_block":[],"string":"set_block.two"}],"string":"Hello, world!"},"after_unknown":{},"before_sensitive":false,"after_sensitive":{"list":[{},{}],"list_block":[{"list_block":[],"set_block":[]},{"list_block":[],"set_block":[]},{"list_block":[],"set_block":[]}],"map":{"one":{},"two":{}},"object":{"object":{}},"set":[{},{}],"set_block":[{"list_block":[],"set_block":[]},{"list_block":[],"set_block":[]}]}}},{"address":"tfcoremock_simple_resource.example","mode":"managed","type":"tfcoremock_simple_resource","name":"example","provider_name":"registry.terraform.io/hashicorp/tfcoremock","change":{"actions":["create"],"before":null,"after":{"bool":false,"float":0,"id":"my-simple-resource","integer":0,"number":0,"string":"Hello, world!"},"after_unknown":{},"before_sensitive":false,"after_sensitive":{}}}],"configuration":{"provider_config":{"tfcoremock":{"name":"tfcoremock","full_name":"registry.terraform.io/hashicorp/tfcoremock"}},"root_module":{"resources":[{"address":"tfcoremock_complex_resource.example","mode":"managed","type":"tfcoremock_complex_resource","name":"example","provider_config_key":"tfcoremock","expressions":{"bool":{"constant_value":true},"float":{"constant_value":0},"id":{"constant_value":"my-complex-resource"},"integer":{"constant_value":0},"list":{"constant_value":[{"string":"list.one"},{"string":"list.two"}]},"list_block":[{"string":{"constant_value":"list_block.one"}},{"string":{"constant_value":"list_block.two"}},{"string":{"constant_value":"list_block.three"}}],"map":{"constant_value":{"one":{"string":"map.one"},"two":{"string":"map.two"}}},"number":{"constant_value":0},"object":{"constant_value":{"object":{"string":"nested nested object"},"string":"nested object"}},"set":{"constant_value":[{"string":"set.one"},{"string":"set.two"}]},"set_block":[{"string":{"constant_value":"set_block.one"}},{"string":{"constant_value":"set_block.two"}}],"string":{"constant_value":"Hello, world!"}},"schema_version":0},{"address":"tfcoremock_simple_resource.example","mode":"managed","type":"tfcoremock_simple_resource","name":"example","provider_config_key":"tfcoremock","expressions":{"bool":{"constant_value":false},"float":{"constant_value":0},"id":{"constant_value":"my-simple-resource"},"integer":{"constant_value":0},"number":{"constant_value":0},"string":{"constant_value":"Hello, world!"}},"schema_version":0}]}}} diff --git a/internal/cloud/testdata/plan-json-no-changes/main.tf b/internal/cloud/testdata/plan-json-no-changes/main.tf new file mode 100644 index 0000000000..3911a2a9b2 --- /dev/null +++ b/internal/cloud/testdata/plan-json-no-changes/main.tf @@ -0,0 +1 @@ +resource "null_resource" "foo" {} diff --git a/internal/cloud/testdata/plan-json-no-changes/plan-redacted.json b/internal/cloud/testdata/plan-json-no-changes/plan-redacted.json new file mode 100644 index 0000000000..b945f22534 --- /dev/null +++ b/internal/cloud/testdata/plan-json-no-changes/plan-redacted.json @@ -0,0 +1,118 @@ +{ + "plan_format_version": "1.1", + "resource_drift": [], + "resource_changes": [ + { + "address": "null_resource.foo", + "mode": "managed", + "type": "null_resource", + "name": "foo", + "provider_name": "registry.terraform.io/hashicorp/null", + "change": { + "actions": [ + "no-op" + ], + "before": { + "id": "3549869958859575216", + "triggers": null + }, + "after": { + "id": "3549869958859575216", + "triggers": null + }, + "after_unknown": {}, + "before_sensitive": {}, + "after_sensitive": {} + } + } + ], + "relevant_attributes": [], + "output_changes": {}, + "provider_schemas": { + "registry.terraform.io/hashicorp/null": { + "provider": { + "version": 0, + "block": { + "description_kind": "plain" + } + }, + "resource_schemas": { + "null_resource": { + "version": 0, + "block": { + "attributes": { + "id": { + "type": "string", + "description": "This is set to a random value at create time.", + "description_kind": "plain", + "computed": true + }, + "triggers": { + "type": [ + "map", + "string" + ], + "description": "A map of arbitrary strings that, when changed, will force the null resource to be replaced, re-running any associated provisioners.", + "description_kind": "plain", + "optional": true + } + }, + "description": "The `null_resource` resource implements the standard resource lifecycle but takes no further action.\n\nThe `triggers` argument allows specifying an arbitrary set of values that, when changed, will cause the resource to be replaced.", + "description_kind": "plain" + } + } + }, + "data_source_schemas": { + "null_data_source": { + "version": 0, + "block": { + "attributes": { + "has_computed_default": { + "type": "string", + "description": "If set, its literal value will be stored and returned. If not, its value defaults to `\"default\"`. This argument exists primarily for testing and has little practical use.", + "description_kind": "plain", + "optional": true, + "computed": true + }, + "id": { + "type": "string", + "description": "This attribute is only present for some legacy compatibility issues and should not be used. It will be removed in a future version.", + "description_kind": "plain", + "deprecated": true, + "computed": true + }, + "inputs": { + "type": [ + "map", + "string" + ], + "description": "A map of arbitrary strings that is copied into the `outputs` attribute, and accessible directly for interpolation.", + "description_kind": "plain", + "optional": true + }, + "outputs": { + "type": [ + "map", + "string" + ], + "description": "After the data source is \"read\", a copy of the `inputs` map.", + "description_kind": "plain", + "computed": true + }, + "random": { + "type": "string", + "description": "A random value. This is primarily for testing and has little practical use; prefer the [hashicorp/random provider](https://registry.terraform.io/providers/hashicorp/random) for more practical random number use-cases.", + "description_kind": "plain", + "computed": true + } + }, + "description": "The `null_data_source` data source implements the standard data source lifecycle but does not\ninteract with any external APIs.\n\nHistorically, the `null_data_source` was typically used to construct intermediate values to re-use elsewhere in configuration. The\nsame can now be achieved using [locals](https://www.terraform.io/docs/language/values/locals.html).\n", + "description_kind": "plain", + "deprecated": true + } + } + } + } + }, + "provider_format_version": "1.0" +} diff --git a/internal/cloud/testdata/plan-json-no-changes/plan-unredacted.json b/internal/cloud/testdata/plan-json-no-changes/plan-unredacted.json new file mode 100644 index 0000000000..259533f0ce --- /dev/null +++ b/internal/cloud/testdata/plan-json-no-changes/plan-unredacted.json @@ -0,0 +1 @@ +{"format_version":"1.1","terraform_version":"1.4.4","planned_values":{"root_module":{"resources":[{"address":"null_resource.foo","mode":"managed","type":"null_resource","name":"foo","provider_name":"registry.terraform.io/hashicorp/null","schema_version":0,"values":{"id":"3549869958859575216","triggers":null},"sensitive_values":{}}]}},"resource_changes":[{"address":"null_resource.foo","mode":"managed","type":"null_resource","name":"foo","provider_name":"registry.terraform.io/hashicorp/null","change":{"actions":["no-op"],"before":{"id":"3549869958859575216","triggers":null},"after":{"id":"3549869958859575216","triggers":null},"after_unknown":{},"before_sensitive":{},"after_sensitive":{}}}],"prior_state":{"format_version":"1.0","terraform_version":"1.4.4","values":{"root_module":{"resources":[{"address":"null_resource.foo","mode":"managed","type":"null_resource","name":"foo","provider_name":"registry.terraform.io/hashicorp/null","schema_version":0,"values":{"id":"3549869958859575216","triggers":null},"sensitive_values":{}}]}}},"configuration":{"provider_config":{"null":{"name":"null","full_name":"registry.terraform.io/hashicorp/null"}},"root_module":{"resources":[{"address":"null_resource.foo","mode":"managed","type":"null_resource","name":"foo","provider_config_key":"null","schema_version":0}]}}} diff --git a/internal/cloud/testdata/plan-json-no-changes/plan.log b/internal/cloud/testdata/plan-json-no-changes/plan.log new file mode 100644 index 0000000000..7b10d42020 --- /dev/null +++ b/internal/cloud/testdata/plan-json-no-changes/plan.log @@ -0,0 +1,2 @@ +{"@level":"info","@message":"Terraform 1.3.7","@module":"terraform.ui","@timestamp":"2023-01-19T10:47:27.409143-05:00","terraform":"1.3.7","type":"version","ui":"1.0"} +{"@level":"info","@message":"Plan: 0 to add, 0 to change, 0 to destroy.","@module":"terraform.ui","@timestamp":"2023-01-19T10:47:27.605906-05:00","changes":{"add":0,"change":0,"remove":0,"operation":"plan"},"type":"change_summary"} diff --git a/internal/cloud/testing.go b/internal/cloud/testing.go index 0f6d89d42e..6752e7e97d 100644 --- a/internal/cloud/testing.go +++ b/internal/cloud/testing.go @@ -83,6 +83,11 @@ func testInput(t *testing.T, answers map[string]string) *mockInput { } func testBackendWithName(t *testing.T) (*Cloud, func()) { + b, _, c := testBackendAndMocksWithName(t) + return b, c +} + +func testBackendAndMocksWithName(t *testing.T) (*Cloud, *MockClient, func()) { obj := cty.ObjectVal(map[string]cty.Value{ "hostname": cty.NullVal(cty.String), "organization": cty.StringVal("hashicorp"), @@ -109,7 +114,8 @@ func testBackendWithTags(t *testing.T) (*Cloud, func()) { ), }), }) - return testBackend(t, obj, nil) + b, _, c := testBackend(t, obj, nil) + return b, c } func testBackendNoOperations(t *testing.T) (*Cloud, func()) { @@ -122,7 +128,8 @@ func testBackendNoOperations(t *testing.T) (*Cloud, func()) { "tags": cty.NullVal(cty.Set(cty.String)), }), }) - return testBackend(t, obj, nil) + b, _, c := testBackend(t, obj, nil) + return b, c } func testBackendWithHandlers(t *testing.T, handlers map[string]func(http.ResponseWriter, *http.Request)) (*Cloud, func()) { @@ -135,7 +142,8 @@ func testBackendWithHandlers(t *testing.T, handlers map[string]func(http.Respons "tags": cty.NullVal(cty.Set(cty.String)), }), }) - return testBackend(t, obj, handlers) + b, _, c := testBackend(t, obj, handlers) + return b, c } func testCloudState(t *testing.T) *State { @@ -212,7 +220,7 @@ func testBackendWithOutputs(t *testing.T) (*Cloud, func()) { return b, cleanup } -func testBackend(t *testing.T, obj cty.Value, handlers map[string]func(http.ResponseWriter, *http.Request)) (*Cloud, func()) { +func testBackend(t *testing.T, obj cty.Value, handlers map[string]func(http.ResponseWriter, *http.Request)) (*Cloud, *MockClient, func()) { var s *httptest.Server if handlers != nil { s = testServerWithHandlers(handlers) @@ -287,7 +295,7 @@ func testBackend(t *testing.T, obj cty.Value, handlers map[string]func(http.Resp } } - return b, s.Close + return b, mc, s.Close } // testUnconfiguredBackend is used for testing the configuration of the backend diff --git a/internal/cloud/tfe_client_mock.go b/internal/cloud/tfe_client_mock.go index ecc7a96767..ea7191bc8c 100644 --- a/internal/cloud/tfe_client_mock.go +++ b/internal/cloud/tfe_client_mock.go @@ -513,7 +513,7 @@ func (m *MockRedactedPlans) Read(ctx context.Context, hostname, token, planID st type MockPlans struct { client *MockClient logs map[string]string - planOutputs map[string]string + planOutputs map[string][]byte plans map[string]*tfe.Plan } @@ -521,7 +521,7 @@ func newMockPlans(client *MockClient) *MockPlans { return &MockPlans{ client: client, logs: make(map[string]string), - planOutputs: make(map[string]string), + planOutputs: make(map[string][]byte), plans: make(map[string]*tfe.Plan), } } @@ -548,6 +548,17 @@ func (m *MockPlans) create(cvID, workspaceID string) (*tfe.Plan, error) { w.WorkingDirectory, "plan.log", ) + + // Try to load unredacted json output, if it exists + outputPath := filepath.Join( + m.client.ConfigurationVersions.uploadPaths[cvID], + w.WorkingDirectory, + "plan-unredacted.json", + ) + if outBytes, err := os.ReadFile(outputPath); err == nil { + m.planOutputs[p.ID] = outBytes + } + m.plans[p.ID] = p return p, nil @@ -608,7 +619,7 @@ func (m *MockPlans) ReadJSONOutput(ctx context.Context, planID string) ([]byte, return nil, tfe.ErrResourceNotFound } - return []byte(planOutput), nil + return planOutput, nil } type MockTaskStages struct { @@ -1101,7 +1112,7 @@ func (m *MockRuns) ReadWithOptions(ctx context.Context, runID string, options *t } logs, _ := ioutil.ReadFile(m.client.Plans.logs[r.Plan.LogReadURL]) - if r.Status == tfe.RunPlanning && r.Plan.Status == tfe.PlanFinished { + if (r.Status == tfe.RunPlanning || r.Status == tfe.RunPlannedAndSaved) && r.Plan.Status == tfe.PlanFinished { hasChanges := r.IsDestroy || bytes.Contains(logs, []byte("1 to add")) || bytes.Contains(logs, []byte("1 to change")) || @@ -1110,6 +1121,7 @@ func (m *MockRuns) ReadWithOptions(ctx context.Context, runID string, options *t r.Actions.IsCancelable = false r.Actions.IsConfirmable = true r.HasChanges = true + r.Plan.HasChanges = true r.Permissions.CanApply = true } From 1f351731928bc4262768e1b132efd2a4b7189be7 Mon Sep 17 00:00:00 2001 From: Nick Fagerlund Date: Wed, 28 Jun 2023 17:47:06 -0700 Subject: [PATCH 16/20] Tests: Add tests for `(*Cloud).ShowPlanForRun()` --- internal/cloud/backend_show_test.go | 217 ++++++++++++++++++++++++++++ 1 file changed, 217 insertions(+) create mode 100644 internal/cloud/backend_show_test.go diff --git a/internal/cloud/backend_show_test.go b/internal/cloud/backend_show_test.go new file mode 100644 index 0000000000..a95af24c6e --- /dev/null +++ b/internal/cloud/backend_show_test.go @@ -0,0 +1,217 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: MPL-2.0 + +package cloud + +import ( + "context" + "path/filepath" + "strings" + "testing" + + tfe "github.com/hashicorp/go-tfe" + "github.com/hashicorp/terraform/internal/plans" +) + +// A brief discourse on the theory of testing for this feature. Doing +// `terraform show cloudplan.tfplan` relies on the correctness of the following +// behaviors: +// +// 1. TFC API returns redacted or unredacted plan JSON on request, if permission +// requirements are met and the run is in a condition where that JSON exists. +// 2. Cloud.ShowPlanForRun() makes correct API calls, calculates metadata +// properly given a tfe.Run, and returns either a cloudplan.RemotePlanJSON or an err. +// 3. The Show command instantiates Cloud backend when given a cloud planfile, +// calls .ShowPlanForRun() on it, and passes result to Display() impls. +// 4. Display() impls yield the correct output when given a cloud plan json biscuit. +// +// 1 is axiomatic and outside our domain. 3 is regrettably totally untestable +// unless we refactor the Meta command to enable stubbing out a backend factory +// or something, which seems inadvisable at this juncture. 4 is exercised over +// in internal/command/views/show_test.go. And thus, this file only cares about +// item 2. + +// 404 on run: special error message +func TestCloud_showMissingRun(t *testing.T) { + b, bCleanup := testBackendWithName(t) + defer bCleanup() + mockSROWorkspace(t, b, testBackendSingleWorkspaceName) + + absentRunID := "run-WwwwXxxxYyyyZzzz" + _, err := b.ShowPlanForRun(context.Background(), absentRunID, "app.terraform.io", true) + if !strings.Contains(err.Error(), "terraform login") { + t.Fatalf("expected error message to suggest checking your login status, instead got: %s", err) + } +} + +// If redacted json is available but unredacted is not +func TestCloud_showMissingUnredactedJson(t *testing.T) { + b, mc, bCleanup := testBackendAndMocksWithName(t) + defer bCleanup() + mockSROWorkspace(t, b, testBackendSingleWorkspaceName) + + ctx := context.Background() + + runID, err := testCloudRunForShow(mc, "./testdata/plan-json-basic-no-unredacted", tfe.RunPlannedAndSaved, tfe.PlanFinished) + if err != nil { + t.Fatalf("failed to init test data: %s", err) + } + // Showing the human-formatted plan should still work as expected! + redacted, err := b.ShowPlanForRun(ctx, runID, "app.terraform.io", true) + if err != nil { + t.Fatalf("failed to show plan for human, even though redacted json should be present: %s", err) + } + if !strings.Contains(string(redacted.JSONBytes), `"plan_format_version":`) { + t.Fatalf("show for human doesn't include expected redacted json content") + } + // Should be marked as containing changes and non-errored + canNotApply := false + errored := false + for _, opt := range redacted.Qualities { + if opt == plans.NoChanges { + canNotApply = true + } + if opt == plans.Errored { + errored = true + } + } + if canNotApply || errored { + t.Fatalf("expected neither errored nor can't-apply in opts, instead got: %#v", redacted.Qualities) + } + + // But show -json should result in a special error. + _, err = b.ShowPlanForRun(ctx, runID, "app.terraform.io", false) + if err == nil { + t.Fatalf("unexpected success: reading unredacted json without admin permissions should have errored") + } + if !strings.Contains(err.Error(), "admin") { + t.Fatalf("expected error message to suggest your permissions are wrong, instead got: %s", err) + } +} + +// If both kinds of json are available, both kinds of show should work +func TestCloud_showIncludesUnredactedJson(t *testing.T) { + b, mc, bCleanup := testBackendAndMocksWithName(t) + defer bCleanup() + mockSROWorkspace(t, b, testBackendSingleWorkspaceName) + + ctx := context.Background() + + runID, err := testCloudRunForShow(mc, "./testdata/plan-json-basic", tfe.RunPlannedAndSaved, tfe.PlanFinished) + if err != nil { + t.Fatalf("failed to init test data: %s", err) + } + // Showing the human-formatted plan should work as expected: + redacted, err := b.ShowPlanForRun(ctx, runID, "app.terraform.io", true) + if err != nil { + t.Fatalf("failed to show plan for human, even though redacted json should be present: %s", err) + } + if !strings.Contains(string(redacted.JSONBytes), `"plan_format_version":`) { + t.Fatalf("show for human doesn't include expected redacted json content") + } + // Showing the external json plan format should work as expected: + unredacted, err := b.ShowPlanForRun(ctx, runID, "app.terraform.io", false) + if err != nil { + t.Fatalf("failed to show plan for robot, even though unredacted json should be present: %s", err) + } + if !strings.Contains(string(unredacted.JSONBytes), `"format_version":`) { + t.Fatalf("show for robot doesn't include expected unredacted json content") + } +} + +func TestCloud_showNoChanges(t *testing.T) { + b, mc, bCleanup := testBackendAndMocksWithName(t) + defer bCleanup() + mockSROWorkspace(t, b, testBackendSingleWorkspaceName) + + ctx := context.Background() + + runID, err := testCloudRunForShow(mc, "./testdata/plan-json-no-changes", tfe.RunPlannedAndSaved, tfe.PlanFinished) + if err != nil { + t.Fatalf("failed to init test data: %s", err) + } + // Showing the human-formatted plan should work as expected: + redacted, err := b.ShowPlanForRun(ctx, runID, "app.terraform.io", true) + if err != nil { + t.Fatalf("failed to show plan for human, even though redacted json should be present: %s", err) + } + // Should be marked as no changes + canNotApply := false + for _, opt := range redacted.Qualities { + if opt == plans.NoChanges { + canNotApply = true + } + } + if !canNotApply { + t.Fatalf("expected opts to include CanNotApply, instead got: %#v", redacted.Qualities) + } +} + +func TestCloud_showFooterNotConfirmable(t *testing.T) { + b, mc, bCleanup := testBackendAndMocksWithName(t) + defer bCleanup() + mockSROWorkspace(t, b, testBackendSingleWorkspaceName) + + ctx := context.Background() + + runID, err := testCloudRunForShow(mc, "./testdata/plan-json-full", tfe.RunDiscarded, tfe.PlanFinished) + if err != nil { + t.Fatalf("failed to init test data: %s", err) + } + + // A little more custom run tweaking: + mc.Runs.Runs[runID].Actions.IsConfirmable = false + + // Showing the human-formatted plan should work as expected: + redacted, err := b.ShowPlanForRun(ctx, runID, "app.terraform.io", true) + if err != nil { + t.Fatalf("failed to show plan for human, even though redacted json should be present: %s", err) + } + + // Footer should mention that you can't apply it: + if !strings.Contains(redacted.RunFooter, "not confirmable") { + t.Fatalf("footer should call out that run isn't confirmable, instead got: %s", redacted.RunFooter) + } +} + +func testCloudRunForShow(mc *MockClient, configDir string, runStatus tfe.RunStatus, planStatus tfe.PlanStatus) (string, error) { + ctx := context.Background() + + // get workspace ID + wsID := mc.Workspaces.workspaceNames[testBackendSingleWorkspaceName].ID + // create and upload config version + cvOpts := tfe.ConfigurationVersionCreateOptions{ + AutoQueueRuns: tfe.Bool(false), + Speculative: tfe.Bool(false), + } + cv, err := mc.ConfigurationVersions.Create(ctx, wsID, cvOpts) + if err != nil { + return "", err + } + absDir, err := filepath.Abs(configDir) + if err != nil { + return "", err + } + err = mc.ConfigurationVersions.Upload(ctx, cv.UploadURL, absDir) + if err != nil { + return "", err + } + // create run + rOpts := tfe.RunCreateOptions{ + PlanOnly: tfe.Bool(false), + IsDestroy: tfe.Bool(false), + RefreshOnly: tfe.Bool(false), + ConfigurationVersion: cv, + Workspace: &tfe.Workspace{ID: wsID}, + } + r, err := mc.Runs.Create(ctx, rOpts) + if err != nil { + return "", err + } + // mess with statuses (this is what requires full access to mock client) + mc.Runs.Runs[r.ID].Status = runStatus + mc.Plans.plans[r.Plan.ID].Status = planStatus + + // return the ID + return r.ID, nil +} From f98f920b678a4f69c3f51501246c5dd838c11ae7 Mon Sep 17 00:00:00 2001 From: Nick Fagerlund Date: Thu, 6 Jul 2023 16:58:08 -0700 Subject: [PATCH 17/20] Add error wrapper types to highlight bad plan/state data This commit uses Go's error wrapping features to transparently add some optional info to certain planfile/state read errors. Specifically, we wrap errors when we think we've identified the file type but are somehow unable to use it. Callers that aren't interested in what we think about our input can just ignore the wrapping; callers that ARE interested can use `errors.As()`. --- internal/cloud/cloudplan/saved_plan.go | 5 +++ internal/plans/planfile/reader.go | 39 ++++++++++++++----- internal/plans/planfile/wrapped.go | 12 ++++-- internal/states/statefile/read.go | 52 ++++++++++++++++++++------ 4 files changed, 82 insertions(+), 26 deletions(-) diff --git a/internal/cloud/cloudplan/saved_plan.go b/internal/cloud/cloudplan/saved_plan.go index 0827f3a884..7257abf281 100644 --- a/internal/cloud/cloudplan/saved_plan.go +++ b/internal/cloud/cloudplan/saved_plan.go @@ -47,6 +47,11 @@ func LoadSavedPlanBookmark(filepath string) (SavedPlanBookmark, error) { return bookmark, err } + // Note that these error cases are somewhat ambiguous, but they *likely* + // mean we're not looking at a saved plan bookmark at all. Since we're not + // certain about the format at this point, it doesn't quite make sense to + // emit a "known file type but bad" error struct the way we do over in the + // planfile and statefile packages. if bookmark.RemotePlanFormat != 1 { return bookmark, ErrInvalidRemotePlanFormat } else if bookmark.Hostname == "" { diff --git a/internal/plans/planfile/reader.go b/internal/plans/planfile/reader.go index ad6cbdb9a1..31e284ab6b 100644 --- a/internal/plans/planfile/reader.go +++ b/internal/plans/planfile/reader.go @@ -21,6 +21,25 @@ const tfstateFilename = "tfstate" const tfstatePreviousFilename = "tfstate-prev" const dependencyLocksFilename = ".terraform.lock.hcl" // matches the conventional name in an input configuration +// ErrUnusableLocalPlan is an error wrapper to indicate that we *think* the +// input represents plan file data, but can't use it for some reason (as +// explained in the error text). Callers can check against this type with +// errors.As() if they need to distinguish between corrupt plan files and more +// fundamental problems like an empty file. +type ErrUnusableLocalPlan struct { + inner error +} + +func errUnusable(err error) *ErrUnusableLocalPlan { + return &ErrUnusableLocalPlan{inner: err} +} +func (e *ErrUnusableLocalPlan) Error() string { + return e.inner.Error() +} +func (e *ErrUnusableLocalPlan) Unwrap() error { + return e.inner +} + // Reader is the main type used to read plan files. Create a Reader by calling // Open. // @@ -42,7 +61,7 @@ func Open(filename string) (*Reader, error) { // like our old plan format from versions prior to 0.12. if b, sErr := ioutil.ReadFile(filename); sErr == nil { if bytes.HasPrefix(b, []byte("tfplan")) { - return nil, fmt.Errorf("the given plan file was created by an earlier version of Terraform; plan files cannot be shared between different Terraform versions") + return nil, errUnusable(fmt.Errorf("the given plan file was created by an earlier version of Terraform; plan files cannot be shared between different Terraform versions")) } } return nil, err @@ -86,12 +105,12 @@ func (r *Reader) ReadPlan() (*plans.Plan, error) { if planFile == nil { // This should never happen because we checked for this file during // Open, but we'll check anyway to be safe. - return nil, fmt.Errorf("the plan file is invalid") + return nil, errUnusable(fmt.Errorf("the plan file is invalid")) } pr, err := planFile.Open() if err != nil { - return nil, fmt.Errorf("failed to retrieve plan from plan file: %s", err) + return nil, errUnusable(fmt.Errorf("failed to retrieve plan from plan file: %s", err)) } defer pr.Close() @@ -108,16 +127,16 @@ func (r *Reader) ReadPlan() (*plans.Plan, error) { // access the prior state (this and the ReadStateFile method). ret, err := readTfplan(pr) if err != nil { - return nil, err + return nil, errUnusable(err) } prevRunStateFile, err := r.ReadPrevStateFile() if err != nil { - return nil, fmt.Errorf("failed to read previous run state from plan file: %s", err) + return nil, errUnusable(fmt.Errorf("failed to read previous run state from plan file: %s", err)) } priorStateFile, err := r.ReadStateFile() if err != nil { - return nil, fmt.Errorf("failed to read prior state from plan file: %s", err) + return nil, errUnusable(fmt.Errorf("failed to read prior state from plan file: %s", err)) } ret.PrevRunState = prevRunStateFile.State @@ -136,12 +155,12 @@ func (r *Reader) ReadStateFile() (*statefile.File, error) { if file.Name == tfstateFilename { r, err := file.Open() if err != nil { - return nil, fmt.Errorf("failed to extract state from plan file: %s", err) + return nil, errUnusable(fmt.Errorf("failed to extract state from plan file: %s", err)) } return statefile.Read(r) } } - return nil, statefile.ErrNoState + return nil, errUnusable(statefile.ErrNoState) } // ReadPrevStateFile reads the previous state file embedded in the plan file, which @@ -154,12 +173,12 @@ func (r *Reader) ReadPrevStateFile() (*statefile.File, error) { if file.Name == tfstatePreviousFilename { r, err := file.Open() if err != nil { - return nil, fmt.Errorf("failed to extract previous state from plan file: %s", err) + return nil, errUnusable(fmt.Errorf("failed to extract previous state from plan file: %s", err)) } return statefile.Read(r) } } - return nil, statefile.ErrNoState + return nil, errUnusable(statefile.ErrNoState) } // ReadConfigSnapshot reads the configuration snapshot embedded in the plan diff --git a/internal/plans/planfile/wrapped.go b/internal/plans/planfile/wrapped.go index 5d87a0b425..600c5fae1c 100644 --- a/internal/plans/planfile/wrapped.go +++ b/internal/plans/planfile/wrapped.go @@ -1,6 +1,7 @@ package planfile import ( + "errors" "fmt" "github.com/hashicorp/terraform/internal/cloud/cloudplan" @@ -81,10 +82,13 @@ func OpenWrapped(filename string) (*WrappedPlanFile, error) { if cloudErr == nil { return &WrappedPlanFile{cloud: &cloud}, nil } - // If neither worked, return both errors. In general we don't care to give - // any advice about how to fix an internal problem in a plan file, since - // both formats are opaque, but we do want to give the user the best chance - // at resolving whatever their problem was. + // If neither worked, prioritize definitive "confirmed the format but can't + // use it" errors, then fall back to dumping everything we know. + var ulp *ErrUnusableLocalPlan + if errors.As(localErr, &ulp) { + return nil, ulp + } + combinedErr := fmt.Errorf("couldn't load the provided path as either a local plan file (%s) or a saved cloud plan (%s)", localErr, cloudErr) return nil, combinedErr } diff --git a/internal/states/statefile/read.go b/internal/states/statefile/read.go index ceb6ce88ab..4243484194 100644 --- a/internal/states/statefile/read.go +++ b/internal/states/statefile/read.go @@ -20,6 +20,25 @@ import ( // ErrNoState is returned by ReadState when the state file is empty. var ErrNoState = errors.New("no state") +// ErrUnusableState is an error wrapper to indicate that we *think* the input +// represents state data, but can't use it for some reason (as explained in the +// error text). Callers can check against this type with errors.As() if they +// need to distinguish between corrupt state and more fundamental problems like +// an empty file. +type ErrUnusableState struct { + inner error +} + +func errUnusable(err error) *ErrUnusableState { + return &ErrUnusableState{inner: err} +} +func (e *ErrUnusableState) Error() string { + return e.inner.Error() +} +func (e *ErrUnusableState) Unwrap() error { + return e.inner +} + // Read reads a state from the given reader. // // Legacy state format versions 1 through 3 are supported, but the result will @@ -55,9 +74,9 @@ func Read(r io.Reader) (*File, error) { return nil, ErrNoState } - state, diags := readState(src) - if diags.HasErrors() { - return nil, diags.Err() + state, err := readState(src) + if err != nil { + return nil, err } if state == nil { @@ -68,7 +87,7 @@ func Read(r io.Reader) (*File, error) { return state, diags.Err() } -func readState(src []byte) (*File, tfdiags.Diagnostics) { +func readState(src []byte) (*File, error) { var diags tfdiags.Diagnostics if looksLikeVersion0(src) { @@ -77,15 +96,20 @@ func readState(src []byte) (*File, tfdiags.Diagnostics) { unsupportedFormat, "The state is stored in a legacy binary format that is not supported since Terraform v0.7. To continue, first upgrade the state using Terraform 0.6.16 or earlier.", )) - return nil, diags + return nil, errUnusable(diags.Err()) } version, versionDiags := sniffJSONStateVersion(src) diags = diags.Append(versionDiags) if versionDiags.HasErrors() { - return nil, diags + // This is the last point where there's a really good chance it's not a + // state file at all. Past here, we'll assume errors mean it's state but + // we can't use it. + return nil, diags.Err() } + var result *File + var err error switch version { case 0: diags = diags.Append(tfdiags.Sourceless( @@ -93,15 +117,14 @@ func readState(src []byte) (*File, tfdiags.Diagnostics) { unsupportedFormat, "The state file uses JSON syntax but has a version number of zero. There was never a JSON-based state format zero, so this state file is invalid and cannot be processed.", )) - return nil, diags case 1: - return readStateV1(src) + result, diags = readStateV1(src) case 2: - return readStateV2(src) + result, diags = readStateV2(src) case 3: - return readStateV3(src) + result, diags = readStateV3(src) case 4: - return readStateV4(src) + result, diags = readStateV4(src) default: thisVersion := tfversion.SemVer.String() creatingVersion := sniffJSONStateTerraformVersion(src) @@ -119,8 +142,13 @@ func readState(src []byte) (*File, tfdiags.Diagnostics) { fmt.Sprintf("The state file uses format version %d, which is not supported by Terraform %s. This state file may have been created by a newer version of Terraform.", version, thisVersion), )) } - return nil, diags } + + if diags.HasErrors() { + err = errUnusable(diags.Err()) + } + + return result, err } func sniffJSONStateVersion(src []byte) (uint64, tfdiags.Diagnostics) { From 1cbc95ce5664d82929ae82fae2aded08b5ce540a Mon Sep 17 00:00:00 2001 From: Nick Fagerlund Date: Thu, 6 Jul 2023 17:03:40 -0700 Subject: [PATCH 18/20] Use wrapped types to clean up error reporting in show command Since terraform show can accept three different kinds of file to act on, its error messages were starting to become untidy and unhelpful. The main issue was that if we successfully identified the file type but then ran into some problem while reading or processing it, the "real" error would be obscured by some other useless errors (since a file of one type is necessarily invalid as the other types). This commit tries to winnow it down to just one best error message, in the "happy path" case where we know what we're dealing with but hit a snag. (If we still have no idea, then we fall back to dumping everything.) --- internal/command/show.go | 98 ++++++++++++++++++++++++++++++++++------ 1 file changed, 83 insertions(+), 15 deletions(-) diff --git a/internal/command/show.go b/internal/command/show.go index 42f23ae0fa..e10ebbcd5b 100644 --- a/internal/command/show.go +++ b/internal/command/show.go @@ -5,6 +5,7 @@ package command import ( "context" + "errors" "fmt" "os" "strings" @@ -23,6 +24,24 @@ import ( "github.com/hashicorp/terraform/internal/tfdiags" ) +// Many of the methods we get data from can emit special error types if they're +// pretty sure about the file type but still can't use it. But they can't all do +// that! So, we have to do a couple ourselves if we want to preserve that data. +type errUnusableDataMisc struct { + inner error + kind string +} + +func errUnusable(err error, kind string) *errUnusableDataMisc { + return &errUnusableDataMisc{inner: err, kind: kind} +} +func (e *errUnusableDataMisc) Error() string { + return e.inner.Error() +} +func (e *errUnusableDataMisc) Unwrap() error { + return e.inner +} + // ShowCommand is a Command implementation that reads and outputs the // contents of a Terraform plan or state file. type ShowCommand struct { @@ -171,13 +190,58 @@ func (c *ShowCommand) showFromPath(path string) (*plans.Plan, *cloudplan.RemoteP if planErr != nil { stateFile, stateErr = getStateFromPath(path) if stateErr != nil { - diags = diags.Append( - tfdiags.Sourceless( - tfdiags.Error, - "Failed to read the given file as a state or plan file", - fmt.Sprintf("State read error: %s\n\nPlan read error: %s", stateErr, planErr), - ), - ) + // To avoid spamming the user with irrelevant errors, first check to + // see if one of our errors happens to know for a fact what file + // type we were dealing with. If so, then we can ignore the other + // ones (which are likely to be something unhelpful like "not a + // valid zip file"). If not, we can fall back to dumping whatever + // we've got. + var unLocal *planfile.ErrUnusableLocalPlan + var unState *statefile.ErrUnusableState + var unMisc *errUnusableDataMisc + if errors.As(planErr, &unLocal) { + diags = diags.Append( + tfdiags.Sourceless( + tfdiags.Error, + "Couldn't show local plan", + fmt.Sprintf("Plan read error: %s", unLocal), + ), + ) + } else if errors.As(planErr, &unMisc) { + diags = diags.Append( + tfdiags.Sourceless( + tfdiags.Error, + fmt.Sprintf("Couldn't show %s", unMisc.kind), + fmt.Sprintf("Plan read error: %s", unMisc), + ), + ) + } else if errors.As(stateErr, &unState) { + diags = diags.Append( + tfdiags.Sourceless( + tfdiags.Error, + "Couldn't show state file", + fmt.Sprintf("Plan read error: %s", unState), + ), + ) + } else if errors.As(stateErr, &unMisc) { + diags = diags.Append( + tfdiags.Sourceless( + tfdiags.Error, + fmt.Sprintf("Couldn't show %s", unMisc.kind), + fmt.Sprintf("Plan read error: %s", unMisc), + ), + ) + } else { + // Ok, give up and show the really big error + diags = diags.Append( + tfdiags.Sourceless( + tfdiags.Error, + "Failed to read the given file as a state or plan file", + fmt.Sprintf("State read error: %s\n\nPlan read error: %s", stateErr, planErr), + ), + ) + } + return nil, nil, nil, nil, diags } } @@ -216,15 +280,19 @@ func (c *ShowCommand) getDataFromCloudPlan(plan *cloudplan.SavedPlanBookmark, re // Set up the backend b, backendDiags := c.Backend(nil) if backendDiags.HasErrors() { - return nil, backendDiags.Err() + return nil, errUnusable(backendDiags.Err(), "cloud plan") } // Cloud plans only work if we're cloud. cl, ok := b.(*cloud.Cloud) if !ok { - return nil, fmt.Errorf("can't show a saved cloud plan unless the current root module is connected to Terraform Cloud") + return nil, errUnusable(fmt.Errorf("can't show a saved cloud plan unless the current root module is connected to Terraform Cloud"), "cloud plan") } - return cl.ShowPlanForRun(context.Background(), plan.RunID, plan.Hostname, redacted) + result, err := cl.ShowPlanForRun(context.Background(), plan.RunID, plan.Hostname, redacted) + if err != nil { + err = errUnusable(err, "cloud plan") + } + return result, err } // getDataFromPlanfileReader returns a plan, statefile, and config, extracted from a local plan file. @@ -244,7 +312,7 @@ func getDataFromPlanfileReader(planReader *planfile.Reader) (*plans.Plan, *state // Get config config, diags := planReader.ReadConfig() if diags.HasErrors() { - return nil, nil, nil, diags.Err() + return nil, nil, nil, errUnusable(diags.Err(), "local plan") } return plan, stateFile, config, err @@ -254,14 +322,14 @@ func getDataFromPlanfileReader(planReader *planfile.Reader) (*plans.Plan, *state func getStateFromPath(path string) (*statefile.File, error) { file, err := os.Open(path) if err != nil { - return nil, fmt.Errorf("Error loading statefile: %s", err) + return nil, fmt.Errorf("Error loading statefile: %w", err) } defer file.Close() var stateFile *statefile.File stateFile, err = statefile.Read(file) if err != nil { - return nil, fmt.Errorf("Error reading %s as a statefile: %s", path, err) + return nil, fmt.Errorf("Error reading %s as a statefile: %w", path, err) } return stateFile, nil } @@ -271,12 +339,12 @@ func getStateFromBackend(b backend.Backend, workspace string) (*statefile.File, // Get the state store for the given workspace stateStore, err := b.StateMgr(workspace) if err != nil { - return nil, fmt.Errorf("Failed to load state manager: %s", err) + return nil, fmt.Errorf("Failed to load state manager: %w", err) } // Refresh the state store with the latest state snapshot from persistent storage if err := stateStore.RefreshState(); err != nil { - return nil, fmt.Errorf("Failed to load state: %s", err) + return nil, fmt.Errorf("Failed to load state: %w", err) } // Get the latest state snapshot and return it From 31cf55fe12e973da39d6bdf3f99aa3d5aa69ea64 Mon Sep 17 00:00:00 2001 From: Sebastian Rivera Date: Mon, 24 Jul 2023 14:04:42 -0400 Subject: [PATCH 19/20] Reword error message when using local exec --- internal/backend/local/backend_local.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/backend/local/backend_local.go b/internal/backend/local/backend_local.go index a38d5b033a..8ef2726dd5 100644 --- a/internal/backend/local/backend_local.go +++ b/internal/backend/local/backend_local.go @@ -78,7 +78,7 @@ func (b *Local) localRun(op *backend.Operation) (*backend.LocalRun, *configload. var ctxDiags tfdiags.Diagnostics var configSnap *configload.Snapshot if op.PlanFile.IsCloud() { - diags = diags.Append(fmt.Errorf("error: using a saved cloud plan with the local backend is not supported")) + diags = diags.Append(fmt.Errorf("error: using a saved cloud plan when executing Terraform locally is not supported")) return nil, nil, nil, diags } From 08e58fd484a460f41749c8cc1d3b1c1aa4a1ee59 Mon Sep 17 00:00:00 2001 From: Sebastian Rivera Date: Mon, 24 Jul 2023 14:06:04 -0400 Subject: [PATCH 20/20] Fix saved plan test regressions, fixtures --- internal/backend/local/backend_local_test.go | 4 +- .../testdata/plan-bookmark/bookmark.json | 5 + internal/command/views/show_test.go | 2 +- internal/command/views/test.go | 6 +- .../views/testdata/plans/redacted-plan.json | 116 ++++++++++++++++++ 5 files changed, 127 insertions(+), 6 deletions(-) create mode 100644 internal/backend/local/testdata/plan-bookmark/bookmark.json create mode 100644 internal/command/views/testdata/plans/redacted-plan.json diff --git a/internal/backend/local/backend_local_test.go b/internal/backend/local/backend_local_test.go index 82eefb634c..2ed77f4854 100644 --- a/internal/backend/local/backend_local_test.go +++ b/internal/backend/local/backend_local_test.go @@ -90,10 +90,10 @@ func TestLocalRun_cloudPlan(t *testing.T) { configDir := "./testdata/apply" b := TestLocal(t) - _, configLoader, configCleanup := initwd.MustLoadConfigForTests(t, configDir) + _, configLoader, configCleanup := initwd.MustLoadConfigForTests(t, configDir, "tests") defer configCleanup() - planPath := "../../cloud/cloudplan/testdata/plan-bookmark/bookmark.json" + planPath := "./testdata/plan-bookmark/bookmark.json" planFile, err := planfile.OpenWrapped(planPath) if err != nil { diff --git a/internal/backend/local/testdata/plan-bookmark/bookmark.json b/internal/backend/local/testdata/plan-bookmark/bookmark.json new file mode 100644 index 0000000000..0a1c73302a --- /dev/null +++ b/internal/backend/local/testdata/plan-bookmark/bookmark.json @@ -0,0 +1,5 @@ +{ + "remote_plan_format": 1, + "run_id": "run-GXfuHMkbyHccAGUg", + "hostname": "app.terraform.io" +} diff --git a/internal/command/views/show_test.go b/internal/command/views/show_test.go index 366a249b3e..4490bc2ec1 100644 --- a/internal/command/views/show_test.go +++ b/internal/command/views/show_test.go @@ -25,7 +25,7 @@ import ( ) func TestShowHuman(t *testing.T) { - redactedPath := "../../cloud/testdata/plan-json-basic/plan-redacted.json" + redactedPath := "./testdata/plans/redacted-plan.json" redactedPlanJson, err := os.ReadFile(redactedPath) if err != nil { t.Fatalf("couldn't read json plan test data at %s for showing a cloud plan. Did the file get moved?", redactedPath) diff --git a/internal/command/views/test.go b/internal/command/views/test.go index 87c0317cbb..b8c6cef231 100644 --- a/internal/command/views/test.go +++ b/internal/command/views/test.go @@ -188,12 +188,12 @@ func (t *TestHuman) Run(run *moduletest.Run, file *moduletest.File) { RelevantAttributes: attrs, } - var opts []jsonformat.PlanRendererOpt + var opts []plans.Quality if !run.Verbose.Plan.CanApply() { - opts = append(opts, jsonformat.CanNotApply) + opts = append(opts, plans.NoChanges) } if run.Verbose.Plan.Errored { - opts = append(opts, jsonformat.Errored) + opts = append(opts, plans.Errored) } renderer.RenderHumanPlan(plan, run.Verbose.Plan.UIMode, opts...) diff --git a/internal/command/views/testdata/plans/redacted-plan.json b/internal/command/views/testdata/plans/redacted-plan.json new file mode 100644 index 0000000000..9becf1bcd0 --- /dev/null +++ b/internal/command/views/testdata/plans/redacted-plan.json @@ -0,0 +1,116 @@ +{ + "plan_format_version": "1.1", + "resource_drift": [], + "resource_changes": [ + { + "address": "null_resource.foo", + "mode": "managed", + "type": "null_resource", + "name": "foo", + "provider_name": "registry.terraform.io/hashicorp/null", + "change": { + "actions": [ + "create" + ], + "before": null, + "after": { + "triggers": null + }, + "after_unknown": { + "id": true + }, + "before_sensitive": false, + "after_sensitive": {} + } + } + ], + "relevant_attributes": [], + "output_changes": {}, + "provider_schemas": { + "registry.terraform.io/hashicorp/null": { + "provider": { + "version": 0, + "block": { + "description_kind": "plain" + } + }, + "resource_schemas": { + "null_resource": { + "version": 0, + "block": { + "attributes": { + "id": { + "type": "string", + "description": "This is set to a random value at create time.", + "description_kind": "plain", + "computed": true + }, + "triggers": { + "type": [ + "map", + "string" + ], + "description": "A map of arbitrary strings that, when changed, will force the null resource to be replaced, re-running any associated provisioners.", + "description_kind": "plain", + "optional": true + } + }, + "description": "The `null_resource` resource implements the standard resource lifecycle but takes no further action.\n\nThe `triggers` argument allows specifying an arbitrary set of values that, when changed, will cause the resource to be replaced.", + "description_kind": "plain" + } + } + }, + "data_source_schemas": { + "null_data_source": { + "version": 0, + "block": { + "attributes": { + "has_computed_default": { + "type": "string", + "description": "If set, its literal value will be stored and returned. If not, its value defaults to `\"default\"`. This argument exists primarily for testing and has little practical use.", + "description_kind": "plain", + "optional": true, + "computed": true + }, + "id": { + "type": "string", + "description": "This attribute is only present for some legacy compatibility issues and should not be used. It will be removed in a future version.", + "description_kind": "plain", + "deprecated": true, + "computed": true + }, + "inputs": { + "type": [ + "map", + "string" + ], + "description": "A map of arbitrary strings that is copied into the `outputs` attribute, and accessible directly for interpolation.", + "description_kind": "plain", + "optional": true + }, + "outputs": { + "type": [ + "map", + "string" + ], + "description": "After the data source is \"read\", a copy of the `inputs` map.", + "description_kind": "plain", + "computed": true + }, + "random": { + "type": "string", + "description": "A random value. This is primarily for testing and has little practical use; prefer the [hashicorp/random provider](https://registry.terraform.io/providers/hashicorp/random) for more practical random number use-cases.", + "description_kind": "plain", + "computed": true + } + }, + "description": "The `null_data_source` data source implements the standard data source lifecycle but does not\ninteract with any external APIs.\n\nHistorically, the `null_data_source` was typically used to construct intermediate values to re-use elsewhere in configuration. The\nsame can now be achieved using [locals](https://www.terraform.io/docs/language/values/locals.html).\n", + "description_kind": "plain", + "deprecated": true + } + } + } + } + }, + "provider_format_version": "1.0" +}