Fix dhcp.dhcp required fields

As it turns out, there's actually a case where these "required" fields
don't exist from the get-go. For WAN interfaces, they already have a
DHCP pool setup that is ignored. There's no `leasetime`, `limit`, or
`start` option associated with them. So you end up in this weird state
of questioning whether you should add these three fields to satiate this
Terraform provider, not import the WAN DHCP pool, delete the DHCP pool
from UCI all up, or something equally obtuse.

None of these things should be a thing someone has to think about. So we
change the attributes to not be required all the time, but only when the
`ignore` attribute is not `true`. We do this by adding a new validator
that checks the other attribute for its value and acts accordingly. We
update the tests to validate this behavior.

This is yet another example of how not having sum types causes so much
extra complexity. Ignoring the fact that OpenWrt shouldn't ship with an
ignored DHCP pool on the WAN interface, this situation is trivial (in
implementation) to represent with a sum type. But trying to represent it
with nothing by product and exponent types is incredibly complex. Ah
well, maybe one day things will get better…

Branch: joneshf/fix-dhcp-dhcp-required-fields
Pull-Request: #130

Author: joneshf

docs/data-sources/dhcp_dhcp.md

@@ -31,11 +31,11 @@ data "openwrt_dhcp_dhcp" "testing" {
 - `dhcpv6` (String) The mode of the DHCPv6 server. Must be one of: "disabled", "relay", "server".
 - `force` (Boolean) Forces DHCP serving on the specified interface even if another DHCP server is detected on the same network segment.
 - `ignore` (Boolean) Specifies whether dnsmasq should ignore this pool.
-- `interface` (String) The interface associated with this DHCP address pool. This name is what the interface is known as in UCI, or the `id` field in Terraform.
-- `leasetime` (String) The lease time of addresses handed out to clients. E.g. `12h`, or `30m`.
-- `limit` (Number) Specifies the size of the address pool. E.g. With start = 100, and limit = 150, the maximum address will be 249.
+- `interface` (String) The interface associated with this DHCP address pool. This name is what the interface is known as in UCI, or the `id` field in Terraform. Required if `ignore` is not `true`.
+- `leasetime` (String) The lease time of addresses handed out to clients. E.g. `12h`, or `30m`. Required if `ignore` is not `true`.
+- `limit` (Number) Specifies the size of the address pool. E.g. With start = 100, and limit = 150, the maximum address will be 249. Required if `ignore` is not `true`.
 - `ra` (String) The mode of Router Advertisements. Must be one of: "disabled", "relay", "server".
 - `ra_flags` (Set of String) Router Advertisement flags to include in messages. Must be one of: "home-agent", "managed-config", "none", "other-config".
-- `start` (Number) Specifies the offset from the network address of the underlying interface to calculate the minimum address that may be leased to clients. It may be greater than 255 to span subnets.
+- `start` (Number) Specifies the offset from the network address of the underlying interface to calculate the minimum address that may be leased to clients. It may be greater than 255 to span subnets. Required if `ignore` is not `true`.
 
 

docs/resources/dhcp_dhcp.md

@@ -57,19 +57,19 @@ resource "openwrt_dhcp_dhcp" "testing" {
 ### Required
 
 - `id` (String) Name of the section. This name is only used when interacting with UCI directly.
-- `leasetime` (String) The lease time of addresses handed out to clients. E.g. `12h`, or `30m`.
-- `limit` (Number) Specifies the size of the address pool. E.g. With start = 100, and limit = 150, the maximum address will be 249.
-- `start` (Number) Specifies the offset from the network address of the underlying interface to calculate the minimum address that may be leased to clients. It may be greater than 255 to span subnets.
 
 ### Optional
 
 - `dhcpv4` (String) The mode of the DHCPv4 server. Must be one of: "disabled", "server".
 - `dhcpv6` (String) The mode of the DHCPv6 server. Must be one of: "disabled", "relay", "server".
 - `force` (Boolean) Forces DHCP serving on the specified interface even if another DHCP server is detected on the same network segment.
 - `ignore` (Boolean) Specifies whether dnsmasq should ignore this pool.
-- `interface` (String) The interface associated with this DHCP address pool. This name is what the interface is known as in UCI, or the `id` field in Terraform.
+- `interface` (String) The interface associated with this DHCP address pool. This name is what the interface is known as in UCI, or the `id` field in Terraform. Required if `ignore` is not `true`.
+- `leasetime` (String) The lease time of addresses handed out to clients. E.g. `12h`, or `30m`. Required if `ignore` is not `true`.
+- `limit` (Number) Specifies the size of the address pool. E.g. With start = 100, and limit = 150, the maximum address will be 249. Required if `ignore` is not `true`.
 - `ra` (String) The mode of Router Advertisements. Must be one of: "disabled", "relay", "server".
 - `ra_flags` (Set of String) Router Advertisement flags to include in messages. Must be one of: "home-agent", "managed-config", "none", "other-config".
+- `start` (Number) Specifies the offset from the network address of the underlying interface to calculate the minimum address that may be leased to clients. It may be greater than 255 to span subnets. Required if `ignore` is not `true`.
 
 ## Import
 

openwrt/dhcp/dhcp/dhcp.go

@@ -4,6 +4,7 @@ import (
 	"github.com/hashicorp/terraform-plugin-framework-validators/setvalidator"
 	"github.com/hashicorp/terraform-plugin-framework-validators/stringvalidator"
 	"github.com/hashicorp/terraform-plugin-framework/datasource"
+	"github.com/hashicorp/terraform-plugin-framework/path"
 	"github.com/hashicorp/terraform-plugin-framework/resource"
 	"github.com/hashicorp/terraform-plugin-framework/schema/validator"
 	"github.com/hashicorp/terraform-plugin-framework/types"
@@ -34,15 +35,15 @@ const (
 	ignoreUCIOption            = "ignore"
 
 	interfaceAttribute            = "interface"
-	interfaceAttributeDescription = "The interface associated with this DHCP address pool. This name is what the interface is known as in UCI, or the `id` field in Terraform."
+	interfaceAttributeDescription = "The interface associated with this DHCP address pool. This name is what the interface is known as in UCI, or the `id` field in Terraform. Required if `ignore` is not `true`."
 	interfaceUCIOption            = "interface"
 
 	leaseTimeAttribute            = "leasetime"
-	leaseTimeAttributeDescription = "The lease time of addresses handed out to clients. E.g. `12h`, or `30m`."
+	leaseTimeAttributeDescription = "The lease time of addresses handed out to clients. E.g. `12h`, or `30m`. Required if `ignore` is not `true`."
 	leaseTimeUCIOption            = "leasetime"
 
 	limitAttribute            = "limit"
-	limitAttributeDescription = "Specifies the size of the address pool. E.g. With start = 100, and limit = 150, the maximum address will be 249."
+	limitAttributeDescription = "Specifies the size of the address pool. E.g. With start = 100, and limit = 150, the maximum address will be 249. Required if `ignore` is not `true`."
 	limitUCIOption            = "limit"
 
 	routerAdvertisementFlagsAttribute            = "ra_flags"
@@ -63,7 +64,7 @@ const (
 	schemaDescription = "Per interface lease pools and settings for serving DHCP requests."
 
 	startAttribute            = "start"
-	startAttributeDescription = "Specifies the offset from the network address of the underlying interface to calculate the minimum address that may be leased to clients. It may be greater than 255 to span subnets."
+	startAttributeDescription = "Specifies the offset from the network address of the underlying interface to calculate the minimum address that may be leased to clients. It may be greater than 255 to span subnets. Required if `ignore` is not `true`."
 	startUCIOption            = "start"
 
 	uciConfig = "dhcp"
@@ -117,20 +118,29 @@ var (
 		ReadResponse:      lucirpcglue.ReadResponseOptionString(modelSetInterface, interfaceAttribute, interfaceUCIOption),
 		ResourceExistence: lucirpcglue.NoValidation,
 		UpsertRequest:     lucirpcglue.UpsertRequestOptionString(modelGetInterface, interfaceAttribute, interfaceUCIOption),
+		Validators: []validator.String{
+			lucirpcglue.RequiredIfAttributeNotEqualBool(path.MatchRoot(ignoreAttribute), true),
+		},
 	}
 
 	leaseTimeSchemaAttribute = lucirpcglue.StringSchemaAttribute[model, lucirpc.Options, lucirpc.Options]{
 		Description:       leaseTimeAttributeDescription,
 		ReadResponse:      lucirpcglue.ReadResponseOptionString(modelSetLeaseTime, leaseTimeAttribute, leaseTimeUCIOption),
-		ResourceExistence: lucirpcglue.Required,
+		ResourceExistence: lucirpcglue.NoValidation,
 		UpsertRequest:     lucirpcglue.UpsertRequestOptionString(modelGetLeaseTime, leaseTimeAttribute, leaseTimeUCIOption),
+		Validators: []validator.String{
+			lucirpcglue.RequiredIfAttributeNotEqualBool(path.MatchRoot(ignoreAttribute), true),
+		},
 	}
 
 	limitSchemaAttribute = lucirpcglue.Int64SchemaAttribute[model, lucirpc.Options, lucirpc.Options]{
 		Description:       limitAttributeDescription,
 		ReadResponse:      lucirpcglue.ReadResponseOptionInt64(modelSetLimit, limitAttribute, limitUCIOption),
-		ResourceExistence: lucirpcglue.Required,
+		ResourceExistence: lucirpcglue.NoValidation,
 		UpsertRequest:     lucirpcglue.UpsertRequestOptionInt64(modelGetLimit, limitAttribute, limitUCIOption),
+		Validators: []validator.Int64{
+			lucirpcglue.RequiredIfAttributeNotEqualBool(path.MatchRoot(ignoreAttribute), true),
+		},
 	}
 
 	routerAdvertisementFlagsSchemaAttribute = lucirpcglue.SetStringSchemaAttribute[model, lucirpc.Options, lucirpc.Options]{
@@ -181,8 +191,11 @@ var (
 	startSchemaAttribute = lucirpcglue.Int64SchemaAttribute[model, lucirpc.Options, lucirpc.Options]{
 		Description:       startAttributeDescription,
 		ReadResponse:      lucirpcglue.ReadResponseOptionInt64(modelSetStart, startAttribute, startUCIOption),
-		ResourceExistence: lucirpcglue.Required,
+		ResourceExistence: lucirpcglue.NoValidation,
 		UpsertRequest:     lucirpcglue.UpsertRequestOptionInt64(modelGetStart, startAttribute, startUCIOption),
+		Validators: []validator.Int64{
+			lucirpcglue.RequiredIfAttributeNotEqualBool(path.MatchRoot(ignoreAttribute), true),
+		},
 	}
 )
 

openwrt/dhcp/dhcp/dhcp_acceptance_test.go

@@ -75,27 +75,62 @@ func TestResourceAcceptance(t *testing.T) {
 
 resource "openwrt_dhcp_dhcp" "testing" {
 	id = "testing"
+	ignore = true
+}
+`,
+			providerBlock,
+		),
+		Check: resource.ComposeAggregateTestCheckFunc(
+			resource.TestCheckResourceAttr("openwrt_dhcp_dhcp.testing", "id", "testing"),
+			resource.TestCheckResourceAttr("openwrt_dhcp_dhcp.testing", "ignore", "true"),
+			resource.TestCheckNoResourceAttr("openwrt_dhcp_dhcp.testing", "dhcpv4"),
+			resource.TestCheckNoResourceAttr("openwrt_dhcp_dhcp.testing", "dhcpv6"),
+			resource.TestCheckNoResourceAttr("openwrt_dhcp_dhcp.testing", "interface"),
+			resource.TestCheckNoResourceAttr("openwrt_dhcp_dhcp.testing", "leasetime"),
+			resource.TestCheckNoResourceAttr("openwrt_dhcp_dhcp.testing", "limit"),
+			resource.TestCheckNoResourceAttr("openwrt_dhcp_dhcp.testing", "ra_flags"),
+			resource.TestCheckNoResourceAttr("openwrt_dhcp_dhcp.testing", "start"),
+		),
+	}
+	importValidation := resource.TestStep{
+		ImportState:       true,
+		ImportStateVerify: true,
+		ResourceName:      "openwrt_dhcp_dhcp.testing",
+	}
+	ignoreWithOtherAttributes := resource.TestStep{
+		Config: fmt.Sprintf(`
+%s
+
+resource "openwrt_dhcp_dhcp" "testing" {
+	dhcpv4 = "server"
+	dhcpv6 = "disabled"
+	id = "testing"
+	ignore = true
 	interface = "testing"
 	leasetime = "12h"
 	limit = 150
+	ra_flags = [
+		"managed-config",
+		"other-config",
+	]
 	start = 100
 }
 `,
 			providerBlock,
 		),
 		Check: resource.ComposeAggregateTestCheckFunc(
 			resource.TestCheckResourceAttr("openwrt_dhcp_dhcp.testing", "id", "testing"),
+			resource.TestCheckResourceAttr("openwrt_dhcp_dhcp.testing", "dhcpv4", "server"),
+			resource.TestCheckResourceAttr("openwrt_dhcp_dhcp.testing", "dhcpv6", "disabled"),
+			resource.TestCheckResourceAttr("openwrt_dhcp_dhcp.testing", "ignore", "true"),
 			resource.TestCheckResourceAttr("openwrt_dhcp_dhcp.testing", "interface", "testing"),
 			resource.TestCheckResourceAttr("openwrt_dhcp_dhcp.testing", "leasetime", "12h"),
 			resource.TestCheckResourceAttr("openwrt_dhcp_dhcp.testing", "limit", "150"),
+			resource.TestCheckResourceAttr("openwrt_dhcp_dhcp.testing", "ra_flags.0", "managed-config"),
+			resource.TestCheckResourceAttr("openwrt_dhcp_dhcp.testing", "ra_flags.1", "other-config"),
 			resource.TestCheckResourceAttr("openwrt_dhcp_dhcp.testing", "start", "100"),
 		),
 	}
-	importValidation := resource.TestStep{
-		ImportState:       true,
-		ImportStateVerify: true,
-		ResourceName:      "openwrt_dhcp_dhcp.testing",
-	}
 	updateAndReadResource := resource.TestStep{
 		Config: fmt.Sprintf(`
 %s
@@ -133,6 +168,7 @@ resource "openwrt_dhcp_dhcp" "testing" {
 		t,
 		createAndReadResource,
 		importValidation,
+		ignoreWithOtherAttributes,
 		updateAndReadResource,
 	)
 }

openwrt/internal/lucirpcglue/attribute.go

@@ -33,6 +33,12 @@ const (
 var (
 	_ validator.Bool = anyValidatorBool{}
 
+	_ validator.Bool   = requiredIfAttributeNot[any]{}
+	_ validator.Int64  = requiredIfAttributeNot[any]{}
+	_ validator.List   = requiredIfAttributeNot[any]{}
+	_ validator.Set    = requiredIfAttributeNot[any]{}
+	_ validator.String = requiredIfAttributeNot[any]{}
+
 	_ validator.Bool   = requiresAttribute[any]{}
 	_ validator.Int64  = requiresAttribute[any]{}
 	_ validator.List   = requiresAttribute[any]{}
@@ -413,6 +419,18 @@ func ReadResponseOptionString[Model any](
 		return ctx, model, diagnostics
 	}
 }
+
+func RequiredIfAttributeNotEqualBool(
+	expression path.Expression,
+	expected bool,
+) requiredIfAttributeNot[bool] {
+	return requiredIfAttributeNotEqual(
+		types.BoolType,
+		expression,
+		expected,
+	)
+}
+
 func RequiresAttributeEqualBool(
 	expression path.Expression,
 	expected bool,
@@ -705,6 +723,163 @@ func hasValue(
 	return !attribute.IsNull() && !attribute.IsUnknown()
 }
 
+type requiredIfAttributeNot[Value any] struct {
+	attrType   attr.Type
+	expected   Value
+	expression path.Expression
+}
+
+func (a requiredIfAttributeNot[Value]) Description(ctx context.Context) string {
+	return a.MarkdownDescription(ctx)
+}
+
+func (a requiredIfAttributeNot[Value]) MarkdownDescription(ctx context.Context) string {
+	return fmt.Sprintf("Ensures that an attribute is set, if %q is also set to %v", a.expression, a.expected)
+}
+
+func (a requiredIfAttributeNot[Value]) ValidateBool(
+	ctx context.Context,
+	req validator.BoolRequest,
+	res *validator.BoolResponse,
+) {
+	diagnostics := a.validate(ctx, req.Config, req.Path, req.ConfigValue)
+	res.Diagnostics.Append(diagnostics...)
+	if res.Diagnostics.HasError() {
+		return
+	}
+}
+
+func (a requiredIfAttributeNot[Value]) ValidateInt64(
+	ctx context.Context,
+	req validator.Int64Request,
+	res *validator.Int64Response,
+) {
+	diagnostics := a.validate(ctx, req.Config, req.Path, req.ConfigValue)
+	res.Diagnostics.Append(diagnostics...)
+	if res.Diagnostics.HasError() {
+		return
+	}
+}
+
+func (a requiredIfAttributeNot[Value]) ValidateList(
+	ctx context.Context,
+	req validator.ListRequest,
+	res *validator.ListResponse,
+) {
+	diagnostics := a.validate(ctx, req.Config, req.Path, req.ConfigValue)
+	res.Diagnostics.Append(diagnostics...)
+	if res.Diagnostics.HasError() {
+		return
+	}
+}
+
+func (a requiredIfAttributeNot[Value]) ValidateSet(
+	ctx context.Context,
+	req validator.SetRequest,
+	res *validator.SetResponse,
+) {
+	diagnostics := a.validate(ctx, req.Config, req.Path, req.ConfigValue)
+	res.Diagnostics.Append(diagnostics...)
+	if res.Diagnostics.HasError() {
+		return
+	}
+}
+
+func (a requiredIfAttributeNot[Value]) ValidateString(
+	ctx context.Context,
+	req validator.StringRequest,
+	res *validator.StringResponse,
+) {
+	diagnostics := a.validate(ctx, req.Config, req.Path, req.ConfigValue)
+	res.Diagnostics.Append(diagnostics...)
+	if res.Diagnostics.HasError() {
+		return
+	}
+}
+
+func (a requiredIfAttributeNot[Value]) validate(
+	ctx context.Context,
+	config tfsdk.Config,
+	requestPath path.Path,
+	configValue interface{ IsNull() bool },
+) (allDiagnostics diag.Diagnostics) {
+	if !configValue.IsNull() {
+		return
+	}
+
+	matchedPaths, diagnostics := config.PathMatches(ctx, a.expression)
+	allDiagnostics.Append(diagnostics...)
+	if allDiagnostics.HasError() {
+		return
+	}
+
+	for _, matchedPath := range matchedPaths {
+		if matchedPath.Equal(requestPath) {
+			allDiagnostics.Append(
+				validatordiag.BugInProviderDiagnostic(
+					fmt.Sprintf("Attribute %q cannot require itself to have a specific value", requestPath),
+				),
+			)
+			continue
+		}
+
+		var actual attr.Value
+		diagnostics = config.GetAttribute(ctx, matchedPath, &actual)
+		allDiagnostics.Append(diagnostics...)
+		if allDiagnostics.HasError() {
+			continue
+		}
+
+		if actual.IsUnknown() {
+			// Ignore this value until it is known.
+			continue
+		}
+
+		if actual.IsNull() {
+			// If the value is null,
+			// it cannot be what we expect.
+			// We ignore the value.
+			continue
+		}
+
+		var expected attr.Value
+		diagnostics = tfsdk.ValueFrom(ctx, a.expected, a.attrType, &expected)
+		allDiagnostics.Append(diagnostics...)
+		if allDiagnostics.HasError() {
+			continue
+		}
+
+		if !actual.Equal(expected) {
+			allDiagnostics.Append(
+				diag.NewAttributeErrorDiagnostic(
+					requestPath,
+					"Missing required argument",
+					fmt.Sprintf("Attribute %q is required when %q is not %v", requestPath, matchedPath, expected),
+				),
+			)
+			continue
+		}
+	}
+
+	if allDiagnostics.HasError() {
+		return
+	}
+
+	return
+}
+
+func requiredIfAttributeNotEqual[Value any](
+	attrType attr.Type,
+	expression path.Expression,
+	expected Value,
+) requiredIfAttributeNot[Value] {
+	return requiredIfAttributeNot[Value]{
+		attrType:   attrType,
+		expected:   expected,
+		expression: expression,
+	}
+}
+
 type requiresAttribute[Value any] struct {
 	attrType   attr.Type
 	expected   Value