Skip to content

Commit 2ea139c

Browse files
authored
refactor: flow control config (#1581)
* 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. * feat(flowcontrol): Add bundled Flow Control config Introduces a new top-level `Config` for the Flow Control layer. This config bundles the configurations for the `controller` and `registry` packages, providing a single, unified point of entry fo validation and default application. This simplifies the management and initialization of the flow control system by centralizing its configuration. * fix: Update controller tests with valid config The previous commit introduced a unified and validated configuration for the flow control system, requiring callers to pass a pre-validated cofig to the controller and registry respectively. This change updates the controller tests to provide a valid configuration instead of relying on the now-removed defaulting logic in the constructor.
1 parent 771dc5e commit 2ea139c

File tree

11 files changed

+245
-195
lines changed

11 files changed

+245
-195
lines changed

pkg/epp/flowcontrol/config.go

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
/*
2+
Copyright 2025 The Kubernetes Authors.
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
17+
package flowcontrol
18+
19+
import (
20+
"fmt"
21+
22+
"sigs.k8s.io/gateway-api-inference-extension/pkg/epp/flowcontrol/controller"
23+
"sigs.k8s.io/gateway-api-inference-extension/pkg/epp/flowcontrol/registry"
24+
)
25+
26+
// Config is the top-level configuration for the entire flow control module.
27+
// It embeds the configurations for the controller and the registry, providing a single point of entry for validation
28+
// and initialization.
29+
type Config struct {
30+
Controller controller.Config
31+
Registry registry.Config
32+
}
33+
34+
// ValidateAndApplyDefaults checks the configuration for validity and populates any empty fields with system defaults.
35+
// It delegates validation to the underlying controller and registry configurations.
36+
// It returns a new, validated `Config` object and does not mutate the receiver.
37+
func (c *Config) ValidateAndApplyDefaults() (*Config, error) {
38+
validatedControllerCfg, err := c.Controller.ValidateAndApplyDefaults()
39+
if err != nil {
40+
return nil, fmt.Errorf("controller config validation failed: %w", err)
41+
}
42+
validatedRegistryCfg, err := c.Registry.ValidateAndApplyDefaults()
43+
if err != nil {
44+
return nil, fmt.Errorf("registry config validation failed: %w", err)
45+
}
46+
return &Config{
47+
Controller: *validatedControllerCfg,
48+
Registry: *validatedRegistryCfg,
49+
}, nil
50+
}

pkg/epp/flowcontrol/config_test.go

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,91 @@
1+
/*
2+
Copyright 2025 The Kubernetes Authors.
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
17+
package flowcontrol
18+
19+
import (
20+
"testing"
21+
"time"
22+
23+
"github.com/stretchr/testify/assert"
24+
"github.com/stretchr/testify/require"
25+
"sigs.k8s.io/gateway-api-inference-extension/pkg/epp/flowcontrol/controller"
26+
"sigs.k8s.io/gateway-api-inference-extension/pkg/epp/flowcontrol/registry"
27+
)
28+
29+
func TestConfig_ValidateAndApplyDefaults(t *testing.T) {
30+
t.Parallel()
31+
32+
// A minimal valid registry config, which is required for the success case.
33+
validRegistryConfig := registry.Config{
34+
PriorityBands: []registry.PriorityBandConfig{
35+
{Priority: 1, PriorityName: "TestBand"},
36+
},
37+
}
38+
39+
testCases := []struct {
40+
name string
41+
input Config
42+
expectErr bool
43+
expectedErrIs error
44+
}{
45+
{
46+
name: "ShouldSucceed_WhenSubConfigsAreValid",
47+
input: Config{
48+
Controller: controller.Config{},
49+
Registry: validRegistryConfig,
50+
},
51+
expectErr: false,
52+
},
53+
{
54+
name: "ShouldFail_WhenControllerConfigIsInvalid",
55+
input: Config{
56+
Controller: controller.Config{
57+
DefaultRequestTTL: -1 * time.Second,
58+
},
59+
Registry: validRegistryConfig,
60+
},
61+
expectErr: true,
62+
},
63+
{
64+
name: "ShouldFail_WhenRegistryConfigIsInvalid",
65+
input: Config{
66+
Controller: controller.Config{},
67+
Registry: registry.Config{
68+
PriorityBands: []registry.PriorityBandConfig{},
69+
},
70+
},
71+
expectErr: true,
72+
},
73+
}
74+
75+
for _, tc := range testCases {
76+
t.Run(tc.name, func(t *testing.T) {
77+
t.Parallel()
78+
originalInput := tc.input
79+
validatedCfg, err := tc.input.ValidateAndApplyDefaults()
80+
81+
if tc.expectErr {
82+
require.Error(t, err, "expected an error but got nil")
83+
} else {
84+
require.NoError(t, err, "expected no error but got: %v", err)
85+
require.NotNil(t, validatedCfg, "validatedCfg should not be nil on success")
86+
}
87+
88+
assert.Equal(t, originalInput, tc.input, "input config should not be mutated")
89+
})
90+
}
91+
}

pkg/epp/flowcontrol/controller/config.go

Lines changed: 21 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -53,46 +53,38 @@ type Config struct {
5353
EnqueueChannelBufferSize int
5454
}
5555

56-
// newConfig performs validation and initialization, returning a guaranteed-valid `Config` object.
57-
// This is the required constructor for creating a new configuration.
58-
// It does not mutate the input `cfg`.
59-
func newConfig(cfg Config) (*Config, error) {
60-
newCfg := cfg.deepCopy()
61-
if err := newCfg.validateAndApplyDefaults(); err != nil {
62-
return nil, err
63-
}
64-
return newCfg, nil
65-
}
56+
// ValidateAndApplyDefaults checks the global configuration for validity and then creates a new `Config` object,
57+
// populating any empty fields with system defaults.
58+
// It does not mutate the receiver.
59+
func (c *Config) ValidateAndApplyDefaults() (*Config, error) {
60+
cfg := c.deepCopy()
6661

67-
// validateAndApplyDefaults checks the global configuration for validity and then mutates the receiver to populate any
68-
// empty fields with system defaults.
69-
func (c *Config) validateAndApplyDefaults() error {
7062
// --- Validation ---
71-
if c.DefaultRequestTTL < 0 {
72-
return fmt.Errorf("DefaultRequestTTL cannot be negative, but got %v", c.DefaultRequestTTL)
63+
if cfg.DefaultRequestTTL < 0 {
64+
return nil, fmt.Errorf("DefaultRequestTTL cannot be negative, but got %v", cfg.DefaultRequestTTL)
7365
}
74-
if c.ExpiryCleanupInterval < 0 {
75-
return fmt.Errorf("ExpiryCleanupInterval cannot be negative, but got %v", c.ExpiryCleanupInterval)
66+
if cfg.ExpiryCleanupInterval < 0 {
67+
return nil, fmt.Errorf("ExpiryCleanupInterval cannot be negative, but got %v", cfg.ExpiryCleanupInterval)
7668
}
77-
if c.ProcessorReconciliationInterval < 0 {
78-
return fmt.Errorf("ProcessorReconciliationInterval cannot be negative, but got %v",
79-
c.ProcessorReconciliationInterval)
69+
if cfg.ProcessorReconciliationInterval < 0 {
70+
return nil, fmt.Errorf("ProcessorReconciliationInterval cannot be negative, but got %v",
71+
cfg.ProcessorReconciliationInterval)
8072
}
81-
if c.EnqueueChannelBufferSize < 0 {
82-
return fmt.Errorf("EnqueueChannelBufferSize cannot be negative, but got %d", c.EnqueueChannelBufferSize)
73+
if cfg.EnqueueChannelBufferSize < 0 {
74+
return nil, fmt.Errorf("EnqueueChannelBufferSize cannot be negative, but got %d", cfg.EnqueueChannelBufferSize)
8375
}
8476

8577
// --- Defaulting ---
86-
if c.ExpiryCleanupInterval == 0 {
87-
c.ExpiryCleanupInterval = defaultExpiryCleanupInterval
78+
if cfg.ExpiryCleanupInterval == 0 {
79+
cfg.ExpiryCleanupInterval = defaultExpiryCleanupInterval
8880
}
89-
if c.ProcessorReconciliationInterval == 0 {
90-
c.ProcessorReconciliationInterval = defaultProcessorReconciliationInterval
81+
if cfg.ProcessorReconciliationInterval == 0 {
82+
cfg.ProcessorReconciliationInterval = defaultProcessorReconciliationInterval
9183
}
92-
if c.EnqueueChannelBufferSize == 0 {
93-
c.EnqueueChannelBufferSize = defaultEnqueueChannelBufferSize
84+
if cfg.EnqueueChannelBufferSize == 0 {
85+
cfg.EnqueueChannelBufferSize = defaultEnqueueChannelBufferSize
9486
}
95-
return nil
87+
return cfg, nil
9688
}
9789

9890
// deepCopy creates a deep copy of the `Config` object.

pkg/epp/flowcontrol/controller/config_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ import (
2424
"github.com/stretchr/testify/require"
2525
)
2626

27-
func TestNewConfig(t *testing.T) {
27+
func TestConfig_ValidateAndApplyDefaults(t *testing.T) {
2828
t.Parallel()
2929

3030
testCases := []struct {
@@ -88,7 +88,7 @@ func TestNewConfig(t *testing.T) {
8888
t.Run(tc.name, func(t *testing.T) {
8989
t.Parallel()
9090
originalInput := tc.input.deepCopy()
91-
validatedCfg, err := newConfig(tc.input)
91+
validatedCfg, err := tc.input.ValidateAndApplyDefaults()
9292

9393
if tc.expectErr {
9494
require.Error(t, err, "expected an error but got nil")

pkg/epp/flowcontrol/controller/controller.go

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -118,13 +118,8 @@ func NewFlowController(
118118
logger logr.Logger,
119119
opts ...flowControllerOption,
120120
) (*FlowController, error) {
121-
validatedConfig, err := newConfig(config)
122-
if err != nil {
123-
return nil, fmt.Errorf("invalid flow controller configuration: %w", err)
124-
}
125-
126121
fc := &FlowController{
127-
config: *validatedConfig,
122+
config: *config.deepCopy(),
128123
registry: registry,
129124
saturationDetector: sd,
130125
clock: clock.RealClock{},

pkg/epp/flowcontrol/controller/controller_test.go

Lines changed: 1 addition & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -273,23 +273,6 @@ func newTestRequest(ctx context.Context, key types.FlowKey) *typesmocks.MockFlow
273273

274274
// --- Test Cases ---
275275

276-
func TestNewFlowController(t *testing.T) {
277-
t.Parallel()
278-
279-
t.Run("ErrorOnInvalidConfig", func(t *testing.T) {
280-
t.Parallel()
281-
invalidCfg := Config{ProcessorReconciliationInterval: -1 * time.Second}
282-
_, err := NewFlowController(
283-
context.Background(),
284-
invalidCfg,
285-
&mockRegistryClient{},
286-
&mocks.MockSaturationDetector{},
287-
logr.Discard(),
288-
)
289-
require.Error(t, err, "NewFlowController must return an error for invalid configuration")
290-
})
291-
}
292-
293276
func TestFlowController_EnqueueAndWait(t *testing.T) {
294277
t.Parallel()
295278

@@ -813,6 +796,7 @@ func TestFlowController_Concurrency(t *testing.T) {
813796
// Use a generous buffer to prevent flakes in the test due to transient queuing delays.
814797
EnqueueChannelBufferSize: numRequests,
815798
DefaultRequestTTL: 1 * time.Second,
799+
ExpiryCleanupInterval: 100 * time.Millisecond,
816800
}, mockRegistry)
817801

818802
var wg sync.WaitGroup

0 commit comments

Comments
 (0)