refactor: flow control config #1581

LukeAVanDrie · 2025-09-13T00:31:12Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

This PR refactors the configuration handling within the pkg/epp/flowcontrol package and its sub-packages (controller, registry) to improve robustness, consistency, and clarity.

The core of the refactoring replaces the newConfig factory function pattern with a public ValidateAndApplyDefaults method on the configuration structs. This change:

Makes the validation and defaulting logic more explicit and discoverable.
Promotes immutability by returning a new, validated configuration object instead of mutating the receiver.
Simplifies the constructors for FlowController and FlowRegistry, as they now expect a pre-validated configuration.
Introduces a new top-level flowcontrol.Config to provide a single, unified entry point for the entire module's configuration (simplifies wiring this layer into the rest of EPP).

Overall, these changes lead to a more maintainable and less error-prone configuration system.

Which issue(s) this PR fixes:

Tracks #674

Does this PR introduce a user-facing change?:

NONE

cc @rahulgurnani

k8s-ci-robot · 2025-09-13T00:31:22Z

Hi @LukeAVanDrie. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

netlify · 2025-09-13T00:31:23Z

✅ Deploy Preview for gateway-api-inference-extension ready!

Name	Link
🔨 Latest commit	`b94dd82`
🔍 Latest deploy log	https://app.netlify.com/projects/gateway-api-inference-extension/deploys/68c8429a3560c00008728252
😎 Deploy Preview	https://deploy-preview-1581--gateway-api-inference-extension.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

ahg-g · 2025-09-15T15:58:23Z

/ok-to-test

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.

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.

LukeAVanDrie · 2025-09-15T16:24:26Z

pkg/epp/flowcontrol/config.go

+
+// ValidateAndApplyDefaults checks the configuration for validity and populates any empty fields with system defaults.
+// It delegates validation to the underlying controller and registry configurations.
+// It returns a new, validated `Config` object and does not mutate the receiver.


@kfswain Let's continue our discussion from the previous controller PR here regarding the config defaulting pattern.

// It returns a new, validated Config object and does not mutate the receiver.

My preference is to keep functions pure. It's easier to test and has no side effects. You could argue that this is better expressed as ValidateAndApplyDefaults(cfg Config) (*Config, error). You can also argue that my existing implementation should mutate the receiver.

Or even that validation and defaulting should be split.

I am not strongly opinionated here. We should just decide on a consistent pattern.

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.

ahg-g · 2025-09-15T20:33:44Z

pkg/epp/flowcontrol/controller/config.go

 	}

 	// --- Defaulting ---
-	if c.ExpiryCleanupInterval == 0 {
-		c.ExpiryCleanupInterval = defaultExpiryCleanupInterval
+	if cfg.ExpiryCleanupInterval == 0 {


nit: in k8s, object defaulting is done before validation, consider having a similar sequence here.

In this case, would we default invalid values (e.g., a negative duration) to the default silently?

if we run validation after defaulting, we protect against that, right?

Yeah; I am just thinking about UX. Say we wire this up to the text-based config. A user specified an invalid byte limit for queue capacity (e.g., outside of a min-max range of 10mb to 1gb -- not something we enforce yet, but meant as an illustrative example). Is it better to default an invalid value (e.g., 5 mb) to the min (possibly logging a warning) or just fail immediately?

Either way is safe. It's just a matter of failing fast and loud vs silently (or transparently) correcting.

Whenever I connect this to the config surface, I can revisit this based on your feedback. Either approach sgtm. For now, there is no way to specify non-default values anyways.

ahg-g · 2025-09-15T20:36:10Z

pkg/epp/flowcontrol/registry/config.go

+	for _, opt := range opts {
+		opt(newCfg)
+	}
+	return newCfg.ValidateAndApplyDefaults()


why are we calling this here again if it was called by the top level ValidateAndApplyDefaults in the flowcontrol/config pkg?

newConfig is a test-only helper for generating a valid, defaulted config for the registry tests. I can move this into registry_test.go as that is the only place it is called. It's purpose is to set the test-only variadic options for overriding the policy/queue factories.

It is also likely that once I update the flow control plugin factories to follow the scheduler pattern, instantiation will be done outside the registry and these will no longer be necessary.

Oh, this is also not a new function by the way. I just un-exported it and moved it toward the bottom of the file.

sounds good, missed the test-only comment.

ahg-g · 2025-09-15T23:56:11Z

/lgtm
/approve

k8s-ci-robot · 2025-09-15T23:56:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahg-g, LukeAVanDrie

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [ahg-g]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 13, 2025

k8s-ci-robot requested review from elevran and kfswain September 13, 2025 00:31

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 13, 2025

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 13, 2025

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 15, 2025

LukeAVanDrie added 2 commits September 15, 2025 16:11

LukeAVanDrie force-pushed the refactor/flow-control-config branch from 185cc02 to 4fad1a9 Compare September 15, 2025 16:13

LukeAVanDrie commented Sep 15, 2025

View reviewed changes

ahg-g reviewed Sep 15, 2025

View reviewed changes

k8s-ci-robot assigned ahg-g Sep 15, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 15, 2025

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 15, 2025

k8s-ci-robot merged commit 2ea139c into kubernetes-sigs:main Sep 16, 2025
13 checks passed

refactor: flow control config #1581

refactor: flow control config #1581

Conversation

LukeAVanDrie commented Sep 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Sep 13, 2025

Uh oh!

netlify bot commented Sep 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for gateway-api-inference-extension ready!

Uh oh!

ahg-g commented Sep 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LukeAVanDrie Sep 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LukeAVanDrie Sep 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahg-g commented Sep 15, 2025

Uh oh!

k8s-ci-robot commented Sep 15, 2025

Uh oh!

Uh oh!

Uh oh!

LukeAVanDrie commented Sep 13, 2025 •

edited

Loading

netlify bot commented Sep 13, 2025 •

edited

Loading

LukeAVanDrie Sep 15, 2025 •

edited

Loading

LukeAVanDrie Sep 15, 2025 •

edited

Loading