settings: support enum cluster setting validation

Previously, enum cluster settings did not support validation check. This commit
adds one. I considered whether validation should take a string or an enum. If we
validate using the enum directly, we’d need separate validator function
signatures for each enum type, since `EnumSetting[T constraints.Integer]` is
generic but `SettingOption` isn’t.  If we take a string, `ParseEnum` happens earlier
and `SET` only sees the enum's integer value, validating directly would require
converting the int back to a string.

I ended up taking in a string since it aligns with the existing design pattern
around enum settings.

1. `RegisterEnumSetting` uses strings for default values. 2. It feels more intuitive
to set the cluster setting with the string input. It did require us adding
`parseEnumInt64`, which converts the enum's integer value back to its string
representation for validation.

Epic: none
Release note: none

Author: wenyihu6

pkg/settings/enum.go

@@ -13,6 +13,7 @@ import (
 	"strconv"
 	"strings"
 
+	"github.com/cockroachdb/errors"
 	"golang.org/x/exp/constraints"
 )
 
@@ -22,13 +23,17 @@ type EnumSetting[T constraints.Integer] struct {
 	defaultValue T
 	// enumValues maps each valid value of T to a lowercase string.
 	enumValues map[T]string
+	validateFn func(string) error
 }
 
 // AnyEnumSetting is an interface that is used when the specific enum type T is
 // not known.
 type AnyEnumSetting interface {
 	internalSetting
 
+	// Validate validates the enum value.
+	Validate(value int64) error
+
 	// ParseEnum parses a string that is either a string in enumValues or an
 	// integer and returns the enum value as an int64 (and a boolean that
 	// indicates if it was parseable).
@@ -85,18 +90,32 @@ func (e *EnumSetting[T]) setToDefault(ctx context.Context, sv *Values) {
 	// See if the default value was overridden.
 	if val := sv.getDefaultOverride(e.slot); val != nil {
 		// As per the semantics of override, these values don't go through
-		// validation.
-		sv.setInt64(ctx, e.slot, val.(int64))
+		// validation. TODO(wenyihu6): there seems to be a bug here. See more in
+		// #149982.
+		_ = e.set(ctx, sv, val.(int64))
 		return
 	}
-	sv.setInt64(ctx, e.slot, int64(e.defaultValue))
+	if err := e.set(ctx, sv, int64(e.defaultValue)); err != nil {
+		panic(err)
+	}
+}
+
+func (e *EnumSetting[T]) set(ctx context.Context, sv *Values, v int64) error {
+	if err := e.Validate(v); err != nil {
+		return err
+	}
+	sv.setInt64(ctx, e.slot, v)
+	return nil
 }
 
 func (e *EnumSetting[T]) decodeAndSet(ctx context.Context, sv *Values, encoded string) error {
 	v, err := strconv.ParseInt(encoded, 10, 64)
 	if err != nil {
 		return err
 	}
+	if err := e.Validate(v); err != nil {
+		return err
+	}
 	sv.setInt64(ctx, e.slot, v)
 	return nil
 }
@@ -124,6 +143,26 @@ func (e *EnumSetting[T]) DecodeToString(encoded string) (string, error) {
 	return encoded, nil
 }
 
+// parseEnumInt64 converts an int64 to its enum string value for validation.
+// The string value is used by validateFn to validate the enum.
+func (e *EnumSetting[T]) parseEnumInt64(value int64) (string, bool) {
+	for k, v := range e.enumValues {
+		if value == int64(k) {
+			return v, true
+		}
+	}
+	return "", false
+}
+
+// Validate validates the enum value.
+func (e *EnumSetting[T]) Validate(value int64) error {
+	parsed, ok := e.parseEnumInt64(value)
+	if !ok {
+		return fmt.Errorf("invalid enum value: %d", value)
+	}
+	return e.validateFn(parsed)
+}
+
 // ParseEnum parses a string that is either a string in enumValues or an integer
 // and returns the enum value as an int64 (and a boolean that indicates if it
 // was parseable).
@@ -212,6 +251,22 @@ func RegisterEnumSetting[T constraints.Integer](
 		enumValuesLower[k] = strings.ToLower(v)
 	}
 
+	validateFn := func(val string) error {
+		for _, opt := range opts {
+			switch {
+			case opt.commonOpt != nil:
+				continue
+			case opt.validateEnumFn != nil:
+			default:
+				panic(errors.AssertionFailedf("wrong validator type"))
+			}
+			if err := opt.validateEnumFn(val); err != nil {
+				return err
+			}
+		}
+		return nil
+	}
+
 	defaultVal := func() T {
 		for k, v := range enumValues {
 			if v == defaultValue {
@@ -221,9 +276,14 @@ func RegisterEnumSetting[T constraints.Integer](
 		panic(fmt.Sprintf("enum registered with default value %s not in map %s", defaultValue, enumValuesToDesc(enumValuesLower)))
 	}()
 
+	if err := validateFn(defaultValue); err != nil {
+		panic(errors.Wrap(err, "invalid default"))
+	}
+
 	setting := &EnumSetting[T]{
 		defaultValue: defaultVal,
 		enumValues:   enumValuesLower,
+		validateFn:   validateFn,
 	}
 
 	register(class, key, fmt.Sprintf("%s %s", desc, enumValuesToDesc(enumValues)), setting)

pkg/settings/options.go

@@ -20,6 +20,7 @@ type SettingOption struct {
 	validateFloat64Fn  func(float64) error
 	validateStringFn   func(*Values, string) error
 	validateProtoFn    func(*Values, protoutil.Message) error
+	validateEnumFn     func(string) error
 }
 
 // NameStatus indicates the status of a setting name.
@@ -119,6 +120,11 @@ func WithValidateProto(fn func(*Values, protoutil.Message) error) SettingOption
 	return SettingOption{validateProtoFn: fn}
 }
 
+// WithValidateEnum adds a validation function for an enum setting.
+func WithValidateEnum(fn func(string) error) SettingOption {
+	return SettingOption{validateEnumFn: fn}
+}
+
 func (c *common) apply(opts []SettingOption) {
 	for _, opt := range opts {
 		if opt.commonOpt != nil {

pkg/settings/validation_test.go

@@ -6,13 +6,16 @@
 package settings_test
 
 import (
+	"context"
 	"fmt"
 	"strconv"
 	"testing"
 	"time"
 
 	"github.com/cockroachdb/cockroach/pkg/settings"
 	"github.com/cockroachdb/cockroach/pkg/testutils"
+	"github.com/cockroachdb/errors"
+	"github.com/stretchr/testify/require"
 )
 
 var cantBeTrue = settings.WithValidateBool(func(sv *settings.Values, b bool) error {
@@ -184,6 +187,70 @@ func TestValidationOptions(t *testing.T) {
 				{val: 21, opt: settings.IntInRangeOrZeroDisable(10, 20), expectedErr: `expected value in range \[10, 20\] or 0 to disable, got: 21`},
 			},
 		},
+		{
+			testLabel: "int64 enum",
+			settingFn: func(n int, ival interface{}, opt ...settings.SettingOption) settings.Setting {
+				val := ival.(string)
+				enumValues := map[int]string{
+					0: "zero",
+					1: "one",
+					2: "two",
+				}
+				return settings.RegisterEnumSetting(
+					settings.SystemOnly, settings.InternalKey(fmt.Sprintf("test-%d", n)),
+					"desc", val, enumValues, opt...,
+				)
+			},
+			subTests: []subTest{
+				{val: "zero", opt: settings.WithValidateEnum(func(s string) error {
+					if s == "zero" {
+						return errors.New("cannot be zero")
+					}
+					return nil
+				}), expectedErr: "cannot be zero"},
+				{val: "one", opt: settings.WithValidateEnum(func(s string) error {
+					if s == "zero" {
+						return errors.New("cannot be zero")
+					}
+					return nil
+				}), expectedErr: ""},
+				{val: "two", opt: settings.WithValidateEnum(func(s string) error {
+					return nil
+				}), expectedErr: ""},
+			},
+		},
+		{
+			testLabel: "uint enum",
+			settingFn: func(n int, ival interface{}, opt ...settings.SettingOption) settings.Setting {
+				val := ival.(string)
+				enumValues := map[uint]string{
+					0: "zero",
+					1: "one",
+					2: "two",
+				}
+				return settings.RegisterEnumSetting(
+					settings.SystemOnly, settings.InternalKey(fmt.Sprintf("test-%d", n)),
+					"desc", val, enumValues, opt...,
+				)
+			},
+			subTests: []subTest{
+				{val: "zero", opt: settings.WithValidateEnum(func(s string) error {
+					if s == "zero" {
+						return errors.New("cannot be zero")
+					}
+					return nil
+				}), expectedErr: "cannot be zero"},
+				{val: "one", opt: settings.WithValidateEnum(func(s string) error {
+					if s == "zero" {
+						return errors.New("cannot be zero")
+					}
+					return nil
+				}), expectedErr: ""},
+				{val: "two", opt: settings.WithValidateEnum(func(s string) error {
+					return nil
+				}), expectedErr: ""},
+			},
+		},
 		{
 			testLabel: "bytesize",
 			settingFn: func(n int, ival interface{}, opt ...settings.SettingOption) settings.Setting {
@@ -261,3 +328,33 @@ func TestValidationOptions(t *testing.T) {
 		})
 	}
 }
+
+var enumWithValidation = settings.RegisterEnumSetting(settings.SystemVisible, "enum.with.validation", "desc", "one", map[int]string{
+	0: "zero",
+	1: "one",
+	2: "two",
+}, settings.WithValidateEnum(func(v string) error {
+	if v == "zero" {
+		return errors.New("cannot be zero")
+	}
+	return nil
+}))
+
+// TestEnumWithValidation tests that the enum setting with validation works as expected.
+func TestEnumWithValidation(t *testing.T) {
+	ctx := context.Background()
+	sv := &settings.Values{}
+	sv.Init(ctx, settings.TestOpaque)
+	u := settings.NewUpdater(sv)
+	err := u.Set(ctx, "enum.with.validation", v(settings.EncodeInt(0), "e"))
+	require.Error(t, err)
+	require.ErrorContains(t, err, "cannot be zero")
+
+	err = u.Set(ctx, "enum.with.validation", v(settings.EncodeInt(2), "e"))
+	require.NoError(t, err)
+	require.Equal(t, 2, enumWithValidation.Get(sv))
+
+	err = u.SetToDefault(ctx, "enum.with.validation")
+	require.NoError(t, err)
+	require.Equal(t, 1, enumWithValidation.Get(sv))
+}

pkg/sql/set_cluster_setting.go

@@ -820,13 +820,19 @@ func toSettingString(
 		if i, intOK := d.(*tree.DInt); intOK {
 			v, ok := setting.ParseEnum(settings.EncodeInt(int64(*i)))
 			if ok {
+				if err := setting.Validate(v); err != nil {
+					return "", err
+				}
 				return settings.EncodeInt(v), nil
 			}
 			return "", errors.WithHint(errors.Errorf("invalid integer value '%d' for enum setting", *i), setting.GetAvailableValuesAsHint())
 		} else if s, ok := d.(*tree.DString); ok {
 			str := string(*s)
 			v, ok := setting.ParseEnum(str)
 			if ok {
+				if err := setting.Validate(v); err != nil {
+					return "", err
+				}
 				return settings.EncodeInt(v), nil
 			}
 			return "", errors.WithHint(errors.Errorf("invalid string value '%s' for enum setting", str), setting.GetAvailableValuesAsHint())