Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
164 changes: 164 additions & 0 deletions api/v1alpha1/ratelimitfilter_types.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,164 @@
// Copyright Envoy Gateway Authors
// SPDX-License-Identifier: Apache-2.0
// The full text of the Apache license is available in the LICENSE file at
// the root of the repo.

package v1alpha1

import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

// +kubebuilder:object:root=true
// +kubebuilder:printcolumn:name="Age",type=date,JSONPath=`.metadata.creationTimestamp`

// RateLimitFilter allows the user to limit the number of incoming requests
// to a predefined value based on attributes within the traffic flow.
type RateLimitFilter struct {
metav1.TypeMeta `json:",inline"`
metav1.ObjectMeta `json:"metadata,omitempty"`

// Spec defines the desired state of RateLimitFilter.
Spec RateLimitFilterSpec `json:"spec"`
}

// +kubebuilder:object:root=true

// RateLimitFilterList contains a list of RateLimitFilter resources.
type RateLimitList struct {
metav1.TypeMeta `json:",inline"`
metav1.ListMeta `json:"metadata,omitempty"`
Items []RateLimitFilter `json:"items"`
}

// RateLimitFilterSpec defines the desired state of RateLimitFilter.
// +union
type RateLimitFilterSpec struct {
// Type decides the scope for the RateLimits.
// Valid RateLimitType values are:
//
// * "Global" - In this mode, the rate limits are applied across all Envoy proxy instances.
//
// +unionDiscriminator
Type RateLimitType `json:"type"`
// Global rate limit configuration.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It should be noted in the godocs that the global field is only applicable for type Global. I understand that Global is the only type currently supported but this will prevent confusion when other types, e.g. Local, are supported in the future. @arkodg please note this in the follow-on issue or @kflynn please resolve this in your PR.

//
// +optional
Global *GlobalRateLimit `json:"global,omitempty"`
}

// RateLimitType specifies the types of RateLimiting.
// +kubebuilder:validation:Enum=Global
type RateLimitType string

const (
// GlobalRateLimitType allows the rate limits to be applied across all Envoy proxy instances.
GlobalRateLimitType RateLimitType = "Global"
)

// GlobalRateLimit defines the global rate limit configuration.
type GlobalRateLimit struct {
// Rules are a list of RateLimit selectors and limits.
// Each rule and its associated limit is applied
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

wording suggestion/assumption on what youre trying to convey here:

Multiple rules may match a request. The effective rate limit applied to a request is the computed aggregate of the rate limits configured in these matching rules.

Once there are docs etc. might be good to point to something giving an example, e.g. if you have two rules with header value matches that overlap, the stricter rate limit will win

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@sunjayBhatia this understanding is incorrect, if there are multiple rules e.g.

rules:
- matches:
  - headers
       name: :method
       value: GET
  limit:
    requests: 2
    unit: SECOND
- matches:
  - headers
       name: :authority
       value: example.com
  limit:
    requests: 2
    unit: HOUR          

lets assume match holds true, both will get applied simultaneously in a mutually exclusive way

Copy link
Copy Markdown
Contributor Author

@arkodg arkodg Dec 20, 2022

Choose a reason for hiding this comment

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

the difference between a HTTPRoute rule is, the request must go to some/one backend, so the top level is ORed(or some precedence is added here), here the RateLimit rule and its limit can apply to any/all

Copy link
Copy Markdown
Member

@sunjayBhatia sunjayBhatia Dec 20, 2022

Choose a reason for hiding this comment

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

if one of those was 2/hour and the other 4/hour, which one would be applied to the 3rd GET within an hour to example.com? would the request be rate limited or not? If i remember right from experience with Contour etc. that it would be due to the 2/hour limit (assuming different descriptors were generated)

Copy link
Copy Markdown
Contributor Author

@arkodg arkodg Dec 20, 2022

Choose a reason for hiding this comment

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

both would i.e. the 4 requests would get matched to each of the rules, outcome is same, 2nd and 4th request gets rate limited, but the count is incremented for each rule

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

similar to how more specific matches or longer path prefixes etc. are documented to "win" in route matching, some documentation on what happens when you have multiple limits that apply to a request would be good to include

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this case captures what I'm saying: #706 (comment)

users should be notified that e.g. Alice and Bob will still be rate limited at 10RPS because that limit is more strict than what they think they may have configured for those specific users, the buckets are counted separately yes but when evaluating if a request has gone over any limits they are aggregated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Even before there are end-user docs, if the design goal is that the strictest match wins, the design doc should say that.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

but the goal is not strictest match wins, its any & all match win

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

yes, but effectively that means that if requests match multiple limiting rules, the buckets will fill up for the stricter limit first so users will see their requests rate limited according to that limit first

in practice I have seen users get very confused as to how things work when such overlapping rate limits are in place, thinking they shouldnt get rate limited on certain requests when their config says they should

// in a mutually exclusive way i.e. if multiple
// rules get selected, each of their associated
// limits get applied, so a single traffic request
// might increase the rate limit counters for multiple
// rules if selected.
Comment on lines +63 to +67
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@kflynn it would be appreciated if you can improve the godocs for this field.

//
// +kubebuilder:validation:MaxItems=16
Rules []RateLimitRule `json:"rules"`
}

// RateLimitRule defines the semantics for matching attributes
// from the incoming requests, and setting limits for them.
type RateLimitRule struct {
// ClientSelectors holds the list of select conditions to select
// specific clients using attributes from the traffic flow.
// All individual select conditions must hold True for this rule
// and its limit to be applied.
// If this field is empty, it is equivalent to True, and
// the limit is applied.
//
// +optional
// +kubebuilder:validation:MaxItems=8
ClientSelectors []RateLimitSelectCondition `json:"clientSelectors,omitempty"`
// Limit holds the rate limit values.
// This limit is applied for traffic flows when the selectors
// compute to True, causing the request to be counted towards the limit.
// The limit is enforced and the request is ratelimited, i.e. a response with
// 429 HTTP status code is sent back to the client when
// the selected requests have reached the limit.
Limit RateLimitValue `json:"limit"`
}

// RateLimitSelectCondition specifies the attributes within the traffic flow that can
// be used to select a subset of clients to be ratelimited.
// All the individual conditions must hold True for the overall condition to hold True.
type RateLimitSelectCondition struct {
// Headers is a list of request headers to match. Multiple header values are ANDed together,
// meaning, a request MUST match all the specified headers.
//
// +listType=map
// +listMapKey=name
// +optional
// +kubebuilder:validation:MaxItems=16
Headers []HeaderMatch `json:"headers,omitempty"`
}

// HeaderMatch defines the match attributes within the HTTP Headers of the request.
type HeaderMatch struct {
// Type specifies how to match against the value of the header.
//
// +optional
// +kubebuilder:default=Exact
Type *HeaderMatchType `json:"type,omitempty"`

// Name of the HTTP header.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we provide additional context to Name as Gateway API does, e.g. multiple rules with the same header name?

xref: https://github.com/kubernetes-sigs/gateway-api/blob/v0.6.0/apis/v1alpha2/grpcroute_types.go#L385-L389

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@kflynn ^ comment is another candidate to resolve in a follow-on PR.

// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=256
Name string `json:"name"`

// Value within the HTTP header. Due to the
// case-insensitivity of header names, "foo" and "Foo" are considered equivalent.
// Do not set this field when Type="Distinct", implying matching on any/all unique values within the header.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This comment can be removed if we drop the Distinct type. See https://github.com/envoyproxy/gateway/pull/706/files#r1054657720 for additional details.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Two of the three header matching types proposed in this PR are the same as upstream Gateway API. Do you know why Gateway API doesn't support the Distinct type? If not, it could be valuable to share our use case and get feedback.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

its naming as well as use case has already been upvoted by reviewers, so I feel strongly to keep it, and proactively present it to the users, and get feedback for this v1alpha1 version.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do you know why Gateway API doesn't support the Distinct type?

"Distinct" isn't really a matching mechanism per se, but rather denotes that each unique value for a header the RLS sees will get its own rate limiting bucket, which doesn't really fit into the idea of matching request attributes to funnel to a backend

// +optional
// +kubebuilder:validation:MaxLength=1024
Value *string `json:"value,omitempty"`
}

// HeaderMatchType specifies the semantics of how HTTP header values should be
// compared. Valid HeaderMatchType values are:
//
// - "Exact": Use this type to match the exact value of the Value field against the value of the specified HTTP Header.
// - "RegularExpression": Use this type to match a regular expression against the value of the specified HTTP Header.
// The regex string must adhere to the syntax documented in https://github.com/google/re2/wiki/Syntax.
// - "Distinct": Use this type to match any and all possible unique values encountered in the specified HTTP Header.
Copy link
Copy Markdown
Contributor

@danehans danehans Dec 21, 2022

Choose a reason for hiding this comment

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

It took me some time to understand the Distinct type. I have a feeling others will feel the same. What if we only supported Exact and RegularExpression? If Value is unspecified for Exact, then all values of name are selected.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

same as #706 (comment)

// Note that each unique value will receive its own rate limit bucket.
//
// +kubebuilder:validation:Enum=Exact;RegularExpression;Distinct
type HeaderMatchType string

// HeaderMatchType constants.
const (
HeaderMatchExact HeaderMatchType = "Exact"
HeaderMatchRegularExpression HeaderMatchType = "RegularExpression"
HeaderMatchDistinct HeaderMatchType = "Distinct"
)

// RateLimitValue defines the limits for rate limiting.
type RateLimitValue struct {
Requests uint `json:"requests"`
Unit RateLimitUnit `json:"unit"`
}

// RateLimitUnit specifies the intervals for setting rate limits.
// Valid RateLimitUnit values are:
//
// * "Second"
// * "Minute"
// * "Hour"
// * "Day"
//
// +kubebuilder:validation:Enum=Second;Minute;Hour;Day
type RateLimitUnit string
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@arkodg constants should be defined for the ^ enums. For example:

const (
	// SecondRateLimitUnit defines the "Second" rate limit unit.
	SecondRateLimitUnit RateLimitUnit = "Second"

	...
)

cc: @kflynn

185 changes: 185 additions & 0 deletions api/v1alpha1/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading