From f1358f9fe8ada76e2a65e1bd033596b0d82c85d9 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Thu, 7 Nov 2024 13:48:10 -0800 Subject: [PATCH] evalchecks: Suggest -exclude as a workaround for unknown count/for_each Previously we made a very generic suggestion to use -target to split a change into two parts as a workaround for the fact that count and for_each must be known during planning. That works, but we didn't have enough information available to tell the operator exactly what to target and so anyone who is not an expert on the configuration they're working with tends to get stuck unable to figure out exactly what they need to do. The new -exclude option gives us an opportunity to do better here: we tend to know for which object we're currently evaluating count or for_each, and so we can mention that object directly in the error message when if we recommend to use -exclude instead of -target. Not all objects that support count/for_each will necessarily be directly targetable, so we can still potentially recommend -target when we're dealing with one of those objects. For example, as of this commit that is true for for_each in a provider block, because there is not currently any syntax for specifying a provider configuration as an addrs.Targetable. Perhaps we'll introduce such a thing in the future, but that's outside the scope of this change that's primarily focused on improving the messaging for resource and module count/for_each. Signed-off-by: Martin Atkins --- internal/configs/provider.go | 2 +- internal/lang/evalchecks/diagnostics.go | 44 +++++++ internal/lang/evalchecks/eval_count.go | 43 ++++++- internal/lang/evalchecks/eval_count_test.go | 100 +++++++++++++++- internal/lang/evalchecks/eval_for_each.go | 63 ++++++++-- .../lang/evalchecks/eval_for_each_test.go | 112 +++++++++++++++++- internal/tofu/context_import.go | 2 +- internal/tofu/eval_count_test.go | 2 +- internal/tofu/eval_iteration.go | 12 +- internal/tofu/node_module_expand.go | 6 +- internal/tofu/node_resource_abstract.go | 4 +- .../tofu/node_resource_abstract_instance.go | 8 +- internal/tofu/node_resource_plan_instance.go | 2 +- internal/tofu/node_resource_validate.go | 2 +- 14 files changed, 361 insertions(+), 41 deletions(-) diff --git a/internal/configs/provider.go b/internal/configs/provider.go index 1c36119e54..dc0835fed7 100644 --- a/internal/configs/provider.go +++ b/internal/configs/provider.go @@ -179,7 +179,7 @@ func (p *Provider) decodeStaticFields(eval *StaticEvaluator) hcl.Diagnostics { return evalContext, diags.Append(evalDiags) } - forVal, evalDiags := evalchecks.EvaluateForEachExpression(p.ForEach, forEachRefsFunc) + forVal, evalDiags := evalchecks.EvaluateForEachExpression(p.ForEach, forEachRefsFunc, nil) diags = append(diags, evalDiags.ToHCL()...) if evalDiags.HasErrors() { return diags diff --git a/internal/lang/evalchecks/diagnostics.go b/internal/lang/evalchecks/diagnostics.go index 1c91d1fe65..1150e1d785 100644 --- a/internal/lang/evalchecks/diagnostics.go +++ b/internal/lang/evalchecks/diagnostics.go @@ -6,6 +6,7 @@ package evalchecks import ( + "github.com/apparentlymart/go-shquot/shquot" "github.com/opentofu/opentofu/internal/tfdiags" ) @@ -45,3 +46,46 @@ var _ tfdiags.DiagnosticExtraBecauseSensitive = DiagnosticCausedBySensitive(true func (e DiagnosticCausedBySensitive) DiagnosticCausedBySensitive() bool { return bool(e) } + +// commandLineArgumentsSuggestion returns a representation of the given command line +// arguments that includes suitable quoting or escaping to make it more likely to work +// unaltered if copy-pasted into the command line on the current host system. +// +// We don't try to determine exactly what shell someone is using, so this won't be +// 100% correct in all cases but will at least deal with the most common hazards of +// quoting strings that contain spaces, and escaping some metacharacters. +// +// The "goos" argument should be the value of [runtime.GOOS] when calling this from +// real code, but can be fixed to a particular value when writing unit tests to ensure +// that they behave the same regardless of what platform is being used for development. +func commandLineArgumentsSuggestion(args []string, goos string) string { + // For now we assume that there are only two possibilities: Windows or "POSIX-ish". + // We use the "shquot" library for this, since we already have it as + // a dependency for other purposes anyway. + var quot shquot.QS + switch goos { + case "windows": + // "WindowsArgvSplit" produces something that's compatible with + // the de-facto standard argument parsing implemented by Microsoft's + // Visual C++ runtime library, which the Go runtime mimics and + // is therefore the most likely to succeed for running OpenTofu. + // + // Unfortunately running normal programs through PowerShell adds + // some additional quoting/escaping hazards which we don't attend + // to here because doing so tends to make the result invalid + // for use outside of PowerShell. + quot = shquot.WindowsArgvSplit + default: + // We'll assume that all other platforms we support are compatible + // with the POSIX shell escaping rules. + quot = shquot.POSIXShellSplit + } + + // We're only interested in the "arguments" part of the result, but + // the shquot library wants us to provide an argv[0] anyway so we'll + // hard-code that as "tofu" but then ignore it altogether in the + // result. + cmdLine := append([]string{"tofu"}, args...) + _, ret := quot(cmdLine) + return ret +} diff --git a/internal/lang/evalchecks/eval_count.go b/internal/lang/evalchecks/eval_count.go index 7d91275784..a9b16c340e 100644 --- a/internal/lang/evalchecks/eval_count.go +++ b/internal/lang/evalchecks/eval_count.go @@ -6,8 +6,10 @@ package evalchecks import ( "fmt" + "runtime" "github.com/hashicorp/hcl/v2" + "github.com/opentofu/opentofu/internal/addrs" "github.com/opentofu/opentofu/internal/tfdiags" "github.com/zclconf/go-cty/cty" "github.com/zclconf/go-cty/cty/gocty" @@ -23,7 +25,11 @@ type EvaluateFunc func(expr hcl.Expression) (cty.Value, tfdiags.Diagnostics) // EvaluateCountExpression differs from EvaluateCountExpressionValue by // returning an error if the count value is not known, and converting the // cty.Value to an integer. -func EvaluateCountExpression(expr hcl.Expression, ctx EvaluateFunc) (int, tfdiags.Diagnostics) { +// +// If excludableAddr is non-nil then the unknown value error will include +// an additional idea to exclude that address using the -exclude +// planning option to converge over multiple plan/apply rounds. +func EvaluateCountExpression(expr hcl.Expression, ctx EvaluateFunc, excludableAddr addrs.Targetable) (int, tfdiags.Diagnostics) { countVal, diags := EvaluateCountExpressionValue(expr, ctx) if !countVal.IsKnown() { // Currently this is a rather bad outcome from a UX standpoint, since we have @@ -32,10 +38,12 @@ func EvaluateCountExpression(expr hcl.Expression, ctx EvaluateFunc) (int, tfdiag // FIXME: In future, implement a built-in mechanism for deferring changes that // can't yet be predicted, and use it to guide the user through several // plan/apply steps until the desired configuration is eventually reached. + + suggestion := countCommandLineExcludeSuggestion(excludableAddr) diags = diags.Append(&hcl.Diagnostic{ Severity: hcl.DiagError, Summary: "Invalid count argument", - Detail: `The "count" value depends on resource attributes that cannot be determined until apply, so OpenTofu cannot predict how many instances will be created. To work around this, use the -target argument to first apply only the resources that the count depends on.`, + Detail: "The \"count\" value depends on resource attributes that cannot be determined until apply, so OpenTofu cannot predict how many instances will be created.\n\n" + suggestion, Subject: expr.Range().Ptr(), // TODO: Also populate Expression and EvalContext in here, but @@ -111,3 +119,34 @@ func EvaluateCountExpressionValue(expr hcl.Expression, ctx EvaluateFunc) (cty.Va return countVal, diags } + +// Returns some English-language text describing a workaround using the -exclude +// planning option to converge over two plan/apply rounds when count has an +// unknown value. +// +// This is intended only for when a count value is too unknown for +// planning to proceed, in [EvaluateCountExpression]. +// +// If excludableAddr is non-nil then the message will refer to it directly, giving +// a full copy-pastable command line argument. Otherwise, the message is a generic +// one without any specific address indicated. +func countCommandLineExcludeSuggestion(excludableAddr addrs.Targetable) string { + // We use an extra indirection here so that we can write tests that make + // the same assertions on all development platforms. + return countCommandLineExcludeSuggestionImpl(excludableAddr, runtime.GOOS) +} + +func countCommandLineExcludeSuggestionImpl(excludableAddr addrs.Targetable, goos string) string { + if excludableAddr == nil { + // We use -target for this case because we can't be sure that the + // object we're complaining about even has its own addrs.Targetable + // address, and so the user might need to target only what it depends + // on instead. + return `To work around this, use the -target option to first apply only the resources that the count depends on, and then apply normally to converge.` + } + + return fmt.Sprintf( + "To work around this, use the planning option -exclude=%s to first apply without this object, and then apply normally to converge.", + commandLineArgumentsSuggestion([]string{excludableAddr.String()}, goos), + ) +} diff --git a/internal/lang/evalchecks/eval_count_test.go b/internal/lang/evalchecks/eval_count_test.go index e62aeeb618..8e8c4e6b8b 100644 --- a/internal/lang/evalchecks/eval_count_test.go +++ b/internal/lang/evalchecks/eval_count_test.go @@ -5,6 +5,7 @@ package evalchecks import ( + "fmt" "reflect" "strings" "testing" @@ -12,6 +13,7 @@ import ( "github.com/davecgh/go-spew/spew" "github.com/hashicorp/hcl/v2" "github.com/hashicorp/hcl/v2/hcltest" + "github.com/opentofu/opentofu/internal/addrs" "github.com/opentofu/opentofu/internal/tfdiags" "github.com/zclconf/go-cty/cty" ) @@ -36,7 +38,7 @@ func TestEvaluateCountExpression_valid(t *testing.T) { for name, test := range tests { t.Run(name, func(t *testing.T) { - actual, diags := EvaluateCountExpression(hcltest.MockExprLiteral(test.val), mockEvaluateFunc(test.val)) + actual, diags := EvaluateCountExpression(hcltest.MockExprLiteral(test.val), mockEvaluateFunc(test.val), nil) if len(diags) != 0 { t.Errorf("unexpected diagnostics %s", spew.Sdump(diags)) @@ -55,50 +57,68 @@ func TestEvaluateCountExpression_valid(t *testing.T) { func TestEvaluateCountExpression_errors(t *testing.T) { tests := map[string]struct { val cty.Value + excludableAddr addrs.Targetable Summary, DetailSubstring string CausedByUnknown bool }{ "null": { cty.NullVal(cty.Number), + nil, "Invalid count argument", `The given "count" argument value is null. An integer is required.`, false, }, "negative": { cty.NumberIntVal(-1), + nil, "Invalid count argument", `The given "count" argument value is unsuitable: must be greater than or equal to zero.`, false, }, "string": { cty.StringVal("i am definitely a number"), + nil, "Invalid count argument", "The given \"count\" argument value is unsuitable: number value is required.", false, }, "list": { cty.ListVal([]cty.Value{cty.StringVal("a"), cty.StringVal("a")}), + nil, "Invalid count argument", "The given \"count\" argument value is unsuitable: number value is required.", false, }, "tuple": { cty.TupleVal([]cty.Value{cty.StringVal("a"), cty.StringVal("b")}), + nil, "Invalid count argument", "The given \"count\" argument value is unsuitable: number value is required.", false, }, "unknown": { cty.UnknownVal(cty.Number), + nil, "Invalid count argument", - `he "count" value depends on resource attributes that cannot be determined until apply, so OpenTofu cannot predict how many instances will be created. To work around this, use the -target argument to first apply only the resources that the count depends on.`, + "The \"count\" value depends on resource attributes that cannot be determined until apply, so OpenTofu cannot predict how many instances will be created.\n\nTo work around this, use the -target option to first apply only the resources that the count depends on, and then apply normally to converge.", + true, + }, + "unknown with excludable address": { + cty.UnknownVal(cty.Number), + addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Name: "foo", + Type: "bar", + }.Absolute(addrs.RootModuleInstance.Child("a", addrs.NoKey)), + "Invalid count argument", + "The \"count\" value depends on resource attributes that cannot be determined until apply, so OpenTofu cannot predict how many instances will be created.\n\nTo work around this, use the planning option -exclude=module.a.bar.foo to first apply without this object, and then apply normally to converge.", true, }, } for name, test := range tests { t.Run(name, func(t *testing.T) { - _, diags := EvaluateCountExpression(hcltest.MockExprLiteral(test.val), mockEvaluateFunc(test.val)) + _, diags := EvaluateCountExpression(hcltest.MockExprLiteral(test.val), mockEvaluateFunc(test.val), test.excludableAddr) if len(diags) != 1 { t.Fatalf("got %d diagnostics; want 1", diags) @@ -119,3 +139,77 @@ func TestEvaluateCountExpression_errors(t *testing.T) { }) } } + +func TestCountCommandLineExcludeSuggestion(t *testing.T) { + noKeyResourceAddr := addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "happycloud_virtual_machine", + Name: "boop", + }.Absolute(addrs.RootModuleInstance) + stringKeyResourceAddr := addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "happycloud_virtual_machine", + Name: "boop", + }.Absolute(addrs.RootModuleInstance.Child("foo", addrs.StringKey("bleep bloop"))) + + tests := []struct { + excludableAddr addrs.Targetable + goos string + want string + }{ + { + nil, + "linux", + `To work around this, use the -target option to first apply only the resources that the count depends on, and then apply normally to converge.`, + }, + { + nil, + "windows", + `To work around this, use the -target option to first apply only the resources that the count depends on, and then apply normally to converge.`, + }, + { + nil, + "darwin", + `To work around this, use the -target option to first apply only the resources that the count depends on, and then apply normally to converge.`, + }, + { + noKeyResourceAddr, + "linux", + `To work around this, use the planning option -exclude=happycloud_virtual_machine.boop to first apply without this object, and then apply normally to converge.`, + }, + { + noKeyResourceAddr, + "windows", + `To work around this, use the planning option -exclude=happycloud_virtual_machine.boop to first apply without this object, and then apply normally to converge.`, + }, + { + noKeyResourceAddr, + "darwin", + `To work around this, use the planning option -exclude=happycloud_virtual_machine.boop to first apply without this object, and then apply normally to converge.`, + }, + { + stringKeyResourceAddr, + "linux", + `To work around this, use the planning option -exclude='module.foo["bleep bloop"].happycloud_virtual_machine.boop' to first apply without this object, and then apply normally to converge.`, + }, + { + stringKeyResourceAddr, + "windows", + `To work around this, use the planning option -exclude="module.foo[\"bleep bloop\"].happycloud_virtual_machine.boop" to first apply without this object, and then apply normally to converge.`, + }, + { + stringKeyResourceAddr, + "darwin", + `To work around this, use the planning option -exclude='module.foo["bleep bloop"].happycloud_virtual_machine.boop' to first apply without this object, and then apply normally to converge.`, + }, + } + + for _, test := range tests { + t.Run(fmt.Sprintf("%v on %s", test.excludableAddr, test.goos), func(t *testing.T) { + got := countCommandLineExcludeSuggestionImpl(test.excludableAddr, test.goos) + if got != test.want { + t.Errorf("wrong suggestion\ngot: %s\nwant: %s", got, test.want) + } + }) + } +} diff --git a/internal/lang/evalchecks/eval_for_each.go b/internal/lang/evalchecks/eval_for_each.go index d61dbfbe49..5552b1ce25 100644 --- a/internal/lang/evalchecks/eval_for_each.go +++ b/internal/lang/evalchecks/eval_for_each.go @@ -6,6 +6,7 @@ package evalchecks import ( "fmt" + "runtime" "github.com/hashicorp/hcl/v2" "github.com/opentofu/opentofu/internal/addrs" @@ -16,8 +17,8 @@ import ( ) const ( - errInvalidUnknownDetailMap = "The \"for_each\" map includes keys derived from resource attributes that cannot be determined until apply, and so OpenTofu cannot determine the full set of keys that will identify the instances of this resource.\n\nWhen working with unknown values in for_each, it's better to define the map keys statically in your configuration and place apply-time results only in the map values.\n\nAlternatively, you could use the -target planning option to first apply only the resources that the for_each value depends on, and then apply a second time to fully converge." - errInvalidUnknownDetailSet = "The \"for_each\" set includes values derived from resource attributes that cannot be determined until apply, and so OpenTofu cannot determine the full set of keys that will identify the instances of this resource.\n\nWhen working with unknown values in for_each, it's better to use a map value where the keys are defined statically in your configuration and where only the values contain apply-time results.\n\nAlternatively, you could use the -target planning option to first apply only the resources that the for_each value depends on, and then apply a second time to fully converge." + errInvalidUnknownDetailMap = "The \"for_each\" map includes keys derived from resource attributes that cannot be determined until apply, and so OpenTofu cannot determine the full set of keys that will identify the instances of this resource.\n\nWhen working with unknown values in for_each, it's better to define the map keys statically in your configuration and place apply-time results only in the map values.\n\n" + errInvalidUnknownDetailSet = "The \"for_each\" set includes values derived from resource attributes that cannot be determined until apply, and so OpenTofu cannot determine the full set of keys that will identify the instances of this resource.\n\nWhen working with unknown values in for_each, it's better to use a map value where the keys are defined statically in your configuration and where only the values contain apply-time results.\n\n" ) type ContextFunc func(refs []*addrs.Reference) (*hcl.EvalContext, tfdiags.Diagnostics) @@ -30,10 +31,14 @@ type ContextFunc func(refs []*addrs.Reference) (*hcl.EvalContext, tfdiags.Diagno // EvaluateForEachExpression differs from EvaluateForEachExpressionValue by // returning an error if the count value is not known, and converting the // cty.Value to a map[string]cty.Value for compatibility with other calls. -func EvaluateForEachExpression(expr hcl.Expression, ctx ContextFunc) (map[string]cty.Value, tfdiags.Diagnostics) { +// +// If excludableAddr is non-nil then the unknown value error will include +// an additional idea to exclude that address using the -exclude +// planning option to converge over multiple plan/apply rounds. +func EvaluateForEachExpression(expr hcl.Expression, ctx ContextFunc, excludableAddr addrs.Targetable) (map[string]cty.Value, tfdiags.Diagnostics) { const unknownsNotAllowed = false const tupleNotAllowed = false - forEachVal, diags := EvaluateForEachExpressionValue(expr, ctx, unknownsNotAllowed, tupleNotAllowed) + forEachVal, diags := EvaluateForEachExpressionValue(expr, ctx, unknownsNotAllowed, tupleNotAllowed, excludableAddr) // forEachVal might be unknown, but if it is then there should already // be an error about it in diags, which we'll return below. @@ -49,7 +54,11 @@ func EvaluateForEachExpression(expr hcl.Expression, ctx ContextFunc) (map[string // except that it returns a cty.Value map or set which can be unknown. // The 'allowTuple' argument is used to support evaluating for_each from tuple // values, and is currently supported when using for_each in import blocks. -func EvaluateForEachExpressionValue(expr hcl.Expression, ctx ContextFunc, allowUnknown bool, allowTuple bool) (cty.Value, tfdiags.Diagnostics) { +// +// If excludableAddr is non-nil then any unknown-value-related error will +// include an additional idea to exclude that address using the -exclude +// planning option to converge over multiple plan/apply rounds. +func EvaluateForEachExpressionValue(expr hcl.Expression, ctx ContextFunc, allowUnknown bool, allowTuple bool, excludableAddr addrs.Targetable) (cty.Value, tfdiags.Diagnostics) { var diags tfdiags.Diagnostics nullMap := cty.NullVal(cty.Map(cty.DynamicPseudoType)) @@ -112,7 +121,7 @@ func EvaluateForEachExpressionValue(expr hcl.Expression, ctx ContextFunc, allowU return nullMap, diags } - forEachVal, diags = performForEachValueChecks(expr, hclCtx, allowUnknown, forEachVal, allowedTypesMessage) + forEachVal, diags = performForEachValueChecks(expr, hclCtx, allowUnknown, forEachVal, allowedTypesMessage, excludableAddr) if diags.HasErrors() { return forEachVal, diags } @@ -121,7 +130,7 @@ func EvaluateForEachExpressionValue(expr hcl.Expression, ctx ContextFunc, allowU } // performForEachValueChecks ensures the for_each argument is valid -func performForEachValueChecks(expr hcl.Expression, hclCtx *hcl.EvalContext, allowUnknown bool, forEachVal cty.Value, allowedTypesMessage string) (cty.Value, tfdiags.Diagnostics) { +func performForEachValueChecks(expr hcl.Expression, hclCtx *hcl.EvalContext, allowUnknown bool, forEachVal cty.Value, allowedTypesMessage string, excludableAddr addrs.Targetable) (cty.Value, tfdiags.Diagnostics) { var diags tfdiags.Diagnostics nullMap := cty.NullVal(cty.Map(cty.DynamicPseudoType)) ty := forEachVal.Type() @@ -146,6 +155,7 @@ func performForEachValueChecks(expr hcl.Expression, hclCtx *hcl.EvalContext, all default: detailMsg = errInvalidUnknownDetailMap } + detailMsg += forEachCommandLineExcludeSuggestion(excludableAddr) diags = diags.Append(&hcl.Diagnostic{ Severity: hcl.DiagError, @@ -167,7 +177,7 @@ func performForEachValueChecks(expr hcl.Expression, hclCtx *hcl.EvalContext, all } if ty.IsSetType() { - setVal, setTypeDiags := performSetTypeChecks(expr, hclCtx, allowUnknown, forEachVal) + setVal, setTypeDiags := performSetTypeChecks(expr, hclCtx, allowUnknown, forEachVal, excludableAddr) diags = diags.Append(setTypeDiags) if diags.HasErrors() { return setVal, diags @@ -178,7 +188,7 @@ func performForEachValueChecks(expr hcl.Expression, hclCtx *hcl.EvalContext, all } // performSetTypeChecks does checks when we have a Set type, as sets have some gotchas -func performSetTypeChecks(expr hcl.Expression, hclCtx *hcl.EvalContext, allowUnknown bool, forEachVal cty.Value) (cty.Value, tfdiags.Diagnostics) { +func performSetTypeChecks(expr hcl.Expression, hclCtx *hcl.EvalContext, allowUnknown bool, forEachVal cty.Value, excludableAddr addrs.Targetable) (cty.Value, tfdiags.Diagnostics) { var diags tfdiags.Diagnostics ty := forEachVal.Type() @@ -189,7 +199,7 @@ func performSetTypeChecks(expr hcl.Expression, hclCtx *hcl.EvalContext, allowUnk diags = diags.Append(&hcl.Diagnostic{ Severity: hcl.DiagError, Summary: "Invalid for_each argument", - Detail: errInvalidUnknownDetailSet, + Detail: errInvalidUnknownDetailSet + forEachCommandLineExcludeSuggestion(excludableAddr), Subject: expr.Range().Ptr(), Expression: expr, EvalContext: hclCtx, @@ -236,3 +246,36 @@ func markSafeLengthInt(val cty.Value) int { v, _ := val.UnmarkDeep() return v.LengthInt() } + +// Returns some English-language text describing a workaround using the -exclude +// planning option to converge over two plan/apply rounds when for_each has an +// unknown value. +// +// This is intended only for when a for_each value is too unknown for +// planning to proceed, in [EvaluateForEachExpression] or [EvaluateForEachExpressionValue]. +// The message always begins with "Alternatively, " because it's intended to be +// appended to one of either [errInvalidUnknownDetailMap] or [errInvalidUnknownDetailSet]. +// +// If excludableAddr is non-nil then the message will refer to it directly, giving +// a full copy-pastable command line argument. Otherwise, the message is a generic +// one without any specific address indicated. +func forEachCommandLineExcludeSuggestion(excludableAddr addrs.Targetable) string { + // We use an extra indirection here so that we can write tests that make + // the same assertions on all development platforms. + return forEachCommandLineExcludeSuggestionImpl(excludableAddr, runtime.GOOS) +} + +func forEachCommandLineExcludeSuggestionImpl(excludableAddr addrs.Targetable, goos string) string { + if excludableAddr == nil { + // We use -target for this case because we can't be sure that the + // object we're complaining about even has its own addrs.Targetable + // address, and so the user might need to target only what it depends + // on instead. + return `Alternatively, you could use the -target option to first apply only the resources that for_each depends on, and then apply normally to converge.` + } + + return fmt.Sprintf( + `Alternatively, you could use the planning option -exclude=%s to first apply without this object, and then apply normally to converge.`, + commandLineArgumentsSuggestion([]string{excludableAddr.String()}, goos), + ) +} diff --git a/internal/lang/evalchecks/eval_for_each_test.go b/internal/lang/evalchecks/eval_for_each_test.go index d9b0742ff8..5ff284a775 100644 --- a/internal/lang/evalchecks/eval_for_each_test.go +++ b/internal/lang/evalchecks/eval_for_each_test.go @@ -5,6 +5,7 @@ package evalchecks import ( + "fmt" "reflect" "strings" "testing" @@ -81,7 +82,7 @@ func TestEvaluateForEachExpression(t *testing.T) { for name, test := range tests { t.Run(name, func(t *testing.T) { - forEachMap, diags := EvaluateForEachExpression(test.Expr, mockRefsFunc()) + forEachMap, diags := EvaluateForEachExpression(test.Expr, mockRefsFunc(), nil) if len(diags) != 0 { t.Errorf("unexpected diagnostics %s", spew.Sdump(diags)) @@ -100,47 +101,55 @@ func TestEvaluateForEachExpression(t *testing.T) { func TestEvaluateForEachExpression_errors(t *testing.T) { tests := map[string]struct { Expr hcl.Expression + ExcludableAddr addrs.Targetable Summary, DetailSubstring string CausedByUnknown, CausedBySensitive bool }{ "null set": { hcltest.MockExprLiteral(cty.NullVal(cty.Set(cty.String))), + nil, "Invalid for_each argument", `the given "for_each" argument value is null`, false, false, }, "string": { hcltest.MockExprLiteral(cty.StringVal("i am definitely a set")), + nil, "Invalid for_each argument", "must be a map, or set of strings, and you have provided a value of type string", false, false, }, "list": { hcltest.MockExprLiteral(cty.ListVal([]cty.Value{cty.StringVal("a"), cty.StringVal("a")})), + nil, "Invalid for_each argument", "must be a map, or set of strings, and you have provided a value of type list", false, false, }, "tuple": { hcltest.MockExprLiteral(cty.TupleVal([]cty.Value{cty.StringVal("a"), cty.StringVal("b")})), + nil, "Invalid for_each argument", "must be a map, or set of strings, and you have provided a value of type tuple", false, false, }, "unknown string set": { hcltest.MockExprLiteral(cty.UnknownVal(cty.Set(cty.String))), + nil, "Invalid for_each argument", "set includes values derived from resource attributes that cannot be determined until apply", true, false, }, "unknown map": { hcltest.MockExprLiteral(cty.UnknownVal(cty.Map(cty.Bool))), + nil, "Invalid for_each argument", "map includes keys derived from resource attributes that cannot be determined until apply", true, false, }, "unknown pseudo-type": { hcltest.MockExprLiteral(cty.UnknownVal(cty.DynamicPseudoType)), + nil, "Invalid for_each argument", "map includes keys derived from resource attributes that cannot be determined until apply", true, false, @@ -150,36 +159,53 @@ func TestEvaluateForEachExpression_errors(t *testing.T) { "a": cty.BoolVal(true), "b": cty.BoolVal(false), }).Mark(marks.Sensitive)), + nil, "Invalid for_each argument", "Sensitive values, or values derived from sensitive values, cannot be used as for_each arguments. If used, the sensitive value could be exposed as a resource instance key.", false, true, }, "set containing booleans": { hcltest.MockExprLiteral(cty.SetVal([]cty.Value{cty.BoolVal(true)})), + nil, "Invalid for_each set argument", "supports sets of strings, but you have provided a set containing type bool", false, false, }, "set containing null": { hcltest.MockExprLiteral(cty.SetVal([]cty.Value{cty.NullVal(cty.String)})), + nil, "Invalid for_each set argument", "must not contain null values", false, false, }, "set containing unknown value": { hcltest.MockExprLiteral(cty.SetVal([]cty.Value{cty.UnknownVal(cty.String)})), + nil, "Invalid for_each argument", - "set includes values derived from resource attributes that cannot be determined until apply", + "The \"for_each\" set includes values derived from resource attributes that cannot be determined until apply, and so OpenTofu cannot determine the full set of keys that will identify the instances of this resource.\n\nWhen working with unknown values in for_each, it's better to use a map value where the keys are defined statically in your configuration and where only the values contain apply-time results.\n\nAlternatively, you could use the -target option to first apply only the resources that for_each depends on, and then apply normally to converge.", + true, false, + }, + "set containing unknown value in excludable object": { + hcltest.MockExprLiteral(cty.SetVal([]cty.Value{cty.UnknownVal(cty.String)})), + addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Name: "foo", + Type: "bar", + }.Absolute(addrs.RootModuleInstance.Child("a", addrs.NoKey)), + "Invalid for_each argument", + "The \"for_each\" set includes values derived from resource attributes that cannot be determined until apply, and so OpenTofu cannot determine the full set of keys that will identify the instances of this resource.\n\nWhen working with unknown values in for_each, it's better to use a map value where the keys are defined statically in your configuration and where only the values contain apply-time results.\n\nAlternatively, you could use the planning option -exclude=module.a.bar.foo to first apply without this object, and then apply normally to converge.", true, false, }, "set containing dynamic unknown value": { hcltest.MockExprLiteral(cty.SetVal([]cty.Value{cty.UnknownVal(cty.DynamicPseudoType)})), + nil, "Invalid for_each argument", "set includes values derived from resource attributes that cannot be determined until apply", true, false, }, "set containing marked values": { hcltest.MockExprLiteral(cty.SetVal([]cty.Value{cty.StringVal("beep").Mark(marks.Sensitive), cty.StringVal("boop")})), + nil, "Invalid for_each argument", "Sensitive values, or values derived from sensitive values, cannot be used as for_each arguments. If used, the sensitive value could be exposed as a resource instance key.", false, true, @@ -188,7 +214,7 @@ func TestEvaluateForEachExpression_errors(t *testing.T) { for name, test := range tests { t.Run(name, func(t *testing.T) { - _, diags := EvaluateForEachExpression(test.Expr, mockRefsFunc()) + _, diags := EvaluateForEachExpression(test.Expr, mockRefsFunc(), test.ExcludableAddr) if len(diags) != 1 { t.Fatalf("got %d diagnostics; want 1", diags) @@ -293,7 +319,7 @@ func TestEvaluateForEachExpression_multi_errors(t *testing.T) { for name, test := range tests { t.Run(name, func(t *testing.T) { - _, diags := EvaluateForEachExpression(test.Expr, mockRefsFunc()) + _, diags := EvaluateForEachExpression(test.Expr, mockRefsFunc(), nil) if len(diags) != len(test.Wanted) { t.Errorf("unexpected diagnostics %s", spew.Sdump(diags)) } @@ -336,7 +362,7 @@ func TestEvaluateForEachExpressionKnown(t *testing.T) { for name, expr := range tests { t.Run(name, func(t *testing.T) { - forEachVal, diags := EvaluateForEachExpressionValue(expr, mockRefsFunc(), true, true) + forEachVal, diags := EvaluateForEachExpressionValue(expr, mockRefsFunc(), true, true, nil) if len(diags) != 0 { t.Errorf("unexpected diagnostics %s", spew.Sdump(diags)) @@ -382,7 +408,7 @@ func TestEvaluateForEachExpressionValueTuple(t *testing.T) { for name, test := range tests { t.Run(name, func(t *testing.T) { - _, diags := EvaluateForEachExpressionValue(test.Expr, mockRefsFunc(), true, test.AllowTuple) + _, diags := EvaluateForEachExpressionValue(test.Expr, mockRefsFunc(), true, test.AllowTuple, nil) if test.ExpectedError == "" { if len(diags) != 0 { @@ -396,3 +422,77 @@ func TestEvaluateForEachExpressionValueTuple(t *testing.T) { }) } } + +func TestForEachCommandLineExcludeSuggestion(t *testing.T) { + noKeyResourceAddr := addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "happycloud_virtual_machine", + Name: "boop", + }.Absolute(addrs.RootModuleInstance) + stringKeyResourceAddr := addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "happycloud_virtual_machine", + Name: "boop", + }.Absolute(addrs.RootModuleInstance.Child("foo", addrs.StringKey("bleep bloop"))) + + tests := []struct { + excludableAddr addrs.Targetable + goos string + want string + }{ + { + nil, + "linux", + `Alternatively, you could use the -target option to first apply only the resources that for_each depends on, and then apply normally to converge.`, + }, + { + nil, + "windows", + `Alternatively, you could use the -target option to first apply only the resources that for_each depends on, and then apply normally to converge.`, + }, + { + nil, + "darwin", + `Alternatively, you could use the -target option to first apply only the resources that for_each depends on, and then apply normally to converge.`, + }, + { + noKeyResourceAddr, + "linux", + `Alternatively, you could use the planning option -exclude=happycloud_virtual_machine.boop to first apply without this object, and then apply normally to converge.`, + }, + { + noKeyResourceAddr, + "windows", + `Alternatively, you could use the planning option -exclude=happycloud_virtual_machine.boop to first apply without this object, and then apply normally to converge.`, + }, + { + noKeyResourceAddr, + "darwin", + `Alternatively, you could use the planning option -exclude=happycloud_virtual_machine.boop to first apply without this object, and then apply normally to converge.`, + }, + { + stringKeyResourceAddr, + "linux", + `Alternatively, you could use the planning option -exclude='module.foo["bleep bloop"].happycloud_virtual_machine.boop' to first apply without this object, and then apply normally to converge.`, + }, + { + stringKeyResourceAddr, + "windows", + `Alternatively, you could use the planning option -exclude="module.foo[\"bleep bloop\"].happycloud_virtual_machine.boop" to first apply without this object, and then apply normally to converge.`, + }, + { + stringKeyResourceAddr, + "darwin", + `Alternatively, you could use the planning option -exclude='module.foo["bleep bloop"].happycloud_virtual_machine.boop' to first apply without this object, and then apply normally to converge.`, + }, + } + + for _, test := range tests { + t.Run(fmt.Sprintf("%v on %s", test.excludableAddr, test.goos), func(t *testing.T) { + got := forEachCommandLineExcludeSuggestionImpl(test.excludableAddr, test.goos) + if got != test.want { + t.Errorf("wrong suggestion\ngot: %s\nwant: %s", got, test.want) + } + }) + } +} diff --git a/internal/tofu/context_import.go b/internal/tofu/context_import.go index fd76d627f8..209e8bce77 100644 --- a/internal/tofu/context_import.go +++ b/internal/tofu/context_import.go @@ -127,7 +127,7 @@ func (ri *ImportResolver) ExpandAndResolveImport(importTarget *ImportTarget, ctx const tupleAllowed = true // The import target has a for_each attribute, so we need to expand it - forEachVal, evalDiags := evaluateForEachExpressionValue(importTarget.Config.ForEach, rootCtx, unknownsNotAllowed, tupleAllowed) + forEachVal, evalDiags := evaluateForEachExpressionValue(importTarget.Config.ForEach, rootCtx, unknownsNotAllowed, tupleAllowed, nil) diags = diags.Append(evalDiags) if diags.HasErrors() { return diags diff --git a/internal/tofu/eval_count_test.go b/internal/tofu/eval_count_test.go index ebb3b93f96..2cfcfc1b07 100644 --- a/internal/tofu/eval_count_test.go +++ b/internal/tofu/eval_count_test.go @@ -34,7 +34,7 @@ func TestEvaluateCountExpression(t *testing.T) { t.Run(name, func(t *testing.T) { ctx := &MockEvalContext{} ctx.installSimpleEval() - countVal, diags := evaluateCountExpression(test.Expr, ctx) + countVal, diags := evaluateCountExpression(test.Expr, ctx, nil) if len(diags) != 0 { t.Errorf("unexpected diagnostics %s", spew.Sdump(diags)) diff --git a/internal/tofu/eval_iteration.go b/internal/tofu/eval_iteration.go index 81c31fa654..65a4a52caf 100644 --- a/internal/tofu/eval_iteration.go +++ b/internal/tofu/eval_iteration.go @@ -32,16 +32,16 @@ func evalContextEvaluate(ctx EvalContext) evalchecks.EvaluateFunc { } } -func evaluateForEachExpression(expr hcl.Expression, ctx EvalContext) (map[string]cty.Value, tfdiags.Diagnostics) { - return evalchecks.EvaluateForEachExpression(expr, evalContextScope(ctx)) +func evaluateForEachExpression(expr hcl.Expression, ctx EvalContext, excludeableAddr addrs.Targetable) (map[string]cty.Value, tfdiags.Diagnostics) { + return evalchecks.EvaluateForEachExpression(expr, evalContextScope(ctx), excludeableAddr) } -func evaluateForEachExpressionValue(expr hcl.Expression, ctx EvalContext, allowUnknown bool, allowTuple bool) (cty.Value, tfdiags.Diagnostics) { - return evalchecks.EvaluateForEachExpressionValue(expr, evalContextScope(ctx), allowUnknown, allowTuple) +func evaluateForEachExpressionValue(expr hcl.Expression, ctx EvalContext, allowUnknown bool, allowTuple bool, excludeableAddr addrs.Targetable) (cty.Value, tfdiags.Diagnostics) { + return evalchecks.EvaluateForEachExpressionValue(expr, evalContextScope(ctx), allowUnknown, allowTuple, excludeableAddr) } -func evaluateCountExpression(expr hcl.Expression, ctx EvalContext) (int, tfdiags.Diagnostics) { - return evalchecks.EvaluateCountExpression(expr, evalContextEvaluate(ctx)) +func evaluateCountExpression(expr hcl.Expression, ctx EvalContext, excludeableAddr addrs.Targetable) (int, tfdiags.Diagnostics) { + return evalchecks.EvaluateCountExpression(expr, evalContextEvaluate(ctx), excludeableAddr) } func evaluateCountExpressionValue(expr hcl.Expression, ctx EvalContext) (cty.Value, tfdiags.Diagnostics) { diff --git a/internal/tofu/node_module_expand.go b/internal/tofu/node_module_expand.go index bf336084f2..ffe4a7a80f 100644 --- a/internal/tofu/node_module_expand.go +++ b/internal/tofu/node_module_expand.go @@ -123,7 +123,7 @@ func (n *nodeExpandModule) Execute(ctx EvalContext, op walkOperation) (diags tfd ctx = ctx.WithPath(module) switch { case n.ModuleCall.Count != nil: - count, ctDiags := evaluateCountExpression(n.ModuleCall.Count, ctx) + count, ctDiags := evaluateCountExpression(n.ModuleCall.Count, ctx, module) diags = diags.Append(ctDiags) if diags.HasErrors() { return diags @@ -131,7 +131,7 @@ func (n *nodeExpandModule) Execute(ctx EvalContext, op walkOperation) (diags tfd expander.SetModuleCount(module, call, count) case n.ModuleCall.ForEach != nil: - forEach, feDiags := evaluateForEachExpression(n.ModuleCall.ForEach, ctx) + forEach, feDiags := evaluateForEachExpression(n.ModuleCall.ForEach, ctx, module) diags = diags.Append(feDiags) if diags.HasErrors() { return diags @@ -268,7 +268,7 @@ func (n *nodeValidateModule) Execute(ctx EvalContext, op walkOperation) (diags t case n.ModuleCall.ForEach != nil: const unknownsAllowed = true const tupleNotAllowed = false - _, forEachDiags := evaluateForEachExpressionValue(n.ModuleCall.ForEach, ctx, unknownsAllowed, tupleNotAllowed) + _, forEachDiags := evaluateForEachExpressionValue(n.ModuleCall.ForEach, ctx, unknownsAllowed, tupleNotAllowed, module) diags = diags.Append(forEachDiags) } diff --git a/internal/tofu/node_resource_abstract.go b/internal/tofu/node_resource_abstract.go index 838d4b7cdf..d2e779ffdd 100644 --- a/internal/tofu/node_resource_abstract.go +++ b/internal/tofu/node_resource_abstract.go @@ -485,7 +485,7 @@ func (n *NodeAbstractResource) writeResourceState(ctx EvalContext, addr addrs.Ab switch { case n.Config != nil && n.Config.Count != nil: - count, countDiags := evaluateCountExpression(n.Config.Count, ctx) + count, countDiags := evaluateCountExpression(n.Config.Count, ctx, addr) diags = diags.Append(countDiags) if countDiags.HasErrors() { return diags @@ -495,7 +495,7 @@ func (n *NodeAbstractResource) writeResourceState(ctx EvalContext, addr addrs.Ab expander.SetResourceCount(addr.Module, n.Addr.Resource, count) case n.Config != nil && n.Config.ForEach != nil: - forEach, forEachDiags := evaluateForEachExpression(n.Config.ForEach, ctx) + forEach, forEachDiags := evaluateForEachExpression(n.Config.ForEach, ctx, addr) diags = diags.Append(forEachDiags) if forEachDiags.HasErrors() { return diags diff --git a/internal/tofu/node_resource_abstract_instance.go b/internal/tofu/node_resource_abstract_instance.go index 896405cf9a..44d758c415 100644 --- a/internal/tofu/node_resource_abstract_instance.go +++ b/internal/tofu/node_resource_abstract_instance.go @@ -839,7 +839,7 @@ func (n *NodeAbstractResourceInstance) plan( } // Evaluate the configuration - forEach, _ := evaluateForEachExpression(n.Config.ForEach, ctx) + forEach, _ := evaluateForEachExpression(n.Config.ForEach, ctx, n.Addr) keyData = EvalDataForInstanceKey(n.ResourceInstanceAddr().Resource.Key, forEach) @@ -1741,7 +1741,7 @@ func (n *NodeAbstractResourceInstance) planDataSource(ctx EvalContext, checkRule objTy := schema.ImpliedType() priorVal := cty.NullVal(objTy) - forEach, _ := evaluateForEachExpression(config.ForEach, ctx) + forEach, _ := evaluateForEachExpression(config.ForEach, ctx, n.Addr) keyData = EvalDataForInstanceKey(n.ResourceInstanceAddr().Resource.Key, forEach) checkDiags := evalCheckRules( @@ -2020,7 +2020,7 @@ func (n *NodeAbstractResourceInstance) applyDataSource(ctx EvalContext, planned return nil, keyData, diags } - forEach, _ := evaluateForEachExpression(config.ForEach, ctx) + forEach, _ := evaluateForEachExpression(config.ForEach, ctx, n.Addr) keyData = EvalDataForInstanceKey(n.Addr.Resource.Key, forEach) checkDiags := evalCheckRules( @@ -2324,7 +2324,7 @@ func (n *NodeAbstractResourceInstance) applyProvisioners(ctx EvalContext, state func (n *NodeAbstractResourceInstance) evalProvisionerConfig(ctx EvalContext, body hcl.Body, self cty.Value, schema *configschema.Block) (cty.Value, tfdiags.Diagnostics) { var diags tfdiags.Diagnostics - forEach, forEachDiags := evaluateForEachExpression(n.Config.ForEach, ctx) + forEach, forEachDiags := evaluateForEachExpression(n.Config.ForEach, ctx, n.Addr) diags = diags.Append(forEachDiags) keyData := EvalDataForInstanceKey(n.ResourceInstanceAddr().Resource.Key, forEach) diff --git a/internal/tofu/node_resource_plan_instance.go b/internal/tofu/node_resource_plan_instance.go index 37ce946f34..4511c5b7ea 100644 --- a/internal/tofu/node_resource_plan_instance.go +++ b/internal/tofu/node_resource_plan_instance.go @@ -402,7 +402,7 @@ func (n *NodePlannableResourceInstance) managedResourceExecute(ctx EvalContext) // values, which could result in a post-condition check relying on that // value being inaccurate. Unless we decide to store the value of the // for-each expression in state, this is unavoidable. - forEach, _ := evaluateForEachExpression(n.Config.ForEach, ctx) + forEach, _ := evaluateForEachExpression(n.Config.ForEach, ctx, n.ResourceAddr()) repeatData := EvalDataForInstanceKey(n.ResourceInstanceAddr().Resource.Key, forEach) checkDiags := evalCheckRules( diff --git a/internal/tofu/node_resource_validate.go b/internal/tofu/node_resource_validate.go index 319cf8ceec..be3ba04494 100644 --- a/internal/tofu/node_resource_validate.go +++ b/internal/tofu/node_resource_validate.go @@ -568,7 +568,7 @@ func validateForEach(ctx EvalContext, expr hcl.Expression) (diags tfdiags.Diagno const unknownsAllowed = true const tupleNotAllowed = false - val, forEachDiags := evaluateForEachExpressionValue(expr, ctx, unknownsAllowed, tupleNotAllowed) + val, forEachDiags := evaluateForEachExpressionValue(expr, ctx, unknownsAllowed, tupleNotAllowed, nil) // If the value isn't known then that's the best we can do for now, but // we'll check more thoroughly during the plan walk if !val.IsKnown() {