Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restore HTTP payload size limit, make it configurable #3130

Merged
merged 6 commits into from
Jun 5, 2023

Conversation

SimonSapin
Copy link
Contributor

@SimonSapin SimonSapin commented May 23, 2023

Fixes #2000

Configuration:

limits:
  experimental_http_max_request_bytes: 2000000 # Default value: 2 MB

Checklist

Complete the checklist (and note appropriate exceptions) before a final PR is raised.

  • Changes are compatible[^1]
  • Documentation[^2] completed
  • Performance impact assessed and acceptable
  • Tests added and passing[^3]
    • Unit Tests
    • Integration Tests
    • Manual Tests

Exceptions

This change could be a compatibility issue for users who rely on large requests and upgrading from a Router version that didn’t have this limit. However this issue is easily fixed through configuration. The error log (but not the HTTP response) mention the relevant configuration key.

Notes

[^1]. It may be appropriate to bring upcoming changes to the attention of other (impacted) groups. Please endeavour to do this before seeking PR approval. The mechanism for doing this will vary considerably, so use your judgement as to how and when to do this.
[^2]. Configuration is an important part of many changes. Where applicable please try to document configuration examples.
[^3]. Tick whichever testing boxes are applicable. If you are adding Manual Tests:
- please document the manual testing (extensively) in the Exceptions.
- please raise a separate issue to automate the test and label it (or ask for it to be labeled) as manual test

Fixes #2000

Configuration:

```yaml
preview_operation_limits:
  http_max_request_bytes: 2000000 # Default value: 2 MB
```
@SimonSapin SimonSapin requested review from Geal, abernix and bnjjj May 23, 2023 14:18
@SimonSapin SimonSapin self-assigned this May 23, 2023
@SimonSapin SimonSapin requested a review from StephenBarlow as a code owner May 23, 2023 14:18
@router-perf
Copy link

router-perf bot commented May 23, 2023

CI performance tests

  • const - Basic stress test that runs with a constant number of users
  • no-graphos - Basic stress test, no GraphOS.
  • step - Basic stress test that steps up the number of users over time
  • reload - Reload test over a long period of time at a constant rate of users
  • large-request - Basic stress test, no GraphOS.

@SimonSapin
Copy link
Contributor Author

Open question: config location

I put this in the preview_operation_limits config section because that’s where we have other config keys that are most similar. However it’s not quite accurate since this new limit is about the entire HTTP body, not just operations. You could exceed it with a small operation and a large variable.

One way to resolve this might be, when moving this set of features out of preview, rename the section from preview_operation_limits to limits instead of to operation_limits.

Comment on lines 557 to 559
Before increasing this limit significantly consider doing performance testing
in an environment similar to your production, especially if some clients are untrusted.
Many concurrent large requests could can the Router to run out of memory.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chandrikas in case you want to have a look at the wording here. (The same is copied in the changelog.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @SimonSapin. Made some minor edits but looks good overall

SimonSapin and others added 2 commits May 31, 2023 18:09
Co-authored-by: Chandrika Srinivasan <chandrikas@users.noreply.github.com>
Co-authored-by: Chandrika Srinivasan <chandrikas@users.noreply.github.com>
@SimonSapin SimonSapin enabled auto-merge (squash) May 31, 2023 16:13
@SimonSapin SimonSapin force-pushed the simon/limit-request-size branch 2 times, most recently from 07e7703 to 492b76d Compare June 1, 2023 15:12
@SimonSapin SimonSapin force-pushed the simon/limit-request-size branch from 492b76d to 46faa0c Compare June 5, 2023 08:10
@SimonSapin SimonSapin merged commit 489c7f1 into dev Jun 5, 2023
@SimonSapin SimonSapin deleted the simon/limit-request-size branch June 5, 2023 14:01
SimonSapin added a commit that referenced this pull request Jun 6, 2023
SimonSapin added a commit that referenced this pull request Jun 6, 2023
SimonSapin added a commit that referenced this pull request Jun 6, 2023
SimonSapin added a commit that referenced this pull request Jun 6, 2023
Follow-up to #3130

<!-- start metadata -->

**Checklist**

Complete the checklist (and note appropriate exceptions) before a final
PR is raised.

- [x] Changes are compatible[^1]
- [x] Documentation[^2] completed
- [x] Performance impact assessed and acceptable
- Tests added and passing[^3]
    - [ ] Unit Tests
    - [ ] Integration Tests
    - [ ] Manual Tests

**Exceptions**

*Note any exceptions here*

**Notes**

[^1]. It may be appropriate to bring upcoming changes to the attention
of other (impacted) groups. Please endeavour to do this before seeking
PR approval. The mechanism for doing this will vary considerably, so use
your judgement as to how and when to do this.
[^2]. Configuration is an important part of many changes. Where
applicable please try to document configuration examples.
[^3]. Tick whichever testing boxes are applicable. If you are adding
Manual Tests:
- please document the manual testing (extensively) in the Exceptions.
- please raise a separate issue to automate the test and label it (or
ask for it to be labeled) as `manual test`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Router doesn't allow a payload larger than 2MB
3 participants