function: Consider preventing parameter name conflicts in schema validation

[austinvalle]

Module version

v1.5.0

Use-cases

When building a provider-defined function with plugin framework, it is currently possible to create a function schema that is invalid, where two parameters (variadic or not) have the same name. When this occurs, you'll receive an error message from validation in Terraform like:

│ Error: Failed to load plugin schemas
│ 
│ Error while loading schemas for plugin components: Failed to obtain provider schema: Could not load the schema for provider registry.terraform.io/austinvalle/sandbox: provider
│ registry.terraform.io/austinvalle/sandbox function "test_func" reuses name "param" for both parameters 0 and 1..

│ Error: Failed to load plugin schemas
│ 
│ Error while loading schemas for plugin components: Failed to obtain provider schema: Could not load the schema for provider registry.terraform.io/austinvalle/sandbox: provider
│ registry.terraform.io/austinvalle/sandbox function "test_func" reuses name "param" for both parameter 0 and its variadic parameter..

Notably, this kind of invalid schema can be achieved inadvertently by omitting the parameter names in your function definition, which the framework currently defaults each name to param:

func (f *TestFunction) Definition(ctx context.Context, req function.DefinitionRequest, resp *function.DefinitionResponse) {
	resp.Definition = function.Definition{
		Parameters: []function.Parameter{
			function.StringParameter{}, // defaults to param
			function.StringParameter{}, // defaults to param
		},
		VariadicParameter: function.StringParameter{}, // defaults to param
		Return:            function.Int64Return{},
	}
}

The full Terraform validation logic can be found here: https://github.com/hashicorp/terraform/blob/82361afd73ae5abb778d50e3b500fc08c2b04393/internal/terraform/context_plugins.go#L143-L168

Current Workaround

The workaround is to explicitly name each parameter (which we would already recommend for all functions):

func (f *TestFunction) Definition(ctx context.Context, req function.DefinitionRequest, resp *function.DefinitionResponse) {
	resp.Definition = function.Definition{
		Parameters: []function.Parameter{
			function.StringParameter{
				Name: "param1",
			},
			function.StringParameter{
				Name: "param2",
			},
		},
		VariadicParameter: function.StringParameter{
			Name: "varparam",
		},
		Return: function.Int64Return{},
	}
}

Proposal

• The framework logic for defaulting function parameter names should be updated to ensure each parameter name is unique. We could potentially employ a strategy of:
  • Defaulting parameter names to param{index number of parameter} => param1
  • Defaulting variadic parameter name to varparam
• We may also want to consider adding validation logic to ensure the final function definition we send to Terraform has no conflicts if explicitly set:

func (f *TestFunction) Definition(ctx context.Context, req function.DefinitionRequest, resp *function.DefinitionResponse) {
	resp.Definition = function.Definition{
		Parameters: []function.Parameter{
			function.StringParameter{
				Name: "paramdup",
			},
			function.StringParameter{
				Name: "paramdup",
			},
		},
		VariadicParameter: function.StringParameter{
			Name: "paramdup",
		},
		Return: function.Int64Return{},
	}
}

References

• Originally found in: function: Switch the representation of variadic arguments to types.Tuple #923 (comment)

[bflad]

Both proposal ideas sound great! For the second validation idea, schemas have a ValidateImplementation method for unit testing (or framework usage 😉 ), so having something similar for function definitions would be extending the existing pattern.

[bflad]

I'm going to relabel this as a bug and set the milestone to v1.6.1. Would you like to fix this, @austinvalle?

[austinvalle]

Yeah I'll take this one! 🙂

[github-actions]

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