api: sharing global ratelimit buckets

[ryanhristovski]

Related to #5194 (comment)

Currently using shared: bool for setting a global rate limiting rule, another option discussed was scope: gateway/httproute.

Does anyone have a strong opinion on how rate limit policies should be labeled to have a global scope?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.00%. Comparing base (22eb14f) to head (ec32888).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5227      +/-   ##
==========================================
+ Coverage   67.94%   68.00%   +0.05%     
==========================================
  Files         214      214              
  Lines       33532    33532              
==========================================
+ Hits        22782    22802      +20     
+ Misses       9360     9345      -15     
+ Partials     1390     1385       -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[arkodg]

added some comments, but overall +1 with the feature
ptal @envoyproxy/gateway-maintainers

the implementation will be tricky, since we cannot rely on per route based keys and descriptors, and find a way to share it, one idea is to create a key prefix in the gateway api layer and use it in the xds layer if its set

[ryanhristovski]

/retest

[ryanhristovski]

/retest

[ryanhristovski]

@arkodg i fixed a test in this PR unrelated to my changes

[arkodg]

LGTM thanks !

[sanposhiho]

Using bool is not recommended generally speaking..
https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#primitive-types

Any strong reason for that? Can we consider enum instead?

[arkodg]

thanks for linking that @sanposhiho , it does say to think twice before using bool, mainly because intent/field could be multidimensional in the future (which enums like type really come in handy).
the intent here is to say that this ratelimit bucket is a singleton or is shared across all targets, can recommendations for enums here ?

[sanposhiho]

You can make a final decision based on other API designs in EG, but, at k/k (around me), we've mostly (or probably always) used enum for new APIs even if we first thought it was a two-value switching and likely won't get changed in the future when we designed them. So, if you ask me, I'd probably change it to enum and change the name from Shared. (some general name, like ApplyPolicy etc)

[arkodg]

I like @rudrakhp's suggestion #5227 (comment)

[rudrakhp]

Suggested change

	Shared *bool `json:"shared,omitempty"`
	BucketScope RateLimitBucketScope `json:"bucketScope,omitempty"`

You can define enums:

type RateLimitBucketScope string

const (
	SharedBucketScope    RateLimitBucketScope = "shared"    // Shared across routes
	PerRouteBucketScope  RateLimitBucketScope = "perRoute" // Independent for each route, default
)

[guydc]

+1 to future proof this API for other scopes (e.g. vhost?)

[arkodg]

prefer just scope
and instead of perRoute it may help to define it as perTarget

[rudrakhp]

Just scope might be confusing with the rate limit scope, i.e, Global/Local. But I guess that could be clarified in the documentation.

[ryanhristovski]

I could see the confusion with scope being confused with type (Global/Local), bucketScope could provide more clarity. Also agree with @arkodg for perTarget rather than perRoute.

I'll push through whichever changes we agree upon here

[arkodg]

i was trying to avoid the bucket prefix, because i was unsure if it should be bucket, rule (which is an existing noun in the API) or somethingElse
we can brainstorm a little more here

[rudrakhp]

How about "context" (values are shared, target, vHost, etc)? I am ok with "scope" as well since it's explicitly defined under GlobalRateLimit API, just that context feels less confusing.

[ryanhristovski]

though i see the potential confusion, scope: shared/target/vHost does seem most clear

can the vHost option be added in another patch? was hoping to just focus on shared/target

[arkodg]

Yeah we can't add vHost yet, because that noun doesn't exist in any user facing docs