From cd9f942d31c1a8e9147ecd91cfd820bcf41ddffc Mon Sep 17 00:00:00 2001 From: Selena Goods Date: Mon, 10 Feb 2025 17:14:13 -0500 Subject: [PATCH 1/8] Remove unused `RequiredWriteOnlyNilsAttributePaths()` function --- .../fwserver/server_planresourcechange.go | 62 ---- .../server_planresourcechange_test.go | 300 ------------------ 2 files changed, 362 deletions(-) diff --git a/internal/fwserver/server_planresourcechange.go b/internal/fwserver/server_planresourcechange.go index aacd2c998..ac8cac6bf 100644 --- a/internal/fwserver/server_planresourcechange.go +++ b/internal/fwserver/server_planresourcechange.go @@ -13,7 +13,6 @@ import ( "github.com/hashicorp/terraform-plugin-framework/attr" "github.com/hashicorp/terraform-plugin-framework/diag" - "github.com/hashicorp/terraform-plugin-framework/internal/fromtftypes" "github.com/hashicorp/terraform-plugin-framework/internal/fwschema" "github.com/hashicorp/terraform-plugin-framework/internal/fwschemadata" "github.com/hashicorp/terraform-plugin-framework/internal/logging" @@ -513,67 +512,6 @@ func NormaliseRequiresReplace(ctx context.Context, rs path.Paths) path.Paths { return ret[:j] } -// RequiredWriteOnlyNilsAttributePaths returns a tftypes.Walk() function -// that populates reqWriteOnlyNilsPaths with the paths of Required and WriteOnly -// attributes that have a null value. -func RequiredWriteOnlyNilsAttributePaths(ctx context.Context, schema fwschema.Schema, reqWriteOnlyNilsPaths *path.Paths) func(path *tftypes.AttributePath, value tftypes.Value) (bool, error) { - return func(attrPath *tftypes.AttributePath, value tftypes.Value) (bool, error) { - // we are only modifying attributes, not the entire resource - if len(attrPath.Steps()) < 1 { - return true, nil - } - - ctx = logging.FrameworkWithAttributePath(ctx, attrPath.String()) - - attribute, err := schema.AttributeAtTerraformPath(ctx, attrPath) - - if err != nil { - if errors.Is(err, fwschema.ErrPathInsideAtomicAttribute) { - // atomic attributes can be nested block objects that contain child writeOnly attributes - return true, nil - } - - if errors.Is(err, fwschema.ErrPathIsBlock) { - // blocks do not have the writeOnly field but can contain child writeOnly attributes - return true, nil - } - - if errors.Is(err, fwschema.ErrPathInsideDynamicAttribute) { - return false, nil - } - - logging.FrameworkError(ctx, "couldn't find attribute in resource schema") - return false, fmt.Errorf("couldn't find attribute in resource schema: %w", err) - } - - if attribute.IsWriteOnly() { - if attribute.IsRequired() && value.IsNull() { - fwPath, diags := fromtftypes.AttributePath(ctx, attrPath, schema) - if diags.HasError() { - for _, diagErr := range diags.Errors() { - logging.FrameworkError(ctx, - "Error converting tftypes.AttributePath to path.Path", - map[string]any{ - logging.KeyError: diagErr.Detail(), - }, - ) - } - - return false, fmt.Errorf("couldn't convert tftypes.AttributePath to path.Path") - } - reqWriteOnlyNilsPaths.Append(fwPath) - - // if the value is nil, there is no need to continue walking - return false, nil - } - // check for any writeOnly child attributes - return true, nil - } - - return false, nil - } -} - // planToState returns a *tfsdk.State with a copied value from a tfsdk.Plan. func planToState(plan tfsdk.Plan) *tfsdk.State { return &tfsdk.State{ diff --git a/internal/fwserver/server_planresourcechange_test.go b/internal/fwserver/server_planresourcechange_test.go index 4e22b36c6..66e693935 100644 --- a/internal/fwserver/server_planresourcechange_test.go +++ b/internal/fwserver/server_planresourcechange_test.go @@ -7,7 +7,6 @@ import ( "context" "fmt" "math/big" - "sort" "testing" "github.com/google/go-cmp/cmp" @@ -364,305 +363,6 @@ func TestMarkComputedNilsAsUnknown(t *testing.T) { } } -func TestRequiredWriteOnlyNilsAttributePath(t *testing.T) { - t.Parallel() - - s := schema.Schema{ - Attributes: map[string]schema.Attribute{ - "string-value": schema.StringAttribute{ - Required: true, - }, - "string-nil-optional-writeonly": schema.StringAttribute{ - Optional: true, - WriteOnly: true, - }, - "string-value-optional-writeonly": schema.StringAttribute{ - Optional: true, - WriteOnly: true, - }, - "string-nil-required-writeonly": schema.StringAttribute{ - Required: true, - WriteOnly: true, - }, - "string-value-required-writeonly": schema.StringAttribute{ - Required: true, - WriteOnly: true, - }, - "list-value": schema.ListAttribute{ - ElementType: types.StringType, - Required: true, - }, - "list-nil-optional-writeonly": schema.ListAttribute{ - ElementType: types.StringType, - Optional: true, - WriteOnly: true, - }, - "list-value-optional-writeonly": schema.ListAttribute{ - ElementType: types.StringType, - Optional: true, - WriteOnly: true, - }, - "list-nil-required-writeonly": schema.ListAttribute{ - ElementType: types.StringType, - Required: true, - WriteOnly: true, - }, - "list-value-required-writeonly": schema.ListAttribute{ - ElementType: types.StringType, - Required: true, - WriteOnly: true, - }, - "dynamic-value": schema.DynamicAttribute{ - Required: true, - }, - "dynamic-nil-optional-writeonly": schema.DynamicAttribute{ - Optional: true, - WriteOnly: true, - }, - "dynamic-value-optional-writeonly": schema.DynamicAttribute{ - Optional: true, - WriteOnly: true, - }, - "dynamic-nil-required-writeonly": schema.DynamicAttribute{ - Required: true, - WriteOnly: true, - }, - "dynamic-value-required-writeonly": schema.DynamicAttribute{ - Required: true, - WriteOnly: true, - }, - // underlying values of dynamic attributes should be left alone - "dynamic-value-with-underlying-list-required-writeonly": schema.DynamicAttribute{ - Required: true, - WriteOnly: true, - }, - "object-nil-required-writeonly": schema.ObjectAttribute{ - AttributeTypes: map[string]attr.Type{ - "string-nil": types.StringType, - "string-set": types.StringType, - }, - Required: true, - WriteOnly: true, - }, - "object-value-required-writeonly": schema.ObjectAttribute{ - AttributeTypes: map[string]attr.Type{ - "string-nil": types.StringType, - "string-set": types.StringType, - }, - Required: true, - WriteOnly: true, - }, - "nested-nil-required-writeonly": schema.SingleNestedAttribute{ - Attributes: map[string]schema.Attribute{ - "string-nil": schema.StringAttribute{ - Required: true, - WriteOnly: true, - }, - "string-set": schema.StringAttribute{ - Required: true, - WriteOnly: true, - }, - }, - Required: true, - WriteOnly: true, - }, - "nested-value-required-writeonly": schema.SingleNestedAttribute{ - Attributes: map[string]schema.Attribute{ - "string-nil": schema.StringAttribute{ - Required: true, - WriteOnly: true, - }, - "string-set": schema.StringAttribute{ - Required: true, - WriteOnly: true, - }, - }, - Required: true, - WriteOnly: true, - }, - "optional-nested-value-required-writeonly-attributes": schema.SingleNestedAttribute{ - Attributes: map[string]schema.Attribute{ - "string-nil": schema.StringAttribute{ - Required: true, - WriteOnly: true, - }, - "string-set": schema.StringAttribute{ - Required: true, - WriteOnly: true, - }, - }, - Optional: true, - WriteOnly: true, - }, - }, - Blocks: map[string]schema.Block{ - "block-nil-required-writeonly-attributes": schema.SetNestedBlock{ - NestedObject: schema.NestedBlockObject{ - Attributes: map[string]schema.Attribute{ - "string-nil": schema.StringAttribute{ - Required: true, - WriteOnly: true, - }, - "string-set": schema.StringAttribute{ - Required: true, - WriteOnly: true, - }, - }, - }, - }, - "block-value-required-writeonly-attributes": schema.SetNestedBlock{ - NestedObject: schema.NestedBlockObject{ - Attributes: map[string]schema.Attribute{ - "string-nil": schema.StringAttribute{ - Required: true, - WriteOnly: true, - }, - "string-set": schema.StringAttribute{ - Required: true, - WriteOnly: true, - }, - }, - }, - }, - }, - } - input := tftypes.NewValue(s.Type().TerraformType(context.Background()), map[string]tftypes.Value{ - "string-value": tftypes.NewValue(tftypes.String, "hello, world"), - "string-nil-optional-writeonly": tftypes.NewValue(tftypes.String, nil), - "string-value-optional-writeonly": tftypes.NewValue(tftypes.String, "hello, world"), - "string-nil-required-writeonly": tftypes.NewValue(tftypes.String, nil), - "string-value-required-writeonly": tftypes.NewValue(tftypes.String, "hello, world"), - "list-value": tftypes.NewValue(tftypes.List{ElementType: tftypes.String}, []tftypes.Value{tftypes.NewValue(tftypes.String, "hello, world")}), - "list-nil-optional-writeonly": tftypes.NewValue(tftypes.List{ElementType: tftypes.String}, nil), - "list-value-optional-writeonly": tftypes.NewValue(tftypes.List{ElementType: tftypes.String}, []tftypes.Value{tftypes.NewValue(tftypes.String, "hello, world")}), - "list-nil-required-writeonly": tftypes.NewValue(tftypes.List{ElementType: tftypes.String}, nil), - "list-value-required-writeonly": tftypes.NewValue(tftypes.List{ElementType: tftypes.String}, []tftypes.Value{ - tftypes.NewValue(tftypes.String, "hello, world"), - tftypes.NewValue(tftypes.String, nil), - }), - "dynamic-value": tftypes.NewValue(tftypes.String, "hello, world"), - "dynamic-nil-optional-writeonly": tftypes.NewValue(tftypes.DynamicPseudoType, nil), - "dynamic-value-optional-writeonly": tftypes.NewValue(tftypes.String, "hello, world"), - "dynamic-nil-required-writeonly": tftypes.NewValue(tftypes.DynamicPseudoType, nil), - "dynamic-value-required-writeonly": tftypes.NewValue(tftypes.String, "hello, world"), - "dynamic-value-with-underlying-list-required-writeonly": tftypes.NewValue( - tftypes.List{ - ElementType: tftypes.Bool, - }, - []tftypes.Value{ - tftypes.NewValue(tftypes.Bool, true), - tftypes.NewValue(tftypes.Bool, nil), - }, - ), - "object-nil-required-writeonly": tftypes.NewValue(tftypes.Object{ - AttributeTypes: map[string]tftypes.Type{ - "string-nil": tftypes.String, - "string-set": tftypes.String, - }, - }, nil), - "object-value-required-writeonly": tftypes.NewValue(tftypes.Object{ - AttributeTypes: map[string]tftypes.Type{ - "string-nil": tftypes.String, - "string-set": tftypes.String, - }, - }, map[string]tftypes.Value{ - "string-nil": tftypes.NewValue(tftypes.String, nil), - "string-set": tftypes.NewValue(tftypes.String, "foo"), - }), - "nested-nil-required-writeonly": tftypes.NewValue(tftypes.Object{ - AttributeTypes: map[string]tftypes.Type{ - "string-nil": tftypes.String, - "string-set": tftypes.String, - }, - }, nil), - "nested-value-required-writeonly": tftypes.NewValue(tftypes.Object{ - AttributeTypes: map[string]tftypes.Type{ - "string-nil": tftypes.String, - "string-set": tftypes.String, - }, - }, map[string]tftypes.Value{ - "string-nil": tftypes.NewValue(tftypes.String, nil), - "string-set": tftypes.NewValue(tftypes.String, "bar"), - }), - "optional-nested-value-required-writeonly-attributes": tftypes.NewValue(tftypes.Object{ - AttributeTypes: map[string]tftypes.Type{ - "string-nil": tftypes.String, - "string-set": tftypes.String, - }, - }, map[string]tftypes.Value{ - "string-nil": tftypes.NewValue(tftypes.String, nil), - "string-set": tftypes.NewValue(tftypes.String, "bar"), - }), - "block-nil-required-writeonly-attributes": tftypes.NewValue(tftypes.Set{ - ElementType: tftypes.Object{ - AttributeTypes: map[string]tftypes.Type{ - "string-nil": tftypes.String, - "string-set": tftypes.String, - }, - }, - }, nil), - "block-value-required-writeonly-attributes": tftypes.NewValue(tftypes.Set{ - ElementType: tftypes.Object{ - AttributeTypes: map[string]tftypes.Type{ - "string-nil": tftypes.String, - "string-set": tftypes.String, - }, - }, - }, []tftypes.Value{ - tftypes.NewValue(tftypes.Object{ - AttributeTypes: map[string]tftypes.Type{ - "string-nil": tftypes.String, - "string-set": tftypes.String, - }, - }, map[string]tftypes.Value{ - "string-nil": tftypes.NewValue(tftypes.String, nil), - "string-set": tftypes.NewValue(tftypes.String, "bar"), - }), - }), - }) - expected := path.Paths{ - path.Root("block-value-required-writeonly-attributes"). - AtSetValue(types.ObjectValueMust( - map[string]attr.Type{ - "string-nil": types.StringType, - "string-set": types.StringType, - }, - map[string]attr.Value{ - "string-nil": types.StringNull(), - "string-set": types.StringValue("bar"), - }, - )). - AtName("string-nil"), - path.Root("dynamic-nil-required-writeonly"), - path.Root("list-nil-required-writeonly"), - path.Root("nested-value-required-writeonly").AtName("string-nil"), - path.Root("object-nil-required-writeonly"), - path.Root("optional-nested-value-required-writeonly-attributes").AtName("string-nil"), - path.Root("string-nil-required-writeonly"), - path.Root("nested-nil-required-writeonly"), - } - - var got path.Paths - err := tftypes.Walk(input, fwserver.RequiredWriteOnlyNilsAttributePaths(context.Background(), s, &got)) - if err != nil { - t.Errorf("Unexpected error: %s", err) - return - } - - sort.Slice(got, func(i, j int) bool { - return got[i].String() < got[j].String() - }) - - sort.Slice(expected, func(i, j int) bool { - return expected[i].String() < expected[j].String() - }) - - if diff := cmp.Diff(got, expected, cmpopts.EquateEmpty()); diff != "" { - t.Errorf("Unexpected diff (+wanted, -got): %s", diff) - return - } -} - func TestNormaliseRequiresReplace(t *testing.T) { t.Parallel() From b2d165b97c972b9c3bd5346e43d6f756ce9724f7 Mon Sep 17 00:00:00 2001 From: Selena Goods Date: Mon, 10 Feb 2025 17:21:43 -0500 Subject: [PATCH 2/8] Remove dynamic type conditional from write_only_nullification.go --- internal/fwserver/write_only_nullification.go | 9 ----- .../fwserver/write_only_nullification_test.go | 37 ++++++++----------- 2 files changed, 16 insertions(+), 30 deletions(-) diff --git a/internal/fwserver/write_only_nullification.go b/internal/fwserver/write_only_nullification.go index 5bfc77609..0d95152e8 100644 --- a/internal/fwserver/write_only_nullification.go +++ b/internal/fwserver/write_only_nullification.go @@ -12,7 +12,6 @@ import ( "github.com/hashicorp/terraform-plugin-framework/internal/fwschema" "github.com/hashicorp/terraform-plugin-framework/internal/logging" - "github.com/hashicorp/terraform-plugin-framework/types/basetypes" ) // NullifyWriteOnlyAttributes transforms a tftypes.Value, setting all write-only attribute values @@ -58,14 +57,6 @@ func NullifyWriteOnlyAttributes(ctx context.Context, resourceSchema fwschema.Sch // Value type from new state to create null with newValueType := attribute.GetType().TerraformType(ctx) - // If the attribute is dynamic set the new value type to DynamicPseudoType - // instead of the underlying concrete type - // TODO: verify if this is the correct behavior once Terraform Core implementation is complete - _, isDynamic := attribute.GetType().(basetypes.DynamicTypable) - if isDynamic { - newValueType = tftypes.DynamicPseudoType - } - if attribute.IsWriteOnly() && !val.IsNull() { logging.FrameworkDebug(ctx, "Nullifying write-only attribute in the newState") diff --git a/internal/fwserver/write_only_nullification_test.go b/internal/fwserver/write_only_nullification_test.go index 7aba06585..c73851772 100644 --- a/internal/fwserver/write_only_nullification_test.go +++ b/internal/fwserver/write_only_nullification_test.go @@ -39,9 +39,6 @@ func TestNullifyWriteOnlyAttributes(t *testing.T) { "dynamic-nil": schema.DynamicAttribute{ Optional: true, }, - "dynamic-underlying-string-nil-computed": schema.DynamicAttribute{ - WriteOnly: true, - }, "dynamic-nil-write-only": schema.DynamicAttribute{ Optional: true, WriteOnly: true, @@ -131,15 +128,14 @@ func TestNullifyWriteOnlyAttributes(t *testing.T) { }, } input := tftypes.NewValue(s.Type().TerraformType(context.Background()), map[string]tftypes.Value{ - "string-value": tftypes.NewValue(tftypes.String, "hello, world"), - "string-nil": tftypes.NewValue(tftypes.String, nil), - "string-nil-write-only": tftypes.NewValue(tftypes.String, nil), - "string-value-write-only": tftypes.NewValue(tftypes.String, "hello, world"), - "dynamic-value": tftypes.NewValue(tftypes.String, "hello, world"), - "dynamic-nil": tftypes.NewValue(tftypes.DynamicPseudoType, nil), - "dynamic-underlying-string-nil-computed": tftypes.NewValue(tftypes.String, nil), - "dynamic-nil-write-only": tftypes.NewValue(tftypes.DynamicPseudoType, nil), - "dynamic-value-write-only": tftypes.NewValue(tftypes.String, "hello, world"), + "string-value": tftypes.NewValue(tftypes.String, "hello, world"), + "string-nil": tftypes.NewValue(tftypes.String, nil), + "string-nil-write-only": tftypes.NewValue(tftypes.String, nil), + "string-value-write-only": tftypes.NewValue(tftypes.String, "hello, world"), + "dynamic-value": tftypes.NewValue(tftypes.String, "hello, world"), + "dynamic-nil": tftypes.NewValue(tftypes.DynamicPseudoType, nil), + "dynamic-nil-write-only": tftypes.NewValue(tftypes.DynamicPseudoType, nil), + "dynamic-value-write-only": tftypes.NewValue(tftypes.String, "hello, world"), "dynamic-value-with-underlying-list-write-only": tftypes.NewValue( tftypes.List{ ElementType: tftypes.Bool, @@ -207,15 +203,14 @@ func TestNullifyWriteOnlyAttributes(t *testing.T) { }), }) expected := tftypes.NewValue(s.Type().TerraformType(context.Background()), map[string]tftypes.Value{ - "string-value": tftypes.NewValue(tftypes.String, "hello, world"), - "string-nil": tftypes.NewValue(tftypes.String, nil), - "string-nil-write-only": tftypes.NewValue(tftypes.String, nil), - "string-value-write-only": tftypes.NewValue(tftypes.String, nil), - "dynamic-value": tftypes.NewValue(tftypes.String, "hello, world"), - "dynamic-nil": tftypes.NewValue(tftypes.DynamicPseudoType, nil), - "dynamic-underlying-string-nil-computed": tftypes.NewValue(tftypes.DynamicPseudoType, nil), - "dynamic-nil-write-only": tftypes.NewValue(tftypes.DynamicPseudoType, nil), - "dynamic-value-write-only": tftypes.NewValue(tftypes.DynamicPseudoType, nil), + "string-value": tftypes.NewValue(tftypes.String, "hello, world"), + "string-nil": tftypes.NewValue(tftypes.String, nil), + "string-nil-write-only": tftypes.NewValue(tftypes.String, nil), + "string-value-write-only": tftypes.NewValue(tftypes.String, nil), + "dynamic-value": tftypes.NewValue(tftypes.String, "hello, world"), + "dynamic-nil": tftypes.NewValue(tftypes.DynamicPseudoType, nil), + "dynamic-nil-write-only": tftypes.NewValue(tftypes.DynamicPseudoType, nil), + "dynamic-value-write-only": tftypes.NewValue(tftypes.DynamicPseudoType, nil), "dynamic-value-with-underlying-list-write-only": tftypes.NewValue(tftypes.DynamicPseudoType, nil), "object-nil-write-only": tftypes.NewValue(tftypes.Object{ AttributeTypes: map[string]tftypes.Type{ From f30b47b01dda288df7dcf6dd56f1914c5b80fd30 Mon Sep 17 00:00:00 2001 From: Selena Goods Date: Mon, 10 Feb 2025 17:24:27 -0500 Subject: [PATCH 3/8] Add a link to private state page --- .../docs/plugin/framework/resources/write-only-arguments.mdx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/website/docs/plugin/framework/resources/write-only-arguments.mdx b/website/docs/plugin/framework/resources/write-only-arguments.mdx index b418e34f0..79549b9cb 100644 --- a/website/docs/plugin/framework/resources/write-only-arguments.mdx +++ b/website/docs/plugin/framework/resources/write-only-arguments.mdx @@ -109,4 +109,4 @@ Since write-only arguments have no prior values, user intent or value changes ca - Pair write-only arguments with a configuration attribute (required or optional) to “trigger” the use of the write-only argument - For example, a `password_wo` write-only argument can be paired with a configured `password_wo_version` attribute. When the `password_wo_version` is modified, the provider will send the `password_wo` value to the API. - Use a keepers attribute (which is used in the [Random Provider](https://registry.terraform.io/providers/hashicorp/random/latest/docs#resource-keepers)) that will take in arbitrary key-pair values. Whenever there is a change to the `keepers` attribute, the provider will use the write-only argument value. -- Use the resource's [private state] to store secure hashes of write-only argument values, the provider will then use the hash to determine if a write-only argument value has changed in later Terraform runs. \ No newline at end of file +- Use the resource's [private state](/terraform/plugin/framework/resources/private-state) to store secure hashes of write-only argument values, the provider will then use the hash to determine if a write-only argument value has changed in later Terraform runs. \ No newline at end of file From aeeedf92a6dbbe32f875c7d2cba769672c92fb4e Mon Sep 17 00:00:00 2001 From: Selena Goods Date: Mon, 10 Feb 2025 17:34:51 -0500 Subject: [PATCH 4/8] Add a return statement after diagnostic --- internal/fwserver/server_planresourcechange.go | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/fwserver/server_planresourcechange.go b/internal/fwserver/server_planresourcechange.go index ac8cac6bf..bd81126f5 100644 --- a/internal/fwserver/server_planresourcechange.go +++ b/internal/fwserver/server_planresourcechange.go @@ -337,6 +337,7 @@ func (s *Server) PlanResourceChange(ctx context.Context, req *PlanResourceChange "This is always an issue in the Terraform Provider and should be reported to the provider developers.\n\n"+ "Ensure all resource plan modifiers do not attempt to change resource plan data from being a null value if the request plan is a null value.", ) + return } // Set any write-only attributes in the plan to null From 5d68910f2869405fb55746699e97bcbbd972b999 Mon Sep 17 00:00:00 2001 From: Selena Goods Date: Tue, 11 Feb 2025 14:32:07 -0500 Subject: [PATCH 5/8] Add validation handling for dynamic underlying null values --- internal/fwserver/attribute_validation.go | 126 ++++++++++++------ .../fwserver/attribute_validation_test.go | 91 +++++++++++++ 2 files changed, 179 insertions(+), 38 deletions(-) diff --git a/internal/fwserver/attribute_validation.go b/internal/fwserver/attribute_validation.go index 3c2492b14..9b5c53fca 100644 --- a/internal/fwserver/attribute_validation.go +++ b/internal/fwserver/attribute_validation.go @@ -101,47 +101,97 @@ func AttributeValidate(ctx context.Context, a fwschema.Attribute, req ValidateAt return } - // Terraform CLI does not automatically perform certain configuration - // checks yet. If it eventually does, this logic should remain at least - // until Terraform CLI versions 0.12 through the release containing the - // checks are considered end-of-life. - // Reference: https://github.com/hashicorp/terraform/issues/30669 - if a.IsComputed() && !a.IsOptional() && !attributeConfig.IsNull() { - resp.Diagnostics.AddAttributeError( - req.AttributePath, - "Invalid Configuration for Read-Only Attribute", - "Cannot set value for this attribute as the provider has marked it as read-only. Remove the configuration line setting the value.\n\n"+ - "Refer to the provider documentation or contact the provider developers for additional information about configurable and read-only attributes that are supported.", - ) - } + // Dynamic values need to perform more logic to check the config value for null/unknown-ness + dynamicValuable, ok := attributeConfig.(basetypes.DynamicValuable) + if ok { + dynamicConfigVal, diags := dynamicValuable.ToDynamicValue(ctx) + resp.Diagnostics.Append(diags...) + if diags.HasError() { + return + } - // Terraform CLI does not automatically perform certain configuration - // checks yet. If it eventually does, this logic should remain at least - // until Terraform CLI versions 0.12 through the release containing the - // checks are considered end-of-life. - // Reference: https://github.com/hashicorp/terraform/issues/30669 - if a.IsRequired() && attributeConfig.IsNull() { - resp.Diagnostics.AddAttributeError( - req.AttributePath, - "Missing Configuration for Required Attribute", - fmt.Sprintf("Must set a configuration value for the %s attribute as the provider has marked it as required.\n\n", req.AttributePath.String())+ - "Refer to the provider documentation or contact the provider developers for additional information about configurable attributes that are required.", - ) - } + // Terraform CLI does not automatically perform certain configuration + // checks yet. If it eventually does, this logic should remain at least + // until Terraform CLI versions 0.12 through the release containing the + // checks are considered end-of-life. + // Reference: https://github.com/hashicorp/terraform/issues/30669 + if a.IsComputed() && !a.IsOptional() && !dynamicConfigVal.IsUnderlyingValueNull() { + resp.Diagnostics.AddAttributeError( + req.AttributePath, + "Invalid Configuration for Read-Only Attribute", + "Cannot set value for this attribute as the provider has marked it as read-only. Remove the configuration line setting the value.\n\n"+ + "Refer to the provider documentation or contact the provider developers for additional information about configurable and read-only attributes that are supported.", + ) + } - // If the client doesn't support write-only attributes (first supported in Terraform v1.11.0), then we raise an early validation error - // to avoid a confusing data consistency error when the provider attempts to return "null" for a write-only attribute in the planned/final state. - // - // Write-only attributes can only be successfully used with a supporting client, so the only option for a practitoner to utilize a write-only attribute - // is to upgrade their Terraform CLI version to v1.11.0 or later. - if !req.ClientCapabilities.WriteOnlyAttributesAllowed && a.IsWriteOnly() && !attributeConfig.IsNull() { - resp.Diagnostics.AddAttributeError( - req.AttributePath, - "WriteOnly Attribute Not Allowed", - fmt.Sprintf("The resource contains a non-null value for WriteOnly attribute %s. Write-only attributes are only supported in Terraform 1.11 and later.", req.AttributePath.String()), - ) - } + // Terraform CLI does not automatically perform certain configuration + // checks yet. If it eventually does, this logic should remain at least + // until Terraform CLI versions 0.12 through the release containing the + // checks are considered end-of-life. + // Reference: https://github.com/hashicorp/terraform/issues/30669 + if a.IsRequired() && dynamicConfigVal.IsUnderlyingValueNull() { + resp.Diagnostics.AddAttributeError( + req.AttributePath, + "Missing Configuration for Required Attribute", + fmt.Sprintf("Must set a configuration value for the %s attribute as the provider has marked it as required.\n\n", req.AttributePath.String())+ + "Refer to the provider documentation or contact the provider developers for additional information about configurable attributes that are required.", + ) + } + + // If the client doesn't support write-only attributes (first supported in Terraform v1.11.0), then we raise an early validation error + // to avoid a confusing data consistency error when the provider attempts to return "null" for a write-only attribute in the planned/final state. + // + // Write-only attributes can only be successfully used with a supporting client, so the only option for a practitoner to utilize a write-only attribute + // is to upgrade their Terraform CLI version to v1.11.0 or later. + if !req.ClientCapabilities.WriteOnlyAttributesAllowed && a.IsWriteOnly() && !dynamicConfigVal.IsUnderlyingValueNull() { + resp.Diagnostics.AddAttributeError( + req.AttributePath, + "WriteOnly Attribute Not Allowed", + fmt.Sprintf("The resource contains a non-null value for WriteOnly attribute %s. Write-only attributes are only supported in Terraform 1.11 and later.", req.AttributePath.String()), + ) + } + } else { + // Terraform CLI does not automatically perform certain configuration + // checks yet. If it eventually does, this logic should remain at least + // until Terraform CLI versions 0.12 through the release containing the + // checks are considered end-of-life. + // Reference: https://github.com/hashicorp/terraform/issues/30669 + if a.IsComputed() && !a.IsOptional() && !attributeConfig.IsNull() { + resp.Diagnostics.AddAttributeError( + req.AttributePath, + "Invalid Configuration for Read-Only Attribute", + "Cannot set value for this attribute as the provider has marked it as read-only. Remove the configuration line setting the value.\n\n"+ + "Refer to the provider documentation or contact the provider developers for additional information about configurable and read-only attributes that are supported.", + ) + } + + // Terraform CLI does not automatically perform certain configuration + // checks yet. If it eventually does, this logic should remain at least + // until Terraform CLI versions 0.12 through the release containing the + // checks are considered end-of-life. + // Reference: https://github.com/hashicorp/terraform/issues/30669 + if a.IsRequired() && attributeConfig.IsNull() { + resp.Diagnostics.AddAttributeError( + req.AttributePath, + "Missing Configuration for Required Attribute", + fmt.Sprintf("Must set a configuration value for the %s attribute as the provider has marked it as required.\n\n", req.AttributePath.String())+ + "Refer to the provider documentation or contact the provider developers for additional information about configurable attributes that are required.", + ) + } + // If the client doesn't support write-only attributes (first supported in Terraform v1.11.0), then we raise an early validation error + // to avoid a confusing data consistency error when the provider attempts to return "null" for a write-only attribute in the planned/final state. + // + // Write-only attributes can only be successfully used with a supporting client, so the only option for a practitoner to utilize a write-only attribute + // is to upgrade their Terraform CLI version to v1.11.0 or later. + if !req.ClientCapabilities.WriteOnlyAttributesAllowed && a.IsWriteOnly() && !attributeConfig.IsNull() { + resp.Diagnostics.AddAttributeError( + req.AttributePath, + "WriteOnly Attribute Not Allowed", + fmt.Sprintf("The resource contains a non-null value for WriteOnly attribute %s. Write-only attributes are only supported in Terraform 1.11 and later.", req.AttributePath.String()), + ) + } + } req.AttributeConfig = attributeConfig switch attributeWithValidators := a.(type) { diff --git a/internal/fwserver/attribute_validation_test.go b/internal/fwserver/attribute_validation_test.go index 6f0fbc5ab..50fe65416 100644 --- a/internal/fwserver/attribute_validation_test.go +++ b/internal/fwserver/attribute_validation_test.go @@ -181,6 +181,29 @@ func TestAttributeValidate(t *testing.T) { }, }, }, + "config-computed-dynamic-underlying-null-value": { + req: ValidateAttributeRequest{ + AttributePath: path.Root("test"), + Config: tfsdk.Config{ + Raw: tftypes.NewValue(tftypes.Object{ + AttributeTypes: map[string]tftypes.Type{ + "test": tftypes.DynamicPseudoType, + }, + }, map[string]tftypes.Value{ + "test": tftypes.NewValue(tftypes.String, nil), + }), + Schema: testschema.Schema{ + Attributes: map[string]fwschema.Attribute{ + "test": testschema.Attribute{ + Computed: true, + Type: types.DynamicType, + }, + }, + }, + }, + }, + resp: ValidateAttributeResponse{}, + }, "config-optional-computed-null": { req: ValidateAttributeRequest{ AttributePath: path.Root("test"), @@ -331,6 +354,38 @@ func TestAttributeValidate(t *testing.T) { }, resp: ValidateAttributeResponse{}, }, + "config-required-dynamic-underlying-null-value": { + req: ValidateAttributeRequest{ + AttributePath: path.Root("test"), + Config: tfsdk.Config{ + Raw: tftypes.NewValue(tftypes.Object{ + AttributeTypes: map[string]tftypes.Type{ + "test": tftypes.DynamicPseudoType, + }, + }, map[string]tftypes.Value{ + "test": tftypes.NewValue(tftypes.String, nil), + }), + Schema: testschema.Schema{ + Attributes: map[string]fwschema.Attribute{ + "test": testschema.Attribute{ + Required: true, + Type: types.DynamicType, + }, + }, + }, + }, + }, + resp: ValidateAttributeResponse{ + Diagnostics: diag.Diagnostics{ + diag.NewAttributeErrorDiagnostic( + path.Root("test"), + "Missing Configuration for Required Attribute", + "Must set a configuration value for the test attribute as the provider has marked it as required.\n\n"+ + "Refer to the provider documentation or contact the provider developers for additional information about configurable attributes that are required.", + ), + }, + }, + }, "no-validation": { req: ValidateAttributeRequest{ AttributePath: path.Root("test"), @@ -1763,6 +1818,42 @@ func TestAttributeValidate(t *testing.T) { }, }, }, + "write-only-attr-with-required-dynamic-underlying-null-value": { + req: ValidateAttributeRequest{ + ClientCapabilities: validator.ValidateSchemaClientCapabilities{ + WriteOnlyAttributesAllowed: true, + }, + AttributePath: path.Root("test"), + Config: tfsdk.Config{ + Raw: tftypes.NewValue(tftypes.Object{ + AttributeTypes: map[string]tftypes.Type{ + "test": tftypes.DynamicPseudoType, + }, + }, map[string]tftypes.Value{ + "test": tftypes.NewValue(tftypes.String, nil), + }), + Schema: testschema.Schema{ + Attributes: map[string]fwschema.Attribute{ + "test": testschema.Attribute{ + Type: types.DynamicType, + WriteOnly: true, + Required: true, + }, + }, + }, + }, + }, + resp: ValidateAttributeResponse{ + Diagnostics: diag.Diagnostics{ + diag.NewAttributeErrorDiagnostic( + path.Root("test"), + "Missing Configuration for Required Attribute", + "Must set a configuration value for the test attribute as the provider has marked it as required.\n\n"+ + "Refer to the provider documentation or contact the provider developers for additional information about configurable attributes that are required.", + ), + }, + }, + }, "write-only-attr-with-optional": { req: ValidateAttributeRequest{ ClientCapabilities: validator.ValidateSchemaClientCapabilities{ From 624dbc4e0eeef17ca99dc8ec50a77a8bd63a58ba Mon Sep 17 00:00:00 2001 From: Selena Goods Date: Tue, 11 Feb 2025 14:58:57 -0500 Subject: [PATCH 6/8] Remove null check --- internal/fwserver/server_upgraderesourcestate.go | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/internal/fwserver/server_upgraderesourcestate.go b/internal/fwserver/server_upgraderesourcestate.go index 536c3dfd0..e64a1339c 100644 --- a/internal/fwserver/server_upgraderesourcestate.go +++ b/internal/fwserver/server_upgraderesourcestate.go @@ -264,16 +264,5 @@ func (s *Server) UpgradeResourceState(ctx context.Context, req *UpgradeResourceS } upgradeResourceStateResponse.State.Raw = modifiedState - // If the write-only nullification results in a null state, then this is a provider error - if upgradeResourceStateResponse.State.Raw.Type() == nil || upgradeResourceStateResponse.State.Raw.IsNull() { - resp.Diagnostics.AddError( - "Missing Upgraded Resource State", - fmt.Sprintf("After attempting a resource state upgrade to version %d, the provider did not return any state data. ", req.Version)+ - "Preventing the unexpected loss of resource state data. "+ - "This is always an issue with the Terraform Provider and should be reported to the provider developer.", - ) - return - } - resp.UpgradedState = &upgradeResourceStateResponse.State } From d514911b3a8e14f0c01d7f5f04f80c40997f2138 Mon Sep 17 00:00:00 2001 From: Selena Goods Date: Thu, 13 Feb 2025 12:38:52 -0500 Subject: [PATCH 7/8] Update logic to check null/unknowness in both the container and underlying value --- internal/fwserver/attribute_validation.go | 159 ++++++++-------------- 1 file changed, 53 insertions(+), 106 deletions(-) diff --git a/internal/fwserver/attribute_validation.go b/internal/fwserver/attribute_validation.go index 9b5c53fca..e040551ec 100644 --- a/internal/fwserver/attribute_validation.go +++ b/internal/fwserver/attribute_validation.go @@ -101,96 +101,63 @@ func AttributeValidate(ctx context.Context, a fwschema.Attribute, req ValidateAt return } - // Dynamic values need to perform more logic to check the config value for null/unknown-ness - dynamicValuable, ok := attributeConfig.(basetypes.DynamicValuable) - if ok { + configHasNullValue := attributeConfig.IsNull() + configHasUnknownValue := attributeConfig.IsUnknown() + // If the value is dynamic, we still need to check if the underlying value is null or unknown + if dynamicValuable, isDynamic := attributeConfig.(basetypes.DynamicValuable); !configHasNullValue && !configHasUnknownValue && isDynamic { dynamicConfigVal, diags := dynamicValuable.ToDynamicValue(ctx) resp.Diagnostics.Append(diags...) if diags.HasError() { return } - - // Terraform CLI does not automatically perform certain configuration - // checks yet. If it eventually does, this logic should remain at least - // until Terraform CLI versions 0.12 through the release containing the - // checks are considered end-of-life. - // Reference: https://github.com/hashicorp/terraform/issues/30669 - if a.IsComputed() && !a.IsOptional() && !dynamicConfigVal.IsUnderlyingValueNull() { - resp.Diagnostics.AddAttributeError( - req.AttributePath, - "Invalid Configuration for Read-Only Attribute", - "Cannot set value for this attribute as the provider has marked it as read-only. Remove the configuration line setting the value.\n\n"+ - "Refer to the provider documentation or contact the provider developers for additional information about configurable and read-only attributes that are supported.", - ) + if dynamicConfigVal.IsUnderlyingValueNull() { + configHasNullValue = true } - // Terraform CLI does not automatically perform certain configuration - // checks yet. If it eventually does, this logic should remain at least - // until Terraform CLI versions 0.12 through the release containing the - // checks are considered end-of-life. - // Reference: https://github.com/hashicorp/terraform/issues/30669 - if a.IsRequired() && dynamicConfigVal.IsUnderlyingValueNull() { - resp.Diagnostics.AddAttributeError( - req.AttributePath, - "Missing Configuration for Required Attribute", - fmt.Sprintf("Must set a configuration value for the %s attribute as the provider has marked it as required.\n\n", req.AttributePath.String())+ - "Refer to the provider documentation or contact the provider developers for additional information about configurable attributes that are required.", - ) + if dynamicConfigVal.IsUnderlyingValueUnknown() { + configHasUnknownValue = true } + } - // If the client doesn't support write-only attributes (first supported in Terraform v1.11.0), then we raise an early validation error - // to avoid a confusing data consistency error when the provider attempts to return "null" for a write-only attribute in the planned/final state. - // - // Write-only attributes can only be successfully used with a supporting client, so the only option for a practitoner to utilize a write-only attribute - // is to upgrade their Terraform CLI version to v1.11.0 or later. - if !req.ClientCapabilities.WriteOnlyAttributesAllowed && a.IsWriteOnly() && !dynamicConfigVal.IsUnderlyingValueNull() { - resp.Diagnostics.AddAttributeError( - req.AttributePath, - "WriteOnly Attribute Not Allowed", - fmt.Sprintf("The resource contains a non-null value for WriteOnly attribute %s. Write-only attributes are only supported in Terraform 1.11 and later.", req.AttributePath.String()), - ) - } - } else { - // Terraform CLI does not automatically perform certain configuration - // checks yet. If it eventually does, this logic should remain at least - // until Terraform CLI versions 0.12 through the release containing the - // checks are considered end-of-life. - // Reference: https://github.com/hashicorp/terraform/issues/30669 - if a.IsComputed() && !a.IsOptional() && !attributeConfig.IsNull() { - resp.Diagnostics.AddAttributeError( - req.AttributePath, - "Invalid Configuration for Read-Only Attribute", - "Cannot set value for this attribute as the provider has marked it as read-only. Remove the configuration line setting the value.\n\n"+ - "Refer to the provider documentation or contact the provider developers for additional information about configurable and read-only attributes that are supported.", - ) - } + // Terraform CLI does not automatically perform certain configuration + // checks yet. If it eventually does, this logic should remain at least + // until Terraform CLI versions 0.12 through the release containing the + // checks are considered end-of-life. + // Reference: https://github.com/hashicorp/terraform/issues/30669 + if a.IsComputed() && !a.IsOptional() && !configHasNullValue { + resp.Diagnostics.AddAttributeError( + req.AttributePath, + "Invalid Configuration for Read-Only Attribute", + "Cannot set value for this attribute as the provider has marked it as read-only. Remove the configuration line setting the value.\n\n"+ + "Refer to the provider documentation or contact the provider developers for additional information about configurable and read-only attributes that are supported.", + ) + } - // Terraform CLI does not automatically perform certain configuration - // checks yet. If it eventually does, this logic should remain at least - // until Terraform CLI versions 0.12 through the release containing the - // checks are considered end-of-life. - // Reference: https://github.com/hashicorp/terraform/issues/30669 - if a.IsRequired() && attributeConfig.IsNull() { - resp.Diagnostics.AddAttributeError( - req.AttributePath, - "Missing Configuration for Required Attribute", - fmt.Sprintf("Must set a configuration value for the %s attribute as the provider has marked it as required.\n\n", req.AttributePath.String())+ - "Refer to the provider documentation or contact the provider developers for additional information about configurable attributes that are required.", - ) - } + // Terraform CLI does not automatically perform certain configuration + // checks yet. If it eventually does, this logic should remain at least + // until Terraform CLI versions 0.12 through the release containing the + // checks are considered end-of-life. + // Reference: https://github.com/hashicorp/terraform/issues/30669 + if a.IsRequired() && configHasNullValue { + resp.Diagnostics.AddAttributeError( + req.AttributePath, + "Missing Configuration for Required Attribute", + fmt.Sprintf("Must set a configuration value for the %s attribute as the provider has marked it as required.\n\n", req.AttributePath.String())+ + "Refer to the provider documentation or contact the provider developers for additional information about configurable attributes that are required.", + ) + } - // If the client doesn't support write-only attributes (first supported in Terraform v1.11.0), then we raise an early validation error - // to avoid a confusing data consistency error when the provider attempts to return "null" for a write-only attribute in the planned/final state. - // - // Write-only attributes can only be successfully used with a supporting client, so the only option for a practitoner to utilize a write-only attribute - // is to upgrade their Terraform CLI version to v1.11.0 or later. - if !req.ClientCapabilities.WriteOnlyAttributesAllowed && a.IsWriteOnly() && !attributeConfig.IsNull() { - resp.Diagnostics.AddAttributeError( - req.AttributePath, - "WriteOnly Attribute Not Allowed", - fmt.Sprintf("The resource contains a non-null value for WriteOnly attribute %s. Write-only attributes are only supported in Terraform 1.11 and later.", req.AttributePath.String()), - ) - } + // If the client doesn't support write-only attributes (first supported in Terraform v1.11.0), then we raise an early validation error + // to avoid a confusing data consistency error when the provider attempts to return "null" for a write-only attribute in the planned/final state. + // + // Write-only attributes can only be successfully used with a supporting client, so the only option for a practitoner to utilize a write-only attribute + // is to upgrade their Terraform CLI version to v1.11.0 or later. + if !req.ClientCapabilities.WriteOnlyAttributesAllowed && a.IsWriteOnly() && !configHasNullValue { + resp.Diagnostics.AddAttributeError( + req.AttributePath, + "WriteOnly Attribute Not Allowed", + fmt.Sprintf("The resource contains a non-null value for WriteOnly attribute %s. Write-only attributes are only supported in Terraform 1.11 and later.", req.AttributePath.String()), + ) } req.AttributeConfig = attributeConfig @@ -224,33 +191,13 @@ func AttributeValidate(ctx context.Context, a fwschema.Attribute, req ValidateAt AttributeValidateNestedAttributes(ctx, a, req, resp) // Show deprecation warnings only for known values. - if a.GetDeprecationMessage() != "" && !attributeConfig.IsNull() && !attributeConfig.IsUnknown() { - // Dynamic values need to perform more logic to check the config value for null/unknown-ness - dynamicValuable, ok := attributeConfig.(basetypes.DynamicValuable) - if !ok { - resp.Diagnostics.AddAttributeWarning( - req.AttributePath, - "Attribute Deprecated", - a.GetDeprecationMessage(), - ) - return - } - - dynamicConfigVal, diags := dynamicValuable.ToDynamicValue(ctx) - resp.Diagnostics.Append(diags...) - if diags.HasError() { - return - } - - // For dynamic values, it's possible to be known when only the type is known. - // The underlying value can still be null or unknown, so check for that here - if !dynamicConfigVal.IsUnderlyingValueNull() && !dynamicConfigVal.IsUnderlyingValueUnknown() { - resp.Diagnostics.AddAttributeWarning( - req.AttributePath, - "Attribute Deprecated", - a.GetDeprecationMessage(), - ) - } + if a.GetDeprecationMessage() != "" && !configHasNullValue && !configHasUnknownValue { + resp.Diagnostics.AddAttributeWarning( + req.AttributePath, + "Attribute Deprecated", + a.GetDeprecationMessage(), + ) + return } } From e5d294d5a291dc058f33ade50af0cce08752ebd6 Mon Sep 17 00:00:00 2001 From: Selena Goods Date: Thu, 13 Feb 2025 16:47:13 -0500 Subject: [PATCH 8/8] Add changelog entry --- .changes/unreleased/BUG FIXES-20250213-164700.yaml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changes/unreleased/BUG FIXES-20250213-164700.yaml diff --git a/.changes/unreleased/BUG FIXES-20250213-164700.yaml b/.changes/unreleased/BUG FIXES-20250213-164700.yaml new file mode 100644 index 000000000..3c49a6f81 --- /dev/null +++ b/.changes/unreleased/BUG FIXES-20250213-164700.yaml @@ -0,0 +1,5 @@ +kind: BUG FIXES +body: 'internal/fwserver: Fixed bug where dynamic attributes would not prompt invalid configuration error messages' +time: 2025-02-13T16:47:00.4821-05:00 +custom: + Issue: "1090"