From b23cfaefe825a82baf7be05a210385483efcc58e Mon Sep 17 00:00:00 2001 From: Sebastian Rivera Date: Tue, 11 Apr 2023 16:47:56 -0400 Subject: [PATCH 1/3] Refactor SRO check to prevent duplicate plan output --- go.mod | 4 +-- go.sum | 10 +++---- internal/cloud/backend_plan.go | 54 +++++++++++++++++++++++++++++++--- 3 files changed, 57 insertions(+), 11 deletions(-) diff --git a/go.mod b/go.mod index fa43307ed4..89fa59ca67 100644 --- a/go.mod +++ b/go.mod @@ -39,7 +39,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.2 - github.com/hashicorp/go-tfe v1.18.0 + github.com/hashicorp/go-tfe v1.21.0 github.com/hashicorp/go-uuid v1.0.3 github.com/hashicorp/go-version v1.6.0 github.com/hashicorp/hcl v0.0.0-20170504190234-a4b07c25de5f @@ -147,7 +147,7 @@ require ( github.com/hashicorp/go-msgpack v0.5.4 // indirect github.com/hashicorp/go-rootcerts v1.0.2 // indirect github.com/hashicorp/go-safetemp v1.0.0 // indirect - github.com/hashicorp/go-slug v0.10.1 // indirect + github.com/hashicorp/go-slug v0.11.0 // indirect github.com/hashicorp/golang-lru v0.5.1 // indirect github.com/hashicorp/serf v0.9.5 // indirect github.com/hashicorp/yamux v0.0.0-20181012175058-2f1d1f20f75d // indirect diff --git a/go.sum b/go.sum index 4d4453986b..76cf8b9182 100644 --- a/go.sum +++ b/go.sum @@ -537,13 +537,13 @@ github.com/hashicorp/go-rootcerts v1.0.2 h1:jzhAVGtqPKbwpyCPELlgNWhE1znq+qwJtW5O github.com/hashicorp/go-rootcerts v1.0.2/go.mod h1:pqUvnprVnM5bf7AOirdbb01K4ccR319Vf4pU3K5EGc8= github.com/hashicorp/go-safetemp v1.0.0 h1:2HR189eFNrjHQyENnQMMpCiBAsRxzbTMIgBhEyExpmo= github.com/hashicorp/go-safetemp v1.0.0/go.mod h1:oaerMy3BhqiTbVye6QuFhFtIceqFoDHxNAB65b+Rj1I= -github.com/hashicorp/go-slug v0.10.1 h1:05SCRWCBpCxOeP7stQHvMgOz0raCBCekaytu8Rg/RZ4= -github.com/hashicorp/go-slug v0.10.1/go.mod h1:Ib+IWBYfEfJGI1ZyXMGNbu2BU+aa3Dzu41RKLH301v4= +github.com/hashicorp/go-slug v0.11.0 h1:l7cHWiBk8cnnskjheloW9h8PwXhihvwXbQiiFw2KqkY= +github.com/hashicorp/go-slug v0.11.0/go.mod h1:Ib+IWBYfEfJGI1ZyXMGNbu2BU+aa3Dzu41RKLH301v4= github.com/hashicorp/go-sockaddr v1.0.0 h1:GeH6tui99pF4NJgfnhp+L6+FfobzVW3Ah46sLo0ICXs= github.com/hashicorp/go-sockaddr v1.0.0/go.mod h1:7Xibr9yA9JjQq1JpNB2Vw7kxv8xerXegt+ozgdvDeDU= github.com/hashicorp/go-syslog v1.0.0/go.mod h1:qPfqrKkXGihmCqbJM2mZgkZGvKG1dFdvsLplgctolz4= -github.com/hashicorp/go-tfe v1.18.0 h1:AjyZe2KSAyGHD1kbGYlY64PVYQPnJJON24qr97IjIqA= -github.com/hashicorp/go-tfe v1.18.0/go.mod h1:T76X7dHKNEPEugPCZI3gDdaDdxUU4P4sqMZO60W57cQ= +github.com/hashicorp/go-tfe v1.21.0 h1:sTZXf/MaC/iQ8HxKwYSL0xJSEVDwY+h4ngh/+na8vdk= +github.com/hashicorp/go-tfe v1.21.0/go.mod h1:jedlLiHHiDeBKKpON4aIpTdsKbc2OaVbklEPI7XEHiY= 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= @@ -762,7 +762,7 @@ github.com/stretchr/testify v1.6.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/ github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= github.com/stretchr/testify v1.7.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= github.com/stretchr/testify v1.8.0/go.mod h1:yNjHg4UonilssWZ8iaSj1OCr/vHnekPRkoO+kdMU+MU= -github.com/stretchr/testify v1.8.1 h1:w7B6lhMri9wdJUVmEZPGGhZzrYTPvgJArz7wNPgYKsk= +github.com/stretchr/testify v1.8.2 h1:+h33VjcLVPDHtOdpUCuF+7gSuG3yGIftsP1YvFihtJ8= github.com/tencentcloud/tencentcloud-sdk-go/tencentcloud/common v1.0.194/go.mod h1:7sCQWVkxcsR38nffDW057DRGk8mUjK1Ing/EFOK8s8Y= github.com/tencentcloud/tencentcloud-sdk-go/tencentcloud/common v1.0.588 h1:DYtBXB7sVc3EOW5horg8j55cLZynhsLYhHrvQ/jXKKM= github.com/tencentcloud/tencentcloud-sdk-go/tencentcloud/common v1.0.588/go.mod h1:7sCQWVkxcsR38nffDW057DRGk8mUjK1Ing/EFOK8s8Y= diff --git a/internal/cloud/backend_plan.go b/internal/cloud/backend_plan.go index 8854d104ec..880233637d 100644 --- a/internal/cloud/backend_plan.go +++ b/internal/cloud/backend_plan.go @@ -11,6 +11,7 @@ import ( "log" "os" "path/filepath" + "strconv" "strings" "syscall" "time" @@ -428,10 +429,10 @@ func (b *Cloud) renderPlanLogs(ctx context.Context, op *backend.Operation, run * return nil } - // Only call the renderer if the remote workspace has structured run output - // enabled. The plan output will have already been rendered when the logs - // were read if this wasn't the case. - if run.Workspace.StructuredRunOutputEnabled && b.renderer != nil { + // Determine whether we should call the renderer to generate the plan output + // in human readable format. Otherwise we risk duplicate plan output since + // plan output may be contained in the streamed log file. + if ok, err := b.shouldRenderStructuredRunOutput(run); ok { // Fetch the redacted plan. redacted, err := readRedactedPlan(ctx, b.client.BaseURL(), b.token, run.Plan.ID) if err != nil { @@ -440,11 +441,56 @@ func (b *Cloud) renderPlanLogs(ctx context.Context, op *backend.Operation, run * // Render plan output. b.renderer.RenderHumanPlan(*redacted, op.PlanMode) + } else if err != nil { + return err } return nil } +// shouldRenderStructuredRunOutput ensures the remote workspace has structured +// run output enabled and, if using Terraform Enterprise, ensures it is a release +// that supports enabling SRO for CLI-driven runs. The plan output will have +// already been rendered when the logs were read if this wasn't the case. +func (b *Cloud) shouldRenderStructuredRunOutput(run *tfe.Run) (bool, error) { + if b.renderer == nil || !run.Workspace.StructuredRunOutputEnabled { + return false, nil + } + + // If the cloud backend is configured against TFC, we only require that + // the workspace has structured run output enabled. + if b.client.IsCloud() && run.Workspace.StructuredRunOutputEnabled { + fmt.Println("we should see this") + return true, nil + } + + // If the cloud backend is configured against TFE, ensure the release version + // supports enabling SRO for CLI runs. + if b.client.IsEnterprise() { + tfeVersion := b.client.RemoteTFEVersion() + if tfeVersion != "" { + v := strings.Split(tfeVersion[1:], "-") + releaseDate, err := strconv.Atoi(v[0]) + if err != nil { + return false, err + } + + fmt.Println(releaseDate) + + // Any release older than 202302-1 will not support enabling SRO for + // CLI-driven runs + if releaseDate < 202302 { + return false, nil + } else if run.Workspace.StructuredRunOutputEnabled { + return true, nil + } + } + } + + // Version of TFE is unknowable + return false, nil +} + const planDefaultHeader = ` [reset][yellow]Running plan in Terraform Cloud. Output will stream here. Pressing Ctrl-C will stop streaming the logs, but will not stop the plan running remotely.[reset] From 5634ae3e1822c6dfc344cd95e513e11ae5318dbf Mon Sep 17 00:00:00 2001 From: Sebastian Rivera Date: Tue, 11 Apr 2023 16:48:36 -0400 Subject: [PATCH 2/3] Unit tests to ensure renderer is appropriately called --- internal/cloud/backend_plan.go | 3 - internal/cloud/backend_plan_test.go | 118 ++++++++++++++++++++++++++++ internal/cloud/backend_test.go | 2 +- internal/cloud/testing.go | 35 +++++++-- 4 files changed, 149 insertions(+), 9 deletions(-) diff --git a/internal/cloud/backend_plan.go b/internal/cloud/backend_plan.go index 880233637d..b2d4651388 100644 --- a/internal/cloud/backend_plan.go +++ b/internal/cloud/backend_plan.go @@ -460,7 +460,6 @@ func (b *Cloud) shouldRenderStructuredRunOutput(run *tfe.Run) (bool, error) { // If the cloud backend is configured against TFC, we only require that // the workspace has structured run output enabled. if b.client.IsCloud() && run.Workspace.StructuredRunOutputEnabled { - fmt.Println("we should see this") return true, nil } @@ -475,8 +474,6 @@ func (b *Cloud) shouldRenderStructuredRunOutput(run *tfe.Run) (bool, error) { return false, err } - fmt.Println(releaseDate) - // Any release older than 202302-1 will not support enabling SRO for // CLI-driven runs if releaseDate < 202302 { diff --git a/internal/cloud/backend_plan_test.go b/internal/cloud/backend_plan_test.go index 4e60510a12..a4c1e4998a 100644 --- a/internal/cloud/backend_plan_test.go +++ b/internal/cloud/backend_plan_test.go @@ -2,6 +2,7 @@ package cloud import ( "context" + "net/http" "os" "os/signal" "strings" @@ -1250,3 +1251,120 @@ func TestCloud_planOtherError(t *testing.T) { t.Fatalf("expected error message, got: %s", err.Error()) } } + +func TestCloud_planShouldRenderSRO(t *testing.T) { + t.Run("when instance is TFC", func(t *testing.T) { + handlers := map[string]func(http.ResponseWriter, *http.Request){ + "/api/v2/ping": func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.Header().Set("TFP-API-Version", "2.5") + w.Header().Set("TFP-AppName", "Terraform Cloud") + }, + } + b, bCleanup := testBackendWithHandlers(t, handlers) + t.Cleanup(bCleanup) + b.renderer = &jsonformat.Renderer{} + + t.Run("and SRO is enabled", func(t *testing.T) { + r := &tfe.Run{ + Workspace: &tfe.Workspace{ + StructuredRunOutputEnabled: true, + }, + } + assertSRORendered(t, b, r, true) + }) + + t.Run("and SRO is not enabled", func(t *testing.T) { + r := &tfe.Run{ + Workspace: &tfe.Workspace{ + StructuredRunOutputEnabled: false, + }, + } + assertSRORendered(t, b, r, false) + }) + + }) + + t.Run("when instance is TFE and version supports CLI SRO", func(t *testing.T) { + handlers := map[string]func(http.ResponseWriter, *http.Request){ + "/api/v2/ping": func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.Header().Set("TFP-API-Version", "2.5") + w.Header().Set("TFP-AppName", "Terraform Enterprise") + w.Header().Set("X-TFE-Version", "v202303-1") + }, + } + b, bCleanup := testBackendWithHandlers(t, handlers) + t.Cleanup(bCleanup) + b.renderer = &jsonformat.Renderer{} + + t.Run("and SRO is enabled", func(t *testing.T) { + r := &tfe.Run{ + Workspace: &tfe.Workspace{ + StructuredRunOutputEnabled: true, + }, + } + assertSRORendered(t, b, r, true) + }) + + t.Run("and SRO is not enabled", func(t *testing.T) { + r := &tfe.Run{ + Workspace: &tfe.Workspace{ + StructuredRunOutputEnabled: false, + }, + } + assertSRORendered(t, b, r, false) + }) + }) + + t.Run("when instance is a known unsupported TFE release", func(t *testing.T) { + handlers := map[string]func(http.ResponseWriter, *http.Request){ + "/api/v2/ping": func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.Header().Set("TFP-API-Version", "2.5") + w.Header().Set("TFP-AppName", "Terraform Enterprise") + w.Header().Set("X-TFE-Version", "v202208-1") + }, + } + b, bCleanup := testBackendWithHandlers(t, handlers) + t.Cleanup(bCleanup) + b.renderer = &jsonformat.Renderer{} + + r := &tfe.Run{ + Workspace: &tfe.Workspace{ + StructuredRunOutputEnabled: true, + }, + } + assertSRORendered(t, b, r, false) + }) + + t.Run("when instance is an unknown TFE release", func(t *testing.T) { + handlers := map[string]func(http.ResponseWriter, *http.Request){ + "/api/v2/ping": func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.Header().Set("TFP-API-Version", "2.5") + }, + } + b, bCleanup := testBackendWithHandlers(t, handlers) + t.Cleanup(bCleanup) + b.renderer = &jsonformat.Renderer{} + + r := &tfe.Run{ + Workspace: &tfe.Workspace{ + StructuredRunOutputEnabled: true, + }, + } + assertSRORendered(t, b, r, false) + }) + +} + +func assertSRORendered(t *testing.T, b *Cloud, r *tfe.Run, shouldRender bool) { + got, err := b.shouldRenderStructuredRunOutput(r) + if err != nil { + t.Fatalf("expected no error: %v", err) + } + if shouldRender != got { + t.Fatalf("expected SRO to be rendered: %t, got %t", shouldRender, got) + } +} diff --git a/internal/cloud/backend_test.go b/internal/cloud/backend_test.go index c73109bf48..081fce6505 100644 --- a/internal/cloud/backend_test.go +++ b/internal/cloud/backend_test.go @@ -647,7 +647,7 @@ func TestCloud_setUnavailableTerraformVersion(t *testing.T) { }), }) - b, bCleanup := testBackend(t, config) + 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/testing.go b/internal/cloud/testing.go index ef0d9ed8b2..4c5302ce4a 100644 --- a/internal/cloud/testing.go +++ b/internal/cloud/testing.go @@ -43,6 +43,13 @@ var ( tfeHost: {"token": testCred}, }) testBackendSingleWorkspaceName = "app-prod" + defaultTFCPing = map[string]func(http.ResponseWriter, *http.Request){ + "/api/v2/ping": func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.Header().Set("TFP-API-Version", "2.5") + w.Header().Set("TFP-AppName", "Terraform Cloud") + }, + } ) // mockInput is a mock implementation of terraform.UIInput. @@ -79,7 +86,7 @@ func testBackendWithName(t *testing.T) (*Cloud, func()) { "tags": cty.NullVal(cty.Set(cty.String)), }), }) - return testBackend(t, obj) + return testBackend(t, obj, defaultTFCPing) } func testBackendWithTags(t *testing.T) (*Cloud, func()) { @@ -96,7 +103,7 @@ func testBackendWithTags(t *testing.T) (*Cloud, func()) { ), }), }) - return testBackend(t, obj) + return testBackend(t, obj, nil) } func testBackendNoOperations(t *testing.T) (*Cloud, func()) { @@ -109,7 +116,20 @@ func testBackendNoOperations(t *testing.T) (*Cloud, func()) { "tags": cty.NullVal(cty.Set(cty.String)), }), }) - return testBackend(t, obj) + return testBackend(t, obj, nil) +} + +func testBackendWithHandlers(t *testing.T, handlers map[string]func(http.ResponseWriter, *http.Request)) (*Cloud, func()) { + obj := cty.ObjectVal(map[string]cty.Value{ + "hostname": cty.NullVal(cty.String), + "organization": cty.StringVal("hashicorp"), + "token": cty.NullVal(cty.String), + "workspaces": cty.ObjectVal(map[string]cty.Value{ + "name": cty.StringVal(testBackendSingleWorkspaceName), + "tags": cty.NullVal(cty.Set(cty.String)), + }), + }) + return testBackend(t, obj, handlers) } func testCloudState(t *testing.T) *State { @@ -186,8 +206,13 @@ func testBackendWithOutputs(t *testing.T) (*Cloud, func()) { return b, cleanup } -func testBackend(t *testing.T, obj cty.Value) (*Cloud, func()) { - s := testServer(t) +func testBackend(t *testing.T, obj cty.Value, handlers map[string]func(http.ResponseWriter, *http.Request)) (*Cloud, func()) { + var s *httptest.Server + if handlers != nil { + s = testServerWithHandlers(handlers) + } else { + s = testServer(t) + } b := New(testDisco(s)) // Configure the backend so the client is created. From 300a60f393a78f4ce4127a7ef2ce4c0f32d302a5 Mon Sep 17 00:00:00 2001 From: Sebastian Rivera Date: Thu, 13 Apr 2023 12:36:46 -0400 Subject: [PATCH 3/3] Fix typo in format version check --- internal/command/jsonformat/renderer.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/command/jsonformat/renderer.go b/internal/command/jsonformat/renderer.go index e176603391..c1adf5c0c1 100644 --- a/internal/command/jsonformat/renderer.go +++ b/internal/command/jsonformat/renderer.go @@ -58,7 +58,7 @@ func (renderer Renderer) RenderHumanPlan(plan Plan, mode plans.Mode, opts ...Pla // version differences. This should work for alpha testing in the meantime. if plan.PlanFormatVersion != jsonplan.FormatVersion || plan.ProviderFormatVersion != jsonprovider.FormatVersion { 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 maybe missing representations of recent features."), + 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."), renderer.Streams.Stdout.Columns())) }