Skip to content

Improve dynamic attribute internal validation handling #1090

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Feb 13, 2025
Merged

Improve dynamic attribute internal validation handling #1090

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
cd9f942
Remove unused `RequiredWriteOnlyNilsAttributePaths()` function
SBGoods Feb 10, 2025
b2d165b
Remove dynamic type conditional from write_only_nullification.go
SBGoods Feb 10, 2025
f30b47b
Add a link to private state page
SBGoods Feb 10, 2025
aeeedf9
Add a return statement after diagnostic
SBGoods Feb 10, 2025
5d68910
Add validation handling for dynamic underlying null values
SBGoods Feb 11, 2025
624dbc4
Remove null check
SBGoods Feb 11, 2025
d514911
Update logic to check null/unknowness in both the container and under…
SBGoods Feb 13, 2025
f83ccd2
Merge branch 'main' into SBGoods/write-only-dynamic-validation
SBGoods Feb 13, 2025
e5d294d
Add changelog entry
SBGoods Feb 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
126 changes: 88 additions & 38 deletions internal/fwserver/attribute_validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
91 changes: 91 additions & 0 deletions internal/fwserver/attribute_validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
Expand Down Expand Up @@ -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"),
Expand Down Expand Up @@ -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{
Expand Down
63 changes: 1 addition & 62 deletions internal/fwserver/server_planresourcechange.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -338,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
Expand Down Expand Up @@ -513,67 +513,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{
Expand Down
Loading
Loading