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

Disable decoderEnforceMaxRstFramesPerWindow for HTTP/2 clients #2752

Merged
merged 2 commits into from
Nov 13, 2023

Conversation

idelpivnitskiy
Copy link
Member

Motivation:

Netty 4.1.100 introduced a new feature for HTTP/2 to protect from DDoS vector, and we added system properties to control its settings in #2728. In version 4.1.101 Netty disabled this protection for clients by default, but due to how we use our system properties, that change won't automatically apply to ServiceTalk.

Modifications:

  • OptimizedHttp2FrameCodecBuilder.decoderEnforceMaxRstFramesPerWindow now considers isServer flag to decide if the inferred values should be applied or not;

Result:

decoderEnforceMaxRstFramesPerWindow is enabled only for HTTP/2 servers.

Motivation:

Netty 4.1.100 introduced a new feature for HTTP/2 to protect from
DDoS vector, and we added system properties to control its settings in
apple#2728. In version 4.1.101 Netty disabled this protection for clients
by default, but due to how we use our system properties, that change
won't automatically apply to ServiceTalk.

Modifications:

- `OptimizedHttp2FrameCodecBuilder.decoderEnforceMaxRstFramesPerWindow`
now considers `isServer` flag to decide if the inferred values should
be applied or not;

Result:

`decoderEnforceMaxRstFramesPerWindow` is enabled only for HTTP/2
servers.
@yanisderbikov
Copy link

Could I be helpful?

Copy link
Contributor

@daschl daschl left a comment

Choose a reason for hiding this comment

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

LGTM, but consider modifying the inline javadoc for the properties to mention that it only applies to the server, not the clients

@idelpivnitskiy idelpivnitskiy merged commit ff33868 into apple:main Nov 13, 2023
15 checks passed
@idelpivnitskiy idelpivnitskiy deleted the isServer branch November 13, 2023 10:28
@idelpivnitskiy
Copy link
Member Author

Hi @yanisderbikov,
Thanks for your interest in this project. I checked our good first issues list and TBH, it's not in a good shape right now. #2502 is already taken. If you like digging, consider exploring #1217. Our team will try to add more issues with this label in the following weeks.

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.

None yet

3 participants