Skip validation of a values missing in config

[gzigzigzeo]

Problem

I have the following schema:

"spec": {
	Attributes: tfsdk.SingleNestedAttributes(map[string]tfsdk.Attribute{
		"options": {
			Attributes: tfsdk.SingleNestedAttributes(map[string]tfsdk.Attribute{
				"lock": {
					Description: "Lock specifies the locking mode (strict|best_effort) to be applied with  the role.",
					Optional:    true,
					Type:        types.StringType,
				},
				"max_sessions": {
					Description: "MaxSessions defines the maximum number of  concurrent sessions per connection.",
					Optional:    true,
					Computed:    true,
					Type:        types.Int64Type,
				},
			}),
			Computed:    true,
			Description: "Options is for OpenSSH options like agent forwarding.",
			Optional:    true,
			PlanModifiers: []tfsdk.AttributePlanModifier{tfsdk.UseStateForUnknown()},
		},

And the following config:

spec {}

"spec" is the base object, "options" is the child computed object, and "max_sessions" is the bottommost Int64 value, which, in turn, is also computed.

options might either be missing in my configuration at all, set partially or set completely, including max_sessions attribute.

When options section is missing in the configuration completely, Terraform crashes:

github.com/hashicorp/terraform-plugin-framework/types.int64Validate({0x103337498, 0x1400030b600}, {{0x0, 0x0}, {0x0, 0x0}}, 0x14000683ce0)
	/Users/gzigzigzeo/go/src/github.com/gravitational/teleport-plugins/vendor/github.com/hashicorp/terraform-plugin-framework/types/int64.go:16 +0x44
github.com/hashicorp/terraform-plugin-framework/types.primitive.Validate(0x3, {0x103337498, 0x1400030b600}, {{0x0, 0x0}, {0x0, 0x0}}, 0x14000683ce0)
	/Users/gzigzigzeo/go/src/github.com/gravitational/teleport-plugins/vendor/github.com/hashicorp/terraform-plugin-framework/types/primitive.go:121 +0xd8
github.com/hashicorp/terraform-plugin-framework/tfsdk.Config.getAttributeValue({{{0x103341820, 0x140004f8690}, {0x1031eb6a0, 0x140004f41e0}}, {0x14000558bd0, 0x0, 0x0, {0x0, 0x0}, {0x0, ...}, ...}}, ...)

Solution

That happens because Terraform Framework tries to validate the type of spec.options.max_sessions config value, which is missing in the config, hence, can't have a type to validate. It must be skipped.

[hashicorp-cla]

All committers have signed the CLA.

[ewbankkit]

Similar crash reported in hashicorp/terraform-provider-awscc#382.
In that case the offending code is

terraform-plugin-framework/tfsdk/state.go

Lines 80 to 100 in 5f18d80

	// Ignoring ErrInvalidStep will allow this method to return a null value of the type.
	if err != nil && !errors.Is(err, tftypes.ErrInvalidStep) {
	diags.AddAttributeError(
	path,
	"State Read Error",
	"An unexpected error was encountered trying to read an attribute from the state. This is always an error in the provider. Please report the following to the provider developer:\n\n"+err.Error(),
	)
	return nil, diags
	}
	// TODO: If ErrInvalidStep, check parent paths for unknown value.
	// If found, convert this value to an unknown value.
	// Reference: https://github.com/hashicorp/terraform-plugin-framework/issues/186
	if attrTypeWithValidate, ok := attrType.(attr.TypeWithValidate); ok {
	diags.Append(attrTypeWithValidate.Validate(ctx, tfValue, path)...)
	if diags.HasError() {
	return nil, diags
	}
	}

[gzigzigzeo]

@ewbankkit Thanks for pointing this out, provided failed test and fixed it as well.

[bflad]

Hi @gzigzigzeo 👋 Thanks for submitting this and sorry you ran into trouble here.

In general, the preference for this framework should be performing attribute validation in all cases. This allows provider developers to check against null values explicitly, rather than the framework making any assumptions about what might be the correct behavior. To that end, this likely means that some of the built-in framework types need to handle this class of nil tftypes.Value issue to prevent panics.

My recommendation here would be to revert the general validation logic and instead perform a nil check before the problematic call into in.Type(), e.g.

if in == nil {
  return diags
}

if !in.Type().Equal(tftypes.Number) { // ...

Please reach out if you have any questions (although it may take a week for a next response).

[gzigzigzeo]

Hey, @bflad! Thank you for the clarification! In fact, reverting of this logic was my initial thought, and performing it in float64.go and int64.go fixed the issue. At that moment, I thought that it was a bit implicit and does not guarantee a custom user validations from failing. I now see why your approach is better and framework should not guarantee anything here. I am going to fix my implementation accordingly, add a counterpart tests for other built-in types and push it to this PR. Thanks again.

[gzigzigzeo]

@bflad Done!

[bflad]

Thanks for the updates, @gzigzigzeo. Since I didn't want to break your main branch by updating it, this will get pulled in as a separate commit. I will also fix the Set validation (#261) as part of pulling this in.

[bflad]

This test looks identical to the one above, so will remove on merge.

[bflad]

Similar to Config, this logic should not be changed, will fix on merge.

[bflad]

Will ensure there is a covering test in plan as well.

[bflad]

Merged as c584343

[github-actions]

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.