Skip to content
This repository has been archived by the owner on Apr 17, 2019. It is now read-only.

[nginx-ingress-controller] Add support for rate limiting in ingress rule locations #1093

Merged
merged 2 commits into from
May 31, 2016

Conversation

aledbf
Copy link
Contributor

@aledbf aledbf commented May 27, 2016

No description provided.

@aledbf aledbf force-pushed the rate-limiting branch 3 times, most recently from 389fc30 to 8b0add6 Compare May 30, 2016 17:40
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)

Choose a reason for hiding this comment

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

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

@bprashanth
Copy link

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 aledbf changed the title WIP: [nginx-ingress-controller] Add support for rate limiting in ingress rule locations [nginx-ingress-controller] Add support for rate limiting in ingress rule locations May 31, 2016
@bprashanth
Copy link

LGTM but you have a merge conflict

@aledbf
Copy link
Contributor Author

aledbf commented May 31, 2016

done

@bprashanth bprashanth merged commit 615448b into kubernetes-retired:master May 31, 2016
@aledbf aledbf deleted the rate-limiting branch May 31, 2016 18:21
@@ -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) }}
Copy link
Contributor

@louis-paul louis-paul Jun 1, 2016

Choose a reason for hiding this comment

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

I'm a bit late but shouldn't this be {{ range $zone := (buildRateLimitZones .servers) }}?

Should I make a PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@louis-paul this is fixed in #1104.
(just in case this PR is not published yet)

Copy link
Contributor

Choose a reason for hiding this comment

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

Great, thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants