Skip to content

Conversation

jschaul
Copy link
Member

@jschaul jschaul commented Mar 9, 2023

…quests

The change introduced in #2786 had no effect thus far. With this PR, it should start to have the desired effect.

See also https://github.com/zinfra/cailleach/pull/1574 for context of where this appeared.

Checklist

  • Add a new entry in an appropriate subdirectory of changelog.d
  • Read and follow the PR guidelines

@jschaul jschaul requested a review from akshaymankar March 9, 2023 13:25
@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Mar 9, 2023
@jschaul
Copy link
Member Author

jschaul commented Mar 27, 2023

I think we should get this in before https://github.com/zinfra/cailleach/pull/1574 to more clearly see possible effects.

Copy link
Member

@akshaymankar akshaymankar left a comment

Choose a reason for hiding this comment

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

Looks good. I am slightly confused by the commit message and the PR title: where is rate_limit_unlimited defined? I only see unlimited_requests_endpoint

@jschaul jschaul changed the title allow rate_limit_unlimited to also have an effect on authenticated re… nginz: enable unlimited_requests_endpoint for authenticated requests, too Mar 28, 2023
@jschaul
Copy link
Member Author

jschaul commented Mar 28, 2023

Looks good. I am slightly confused by the commit message and the PR title: where is rate_limit_unlimited defined? I only see unlimited_requests_endpoint

Ah thanks, I got the words all tangled up. Changed description and changelog to use the same word as in the actual settings.

@jschaul jschaul merged commit 4c39f7d into develop Mar 28, 2023
@jschaul jschaul deleted the rate-limit-assets branch March 28, 2023 11:51
@jschaul
Copy link
Member Author

jschaul commented Mar 28, 2023

With this PR, the config diff for the assets endpoint is this:

          location ~* ^(/v[0-9]+)?/assets/(.*) {
          # ...
+                      # Note that this endpoint has no rate limit per user for authenticated requests
          # ...

whereas for other authenticated endpoints:

          location ~* ^(/v[0-9]+)?/system/settings$ {
          # ...
+                      limit_req zone=reqs_per_user burst=20;
          # ...

and at the top

-       # Limit by $zauth_user if present and not part of rate limit exemptions
-       limit_req zone=reqs_per_user burst=20;

jschaul added a commit that referenced this pull request Mar 29, 2023
Also allow more than 25 concurrent connections for unlimited_requests_endpoint
jschaul added a commit that referenced this pull request Mar 29, 2023
Also allow more than 25 concurrent connections for unlimited_requests_endpoint
jschaul added a commit that referenced this pull request Mar 29, 2023
Increase allowed connections per user: compensate for topologyAwareRouting
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants