From d523b9d409eb2916b4d86cb85d4cc8a8882be09f Mon Sep 17 00:00:00 2001 From: Daniel Schmidt Date: Fri, 15 Nov 2024 15:31:11 +0100 Subject: [PATCH 01/11] ephemeral: add tests for write-only attributes --- .../terraform/context_apply_ephemeral_test.go | 117 ++++++++++++++++++ .../terraform/context_plan_ephemeral_test.go | 102 +++++++++++++++ 2 files changed, 219 insertions(+) diff --git a/internal/terraform/context_apply_ephemeral_test.go b/internal/terraform/context_apply_ephemeral_test.go index d23d648c6012..1ead400508b5 100644 --- a/internal/terraform/context_apply_ephemeral_test.go +++ b/internal/terraform/context_apply_ephemeral_test.go @@ -359,3 +359,120 @@ resource "test_object" "test" { _, diags = ctx.Apply(plan, m, nil) assertNoDiagnostics(t, diags) } + +func TestContext2Apply_write_only_attribute_not_in_plan_and_state(t *testing.T) { + m := testModuleInline(t, map[string]string{ + "main.tf": ` +variable "ephem" { + type = string + ephemeral = true +} + +resource "ephem_write_only" "wo" { + normal = "normal" + write_only = var.ephem +} +`, + }) + + ephem := &testing_provider.MockProvider{ + GetProviderSchemaResponse: &providers.GetProviderSchemaResponse{ + ResourceTypes: map[string]providers.Schema{ + "ephem_write_only": { + Block: &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "normal": { + Type: cty.String, + Required: true, + }, + "write_only": { + Type: cty.String, + WriteOnly: true, + Required: true, + }, + }, + }, + }, + }, + }, + } + + ctx := testContext2(t, &ContextOpts{ + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("ephem"): testProviderFuncFixed(ephem), + }, + }) + + ephemVar := &InputValue{ + Value: cty.StringVal("ephemeral_value"), + SourceType: ValueFromCLIArg, + } + plan, diags := ctx.Plan(m, nil, &PlanOpts{ + Mode: plans.NormalMode, + SetVariables: InputValues{ + "ephem": ephemVar, + }, + }) + assertNoDiagnostics(t, diags) + + if len(plan.Changes.Resources) != 1 { + t.Fatalf("Expected 1 resource change, got %d", len(plan.Changes.Resources)) + } + + // if len(plan.Changes.Resources[0].AfterWriteOnlyPaths) != 1 { + // t.Fatalf("Expected 1 write-only attribute, got %d", len(plan.Changes.Resources[0].AfterWriteOnlyPaths)) + // } + schemas, schemaDiags := ctx.Schemas(m, plan.PriorState) + assertNoDiagnostics(t, schemaDiags) + planChanges, err := plan.Changes.Decode(schemas) + if err != nil { + t.Fatalf("Failed to decode plan changes: %v.", err) + } + if !planChanges.Resources[0].After.GetAttr("write_only").IsNull() { + t.Fatalf("Expected write_only to be null, got %v", planChanges.Resources[0].After.GetAttr("write_only")) + } + + state, diags := ctx.Apply(plan, m, &ApplyOpts{ + SetVariables: InputValues{ + "ephem": ephemVar, + }, + }) + assertNoDiagnostics(t, diags) + + resource := state.Resource(addrs.AbsResource{ + Module: addrs.RootModuleInstance, + Resource: addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "ephem_write_only", + Name: "wo", + }, + }) + + if resource == nil { + t.Fatalf("Resource not found") + } + + resourceInstance := resource.Instances[addrs.NoKey] + if resourceInstance == nil { + t.Fatalf("Resource instance not found") + } + + attrs, err := resourceInstance.Current.Decode(cty.Object(map[string]cty.Type{ + "normal": cty.String, + "write_only": cty.String, + })) + if err != nil { + t.Fatalf("Failed to decode attributes: %v", err) + } + if attrs.Value.GetAttr("normal").AsString() != "normal" { + t.Fatalf("normal attribute not as expected") + } + + // if len(resourceInstance.Current.AttrWriteOnlyPaths) != 1 { + // t.Fatalf("Expected 1 write only attribute") + // } + + // if !resourceInstance.Current.AttrWriteOnlyPaths[0].Equals(cty.GetAttrPath("write_only")) { + // t.Fatalf("Expected write_only to be a write only attribute") + // } +} diff --git a/internal/terraform/context_plan_ephemeral_test.go b/internal/terraform/context_plan_ephemeral_test.go index e84c93e38fc7..890fbef742fa 100644 --- a/internal/terraform/context_plan_ephemeral_test.go +++ b/internal/terraform/context_plan_ephemeral_test.go @@ -21,6 +21,7 @@ import ( func TestContext2Plan_ephemeralValues(t *testing.T) { for name, tc := range map[string]struct { + toBeImplemented bool module map[string]string expectValidateDiagnostics func(m *configs.Config) tfdiags.Diagnostics expectPlanDiagnostics func(m *configs.Config) tfdiags.Diagnostics @@ -549,8 +550,96 @@ You can correct this by removing references to ephemeral values, or by carefully }) }, }, + + "write_only attribute": { + toBeImplemented: true, + module: map[string]string{ + "main.tf": ` +ephemeral "ephem_resource" "data" { +} +resource "ephem_write_only" "test" { + write_only = ephemeral.ephem_resource.data.value +} +`, + }, + expectOpenEphemeralResourceCalled: true, + expectValidateEphemeralResourceConfigCalled: true, + expectCloseEphemeralResourceCalled: true, + }, + + "write_only with hardcoded value": { + toBeImplemented: true, + module: map[string]string{ + "main.tf": ` +variable "ephem" { + type = string + ephemeral = true +} + +resource "ephem_write_only" "producer" { + write_only = "this is not allowed" +} + +output "out" { + value = ephemeralasnull(var.ephem) +} +`, + }, + inputs: InputValues{ + "ephem": &InputValue{ + Value: cty.StringVal("ami"), + }, + }, + expectPlanDiagnostics: func(m *configs.Config) (diags tfdiags.Diagnostics) { + return diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Cannot use hard-coded value in write-only attribute", + Detail: `The attribute "write_only" of resource "ephem_write_only.producer" is write-only and cannot be used with a static value, please use an ephemeral variable or an ephemeral resource instead.`, + }) + }, + }, + + "write_only attribute references": { + toBeImplemented: true, + module: map[string]string{ + "main.tf": ` +variable "ephem" { + type = string + ephemeral = true +} + +resource "ephem_write_only" "producer" { + write_only = var.ephem # Valid usage +} + +resource "ephem_write_only" "consumer" { + write_only = ephem_write_only.producer.write_only # Invalid usage +} + +output "out" { + value = ephemeralasnull(var.ephem) +} +`, + }, + inputs: InputValues{ + "ephem": &InputValue{ + Value: cty.StringVal("ami"), + }, + }, + expectValidateDiagnostics: func(m *configs.Config) (diags tfdiags.Diagnostics) { + return diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Cannot reference write-only attribute", + Detail: `The attribute "write_only" of resource "ephem_write_only.producer" is write-only and cannot be referenced in this context.`, + }) + }, + }, } { t.Run(name, func(t *testing.T) { + if tc.toBeImplemented { + t.Skip("To be implemented") + } + m := testModuleInline(t, tc.module) ephem := &testing_provider.MockProvider{ @@ -582,6 +671,19 @@ You can correct this by removing references to ephemeral values, or by carefully }, }, }, + ResourceTypes: map[string]providers.Schema{ + "ephem_write_only": { + Block: &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "write_only": { + Type: cty.String, + WriteOnly: true, + Optional: true, + }, + }, + }, + }, + }, Functions: map[string]providers.FunctionDecl{ "either": { Parameters: []providers.FunctionParam{ From 8b643fa08281e122673b588f9c5d849bd44009fc Mon Sep 17 00:00:00 2001 From: Daniel Schmidt Date: Wed, 20 Nov 2024 11:27:06 +0100 Subject: [PATCH 02/11] ephemeral: add helper to remove ephemeral values from cty.Values --- internal/lang/ephemeral/marshal.go | 40 ++++++++++++++++ internal/lang/ephemeral/marshal_test.go | 61 +++++++++++++++++++++++++ internal/lang/funcs/conversion.go | 25 +--------- 3 files changed, 103 insertions(+), 23 deletions(-) create mode 100644 internal/lang/ephemeral/marshal.go create mode 100644 internal/lang/ephemeral/marshal_test.go diff --git a/internal/lang/ephemeral/marshal.go b/internal/lang/ephemeral/marshal.go new file mode 100644 index 000000000000..28f9cb8bad37 --- /dev/null +++ b/internal/lang/ephemeral/marshal.go @@ -0,0 +1,40 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: BUSL-1.1 + +package ephemeral + +import ( + "github.com/hashicorp/terraform/internal/lang/marks" + "github.com/zclconf/go-cty/cty" +) + +// RemoveEphemeralValues takes a value that possibly contains ephemeral +// values and returns an equal value without ephemeral values. If an attribute contains +// an ephemeral value it will be set to null. +func RemoveEphemeralValues(value cty.Value) cty.Value { + // We currently have no error case, so we can ignore the error + val, _ := cty.Transform(value, func(p cty.Path, v cty.Value) (cty.Value, error) { + _, givenMarks := v.Unmark() + if _, isEphemeral := givenMarks[marks.Ephemeral]; isEphemeral { + // We'll strip the ephemeral mark but retain any other marks + // that might be present on the input. + delete(givenMarks, marks.Ephemeral) + if !v.IsKnown() { + // If the source value is unknown then we must leave it + // unknown because its final type might be more precise + // than the associated type constraint and returning a + // typed null could therefore over-promise on what the + // final result type will be. + // We're deliberately constructing a fresh unknown value + // here, rather than returning the one we were given, + // because we need to discard any refinements that the + // unknown value might be carrying that definitely won't + // be honored when we force the final result to be null. + return cty.UnknownVal(v.Type()).WithMarks(givenMarks), nil + } + return cty.NullVal(v.Type()).WithMarks(givenMarks), nil + } + return v, nil + }) + return val +} diff --git a/internal/lang/ephemeral/marshal_test.go b/internal/lang/ephemeral/marshal_test.go new file mode 100644 index 000000000000..f61e38d11056 --- /dev/null +++ b/internal/lang/ephemeral/marshal_test.go @@ -0,0 +1,61 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: BUSL-1.1 + +package ephemeral + +import ( + "testing" + + "github.com/hashicorp/terraform/internal/lang/marks" + "github.com/zclconf/go-cty/cty" +) + +func TestEphemeral_removeEphemeralValues(t *testing.T) { + for name, tc := range map[string]struct { + input cty.Value + want cty.Value + }{ + "empty case": { + input: cty.NullVal(cty.DynamicPseudoType), + want: cty.NullVal(cty.DynamicPseudoType), + }, + "ephemeral marks case": { + input: cty.ObjectVal(map[string]cty.Value{ + "ephemeral": cty.StringVal("ephemeral_value").Mark(marks.Ephemeral), + "normal": cty.StringVal("normal_value"), + }), + want: cty.ObjectVal(map[string]cty.Value{ + "ephemeral": cty.NullVal(cty.String), + "normal": cty.StringVal("normal_value"), + }), + }, + "sensitive marks case": { + input: cty.ObjectVal(map[string]cty.Value{ + "sensitive": cty.StringVal("sensitive_value").Mark(marks.Sensitive), + "normal": cty.StringVal("normal_value"), + }), + want: cty.ObjectVal(map[string]cty.Value{ + "sensitive": cty.StringVal("sensitive_value").Mark(marks.Sensitive), + "normal": cty.StringVal("normal_value"), + }), + }, + "sensitive and ephemeral marks case": { + input: cty.ObjectVal(map[string]cty.Value{ + "sensitive_and_ephemeral": cty.StringVal("sensitive_and_ephemeral_value").Mark(marks.Sensitive).Mark(marks.Ephemeral), + "normal": cty.StringVal("normal_value"), + }), + want: cty.ObjectVal(map[string]cty.Value{ + "sensitive_and_ephemeral": cty.NullVal(cty.String).Mark(marks.Sensitive), + "normal": cty.StringVal("normal_value"), + }), + }, + } { + t.Run(name, func(t *testing.T) { + got := RemoveEphemeralValues(tc.input) + + if !got.RawEquals(tc.want) { + t.Errorf("got %#v, want %#v", got, tc.want) + } + }) + } +} diff --git a/internal/lang/funcs/conversion.go b/internal/lang/funcs/conversion.go index 25609ff02451..7b42bcd7224a 100644 --- a/internal/lang/funcs/conversion.go +++ b/internal/lang/funcs/conversion.go @@ -6,6 +6,7 @@ package funcs import ( "strconv" + "github.com/hashicorp/terraform/internal/lang/ephemeral" "github.com/hashicorp/terraform/internal/lang/marks" "github.com/hashicorp/terraform/internal/lang/types" "github.com/zclconf/go-cty/cty" @@ -126,29 +127,7 @@ var EphemeralAsNullFunc = function.New(&function.Spec{ return args[0].Type(), nil }, Impl: func(args []cty.Value, retType cty.Type) (cty.Value, error) { - return cty.Transform(args[0], func(p cty.Path, v cty.Value) (cty.Value, error) { - _, givenMarks := v.Unmark() - if _, isEphemeral := givenMarks[marks.Ephemeral]; isEphemeral { - // We'll strip the ephemeral mark but retain any other marks - // that might be present on the input. - delete(givenMarks, marks.Ephemeral) - if !v.IsKnown() { - // If the source value is unknown then we must leave it - // unknown because its final type might be more precise - // than the associated type constraint and returning a - // typed null could therefore over-promise on what the - // final result type will be. - // We're deliberately constructing a fresh unknown value - // here, rather than returning the one we were given, - // because we need to discard any refinements that the - // unknown value might be carrying that definitely won't - // be honored when we force the final result to be null. - return cty.UnknownVal(v.Type()).WithMarks(givenMarks), nil - } - return cty.NullVal(v.Type()).WithMarks(givenMarks), nil - } - return v, nil - }) + return ephemeral.RemoveEphemeralValues(args[0]), nil }, }) From 898546edfa931c8df73e586f3976771335d0bef1 Mon Sep 17 00:00:00 2001 From: Daniel Schmidt Date: Wed, 20 Nov 2024 12:18:21 +0100 Subject: [PATCH 03/11] ephemeral: add write only attribute paths --- internal/plans/changes.go | 2 ++ internal/plans/changes_src.go | 4 ++++ internal/states/instance_object_src.go | 4 ++++ internal/states/state_deepcopy.go | 15 +++++++++++---- internal/states/statefile/version4.go | 11 +++++++++++ .../terraform/context_apply_ephemeral_test.go | 19 ++++++++++--------- 6 files changed, 42 insertions(+), 13 deletions(-) diff --git a/internal/plans/changes.go b/internal/plans/changes.go index 1ed49724b055..9d069aa907c9 100644 --- a/internal/plans/changes.go +++ b/internal/plans/changes.go @@ -645,6 +645,8 @@ func (c *Change) Encode(ty cty.Type) (*ChangeSrc, error) { After: afterDV, BeforeSensitivePaths: sensitiveAttrsBefore, AfterSensitivePaths: sensitiveAttrsAfter, + BeforeWriteOnlyPaths: nil, // TODO: Add write-only paths + AfterWriteOnlyPaths: nil, // TODO: Add write-only paths Importing: c.Importing.Encode(), GeneratedConfig: c.GeneratedConfig, }, nil diff --git a/internal/plans/changes_src.go b/internal/plans/changes_src.go index ff851ea83e62..d82ed4536947 100644 --- a/internal/plans/changes_src.go +++ b/internal/plans/changes_src.go @@ -388,6 +388,10 @@ type ChangeSrc struct { // the serialized change. BeforeSensitivePaths, AfterSensitivePaths []cty.Path + // BeforeWriteOnlyPaths and AfterWriteOnlyPaths are paths for any values + // in Before or After (respectively) that are considered to be write-only. + BeforeWriteOnlyPaths, AfterWriteOnlyPaths []cty.Path + // Importing is present if the resource is being imported as part of this // change. // diff --git a/internal/states/instance_object_src.go b/internal/states/instance_object_src.go index 7960524b66a8..617e2eaf7530 100644 --- a/internal/states/instance_object_src.go +++ b/internal/states/instance_object_src.go @@ -57,6 +57,10 @@ type ResourceInstanceObjectSrc struct { // state, or to save as sensitive paths when saving state AttrSensitivePaths []cty.Path + // AttrWriteOnlyPaths is an array of paths to mark as ephemeral coming out of + // state, or to save as write_only paths when saving state + AttrWriteOnlyPaths []cty.Path + // These fields all correspond to the fields of the same name on // ResourceInstanceObject. Private []byte diff --git a/internal/states/state_deepcopy.go b/internal/states/state_deepcopy.go index 8486e9548565..cf830512b2c7 100644 --- a/internal/states/state_deepcopy.go +++ b/internal/states/state_deepcopy.go @@ -142,10 +142,16 @@ func (os *ResourceInstanceObjectSrc) DeepCopy() *ResourceInstanceObjectSrc { copy(attrsJSON, os.AttrsJSON) } - var attrPaths []cty.Path + var sensitiveAttrPaths []cty.Path if os.AttrSensitivePaths != nil { - attrPaths = make([]cty.Path, len(os.AttrSensitivePaths)) - copy(attrPaths, os.AttrSensitivePaths) + sensitiveAttrPaths = make([]cty.Path, len(os.AttrSensitivePaths)) + copy(sensitiveAttrPaths, os.AttrSensitivePaths) + } + + var writeOnlyAttrPaths []cty.Path + if os.AttrWriteOnlyPaths != nil { + writeOnlyAttrPaths = make([]cty.Path, len(os.AttrWriteOnlyPaths)) + copy(writeOnlyAttrPaths, os.AttrWriteOnlyPaths) } var private []byte @@ -168,7 +174,8 @@ func (os *ResourceInstanceObjectSrc) DeepCopy() *ResourceInstanceObjectSrc { Private: private, AttrsFlat: attrsFlat, AttrsJSON: attrsJSON, - AttrSensitivePaths: attrPaths, + AttrSensitivePaths: sensitiveAttrPaths, + AttrWriteOnlyPaths: writeOnlyAttrPaths, Dependencies: dependencies, CreateBeforeDestroy: os.CreateBeforeDestroy, decodeValueCache: os.decodeValueCache, diff --git a/internal/states/statefile/version4.go b/internal/states/statefile/version4.go index 6477e66cf24c..d38e0b0b284b 100644 --- a/internal/states/statefile/version4.go +++ b/internal/states/statefile/version4.go @@ -166,6 +166,16 @@ func prepareStateV4(sV4 *stateV4) (*File, tfdiags.Diagnostics) { obj.AttrSensitivePaths = paths } + // write-only paths + if isV4.AttributeWriteOnlyPaths != nil { + paths, pathsDiags := unmarshalPaths([]byte(isV4.AttributeWriteOnlyPaths)) + diags = diags.Append(pathsDiags) + if pathsDiags.HasErrors() { + continue + } + obj.AttrWriteOnlyPaths = paths + } + { // Status raw := isV4.Status @@ -701,6 +711,7 @@ type instanceObjectStateV4 struct { AttributesRaw json.RawMessage `json:"attributes,omitempty"` AttributesFlat map[string]string `json:"attributes_flat,omitempty"` AttributeSensitivePaths json.RawMessage `json:"sensitive_attributes,omitempty"` + AttributeWriteOnlyPaths json.RawMessage `json:"write_only_attributes,omitempty"` PrivateRaw []byte `json:"private,omitempty"` diff --git a/internal/terraform/context_apply_ephemeral_test.go b/internal/terraform/context_apply_ephemeral_test.go index 1ead400508b5..19109466403e 100644 --- a/internal/terraform/context_apply_ephemeral_test.go +++ b/internal/terraform/context_apply_ephemeral_test.go @@ -419,9 +419,10 @@ resource "ephem_write_only" "wo" { t.Fatalf("Expected 1 resource change, got %d", len(plan.Changes.Resources)) } - // if len(plan.Changes.Resources[0].AfterWriteOnlyPaths) != 1 { - // t.Fatalf("Expected 1 write-only attribute, got %d", len(plan.Changes.Resources[0].AfterWriteOnlyPaths)) - // } + if len(plan.Changes.Resources[0].AfterWriteOnlyPaths) != 1 { + t.Fatalf("Expected 1 write-only attribute, got %d", len(plan.Changes.Resources[0].AfterWriteOnlyPaths)) + } + schemas, schemaDiags := ctx.Schemas(m, plan.PriorState) assertNoDiagnostics(t, schemaDiags) planChanges, err := plan.Changes.Decode(schemas) @@ -468,11 +469,11 @@ resource "ephem_write_only" "wo" { t.Fatalf("normal attribute not as expected") } - // if len(resourceInstance.Current.AttrWriteOnlyPaths) != 1 { - // t.Fatalf("Expected 1 write only attribute") - // } + if len(resourceInstance.Current.AttrWriteOnlyPaths) != 1 { + t.Fatalf("Expected 1 write only attribute") + } - // if !resourceInstance.Current.AttrWriteOnlyPaths[0].Equals(cty.GetAttrPath("write_only")) { - // t.Fatalf("Expected write_only to be a write only attribute") - // } + if !resourceInstance.Current.AttrWriteOnlyPaths[0].Equals(cty.GetAttrPath("write_only")) { + t.Fatalf("Expected write_only to be a write only attribute") + } } From 3ccb4f4ca1122b464d1e20f5e472f730e74a5e2f Mon Sep 17 00:00:00 2001 From: Daniel Schmidt Date: Wed, 20 Nov 2024 13:47:09 +0100 Subject: [PATCH 04/11] ephemeral: allow ephemeral values in write-only attributes --- internal/terraform/node_resource_validate.go | 31 +++++++++++--------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/internal/terraform/node_resource_validate.go b/internal/terraform/node_resource_validate.go index 9725f3437999..ab289f0a2efe 100644 --- a/internal/terraform/node_resource_validate.go +++ b/internal/terraform/node_resource_validate.go @@ -762,21 +762,24 @@ func validateDependsOn(ctx EvalContext, dependsOn []hcl.Traversal) (diags tfdiag // by calling [tfdiags.Diagnostics.InConfigBody] before returning them to // any caller that expects fully-resolved diagnostics. func validateResourceForbiddenEphemeralValues(ctx EvalContext, value cty.Value, schema *configschema.Block) (diags tfdiags.Diagnostics) { - // NOTE: We take a schema argument in anticipation of a future feature - // that might allow managed resources to declare certain attributes as - // being "write-only", which would create a little nested island where - // ephemeral values are permitted in return for providers accepting that - // those values will not be preserved between plan and apply or between - // sequential plan/apply rounds. But we aren't doing that yet, so we - // just ignore that argument for now. - for _, path := range ephemeral.EphemeralValuePaths(value) { - diags = diags.Append(tfdiags.AttributeValue( - tfdiags.Error, - "Invalid use of ephemeral value", - "Ephemeral values are not valid in resource arguments, because resource instances must persist between Terraform phases.", - path, - )) + attr := schema.AttributeByPath(path) + if attr == nil { + diags = diags.Append(tfdiags.AttributeValue( + tfdiags.Error, + "Could not find schema for attribute", + "This is most likely a bug in Terraform, please report it.", + path, + )) + } else if !attr.WriteOnly { + diags = diags.Append(tfdiags.AttributeValue( + tfdiags.Error, + "Invalid use of ephemeral value", + "Ephemeral values are not valid in resource arguments, because resource instances must persist between Terraform phases.", + path, + )) + } + } return diags } From b128e7774f480f44139377119fe304cc42dcb16f Mon Sep 17 00:00:00 2001 From: Daniel Schmidt Date: Wed, 20 Nov 2024 13:59:16 +0100 Subject: [PATCH 05/11] ephemeral: guard against plan with no changes --- internal/terraform/context_plan.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/terraform/context_plan.go b/internal/terraform/context_plan.go index 7b7c06b7fdbd..4038cff42766 100644 --- a/internal/terraform/context_plan.go +++ b/internal/terraform/context_plan.go @@ -349,7 +349,7 @@ The -target option is not for routine use, and is provided only for exceptional panic("nil plan but no errors") } - if plan != nil { + if plan != nil && plan.Changes != nil { relevantAttrs, rDiags := c.relevantResourceAttrsForPlan(config, plan) diags = diags.Append(rDiags) plan.RelevantAttributes = relevantAttrs From 9ccd97262c1dba86d5c405191c3bdf13ebc34f73 Mon Sep 17 00:00:00 2001 From: Daniel Schmidt Date: Thu, 21 Nov 2024 13:57:06 +0100 Subject: [PATCH 06/11] ephemeral: add support for write-only attributes --- internal/plans/changes.go | 8 +- internal/plans/changes_src.go | 6 + internal/states/instance_object.go | 5 + internal/states/instance_object_src.go | 2 + internal/states/remote/state_test.go | 10 +- internal/states/statefile/version4.go | 3 + .../terraform/context_apply_ephemeral_test.go | 292 +++++++++++++++++- .../terraform/context_plan_ephemeral_test.go | 1 - .../node_resource_abstract_instance.go | 15 +- .../terraform/node_resource_apply_instance.go | 5 + 10 files changed, 335 insertions(+), 12 deletions(-) diff --git a/internal/plans/changes.go b/internal/plans/changes.go index 9d069aa907c9..d4e648f4ad64 100644 --- a/internal/plans/changes.go +++ b/internal/plans/changes.go @@ -582,6 +582,10 @@ type Change struct { // collections/structures. Before, After cty.Value + // BeforeWriteOnlyPaths and AfterWriteOnlyPaths are paths for any values + // in Before or After (respectively) that are considered to be write-only. + BeforeWriteOnlyPaths, AfterWriteOnlyPaths []cty.Path + // Importing is present if the resource is being imported as part of this // change. // @@ -645,8 +649,8 @@ func (c *Change) Encode(ty cty.Type) (*ChangeSrc, error) { After: afterDV, BeforeSensitivePaths: sensitiveAttrsBefore, AfterSensitivePaths: sensitiveAttrsAfter, - BeforeWriteOnlyPaths: nil, // TODO: Add write-only paths - AfterWriteOnlyPaths: nil, // TODO: Add write-only paths + BeforeWriteOnlyPaths: c.BeforeWriteOnlyPaths, + AfterWriteOnlyPaths: c.AfterWriteOnlyPaths, Importing: c.Importing.Encode(), GeneratedConfig: c.GeneratedConfig, }, nil diff --git a/internal/plans/changes_src.go b/internal/plans/changes_src.go index d82ed4536947..1a03dbd0e22b 100644 --- a/internal/plans/changes_src.go +++ b/internal/plans/changes_src.go @@ -130,6 +130,12 @@ func (c *ChangesSrc) Decode(schemas *schemarepo.Schemas) (*Changes, error) { rc.Before = marks.MarkPaths(rc.Before, marks.Sensitive, rcs.BeforeSensitivePaths) rc.After = marks.MarkPaths(rc.After, marks.Sensitive, rcs.AfterSensitivePaths) + rc.Before = marks.MarkPaths(rc.Before, marks.Ephemeral, rcs.BeforeWriteOnlyPaths) + rc.After = marks.MarkPaths(rc.After, marks.Ephemeral, rcs.BeforeWriteOnlyPaths) + + rc.BeforeWriteOnlyPaths = rcs.BeforeWriteOnlyPaths + rc.AfterWriteOnlyPaths = rcs.AfterWriteOnlyPaths + changes.Resources = append(changes.Resources, rc) } diff --git a/internal/states/instance_object.go b/internal/states/instance_object.go index f398d01c6352..301d7ecc055b 100644 --- a/internal/states/instance_object.go +++ b/internal/states/instance_object.go @@ -50,6 +50,10 @@ type ResourceInstanceObject struct { // destroy operations, we need to record the status to ensure a resource // removed from the config will still be destroyed in the same manner. CreateBeforeDestroy bool + + // AttrWriteOnlyPaths is an array of paths to mark as ephemeral coming out of + // state, or to save as write_only paths when saving state + AttrWriteOnlyPaths []cty.Path } // ObjectStatus represents the status of a RemoteObject. @@ -135,6 +139,7 @@ func (o *ResourceInstanceObject) Encode(ty cty.Type, schemaVersion uint64) (*Res SchemaVersion: schemaVersion, AttrsJSON: src, AttrSensitivePaths: sensitivePaths, + AttrWriteOnlyPaths: o.AttrWriteOnlyPaths, Private: o.Private, Status: o.Status, Dependencies: dependencies, diff --git a/internal/states/instance_object_src.go b/internal/states/instance_object_src.go index 617e2eaf7530..5c44011e2783 100644 --- a/internal/states/instance_object_src.go +++ b/internal/states/instance_object_src.go @@ -100,6 +100,7 @@ func (os *ResourceInstanceObjectSrc) Decode(ty cty.Type) (*ResourceInstanceObjec default: val, err = ctyjson.Unmarshal(os.AttrsJSON, ty) val = marks.MarkPaths(val, marks.Sensitive, os.AttrSensitivePaths) + val = marks.MarkPaths(val, marks.Ephemeral, os.AttrWriteOnlyPaths) if err != nil { return nil, err } @@ -111,6 +112,7 @@ func (os *ResourceInstanceObjectSrc) Decode(ty cty.Type) (*ResourceInstanceObjec Dependencies: os.Dependencies, Private: os.Private, CreateBeforeDestroy: os.CreateBeforeDestroy, + AttrWriteOnlyPaths: os.AttrWriteOnlyPaths, }, nil } diff --git a/internal/states/remote/state_test.go b/internal/states/remote/state_test.go index cbb6a6219f07..43dd654fa687 100644 --- a/internal/states/remote/state_test.go +++ b/internal/states/remote/state_test.go @@ -129,8 +129,9 @@ func TestStatePersist(t *testing.T) { "attributes_flat": map[string]interface{}{ "filename": "file.txt", }, - "schema_version": 0.0, - "sensitive_attributes": []interface{}{}, + "schema_version": 0.0, + "sensitive_attributes": []interface{}{}, + "write_only_attributes": []interface{}{}, }, }, "mode": "managed", @@ -167,8 +168,9 @@ func TestStatePersist(t *testing.T) { "attributes_flat": map[string]interface{}{ "filename": "file.txt", }, - "schema_version": 0.0, - "sensitive_attributes": []interface{}{}, + "schema_version": 0.0, + "sensitive_attributes": []interface{}{}, + "write_only_attributes": []interface{}{}, }, }, "mode": "managed", diff --git a/internal/states/statefile/version4.go b/internal/states/statefile/version4.go index d38e0b0b284b..ba47079fbec4 100644 --- a/internal/states/statefile/version4.go +++ b/internal/states/statefile/version4.go @@ -492,6 +492,8 @@ func appendInstanceObjectStateV4(rs *states.Resource, is *states.ResourceInstanc // Marshal paths to JSON attributeSensitivePaths, pathsDiags := marshalPaths(obj.AttrSensitivePaths) diags = diags.Append(pathsDiags) + attributeWriteOnlyPaths, pathsDiags := marshalPaths(obj.AttrWriteOnlyPaths) + diags = diags.Append(pathsDiags) return append(isV4s, instanceObjectStateV4{ IndexKey: rawKey, @@ -501,6 +503,7 @@ func appendInstanceObjectStateV4(rs *states.Resource, is *states.ResourceInstanc AttributesFlat: obj.AttrsFlat, AttributesRaw: obj.AttrsJSON, AttributeSensitivePaths: attributeSensitivePaths, + AttributeWriteOnlyPaths: attributeWriteOnlyPaths, PrivateRaw: privateRaw, Dependencies: deps, CreateBeforeDestroy: obj.CreateBeforeDestroy, diff --git a/internal/terraform/context_apply_ephemeral_test.go b/internal/terraform/context_apply_ephemeral_test.go index 19109466403e..60d34d19c20c 100644 --- a/internal/terraform/context_apply_ephemeral_test.go +++ b/internal/terraform/context_apply_ephemeral_test.go @@ -14,6 +14,7 @@ import ( "github.com/hashicorp/terraform/internal/plans" "github.com/hashicorp/terraform/internal/providers" testing_provider "github.com/hashicorp/terraform/internal/providers/testing" + "github.com/hashicorp/terraform/internal/states" "github.com/zclconf/go-cty/cty" ) @@ -420,7 +421,7 @@ resource "ephem_write_only" "wo" { } if len(plan.Changes.Resources[0].AfterWriteOnlyPaths) != 1 { - t.Fatalf("Expected 1 write-only attribute, got %d", len(plan.Changes.Resources[0].AfterWriteOnlyPaths)) + t.Fatalf("Expected 1 write-only attribute in plan, got %d", len(plan.Changes.Resources[0].AfterWriteOnlyPaths)) } schemas, schemaDiags := ctx.Schemas(m, plan.PriorState) @@ -429,6 +430,288 @@ resource "ephem_write_only" "wo" { if err != nil { t.Fatalf("Failed to decode plan changes: %v.", err) } + + if !planChanges.Resources[0].After.GetAttr("write_only").IsNull() { + t.Fatalf("Expected write_only to be null, got %v", planChanges.Resources[0].After.GetAttr("write_only")) + } + + state, diags := ctx.Apply(plan, m, &ApplyOpts{ + SetVariables: InputValues{ + "ephem": ephemVar, + }, + }) + assertNoDiagnostics(t, diags) + + resource := state.Resource(addrs.AbsResource{ + Module: addrs.RootModuleInstance, + Resource: addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "ephem_write_only", + Name: "wo", + }, + }) + + if resource == nil { + t.Fatalf("Resource not found") + } + + resourceInstance := resource.Instances[addrs.NoKey] + if resourceInstance == nil { + t.Fatalf("Resource instance not found") + } + + attrs, err := resourceInstance.Current.Decode(cty.Object(map[string]cty.Type{ + "normal": cty.String, + "write_only": cty.String, + })) + if err != nil { + t.Fatalf("Failed to decode attributes: %v", err) + } + + if attrs.Value.GetAttr("normal").AsString() != "normal" { + t.Fatalf("normal attribute not as expected") + } + + if !attrs.Value.GetAttr("write_only").IsNull() { + t.Fatalf("write_only attribute should be null") + } + + if len(resourceInstance.Current.AttrWriteOnlyPaths) != 1 { + t.Fatalf("Expected 1 write only attribute in apply") + } + + if !resourceInstance.Current.AttrWriteOnlyPaths[0].Equals(cty.GetAttrPath("write_only")) { + t.Fatalf("Expected write_only to be a write only attribute") + } +} + +func TestContext2Apply_update_write_only_attribute_not_in_plan_and_state(t *testing.T) { + m := testModuleInline(t, map[string]string{ + "main.tf": ` +variable "ephem" { + type = string + ephemeral = true +} + +resource "ephem_write_only" "wo" { + normal = "normal" + write_only = var.ephem +} +`, + }) + + ephem := &testing_provider.MockProvider{ + GetProviderSchemaResponse: &providers.GetProviderSchemaResponse{ + ResourceTypes: map[string]providers.Schema{ + "ephem_write_only": { + Block: &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "normal": { + Type: cty.String, + Required: true, + }, + "write_only": { + Type: cty.String, + WriteOnly: true, + Required: true, + }, + }, + }, + }, + }, + }, + } + + ctx := testContext2(t, &ContextOpts{ + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("ephem"): testProviderFuncFixed(ephem), + }, + }) + + ephemVar := &InputValue{ + Value: cty.StringVal("ephemeral_value"), + SourceType: ValueFromCLIArg, + } + + priorState := states.BuildState(func(state *states.SyncState) { + state.SetResourceInstanceCurrent( + mustResourceInstanceAddr("ephem_write_only.wo"), + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: mustParseJson(map[string]interface{}{ + "normal": "outdated", + }), + }, + addrs.AbsProviderConfig{ + Provider: addrs.NewDefaultProvider("ephem"), + Module: addrs.RootModule, + }) + }) + + plan, diags := ctx.Plan(m, priorState, &PlanOpts{ + Mode: plans.NormalMode, + SetVariables: InputValues{ + "ephem": ephemVar, + }, + }) + assertNoDiagnostics(t, diags) + + if len(plan.Changes.Resources) != 1 { + t.Fatalf("Expected 1 resource change, got %d", len(plan.Changes.Resources)) + } + + if len(plan.Changes.Resources[0].AfterWriteOnlyPaths) != 1 { + t.Fatalf("Expected 1 write-only attribute in plan, got %d", len(plan.Changes.Resources[0].AfterWriteOnlyPaths)) + } + + schemas, schemaDiags := ctx.Schemas(m, plan.PriorState) + assertNoDiagnostics(t, schemaDiags) + planChanges, err := plan.Changes.Decode(schemas) + if err != nil { + t.Fatalf("Failed to decode plan changes: %v.", err) + } + + if !planChanges.Resources[0].After.GetAttr("write_only").IsNull() { + t.Fatalf("Expected write_only to be null, got %v", planChanges.Resources[0].After.GetAttr("write_only")) + } + + state, diags := ctx.Apply(plan, m, &ApplyOpts{ + SetVariables: InputValues{ + "ephem": ephemVar, + }, + }) + assertNoDiagnostics(t, diags) + + resource := state.Resource(addrs.AbsResource{ + Module: addrs.RootModuleInstance, + Resource: addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "ephem_write_only", + Name: "wo", + }, + }) + + if resource == nil { + t.Fatalf("Resource not found") + } + + resourceInstance := resource.Instances[addrs.NoKey] + if resourceInstance == nil { + t.Fatalf("Resource instance not found") + } + + attrs, err := resourceInstance.Current.Decode(cty.Object(map[string]cty.Type{ + "normal": cty.String, + "write_only": cty.String, + })) + if err != nil { + t.Fatalf("Failed to decode attributes: %v", err) + } + + if attrs.Value.GetAttr("normal").AsString() != "normal" { + t.Fatalf("normal attribute not as expected") + } + + if !attrs.Value.GetAttr("write_only").IsNull() { + t.Fatalf("write_only attribute should be null") + } + + if len(resourceInstance.Current.AttrWriteOnlyPaths) != 1 { + t.Fatalf("Expected 1 write only attribute in apply") + } + + if !resourceInstance.Current.AttrWriteOnlyPaths[0].Equals(cty.GetAttrPath("write_only")) { + t.Fatalf("Expected write_only to be a write only attribute") + } +} + +func TestContext2Apply_normal_attributes_becomes_write_only_attribute(t *testing.T) { + m := testModuleInline(t, map[string]string{ + "main.tf": ` +variable "ephem" { + type = string + ephemeral = true +} + +resource "ephem_write_only" "wo" { + normal = "normal" + write_only = var.ephem +} +`, + }) + + ephem := &testing_provider.MockProvider{ + GetProviderSchemaResponse: &providers.GetProviderSchemaResponse{ + ResourceTypes: map[string]providers.Schema{ + "ephem_write_only": { + Block: &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "normal": { + Type: cty.String, + Required: true, + }, + "write_only": { + Type: cty.String, + WriteOnly: true, + Required: true, + }, + }, + }, + }, + }, + }, + } + + ctx := testContext2(t, &ContextOpts{ + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("ephem"): testProviderFuncFixed(ephem), + }, + }) + + ephemVar := &InputValue{ + Value: cty.StringVal("ephemeral_value"), + SourceType: ValueFromCLIArg, + } + + priorState := states.BuildState(func(state *states.SyncState) { + state.SetResourceInstanceCurrent( + mustResourceInstanceAddr("ephem_write_only.wo"), + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: mustParseJson(map[string]interface{}{ + "normal": "normal", + "write_only": "this was not ephemeral but now is", + }), + }, + addrs.AbsProviderConfig{ + Provider: addrs.NewDefaultProvider("ephem"), + Module: addrs.RootModule, + }) + }) + + plan, diags := ctx.Plan(m, priorState, &PlanOpts{ + Mode: plans.NormalMode, + SetVariables: InputValues{ + "ephem": ephemVar, + }, + }) + assertNoDiagnostics(t, diags) + + if len(plan.Changes.Resources) != 1 { + t.Fatalf("Expected 1 resource change, got %d", len(plan.Changes.Resources)) + } + + if len(plan.Changes.Resources[0].AfterWriteOnlyPaths) != 1 { + t.Fatalf("Expected 1 write-only attribute in plan, got %d", len(plan.Changes.Resources[0].AfterWriteOnlyPaths)) + } + + schemas, schemaDiags := ctx.Schemas(m, plan.PriorState) + assertNoDiagnostics(t, schemaDiags) + planChanges, err := plan.Changes.Decode(schemas) + if err != nil { + t.Fatalf("Failed to decode plan changes: %v.", err) + } + if !planChanges.Resources[0].After.GetAttr("write_only").IsNull() { t.Fatalf("Expected write_only to be null, got %v", planChanges.Resources[0].After.GetAttr("write_only")) } @@ -465,12 +748,17 @@ resource "ephem_write_only" "wo" { if err != nil { t.Fatalf("Failed to decode attributes: %v", err) } + if attrs.Value.GetAttr("normal").AsString() != "normal" { t.Fatalf("normal attribute not as expected") } + if !attrs.Value.GetAttr("write_only").IsNull() { + t.Fatalf("write_only attribute should be null") + } + if len(resourceInstance.Current.AttrWriteOnlyPaths) != 1 { - t.Fatalf("Expected 1 write only attribute") + t.Fatalf("Expected 1 write only attribute in apply") } if !resourceInstance.Current.AttrWriteOnlyPaths[0].Equals(cty.GetAttrPath("write_only")) { diff --git a/internal/terraform/context_plan_ephemeral_test.go b/internal/terraform/context_plan_ephemeral_test.go index 890fbef742fa..f6b99a1ae24d 100644 --- a/internal/terraform/context_plan_ephemeral_test.go +++ b/internal/terraform/context_plan_ephemeral_test.go @@ -552,7 +552,6 @@ You can correct this by removing references to ephemeral values, or by carefully }, "write_only attribute": { - toBeImplemented: true, module: map[string]string{ "main.tf": ` ephemeral "ephem_resource" "data" { diff --git a/internal/terraform/node_resource_abstract_instance.go b/internal/terraform/node_resource_abstract_instance.go index a6dc801f4889..a8657f046c19 100644 --- a/internal/terraform/node_resource_abstract_instance.go +++ b/internal/terraform/node_resource_abstract_instance.go @@ -17,6 +17,7 @@ import ( "github.com/hashicorp/terraform/internal/configs" "github.com/hashicorp/terraform/internal/configs/configschema" "github.com/hashicorp/terraform/internal/instances" + "github.com/hashicorp/terraform/internal/lang/ephemeral" "github.com/hashicorp/terraform/internal/lang/marks" "github.com/hashicorp/terraform/internal/moduletest/mocking" "github.com/hashicorp/terraform/internal/plans" @@ -1180,6 +1181,10 @@ func (n *NodeAbstractResourceInstance) plan( } } + // We don't need ephemeral values anymore since the value has been planned + ephemeralValuePaths, _ := marks.PathsWithMark(unmarkedPaths, marks.Ephemeral) + plannedNewVal = ephemeral.RemoveEphemeralValues(plannedNewVal) + // Call post-refresh hook diags = diags.Append(ctx.Hook(func(h Hook) (HookAction, error) { return h.PostDiff(n.HookResourceIdentity(), addrs.NotDeposed, action, priorVal, plannedNewVal) @@ -1202,6 +1207,9 @@ func (n *NodeAbstractResourceInstance) plan( // Marks will be removed when encoding. After: plannedNewVal, GeneratedConfig: n.generatedConfigHCL, + + BeforeWriteOnlyPaths: ephemeral.EphemeralValuePaths(priorVal), + AfterWriteOnlyPaths: ephemeralValuePaths, }, ActionReason: actionReason, RequiredReplace: reqRep, @@ -1215,9 +1223,10 @@ func (n *NodeAbstractResourceInstance) plan( // must _also_ record the returned change in the active plan, // which the expression evaluator will use in preference to this // incomplete value recorded in the state. - Status: states.ObjectPlanned, - Value: plannedNewVal, - Private: plannedPrivate, + Status: states.ObjectPlanned, + Value: plannedNewVal, + Private: plannedPrivate, + AttrWriteOnlyPaths: ephemeralValuePaths, } return plan, state, deferred, keyData, diags diff --git a/internal/terraform/node_resource_apply_instance.go b/internal/terraform/node_resource_apply_instance.go index 552807646c96..b3da83443d77 100644 --- a/internal/terraform/node_resource_apply_instance.go +++ b/internal/terraform/node_resource_apply_instance.go @@ -10,6 +10,7 @@ import ( "github.com/hashicorp/terraform/internal/addrs" "github.com/hashicorp/terraform/internal/configs" "github.com/hashicorp/terraform/internal/instances" + "github.com/hashicorp/terraform/internal/lang/ephemeral" "github.com/hashicorp/terraform/internal/plans" "github.com/hashicorp/terraform/internal/plans/objchange" "github.com/hashicorp/terraform/internal/providers" @@ -323,6 +324,10 @@ func (n *NodeApplyableResourceInstance) managedResourceExecute(ctx EvalContext) if state != nil { // dependencies are always updated to match the configuration during apply state.Dependencies = n.Dependencies + + // The value is updated to remove ephemeral values + state.Value = ephemeral.RemoveEphemeralValues(state.Value) + state.AttrWriteOnlyPaths = diffApply.AfterWriteOnlyPaths } err = n.writeResourceInstanceState(ctx, state, workingState) if err != nil { From 973df6abcecd6b73916bcce58f9492162bc27ec8 Mon Sep 17 00:00:00 2001 From: Daniel Schmidt Date: Thu, 21 Nov 2024 16:03:24 +0100 Subject: [PATCH 07/11] ephemeral: send diagnostics if plan responds with an ephemeral value --- .../terraform/node_resource_abstract_instance.go | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/internal/terraform/node_resource_abstract_instance.go b/internal/terraform/node_resource_abstract_instance.go index a8657f046c19..debc3bef2442 100644 --- a/internal/terraform/node_resource_abstract_instance.go +++ b/internal/terraform/node_resource_abstract_instance.go @@ -1181,9 +1181,17 @@ func (n *NodeAbstractResourceInstance) plan( } } - // We don't need ephemeral values anymore since the value has been planned ephemeralValuePaths, _ := marks.PathsWithMark(unmarkedPaths, marks.Ephemeral) - plannedNewVal = ephemeral.RemoveEphemeralValues(plannedNewVal) + if len(ephemeralValuePaths) > 0 { + for _, path := range ephemeralValuePaths { + diags = diags.Append(tfdiags.AttributeValue( + tfdiags.Error, + "Provider set ephemeral value during plan", + "The provider set an ephemeral value during plan, this is not allowed. This is most likely a bug in the provider, please file an issue on the providers issue tracker.", + path, + )) + } + } // Call post-refresh hook diags = diags.Append(ctx.Hook(func(h Hook) (HookAction, error) { From d3c247b8403e671a3ed809a0357bac9ea6156b1e Mon Sep 17 00:00:00 2001 From: Daniel Schmidt Date: Mon, 25 Nov 2024 15:56:50 +0100 Subject: [PATCH 08/11] ephemeral: remove write only paths we might need them later on, but it will be easier to find the right place and abstraction when the need to use them arises --- internal/plans/changes.go | 6 ---- internal/plans/changes_src.go | 10 ------ internal/states/instance_object.go | 5 --- internal/states/instance_object_src.go | 6 ---- internal/states/remote/state_test.go | 10 +++--- internal/states/state_deepcopy.go | 7 ---- internal/states/statefile/version4.go | 14 -------- .../terraform/context_apply_ephemeral_test.go | 36 ------------------- .../node_resource_abstract_instance.go | 10 ++---- .../terraform/node_resource_apply_instance.go | 5 --- 10 files changed, 7 insertions(+), 102 deletions(-) diff --git a/internal/plans/changes.go b/internal/plans/changes.go index d4e648f4ad64..1ed49724b055 100644 --- a/internal/plans/changes.go +++ b/internal/plans/changes.go @@ -582,10 +582,6 @@ type Change struct { // collections/structures. Before, After cty.Value - // BeforeWriteOnlyPaths and AfterWriteOnlyPaths are paths for any values - // in Before or After (respectively) that are considered to be write-only. - BeforeWriteOnlyPaths, AfterWriteOnlyPaths []cty.Path - // Importing is present if the resource is being imported as part of this // change. // @@ -649,8 +645,6 @@ func (c *Change) Encode(ty cty.Type) (*ChangeSrc, error) { After: afterDV, BeforeSensitivePaths: sensitiveAttrsBefore, AfterSensitivePaths: sensitiveAttrsAfter, - BeforeWriteOnlyPaths: c.BeforeWriteOnlyPaths, - AfterWriteOnlyPaths: c.AfterWriteOnlyPaths, Importing: c.Importing.Encode(), GeneratedConfig: c.GeneratedConfig, }, nil diff --git a/internal/plans/changes_src.go b/internal/plans/changes_src.go index 1a03dbd0e22b..ff851ea83e62 100644 --- a/internal/plans/changes_src.go +++ b/internal/plans/changes_src.go @@ -130,12 +130,6 @@ func (c *ChangesSrc) Decode(schemas *schemarepo.Schemas) (*Changes, error) { rc.Before = marks.MarkPaths(rc.Before, marks.Sensitive, rcs.BeforeSensitivePaths) rc.After = marks.MarkPaths(rc.After, marks.Sensitive, rcs.AfterSensitivePaths) - rc.Before = marks.MarkPaths(rc.Before, marks.Ephemeral, rcs.BeforeWriteOnlyPaths) - rc.After = marks.MarkPaths(rc.After, marks.Ephemeral, rcs.BeforeWriteOnlyPaths) - - rc.BeforeWriteOnlyPaths = rcs.BeforeWriteOnlyPaths - rc.AfterWriteOnlyPaths = rcs.AfterWriteOnlyPaths - changes.Resources = append(changes.Resources, rc) } @@ -394,10 +388,6 @@ type ChangeSrc struct { // the serialized change. BeforeSensitivePaths, AfterSensitivePaths []cty.Path - // BeforeWriteOnlyPaths and AfterWriteOnlyPaths are paths for any values - // in Before or After (respectively) that are considered to be write-only. - BeforeWriteOnlyPaths, AfterWriteOnlyPaths []cty.Path - // Importing is present if the resource is being imported as part of this // change. // diff --git a/internal/states/instance_object.go b/internal/states/instance_object.go index 301d7ecc055b..f398d01c6352 100644 --- a/internal/states/instance_object.go +++ b/internal/states/instance_object.go @@ -50,10 +50,6 @@ type ResourceInstanceObject struct { // destroy operations, we need to record the status to ensure a resource // removed from the config will still be destroyed in the same manner. CreateBeforeDestroy bool - - // AttrWriteOnlyPaths is an array of paths to mark as ephemeral coming out of - // state, or to save as write_only paths when saving state - AttrWriteOnlyPaths []cty.Path } // ObjectStatus represents the status of a RemoteObject. @@ -139,7 +135,6 @@ func (o *ResourceInstanceObject) Encode(ty cty.Type, schemaVersion uint64) (*Res SchemaVersion: schemaVersion, AttrsJSON: src, AttrSensitivePaths: sensitivePaths, - AttrWriteOnlyPaths: o.AttrWriteOnlyPaths, Private: o.Private, Status: o.Status, Dependencies: dependencies, diff --git a/internal/states/instance_object_src.go b/internal/states/instance_object_src.go index 5c44011e2783..7960524b66a8 100644 --- a/internal/states/instance_object_src.go +++ b/internal/states/instance_object_src.go @@ -57,10 +57,6 @@ type ResourceInstanceObjectSrc struct { // state, or to save as sensitive paths when saving state AttrSensitivePaths []cty.Path - // AttrWriteOnlyPaths is an array of paths to mark as ephemeral coming out of - // state, or to save as write_only paths when saving state - AttrWriteOnlyPaths []cty.Path - // These fields all correspond to the fields of the same name on // ResourceInstanceObject. Private []byte @@ -100,7 +96,6 @@ func (os *ResourceInstanceObjectSrc) Decode(ty cty.Type) (*ResourceInstanceObjec default: val, err = ctyjson.Unmarshal(os.AttrsJSON, ty) val = marks.MarkPaths(val, marks.Sensitive, os.AttrSensitivePaths) - val = marks.MarkPaths(val, marks.Ephemeral, os.AttrWriteOnlyPaths) if err != nil { return nil, err } @@ -112,7 +107,6 @@ func (os *ResourceInstanceObjectSrc) Decode(ty cty.Type) (*ResourceInstanceObjec Dependencies: os.Dependencies, Private: os.Private, CreateBeforeDestroy: os.CreateBeforeDestroy, - AttrWriteOnlyPaths: os.AttrWriteOnlyPaths, }, nil } diff --git a/internal/states/remote/state_test.go b/internal/states/remote/state_test.go index 43dd654fa687..cbb6a6219f07 100644 --- a/internal/states/remote/state_test.go +++ b/internal/states/remote/state_test.go @@ -129,9 +129,8 @@ func TestStatePersist(t *testing.T) { "attributes_flat": map[string]interface{}{ "filename": "file.txt", }, - "schema_version": 0.0, - "sensitive_attributes": []interface{}{}, - "write_only_attributes": []interface{}{}, + "schema_version": 0.0, + "sensitive_attributes": []interface{}{}, }, }, "mode": "managed", @@ -168,9 +167,8 @@ func TestStatePersist(t *testing.T) { "attributes_flat": map[string]interface{}{ "filename": "file.txt", }, - "schema_version": 0.0, - "sensitive_attributes": []interface{}{}, - "write_only_attributes": []interface{}{}, + "schema_version": 0.0, + "sensitive_attributes": []interface{}{}, }, }, "mode": "managed", diff --git a/internal/states/state_deepcopy.go b/internal/states/state_deepcopy.go index cf830512b2c7..f99828fea3b6 100644 --- a/internal/states/state_deepcopy.go +++ b/internal/states/state_deepcopy.go @@ -148,12 +148,6 @@ func (os *ResourceInstanceObjectSrc) DeepCopy() *ResourceInstanceObjectSrc { copy(sensitiveAttrPaths, os.AttrSensitivePaths) } - var writeOnlyAttrPaths []cty.Path - if os.AttrWriteOnlyPaths != nil { - writeOnlyAttrPaths = make([]cty.Path, len(os.AttrWriteOnlyPaths)) - copy(writeOnlyAttrPaths, os.AttrWriteOnlyPaths) - } - var private []byte if os.Private != nil { private = make([]byte, len(os.Private)) @@ -175,7 +169,6 @@ func (os *ResourceInstanceObjectSrc) DeepCopy() *ResourceInstanceObjectSrc { AttrsFlat: attrsFlat, AttrsJSON: attrsJSON, AttrSensitivePaths: sensitiveAttrPaths, - AttrWriteOnlyPaths: writeOnlyAttrPaths, Dependencies: dependencies, CreateBeforeDestroy: os.CreateBeforeDestroy, decodeValueCache: os.decodeValueCache, diff --git a/internal/states/statefile/version4.go b/internal/states/statefile/version4.go index ba47079fbec4..6477e66cf24c 100644 --- a/internal/states/statefile/version4.go +++ b/internal/states/statefile/version4.go @@ -166,16 +166,6 @@ func prepareStateV4(sV4 *stateV4) (*File, tfdiags.Diagnostics) { obj.AttrSensitivePaths = paths } - // write-only paths - if isV4.AttributeWriteOnlyPaths != nil { - paths, pathsDiags := unmarshalPaths([]byte(isV4.AttributeWriteOnlyPaths)) - diags = diags.Append(pathsDiags) - if pathsDiags.HasErrors() { - continue - } - obj.AttrWriteOnlyPaths = paths - } - { // Status raw := isV4.Status @@ -492,8 +482,6 @@ func appendInstanceObjectStateV4(rs *states.Resource, is *states.ResourceInstanc // Marshal paths to JSON attributeSensitivePaths, pathsDiags := marshalPaths(obj.AttrSensitivePaths) diags = diags.Append(pathsDiags) - attributeWriteOnlyPaths, pathsDiags := marshalPaths(obj.AttrWriteOnlyPaths) - diags = diags.Append(pathsDiags) return append(isV4s, instanceObjectStateV4{ IndexKey: rawKey, @@ -503,7 +491,6 @@ func appendInstanceObjectStateV4(rs *states.Resource, is *states.ResourceInstanc AttributesFlat: obj.AttrsFlat, AttributesRaw: obj.AttrsJSON, AttributeSensitivePaths: attributeSensitivePaths, - AttributeWriteOnlyPaths: attributeWriteOnlyPaths, PrivateRaw: privateRaw, Dependencies: deps, CreateBeforeDestroy: obj.CreateBeforeDestroy, @@ -714,7 +701,6 @@ type instanceObjectStateV4 struct { AttributesRaw json.RawMessage `json:"attributes,omitempty"` AttributesFlat map[string]string `json:"attributes_flat,omitempty"` AttributeSensitivePaths json.RawMessage `json:"sensitive_attributes,omitempty"` - AttributeWriteOnlyPaths json.RawMessage `json:"write_only_attributes,omitempty"` PrivateRaw []byte `json:"private,omitempty"` diff --git a/internal/terraform/context_apply_ephemeral_test.go b/internal/terraform/context_apply_ephemeral_test.go index 60d34d19c20c..6ae37bc03029 100644 --- a/internal/terraform/context_apply_ephemeral_test.go +++ b/internal/terraform/context_apply_ephemeral_test.go @@ -420,10 +420,6 @@ resource "ephem_write_only" "wo" { t.Fatalf("Expected 1 resource change, got %d", len(plan.Changes.Resources)) } - if len(plan.Changes.Resources[0].AfterWriteOnlyPaths) != 1 { - t.Fatalf("Expected 1 write-only attribute in plan, got %d", len(plan.Changes.Resources[0].AfterWriteOnlyPaths)) - } - schemas, schemaDiags := ctx.Schemas(m, plan.PriorState) assertNoDiagnostics(t, schemaDiags) planChanges, err := plan.Changes.Decode(schemas) @@ -475,14 +471,6 @@ resource "ephem_write_only" "wo" { if !attrs.Value.GetAttr("write_only").IsNull() { t.Fatalf("write_only attribute should be null") } - - if len(resourceInstance.Current.AttrWriteOnlyPaths) != 1 { - t.Fatalf("Expected 1 write only attribute in apply") - } - - if !resourceInstance.Current.AttrWriteOnlyPaths[0].Equals(cty.GetAttrPath("write_only")) { - t.Fatalf("Expected write_only to be a write only attribute") - } } func TestContext2Apply_update_write_only_attribute_not_in_plan_and_state(t *testing.T) { @@ -560,10 +548,6 @@ resource "ephem_write_only" "wo" { t.Fatalf("Expected 1 resource change, got %d", len(plan.Changes.Resources)) } - if len(plan.Changes.Resources[0].AfterWriteOnlyPaths) != 1 { - t.Fatalf("Expected 1 write-only attribute in plan, got %d", len(plan.Changes.Resources[0].AfterWriteOnlyPaths)) - } - schemas, schemaDiags := ctx.Schemas(m, plan.PriorState) assertNoDiagnostics(t, schemaDiags) planChanges, err := plan.Changes.Decode(schemas) @@ -615,14 +599,6 @@ resource "ephem_write_only" "wo" { if !attrs.Value.GetAttr("write_only").IsNull() { t.Fatalf("write_only attribute should be null") } - - if len(resourceInstance.Current.AttrWriteOnlyPaths) != 1 { - t.Fatalf("Expected 1 write only attribute in apply") - } - - if !resourceInstance.Current.AttrWriteOnlyPaths[0].Equals(cty.GetAttrPath("write_only")) { - t.Fatalf("Expected write_only to be a write only attribute") - } } func TestContext2Apply_normal_attributes_becomes_write_only_attribute(t *testing.T) { @@ -701,10 +677,6 @@ resource "ephem_write_only" "wo" { t.Fatalf("Expected 1 resource change, got %d", len(plan.Changes.Resources)) } - if len(plan.Changes.Resources[0].AfterWriteOnlyPaths) != 1 { - t.Fatalf("Expected 1 write-only attribute in plan, got %d", len(plan.Changes.Resources[0].AfterWriteOnlyPaths)) - } - schemas, schemaDiags := ctx.Schemas(m, plan.PriorState) assertNoDiagnostics(t, schemaDiags) planChanges, err := plan.Changes.Decode(schemas) @@ -756,12 +728,4 @@ resource "ephem_write_only" "wo" { if !attrs.Value.GetAttr("write_only").IsNull() { t.Fatalf("write_only attribute should be null") } - - if len(resourceInstance.Current.AttrWriteOnlyPaths) != 1 { - t.Fatalf("Expected 1 write only attribute in apply") - } - - if !resourceInstance.Current.AttrWriteOnlyPaths[0].Equals(cty.GetAttrPath("write_only")) { - t.Fatalf("Expected write_only to be a write only attribute") - } } diff --git a/internal/terraform/node_resource_abstract_instance.go b/internal/terraform/node_resource_abstract_instance.go index debc3bef2442..aac4f0e9051c 100644 --- a/internal/terraform/node_resource_abstract_instance.go +++ b/internal/terraform/node_resource_abstract_instance.go @@ -1215,9 +1215,6 @@ func (n *NodeAbstractResourceInstance) plan( // Marks will be removed when encoding. After: plannedNewVal, GeneratedConfig: n.generatedConfigHCL, - - BeforeWriteOnlyPaths: ephemeral.EphemeralValuePaths(priorVal), - AfterWriteOnlyPaths: ephemeralValuePaths, }, ActionReason: actionReason, RequiredReplace: reqRep, @@ -1231,10 +1228,9 @@ func (n *NodeAbstractResourceInstance) plan( // must _also_ record the returned change in the active plan, // which the expression evaluator will use in preference to this // incomplete value recorded in the state. - Status: states.ObjectPlanned, - Value: plannedNewVal, - Private: plannedPrivate, - AttrWriteOnlyPaths: ephemeralValuePaths, + Status: states.ObjectPlanned, + Value: plannedNewVal, + Private: plannedPrivate, } return plan, state, deferred, keyData, diags diff --git a/internal/terraform/node_resource_apply_instance.go b/internal/terraform/node_resource_apply_instance.go index b3da83443d77..552807646c96 100644 --- a/internal/terraform/node_resource_apply_instance.go +++ b/internal/terraform/node_resource_apply_instance.go @@ -10,7 +10,6 @@ import ( "github.com/hashicorp/terraform/internal/addrs" "github.com/hashicorp/terraform/internal/configs" "github.com/hashicorp/terraform/internal/instances" - "github.com/hashicorp/terraform/internal/lang/ephemeral" "github.com/hashicorp/terraform/internal/plans" "github.com/hashicorp/terraform/internal/plans/objchange" "github.com/hashicorp/terraform/internal/providers" @@ -324,10 +323,6 @@ func (n *NodeApplyableResourceInstance) managedResourceExecute(ctx EvalContext) if state != nil { // dependencies are always updated to match the configuration during apply state.Dependencies = n.Dependencies - - // The value is updated to remove ephemeral values - state.Value = ephemeral.RemoveEphemeralValues(state.Value) - state.AttrWriteOnlyPaths = diffApply.AfterWriteOnlyPaths } err = n.writeResourceInstanceState(ctx, state, workingState) if err != nil { From 1d93ed4f0da6883bb1a06c92ffeeebc02d525bc7 Mon Sep 17 00:00:00 2001 From: Daniel Schmidt Date: Mon, 25 Nov 2024 16:02:28 +0100 Subject: [PATCH 09/11] ephemeral: after plan set ephemeral values to null we got the marks from the proposed value, the provider gets the value without the marks we then reapply the marks and need to also remove the write-only values by setting the values to null --- .../terraform/node_resource_abstract_instance.go | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/internal/terraform/node_resource_abstract_instance.go b/internal/terraform/node_resource_abstract_instance.go index aac4f0e9051c..f884c6ed34c4 100644 --- a/internal/terraform/node_resource_abstract_instance.go +++ b/internal/terraform/node_resource_abstract_instance.go @@ -1046,6 +1046,10 @@ func (n *NodeAbstractResourceInstance) plan( plannedNewVal = marks.MarkPaths(plannedNewVal, marks.Sensitive, sensitivePaths) } + // From the point of view of the provider ephemeral value marks have been removed before plan + // and are reapplied now so we now need to set the values at these marks to null again. + plannedNewVal = ephemeral.RemoveEphemeralValues(plannedNewVal) + reqRep, reqRepDiags := getRequiredReplaces(unmarkedPriorVal, unmarkedPlannedNewVal, resp.RequiresReplace, n.ResolvedProvider.Provider, n.Addr) diags = diags.Append(reqRepDiags) if diags.HasErrors() { @@ -1181,18 +1185,6 @@ func (n *NodeAbstractResourceInstance) plan( } } - ephemeralValuePaths, _ := marks.PathsWithMark(unmarkedPaths, marks.Ephemeral) - if len(ephemeralValuePaths) > 0 { - for _, path := range ephemeralValuePaths { - diags = diags.Append(tfdiags.AttributeValue( - tfdiags.Error, - "Provider set ephemeral value during plan", - "The provider set an ephemeral value during plan, this is not allowed. This is most likely a bug in the provider, please file an issue on the providers issue tracker.", - path, - )) - } - } - // Call post-refresh hook diags = diags.Append(ctx.Hook(func(h Hook) (HookAction, error) { return h.PostDiff(n.HookResourceIdentity(), addrs.NotDeposed, action, priorVal, plannedNewVal) From 679d1b09c09d0ecf076602ff82dde9923d34ab4e Mon Sep 17 00:00:00 2001 From: Daniel Schmidt Date: Tue, 26 Nov 2024 14:29:00 +0100 Subject: [PATCH 10/11] ephemeral: providers are responsible for setting write-only attributes to null --- internal/plans/objchange/plan_valid.go | 10 ++ internal/plans/objchange/plan_valid_test.go | 62 +++++++ internal/providers/testing/provider_mock.go | 5 + .../terraform/context_apply_ephemeral_test.go | 155 ++++++++++++++++++ .../node_resource_abstract_instance.go | 55 ++++++- .../node_resource_abstract_instance_test.go | 135 +++++++++++++++ 6 files changed, 413 insertions(+), 9 deletions(-) diff --git a/internal/plans/objchange/plan_valid.go b/internal/plans/objchange/plan_valid.go index f03666a87c49..721fca77007e 100644 --- a/internal/plans/objchange/plan_valid.go +++ b/internal/plans/objchange/plan_valid.go @@ -301,6 +301,16 @@ func assertPlannedValueValid(attrS *configschema.Attribute, priorV, configV, pla return errs } + if attrS.WriteOnly { + // The provider is not allowed to return non-null values for write-only attributes + if !plannedV.IsNull() { + errs = append(errs, path.NewErrorf("planned value for write-only attribute is not null")) + } + + // We don't want to evaluate further if the attribute is write-only and null + return errs + } + switch { // The provider can plan any value for a computed-only attribute. There may // be a config value here in the case where a user used `ignore_changes` on diff --git a/internal/plans/objchange/plan_valid_test.go b/internal/plans/objchange/plan_valid_test.go index 4445f29bb6c2..614460869458 100644 --- a/internal/plans/objchange/plan_valid_test.go +++ b/internal/plans/objchange/plan_valid_test.go @@ -10,6 +10,7 @@ import ( "github.com/zclconf/go-cty/cty" "github.com/hashicorp/terraform/internal/configs/configschema" + "github.com/hashicorp/terraform/internal/lang/marks" "github.com/hashicorp/terraform/internal/tfdiags" ) @@ -1965,6 +1966,67 @@ func TestAssertPlanValid(t *testing.T) { }), []string{}, }, + + "write-only attributes": { + &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "foo": { + Type: cty.String, + WriteOnly: true, + }, + }, + }, + cty.ObjectVal(map[string]cty.Value{ + "foo": cty.NullVal(cty.String), + }), + cty.ObjectVal(map[string]cty.Value{ + "foo": cty.StringVal("write-only").Mark(marks.Ephemeral), + }), + cty.ObjectVal(map[string]cty.Value{ + "foo": cty.NullVal(cty.String), + }), + []string{}, + }, + + "nested write-only attributes": { + &configschema.Block{ + BlockTypes: map[string]*configschema.NestedBlock{ + "nested": { + Nesting: configschema.NestingList, + Block: configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "foo": { + Type: cty.String, + WriteOnly: true, + }, + }, + }, + }, + }, + }, + cty.ObjectVal(map[string]cty.Value{ + "nested": cty.ListVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "foo": cty.NullVal(cty.String), + }), + }), + }), + cty.ObjectVal(map[string]cty.Value{ + "nested": cty.ListVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "foo": cty.StringVal("write-only").Mark(marks.Ephemeral), + }), + }), + }), + cty.ObjectVal(map[string]cty.Value{ + "nested": cty.ListVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "foo": cty.NullVal(cty.String), + }), + }), + }), + []string{}, + }, } for name, test := range tests { diff --git a/internal/providers/testing/provider_mock.go b/internal/providers/testing/provider_mock.go index 08c4b41ea3b0..1f350c9531ea 100644 --- a/internal/providers/testing/provider_mock.go +++ b/internal/providers/testing/provider_mock.go @@ -422,6 +422,11 @@ func (p *MockProvider) PlanResourceChange(r providers.PlanResourceChangeRequest) return v, nil } + // Write-only attributes always return null + if attrSchema.WriteOnly { + return cty.NullVal(v.Type()), nil + } + // get the current configuration value, to detect when a // computed+optional attributes has become unset configVal, err := path.Apply(r.Config) diff --git a/internal/terraform/context_apply_ephemeral_test.go b/internal/terraform/context_apply_ephemeral_test.go index 6ae37bc03029..3f3e1fe30631 100644 --- a/internal/terraform/context_apply_ephemeral_test.go +++ b/internal/terraform/context_apply_ephemeral_test.go @@ -15,6 +15,7 @@ import ( "github.com/hashicorp/terraform/internal/providers" testing_provider "github.com/hashicorp/terraform/internal/providers/testing" "github.com/hashicorp/terraform/internal/states" + "github.com/hashicorp/terraform/internal/tfdiags" "github.com/zclconf/go-cty/cty" ) @@ -729,3 +730,157 @@ resource "ephem_write_only" "wo" { t.Fatalf("write_only attribute should be null") } } + +func TestContext2Apply_write_only_attribute_provider_plans_with_non_null_value(t *testing.T) { + m := testModuleInline(t, map[string]string{ + "main.tf": ` +variable "ephem" { + type = string + ephemeral = true +} + +resource "ephem_write_only" "wo" { + normal = "normal" + write_only = var.ephem +} +`, + }) + + ephem := &testing_provider.MockProvider{ + GetProviderSchemaResponse: &providers.GetProviderSchemaResponse{ + ResourceTypes: map[string]providers.Schema{ + "ephem_write_only": { + Block: &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "normal": { + Type: cty.String, + Required: true, + }, + "write_only": { + Type: cty.String, + WriteOnly: true, + Required: true, + }, + }, + }, + }, + }, + }, + PlanResourceChangeResponse: &providers.PlanResourceChangeResponse{ + PlannedState: cty.ObjectVal(map[string]cty.Value{ + "normal": cty.StringVal("normal"), + "write_only": cty.StringVal("the provider should have set this to null"), + }), + }, + } + + ctx := testContext2(t, &ContextOpts{ + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("ephem"): testProviderFuncFixed(ephem), + }, + }) + + ephemVar := &InputValue{ + Value: cty.StringVal("ephemeral_value"), + SourceType: ValueFromCLIArg, + } + + _, diags := ctx.Plan(m, nil, &PlanOpts{ + Mode: plans.NormalMode, + SetVariables: InputValues{ + "ephem": ephemVar, + }, + }) + + var expectedDiags tfdiags.Diagnostics + + expectedDiags = append(expectedDiags, tfdiags.Sourceless( + tfdiags.Error, + "Provider produced invalid plan", + `Provider "registry.terraform.io/hashicorp/ephem" planned an invalid value for ephem_write_only.wo.write_only: planned value for write-only attribute is not null. + +This is a bug in the provider, which should be reported in the provider's own issue tracker.`, + )) + + assertDiagnosticsMatch(t, diags, expectedDiags) +} + +func TestContext2Apply_write_only_attribute_provider_applies_with_non_null_value(t *testing.T) { + m := testModuleInline(t, map[string]string{ + "main.tf": ` +variable "ephem" { + type = string + ephemeral = true +} + +resource "ephem_write_only" "wo" { + normal = "normal" + write_only = var.ephem +} +`, + }) + + ephem := &testing_provider.MockProvider{ + GetProviderSchemaResponse: &providers.GetProviderSchemaResponse{ + ResourceTypes: map[string]providers.Schema{ + "ephem_write_only": { + Block: &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "normal": { + Type: cty.String, + Required: true, + }, + "write_only": { + Type: cty.String, + WriteOnly: true, + Required: true, + }, + }, + }, + }, + }, + }, + ApplyResourceChangeResponse: &providers.ApplyResourceChangeResponse{ + NewState: cty.ObjectVal(map[string]cty.Value{ + "normal": cty.StringVal("normal"), + "write_only": cty.StringVal("the provider should have set this to null"), + }), + }, + } + + ctx := testContext2(t, &ContextOpts{ + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("ephem"): testProviderFuncFixed(ephem), + }, + }) + + ephemVar := &InputValue{ + Value: cty.StringVal("ephemeral_value"), + SourceType: ValueFromCLIArg, + } + + plan, planDiags := ctx.Plan(m, nil, &PlanOpts{ + Mode: plans.NormalMode, + SetVariables: InputValues{ + "ephem": ephemVar, + }, + }) + + assertNoDiagnostics(t, planDiags) + + _, diags := ctx.Apply(plan, m, &ApplyOpts{ + SetVariables: InputValues{ + "ephem": ephemVar, + }, + }) + + var expectedDiags tfdiags.Diagnostics + + expectedDiags = append(expectedDiags, tfdiags.Sourceless( + tfdiags.Error, + "Write-only attribute set during apply", + `Provider "provider[\"registry.terraform.io/hashicorp/ephem\"]" set the write-only attribute "ephem_write_only.wo.write_only" during apply. Write-only attributes must not be set by the provider during apply, so this is a bug in the provider, which should be reported in the provider's own issue tracker.`, + )) + + assertDiagnosticsMatch(t, diags, expectedDiags) +} diff --git a/internal/terraform/node_resource_abstract_instance.go b/internal/terraform/node_resource_abstract_instance.go index f884c6ed34c4..93d163970d71 100644 --- a/internal/terraform/node_resource_abstract_instance.go +++ b/internal/terraform/node_resource_abstract_instance.go @@ -17,7 +17,6 @@ import ( "github.com/hashicorp/terraform/internal/configs" "github.com/hashicorp/terraform/internal/configs/configschema" "github.com/hashicorp/terraform/internal/instances" - "github.com/hashicorp/terraform/internal/lang/ephemeral" "github.com/hashicorp/terraform/internal/lang/marks" "github.com/hashicorp/terraform/internal/moduletest/mocking" "github.com/hashicorp/terraform/internal/plans" @@ -1039,17 +1038,16 @@ func (n *NodeAbstractResourceInstance) plan( // Add the marks back to the planned new value -- this must happen after // ignore changes have been processed. We add in the schema marks as well, // to ensure that provider defined private attributes are marked correctly - // here. + // here. We remove the ephemeral marks, the provider is expected to return null + // for write-only attributes (the only place where ephemeral values are allowed). + // This is verified in objchange.AssertPlanValid already. unmarkedPlannedNewVal := plannedNewVal - plannedNewVal = plannedNewVal.MarkWithPaths(unmarkedPaths) + _, nonEphemeralMarks := marks.PathsWithMark(unmarkedPaths, marks.Ephemeral) + plannedNewVal = plannedNewVal.MarkWithPaths(nonEphemeralMarks) if sensitivePaths := schema.SensitivePaths(plannedNewVal, nil); len(sensitivePaths) != 0 { plannedNewVal = marks.MarkPaths(plannedNewVal, marks.Sensitive, sensitivePaths) } - // From the point of view of the provider ephemeral value marks have been removed before plan - // and are reapplied now so we now need to set the values at these marks to null again. - plannedNewVal = ephemeral.RemoveEphemeralValues(plannedNewVal) - reqRep, reqRepDiags := getRequiredReplaces(unmarkedPriorVal, unmarkedPlannedNewVal, resp.RequiresReplace, n.ResolvedProvider.Provider, n.Addr) diags = diags.Append(reqRepDiags) if diags.HasErrors() { @@ -1126,8 +1124,8 @@ func (n *NodeAbstractResourceInstance) plan( plannedNewVal = resp.PlannedState plannedPrivate = resp.PlannedPrivate - if len(unmarkedPaths) > 0 { - plannedNewVal = plannedNewVal.MarkWithPaths(unmarkedPaths) + if len(nonEphemeralMarks) > 0 { + plannedNewVal = plannedNewVal.MarkWithPaths(nonEphemeralMarks) } for _, err := range plannedNewVal.Type().TestConformance(schema.ImpliedType()) { @@ -2562,6 +2560,25 @@ func (n *NodeAbstractResourceInstance) apply( return nil, diags } + // Providers are supposed to return null values for all write-only attributes + var writeOnlyDiags tfdiags.Diagnostics + if writeOnlyPaths := NonNullWriteOnlyPaths(newVal, schema, nil); len(writeOnlyPaths) != 0 { + for _, p := range writeOnlyPaths { + writeOnlyDiags = writeOnlyDiags.Append(tfdiags.Sourceless( + tfdiags.Error, + "Write-only attribute set during apply", + fmt.Sprintf( + "Provider %q set the write-only attribute \"%s%s\" during apply. Write-only attributes must not be set by the provider during apply, so this is a bug in the provider, which should be reported in the provider's own issue tracker.", + n.ResolvedProvider.String(), n.Addr.String(), tfdiags.FormatCtyPath(p), + ), + )) + } + diags = diags.Append(writeOnlyDiags) + } + if writeOnlyDiags.HasErrors() { + return nil, diags + } + // After this point we have a type-conforming result object and so we // must always run to completion to ensure it can be saved. If n.Error // is set then we must not return a non-nil error, in order to allow @@ -2733,6 +2750,26 @@ func (n *NodeAbstractResourceInstance) prevRunAddr(ctx EvalContext) addrs.AbsRes return resourceInstancePrevRunAddr(ctx, n.Addr) } +// NonNullWriteOnlyPaths returns a list of paths to attributes that are write-only +// and non-null in the given value. +func NonNullWriteOnlyPaths(val cty.Value, schema *configschema.Block, p cty.Path) (paths []cty.Path) { + for name, attr := range schema.Attributes { + attrPath := append(p, cty.GetAttrStep{Name: name}) + attrVal, _ := attrPath.Apply(val) + if attr.WriteOnly && !attrVal.IsNull() { + paths = append(paths, attrPath) + } + } + + for name, blockS := range schema.BlockTypes { + blockPath := append(p, cty.GetAttrStep{Name: name}) + x := NonNullWriteOnlyPaths(val, &blockS.Block, blockPath) + paths = append(paths, x...) + } + + return paths +} + func resourceInstancePrevRunAddr(ctx EvalContext, currentAddr addrs.AbsResourceInstance) addrs.AbsResourceInstance { table := ctx.MoveResults() return table.OldAddr(currentAddr) diff --git a/internal/terraform/node_resource_abstract_instance_test.go b/internal/terraform/node_resource_abstract_instance_test.go index 79810b81a1fc..46eea2404641 100644 --- a/internal/terraform/node_resource_abstract_instance_test.go +++ b/internal/terraform/node_resource_abstract_instance_test.go @@ -250,3 +250,138 @@ func TestNodeAbstractResourceInstance_refresh_with_deferred_read(t *testing.T) { t.Fatalf("expected deferral to be AbsentPrereq, got %s", deferred.Reason) } } + +func TestNonNullWriteOnlyPaths(t *testing.T) { + for name, tc := range map[string]struct { + val cty.Value + schema *configschema.Block + + expectedPaths []cty.Path + }{ + "no write-only attributes": { + val: cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("i-abc123"), + }), + schema: &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "id": { + Type: cty.String, + }, + }, + }, + }, + + "write-only attribute with null value": { + val: cty.ObjectVal(map[string]cty.Value{ + "id": cty.NullVal(cty.String), + }), + schema: &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "id": { + Type: cty.String, + Optional: true, + WriteOnly: true, + }, + }, + }, + }, + + "write-only attribute with non-null value": { + val: cty.ObjectVal(map[string]cty.Value{ + "valid": cty.NullVal(cty.String), + "id": cty.StringVal("i-abc123"), + }), + schema: &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "valid": { + Type: cty.String, + Optional: true, + WriteOnly: true, + }, + "id": { + Type: cty.String, + Optional: true, + WriteOnly: true, + }, + }, + }, + expectedPaths: []cty.Path{cty.GetAttrPath("id")}, + }, + + "write-only attributes in blocks": { + val: cty.ObjectVal(map[string]cty.Value{ + "foo": cty.ObjectVal(map[string]cty.Value{ + "valid-write-only": cty.NullVal(cty.String), + "valid": cty.StringVal("valid"), + "id": cty.StringVal("i-abc123"), + "bar": cty.ObjectVal(map[string]cty.Value{ + "valid-write-only": cty.NullVal(cty.String), + "valid": cty.StringVal("valid"), + "id": cty.StringVal("i-abc123"), + }), + }), + }), + schema: &configschema.Block{ + BlockTypes: map[string]*configschema.NestedBlock{ + "foo": { + Block: configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "valid-write-only": { + Type: cty.String, + Optional: true, + WriteOnly: true, + }, + "valid": { + Type: cty.String, + Optional: true, + }, + "id": { + Type: cty.String, + Optional: true, + WriteOnly: true, + }, + }, + BlockTypes: map[string]*configschema.NestedBlock{ + "bar": { + Block: configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "valid-write-only": { + Type: cty.String, + Optional: true, + WriteOnly: true, + }, + "valid": { + Type: cty.String, + Optional: true, + }, + "id": { + Type: cty.String, + Optional: true, + WriteOnly: true, + }, + }, + }, + }, + }, + }, + }, + }, + }, + expectedPaths: []cty.Path{cty.GetAttrPath("foo").GetAttr("id"), cty.GetAttrPath("foo").GetAttr("bar").GetAttr("id")}, + }, + } { + t.Run(name, func(t *testing.T) { + paths := NonNullWriteOnlyPaths(tc.val, tc.schema, nil) + + if len(paths) != len(tc.expectedPaths) { + t.Fatalf("expected %d write-only paths, got %d", len(tc.expectedPaths), len(paths)) + } + + for i, path := range paths { + if !path.Equals(tc.expectedPaths[i]) { + t.Fatalf("expected path %#v, got %#v", tc.expectedPaths[i], path) + } + } + }) + } +} From 846838752a1c3045c3583b1e4dfd92c69cf40a25 Mon Sep 17 00:00:00 2001 From: Daniel Schmidt Date: Thu, 28 Nov 2024 10:48:45 +0100 Subject: [PATCH 11/11] ephemeral: add write-only attribute to proto to schema conversion --- internal/plugin/convert/schema.go | 1 + internal/plugin6/convert/schema.go | 1 + 2 files changed, 2 insertions(+) diff --git a/internal/plugin/convert/schema.go b/internal/plugin/convert/schema.go index 41e48ba61806..05db62bcbff3 100644 --- a/internal/plugin/convert/schema.go +++ b/internal/plugin/convert/schema.go @@ -118,6 +118,7 @@ func ProtoToConfigSchema(b *proto.Schema_Block) *configschema.Block { Computed: a.Computed, Sensitive: a.Sensitive, Deprecated: a.Deprecated, + WriteOnly: a.WriteOnly, } if err := json.Unmarshal(a.Type, &attr.Type); err != nil { diff --git a/internal/plugin6/convert/schema.go b/internal/plugin6/convert/schema.go index 39c1ff62044b..6d10ed8f422c 100644 --- a/internal/plugin6/convert/schema.go +++ b/internal/plugin6/convert/schema.go @@ -124,6 +124,7 @@ func ProtoToConfigSchema(b *proto.Schema_Block) *configschema.Block { Computed: a.Computed, Sensitive: a.Sensitive, Deprecated: a.Deprecated, + WriteOnly: a.WriteOnly, } if a.Type != nil {