From 4991cc48354275f5a190b12e0f8cdafb952fdc0a Mon Sep 17 00:00:00 2001 From: Alisdair McDiarmid Date: Mon, 8 Feb 2021 13:29:42 -0500 Subject: [PATCH] cli: Improve error for invalid -target flags Errors encountered when parsing flags for apply, plan, and refresh were being suppressed. This resulted in a generic usage error when using an invalid `-target` flag. This commit makes several changes to address this. First, these commands now output the flag parse error before exiting, leaving at least some hint about the error. You can verify this manually with something like: terraform apply -invalid-flag We also change how target attributes are parsed, moving the responsibility from the flags instance to the command. This allows us to customize the diagnostic output to be more user friendly. The diagnostics now look like: ```shellsession $ terraform apply -no-color -target=foo Error: Invalid target "foo" Resource specification must include a resource type and name. ``` Finally, we add test coverage for both parsing of target flags, and at the command level for successful use of resource targeting. These tests focus on the UI output (via the change summary and refresh logs), as the functionality of targeting is covered by the context tests in the terraform package. --- command/apply.go | 7 +- command/apply_destroy_test.go | 155 +++++++++++++++++- command/apply_test.go | 86 ++++++++++ command/flag_kv.go | 36 ---- command/meta.go | 44 ++++- command/plan.go | 9 +- command/plan_test.go | 84 ++++++++++ command/refresh.go | 9 +- command/refresh_test.go | 91 ++++++++++ .../testdata/apply-destroy-targeted/main.tf | 2 +- command/testdata/apply-targeted/main.tf | 9 + command/testdata/refresh-targeted/main.tf | 7 + 12 files changed, 494 insertions(+), 45 deletions(-) create mode 100644 command/testdata/apply-targeted/main.tf create mode 100644 command/testdata/refresh-targeted/main.tf diff --git a/command/apply.go b/command/apply.go index c433400a9..d84745f19 100644 --- a/command/apply.go +++ b/command/apply.go @@ -42,10 +42,15 @@ func (c *ApplyCommand) Run(args []string) int { cmdFlags.DurationVar(&c.Meta.stateLockTimeout, "lock-timeout", 0, "lock timeout") cmdFlags.Usage = func() { c.Ui.Error(c.Help()) } if err := cmdFlags.Parse(args); err != nil { + c.Ui.Error(fmt.Sprintf("Error parsing command-line flags: %s\n", err.Error())) return 1 } - var diags tfdiags.Diagnostics + diags := c.parseTargetFlags() + if diags.HasErrors() { + c.showDiagnostics(diags) + return 1 + } args = cmdFlags.Args() var planPath string diff --git a/command/apply_destroy_test.go b/command/apply_destroy_test.go index 6bc51accb..ec3b2e9ea 100644 --- a/command/apply_destroy_test.go +++ b/command/apply_destroy_test.go @@ -358,7 +358,10 @@ func TestApply_destroyPath(t *testing.T) { } } -func TestApply_destroyTargeted(t *testing.T) { +// Config with multiple resources with dependencies, targeting destroy of a +// root node, expecting all other resources to be destroyed due to +// dependencies. +func TestApply_destroyTargetedDependencies(t *testing.T) { // Create a temporary working directory that is empty td := tempDir(t) testCopyDir(t, testFixturePath("apply-destroy-targeted"), td) @@ -488,6 +491,156 @@ func TestApply_destroyTargeted(t *testing.T) { } } +// Config with multiple resources with dependencies, targeting destroy of a +// leaf node, expecting the other resources to remain. +func TestApply_destroyTargeted(t *testing.T) { + // Create a temporary working directory that is empty + td := tempDir(t) + testCopyDir(t, testFixturePath("apply-destroy-targeted"), td) + defer os.RemoveAll(td) + defer testChdir(t, td)() + + originalState := states.BuildState(func(s *states.SyncState) { + s.SetResourceInstanceCurrent( + addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "test_instance", + Name: "foo", + }.Instance(addrs.IntKey(0)).Absolute(addrs.RootModuleInstance), + &states.ResourceInstanceObjectSrc{ + AttrsJSON: []byte(`{"id":"i-ab123"}`), + Status: states.ObjectReady, + }, + addrs.AbsProviderConfig{ + Provider: addrs.NewDefaultProvider("test"), + Module: addrs.RootModule, + }, + ) + s.SetResourceInstanceCurrent( + addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "test_load_balancer", + Name: "foo", + }.Instance(addrs.NoKey).Absolute(addrs.RootModuleInstance), + &states.ResourceInstanceObjectSrc{ + AttrsJSON: []byte(`{"id":"i-abc123"}`), + Dependencies: []addrs.ConfigResource{mustResourceAddr("test_instance.foo")}, + Status: states.ObjectReady, + }, + addrs.AbsProviderConfig{ + Provider: addrs.NewDefaultProvider("test"), + Module: addrs.RootModule, + }, + ) + }) + wantState := states.BuildState(func(s *states.SyncState) { + s.SetResourceInstanceCurrent( + addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "test_instance", + Name: "foo", + }.Instance(addrs.IntKey(0)).Absolute(addrs.RootModuleInstance), + &states.ResourceInstanceObjectSrc{ + AttrsJSON: []byte(`{"id":"i-ab123"}`), + Status: states.ObjectReady, + }, + addrs.AbsProviderConfig{ + Provider: addrs.NewDefaultProvider("test"), + Module: addrs.RootModule, + }, + ) + }) + statePath := testStateFile(t, originalState) + + p := testProvider() + p.GetSchemaResponse = &providers.GetSchemaResponse{ + ResourceTypes: map[string]providers.Schema{ + "test_instance": { + Block: &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "id": {Type: cty.String, Computed: true}, + }, + }, + }, + "test_load_balancer": { + Block: &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "id": {Type: cty.String, Computed: true}, + "instances": {Type: cty.List(cty.String), Optional: true}, + }, + }, + }, + }, + } + p.PlanResourceChangeFn = func(req providers.PlanResourceChangeRequest) providers.PlanResourceChangeResponse { + return providers.PlanResourceChangeResponse{ + PlannedState: req.ProposedNewState, + } + } + + ui := new(cli.MockUi) + c := &ApplyCommand{ + Destroy: true, + Meta: Meta{ + testingOverrides: metaOverridesForProvider(p), + Ui: ui, + }, + } + + // Run the apply command pointing to our existing state + args := []string{ + "-auto-approve", + "-target", "test_load_balancer.foo", + "-state", statePath, + } + if code := c.Run(args); code != 0 { + t.Fatalf("bad: %d\n\n%s", code, ui.ErrorWriter.String()) + } + + // Verify a new state exists + if _, err := os.Stat(statePath); err != nil { + t.Fatalf("err: %s", err) + } + + f, err := os.Open(statePath) + if err != nil { + t.Fatalf("err: %s", err) + } + defer f.Close() + + stateFile, err := statefile.Read(f) + if err != nil { + t.Fatalf("err: %s", err) + } + if stateFile == nil || stateFile.State == nil { + t.Fatal("state should not be nil") + } + + actualStr := strings.TrimSpace(stateFile.State.String()) + expectedStr := strings.TrimSpace(wantState.String()) + if actualStr != expectedStr { + t.Fatalf("bad:\n\nactual:\n%s\n\nexpected:\nb%s", actualStr, expectedStr) + } + + // Should have a backup file + f, err = os.Open(statePath + DefaultBackupExtension) + if err != nil { + t.Fatalf("err: %s", err) + } + + backupStateFile, err := statefile.Read(f) + f.Close() + if err != nil { + t.Fatalf("err: %s", err) + } + + backupActualStr := strings.TrimSpace(backupStateFile.State.String()) + backupExpectedStr := strings.TrimSpace(originalState.String()) + if backupActualStr != backupExpectedStr { + t.Fatalf("bad:\n\nactual:\n%s\n\nexpected:\nb%s", backupActualStr, backupExpectedStr) + } +} + const testApplyDestroyStr = ` ` diff --git a/command/apply_test.go b/command/apply_test.go index ebc22e9b8..8857c2aaf 100644 --- a/command/apply_test.go +++ b/command/apply_test.go @@ -1702,6 +1702,92 @@ output = test testStateOutput(t, statePath, expected) } +// Config with multiple resources, targeting apply of a subset +func TestApply_targeted(t *testing.T) { + td := tempDir(t) + testCopyDir(t, testFixturePath("apply-targeted"), td) + defer os.RemoveAll(td) + defer testChdir(t, td)() + + p := testProvider() + p.GetSchemaResponse = &providers.GetSchemaResponse{ + ResourceTypes: map[string]providers.Schema{ + "test_instance": { + Block: &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "id": {Type: cty.String, Computed: true}, + }, + }, + }, + }, + } + p.PlanResourceChangeFn = func(req providers.PlanResourceChangeRequest) providers.PlanResourceChangeResponse { + return providers.PlanResourceChangeResponse{ + PlannedState: req.ProposedNewState, + } + } + + ui := new(cli.MockUi) + c := &ApplyCommand{ + Meta: Meta{ + testingOverrides: metaOverridesForProvider(p), + Ui: ui, + }, + } + + args := []string{ + "-auto-approve", + "-target", "test_instance.foo", + "-target", "test_instance.baz", + } + if code := c.Run(args); code != 0 { + t.Fatalf("bad: %d\n\n%s", code, ui.ErrorWriter.String()) + } + + if got, want := ui.OutputWriter.String(), "3 added, 0 changed, 0 destroyed"; !strings.Contains(got, want) { + t.Fatalf("bad change summary, want %q, got:\n%s", want, got) + } +} + +// Diagnostics for invalid -target flags +func TestApply_targetFlagsDiags(t *testing.T) { + testCases := map[string]string{ + "test_instance.": "Dot must be followed by attribute name.", + "test_instance": "Resource specification must include a resource type and name.", + } + + for target, wantDiag := range testCases { + t.Run(target, func(t *testing.T) { + td := testTempDir(t) + defer os.RemoveAll(td) + defer testChdir(t, td)() + + ui := new(cli.MockUi) + c := &ApplyCommand{ + Meta: Meta{ + Ui: ui, + }, + } + + args := []string{ + "-auto-approve", + "-target", target, + } + if code := c.Run(args); code != 1 { + t.Fatalf("bad: %d\n\n%s", code, ui.ErrorWriter.String()) + } + + got := ui.ErrorWriter.String() + if !strings.Contains(got, target) { + t.Fatalf("bad error output, want %q, got:\n%s", target, got) + } + if !strings.Contains(got, wantDiag) { + t.Fatalf("bad error output, want %q, got:\n%s", wantDiag, got) + } + }) + } +} + // applyFixtureSchema returns a schema suitable for processing the // configuration in testdata/apply . This schema should be // assigned to a mock provider named "test". diff --git a/command/flag_kv.go b/command/flag_kv.go index 9f38018af..b084c5135 100644 --- a/command/flag_kv.go +++ b/command/flag_kv.go @@ -3,11 +3,6 @@ package command import ( "fmt" "strings" - - "github.com/hashicorp/hcl/v2" - "github.com/hashicorp/hcl/v2/hclsyntax" - "github.com/hashicorp/terraform/addrs" - "github.com/hashicorp/terraform/tfdiags" ) // FlagStringKV is a flag.Value implementation for parsing user variables @@ -46,34 +41,3 @@ func (v *FlagStringSlice) Set(raw string) error { return nil } - -// FlagTargetSlice is a flag.Value implementation for parsing target addresses -// from the command line, such as -target=aws_instance.foo -target=aws_vpc.bar . -type FlagTargetSlice []addrs.Targetable - -func (v *FlagTargetSlice) String() string { - return "" -} - -func (v *FlagTargetSlice) Set(raw string) error { - // FIXME: This is not an ideal way to deal with this because it requires - // us to do parsing in a context where we can't nicely return errors - // to the user. - - var diags tfdiags.Diagnostics - synthFilename := fmt.Sprintf("-target=%q", raw) - traversal, syntaxDiags := hclsyntax.ParseTraversalAbs([]byte(raw), synthFilename, hcl.Pos{Line: 1, Column: 1}) - diags = diags.Append(syntaxDiags) - if syntaxDiags.HasErrors() { - return diags.Err() - } - - target, targetDiags := addrs.ParseTarget(traversal) - diags = diags.Append(targetDiags) - if targetDiags.HasErrors() { - return diags.Err() - } - - *v = append(*v, target.Subject) - return nil -} diff --git a/command/meta.go b/command/meta.go index d4f66da98..033c6f39b 100644 --- a/command/meta.go +++ b/command/meta.go @@ -15,6 +15,8 @@ import ( "time" plugin "github.com/hashicorp/go-plugin" + "github.com/hashicorp/hcl/v2" + "github.com/hashicorp/hcl/v2/hclsyntax" "github.com/hashicorp/terraform-svchost/disco" "github.com/hashicorp/terraform/addrs" "github.com/hashicorp/terraform/backend" @@ -164,7 +166,8 @@ type Meta struct { input bool // Targets for this context (private) - targets []addrs.Targetable + targets []addrs.Targetable + targetFlags []string // Internal fields color bool @@ -523,7 +526,7 @@ func (m *Meta) extendedFlagSet(n string) *flag.FlagSet { f := m.defaultFlagSet(n) f.BoolVar(&m.input, "input", true, "input") - f.Var((*FlagTargetSlice)(&m.targets), "target", "resource to target") + f.Var((*FlagStringSlice)(&m.targetFlags), "target", "resource to target") f.BoolVar(&m.compactWarnings, "compact-warnings", false, "use compact warnings") if m.variableArgs.items == nil { @@ -541,6 +544,43 @@ func (m *Meta) extendedFlagSet(n string) *flag.FlagSet { return f } +// parseTargetFlags must be called for any commands supporting -target +// arguments. This method attempts to parse each -target flag into an +// addrs.Target, storing in the Meta.targets slice. +// +// If any flags cannot be parsed, we rewrap the first error diagnostic with a +// custom title to clarify the source of the error. The normal approach of +// directly returning the diags from HCL or the addrs package results in +// confusing incorrect "source" results when presented. +func (m *Meta) parseTargetFlags() tfdiags.Diagnostics { + var diags tfdiags.Diagnostics + m.targets = nil + for _, tf := range m.targetFlags { + traversal, syntaxDiags := hclsyntax.ParseTraversalAbs([]byte(tf), "", hcl.Pos{Line: 1, Column: 1}) + if syntaxDiags.HasErrors() { + diags = diags.Append(tfdiags.Sourceless( + tfdiags.Error, + fmt.Sprintf("Invalid target %q", tf), + syntaxDiags[0].Detail, + )) + continue + } + + target, targetDiags := addrs.ParseTarget(traversal) + if targetDiags.HasErrors() { + diags = diags.Append(tfdiags.Sourceless( + tfdiags.Error, + fmt.Sprintf("Invalid target %q", tf), + targetDiags[0].Description().Detail, + )) + continue + } + + m.targets = append(m.targets, target.Subject) + } + return diags +} + // process will process the meta-parameters out of the arguments. This // will potentially modify the args in-place. It will return the resulting // slice. diff --git a/command/plan.go b/command/plan.go index 7175c0602..ce5ea0800 100644 --- a/command/plan.go +++ b/command/plan.go @@ -32,6 +32,13 @@ func (c *PlanCommand) Run(args []string) int { cmdFlags.DurationVar(&c.Meta.stateLockTimeout, "lock-timeout", 0, "lock timeout") cmdFlags.Usage = func() { c.Ui.Error(c.Help()) } if err := cmdFlags.Parse(args); err != nil { + c.Ui.Error(fmt.Sprintf("Error parsing command-line flags: %s\n", err.Error())) + return 1 + } + + diags := c.parseTargetFlags() + if diags.HasErrors() { + c.showDiagnostics(diags) return 1 } @@ -47,8 +54,6 @@ func (c *PlanCommand) Run(args []string) int { return 1 } - var diags tfdiags.Diagnostics - var backendConfig *configs.Backend var configDiags tfdiags.Diagnostics backendConfig, configDiags = c.loadBackendConfig(configPath) diff --git a/command/plan_test.go b/command/plan_test.go index 357371fd4..c9c09476e 100644 --- a/command/plan_test.go +++ b/command/plan_test.go @@ -901,6 +901,90 @@ func TestPlan_init_required(t *testing.T) { } } +// Config with multiple resources, targeting plan of a subset +func TestPlan_targeted(t *testing.T) { + td := tempDir(t) + testCopyDir(t, testFixturePath("apply-targeted"), td) + defer os.RemoveAll(td) + defer testChdir(t, td)() + + p := testProvider() + p.GetSchemaResponse = &providers.GetSchemaResponse{ + ResourceTypes: map[string]providers.Schema{ + "test_instance": { + Block: &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "id": {Type: cty.String, Computed: true}, + }, + }, + }, + }, + } + p.PlanResourceChangeFn = func(req providers.PlanResourceChangeRequest) providers.PlanResourceChangeResponse { + return providers.PlanResourceChangeResponse{ + PlannedState: req.ProposedNewState, + } + } + + ui := new(cli.MockUi) + c := &PlanCommand{ + Meta: Meta{ + testingOverrides: metaOverridesForProvider(p), + Ui: ui, + }, + } + + args := []string{ + "-target", "test_instance.foo", + "-target", "test_instance.baz", + } + if code := c.Run(args); code != 0 { + t.Fatalf("bad: %d\n\n%s", code, ui.ErrorWriter.String()) + } + + if got, want := ui.OutputWriter.String(), "3 to add, 0 to change, 0 to destroy"; !strings.Contains(got, want) { + t.Fatalf("bad change summary, want %q, got:\n%s", want, got) + } +} + +// Diagnostics for invalid -target flags +func TestPlan_targetFlagsDiags(t *testing.T) { + testCases := map[string]string{ + "test_instance.": "Dot must be followed by attribute name.", + "test_instance": "Resource specification must include a resource type and name.", + } + + for target, wantDiag := range testCases { + t.Run(target, func(t *testing.T) { + td := testTempDir(t) + defer os.RemoveAll(td) + defer testChdir(t, td)() + + ui := new(cli.MockUi) + c := &PlanCommand{ + Meta: Meta{ + Ui: ui, + }, + } + + args := []string{ + "-target", target, + } + if code := c.Run(args); code != 1 { + t.Fatalf("bad: %d\n\n%s", code, ui.ErrorWriter.String()) + } + + got := ui.ErrorWriter.String() + if !strings.Contains(got, target) { + t.Fatalf("bad error output, want %q, got:\n%s", target, got) + } + if !strings.Contains(got, wantDiag) { + t.Fatalf("bad error output, want %q, got:\n%s", wantDiag, got) + } + }) + } +} + // planFixtureSchema returns a schema suitable for processing the // configuration in testdata/plan . This schema should be // assigned to a mock provider named "test". diff --git a/command/refresh.go b/command/refresh.go index 42ed0c4d1..5c512d711 100644 --- a/command/refresh.go +++ b/command/refresh.go @@ -25,6 +25,13 @@ func (c *RefreshCommand) Run(args []string) int { cmdFlags.DurationVar(&c.Meta.stateLockTimeout, "lock-timeout", 0, "lock timeout") cmdFlags.Usage = func() { c.Ui.Error(c.Help()) } if err := cmdFlags.Parse(args); err != nil { + c.Ui.Error(fmt.Sprintf("Error parsing command-line flags: %s\n", err.Error())) + return 1 + } + + diags := c.parseTargetFlags() + if diags.HasErrors() { + c.showDiagnostics(diags) return 1 } @@ -34,8 +41,6 @@ func (c *RefreshCommand) Run(args []string) int { return 1 } - var diags tfdiags.Diagnostics - // Check for user-supplied plugin path if c.pluginPath, err = c.loadPluginPath(); err != nil { c.Ui.Error(fmt.Sprintf("Error loading plugin path: %s", err)) diff --git a/command/refresh_test.go b/command/refresh_test.go index 339290677..e8e30f1bd 100644 --- a/command/refresh_test.go +++ b/command/refresh_test.go @@ -736,6 +736,97 @@ func TestRefresh_displaysOutputs(t *testing.T) { } } +// Config with multiple resources, targeting refresh of a subset +func TestRefresh_targeted(t *testing.T) { + td := tempDir(t) + testCopyDir(t, testFixturePath("refresh-targeted"), td) + defer os.RemoveAll(td) + defer testChdir(t, td)() + + state := testState() + statePath := testStateFile(t, state) + + p := testProvider() + p.GetSchemaResponse = &providers.GetSchemaResponse{ + ResourceTypes: map[string]providers.Schema{ + "test_instance": { + Block: &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "id": {Type: cty.String, Computed: true}, + }, + }, + }, + }, + } + p.PlanResourceChangeFn = func(req providers.PlanResourceChangeRequest) providers.PlanResourceChangeResponse { + return providers.PlanResourceChangeResponse{ + PlannedState: req.ProposedNewState, + } + } + + ui := new(cli.MockUi) + c := &RefreshCommand{ + Meta: Meta{ + testingOverrides: metaOverridesForProvider(p), + Ui: ui, + }, + } + + args := []string{ + "-target", "test_instance.foo", + "-state", statePath, + } + if code := c.Run(args); code != 0 { + t.Fatalf("bad: %d\n\n%s", code, ui.ErrorWriter.String()) + } + + got := ui.OutputWriter.String() + if want := "test_instance.foo: Refreshing"; !strings.Contains(got, want) { + t.Fatalf("expected output to contain %q, got:\n%s", want, got) + } + if doNotWant := "test_instance.bar: Refreshing"; strings.Contains(got, doNotWant) { + t.Fatalf("expected output not to contain %q, got:\n%s", doNotWant, got) + } +} + +// Diagnostics for invalid -target flags +func TestRefresh_targetFlagsDiags(t *testing.T) { + testCases := map[string]string{ + "test_instance.": "Dot must be followed by attribute name.", + "test_instance": "Resource specification must include a resource type and name.", + } + + for target, wantDiag := range testCases { + t.Run(target, func(t *testing.T) { + td := testTempDir(t) + defer os.RemoveAll(td) + defer testChdir(t, td)() + + ui := new(cli.MockUi) + c := &RefreshCommand{ + Meta: Meta{ + Ui: ui, + }, + } + + args := []string{ + "-target", target, + } + if code := c.Run(args); code != 1 { + t.Fatalf("bad: %d\n\n%s", code, ui.ErrorWriter.String()) + } + + got := ui.ErrorWriter.String() + if !strings.Contains(got, target) { + t.Fatalf("bad error output, want %q, got:\n%s", target, got) + } + if !strings.Contains(got, wantDiag) { + t.Fatalf("bad error output, want %q, got:\n%s", wantDiag, got) + } + }) + } +} + // configuration in testdata/refresh . This schema should be // assigned to a mock provider named "test". func refreshFixtureSchema() *providers.GetSchemaResponse { diff --git a/command/testdata/apply-destroy-targeted/main.tf b/command/testdata/apply-destroy-targeted/main.tf index 45ebc5b97..0f249b384 100644 --- a/command/testdata/apply-destroy-targeted/main.tf +++ b/command/testdata/apply-destroy-targeted/main.tf @@ -3,5 +3,5 @@ resource "test_instance" "foo" { } resource "test_load_balancer" "foo" { - instances = ["${test_instance.foo.*.id}"] + instances = test_instance.foo.*.id } diff --git a/command/testdata/apply-targeted/main.tf b/command/testdata/apply-targeted/main.tf new file mode 100644 index 000000000..1b6c42450 --- /dev/null +++ b/command/testdata/apply-targeted/main.tf @@ -0,0 +1,9 @@ +resource "test_instance" "foo" { + count = 2 +} + +resource "test_instance" "bar" { +} + +resource "test_instance" "baz" { +} diff --git a/command/testdata/refresh-targeted/main.tf b/command/testdata/refresh-targeted/main.tf new file mode 100644 index 000000000..734f58549 --- /dev/null +++ b/command/testdata/refresh-targeted/main.tf @@ -0,0 +1,7 @@ +resource "test_instance" "foo" { + id = "foo" +} + +resource "test_instance" "bar" { + id = "bar" +}