From d6611bf622bd674980e94cdc794f91d29f08ad62 Mon Sep 17 00:00:00 2001 From: Liam Cervante Date: Fri, 3 Jan 2025 09:55:35 +0100 Subject: [PATCH 1/2] logging: add trace statements when changes are ignored --- internal/backend/local/backend_apply.go | 3 +- internal/command/views/json/diagnostic.go | 134 +----------------- internal/terraform/eval_context_builtin.go | 2 +- .../node_resource_abstract_instance.go | 28 +++- internal/terraform/node_resource_validate.go | 2 +- internal/terraform/reduce_plan_test.go | 3 +- internal/tfdiags/format.go | 133 +++++++++++++++++ 7 files changed, 164 insertions(+), 141 deletions(-) create mode 100644 internal/tfdiags/format.go diff --git a/internal/backend/local/backend_apply.go b/internal/backend/local/backend_apply.go index 4b9765d1b348..cb8bd0268180 100644 --- a/internal/backend/local/backend_apply.go +++ b/internal/backend/local/backend_apply.go @@ -16,7 +16,6 @@ import ( "github.com/hashicorp/terraform/internal/addrs" "github.com/hashicorp/terraform/internal/backend/backendrun" "github.com/hashicorp/terraform/internal/command/views" - viewsjson "github.com/hashicorp/terraform/internal/command/views/json" "github.com/hashicorp/terraform/internal/configs" "github.com/hashicorp/terraform/internal/logging" "github.com/hashicorp/terraform/internal/plans" @@ -348,7 +347,7 @@ func (b *Local) opApply( diags = diags.Append(&hcl.Diagnostic{ Severity: hcl.DiagError, Summary: "Can't change variable when applying a saved plan", - Detail: fmt.Sprintf("The variable %s cannot be set using the -var and -var-file options when applying a saved plan file, because a saved plan includes the variable values that were set when it was created. The saved plan specifies %s as the value whereas during apply the value %s was %s. To declare an ephemeral variable which is not saved in the plan file, use ephemeral = true.", varName, viewsjson.CompactValueStr(parsedVar.Value), viewsjson.CompactValueStr(plannedVar), parsedVar.SourceType.DiagnosticLabel()), + Detail: fmt.Sprintf("The variable %s cannot be set using the -var and -var-file options when applying a saved plan file, because a saved plan includes the variable values that were set when it was created. The saved plan specifies %s as the value whereas during apply the value %s was %s. To declare an ephemeral variable which is not saved in the plan file, use ephemeral = true.", varName, tfdiags.CompactValueStr(parsedVar.Value), tfdiags.CompactValueStr(plannedVar), parsedVar.SourceType.DiagnosticLabel()), Subject: rng, }) } diff --git a/internal/command/views/json/diagnostic.go b/internal/command/views/json/diagnostic.go index 4d4032814a08..daef06c1b186 100644 --- a/internal/command/views/json/diagnostic.go +++ b/internal/command/views/json/diagnostic.go @@ -5,7 +5,6 @@ package json import ( "bufio" - "bytes" "fmt" "sort" "strings" @@ -14,9 +13,10 @@ import ( "github.com/hashicorp/hcl/v2/hcled" "github.com/hashicorp/hcl/v2/hclparse" "github.com/hashicorp/hcl/v2/hclsyntax" + "github.com/zclconf/go-cty/cty" + "github.com/hashicorp/terraform/internal/lang/marks" "github.com/hashicorp/terraform/internal/tfdiags" - "github.com/zclconf/go-cty/cty" ) // These severities map to the tfdiags.Severity values, plus an explicit @@ -289,7 +289,7 @@ func NewDiagnostic(diag tfdiags.Diagnostic, sources map[string][]byte) *Diagnost continue } - traversalStr := traversalStr(traversal) + traversalStr := tfdiags.TraversalStr(traversal) if _, exists := seen[traversalStr]; exists { continue Traversals // don't show duplicates when the same variable is referenced multiple times } @@ -382,7 +382,7 @@ func NewDiagnostic(diag tfdiags.Diagnostic, sources map[string][]byte) *Diagnost value.Statement = "will be known only after apply" } default: - value.Statement = fmt.Sprintf("is %s", compactValueStr(val)) + value.Statement = fmt.Sprintf("is %s", tfdiags.CompactValueStr(val)) } values = append(values, value) seen[traversalStr] = struct{}{} @@ -437,129 +437,3 @@ func parseRange(src []byte, rng hcl.Range) (*hcl.File, int) { return file, offset } - -// CompactValueStr produces a compact, single-line summary of a given value -// that is suitable for display in the UI. -// -// For primitives it returns a full representation, while for more complex -// types it instead summarizes the type, size, etc to produce something -// that is hopefully still somewhat useful but not as verbose as a rendering -// of the entire data structure. -func CompactValueStr(val cty.Value) string { - return compactValueStr(val) -} - -func compactValueStr(val cty.Value) string { - // This is a specialized subset of value rendering tailored to producing - // helpful but concise messages in diagnostics. It is not comprehensive - // nor intended to be used for other purposes. - - val, valMarks := val.Unmark() - for mark := range valMarks { - switch mark { - case marks.Sensitive: - // We check this in here just to make sure, but note that the caller - // of compactValueStr ought to have already checked this and skipped - // calling into compactValueStr anyway, so this shouldn't actually - // be reachable. - return "(sensitive value)" - case marks.Ephemeral: - // A non-sensitive ephemeral value is fine to show in the UI. Values - // that are both ephemeral and sensitive should have both markings - // and should therefore get caught by the marks.Sensitive case - // above. - continue - default: - // We don't know about any other marks, so we'll be conservative. - // This shouldn't actuallyr eachable since the caller should've - // checked this and skipped calling compactValueStr anyway. - return "value with unrecognized marks (this is a bug in Terraform)" - } - } - - // WARNING: We've only checked that the value isn't sensitive _shallowly_ - // here, and so we must never show any element values from complex types - // in here. However, it's fine to show map keys and attribute names because - // those are never sensitive in isolation: the entire value would be - // sensitive in that case. - - ty := val.Type() - switch { - case val.IsNull(): - return "null" - case !val.IsKnown(): - // Should never happen here because we should filter before we get - // in here, but we'll do something reasonable rather than panic. - return "(not yet known)" - case ty == cty.Bool: - if val.True() { - return "true" - } - return "false" - case ty == cty.Number: - bf := val.AsBigFloat() - return bf.Text('g', 10) - case ty == cty.String: - // Go string syntax is not exactly the same as HCL native string syntax, - // but we'll accept the minor edge-cases where this is different here - // for now, just to get something reasonable here. - return fmt.Sprintf("%q", val.AsString()) - case ty.IsCollectionType() || ty.IsTupleType(): - l := val.LengthInt() - switch l { - case 0: - return "empty " + ty.FriendlyName() - case 1: - return ty.FriendlyName() + " with 1 element" - default: - return fmt.Sprintf("%s with %d elements", ty.FriendlyName(), l) - } - case ty.IsObjectType(): - atys := ty.AttributeTypes() - l := len(atys) - switch l { - case 0: - return "object with no attributes" - case 1: - var name string - for k := range atys { - name = k - } - return fmt.Sprintf("object with 1 attribute %q", name) - default: - return fmt.Sprintf("object with %d attributes", l) - } - default: - return ty.FriendlyName() - } -} - -// traversalStr produces a representation of an HCL traversal that is compact, -// resembles HCL native syntax, and is suitable for display in the UI. -func traversalStr(traversal hcl.Traversal) string { - // This is a specialized subset of traversal rendering tailored to - // producing helpful contextual messages in diagnostics. It is not - // comprehensive nor intended to be used for other purposes. - - var buf bytes.Buffer - for _, step := range traversal { - switch tStep := step.(type) { - case hcl.TraverseRoot: - buf.WriteString(tStep.Name) - case hcl.TraverseAttr: - buf.WriteByte('.') - buf.WriteString(tStep.Name) - case hcl.TraverseIndex: - buf.WriteByte('[') - if keyTy := tStep.Key.Type(); keyTy.IsPrimitiveType() { - buf.WriteString(compactValueStr(tStep.Key)) - } else { - // We'll just use a placeholder for more complex values, - // since otherwise our result could grow ridiculously long. - buf.WriteString("...") - } - buf.WriteByte(']') - } - } - return buf.String() -} diff --git a/internal/terraform/eval_context_builtin.go b/internal/terraform/eval_context_builtin.go index ec7226aac0a6..0a7610a78f49 100644 --- a/internal/terraform/eval_context_builtin.go +++ b/internal/terraform/eval_context_builtin.go @@ -407,7 +407,7 @@ func (ctx *BuiltinEvalContext) EvaluateReplaceTriggeredBy(expr hcl.Expression, r return nil, false, diags } - path := traversalToPath(ref.Remaining) + path, _ := traversalToPath(ref.Remaining) attrBefore, _ := path.Apply(change.Before) attrAfter, _ := path.Apply(change.After) diff --git a/internal/terraform/node_resource_abstract_instance.go b/internal/terraform/node_resource_abstract_instance.go index f41428104faf..e7f223e8df81 100644 --- a/internal/terraform/node_resource_abstract_instance.go +++ b/internal/terraform/node_resource_abstract_instance.go @@ -1252,7 +1252,7 @@ func (n *NodeAbstractResource) processIgnoreChanges(prior, config cty.Value, sch return config, nil } - ignoreChanges := traversalsToPaths(n.Config.Managed.IgnoreChanges) + ignoreChanges, keys := traversalsToPaths(n.Config.Managed.IgnoreChanges) ignoreAll := n.Config.Managed.IgnoreAllChanges if len(ignoreChanges) == 0 && !ignoreAll { @@ -1260,6 +1260,8 @@ func (n *NodeAbstractResource) processIgnoreChanges(prior, config cty.Value, sch } if ignoreAll { + log.Printf("[TRACE] processIgnoreChanges: Ignoring all changes for %s", n.Addr) + // Legacy providers need up to clean up their invalid plans and ensure // no changes are passed though, but that also means making an invalid // config with computed values. In that case we just don't supply a @@ -1282,6 +1284,7 @@ func (n *NodeAbstractResource) processIgnoreChanges(prior, config cty.Value, sch return ret, nil } + log.Printf("[TRACE] processIgnoreChanges: Ignoring changes for %s at [%s]", n.Addr, strings.Join(keys, ", ")) if prior.IsNull() || config.IsNull() { // Ignore changes doesn't apply when we're creating for the first time. @@ -1296,36 +1299,49 @@ func (n *NodeAbstractResource) processIgnoreChanges(prior, config cty.Value, sch // Convert the hcl.Traversal values we get form the configuration to the // cty.Path values we need to operate on the cty.Values -func traversalsToPaths(traversals []hcl.Traversal) []cty.Path { +func traversalsToPaths(traversals []hcl.Traversal) ([]cty.Path, []string) { paths := make([]cty.Path, len(traversals)) + keys := make([]string, len(traversals)) for i, traversal := range traversals { - path := traversalToPath(traversal) + path, key := traversalToPath(traversal) paths[i] = path + keys[i] = key } - return paths + return paths, keys } -func traversalToPath(traversal hcl.Traversal) cty.Path { +func traversalToPath(traversal hcl.Traversal) (cty.Path, string) { path := make(cty.Path, len(traversal)) + var key strings.Builder for si, step := range traversal { switch ts := step.(type) { case hcl.TraverseRoot: path[si] = cty.GetAttrStep{ Name: ts.Name, } + key.WriteString(ts.Name) case hcl.TraverseAttr: path[si] = cty.GetAttrStep{ Name: ts.Name, } + key.WriteString(".") + key.WriteString(ts.Name) case hcl.TraverseIndex: path[si] = cty.IndexStep{ Key: ts.Key, } + if ts.Key.Type().IsPrimitiveType() { + key.WriteString("[") + key.WriteString(tfdiags.CompactValueStr(ts.Key)) + key.WriteString("]") + } else { + key.WriteString("[...]") + } default: panic(fmt.Sprintf("unsupported traversal step %#v", step)) } } - return path + return path, key.String() } func processIgnoreChangesIndividual(prior, config cty.Value, ignoreChangesPath []cty.Path) (cty.Value, tfdiags.Diagnostics) { diff --git a/internal/terraform/node_resource_validate.go b/internal/terraform/node_resource_validate.go index 7325eaaf4e8d..42d03a9fcff3 100644 --- a/internal/terraform/node_resource_validate.go +++ b/internal/terraform/node_resource_validate.go @@ -366,7 +366,7 @@ func (n *NodeValidatableResource) validateResource(ctx EvalContext) tfdiags.Diag // use that to check whether the Attribute is Computed and // non-Optional. if !diags.HasErrors() { - path := traversalToPath(traversal) + path, _ := traversalToPath(traversal) attrSchema := schema.AttributeByPath(path) diff --git a/internal/terraform/reduce_plan_test.go b/internal/terraform/reduce_plan_test.go index 2148b1954b9c..142782ec97c2 100644 --- a/internal/terraform/reduce_plan_test.go +++ b/internal/terraform/reduce_plan_test.go @@ -433,7 +433,8 @@ func TestProcessIgnoreChangesIndividual(t *testing.T) { ignore[i] = trav } - ret, diags := processIgnoreChangesIndividual(test.Old, test.New, traversalsToPaths(ignore)) + paths, _ := traversalsToPaths(ignore) + ret, diags := processIgnoreChangesIndividual(test.Old, test.New, paths) if diags.HasErrors() { t.Fatal(diags.Err()) } diff --git a/internal/tfdiags/format.go b/internal/tfdiags/format.go new file mode 100644 index 000000000000..80bfbb4347ee --- /dev/null +++ b/internal/tfdiags/format.go @@ -0,0 +1,133 @@ +package tfdiags + +import ( + "bytes" + "fmt" + + "github.com/hashicorp/hcl/v2" + "github.com/zclconf/go-cty/cty" + + "github.com/hashicorp/terraform/internal/lang/marks" +) + +// CompactValueStr produces a compact, single-line summary of a given value +// that is suitable for display in the UI. +// +// For primitives it returns a full representation, while for more complex +// types it instead summarizes the type, size, etc to produce something +// that is hopefully still somewhat useful but not as verbose as a rendering +// of the entire data structure. +func CompactValueStr(val cty.Value) string { + // This is a specialized subset of value rendering tailored to producing + // helpful but concise messages in diagnostics. It is not comprehensive + // nor intended to be used for other purposes. + + val, valMarks := val.Unmark() + for mark := range valMarks { + switch mark { + case marks.Sensitive: + // We check this in here just to make sure, but note that the caller + // of compactValueStr ought to have already checked this and skipped + // calling into compactValueStr anyway, so this shouldn't actually + // be reachable. + return "(sensitive value)" + case marks.Ephemeral: + // A non-sensitive ephemeral value is fine to show in the UI. Values + // that are both ephemeral and sensitive should have both markings + // and should therefore get caught by the marks.Sensitive case + // above. + continue + default: + // We don't know about any other marks, so we'll be conservative. + // This shouldn't actuallyr eachable since the caller should've + // checked this and skipped calling compactValueStr anyway. + return "value with unrecognized marks (this is a bug in Terraform)" + } + } + + // WARNING: We've only checked that the value isn't sensitive _shallowly_ + // here, and so we must never show any element values from complex types + // in here. However, it's fine to show map keys and attribute names because + // those are never sensitive in isolation: the entire value would be + // sensitive in that case. + + ty := val.Type() + switch { + case val.IsNull(): + return "null" + case !val.IsKnown(): + // Should never happen here because we should filter before we get + // in here, but we'll do something reasonable rather than panic. + return "(not yet known)" + case ty == cty.Bool: + if val.True() { + return "true" + } + return "false" + case ty == cty.Number: + bf := val.AsBigFloat() + return bf.Text('g', 10) + case ty == cty.String: + // Go string syntax is not exactly the same as HCL native string syntax, + // but we'll accept the minor edge-cases where this is different here + // for now, just to get something reasonable here. + return fmt.Sprintf("%q", val.AsString()) + case ty.IsCollectionType() || ty.IsTupleType(): + l := val.LengthInt() + switch l { + case 0: + return "empty " + ty.FriendlyName() + case 1: + return ty.FriendlyName() + " with 1 element" + default: + return fmt.Sprintf("%s with %d elements", ty.FriendlyName(), l) + } + case ty.IsObjectType(): + atys := ty.AttributeTypes() + l := len(atys) + switch l { + case 0: + return "object with no attributes" + case 1: + var name string + for k := range atys { + name = k + } + return fmt.Sprintf("object with 1 attribute %q", name) + default: + return fmt.Sprintf("object with %d attributes", l) + } + default: + return ty.FriendlyName() + } +} + +// TraversalStr produces a representation of an HCL traversal that is compact, +// resembles HCL native syntax, and is suitable for display in the UI. +func TraversalStr(traversal hcl.Traversal) string { + // This is a specialized subset of traversal rendering tailored to + // producing helpful contextual messages in diagnostics. It is not + // comprehensive nor intended to be used for other purposes. + + var buf bytes.Buffer + for _, step := range traversal { + switch tStep := step.(type) { + case hcl.TraverseRoot: + buf.WriteString(tStep.Name) + case hcl.TraverseAttr: + buf.WriteByte('.') + buf.WriteString(tStep.Name) + case hcl.TraverseIndex: + buf.WriteByte('[') + if keyTy := tStep.Key.Type(); keyTy.IsPrimitiveType() { + buf.WriteString(CompactValueStr(tStep.Key)) + } else { + // We'll just use a placeholder for more complex values, + // since otherwise our result could grow ridiculously long. + buf.WriteString("...") + } + buf.WriteByte(']') + } + } + return buf.String() +} From 2a0dafc9012914d49215dc2fb719741df76ea0d0 Mon Sep 17 00:00:00 2001 From: Liam Cervante Date: Fri, 3 Jan 2025 10:13:08 +0100 Subject: [PATCH 2/2] copywrite headers --- internal/tfdiags/format.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/internal/tfdiags/format.go b/internal/tfdiags/format.go index 80bfbb4347ee..c7e6bdc97cc3 100644 --- a/internal/tfdiags/format.go +++ b/internal/tfdiags/format.go @@ -1,3 +1,6 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: BUSL-1.1 + package tfdiags import (