Skip to content

fix: validate that fillInterval must not be '0s' #11619

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

MariamFahmy98
Copy link
Contributor

Description

This PR adds a validation check to fillInterval so that it must not be set to '0s'.
Closes #11560

Change Type

/kind bug_fix

Changelog

None

Additional Notes

@github-actions github-actions bot added kind/fix Categorizes issue or PR as related to a bug. release-note-none labels Jul 8, 2025
Copy link
Member

@timflannagan timflannagan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking a stab at this and fixing the other bug related to the local rate limit API. Had a question/comment about whether this proposed rule is sufficient to catch all the gwv1.Duration possible fields.

@@ -278,6 +278,7 @@ type TokenBucket struct {
// This value must be a valid duration string (e.g., "1s", "500ms").
// It determines the frequency of token replenishment.
// +required
// +kubebuilder:validation:XValidation:rule="self != '0s'",message="fillInterval must not be '0s'"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this catch other zero intervals being provided, e.g. "0h", "0m", etc? I'm looking at the

fillInterval, err := time.ParseDuration(string(t.TokenBucket.FillInterval))
if err != nil {
return nil, err
}
and I think Envoy's local rate limit protobuf validation would still flag this configuration. See https://go.dev/play/p/E_k6jfT12BU for an easy reproducer.

I think the following rule could catch those edge cases:

+kubebuilder:validation:XValidation:rule="duration(self) > duration('0s')",message="fillInterval must not be '0s'"

We can validate by manually testing || adding a test case in the api_validation_test.go suite.

FYI @shashankram using gwv1.Duration as the underlying type instead of metav1.Duration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/fix Categorizes issue or PR as related to a bug. release-note-none
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve TokenBucket fillInterval validation
2 participants