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

channel: reject oversized messages on the sender side(, too). #171

Merged
merged 3 commits into from
Sep 26, 2024

Conversation

klihub
Copy link
Member

@klihub klihub commented Aug 21, 2024

Reject oversized messages on the sender side (keeping the receiver side rejection code intact). Do not forcibly close in channel.send() the underlying connection when an oversized message is rejected.

This should provide minimal low-level means for ttrpc-based applications to detect oversized requests in client.Call() and attempt an application level recovery/corrective actions, if the high-level protocol is designed with this in mind.

@klihub klihub requested review from dmcgowan and fuweid August 21, 2024 14:31
klihub and others added 2 commits August 21, 2024 17:52
Reject oversized messages on the sender side, keeping the
receiver side rejection intact. This should provide minimal
low-level plumbing for clients to attempt application level
corrective actions on the requestor side, if the high-level
protocol is designed with this in mind.

Co-authored-by: Alessio Cantillo <[email protected]>
Co-authored-by: Qian Zhang <[email protected]>
Signed-off-by: Krisztian Litkey <[email protected]>
Co-authored-by: Alessio Cantillo <[email protected]>
Co-authored-by: Qian Zhang <[email protected]>
Signed-off-by: Krisztian Litkey <[email protected]>
@klihub klihub force-pushed the devel/sender-side-oversize-rejection branch from e67f276 to d8c00df Compare August 21, 2024 14:53
@klihub
Copy link
Member Author

klihub commented Aug 21, 2024

Uhmmkay... my intention was/is not to turn this into a Battle of the PRs (#119, #127 and this).

I simply ran into this message length limitation myself. But I did in a context where the higher-level protocol was fairly easy to update to allow sending 'multi-part' requests, which were then transparently recombined on the receiving end into a single one and 'handed that up in the communication stack' for processing. Also I did not have the danger of a response overflowing. But since I still considered hitting the size limit a corner case, I wrote an implementation which tries sending everything in a single request and falls back to splitting it up only if it hits the size limit. But for that I was missing a reliable way to detect an overflow, and do it in a way which does not render the underlying transport unusable for sending further messages (which is the case for #127, unconditionally). Hence I ended up rolling this.

Unfortunately I did not check if there are pending PRs trying to do the same. Once I noticed it, I tossed in Co-authored-by's for the other attempt's authors. I believe that this version does not suffer from the subject of the unaddressed review comments in the other PRs...

@klihub klihub force-pushed the devel/sender-side-oversize-rejection branch 5 times, most recently from 2268863 to 317417c Compare August 24, 2024 06:05
@klihub
Copy link
Member Author

klihub commented Aug 24, 2024

@dmcgowan @fuweid Updated to implement the stock error Wrap interface instead of grpc's GRPCStatus() as discussed in the last community meeting.

@klihub klihub force-pushed the devel/sender-side-oversize-rejection branch 4 times, most recently from 6ed75d9 to 1a59039 Compare August 27, 2024 08:01
Use a dedicated, grpc Status-compatible error to wrap the
unique grpc status code, the size of the rejected message
and the maximum allowed size when a message is rejected
due to size limitations by the sending side.

Signed-off-by: Krisztian Litkey <[email protected]>
@klihub klihub force-pushed the devel/sender-side-oversize-rejection branch from 1a59039 to b5cd6e4 Compare August 27, 2024 08:23
@klihub klihub requested a review from mikebrow August 27, 2024 13:23
@dmcgowan
Copy link
Member

@fuweid @mikebrow PTAL

Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

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

LGTM on the proviso that the follow up prs will handle oversize initiated by the server, if/where > lengthmax was possible before we tag and roll this behavior change into a public release.. IOW revert if we don't get all of the follow ups merged like 172. 172 currently disabling the server side check by default should be good enough given prior server side impls would continue with no behavior changes, unless configured to change where certain ttrpc impls are fixed to handle the max exceeded error... At least that's how I'm reading the golden path here :)

@dmcgowan dmcgowan merged commit bcc40a4 into containerd:main Sep 26, 2024
11 checks passed
@klihub klihub deleted the devel/sender-side-oversize-rejection branch September 27, 2024 05:54
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.

3 participants