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

[release/7.0-staging] Send connection WINDOW_UPDATE before RTT PING #98384

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Feb 13, 2024

Backport of #97881 to release/7.0-staging

Fixes #97131

Customer Impact

Problem: gRPC server-streaming connections may be closed by server when communicating with service on GCP behind Google Cloud L7 External Load Balancer. It depends on size and frequency of gRPC messages.
Reported by customers in grpc/grpc-dotnet#2361 and grpc/grpc-dotnet#2358

gRPC is built on top of HTTP/2 protocol. In 'HttpClient' we use PING frames to measure RTT (Round-Trip Time) to leverage connections efficiently. Our usage of PING frames triggers DoS protection in Google Cloud L7 External Load Balancer and they close the connection from the server side. They are unwilling to change their implementation.
We worked with them to design ordering of frames mixed with WINDOW_UPDATE frames in a way that will avoid triggering their DoS protection and will allow us to measure RTT.

Regression

No. The behavior is specific to Google Cloud L7 External Load Balancer.

Testing

  • A functional test has been added to emulate the customer scenario.
  • The change has been manually tested against customer endpoint (i.e. GCP-hosted service behind Google Cloud L7 External Load Balancer).

Risk

Low-Medium

We are now sending higher volume of WINDOW_UPDATE frames. In theory, some servers might have problem with that. Mitigations:

  • We have a pre-existing switch to turn off RTT measurements entirely - System.Net.SocketsHttpHandler.Http2FlowControl.DisableDynamicWindowSizing
  • grpc-go client implementation has exactly same behavior which we are proposing for many years now (although it is gRPC only, not general HTTP/2 as our)
  • We did code review of popular HTTP/2 implementations -- nginx and nghttp2. We didn't find any DoS protection mechanism that would be triggered by more WINDOW_UPDATE frames.

@ghost
Copy link

ghost commented Feb 13, 2024

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Backport of #97881 to release/7.0-staging

/cc @antonfirsov

Customer Impact

  • Customer reported
  • Found internally

[Select one or both of the boxes. Describe how this issue impacts customers, citing the expected and actual behaviors and scope of the issue. If customer-reported, provide the issue number.]

Regression

  • Yes
  • No

[If yes, specify when the regression was introduced. Provide the PR or commit if known.]

Testing

[How was the fix verified? How was the issue missed previously? What tests were added?]

Risk

[High/Medium/Low. Justify the indication by mentioning how risks were measured and addressed.]

IMPORTANT: If this backport is for a servicing release, please verify that:

  • The PR target branch is release/X.0-staging, not release/X.0.

  • If the change touches code that ships in a NuGet package, you have added the necessary package authoring and gotten it explicitly reviewed.

Author: github-actions[bot]
Assignees: -
Labels:

area-System.Net.Http

Milestone: -

@antonfirsov antonfirsov added the Servicing-consider Issue for next servicing release review label Feb 14, 2024
@antonfirsov antonfirsov requested a review from a team February 14, 2024 23:45
@antonfirsov antonfirsov removed the Servicing-consider Issue for next servicing release review label Feb 14, 2024
@antonfirsov antonfirsov added the Servicing-consider Issue for next servicing release review label Feb 15, 2024
@antonfirsov
Copy link
Member

/azp run runtime-libraries-coreclr outerloop

1 similar comment
@antonfirsov
Copy link
Member

/azp run runtime-libraries-coreclr outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@carlossanlop
Copy link
Member

Friendly reminder that Monday March 11th is the Code Complete date for the April Release. Please make sure to get the CI green or confirm the failures are unrelated and send an email to Tactics requesting approval.

@rbhanda rbhanda removed the Servicing-consider Issue for next servicing release review label Mar 28, 2024
@antonfirsov
Copy link
Member

We are not backporting this to .NET 7 since EOL is close.

@antonfirsov antonfirsov closed this Apr 9, 2024
@jkotas jkotas deleted the backport/pr-97881-to-release/7.0-staging branch April 17, 2024 01:27
@github-actions github-actions bot locked and limited conversation to collaborators May 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants