From 9725ed74cb13a59a67b680da0966f3ee641a83fa Mon Sep 17 00:00:00 2001 From: Bruno Schaatsbergen Date: Fri, 3 Jan 2025 15:46:19 +0100 Subject: [PATCH 1/7] null: disallow unknown values and validate if the underlying value is null. --- internal/provider/null_function.go | 11 +++- internal/provider/null_function_test.go | 84 ++++++++++++++++++++++++- 2 files changed, 91 insertions(+), 4 deletions(-) diff --git a/internal/provider/null_function.go b/internal/provider/null_function.go index 3b4622e..cd8268a 100644 --- a/internal/provider/null_function.go +++ b/internal/provider/null_function.go @@ -30,7 +30,7 @@ func (r IsNullFunction) Definition(_ context.Context, _ function.DefinitionReque Parameters: []function.Parameter{ function.DynamicParameter{ AllowNullValue: true, - AllowUnknownValues: true, + AllowUnknownValues: false, Description: "The argument to check", Name: "argument", }, @@ -47,7 +47,14 @@ func (r IsNullFunction) Run(ctx context.Context, req function.RunRequest, resp * return } - if argument.UnderlyingValue() == nil { + if argument.IsNull() { + if err := resp.Result.Set(ctx, true); err == nil { + return + } + return + } + + if argument.IsUnderlyingValueNull() { if err := resp.Result.Set(ctx, true); err == nil { return } diff --git a/internal/provider/null_function_test.go b/internal/provider/null_function_test.go index 8704d12..04e7697 100644 --- a/internal/provider/null_function_test.go +++ b/internal/provider/null_function_test.go @@ -7,11 +7,12 @@ import ( "testing" "github.com/hashicorp/go-version" + "github.com/hashicorp/terraform-plugin-testing/config" "github.com/hashicorp/terraform-plugin-testing/helper/resource" "github.com/hashicorp/terraform-plugin-testing/tfversion" ) -func TestIsNullFunction(t *testing.T) { +func TestNullFunction(t *testing.T) { t.Parallel() resource.UnitTest(t, resource.TestCase{ TerraformVersionChecks: []tfversion.TerraformVersionCheck{ @@ -36,7 +37,86 @@ output "test" { }) } -func TestIsNullFunction_falseCases(t *testing.T) { +func TestNullFunction_crossObjectValidation(t *testing.T) { + t.Parallel() + resource.UnitTest(t, resource.TestCase{ + TerraformVersionChecks: []tfversion.TerraformVersionCheck{ + tfversion.SkipBelow(version.Must(version.NewVersion(MinimalRequiredTerraformVersion))), + }, + ExternalProviders: map[string]resource.ExternalProvider{ + "wireguard": { + Source: "OJFord/wireguard", + VersionConstraint: "0.3.1", + }, + }, + ProtoV6ProviderFactories: testAccProtoV6ProviderFactories, + Steps: []resource.TestStep{ + { + Config: ` +resource "wireguard_asymmetric_key" "main" {} + +data "wireguard_config_document" "main" { + private_key = wireguard_asymmetric_key.main.private_key +} + +output "test" { + // .addresses is always null in this configuration + value = provider::assert::null(data.wireguard_config_document.main.addresses) +} + `, + Check: resource.ComposeAggregateTestCheckFunc( + resource.TestCheckOutput("test", "true"), + ), + }, + }, + }) +} + +func TestNullFunction_compoundValidation(t *testing.T) { + t.Parallel() + resource.UnitTest(t, resource.TestCase{ + TerraformVersionChecks: []tfversion.TerraformVersionCheck{ + tfversion.SkipBelow(version.Must(version.NewVersion(MinimalRequiredTerraformVersion))), + }, + ProtoV6ProviderFactories: testAccProtoV6ProviderFactories, + Steps: []resource.TestStep{ + { + Config: ` +variable "ipv4_ipam_pool_id" { + default = null + description = "ID of the IPv4 IPAM pool to use for the VPC." + type = string +} + +variable "cidr_block" { + default = null + description = "CIDR block for the VPC." + type = string + + validation { + condition = provider::assert::cidr(var.cidr_block) + error_message = "CIDR block must be a valid CIDR range." + } + + validation { + condition = anytrue([ + !provider::assert::null(var.cidr_block), + !provider::assert::null(var.ipv4_ipam_pool_id) + ]) + error_message = "Exactly one of cidr_block or ipv4_ipam_pool_id must be provided." + } +} + `, + ConfigVariables: config.Variables{ + "cidr_block": config.StringVariable("10.0.42.0/24"), + }, + Check: resource.ComposeAggregateTestCheckFunc(), + }, + }, + }) +} + +func TestNullFunction_falseCases(t *testing.T) { t.Parallel() resource.UnitTest(t, resource.TestCase{ TerraformVersionChecks: []tfversion.TerraformVersionCheck{ From e90a41b963175e003a91d9754665a3cb2b4dc2b6 Mon Sep 17 00:00:00 2001 From: Bruno Schaatsbergen Date: Fri, 3 Jan 2025 15:46:36 +0100 Subject: [PATCH 2/7] not_null: disallow unknown values and check if the underlying value is null. --- internal/provider/not_null_function.go | 16 ++++- internal/provider/not_null_function_test.go | 68 +++++++++++++++++++++ 2 files changed, 83 insertions(+), 1 deletion(-) diff --git a/internal/provider/not_null_function.go b/internal/provider/not_null_function.go index 6a4b356..98eb585 100644 --- a/internal/provider/not_null_function.go +++ b/internal/provider/not_null_function.go @@ -47,5 +47,19 @@ func (r NotNullFunction) Run(ctx context.Context, req function.RunRequest, resp return } - resp.Error = function.ConcatFuncErrors(resp.Result.Set(ctx, !argument.IsNull())) + if argument.IsNull() { + if err := resp.Result.Set(ctx, false); err == nil { + return + } + return + } + + if !argument.IsUnderlyingValueNull() { + if err := resp.Result.Set(ctx, true); err == nil { + return + } + return + } + + resp.Error = function.ConcatFuncErrors(resp.Result.Set(ctx, false)) } diff --git a/internal/provider/not_null_function_test.go b/internal/provider/not_null_function_test.go index 698de0e..3074c82 100644 --- a/internal/provider/not_null_function_test.go +++ b/internal/provider/not_null_function_test.go @@ -7,6 +7,7 @@ import ( "testing" "github.com/hashicorp/go-version" + "github.com/hashicorp/terraform-plugin-testing/config" "github.com/hashicorp/terraform-plugin-testing/helper/resource" "github.com/hashicorp/terraform-plugin-testing/tfversion" ) @@ -220,6 +221,50 @@ output "test" { }) } +func TestNotNullFunction_compoundValidation(t *testing.T) { + t.Parallel() + resource.UnitTest(t, resource.TestCase{ + TerraformVersionChecks: []tfversion.TerraformVersionCheck{ + tfversion.SkipBelow(version.Must(version.NewVersion(MinimalRequiredTerraformVersion))), + }, + ProtoV6ProviderFactories: testAccProtoV6ProviderFactories, + Steps: []resource.TestStep{ + { + Config: ` +variable "ipv4_ipam_pool_id" { + default = null + description = "ID of the IPv4 IPAM pool to use for the VPC." + type = string +} + +variable "cidr_block" { + default = null + description = "CIDR block for the VPC." + type = string + + validation { + condition = provider::assert::cidr(var.cidr_block) + error_message = "CIDR block must be a valid CIDR range." + } + + validation { + condition = anytrue([ + provider::assert::not_null(var.cidr_block), + provider::assert::not_null(var.ipv4_ipam_pool_id) + ]) + error_message = "Exactly one of cidr_block or ipv4_ipam_pool_id must be provided." + } +} + `, + ConfigVariables: config.Variables{ + "cidr_block": config.StringVariable("10.0.42.0/24"), + }, + Check: resource.ComposeAggregateTestCheckFunc(), + }, + }, + }) +} + func TestNotNullFunction_falseCases(t *testing.T) { t.Parallel() resource.UnitTest(t, resource.TestCase{ @@ -227,6 +272,12 @@ func TestNotNullFunction_falseCases(t *testing.T) { tfversion.SkipBelow(version.Must(version.NewVersion(MinimalRequiredTerraformVersion))), }, ProtoV6ProviderFactories: testAccProtoV6ProviderFactories, + ExternalProviders: map[string]resource.ExternalProvider{ + "wireguard": { + Source: "OJFord/wireguard", + VersionConstraint: "0.3.1", + }, + }, Steps: []resource.TestStep{ { Config: ` @@ -235,6 +286,23 @@ locals { } output "test" { value = provider::assert::not_null(local.object) +} + `, + Check: resource.ComposeAggregateTestCheckFunc( + resource.TestCheckOutput("test", "false"), + ), + }, + { + Config: ` +resource "wireguard_asymmetric_key" "main" {} + +data "wireguard_config_document" "main" { + private_key = wireguard_asymmetric_key.main.private_key +} + +output "test" { + // .addresses is always null in this configuration + value = provider::assert::not_null(data.wireguard_config_document.main.addresses) } `, Check: resource.ComposeAggregateTestCheckFunc( From aecff3b305ef911b1a4b44f25d428796f69ca389 Mon Sep 17 00:00:00 2001 From: Bruno Schaatsbergen Date: Fri, 3 Jan 2025 15:49:46 +0100 Subject: [PATCH 3/7] terrafmt configurations --- internal/provider/not_null_function_test.go | 8 ++++---- internal/provider/null_function_test.go | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/internal/provider/not_null_function_test.go b/internal/provider/not_null_function_test.go index 3074c82..2c7aabf 100644 --- a/internal/provider/not_null_function_test.go +++ b/internal/provider/not_null_function_test.go @@ -243,15 +243,15 @@ variable "cidr_block" { type = string validation { - condition = provider::assert::cidr(var.cidr_block) + condition = provider::assert::cidr(var.cidr_block) error_message = "CIDR block must be a valid CIDR range." } validation { condition = anytrue([ - provider::assert::not_null(var.cidr_block), - provider::assert::not_null(var.ipv4_ipam_pool_id) - ]) + provider::assert::not_null(var.cidr_block), + provider::assert::not_null(var.ipv4_ipam_pool_id) + ]) error_message = "Exactly one of cidr_block or ipv4_ipam_pool_id must be provided." } } diff --git a/internal/provider/null_function_test.go b/internal/provider/null_function_test.go index 04e7697..1a1347e 100644 --- a/internal/provider/null_function_test.go +++ b/internal/provider/null_function_test.go @@ -94,15 +94,15 @@ variable "cidr_block" { type = string validation { - condition = provider::assert::cidr(var.cidr_block) + condition = provider::assert::cidr(var.cidr_block) error_message = "CIDR block must be a valid CIDR range." } validation { condition = anytrue([ - !provider::assert::null(var.cidr_block), - !provider::assert::null(var.ipv4_ipam_pool_id) - ]) + !provider::assert::null(var.cidr_block), + !provider::assert::null(var.ipv4_ipam_pool_id) + ]) error_message = "Exactly one of cidr_block or ipv4_ipam_pool_id must be provided." } } From eb178d04ab605e7784e728f1a48adfc7df359c92 Mon Sep 17 00:00:00 2001 From: Bruno Schaatsbergen Date: Fri, 3 Jan 2025 16:00:14 +0100 Subject: [PATCH 4/7] workflows: add 1.10 to test matrix --- .github/workflows/test.yml | 2 ++ internal/provider/not_null_function_test.go | 26 ++++++++------------- internal/provider/null_function_test.go | 23 +++++++----------- 3 files changed, 21 insertions(+), 30 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 46681b3..7f380f3 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -34,6 +34,7 @@ jobs: terraform: - '1.8.*' - '1.9.*' + - '1.10.*' steps: - uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1 - uses: actions/setup-go@0c52d547c9bc32b1aa3301fd7a9cb496313a4491 # v5.0.0 @@ -80,6 +81,7 @@ jobs: terraform: - '1.8.*' - '1.9.*' + - '1.10.*' steps: - uses: actions/checkout@a5ac7e51b41094c92402da3b24376905380afc29 # v4.1.6 - uses: actions/setup-go@cdcb36043654635271a94b9a6d1392de5bb323a7 # v5.0.1 diff --git a/internal/provider/not_null_function_test.go b/internal/provider/not_null_function_test.go index 2c7aabf..36437ef 100644 --- a/internal/provider/not_null_function_test.go +++ b/internal/provider/not_null_function_test.go @@ -225,39 +225,33 @@ func TestNotNullFunction_compoundValidation(t *testing.T) { t.Parallel() resource.UnitTest(t, resource.TestCase{ TerraformVersionChecks: []tfversion.TerraformVersionCheck{ - tfversion.SkipBelow(version.Must(version.NewVersion(MinimalRequiredTerraformVersion))), + tfversion.SkipBelow(version.Must(version.NewVersion("1.2.0"))), }, ProtoV6ProviderFactories: testAccProtoV6ProviderFactories, Steps: []resource.TestStep{ { Config: ` -variable "ipv4_ipam_pool_id" { +variable "example_value_a" { default = null - description = "ID of the IPv4 IPAM pool to use for the VPC." + description = "Example input A for validation." type = string } - -variable "cidr_block" { +variable "example_value_b" { default = null - description = "CIDR block for the VPC." + description = "Example input B for validation." type = string - - validation { - condition = provider::assert::cidr(var.cidr_block) - error_message = "CIDR block must be a valid CIDR range." - } - validation { condition = anytrue([ - provider::assert::not_null(var.cidr_block), - provider::assert::not_null(var.ipv4_ipam_pool_id) + provider::assert::not_null(var.example_value_a), + provider::assert::not_null(var.example_value_b) + ]) - error_message = "Exactly one of cidr_block or ipv4_ipam_pool_id must be provided." + error_message = "At least one of example_value_a or example_value_b must be provided." } } `, ConfigVariables: config.Variables{ - "cidr_block": config.StringVariable("10.0.42.0/24"), + "example_value_b": config.StringVariable("example-format-value"), }, Check: resource.ComposeAggregateTestCheckFunc(), }, diff --git a/internal/provider/null_function_test.go b/internal/provider/null_function_test.go index 1a1347e..bc35576 100644 --- a/internal/provider/null_function_test.go +++ b/internal/provider/null_function_test.go @@ -76,39 +76,34 @@ func TestNullFunction_compoundValidation(t *testing.T) { t.Parallel() resource.UnitTest(t, resource.TestCase{ TerraformVersionChecks: []tfversion.TerraformVersionCheck{ - tfversion.SkipBelow(version.Must(version.NewVersion(MinimalRequiredTerraformVersion))), + tfversion.SkipBelow(version.Must(version.NewVersion("1.2.0"))), }, ProtoV6ProviderFactories: testAccProtoV6ProviderFactories, Steps: []resource.TestStep{ { Config: ` -variable "ipv4_ipam_pool_id" { +variable "example_value_a" { default = null - description = "ID of the IPv4 IPAM pool to use for the VPC." + description = "Example option A for testing null validation." type = string } -variable "cidr_block" { +variable "example_value_b" { default = null - description = "CIDR block for the VPC." + description = "Example option B for testing null validation." type = string - validation { - condition = provider::assert::cidr(var.cidr_block) - error_message = "CIDR block must be a valid CIDR range." - } - validation { condition = anytrue([ - !provider::assert::null(var.cidr_block), - !provider::assert::null(var.ipv4_ipam_pool_id) + !provider::assert::null(var.example_value_a), + !provider::assert::null(var.example_value_b) ]) - error_message = "Exactly one of cidr_block or ipv4_ipam_pool_id must be provided." + error_message = "Exactly one of example_value_a or example_value_b must be provided." } } `, ConfigVariables: config.Variables{ - "cidr_block": config.StringVariable("10.0.42.0/24"), + "example_value_b": config.StringVariable("example-value"), }, Check: resource.ComposeAggregateTestCheckFunc(), }, From 6728c7bc9203ad552f7ddba3e6fa51fe4bf0240e Mon Sep 17 00:00:00 2001 From: Bruno Schaatsbergen Date: Fri, 3 Jan 2025 16:41:43 +0100 Subject: [PATCH 5/7] simplify setting the bool result --- internal/provider/not_null_function.go | 8 ++------ internal/provider/null_function.go | 8 ++------ 2 files changed, 4 insertions(+), 12 deletions(-) diff --git a/internal/provider/not_null_function.go b/internal/provider/not_null_function.go index 98eb585..b31fcad 100644 --- a/internal/provider/not_null_function.go +++ b/internal/provider/not_null_function.go @@ -48,16 +48,12 @@ func (r NotNullFunction) Run(ctx context.Context, req function.RunRequest, resp } if argument.IsNull() { - if err := resp.Result.Set(ctx, false); err == nil { - return - } + resp.Error = resp.Result.Set(ctx, false) return } if !argument.IsUnderlyingValueNull() { - if err := resp.Result.Set(ctx, true); err == nil { - return - } + resp.Error = resp.Result.Set(ctx, true) return } diff --git a/internal/provider/null_function.go b/internal/provider/null_function.go index cd8268a..7be6c30 100644 --- a/internal/provider/null_function.go +++ b/internal/provider/null_function.go @@ -48,16 +48,12 @@ func (r IsNullFunction) Run(ctx context.Context, req function.RunRequest, resp * } if argument.IsNull() { - if err := resp.Result.Set(ctx, true); err == nil { - return - } + resp.Error = resp.Result.Set(ctx, true) return } if argument.IsUnderlyingValueNull() { - if err := resp.Result.Set(ctx, true); err == nil { - return - } + resp.Error = resp.Result.Set(ctx, true) return } From aa46b66bb301788f949f588d77cfacb916d20296 Mon Sep 17 00:00:00 2001 From: Bruno Schaatsbergen Date: Fri, 3 Jan 2025 16:43:14 +0100 Subject: [PATCH 6/7] remove whiteline --- internal/provider/not_null_function_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/internal/provider/not_null_function_test.go b/internal/provider/not_null_function_test.go index 36437ef..65488f5 100644 --- a/internal/provider/not_null_function_test.go +++ b/internal/provider/not_null_function_test.go @@ -244,7 +244,6 @@ variable "example_value_b" { condition = anytrue([ provider::assert::not_null(var.example_value_a), provider::assert::not_null(var.example_value_b) - ]) error_message = "At least one of example_value_a or example_value_b must be provided." } From 0fa7e775600c4ea1c143bae967e6743843a97d58 Mon Sep 17 00:00:00 2001 From: Bruno Schaatsbergen Date: Fri, 10 Jan 2025 17:23:03 +0100 Subject: [PATCH 7/7] not_null: avoid accepting unknown values MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Unknown values could ultimately resolve to null or a valid value, so it’s best to avoid accepting them. --- internal/provider/not_null_function.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/provider/not_null_function.go b/internal/provider/not_null_function.go index b31fcad..84240eb 100644 --- a/internal/provider/not_null_function.go +++ b/internal/provider/not_null_function.go @@ -30,7 +30,7 @@ func (r NotNullFunction) Definition(_ context.Context, _ function.DefinitionRequ Parameters: []function.Parameter{ function.DynamicParameter{ AllowNullValue: true, - AllowUnknownValues: true, + AllowUnknownValues: false, Description: "The argument to check", Name: "argument", },