refactor: Standardize FC config validation pattern

Introduces a consistent `ValidateAndApplyDefaults` method to the
configuration structs in the `flowcontrol/controller` and
`flowcontrol/registry` packages.

This change refactors the configuration handling to follow a unified
pattern:
- Configuration structs now have a `ValidateAndApplyDefaults` method
  that returns a new, validated config object without mutating the
  original.
- Constructors for `FlowController` and `FlowRegistry` now assume they
  receive a valid configuration, simplifying their logic and pushing the
  responsibility of validation to the caller.
- The `deepCopy` logic is corrected to ensure test assertions for
  immutability pass reliably.

This improves the clarity and robustness of configuration management
within the flow control module, creating a consistent foundation for
future wiring work.

Author: LukeAVanDrie

pkg/epp/flowcontrol/controller/config.go

@@ -53,46 +53,38 @@ type Config struct {
 	EnqueueChannelBufferSize int
 }
 
-// newConfig performs validation and initialization, returning a guaranteed-valid `Config` object.
-// This is the required constructor for creating a new configuration.
-// It does not mutate the input `cfg`.
-func newConfig(cfg Config) (*Config, error) {
-	newCfg := cfg.deepCopy()
-	if err := newCfg.validateAndApplyDefaults(); err != nil {
-		return nil, err
-	}
-	return newCfg, nil
-}
+// ValidateAndApplyDefaults checks the global configuration for validity and then creates a new `Config` object,
+// populating any empty fields with system defaults.
+// It does not mutate the receiver.
+func (c *Config) ValidateAndApplyDefaults() (*Config, error) {
+	cfg := c.deepCopy()
 
-// validateAndApplyDefaults checks the global configuration for validity and then mutates the receiver to populate any
-// empty fields with system defaults.
-func (c *Config) validateAndApplyDefaults() error {
 	// --- Validation ---
-	if c.DefaultRequestTTL < 0 {
-		return fmt.Errorf("DefaultRequestTTL cannot be negative, but got %v", c.DefaultRequestTTL)
+	if cfg.DefaultRequestTTL < 0 {
+		return nil, fmt.Errorf("DefaultRequestTTL cannot be negative, but got %v", cfg.DefaultRequestTTL)
 	}
-	if c.ExpiryCleanupInterval < 0 {
-		return fmt.Errorf("ExpiryCleanupInterval cannot be negative, but got %v", c.ExpiryCleanupInterval)
+	if cfg.ExpiryCleanupInterval < 0 {
+		return nil, fmt.Errorf("ExpiryCleanupInterval cannot be negative, but got %v", cfg.ExpiryCleanupInterval)
 	}
-	if c.ProcessorReconciliationInterval < 0 {
-		return fmt.Errorf("ProcessorReconciliationInterval cannot be negative, but got %v",
-			c.ProcessorReconciliationInterval)
+	if cfg.ProcessorReconciliationInterval < 0 {
+		return nil, fmt.Errorf("ProcessorReconciliationInterval cannot be negative, but got %v",
+			cfg.ProcessorReconciliationInterval)
 	}
-	if c.EnqueueChannelBufferSize < 0 {
-		return fmt.Errorf("EnqueueChannelBufferSize cannot be negative, but got %d", c.EnqueueChannelBufferSize)
+	if cfg.EnqueueChannelBufferSize < 0 {
+		return nil, fmt.Errorf("EnqueueChannelBufferSize cannot be negative, but got %d", cfg.EnqueueChannelBufferSize)
 	}
 
 	// --- Defaulting ---
-	if c.ExpiryCleanupInterval == 0 {
-		c.ExpiryCleanupInterval = defaultExpiryCleanupInterval
+	if cfg.ExpiryCleanupInterval == 0 {
+		cfg.ExpiryCleanupInterval = defaultExpiryCleanupInterval
 	}
-	if c.ProcessorReconciliationInterval == 0 {
-		c.ProcessorReconciliationInterval = defaultProcessorReconciliationInterval
+	if cfg.ProcessorReconciliationInterval == 0 {
+		cfg.ProcessorReconciliationInterval = defaultProcessorReconciliationInterval
 	}
-	if c.EnqueueChannelBufferSize == 0 {
-		c.EnqueueChannelBufferSize = defaultEnqueueChannelBufferSize
+	if cfg.EnqueueChannelBufferSize == 0 {
+		cfg.EnqueueChannelBufferSize = defaultEnqueueChannelBufferSize
 	}
-	return nil
+	return cfg, nil
 }
 
 // deepCopy creates a deep copy of the `Config` object.

pkg/epp/flowcontrol/controller/config_test.go

@@ -24,7 +24,7 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-func TestNewConfig(t *testing.T) {
+func TestConfig_ValidateAndApplyDefaults(t *testing.T) {
 	t.Parallel()
 
 	testCases := []struct {
@@ -88,7 +88,7 @@ func TestNewConfig(t *testing.T) {
 		t.Run(tc.name, func(t *testing.T) {
 			t.Parallel()
 			originalInput := tc.input.deepCopy()
-			validatedCfg, err := newConfig(tc.input)
+			validatedCfg, err := tc.input.ValidateAndApplyDefaults()
 
 			if tc.expectErr {
 				require.Error(t, err, "expected an error but got nil")

pkg/epp/flowcontrol/controller/controller.go

@@ -118,13 +118,8 @@ func NewFlowController(
 	logger logr.Logger,
 	opts ...flowControllerOption,
 ) (*FlowController, error) {
-	validatedConfig, err := newConfig(config)
-	if err != nil {
-		return nil, fmt.Errorf("invalid flow controller configuration: %w", err)
-	}
-
 	fc := &FlowController{
-		config:             *validatedConfig,
+		config:             *config.deepCopy(),
 		registry:           registry,
 		saturationDetector: sd,
 		clock:              clock.RealClock{},

pkg/epp/flowcontrol/controller/controller_test.go

@@ -273,23 +273,6 @@ func newTestRequest(ctx context.Context, key types.FlowKey) *typesmocks.MockFlow
 
 // --- Test Cases ---
 
-func TestNewFlowController(t *testing.T) {
-	t.Parallel()
-
-	t.Run("ErrorOnInvalidConfig", func(t *testing.T) {
-		t.Parallel()
-		invalidCfg := Config{ProcessorReconciliationInterval: -1 * time.Second}
-		_, err := NewFlowController(
-			context.Background(),
-			invalidCfg,
-			&mockRegistryClient{},
-			&mocks.MockSaturationDetector{},
-			logr.Discard(),
-		)
-		require.Error(t, err, "NewFlowController must return an error for invalid configuration")
-	})
-}
-
 func TestFlowController_EnqueueAndWait(t *testing.T) {
 	t.Parallel()
 

pkg/epp/flowcontrol/registry/config.go

@@ -112,7 +112,7 @@ type Config struct {
 // that operate at this priority level.
 type PriorityBandConfig struct {
 	// Priority is the unique numerical priority level for this band.
-	// Convention: Lower numerical values indicate lower priority.
+	// Convention: Highest numeric value corresponds to highest priority (centered on 0).
 	// Required.
 	Priority int
 
@@ -140,20 +140,6 @@ type PriorityBandConfig struct {
 	MaxBytes uint64
 }
 
-// NewConfig performs validation and initialization, returning a guaranteed-valid `Config` object.
-// This is the required constructor for creating a new configuration. It applies provided functional options (primarily
-// for testing) and does not mutate the input `cfg`.
-func NewConfig(cfg Config, opts ...configOption) (*Config, error) {
-	newCfg := cfg.deepCopy()
-	for _, opt := range opts {
-		opt(newCfg)
-	}
-	if err := newCfg.validateAndApplyDefaults(); err != nil {
-		return nil, err
-	}
-	return newCfg, nil
-}
-
 // =============================================================================
 // Shard-Level Configuration
 // =============================================================================
@@ -205,52 +191,55 @@ func (sc *ShardConfig) getBandConfig(priority int) (*ShardPriorityBandConfig, er
 
 // --- Validation and Defaulting ---
 
-// validateAndApplyDefaults checks the global configuration for validity (including plugin compatibility) and mutates
-// the receiver to populate any empty fields with system defaults. It also initializes internal lookup maps.
-func (c *Config) validateAndApplyDefaults() error {
+// ValidateAndApplyDefaults checks the global configuration for validity and then creates a new `Config` object,
+// populating any empty fields with system defaults.
+// It does not mutate the receiver.
+func (c *Config) ValidateAndApplyDefaults() (*Config, error) {
+	cfg := c.deepCopy()
+
 	// Apply defaults to top-level fields.
-	if c.InitialShardCount <= 0 {
-		c.InitialShardCount = defaultInitialShardCount
+	if cfg.InitialShardCount <= 0 {
+		cfg.InitialShardCount = defaultInitialShardCount
 	}
-	if c.FlowGCTimeout <= 0 {
-		c.FlowGCTimeout = defaultFlowGCTimeout
+	if cfg.FlowGCTimeout <= 0 {
+		cfg.FlowGCTimeout = defaultFlowGCTimeout
 	}
-	if c.EventChannelBufferSize <= 0 {
-		c.EventChannelBufferSize = defaultEventChannelBufferSize
+	if cfg.EventChannelBufferSize <= 0 {
+		cfg.EventChannelBufferSize = defaultEventChannelBufferSize
 	}
 
 	// Ensure the DI factories are initialized for production use if `NewConfig` was called without options.
-	if c.interFlowDispatchPolicyFactory == nil {
-		c.interFlowDispatchPolicyFactory = inter.NewPolicyFromName
+	if cfg.interFlowDispatchPolicyFactory == nil {
+		cfg.interFlowDispatchPolicyFactory = inter.NewPolicyFromName
 	}
-	if c.intraFlowDispatchPolicyFactory == nil {
-		c.intraFlowDispatchPolicyFactory = intra.NewPolicyFromName
+	if cfg.intraFlowDispatchPolicyFactory == nil {
+		cfg.intraFlowDispatchPolicyFactory = intra.NewPolicyFromName
 	}
-	if c.queueFactory == nil {
-		c.queueFactory = queue.NewQueueFromName
+	if cfg.queueFactory == nil {
+		cfg.queueFactory = queue.NewQueueFromName
 	}
 
-	if len(c.PriorityBands) == 0 {
-		return errors.New("config validation failed: at least one priority band must be defined")
+	if len(cfg.PriorityBands) == 0 {
+		return nil, errors.New("config validation failed: at least one priority band must be defined")
 	}
 
 	// Validate and default each priority band.
 	priorities := make(map[int]struct{})
 	priorityNames := make(map[string]struct{})
-	c.priorityBandMap = make(map[int]*PriorityBandConfig, len(c.PriorityBands))
+	cfg.priorityBandMap = make(map[int]*PriorityBandConfig, len(cfg.PriorityBands))
 
-	for i := range c.PriorityBands {
-		band := &c.PriorityBands[i]
+	for i := range cfg.PriorityBands {
+		band := &cfg.PriorityBands[i]
 		if _, exists := priorities[band.Priority]; exists {
-			return fmt.Errorf("config validation failed: duplicate priority level %d found", band.Priority)
+			return nil, fmt.Errorf("config validation failed: duplicate priority level %d found", band.Priority)
 		}
 		priorities[band.Priority] = struct{}{}
 
 		if band.PriorityName == "" {
-			return fmt.Errorf("config validation failed: PriorityName is required for priority band %d", band.Priority)
+			return nil, fmt.Errorf("config validation failed: PriorityName is required for priority band %d", band.Priority)
 		}
 		if _, exists := priorityNames[band.PriorityName]; exists {
-			return fmt.Errorf("config validation failed: duplicate priority name %q found", band.PriorityName)
+			return nil, fmt.Errorf("config validation failed: duplicate priority name %q found", band.PriorityName)
 		}
 		priorityNames[band.PriorityName] = struct{}{}
 
@@ -267,12 +256,12 @@ func (c *Config) validateAndApplyDefaults() error {
 			band.MaxBytes = defaultPriorityBandMaxBytes
 		}
 
-		if err := c.validateBandCompatibility(*band); err != nil {
-			return err
+		if err := cfg.validateBandCompatibility(*band); err != nil {
+			return nil, err
 		}
-		c.priorityBandMap[band.Priority] = band
+		cfg.priorityBandMap[band.Priority] = band
 	}
-	return nil
+	return cfg, nil
 }
 
 // validateBandCompatibility verifies that a band's configured queue type has the necessary capabilities.
@@ -423,6 +412,18 @@ func withQueueFactory(factory queueFactory) configOption {
 	}
 }
 
+// newConfig creates a new validated and defaulted `Config` object.
+// It applies provided test-only functional options before validation and defaulting.
+// It does not mutate the input `cfg`.
+// test-only
+func newConfig(cfg Config, opts ...configOption) (*Config, error) {
+	newCfg := cfg.deepCopy()
+	for _, opt := range opts {
+		opt(newCfg)
+	}
+	return newCfg.ValidateAndApplyDefaults()
+}
+
 // --- Internal Utilities ---
 
 // deepCopy creates a deep copy of the `Config` object.
@@ -436,7 +437,6 @@ func (c *Config) deepCopy() *Config {
 		FlowGCTimeout:                  c.FlowGCTimeout,
 		EventChannelBufferSize:         c.EventChannelBufferSize,
 		PriorityBands:                  make([]PriorityBandConfig, len(c.PriorityBands)),
-		priorityBandMap:                make(map[int]*PriorityBandConfig, len(c.PriorityBands)),
 		interFlowDispatchPolicyFactory: c.interFlowDispatchPolicyFactory,
 		intraFlowDispatchPolicyFactory: c.intraFlowDispatchPolicyFactory,
 		queueFactory:                   c.queueFactory,
@@ -445,11 +445,14 @@ func (c *Config) deepCopy() *Config {
 	// PriorityBandConfig contains only value types, so a slice copy is sufficient for a deep copy.
 	copy(newCfg.PriorityBands, c.PriorityBands)
 
-	// Crucial: We must rebuild the map and take the address of the elements within the new slice (`newCfg.PriorityBands`)
-	// to ensure the map pointers are correct for the newly created `Config` instance.
-	for i := range newCfg.PriorityBands {
-		band := &newCfg.PriorityBands[i]
-		newCfg.priorityBandMap[band.Priority] = band
+	if c.priorityBandMap != nil {
+		newCfg.priorityBandMap = make(map[int]*PriorityBandConfig, len(c.PriorityBands))
+		// Crucial: We must rebuild the map and take the address of the elements within the new slice (`newCfg.PriorityBands`)
+		// to ensure the map pointers are correct for the newly created `Config` instance.
+		for i := range newCfg.PriorityBands {
+			band := &newCfg.PriorityBands[i]
+			newCfg.priorityBandMap[band.Priority] = band
+		}
 	}
 	return newCfg
 }

pkg/epp/flowcontrol/registry/config_test.go

@@ -35,7 +35,7 @@ import (
 	"sigs.k8s.io/gateway-api-inference-extension/pkg/epp/flowcontrol/framework/plugins/queue/listqueue"
 )
 
-func TestConfig_NewConfig(t *testing.T) {
+func TestConfig_ValidateAndApplyDefaults(t *testing.T) {
 	t.Parallel()
 
 	testCases := []struct {
@@ -286,28 +286,32 @@ func TestConfig_NewConfig(t *testing.T) {
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
 			t.Parallel()
-			originalInputCopy := tc.input.deepCopy()
-			newCfg, err := NewConfig(tc.input, tc.opts...)
+			originalInput := tc.input.deepCopy()
+			validatedCfg, err := newConfig(tc.input, tc.opts...)
+
 			if tc.expectErr {
-				require.Error(t, err, "NewConfig should have returned an error")
+				require.Error(t, err, "expected an error but got nil")
 				if tc.expectedErrIs != nil {
-					assert.ErrorIs(t, err, tc.expectedErrIs, "Error should wrap the expected error type")
+					assert.ErrorIs(t, err, tc.expectedErrIs, "error should wrap the expected error type")
 				}
-				assert.Nil(t, newCfg, "On error, the returned config should be nil")
+				assert.Nil(t, validatedCfg, "validatedCfg should be nil on error")
 			} else {
-				require.NoError(t, err, "NewConfig should not have returned an error")
-				require.NotNil(t, newCfg, "On success, the returned config should not be nil")
+				require.NoError(t, err, "expected no error but got: %v", err)
+				require.NotNil(t, validatedCfg, "validatedCfg should not be nil on success")
 				if tc.assertion != nil {
-					tc.assertion(t, *originalInputCopy, newCfg)
+					tc.assertion(t, *originalInput, validatedCfg)
 				}
+
+				// Ensure the original config is not mutated.
+				assert.Equal(t, *originalInput, tc.input, "input config should not be mutated")
 			}
 		})
 	}
 }
 
 func TestConfig_Partition(t *testing.T) {
 	t.Parallel()
-	baseCfg, err := NewConfig(Config{
+	baseCfg, err := newConfig(Config{
 		MaxBytes: 103, // Will not distribute evenly
 		PriorityBands: []PriorityBandConfig{
 			{Priority: 1, PriorityName: "High", MaxBytes: 55}, // Will not distribute evenly
@@ -391,7 +395,7 @@ func TestConfig_Partition(t *testing.T) {
 
 func TestConfig_GetBandConfig(t *testing.T) {
 	t.Parallel()
-	cfg, err := NewConfig(Config{
+	cfg, err := newConfig(Config{
 		PriorityBands: []PriorityBandConfig{
 			{Priority: 10, PriorityName: "High"},
 		},
@@ -427,7 +431,7 @@ func TestConfig_DeepCopy(t *testing.T) {
 		},
 	}
 	// Create a fully initialized "original" config to be the source of the copy.
-	original, err := NewConfig(baseCfg)
+	original, err := newConfig(baseCfg)
 	require.NoError(t, err, "Setup for deep copy should not fail")
 
 	t.Run("ShouldReturnNil_ForNilReceiver", func(t *testing.T) {
@@ -481,7 +485,7 @@ func TestConfig_DeepCopy(t *testing.T) {
 
 func TestShardConfig_GetBandConfig(t *testing.T) {
 	t.Parallel()
-	baseCfg, err := NewConfig(Config{
+	baseCfg, err := newConfig(Config{
 		PriorityBands: []PriorityBandConfig{
 			{Priority: 10, PriorityName: "High"},
 			{Priority: 20, PriorityName: "Low"},