Add type-based provider-defined function parameter validation

[bendbennett]

Closes: #589
Closes: #893

Background

Provider-defined functions can accept parameters, or arguments as input. There is an opportunity to provide validation of such parameters in an analogous manner to the validation of values supplied in configuration for attributes, by implementing parameter-based and type-based validation for provider-defined function parameters.

This PR is concerned with the addition of type-based validation, which enables provider developers using custom value types to implement parameter validation.

Validation Package

This PR adds type-based provider-defined function parameter validation through the introduction of a validation package with a new ValidateableParameter interface.

type ValidateableParameter interface {
	ValidateParameter(context.Context, ValidateParameterRequest, *ValidateParameterResponse)
}

type ValidateParameterRequest struct {
	Position int64
}

type ValidateParameterResponse struct {
	Error *function.FuncError
}

Deprecation of xattr.TypeWithValidate

This PR also introduces a new ValidateableAttribute interface, and deprecates the xattr.TypeWithValidate interface. Provider developers who are using custom value types that will be used in the context of both attribute validation and parameter validation would need to implement both interfaces on the custom value type, but could share the validation logic to reduce duplication.

type ValidateableAttribute interface {
	ValidateAttribute(context.Context, ValidateAttributeRequest, *ValidateAttributeResponse)
}

type ValidateAttributeRequest struct {
	Path path.Path
}

type ValidateAttributeResponse struct {
	Diagnostics diag.Diagnostics
}

[austinvalle]

Looks great! I left some general thoughts around package structure and some of the questions you asked.

[austinvalle]

A consequence of the reflect package's interface, but I wonder if there will be general confusion around "which validation logic" is being called and when.

For example, in the case of a custom type only being used on a function parameter, it might not implement this ValidateableAttribute interface. When the original ArgumentData is being built from tfplugin-go, it would call the ValidateableParameter interface, but when calling (ArgumentData).Get or (ResultData).Set it would be using the ValidateableAttribute interface (via the reflection) and therefore not get called.

I wonder how big of a refactor it would be to completely remove validation from the reflect package and make it the responsibility of the caller to validate the reflection value is valid. So (ArgumentData).Get, (ResultData).Set, (Data).Get, (Data).Set, etc. would need to perform this validation.

Or perhaps we should be attempting to add the parameter validation into the reflect logic, although we'll likely run into import cycles there because of FuncError, so maybe if we don't do anything here it's just a documentation problem?

[bendbennett]

For example, in the case of a custom type only being used on a function parameter, it might not implement this ValidateableAttribute interface. When the original ArgumentData is being built from tfplugin-go, it would call the ValidateableParameter interface, but when calling (ArgumentData).Get or (ResultData).Set it would be using the ValidateableAttribute interface (via the reflection) and therefore not get called.

If a custom type that is only being used as a function parameter implements ValidateableParameter but does not implement ValidateableAttribute, the call to ValidateParameter() will occur during the CallFunction RPC when fromproto5/6.ArgumentsData() is called. The call to fromproto5/6.ArgumentsData() occurs, prior to calling (ArgumentData).Get() or (ResultData).Set(), the latter of which are typically called within the Run() method of the provider-defined function. Does this answer your concern?

I wonder how big of a refactor it would be to completely remove validation from the reflect package and make it the responsibility of the caller to validate the reflection value is valid. So (ArgumentData).Get, (ResultData).Set, (Data).Get, (Data).Set, etc. would need to perform this validation.

I chatted with Brian about this out-of-band, as I was also wondering about the embedded validation within reflection. I think this is worthy of further investigation but I believe would be best suited as a follow-up issue/discussion.

Or perhaps we should be attempting to add the parameter validation into the reflect logic, although we'll likely run into import cycles there because of FuncError, so maybe if we don't do anything here it's just a documentation problem?

In terms of the documentation, is the concern that it may not be clear under which circumstances to implement ValidateableParameter vs ValidateableAttribute? There's some updates to the docs in this PR to try and make this clear, but let me know if you think this should be expanded.

[austinvalle]

Definitely agree about any refactoring removing validation from reflection should be a separate issue 👍🏻

If a custom type that is only being used as a function parameter implements ValidateableParameter but does not implement ValidateableAttribute, the call to ValidateParameter() will occur during the CallFunction RPC when fromproto5/6.ArgumentsData() is called. The call to fromproto5/6.ArgumentsData() occurs, prior to calling (ArgumentData).Get() or (ResultData).Set(), the latter of which are typically called within the Run() method of the provider-defined function. Does this answer your concern?

That answers the (ArgumentData).Get() portion, but is there an equivalent for the (ResultData).Set() direction?

For example sake, let's say that iptypes.IPv4AddressType only implements ValidateableParameter

func (f ExampleFunction) Definition(ctx context.Context, req function.DefinitionRequest, resp *function.DefinitionResponse) {
    resp.Definition = function.Definition{
        Parameters: []function.Parameter{
            function.StringParameter{
                Name:       "ip_address",
                CustomType: iptypes.IPv4AddressType{},
            },
        },
        Return: function.StringReturn{
            CustomType: iptypes.IPv4AddressType{},
        },
    }
}

func (f ExampleFunction) Run(ctx context.Context, req function.RunRequest, resp *function.RunResponse) {
    var arg string

    resp.Error = req.Arguments.Get(ctx, &arg)

    resp.Error = function.ConcatFuncErrors(resp.Error, resp.Result.Set(ctx, "not an IP address"))
}

I think based on what you described, the req.Arguments.Get would be protected implicitly because of the fromproto logic already calling ValidateableParameter, but the resp.Result.Set wouldn't call validation because the custom type doesn't implement ValidateableAttribute.

If that's the case, maybe a Go doc comment could reflect that ValidateableAttribute is used during reflection? This does feel like enough of an "edge" that it shouldn't be a regular problem (I would expect provider developers using custom types to not use reflection for setting?)

[bendbennett]

This is an interesting case! If we want to explicitly validate values that are being passed to Result.Set it seems that maybe neither ValidateableParameter nor ValidateableAttribute are appropriate interfaces, as the value is neither a parameter nor an attribute.

One option might be to consider a Validateable interface that operates solely on the value. Perhaps something along the following lines:

type Validateable interface {
	Validate(context.Context, ValidateRequest, *ValidateResponse)
}

type ValidateRequest struct{}

type ValidateResponse struct {
	Error *FuncError
}

A custom type that was to be used for validating a provider-defined function parameter, and the result from the function could be defined as follows:

func (v CustomStringValue) ValidateParameter(ctx context.Context, req function.ValidateParameterRequest, resp *function.ValidateParameterResponse) {
	if v.IsNull() || v.IsUnknown() {
		return
	}

	if !v.isValid(v.ValueString()) {
		resp.Error = function.NewArgumentFuncError(
			req.Position,
			fmt.Sprintf("Invalid String Value: value %q length does not equal %d", v.ValueString(), 10))
	}
}

func (v CustomStringValue) Validate(ctx context.Context, req function.ValidateRequest, resp *function.ValidateResponse) {
	if v.IsNull() || v.IsUnknown() {
		return
	}

	if !v.isValid(v.ValueString()) {
		resp.Error = function.NewFuncError(
			fmt.Sprintf("Invalid String Value: value %q length does not equal %d", v.ValueString(), 10))
	}
}

func (v CustomStringValue) isValid(in string) bool {
	return len(in) == 10
}

The function definition could then be specified as:

func (f ExampleFunction) Definition(ctx context.Context, req function.DefinitionRequest, resp *function.DefinitionResponse) {
	resp.Definition = function.Definition{
		Parameters: []function.Parameter{
			function.StringParameter{
				CustomType: CustomStringType{},
				Name:       "arg0",
			},
		},
		Return: function.StringReturn{
			CustomType: CustomStringType{},
		},
	}
}

The validation of the result value could then be handled by modifying the ResultData.Set method:

func (d *ResultData) Set(ctx context.Context, value any) *FuncError {
	reflectValue, reflectDiags := fwreflect.FromValue(ctx, d.value.Type(ctx), value, path.Empty())

	funcErr := FuncErrorFromDiags(ctx, reflectDiags)

	if funcErr != nil {
		return funcErr
	}

	if v, ok := reflectValue.(Validateable); ok {
		resp := ValidateResponse{}

		v.Validate(ctx, ValidateRequest{}, &resp)

		if resp.Error != nil {
			return resp.Error
		}
	}

	d.value = reflectValue

	return nil
}

Thoughts?

[austinvalle]

In the case of something like Validateable, I'd assume it would end up just returning a plain error 🤔. I'm not 100% sure adding a new interface just for validation in reflection is worth it, but I think the general problem has been shown here.

I.e. the ValidateableAttribute interface is being used for more than just attributes. Perhaps something that we can't avoid? Something we don't necessarily need to avoid? 🤷🏻

Would be interested to hear the rest of the teams thoughts on this, although I don't think it's really a showstopper 

[SBGoods]

This is a tough problem, I'm reluctant to expose a new interface just for reflection which is more of an implementation detail than it is a general Terraform provider development concept like parameter, attribute, or value. I think documenting that the ValidatebleAttribute is being used in reflection logic in the Go doc is good enough for now and we can tackle this issue later own when we look into decoupling validation from the reflection logic.

[bendbennett]

Thanks for the feedback. Ok, so I've amended the Go doc for ValidateableAttribute as follows:

// ValidateableAttribute defines an interface for validating an attribute value.
// The ValidateAttribute method is called implicitly by the framework when value
// types from Terraform are converted into framework types.
type ValidateableAttribute interface {

[austinvalle]

LGTM 👍🏻

[austinvalle]

For the types still implementing the legacy validation (float64, int64, list, set, map), should we create a follow-up issue to add this validation implementations for both ValidateableParameter and ValidateableAttribute?

[bendbennett]

I was wondering about this. For float64 and int64 I was thinking that this may not be necessary as the calls to <Float64|Int64>Type.ValueFromTerraform ensure that the tftypes.Value supplied can be represented as 64-bit floating point, and integer values, respectively. The constructors on the corresponding value types only accept 64-bit floating point, and integer values too (e.g., NewFloat64Value). So it doesn't seem that there is a way to generate a float64 or int64 value type that would have a value that couldn't be represented as a 64-bit floating point, or integer value, respectively.

I'll create a follow-up issue for list, map, set. Let me know if you think float64 and int64 should be included and we can discuss this further.

[austinvalle]

Ah! That makes sense to me 👍🏻

[austinvalle]

Great work Ben! 🚀 🎸

[austinvalle]

Definitely agree about any refactoring removing validation from reflection should be a separate issue 👍🏻

If a custom type that is only being used as a function parameter implements ValidateableParameter but does not implement ValidateableAttribute, the call to ValidateParameter() will occur during the CallFunction RPC when fromproto5/6.ArgumentsData() is called. The call to fromproto5/6.ArgumentsData() occurs, prior to calling (ArgumentData).Get() or (ResultData).Set(), the latter of which are typically called within the Run() method of the provider-defined function. Does this answer your concern?

That answers the (ArgumentData).Get() portion, but is there an equivalent for the (ResultData).Set() direction?

For example sake, let's say that iptypes.IPv4AddressType only implements ValidateableParameter

func (f ExampleFunction) Definition(ctx context.Context, req function.DefinitionRequest, resp *function.DefinitionResponse) {
    resp.Definition = function.Definition{
        Parameters: []function.Parameter{
            function.StringParameter{
                Name:       "ip_address",
                CustomType: iptypes.IPv4AddressType{},
            },
        },
        Return: function.StringReturn{
            CustomType: iptypes.IPv4AddressType{},
        },
    }
}

func (f ExampleFunction) Run(ctx context.Context, req function.RunRequest, resp *function.RunResponse) {
    var arg string

    resp.Error = req.Arguments.Get(ctx, &arg)

    resp.Error = function.ConcatFuncErrors(resp.Error, resp.Result.Set(ctx, "not an IP address"))
}

I think based on what you described, the req.Arguments.Get would be protected implicitly because of the fromproto logic already calling ValidateableParameter, but the resp.Result.Set wouldn't call validation because the custom type doesn't implement ValidateableAttribute.

If that's the case, maybe a Go doc comment could reflect that ValidateableAttribute is used during reflection? This does feel like enough of an "edge" that it shouldn't be a regular problem (I would expect provider developers using custom types to not use reflection for setting?)

[austinvalle]

Ah! That makes sense to me 👍🏻

[SBGoods]

Great job, Ben!

[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.