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

Block sending of oversize messages #127

Closed
wants to merge 1 commit into from

Conversation

acclassic
Copy link
Contributor

While looking to solve the containerd bug #6248 (containerd/containerd#6248 (comment)) I saw that it was already solved but the message would only be checked for the length after sending. Like @dmcgowan noted the message should not even be sent because it will be discarded anyway. I changed the code so that the send function will check for the size and not send the message in case the length is longer than the max message length. I opted for an GRPC error code 8 (RESOURCE_EXHAUSTED) over error code 3 (INVALID_ARGUMENT) because it describes the error better.
I hope that this is what we were looking for and I'm open to any input.

channel.go Outdated Show resolved Hide resolved
@kzys
Copy link
Member

kzys commented Jan 26, 2023

Regarding RESOURCE_EXHAUSTED,

https://github.com/grpc/grpc-go/blob/87eb5b7502493f758e76c4d09430c0049a81a557/codes/codes.go#L92-L98

This error code will be generated by the gRPC framework in out-of-memory and server overload situations, or when a message is larger than the configured maximum size.

Seems gRPC is doing the same.

@kzys
Copy link
Member

kzys commented Jan 26, 2023

Would it replace #119?

When trying to send a message that is bigger than the max message length
the send function will return an GRPC error 8 (RESOURCE_EXHAUSTED). It
will also close the connection so that the read function discards the
unused message.

Signed-off-by: Alessio Cantillo <[email protected]>
@acclassic acclassic force-pushed the containerd-issue-6248 branch from 8a595b2 to 6fd1b7a Compare January 30, 2023 15:43
@acclassic
Copy link
Contributor Author

Would it replace #119?

Thank you for checking my code.

Yes that is correct. This replacec PR #119.


if status.Code() != codes.ResourceExhausted {
t.Fatalf("expected grpc status code: %v != %v", status.Code(), codes.ResourceExhausted)
if err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

This test shouldn't branch, if an error is expected this should assert the error is not nil.

@thaJeztah
Copy link
Member

@acclassic are you still working on this? Are you able to address the review comments?

@klihub
Copy link
Member

klihub commented Sep 27, 2024

Closing, PR #171 merged with implementation for sender-side reject.

@klihub klihub closed this Sep 27, 2024
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.

5 participants