[nginx-ingress-controller] Add support for rate limiting in ingress rule locations #1093
Conversation
aledbf
force-pushed
the
rate-limiting
branch
3 times, most recently
from
389fc30
to
8b0add6
Compare
| rl, err := ratelimit.ParseAnnotations(ing) | ||
| glog.V(3).Infof("nginx rate limit %v", rl) | ||
| if err != nil { | ||
| glog.V(3).Infof("error reading rate limit annotation in Ingress %v/%v: %v", ing.GetNamespace(), ing.GetName(), err) |
There was a problem hiding this comment.
things like this feel event worthy, if it's easy enough to send an event. If the user thought they specified the right annotations but didn't, they should know their backend isn't getting ratelimited without having to debug a bunch and look around in the logs
|
Mostly similar comments to those in the rewrite pr (#1079). I think we should err on the side of caution and leave as many explicit examples and send as many events as we can, when exposing site specific features through annotations, because it's so easy to get wrong. Both from a user perspective, and from a cross-platform perspective (i.e I look at how the nginx controller is handling redirects to make sure it's the same for other platforms) |
aledbf
changed the title
|
LGTM but you have a merge conflict |
|
done |
| @@ -156,6 +156,12 @@ http { | |||
| } | |||
| {{ end }} | |||
|
|
|||
| {{/* build all the required rate limit zones. Each annotation requires a dedicated zone */}} | |||
| {{/* 1MB -> 16 thousand 64-byte states or about 8 thousand 128-byte states */}} | |||
| {{ $zone := range (buildRateLimitZones .servers) }} | |||
There was a problem hiding this comment.
I'm a bit late but shouldn't this be {{ range $zone := (buildRateLimitZones .servers) }}?
Should I make a PR?
There was a problem hiding this comment.
@louis-paul this is fixed in #1104.
(just in case this PR is not published yet)
No description provided.