From 970ff7f6ecc6e739cabfe70fadeb4067d4002d44 Mon Sep 17 00:00:00 2001 From: Daniel Schmidt Date: Tue, 26 Nov 2024 14:29:00 +0100 Subject: [PATCH] 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 f03666a87c..721fca7700 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 4445f29bb6..6144608694 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 08c4b41ea3..1f350c9531 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 6ae37bc030..3f3e1fe306 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 a9887ba7f4..75ac7dc245 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" @@ -1026,17 +1025,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() { @@ -1113,8 +1111,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()) { @@ -2549,6 +2547,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 @@ -2720,6 +2737,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 79810b81a1..46eea24046 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) + } + } + }) + } +}