From c8b58e949b54653eb7710e85bc152ee34ce6f67d Mon Sep 17 00:00:00 2001 From: Christian Mesh Date: Tue, 9 Sep 2025 07:32:25 -0400 Subject: [PATCH] Address review comments Co-authored-by: James Humphries Co-authored-by: Ilia Gogotchuri Signed-off-by: Christian Mesh --- internal/backend/local/backend_local.go | 6 +-- internal/command/format/object_id.go | 16 +++--- internal/genconfig/generate_config.go | 52 +++++++++---------- internal/lang/funcs/ephemeral_test.go | 1 - internal/lang/funcs/redact.go | 9 +++- internal/lang/funcs/redact_test.go | 2 +- internal/plugin/validation/write_only.go | 3 +- internal/plugin6/validation/write_only.go | 1 + website/docs/cli/commands/apply.mdx | 2 +- website/docs/language/ephemerality/index.mdx | 4 +- .../ephemerality/write-only-attributes.mdx | 3 +- 11 files changed, 54 insertions(+), 45 deletions(-) diff --git a/internal/backend/local/backend_local.go b/internal/backend/local/backend_local.go index 2df9993ced..bf6b455dcb 100644 --- a/internal/backend/local/backend_local.go +++ b/internal/backend/local/backend_local.go @@ -429,17 +429,17 @@ func generateEphemeralPlanValues(vv map[string]backend.UnparsedVariableValue, vc if diags.HasErrors() { return nil, diags } - ephemeralVars, ephemeralDiags := ephemeralValuesForPlanDuringFromUserInput(parsedVars, vcfgs) + ephemeralVars, ephemeralDiags := ephemeralValuesForPlanFromVariables(parsedVars, vcfgs) diags = diags.Append(ephemeralDiags) return ephemeralVars, diags } -// ephemeralValuesForPlanDuringFromUserInput is creating a map ready to be given to the plan to merge these together +// ephemeralValuesForPlanFromVariables is creating a map ready to be given to the plan to merge these together // with the variables that are already in the plan. // This function is handling only the ephemeral variables since those are the only ones that are not // stored in the plan, so the only way to pass then into the apply phase is to provide them again // in the -var/-var-file. -func ephemeralValuesForPlanDuringFromUserInput(parsedVars tofu.InputValues, variables map[string]*configs.Variable) (map[string]cty.Value, tfdiags.Diagnostics) { +func ephemeralValuesForPlanFromVariables(parsedVars tofu.InputValues, variables map[string]*configs.Variable) (map[string]cty.Value, tfdiags.Diagnostics) { var diags tfdiags.Diagnostics out := make(map[string]cty.Value) for vn, vc := range variables { diff --git a/internal/command/format/object_id.go b/internal/command/format/object_id.go index ad75901b2c..22216c272a 100644 --- a/internal/command/format/object_id.go +++ b/internal/command/format/object_id.go @@ -10,6 +10,10 @@ import ( "github.com/zclconf/go-cty/cty" ) +func isValueMarkedUnusable(v cty.Value) bool { + return v.HasMark(marks.Sensitive) || v.HasMark(marks.Ephemeral) +} + // ObjectValueID takes a value that is assumed to be an object representation // of some resource instance object and attempts to heuristically find an // attribute of it that is likely to be a unique identifier in the remote @@ -36,7 +40,7 @@ func ObjectValueID(obj cty.Value) (k, v string) { case atys["id"] == cty.String: v := obj.GetAttr("id") - if v.HasMark(marks.Sensitive) || v.HasMark(marks.Ephemeral) { + if isValueMarkedUnusable(v) { break } v, _ = v.Unmark() @@ -49,7 +53,7 @@ func ObjectValueID(obj cty.Value) (k, v string) { // "name" isn't always globally unique, but if there isn't also an // "id" then it _often_ is, in practice. v := obj.GetAttr("name") - if v.HasMark(marks.Sensitive) || v.HasMark(marks.Ephemeral) { + if isValueMarkedUnusable(v) { break } v, _ = v.Unmark() @@ -93,7 +97,7 @@ func ObjectValueName(obj cty.Value) (k, v string) { case atys["name"] == cty.String: v := obj.GetAttr("name") - if v.HasMark(marks.Sensitive) || v.HasMark(marks.Ephemeral) { + if isValueMarkedUnusable(v) { break } v, _ = v.Unmark() @@ -104,7 +108,7 @@ func ObjectValueName(obj cty.Value) (k, v string) { case atys["tags"].IsMapType() && atys["tags"].ElementType() == cty.String: tags := obj.GetAttr("tags") - if tags.IsNull() || !tags.IsWhollyKnown() || tags.HasMark(marks.Sensitive) || tags.HasMark(marks.Ephemeral) { + if tags.IsNull() || !tags.IsWhollyKnown() || isValueMarkedUnusable(tags) { break } tags, _ = tags.Unmark() @@ -112,7 +116,7 @@ func ObjectValueName(obj cty.Value) (k, v string) { switch { case tags.HasIndex(cty.StringVal("name")).RawEquals(cty.True): v := tags.Index(cty.StringVal("name")) - if v.HasMark(marks.Sensitive) || v.HasMark(marks.Ephemeral) { + if isValueMarkedUnusable(v) { break } v, _ = v.Unmark() @@ -123,7 +127,7 @@ func ObjectValueName(obj cty.Value) (k, v string) { case tags.HasIndex(cty.StringVal("Name")).RawEquals(cty.True): // AWS-style naming convention v := tags.Index(cty.StringVal("Name")) - if v.HasMark(marks.Sensitive) || v.HasMark(marks.Ephemeral) { + if isValueMarkedUnusable(v) { break } v, _ = v.Unmark() diff --git a/internal/genconfig/generate_config.go b/internal/genconfig/generate_config.go index 09e2e329d4..0c678c4252 100644 --- a/internal/genconfig/generate_config.go +++ b/internal/genconfig/generate_config.go @@ -95,7 +95,7 @@ func writeConfigAttributes(addr addrs.AbsResourceInstance, buf *strings.Builder, if !hclsyntax.ValidIdentifier(name) { name = string(hclwrite.TokensForValue(cty.StringVal(name)).Bytes()) } - _, _ = fmt.Fprintf(buf, "%s = ", name) + fmt.Fprintf(buf, "%s = ", name) tok := hclwrite.TokensForValue(attrS.EmptyValue()) if _, err := tok.WriteTo(buf); err != nil { diags = diags.Append(&hcl.Diagnostic{ @@ -113,7 +113,7 @@ func writeConfigAttributes(addr addrs.AbsResourceInstance, buf *strings.Builder, if !hclsyntax.ValidIdentifier(name) { name = string(hclwrite.TokensForValue(cty.StringVal(name)).Bytes()) } - _, _ = fmt.Fprintf(buf, "%s = ", name) + fmt.Fprintf(buf, "%s = ", name) tok := hclwrite.TokensForValue(attrS.EmptyValue()) if _, err := tok.WriteTo(buf); err != nil { diags = diags.Append(&hcl.Diagnostic{ @@ -154,7 +154,7 @@ func writeConfigAttributesFromExisting(addr addrs.AbsResourceInstance, buf *stri // Exclude computed-only attributes if attrS.Required || attrS.Optional { buf.WriteString(strings.Repeat(" ", indent)) - _, _ = fmt.Fprintf(buf, "%s = ", name) + fmt.Fprintf(buf, "%s = ", name) var val cty.Value if !stateVal.IsNull() && stateVal.Type().HasAttribute(name) { @@ -163,7 +163,7 @@ func writeConfigAttributesFromExisting(addr addrs.AbsResourceInstance, buf *stri val = attrS.EmptyValue() } if attrS.Sensitive || val.HasMark(marks.Sensitive) { - _, _ = fmt.Fprintf(buf, "null # sensitive%s", writeOnlyComment(attrS, false)) + fmt.Fprintf(buf, "null # sensitive%s", writeOnlyComment(attrS, false)) } else { if val.Type() == cty.String { unmarked, valMarks := val.Unmark() @@ -191,7 +191,7 @@ func writeConfigAttributesFromExisting(addr addrs.AbsResourceInstance, buf *stri }) continue } - _, _ = fmt.Fprintf(buf, "%s", writeOnlyComment(attrS, true)) + fmt.Fprintf(buf, "%s", writeOnlyComment(attrS, true)) } buf.WriteString("\n") @@ -228,7 +228,7 @@ func writeConfigNestedBlock(addr addrs.AbsResourceInstance, buf *strings.Builder switch schema.Nesting { case configschema.NestingSingle, configschema.NestingGroup: buf.WriteString(strings.Repeat(" ", indent)) - _, _ = fmt.Fprintf(buf, "%s {", name) + fmt.Fprintf(buf, "%s {", name) writeBlockTypeConstraint(buf, schema) diags = diags.Append(writeConfigAttributes(addr, buf, schema.Attributes, indent+2)) diags = diags.Append(writeConfigBlocks(addr, buf, schema.BlockTypes, indent+2)) @@ -236,7 +236,7 @@ func writeConfigNestedBlock(addr addrs.AbsResourceInstance, buf *strings.Builder return diags case configschema.NestingList, configschema.NestingSet: buf.WriteString(strings.Repeat(" ", indent)) - _, _ = fmt.Fprintf(buf, "%s {", name) + fmt.Fprintf(buf, "%s {", name) writeBlockTypeConstraint(buf, schema) diags = diags.Append(writeConfigAttributes(addr, buf, schema.Attributes, indent+2)) diags = diags.Append(writeConfigBlocks(addr, buf, schema.BlockTypes, indent+2)) @@ -245,7 +245,7 @@ func writeConfigNestedBlock(addr addrs.AbsResourceInstance, buf *strings.Builder case configschema.NestingMap: buf.WriteString(strings.Repeat(" ", indent)) // we use an arbitrary placeholder key (block label) "key" - _, _ = fmt.Fprintf(buf, "%s \"key\" {", name) + fmt.Fprintf(buf, "%s \"key\" {", name) writeBlockTypeConstraint(buf, schema) diags = diags.Append(writeConfigAttributes(addr, buf, schema.Attributes, indent+2)) diags = diags.Append(writeConfigBlocks(addr, buf, schema.BlockTypes, indent+2)) @@ -262,7 +262,7 @@ func writeConfigNestedTypeAttribute(addr addrs.AbsResourceInstance, buf *strings var diags tfdiags.Diagnostics buf.WriteString(strings.Repeat(" ", indent)) - _, _ = fmt.Fprintf(buf, "%s = ", name) + fmt.Fprintf(buf, "%s = ", name) switch schema.NestedType.Nesting { case configschema.NestingSingle: @@ -333,7 +333,7 @@ func writeConfigNestedTypeAttributeFromExisting(addr addrs.AbsResourceInstance, case configschema.NestingSingle: if schema.Sensitive || stateVal.HasMark(marks.Sensitive) { buf.WriteString(strings.Repeat(" ", indent)) - _, _ = fmt.Fprintf(buf, "%s = {} # sensitive%s\n", name, writeOnlyComment(schema, false)) + fmt.Fprintf(buf, "%s = {} # sensitive%s\n", name, writeOnlyComment(schema, false)) return diags } @@ -349,12 +349,12 @@ func writeConfigNestedTypeAttributeFromExisting(addr addrs.AbsResourceInstance, // There is a difference between a null object, and an object with // no attributes. buf.WriteString(strings.Repeat(" ", indent)) - _, _ = fmt.Fprintf(buf, "%s = null%s\n", name, writeOnlyComment(schema, true)) + fmt.Fprintf(buf, "%s = null%s\n", name, writeOnlyComment(schema, true)) return diags } buf.WriteString(strings.Repeat(" ", indent)) - _, _ = fmt.Fprintf(buf, "%s = {\n", name) + fmt.Fprintf(buf, "%s = {\n", name) diags = diags.Append(writeConfigAttributesFromExisting(addr, buf, nestedVal, schema.NestedType.Attributes, indent+2)) buf.WriteString("}\n") return diags @@ -363,7 +363,7 @@ func writeConfigNestedTypeAttributeFromExisting(addr addrs.AbsResourceInstance, if schema.Sensitive || stateVal.HasMark(marks.Sensitive) { buf.WriteString(strings.Repeat(" ", indent)) - _, _ = fmt.Fprintf(buf, "%s = [] # sensitive%s\n", name, writeOnlyComment(schema, false)) + fmt.Fprintf(buf, "%s = [] # sensitive%s\n", name, writeOnlyComment(schema, false)) return diags } @@ -371,12 +371,12 @@ func writeConfigNestedTypeAttributeFromExisting(addr addrs.AbsResourceInstance, if listVals == nil { // There is a difference between an empty list and a null list buf.WriteString(strings.Repeat(" ", indent)) - _, _ = fmt.Fprintf(buf, "%s = null%s\n", name, writeOnlyComment(schema, true)) + fmt.Fprintf(buf, "%s = null%s\n", name, writeOnlyComment(schema, true)) return diags } buf.WriteString(strings.Repeat(" ", indent)) - _, _ = fmt.Fprintf(buf, "%s = [\n", name) + fmt.Fprintf(buf, "%s = [\n", name) for i := range listVals { buf.WriteString(strings.Repeat(" ", indent+2)) // The entire element is marked. @@ -397,7 +397,7 @@ func writeConfigNestedTypeAttributeFromExisting(addr addrs.AbsResourceInstance, case configschema.NestingMap: if schema.Sensitive || stateVal.HasMark(marks.Sensitive) { buf.WriteString(strings.Repeat(" ", indent)) - _, _ = fmt.Fprintf(buf, "%s = {} # sensitive%s\n", name, writeOnlyComment(schema, false)) + fmt.Fprintf(buf, "%s = {} # sensitive%s\n", name, writeOnlyComment(schema, false)) return diags } @@ -405,7 +405,7 @@ func writeConfigNestedTypeAttributeFromExisting(addr addrs.AbsResourceInstance, if attr.IsNull() { // There is a difference between an empty map and a null map. buf.WriteString(strings.Repeat(" ", indent)) - _, _ = fmt.Fprintf(buf, "%s = null%s\n", name, writeOnlyComment(schema, true)) + fmt.Fprintf(buf, "%s = null%s\n", name, writeOnlyComment(schema, true)) return diags } @@ -418,10 +418,10 @@ func writeConfigNestedTypeAttributeFromExisting(addr addrs.AbsResourceInstance, sort.Strings(keys) buf.WriteString(strings.Repeat(" ", indent)) - _, _ = fmt.Fprintf(buf, "%s = {\n", name) + fmt.Fprintf(buf, "%s = {\n", name) for _, key := range keys { buf.WriteString(strings.Repeat(" ", indent+2)) - _, _ = fmt.Fprintf(buf, "%s = {", key) + fmt.Fprintf(buf, "%s = {", key) // This entire value is marked if vals[key].HasMark(marks.Sensitive) { @@ -453,7 +453,7 @@ func writeConfigNestedBlockFromExisting(addr addrs.AbsResourceInstance, buf *str return diags } buf.WriteString(strings.Repeat(" ", indent)) - _, _ = fmt.Fprintf(buf, "%s {", name) + fmt.Fprintf(buf, "%s {", name) // If the entire value is marked, don't print any nested attributes if stateVal.HasMark(marks.Sensitive) { @@ -468,13 +468,13 @@ func writeConfigNestedBlockFromExisting(addr addrs.AbsResourceInstance, buf *str case configschema.NestingList, configschema.NestingSet: if stateVal.HasMark(marks.Sensitive) { buf.WriteString(strings.Repeat(" ", indent)) - _, _ = fmt.Fprintf(buf, "%s {} # sensitive\n", name) + fmt.Fprintf(buf, "%s {} # sensitive\n", name) return diags } listVals := ctyCollectionValues(stateVal) for i := range listVals { buf.WriteString(strings.Repeat(" ", indent)) - _, _ = fmt.Fprintf(buf, "%s {\n", name) + fmt.Fprintf(buf, "%s {\n", name) diags = diags.Append(writeConfigAttributesFromExisting(addr, buf, listVals[i], schema.Attributes, indent+2)) diags = diags.Append(writeConfigBlocksFromExisting(addr, buf, listVals[i], schema.BlockTypes, indent+2)) buf.WriteString("}\n") @@ -483,7 +483,7 @@ func writeConfigNestedBlockFromExisting(addr addrs.AbsResourceInstance, buf *str case configschema.NestingMap: // If the entire value is marked, don't print any nested attributes if stateVal.HasMark(marks.Sensitive) { - _, _ = fmt.Fprintf(buf, "%s {} # sensitive\n", name) + fmt.Fprintf(buf, "%s {} # sensitive\n", name) return diags } @@ -496,7 +496,7 @@ func writeConfigNestedBlockFromExisting(addr addrs.AbsResourceInstance, buf *str sort.Strings(keys) for _, key := range keys { buf.WriteString(strings.Repeat(" ", indent)) - _, _ = fmt.Fprintf(buf, "%s %q {", name, key) + fmt.Fprintf(buf, "%s %q {", name, key) // This entire map element is marked if vals[key].HasMark(marks.Sensitive) { buf.WriteString("} # sensitive\n") @@ -523,9 +523,9 @@ func writeAttrTypeConstraint(buf *strings.Builder, schema *configschema.Attribut } if schema.NestedType != nil { - _, _ = fmt.Fprintf(buf, "%s\n", schema.NestedType.ImpliedType().FriendlyName()) + fmt.Fprintf(buf, "%s\n", schema.NestedType.ImpliedType().FriendlyName()) } else { - _, _ = fmt.Fprintf(buf, "%s\n", schema.Type.FriendlyName()) + fmt.Fprintf(buf, "%s\n", schema.Type.FriendlyName()) } } diff --git a/internal/lang/funcs/ephemeral_test.go b/internal/lang/funcs/ephemeral_test.go index f21595da86..26a096e3f3 100644 --- a/internal/lang/funcs/ephemeral_test.go +++ b/internal/lang/funcs/ephemeral_test.go @@ -124,5 +124,4 @@ func TestEphemeralAsNullFunc(t *testing.T) { } }) } - } diff --git a/internal/lang/funcs/redact.go b/internal/lang/funcs/redact.go index 5bb8c31c58..2b3a54fb8a 100644 --- a/internal/lang/funcs/redact.go +++ b/internal/lang/funcs/redact.go @@ -13,10 +13,15 @@ import ( ) func redactIfSensitiveOrEphemeral(value interface{}, valueMarks ...cty.ValueMarks) string { - if marks.Has(cty.DynamicVal.WithMarks(valueMarks...), marks.Ephemeral) { + isEphemeral := marks.Has(cty.DynamicVal.WithMarks(valueMarks...), marks.Ephemeral) + isSensitive := marks.Has(cty.DynamicVal.WithMarks(valueMarks...), marks.Sensitive) + if isEphemeral && isSensitive { + return "(ephemeral sensitive value)" + } + if isEphemeral { return "(ephemeral value)" } - if marks.Has(cty.DynamicVal.WithMarks(valueMarks...), marks.Sensitive) { + if isSensitive { return "(sensitive value)" } switch v := value.(type) { diff --git a/internal/lang/funcs/redact_test.go b/internal/lang/funcs/redact_test.go index 039455128a..a98571f0ab 100644 --- a/internal/lang/funcs/redact_test.go +++ b/internal/lang/funcs/redact_test.go @@ -51,7 +51,7 @@ func TestRedactIfSensitive(t *testing.T) { "ephemeral and sensitive string": { value: "foo", marks: []cty.ValueMarks{cty.NewValueMarks(marks.Ephemeral, marks.Sensitive)}, - want: "(ephemeral value)", + want: "(ephemeral sensitive value)", }, } diff --git a/internal/plugin/validation/write_only.go b/internal/plugin/validation/write_only.go index b35e9f4053..fafc212453 100644 --- a/internal/plugin/validation/write_only.go +++ b/internal/plugin/validation/write_only.go @@ -22,6 +22,7 @@ import ( // and there is no issue in the provider SDK when it comes to the write-only attributes. // Returning those with actual values can create unknown behavior leading to possible confidential // information exposure. +// NOTE: Keep this in sync with the equivalent in internal/plugin6/validation/write_only.go func WriteOnlyAttributes(schema *configschema.Block, v cty.Value, resAddr string) (diags tfdiags.Diagnostics) { if !schema.ContainsWriteOnly() { return diags @@ -32,7 +33,7 @@ func WriteOnlyAttributes(schema *configschema.Block, v cty.Value, resAddr string pathVal, err := path.Apply(v) if err != nil { - log.Printf("[WARN] Error when tried to get the path (%s) value from the given object: %s", pathAsString, err) + log.Printf("[WARN] Error when trying to get the path (%s) value from the given object: %s", pathAsString, err) continue } if pathVal.IsNull() { diff --git a/internal/plugin6/validation/write_only.go b/internal/plugin6/validation/write_only.go index b35e9f4053..f084af9599 100644 --- a/internal/plugin6/validation/write_only.go +++ b/internal/plugin6/validation/write_only.go @@ -22,6 +22,7 @@ import ( // and there is no issue in the provider SDK when it comes to the write-only attributes. // Returning those with actual values can create unknown behavior leading to possible confidential // information exposure. +// NOTE: Keep this in sync with the equivalent in internal/plugin/validation/write_only.go func WriteOnlyAttributes(schema *configschema.Block, v cty.Value, resAddr string) (diags tfdiags.Diagnostics) { if !schema.ContainsWriteOnly() { return diags diff --git a/website/docs/cli/commands/apply.mdx b/website/docs/cli/commands/apply.mdx index 9377f48e0e..3a1fadc0e8 100644 --- a/website/docs/cli/commands/apply.mdx +++ b/website/docs/cli/commands/apply.mdx @@ -39,7 +39,7 @@ decisions. #### Ephemeral variables Since ephemeral variables can't be stored in a planfile, any ephemeral variables set during the generation of a planfile from `tofu plan` must also be set when running tofu apply. -For example, if `-var` has been used to create the planfile (`tofu plan -var "token=$MY_TOKEN"`), the same argument _must_ +For example, if `-var` has been used to provide an ephemeral variable `token` to create the planfile (`tofu plan -var "token=$MY_TOKEN"`), the same argument _must_ be set for the apply command (`tofu apply -var "token=$MY_TOKEN" planfile`). If required ephemeral variables are not provided to the apply command, OpenTofu will exit with a clear message of additional variables that must be specified. diff --git a/website/docs/language/ephemerality/index.mdx b/website/docs/language/ephemerality/index.mdx index f1c0aa23d1..7f556ecb94 100644 --- a/website/docs/language/ephemerality/index.mdx +++ b/website/docs/language/ephemerality/index.mdx @@ -36,7 +36,7 @@ As part of this concept, the following constructs can be used: ## Compatibility :::warning This concept is inspired by and aims for compatibility with the equivalent "Ephemeral" concept in Terraform v1.12. -If incompatibilities are discovered, we will consider accepting breaking changes in subsequent versions +If incompatibilities are discovered, the OpenTofu team will consider accepting breaking changes in subsequent versions of OpenTofu to ensure compatibility. ::: @@ -59,7 +59,7 @@ terraform { required_providers { aws = { source = "hashicorp/aws" - version = "6.0.0-beta1" + version = ">=6.0.0" } } } diff --git a/website/docs/language/ephemerality/write-only-attributes.mdx b/website/docs/language/ephemerality/write-only-attributes.mdx index e47c678752..adfbbccb69 100644 --- a/website/docs/language/ephemerality/write-only-attributes.mdx +++ b/website/docs/language/ephemerality/write-only-attributes.mdx @@ -11,8 +11,7 @@ description: >- Write-only attributes can be used only with OpenTofu v1.11 onwards. ::: -This attribute is only found in [`managed resources`](../resources/index.mdx) -designed to accept transient values that will never be stored in the state or plan. +This attribute is only found in [`managed resources`](../resources/index.mdx) that are designed to accept transient values that will never be stored in the state or plan. For example, a secret can be read by using an ephemeral resource and then passed into the write-only attribute `password_wo` of a managed resource.