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

etcdserver: Fix 64 KB websocket notification message limit #12402

Merged
merged 1 commit into from Feb 3, 2021

Conversation

vitalif
Copy link
Contributor

@vitalif vitalif commented Oct 19, 2020

Hi!
I discovered a bug in etcd which makes it impossible for clients to receive server messages (notifications) longer than 64 KB via the websocket.
It's easy to reproduce, you just need to create a key/value entry in etcd with a value longer than 64 KB and you won't get it back in the notification.
A test example is here:
https://gist.github.com/vitalif/a634ac0543e6cacdda4ec288d922d9cf
The patch fixes this issue by updating grpc-websocket-proxy and setting the WithMaxRespBodyBufferSize option to 2^31-1.

@vitalif
Copy link
Contributor Author

vitalif commented Oct 28, 2020

Some tests fail but it's unrelated to this PR, they fail in release-3.4 anyway :)

@stale
Copy link

stale bot commented Jan 26, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 21 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jan 26, 2021
@vitalif
Copy link
Contributor Author

vitalif commented Jan 26, 2021

Can you merge the PR?

@stale stale bot removed the stale label Jan 26, 2021
@socketpair
Copy link

@xiang90 PING

This fixes etcd being unable to send any message longer than 64 KB as
a notification over the websocket. This was because the older version
of grpc-websocket-proxy was used and WithMaxRespBodyBufferSize option
wasn't set.
Copy link
Contributor

@ptabor ptabor left a comment

Choose a reason for hiding this comment

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

Thank you for the fix.

Could you, please:

  1. Prepare PR that adds this first to the master branch. Backport to 3.4. can happen later.
  2. Add a test that demonstrate this problem.

@@ -286,6 +286,7 @@ func (sctx *serveCtx) createMux(gwmux *gw.ServeMux, handler http.Handler) *http.
return outgoing
},
),
wsproxy.WithMaxRespBodyBufferSize(0x7fffffff),
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm scarried by unlimited buffers.

Should this one by in the same order of magnitude as:

MaxRequestBytes uint `json:"max-request-bytes"`

?

Copy link
Contributor Author

@vitalif vitalif Feb 2, 2021

Choose a reason for hiding this comment

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

I think messages that are generated by the server itself shouldn't be limited by the websocket library. Because it's very funny if the server generates the message but is then unable to actually send it. If it wants to limit message size it should split the message itself, and that's in fact what it actually does sometimes. So I think this buffer shouldn't be limited...

Copy link
Contributor Author

@vitalif vitalif Feb 2, 2021

Choose a reason for hiding this comment

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

The PR to the master branch is already merged long ago #12403

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you. It was not clear from the RP description that its a backport.

@ptabor ptabor merged commit a1c5f59 into etcd-io:release-3.4 Feb 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants