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

@@ -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

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 = `
 <no state>
 `

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".

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
-}

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.

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)

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".

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))

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 {

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
 }

command/testdata/apply-targeted/main.tf

@@ -0,0 +1,9 @@
+resource "test_instance" "foo" {
+  count = 2
+}
+
+resource "test_instance" "bar" {
+}
+
+resource "test_instance" "baz" {
+}

command/testdata/refresh-targeted/main.tf

@@ -0,0 +1,7 @@
+resource "test_instance" "foo" {
+  id = "foo"
+}
+
+resource "test_instance" "bar" {
+  id = "bar"
+}