Add annotations for controlling request rate limiting

[dbaumgarten]

Proposed changes

This implements #4603.
Ingress resources can now have annotations to control request-rate-limiting.

Key points:

• Rate-limits apply per ingress object (all routes/paths of an ingress share a common limit_req_zone and have common settings)
• Supports different limits per path using minions. (Each mergeable minion can have separate rate-limiting configuration. Each minion uses it's own zone.
• By default, no rate-limit is applied. limit_req_status defaults to 429. Zone size defaults to 10m.

Checklist

Before creating a PR, run through this checklist and mark each as complete.

• I have read the CONTRIBUTING doc
• I have added tests that prove my fix is effective or that my feature works
• I have checked that all unit tests pass after adding my changes
• I have updated necessary documentation
• I have rebased my branch onto main
• I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

[jjngx]

@dbaumgarten could you please fix conflicts and push updates?

[jjngx]

@dbaumgarten could you please resolve conflicts and push updates?

[haywoodsh]

Hi @dbaumgarten I pulled down your PR and it worked well!

One suggestion I have is that you can align the annotation names to the corresponding fields of the existing VirtuaServer rateLimit policy instead of the underlying NGINX directives, e.g. nginx.org/limit-req can be nginx.org/limit-req-rate, and nginx.org/limit-req-status can be nginx.org/limit-req-reject-code. What do you think?

Another thing I noticed is that, the key of limit_req_zone is hardcoded to $binary_remote_addr, unlike in the virtualserver policy where it is configurable, as well as other fields like dryRun and logLevel are available as configurable annotations. Is there any plan to include them in this PR?

[jjngx]

Left one question regarding validation.

[dbaumgarten]

@haywoodsh aligning the annotations with the keys of VirtualServer sounds like a good idea. Will do.
Making the key for limit_req_zone configurable sounds like a good idea.
For my first draft I wanted to keep the number of added annotations to a minimum.
But if you think dryRun and LogLevel would be useful I can add these.

@jjngx Validating if the string ends in a proper unit should be pretty simple. Necessary or not it might save some people some headaches

[jjngx]

@dbaumgarten thank you for contributing!

Could you please add validators and positive and negative tests for requests limiting and zone sizes annotations? The rules (suffixes) are listed here.
Please reach out if you need assistance.

cc @haywoodsh @j1m-ryan @vepatel

[j1m-ryan]

Hi @dbaumgarten, heres info from some manual testing I did which should help you in the validation for the nginx.org/limit-req-rate annotation
The rate argugment can accept two types of values r/s (requests per second) and r/m (requests per minute). The other time intervals will be rejected by nginx (for example r/ms requests per millisecond or r/h requests per hour would not be valid).
If no requests per time-interval is specificed, it assumes it is r/s, however, for the validation in NIC, I'd be happy if we were explicit in requiring either r/s or r/m for this annotation.
As a last piece of validation ensure the number before r/s or r/m is a whole number greater than 0. Thanks again.

[netlify]

👷 Deploy request for nginx-kubernetes-ingress pending review.

Visit the deploys page to approve it

Name	Link
🔨 Latest commit	ce6f774

[dbaumgarten]

Hi,
PR has been rebased, annotations are aligned with the virtual-server resource and validations have been added

[codecov]

Codecov Report

Attention: 21 lines in your changes are missing coverage. Please review.

Comparison is base (38a03fa) 52.11% compared to head (b26fb39) 52.31%.
Report is 7 commits behind head on main.

❗ Current head b26fb39 differs from pull request most recent head ce6f774. Consider uploading reports for the commit ce6f774 to get more accurate results

Files	Patch %	Lines
internal/configs/annotations.go	68.42%	7 Missing and 11 partials ⚠️
internal/configs/parsing_helpers.go	81.25%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4660      +/-   ##
==========================================
+ Coverage   52.11%   52.31%   +0.19%     
==========================================
  Files          60       60              
  Lines       17394    17501     +107     
==========================================
+ Hits         9065     9155      +90     
- Misses       8011     8018       +7     
- Partials      318      328      +10     

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

[j1m-ryan]

Hi @dbaumgarten, looks good to me so far. Just failing the lint. Can you install pre-commit (via pip or brew), the run pre-commit install in your repo. It picked up a couple small formatting things.

  Error: cyclomatic complexity 18 of func `parseRateLimitAnnotations` is high (> 15) (gocyclo)
  Error: File is not `gofumpt`-ed (gofumpt)
  Error: `Occuring` is a misspelling of `Occurring` (misspell)
  Error: non-wrapping format verb for fmt.Errorf. Use `%w` to format errors (errorlint)
  Error: non-wrapping format verb for fmt.Errorf. Use `%w` to format errors (errorlint)```

[dbaumgarten]

I fixed the linter-findings.
However I "fixed" the too-high complexity of parseRateLimitAnnotations by simply excluding it from gocyclo, as I don't really see any value in splitting it up.
Is that okay?

[j1m-ryan]

I'm ok with this @dbaumgarten as the cyclometric complexity is coming from linear if's, rather than nested ifs, and this feature had a lot of annotations. So thats fine IMO.

As @jjngx pointed out, can you change the t.Errorf's to t.Error's, where you are not using format strings. Theres still a few examples of this in annotations_test.go

Other than that its good to go imo

[dbaumgarten]

Okay, I hope I found all remaining t.Errorfs ^^

[jjngx]

👍🏻 thanks for contributing @dbaumgarten

[ADubhlaoich]

LGTM on the documentation side.

[dbaumgarten]

There is so much going on on main, the PR is effectively always outdated...