From b74c2e59efa17e91a58e2a7556331a24e20ede68 Mon Sep 17 00:00:00 2001 From: Hardy Jones Date: Sat, 25 Mar 2023 09:17:00 -0700 Subject: [PATCH] Fix `peerdns` attribute The way we setup `peerdns` isn't correct. We don't want to require both that the `proto` be `"dhcp"` and `"dhcpv6"`. That doesn't even make sense. And the fact that we can say that makes even less sense. In order to show the failure, we add a failing test. We then implement a fix by adding a validator that disjoins multiple other validators. The test is effectively what we want in reality, so it passing here should give good confidence that we can do what we want. It's unfortunate that we have to write this validator. Hopefully, it's an oversight and not intentional. We don't want to maintain this indefinitely. See https://github.com/hashicorp/terraform-plugin-framework-validators/issues/122 for the upstream issue. Branch: joneshf/failing-peerdns-test Pull-Request: https://github.com/joneshf/terraform-provider-openwrt/pull/110 --- openwrt/internal/lucirpcglue/attribute.go | 44 ++++++++++ openwrt/internal/network/interface.go | 16 ++-- .../network/interface_acceptance_test.go | 86 +++++++++++++++++++ 3 files changed, 139 insertions(+), 7 deletions(-) diff --git a/openwrt/internal/lucirpcglue/attribute.go b/openwrt/internal/lucirpcglue/attribute.go index b34d0c0..60b33ca 100644 --- a/openwrt/internal/lucirpcglue/attribute.go +++ b/openwrt/internal/lucirpcglue/attribute.go @@ -3,6 +3,7 @@ package lucirpcglue import ( "context" "fmt" + "strings" "github.com/hashicorp/terraform-plugin-framework-validators/helpers/validatordiag" "github.com/hashicorp/terraform-plugin-framework/attr" @@ -30,6 +31,8 @@ const ( ) var ( + _ validator.Bool = anyValidatorBool{} + _ validator.Bool = requiresAttribute[any]{} _ validator.Int64 = requiresAttribute[any]{} _ validator.List = requiresAttribute[any]{} @@ -37,6 +40,47 @@ var ( _ validator.String = requiresAttribute[any]{} ) +// AnyBool returns a validator which ensures that any configured attribute value passes at least one of the given validators. +func AnyBool(validators ...validator.Bool) validator.Bool { + return anyValidatorBool{ + validators: validators, + } +} + +type anyValidatorBool struct { + validators []validator.Bool +} + +func (v anyValidatorBool) Description(ctx context.Context) string { + var descriptions []string + + for _, subValidator := range v.validators { + descriptions = append(descriptions, subValidator.Description(ctx)) + } + + return fmt.Sprintf("Value must satisfy at least one of the validations: %s", strings.Join(descriptions, " + ")) +} + +func (v anyValidatorBool) MarkdownDescription(ctx context.Context) string { + return v.Description(ctx) +} + +func (v anyValidatorBool) ValidateBool(ctx context.Context, req validator.BoolRequest, resp *validator.BoolResponse) { + for _, subValidator := range v.validators { + validateResp := &validator.BoolResponse{} + + subValidator.ValidateBool(ctx, req, validateResp) + + if !validateResp.Diagnostics.HasError() { + resp.Diagnostics = validateResp.Diagnostics + + return + } + + resp.Diagnostics.Append(validateResp.Diagnostics...) + } +} + type AttributeExistence int func (e AttributeExistence) ToComputed() bool { diff --git a/openwrt/internal/network/interface.go b/openwrt/internal/network/interface.go index ad30a1d..72100f3 100644 --- a/openwrt/internal/network/interface.go +++ b/openwrt/internal/network/interface.go @@ -224,13 +224,15 @@ var ( ResourceExistence: lucirpcglue.NoValidation, UpsertRequest: lucirpcglue.UpsertRequestOptionBool(interfaceModelGetPeerDNS, interfacePeerDNSAttribute, interfacePeerDNSUCIOption), Validators: []validator.Bool{ - lucirpcglue.RequiresAttributeEqualString( - path.MatchRoot(interfaceProtocolAttribute), - interfaceProtocolDHCP, - ), - lucirpcglue.RequiresAttributeEqualString( - path.MatchRoot(interfaceProtocolAttribute), - interfaceProtocolDHCPV6, + lucirpcglue.AnyBool( + lucirpcglue.RequiresAttributeEqualString( + path.MatchRoot(interfaceProtocolAttribute), + interfaceProtocolDHCP, + ), + lucirpcglue.RequiresAttributeEqualString( + path.MatchRoot(interfaceProtocolAttribute), + interfaceProtocolDHCPV6, + ), ), }, } diff --git a/openwrt/internal/network/interface_acceptance_test.go b/openwrt/internal/network/interface_acceptance_test.go index 6c5c9da..35ce529 100644 --- a/openwrt/internal/network/interface_acceptance_test.go +++ b/openwrt/internal/network/interface_acceptance_test.go @@ -134,3 +134,89 @@ resource "openwrt_network_interface" "testing" { updateAndReadResource, ) } + +func TestNetworkInterfaceResourcePeerDNSWithDHCPAcceptance(t *testing.T) { + t.Parallel() + + ctx := context.Background() + providerBlock := acceptancetest.RunOpenWrtServerWithProviderBlock( + ctx, + *dockerPool, + t, + ) + + step := resource.TestStep{ + Config: fmt.Sprintf(` +%s + +resource "openwrt_network_interface" "testing" { + device = "br-testing" + dns = [ + "9.9.9.9", + "1.1.1.1", + ] + id = "testing" + peerdns = false + proto = "dhcp" +} +`, + providerBlock, + ), + Check: resource.ComposeAggregateTestCheckFunc( + resource.TestCheckResourceAttr("openwrt_network_interface.testing", "id", "testing"), + resource.TestCheckResourceAttr("openwrt_network_interface.testing", "device", "br-testing"), + resource.TestCheckResourceAttr("openwrt_network_interface.testing", "dns.0", "9.9.9.9"), + resource.TestCheckResourceAttr("openwrt_network_interface.testing", "dns.1", "1.1.1.1"), + resource.TestCheckResourceAttr("openwrt_network_interface.testing", "peerdns", "false"), + resource.TestCheckResourceAttr("openwrt_network_interface.testing", "proto", "dhcp"), + ), + } + + acceptancetest.TerraformSteps( + t, + step, + ) +} + +func TestNetworkInterfaceResourcePeerDNSWithDHCPV6Acceptance(t *testing.T) { + t.Parallel() + + ctx := context.Background() + providerBlock := acceptancetest.RunOpenWrtServerWithProviderBlock( + ctx, + *dockerPool, + t, + ) + + step := resource.TestStep{ + Config: fmt.Sprintf(` +%s + +resource "openwrt_network_interface" "testing" { + device = "br-testing" + dns = [ + "9.9.9.9", + "1.1.1.1", + ] + id = "testing" + peerdns = false + proto = "dhcpv6" +} +`, + providerBlock, + ), + Check: resource.ComposeAggregateTestCheckFunc( + resource.TestCheckResourceAttr("openwrt_network_interface.testing", "id", "testing"), + resource.TestCheckResourceAttr("openwrt_network_interface.testing", "device", "br-testing"), + resource.TestCheckResourceAttr("openwrt_network_interface.testing", "dns.0", "9.9.9.9"), + resource.TestCheckResourceAttr("openwrt_network_interface.testing", "dns.1", "1.1.1.1"), + resource.TestCheckResourceAttr("openwrt_network_interface.testing", "peerdns", "false"), + resource.TestCheckResourceAttr("openwrt_network_interface.testing", "proto", "dhcpv6"), + ), + } + + acceptancetest.TerraformSteps( + t, + step, + ) +}